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

Add snapshot_utils::bank_from_latest_snapshot_archives() #18983

Merged

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Jul 29, 2021

While reviewing PR #18565, as issue was brought up to refactor some code
around verifying the bank after rebuilding from snapshots. A new
top-level function has been added to get the latest snapshot archives
and load the bank then verify. Additionally, new tests have been
written and existing tests have been updated to use this new function.

Fixes #18973

While resolving the issue, it became clear there was some additional
low-hanging fruit this change enabled. Specifically, the functions
bank_to_xxx_snapshot_archive() now return their respective
SnapshotArchiveInfo. And on the flip side,
bank_from_snapshot_archives() now takes SnapshotArchiveInfos instead
of separate paths and archive formats. This bundling simplifies bank
rebuilding.

@codecov
Copy link

codecov bot commented Jul 30, 2021

Codecov Report

Merging #18983 (ffc397c) into master (592013e) will increase coverage by 0.0%.
The diff coverage is 86.1%.

@@           Coverage Diff            @@
##           master   #18983    +/-   ##
========================================
  Coverage    82.8%    82.9%            
========================================
  Files         449      449            
  Lines      128281   128344    +63     
========================================
+ Hits       106280   106410   +130     
+ Misses      22001    21934    -67     

@brooksprumo brooksprumo marked this pull request as ready for review July 30, 2021 01:24
@brooksprumo brooksprumo force-pushed the iss-bank-from-latest-snapshots branch from 908b478 to 09e8ae2 Compare August 4, 2021 20:11
lijunwangs
lijunwangs previously approved these changes Aug 5, 2021
Copy link
Contributor

@lijunwangs lijunwangs left a comment

Choose a reason for hiding this comment

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

Looks good to me overall.

core/tests/snapshots.rs Outdated Show resolved Hide resolved
runtime/src/snapshot_utils.rs Outdated Show resolved Hide resolved
runtime/src/snapshot_utils.rs Outdated Show resolved Hide resolved
@mergify mergify bot dismissed lijunwangs’s stale review August 5, 2021 23:09

Pull request has been modified.

@brooksprumo
Copy link
Contributor Author

@carllin @ryoqun Let me know if you'd like more time to review this PR. If yes, leave a comment here. If not, no worries! I won't merge before tomorrow mid-day.

Comment on lines 162 to 166
fn hash(&self) -> &Hash {
&self.inner.hash
}

fn _archive_format(&self) -> &ArchiveFormat {
fn archive_format(&self) -> &ArchiveFormat {
Copy link
Contributor

@carllin carllin Aug 6, 2021

Choose a reason for hiding this comment

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

Side note, I think a good way to reduce this code duplication between FullSnapshotArchiveInfo and IncrementalSnapshotArchiveInfo is to create a trait SnapshotArchiveInfo like so:

trait SnapshotArchiveInfo {
   // constructor 
    fn from_snapshot_archive_info(snapshot_archive_info: SnapshotArchiveInfo) -> Self;

    // getter
    fn snapshot_archive_info(&self) -> &SnapshotArchiveInfo;

  // default methods to avoid duplication
    fn hash(&self) -> &Hash {
        &self.snapshot_archive_info().hash
    }

    fn archive_format(&self) -> &ArchiveFormat {
      &self.snapshot_archive_info().archive_format
     }
}

and have FullSnapshotArchiveInfo and IncrementalSnapshotArchiveInfo implement this trait

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this idea! One thing is that the IncrementalSnapshotArchiveInfo needs a base_slot parameter too. Do you know if there's a way to handle this within the Trait?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm implemented the trait but without a constructor. I think for now that should be good.

The next thing I'm working on is adding in incremental snapshot support to the background services (AccountsBackgroundService, AcccountsHashVerifier, SnapshotPackagerService). I'll by building out the SnapshotArchiveInfo more. In particular, moving Full and Incremental under a single enum, so that way when a background service makes a package, it'll contain the info about full vs incremental, and it'll carry through to the next service.

So yeah, is this OK for now? I did change slot() and archive_format() to not return references anymore, since they are Copy and small, and always used as values anyway.

)
);

let (bank, timings) = bank_from_snapshot_archives(
Copy link
Contributor

@carllin carllin Aug 6, 2021

Choose a reason for hiding this comment

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

heh just clarifying, the real hashing/verification of the account state actually happens in here, when we call down to

verify_snapshot_bank() -> verify_bank_hash() -> verify_bank_hash_and_lamports() -> calculate_accounts_hash_helper_with_verify(), and we verify the
calculated_hash of the accounts state against the expected serialized bank_hashes in the snapshot

While reviewing PR solana-labs#18565, as issue was brought up to refactor some code
around verifying the bank after rebuilding from snapshots.  A new
top-level function has been added to get the latest snapshot archives
and load the bank then verify.  Additionally, new tests have been
written and existing tests have been updated to use this new function.

Fixes solana-labs#18973

While resolving the issue, it became clear there was some additional
low-hanging fruit this change enabled.  Specifically, the functions
`bank_to_xxx_snapshot_archive()` now return their respective
`SnapshotArchiveInfo`.  And on the flip side,
`bank_from_snapshot_archives()` now takes `SnapshotArchiveInfo`s instead
of separate paths and archive formats.  This bundling simplifies bank
rebuilding.
@brooksprumo brooksprumo force-pushed the iss-bank-from-latest-snapshots branch from 9191fef to 2cf7f4f Compare August 6, 2021 18:05
@brooksprumo brooksprumo requested a review from carllin August 6, 2021 20:37
carllin
carllin previously approved these changes Aug 6, 2021
@mergify mergify bot dismissed carllin’s stale review August 6, 2021 21:30

Pull request has been modified.

@brooksprumo brooksprumo merged commit 0089095 into solana-labs:master Aug 7, 2021
@brooksprumo brooksprumo deleted the iss-bank-from-latest-snapshots branch August 7, 2021 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants