Network activity toggle #2412

jonls · 2013-03-26T02:33:14Z

Allow the network activity of the client to be toggled temporarily.

When network activity is disabled the client will close all connections, stop accepting inbound connections, and stop opening new outbound connections, until the network activity is reenabled. The first commit adds this feature to the core, accessed through SetNetworkActive().

Second commit adds an RPC command "togglenetwork" to toggle on/off.

Third commit adds further connections to the gui. When the network activity is disabled the status bar and the debug window will show this. In addition the commit adds a button to the debug window to toggle network activity.

Open issues:

Should the core close the listening socket or is it enough to just disregard incoming connections by closing them immediately?
Should SetNetworkActive() return an error code or can it throw exceptions?

jonls · 2013-03-26T02:35:27Z

The feature was suggested earlier in issue #1958 and may also alleviate some problems from #273.

Diapolo · 2013-03-26T06:21:39Z

Nice feature, but for the GUI I think it should a) show the current state in the debug window and b) allow a click or double-klick on the connection symbol in the main window, instead of a new button in the debug window. What do you think?

Diapolo · 2013-03-26T06:25:27Z

src/qt/rpcconsole.cpp

@@ -5,6 +5,8 @@
 #include "bitcoinrpc.h"
 #include "guiutil.h"

+#include "net.h"


I think we should avoid direct access to net.h here... best way perhaps is to use the ClientModel and introduce a setNetworkActive() function.

Makes sense. Using ClientModel would be a better solution.

jonls · 2013-03-26T09:45:03Z

Regarding a: The debug window actually shows the state (but maybe not in the best way), in that it appends "(Disabled)" to the number of connections when network activity is disabled. Are you suggesting a row in the debug window showing for example "Network state: "?

I think double clicking on the connection symbol could also be a solution. I'm wondering though if the feature will be too hard to discover in that case, since it is not apparent that the symbol can be clicked. Also, if somebody clicked it by accident (maybe not likely?) they would not know how to get connected again.

Diapolo · 2013-03-26T11:12:37Z

src/qt/rpcconsole.cpp

+    if(networkActive)
+        ui->numberOfConnections->setText(QString::number(count));
+    else
+        ui->numberOfConnections->setText(tr("%1 (Disabled)").arg(count));


May I suggest to use something like:
ui->numberOfConnections->setText(QString::number(count) + " (" + tr("Network activity disabled") + ")");

This has the advantage to re-use the string (and translation of) Network activity disabled without the need for another translation + it is clearer than just (Disabled).

Diapolo · 2013-03-26T11:16:52Z

The button Toggle Network activity(activity lowercase IMO) is fine, perhaps the toggle could just also be added to the connection symbol in the main window + a tray message that gives a small warning (use BitcoinGUI::message() for that), when the state is changed.

jonls · 2013-03-26T14:50:56Z

I've updated the patches: 1) ToggleNetworkActive() renamed to SetNetworkActive(). I think this name makes more sense and is more consistent with naming of other functions. 2) SetNetworkActive() prints log ouput if fDebug is set, 3) ClientModel now has a method to control network activity state setNetworkActive(). This is used by RPCConsole. 4) The string in RPCConsole is the same as the string in the status bar tooltip so they can share translations.

I'll look into the rest of the suggestions on the gui later.

Diapolo · 2013-03-26T14:54:36Z

src/rpcnet.cpp

@@ -199,3 +199,14 @@ Value getaddednodeinfo(const Array& params, bool fHelp)
    return ret;
 }

+Value togglenetwork(const Array& params, bool fHelp)


Is there any way to query via RPC if the network was disabled?

Not currently. Do you think this could be returned in "getinfo" or should there rather be a separate command? Perhaps togglenetwork itself should return the new state as well.

The state is returned now in "getinfo" and as the return value of "togglenetwork".

Diapolo · 2013-03-26T14:58:14Z

I'm no core-developer, but did some Qt things, so thanks for your update. I like the pull and when I have more time I'll compile it and see how it feels when using it :).

@laanwj What do you think?

laanwj · 2013-03-29T08:34:57Z

I'm OK with the GUI code changes -- nice work.

About the general idea:

Can you give a use case for this? What are you using this for?
I am not sure about the consequences for the network of accepting connections but immediately dropping them, and what would be proper behavior in this case. I think closing the socket so that clients get "connection refused" is preferable. Maybe @sipa or @gavinandresen could comment.

jonls · 2013-03-29T10:27:28Z

Use case: Sometimes the bitcoin client can put a lot of load on the internet connection of the user resulting in a high latency for other internet activities. This may be acceptable for the user most of the time, but in some cases the user temporarily needs a low latency connection. In this case the only option currently is to close the client and reopen it later, which can take quite a while and create a lot of disk activity when the block chain is reloaded. Being able to temporarily close all connections provides a solution for this.

jonls · 2013-03-29T16:25:53Z

Update: 1) RPC commands togglenetwork and getinfo return info on current network state. 2) Signal about network active state change is sent with a separate signal NotifyNetworkActiveChanged. This means that the gui will update immediately when the button is clicked.

I've looked into adding the suggestion of having the connection symbol toggle the network state as well, however the Qt label does not allow mouse clicks to be detected. This could be worked around by implementing a custom subclass of label.

Diapolo · 2013-03-29T20:35:55Z

I also think this is nice during IBD to get a new peer from which the client is downloading it's blocks.

rebroad · 2013-03-30T00:31:42Z

Can transactions still be sent while the network activity is switched off? It might be nice to be able to do this still at least, and optionally download new blocks too. (I recently added an "-antisocial" command line option which stops transaction relaying due to using a limited internet connection here which charges per MB, but a toggleable version of this would be much nicer - for when I switch to a different internet provider).

jonls · 2013-03-30T10:09:54Z

@rebroad Ok, but that seems to require a different implementation. This patch simply cuts all connections, so I think your proposal is better suited for another issue. Also it seems that any kind of throttling or filtering of the connections eventually end up being too controversial to get anywhere (e.g. #273).

BitcoinPullTester · 2013-03-30T11:26:08Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b1ff0108f94a2a6365f9957708041269c63a351c for binaries and test log.
This is an automated test script which runs test cases on each commit every time is updated.
It, however, dies sometimes and fails to test properly, if you are waiting on a test, please check timestamps and if the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
and contact BlueMatt on freenode if something looks broken.

laanwj · 2013-03-31T05:22:00Z

IMO the current implementation of simply cutting the whole connection is best. It is also clearest to the user. Once you start making exceptions there's a large chance of introducing bugs or even network-breaking bugs, which are why such features are unlikely to make it into the reference client.

Diapolo · 2013-04-02T08:23:31Z

@sipa @gavinandresen I'm asking myself, if the core-devs are willing to pull such a thing or if this should be GUI-only?

luke-jr · 2013-04-08T05:33:12Z

Needs rebase.

jonls · 2013-04-08T09:59:22Z

@luke-jr I will be happy to do rebases if this has a chance of actually being pulled but so far I'm not sure. Whet do you think?

gavinandresen · 2013-04-08T13:34:44Z

Testing is our bottleneck.

This gives use Yet Another State to test, which is a bad thing because it just makes one more thing that can break.

I'm against it.

sipa · 2013-04-12T21:04:15Z

Haven't tested but the code changes to core look sane to me.

Diapolo · 2013-04-12T23:37:19Z

I still like the idea!

jonls · 2013-04-22T00:46:18Z

Rebased.

Diapolo · 2013-05-07T21:11:13Z

@gavinandresen What do you think about that? Would be sad to just forget about it :).

gmaxwell · 2013-05-07T21:13:59Z

I missed the rebase, I'll test it some.

jonasschnelli · 2013-09-11T18:38:15Z

@jonls could you rebase once more?

jonls · 2013-09-13T01:14:55Z

@jonasschnelli done

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.

Diapolo · 2013-10-20T00:53:04Z

What is the current state for this?

BitcoinPullTester · 2013-10-20T00:59:56Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/22570316ad74df51e459c17d72e02ce76e03751d for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

gavinandresen · 2013-10-21T01:37:52Z

Needs testing, and a test plan that has tests like:

Toggle networking on and off as rapidly as possible eleven times using the GUI. EXPECT: no crashes, always ends up in properly connected or disconnected state.

Toggle networking on and off using the JSON-RPC call. EXPECT: no crashes...

Toggle networking off, send a transaction. EXPECT: ??? what is reasonable to expect here? Message to user? Transaction gets broadcast when network is turned on or client restarted ???

gavinandresen · 2013-11-29T21:00:38Z

Closing due to inactivity. Feel free to bring back a rebased version that has a test plan.

luke-jr · 2014-09-13T08:03:11Z

@jonls Do you plan to update this?

jonls · 2014-09-13T20:24:05Z

@luke-jr No, I have no plans to update it.

Diapolo reviewed Mar 26, 2013
View reviewed changes

brandondahler mentioned this pull request Apr 7, 2013

Allow capping of bandwidth on overall or command-specific basis, reset via cron-like expression #2479

Closed

jonls added 3 commits October 19, 2013 20:11

Add RPC command "togglenetwork" to toggle network activity temporarily.

c8e75ca

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

gavinandresen closed this Nov 29, 2013

jonasschnelli mentioned this pull request Nov 19, 2014

Network activity toggle #5314

Closed

luke-jr mentioned this pull request Oct 23, 2016

Network activity toggle #8996

Merged

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

Network activity toggle #2412

Network activity toggle #2412

Conversation

jonls commented Mar 26, 2013

jonls commented Mar 26, 2013

Diapolo commented Mar 26, 2013

Diapolo Mar 26, 2013

Choose a reason for hiding this comment

jonls Mar 26, 2013

Choose a reason for hiding this comment

jonls commented Mar 26, 2013

Diapolo Mar 26, 2013

Choose a reason for hiding this comment

Diapolo commented Mar 26, 2013

jonls commented Mar 26, 2013

Diapolo Mar 26, 2013

Choose a reason for hiding this comment

jonls Mar 26, 2013

Choose a reason for hiding this comment

jonls Mar 29, 2013

Choose a reason for hiding this comment

Diapolo commented Mar 26, 2013

laanwj commented Mar 29, 2013

jonls commented Mar 29, 2013

jonls commented Mar 29, 2013

Diapolo commented Mar 29, 2013

rebroad commented Mar 30, 2013

jonls commented Mar 30, 2013

BitcoinPullTester commented Mar 30, 2013

laanwj commented Mar 31, 2013

Diapolo commented Apr 2, 2013

luke-jr commented Apr 8, 2013

jonls commented Apr 8, 2013

gavinandresen commented Apr 8, 2013

sipa commented Apr 12, 2013

Diapolo commented Apr 12, 2013

jonls commented Apr 22, 2013

Diapolo commented May 7, 2013

gmaxwell commented May 7, 2013

jonasschnelli commented Sep 11, 2013

jonls commented Sep 13, 2013

Diapolo commented Oct 20, 2013

BitcoinPullTester commented Oct 20, 2013

gavinandresen commented Oct 21, 2013

gavinandresen commented Nov 29, 2013

luke-jr commented Sep 13, 2014

jonls commented Sep 13, 2014