p2p: prevent unsolicited addr relay token abuse #34774

1. 
   taki-abedesselam commented at 4:11 am on March 9, 2026: none

   The current addr relay token logic opens an attack that allows a malicious outbound peer to abuse our unsolicited message relaying as mentioned on #34717 (comment):

   • We establish an outbound connection to a node X.
   • We increase the token counter to +1000 tokens since we expect to receive up to 1000 addresses in response from node X.
   • Node X sends a buffer containing only 1 address, to bypasses the initial m_getaddr_sent check (or this negligible protection disappears entirely if the variable is removed as proposed in this PR)
   • Node X then starts sending unsolicited messages of size 10, since it knows it can use up to 1000 tokens (each address they will consume 1 token and if the number of addresses in the buffer is less or equal to 10 our node will relay it).
   • Our node become an intermediate relay, unintentionally helping hide this malicious node, since its (our node) the one who will relay the unsolicited messages.
   • Our node they will consume all of its tokens with other nodes by forwarding malicious addresses instead of forwarding addresses for legitimate nodes.

   This issue predates PR #34146, which introduced a new addr self-announcement mechanism that rendered m_getaddr_sent entirely ineffective, leading PR #34717 to propose removing it entirely.

   Instead of removing m_getaddr_sent, this PR keeps it and improves the token handling logic. The addr self-announcement is ordered to be sent after the GETADDR response, allowing the GETADDR sender to clearly distinguish between the response and the self-announcement, and safely assume the first incoming message is the legitimate response. Once the GETADDR response is processed, the token bucket is adjusted to count only the addresses actually received.

2. p2p: prevent unsolicited addr relay token abuse 97e9554edc
3. DrahtBot added the label P2P on Mar 9, 2026
4. 
   DrahtBot commented at 4:11 am on March 9, 2026: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept NACK	stratospher

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #34717 (p2p: remove m_getaddr_sent by naiyoma)
   • #34588 (refactor: decompose Peer struct into focused sub-components by w0xlt)
   • #34059 (refactor: Use NodeClock::time_point for m_addr_token_timestamp by maflcko)

   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.

   LLM Linter (✨ experimental)

   Possible typos and grammar issues:

   • only for the -> Only for outbound connections. [Sentence fragment and lowercase start after a period; clarifies intent and fixes capitalization/grammar]
   • in separate ADDR/ADDRV2 message -> in a separate ADDR/ADDRV2 message [missing article “a”, which makes the phrase grammatically incorrect]
   • truck -> track [typo: “truck” is incorrect in context; should be “track”]
   • recive -> receive [spelling error]
   • its -> it’s (or it is) [incorrect possessive vs. contraction; should be “it’s” (it is) to convey “it is supposed to…”]

   2026-03-09 04:11:58

5. 
   stratospher commented at 11:09 am on March 9, 2026: contributor

   concept NACK but liked your thought process! however I don’t this approach will work + would cause backward compatibility problems with nodes that don’t upgrade + other clients.

   main problem is this strict assumption which isn’t true:

   The addr self-announcement is ordered to be sent after the GETADDR response, allowing the GETADDR sender to clearly distinguish between the response and the self-announcement, and safely assume the first incoming message is the legitimate response.

   in the codebase even before #34146, race conditions are possible and self-announcement can end up getting sent before GETADDR response.

   Ex: see #22096. I was surprised when reviewing #34146 that even from v22 we already had code to not disconnect when we receive self announcements first (before GETADDR response) in addr fetch connections. there’s a good explanation to why here - #19794 (comment).

   Also regarding the attack scenario:

   Our node they will consume all of its tokens with other nodes by forwarding malicious addresses instead of forwarding addresses for legitimate nodes.

   we don’t send all 1000 attacker addresses to the same 1 or 2 fixed peers for relay! for each address we want to relay, we use a hasher based on the ip address and other stuff to select which peers we will relay the address to. so 1000 different attacker addresses can’t all hammer the same 2 peers :)

   let’s say node only has 8 outbound connections and each of these peers receive 2/8 * 1000 = 250 addresses on average. since MAX_ADDR_RATE_PER_SECOND = 0.1 addr/sec, 250 / 0.1 = 2500 seconds ≈ 41 minutes, we’s have tokens again after 41 minutes!

   even if by some unpractical scenario (not possible), the same 2 peers are chosen always - we’d have token again after ~ 2 hours, 46 minutes.

   so think that the damages are bounded. but maybe something can be improved? not sure.

   guess this is pretty cool for an adversarial P2P network - even though some of our address relay might get blocked here for few minutes to hours, there are other paths through which the good addresses can propagate! Even if this attacker disconnects, it is extremely unlikely that we select the attacker again as our outbound peer.

6. taki-abedesselam closed this on Mar 9, 2026

   ---

7. taki-abedesselam deleted the branch on Mar 9, 2026
8. taki-abedesselam restored the branch on Mar 10, 2026

Contributors
taki-abedesselam DrahtBot stratospher

Labels
P2P