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

core/state/snapshot: update generator marker in sync with flushes #21804

Merged
merged 1 commit into from
Nov 9, 2020

Conversation

karalabe
Copy link
Member

@karalabe karalabe commented Nov 9, 2020

This PR fixes a data corruption in the snapshot mechanism if it crashes mid-storage.

The generator works by iterating the state trie and pushing the account/storage leaf data directly to disk. To avoid thrashing the database, the generator accumulates writes into a batch and only flushes to disk after a certain threshold is exceeded. When the generator flushes to disk, it's internal progress marker is also updates so that it knows where to continue from if it's interrupted.

The bug was that the marker was not written to disk at the same time as the data batch, rather was kept in memory and only flushed on shutdown. This however means that in case of a crash (or Ctrl+C without waiting for graceful shutdown), if the generator was in progress, then the database would go out of sync, the marker being behind of the data already indexed.

Most of the time this was not noticeable as upon restart, the generator just picked up in it's stale position and reindexed the same data once again. The rare issue however is if the chain progresses before the restart and some of the indexed - but rewound - data slots become deleted. In that case, the generator will not realize that there is junk in the database and will only add the new data, but not delete the stale junk.

The fix is to ensure that every time new snapshot data is pushed to disk, the generator marker is also updated atomically in the same go. From a thrashing perspective this does incur some additional overhead since we need to bump the same database key over and over again on every flush, but it's a price we need to pay.

@karalabe karalabe added this to the 1.9.24 milestone Nov 9, 2020
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

LGTM

@karalabe karalabe merged commit 7c30f4d into ethereum:master Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants