net: fix GetListenPort() to derive the proper port #20196

pull vasild wants to merge 6 commits into bitcoin:master from vasild:fix_GetListenPort changing 12 files +454 −27
  1. vasild commented at 10:01 am on October 20, 2020: member

    GetListenPort() uses a simple logic: “if -port=P is given, then we must be listening on P, otherwise we must be listening on 8333”. This is however not true if -bind= has been provided with :port part or if -whitebind= has been provided. Thus, extend GetListenPort() to return the port from -bind= or -whitebind=, if any.

    Also, if -bind= is provided then we would bind only to a particular address and should not add all the other addresses of the machine to the list of local addresses.

    Fixes #20184

  2. fanquake added the label P2P on Oct 20, 2020
  3. MarcoFalke added this to the milestone 0.21.1 on Oct 20, 2020
  4. MarcoFalke removed this from the milestone 0.21.1 on Oct 20, 2020
  5. vasild force-pushed on Oct 20, 2020
  6. DrahtBot commented at 2:37 pm on October 20, 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:

    • #24455 (refactor: Split ArgsManager out of util/system by Empact)
    • #24273 (p2p: Split network logging into two categories #24247 by anshu-khare-design)
    • #23561 (BIP324: Handshake prerequisites by dhruv)
    • #23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)

    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.

  7. in test/functional/feature_bind_port_externalip.py:72 in e6c3de7178 outdated
    68+            for local in self.nodes[i].getnetworkinfo()['localaddresses']:
    69+                if local['address'] == '2.2.2.2':
    70+                    assert_equal(local['port'], expected_port)
    71+                    found = True
    72+                    break
    73+            assert(found)
    


    MarcoFalke commented at 2:40 pm on October 21, 2020:

    I believe this can be written shorter

    0            local = next(l for l in self.nodes[i].getnetworkinfo()['localaddresses'] if l['address'] == '2.2.2.2')
    1            assert_equal(local['port'], expected_port)
    

    vasild commented at 9:13 am on October 22, 2020:

    To me that is less readable. Maybe my python level is poor, but then maybe other readers of this are also poor pythoners. I prefer to keep it simple stupid:

    0for ...
    1    if ...
    2        break
    

    that is nearly the same in any programming language.


    jonatack commented at 9:57 pm on March 17, 2021:

    A few micro-optimisations

     0+LEN_EXPECTED = len(EXPECTED)
     1 
     2 class BindPortExternalIPTest(BitcoinTestFramework):
     3     def set_test_params(self):
     4         # Avoid any -bind= on the command line. Force the framework to avoid adding -bind=127.0.0.1.
     5         self.setup_clean_chain = True
     6         self.bind_to_localhost_only = False
     7-        self.num_nodes = len(EXPECTED)
     8+        self.num_nodes = LEN_EXPECTED
     9         self.extra_args = list(map(lambda e: e[0], EXPECTED))
    10 
    11     def add_options(self, parser):
    12@@ -58,11 +59,13 @@ class BindPortExternalIPTest(BitcoinTestFramework):
    13                 "to one of the interfaces on this machine and rerun with --ihave1111".format(ADDR))
    14 
    15     def run_test(self):
    16-        for i in range(len(EXPECTED)):
    17-            if EXPECTED[i][1] == 'default_p2p_port':
    18+        self.log.info("Test the proper port is used for -externalip=")
    19+        for i in range(LEN_EXPECTED):
    20+            port = EXPECTED[i][1]
    21+            if port == 'default_p2p_port':
    22                 expected_port = p2p_port(i)
    23             else:
    24-                expected_port = EXPECTED[i][1]
    25+                expected_port = port
    
  8. in test/functional/feature_bind_port_externalip.py:53 in e6c3de7178 outdated
    48+        parser.add_argument(
    49+            "--ihave1111", action='store_true', dest="ihave1111",
    50+            help="Run the test, assuming {} is configured on the machine".format(addr),
    51+            default=False)
    52+
    53+    def setup_nodes(self):
    


    MarcoFalke commented at 2:45 pm on October 21, 2020:
    Does this need to be overwritten? You can use skip_test_if_missing_module

    vasild commented at 9:09 am on October 22, 2020:

    raise SkipTest() is not the main reason to override setup_nodes(). It is necessary to specify the command line arguments for each bitcoind node. The current test does that by self.add_nodes(..., extra_args=my_variable_here) in setup_nodes(). Other tests do that either in setup_nodes() or setup_network().

    Is there a better way?


    MarcoFalke commented at 10:51 am on October 22, 2020:
    Set self.extra_args = ...?

    vasild commented at 11:21 am on October 22, 2020:
    Indeed! Changed, thanks for the suggestion!
  9. in test/functional/feature_bind_port_externalip.py:23 in e6c3de7178 outdated
    18+# ifconfig lo0 1.1.1.1/32 alias  # to set up
    19+# ifconfig lo0 1.1.1.1 -alias  # to remove it, after the test
    20+addr = '1.1.1.1'
    21+
    22+# array of tuples [arguments, expected port in localaddresses]
    23+expected = [
    


    MarcoFalke commented at 2:45 pm on October 21, 2020:
    uppercase constants

    vasild commented at 11:22 am on October 22, 2020:
    Changed
  10. in src/init.cpp:1947 in e6c3de7178 outdated
    1941@@ -1944,11 +1942,15 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
    1942     connOptions.nMaxOutboundLimit = nMaxOutboundLimit;
    1943     connOptions.m_peer_connect_timeout = peer_connect_timeout;
    1944 
    1945+    // Port to bind to if `-bind=addr` is provided without a `:port` suffix.
    1946+    const uint16_t default_bind_port =
    1947+        static_cast<uint16_t>(gArgs.GetArg("-port", Params().GetDefaultPort()));
    


    MarcoFalke commented at 2:46 pm on October 21, 2020:
    0        static_cast<uint16_t>(args.GetArg("-port", Params().GetDefaultPort()));
    

    vasild commented at 11:22 am on October 22, 2020:
    Changed
  11. in src/test/net_tests.cpp:757 in e6c3de7178 outdated
    752+    // Without the fix peer_us:8333 ends up in vAddrToSend instead of the proper
    753+    // peer_us:peer_us.GetPort().
    754+    BOOST_CHECK(std::any_of(peer_in.vAddrToSend.begin(), peer_in.vAddrToSend.end(),
    755+                            [&peer_us](const CAddress& cur) { return cur == peer_us; }));
    756+
    757+    gArgs.ForceSetArg("-bind", "");
    


    MarcoFalke commented at 2:48 pm on October 21, 2020:
    would be good to remove the global here, but that can be done in a follow-up

    vasild commented at 11:23 am on October 22, 2020:
    Changed to m_node.args->ForceSetArg("-bind", "");
  12. in src/test/net_tests.cpp:887 in e6c3de7178 outdated
    881+
    882+    // Force CChainState::IsInitialBlockDownload() to return false.
    883+    const auto nMinimumChainWork_orig = nMinimumChainWork;
    884+    const auto nMaxTipAge_orig = nMaxTipAge;
    885+    nMinimumChainWork = 0;
    886+    nMaxTipAge = GetTime();
    


    MarcoFalke commented at 2:51 pm on October 21, 2020:

    Can be written shorter

    0    generatetoaddress(m_node, ADDRESS_BCRT1_UNSPENDABLE);
    

    vasild commented at 10:19 am on October 22, 2020:

    CChainState::IsInitialBlockDownload() still returns true :/

    (it was necessary to s/TestingSetup/RegTestingSetup/ in BOOST_FIXTURE_TEST_SUITE() in order to get generatetoaddress() to accept ADDRESS_BCRT1_UNSPENDABLE)


    MarcoFalke commented at 11:37 am on October 22, 2020:

    Oh, sorry my bad. The mined block will have the timestamp of genesis (plus one), so the time is still too far back.

    An alternative would be to use mocktime, also ugly, but less LOC.

     0diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
     1index 9519f3fd0c..7e77b758b2 100644
     2--- a/src/test/net_tests.cpp
     3+++ b/src/test/net_tests.cpp
     4@@ -88,7 +88,7 @@ static CDataStream AddrmanToStream(CAddrManSerializationMock& _addrman)
     5     return CDataStream(vchData, SER_DISK, CLIENT_VERSION);
     6 }
     7 
     8-BOOST_FIXTURE_TEST_SUITE(net_tests, TestingSetup)
     9+BOOST_FIXTURE_TEST_SUITE(net_tests, RegTestingSetup)
    10 
    11 BOOST_AUTO_TEST_CASE(cnode_listen_port)
    12 {
    13@@ -879,11 +879,10 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
    14 
    15     CDataStream recv_stream{msg.data, SER_NETWORK, PROTOCOL_VERSION};
    16 
    17-    // Force CChainState::IsInitialBlockDownload() to return false.
    18-    const auto nMinimumChainWork_orig = nMinimumChainWork;
    19-    const auto nMaxTipAge_orig = nMaxTipAge;
    20-    nMinimumChainWork = 0;
    21-    nMaxTipAge = GetTime();
    22+    // Force CChainState::IsInitialBlockDownload() to return false, otherwise PushAddress isn't called
    23+    SetMockTime(1);
    24+    BOOST_REQUIRE(!m_node.chainman->ActiveChainstate().IsInitialBlockDownload());
    25+    SetMockTime(0);
    26 
    27     m_node.peerman->InitializeNode(&peer);
    28 
    29@@ -898,9 +897,6 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
    30     BOOST_CHECK(std::any_of(peer.vAddrToSend.begin(), peer.vAddrToSend.end(),
    31                             [&expected](const CAddress& cur) { return cur == expected; }));
    32 
    33-    nMinimumChainWork = nMinimumChainWork_orig;
    34-    nMaxTipAge = nMaxTipAge_orig;
    35-
    36     gArgs.ForceSetArg("-bind", "");
    37 }
    38 
    

    vasild commented at 12:34 pm on October 22, 2020:
    Changed, less LOC. Thanks!
  13. MarcoFalke commented at 2:51 pm on October 21, 2020: member
    left some style test nits
  14. vasild force-pushed on Oct 22, 2020
  15. vasild commented at 11:25 am on October 22, 2020: member
    Addressed review suggestions and added a test to cover case 4. from #20184.
  16. in test/functional/feature_bind_port_discover.py:46 in 1ae0d404f8 outdated
    37+            help="Run the test, assuming {} and {} are configured on the machine".format(ADDR1, ADDR2),
    38+            default=False)
    39+
    40+    def setup_nodes(self):
    41+        if not self.options.ihave1111and2222:
    42+            raise SkipTest(
    


    MarcoFalke commented at 11:43 am on October 22, 2020:
    same feedback here ;)

    vasild commented at 12:33 pm on October 22, 2020:
    Changed
  17. vasild force-pushed on Oct 22, 2020
  18. vasild commented at 12:34 pm on October 22, 2020: member
    Addressed review suggestions.
  19. vasild force-pushed on Oct 22, 2020
  20. vasild commented at 4:32 pm on October 22, 2020: member
    Fixed linter error feature_bind_port_discover.py:25:18: E703 statement ends with a semicolon :facepalm:
  21. in test/functional/feature_bind_port_externalip.py:24 in 053ba089fc outdated
    19+# ifconfig lo0 1.1.1.1 -alias  # to remove it, after the test
    20+ADDR = '1.1.1.1'
    21+
    22+# array of tuples [arguments, expected port in localaddresses]
    23+EXPECTED = [
    24+    [['-externalip=2.2.2.2',       '-port=20001'                               ], 20001],
    


    vasild commented at 1:37 pm on October 23, 2020:
    The ports used better not be hardcoded but derived from p2p_port() in order not to clash with other tests that may be running on the system at the same time.

    MarcoFalke commented at 2:06 pm on October 23, 2020:
    Or you could start in a fresh range (ports that are not used by p2p or rpc in other tests)

    vasild commented at 5:33 pm on December 17, 2020:
    Changed to start in a fresh range from 30000 as that is the easiest (maximum that can be returned by p2p_port() or rpc_port() is PORT_MIN + 2 * PORT_RANGE which his 21000 :eyes:).
  22. DrahtBot added the label Needs rebase on Dec 16, 2020
  23. vasild force-pushed on Dec 17, 2020
  24. vasild commented at 5:34 pm on December 17, 2020: member
    Rebased and changed port numbers used in the functional tests.
  25. DrahtBot removed the label Needs rebase on Dec 17, 2020
  26. DrahtBot added the label Needs rebase on Jan 7, 2021
  27. in src/test/net_tests.cpp:1042 in 3e55fbfe31 outdated
    1037+
    1038+    // Force CChainState::IsInitialBlockDownload() to return false.
    1039+    // Otherwise PushAddress() isn't called by PeerManager::ProcessMessage().
    1040+    SetMockTime(1);
    1041+    BOOST_REQUIRE(!m_node.chainman->ActiveChainstate().IsInitialBlockDownload());
    1042+    SetMockTime(0);
    


    MarcoFalke commented at 10:52 am on January 13, 2021:

    nit: Might be possible to write, shorter, and to abstract away the inner workings of IsIbd

    0TestChainState& chainstate = *(TestChainState*)&m_node.chainman->ActiveChainstate();
    1chainstate.JumpOutOfIbd();
    

    vasild commented at 1:05 pm on January 14, 2021:
    Done. This cast to a class which it actually is not looks quite hackish.
  28. vasild force-pushed on Jan 14, 2021
  29. vasild commented at 1:04 pm on January 14, 2021: member
    3e55fbfe3…5647ca32e: rebased due to conflicts + address a suggestion.
  30. DrahtBot removed the label Needs rebase on Jan 14, 2021
  31. vasild force-pushed on Jan 17, 2021
  32. vasild commented at 12:33 pm on January 17, 2021: member
    5647ca32e…58755690a: rebase, what’s up with appveyor?
  33. hebasto commented at 1:13 pm on January 17, 2021: member

    @vasild

    what’s up with appveyor?

    What do you mean?

  34. in src/test/net_tests.cpp:960 in 58755690a7 outdated
    1045+
    1046+    std::atomic<bool> interrupt_dummy{false};
    1047+    std::chrono::microseconds time_received_dummy{0};
    1048+
    1049+    m_node.peerman->ProcessMessage(peer, NetMsgType::VERSION, recv_stream, time_received_dummy,
    1050+                                   interrupt_dummy);
    


    MarcoFalke commented at 1:21 pm on January 17, 2021:
    I presume this line will make appveyor etc fail if the test process isn’t restarted in between test cases.

    MarcoFalke commented at 1:22 pm on January 17, 2021:

    See

    0Test cases order is shuffled using seed: 193951562
    

    https://cirrus-ci.com/task/5953114714931200?command=ci#L3792

    Which should make it reproducible


    vasild commented at 1:47 pm on January 18, 2021:
    Thanks, that helped a lot nailing the issue down. Fixed now.
  35. vasild force-pushed on Jan 18, 2021
  36. vasild commented at 1:49 pm on January 18, 2021: member
    58755690a…d8c6dc0b6: undo the effects of AddTimeData() called indirectly by the newly added test net_tests/initial_advertise_from_version_message so that timedata_tests/addtimedata is not confused. Thanks, @MarcoFalke!
  37. in src/test/timedata_tests.cpp:102 in 9d8bb13b1d outdated
     95@@ -96,9 +96,10 @@ BOOST_AUTO_TEST_CASE(addtimedata)
     96     // not to fix this because it prevents possible attacks. See the comment in AddTimeData() or issue #4521
     97     // for a more detailed explanation.
     98     MultiAddTimeData(2, 100); // filter median is 100 now, but nTimeOffset will not change
     99+    // We want this test to end with nTimeOffset==0, otherwise subsequent tests of the suite will fail.
    100     BOOST_CHECK_EQUAL(GetTimeOffset(), 0);
    101 
    102-    // We want this test to end with nTimeOffset==0, otherwise subsequent tests of the suite will fail.
    103+    ResetTimeData();
    


    MarcoFalke commented at 4:37 pm on January 18, 2021:
    in 9d8bb13b1dfc5103e2678ba803ecd3220dce052d: Shouldn’t the reset be before the check equal?

    vasild commented at 10:46 am on January 19, 2021:
    I don’t think so. The BOOST_CHECK_EQUAL(GetTimeOffset(), 0) check is to ensure that the net result from all the previous calls is 0. This is still true and better keep that check to ensure it remains so. Surely the check will also pass after ResetTimeData() not because the net result is 0, but because the reset obliterated the entire state.
  38. in src/timedata.cpp:111 in 9d8bb13b1d outdated
    106@@ -106,3 +107,12 @@ void AddTimeData(const CNetAddr& ip, int64_t nOffsetSample)
    107         }
    108     }
    109 }
    110+
    111+void ResetTimeData()
    


    MarcoFalke commented at 4:38 pm on January 18, 2021:
    Could rename this to TestOnlyResetTimeData?

    vasild commented at 10:56 am on January 19, 2021:
    Renamed. I would have preferred to surround it in #ifdef UNIT_TESTS_ARE_ENABLED but couldn’t find such a macro.
  39. vasild force-pushed on Jan 19, 2021
  40. vasild commented at 10:57 am on January 19, 2021: member
    d8c6dc0b6…70cd1eaef: address suggestion
  41. DrahtBot added the label Needs rebase on Feb 5, 2021
  42. vasild force-pushed on Feb 10, 2021
  43. vasild force-pushed on Feb 10, 2021
  44. vasild commented at 3:57 pm on February 10, 2021: member
    70cd1eaef…698590c57: rebase due to conflicts
  45. DrahtBot removed the label Needs rebase on Feb 10, 2021
  46. DrahtBot added the label Needs rebase on Feb 19, 2021
  47. vasild force-pushed on Feb 24, 2021
  48. vasild commented at 1:37 pm on February 24, 2021: member
    698590c57...400cbd480: rebase due to conflicts
  49. DrahtBot removed the label Needs rebase on Feb 24, 2021
  50. DrahtBot added the label Needs rebase on Mar 12, 2021
  51. vasild force-pushed on Mar 15, 2021
  52. vasild commented at 3:36 pm on March 15, 2021: member
    400cbd480...b6fb4b928: rebase due to conflicts
  53. DrahtBot removed the label Needs rebase on Mar 15, 2021
  54. jonatack commented at 5:09 pm on March 15, 2021: member
    Concept ACK
  55. DrahtBot added the label Needs rebase on Mar 17, 2021
  56. vasild force-pushed on Mar 17, 2021
  57. vasild commented at 1:28 pm on March 17, 2021: member
    b6fb4b928...0c24971f9: rebase due to conflicts
  58. DrahtBot removed the label Needs rebase on Mar 17, 2021
  59. in src/timedata.cpp:76 in 0c24971f97 outdated
    71@@ -70,26 +72,25 @@ void AddTimeData(const CNetAddr& ip, int64_t nOffsetSample)
    72     // So we should hold off on fixing this and clean it up as part of
    73     // a timing cleanup that strengthens it in a number of other ways.
    74     //
    75-    if (vTimeOffsets.size() >= 5 && vTimeOffsets.size() % 2 == 1) {
    76-        int64_t nMedian = vTimeOffsets.median();
    77-        std::vector<int64_t> vSorted = vTimeOffsets.sorted();
    78+    if (g_time_offsets.size() >= 5 && g_time_offsets.size() % 2 == 1) {
    


    jonatack commented at 8:11 pm on March 17, 2021:
    950c9f6 ~0 on updating these names, as it is done only partially, the header file isn’t updated, nor is nTimeOffset, and there are omissions: 2 occurrences of vTimeOffsets in the comments were not updated (a scripted diff should catch this)

    vasild commented at 11:29 am on March 26, 2021:

    as it is done only partially

    The commit does not claim to rename all variables, just the 3 ones mentioned in the commit message.

    the header file isn’t updated

    What do you think needs to be updated in the header file (I assume timedata.h)? The variables that were renamed are static in timedata.cpp.

    2 occurrences of vTimeOffsets in the comments were not updated

    Updated the comments, thanks!

  60. in test/functional/feature_bind_port_externalip.py:37 in 0c24971f97 outdated
    32+    [['-externalip=2.2.2.2:30011', '-port=30012', '-bind={}:30013'.format(ADDR)], 30011],
    33+    [['-externalip=2.2.2.2:30014',                '-bind={}:30015'.format(ADDR)], 30014],
    34+    [['-externalip=2.2.2.2',       '-port=30016', '-bind={}:30017'.format(ADDR),
    35+                                             '-whitebind={}:30018'.format(ADDR)], 30017],
    36+    [['-externalip=2.2.2.2',       '-port=30019',
    37+                                             '-whitebind={}:30020'.format(ADDR)], 30020],
    


    jonatack commented at 8:25 pm on March 17, 2021:
    39b4a0c can replace all the .format() commands in the 2 new functional test files with the more compact, cleaner f-strings

    vasild commented at 11:45 am on March 26, 2021:
    Done.
  61. in src/test/net_tests.cpp:1049 in 0c24971f97 outdated
    1043+               0,
    1044+               0,
    1045+               CAddress{},
    1046+               std::string{},
    1047+               ConnectionType::OUTBOUND_FULL_RELAY,
    1048+               false};
    


    jonatack commented at 8:41 pm on March 17, 2021:

    39b4a0c309c5f63bd here and 2 other places in the get_local_addr_for_peer_port test above

    0-               0,
    1-               0,
    2+               /* nKeyedNetGroupIn = */ 0,
    3+               /* nLocalHostNonceIn = */ 0,
    4                CAddress{},
    5                std::string{},
    6                ConnectionType::OUTBOUND_FULL_RELAY,
    7-               false};
    8+               /* inbound_onion = */ false};
    

    vasild commented at 12:12 pm on March 26, 2021:
    Done.
  62. in src/test/net_tests.cpp:1061 in 0c24971f97 outdated
    1055+
    1056+    CDataStream recv_stream{msg.data, SER_NETWORK, PROTOCOL_VERSION};
    1057+
    1058+    // Force CChainState::IsInitialBlockDownload() to return false.
    1059+    // Otherwise PushAddress() isn't called by PeerManager::ProcessMessage().
    1060+    TestChainState& chainstate = *(TestChainState*)&m_node.chainman->ActiveChainstate();
    


    jonatack commented at 8:53 pm on March 17, 2021:

    39b4a0c309c5f63bdf0bdbde0d76ee3edce3ac19

    0    TestChainState& chainstate = *static_cast<TestChainState*>(&m_node.chainman->ActiveChainstate());
    

    vasild commented at 12:14 pm on March 26, 2021:
    Done.
  63. in src/net_processing.cpp:2453 in 0c24971f97 outdated
    2464@@ -2465,6 +2465,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
    2465                     LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString());
    2466                     pfrom.PushAddress(addr, insecure_rand);
    2467                 } else if (IsPeerAddrLocalGood(&pfrom)) {
    2468+                    // Override just the address with whatever the peer sees us as.
    2469+                    // Leave the port in addr as it was returned by GetLocalAddress()
    2470+                    // above because this is outbound connection and the peer cannot
    


    jonatack commented at 9:01 pm on March 17, 2021:

    39b4a0c309c5f

    0                    // above, as this is an outbound connection and the peer cannot
    

    vasild commented at 12:15 pm on March 26, 2021:
    Done.
  64. in src/net.cpp:257 in 0c24971f97 outdated
    240+            addrLocal = CAddress(pnode->GetAddrLocal(), addrLocal.nServices);
    241+        } else {
    242+            // For outbound connections assume just the address as seen from
    243+            // the peer and leave the port in `addrLocal` as returned by
    244+            // `GetLocalAddress()` above. The peer has no way to observe our
    245+            // listening port when we have initiated the connection.
    


    jonatack commented at 9:06 pm on March 17, 2021:

    39b4a0c309 a few ideas

     0         if (pnode->IsInboundConn()) {
     1-            // For inbound connections assume both the address and the port
     2+            // For inbound connections, assume both the address and the port
     3             // as seen from the peer.
     4-            addrLocal = CAddress(pnode->GetAddrLocal(), addrLocal.nServices);
     5+            addrLocal = CAddress{pnode->GetAddrLocal(), addrLocal.nServices};
     6         } else {
     7-            // For outbound connections assume just the address as seen from
     8+            // For outbound connections, assume only the address as seen from
     9             // the peer and leave the port in `addrLocal` as returned by
    10             // `GetLocalAddress()` above. The peer has no way to observe our
    11             // listening port when we have initiated the connection.
    

    vasild commented at 12:17 pm on March 26, 2021:
    Done.
  65. in src/net.h:189 in 0c24971f97 outdated
    183@@ -184,7 +184,15 @@ enum class ConnectionType {
    184 
    185 /** Convert ConnectionType enum to a string value */
    186 std::string ConnectionTypeAsString(ConnectionType conn_type);
    187+
    188+/**
    189+ * Lookup IP addresses from all interfaces on the machine and add them to the
    


    jonatack commented at 9:29 pm on March 17, 2021:

    0c24971f nits: “lookup” is a noun, “look up” is the verb; s/for/to/

    0- * Lookup IP addresses from all interfaces on the machine and add them to the
    1- * list of local addresses for self-advertise.
    2+ * Look up IP addresses from all interfaces on the machine and add them to the
    3+ * list of local addresses to self-advertise.
    

    vasild commented at 12:19 pm on March 26, 2021:
    Done.
  66. in test/functional/feature_bind_port_discover.py:47 in 0c24971f97 outdated
    42+            default=False)
    43+
    44+    def skip_test_if_missing_module(self):
    45+        if not self.options.ihave1111and2222:
    46+            raise SkipTest(
    47+                "To run this test make sure that {} and {} (a routable addresses) are assigned "
    


    jonatack commented at 9:42 pm on March 17, 2021:

    0c24971f97d5e6b03aef4911a s/a routable/routable/, or…

    0                "To run this test make sure that routable addresses {} and {} are assigned "
    

    vasild commented at 12:23 pm on March 26, 2021:
    Done.
  67. in test/functional/feature_bind_port_discover.py:6 in 0c24971f97 outdated
    0@@ -0,0 +1,74 @@
    1+#!/usr/bin/env python3
    2+# Copyright (c) 2020 The Bitcoin Core developers
    3+# Distributed under the MIT software license, see the accompanying
    4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
    5+"""
    6+Test that -discover does not add all intefaces' addresses if we listen just on some
    


    jonatack commented at 9:44 pm on March 17, 2021:

    0c24971f97d5e6b03aef4911a1f772b6

    0Test that -discover does not add all interfaces' addresses if we listen on only some of them.
    

    vasild commented at 12:25 pm on March 26, 2021:
    Done.
  68. in test/functional/feature_bind_port_discover.py:15 in 0c24971f97 outdated
    10+from test_framework.util import assert_equal
    11+
    12+# We need to bind to a routable address for this test to exercise the relevant code
    13+# and also must have another routable address on another interface which must not
    14+# be named "lo" or "lo0".
    15+# To set a routable address on the machine use:
    


    jonatack commented at 9:46 pm on March 17, 2021:

    0c24971f97d5e6b0

    0# To set these routable addresses on the machine, use:
    

    vasild commented at 12:26 pm on March 26, 2021:
    Done.
  69. in test/functional/feature_bind_port_discover.py:51 in 0c24971f97 outdated
    46+            raise SkipTest(
    47+                "To run this test make sure that {} and {} (a routable addresses) are assigned "
    48+                "to the interfaces on this machine and rerun with --ihave1111and2222".format(ADDR1, ADDR2))
    49+
    50+    def run_test(self):
    51+        # Check that if no -bind= is given, then all addresses are added to localaddresses.
    


    jonatack commented at 9:50 pm on March 17, 2021:

    0c24971f97d5e6b03a

    0        self.log.info("Test if -bind= is not passed, all addresses are added to localaddresses")
    

    vasild commented at 12:30 pm on March 26, 2021:
    Done.
  70. in test/functional/feature_bind_port_discover.py:64 in 0c24971f97 outdated
    59+                found_addr2 = True
    60+                assert_equal(local['port'], BIND_PORT)
    61+        assert(found_addr1)
    62+        assert(found_addr2)
    63+
    64+        # Check that if -bind= is given, just that address is added to localaddresses.
    


    jonatack commented at 9:50 pm on March 17, 2021:

    0c24971f97d5e6b03a

    0        self.log.info("Test if -bind= is passed, only that address is added to localaddresses")
    

    vasild commented at 12:30 pm on March 26, 2021:
    Done.
  71. jonatack commented at 10:15 pm on March 17, 2021: member

    Approach ACK after going through each commit and running the new tests. Some suggestions, feel free to pick and choose. There are a couple of things I’d like to verify in the net code when it’s not a late hour but globally looks good. I did see a couple of failures in the new functional tests on first run, but they seemed stable after that. (Edit: as these new functional tests will probably only be run manually and not by the CI, it may not be an issue anyway. )

     0$ test/functional/feature_bind_port_discover.py --ihave1111and2222
     12021-03-17T21:48:41.949000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_vh3luuec
     22021-03-17T21:48:42.464000Z TestFramework (ERROR): Assertion failed
     3Traceback (most recent call last):
     4  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 523, in start_nodes
     5    node.wait_for_rpc_connection()
     6  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 224, in wait_for_rpc_connection
     7    raise FailedToStartError(self._node_msg(
     8test_framework.test_node.FailedToStartError: [node 1] bitcoind exited with status 1 during initialization
     9
    10During handling of the above exception, another exception occurred:
    11
    12Traceback (most recent call last):
    13  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 127, in main
    14    self.setup()
    15  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 278, in setup
    16    self.setup_network()
    17  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 372, in setup_network
    18    self.setup_nodes()
    19  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 397, in setup_nodes
    20    self.start_nodes()
    21  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 526, in start_nodes
    22    self.stop_nodes()
    23  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 541, in stop_nodes
    24    node.stop_node(wait=wait, wait_until_stopped=False)
    25  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 323, in stop_node
    26    self.stop(wait=wait)
    27  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 183, in __getattr__
    28    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
    29AssertionError: [node 1] Error: no RPC connection
    302021-03-17T21:48:42.515000Z TestFramework (INFO): Stopping nodes
    31Traceback (most recent call last):
    32  File "/home/jon/projects/bitcoin/bitcoin/test/functional/feature_bind_port_discover.py", line 74, in <module>
    33    BindPortDiscoverTest().main()
    34  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 151, in main
    35    exit_code = self.shutdown()
    36  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 294, in shutdown
    37    self.stop_nodes()
    38  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 541, in stop_nodes
    39    node.stop_node(wait=wait, wait_until_stopped=False)
    40  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 323, in stop_node
    41    self.stop(wait=wait)
    42  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
    43    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
    44  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/authproxy.py", line 144, in __call__
    45    response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
    46  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/authproxy.py", line 107, in _request
    47    self.__conn.request(method, path, postdata, headers)
    48  File "/usr/lib/python3.9/http/client.py", line 1255, in request
    49    self._send_request(method, url, body, headers, encode_chunked)
    50  File "/usr/lib/python3.9/http/client.py", line 1301, in _send_request
    51    self.endheaders(body, encode_chunked=encode_chunked)
    52  File "/usr/lib/python3.9/http/client.py", line 1250, in endheaders
    53    self._send_output(message_body, encode_chunked=encode_chunked)
    54  File "/usr/lib/python3.9/http/client.py", line 1010, in _send_output
    55    self.send(msg)
    56  File "/usr/lib/python3.9/http/client.py", line 950, in send
    57    self.connect()
    58  File "/usr/lib/python3.9/http/client.py", line 921, in connect
    59    self.sock = self._create_connection(
    60  File "/usr/lib/python3.9/socket.py", line 843, in create_connection
    61    raise err
    62  File "/usr/lib/python3.9/socket.py", line 831, in create_connection
    63    sock.connect(sa)
    64ConnectionRefusedError: [Errno 111] Connection refused
    65[node 1] Cleaning up leftover process
    66[node 0] Cleaning up leftover process
    
  72. in src/test/net_tests.cpp:745 in 0c24971f97 outdated
    739+    }
    740 }
    741 
    742+BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port)
    743+{
    744+    // Test that GetLocalAddrForPeer() properly selects the address for self-advertise:
    


    jonatack commented at 6:17 pm on March 18, 2021:
    39b4a0c309c5f63bdf0bdbde0d76ee3edce3ac19 “for self-advertisement” or “to self-advertise”

    vasild commented at 1:11 pm on March 26, 2021:
    Done.
  73. jonatack commented at 6:36 pm on March 18, 2021: member

    Looked at this again today, verified a few additional aspects, and it is really good work.

    ACK 0c24971f97d5e6b03aef4911a1f772b6ccfcd6b7 modulo any of the comments you’d like to take

  74. DrahtBot added the label Needs rebase on Mar 19, 2021
  75. vasild force-pushed on Mar 26, 2021
  76. vasild commented at 9:47 am on March 26, 2021: member
    0c24971f9...4977c0fdb: rebase due to conflicts
  77. DrahtBot removed the label Needs rebase on Mar 26, 2021
  78. vasild force-pushed on Mar 26, 2021
  79. vasild commented at 1:22 pm on March 26, 2021: member
    4977c0fdb...7d5d2416e: address review suggestions
  80. in src/test/net_tests.cpp:799 in 7d5d2416e7 outdated
    740+    }
    741 }
    742 
    743+BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port)
    744+{
    745+    // Test that GetLocalAddrForPeer() properly selects the address to self-advertis:
    


    jonatack commented at 9:37 pm on March 29, 2021:
    0    // Test that GetLocalAddrForPeer() properly selects the address to self-advertise:
    

    vasild commented at 1:09 pm on April 9, 2021:
    Fixed.
  81. in test/functional/feature_bind_port_discover.py:6 in 7d5d2416e7 outdated
    0@@ -0,0 +1,78 @@
    1+#!/usr/bin/env python3
    2+# Copyright (c) 2020-2021 The Bitcoin Core developers
    3+# Distributed under the MIT software license, see the accompanying
    4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
    5+"""
    6+Test that -discover does not add all interfaces' addresses if we listen just on only some of them
    


    jonatack commented at 9:38 pm on March 29, 2021:
    0Test that -discover does not add all interfaces' addresses if we listen on only some of them
    

    vasild commented at 1:12 pm on April 9, 2021:
    Changed.
  82. in test/functional/feature_bind_port_externalip.py:62 in 7d5d2416e7 outdated
    56+            raise SkipTest(
    57+                f"To run this test make sure that {ADDR} (a routable address) is assigned "
    58+                "to one of the interfaces on this machine and rerun with --ihave1111")
    59+
    60+    def run_test(self):
    61+        for i in range(len(EXPECTED)):
    


    jonatack commented at 8:39 am on March 30, 2021:

    would be good to add the missing logging suggested in #20196 (review)

     0     def run_test(self):
     1+        self.log.info("Test the proper port is used for -externalip=")
     2         for i in range(len(EXPECTED)):
     3             if EXPECTED[i][1] == 'default_p2p_port':
     4                 expected_port = p2p_port(i)
     5@@ -69,7 +70,8 @@ class BindPortExternalIPTest(BitcoinTestFramework):
     6                     assert_equal(local['port'], expected_port)
     7                     found = True
     8                     break
     9-            assert(found)
    10+            assert found
    11+
    

    nit, remove brackets after the assert and two lines before the main section)


    vasild commented at 1:18 pm on April 9, 2021:
    Done.
  83. jonatack commented at 8:52 am on March 30, 2021: member

    ACK 7d5d2416e70925ee65bca455d88427335dc12008 modulo some nits below and a test that accepts the “wrong” option:

     0$ test/functional/feature_bind_port_discover.py
     12021-03-30T08:48:27.978000Z TestFramework (WARNING): Test Skipped: To run this test make sure that 1.1.1.1 and 2.2.2.2 (routable addresses) are assigned to the interfaces on this machine and rerun with --ihave1111and2222
     22021-03-30T08:48:28.029000Z TestFramework (INFO): Stopping nodes
     32021-03-30T08:48:28.029000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_gsbqg2e2 on exit
     42021-03-30T08:48:28.029000Z TestFramework (INFO): Test skipped
     5
     6$ test/functional/feature_bind_port_discover.py --ihave1111
     72021-03-30T08:46:34.603000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_b4bp9ak2
     82021-03-30T08:46:34.914000Z TestFramework (INFO): Test that if -bind= is not passed then all addresses are added to localaddresses
     92021-03-30T08:46:34.916000Z TestFramework (INFO): Test that if -bind= is passed then only that address is added to localaddresses
    102021-03-30T08:46:34.967000Z TestFramework (INFO): Stopping nodes
    112021-03-30T08:46:35.124000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_b4bp9ak2 on exit
    122021-03-30T08:46:35.125000Z TestFramework (INFO): Tests successful
    13
    14$ test/functional/feature_bind_port_discover.py --ihave1111and2222
    152021-03-30T08:50:04.718000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test__iqqpdav
    162021-03-30T08:50:05.013000Z TestFramework (INFO): Test that if -bind= is not passed then all addresses are added to localaddresses
    172021-03-30T08:50:05.015000Z TestFramework (INFO): Test that if -bind= is passed then only that address is added to localaddresses
    182021-03-30T08:50:05.070000Z TestFramework (INFO): Stopping nodes
    192021-03-30T08:50:05.245000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test__iqqpdav on exit
    202021-03-30T08:50:05.245000Z TestFramework (INFO): Tests successful
    
  84. DrahtBot added the label Needs rebase on Mar 30, 2021
  85. vasild commented at 12:53 pm on April 9, 2021: member

    …a test that accepts the “wrong” option

    That must be some “feature” in the testing framework - it also accepts --ihave11.

  86. vasild commented at 1:30 pm on April 9, 2021: member
    7d5d2416e...3501180d3: rebase due to conflicts
  87. vasild force-pushed on Apr 9, 2021
  88. vasild force-pushed on Apr 9, 2021
  89. vasild commented at 1:31 pm on April 9, 2021: member
    3501180d3...33b0a79df: address suggestions
  90. DrahtBot removed the label Needs rebase on Apr 9, 2021
  91. in src/net.cpp:139 in 33b0a79df6 outdated
    134+    // (-whitebind= is required to have ":port").
    135+    for (const std::string& whitebind_arg : gArgs.GetArgs("-whitebind")) {
    136+        NetWhitebindPermissions whitebind;
    137+        bilingual_str error;
    138+        if (NetWhitebindPermissions::TryParse(whitebind_arg, whitebind, error)) {
    139+            if (!(whitebind.m_flags & PF_NOBAN)) {
    


    jonatack commented at 4:45 pm on April 9, 2021:

    7d06e0b4c09d0faed2673cf18b51a77201a91222 PF_NOBAN is a multi-flag, so this will return false for both the “noban” and the “download” statuses. If you want it to be false only for noban, use HasFlag()

    0            if (!NetPermissions::HasFlag(whitebind.m_flags, NetPermissionFlags::PF_NOBAN)) {
    

    (and if you want it to be false for both “noban” and “download” the following would be more explicit and simplify the change in #21506)

    0            if (!NetPermissions::HasFlag(whitebind.m_flags, NetPermissionFlags::PF_NOBAN) &&
    1                !NetPermissions::HasFlag(whitebind.m_flags, NetPermissionFlags::PF_DOWNLOAD)) {
    

    To illustrate, these tests pass if added to netbase_tests.cpp in the “netpermissions_test”:

     0    // "noban" implies "download"
     1    BOOST_CHECK(NetWhitebindPermissions::TryParse("noban@1.2.3.4:32", whitebindPermissions, error));
     2    BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_NOBAN);
     3    BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_NOBAN));
     4    BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_DOWNLOAD));
     5    BOOST_CHECK(whitebindPermissions.m_flags & PF_NOBAN);
     6    BOOST_CHECK(whitebindPermissions.m_flags & PF_DOWNLOAD);
     7
     8    // "download" excludes (does not imply) "noban"
     9    BOOST_CHECK(NetWhitebindPermissions::TryParse("download@1.2.3.4:32", whitebindPermissions, error));
    10    BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_DOWNLOAD);
    11    BOOST_CHECK(whitebindPermissions.m_flags != PF_NOBAN);
    12    BOOST_CHECK(!NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_NOBAN));
    13    BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_DOWNLOAD));
    14    BOOST_CHECK(whitebindPermissions.m_flags & PF_NOBAN);
    15    BOOST_CHECK(whitebindPermissions.m_flags & PF_DOWNLOAD);
    

    vasild commented at 4:28 pm on April 13, 2021:

    Good catch!

    Previously, if -whitebind=download@1.1.1.1:8765 was specified we would not have returned 8765 from GetListenPort() but we should have.


    jonatack commented at 6:16 pm on April 13, 2021:

    Yes, I’ve been working on this issue with #21644 that fixes the same issue and #21506 that would eliminate this form of NetPermissionFlags bug.

    Can you please use the NetPermissionFlags:: scope as suggested in #20196 (review)? This is already the case in other places and it would avoid an unnecessary future commit (or rebase of #21506).

  92. jonatack commented at 5:27 pm on April 9, 2021: member
    Code review ACK 33b0a79df62d99baaf1e37f13a9c52b7bbe3955e per git range-diff 4ad83a9 7d5d241 33b0a79 to compare with my last review, modulo one comment below
  93. vasild force-pushed on Apr 13, 2021
  94. vasild commented at 4:29 pm on April 13, 2021: member
    33b0a79df...a5590dcf9: use NetPermissions::HasFlag() to check if PF_NOBAN is set, thanks @jonatack!
  95. jonatack commented at 6:19 pm on April 13, 2021: member

    ACK a5590dcf9dccd82bb0784349d2a3c91dec263380

    Happy to re-ack if you add the NetPermissionFlags:: scope mentioned in #20196 (review).

  96. vasild force-pushed on Apr 14, 2021
  97. vasild commented at 6:42 am on April 14, 2021: member
    a5590dcf9...7126e12fa: use NetPermissionFlags::PF_NOBAN instead of PF_NOBAN
  98. jonatack commented at 8:39 am on April 14, 2021: member
    re-ACK 7126e12fab1eff95efeaab349a49e4f1f714ed4f per git diff a5590dc 7126e12
  99. laanwj referenced this in commit 4da26fb85d on May 19, 2021
  100. jonatack commented at 11:29 am on May 19, 2021: member
    Bump. This could use a review by someone!
  101. vasild force-pushed on May 19, 2021
  102. vasild commented at 11:43 am on May 19, 2021: member
    7126e12fab...9c15f204e5: conflictless rebase to get green CI
  103. vasild force-pushed on May 19, 2021
  104. vasild commented at 12:19 pm on May 19, 2021: member
    9c15f204e5...2519696300: fix silent merge conflict
  105. in src/net.cpp:134 in 2519696300 outdated
    129+                return bind_addr.GetPort();
    130+            }
    131+        }
    132+    }
    133+
    134+    // Otherwise, if -whitebind= without "noban" is provided, use that
    


    jonatack commented at 2:20 pm on May 19, 2021:

    nit, while updating here could be consistent with 7075f604e8d0b21 (and sorry for causing the rebase)

    0   // Otherwise, if -whitebind= without NetPermissionFlags::NoBan permission is provided, use that
    

    vasild commented at 4:30 pm on May 19, 2021:
    Done.
  106. jonatack commented at 2:24 pm on May 19, 2021: member

    ACK 2519696300a356ba2876d37bb01ba620ad46bc0c per git range-diff d4c409cf0 7126e12 2519696

    happy to re-ACK if you update per the comment

  107. vasild force-pushed on May 19, 2021
  108. vasild commented at 4:30 pm on May 19, 2021: member
    2519696300...b962cbc22c: address review suggestion
  109. jonatack commented at 4:41 pm on May 19, 2021: member

    Thanks!

    Code review re-ACK b962cbc22cd35a6f7c5b42a0f3a113c109b4cee2 per git range-diff d4c409c 2519696 b962cbc

  110. DrahtBot added the label Needs rebase on Jul 12, 2021
  111. vasild force-pushed on Jul 20, 2021
  112. vasild commented at 10:36 am on July 20, 2021: member
    b962cbc22c...4ea0a60f75: rebase due to conflicts
  113. DrahtBot removed the label Needs rebase on Jul 20, 2021
  114. jonatack commented at 2:34 pm on July 20, 2021: member

    Build:

    0test/net_tests.cpp:964:34: error: no member named 'vAddrToSend' in 'CNode'
    1    BOOST_CHECK(std::any_of(peer.vAddrToSend.begin(), peer.vAddrToSend.end(),
    2                            ~~~~ ^
    3
    4test/net_tests.cpp:964:60: error: no member named 'vAddrToSend' in 'CNode'
    5    BOOST_CHECK(std::any_of(peer.vAddrToSend.begin(), peer.vAddrToSend.end(),
    6                                                      ~~~~ ^
    72 errors generated.
    
  115. vasild commented at 3:08 pm on July 20, 2021: member
    Yes :sob: after recent changes CNode::vAddrToSend has been moved to Peer::m_addrs_to_send (Peer is defined only in net_processing.cpp, not exposed outside). So this is now nearly untestable. I am considering how to resolve this. Would hate to drop the test.
  116. vasild force-pushed on Jul 22, 2021
  117. vasild commented at 4:43 pm on July 22, 2021: member
    4ea0a60f75...6813052724: rewrite the test net_tests/initial_advertise_from_version_message so that it works with latest master
  118. vasild force-pushed on Jul 23, 2021
  119. vasild commented at 6:11 am on July 23, 2021: member
    6813052724...2aee1b92c5: fix net_tests/get_local_addr_for_peer_port: compare CService instead of CAddress - “time” and “services” (part of CAddress) are irrelevant and should not be compared (they differ).
  120. in src/net.cpp:3147 in 2aee1b92c5 outdated
    3140@@ -3102,3 +3141,9 @@ void CaptureMessage(const CAddress& addr, const std::string& msg_type, const Spa
    3141     ser_writedata32(f, size);
    3142     f.write((const char*)data.data(), data.size());
    3143 }
    3144+
    3145+std::function<void(const CAddress& addr,
    3146+                   const std::string& msg_type,
    3147+                   const Span<const unsigned char>& data,
    


    jonatack commented at 1:27 pm on July 27, 2021:
    c8034d4 suggest passing the Span param here, line 3116 above, in src/net.h and in src/test/net_tests.cpp by value, as these are lightweight, and previous feedback elsewhere suggested the codebase should move toward passing them by value. (Have a look at git grep "Span<unsigned char>")

    vasild commented at 4:04 pm on July 29, 2021:
    Done.
  121. in src/init.cpp:1705 in 2aee1b92c5 outdated
    1701     for (const std::string& bind_arg : args.GetArgs("-bind")) {
    1702         CService bind_addr;
    1703         const size_t index = bind_arg.rfind('=');
    1704         if (index == std::string::npos) {
    1705-            if (Lookup(bind_arg, bind_addr, GetListenPort(), false)) {
    1706+            if (Lookup(bind_arg, bind_addr, default_bind_port, false)) {
    


    jonatack commented at 1:31 pm on July 27, 2021:

    47606db1d721c45cbd053a9a7651b674e2bd4bb1 pico-suggestion while touching this line

    0            if (Lookup(bind_arg, bind_addr, default_bind_port, /* allow_lookup */ false)) {
    

    vasild commented at 4:05 pm on July 29, 2021:
    Done.
  122. in src/timedata.cpp:116 in 2aee1b92c5 outdated
    111+void TestOnlyResetTimeData()
    112+{
    113+    LOCK(g_timeoffset_mutex);
    114+    nTimeOffset = 0;
    115+    g_sources.clear();
    116+    g_time_offsets = CMedianFilter<int64_t>(BITCOIN_TIMEDATA_MAX_SAMPLES, 0);
    


    jonatack commented at 1:35 pm on July 27, 2021:

    1882599

     0@@ -39,7 +39,7 @@ int64_t GetAdjustedTime()
     1 #define BITCOIN_TIMEDATA_MAX_SAMPLES 200
     2 
     3 static std::set<CNetAddr> g_sources;
     4-static CMedianFilter<int64_t> g_time_offsets(BITCOIN_TIMEDATA_MAX_SAMPLES, 0);
     5+static CMedianFilter<int64_t> g_time_offsets{BITCOIN_TIMEDATA_MAX_SAMPLES, 0};
     6 static bool g_warning_emitted;
     7 
     8 void AddTimeData(const CNetAddr& ip, int64_t nOffsetSample)
     9@@ -113,6 +113,6 @@ void TestOnlyResetTimeData()
    10     LOCK(g_timeoffset_mutex);
    11     nTimeOffset = 0;
    12     g_sources.clear();
    13-    g_time_offsets = CMedianFilter<int64_t>(BITCOIN_TIMEDATA_MAX_SAMPLES, 0);
    14+    g_time_offsets = CMedianFilter<int64_t>{BITCOIN_TIMEDATA_MAX_SAMPLES, 0};
    15     g_warning_emitted = false;
    16 }
    

    vasild commented at 4:05 pm on July 29, 2021:
    Done.
  123. in src/net.cpp:252 in 2aee1b92c5 outdated
    240@@ -215,7 +241,17 @@ std::optional<CAddress> GetLocalAddrForPeer(CNode *pnode)
    241     if (IsPeerAddrLocalGood(pnode) && (!addrLocal.IsRoutable() ||
    242          rng.randbits((GetnScore(addrLocal) > LOCAL_MANUAL) ? 3 : 1) == 0))
    243     {
    244-        addrLocal.SetIP(pnode->GetAddrLocal());
    245+        if (pnode->IsInboundConn()) {
    246+            // For inbound connections, assume both the address and the port
    247+            // as seen from the peer.
    248+            addrLocal = CAddress{pnode->GetAddrLocal(), addrLocal.nServices};
    


    jonatack commented at 2:17 pm on July 27, 2021:

    47606db verified that without this change, this assertion of the new test fails

    0test/net_tests.cpp(836): error: in "net_tests/get_local_addr_for_peer_port": check *chosen_local_addr == peer_us has failed
    
  124. in src/net.cpp:139 in 2aee1b92c5 outdated
    129+        if (Lookup(bind_arg, bind_addr, dummy_port, allow_lookup)) {
    130+            if (bind_addr.GetPort() != dummy_port) {
    131+                return bind_addr.GetPort();
    132+            }
    133+        }
    134+    }
    


    jonatack commented at 2:26 pm on July 27, 2021:

    47606db verified that without this new section, the following asserts in this commit fail:

    0test/net_tests.cpp(815): error: in "net_tests/get_local_addr_for_peer_port": check *chosen_local_addr == expected has failed
    1test/net_tests.cpp(1007): error: in "net_tests/initial_advertise_from_version_message": check sent has failed
    

    Avoiding an unneeded declaration seems clearer to me:

     0     // If -bind= is provided with ":port" part, use that (first one if multiple are provided).
     1     for (const std::string& bind_arg : gArgs.GetArgs("-bind")) {
     2-        constexpr uint16_t dummy_port = 0;
     3-        constexpr bool allow_lookup = false;
     4         CService bind_addr;
     5+        constexpr uint16_t dummy_port = 0;
     6 
     7-        if (Lookup(bind_arg, bind_addr, dummy_port, allow_lookup)) {
     8+        if (Lookup(bind_arg, bind_addr, dummy_port, /* allow_lookup */ false)) {
     9             if (bind_addr.GetPort() != dummy_port) {
    10                 return bind_addr.GetPort();
    11             }
    

    vasild commented at 4:05 pm on July 29, 2021:
    Done.
  125. in src/net.cpp:151 in 2aee1b92c5 outdated
    141+        if (NetWhitebindPermissions::TryParse(whitebind_arg, whitebind, error)) {
    142+            if (!NetPermissions::HasFlag(whitebind.m_flags, NetPermissionFlags::NoBan)) {
    143+                return whitebind.m_service.GetPort();
    144+            }
    145+        }
    146+    }
    


    jonatack commented at 2:28 pm on July 27, 2021:

    47606db removing this new section causes no failure in the net_tests added in this commit, but it does raise in the added functional test:

    0File "~/bitcoin/test/functional/feature_bind_port_externalip.py", line 69, in run_test
    1    assert_equal(local['port'], expected_port)
    2    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
    3AssertionError: not(30019 == 30020)
    
  126. jonatack commented at 3:42 pm on July 27, 2021: member
    Almost re-ACK WIP review, quite a few changes and context to swap back into memory since my last review a few months ago. Some checkpoint comments and notes per commit follow, feel free to ignore.
  127. in src/init.cpp:1766 in 2aee1b92c5 outdated
    1746@@ -1745,6 +1747,12 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
    1747         StartTorControl(onion_service_target);
    1748     }
    1749 
    1750+    if (connOptions.bind_on_any) {
    1751+        // Only add all IP addresses of the machine if we would be listening on
    1752+        // any address - 0.0.0.0 (IPv4) and :: (IPv6).
    1753+        Discover();
    


    jonatack commented at 3:53 pm on July 27, 2021:

    2aee1b92 verified that without the change in this commit, the accompanying functional test fails at the expected assertion

    0File "~/bitcoin/test/functional/feature_bind_port_discover.py", line 74, in run_test
    1    assert local['address'] != ADDR2
    2AssertionError
    
  128. vasild force-pushed on Jul 29, 2021
  129. vasild commented at 4:04 pm on July 29, 2021: member
    2aee1b92c5...ad6c1fbe8f: address suggestions
  130. vasild force-pushed on Jul 29, 2021
  131. vasild commented at 4:28 pm on July 29, 2021: member
    ad6c1fbe8f...e4474f66d9: change forgotten argument to pass Span by value
  132. jonatack commented at 4:52 pm on July 29, 2021: member

    ACK e4474f66d91228b8ddcc8ce4fb5ebdb447ecede5 rebased and debug-built to current master to check for silent merge conflicts, reviewed code, removed the code behind each behavior change and verified the relevant test failed where expected, and passed with the code re-inserted

    Possible additional to-do: manually reproduce and verify the issues outlined in #20184.

    This PR fixes a number of issues–nice work!

  133. DrahtBot added the label Needs rebase on Aug 18, 2021
  134. vasild force-pushed on Aug 20, 2021
  135. vasild commented at 11:31 am on August 20, 2021: member
    e4474f66d9...812deb7470: rebase due to conflicts
  136. DrahtBot removed the label Needs rebase on Aug 20, 2021
  137. sipa commented at 9:39 pm on August 20, 2021: member
    utACK 812deb7470d1d60ee6756b2cbec72bb32999312e. Agree with the concept and code changes. I’ve only superficially reviewed the new test.
  138. jonatack commented at 9:50 pm on August 20, 2021: member
    Code review re-ACK 812deb7470d1d60ee6756b2cbec72bb32999312e per git range-diff f8b837 e4474f6 812deb7 rebase-only since my last review
  139. DrahtBot added the label Needs rebase on Sep 27, 2021
  140. vasild force-pushed on Sep 29, 2021
  141. vasild commented at 1:44 pm on September 29, 2021: member

    812deb7470...cb86c1fd20: rebase due to conflicts

    Invalidates ACKs from @sipa, @jonatack.

  142. DrahtBot removed the label Needs rebase on Sep 29, 2021
  143. jonatack approved
  144. jonatack commented at 9:18 pm on September 30, 2021: member
    re-ACK cb86c1fd20715c9a7e95dea1fd2ce6894d7e9ad5 per git range-diff 571bb94 812deb7 cb86c1f :coffee:
  145. theStack commented at 1:35 am on December 20, 2021: member
    Concept ACK
  146. DrahtBot added the label Needs rebase on Jan 20, 2022
  147. laanwj added this to the milestone 23.0 on Feb 10, 2022
  148. jonatack commented at 2:42 pm on February 11, 2022: member
    Tagged for v23, needs rebase.
  149. vasild force-pushed on Feb 11, 2022
  150. vasild commented at 3:34 pm on February 11, 2022: member

    cb86c1fd20...38f49fc11d: rebase due to conflicts (thanks for the poke!)

    Invalidates ACK from @jonatack

    Previously invalidated ACK from @sipa

  151. DrahtBot removed the label Needs rebase on Feb 11, 2022
  152. in src/init.cpp:1707 in 38f49fc11d outdated
    1703     for (const std::string& bind_arg : args.GetArgs("-bind")) {
    1704         CService bind_addr;
    1705         const size_t index = bind_arg.rfind('=');
    1706         if (index == std::string::npos) {
    1707-            if (Lookup(bind_arg, bind_addr, GetListenPort(), false)) {
    1708+            if (Lookup(bind_arg, bind_addr, default_bind_port, /* allow_lookup */ false)) {
    


    jonatack commented at 10:26 pm on February 12, 2022:

    a6d15f47 if you re-push

    0            if (Lookup(bind_arg, bind_addr, default_bind_port, /*allow_lookup=*/false)) {
    

    vasild commented at 8:15 am on February 14, 2022:
    Done.
  153. jonatack commented at 10:28 pm on February 12, 2022: member
    re-ACK 38f49fc11d1d184a6c804d46340e4f91f47901df
  154. vasild force-pushed on Feb 14, 2022
  155. vasild commented at 8:15 am on February 14, 2022: member

    38f49fc11d...35ec977b06: rebase and use /*fAllowLookup=*/

    Invalidates ACK from @jonatack

    Previously invalidated ACK from @sipa

  156. jonatack commented at 9:56 am on February 14, 2022: member
    re-ACK 35ec977b067d05c318b68b4eafbf708870ba9d3f
  157. jonatack commented at 2:58 pm on March 1, 2022: member
    This has been open for a year and half, has seen a fair amount of review, has ACKs by myself and @sipa, and is tagged for 23.0, if anyone else would like to have a look!
  158. DrahtBot added the label Needs rebase on Mar 2, 2022
  159. timedata: make it possible to reset the state
    Add a new function `TestOnlyResetTimeData()` which would reset the
    internal state used by `GetTimeOffset()`, `GetAdjustedTime()` and
    `AddTimeData()`.
    
    This is needed so that unit tests that call `AddTimeData()` can restore
    the state in order not to confuse other tests that rely on it.
    
    Currently `timedata_tests/addtimedata` is the only test that modifies
    the state (via `AddTimeData()`) and also the only test that relies on
    that state.
    60da1eaa11
  160. timedata: rename variables to match the coding style
    Rename the local variables in `src/timedata.cpp`:
    `setKnown` -> `g_sources`
    `vTimeOffsets` -> `g_time_offsets`
    `fDone` -> `g_warning_emitted`
    43868ba416
  161. net: make CaptureMessage() mockable
    Rename `CaptureMessage()` to `CaptureMessageToFile()` and introduce a
    `std::function` variable called `CaptureMessage` whose value can be
    changed by unit tests, should they need to inspect message contents.
    3cb9d9c861
  162. net: pass Span by value to CaptureMessage()
    Span is lightweight and need not be passed by const reference.
    f98cdcb357
  163. net: fix GetListenPort() to derive the proper port
    `GetListenPort()` uses a simple logic: "if `-port=P` is given, then we
    must be listening on `P`, otherwise we must be listening on `8333`".
    This is however not true if `-bind=` has been provided with `:port` part
    or if `-whitebind=` has been provided. Thus, extend `GetListenPort()` to
    return the port from `-bind=` or `-whitebind=`, if any.
    
    Fixes https://github.com/bitcoin/bitcoin/issues/20184 (cases 1. 2. 3. 5.)
    0cfc0cd322
  164. net: only assume all local addresses if listening on any
    If `-bind=` is provided then we would bind only to a particular address
    and should not add all the other addresses of the machine to the list of
    local addresses.
    
    Fixes https://github.com/bitcoin/bitcoin/issues/20184 (case 4.)
    7d64ea4a01
  165. vasild force-pushed on Mar 2, 2022
  166. vasild commented at 2:54 pm on March 2, 2022: member

    35ec977b06...7d64ea4a01: rebase due to conflicts

    Invalidates ACK from @jonatack

    Previously invalidated ACK from @sipa

  167. sipa commented at 3:06 pm on March 2, 2022: member
    utACK 7d64ea4a01920bb55bc6de0de6766712ec792a11. I didn’t review the tests in detail.
  168. DrahtBot removed the label Needs rebase on Mar 2, 2022
  169. jonatack commented at 4:37 pm on March 2, 2022: member
    re-ACK 7d64ea4a01920bb55bc6de0de6766712ec792a11 per git range-diff 08bcfa27 35ec977 7d64ea4, changes are rebase-only, light re-review, re-ran the new tests locally
  170. ghost commented at 4:46 pm on March 2, 2022: none
    Concept ACK
  171. laanwj merged this on Mar 3, 2022
  172. laanwj closed this on Mar 3, 2022

  173. vasild deleted the branch on Mar 3, 2022
  174. sidhujag referenced this in commit 93f6b84cf6 on Mar 3, 2022
  175. DrahtBot locked this on Mar 3, 2023

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: 2024-11-17 09:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me