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?