p2p: Improve diversification of new connections #27264

stratospher · 2023-03-15T16:04:15Z

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

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

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

DrahtBot · 2023-03-15T16:04:18Z

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.

vasild

ACK 72e8ffd

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

stratospher · 2023-03-15T18:59:44Z

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

vasild · 2023-03-16T09:02:43Z

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

Code Review ACK 72e8ffd

(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 · 2023-03-16T20:59:16Z

cc @naumenkogs @amitiuttarwar @sdaftuar

brunoerg

crACK 72e8ffd

amitiuttarwar · 2023-03-18T05:22:02Z

code review ACK 72e8ffd

sdaftuar · 2023-03-18T15:08:52Z

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

vasild · 2023-03-22T04:52:08Z

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 · 2023-03-23T14:42:57Z

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 · 2023-03-23T19:37:25Z

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 · 2023-03-30T11:40:39Z

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

…ections for tor/i2p/cjdns b5585ba p2p: skip netgroup diversity of new connections for tor/i2p/cjdns networks (stratospher) Pull request description: Follow up for #27264. 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`. Current `GetGroup()` logic assumes route-based diversification behaviour for tor/i2p/cjdns addresses (addresses are public key based and not route-based). Distinct netgroups possible (according to the current `GetGroup()` logic) for: 1. tor => 030f, 031f, .. 03ff (16 possibilities) 2. i2p => 040f, 041f, .. 04ff (16 possibilities) 3. cjdns => 05fc0f, 05fc1f, ... 05fcff (16 possibilities) `setConnected` is used in `ThreadOpenConnections()` before making [outbound](https://github.com/bitcoin/bitcoin/blob/84f4ac39fda7ffa5dc84e92d92dd1eeeb5e20f8c/src/net.cpp#L1846) and [anchor](https://github.com/bitcoin/bitcoin/blob/84f4ac39fda7ffa5dc84e92d92dd1eeeb5e20f8c/src/net.cpp#L1805) connections to new peers so that they belong to distinct netgroups. **behaviour on master** - if we run a node only on tor/i2p/cjdns - we wouldn't be able to open more than 16 outbound connections(manual, block-relay-only anchor, outbound full relay, block-relay-only connections) because we run out of possible netgroups. - see bitcoin/bitcoin#27264 (comment) - tested by changing `MAX_OUTBOUND_FULL_RELAY_CONNECTIONS` to 17 with `onlynet=onion` and observed how node wouldn't make more than 16 outbound connections. **behaviour on PR** - netgroup diversity checks are skipped for tor/i2p/cjdns addresses. - we don't insert tor/i2p/cjdns address in `setConnected` and `GetGroup` doesn't get called on tor/i2p/cjdns(see #27369) ACKs for top commit: achow101: ACK b5585ba mzumsande: ACK b5585ba vasild: ACK b5585ba Tree-SHA512: c120b3f9ca7f0be3f29ea665cd2f7dfb40cd1d7ec7058984252fb6e0295e414f736c5b4fba03c31188188a5ae4f543fb2654f6ee9776bad745c7ca72d23d5b9b

naumenkogs added 2 commits March 15, 2023 20:12

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

3faae99

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

DrahtBot added the P2P label Mar 15, 2023

stratospher mentioned this pull request Mar 15, 2023

p2p: Improve diversification of new connections #19860

Closed

vasild approved these changes Mar 15, 2023

View reviewed changes

This was referenced Mar 16, 2023

refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg #27257

Merged

p2p, rpc: Manual block-relay-only connections with addnode #24170

Closed

mzumsande reviewed Mar 16, 2023

View reviewed changes

brunoerg approved these changes Mar 17, 2023

View reviewed changes

izlan90 approved these changes Mar 18, 2023

View reviewed changes

fanquake merged commit 053b2d3 into bitcoin:master Mar 19, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 19, 2023

Merge bitcoin#27264: p2p: Improve diversification of new connections

52cafb0

vasild mentioned this pull request Mar 30, 2023

Fix net grouping of non-IP networks #27369

Open

stratospher mentioned this pull request Mar 30, 2023

p2p: skip netgroup diversity of new connections for tor/i2p/cjdns #27374

Merged

rajarshimaitra mentioned this pull request Apr 10, 2023

p2p: skip netgroup diversity of new connections for tor/i2p/cjdns #27374 Bitshala/BitcoinCore-PR-Review-Club#23

Closed

bitcoin locked and limited conversation to collaborators Mar 29, 2024

stratospher deleted the p2p-diverse-new-conn branch July 17, 2024 06:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

p2p: Improve diversification of new connections #27264

p2p: Improve diversification of new connections #27264

Uh oh!

stratospher commented Mar 15, 2023

Uh oh!

DrahtBot commented Mar 15, 2023 •

edited

Loading

Uh oh!

vasild left a comment

Uh oh!

stratospher commented Mar 15, 2023

Uh oh!

vasild commented Mar 16, 2023

Uh oh!

mzumsande left a comment

Uh oh!

fanquake commented Mar 16, 2023

Uh oh!

brunoerg left a comment

Uh oh!

amitiuttarwar commented Mar 18, 2023

Uh oh!

sdaftuar commented Mar 18, 2023

Uh oh!

vasild commented Mar 22, 2023

Uh oh!

mzumsande commented Mar 23, 2023 •

edited

Loading

Uh oh!

stratospher commented Mar 23, 2023 •

edited

Loading

Uh oh!

vasild commented Mar 30, 2023

Uh oh!

Uh oh!

p2p: Improve diversification of new connections #27264

p2p: Improve diversification of new connections #27264

Uh oh!

Conversation

stratospher commented Mar 15, 2023

Uh oh!

DrahtBot commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

stratospher commented Mar 15, 2023

Uh oh!

vasild commented Mar 16, 2023

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Mar 16, 2023

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

amitiuttarwar commented Mar 18, 2023

Uh oh!

sdaftuar commented Mar 18, 2023

Uh oh!

vasild commented Mar 22, 2023

Uh oh!

mzumsande commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stratospher commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Mar 30, 2023

Uh oh!

Uh oh!

DrahtBot commented Mar 15, 2023 •

edited

Loading

mzumsande commented Mar 23, 2023 •

edited

Loading

stratospher commented Mar 23, 2023 •

edited

Loading