eth/protocols/snap: fix batch writer when resuming an aborted sync #27842
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
|
Ah, the issue might be that the last batch finishes, and then will never get checked again, whilst the others are still pulling data and piling it into the last batch. |
DarianShawn
pushed a commit
to dogechain-lab/dbsc
that referenced
this pull request
Sep 4, 2023
### Description upstream PR: [go-ethereum#27842](ethereum/go-ethereum#27842) Fixes ethereum/go-ethereum#27813. The closure caused all subtasks to reference the same one batch when writing, so instead of each subtask of a contract growing and flushing it's own batch, they all grew the last one's, but flushed their own. Interestingly however, this should only be an issue if there's also a lack of peers, otherwise whenever a delivery into the last batch is made, it should have flushed out everyone else's data too.
devopsbo3
pushed a commit
to HorizenOfficial/go-ethereum
that referenced
this pull request
Nov 10, 2023
devopsbo3
added a commit
to HorizenOfficial/go-ethereum
that referenced
this pull request
Nov 10, 2023
… sync (ethereum#27842)" This reverts commit 5e11ddf.
devopsbo3
added a commit
to HorizenOfficial/go-ethereum
that referenced
this pull request
Nov 10, 2023
… sync (ethereum#27842)" This reverts commit 5e11ddf.
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.
Fixes #27813.
The closure caused all subtasks to reference the same one batch when writing, so instead of each subtask of a contract growing and flushing it's own batch, they all grew the last one's, but flushed their own.
Interestingly however, this should only be an issue if there's also a lack of peers, otherwise whenever a delivery into the last batch is made, it should have flushed out everyone else's data too.
Edit: Actually, it might happen that the last chunk finishes earlier than the rest, which will cause memory to keep piling there. This will result in the node growing in memory until the pivot moves and sync restarts. This was confirmed via a bench pair comparing master (yellow) with PR (green):
Additionally, this might also mean that all that extra memory that was piled into the last batch post-last-chunk-completion will actually get lost and the healer will need to redownload it. Waiting for benchmarkers to progress to the heal phase to verify.