net: Add CNode::ConnectedThroughNetwork member function

hebasto commented at 9:17 pm on September 22, 2020: member

This PR:

adds CNode::ConnectedThroughNetwork member function
is based on #19991, and only last two commits belong to it
is required for https://github.com/bitcoin-core/gui/pull/86 and #20002

DrahtBot added the label P2P on Sep 22, 2020

DrahtBot added the label RPC/REST/ZMQ on Sep 22, 2020

DrahtBot added the label Validation on Sep 22, 2020

DrahtBot commented at 5:18 am on September 23, 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:

#20002 (net, rpc, cli: expose peer network in getpeerinfo; simplify/improve -netinfo by jonatack)
#19911 (net: guard vRecvGetData with cs_vRecv and orphan_work_set with g_cs_orphans by narula)
#18077 (net: Add NAT-PMP port forwarding support by hebasto)

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.

practicalswift commented at 5:35 am on September 23, 2020: contributor

Concept ACK

Nice! :)

hebasto force-pushed on Sep 23, 2020

laanwj commented at 11:05 am on September 23, 2020: member

Concept ACK

What about a multiple-choice option that more generally specifies the network that the connection (comes in/goes out) through, instead of specializing it for Tor? Might want to have the same for I2P later, for example.

Also this should be ‘onion’ to be consistent with our other uses.

hebasto commented at 11:21 am on September 23, 2020: member

@laanwj

What about a multiple-choice option that more generally specifies the network that the connection (comes in/goes out) through, instead of specializing it for Tor? Might want to have the same for I2P later, for example.

Do I understand you correctly that smth following is expected:

0$ src/bitcoin-cli help getpeerinfo | grep network_type
1    "network_type" : clearnet|onion|I2P,        Type of the network the peer has connected via

?

naumenkogs commented at 2:12 pm on September 23, 2020: member

Concept ACK. I agree with wladimir’s suggestions. @hebasto I think your understanding is correct.

jonatack commented at 2:21 pm on September 23, 2020: member

Concept ACK

What about a multiple-choice option that more generally specifies the network that the connection (comes in/goes out) through, instead of specializing it for Tor? Might want to have the same for I2P later, for example.

Also this should be ‘onion’ to be consistent with our other uses.

This was the goal in #20002, which exposes the GetNetClass() index (allowing for a significant code simplification in -netinfo). Feedback welcome if it should also expose the network class name.

hebasto commented at 2:33 pm on September 23, 2020: member

@jonatack

This was the goal in #20002, which exposes the GetNetClass() index (allowing for a significant code simplification in -netinfo). Feedback welcome if it should also expose the network class name.

What about limiting this PR to introducing CNode::GetNetClass() in addition to CNetAddr::GetNetClass() as the latter does not work for inbound connections via Tor?

Or just drop the last commit in favor of #20002 (if CNode::ConnectedViaTor() logic is sufficient)?

jonatack commented at 4:03 pm on September 23, 2020: member

@hebasto after chatting offline, that makes sense, will look at building on your commit “net: Add CNode::ConnectedViaTor() member function”.

Concept ACK in any case, will test your inbound onions detection.

hebasto force-pushed on Sep 23, 2020

hebasto commented at 4:09 pm on September 23, 2020: member

Updated fcd938e80abe634ccd0c0a289a8bdd339c186d24 -> 911a3f9929e2c10493c1ad525ad592dd6859df3f (pr19998.02 -> pr19998.03, diff):

dropped the last commit “rpc: Add via_tor to getpeerinfo output” in favor of more consistent approach in #20002 @laanwj @naumenkogs RPC part has been moved to #20002 now.

hebasto renamed this:
~~rpc: Add `via_tor` to `getpeerinfo` output~~
Net: Add CNode::ConnectedViaTor()
on Sep 23, 2020

in src/net.cpp:519 in 911a3f9929 outdated

513@@ -514,6 +514,15 @@ void CNode::SetAddrLocal(const CService& addrLocalIn) {
514     }
515 }
516 
517+bool CNode::ConnectedViaTor() const
518+{
519+    if (m_conn_type == ConnectionType::INBOUND) {

jonatack commented at 8:38 pm on September 23, 2020:

0    if (IsInboundConn()) {

hebasto commented at 5:28 pm on September 30, 2020:

Updated.

DrahtBot added the label Needs rebase on Sep 29, 2020

hebasto renamed this:
~~Net: Add CNode::ConnectedViaTor()~~
net: Add CNode::ConnectedViaTor()
on Sep 29, 2020

laanwj removed the label Validation on Sep 30, 2020

laanwj commented at 9:02 am on September 30, 2020: member

Haven’t reveiewed the code, but conceptually, for future-readyness I’d prefer this to be called ConnectedThroughNetwork and return an enumeration instead of have a Tor-specific call one the API.

Edit: oh, I suggested this before. Apparently I feel strongly about this :smile: .

$ src/bitcoin-cli help getpeerinfo | grep network_type “network_type” : clearnet|onion|I2P, Type of the network the peer has connected via

Yes, exactly! Or maybe even split clearnet into IPv4 and IPv6.

jonatack commented at 10:07 am on September 30, 2020: member

Yes, ATM returning an enum is delegated to the follow-up #20002, where the Network enum is returned using the existing GetNetworkName() function for the enum-to-string conversion. New networks like I2P and CJDNS can be added to that function when they are supported.

Feel free to combine the two steps into one if that is better.

hebasto force-pushed on Sep 30, 2020

hebasto renamed this:
~~net: Add CNode::ConnectedViaTor()~~
net: Add CNode::GetNetwork member function
on Sep 30, 2020

hebasto force-pushed on Sep 30, 2020

hebasto renamed this:
~~net: Add CNode::GetNetwork member function~~
net: Add CNode::ConnectedThroughNetwork member function
on Sep 30, 2020

DrahtBot removed the label Needs rebase on Sep 30, 2020

hebasto force-pushed on Sep 30, 2020

hebasto commented at 6:13 pm on September 30, 2020: member

Updated 911a3f9929e2c10493c1ad525ad592dd6859df3f -> cf532ae5d68c2e35e73c793923398e01ee7bca7f (pr19998.03 -> pr19998.06):

rebased on top of the recent #19991
addressed @laanwj’s comment:

Haven’t reveiewed the code, but conceptually, for future-readyness I’d prefer this to be called ConnectedThroughNetwork and return an enumeration instead of have a Tor-specific call one the API.

Edit: oh, I suggested this before. Apparently I feel strongly about this smile .

$ src/bitcoin-cli help getpeerinfo | grep network_type “network_type” : clearnet|onion|I2P, Type of the network the peer has connected via

Yes, exactly! Or maybe even split clearnet into IPv4 and IPv6.

hebasto force-pushed on Oct 1, 2020

hebasto commented at 4:37 pm on October 1, 2020: member

Updated cf532ae5d68c2e35e73c793923398e01ee7bca7f -> c972792511f764ff300602bd3c71df1fea90ddcc (pr19998.06 -> pr19998.07) to the recent #19991.

Sjors commented at 6:25 pm on October 1, 2020: member

utACK c972792

This will be easier to test after #20002 is refreshed (don’t forget to mention it in the description)

The asmap test failure on macOS Travis seems spurious (it passes on my machine)

hebasto force-pushed on Oct 2, 2020

hebasto commented at 11:43 am on October 2, 2020: member

Updated c972792511f764ff300602bd3c71df1fea90ddcc -> ac0f4975fe00f2f72c408cbd6b642b29b0e2f8ce (pr19998.07 -> pr19998.08) due to the conflict with #19991.

jonatack commented at 12:05 pm on October 2, 2020: member

Do you plan to add test coverage for the changes here (and in #19991?)

hebasto commented at 12:10 pm on October 2, 2020: member

@jonatack

Do you plan to add test coverage for the changes here (and in #19991?)

I thought your tests could cover all aspects, right?

jonatack commented at 12:39 pm on October 2, 2020: member

@hebasto I don’t plan to open a PR just for follow-up tests. I think it’s easiest when the coverage accompanies the changes.

jonatack commented at 5:04 pm on October 2, 2020: member

Starting to see inbound onions while testing this on #20002 (Edit: WIP update now pushed if anyone wants to use it for testing).

0<-> relay   net mping   ping send recv  txn  blk uptime  asmap  id address
1 in  full onion   358    469    0   90               49         35 127.0.0.1:43064
2 in block onion   450    722   29  104               35         66 127.0.0.1:45118

The updated function here is easy to use, too.

0stats.m_network = GetNetworkName(ConnectedThroughNetwork());

in src/net.h:590 in ac0f4975fe outdated

586@@ -586,6 +587,9 @@ class CConnman
587 
588     std::atomic<int64_t> m_next_send_inv_to_incoming{0};
589 
590+    //! -bind=<address>:<port>=onion arguments.

jonatack commented at 9:13 am on October 3, 2020:

this documentation is good to provide but a bit succinct; maybe also describe what m_onion_binds is
could use the /** doxygen formatting like m_try_another_outbound_peer just above

hebasto commented at 9:51 am on October 3, 2020:

Developer Notes suggest //! style to describe a member.

hebasto commented at 11:41 am on October 3, 2020:

Updated.

in src/net.cpp:547 in ac0f4975fe outdated

538@@ -538,6 +539,14 @@ void CNode::SetAddrLocal(const CService& addrLocalIn) {
539     }
540 }
541 
542+Network CNode::ConnectedThroughNetwork() const
543+{
544+    if (IsInboundConn() && m_inbound_onion) {
545+        return NET_ONION;
546+    }
547+    return addr.GetNetClass();

jonatack commented at 9:29 am on October 3, 2020:

optional style nit, could just simplify to a straight ternary

0-    if (IsInboundConn() && m_inbound_onion) {
1-        return NET_ONION;
2-    }
3-    return addr.GetNetClass();
4+    return IsInboundConn() && m_inbound_onion ? NET_ONION : addr.GetNetClass();

or a one-line guard conditional

0-    if (IsInboundConn() && m_inbound_onion) {
1-        return NET_ONION;
2-    }
3+    if (IsInboundConn() && m_inbound_onion) return NET_ONION;
4     return addr.GetNetClass();

hebasto commented at 11:41 am on October 3, 2020:

Updated.

in src/net.h:974 in ac0f4975fe outdated

957@@ -954,6 +958,8 @@ class CNode
958         assert(false);
959     }
960 
961+    Network ConnectedThroughNetwork() const;

jonatack commented at 9:42 am on October 3, 2020:

Can you add a doxygen comment here? Maybe provide a quick context on the difference between this and GetNetClass() or why it’s needed, for future readers of this code.

hebasto commented at 11:41 am on October 3, 2020:

Updated.

hebasto commented at 9:45 am on October 3, 2020: member

Added test coverage.

jonatack commented at 9:47 am on October 3, 2020: member

ACK, I’ve been testing this with #20002 and also rebased on #19954 (it’s thankfully a clean merge) and it seems to be working. A few optional comments below. It may be good to update fuzz/netaddress.cpp (or other fuzzers, I didn’t look further) with ConnectedThroughNetwork() and add unit or functional tests for the new -bind functionality. I did add some testing in #20002 for the values returned by ConnectedThroughNetwork(). Edit: reviewing the new push!

hebasto force-pushed on Oct 3, 2020

in src/test/net_tests.cpp:197 in 5fc8f15554 outdated

191@@ -192,14 +192,34 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test)
192     BOOST_CHECK(pnode1->IsFeelerConn() == false);
193     BOOST_CHECK(pnode1->IsAddrFetchConn() == false);
194     BOOST_CHECK(pnode1->IsInboundConn() == false);
195+    BOOST_CHECK_EQUAL(pnode1->ConnectedThroughNetwork(), Network::NET_IPV4);
196 
197-    std::unique_ptr<CNode> pnode2 = MakeUnique<CNode>(id++, NODE_NETWORK, height, hSocket, addr, 1, 1, CAddress(), pszDest, ConnectionType::INBOUND);
198+    std::unique_ptr<CNode> pnode2 = MakeUnique<CNode>(id++, NODE_NETWORK, height, hSocket, addr, 1, 1, CAddress(), pszDest, ConnectionType::INBOUND, false);

jonatack commented at 10:26 am on October 3, 2020:

here and lines 206 and 215

0    std::unique_ptr<CNode> pnode2 = MakeUnique<CNode>(id++, NODE_NETWORK, height, hSocket, addr, 1, 1, CAddress(), pszDest, ConnectionType::INBOUND, /* inbound_onion= */ false);

could optionally also add /*nKeyedNetGroupIn=*/ 1 and /*nLocalHostNonceIn=*/ 1

hebasto commented at 11:41 am on October 3, 2020:

Thanks! Updated.

jonatack commented at 10:37 am on October 3, 2020: member

ACK. The new unit tests are green for me locally. Not sure what’s up with the travis linter.

net: Add CNode::m_inbound_onion data member d4dde24034

hebasto force-pushed on Oct 3, 2020

hebasto commented at 11:40 am on October 3, 2020: member

Updated 5fc8f155549b1ad39aeef1c3220ebd6026d7f845 -> 3989fcfb6713b074c6c6be143db3aaa33e50525a (pr19998.10 -> pr19998.11):

addressed all of the recent @jonatack’s comments
rebased to avoid a weird CI error message

in src/net.h:968 in 3989fcfb67 outdated

960@@ -954,6 +961,16 @@ class CNode
961         assert(false);
962     }
963 
964+    /**
965+     * Get network the peer connected through.
966+     *
967+     * CNetAddr::GetNetClass() is used excluding inbound connections that are
968+     * explicitly tagged as connections from the Tor onion service(s).

jonatack commented at 11:54 am on October 3, 2020:

It’s much better. Perhaps:

Returns Network::NET_ONION for *inbound* onion connections that CNetAddr::GetNetClass() does not currently detect. Otherwise, returns CNetAddr::GetNetClass().

hebasto commented at 12:12 pm on October 3, 2020:

It’s for sure that my comment could be improved, but I don’t think that it is a correct assumption that it’s responsibility of CNetAddr class to detect the network a peer is connected through. I mean “CNetAddr::GetNetClass() does not currently detect” part.

jonatack commented at 12:30 pm on October 3, 2020:

Yes. That seems like a worthwhile clarification. Maybe something ~like:

Returns Network::NET_ONION for *inbound* onion connections, and CNetAddr::GetNetClass() otherwise. The latter cannot be used directly because it doesn't detect the former, and it's not the responsability of the CNetAddr class to know the actual network a peer is connected through.

hebasto commented at 12:42 pm on October 3, 2020:

Thanks! Updated.

jonatack approved

jonatack commented at 12:00 pm on October 3, 2020: member

ACK 3989fcfb6713b074c6c6be143db3aaa33e50525a

net: Add CNode::ConnectedThroughNetwork member function 49fba9c1aa

test: Add tests for CNode::ConnectedThroughNetwork 3984b78cd7

hebasto force-pushed on Oct 3, 2020

hebasto commented at 12:41 pm on October 3, 2020: member

Updated 3989fcfb6713b074c6c6be143db3aaa33e50525a -> 3984b78cd7f49e409377f2175a56e8e4bd71d1d8 (pr19998.11 -> pr19998.12, diff):

addressed @jonatack’s comment

jonatack commented at 1:12 pm on October 3, 2020: member

Thanks for your patience with the suggestions.

re-ACK 3984b78cd7f49e409377f2175a56e8e4bd71d1d8 per git diff 3989fcf 3984b78c

Edit: testing with #20002 (inbound onions detected server-side) vs on master (inbound onions detected client-side), I’m seeing the same ones detected.

laanwj commented at 4:37 pm on October 12, 2020: member

Code review ACK 3984b78cd7f49e409377f2175a56e8e4bd71d1d8 Thanks for addressing my suggestion. And making CNode::ConnectedThroughNetwork() return a Network enum directly is a nice touch.

laanwj merged this on Oct 12, 2020

laanwj closed this on Oct 12, 2020

hebasto deleted the branch on Oct 12, 2020

sidhujag referenced this in commit 2180fb4d7a on Oct 13, 2020

laanwj referenced this in commit 0d22482353 on Oct 15, 2020

sidhujag referenced this in commit 37b55282d6 on Oct 16, 2020

Fabcien referenced this in commit 498f31a05d on Nov 12, 2021

Fabcien referenced this in commit dd6ee52298 on Nov 12, 2021

Fabcien referenced this in commit 84b18a9f09 on Nov 12, 2021

DrahtBot locked this on Feb 15, 2022

net: Add CNode::ConnectedThroughNetwork member function #19998

Conflicts