Run feature_bind_port_(discover|externalip).py in CI

vasild commented at 7:50 am on September 11, 2025: contributor

This PR includes #32757 (first two commits here).

Then, on top of that:

feature_bind_port_discover.py and feature_bind_port_externalip.py require a routable address on the machine to run. Since that was not predictably available on CI, those tests required a manual setting up of IP addresses (e.g. using ifconfig) and then running the tests with a command line option telling them that the addresses are set up. The tests were not run in CI and got rot.

Change that to auto-detect, from the tests, whether the needed IP addresses are present and if yes, run the test, otherwise skip it. Also change the CI to configure the needed addresses when running the functional tests. This way the tests will be run regularly on CI.

Fixes: #31293 Fixes: #31336

DrahtBot commented at 7:50 am on September 11, 2025: 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/33362.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33732 (ci: Call docker exec from Python script to fix word splitting by maflcko)
#32757 (net: Fix Discover() not running when using -bind=0.0.0.0:port by b-l-u-e)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

interface on this machine’ -> a non-loopback interface on this machine’ [adds the missing article “a” to make the sentence grammatical and clear]

drahtbot_id_5_m

vasild force-pushed on Sep 11, 2025

vasild commented at 8:39 am on September 11, 2025: contributor

69177404ba...f047fce8ab: fix typo

in ci/test/02_run_container.sh:91 in f047fce8ab outdated

85@@ -86,7 +86,15 @@ if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then
86   docker image prune --force --filter "label=$CI_IMAGE_LABEL"
87 
88   # shellcheck disable=SC2086
89-  CI_CONTAINER_ID=$(docker run --cap-add LINUX_IMMUTABLE $CI_CONTAINER_CAP --rm --interactive --detach --tty \
90+  CI_CONTAINER_ID=$(docker run \
91+                  --cap-add LINUX_IMMUTABLE \
92+                  --cap-add NET_ADMIN \

willcl-ark commented at 12:15 pm on September 11, 2025:

What is the effect on a developer running CI locally on their machine, of adding NET_ADMIN capability? Is is possible for the CI to then modify/break the developer machine networking?

   CAP_NET_ADMIN
          Perform various network-related operations:
          •  interface configuration;
          •  administration of IP firewall, masquerading, and
             accounting;
          •  modify routing tables;
          •  bind to any address for transparent proxying;
          •  set type-of-service (TOS);
          •  clear driver statistics;
          •  set promiscuous mode;
          •  enabling multicasting;
          •  use setsockopt(2) to set the following socket options:
             SO_DEBUG, SO_MARK, SO_PRIORITY (for a priority outside
             the range 0 to 6), SO_RCVBUFFORCE, and SO_SNDBUFFORCE.

We use our own --network in CI:

https://github.com/bitcoin/bitcoin/blob/ee42d59d4de970769ebabf77b89ff4269498f61e/ci/test/02_run_container.sh#L71

But I am unsure if e.g. ip addr add "1.1.1.1/32" adds it to this network only, or the host network?

If this is affecting (or has the potential to affect) host network, perhaps it can be gated by if $CI or similar?

vasild commented at 1:07 pm on September 11, 2025:

The privileges only apply to the guest, not to host. The guest (“container” in docker language) has no way to manipulate the host’s networking.

https://docs.docker.com/engine/containers/run/#container-networking https://docs.docker.com/engine/network/

We use our own --network in CI …

Hmm. Maybe that could be an easier way to add the dummy routable addresses. Then they will be available for the whole duration of the entire CI run, not only during the functional tests as in this PR. I guess this should be ok, will look into this…

vasild commented at 2:51 pm on September 12, 2025:

We use our own --network in CI …

Maybe that could be an easier way to add the dummy routable addresses

Yes, done like that. Seems to be smaller change and looks more robust.

vasild force-pushed on Sep 12, 2025

vasild commented at 2:50 pm on September 12, 2025: contributor

f047fce8ab...9b30b0bea8: set up the needed addresses of the VMs outside of the VMs, when setting them up using docker run and docker network connect, instead of from inside the VM. The former seems to be simpler. Idea from the discussion above.

DrahtBot added the label Needs rebase on Oct 29, 2025

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

test: Update feature_bind_port_discover.py to use dynamic ports and add comprehensive test coverage

test: use tor_port to prevent bind conflicts

test: Update feature_bind_port_discover.py instructions
Replace deprecated ifconfig with modern ip commands and updated
instructions for non-loopback interfaces.

37e5e9e7c0

test: make feature_bind_port_(discover|externalip).py auto-detect the skip condition

Instead of requiring a run with an explicit `--ihave1111and2222`, detect
whether the routable addresses are set up and if not, then skip the test.

To detect whether the addresses are set use `bitcoind` - start it
and ask it to bind on them and see if it will error with
"Unable to bind". Since this is what the tests do anyway, just start
the nodes and see if an exception will be raised like
`FailedToStartError` / "Unable to bind".

This makes it possible for the CI to run
`feature_bind_port_discover.py` and
`feature_bind_port_externalip.py` by just setting up the
addresses, without having to explicitly provide `--ihave1111and2222`.

560395103b

ci: add one more routable address to the VMs (docker containers)

Also explicitly specify which addresses from the docker network to
assign to the VM.

With `1.1.1.5` and `1111:1111::5` set on the machine, the tests
`feature_bind_port_discover.py` and
`feature_bind_port_externalip.py` will run.

e245da32dc

vasild force-pushed on Nov 6, 2025

vasild commented at 4:55 pm on November 6, 2025: contributor

9b30b0bea8...e245da32dc: rebase due to conflicts

DrahtBot removed the label Needs rebase on Nov 6, 2025

DrahtBot added the label Needs rebase on Dec 19, 2025

DrahtBot commented at 4:44 pm on December 19, 2025: contributor

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

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

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)