Skip to content

net: learn an addr-fetch getaddr reply, then disconnect - #1101

Merged
reardencode merged 2 commits into
reardencode:masterfrom
portlandhodl:net/addrfetch-learn-reply
Oct 20, 2026
Merged

reardencode merged 2 commits into
reardencode:masterfrom
portlandhodl:net/addrfetch-learn-reply

Conversation

@portlandhodl

@portlandhodl portlandhodl commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Problem

An addr-fetch session (the --proxy DNS seeds, -seednode) that receives more than one address calls punish_disconnect and returns before learn_addrv2. The reply the node asked for never reaches AddrMan, and the seed takes a full ban score. The getaddr also never credits the token bucket, so a learned reply would still be cut to one address. A v1 addr reply never reaches AddrMan at all.

(An earlier version of this description cited a live received: addrv2 (18385 bytes) trace. As review pointed out, that message is over MAX_ADDR_TO_SEND, so the size check still drops it, as Core does. It is not the case this PR fixes.)

Fix (Core ProcessMessage ADDR/ADDRV2)

  • Sending getaddr adds MAX_ADDR_TO_SEND tokens (m_addr_token_bucket += MAX_ADDR_TO_SEND).
  • On an addr-fetch session, learn_addrfetch_reply is the one path for addrv2 and for v1 addr (converted): the addresses are learned and not relayed (Core sets m_getaddr_sent), and more than one address ends the session with request_disconnect and no misbehavior. A one-address self-announce is learned and the session stays up.
  • The MAX_ADDR_TO_SEND size check is unchanged.
  • Out of scope: a v1 addr from a non-addr-fetch peer is still not learned, as on master.

Test

addrfetch_learns_the_getaddr_reply_then_disconnects (crates/rbitcoin-net/tests/peer/mod.rs) drives maybe_queue_addrfetch_getaddr and then handle_peer_frame on live addr-fetch sessions, with a wants_addrv2 neighbor attached:

  • a one-address self-announce is learned and the session stays up
  • a 1000-address addrv2 reply: at least 900 learned, session stopped, ban_score == 0, nothing relayed to the neighbor
  • a two-address v1 addr reply: reaches the book, session stopped, ban_score == 0, nothing relayed

On master it fails with learned 0. Hand mutations were checked: relaying from addr-fetch fails "an addr-fetch reply is not relayed", and not learning v1 fails "a v1 getaddr reply is learned".

Part of making a fresh --proxy node bootstrap, together with #1102 (stacked on this), #1103, #1104 and #1105.

🤖 Generated with Claude Code

@rearden-grok rearden-grok Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

The addrv2 path for a reply of 2..=1000 addresses does what this PR claims. Sending getaddr on an addr-fetch session credits MAX_ADDR_TO_SEND, learn_addrv2 runs, then request_disconnect with ban score 0, and that multi-address message is not relayed. Non-addr-fetch peers never take the credit or the new disconnect. addrfetch_learns_the_getaddr_reply_then_disconnects fails on master: the old punish_disconnect returns before learn_addrv2 and sets the ban score, so learned > 1 and ban_score == 0 both fail.

Leaving these as comments. The 2..=1000 fix is the Core getaddr behavior this commit is for.

Severity Location Finding
Medium crates/rbitcoin-net/src/peer.rs:3036 The live addrv2 (18385 bytes) trace is still dropped. That payload is 1414 IPv4 records (3 + 1414 * 13). n > MAX_ADDR_TO_SEND still bans and returns before learn_addrv2. A Core seed caps a reply at 1000, so keep the cap; the cited trace is the size check, and the new test sends exactly 1000.
Medium crates/rbitcoin-net/src/peer.rs:3009 A v1 addr reply still never reaches AddrMan. on_addr_list only disconnects. Peers that answer getaddr with addr are no longer banned and still contribute nothing. The v1 half of the test asserts stop and ban_score == 0 and does not read the book.
Medium crates/rbitcoin-net/src/peer.rs:3016 Disconnect and the relay skip share n > 1 on a single message. After the new credit, a stream of one-address addrv2 messages is learned and falls through into the neighbor relay loop, and the session stays up until a larger message or the 300s addr-fetch timeout. Core disconnects on vAddr.size() > 1 as well, and only relays when the message has at most 10 addresses with a recent nTime. This relay path has neither check. One credit plus the 0.1 token/s refill bounds it.
Low crates/rbitcoin-net/src/peer_tests.rs:5013 learned > 1 passes if 2 of 1000 are stored. The test never registers a second wants_addrv2 peer, so a relay regression stays green.

credit_getaddr_reply is only reached from maybe_queue_addrfetch_getaddr, and end_addrfetch_on_reply is false unless conn_type is AddrFetch. n == 1 still stays connected, which is Core's self-announce exception.

portlandhodl added a commit to portlandhodl/rbitcoin that referenced this pull request Oct 20, 2026
Review follow-up. The wait had no deadline, so a proxy that never
answered held run_p2p before catch-up and RPC for good. It now gives up
after PROXY_SEED_WAIT (180 s) and the empty book takes the existing
awaiting_dialable path.

It also ended on "book non-empty and no addr-fetch session live", which
a stored one-address self-announce can satisfy. PeerHub now counts
addr-fetch getaddr replies learned (more than one address, bumped only
in learn_addrfetch_reply), and the wait ends when that count rises.
Seeds are re-queued only while no addr-fetch session is live.

Stacked on the addr-fetch learn fix (reardencode#1101): without it no seed reply
is learned and the count never rises.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@portlandhodl

Copy link
Copy Markdown
Contributor Author

Thanks. Addressed in 3e05721f:

Finding Change
Medium, peer.rs:3036: the 18385-byte trace is over the cap Agreed, the cap stays. I removed that trace from the PR description; it is not the case this fixes.
Medium, peer.rs:3009: v1 addr reply never reaches AddrMan An addr-fetch v1 addr reply is now converted and learned through the same learn_addrfetch_reply as addrv2. A v1 addr from other peers is unchanged (still not learned), to keep this PR to the getaddr reply.
Medium, peer.rs:3016: one-address messages fall through to relay An addr-fetch session never relays: learn_addrfetch_reply learns, ends the session on more than one address, and returns before the relay loop (Core m_getaddr_sent).
Low, peer_tests.rs:5013: weak assertion, no relay check The test asserts at least 900 of 1000 are learned and attaches a wants_addrv2 neighbor that must receive nothing. It also covers the one-address self-announce (learned, session stays up) and the v1 reply. Hand mutations of the relay skip and of the v1 learn each fail it.

portlandhodl and others added 2 commits October 20, 2026 10:24
An addr-fetch session (the --proxy DNS seeds, -seednode) that got more
than one address back called punish_disconnect and returned before
learn_addrv2, so the reply never reached AddrMan and the seed took a
full ban score. The getaddr also never credited the token bucket, so
even a learned reply would have kept one address.

Follow Core ProcessMessage ADDR/ADDRV2: sending getaddr adds
MAX_ADDR_TO_SEND tokens, the reply is learned, and the addr-fetch
session then ends with request_disconnect and no misbehavior.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review follow-up. A v1 addr reply to an addr-fetch getaddr was only a
disconnect and never reached AddrMan; it is now converted and learned.
An addr-fetch session no longer relays what it learns: Core sets
m_getaddr_sent and does not relay a getaddr reply, and a run of
one-address addrv2 messages fell through to the neighbor relay.
learn_addrfetch_reply is the one path for both.

The test checks at least 900 of the 1000 addresses are learned, that a
wants_addrv2 neighbor gets nothing relayed, that a one-address
self-announce is learned without ending the session, and that the v1
reply reaches the book.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@portlandhodl
portlandhodl force-pushed the net/addrfetch-learn-reply branch from 3e05721 to 507bda9 Compare October 20, 2026 17:41
portlandhodl added a commit to portlandhodl/rbitcoin that referenced this pull request Oct 20, 2026
Review follow-up. The wait had no deadline, so a proxy that never
answered held run_p2p before catch-up and RPC for good. It now gives up
after PROXY_SEED_WAIT (180 s) and the empty book takes the existing
awaiting_dialable path.

It also ended on "book non-empty and no addr-fetch session live", which
a stored one-address self-announce can satisfy. PeerHub now counts
addr-fetch getaddr replies learned (more than one address, bumped only
in learn_addrfetch_reply), and the wait ends when that count rises.
Seeds are re-queued only while no addr-fetch session is live.

Stacked on the addr-fetch learn fix (reardencode#1101): without it no seed reply
is learned and the count never rises.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@rearden-grok rearden-grok Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

The holes from the last review are fixed on 507bda95.

An addr-fetch getaddr credits MAX_ADDR_TO_SEND, then both addrv2 and v1 addr go through learn_addrfetch_reply and return before the neighbor relay. Learn runs before request_disconnect. More than one address ends the session with ban score 0. A one-address self-announce is learned and the session stays up. Non-addr-fetch addrv2 still learns and relays. Non-addr-fetch v1 is still ignored. A count above MAX_ADDR_TO_SEND still calls misbehaving and returns before learn, which is the right place for the 1414-record message.

addrfetch_learns_the_getaddr_reply_then_disconnects now requires at least 900 of 1000 stored, an empty neighbor channel, a learned v1 reply, ban score 0, and a one-address self-announce that leaves the session up. On master the n > 1 path still punish-disconnects before learn, so that test fails there.

@reardencode
reardencode merged commit d6f5dbf into reardencode:master Oct 20, 2026
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants