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

Reviews

See the guideline for information on the review process.

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:

• #34750 (test: fix addr relay test silently passing and other improvements by stratospher)
• #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.

Sharing this here in case it’s helpful for review.

1. This PR also addresses the concern raised here → p2p: send first addr self-announcement in separate message 🎄 #34146 (comment). I found the comment a bit confusing, as I didn’t fully understand how the self-announcement is disadvantaged. That said, the flag being set incorrectly is true.

2. The issue discussed here -> p2p: Remove fGetAddr flag #19794 (comment)
   Removing this flag means that we can relay a (small <= 10) getaddr reponse if an addrman is very small , maybe this is still an issue, but this flag will not prevent this from happening.

3. If you cherry-pick this commit 4811358 you can test that this assertion https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_addr_relay.py#L189 passes even without the flag, hence the deletion

4. Some logs with the messages in order:

2026-02-25T10:48:57Z [net] ADDR recv: msg #1 from peer=15, contains 1 addresses
2026-02-25T10:48:57Z [net] ADDR msg #1 from peer=15: 1 received, 1 processed, 0 rate-limited, 0 relayed
2026-02-25T10:48:57Z [net] ADDR state: peer=15 m_getaddr_sent=true

2026-02-25T10:49:05Z [net] ADDR recv: msg #2 from peer=15, contains 999 addresses
2026-02-25T10:49:05Z [net] ADDR msg #2 from peer=15: 999 received, 999 processed, 0 rate-limited, 0 relayed
2026-02-25T10:49:05Z [net] ADDR state: peer=15 m_getaddr_sent=false

2026-02-25T10:49:28Z [net] ADDR recv: msg #3 from peer=15, contains 2 addresses
2026-02-25T10:49:28Z [net] ADDR relay: [2a02:908:530:c1a0:bd2e:450e:8330:e0f2]:8333 from msg #3 (peer=15)
2026-02-25T10:49:28Z [net] ADDR relay: j3og3zy5og3acpbcwl7jncmcobhfk66bmrphcev7ct6pjc765zi4t5id.onion:8333 from msg #3 (peer=15)
2026-02-25T10:49:28Z [net] ADDR msg #3 from peer=15: 2 received, 2 processed, 0 rate-limited, 2 relayed
2026-02-25T10:49:28Z [net] ADDR state: peer=15 m_getaddr_sent=false

msg # 1 message one is the self announcement

msg # 2 getaddr response

msg # 3 addresses to relay

Interesting. Concept ACK.

concept ack.

cc @mzumsande

cc @0xB10C

2. (good change -fixes an inconsistent behaviour on master) currently we don’t relay the 1st ADDR message received from an outbound peer with size ≤ 10. now we relay it.

Thanks for reviewing
A quick clarification on this: we started relaying the first ADDR message after PR 34146, not this PR.
This is because the self-announcement sets this flag to false. If you look at the logs here -> #34717 (comment)
The first ADDR message, which is message 3 (msg # 3) that you are referring to, is being relayed, also, notice that by the time we receive this message, m_getaddr_sent = false, so the relay condition is true.

crACK 9e34acb

The first addr message is relayed because the flag is false(this was not the case before #34146).

Can you elaborate? It seems like the very first addr message was not being relayed, like you said?

For the first regular ADDR after the GETADDR response, this condition was false because m_getaddr_sent was true

I think the intended logic is (was) "we sent a getaddr, therefore the next addr message we receive will probably be old information which we shouldn't relay further".

So #34146 did introduce a "bug" in that it became no longer true that next next addr message contained old information -- instead it would contain a self-announcement. That would then clear the flag, and we'd risk relaying the old information in the following addr message, that was in response to the getaddr request. The < 10 check and the 10 minute checks both probably made the impact of that "bug" pretty rare/trivial however. So I don't think this is urgent for 31.0.

I think this change is fine though -- the 10min and <10 check seem already sufficient: if we get a small response to our getaddr, that's full of (honestly) recent information, then relaying that is either good or at worst harmless; and if an attacker were trying to exploit that ability, they could simply have sent a dummy addr message, and the real attack data later, avoiding the m_get_addr_sent gate entirely.

I'm not sure that improved relay of self-announcements of nodes that only just connected to you is necessarily a good thing. Currently (and prior to #34146) your node's IP would likely only be relayed reliably once you've been connected to a peer for a couple of hours or so, which serves as an automatic way of avoiding cluttering up everyone's address book with (honest) nodes that start up, sync, and shut down. So Concept ~0. Code looks fine if the behaviour is desirable, though.

I'm not sure that improved relay of self-announcements of nodes that only just connected to you is necessarily a good thing.

Does this have the direction mixed up? This is about improved relay of self-announcements originating from peers that we connected to - we, the initiating node, issue the GETADDR request, set m_get_addr_sent and process the answer / the self-announcement of that peer and maybe relay it further. So the affected self-announcements are those from reachable, usually reliable nodes (after all we picked them somehow).

The reverse direction (we, a possibly new/unreliable node, connect to a peer, and our own self-advertisment gets relayed by that peer or not) is not affected by this PR at all I think because this would involve m_getaddr_recvd instead of m_get_addr_sent.

reACK b591499

Does this have the direction mixed up? This is about improved relay of self-announcements originating from peers that we connected to - we, the initiating node, issue the GETADDR request, set m_get_addr_sent and process the answer / the self-announcement of that peer and maybe relay it further. So the affected self-announcements are those from reachable, usually reliable nodes (after all we picked them somehow).

Yeah, it does have the direction mixed up. Still not sure how much benefit it makes to relay the initial self-announcement though. If they're already reachable/reliable nodes, then their address is already well known, and only getting re-announced by long-term connections is probably not terrible? I guess if it weren't useful to relay it, the peer shouldn't bother sending it to us -- we've already connected successfully, so we already have a working address for the peer and don't need to be told; so why else would they send it unless they wanted it relayed so others could more easily connect to them? Delaying the self-announcement until the connection's been alive a random amount of time would obviously be possible on the sender's side; (eg suggested here) so why treat it specially on the receiver side?

I guess this doesn't have privacy implications -- I don't think you can distinguish a relayed self-announcement between being one of our outbounds or one of our inbounds; and distinguishing an addr relay for a direct connection vs a random forwarded addr seems like it requires a long-term connection and probably a lot of probability analysis?

From #19794 (comment) :

(I wish the response to a getaddr was a different message type than an unsolicited address being relayed to us!)

I suppose that's something we could consider. I suppose we could also consider limiting our GETADDR responses to addr entries that are more than 10min old, or artificially inflating the ages of the addresses we send to ensure they're more than 10min old (and perhaps documenting that in a BIP so other node software that cares about the privacy concerns raised in that comment can implement compatible logic).

Concept NACK

I do not agree with this solution for removing m_getaddr_sent. Instead we should consider reusing it. I believe there is a significant issue in the bitcoin code: it gives a malicious neighbor node the ability to abuse our unsolicited message relaying. This could happen as follows :

• 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 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.

For these reasons, instead of removing m_getaddr_sent, we should keep it and improve the token handling logic for GETADDR messages.

I do not agree with this solution for removing m_getaddr_sent. Instead we should consider reusing it. I believe there is a significant issue in the bitcoin code: it gives a malicious neighbor node the ability to abuse our unsolicited message relaying.

Can't this be done independent of this patch? Also, the GETADDR is only sent for outbound connections so its impact is limited?

• We increase the token counter to +1000 tokens since we expect to receive up to 1000 addresses in response from node X.

I think this doesn't make sense -- if we want to treat addresses received as a response to "GETADDR" differently, then m_getaddr_sent should be a bucket that's normally 0, but is bumped to 1000 when we send "GETADDR". Then the idea might be:

    bool relay{true};
    if (peer.m_addr_token_bucket < 1.0) {
        if (rate_limited) {
            if (m_getaddr_bucket == 0) {
                ++num_rate_limit; continue;
           } else {
                 --m_getaddr_bucket;
                relay = false;
           }
        }
    } else {
        peer.m_addr_token_bucket -= 1.0;
    }
    ...
    if (relay && addr.nTime > current_a_time - 10min && vAddr.size() <= 10 && addr.IsRoutable()) {
        // Relay to a limited number of other nodes
        RelayAddress(pfrom.GetId(), addr, reachable);
    }

This doesn't need to be part of v31. As mentioned above, even though it is possible for us to relay a GETADDR response currently, the probability is low because the default node behaviour will almost always have around 1000 addresses (> 10 addresses, hence we don't relay), and GETADDR responses are usually older.

I do not agree with this solution for removing m_getaddr_sent. Instead we should consider reusing it. I believe there is a significant issue in the bitcoin code: it gives a malicious neighbor node the ability to abuse our unsolicited message relaying. This could happen as follows :

This attack is still possible with or without this flag in normal address relay, not just in response to GETADDR.

reACK b591499.

agree regarding it not being important for v31 - it's just an extra address relay anyways. I thought of this as a logical couple to #34150 - we're changing initial self announcement sending logic in that PR - so makes sense that the receiving logic code is also updated which is why I initially suggested it for v31. but it's just an inconsistency in our code and won't matter to people running the software.

I guess the initial self announcement being relayed + this not being done before is what's causing concern. There's the alternative (naiyoma#14) which naiyoma proposed of making m_get_addrsent more strict - but it can't act as a proper gate anyways and might as well remove it.

also liked aj's alternative suggestion of making sure GETADDR responses are actually old when selecting from addrman. I collected some address relay stats of my node last month. don't quote me on this since I haven't cross checked with other GETADDR responses. this is the address staleness of the GETADDR response of 1 particular peer (GPT plotted a histogram from the data):

also worth noting - the previous version of PR in #19794 was made at a time when ADDR token logic wasn't there + possibility of someone spamming us without any rate limiting.

then m_getaddr_sent should be a bucket that's normally 0

good idea to separate the logic of the response from normal forwarding , could i also suggest the following: that during the first X seconds we are expecting that we will receive a self-announcement and a response to our GETADDR message, for self-announcement we could distinguish it by checking if the sender is the same as the address in the buffer + the time it will be fresh, so we will relay it. and for the other addresses we will use peer.m_addr_token_bucket and a timer tracker if now - get_addr_send_time <= X,
otherwise we will switch to the normal tokens.

🐙 This pull request conflicts with the target branch and needs rebase.