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

b-l-u-e commented at 4:28 am on June 16, 2025: none

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

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.

This PR:

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.
Ensures correct address discovery The node will now discover its own addresses when using explicit “any” binds, matching user expectations and fixing the test.
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)”
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

The #31336 has the overall test failure and requires both this PR and #33362 to be fully resolved.

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
Stale ACK	PeterWrighten, yuvicc, pinheadmz, mzumsande, vasild, frankomosh

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

DrahtBot added the label P2P on Jun 16, 2025

b-l-u-e force-pushed on Jun 16, 2025

b-l-u-e requested review from maflcko on Jun 16, 2025

yuvicc commented at 3:40 pm on June 16, 2025: contributor

Concept ACK

Removing duplicate code from DefaultOnionServiceTarget() is better IMO

b-l-u-e force-pushed on Jun 16, 2025

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.

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.

yuvicc commented at 10:25 pm on June 20, 2025: contributor

lgtm ACK 776b7ee5855f7572798eff3014030bcc53aa7393

in src/init.cpp:2130 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.

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

vasild commented at 11:12 am on June 30, 2025: contributor

Approach ACK 776b7ee585

b-l-u-e force-pushed on Jul 4, 2025

b-l-u-e commented at 6:50 am on July 4, 2025: none

vasild thanks for the suggestions…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.

DrahtBot added the label CI failed on Jul 4, 2025

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.

DrahtBot removed the label CI failed on Jul 4, 2025

b-l-u-e force-pushed on Jul 4, 2025

b-l-u-e requested review from vasild on Jul 4, 2025

in test/functional/feature_bind_port_discover.py:106 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.

pinheadmz approved

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

DrahtBot requested review from yuvicc on Jul 15, 2025

DrahtBot requested review from PeterWrighten on Jul 15, 2025

b-l-u-e force-pushed on Jul 16, 2025

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

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)

b-l-u-e force-pushed on Jul 16, 2025

b-l-u-e force-pushed on Jul 17, 2025

b-l-u-e requested review from pinheadmz on Jul 17, 2025

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.

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.

b-l-u-e force-pushed on Jul 22, 2025

b-l-u-e commented at 11:09 am on July 22, 2025: none

My initial assessment that the test framework was automatically adding an onion bind was incorrect… it is bitcoind itself that handles the onion bind based on configuration.

about nodes[1] and nodes[2] conflicting, sorry for the confusion in my previous comment. The primary port conflict I was encountering was not between nodes[1] and nodes[2], but rather within nodes[2] itself.

the issue was raised 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.

suggestion to use tor_port(3) to allocate the onion bind port is implemented in the latest commit. the onion bind for 127.0.0.1 uses a distinct, framework-managed port, resolves the binding conflict within nodes[2] and aligning with the test framework’s design for parallel execution.

b-l-u-e requested review from vasild on Jul 22, 2025

vasild commented at 4:27 pm on July 22, 2025: contributor

Better squash the last two commits into one.

b-l-u-e force-pushed on Jul 22, 2025

vasild approved

vasild commented at 4:45 am on July 23, 2025: contributor

ACK 1dc6b1e8b04363107b4b44cbf42f8a25f2efabfe

Thank you!

pinheadmz approved

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

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

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

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.

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.

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.

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.

b-l-u-e force-pushed on Jul 25, 2025

b-l-u-e commented at 5:23 am on July 25, 2025: none

found the issues raised and found the cause.

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 what issue #31336 reported - users following the instructions got empty local addresses.

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

pinheadmz: 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: confirmed mzumsande observation that loopback addresses don’t work, but non-loopback addresses do work and suggested updating the instructions to use non-loopback interfaces.

I tested both scenarios

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

This makes the loopback filtering working as intended.

updated the test instructions to be more educational and flexible

Uses ip commands for Linux
Uses placeholder names instead of assuming specific interfaces
Provides instructions for Linux, macOS, and FreeBSD
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 jthe instructions were wrong.

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.

vasild approved

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

DrahtBot requested review from pinheadmz on Sep 9, 2025

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

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

b-l-u-e commented at 7:02 pm on November 3, 2025: none

Are you still working on this?

Yes am still working on this Caught up on something else

DrahtBot requested review from mzumsande on Dec 7, 2025

b-l-u-e force-pushed on Dec 7, 2025

b-l-u-e requested review from vasild on Dec 7, 2025

vasild approved

vasild commented at 2:54 pm on December 9, 2025: contributor

ACK e0463f38f15c2bf51b6fd99da369c2ad5271f13e

in src/init.cpp:2132 in e0463f38f1 outdated

2122@@ -2123,7 +2123,26 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
2123         StartTorControl(onion_service_target);
2124     }
2125 
2126-    if (connOptions.bind_on_any) {
2127+    bool should_discover = connOptions.bind_on_any;
2128+    if (!should_discover) {

frankomosh commented at 7:42 am on December 29, 2025:

tiny nit(feel free to ignore)

These nested checks could be flattened for readability. I think eliminating the double-negative (!should_discover) pattern would make the logic more declarative?

b-l-u-e commented at 10:18 am on December 29, 2025:

thank you for the feedback

frankomosh commented at 7:48 am on December 29, 2025: contributor

Code Review ACK e0463f3

Maybe you could clarify the PR description to reflect that it closes #31293, and #31336 would only be fully resolved with #33362. I think this would accurately reflect that #31336 requires both PRs.

b-l-u-e force-pushed on Jan 5, 2026

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

Signed-off-by: b-l-u-e <winnie.gitau282@gmail.com>

2577ab0d48

test: Update feature_bind_port_discover.py to use dynamic ports and add comprehensive test coverage

Signed-off-by: b-l-u-e <winnie.gitau282@gmail.com>

37855aee7d

b-l-u-e force-pushed on Jan 5, 2026

b-l-u-e requested review from frankomosh on Jan 5, 2026

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

Code Coverage & Benchmarks

Reviews

Conflicts