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

andremralves · 2024-12-13T14:32:00Z

If -bind=0.0.0.0 or -bind=:: is specified, it indicates that we can bind to any available address. In this case, the Discover() function should be executed to retrieve the local addresses. This behavior should also work for "=onion" addresses.

I updated the GetListenPort() function to also consider ports from onion address. Without this change, when -bind=0.0.0.0:port=onion is used, the bound port would not match the specified port.

How to test

Prepare the addresses to be found by Discover()

ip link add name lo_dummy type dummy
ip addr add 127.0.1.2/8 dev lo_dummy
ip link set lo_dummy up
ifconfig lo_dummy:0 1.1.1.1/32 up && ifconfig lo_dummy:1 2.2.2.2/32 up

Execute the test

./build/test/functional/test_runner.py --ihave1111and2222 feature_bind_port_discover

Undo the changes after the test execution

ifconfig lo_dummy:0 down && ifconfig lo_dummy:1
ip link set lo_dummy down
ip link delete lo_dummy

Fixes #31293
This PR also fixes the test feature_bind_port_discover.py that was falling.
Fixes #31336

DrahtBot · 2024-12-13T14:32:04Z

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/31492.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	zaidmstrr, yuvicc, ryanofsky

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29500 (test: create assert_not_equal util by kevkevinpal)

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.

tnndbtc · 2024-12-15T04:20:00Z

@andremralves Thanks for putting up this PR so that it helped me run the test on Mac. In addition to your changes, the full setup on mac would require comment made by @evgeniytar in #31293 (comment) , which I quote:

sudo ifconfig bridge1 create
sudo ifconfig bridge1 inet 1.1.1.1/24 up
sudo ifconfig bridge1 inet 2.2.2.2/24 alias

So, the following tweak on lo0 is not required:
sudo ifconfig lo0 alias 1.1.1.1/32
sudo ifconfig lo0 up

Then run:
build/test/functional/test_runner.py --ihave1111and2222 build/test/functional/feature_bind_port_discover.py
Temporary test directory at /var/folders/mw/_h9qr9ws33q1pc_jwk49l3n40000gp/T/test_runner_₿_🏃_20241214_225503
Remaining jobs: [feature_bind_port_discover.py]
1/1 - feature_bind_port_discover.py passed, Duration: 2 s
TEST | STATUS | DURATION
feature_bind_port_discover.py | ✓ Passed | 2 s
ALL | ✓ Passed | 2 s (accumulated)
Runtime: 2 s

sudo ifconfig bridge1 destroy    # to delete bridge1 on mac after test

Otherwise, it will hit error below:
2024-12-15T03:50:03.266000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "<...>/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
self.run_test()
~~~~~~~~~~~~~^^
File "<...>/bitcoin/build/test/functional/feature_bind_port_discover.py", line 90, in run_test
self.verify_addrs(self.nodes[0].getnetworkinfo()['localaddresses'], True, True, BIND_PORT)
~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/tnnd/data/code/bitcoin/build/test/functional/feature_bind_port_discover.py", line 83, in verify_addrs
assert found_addr1 == expected_addr1
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Let me know if a separated PR is preferable so that I can submit one after your PR is in.

yuvicc

Tested ACK a25ce3b22ff3b5771353ab4bcb66a932702bfbd1

Thanks for creating this PR @andremralves.

Log output at master@85bcfeea23568053ea09013fb8263fa1511d7123

Note: to reproduce, use the above code by @andremralves #31492 (comment)

Log output at commit a25ce3b22ff3b5771353ab4bcb66a932702bfbd1

For mac see the comment by @tnndbtc

P.S. I have also checked the following:

if no -bind is used then we are automatically binding 0.0.0.0, execute Discover()
-bind is used with either 0.0.0.0 or ::, execute Discover()

tnndbtc · 2024-12-18T05:07:42Z

@andremralves I also tested your PR on mac and provided comments in #31293 (comment) . This is a pass to me as I validated on Mac and checked workflow by adding more debug traces.
CC: @i-am-yuvi

tnndbtc

The changes are validated on Mac (15.1.1, Apple M1 chipset). Review comment is provided in #31293 (comment). But, still need to fix the merge conflict.

yuvicc · 2024-12-18T16:06:09Z

@andremralves I also tested your PR on mac and provided comments in #31293 (comment) . This is a pass to me as I validated on Mac and checked workflow by adding more debug traces. CC: @i-am-yuvi

Agree with you, works on M2 chip as well!!

andremralves · 2024-12-18T16:53:36Z

The changes are validated on Mac (15.1.1, Apple M1 chipset). Review comment is provided in #31293 (comment). But, still need to fix the merge conflict.

Thank you for the review @tnndbtc and @i-am-yuvi. I will fix the merge conflicts.

zaidmstrr · 2024-12-21T14:14:57Z

src/init.cpp

Hey, I'm reviewing this PR; can you please explain to me the thoughts behind removing this line of code?

As onion_service_target will contain the one and only onion service target in this branch of logic, we don't need to explicitly add it to a list of onion service targets. This is especially true since connOptions.onion_binds is only used to make sure there is only one onion bind; we have already checked that above (line 1906).

Hey, I'm reviewing this PR; can you please explain to me the thoughts behind removing this line of code?

Without this change the test case ./build/src/bitcoind --bind=0.0.0.0:port=onion would fail. Since we are now setting bind_on_any as true when -bind=0.0.0.0, the server would attempt to bind to 0.0.0.0:port two times in CConnman::InitBinds (lines 3272 and 3286). The addition in lines 3279-3296 handles the default bind for onion and fixes the issue.

fanquake · 2025-02-20T22:14:28Z

I will fix the merge conflicts.

Moved to draft. @andremralves are you circling back to fix the conflicts?

andremralves · 2025-02-26T02:53:28Z

Moved to draft. @andremralves are you circling back to fix the conflicts?

Sorry for the delay! I've been a little bit busy lately, but I'll take care of the conflicts now.

DrahtBot · 2025-02-26T03:42:22Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/37827464411}

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.

andremralves · 2025-02-26T07:06:13Z

Fixed the conflicts!

zaidmstrr · 2025-02-26T08:12:06Z

Tested ACK 1561575
After creating the dummy interfaces, I ran the test with args ihave1111and2222 and the test passed successfully.

yuvicc

re-ACK 1561575

Tested all the changes

ryanofsky

Code review ACK 1561575, but would be nice to have feedback from @vasild here since this is supposed to fix #31293.

The changes here all seem to make sense but the interactions between them are complicated. I think this PR would be easier to review if the changes could be broken down as much as possible. It would be nice to add new tests in a first commit to clarify current buggy behaviors. Then it seems like the GetListenPort =onion parsing fix could be a separate commit. Then the refactoring change replacing connOptions.onion_binds.push_back(onion_service_target) with new code in InitBinds could be a another commit. After that the main Discover/bind_on_any part of the change would more clear.

ryanofsky · 2025-03-23T16:24:07Z

src/net.cpp

+        struct in_addr onion_service_target;
+        onion_service_target.s_addr = htonl(INADDR_LOOPBACK);
+        const uint16_t onion_port = GetListenPort() + 1;
+        const CService onion_addr = {onion_service_target, onion_port}; // 127.0.0.1


In commit "net: execute Discover() when bind=0.0.0.0 or :: is set" (c1de112)

Might be missing something, but it seems like this could be simplified:

const CService onion_addr{DefaultOnionServiceTarget(GetListenPort() + 1);

Right. This new code duplicates the code from DefaultOnionServiceTarget():

bitcoin/src/torcontrol.cpp

Lines 726 to 731 in 5b8046a

CService DefaultOnionServiceTarget(uint16_t port)

{

struct in_addr onion_service_target;

onion_service_target.s_addr = htonl(INADDR_LOOPBACK);

return {onion_service_target, port};

}

and the port logic from init.cpp:

bitcoin/src/init.cpp

Line 1926 in 5b8046a

const uint16_t default_bind_port_onion = default_bind_port + 1;

Which is problematic because if this logic has to be changed then it will have to be changed in more than one place.

Before this PR the outcome of DefaultOnionServiceTarget(default_bind_port_onion) was communicated to InitBinds() (here) via the entry in options.onion_binds. Adding that entry however was removed in this PR to facilitate the new condition above: options.bind_on_any && options.vBinds.empty() && options.onion_binds.empty() (onion_binds needs to be empty now).

I think a smaller, less invasive change can replace commit c1de112 net: execute Discover() when bind=0.0.0.0 or :: is set:

[patch] call Discover() also if -bind=0.0.0.0[:port] is configured

--- i/src/init.cpp +++ w/src/init.cpp @@ -1995,13 +1995,20 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) "for the automatically created Tor onion service."), onion_service_target.ToStringAddrPort())); } StartTorControl(onion_service_target); } - if (connOptions.bind_on_any) { + // If -bind=0.0.0.0[:port] or -whitebind=0.0.0.0[:port] was configured. + // Deliberately omit -bind=0.0.0.0[:port]=onion (if configured) because that + // listening address should not be advertised. + const bool some_binds_are_any{ + std::ranges::any_of(connOptions.vBinds, [](const CService& b) { return b.IsBindAny(); }) || + std::ranges::any_of(connOptions.vWhiteBinds, [](const NetWhitebindPermissions& w) { return w.m_service.IsBindAny(); })}; + + if (connOptions.bind_on_any || some_binds_are_any) { // Only add all IP addresses of the machine if we would be listening on // any address - 0.0.0.0 (IPv4) and :: (IPv6). Discover(); } for (const auto& net : args.GetArgs("-whitelist")) {

DrahtBot · 2025-04-01T18:26:47Z

🐙 This pull request conflicts with the target branch and needs rebase.

vasild · 2025-05-02T10:38:36Z

src/net.cpp

+        struct in_addr onion_service_target;
+        onion_service_target.s_addr = htonl(INADDR_LOOPBACK);
+        const uint16_t onion_port = GetListenPort() + 1;
+        const CService onion_addr = {onion_service_target, onion_port}; // 127.0.0.1


Right. This new code duplicates the code from DefaultOnionServiceTarget():

bitcoin/src/torcontrol.cpp

Lines 726 to 731 in 5b8046a

CService DefaultOnionServiceTarget(uint16_t port)

{

struct in_addr onion_service_target;

onion_service_target.s_addr = htonl(INADDR_LOOPBACK);

return {onion_service_target, port};

}

and the port logic from init.cpp:

bitcoin/src/init.cpp

Line 1926 in 5b8046a

const uint16_t default_bind_port_onion = default_bind_port + 1;

Which is problematic because if this logic has to be changed then it will have to be changed in more than one place.

Before this PR the outcome of DefaultOnionServiceTarget(default_bind_port_onion) was communicated to InitBinds() (here) via the entry in options.onion_binds. Adding that entry however was removed in this PR to facilitate the new condition above: options.bind_on_any && options.vBinds.empty() && options.onion_binds.empty() (onion_binds needs to be empty now).

I think a smaller, less invasive change can replace commit c1de112 net: execute Discover() when bind=0.0.0.0 or :: is set:

[patch] call Discover() also if -bind=0.0.0.0[:port] is configured

--- i/src/init.cpp +++ w/src/init.cpp @@ -1995,13 +1995,20 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) "for the automatically created Tor onion service."), onion_service_target.ToStringAddrPort())); } StartTorControl(onion_service_target); } - if (connOptions.bind_on_any) { + // If -bind=0.0.0.0[:port] or -whitebind=0.0.0.0[:port] was configured. + // Deliberately omit -bind=0.0.0.0[:port]=onion (if configured) because that + // listening address should not be advertised. + const bool some_binds_are_any{ + std::ranges::any_of(connOptions.vBinds, [](const CService& b) { return b.IsBindAny(); }) || + std::ranges::any_of(connOptions.vWhiteBinds, [](const NetWhitebindPermissions& w) { return w.m_service.IsBindAny(); })}; + + if (connOptions.bind_on_any || some_binds_are_any) { // Only add all IP addresses of the machine if we would be listening on // any address - 0.0.0.0 (IPv4) and :: (IPv6). Discover(); } for (const auto& net : args.GetArgs("-whitelist")) {

vasild · 2025-05-02T11:45:29Z

src/net.cpp

    // If -bind= is provided with ":port" part, use that (first one if multiple are provided).
    for (const std::string& bind_arg : gArgs.GetArgs("-bind")) {
        constexpr uint16_t dummy_port = 0;
+        // maybe bind_arg has "=onion"
+        const std::string truncated_bind_arg = bind_arg.substr(0, bind_arg.rfind('='));

-        const std::optional<CService> bind_addr{Lookup(bind_arg, dummy_port, /*fAllowLookup=*/false)};
+        const std::optional<CService> bind_addr{Lookup(truncated_bind_arg, dummy_port, /*fAllowLookup=*/false)};
        if (bind_addr.has_value() && bind_addr->GetPort() != dummy_port) return bind_addr->GetPort();
    }


I think this change to GetListenPort() will produce unwanted results if the configuration goes like:
-bind=1.1.1.1:1111=onion -bind=2.2.2.2:2222. In that case, I guess it should return 2222 like before this change, but after it, it would return 1111.

vasild · 2025-05-02T11:48:14Z

src/init.cpp

        if (index == std::string::npos) {
            bind_addr = Lookup(bind_arg, default_bind_port, /*fAllowLookup=*/false);
            if (bind_addr.has_value()) {
+                connOptions.bind_on_any |= bind_addr.value().IsBindAny();


If the semantic of bind_on_any is to be changed then its comment deserves an update:

bitcoin/src/net.h

Lines 1072 to 1074 in 5b8046a

/// True if the user did not specify -bind= or -whitebind= and thus

/// we should bind on `0.0.0.0` (IPv4) and `::` (IPv6).

bool bind_on_any;

b-l-u-e · 2025-06-04T22:29:19Z

I've been working on this same issue and my findings align well with @vasild feedback for a simpler approach.

Regarding the core fix: @vasild suggested approach is exactly right - the fix can be much simpler without the code duplication issues. Here's what I implemented successfully:
bitcoin/src/init.cpp

// We should discover addresses in two cases:
// 1. When no -bind or -whitebind is specified (bind_on_any is true)  
// 2. When any -bind address is 0.0.0.0 or ::
bool should_discover = connOptions.bind_on_any;
if (!should_discover) {
    for (const auto& bind_addr : connOptions.vBinds) {
        if (bind_addr.IsBindAny()) {
            should_discover = true;
            break;
        }
    }
}
if (should_discover) {
    Discover();
}

This avoids:

Modifying GetListenPort()
Code duplication with DefaultOnionServiceTarget()
Complex changes to bind_on_any semantics

Regarding the test fix: I also resolved the feature_bind_port_discover.py test failure. The issue was that the test framework overrides port settings, so the test expected port 31001 but nodes were actually using different ports (14251, 14252). The fix is to use the actual port from the node's network info rather than expecting a hardcoded port.

The test now passes successfully with this approach. Happy to share the complete working implementation if helpful.

vasild · 2025-06-12T08:09:47Z

@andremralves, what do you think about the above feedback? This PR is in "needs rebase" for about 2 months now, do you plan to work on it further?

@b-l-u-e, if there is no activity here for some time, then at some point it would make sense to open a separate PR with your patch, up to your judgement.

yuvicc · 2025-06-12T14:08:53Z

// We should discover addresses in two cases:
// 1. When no -bind or -whitebind is specified (bind_on_any is true)  
// 2. When any -bind address is 0.0.0.0 or ::
bool should_discover = connOptions.bind_on_any;
if (!should_discover) {
    for (const auto& bind_addr : connOptions.vBinds) {
        if (bind_addr.IsBindAny()) {
            should_discover = true;
            break;
        }
    }
}

ACK on the approach, if there's no activity open a separate pr as suggested by @vasild !

yuvicc · 2025-06-20T21:10:20Z

Summary

This PR can be closed as its taken up by #32757 with more better approach.

maflcko · 2025-06-21T08:50:22Z

Closing for now, due to inactivity. Please leave a comment, if you want this reopened and want to work on this again. You may also create a fresh pull request, referencing this one.

andremralves closed this Dec 13, 2024

andremralves reopened this Dec 13, 2024

DrahtBot added the Needs rebase label Dec 14, 2024

This was referenced Dec 14, 2024

scripted-diff: Type-safe settings retrieval #31260

Open

net, init: derive default onion port if a user specified a -port #31223

Merged

tnndbtc mentioned this pull request Dec 15, 2024

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

Open

yuvicc mentioned this pull request Dec 16, 2024

Review Club: Execute Discover() when bind=0.0.0.0 or :: is set Bitshala/BitcoinCore-PR-Review-Club#94

Closed

yuvicc approved these changes Dec 16, 2024

View reviewed changes

tnndbtc approved these changes Dec 18, 2024

View reviewed changes

zaidmstrr reviewed Jan 1, 2025

View reviewed changes

fanquake marked this pull request as draft February 20, 2025 22:14

andremralves force-pushed the discover-bind branch from db30b9c to 8f753ec Compare February 26, 2025 03:28

DrahtBot added the CI failed label Feb 26, 2025

andremralves force-pushed the discover-bind branch from 8f753ec to 0db64fe Compare February 26, 2025 04:24

DrahtBot removed the Needs rebase label Feb 26, 2025

andremralves force-pushed the discover-bind branch from 0db64fe to 9626c04 Compare February 26, 2025 06:06

andremralves added 2 commits February 26, 2025 04:01

net: execute Discover() when bind=0.0.0.0 or :: is set

c1de112

Fix feature_bind_port_discover.py and add new test cases

1561575

andremralves force-pushed the discover-bind branch from 9626c04 to 1561575 Compare February 26, 2025 07:01

andremralves marked this pull request as ready for review February 26, 2025 07:06

DrahtBot removed the CI failed label Feb 26, 2025

DrahtBot requested a review from yuvicc February 26, 2025 08:12

yuvicc reviewed Feb 26, 2025

View reviewed changes

DrahtBot mentioned this pull request Feb 26, 2025

test: create assert_not_equal util #29500

Merged

ryanofsky approved these changes Mar 23, 2025

View reviewed changes

DrahtBot added the Needs rebase label Apr 1, 2025

vasild reviewed May 2, 2025

View reviewed changes

b-l-u-e mentioned this pull request Jun 16, 2025

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

Open

maflcko closed this Jun 21, 2025

	CService DefaultOnionServiceTarget(uint16_t port)
	{
	struct in_addr onion_service_target;
	onion_service_target.s_addr = htonl(INADDR_LOOPBACK);
	return {onion_service_target, port};
	}

	/// True if the user did not specify -bind= or -whitebind= and thus
	/// we should bind on `0.0.0.0` (IPv4) and `::` (IPv6).
	bool bind_on_any;

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

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

Uh oh!

Conversation

andremralves commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to test

Uh oh!

DrahtBot commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

tnndbtc commented Dec 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuvicc 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

Log output at master@85bcfeea23568053ea09013fb8263fa1511d7123

Log output at commit a25ce3b22ff3b5771353ab4bcb66a932702bfbd1

Uh oh!

tnndbtc commented Dec 18, 2024

Uh oh!

tnndbtc 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 commented Dec 18, 2024

Uh oh!

andremralves commented Dec 18, 2024

Uh oh!

zaidmstrr Dec 21, 2024

Choose a reason for hiding this comment

Uh oh!

SBNovaScript Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

andremralves Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

fanquake commented Feb 20, 2025

Uh oh!

andremralves commented Feb 26, 2025

Uh oh!

DrahtBot commented Feb 26, 2025

Uh oh!

andremralves commented Feb 26, 2025

Uh oh!

zaidmstrr commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuvicc left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 23, 2025 • 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!

vasild May 2, 2025

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Apr 1, 2025

Uh oh!

vasild May 2, 2025

Choose a reason for hiding this comment

Uh oh!

vasild May 2, 2025

Choose a reason for hiding this comment

Uh oh!

vasild May 2, 2025

Choose a reason for hiding this comment

Uh oh!

b-l-u-e commented Jun 4, 2025

Uh oh!

vasild commented Jun 12, 2025

andremralves commented Dec 13, 2024 •

edited

Loading

DrahtBot commented Dec 13, 2024 •

edited

Loading

tnndbtc commented Dec 15, 2024 •

edited

Loading

yuvicc left a comment •

edited

Loading

tnndbtc left a comment •

edited

Loading

zaidmstrr commented Feb 26, 2025 •

edited

Loading

ryanofsky Mar 23, 2025 •

edited

Loading

yuvicc commented Jun 12, 2025 •

edited

Loading

yuvicc commented Jun 20, 2025 •

edited

Loading