Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eth/protocols/snap: fix batch writer when resuming an aborted sync #22

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

0xcb9ff9
Copy link

@0xcb9ff9 0xcb9ff9 commented Sep 1, 2023

Description

upstream PR: 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.

@0xcb9ff9 0xcb9ff9 added the upstream upstream PR label Sep 1, 2023
@0xcb9ff9 0xcb9ff9 self-assigned this Sep 1, 2023
Copy link
Collaborator

@DarianShawn DarianShawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I guess it's a mistaken description of its issue.
Iterating map in golang will make a random sequence, if we use the map in a closure without assigning it. Then re-sequence happen, and something already iterated might iterate again.

I think we'd better provide some experimental data to support our conclusions.

@0xcb9ff9
Copy link
Author

0xcb9ff9 commented Sep 4, 2023

Well, I guess it's a mistaken description of its issue. Iterating map in golang will make a random sequence, if we use the map in a closure without assigning it. Then re-sequence happen, and something already iterated might iterate again.

I think we'd better provide some experimental data to support our conclusions.

The point is not iterate map, the for loop bug golang/go#56010

@DarianShawn
Copy link
Collaborator

Well, I guess it's a mistaken description of its issue. Iterating map in golang will make a random sequence, if we use the map in a closure without assigning it. Then re-sequence happen, and something already iterated might iterate again.
I think we'd better provide some experimental data to support our conclusions.

The point is not iterate map, the for loop bug golang/go#56010

Alright, I misunderstood the issue.

Copy link
Collaborator

@DarianShawn DarianShawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DarianShawn DarianShawn force-pushed the upstream/27842-snapsync-batch-fix branch from 9b0f3bb to 08a3b43 Compare September 4, 2023 02:21
@DarianShawn DarianShawn merged commit 829b516 into dev Sep 4, 2023
@DarianShawn DarianShawn deleted the upstream/27842-snapsync-batch-fix branch September 4, 2023 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream upstream PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic: pebble: batch too large: >= 4.0 G
2 participants