Execute Discover() when bind=0.0.0.0 or

andremralves commented at 2:32 pm on December 13, 2024: contributor

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

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

Execute the test

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

Undo the changes after the test execution

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

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

DrahtBot commented at 2:32 pm on December 13, 2024: 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/31492.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	zaidmstrr, i-am-yuvi, 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.

andremralves closed this on Dec 13, 2024

andremralves reopened this on Dec 13, 2024

DrahtBot added the label Needs rebase on Dec 14, 2024

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

@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.

i-am-yuvi approved

i-am-yuvi commented at 6:01 pm on December 16, 2024: contributor

Tested ACK a25ce3b22ff3b5771353ab4bcb66a932702bfbd1

Thanks for creating this PR @andremralves.

Log output at master@85bcfeea23568053ea09013fb8263fa1511d7123

Screenshot from 2024-12-15 16-09-33

Note: to reproduce, use the above code by @andremralves #31492#issue-2738533124

Log output at commit a25ce3b22ff3b5771353ab4bcb66a932702bfbd1

Screenshot from 2024-12-15 15-39-14

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 commented at 5:07 am on December 18, 2024: none

@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 approved

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

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.

i-am-yuvi commented at 4:06 pm on December 18, 2024: contributor

@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 commented at 4:53 pm on December 18, 2024: contributor

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.

in src/init.cpp:1966 in db30b9c9c9 outdated

1908@@ -1907,7 +1909,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1909         onion_service_target = connOptions.vBinds.front();
1910     } else {
1911         onion_service_target = DefaultOnionServiceTarget();
1912-        connOptions.onion_binds.push_back(onion_service_target);

zaidmstrr commented at 2:14 pm on December 21, 2024:

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

SBNovaScript commented at 0:14 am on February 6, 2025:

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

andremralves commented at 6:00 am on February 26, 2025:

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 marked this as a draft on Feb 20, 2025

fanquake commented at 10:14 pm on February 20, 2025: member

I will fix the merge conflicts.

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

andremralves commented at 2:53 am on February 26, 2025: contributor

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.

andremralves force-pushed on Feb 26, 2025

DrahtBot added the label CI failed on Feb 26, 2025

DrahtBot commented at 3:42 am on February 26, 2025: contributor

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

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 force-pushed on Feb 26, 2025

DrahtBot removed the label Needs rebase on Feb 26, 2025

andremralves force-pushed on Feb 26, 2025

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

Fix feature_bind_port_discover.py and add new test cases 1561575c2d

andremralves force-pushed on Feb 26, 2025

andremralves commented at 7:06 am on February 26, 2025: contributor

Fixed the conflicts!

andremralves marked this as ready for review on Feb 26, 2025

DrahtBot removed the label CI failed on Feb 26, 2025

zaidmstrr commented at 8:12 am on February 26, 2025: none

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

DrahtBot requested review from i-am-yuvi on Feb 26, 2025

i-am-yuvi commented at 2:19 pm on February 26, 2025: contributor

re-ACK 1561575c2d984fd94c9679de3d1de207ef6c7c06

Tested all the changes

in src/net.cpp:3295 in c1de112715 outdated

3287@@ -3286,6 +3288,14 @@ bool CConnman::InitBinds(const Options& options)
3288         if (!Bind(ipv4_any, BF_REPORT_ERROR, NetPermissionFlags::None)) {
3289             return false;
3290         }
3291+
3292+        struct in_addr onion_service_target;
3293+        onion_service_target.s_addr = htonl(INADDR_LOOPBACK);
3294+        const uint16_t onion_port = GetListenPort() + 1;
3295+        const CService onion_addr = {onion_service_target, onion_port}; // 127.0.0.1

ryanofsky commented at 4:24 pm on March 23, 2025:

In commit “net: execute Discover() when bind=0.0.0.0 or :: is set” (c1de1127153259b88f9d26cae983f9036673be90)

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

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

ryanofsky approved

ryanofsky commented at 4:39 pm on March 23, 2025: contributor

Code review ACK 1561575c2d984fd94c9679de3d1de207ef6c7c06, 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.

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

How to test

Code Coverage & Benchmarks

Reviews

Conflicts

Log output at master@85bcfeea23568053ea09013fb8263fa1511d7123

Log output at commit a25ce3b22ff3b5771353ab4bcb66a932702bfbd1