Network activity toggle #8996

Merged
merged 6 commits into from Nov 11, 2016

Projects

None yet

7 participants

@luke-jr
Member
luke-jr commented Oct 23, 2016

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...?

@fanquake fanquake added the P2P label Oct 23, 2016
@paveljanik
Contributor

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
Contributor

There is a commit for RPC togglenetwork (f83a1e2), 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
Contributor

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
Member
luke-jr commented Oct 23, 2016

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.

src/qt/bitcoingui.cpp
+}
+
+/** Lets the control know about the Client Model */
+void NetworkToggleStatusBarControl::setClientModel(ClientModel *clientModel)
@paveljanik
paveljanik Oct 23, 2016 Contributor

_clientModel please to prevent shadowing.

@paveljanik
Contributor

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.

jonls and others added some commits Mar 26, 2013
@jonls @luke-jr jonls Allow network activity to be temporarily suspended.
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.
7c9a98a
@jonls @luke-jr jonls RPC: Add "togglenetwork" method to toggle network activity temporarily
RPC command "togglenetwork" toggles network and returns new state after command.
RPC command "getinfo" returns "networkactive" field in output.
e38993b
@jonls @luke-jr jonls Qt: Add GUI feedback and control of network activity state.
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.
32efa79
@jonasschnelli @luke-jr jonasschnelli Overhaul network activity toggle
- Rename RPC command "togglenetwork" to "setnetworkactive (true|false)"
- Add simple test case
- GUI toggle added to connections icon in statusbar
b2b33d9
@luke-jr luke-jr RPC/Net: Use boolean consistently for networkactive, and remove from …
…getinfo
54cf997
@luke-jr luke-jr Qt: New network_disabled icon 19f46f1
@paveljanik
Contributor

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):

screen shot 2016-10-24 at 18 58 52

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
Contributor

@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
Member
sipa commented Oct 24, 2016

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

@luke-jr
Member
luke-jr commented Oct 25, 2016

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

@sipa
Member
sipa commented Oct 25, 2016

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

@paveljanik
Contributor

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

@paveljanik
Contributor

@jonasschnelli Jonas, can you please have a look?

@jonasschnelli
Member

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
Contributor

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
Contributor

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 jonasschnelli added the GUI label Nov 11, 2016
@jonasschnelli
Member

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.

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jonasschnelli jonasschnelli added a commit that referenced this pull request Nov 11, 2016
@jonasschnelli jonasschnelli Merge #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)
ab914a6
+ }
+
+ if (!active) {
+ fNetworkActive = false;
@sipa
sipa Nov 11, 2016 Member

No locking?

@jonasschnelli
jonasschnelli Nov 11, 2016 Member

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

@jonasschnelli
Member

Sorry. This merge was a little bit pro active.

@sipa sipa referenced this pull request Jan 10, 2017
Open

TODO for release notes 0.14.0 #8455

1 of 18 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment