wen_restart: Find the bank hash of the heaviest fork, replay if necessary. #420
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #420 +/- ##
========================================
Coverage 81.8% 81.8%
========================================
Files 848 848
Lines 229160 229292 +132
========================================
+ Hits 187547 187681 +134
+ Misses 41613 41611 -2 |
ledger/src/blockstore_processor.rs
Outdated
| assert_eq!(bank_forks.read().unwrap().banks().len(), 1); | ||
| if !opts.skip_bankforks_checks_for_wen_restart { | ||
| assert_eq!(bank_forks.read().unwrap().banks().len(), 1); | ||
| } |
There was a problem hiding this comment.
This doesn't seem like the right entrypoint for the wen restart logic, hence the need to hack this flag.
Among other things, process_blockstore_from_root() will play every possible fork in blockstore, we just want to play the slots in a given list (the ones we repaired).
I think we should have a separate function that just calls process_single_slot() on every one of the repaired slots, should be much cleaner and simpler than repurposing this function
There was a problem hiding this comment.
Hmm, re-reading Steve's comments, actually this will cause a problem, it may insert already present blocks. I'll change it, hold on.
There was a problem hiding this comment.
Re-checked, actually using process_single_slot is a good idea, I've changed it. Ready for another look.
ledger/src/blockstore_processor.rs
Outdated
| // Normally when we process blocks in blockstore we start from a snapshot where the bankforks only holds | ||
| // one bank. But during wen_restart we may need to replay the new blocks repaired, and the bankforks may | ||
| // hold any number of frozen banks. |
There was a problem hiding this comment.
Isn't the find heaviest bank logic only run a single time after all the forks are already repaired, and we pass in an empty bank forks?
There was a problem hiding this comment.
The bank forks is reconstructed before entering wait_for_wenrestart, actually it's the result of process_blockstore_from_root() upon restart, so at this point the bank forks should be populated with banks already on the validator.
…latter may re-insert banks already frozen.
There was a problem hiding this comment.
I know you asked me about the blockstore_processor stuff specifically. With the current state of things, I think you found the right entrypoint to utilize to strike the balance between code re-use and having separate entrypoints for different purposes (ie not trying to overload process_blockstore_from_root()
// When counting Heaviest Fork, only count those with no less than
// 67% - 5% - (100% - active_stake) = active_stake - 38% stake.
const HEAVIEST_FORK_THRESHOLD_DELTA: f64 = 0.38;
As someone who isn't actively working on consensus, I think this comment could benefit of an explanation of where the various terms (67 & 5) come from. Also, are these constants declared / duplicated anywhere else in the codebase ?
wen-restart/src/wen_restart.rs
Outdated
|
|
||
| let leader_schedule_cache = LeaderScheduleCache::new_from_bank(&root_bank); | ||
| let replay_tx_thread_pool = rayon::ThreadPoolBuilder::new() | ||
| .num_threads(1) |
There was a problem hiding this comment.
I think you probably want to bump this thread count up:
- I believe this will get called at a time when the system is otherwise not doing much (ie regular block production has stopped) so higher amount of parallelism is fine
- For replaying full blocks, there is definite benefit to multiple threads. All of the entry verification (PoH and tx verification) can occur in parallel, and blockstore_processor will attempt to replay tx's in parallel as well
There was a problem hiding this comment.
That's fair, I've changed it to default number of threads instead, which should be equal to the number of logical threads.
| @@ -62,6 +70,9 @@ impl std::fmt::Display for WenRestartError { | |||
| WenRestartError::BlockNotFound(slot) => { | |||
| write!(f, "Block not found: {}", slot) | |||
| } | |||
| WenRestartError::BlockNotFrozenAfterReplay(slot, err) => { | |||
There was a problem hiding this comment.
Outside of the scope of this PR but mentioning it because I see it, but this error types looks like a good candidate for thiserror. This crate allows you to avoid having to write the custom formatter here:
https://docs.rs/thiserror/latest/thiserror/
Here is an example of an error type that I converted to use thiserror fairly recently:
agave/ledger/src/bank_forks_utils.rs
Lines 34 to 63 in c59143b
There was a problem hiding this comment.
We did thiserror a while ago, but decided to switch to anyhow because we wanted backtrace:
solana-labs#33892 (comment)
Could I do thiserror and enable backtrace?
There was a problem hiding this comment.
Oh neat, I didn't know about anyhow / backtracing functionality, and didn't really notice that is what is in use in this crate.
Could I do thiserror and enable backtrace?
Good question and I'm not sure, would need to dig around or try a quick experiment
| if exit.load(Ordering::Relaxed) { | ||
| return Err(WenRestartError::Exiting.into()); | ||
| } | ||
| if let Ok(Some(block_meta)) = blockstore.meta(slot) { | ||
| if let Ok(Some(block_meta)) = blockstore.meta(*slot) { |
There was a problem hiding this comment.
Not sure if it is an unrecoverable error, but since we're already pulling the SlotMeta out, we could also check if the slot is full. Unless we'll be actively repairing / inserting shreds at this point in time, but this could be one of the causes of a bank not being frozen after replay below
wen-restart/src/wen_restart.rs
Outdated
| let recyclers = VerifyRecyclers::default(); | ||
| let mut timing = ExecuteTimings::default(); | ||
| let opts = ProcessOptions::default(); | ||
| // Grab a big lock because we are the only person touching bankforks now. |
There was a problem hiding this comment.
big ? I guess big in the sense that we're holding it for a while?
Changed. |
wen-restart/src/wen_restart.rs
Outdated
| for slot in &slots { | ||
| if exit.load(Ordering::Relaxed) { | ||
| return Err(WenRestartError::Exiting.into()); | ||
| } | ||
| if let Some(bank) = my_bankforks.get(*slot) { | ||
| if !bank.is_frozen() { | ||
| return Err(WenRestartError::BlockNotFrozenAfterReplay(*slot, None).into()); | ||
| } | ||
| if bank.parent_slot() != parent_slot { | ||
| return Err(WenRestartError::BlockNotLinkedToExpectedParent( | ||
| *slot, | ||
| Some(bank.parent_slot()), | ||
| parent_slot, | ||
| ) | ||
| .into()); | ||
| } | ||
| } | ||
| parent_slot = *slot; | ||
| } |
There was a problem hiding this comment.
This should be unncessary given the find_heaviest_fork() checks above
There was a problem hiding this comment.
Theoretically some dump and repair logic could go between and silently change things from under us?
There was a problem hiding this comment.
I thought this phase was only run after all slots had already been repaired?
There was a problem hiding this comment.
Yes, but could this happen?
repaired all -> entering find heaviest fork phase -> somehow a duplicate notification comes -> dump and repair -> oops some repaired block disappeared from under us.
There was a problem hiding this comment.
we should avoid duplicate notifications here, I think that would need to be handled outside this protocol
There was a problem hiding this comment.
Okay, removed this check, we can maybe stop duplicate notification in another PR.
Find the bank hash for the selected heaviest fork. Replay blocks if necessary and report error if any slot is missing.