p2p: remove m_getaddr

naiyoma commented at 12:04 pm on March 3, 2026: contributor

This PR removes m_getaddr_sent, as it no longer behaves as originally intended. Initially, this flag was meant to track when a getaddr message was sent to a peer.

Now that self-announcements are sent separately from getaddr responses, the self-announcement sets m_getaddr_sent to false even though we are still waiting for the getaddr response (1000 addresses). When the getaddr response does arrive, we rely on the size of the addr message, not the flag, to decide whether it should be relayed. This makes the flag redundant.

This is the current behavior:

The initial self-announcement is not relayed but it flips m_getaddr_sent to be false
We use addr.size() to avoid relaying (1000) getaddr response.(assuming the other two flags are false)
The first addr message is relayed because the flag is false(this was not the case before #34146).

Removing this flag ensures that:

The initial self-announcement is relayed.
The getaddr response is still not relayed, using the existing size-based check.

I had initially considered an alternative approach https://github.com/naiyoma/bitcoin/pull/14, where the self-announcement would not affect this flag and would therefore retain the initial behaviour, but i decided to reattempt its removal, as this was previously attempted, see #19794. Given the changes since then, I believe revisiting it is now more appropriate.

net: remove m_getaddr_sent

The flag was intended to track whether we sent a GETADDR to a peer,
but it became redundant after the initial self-announcement sets it to false,
even though we still expect the actual getaddr response.

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

481135858a

DrahtBot added the label P2P on Mar 3, 2026

DrahtBot commented at 12:04 pm on March 3, 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
ACK	Crypt-iQ
Concept NACK	taki-abedesselam
Concept ACK	w0xlt, mzumsande
Stale ACK	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:

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

naiyoma commented at 12:06 pm on March 3, 2026: contributor

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

This PR also addresses the concern raised here → #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.
The issue discussed here -> #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.
If you cherry-pick this commit 481135858a743bc28fec20a45828293eb6512a18 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
Some logs with the messages in order:

 02026-02-25T10:48:57Z [net] ADDR recv: msg [#1](/bitcoin-bitcoin/1/) from peer=15, contains 1 addresses
 12026-02-25T10:48:57Z [net] ADDR msg [#1](/bitcoin-bitcoin/1/) from peer=15: 1 received, 1 processed, 0 rate-limited, 0 relayed
 22026-02-25T10:48:57Z [net] ADDR state: peer=15 m_getaddr_sent=true
 3
 42026-02-25T10:49:05Z [net] ADDR recv: msg [#2](/bitcoin-bitcoin/2/) from peer=15, contains 999 addresses
 52026-02-25T10:49:05Z [net] ADDR msg [#2](/bitcoin-bitcoin/2/) from peer=15: 999 received, 999 processed, 0 rate-limited, 0 relayed
 62026-02-25T10:49:05Z [net] ADDR state: peer=15 m_getaddr_sent=false
 7
 82026-02-25T10:49:28Z [net] ADDR recv: msg [#3](/bitcoin-bitcoin/3/) from peer=15, contains 2 addresses
 92026-02-25T10:49:28Z [net] ADDR relay: [2a02:908:530:c1a0:bd2e:450e:8330:e0f2]:8333 from msg [#3](/bitcoin-bitcoin/3/) (peer=15)
102026-02-25T10:49:28Z [net] ADDR relay: j3og3zy5og3acpbcwl7jncmcobhfk66bmrphcev7ct6pjc765zi4t5id.onion:8333 from msg [#3](/bitcoin-bitcoin/3/) (peer=15)
112026-02-25T10:49:28Z [net] ADDR msg [#3](/bitcoin-bitcoin/3/) from peer=15: 2 received, 2 processed, 0 rate-limited, 2 relayed
122026-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

w0xlt commented at 3:57 pm on March 3, 2026: contributor

Interesting. Concept ACK.

chriszeng1010 commented at 8:48 pm on March 3, 2026: none

concept ack.

in test/functional/p2p_addr_relay.py:182 in 9e34acb1db outdated

187-        # large GETADDR responses from being relayed, it now typically affects the self-announcement
188-        # of the outbound peer which is often sent before the GETADDR response.
189-        assert_equal(inbound_peer.num_ipv4_received, 0)
190-
191-        # Send an empty ADDR message to initialize address relay on this connection.
192+

stratospher commented at 9:47 am on March 5, 2026:

9e34acb: nit: I think keeping this comment “# Send an empty ADDR message to initialize address relay on this connection.” is useful.

Crypt-iQ commented at 3:28 pm on March 5, 2026:

I agree, it took me a minute to figure out what was going on here.

Also, unrelated to this PR, I noticed the outbound p2p connections are sending GETADDR that will get ignored.

naiyoma commented at 10:59 am on March 6, 2026:

comment added back b5914990464e581731f8214c0ff1c3cb6b58cb94

naiyoma commented at 3:45 pm on March 6, 2026:

Also, unrelated to this PR, I noticed the outbound p2p connections are sending GETADDR that will get ignored.

I think its because -> https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.cpp#L4911

stratospher commented at 10:49 am on March 5, 2026: contributor

ACK 9e34acb1. nice find!

m_getaddr_sent logic is broken after #34146 and doesn’t do what it’s supposed to do anymore.

i think removing it is a logical bug fix which would be nice to have in v31. (not sure if it’s too late for that)

there would be a few behaviour changes with this PR:

(might be debatable change) we would relay ADDR messages to peers just based on size ≤10. feel it’s ok because:
1. is it ok if someone send 5 ADDR as the GETADDR response (instead of the usual 1000 ADDR) and we relay it?
  1. we do GETADDR requests only to outbound peers ( so unlikely for it to be attacker controlled).
  2. in some remote scenario (where we got eclipsed) , suppose an attacker is chosen as our outbound peer - and it only sends us 5 ADDR as GETADDR response. we will relay it to our other peers once during the start of node’s lifecycle (wouldn’t have relayed these 5 ADDR before this PR). we also will have bigger problems to worry about than if address got relayed.
  3. there are much more effective ways for an attacker to force certain ADDR to be relayed than focus on the response to GETADDR - a normal node only sends GETADDR once during the version handshake.
(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.
(good change) initial self announcement always gets relayed.

fanquake commented at 3:50 pm on March 5, 2026: member

cc @mzumsande

Crypt-iQ commented at 3:59 pm on March 5, 2026: contributor

crACK 9e34acb1dbff61efb1c5e4e9cfce2a4591c2832f

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?

fanquake commented at 10:20 am on March 6, 2026: member

cc @0xB10C

fanquake added this to the milestone 31.0 on Mar 6, 2026

test: delete redundant addr relay assertion

The assertion was passing because inbound_peer had m_addr_relay_enabled
set to false, not for the intended reason of m_getaddr_sent being true.

b591499046

naiyoma force-pushed on Mar 6, 2026

naiyoma commented at 11:29 am on March 6, 2026: contributor

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

naiyoma commented at 11:42 am on March 6, 2026: contributor

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 https://github.com/bitcoin/bitcoin/blob/01651324f4e540f6b96cff31e89752c3f9417293/src/net_processing.cpp#L4074

mzumsande commented at 11:45 am on March 6, 2026: contributor

Concept ACK

Just making clear that while #34146 had the effect that m_get_addr_sent doesn’t match the GETADDR answer anymore, it didn’t make anything worse: the flag was already unnecessary before (due to the <10 criterion), and the first self-announcement (when it was mixed into the 1000 size GETADDR answer) did not get relayed to peers before either.

So I’d say this PR doesn’t correct a bug introduced by #34146, but improves logic that was already broken before.

ajtowns commented at 12:51 pm on March 6, 2026: contributor

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.

mzumsande commented at 1:26 pm on March 6, 2026: contributor

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.

Crypt-iQ commented at 2:34 pm on March 6, 2026: contributor

reACK b5914990464e581731f8214c0ff1c3cb6b58cb94

DrahtBot requested review from mzumsande on Mar 6, 2026

DrahtBot requested review from stratospher on Mar 6, 2026

ajtowns commented at 5:26 pm on March 6, 2026: contributor

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

taki-abedesselam commented at 2:11 am on March 9, 2026: none

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

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

p2p: remove m_getaddr_sent #34717

Reviews

Conflicts