net: Fix Discover() not running when using -bind=0.0.0.0:port by b-l-u-e · Pull Request #32757 · bitcoin/bitcoin

b-l-u-e · 2025-06-16T04:28:44Z

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.

References

Implementation follows the approach proposed in my review comment on #31492.

Closes #31293

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

DrahtBot · 2025-06-16T04:28:47Z

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

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33362 (Run feature_bind_port_(discover|externalip).py in CI by vasild)

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.

yuvicc · 2025-06-16T15:40:07Z

Concept ACK

Removing duplicate code from DefaultOnionServiceTarget() is better IMO

PeterWrighten

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.

yuvicc

lgtm ACK 776b7ee5855f7572798eff3014030bcc53aa7393

src/init.cpp

vasild

Approach ACK 776b7ee585

src/init.cpp

test/functional/feature_bind_port_discover.py

b-l-u-e · 2025-07-04T06:50:04Z

vasild about the self.extra_args configuration.

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

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

But, 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):

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

With dummy onion bind current implementation:

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

So, I added a dummy onion bind:

['-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 while still testing the intended -whitebind=0.0.0.0 functionality.

DrahtBot · 2025-07-04T06:56:41Z

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

Hints

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.

pinheadmz

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

Show Signature

pinheadmz's public key is on openpgp.org

test/functional/feature_bind_port_discover.py

pinheadmz

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)

test/functional/feature_bind_port_discover.py

vasild

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:

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

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.

test/functional/feature_bind_port_discover.py

vasild

ACK 210dd40

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

test/functional/feature_bind_port_discover.py

vasild · 2025-09-11T07:58:56Z

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 #33362

b-l-u-e · 2025-11-03T19:02:14Z

Are you still working on this?

Yes am still working on this
Caught up on something else

vasild

ACK e0463f3

frankomosh

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.

src/init.cpp

vasild

ACK 69b54df

test/functional/feature_bind_port_discover.py

Signed-off-by: b-l-u-e <[email protected]>

NicolaLS · 2026-02-16T16:24:29Z

tested ACK bb00fd2

Reviewed code changes and tested on macos/arm64 the feature_bind_port_discover.py test fails on master because Discover is never called, with this PR Discover is called when any of whitebind/bind addresses are IsBindAny (0.0.0.0 and ::). The test was improved (documentation/don't connect nodes as chain) and passes now.

I tested that this builds and the test passes I did not manually test this in regtest for example. The test is simple to understand and passes which gives me enough confidence without more testing.

This closes #31293 and prepares for #33362

Question: would it make sense to also test IPv6 :: not only 0.0.0.0 here?

sedited · 2026-03-12T12:09:56Z

This has six stale ACKs. Could previous reviewers take another look here?

DrahtBot added the P2P label Jun 16, 2025

DrahtBot mentioned this pull request Jun 16, 2025

docs: adds correct updated documentation links #32699

Closed

b-l-u-e force-pushed the net-fix-discover-bind-any branch from 5d5bf98 to b17208d Compare June 16, 2025 07:19

b-l-u-e requested a review from maflcko June 16, 2025 08:15

This was referenced Jun 16, 2025

index: store per-block transaction locations for efficient lookups #32541

Closed

Bugfix: Correct first-run free space checks #29678

Open

index: initial sync speedup, parallelize process #26966

Draft

b-l-u-e force-pushed the net-fix-discover-bind-any branch from b17208d to 776b7ee Compare June 16, 2025 16:23

PeterWrighten reviewed Jun 20, 2025

View reviewed changes

yuvicc mentioned this pull request Jun 20, 2025

Execute Discover() when bind=0.0.0.0 or :: is set #31492

Closed

yuvicc reviewed Jun 20, 2025

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

vasild reviewed Jun 30, 2025

View reviewed changes

src/init.cpp Show resolved Hide resolved

test/functional/feature_bind_port_discover.py Show resolved Hide resolved

b-l-u-e force-pushed the net-fix-discover-bind-any branch from 776b7ee to 99f8a6b Compare July 4, 2025 05:45

DrahtBot added the CI failed label Jul 4, 2025

DrahtBot removed the CI failed label Jul 4, 2025

b-l-u-e force-pushed the net-fix-discover-bind-any branch from 4fdc0b1 to ad3f60b Compare July 4, 2025 10:43

b-l-u-e requested a review from vasild July 4, 2025 10:55

pinheadmz approved these changes Jul 15, 2025

View reviewed changes

test/functional/feature_bind_port_discover.py Show resolved Hide resolved

DrahtBot requested review from PeterWrighten and yuvicc July 15, 2025 14:57

b-l-u-e force-pushed the net-fix-discover-bind-any branch from ad3f60b to 894fc0c Compare July 16, 2025 06:11

pinheadmz reviewed Jul 16, 2025

View reviewed changes

test/functional/feature_bind_port_discover.py Outdated Show resolved Hide resolved

b-l-u-e force-pushed the net-fix-discover-bind-any branch 2 times, most recently from 57588f7 to a9b7542 Compare July 17, 2025 00:23

b-l-u-e requested a review from pinheadmz July 17, 2025 10:14

vasild reviewed Jul 22, 2025

View reviewed changes

test/functional/feature_bind_port_discover.py Outdated Show resolved Hide resolved

vasild approved these changes Sep 9, 2025

View reviewed changes

DrahtBot requested a review from pinheadmz September 9, 2025 08:47

vasild reviewed Sep 9, 2025

View reviewed changes

test/functional/feature_bind_port_discover.py Outdated Show resolved Hide resolved

vasild mentioned this pull request Sep 11, 2025

Run feature_bind_port_(discover|externalip).py in CI #33362

Draft

DrahtBot requested a review from mzumsande December 7, 2025 22:06

b-l-u-e force-pushed the net-fix-discover-bind-any branch from 210dd40 to e0463f3 Compare December 7, 2025 22:53

b-l-u-e requested a review from vasild December 7, 2025 22:56

vasild approved these changes Dec 9, 2025

View reviewed changes

frankomosh reviewed Dec 29, 2025

View reviewed changes

src/init.cpp Show resolved Hide resolved

b-l-u-e force-pushed the net-fix-discover-bind-any branch 2 times, most recently from 8c5f2b4 to 37855ae Compare January 5, 2026 19:00

b-l-u-e requested a review from frankomosh January 5, 2026 19:06

DrahtBot added the CI failed label Jan 11, 2026

DrahtBot closed this Jan 19, 2026

DrahtBot reopened this Jan 19, 2026

b-l-u-e force-pushed the net-fix-discover-bind-any branch from 37855ae to 69b54df Compare January 19, 2026 14:13

DrahtBot removed the CI failed label Jan 19, 2026

AnmolM-777 mentioned this pull request Feb 2, 2026

Fix discover bind any v26 #34482

Closed

vasild approved these changes Feb 4, 2026

View reviewed changes

vasild mentioned this pull request Feb 4, 2026

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

Open

NicolaLS reviewed Feb 12, 2026

View reviewed changes

test/functional/feature_bind_port_discover.py Show resolved Hide resolved

b-l-u-e added 3 commits February 15, 2026 00:37

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

b8827ce

Signed-off-by: b-l-u-e <[email protected]>

test: dont connect nodes in feature_bind_port_discover

4f19508

Signed-off-by: b-l-u-e <[email protected]>

test: use dynamic ports and add coverage in feature_bind_port_discover

bb00fd2

Signed-off-by: b-l-u-e <[email protected]>

b-l-u-e force-pushed the net-fix-discover-bind-any branch from 69b54df to bb00fd2 Compare February 14, 2026 21:38

DrahtBot requested a review from vasild February 16, 2026 16:24

Conversation

b-l-u-e commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

yuvicc commented Jun 16, 2025

Uh oh!

PeterWrighten left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuvicc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

b-l-u-e commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 4, 2025

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild commented Sep 11, 2025

Uh oh!

b-l-u-e commented Nov 3, 2025

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

frankomosh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NicolaLS commented Feb 16, 2026

Uh oh!

sedited commented Mar 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

b-l-u-e commented Jun 16, 2025 •

edited

Loading

DrahtBot commented Jun 16, 2025 •

edited

Loading

PeterWrighten left a comment •

edited

Loading

b-l-u-e commented Jul 4, 2025 •

edited

Loading