test: fix addr relay test silently passing and other improvements#34750
Merged
fanquake merged 3 commits intobitcoin:masterfrom Mar 12, 2026
Merged
test: fix addr relay test silently passing and other improvements#34750fanquake merged 3 commits intobitcoin:masterfrom
fanquake merged 3 commits intobitcoin:masterfrom
Conversation
the test silently passes on master because SetupAddressRelay isn't called by default for inbound connections.
since we're bumping mocktime more than CHAIN_SYNC_TIMEOUT = 20 * 60, it's possible for disconnections like this to happen in the test: $ test/functional/p2p_addr_relay.py --randomseed=7758649581790797022 ... TestFramework (INFO): Check that we answer getaddr messages only once per connection TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:58829 due to [Errno 54] Connection reset by peer ...
we don't need to send GETADDR for initial self announcement anymore + can construct addr_receivers using AddrReceiver(send_getaddr=False). however we would need to send an empty ADDR message to each of the addr_receivers to initialise addr relay for inbound connections. so current code is simpler and we can just clarify the comment.
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, 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. |
Contributor
Author
|
(CI failure is unrelated) |
Bortlesboat
reviewed
Mar 6, 2026
Bortlesboat
left a comment
There was a problem hiding this comment.
utACK 57bfa86
The move of msg_addr() before the relay check is the key fix — without it, the num_ipv4_received == 0 assertion was passing because address relay was never initialized, not because relay was actually suppressed.
The peerinfo index fix and header announcement to prevent eviction are straightforward.
Bortlesboat
reviewed
Mar 8, 2026
Bortlesboat
left a comment
There was a problem hiding this comment.
ACK 57bfa86. Ran both p2p_addr_relay.py and p2p_addr_selfannouncement.py locally, both pass. Good catch on the stale peerinfo reference in inbound_blackhole_tests — that would silently check the wrong peer.
Member
naiyoma
reviewed
Mar 11, 2026
Contributor
There was a problem hiding this comment.
ACK 57bfa86
I would suggest testing this flow, as it was previously an edge case but is now the default behaviour:
- self-announcement
- GETADDR
- ADDR.
diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py
index 65b21c0d50..af8a10e5c0 100755
--- a/test/functional/p2p_addr_relay.py
+++ b/test/functional/p2p_addr_relay.py
+ tip_header = from_hex(CBlockHeader(), self.nodes[0].getblockheader(self.nodes[0].getbestblockhash(), False))
+ full_outbound_peer.send_and_ping(msg_headers([tip_header]))
+
+ # Self-announcement is not relayed (m_getaddr_sent=true, set when node sent GETADDR)
+ self_announcement = self.setup_addr_msg(1)
+ self.send_addr_msg(full_outbound_peer, self_announcement, [inbound_peer])
+ self.log.info('Check that self-announcement from outbound peer is not relayed')
+ assert_equal(inbound_peer.num_ipv4_received, 0)
+
+ # GETADDR response is not relayed (vAddr.size() >= 1000)
+ getaddr_response = self.setup_addr_msg(1000)
+ self.send_addr_msg(full_outbound_peer, getaddr_response, [inbound_peer])
+ self.log.info('Check that GETADDR response from outbound peer is not relayed')
assert_equal(inbound_peer.num_ipv4_received, 0)
- self.log.info('Check that subsequent addr messages sent from an outbound peer are relayed')
+
+ self.log.info('Check that first subsequent addr messages sent from an outbound peer are relayed(m_getaddr_sent=false)')
Contributor
|
An alternative would be to check that a small getaddr response would be relayed, but a larger response wouldn't. (Both this and the previous suggestion might be out of scope—feel free to ignore.) + self.log.info('Test that self-announcement resets m_getaddr_sent, causing small GETADDR responses to be relayed')
+ outbound_peer1 = self.nodes[0].add_outbound_p2p_connection(
+ AddrReceiver(), p2p_idx=0, connection_type="outbound-full-relay"
+ )
+ inbound_peer1 = self.nodes[0].add_p2p_connection(AddrReceiver(send_getaddr=False))
+ # Enable addr relay on inbound_peer1
+ inbound_peer1.send_and_ping(msg_addr())
+
+ # Outbound peer sends self-announcement -> resets m_getaddr_sent to False
+ self_announcement = self.setup_addr_msg(1)
+ outbound_peer1.send_and_ping(self_announcement)
+
+ # small GETADDR response (9 addrs) gets relayed
+ getaddr_response_small = self.setup_addr_msg(9)
+ self.send_addr_msg(outbound_peer1, getaddr_response_small, [inbound_peer1])
+ assert_equal(inbound_peer1.num_ipv4_received, 9)
+
+ # Large GETADDR response (1000 addrs) - not relayed, but only due to size check
+ getaddr_response_large = self.setup_addr_msg(1000)
+ self.send_addr_msg(outbound_peer1, getaddr_response_large, [inbound_peer1])
+ assert_equal(inbound_peer1.num_ipv4_received, 9)
+
+ self.nodes[0].disconnect_p2ps() |
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Mar 12, 2026
the test silently passes on master because SetupAddressRelay isn't called by default for inbound connections. Github-Pull: bitcoin#34750 Rebased-From: ecb5ce6
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Mar 12, 2026
since we're bumping mocktime more than CHAIN_SYNC_TIMEOUT = 20 * 60, it's possible for disconnections like this to happen in the test: $ test/functional/p2p_addr_relay.py --randomseed=7758649581790797022 ... TestFramework (INFO): Check that we answer getaddr messages only once per connection TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:58829 due to [Errno 54] Connection reset by peer ... Github-Pull: bitcoin#34750 Rebased-From: 7ee8c0a
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Mar 12, 2026
we don't need to send GETADDR for initial self announcement anymore + can construct addr_receivers using AddrReceiver(send_getaddr=False). however we would need to send an empty ADDR message to each of the addr_receivers to initialise addr relay for inbound connections. so current code is simpler and we can just clarify the comment. Github-Pull: bitcoin#34750 Rebased-From: 57bfa86
Member
|
Backported to |
Draft
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
couple of improvements in the addr relay test:
ConsiderEviction