Network activity toggle

jonls commented at 2:33 am on March 26, 2013: contributor

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.

bitcoin-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 commented at 2:35 am on March 26, 2013: contributor

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

Diapolo commented at 6:21 am on March 26, 2013: none

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?

in src/qt/rpcconsole.cpp: in 2c7ceb923a outdated

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

Diapolo commented at 6:25 am on March 26, 2013:

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

jonls commented at 9:36 am on March 26, 2013:

Makes sense. Using ClientModel would be a better solution.

in src/net.cpp: in 2c7ceb923a outdated

2000@@ -1995,6 +2001,22 @@ bool StopNode()
2001     return true;
2002 }
2003 
2004+void ToggleNetworkActive(bool enable)

Diapolo commented at 6:26 am on March 26, 2013:

Is there something in here that needs our attention if it fails (catch errors / handle problems)?

Edit: Perhaps it would be good to have a message for the debug.log in here with an if (fDebug) in front.

jonls commented at 9:45 am on March 26, 2013: contributor

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.

in src/qt/rpcconsole.cpp: in 2c7ceb923a outdated

339 {
340-    ui->numberOfConnections->setText(QString::number(count));
341+    if(networkActive)
342+        ui->numberOfConnections->setText(QString::number(count));
343+    else
344+        ui->numberOfConnections->setText(tr("%1 (Disabled)").arg(count));

Diapolo commented at 11:12 am on March 26, 2013:

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 commented at 11:16 am on March 26, 2013: none

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 commented at 2:50 pm on March 26, 2013: contributor

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.

in src/rpcnet.cpp: in 1df1718cf0 outdated

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

Diapolo commented at 2:54 pm on March 26, 2013:

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

jonls commented at 2:58 pm on March 26, 2013:

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.

jonls commented at 4:18 pm on March 29, 2013:

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

in src/qt/forms/rpcconsole.ui: in 1df1718cf0 outdated

211@@ -212,6 +212,19 @@
212         </widget>
213        </item>
214        <item row="9" column="0">
215+        <widget class="QPushButton" name="toggleNetworkActiveButton">
216+         <property name="toolTip">
217+          <string>Temporarily toggle network activity.</string>
218+         </property>
219+         <property name="text">
220+          <string>&amp;Toggle Network Activity</string>

Diapolo commented at 2:56 pm on March 26, 2013:

Nit-pick, I’m unsure whether to write all uppercase or just the first word.

jonls commented at 3:01 pm on March 26, 2013:

I’m unsure as well whether there is a policy regarding this. I see “Backup Wallet” in the File menu and then below “Sign message” so it seems there are some inconsistencies already.

Diapolo commented at 2:58 pm on March 26, 2013: none

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 commented at 8:34 am on March 29, 2013: member

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 commented at 10:27 am on March 29, 2013: contributor

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 commented at 4:25 pm on March 29, 2013: contributor

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 commented at 8:35 pm on March 29, 2013: none

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

rebroad commented at 0:31 am on March 30, 2013: contributor

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 commented at 10:09 am on March 30, 2013: contributor

@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 commented at 11:26 am on March 30, 2013: none

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 commented at 5:22 am on March 31, 2013: member

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 commented at 8:23 am on April 2, 2013: none

@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 commented at 5:33 am on April 8, 2013: member

Needs rebase.

jonls commented at 9:59 am on April 8, 2013: contributor

@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 commented at 1:34 pm on April 8, 2013: contributor

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 commented at 9:04 pm on April 12, 2013: member

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

Diapolo commented at 11:37 pm on April 12, 2013: none

I still like the idea!

jonls commented at 0:46 am on April 22, 2013: contributor

Rebased.

Diapolo commented at 9:11 pm on May 7, 2013: none

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

gmaxwell commented at 9:13 pm on May 7, 2013: contributor

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

jonasschnelli commented at 6:38 pm on September 11, 2013: contributor

@jonls could you rebase once more?

jonls commented at 1:14 am on September 13, 2013: contributor

@jonasschnelli done

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.

9189d7c949

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

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

c8e75ca135

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.

22570316ad

Diapolo commented at 0:53 am on October 20, 2013: none

What is the current state for this?

BitcoinPullTester commented at 0:59 am on October 20, 2013: none

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 commented at 1:37 am on October 21, 2013: contributor

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 commented at 9:00 pm on November 29, 2013: contributor

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

gavinandresen closed this on Nov 29, 2013

luke-jr commented at 8:03 am on September 13, 2014: member

@jonls Do you plan to update this?

jonls commented at 8:24 pm on September 13, 2014: contributor

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

DrahtBot locked this on Sep 8, 2021

Network activity toggle #2412