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…?
Concept ACK
While trying to test this, I have got:
0Assertion 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…
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).
During startup, when there is 1 outgoing connection only:
02016-10-23 16:44:42 SetNetworkActive: false
12016-10-23 16:44:42 disconnecting peer=1
22016-10-23 16:44:42 socket select error Bad file descriptor (9)
32016-10-23 16:44:42 disconnecting peer=2
4nMaxConnections = 8
5nMaxFeeler = 1
6nMaxOutbound = 8
7nMaxInbound = -1
8Assertion failed: (nMaxInbound > 0), function AcceptConnection, file net.cpp, line 979.
@EthanHeilman Do you have an idea?
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.
1238+ clientModel->setNetworkActive(!clientModel->getNetworkActive());
1239+ }
1240+}
1241+
1242+/** Lets the control know about the Client Model */
1243+void NetworkToggleStatusBarControl::setClientModel(ClientModel *clientModel)
_clientModel please to prevent shadowing.
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
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:
@paveljanik At the risk of stating the obvious if nMaxConnections is set to 8 then nMaxInbound will be set to -1 triggering the assert.
0int nMaxInbound = nMaxConnections - (nMaxOutbound + nMaxFeeler);
1assert(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?
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.
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!
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.
Tested ACK 19f46f177ec5e1913f9be5b257dad95bc7a57c38.
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.
2038+ if (fDebug) {
2039+ LogPrint("net", "SetNetworkActive: %s\n", active);
2040+ }
2041+
2042+ if (!active) {
2043+ fNetworkActive = false;
std::atomic<bool>, but it’s a plain bool.