rpc, net: Expose connections_onion_only in getnetworkinfo RPC output

hebasto commented at 8:56 pm on October 16, 2020: member

This is a non-gui part of adding Tor icon to the GUI.

Split from https://github.com/bitcoin-core/gui/pull/86 as requested:

Also, the RPC and net changes are not gui related, so should go into the main repo

The first approach was in #19926.

net: Replace uiInterface global with CConnman::clientInterface member 2596322109

Drop unused argument in NetworkActiveChanged signal 7eccce5788

Drop unused argument in NumConnectionsChanged signal f4cbe6b4c2

hebasto commented at 10:03 pm on October 16, 2020: member

The commits moved from https://github.com/bitcoin-core/gui/pull/86 already have two ACKs:

from @jnewbery https://github.com/bitcoin-core/gui/pull/86#issuecomment-691905056

Code review ACK https://github.com/bitcoin-core/gui/commit/6549abcb072d73e0ac91c7992cc5d86eb4064b70 (node changes only. Did not fully review qt changes)

from @Sjors https://github.com/bitcoin-core/gui/pull/86#pullrequestreview-489554178

tACK https://github.com/bitcoin-core/gui/commit/6549abcb072d73e0ac91c7992cc5d86eb4064b70 with notes for followup

in src/rpc/net.cpp:540 in 174848ffb8 outdated

532@@ -533,10 +533,11 @@ static RPCHelpMan getnetworkinfo()
533                         }},
534                         {RPCResult::Type::BOOL, "localrelay", "true if transaction relay is requested from peers"},
535                         {RPCResult::Type::NUM, "timeoffset", "the time offset"},
536+                        {RPCResult::Type::BOOL, "networkactive", "whether p2p networking is enabled"},
537                         {RPCResult::Type::NUM, "connections", "the total number of connections"},
538                         {RPCResult::Type::NUM, "connections_in", "the number of inbound connections"},
539                         {RPCResult::Type::NUM, "connections_out", "the number of outbound connections"},
540-                        {RPCResult::Type::BOOL, "networkactive", "whether p2p networking is enabled"},
541+                        {RPCResult::Type::BOOL, "connections_onion_only", "whether all connection are through the onion network"},

jonatack commented at 10:30 pm on October 16, 2020:

s/all connection/all connections/

hebasto commented at 12:26 pm on October 17, 2020:

Thanks! Updated.

in src/net.h:200 in 174848ffb8 outdated

195+    }
196+
197+    ConnCounts(int num_in, int num_out, bool is_onion_only)
198+        : all(num_in + num_out), in(num_in), out(num_out), onion_only(is_onion_only)
199+    {
200+    }

jonatack commented at 10:36 pm on October 16, 2020:

Style nit: I’d put both of the ConnCounts declarations all on one line with {}. It looks like this is clang-format output, but ISTM this particular formatting is ugly, wastes space, and isn’t followed in the new code additions we see while reviewing. Maybe others disagree :man_shrugging:

hebasto commented at 12:26 pm on October 17, 2020:

Thanks! Updated.

DrahtBot added the label GUI on Oct 16, 2020

DrahtBot added the label Mining on Oct 16, 2020

DrahtBot added the label P2P on Oct 16, 2020

DrahtBot added the label RPC/REST/ZMQ on Oct 16, 2020

jonatack commented at 11:05 pm on October 16, 2020: member

Approach ACK, only skimmed the code so far but the changes look good. Needs test coverage for the new getnetworkinfo “connections_onion_only” field. Debug build is clean and I did some quick manual testing. The counts fields contain the correct values. With mixed peers:

0  "connections_onion_only": false,

With only onion peers:

0  "connections_onion_only": true,

RPC help:

0  "connections_onion_only" : true|false,             (boolean) whether all connection are through the onion network

DrahtBot commented at 2:57 am on October 17, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20494 (refactor: Move node and wallet code out of src/interfaces by ryanofsky)
#20267 (Disable and fix tests for when BDB is not compiled by achow101)
#19937 (signet mining utility by ajtowns)
#19771 (net: Replace enum CConnMan::NumConnections with enum class ConnectionDirection by luke-jr)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#19315 ([tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar)
#19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
#17167 (Allow whitelisting outgoing connections by luke-jr)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Sjors commented at 7:52 am on October 17, 2020: member

Concept ACK.

Agree that a functional test for connections_onion_only would be good, but not sure how easy it is. You could spin up two test nodes, 0 with default settings, 1 with -onlynet=onion and -bind=...=onion, and then connect node 0 to node 1 via it’s “onion” port. In that case node 1 should have connections_onion_only=true.

hebasto force-pushed on Oct 17, 2020

hebasto commented at 12:25 pm on October 17, 2020: member

Updated 174848ffb88767fe9190dd7fa160e7187dfcdc07 -> 9371757d9031ad40ca99543bb4d24fc48e00b32e (pr20172.01 -> pr20172.02, diff):

addressed all of @jonatack’s and @Sjors’ comments
added a functional test

hebasto force-pushed on Oct 17, 2020

hebasto commented at 9:18 pm on October 17, 2020: member

~Hmm, the current pr20172.03 branch does not pass macOS 10.14 build on Travis.~

~The alternative pr20172.04 branch pass locally on macOS 10.15 and Linux Mint 20 (Focal codebase), but it does not pass on Travis at all.~

~What is the correct portable way to -bind=localhost and -bind=localhost:port=onion simultaneously?~

UPDATE: it was such a silly error, and I’ve caught it after sleep :smiley:

hebasto force-pushed on Oct 18, 2020

hebasto commented at 12:02 pm on October 18, 2020: member

Updated 9371757d9031ad40ca99543bb4d24fc48e00b32e -> d301cc5c564f8c130949567602e182917efef18e (pr20172.02 -> pr20172.06, diff):

fixed functional test

Travis is green now!

in src/net.h:354 in d301cc5c56 outdated

349@@ -346,7 +350,8 @@ class CConnman
350     bool RemoveAddedNode(const std::string& node);
351     std::vector<AddedNodeInfo> GetAddedNodeInfo();
352 
353-    size_t GetNodeCount(NumConnections num);
354+    size_t PeerCount() { return WITH_LOCK(cs_vNodes, return vNodes.size()); }
355+    ConnCounts ConnectionCounts();

jnewbery commented at 8:44 am on October 19, 2020:

Perhaps name this function GetConnectionCounts()?

hebasto commented at 10:35 pm on October 19, 2020:

Sure! Updated.

in test/functional/feature_onion.py:61 in d301cc5c56 outdated

56+        assert_equal(network_info["connections_in"], 1)
57+        assert_equal(network_info["connections_out"], 0)
58+        assert_equal(network_info["connections_onion_only"], True)
59+
60+        self.log.info("Creating an outbound onion connection, and checking...")
61+        self.nodes[0].addnode("bitcoinostk4e4re.onion:8333", "onetry")

jnewbery commented at 9:07 am on October 19, 2020:

What is this onion address?

hebasto commented at 9:14 am on October 19, 2020:

It has been taken from https://github.com/bitcoin/bitcoin/blob/d9d9a293520d26ca8fec8caebab46db103b29903/test/functional/feature_proxy.py#L139

jnewbery commented at 9:09 am on October 19, 2020: member

One question and one small naming suggestion.

This PR contains qt-only changes. It shouldn’t be too difficult to separate the changes so that PRs to the main repo only contain changes to the node code, and PRs to the GUI repo only contain changes to qt code.

hebasto commented at 9:20 am on October 19, 2020: member

@jnewbery

This PR contains qt-only changes. It shouldn’t be too difficult to separate the changes so that PRs to the main repo only contain changes to the node code, and PRs to the GUI repo only contain changes to qt code.

Clear qt-only changes are already leaved in the GUI repo. This pull contains two commits that modify signal signatures. As these signals propagate through the qt code, I do not want to break those commits in “non-qt” and “qt” parts.

in test/functional/feature_onion.py:50 in d301cc5c56 outdated

45+        self.log.info("Checking a node without any peers...")
46+        network_info = self.nodes[0].getnetworkinfo()
47+        assert_equal(network_info["connections"], 0)
48+        assert_equal(network_info["connections_in"], 0)
49+        assert_equal(network_info["connections_out"], 0)
50+        assert_equal(network_info["connections_onion_only"], False)

jnewbery commented at 10:06 am on October 19, 2020:

This repetitious code could potentially be refactored into a small helper function get_connection_info().

hebasto commented at 10:36 pm on October 19, 2020:

Thanks! Reworked.

in test/functional/feature_onion.py:65 in d301cc5c56 outdated

60+        self.log.info("Creating an outbound onion connection, and checking...")
61+        self.nodes[0].addnode("bitcoinostk4e4re.onion:8333", "onetry")
62+        network_info = self.nodes[0].getnetworkinfo()
63+        assert_equal(network_info["connections"], 2)
64+        assert_equal(network_info["connections_in"], 1)
65+        assert_equal(network_info["connections_out"], 1)

jnewbery commented at 10:07 am on October 19, 2020:

I’m confused about how this works. It creates an outbound connection to where? Why does that outbound connection show up here, but is gone by the time you call getnetworkinfo() again?

hebasto commented at 10:12 am on October 19, 2020:

It creates an outbound connection to where?

To created self.proxy.

Why does that outbound connection show up here, but is gone by the time you call getnetworkinfo() again?

~IDK why add_p2p_connection(P2PInterface()) causes drop of the connection to the proxy.~

hebasto commented at 10:29 pm on October 19, 2020:

Why does that outbound connection show up here, but is gone by the time you call getnetworkinfo() again?

Connections to a dummy Socks5 server are short-lived:https://github.com/bitcoin/bitcoin/blob/45385018e13e167521e655c36128d8ee4f2a3e0b/test/functional/test_framework/socks5.py#L113-L124

hebasto commented at 10:36 pm on October 19, 2020:

Reworked.

Split CConnman::GetNodeCount() in two functions

This change prevents repetitive traversing vNodes.

72632da03f

rpc: Add `connections_onion_only` to `getnetworkinfo` output a855538a20

hebasto force-pushed on Oct 19, 2020

hebasto commented at 10:35 pm on October 19, 2020: member

Updated d301cc5c564f8c130949567602e182917efef18e -> caebabfc4b239bfb854228f041a6b4220e87e3af (pr20172.06 -> pr20172.09, diff):

reworked functional test
addressed @jnewbery’s comment:

This repetitious code could potentially be refactored into a small helper function get_connection_info().

hebasto force-pushed on Oct 19, 2020

qa: Add feature_onion.py da8b1092fc

in test/functional/feature_onion.py:33 in 85ff09ef5d outdated

28+        self.add_nodes(self.num_nodes)
29+        self.start_nodes()
30+
31+    def test_connections(self, in_clearnet=0, in_onion=0, out_onion=0):
32+        self.restart_node(0, ['-bind={}=onion'.format(self.bind_onion), self.arg_onion])
33+        self.restart_node(1)

jnewbery commented at 8:01 am on October 20, 2020:

Stop-starting the nodes 11 times just to test new connections seems very inefficient. Is there a reason you need to do this stop-start?

hebasto commented at 10:47 am on October 20, 2020:

Thanks! Updated.

hebasto force-pushed on Oct 20, 2020

hebasto commented at 10:47 am on October 20, 2020: member

Updated 85ff09ef5d4da9991ee1e9761662d003ff464679 -> da8b1092fc2d5fa5ee9da3310dae16d4c919bc1b (pr20172.10 -> pr20172.11, diff):

addressed @jnewbery’s comment:

Stop-starting the nodes 11 times just to test new connections seems very inefficient. Is there a reason you need to do this stop-start?

improved robustness

Sjors commented at 12:39 pm on October 20, 2020: member

tACK da8b109

luke-jr changes_requested

luke-jr commented at 7:16 pm on October 24, 2020: member

Concept NACK per https://github.com/bitcoin-core/gui/pull/86#pullrequestreview-516300696

kristapsk commented at 3:30 pm on November 5, 2020: contributor

I’m thinking about this in a context of if / when other privacy network support is added to Bitcoin Core (like I2P, #20254). What people might want then is not “are all of my connections Tor only?”, but “are all of my connections using privacy networks?” and “which privacy networks are they using?”.

DrahtBot added the label Needs rebase on Dec 1, 2020

DrahtBot commented at 9:43 am on December 1, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

hebasto commented at 7:55 pm on May 3, 2021: member

Closing. Leaving it up for grabs.

hebasto closed this on May 3, 2021

DrahtBot locked this on Aug 18, 2022

rpc, net: Expose connections_onion_only in getnetworkinfo RPC output #20172

Conflicts