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

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

fanquake added the label P2P on Oct 20, 2020

MarcoFalke added this to the milestone 0.21.1 on Oct 20, 2020

MarcoFalke removed this from the milestone 0.21.1 on Oct 20, 2020

vasild force-pushed on Oct 20, 2020

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.

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

            local = next(l for l in self.nodes[i].getnetworkinfo()['localaddresses'] if l['address'] == '2.2.2.2')
            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:

for ...
    if ...
        break

that is nearly the same in any programming language.

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

A few micro-optimisations

+LEN_EXPECTED = len(EXPECTED)
 
 class BindPortExternalIPTest(BitcoinTestFramework):
     def set_test_params(self):
         # Avoid any -bind= on the command line. Force the framework to avoid adding -bind=127.0.0.1.
         self.setup_clean_chain = True
         self.bind_to_localhost_only = False
-        self.num_nodes = len(EXPECTED)
+        self.num_nodes = LEN_EXPECTED
         self.extra_args = list(map(lambda e: e[0], EXPECTED))
 
     def add_options(self, parser):
@@ -58,11 +59,13 @@ class BindPortExternalIPTest(BitcoinTestFramework):
                 "to one of the interfaces on this machine and rerun with --ihave1111".format(ADDR))
 
     def run_test(self):
-        for i in range(len(EXPECTED)):
-            if EXPECTED[i][1] == 'default_p2p_port':
+        self.log.info("Test the proper port is used for -externalip=")
+        for i in range(LEN_EXPECTED):
+            port = EXPECTED[i][1]
+            if port == 'default_p2p_port':
                 expected_port = p2p_port(i)
             else:
-                expected_port = EXPECTED[i][1]
+                expected_port = port

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!

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

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:

        static_cast<uint16_t>(args.GetArg("-port", Params().GetDefaultPort()));

vasild commented at 11:22 AM on October 22, 2020:

Changed

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", "");

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

    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.

diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index 9519f3fd0c..7e77b758b2 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -88,7 +88,7 @@ static CDataStream AddrmanToStream(CAddrManSerializationMock& _addrman)
     return CDataStream(vchData, SER_DISK, CLIENT_VERSION);
 }
 
-BOOST_FIXTURE_TEST_SUITE(net_tests, TestingSetup)
+BOOST_FIXTURE_TEST_SUITE(net_tests, RegTestingSetup)
 
 BOOST_AUTO_TEST_CASE(cnode_listen_port)
 {
@@ -879,11 +879,10 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
 
     CDataStream recv_stream{msg.data, SER_NETWORK, PROTOCOL_VERSION};
 
-    // Force CChainState::IsInitialBlockDownload() to return false.
-    const auto nMinimumChainWork_orig = nMinimumChainWork;
-    const auto nMaxTipAge_orig = nMaxTipAge;
-    nMinimumChainWork = 0;
-    nMaxTipAge = GetTime();
+    // Force CChainState::IsInitialBlockDownload() to return false, otherwise PushAddress isn't called
+    SetMockTime(1);
+    BOOST_REQUIRE(!m_node.chainman->ActiveChainstate().IsInitialBlockDownload());
+    SetMockTime(0);
 
     m_node.peerman->InitializeNode(&peer);
 
@@ -898,9 +897,6 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
     BOOST_CHECK(std::any_of(peer.vAddrToSend.begin(), peer.vAddrToSend.end(),
                             [&expected](const CAddress& cur) { return cur == expected; }));
 
-    nMinimumChainWork = nMinimumChainWork_orig;
-    nMaxTipAge = nMaxTipAge_orig;
-
     gArgs.ForceSetArg("-bind", "");
 }

vasild commented at 12:34 PM on October 22, 2020:

Changed, less LOC. Thanks!

MarcoFalke commented at 2:51 PM on October 21, 2020: member

left some style test nits

vasild force-pushed on Oct 22, 2020

vasild commented at 11:25 AM on October 22, 2020: member

Addressed review suggestions and added a test to cover case 4. from #20184.

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

vasild force-pushed on Oct 22, 2020

vasild commented at 12:34 PM on October 22, 2020: member

Addressed review suggestions.

vasild force-pushed on Oct 22, 2020

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:

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

DrahtBot added the label Needs rebase on Dec 16, 2020

vasild force-pushed on Dec 17, 2020

vasild commented at 5:34 PM on December 17, 2020: member

Rebased and changed port numbers used in the functional tests.

DrahtBot removed the label Needs rebase on Dec 17, 2020

DrahtBot added the label Needs rebase on Jan 7, 2021

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

TestChainState& chainstate = *(TestChainState*)&m_node.chainman->ActiveChainstate();
chainstate.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.

vasild force-pushed on Jan 14, 2021

vasild commented at 1:04 PM on January 14, 2021: member

3e55fbfe3...5647ca32e: rebased due to conflicts + address a suggestion.

DrahtBot removed the label Needs rebase on Jan 14, 2021

vasild force-pushed on Jan 17, 2021

vasild commented at 12:33 PM on January 17, 2021: member

5647ca32e...58755690a: rebase, what's up with appveyor?

hebasto commented at 1:13 PM on January 17, 2021: member

@vasild

what's up with appveyor?

What do you mean?

vasild commented at 1:15 PM on January 17, 2021: member

@hebasto https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/37305037

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

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

vasild force-pushed on Jan 18, 2021

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!

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.

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.

vasild force-pushed on Jan 19, 2021

vasild commented at 10:57 AM on January 19, 2021: member

d8c6dc0b6...70cd1eaef: address suggestion

DrahtBot added the label Needs rebase on Feb 5, 2021

vasild force-pushed on Feb 10, 2021

vasild commented at 3:57 PM on February 10, 2021: member

70cd1eaef...698590c57: rebase due to conflicts

DrahtBot removed the label Needs rebase on Feb 10, 2021

DrahtBot added the label Needs rebase on Feb 19, 2021

vasild force-pushed on Feb 24, 2021

vasild commented at 1:37 PM on February 24, 2021: member

698590c57...400cbd480: rebase due to conflicts

DrahtBot removed the label Needs rebase on Feb 24, 2021

DrahtBot added the label Needs rebase on Mar 12, 2021

vasild force-pushed on Mar 15, 2021

vasild commented at 3:36 PM on March 15, 2021: member

400cbd480...b6fb4b928: rebase due to conflicts

DrahtBot removed the label Needs rebase on Mar 15, 2021

jonatack commented at 5:09 PM on March 15, 2021: member

Concept ACK

DrahtBot added the label Needs rebase on Mar 17, 2021

vasild force-pushed on Mar 17, 2021

vasild commented at 1:28 PM on March 17, 2021: member

b6fb4b928...0c24971f9: rebase due to conflicts

DrahtBot removed the label Needs rebase on Mar 17, 2021

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!

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.

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,
+               /* nKeyedNetGroupIn = */ 0,
+               /* nLocalHostNonceIn = */ 0,
                CAddress{},
                std::string{},
                ConnectionType::OUTBOUND_FULL_RELAY,
-               false};
+               /* inbound_onion = */ false};

vasild commented at 12:12 PM on March 26, 2021:

Done.

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

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

vasild commented at 12:14 PM on March 26, 2021:

Done.

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

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

vasild commented at 12:15 PM on March 26, 2021:

Done.

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

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

vasild commented at 12:17 PM on March 26, 2021:

Done.

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/

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

vasild commented at 12:19 PM on March 26, 2021:

Done.

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

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

vasild commented at 12:23 PM on March 26, 2021:

Done.

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

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

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

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

vasild commented at 12:26 PM on March 26, 2021:

Done.

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

        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.

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

        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.

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

<details><summary>one test failure</summary><p>

$ test/functional/feature_bind_port_discover.py --ihave1111and2222
2021-03-17T21:48:41.949000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_vh3luuec
2021-03-17T21:48:42.464000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 523, in start_nodes
    node.wait_for_rpc_connection()
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 224, in wait_for_rpc_connection
    raise FailedToStartError(self._node_msg(
test_framework.test_node.FailedToStartError: [node 1] bitcoind exited with status 1 during initialization

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 127, in main
    self.setup()
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 278, in setup
    self.setup_network()
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 372, in setup_network
    self.setup_nodes()
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 397, in setup_nodes
    self.start_nodes()
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 526, in start_nodes
    self.stop_nodes()
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 541, in stop_nodes
    node.stop_node(wait=wait, wait_until_stopped=False)
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 323, in stop_node
    self.stop(wait=wait)
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 183, in __getattr__
    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
AssertionError: [node 1] Error: no RPC connection
2021-03-17T21:48:42.515000Z TestFramework (INFO): Stopping nodes
Traceback (most recent call last):
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/feature_bind_port_discover.py", line 74, in <module>
    BindPortDiscoverTest().main()
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 151, in main
    exit_code = self.shutdown()
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 294, in shutdown
    self.stop_nodes()
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 541, in stop_nodes
    node.stop_node(wait=wait, wait_until_stopped=False)
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 323, in stop_node
    self.stop(wait=wait)
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/authproxy.py", line 144, in __call__
    response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/authproxy.py", line 107, in _request
    self.__conn.request(method, path, postdata, headers)
  File "/usr/lib/python3.9/http/client.py", line 1255, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/lib/python3.9/http/client.py", line 1301, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.9/http/client.py", line 1250, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.9/http/client.py", line 1010, in _send_output
    self.send(msg)
  File "/usr/lib/python3.9/http/client.py", line 950, in send
    self.connect()
  File "/usr/lib/python3.9/http/client.py", line 921, in connect
    self.sock = self._create_connection(
  File "/usr/lib/python3.9/socket.py", line 843, in create_connection
    raise err
  File "/usr/lib/python3.9/socket.py", line 831, in create_connection
    sock.connect(sa)
ConnectionRefusedError: [Errno 111] Connection refused
[node 1] Cleaning up leftover process
[node 0] Cleaning up leftover process

</p></details>

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.

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

DrahtBot added the label Needs rebase on Mar 19, 2021

vasild force-pushed on Mar 26, 2021

vasild commented at 9:47 AM on March 26, 2021: member

0c24971f9...4977c0fdb: rebase due to conflicts

DrahtBot removed the label Needs rebase on Mar 26, 2021

vasild force-pushed on Mar 26, 2021

vasild commented at 1:22 PM on March 26, 2021: member

4977c0fdb...7d5d2416e: address review suggestions

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:

    // Test that GetLocalAddrForPeer() properly selects the address to self-advertise:

vasild commented at 1:09 PM on April 9, 2021:

Fixed.

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:

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

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)

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

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

vasild commented at 1:18 PM on April 9, 2021:

Done.

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:

$ test/functional/feature_bind_port_discover.py
2021-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
2021-03-30T08:48:28.029000Z TestFramework (INFO): Stopping nodes
2021-03-30T08:48:28.029000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_gsbqg2e2 on exit
2021-03-30T08:48:28.029000Z TestFramework (INFO): Test skipped

$ test/functional/feature_bind_port_discover.py --ihave1111
2021-03-30T08:46:34.603000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_b4bp9ak2
2021-03-30T08:46:34.914000Z TestFramework (INFO): Test that if -bind= is not passed then all addresses are added to localaddresses
2021-03-30T08:46:34.916000Z TestFramework (INFO): Test that if -bind= is passed then only that address is added to localaddresses
2021-03-30T08:46:34.967000Z TestFramework (INFO): Stopping nodes
2021-03-30T08:46:35.124000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_b4bp9ak2 on exit
2021-03-30T08:46:35.125000Z TestFramework (INFO): Tests successful

$ test/functional/feature_bind_port_discover.py --ihave1111and2222
2021-03-30T08:50:04.718000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test__iqqpdav
2021-03-30T08:50:05.013000Z TestFramework (INFO): Test that if -bind= is not passed then all addresses are added to localaddresses
2021-03-30T08:50:05.015000Z TestFramework (INFO): Test that if -bind= is passed then only that address is added to localaddresses
2021-03-30T08:50:05.070000Z TestFramework (INFO): Stopping nodes
2021-03-30T08:50:05.245000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test__iqqpdav on exit
2021-03-30T08:50:05.245000Z TestFramework (INFO): Tests successful

DrahtBot added the label Needs rebase on Mar 30, 2021

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.

vasild commented at 1:30 PM on April 9, 2021: member

7d5d2416e...3501180d3: rebase due to conflicts

vasild force-pushed on Apr 9, 2021

vasild commented at 1:31 PM on April 9, 2021: member

3501180d3...33b0a79df: address suggestions

DrahtBot removed the label Needs rebase on Apr 9, 2021

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()

            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)

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

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

    // "noban" implies "download"
    BOOST_CHECK(NetWhitebindPermissions::TryParse("noban@1.2.3.4:32", whitebindPermissions, error));
    BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_NOBAN);
    BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_NOBAN));
    BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_DOWNLOAD));
    BOOST_CHECK(whitebindPermissions.m_flags & PF_NOBAN);
    BOOST_CHECK(whitebindPermissions.m_flags & PF_DOWNLOAD);

    // "download" excludes (does not imply) "noban"
    BOOST_CHECK(NetWhitebindPermissions::TryParse("download@1.2.3.4:32", whitebindPermissions, error));
    BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_DOWNLOAD);
    BOOST_CHECK(whitebindPermissions.m_flags != PF_NOBAN);
    BOOST_CHECK(!NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_NOBAN));
    BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_DOWNLOAD));
    BOOST_CHECK(whitebindPermissions.m_flags & PF_NOBAN);
    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).

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

vasild force-pushed on Apr 13, 2021

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!

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

vasild force-pushed on Apr 14, 2021

vasild commented at 6:42 AM on April 14, 2021: member

a5590dcf9...7126e12fa: use NetPermissionFlags::PF_NOBAN instead of PF_NOBAN

jonatack commented at 8:39 AM on April 14, 2021: member

re-ACK 7126e12fab1eff95efeaab349a49e4f1f714ed4f per git diff a5590dc 7126e12

laanwj referenced this in commit 4da26fb85d on May 19, 2021

jonatack commented at 11:29 AM on May 19, 2021: member

Bump. This could use a review by someone!

vasild force-pushed on May 19, 2021

vasild commented at 11:43 AM on May 19, 2021: member

7126e12fab...9c15f204e5: conflictless rebase to get green CI

vasild force-pushed on May 19, 2021

vasild commented at 12:19 PM on May 19, 2021: member

9c15f204e5...2519696300: fix silent merge conflict

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)

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

vasild commented at 4:30 PM on May 19, 2021:

Done.

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

vasild force-pushed on May 19, 2021

vasild commented at 4:30 PM on May 19, 2021: member

2519696300...b962cbc22c: address review suggestion

jonatack commented at 4:41 PM on May 19, 2021: member

Thanks!

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

DrahtBot added the label Needs rebase on Jul 12, 2021

vasild force-pushed on Jul 20, 2021

vasild commented at 10:36 AM on July 20, 2021: member

b962cbc22c...4ea0a60f75: rebase due to conflicts

DrahtBot removed the label Needs rebase on Jul 20, 2021

jonatack commented at 2:34 PM on July 20, 2021: member

Build:

test/net_tests.cpp:964:34: error: no member named 'vAddrToSend' in 'CNode'
    BOOST_CHECK(std::any_of(peer.vAddrToSend.begin(), peer.vAddrToSend.end(),
                            ~~~~ ^

test/net_tests.cpp:964:60: error: no member named 'vAddrToSend' in 'CNode'
    BOOST_CHECK(std::any_of(peer.vAddrToSend.begin(), peer.vAddrToSend.end(),
                                                      ~~~~ ^
2 errors generated.

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.

vasild force-pushed on Jul 22, 2021

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

vasild force-pushed on Jul 23, 2021

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

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.

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

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

vasild commented at 4:05 PM on July 29, 2021:

Done.

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

@@ -39,7 +39,7 @@ int64_t GetAdjustedTime()
 #define BITCOIN_TIMEDATA_MAX_SAMPLES 200
 
 static std::set<CNetAddr> g_sources;
-static CMedianFilter<int64_t> g_time_offsets(BITCOIN_TIMEDATA_MAX_SAMPLES, 0);
+static CMedianFilter<int64_t> g_time_offsets{BITCOIN_TIMEDATA_MAX_SAMPLES, 0};
 static bool g_warning_emitted;
 
 void AddTimeData(const CNetAddr& ip, int64_t nOffsetSample)
@@ -113,6 +113,6 @@ void TestOnlyResetTimeData()
     LOCK(g_timeoffset_mutex);
     nTimeOffset = 0;
     g_sources.clear();
-    g_time_offsets = CMedianFilter<int64_t>(BITCOIN_TIMEDATA_MAX_SAMPLES, 0);
+    g_time_offsets = CMedianFilter<int64_t>{BITCOIN_TIMEDATA_MAX_SAMPLES, 0};
     g_warning_emitted = false;
 }

vasild commented at 4:05 PM on July 29, 2021:

Done.

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

test/net_tests.cpp(836): error: in "net_tests/get_local_addr_for_peer_port": check *chosen_local_addr == peer_us has failed

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:

test/net_tests.cpp(815): error: in "net_tests/get_local_addr_for_peer_port": check *chosen_local_addr == expected has failed
test/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:

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

vasild commented at 4:05 PM on July 29, 2021:

Done.

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:

File "~/bitcoin/test/functional/feature_bind_port_externalip.py", line 69, in run_test
    assert_equal(local['port'], expected_port)
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(30019 == 30020)

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.

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

File "~/bitcoin/test/functional/feature_bind_port_discover.py", line 74, in run_test
    assert local['address'] != ADDR2
AssertionError

vasild force-pushed on Jul 29, 2021

vasild commented at 4:04 PM on July 29, 2021: member

2aee1b92c5...ad6c1fbe8f: address suggestions

vasild force-pushed on Jul 29, 2021

vasild commented at 4:28 PM on July 29, 2021: member

ad6c1fbe8f...e4474f66d9: change forgotten argument to pass Span by value

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!

DrahtBot added the label Needs rebase on Aug 18, 2021

vasild force-pushed on Aug 20, 2021

vasild commented at 11:31 AM on August 20, 2021: member

e4474f66d9...812deb7470: rebase due to conflicts

DrahtBot removed the label Needs rebase on Aug 20, 2021

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.

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

DrahtBot added the label Needs rebase on Sep 27, 2021

vasild force-pushed on Sep 29, 2021

vasild commented at 1:44 PM on September 29, 2021: member

812deb7470...cb86c1fd20: rebase due to conflicts

Invalidates ACKs from @sipa, @jonatack.

DrahtBot removed the label Needs rebase on Sep 29, 2021

jonatack approved

jonatack commented at 9:18 PM on September 30, 2021: member

re-ACK cb86c1fd20715c9a7e95dea1fd2ce6894d7e9ad5 per git range-diff 571bb94 812deb7 cb86c1f :coffee:

theStack commented at 1:35 AM on December 20, 2021: member

Concept ACK

DrahtBot added the label Needs rebase on Jan 20, 2022

laanwj added this to the milestone 23.0 on Feb 10, 2022

jonatack commented at 2:42 PM on February 11, 2022: member

Tagged for v23, needs rebase.

vasild force-pushed on Feb 11, 2022

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

DrahtBot removed the label Needs rebase on Feb 11, 2022

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

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

vasild commented at 8:15 AM on February 14, 2022:

Done.

jonatack commented at 10:28 PM on February 12, 2022: member

re-ACK 38f49fc11d1d184a6c804d46340e4f91f47901df

vasild force-pushed on Feb 14, 2022

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

jonatack commented at 9:56 AM on February 14, 2022: member

re-ACK 35ec977b067d05c318b68b4eafbf708870ba9d3f

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!

DrahtBot added the label Needs rebase on Mar 2, 2022

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

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

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

net: pass Span by value to CaptureMessage()

Span is lightweight and need not be passed by const reference.

f98cdcb357

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

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

vasild force-pushed on Mar 2, 2022

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

sipa commented at 3:06 PM on March 2, 2022: member

utACK 7d64ea4a01920bb55bc6de0de6766712ec792a11. I didn't review the tests in detail.

DrahtBot removed the label Needs rebase on Mar 2, 2022

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

ghost commented at 4:46 PM on March 2, 2022: none

Concept ACK

laanwj merged this on Mar 3, 2022

laanwj closed this on Mar 3, 2022

vasild deleted the branch on Mar 3, 2022

sidhujag referenced this in commit 93f6b84cf6 on Mar 3, 2022

DrahtBot locked this on Mar 3, 2023