p2p: Improve diversification of new connections

stratospher commented at 4:04 pm on March 15, 2023: contributor

Revives #19860.

In order to make sure that our persistent outbound slots belong to different netgroups, distinct net groups of our peers are added to setConnected. We’d only open a persistent outbound connection to peers which have a different netgroup compared to those netgroups present in setConnected.

behaviour on master

we open persistent outbound connections to peers which have different netgroups compared to outbound full relay, block relay, addrfetch and feeler connection peers.

behaviour on PR

netgroup diversity is based on outbound full relay, block relay and manual connection peers.

rationale

addrfetch and feeler connections are short lived connections and shouldn’t affect how we select outbound peers from addrman.
manual connections are like regular connections when viewed from addrman’s netgroup diversity point of view and should affect how we select outbound peers from addrman

p2p: Diversify connections only w.r.t *persistent* outbound peers

ADDR_FETCH and FEELER are short-lived connections,
and they should not affect our choice of peers.

Also, improve comments.

3faae99c3d

p2p: Account for MANUAL conns when diversifying persistent outbound conns

Previously, we would make connections to peer from the netgroups to which
our MANUAL outbound connections belong.
However, they should be seen as regular connections from Addrman when it comes to netgroup diversity check, since the same rationale can be applied.

Note, this has nothing to do with how we connect to MANUAL connections:
we connect to them unconditionally.

72e8ffd7f8

DrahtBot commented at 4:04 pm on March 15, 2023: 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	vasild, mzumsande, brunoerg, amitiuttarwar
Concept ACK	sdaftuar

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:

#27257 (refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg by dergoegge)
#24170 (p2p, rpc: Manual block-relay-only connections with addnode by mzumsande)

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 Mar 15, 2023

vasild approved

vasild commented at 4:39 pm on March 15, 2023: contributor

ACK 72e8ffd7f8dbf908e65da6d012ede914596737ab

IMO the last commit from #19860 can also be brought up: doc: peer selection rules do not apply to MANUAL and ADDR_FETCH

stratospher commented at 6:59 pm on March 15, 2023: contributor

I wasn’t able to answer this #19860 (review). (so didn’t include it) Do you(/anyone) know why that commit is needed?

vasild commented at 9:02 am on March 16, 2023: contributor

I guess to ensure that it was set according to the expectations and also to serve as a hint to the reader that it will never be those values. Usually such asserts are put right after setting the variable is finished. It is ok without it too. Up to you. Thanks!

mzumsande commented at 7:39 pm on March 16, 2023: contributor

Code Review ACK 72e8ffd7f8dbf908e65da6d012ede914596737ab

(I had already ack’ed #19860 ). I prefer the current version without the last commit of 19860. Even though the asserts are safe, they don’t seem necessary. In general, I think we should use asserts very sparingly in p2p code (only in situations where not crashing would lead to serious problems), because of the risk of future remote crash bugs.

fanquake commented at 8:59 pm on March 16, 2023: member

cc @naumenkogs @amitiuttarwar @sdaftuar

brunoerg approved

brunoerg commented at 12:43 pm on March 17, 2023: contributor

crACK 72e8ffd7f8dbf908e65da6d012ede914596737ab

amitiuttarwar commented at 5:22 am on March 18, 2023: contributor

code review ACK 72e8ffd7f8dbf908e65da6d012ede914596737ab

izlan90 approved

sdaftuar commented at 3:08 pm on March 18, 2023: member

utACK

I was least sure about excluding netgroups that we have MANUAL connections to. As best as I can tell, @lightlike’s reason for doing so seems to be the strongest reason:

On the other hand, even if you do trust them, there could still be attacks on an ISP level that would apply to multiple nodes from one group, so I think it makes sense to include them in the diversification as well.

I think that makes sense to me. However, it occurred to me that for networks where we have authenticated connections – I2P, onion, CJDNS(?) – that this argument would not apply. Moreover I guess doing any kind of “netgroup”-based diversification for these peers doesn’t make sense, since for those addresses, our group assignments are arbitrary and there’s no relationship between the address and our route? (If that understanding is correct then that is a separate issue from the improvement here, so could be picked up in another PR.)

fanquake merged this on Mar 19, 2023

fanquake closed this on Mar 19, 2023

sidhujag referenced this in commit 52cafb0060 on Mar 19, 2023

vasild commented at 4:52 am on March 22, 2023: contributor

we have authenticated connections … CJDNS(?)

Yes, in CJDNS the address is (a representation) of the owner’s public key.

there’s no relationship between the address and our route?

My understanding is that this is true for Tor, I2P and CJDNS.

mzumsande commented at 2:42 pm on March 23, 2023: contributor

Moreover I guess doing any kind of “netgroup”-based diversification for these peers doesn’t make sense, since for those addresses, our group assignments are arbitrary and there’s no relationship between the address and our route? (If that understanding is correct then that is a separate issue from the improvement here, so could be picked up in another PR.)

I agree, and I think there is another problem that is exacerbated by this PR:

The grouping is not just arbitrary for these networks, it also prevents us from making connections to addresses from groups we are already connected to, which is a real issue here because there are only 16 possible netgroups for each of the non-clearnet networks (see NetGroupManager::GetGroup(), the code is not exactly trivial to understand though) So if we’re -onlynet=onion (or i2p, cjdns) and choose 8 manual connections, each from a different group, these count for setConnected after this PR. If we then make 8 full outbound connections we have one connection to each possible netgroup - so that we wouldn’t be able to make block-relay-only connections anymore! I think we should do something about that for 25.0, I believe @stratospher would be interested in doing that as a follow-up?

stratospher commented at 7:37 pm on March 23, 2023: contributor

yes! thanks @mzumsande, will address this in a follow-up PR.

update: opened #27374 to skip tor/i2p/cjdns addresses in setConnected for now.

vasild commented at 11:40 am on March 30, 2023: contributor

How? I opened #27369 to discuss ideas on how to approach this.

achow101 referenced this in commit 2bfe43db16 on Apr 13, 2023

bitcoin locked this on Mar 29, 2024

p2p: Improve diversification of new connections #27264

Reviews

Conflicts