Avoid generating outbound traffic on a non-loopback interface during tests. Fix all tests, including ones that generate DNS traffic.
This is a subset of #31349 containing only the changes to the tests, without the CI changes to detect future regressions.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31646.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | brunoerg, kevkevinpal, BrandonOdiwuor, jonatack |
| Concept ACK | tdb3 |
| Stale ACK | mzumsande |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
Concept ACK
What about having an option to set this in BitcoinTestFramework and avoid the duplicated code?
16@@ -17,32 +17,34 @@
17 class P2PSeedNodes(BitcoinTestFramework):
18 def set_test_params(self):
19 self.num_nodes = 1
20+ # Specify a non-working proxy to make sure no actual connections to random IPs are attempted.
21+ self.extra_args = [["-proxy=127.0.0.1:1"]]
UNREACHABLE_PROXY_ARG here as well
Concept ACK
Nice. Better consistency/privacy as well as a test runtime reduction. Will circle back to take a deeper look.
`p2p_seednode.py` would try to connect to `0.0.0.1` and `0.0.0.2` as
seed nodes. This sends outbound TCP packets on a non-loopback interface
to the default router.
Configure an unavailable proxy for all executions of `bitcoind` during
this test. Also change `0.0.0.1` and `0.0.0.2` because connecting to
them would skip the `-proxy=` setting because for such an address:
* `CNetAddr::IsLocal()` is true, thus
* `CNetAddr::IsRoutable()` is false, thus
* `CNetAddr::GetNetwork()` is `NET_UNROUTABLE`, even though
`CNetAddr::m_net` is `NET_IPV4`.
This speeds up the execution time of `p2p_seednode.py`
from 12.5s to 2.5s.
`feature_config_args.py` uses a proxy address of `1.2.3.4`. This results
in actually trying to open TCP connections over the internet to
`1.2.3.4:9050`.
The test does not need those to succeed so use `127.0.0.1:1` instead.
Also avoid `-noconnect=0` because that is interpreted as `-connect=1`
which is interpreted as `-connect=0.0.0.1` and a connection to
`0.0.0.1:18444` is attempted.
`p2p_dns_seeds.py` would try to connect to the DNS server configured on
the machine and resolve `dummySeed.invalid`.
To block that configure an unavailable proxy which will be used also to
connect to the name server. The test needs 2 successful connections to
other peers (two Python `P2PInterface`s) and they work in spite of the
unavailable proxy because they are on `127.0.0.1` (`NET_UNROUTABLE`) and
the proxy is not used for that.
66dbfaca22...2ed161c5ce: do #31646 (review)
light code review ACK 2ed161c
I think it makes sense that our test suite should not attempt to make outbound connections this is a welcomed change
ACK 2ed161c5ce648cb66ec3d2941b02d68b6ca4c390
(did not observe an increase in speed on arm64 m1 max macos 15.2)
Post-merge ACK 2ed161c5ce648cb66ec3d2941b02d68b6ca4c390
Executed functional tests while running Wireshark. Observed no node related traffic with external addresses. Traffic to 0.0.0.1, 0.0.0.2, and 1.2.3.4 was observed on master before this PR was merged (e.g. bb5f76ee013ee439f70e86319d22f308db8a24ea).
