net: Fix Discover() not running when using -bind=0.0.0.0:port #32757

pull b-l-u-e wants to merge 2 commits into bitcoin:master from b-l-u-e:net-fix-discover-bind-any changing 2 files +83 −23
  1. b-l-u-e commented at 4:28 am on June 16, 2025: none

    Summary:
    This PR fixes two related issues with address discovery when using explicit bind addresses in Bitcoin Core:

    Problem

    When using -bind=0.0.0.0:port (or -bind=::), the Discover() function was not being executed because the code only checked the bind_on_any flag. This led to two problems:

    • The node would not discover its own local addresses if an explicit “any” bind was used.
    • The functional test feature_bind_port_discover.py would fail, as it expects local addresses to be discovered in these cases.

    The Fix

    This PR:

    1. Checks both bind_on_any and any bind addresses using IsBindAny()
      Ensures Discover() runs when binding to 0.0.0.0 or ::, even if specified explicitly.
    2. Ensures correct address discovery
      The node will now discover its own addresses when using explicit “any” binds, matching user expectations and fixing the test.
    3. Maintains backward compatibility
      The semantic meaning of bind_on_any is preserved as defined in net.h:

      “True if the user did not specify -bind= or -whitebind= and thus we should bind on 0.0.0.0 (IPv4) and :: (IPv6)”

    4. Updates the test to use dynamic ports
      The functional test feature_bind_port_discover.py is updated to use dynamic ports instead of hardcoded ones, improving reliability.

    How this PR Differs from #31492

    • Preserves the semantic meaning of bind_on_any (see net.h).
    • Uses a simpler approach: checks IsBindAny() on bind addresses, without modifying GetListenPort().
    • Avoids code duplication with DefaultOnionServiceTarget().

    References

    • Implementation follows the approach proposed in my [review comment on #31492](/bitcoin-bitcoin/31492/#issuecomment-2941773645).
    • Closes #31293 and #31336.
  2. DrahtBot commented at 4:28 am on June 16, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32757.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK vasild
    Stale ACK PeterWrighten, yuvicc, pinheadmz
    User requested bot ignore mzumsande

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label P2P on Jun 16, 2025
  4. in src/init.cpp:506 in 5d5bf98bdb outdated 
    502@@ -503,7 +503,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
503                    ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
504     argsman.AddArg("-pid=<file>", strprintf("Specify pid file. Relative paths will be prefixed by a net-specific datadir location. (default: %s)", BITCOIN_PID_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
505     argsman.AddArg("-prune=<n>", strprintf("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex. "
506-            "Warning: Reverting this setting requires re-downloading the entire blockchain. "
507+                                           "Warning: Reverting this setting requires re-downloading the entire blockchain. "
    maflcko commented at 6:34 am on June 16, 2025:
    all the whitespace changes look unrelated. Please remove them again. Otherwise, it is unclear what you are really changing in this commit.
  5. b-l-u-e force-pushed on Jun 16, 2025
  6. b-l-u-e requested review from maflcko on Jun 16, 2025
  7. yuvicc commented at 3:40 pm on June 16, 2025: contributor

    Concept ACK

    Removing duplicate code from DefaultOnionServiceTarget() is better IMO

  8. b-l-u-e force-pushed on Jun 16, 2025
  9. PeterWrighten commented at 3:43 am on June 20, 2025: none
    utACK 776b7ee I consider you could report error message when running in original codebase, and in comparison with this PR one. Or you can find related issue and declare to fix them, it could be clearer for reviewers.
  10. in src/init.cpp:2029 in 705fe35719 outdated 
    2023@@ -2024,7 +2024,17 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
2024         StartTorControl(onion_service_target);
2025     }
2026 
2027-    if (connOptions.bind_on_any) {
2028+    bool should_discover = connOptions.bind_on_any;
2029+    if (!should_discover) {
2030+        for (const auto& bind_addr : connOptions.vBinds) {
    yuvicc commented at 10:14 pm on June 20, 2025:
    What about -whitebind here? There was a patch suggestion from vasild here
    yuvicc commented at 10:17 pm on June 20, 2025:
    ahh my bad its already checked with bind_on_any bool.
    vasild commented at 11:04 am on June 30, 2025:

    its already checked with bind_on_any bool.

    I do not think so - the bind_on_any bool will be true if -whitebind and -bind are not given. The current patch will not run Discover() if -whitebind=0.0.0.0 is used but it should. Here is an example patch to fix that:

     0--- i/src/init.cpp
 1+++ w/src/init.cpp
 2@@ -2030,12 +2030,20 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 3             if (bind_addr.IsBindAny()) {
 4                 should_discover = true;
 5                 break;
 6             }
 7         }
 8     }
 9+    if (!should_discover) {
10+        for (const auto& whitebind : connOptions.vWhiteBinds) {
11+            if (whitebind.m_service.IsBindAny()) {
12+                should_discover = true;
13+                break;
14+            }
15+        }
16+    }
17     
18     if (should_discover) {
19         // Only add all IP addresses of the machine if we would be listening on
20         // any address - 0.0.0.0 (IPv4) and :: (IPv6).
21         Discover();
22     }

    you can amend it to 705fe35719 net: Fix Discover() not running when using -bind=0.0.0.0:port

    b-l-u-e commented at 8:50 am on July 1, 2025:
    thank you for the suggestion.
  11. yuvicc commented at 10:25 pm on June 20, 2025: contributor
    lgtm ACK 776b7ee5855f7572798eff3014030bcc53aa7393
  12. in src/init.cpp:2032 in 776b7ee585 outdated 
    2023@@ -2024,7 +2024,17 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
2024         StartTorControl(onion_service_target);
2025     }
2026
    vasild commented at 11:09 am on June 30, 2025:
    Would be nice to keep the length of lines in commit messages shorter.
  13. in test/functional/feature_bind_port_discover.py:36 in 776b7ee585 outdated 
    35         self.bind_to_localhost_only = False
36+        # Get dynamic ports for each node from the test framework
37+        self.bind_port_0 = p2p_port(0)
38+        self.bind_port_1 = p2p_port(1)
39         self.extra_args = [
40-            ['-discover', f'-port={BIND_PORT}'], # bind on any
    vasild commented at 11:12 am on June 30, 2025:

    The testing framework will add -bind=0.0.0.0, we want to test this. But we also want to test if no -bind is given at all. Here is a patch to do that and also to test -whitebind=0.0.0.0:

      0--- i/test/functional/feature_bind_port_discover.py
  1+++ w/test/functional/feature_bind_port_discover.py
  2@@ -26,23 +26,47 @@ from test_framework.util import (
  3 ADDR1 = '1.1.1.1'
  4 ADDR2 = '2.2.2.2'
  5 
  6 class BindPortDiscoverTest(BitcoinTestFramework):
  7     def set_test_params(self):
  8         # Avoid any -bind= on the command line. Force the framework to avoid adding -bind=127.0.0.1.
  9-        self.setup_clean_chain = True
 10         self.bind_to_localhost_only = False
 11         # Get dynamic ports for each node from the test framework
 12-        self.bind_port_0 = p2p_port(0)
 13-        self.bind_port_1 = p2p_port(1)
 14+        self.bind_ports = [
 15+            p2p_port(0),
 16+            p2p_port(2), # node0 will use their port + 1 for onion listen, which is the same as p2p_port(1), so avoid collision
 17+            p2p_port(3),
 18+            p2p_port(4),
 19+        ]
 20         self.extra_args = [
 21-            ['-discover', f'-port={self.bind_port_0}'], # bind on any
 22-            ['-discover', f'-bind={ADDR1}:{self.bind_port_1}'],
 23+            ['-discover', f'-port={self.bind_ports[0]}', '-listen=1'], # Without any -bind
 24+            ['-discover', f'-bind=0.0.0.0:{self.bind_ports[1]}'], # Explicit -bind=0.0.0.0
 25+            ['-discover', f'-whitebind=0.0.0.0:{self.bind_ports[2]}'], # Explicit -whitebind=0.0.0.0
 26+            ['-discover', f'-bind={ADDR1}:{self.bind_ports[3]}'], # Explicit -bind=routable_addr
 27         ]
 28         self.num_nodes = len(self.extra_args)
 29 
 30+    def setup_network(self):
 31+        """
 32+        BitcoinTestFramework.setup_network() without connecting node0 to node1,
 33+        we don't need that and avoiding it makes the test faster.
 34+        """
 35+        self.setup_nodes()
 36+
 37+    def setup_nodes(self):
 38+        """
 39+        BitcoinTestFramework.setup_nodes() with overriden has_explicit_bind=True
 40+        for each node and without the chain setup.
 41+        """
 42+        self.add_nodes(self.num_nodes, self.extra_args)
 43+        # TestNode.start() will add -bind= to extra_args if has_explicit_bind is
 44+        # False. We do not want any -bind= thus set has_explicit_bind to True.
 45+        for node in self.nodes:
 46+            node.has_explicit_bind = True
 47+        self.start_nodes()
 48+
 49     def add_options(self, parser):
 50         parser.add_argument(
 51             "--ihave1111and2222", action='store_true', dest="ihave1111and2222",
 52             help=f"Run the test, assuming {ADDR1} and {ADDR2} are configured on the machine",
 53             default=False)
 54 
 55@@ -51,33 +75,39 @@ class BindPortDiscoverTest(BitcoinTestFramework):
 56             raise SkipTest(
 57                 f"To run this test make sure that {ADDR1} and {ADDR2} (routable addresses) are "
 58                 "assigned to the interfaces on this machine and rerun with --ihave1111and2222")
 59 
 60     def run_test(self):
 61         self.log.info(
 62-                "Test that if -bind= is not passed then all addresses are "
 63+                "Test that if -bind= is not passed or -bind=0.0.0.0 is used then all addresses are "
 64                 "added to localaddresses")
 65-        found_addr1 = False
 66-        found_addr2 = False
 67-        for local in self.nodes[0].getnetworkinfo()['localaddresses']:
 68-            if local['address'] == ADDR1:
 69-                found_addr1 = True
 70-                assert_equal(local['port'], self.bind_port_0)
 71-            if local['address'] == ADDR2:
 72-                found_addr2 = True
 73-                assert_equal(local['port'], self.bind_port_0)
 74-        assert found_addr1
 75-        assert found_addr2
 76+        for i in [0, 1, 2]:
 77+            found_addr1 = False
 78+            found_addr2 = False
 79+            for local in self.nodes[i].getnetworkinfo()['localaddresses']:
 80+                if local['address'] == ADDR1:
 81+                    found_addr1 = True
 82+                    assert_equal(local['port'], self.bind_ports[i])
 83+                if local['address'] == ADDR2:
 84+                    found_addr2 = True
 85+                    assert_equal(local['port'], self.bind_ports[i])
 86+            if not found_addr1:
 87+                self.log.error(f"Address {ADDR1} not found in node{i}'s local addresses: {local}")
 88+                assert False
 89+            if not found_addr2:
 90+                self.log.error(f"Address {ADDR2} not found in node{i}'s local addresses: {local}")
 91+                assert False
 92 
 93         self.log.info(
 94-                "Test that if -bind= is passed then only that address is "
 95+                "Test that if -bind=routable_addr is passed then only that address is "
 96                 "added to localaddresses")
 97         found_addr1 = False
 98-        for local in self.nodes[1].getnetworkinfo()['localaddresses']:
 99+        i = 3
100+        for local in self.nodes[i].getnetworkinfo()['localaddresses']:
101             if local['address'] == ADDR1:
102                 found_addr1 = True
103-                assert_equal(local['port'], self.bind_port_1)
104+                assert_equal(local['port'], self.bind_ports[i])
105             assert_not_equal(local['address'], ADDR2)
106         assert found_addr1
107 
108 if __name__ == '__main__':
109     BindPortDiscoverTest(__file__).main()
    b-l-u-e commented at 8:51 am on July 1, 2025:
    thank you for the suggestion..i will make changes
  14. vasild commented at 11:12 am on June 30, 2025: contributor
    Approach ACK 776b7ee585
  15. b-l-u-e force-pushed on Jul 4, 2025
  16. b-l-u-e commented at 6:50 am on July 4, 2025: none

    @vasild Thanks for the suggested improvements! I have a question about the self.extra_args configuration.

    In your patch, node 2 only has -whitebind=0.0.0.0 without any onion bind:

    0['-discover', f'-whitebind=0.0.0.0:{self.bind_ports[2]}'], # Explicit -whitebind=0.0.0.0

    However, when I tried this configuration, I encountered port conflicts because the test framework automatically adds an onion bind to node 2, which conflicts with the port already used by node 1.

    Test Results:

    Without dummy onion bind (your suggested configuration):

    0Error: Unable to bind to 127.0.0.1:12866 on this computer. Bitcoin Core is probably already running.
1Error: Failed to listen on any port. Use -listen=0 if you want this.

    With dummy onion bind (current implementation):

    0TestFramework (INFO): Tests successful

    The issue is that when using only -whitebind=0.0.0.0, the test framework automatically adds an onion bind to the same node, causing a port conflict with other nodes in the test.

    To avoid this, I added a dummy onion bind:

    0['-discover', f'-whitebind=0.0.0.0:{self.bind_ports[2]}', f'-bind=127.0.0.1:{self.bind_ports[2]+100}=onion']

    This ensures the test runs successfully while still testing the intended -whitebind=0.0.0.0 functionality. Would you like me to explore alternative approaches to avoid this port conflict?

    what do you think?

  17. DrahtBot added the label CI failed on Jul 4, 2025
  18. DrahtBot commented at 6:56 am on July 4, 2025: contributor

    🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/45342616996 LLM reason (✨ experimental): The CI failure is caused by lint errors due to trailing whitespace and missing trailing newline in the code.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  19. DrahtBot removed the label CI failed on Jul 4, 2025
  20. b-l-u-e force-pushed on Jul 4, 2025
  21. b-l-u-e requested review from vasild on Jul 4, 2025
  22. in test/functional/feature_bind_port_discover.py:107 in ad3f60b0ea outdated 
    105+            if not found_addr1:
106+                self.log.error(f"Address {ADDR1} not found in node{i}'s local addresses: {local}")
107+                assert False
108+            if not found_addr2:
109+                self.log.error(f"Address {ADDR2} not found in node{i}'s local addresses: {local}")
110+                assert False
    pinheadmz commented at 2:25 pm on July 15, 2025:
    The variable local in these lines is being used outside the for loop which defines it. If localaddresses is empty [] the error message itself will cause an error.
  23. pinheadmz approved
  24. pinheadmz commented at 2:57 pm on July 15, 2025: member

    ACK ad3f60b0ea59dbbe7aedcfdcca1caddefffe6976

    Built and tested on macos/arm64 and debian/x86. Reviewed code changes which are minimal, this PR searches bind settings for 0.0.0.0 specifically to call Discover() which attempts to add local addresses for peer advertising. Also adds test coverage using 1.1.1.1 and 2.2.2.2 which I was able to set up and check locally on both platforms. Also tested on main.

    I would really like to know if its possible to run this test in CI as vasild suggested here and if so, should probably be added to this PR

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK ad3f60b0ea59dbbe7aedcfdcca1caddefffe6976
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmh2awsACgkQ5+KYS2KJ
 7yTouuhAAmT1tLlOOKdhryZE9K+44Bcsn6prMoLono71Dzf+U+hkQpfxaCcwlI17c
 8hIW51zz/KZWTudgkDLSWU1RMhCDxMk8XFRdh47TD13OhOVrFJF3Al/uCrMFTXOB5
 9mPX2c7v6xOvx6DRlqZlCNDKKTOV1gPIM3+vyScmPA68Vh4ibxwzfYK9Lr0ss92fA
10ceyCZttPOV6v57dazDn2Lm3siGWfS0+urnwf02TIlShklfW6+juz9AWfa8cmHw8E
11GjO0D/r6BNW0TSeHSgt8jpxkVuGxl6opVkn+295hkZ4m9PM44eq7C1avK3yL9exl
12LlsGdk9e/vsesJZne+hnppux1xivbfRbze0Z2J3fOpSdckU8mL8TpJlnyoRbQXAo
13l3Cawmarwjm7sSQBRZsnAHNw6dN8qQ3ICkCPvK86UG8GLXhG76hcQBZY/NmwMqg7
14hl8Np64L3bpH8dHU1Q/9gxHy6h3lPw9GNiOZFaQVvGQZicTzY7WxjhHcAevEHDvu
15h9JxUBUPyroHxQkX2vDVBiyLM5pz8FtUOb7MWj5K0bFE1fg6IuY8XZog4BC/zWUL
16eTLX330l883y2WoUrILAyrSe1B0HSt0SEh5yOqdwaBAuJ3lcvpht50D+0Z3wPvXU
17FShlGSxgvUHBAaDSf/7aG3d48uYkoSKKnek5i65RZIG1oViucUE=
18=snJ5
19-----END PGP SIGNATURE-----

    pinheadmz’s public key is on openpgp.org

  25. DrahtBot requested review from yuvicc on Jul 15, 2025
  26. DrahtBot requested review from PeterWrighten on Jul 15, 2025
  27. b-l-u-e force-pushed on Jul 16, 2025
  28. in test/functional/feature_bind_port_discover.py:97 in 894fc0ce17 outdated 
    90@@ -91,10 +91,10 @@ def run_test(self):
91                     found_addr2 = True
92                     assert_equal(local['port'], self.bind_ports[i])
93             if not found_addr1:
94-                self.log.error(f"Address {ADDR1} not found in node{i}'s local addresses: {local}")
95+                self.log.error(f"Address {ADDR1} not found in node{i}'s local addresses: {self.nodes[i].getnetworkinfo()['localaddresses']}")
96                 assert False
97             if not found_addr2:
98-                self.log.error(f"Address {ADDR2} not found in node{i}'s local addresses: {local}")
99+                self.log.error(f"Address {ADDR2} not found in node{i}'s local addresses: {self.nodes[i].getnetworkinfo()['localaddresses']}")
    pinheadmz commented at 10:23 am on July 16, 2025:
    If you really want to log the RPC output, you could capture it in a variable just before the for local in... and then iterate that variable inside the for loop, and print it in the error message. It’s easy to reproduce this if you need to test it (I had trouble getting 1.1.1.1 and 2.2.2.2 set up on my machine for this test!)
  29. pinheadmz commented at 10:27 am on July 16, 2025: member

    Thanks for fixing the nit in the test.

    In general, nit-fixes should be squashed (not added as an extra commit) so the final commit history is clean and perfect ;-)

    Also I think you rebased on master which is ok, but it makes review slightly harder because my local copy of your branch is now dozens of commits different from the current state of the PR. I used git range-diff to isolate the changes which is why its not like, a super hard rule, but again in general, try not to rebase on master unless you have to (to fix a conflict, include other merged commits, etc)

  30. b-l-u-e force-pushed on Jul 16, 2025
  31. b-l-u-e force-pushed on Jul 17, 2025
  32. b-l-u-e requested review from pinheadmz on Jul 17, 2025
  33. in test/functional/feature_bind_port_discover.py:44 in a9b7542b6e outdated 
    44-            ['-discover', f'-port={BIND_PORT}'], # bind on any
45-            ['-discover', f'-bind={ADDR1}:{BIND_PORT}'],
46+            ['-discover', f'-port={self.bind_ports[0]}', '-listen=1'], # Without any -bind
47+            ['-discover', f'-bind=0.0.0.0:{self.bind_ports[1]}'], # Explicit -bind=0.0.0.0
48+            # Explicit -whitebind=0.0.0.0, add dummy onion bind to avoid port conflict
49+            ['-discover', f'-whitebind=0.0.0.0:{self.bind_ports[2]}', f'-bind=127.0.0.1:{self.bind_ports[2]+100}=onion'],
    vasild commented at 7:29 am on July 22, 2025:

    The testing framework is designed so that it can run parallel executions on the same machine. For example, running two separate tests independently at the same time. To avoid port collisions between such independent executions is has p2p_port(n) and tor_port(n) which allocate ports in a range which (hopefully) will not overlap with the range from another test that executes concurrently (e.g. in another terminal). This +100 seems a odds with this, maybe it could cause collisions.

    I would suggest to use only use p2p_port() or tor_port(). Something like this:

     0--- i/test/functional/feature_bind_port_discover.py
 1+++ w/test/functional/feature_bind_port_discover.py
 2@@ -8,12 +8,13 @@ Test that -discover does not add all interfaces' addresses if we listen on only
 3 
 4 from test_framework.test_framework import BitcoinTestFramework, SkipTest
 5 from test_framework.util import (
 6     assert_equal,
 7     assert_not_equal,
 8     p2p_port,
 9+    tor_port,
10 )
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 these routable addresses on the machine, use:
16@@ -37,14 +38,14 @@ class BindPortDiscoverTest(BitcoinTestFramework):
17             p2p_port(3),
18             p2p_port(4),
19         ]
20         self.extra_args = [
21             ['-discover', f'-port={self.bind_ports[0]}', '-listen=1'], # Without any -bind
22             ['-discover', f'-bind=0.0.0.0:{self.bind_ports[1]}'], # Explicit -bind=0.0.0.0
23-            # Explicit -whitebind=0.0.0.0, add dummy onion bind to avoid port conflict
24-            ['-discover', f'-whitebind=0.0.0.0:{self.bind_ports[2]}', f'-bind=127.0.0.1:{self.bind_ports[2]+100}=onion'],
25+            # Explicit -whitebind=0.0.0.0, add onion bind to avoid port conflict
26+            ['-discover', f'-whitebind=0.0.0.0:{self.bind_ports[2]}', f'-bind=127.0.0.1:{tor_port(3)}=onion'],
27             ['-discover', f'-bind={ADDR1}:{self.bind_ports[3]}'], # Explicit -bind=routable_addr
28         ]
29         self.num_nodes = len(self.extra_args)
30 
31     def setup_network(self):
32         """

    Note: the above patch avoids having nodes[2] bind on the same port for whitebind and tor. Without the above patch:

    0Command-line arg: whitebind="0.0.0.0:11107"
1...
2Bound to 0.0.0.0:11107
3Bound to 127.0.0.1:11107

    with it:

    0Command-line arg: whitebind="0.0.0.0:13642"
1...
2Bound to 0.0.0.0:13642
3Bound to 127.0.0.1:23642

    if you got it conflicting with the port of nodes[1], then that is a different issue.

  34. vasild commented at 7:38 am on July 22, 2025: contributor

    Almost ACK a9b7542b6ea1af770d93cbbf5bb55dae6841334f

    I encountered port conflicts because the test framework automatically adds an onion bind to node 2,

    Hmm, I think it is not the test framework but bitcoind itself that adds the onion bind. To figure out how bitcoind is started, e.g. for nodes[2] I look at /tmp/bitcoin_func_test_.../node2/regtest/debug.log for the lines like:

    0...
1Config file arg: [regtest] shrinkdebugfile="0"
2Config file arg: [regtest] unsafesqlitesync="1"
3Command-line arg: bind="127.0.0.1:23642=onion"
4Command-line arg: datadir="/tmp/bitcoin_func_test_h7qvveos/node2"
5...

    which conflicts with the port already used by node 1

    Are you sure it is nodes[1] and nodes[2] conflicting? What is the error message? What is the output of ./test/functional/combine_logs.py |grep 'Bound to'? I got nodes[2] to bind the whitebind and tor on the same port, see my comment below.

  35. b-l-u-e force-pushed on Jul 22, 2025
  36. b-l-u-e commented at 11:09 am on July 22, 2025: none

    Thanks for the thorough review and the helpful suggestions, @vasild

    You are right. My initial assessment that the test framework was automatically adding an onion bind was incorrect; it is indeed bitcoind itself that handles the onion bind based on configuration.

    Regarding your question about nodes[1] and nodes[2] conflicting, my apologies for the confusion in my previous comment. The primary port conflict I was encountering was not between nodes[1] and nodes[2], but rather, as you correctly identified, within nodes[2] itself. Specifically, the issue arose because bitcoind attempted to bind to 127.0.0.1 on self.bind_ports[2] (due to the whitebind=0.0.0.0 argument), while also potentially trying to establish an implicit bind or an onion bind on the same port for 127.0.0.1. This caused the FailedToStartError: Unable to bind to 127.0.0.1:[port] on this computer.

    Your suggestion to use tor_port(3) to allocate the onion bind port is precisely the correct and most robust solution. I’ve implemented this change in the latest commit. This ensures that the onion bind for 127.0.0.1 uses a distinct, framework-managed port, completely resolving the binding conflict within nodes[2] and aligning with the test framework’s design for parallel execution.

  37. b-l-u-e requested review from vasild on Jul 22, 2025
  38. vasild commented at 4:27 pm on July 22, 2025: contributor
    Better squash the last two commits into one.
  39. b-l-u-e force-pushed on Jul 22, 2025
  40. vasild approved
  41. vasild commented at 4:45 am on July 23, 2025: contributor

    ACK 1dc6b1e8b04363107b4b44cbf42f8a25f2efabfe

    Thank you!

  42. pinheadmz approved
  43. pinheadmz commented at 2:41 pm on July 23, 2025: member

    ACK 1dc6b1e8b04363107b4b44cbf42f8a25f2efabfe

    Built and tested on macos/arm64. Reviewed code, minimal changes since last ACK, addressing clean-ups in the test. Tested on mainnet and compared against master to check that local address was discovered when -bind=0.0.0.0

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 1dc6b1e8b04363107b4b44cbf42f8a25f2efabfe
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmiA9C8ACgkQ5+KYS2KJ
 7yTpmvhAAkvyMzMl7iYnSoSFwv/PJs0zGzXbr+iYzp8IrIoNNS7tTZj5S3a4uc6LA
 8i2aWgAG8NfEcW696R42h6bL2XTIRmLvxbYQyhpZAZRWK9oydK27e2sopsdQFJVxa
 9+wWcRqjhPLt7vOqzp/BkEQwlU60GLEZ7k52/xfs873F9MhU/u37DD2PkcNvf9nAQ
10Y3fNlJYFEA1RBNIBLWTEwkYtKeWS+n2un+nj1APTqHH6rNHRODSYWe/LU2GG2et0
11r2JTPEr0bADMi0P5+K8CZEUSsIcnDEcYc+UmayDEKvQu6kvZHXg0LQuNMHRlo7fM
12uns5ZK5O+e4AVlBdICGRC70+qWL2C7WOd87gOFqsUYYyz1poYgnvEYHb+mHQh9Ti
13juXof2FvxmV6cgdyZJtsAW78poT7Iwkb5Kn3767gum+Y4cGAAOUthVkwxFSZLBuI
14j8T5oNC6AUcKegNhrwYD8uZJta/RNGW0Rzud1MQklE4lvmAwSKPJbDR9yHhuwh7R
15XFaJmjIR7lE+RGEisQ3fO9FI8KmiUZgovHLaRzBITcTDINhtbyakWeanZHgqto8+
16KCsIsoaM/Gk+QCvZifCfdrMxSuTickcCU8XQS53RhTgskAYxluQ4DFGputFi55T7
17rXH1m+1hjgF0CmHF4PtiR8uApS1M8KmEnveepIUNbrrjxnTll6o=
18=2P0/
19-----END PGP SIGNATURE-----

    pinheadmz’s public key is on openpgp.org

  44. mzumsande commented at 9:54 pm on July 23, 2025: contributor

    Does the changed test succeed for others? I tried to follow the instructions and get

    0 test  2025-07-23T21:50:01.787971Z TestFramework (ERROR): Address 1.1.1.1 not found in node0's local addresses: [] 
1 test  2025-07-23T21:50:01.788044Z TestFramework (ERROR): Assertion failed 
2                                   Traceback (most recent call last):
3                                     File "XXX/bitcoin/32757_bind0000/test/functional/test_framework/test_framework.py", line 195, in main
4                                       self.run_test()
5                                     File "XXX/32757_bind0000/build/test/functional/feature_bind_port_discover.py", line 97, in run_test
6                                       assert False

    It also fails on master with the different error assert found_addr1 like #31336 describes. As an aside, ifconfig is deprecated by many linux versions and should probably be replaced by ip (though not necessary to do it here).

    [Edit]: Using Ubuntu 24.04.2 LTS

  45. pinheadmz commented at 9:58 pm on July 23, 2025: member

    Test worked for me on mac and failed as expected without 1.1.1.1 and 2.2.2.2

    On my system the command was ifconfig en0 alias 1.1.1.1

  46. mzumsande commented at 3:58 pm on July 24, 2025: contributor

    ifconfig en0 alias 1.1.1.1

    ok, you don’t add it to the loopback interface. But the instructions for Linux ifconfig lo:0 1.1.1.1/32 up && ifconfig lo:1 2.2.2.2/32 up # to set up do that, while IFF_LOOPBACK addresses are being filtered out here. So I don’t think the problem from #31336 is fixed by this as the OP says.

  47. achow101 commented at 10:36 pm on July 24, 2025: member

    Also seeing @mzumsande’s observation if I add the addresses to loopback, but not if I add them to a different interface.

    I think it’s reasonable to filter out all loopback addresses, so the resolution is probably to just update the instructions to have the addresses bound to a non-loopback interface.

  48. pinheadmz commented at 10:51 pm on July 24, 2025: member
    If @b-l-u-e is going to retouch to update the instructions in the test comment I’d also really like to see those instructions executed in a CI test as well.
  49. mzumsande commented at 10:59 pm on July 24, 2025: contributor
    For me it would also be ok to leave the resolution of #31336 for a separate PR - but this PR shouldn’t claim to solve the issue then.
  50. net: Fix Discover() not running when using -bind=0.0.0.0:port and -whitebind=0.0.0.0 ce4997d692
  51. b-l-u-e force-pushed on Jul 25, 2025
  52. test: Update feature_bind_port_discover.py to use dynamic ports and add comprehensive test coverage 
    test: use tor_port to prevent bind conflicts

test: Update feature_bind_port_discover.py instructions
Replace deprecated ifconfig with modern ip commands and updated
instructions for non-loopback interfaces.
    210dd40611
  53. b-l-u-e force-pushed on Jul 25, 2025
  54. b-l-u-e commented at 5:23 am on July 25, 2025: none

    Thanks for the thorough reviews! I’ve investigated the issues you raised and found the root cause.

    🔍 Root Cause Analysis

    The test was failing because the instructions were wrong. The original instructions told users to use loopback interfaces (lo:0, lo:1), but the code filters out loopback addresses (line 340 in netif.cpp).

    This is exactly what issue #31336 reported - users following the instructions got empty local addresses.

    📝 Reviewer Feedback Addressed

    @mzumsande: You correctly identified that the test was failing with “Address 1.1.1.1 not found in node0’s local addresses: []” and noted that ifconfig is deprecated. You also pointed out that the loopback filtering means this doesn’t actually fix #31336.

    @pinheadmz: You confirmed the test worked on macOS with non-loopback interfaces (ifconfig en0 alias 1.1.1.1) and failed as expected without the addresses.

    @achow101: You confirmed @mzumsande’s observation that loopback addresses don’t work, but non-loopback addresses do work. You suggested updating the instructions to use non-loopback interfaces.

    ✅ Test Results

    I tested both scenarios:

    • ✅ Non-loopback interface (wlo1): Test passes successfully
    • ❌ Loopback interface (lo): Test fails as expected (validates your concerns)

    This confirms the loopback filtering is working as intended.

    �� The Solution

    I’ve updated the test instructions to be more educational and flexible:

    • Modern commands: Uses ip commands for Linux
    • Flexible: Uses placeholder names instead of assuming specific interfaces
    • OS-specific: Provides instructions for Linux, macOS, and FreeBSD
    • Clear: Explains why loopback interfaces don’t work

    Regarding #31336

    This doesn’t fix the core loopback filtering issue in #31336. However, it does fix the immediate problem where users following the test instructions get confused. The test works perfectly when users follow the correct instructions.

    The test logic itself is correct - the problem was just that the instructions were wrong.

  55. Zeegaths commented at 10:14 pm on July 27, 2025: none

    I ran these commands on Ubuntu 22.04:

    sudo ip addr add 1.1.1.1/32 dev wlo1 sudo ip addr add 2.2.2.2/32 dev wlo1

    ./build/bin/bitcoind -regtest -bind=0.0.0.0:18444 -discover -daemon ./build/bin/bitcoin-cli -regtest getnetworkinfo | grep -A 10 “localaddresses”

    Result on master: “localaddresses”: [ ], “warnings”: [ “This is a pre-release test build - use at your own risk - do not use for mining or merchant applications” ]

    Result on PR: “localaddresses”: [ { “address”: “1.1.1.1”, “port”: 18444, “score”: 1 }, { “address”: “2.2.2.2”, “port”: 18444, “score”: 1 }

    Conclusion: PR successfully fixes the issue described in #31293. When using explicit -bind=0.0.0.0, the Discover() function now correctly runs and discovers local addresses, whereas on master it returns empty localaddresses.

  56. vasild approved
  57. vasild commented at 8:47 am on September 9, 2025: contributor

    ACK 210dd4061193f221a05c630ef5819f36dde9b15f

    It suffices to prefix only the first line of the commit message with “test:”.

  58. DrahtBot requested review from pinheadmz on Sep 9, 2025
  59. in test/functional/feature_bind_port_discover.py:19 in 210dd40611 
    14+    tor_port,
15 )
16 
17 # We need to bind to a routable address for this test to exercise the relevant code
18 # and also must have another routable address on another interface which must not
19 # be named "lo" or "lo0".
    vasild commented at 1:11 pm on September 9, 2025:

    If you retouch, this comment is also outdated and needs a tweak, for example:

    0# We need to bind to a routable address for this test to exercise the relevant code
1# and also must have another routable address. Those addresses must be on an interface
2# that is UP and is not a loopback interface (IFF_LOOPBACK).
    b-l-u-e commented at 3:46 pm on September 18, 2025:
    Thanks for the feedback I will make changes
  60. vasild commented at 7:58 am on September 11, 2025: contributor

    I would really like to know if its possible to run this test in CI …

    IFF_LOOPBACK addresses are being filtered out here. So I don’t think the problem from #31336 is fixed by this as the OP says.

    Both resolved in https://github.com/bitcoin/bitcoin/pull/33362

  61. maflcko commented at 1:03 pm on October 29, 2025: member
    Are you still working on this?


b-l-u-e DrahtBot maflcko yuvicc PeterWrighten vasild pinheadmz mzumsande achow101 Zeegaths


pinheadmz maflcko PeterWrighten yuvicc

Labels
P2P

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: 2025-10-31 18:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me