This repository was archived by the owner on Jan 22, 2025. It is now read-only.
Propagate dead slots up to replay#17227
Merged
carllin merged 3 commits intosolana-labs:masterfrom May 25, 2021
Merged
Conversation
carllin
force-pushed
the
FixBlockstoreProcessor
branch
2 times, most recently
from
abdef06 to
ce586c8
Compare
Codecov Report
@@ Coverage Diff @@
## master #17227 +/- ##
=======================================
Coverage 82.7% 82.7%
=======================================
Files 425 425
Lines 118809 118803 -6
=======================================
+ Hits 98265 98270 +5
+ Misses 20544 20533 -11 |
ryoqun
reviewed
May 20, 2021
Comment on lines
2758
to
2759
| let valid_next_slots: Vec<u64> = meta.next_slots.to_vec(); | ||
| (*height, valid_next_slots) |
Contributor
There was a problem hiding this comment.
nit: fold into (*height, meta.next_slots.to_vec())?
ryoqun
reviewed
May 20, 2021
Comment on lines
955
to
964
| // Block must be frozen by this point, otherwise `process_single_slot` would | ||
| // have errored above |
Contributor
There was a problem hiding this comment.
nits: // Bank must be frozen by this point because any errors should have been bailed out above.
ryoqun
reviewed
May 20, 2021
| @@ -2755,12 +2755,7 @@ impl Blockstore { | |||
| .zip(slot_metas) | |||
Contributor
There was a problem hiding this comment.
it looks like get_slots_since isn't used for rpc. So, it's only used by replay stage.
ryoqun
reviewed
May 20, 2021
ryoqun
reviewed
May 20, 2021
| ); | ||
| // Insert even dead banks into the BankForks so that cleanup | ||
| // of account state created on Bank creation via `Bank::new_from_parent()` | ||
| // will occur when the bank is dropped. Otherwise, this state is not |
Contributor
There was a problem hiding this comment.
lazy question: So, purge_slot() called inside Bank::drop() isn't enough for cleanup of account state?
Contributor
Author
ryoqun
reviewed
May 20, 2021
ryoqun
reviewed
May 20, 2021
| // 1) Validator can't vote on earlier ancestor of last vote due to switch threshold (can't vote | ||
| // on ancestors of last vote) | ||
| // 2) Won't reset to this earlier ancestor becasue reset can only happen on same voted fork if | ||
| // it's for the last vote slot or later |
carllin
force-pushed
the
FixBlockstoreProcessor
branch
from
ce586c8 to
74d7f29
Compare
carllin
force-pushed
the
FixBlockstoreProcessor
branch
from
74d7f29 to
2d1be87
Compare
jstarry
approved these changes
May 25, 2021
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
Problem
The state machine
cluster_slot_state_verifierin replay needs dead slots in the form of banks to make decisions about duplicate slot (detecting a confirmed version, dumping their descendants, repairing the correct version, etc.). However, currentlyget_slots_since()currently ignores dead slots if they were marked dead from a previous replay (and then the validator restarted for instance), so those slots never become banks.Another issue is blockstore processor currently replays banks, but if they error/are marked dead, it continues here:
solana/ledger/src/blockstore_processor.rs
Line 951 in 6e9deaf
initial_forksvia the following callsolana/ledger/src/blockstore_processor.rs
Line 1027 in 6e9deaf
BankForks. However, the call tonew_from_parent()while constructing these dead banks already modified the account state by adding sysvars to the accounts state for that dead slot.This inconsistency means if we then try to reconstruct the dead banks during replay via another call to
new_from_parent(), we encounter errors/conflicts with that existing state.Summary of Changes
Blockstore processor now returns the tips of dead slots unless they have been outdated by a root, and then clean will sweep up all the dead state once a new root is set
Fixes #