eth, eth/downloader,p2p: reserve half peer slots for snap peers during snap sync #22171
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
holiman
requested review from
fjl,
karalabe,
rjl493456442 and
zsfelfoldi
as code owners
holiman
commented
Jan 14, 2021
eth/downloader/downloader.go
Outdated
| @@ -289,6 +289,10 @@ func (d *Downloader) Synchronising() bool { | |||
| return atomic.LoadInt32(&d.synchronising) > 0 | |||
| } | |||
|
|
|||
| func (d *Downloader) SnapsyncInProgress() bool{ | |||
| return d.snapSync && d.Synchronising() | |||
There was a problem hiding this comment.
Hm, this probably won't work, becase synchronizing is only set after we find a peer to start syncing against
There was a problem hiding this comment.
Checking the d.snapSync should be sufficient, it's just that we currently don't set it to 0 after the sync is done
|
Deployed this on => 25 (ish) non-snap peers |
|
One snap peer found ...but gone one second later |
Closed
|
It finished eventually |
|
Later on, after the sync is done, we're back at |
karalabe
reviewed
Jan 25, 2021
eth/handler.go
Outdated
| if atomic.LoadUint32(&h.snapSync) == 1 && !peer.SupportsCap("snap", 1) { | ||
| // If we are running snap-sync, we want to reserve half the peer slots for | ||
| // peers supporting the snap protocol | ||
| reserved = h.maxPeers/2 - len(h.peers.snapPeers) |
There was a problem hiding this comment.
peers needs a lock before you can access an internal field. We already have a Len for the eth peers, we can add a second one for the snap peers.
karalabe
reviewed
Jan 25, 2021
| } | ||
| if reserved < 0 { | ||
| reserved = 0 | ||
| } |
There was a problem hiding this comment.
I like it that the solution is short and sweet, but there's a cornercase we need to support. Currently the P2P layer splits the allowed peer count into 2, reserving half for inbound and half for outbound connections. If my node is NATed, I won't get any inbound, so I'll use up 1/2 of my slots outbound. The issue is that I'm able to saturate my outbound connections fully with eth only peers without triggering the above snap reservation.
One solution is that we reserve half of the inbound and half of the outbound connections separately for snap. That would solve the above mentioned issue, but it can still be a bit flaky if I only have 1/4 peer.
Another alternative could be to enforce the restriction based on the currently connection peer count. E.g. if peers.LenEth() > min(2 * peers.LenSnap(), 5), then reject. That would guarantee that we can always get some baseline number of eth peers, but above that, we're enforicng half and half.
There was a problem hiding this comment.
Not quite sure I understand... Do you mean like this?
snapPeers := h.peers.SnapLen()
ethPeers := h.peers.Len() - snapPeers // snap-peers are a subset of eth-peers
if ethPeers > 5 && ethPeers > 2 *snapPeers{
reject = true
}
In that case, it would always accept up to 5 non-snap peers, but would otherwise allow twice as many non-snap as snap.
Alternatively;
snapPeers := h.peers.SnapLen()
ethPeers := h.peers.Len() - snapPeers // snap-peers are a subset of eth-peers
if ethPeers > 5 && ethPeers > snapPeers{
reject = true
}
In that case, it would always accept up to 5 non-snap peers, but would otherwise allow only as many non-snap as snap.
There was a problem hiding this comment.
I guess a different method would be to "reject if non-snap peers are more than 5+snap-peers".
As it is currently (83a9567), going from 5 to you need 3 snap peers to join. before you allow the 6th non-snap peer.
The alternative would only require 1 snap-peer before the 6th is allowed in. And eventually, with 50 peers, you'll have something like 23 snap and 27 non-snap peers.
|
I made an implementation of what I think you meant, PTAL |
bulgakovk
pushed a commit
to bulgakovk/go-ethereum
that referenced
this pull request
Jan 26, 2021
…hereum#22171) * eth, p2p: reserve half peer slots for snap peers during snap sync * eth: less logging * eth: rework the eth/snap peer reservation logic * eth: rework the eth/snap peer reservation logic (again)
This was referenced Mar 9, 2021
9 tasks
9 tasks
This was referenced Sep 23, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR attempts to reseve half the peer slots for
snap/1peers, if we're actively trying to perform a snap sync.Some caveats
--snapshot, butsyncmode=fast, then it should do nothing--snapshot, andsyncmode=snap, but are done syncing, it should do nothing