Discover() will not run if listening on any address with an explicit bind=0.0.0.0

vasild commented at 8:54 am on November 15, 2024: contributor

Current behaviour

https://github.com/bitcoin/bitcoin/blob/85bcfeea23568053ea09013fb8263fa1511d7123/src/init.cpp#L1890-L1892

Discover() will run only if we are listening on all addresses (bind_on_any is true). However if -bind=0.0.0.0:port is explicitly given, then bind_on_any will end up being false and thus Discover() will not run when it should.

Expected behaviour

Discover own addresses even if -bind=0.0.0.0:port is given.

Steps to reproduce

Use -bind=0.0.0.0:port.

How did you obtain Bitcoin Core

Compiled from source

What version of Bitcoin Core are you using?

master@85bcfeea23568053ea09013fb8263fa1511d7123

Operating system and version

Windows 3.11

Background

See #31133 (comment)

sipa commented at 12:37 pm on November 15, 2024: member

Operating system and version

Windows 3.11

Doubt.

andremralves commented at 3:54 pm on November 20, 2024: contributor

Is Discover() working properly? I tried running test_runner.py --ihave1111and2222 feature_bind_port_discover.py and the test didn’t find the 1.1.1.1 and 2.2.2.2 addresses. In the commit where feature-bind_port_discover.py was added, the test works as expected and passes. Actually, I think, now, only ipv6 addresses are being discovered.

vasild commented at 11:47 am on November 25, 2024: contributor

@andremralves good observation! Discover() seems to be ok, but the test has rotten because it is not run regularly in CI (it was clear upfront that this may happen, but still it is better to have it than not, maybe it can be made to be run by CI if we can ifconfig lo:0 1.1.1.1/32 up in the CI?).

At some point the test framework was changed to add some -binds:

https://github.com/bitcoin/bitcoin/blob/2638fdb4f934be96b7c798dbac38ea5ab8a6374a/test/functional/test_framework/test_node.py#L231-L232

and this messes with feature_bind_port_discover.py’s requirement to not have any -bind.

Here is a patch to fix the test:

 0--- i/test/functional/feature_bind_port_discover.py
 1+++ w/test/functional/feature_bind_port_discover.py
 2@@ -24,20 +24,38 @@ ADDR2 = '2.2.2.2'
 3 
 4 BIND_PORT = 31001
 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         self.extra_args = [
12             ['-discover', f'-port={BIND_PORT}'], # bind on any
13             ['-discover', f'-bind={ADDR1}:{BIND_PORT}'],
14         ]
15         self.num_nodes = len(self.extra_args)
16 
17+    def setup_network(self):
18+        """
19+        BitcoinTestFramework.setup_network() without connecting node0 to node1,
20+        we don't need that and avoiding it makes the test faster.
21+        """
22+        self.setup_nodes()
23+
24+    def setup_nodes(self):
25+        """
26+        BitcoinTestFramework.setup_nodes() with overriden has_explicit_bind=True
27+        for each node and without the chain setup.
28+        """
29+        self.add_nodes(self.num_nodes, self.extra_args)
30+        # TestNode.start() will add -bind= to extra_args if has_explicit_bind is
31+        # False. We do not want any -bind= thus set has_explicit_bind to True.
32+        for node in self.nodes:
33+            node.has_explicit_bind = True
34+        self.start_nodes()
35+
36     def add_options(self, parser):
37         parser.add_argument(
38             "--ihave1111and2222", action='store_true', dest="ihave1111and2222",
39             help=f"Run the test, assuming {ADDR1} and {ADDR2} are configured on the machine",
40             default=False)
41

This boils down to the same issue: that Discover() is only ran if args.GetArgs("-bind").empty() && args.GetArgs("-whitebind").empty() is true, but it should also be ran if -bind=0.0.0.0:port is used or even if -bind=0.0.0.0:port -bind=127.0.0.1:torport=onion is used.

So, if that is fixed, then the above fix to the test will not be necessary.

Edit: I guess the logic in init.cpp should be: run Discover() if:

no -bind is used (because then we will bind automatically to 0.0.0.0); or
-bind is used and any of the given -bind options is for 0.0.0.0 or ::.

fanquake commented at 11:56 am on November 25, 2024: member

At some point the test framework was changed to add some -binds:

That was done in #22729.

laanwj added the label P2P on Nov 25, 2024

laanwj added the label good first issue on Nov 25, 2024

evgeniytar commented at 10:36 am on November 26, 2024: none

Applied patch provided by @vasild and test run. But seems like Discover() indeed doesn’t see IPv4. I printed self.nodes[0].getnetworkinfo()['localaddresses'] before first assertion and it shows only IPv6 addresses.

0[{'address': '2a0d:3344:34da:6410::fc5', 'port': 31001, 'score': 1}, {'address': '2a0d:3344:34da:6410:80:af5f:cf76:f4b9', 'port': 31001, 'score': 1}, {'address': '2a0d:3344:34da:6410:8b8:ca87:43b2:565f', 'port': 31001, 'score': 1}]

Second node displays empty array

evgeniytar commented at 9:21 pm on November 26, 2024: none

It looks like Discover() skips loopback interfaces intentionally, it’s done by this line of code

https://github.com/bitcoin/bitcoin/blob/master/src/common/netif.cpp#L290

Once it commented the test passes. Maybe this test needs another routable address setup…

andremralves commented at 7:27 pm on November 27, 2024: contributor

It looks like Discover() skips loopback interfaces intentionally, it’s done by this line of code

https://github.com/bitcoin/bitcoin/blob/master/src/common/netif.cpp#L290

Once it commented the test passes. Maybe this test needs another routable address setup…

Yes, when this test file was created this part of the code was done by using string comparison. Just looking for “lo” and “lo0” patterns. That’s why, in the test, the instruction says to create “lol:0” and “lol:1” interfaces.

I’ve created an issue regarding this subject. #31336

Maybe it’s just a matter of updating the instruction of how to run the tests, changing ifconfig lo:0 1.1.1.1/32 up && ifconfig lo:1 2.2.2.2/32 up for something else. @vasild, do you know what we could use instead?

evgeniytar commented at 10:38 am on December 10, 2024: none

I found a solution to set up routable addresses with bridge on my mac so the tests are passing.

0sudo ifconfig bridge1 create
1sudo ifconfig bridge1 inet 1.1.1.1/24 up
2sudo ifconfig bridge1 inet 2.2.2.2/24 alias

But during preparing a patch I mentioned that args.GetArgs("-bind") is a vector of strings, this means several bindings may be provided, so I wonder what would be a legit condition to set connOptions.bind_on_any into true? Checking that there is at least one occurrence starts with 0.0.0.0 would work or there is better solution?

kind ping @vasild

tnndbtc commented at 4:23 am on December 15, 2024: none

@evgeniytar Thanks for your workaround. Somehow there is still runtime error on my mac (Sequoia 15.1.1 Chip: Apple M1). I have followed the fix on #31492 (thank you, @andremralves ), along with your workaround and made the test run. More details can be found in comments of #31492

I’d like to leave a comment here in case other people on mac hit the same error and would like to find out the solution.

tnndbtc commented at 5:03 am on December 18, 2024: none

@vasild After I reviewed @andremralves ’s PR: https://github.com/bitcoin/bitcoin/pull/31492/files and ran functional test with log trace kept: build/test/functional/test_runner.py –ihave1111and2222 build/test/functional/feature_bind_port_discover.py –tmpdir /tmp/ –nocleanup

The Discover() is invoked for 0.0.0.0:.

The fix is in src/init.cpp: if (bind_addr.has_value()) { connOptions.bind_on_any |= bind_addr.value().IsBindAny(); connOptions.vBinds.push_back(bind_addr.value());

New functional test added in test/functional/feature_bind_port_discover.py: self.bind_to_localhost_only = False self.extra_args = [ [’-discover’, f’-port={BIND_PORT}’], # bind on any [’-discover’, f’-bind={ADDR1}:{BIND_PORT}’], [’-discover’, f’-bind=0.0.0.0:{BIND_PORT+1}’], [’-discover’, f’-bind=[::]:{BIND_PORT+2}’], [’-discover’, f’-bind=::’, f’-port={BIND_PORT+3}’], [’-discover’, f’-bind=0.0.0.0:{BIND_PORT+4}=onion’],

The log trace (/tmp/test_runner_₿_🏃_20241217_234536/feature_bind_port_discover_0/test_framework.log): 2024-12-18T04:45:38.268000Z TestFramework (INFO): Test that if -bind= is not passed then all addresses are added to localaddresses 2024-12-18T04:45:38.268000Z TestFramework (INFO): Test that if -bind= is passed then only that address is added to localaddresses 2024-12-18T04:45:38.269000Z TestFramework (INFO): Test that if -bind=0.0.0.0:port is passed then all addresses are added to localaddresses 2024-12-18T04:45:38.269000Z TestFramework (INFO): Test that if -bind=[::]:port is passed then all addresses are added to localaddresses 2024-12-18T04:45:38.270000Z TestFramework (INFO): Test that if -bind=:: is passed then all addresses are added to localaddresses 2024-12-18T04:45:38.270000Z TestFramework (INFO): Test that if -bind=0.0.0.0:port=onion is passed then all addresses are added to localaddresses

So, #31492 looks like a fix candidate to this issue to me.

CC: @evgeniytar , @i-am-yuvi