test: cover outbound service flag filtering in ThreadOpenConnections #36089

tialkan commented at 8:44 AM on August 26, 2026: none

ThreadOpenConnections skips candidate addresses that fail HasAllDesirableServiceFlags before attempting a connection (net.cpp, in the addrman try loop). That filter has no functional test coverage today, and cannot have any: addpeeraddress hardcodes NODE_NETWORK | NODE_WITNESS, so every address a test can place into addrman already carries the desirable flags. There is no way to put an address there that the filter should skip.

peerman_tests covers GetDesirableServiceFlags itself, but not the selection path that consumes it.

Two commits:

test: allow addpeeraddress to set service flags adds an optional services argument, defaulting to the value that is currently hardcoded, so existing callers are unaffected. Covered in rpc_net.py: default flags are recorded, explicit flags round-trip through getrawaddrman, and an address lacking desirable flags can be placed.
test: cover outbound service flag filtering in ThreadOpenConnections uses it. Two otherwise identical nodes differ only in the service flags of the addresses in their addrman. The node holding only addresses without desirable flags makes no connection attempt; the node holding usable ones does. Both run with an unreachable proxy, so no attempt can succeed — what is observed is whether one is made at all.

The new test sets disable_autoconnect = False, since the behaviour under test is the node's own addrman-driven outbound connection logic.

Tested on macOS/arm64. p2p_outbound_service_flags.py passes on three consecutive runs; rpc_net.py (both transports), p2p_dns_seeds.py, p2p_seednode.py, p2p_addr_relay.py and feature_asmap.py also pass.

test: allow addpeeraddress to set service flags

addpeeraddress hardcodes NODE_NETWORK | NODE_WITNESS, so every address a
functional test puts into addrman carries the desirable service flags.
That leaves the service flag filtering in ThreadOpenConnections
(net.cpp, the HasAllDesirableServiceFlags checks around address
selection) with no way to be exercised from a functional test: there is
no way to place an address that should be skipped.

GetDesirableServiceFlags itself is covered by peerman_tests, but the
addrman selection path that consumes it is not.

Add an optional services argument, defaulting to the current hardcoded
value so existing callers are unaffected, and cover it in rpc_net:
default flags are recorded, explicit flags round-trip through
getrawaddrman, and an address lacking desirable flags can be placed.

d32d5d0273

test: cover outbound service flag filtering in ThreadOpenConnections

ThreadOpenConnections skips candidate addresses that fail
HasAllDesirableServiceFlags before attempting a connection. That filter
had no functional test coverage. peerman_tests covers
GetDesirableServiceFlags itself, but not the selection path consuming it.

Add a test with two otherwise identical nodes, differing only in the
service flags of the addresses in their addrman: the node holding only
addresses without desirable flags makes no connection attempt, the node
holding usable ones does. Both use an unreachable proxy, so no attempt
can succeed; what is observed is whether one is made.

Two nodes rather than two phases on one node, so addresses from the
first case cannot influence the second.

1a4cfe037a

DrahtBot added the label Tests on Aug 26, 2026

DrahtBot commented at 8:44 AM on August 26, 2026: 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/36089.

Reviews

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

LLM Linter (✨ experimental)

Possible typos and grammar issues:

while a node whose addrman holds usable addresses should. -> while a node whose addrman holds usable addresses should attempt. [The sentence is incomplete as written; a verb is missing.]

in test/functional/rpc_net.py:438 in 1a4cfe037a

 433 | +        assert_equal(node.addpeeraddress(address="3.0.0.0", port=8333, services=NODE_NETWORK), {"success": True})
 434 | +        assert_equal(self.find_addrman_entry(node.getrawaddrman()["new"], "3.0.0.0")["services"], NODE_NETWORK)
 435 | +
 436 | +        self.log.debug("Test that an address lacking desirable service flags can be recorded")
 437 | +        assert_equal(node.addpeeraddress(address="4.0.0.0", port=8333, services=NODE_NONE), {"success": True})
 438 | +        assert_equal(self.find_addrman_entry(node.getrawaddrman()["new"], "4.0.0.0")["services"], NODE_NONE)

willcl-ark commented at 9:17 AM on August 26, 2026:

These addresses won't work with addrman correctly as they are RFC1918 non-routable addresses (see src/netaddress.h).

Since fill_addrman() ignores addpeeraddress’s result, you are effectively testing an empty addrman, rather than verifying an addrman entry with NODE_NONE is skipped here.

maflcko commented at 10:24 AM on August 26, 2026: member

Was this LLM generated? Which mutation is this killing? What coverage is this adding?

Please note that contributors are required to fully understand their authored code themselves. Also, LLM generated content is not allowed in comments/communications.

See https://github.com/bitcoin/bitcoin/blob/master/doc/AI_POLICY.md

sedited commented at 8:01 AM on August 27, 2026: contributor

Closing this again. The patch is currently wrong as laid out by @willcl-ark and I don't think this really adds valuable coverage in the first place.

sedited closed this on Aug 27, 2026