Network activity toggle #8996

luke-jr · 2016-10-23T06:30:06Z

Yet another rebase of #2412 / #5314

I've made the RPC interface more consistent (booleans only) and removed the getinfo addition.

Also removed the problematically-licensed icon and replaced it with an equivalent using our new style, from Typfonts.

I'd like to replace the PNG with its SVG equivalent, but I don't know if this is safe right now...?

paveljanik · 2016-10-23T16:19:41Z

Concept ACK

While trying to test this, I have got:

Assertion failed: (nMaxInbound > 0), function AcceptConnection, file net.cpp, line 973.

almost immediately I pressed the network toggle icon.

When the network is toggled off, the icon is hidden and I can't turn the network back on without trying to click in the empty space...

paveljanik · 2016-10-23T16:39:37Z

There is a commit for RPC togglenetwork (f83a1e2bd83625997916963fe8b1942e504070ab), but it is renamed later, so commits do not match.

The RPC test is still named togglenetwork (https://github.com/bitcoin/bitcoin/pull/8996/files#diff-b89234789a9a9ee29cb0758f69a73133R87).

paveljanik · 2016-10-23T16:48:05Z

During startup, when there is 1 outgoing connection only:

2016-10-23 16:44:42 SetNetworkActive: false
2016-10-23 16:44:42 disconnecting peer=1
2016-10-23 16:44:42 socket select error Bad file descriptor (9)
2016-10-23 16:44:42 disconnecting peer=2
nMaxConnections = 8
nMaxFeeler = 1
nMaxOutbound = 8
nMaxInbound = -1
Assertion failed: (nMaxInbound > 0), function AcceptConnection, file net.cpp, line 979.

@EthanHeilman Do you have an idea?

luke-jr · 2016-10-23T17:05:54Z

Assertion failed: (nMaxInbound > 0), function AcceptConnection, file net.cpp, line 973.

I don't believe this can be related to this PR...

When the network is toggled off, the icon is hidden and I can't turn the network back on without trying to click in the empty space...

Sounds like a build system problem - it didn't update the resources with the new icon?

There is a commit for RPC togglenetwork (f83a1e2), but it is renamed later, so commits do not match.

I don't see this as a problem.

paveljanik · 2016-10-23T17:31:47Z

src/qt/bitcoingui.cpp

_clientModel please to prevent shadowing.

paveljanik · 2016-10-23T17:33:59Z

The missing icon issue was local one, yes (solved by clean build). Sorry for confusion.

The icon should probably contain some visual reference to the network (compare with the no-HD icon). I can't create screenshot because it aborts almost immediately here.

Added the function SetNetworkActive() which when called with argument set to false disconnects all nodes and sets the flag fNetworkActive to false. As long as this flag is false no new connections are attempted and no incoming connections are accepted. Network activity is reenabled by calling the function with argument true.

RPC command "togglenetwork" toggles network and returns new state after command. RPC command "getinfo" returns "networkactive" field in output.

Add getNetworkActive()/setNetworkActive() method to client model. Send network active status through NotifyNetworkActiveChanged. Indicate in tool tip of gui status bar network indicator whether network activity is disabled. Indicate in debug window whether network activity is disabled and add button to allow user to toggle network activity state.

- Rename RPC command "togglenetwork" to "setnetworkactive (true|false)" - Add simple test case - GUI toggle added to connections icon in statusbar

…getinfo

paveljanik · 2016-10-24T17:06:51Z

It works as designed. Icon in the UI flips on RPC calls, can be clicked, nice tooltips, it works.

When the network is disabled, it looks like this (with non-HD wallet):

I still think that it should be done in 2 commits instead of this mess:

RPC: New method setnetworkactive
GUI: UI part of the network enable/disable

EthanHeilman · 2016-10-24T22:53:07Z

@paveljanik At the risk of stating the obvious if nMaxConnections is set to 8 then nMaxInbound will be set to -1 triggering the assert.

int nMaxInbound = nMaxConnections - (nMaxOutbound + nMaxFeeler);
assert(nMaxInbound > 0);

The value nMaxConnections is typically set to 125, I don't understand how it is getting set to 8, perhaps the system is running low on available sockets?

sipa · 2016-10-24T22:55:57Z

@EthanHeilman nMaxConnections is user configurable. I typically set it to 3 on low-memory devices.

luke-jr · 2016-10-25T01:58:19Z

I think the assert failure with a low maxconnections is an issue unrelated to this PR.

sipa · 2016-10-25T04:45:40Z

@luke-jr @EthanHeilman Indeed, let's move to #9007.

paveljanik · 2016-11-04T18:52:59Z

I think this is very useful function for some users. Please review.

paveljanik · 2016-11-10T08:37:40Z

@jonasschnelli Jonas, can you please have a look?

jonasschnelli · 2016-11-10T08:42:57Z

The reason why I'm waiting for this are the conceptual NACK's on the previous attempt to get this in: #5314

Some devs where claiming that we need a better solution then just disabling the network connection. Ideally, this mode should connect to the p2p network in case you have uncommitted wtxs.

I'd like to get more Concept ACKs from others.
I always likes this solution as a first step:

Concept ACK.

paveljanik · 2016-11-10T08:49:46Z

The previous NACK (there was one ;-) was about togglenetwork interface. This was removed.

Wallet showing wrong info without network is the current state anyway. We could slide the "gray overlay" up as we do during IBD to make it clear!

paveljanik · 2016-11-10T21:09:26Z

Some more thoughts about this: it depends how you look at this. So far, my view was as simple as follows. I do not care at all about the displayed stuff in the GUI, I just want GUI to immediately stop all communication. Be it if I plan to connect to the untrusted network, or slow network at parents', or limited usage network where every eight connections to the outside mean that no one else being able to use the network... I was solving this by suspending the GUI process (I do not want to stop it and start again when I need it). This brings new and elegant solution!

The "better solution" mentioned above probably wants to solve different problem(s) though.

jonasschnelli · 2016-11-11T10:11:50Z

Tested ACK 19f46f1.

Would be nice if we would at least add a tooltip "Press to enable/disable network activity" over the connection statusbar icon. From the GUI perspective, it's kind of a hidden feature right now.

19f46f1 Qt: New network_disabled icon (Luke Dashjr) 54cf997 RPC/Net: Use boolean consistently for networkactive, and remove from getinfo (Luke Dashjr) b2b33d9 Overhaul network activity toggle (Jonas Schnelli) 32efa79 Qt: Add GUI feedback and control of network activity state. (Jon Lund Steffensen) e38993b RPC: Add "togglenetwork" method to toggle network activity temporarily (Jon Lund Steffensen) 7c9a98a Allow network activity to be temporarily suspended. (Jon Lund Steffensen)

sipa · 2016-11-11T12:53:45Z

src/net.cpp

+    }
+
+    if (!active) {
+        fNetworkActive = false;


No locking?

Oh! I though this was a std::atomic<bool>, but it's a plain bool.

jonasschnelli · 2016-11-11T19:21:58Z

Sorry. This merge was a little bit pro active.

19f46f1 Qt: New network_disabled icon (Luke Dashjr) 54cf997 RPC/Net: Use boolean consistently for networkactive, and remove from getinfo (Luke Dashjr) b2b33d9 Overhaul network activity toggle (Jonas Schnelli) 32efa79 Qt: Add GUI feedback and control of network activity state. (Jon Lund Steffensen) e38993b RPC: Add "togglenetwork" method to toggle network activity temporarily (Jon Lund Steffensen) 7c9a98a Allow network activity to be temporarily suspended. (Jon Lund Steffensen)

* Merge bitcoin#8996: Network activity toggle 19f46f1 Qt: New network_disabled icon (Luke Dashjr) 54cf997 RPC/Net: Use boolean consistently for networkactive, and remove from getinfo (Luke Dashjr) b2b33d9 Overhaul network activity toggle (Jonas Schnelli) 32efa79 Qt: Add GUI feedback and control of network activity state. (Jon Lund Steffensen) e38993b RPC: Add "togglenetwork" method to toggle network activity temporarily (Jon Lund Steffensen) 7c9a98a Allow network activity to be temporarily suspended. (Jon Lund Steffensen) * Revert on-click behavior of network status icon to showing peers list Stay with the way Dash handled clicking on the status icon * Add theme support for network disabled icon * Merge bitcoin#8874: Multiple Selection for peer and ban tables 1077577 Fix auto-deselection of peers (Andrew Chow) addfdeb Multiple Selection for peer and ban tables (Andrew Chow) * Merge bitcoin#9190: qt: Plug many memory leaks ed998ea qt: Avoid OpenSSL certstore-related memory leak (Wladimir J. van der Laan) 5204598 qt: Avoid shutdownwindow-related memory leak (Wladimir J. van der Laan) e4f126a qt: Avoid splash-screen related memory leak (Wladimir J. van der Laan) 693384e qt: Prevent thread/memory leak on exiting RPCConsole (Wladimir J. van der Laan) 47db075 qt: Plug many memory leaks (Wladimir J. van der Laan) * Merge bitcoin#9218: qt: Show progress overlay when clicking spinner icon 042f9fa qt: Show progress overlay when clicking spinner icon (Wladimir J. van der Laan) 827d9a3 qt: Replace NetworkToggleStatusBarControl with generic ClickableLabel (Wladimir J. van der Laan) * Merge bitcoin#9266: Bugfix: Qt/RPCConsole: Put column enum in the right places df17fe0 Bugfix: Qt/RPCConsole: Put column enum in the right places (Luke Dashjr) * Merge bitcoin#9255: qt: layoutAboutToChange signal is called layoutAboutToBeChanged f36349e qt: Remove on_toggleNetworkActiveButton_clicked from RPCConsole (Wladimir J. van der Laan) 297cc20 qt: layoutAboutToChange signal is called layoutAboutToBeChanged (Wladimir J. van der Laan) * Use UniValue until bitcoin PR bitcoin#8788 is backported Network active toggle was already based on "[RPC] Give RPC commands more information about the RPC request" We need to use the old UniValue style until that one is backported * Merge bitcoin#8906: [qt] sync-overlay: Don't show progress twice fafeec3 [qt] sync-overlay: Don't show progress twice (MarcoFalke) * Merge bitcoin#8985: Use pindexBestHeader instead of setBlockIndexCandidates for NotifyHeaderTip() 3154d6e [Qt] use NotifyHeaderTip's height and date for the progress update (Jonas Schnelli) 0a261b6 Use pindexBestHeader instead of setBlockIndexCandidates for NotifyHeaderTip() (Jonas Schnelli) * Merge bitcoin#9280: [Qt] Show ModalOverlay by pressing the progress bar, allow hiding 89a3723 [Qt] Show ModalOverlay by pressing the progress bar, disabled show() in sync mode (Jonas Schnelli) * Merge bitcoin#9461: [Qt] Improve progress display during headers-sync and peer-finding 40ec7c7 [Qt] Improve progress display during headers-sync and peer-finding (Jonas Schnelli) * Merge bitcoin#9588: qt: Use nPowTargetSpacing constant fa4d478 qt: Use nPowTargetSpacing constant (MarcoFalke) * Hide modal overlay forever when syncing has catched up Don't allow to open it again by clicking on the progress bar and spinner icon. Currently the overlay does not show meaningful information about masternode sync and it gives the impression of being stuck after the block chain sync is done. * Don't include chainparams.h in sendcoinsdialog.cpp This was just a remainder of a backported PR which meant to change some calculation in this file which does not apply to Dash. * Also check for fNetworkActive in ConnectNode * Merge bitcoin#9528: [qt] Rename formateNiceTimeOffset(qint64) to formatNiceTimeOffset(qint64) 988d300 [qt] Rename formateNiceTimeOffset(qint64) to formatNiceTimeOffset(qint64) (practicalswift) * Merge bitcoin#11237: qt: Fixing division by zero in time remaining c8d38ab Refactor tipUpdate as per style guide (MeshCollider) 3b69a08 Fix division by zero in time remaining (MeshCollider) Pull request description: Fixes bitcoin#10291, bitcoin#11265 progressDelta may be 0 (or even negative according to 11265), this checks for that and prints unknown if it is, because we cannot calculate an estimate for the time remaining (would be infinite or negative). Tree-SHA512: bc5708e5ed6e4670d008219558c5fbb25709bd99a32c98ec39bb74f94a0b7fa058f3d03389ccdd39e6723e6b5b48e34b13ceee7c051c2db631e51d8ec3e1d68c

fanquake added the P2P label Oct 23, 2016

paveljanik reviewed Oct 23, 2016

View reviewed changes

luke-jr force-pushed the networkactive branch 2 times, most recently from 42d9d12 to 967c74f Compare October 24, 2016 09:23

jonls and others added 6 commits October 24, 2016 10:23

RPC: Add "togglenetwork" method to toggle network activity temporarily

e38993b

RPC command "togglenetwork" toggles network and returns new state after command. RPC command "getinfo" returns "networkactive" field in output.

Overhaul network activity toggle

b2b33d9

- Rename RPC command "togglenetwork" to "setnetworkactive (true|false)" - Add simple test case - GUI toggle added to connections icon in statusbar

RPC/Net: Use boolean consistently for networkactive, and remove from …

54cf997

…getinfo

Qt: New network_disabled icon

19f46f1

luke-jr force-pushed the networkactive branch from 967c74f to 19f46f1 Compare October 24, 2016 10:25

paveljanik mentioned this pull request Oct 24, 2016

Assertion failed: (nMaxInbound > 0), function AcceptConnection, file net.cpp, line 973. #9007

Closed

jonasschnelli added the GUI label Nov 11, 2016

jonasschnelli merged commit 19f46f1 into bitcoin:master Nov 11, 2016

jonasschnelli mentioned this pull request Nov 11, 2016

Feature Request: Pause/Resume network activity #1958

Closed

paveljanik mentioned this pull request Nov 11, 2016

Mention the new network toggle functionality in the tooltip. #9130

Merged

sipa reviewed Nov 11, 2016

View reviewed changes

jonasschnelli mentioned this pull request Nov 11, 2016

fNetworkActive is not protected by a lock, use an atomic #9131

Merged

sipa mentioned this pull request Jan 10, 2017

TODO for release notes 0.14.0 #8455

Closed

18 tasks

sickpig mentioned this pull request Apr 14, 2017

[port] PR #8065 Addrman offline attempts BitcoinUnlimited/BitcoinUnlimited#449

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Network activity toggle #8996

Network activity toggle #8996

Uh oh!

Conversation

luke-jr commented Oct 23, 2016

Uh oh!

paveljanik commented Oct 23, 2016

Uh oh!

paveljanik commented Oct 23, 2016

Uh oh!

paveljanik commented Oct 23, 2016

Uh oh!

luke-jr commented Oct 23, 2016

Uh oh!

paveljanik Oct 23, 2016

Choose a reason for hiding this comment

Uh oh!

luke-jr Oct 24, 2016

Choose a reason for hiding this comment

Uh oh!

paveljanik commented Oct 23, 2016

Uh oh!

paveljanik commented Oct 24, 2016

Uh oh!

EthanHeilman commented Oct 24, 2016

Uh oh!

sipa commented Oct 24, 2016

Uh oh!

luke-jr commented Oct 25, 2016

Uh oh!

sipa commented Oct 25, 2016

Uh oh!

paveljanik commented Nov 4, 2016

Uh oh!

paveljanik commented Nov 10, 2016

Uh oh!

jonasschnelli commented Nov 10, 2016

Uh oh!

paveljanik commented Nov 10, 2016

Uh oh!

paveljanik commented Nov 10, 2016

Uh oh!

jonasschnelli commented Nov 11, 2016

Uh oh!

sipa Nov 11, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Nov 11, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Nov 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants