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

Squash supermajority root on blockstore replay at startup#11727

Merged
carllin merged 3 commits intosolana-labs:masterfrom
carllin:FixMetrics
Aug 21, 2020
Merged

Squash supermajority root on blockstore replay at startup#11727
carllin merged 3 commits intosolana-labs:masterfrom
carllin:FixMetrics

Conversation

@carllin
Copy link
Copy Markdown
Contributor

@carllin carllin commented Aug 20, 2020

Problem

After long periods of no progress, a single node can have a long ledger it needs to replay, which is prohibitively taxing on memory and compute

Summary of Changes

Squash banks that supermajority has rooted on replay at startup

Fixes #

@carllin carllin requested a review from mvines August 20, 2020 07:14
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 20, 2020

Codecov Report

Merging #11727 into master will increase coverage by 0.0%.
The diff coverage is 95.2%.

@@           Coverage Diff            @@
##           master   #11727    +/-   ##
========================================
  Coverage    82.0%    82.0%            
========================================
  Files         330      330            
  Lines       76742    76862   +120     
========================================
+ Hits        62970    63083   +113     
- Misses      13772    13779     +7     

mvines
mvines previously approved these changes Aug 20, 2020
Copy link
Copy Markdown
Contributor

@mvines mvines left a comment

Choose a reason for hiding this comment

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

Thanks! A backport to v1.3 would be very nice. Maybe not v1.2 though, let's let this bake on testnet

Comment thread ledger/src/blockstore_processor.rs Outdated
Comment thread ledger/src/blockstore_processor.rs Outdated
@sakridge
Copy link
Copy Markdown
Contributor

Is it the best way to do this? Seems like a lot to keep in sync with how roots are actually set on the replay side.

@carllin carllin added the v1.3 label Aug 20, 2020
Co-authored-by: Michael Vines <mvines@gmail.com>
@mergify mergify Bot dismissed mvines’s stale review August 20, 2020 17:39

Pull request has been modified.

@carllin
Copy link
Copy Markdown
Contributor Author

carllin commented Aug 20, 2020

@sakridge yeah I think it's ok because it's equivalent to restarting at a snapshot with a higher root than your local root.

With @ryoqun's persistent tower fixes, such scenarios will become safer for the cluster across restarts

Co-authored-by: Michael Vines <mvines@gmail.com>
@carllin carllin merged commit f7adb68 into solana-labs:master Aug 21, 2020
mergify Bot pushed a commit that referenced this pull request Aug 21, 2020
carllin added a commit that referenced this pull request Aug 21, 2020
mergify Bot added a commit that referenced this pull request Aug 21, 2020
…11765)

(cherry picked from commit f7adb68)

Co-authored-by: carllin <wumu727@gmail.com>
vote_state
.unwrap()
.root_slot
.map(|root_slot| (root_slot, stake))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@carllin Well is this ok? What if different vote account has same root_slot? I think it's silently overwritten when collect()-ing into roots? Or does collect() summing automatically for us in this case?

Copy link
Copy Markdown
Contributor Author

@carllin carllin Sep 17, 2020

Choose a reason for hiding this comment

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

@ryoqun that's a bug, it definitely doesn't sum it automatically. I forgot to update it when I switched the type to BTreeMap from Vec. Thanks for the catch!

@ryoqun ryoqun mentioned this pull request Sep 17, 2020
1 task
};

if let Some(new_root_bank) = new_root_bank {
*root = new_root_bank.slot();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Btw, how the non-empty slot interval (blockstore.max_root(), supermajority_root] is marked at rooted for the blockstore?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ryoqun it's currently not. We could, but I think the current implementation is equivalent to starting a snapshot at supermajority_root, in which case those rooted slots in the range (blockstore.max_root(), supermajority_root] won't be marked in blockstore anyways.

// If we've reached the last known root in blockstore, start looking
// for newer cluster confirmed roots
let new_root_bank = {
if *root == max_root {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(for the record:) this condition was wrong... #14557

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.

4 participants