p2p: Don't self-advertise during version processing #26199

mzumsande commented at 9:43 PM on September 28, 2022: contributor

This picks up the last commit from #19843.

Previously, we would prepare to self-announce to a new peer while parsing a version message from that peer. This is redundant, because we do something very similar in MaybeSendAddr(), which is called from SendMessages() after the version handshake is finished.

There are a couple of differences:

MaybeSendAddr() self-advertises to all peers we do address relay with, not just outbound ones.
GetLocalAddrForPeer() called from MaybeSendAddr() makes a probabilistic decision to either advertise what they think we are or what we think we are, while PushAddress() on version deterministically only does the former if the address from the latter is unroutable.
During version processing, we haven't received a potential sendaddrv2 message from our peer yet, so self-advertisements with addresses from addrV2-only networks would always be dropped in PushAddress().

Since it's confusing to have two slightly different mechanisms for self-advertising, and the one in MaybeSendAddr() is better, remove the one in version.

p2p: Don't self-advertise during VERSION processing

Previously, we would prepare to self-announce to a new peer while
parsing a VERSION message from that peer. This is redundant, because we
do something very similar in MaybeSendAddr(), which is called from
SendMessages() after the version handshake is finished.

There are a couple of differences:

1) MaybeSendAddr() self-advertises to all peers we do address relay with,
   not just outbound ones.
2) GetLocalAddrForPeer() called from MaybeSendAddr() makes a
   probabilistic decision to either advertise
   what they think we are or what we think we are, while
   PushAddress(self) on VERSION deterministically only does
   the former if the address from the latter is unroutable.
3) During VERSION processing, we haven't received a potential sendaddrv2 message
   from our peer yet, so self-advertisements with addresses from addrV2-only networks
   would always be dropped in PushAddress().

Since it's confusing to have two slightly different mechanisms for self-advertising,
and the one in MaybeSendAddr() is better, remove the one in VERSION.

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

3c43d9db1e

mzumsande commented at 9:53 PM on September 28, 2022: contributor

FYI @naumenkogs @dergoegge

sipa commented at 10:05 PM on September 28, 2022: member

Concept ACK

DrahtBot commented at 11:18 PM on September 28, 2022: 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	amitiuttarwar, stratospher, naumenkogs
Concept ACK	sipa, dergoegge, glozow

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25619 (net: avoid overriding non-virtual ToString() in CService and use better naming by vasild)
#24008 (assumeutxo: net_processing changes by jamesob)

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.

DrahtBot added the label P2P on Sep 28, 2022

in src/net_processing.cpp:3277 in 3c43d9db1e outdated

3273 | @@ -3274,39 +3274,16 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3274 |              m_num_preferred_download_peers += state->fPreferredDownload;
3275 |          }
3276 |  
3277 | -        // Self advertisement & GETADDR logic
3278 |          if (!pfrom.IsInboundConn() && SetupAddressRelay(pfrom, *peer)) {

naumenkogs commented at 7:43 AM on September 29, 2022:

SetupAddressRelay used to have a misleading comment: "First addr message we have received from the peer, initialize m_addr_known". We were not receiving any addr messages from the peer here, yet we initialized it. It seems like this was the case from the beginning.
After this PR, calling SetupAddressRelay here seems unnecessary. If we drop it, the comment becomes valid :) Although the inconsistency is a good signal the comment is confusing, so perhaps we should drop it. Not very useful anyway, at least the current code is pretty readable.

naumenkogs commented at 7:44 AM on September 29, 2022:

So yeah: 1) Drop SetupAddressRelay here; 2) Possibly drop the useless comment from SetupAddressRelay?

naumenkogs commented at 7:53 AM on September 29, 2022:

Alright, this has side-effects. MaybeSendAddr (another self-ad) will stop working until we get an ADDR message from the peer (not the case before the change i suggest here).

Perhaps this code should be:

if (!pfrom.IsInboundConn()) {
    SetupAddressRelay(pfrom, *peer);
    ....
}

mzumsande commented at 2:28 PM on September 29, 2022:

I think the current code is correct, SetupAddressRelay here has three tasks:

Initialize m_addr_known for outbound peers (unless block-relay-only), see #21528
Prevent Self-Advertisements to block-relay-only outbound peers (for which SetupAddressRelay returns false)
Prevent a GETADDR to block-relay-only outbound peers

Since 1) and 3) are still valid after this PR, I think we both need to keep calling SetupAddressRelay and skip if it returns false. I think this should be documented better (I had to revisit 21528 to recall how it worked while preparing this PR), so I will add some more documentation for this soon.

naumenkogs commented at 8:18 AM on September 30, 2022:

The refactoring I suggested in my last message here is indeed wrong, because it ignores the returned value.

Do you agree that the comment inside SetupAddressRelay is wrong though? That's the only thing that remains worrying me.

mzumsande commented at 3:36 PM on September 30, 2022:

Do you agree that the comment inside SetupAddressRelay is wrong though?

Yeah, it's incomplete because it only applies to inbound peers. I added documentation there and before the call to SetupAddressRelay during version processing (as a second commit, because it's not directly related to the changes).

naumenkogs commented at 8:33 AM on October 3, 2022:

nit: "Prevent GETADDR from being sent" sounds very complicated, and also might have additional unnecessary meaning of pro-actively preventing something.

I'd say "Don't send GETADDR [...]" is better. Feel free to ignore.

mzumsande commented at 9:31 PM on October 13, 2022:

missed this comment, changed wording now.

amitiuttarwar commented at 11:12 PM on November 21, 2022:

agreed that the comment in SetupAddressRelay was misleading. thanks for catching / fixing. I didn't hunt down the history, but I bet that was my fault 😅

RE current comment about "Don't send GETADDR...": I find it a bit confusing, because we're also not sending GETADDR message to inbound peers. maybe this code is too dense and is better communicated in a bit more verbose manner?

eg. in non-syntax-adhering code:

bool send_getaddr = false;
if (!pfrom.IsInboundConn()) { 
		send_getaddr = SetupAddressRelay();
    }
if (send_getaddr){
	// do GETADDR stuff
}

also cool if you want to skip the refactor and just want to update the comment to something like..

// Initialize address relay for all outbound peers
// Send GETADDR to non-block-relay-only outbound peers

I wish we had a better way to refer to non-block-relay-only outbound peers 😬

mzumsande commented at 9:19 PM on November 23, 2022:

I agree that it this would be better to understand if it's a bit more spread out, so I took your suggestion and changed the comment - hopefully it's clear now?

dergoegge commented at 8:47 AM on September 30, 2022: member

Concept ACK

naumenkogs commented at 9:39 AM on September 30, 2022: member

I dunno if I'm supposed to ACK this since this is my commit, but I approve the changes 3c43d9db1e0f84279b08a93afd730caeece061a9 I guess :)

An easy way to look at this change is to notice that this code is almost entirely a subset of MaybeSendAddr. At the same time, it heavily relies on MaybSendAddr to actually do something, not much would change if you drop the duplicate subset (what this PR does).

One real difference this could make is that due to the "makes a probabilistic decision to either advertise what they think we are or what we think we are" of this behavior, we could have ended up having two self-ads in one ADDR message (one added here, one added in MaybeSendAddr if they are different). I don't think this is a show-stopper for this PR. I don't think we want to preserve this behavior.

UPD:

Another important difference could be the time between the two calls. This might make sense if there's a risk of someone inserting our self-ad (PushAddress) with the wrong metadata, eg nTime. Then our latter self-ad insertion would be dismissed (because we just look up the key), while the VERSION-one is protected from this issue. I don't think this is a show-stopper for this PR, although this surface could indeed be improved (e.g., handle duplicates better, or handle other-nodes-announce-us-through-us differently).

glozow commented at 10:09 AM on September 30, 2022: member

Concept ACK

mzumsande commented at 6:26 PM on September 30, 2022: contributor

One real difference this could make is that due to the "makes a probabilistic decision to either advertise what they think we are or what we think we are" of this behavior, we could have ended up having two self-ads in one ADDR message (one added here, one added in MaybeSendAddr if they are different). I don't think this is a show-stopper for this PR. I don't think we want to preserve this behavior.

Yes, advertising both addresses is something that can happen, but only in a few cases: e.g. if the address during version message processing is AddrV2-only, it is not sent out because we haven't gotten sendaddrv2 yet. I think it might happen if we discovered a local IPv6 address and our peer sees us under a IPv4 address? I think that if this is something we really want, we could always advertise both (similar to your commit https://github.com/bitcoin/bitcoin/pull/19843/commits/10fc62f0c29f640fdefc7114576133fbf18c488a) but I'm not sure it is necessary - after all, we have the nScore logic to make an educated guess which address is better.

Another important difference could be the time between the two calls. This might make sense if there's a risk of someone inserting our self-ad (PushAddress) with the wrong metadata, eg nTime. Then our latter self-ad insertion would be dismissed (because we just look up the key), while the VERSION-one is protected from this issue. I don't think this is a show-stopper for this PR, although this surface could indeed be improved (e.g., handle duplicates better, or handle other-nodes-announce-us-through-us differently).

I think I don't understand this completely, but the time between advertising during version message and MaybeSendAddr should usually be negligible, because m_next_local_addr_send is initialized to 0, so MaybeSendAddr always advertises right after the version handshake is completed. Also, we clear m_addr_known right before self-advertising in MaybeSendAddr (code) so it wouldn't be possible for an attacker to do something like preventing us from self-advertising by sending us our own address. Is that what you meant?

naumenkogs commented at 8:35 AM on October 3, 2022: member

ACK 24608c2cc576f548ae43099dd1cc36d5f05399d7

mzumsande force-pushed on Oct 13, 2022

in src/net_processing.cpp:3283 in 1002cd193d outdated

3285 | -            // we're starting up for the first time, our addrman may be pretty
3286 | -            // empty and no one will know who we are, so these mechanisms are
3287 | +            // For outbound peers, we do a one-time address fetch
3288 | +            // (to help populate/update our addrman).
3289 | +            // If we're starting up for the first time, our addrman may be pretty
3290 | +            // empty and no one will know who we are, so this mechanism is

amitiuttarwar commented at 11:28 PM on November 21, 2022:

            // Do a one-time address fetch to help populate/update our addrman.
            // If we're starting up for the first time, our addrman may be pretty
            // empty, so this mechanism is

amitiuttarwar commented at 11:34 PM on November 21, 2022:

just deleted the parts that are nuanced & already explained or now handled elsewhere. no big deal either way.

mzumsande commented at 9:19 PM on November 23, 2022:

done.

amitiuttarwar commented at 2:37 AM on November 22, 2022: contributor

code review ACK 1002cd193d6f4ae216181aafae80076511645df3. this PR improves the encapsulation of self-advertisement logic, which simplifies the code as well as our mental models of addr relay on the network. @naumenkogs

Another important difference could be the time between the two calls. @mzumsande I think I don't understand this completely, but the time between advertising during version message and MaybeSendAddr should usually be negligible, because m_next_local_addr_send is initialized to 0, so MaybeSendAddr always advertises right after the version handshake is completed.

Agreed, but I'd take it slightly further. I think there is no difference in when the advertisement would actually occur. the ProcessMessage -> version logic called PushAddress which (at best) adds the address to m_addrs_to_send. The actual constructing & sending of the message wouldn't occur until the SendMessages loop calls MaybeSendAddr.

My understanding of this PR is that there would be no tangible behavioral differences in the cases where the version logic returned the same address as MaybeSendAddr logic. on master, if they result in two different addr, I believe we would advertise both. after this PR, we would only advertise the result of GetLocalAddrForPeer, which will select one of the two.

Since the version logic always prioritized GetLocalAddress (aka what we think we are), the behavioral difference would occur in instances when GetLocalAddrForPeer (aka weighted guess) selects the result from IsPeerAddrLocalGood (aka what peer thinks we are).

I think the only time this would cause a connectivity issue is if your peers are somehow reporting your address to be something that can't be connected to, and you would have advertised something usable with GetLocalAddress. I expect this to be quite rare, but additionally your self-identified address would eventually get out when GetLocalAddrForPeer selects the result of GetLocalAddress.

I think that if this is something we really want, we could always advertise both (similar to your commit https://github.com/bitcoin/bitcoin/commit/10fc62f0c29f640fdefc7114576133fbf18c488a) but I'm not sure it is necessary - after all, we have the nScore logic to make an educated guess which address is better.

For the reasons I stated above, I agree. Advertising both seems unnecessary, and the worst case should just be slower time for inbounds to connect to you? @mzumsande @naumenkogs curious to hear your feedback. do these claims track with your understanding?

amitiuttarwar commented at 5:08 PM on November 22, 2022: contributor

oops, didn't post last night - just some suggestions around language in the comments. not essential to incorporate.

naumenkogs commented at 7:49 AM on November 23, 2022: member

Agreed, but I'd take it slightly further. I think there is no difference in when the advertisement would actually occur.

This was the initial purpose of the PR, yeah. In my reading of your comment, I'm not sure it "takes further" in that sense. My finding was more niche (again, probably very hypothetical) — if only the latter call is in place, the data could be hijacked. Trying to make sure we're on the same page :)

I agree with the rest of your comment.

refactor, doc: Improve SetupAddressRelay call in version processing

This code was a bit hard to understand, so make it less dense and
add more explanations. Doesn't change behavior.

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>

956c67059c

mzumsande force-pushed on Nov 23, 2022

mzumsande commented at 10:12 PM on November 23, 2022: contributor

1002cd1 to 956c670: Applied suggestions by @amitiuttarwar to second commit - thanks!

do these claims track with your understanding?

yes, I agree!

the data could be hijacked.

I didn't understand this the first time, but I think I do now - you mean an attacker could continuously spam us with our own address from other connections (but with bad metadata), and thus prevent us from sending our self-advertisement (with correct metadata) to new peers because they'd inserted a duplicate in between our processing of the version messages and us getting to MaybeSendAddress().

I will have to think about this a bit more, but even if the timing issues and rate-limiting issues were solved, it would probably be really hard / impossible to that because of the way RelayAddress() relays a given address (no matter which peer it is received from) always to the same peers within 24 hours (which might change slightly with peers fluctuating, but not very much). It should be impossible for an attacker to make us relay a specific node in a targeted way to new nodes.

stratospher commented at 7:43 PM on November 24, 2022: contributor

Concept ACK. I went through the self advertisement logic and couldn't think of any concerning behavioral change this PR might induce. I do have a couple of questions though.

There were some entries in my node's debug.log where the peer would send incorrect entries(here, the peer's own address/they could be tampered with) for what address the peer thinks we are (vRecv >> addrMe).
- with the version processing mechanism, we'd self advertise our correct address at least once. (GetLocalAddress)
- with the MaybeSendAddr mechanism, we'd sometimes self advertise addrMe (what peer thinks we are/could be tampered with) only. (GetLocalAddrForPeer)
- The connections would last for only a short duration though(around 4 mins). Could this scenario be problematic?
even if we don't accept inbound connections, we still self-advertise. is it because we can't detect them?

mzumsande commented at 4:58 PM on November 28, 2022: contributor

There were some entries in my node's debug.log where the peer would send incorrect entries(here, the peer's own address/they could be tampered with) for what address the peer thinks we are (vRecv >> addrMe).

with the version processing mechanism, we'd self advertise our correct address at least once. (GetLocalAddress)

with the MaybeSendAddr mechanism, we'd sometimes self advertise addrMe (what peer thinks we are/could be tampered with) only. (GetLocalAddrForPeer)

The connections would last for only a short duration though(around 4 mins). Could this scenario be problematic?

I think that it's not possible to say in general which address (the local or peer's one) is better, or "correct". In some cases, the peer may have a better idea how we are reachable than we do. In other cases, it's the other way round, or maybe neither are working if we're just not reachable by others.

Also note that if we overrule our own local address and advertise with the address a peer sees us with, we'll only do that back to that same peer. We never send an addrMe address from one peer to other peers. Therefore, I think that peers can only influence the weight (nScore) between multiple local addresses (all of which we detected by ourselves at startup!) via SeenLocal() but never insert new ones that they pick - this should make it impossible to trick us into advertising an incorrect address to other peers. If a peer is malicious, they can just discard the self-announcements we send them anyway, so it's not an attack if such a peer makes us advertise to them with a wrong address.

even if we don't accept inbound connections, we still self-advertise. is it because we can't detect them?

If we explicitly disable inbound connections (fListen == false), we don't self-advertise, see here. But yes, it is a very common situation (e.g. if we're behind a NAT, or the port for bitcoin is blocked by our firewall) that bitcoind isn't able to detect that we're actually unreachable - so self-advertisements happen in this case.

amitiuttarwar approved

amitiuttarwar commented at 9:38 PM on November 30, 2022: contributor

@naumenkogs @mzumsande thanks for the feedback. I think I now better understand the hypothetical issue in terms of when we queue up the addresses internally, not when we actually trigger the message. due to the various mechanisms Martin mentioned, I think most viable attacks would be mitigated.

reACK 956c67059c

stratospher commented at 4:45 PM on December 1, 2022: contributor

ACK 956c670

fanquake requested review from naumenkogs on Dec 1, 2022

naumenkogs commented at 8:41 AM on December 2, 2022: member

ACK 956c67059caf3807b3ceacdd5de1caaae538f009

maflcko merged this on Dec 12, 2022

maflcko closed this on Dec 12, 2022

sidhujag referenced this in commit 6790331747 on Dec 12, 2022

mzumsande deleted the branch on Dec 12, 2022

bitcoin locked this on Dec 12, 2023