Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Only serialize rooted append vecs#7281

Merged
sakridge merged 1 commit intosolana-labs:masterfrom
sakridge:snapshot-only-roots
Dec 5, 2019
Merged

Only serialize rooted append vecs#7281
sakridge merged 1 commit intosolana-labs:masterfrom
sakridge:snapshot-only-roots

Conversation

@sakridge
Copy link
Copy Markdown
Contributor

@sakridge sakridge commented Dec 5, 2019

Problem

Slots which are not rooted could end up in a snapshot and cause account mismatch issues since we mark each present slot as rooted when we ingest the snapshot.

Summary of Changes

Only add rooted append-vecs to the snapshot, for now skip any failures when we load the snapshot if an appendvec is in the index but not present as a file.

  • skipping the file on failure is not great, but when we serialize the storage entries, we don't know which slots are rooted. Find a better solution for that.

Fixes #7244

@sakridge sakridge force-pushed the snapshot-only-roots branch from 99c62d6 to 0031882 Compare December 5, 2019 00:36
@sakridge sakridge requested a review from ryoqun December 5, 2019 00:36
@sakridge sakridge force-pushed the snapshot-only-roots branch 2 times, most recently from 184f56e to d379ab2 Compare December 5, 2019 00:40
@sakridge
Copy link
Copy Markdown
Contributor Author

sakridge commented Dec 5, 2019

@ryoqun this is just kind of a guess, let me look at your snapshot file, those accounts should be present in the older fork as well for this to make sense.

@ryoqun
Copy link
Copy Markdown
Contributor

ryoqun commented Dec 5, 2019

@ryoqun this is just kind of a guess, let me look at your snapshot file, those accounts should be present in the older fork as well for this to make sense.

@sakridge Sure! I've uploaded here Could you use this to confirm?

@mvines mvines added the v0.21 label Dec 5, 2019
@sakridge sakridge force-pushed the snapshot-only-roots branch from d379ab2 to 3e25a8d Compare December 5, 2019 01:07
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 5, 2019

Codecov Report

Merging #7281 into master will decrease coverage by 4.5%.
The diff coverage is 77%.

@@           Coverage Diff            @@
##           master   #7281     +/-   ##
========================================
- Coverage    73.2%   68.6%   -4.6%     
========================================
  Files         240     237      -3     
  Lines       50234   52368   +2134     
========================================
- Hits        36800   35965    -835     
- Misses      13434   16403   +2969

@sakridge sakridge force-pushed the snapshot-only-roots branch from 3e25a8d to 9463125 Compare December 5, 2019 06:05
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

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

Just viewed the code. Really thanks for proposing PRs quickly once after I find the bugs!! The impl code looks nice for now.

Only add rooted append-vecs to the snapshot, for now skip any failures when we load the snapshot if an appendvec is in the index but not present as a file.

As you said, this might not be elegant, but it'll work. Maybe put empty files in place for skipped AppendVecs and skip empty files when ingesting snapshot. That might be enough. We'd want to land this rather quickly. :)

@sakridge
Copy link
Copy Markdown
Contributor Author

sakridge commented Dec 5, 2019

Just viewed the code. Really thanks for proposing PRs quickly once after I find the bugs!! The impl code looks nice for now.

Only add rooted append-vecs to the snapshot, for now skip any failures when we load the snapshot if an appendvec is in the index but not present as a file.

As you said, this might not be elegant, but it'll work. Maybe put empty files in place for skipped AppendVecs and skip empty files when ingesting snapshot. That might be enough. We'd want to land this rather quickly. :)

I agree, I think we should just land this and then the cleanup could be a follow-up PR.

@sakridge sakridge marked this pull request as ready for review December 5, 2019 18:00
@sakridge sakridge merged commit cfc21e1 into solana-labs:master Dec 5, 2019
@sakridge sakridge deleted the snapshot-only-roots branch December 5, 2019 22:27
mergify Bot pushed a commit that referenced this pull request Dec 5, 2019
solana-grimes pushed a commit that referenced this pull request Dec 5, 2019
@ryoqun
Copy link
Copy Markdown
Contributor

ryoqun commented Dec 5, 2019

@sakridge Thanks for merging and backporting! I'll build this locally and will run against tds to see snapshot errors disappear really. :)

@sakridge
Copy link
Copy Markdown
Contributor Author

sakridge commented Dec 6, 2019

@sakridge Thanks for merging and backporting! I'll build this locally and will run against tds to see snapshot errors disappear really. :)

Thanks! I'm crossing my fingers for good luck :)

@ryoqun
Copy link
Copy Markdown
Contributor

ryoqun commented Jun 16, 2020

[ ] skipping the file on failure is not great, but when we serialize the storage entries, we don't know which slots are rooted. Find a better solution for that.

@sakridge Finally, this is now addressed at #10580. (FYI: @svenski123)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sporadic snapshot verification error

3 participants