net: Add CNode::ConnectedThroughNetwork member function #19998

pull hebasto wants to merge 3 commits into bitcoin:master from hebasto:200922-istor changing 6 files +83 −10
  1. hebasto commented at 9:17 PM on September 22, 2020: member

    This PR:

  2. DrahtBot added the label P2P on Sep 22, 2020
  3. DrahtBot added the label RPC/REST/ZMQ on Sep 22, 2020
  4. DrahtBot added the label Validation on Sep 22, 2020
  5. DrahtBot commented at 5:18 AM on September 23, 2020: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  6. practicalswift commented at 5:35 AM on September 23, 2020: contributor

    Concept ACK

    Nice! :)

  7. hebasto force-pushed on Sep 23, 2020
  8. 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.

  9. 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:

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

    ?

  10. 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.

  11. 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.

  12. 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)?

  13. 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.

  14. hebasto force-pushed on Sep 23, 2020
  15. 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.
  16. hebasto renamed this:
    rpc: Add `via_tor` to `getpeerinfo` output
    Net: Add CNode::ConnectedViaTor()
    on Sep 23, 2020
  17. 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: 
        if (IsInboundConn()) {
    hebasto commented at 5:28 PM on September 30, 2020:

    Updated.

  18. DrahtBot added the label Needs rebase on Sep 29, 2020
  19. hebasto renamed this:
    Net: Add CNode::ConnectedViaTor()
    net: Add CNode::ConnectedViaTor()
    on Sep 29, 2020
  20. laanwj removed the label Validation on Sep 30, 2020
  21. 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.

  22. 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.

  23. hebasto force-pushed on Sep 30, 2020
  24. hebasto renamed this:
    net: Add CNode::ConnectedViaTor()
    net: Add CNode::GetNetwork member function
    on Sep 30, 2020
  25. hebasto force-pushed on Sep 30, 2020
  26. hebasto renamed this:
    net: Add CNode::GetNetwork member function
    net: Add CNode::ConnectedThroughNetwork member function
    on Sep 30, 2020
  27. DrahtBot removed the label Needs rebase on Sep 30, 2020
  28. hebasto force-pushed on Sep 30, 2020
  29. hebasto commented at 6:13 PM on September 30, 2020: member

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

    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.

  30. hebasto force-pushed on Oct 1, 2020
  31. hebasto commented at 4:37 PM on October 1, 2020: member

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

  32. 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)

  33. hebasto force-pushed on Oct 2, 2020
  34. hebasto commented at 11:43 AM on October 2, 2020: member

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

  35. 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?)

  36. 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?

  37. 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.

  38. 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).

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

    The updated function here is easy to use, too.

    stats.m_network = GetNetworkName(ConnectedThroughNetwork());
  39. 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.

  40. 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

    -    if (IsInboundConn() && m_inbound_onion) {
-        return NET_ONION;
-    }
-    return addr.GetNetClass();
+    return IsInboundConn() && m_inbound_onion ? NET_ONION : addr.GetNetClass();

    or a one-line guard conditional

    -    if (IsInboundConn() && m_inbound_onion) {
-        return NET_ONION;
-    }
+    if (IsInboundConn() && m_inbound_onion) return NET_ONION;
     return addr.GetNetClass();
    hebasto commented at 11:41 AM on October 3, 2020:

    Updated.

  41. 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.

  42. hebasto commented at 9:45 AM on October 3, 2020: member

    Added test coverage.

  43. 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!

  44. hebasto force-pushed on Oct 3, 2020
  45. 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

        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.

  46. 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.

  47. net: Add CNode::m_inbound_onion data member d4dde24034
  48. hebasto force-pushed on Oct 3, 2020
  49. hebasto commented at 11:40 AM on October 3, 2020: member

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

  50. 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.

  51. jonatack approved
  52. jonatack commented at 12:00 PM on October 3, 2020: member

    ACK 3989fcfb6713b074c6c6be143db3aaa33e50525a

  53. net: Add CNode::ConnectedThroughNetwork member function 49fba9c1aa
  54. test: Add tests for CNode::ConnectedThroughNetwork 3984b78cd7
  55. hebasto force-pushed on Oct 3, 2020
  56. hebasto commented at 12:41 PM on October 3, 2020: member

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

  57. 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.

  58. 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.

  59. laanwj merged this on Oct 12, 2020
  60. laanwj closed this on Oct 12, 2020
  61. hebasto deleted the branch on Oct 12, 2020
  62. sidhujag referenced this in commit 2180fb4d7a on Oct 13, 2020
  63. laanwj referenced this in commit 0d22482353 on Oct 15, 2020
  64. sidhujag referenced this in commit 37b55282d6 on Oct 16, 2020
  65. Fabcien referenced this in commit 498f31a05d on Nov 12, 2021
  66. Fabcien referenced this in commit dd6ee52298 on Nov 12, 2021
  67. Fabcien referenced this in commit 84b18a9f09 on Nov 12, 2021
  68. DrahtBot locked this on Feb 15, 2022
Contributors
hebastoDrahtBotpracticalswiftlaanwjnaumenkogsjonatackSjors
Labels
RPC/REST/ZMQP2P
Linked (view graph)
#19991 net: Use alternative port for incoming Tor connections#20002 net, rpc, cli: expose peer network in getpeerinfo; simplify/improve -netinfo
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 21:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me