Run feature_bind_port_(discover|externalip).py in CI by vasild · Pull Request #33362 · bitcoin/bitcoin

vasild · 2025-09-11T07:50:42Z

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 · 2025-09-11T07:50:46Z

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:

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

vasild · 2025-09-11T08:39:39Z

69177404ba...f047fce8ab: fix typo

ci/test/02_run_container.sh

vasild · 2025-09-12T14:50:21Z

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.

vasild · 2025-11-06T16:55:30Z

9b30b0bea8...e245da32dc: rebase due to conflicts

frankomosh · 2025-12-31T07:41:49Z

ci/test/02_run_container.py

        ).stdout.strip()
        os.environ["CI_CONTAINER_ID"] = container_id

+        run(["docker", "network", "connect", "--ip=1.1.1.5", "ci-ip4net", container_id])


The container has been attached to both the default bridge network and ci-ip4net, hence two
IPv4 addresses. Do you think getifaddrs() may return the bridge IP first, and cause Discover() to find the wrong address?

If thats the case then I think it might be worth it to disconnect from bridge after connecting to ci-ip4net.

Discover() finds all addresses, not just the first one?

frankomosh · 2025-12-31T07:43:41Z

ci/test/02_run_container.py

            CI_CCACHE_MOUNT = f"type=bind,src={os.environ['CCACHE_DIR']},dst={os.environ['CCACHE_DIR']}"

        run(["docker", "network", "create", "--ipv6", "--subnet", "1111:1111::/112", "ci-ip6net"], check=False)
+        run(["docker", "network", "create", "--subnet", "1.1.1.0/24", "ci-ip4net"], check=False)


Would it make sense to use RFC-5737 here? (I think they are reserved for documentation and testing)

That would mess with IsRoutable():

bitcoin/src/netaddress.cpp

Lines 462 to 465 in 3532e24

bool CNetAddr::IsRoutable() const

{

return IsValid() && !(IsRFC1918() || IsRFC2544() || IsRFC3927() || IsRFC4862() || IsRFC6598() || IsRFC5737() || IsRFC4193() || IsRFC4843() || IsRFC7343() || IsLocal() || IsInternal());

}

bitcoin/src/netaddress.cpp

Lines 334 to 339 in 3532e24

bool CNetAddr::IsRFC5737() const

{

return IsIPv4() && (HasPrefix(m_addr, std::array<uint8_t, 3>{192, 0, 2}) ||

HasPrefix(m_addr, std::array<uint8_t, 3>{198, 51, 100}) ||

HasPrefix(m_addr, std::array<uint8_t, 3>{203, 0, 113}));

}

willcl-ark · 2026-01-05T14:47:48Z

test/functional/feature_bind_port_externalip.py

+        try:
+            self.start_nodes()
+        except FailedToStartError as e:
+            for node in self.nodes:


This error/cleanup code is duplicated in both files (with the exception of printing 1 or 2 addresses), would it be worth extracting this into a helper in test_node.py or somewhere?

Maybe, do you want me to take a short at that?

Using rg -C 2 "self.start_nodes()" . it does not seem like any other tests do this (surround with try/except) so I am wondering is this cleanup even necessary?

In case it is, is it only for this test or for all tests? In case you can't argue why it is necessary this should be removed in both files, in case it is only for these tests adding a helper is not worth it imo. in case its relevant for all tests this should be a separate PR (but you can leave the duplicated logic in this PR)

…itebind=0.0.0.0

…dd 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.

… 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`.

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.

vasild · 2026-02-04T09:30:07Z

e245da32dc4410be684b25c197e64458378a5d11...70558c66068c977df5673adeca4d68a80a3d853f: rebase due to conflicts

NicolaLS · 2026-02-12T14:55:55Z

test/functional/feature_bind_port_externalip.py

+        for node in self.nodes:
+            node.has_explicit_bind = True


In case this PR is intended to build on top of #32757 you can add a commit that removes this as this is a workaround that is required before that PR.

See: #31293 (comment)

So, if that is fixed, then the above fix to the test will not be necessary.

Removing it also indirectly tests that the discover fix works and makes this test better.

fanquake · 2026-02-27T13:31:37Z

Moved to draft, given this builds on #32757, and review should go there.

vasild mentioned this pull request Sep 11, 2025

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

Open

vasild force-pushed the ci_add1111addr branch from 6917740 to f047fce Compare September 11, 2025 08:39

willcl-ark reviewed Sep 11, 2025

View reviewed changes

ci/test/02_run_container.sh Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Sep 12, 2025

ci: detect outbound internet traffic generated while running tests #31349

Open

vasild force-pushed the ci_add1111addr branch from f047fce to 9b30b0b Compare September 12, 2025 14:47

DrahtBot mentioned this pull request Oct 22, 2025

ci: Retry image building once on failure #33677

Merged

DrahtBot added the Needs rebase label Oct 29, 2025

vasild force-pushed the ci_add1111addr branch from 9b30b0b to e245da3 Compare November 6, 2025 16:55

DrahtBot removed the Needs rebase label Nov 6, 2025

DrahtBot mentioned this pull request Nov 7, 2025

ci: Call docker exec from Python script to fix word splitting #33732

Merged

DrahtBot added the Needs rebase label Dec 19, 2025

frankomosh reviewed Dec 31, 2025

View reviewed changes

willcl-ark reviewed Jan 5, 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

b-l-u-e and others added 4 commits February 4, 2026 10:14

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

ef9158c

…itebind=0.0.0.0

vasild force-pushed the ci_add1111addr branch from e245da3 to 70558c6 Compare February 4, 2026 09:20

DrahtBot removed the Needs rebase label Feb 4, 2026

NicolaLS reviewed Feb 12, 2026

View reviewed changes

fanquake marked this pull request as draft February 27, 2026 13:31

	bool CNetAddr::IsRoutable() const
	{
	return IsValid() && !(IsRFC1918() \|\| IsRFC2544() \|\| IsRFC3927() \|\| IsRFC4862() \|\| IsRFC6598() \|\| IsRFC5737() \|\| IsRFC4193() \|\| IsRFC4843() \|\| IsRFC7343() \|\| IsLocal() \|\| IsInternal());
	}

	bool CNetAddr::IsRFC5737() const
	{
	return IsIPv4() && (HasPrefix(m_addr, std::array<uint8_t, 3>{192, 0, 2}) \|\|
	HasPrefix(m_addr, std::array<uint8_t, 3>{198, 51, 100}) \|\|
	HasPrefix(m_addr, std::array<uint8_t, 3>{203, 0, 113}));
	}

Conversation

vasild commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

vasild commented Sep 11, 2025

Uh oh!

Uh oh!

vasild commented Sep 12, 2025

Uh oh!

vasild commented Nov 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild commented Feb 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented Feb 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vasild commented Sep 11, 2025 •

edited

Loading

DrahtBot commented Sep 11, 2025 •

edited

Loading