Investigate/peering #582
Draft
Investigate/peering #582
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
|
@meowsbits what do you think of merging this PR and start using the snap DNS list? |
I thought this used to exist, but I don't see logs around bootnodes/discovery config anymore, so I'm adding them because I like verbose logging. Date: 2023-11-02 17:01:51-06:00 Signed-off-by: meows <b5c6@protonmail.com>
We have all.classic.blockd.info AND snap.classic.blockd.info, so let's use both of them in reasonable places. Date: 2023-11-02 17:02:45-06:00 Signed-off-by: meows <b5c6@protonmail.com>
Snap imports should happen below the pivot, and full import above the pivot. Normally, blocks are like 18M high and pivot blocks only a few (ie 128 blocks) below the chain head. In a special case, though, a peer can have a full block > 0 but < 128, say, 2 blocks, and we can try to sync from them. When doing so, our pivot will be at 0, and as such, the delegation fn splitAroundPivot will separate blocks AFTER the pivot (0) for FULL import, while blocks before the pivot -- which normally should be SnapImported -- are none, and so all blocks above 0 are imported with importBlockResults rather than commitSnapSyncData. This results in block 2 being considered (or marked) FULL at the blockchain level. Snap sync (as implemented) does not expect FULL blocks below the pivot, so it should be confused about whether its snap-syncing or full-syncing. Then, when and if rollbacks happen (eg b/c bad sync), it might be that the rollback under snap parameters does not adequately rollback those full-imported low blocks. Date: 2023-11-03 12:48:02-06:00 Signed-off-by: meows <b5c6@protonmail.com> eth/downloader: fix logic from last commit (bad return broke loop) Date: 2023-11-03 14:16:53-06:00 Signed-off-by: meows <b5c6@protonmail.com> eth/downloader: fix redundant condition (noop) Date: 2023-11-04 08:17:33-06:00 Signed-off-by: meows <b5c6@protonmail.com>
A checkpoint condition exists to prevent snap sync with peers having heads less than the checkpoint. ETC does not have checkpoints installed, so this is a noop. This patch adds a condition to prevent snap sync with peers reporting heads less than fsMinFull blocks to prevent edge case badnesses around rollbacks and pivot handling when the target peers has very low blocks (eg. 2). Date: 2023-11-06 09:15:50-07:00 Signed-off-by: meows <b5c6@protonmail.com>
This reverts commit 86f11f11167f54119fe0f7a294f1cf14c7be778e.
By installing a checkpoint we'll avoid some snap sync issues where we're attempting to sync with very low, unsynced, nodes and running into issues with pivoting to full sync too early because of that (or encountering bugs related to aborted sync rewinds). The current block number is 18_666_111, so the checkpoint block 18_186_239 is well below that; safely canonical. I just derived the checkpoint values manually, though if we get the les client or server working again we can theoretically use the API les.latestCheckpoint. Date: 2023-11-06 11:03:47-07:00 Signed-off-by: meows <b5c6@protonmail.com>
meowsbits
force-pushed
the
investigate/peering
branch
from
9bdf2fa
to
ec42a29
Compare
|
Thanks for the bump on this. I just rebased on master and pushed. There were 4 subsequent commits that I hadn't pushed, but now have. I'd like to freshen up on what's going on here and then I'll come back to ya, especially since it seems like we've addressed probably the brunt of the peering issues with your #611. |
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.
Reports like #567 and others in other channels (mostly Discord) suggest some kind of vague issue with peering. This is a WIP to explore the problem and potential solutions.