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

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

Fix feature_bind_port_discover.py and add new test cases db30b9c9c9

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
Stale ACK	i-am-yuvi

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:

#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#31223 (net, init: derive default onion port if a user specified a -port by mzumsande)

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

DrahtBot commented at 0:57 am on December 14, 2024: contributor

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

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: none

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: 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

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

andremralves commented at 4:53 pm on December 18, 2024: contributor