Add test_bank_forks_incremental_snapshot() #18565

brooksprumo · 2021-07-09T22:10:24Z

This commit builds on PR #18504 by adding a test to core/tests/snapshot.rs for Incremental Snapshots. The test adds banks to bank forks in a loop and takes both full snapshots and incremental snapshots at intervals, and validates they are rebuild-able.

For background info about Incremental Snapshots, see #17088.

codecov · 2021-07-12T15:44:11Z

Codecov Report

Merging #18565 (ba0f6dc) into master (d2d5f36) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #18565   +/-   ##
=======================================
  Coverage    82.8%    82.8%           
=======================================
  Files         443      443           
  Lines      126301   126318   +17     
=======================================
+ Hits       104675   104694   +19     
+ Misses      21626    21624    -2

stale · 2021-07-20T05:13:32Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

brooksprumo · 2021-07-24T14:23:04Z

@ryoqun @carllin @lijunwangs Here's the next PR for incremental snapshots. This one is just a test, and should be much easier to review. Thanks in advance!

core/tests/snapshots.rs

carllin · 2021-07-27T21:34:25Z

core/tests/snapshots.rs

+            false,
+            None,
+            accounts_db::AccountShrinkThreshold::default(),
+            false,


This flag to not check the hash was introduced here: https://github.com/solana-labs/solana/pull/17939/files#diff-b5dead0a38e5f63447595bbeda1428d3aa851d33954343bb1bc38b4b00add5fdR149, not sure why it's not safe to check the hash here.

Maybe the hash is verified elsewhere already? Would be good to check the accounts state is being verified to match the snapshot hash somewhere in the test.

heh, looks like in the normal path, the hash is verified after the call to bank_from_snapshot_archives()

solana/ledger/src/bank_forks_utils.rs

Lines 149 to 171 in 86bda02

if let Some(shrink_paths) = shrink_paths {

deserialized_bank.set_shrink_paths(shrink_paths);

}

let deserialized_bank_slot_and_hash = (

deserialized_bank.slot(),

deserialized_bank.get_accounts_hash(),

);

if deserialized_bank_slot_and_hash

!= (

*full_snapshot_archive_info.slot(),

*full_snapshot_archive_info.hash(),

)

{

error!(

"Snapshot has mismatch:\narchive: {:?}\ndeserialized: {:?}",

(

full_snapshot_archive_info.slot(),

full_snapshot_archive_info.hash()

),

deserialized_bank_slot_and_hash

);

.

Can we extract this piece of logic (the unpacking and the hash verify), into a separate function like deserialize_and_verify_bank and call that function from here

@carllin Sure thing. Can I break this out into a separate PR/issue instead of fixing inside this PR?

If extracting the above logic into a function and calling it from the test works, I think it makes sense to include in this PR, as testing the accounts hash seems important to the snapshot tests.

If somethings broken/hash doesn't verify, then a separate PR makes sense to me.

Carl and I talked. I've created a new issue to track this request: #18973.

…olana-labs#18972)

brooksprumo · 2021-07-29T19:19:11Z

@carllin I refactored the functions for package, process, and archive as per our call (and issue #18972). I'll handle the "verify" stuff (issue #18973) in a separate PR. So this PR should be good-to-go!

carllin

Thanks, looks much cleaner!

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.

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 `SnapshotArchiveInfo`s instead of separate paths and archive formats. This bundling simplifies bank rebuilding.

brooksprumo force-pushed the iss-v3-snapshot-tests branch from 2d001a3 to d0c3f66 Compare July 9, 2021 22:19

brooksprumo marked this pull request as ready for review July 12, 2021 15:59

brooksprumo marked this pull request as draft July 12, 2021 16:01

brooksprumo force-pushed the iss-v3-snapshot-tests branch from 7c86926 to b1eb040 Compare July 12, 2021 18:36

stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 20, 2021

brooksprumo force-pushed the iss-v3-snapshot-tests branch from b1eb040 to d13bf96 Compare July 20, 2021 14:59

stale bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 20, 2021

brooksprumo force-pushed the iss-v3-snapshot-tests branch from d13bf96 to aafec57 Compare July 21, 2021 22:45

brooksprumo linked an issue Jul 21, 2021 that may be closed by this pull request

[Incremental Snapshots] Add integration tests #18829

Closed

Add test_bank_forks_incremental_snapshot()

87a7667

brooksprumo force-pushed the iss-v3-snapshot-tests branch from f8ed393 to 87a7667 Compare July 23, 2021 15:32

brooksprumo marked this pull request as ready for review July 24, 2021 14:22

brooksprumo requested review from carllin, lijunwangs and ryoqun July 24, 2021 14:22

brooksprumo mentioned this pull request Jul 27, 2021

Handle cleaning of zero-lamport accounts w.r.t. Incremental Snapshots #18870

Merged

carllin reviewed Jul 27, 2021

View reviewed changes

core/tests/snapshots.rs Outdated Show resolved Hide resolved

carllin reviewed Jul 27, 2021

View reviewed changes

brooksprumo linked an issue Jul 29, 2021 that may be closed by this pull request

[Incremental Snapshots] Refactor the common code in snapshot_utils::bank_to_xxx_snapshot_archive() and core/tests/snapshots.rs::make_xxx_snapshot_archive() #18972

Closed

Add snapshot_utils::package_process_and_archive_xxx_snapshot() (Fixes s…

ba0f6dc

…olana-labs#18972)

brooksprumo requested a review from carllin July 29, 2021 19:19

carllin approved these changes Jul 29, 2021

View reviewed changes

brooksprumo merged commit b05fb87 into solana-labs:master Jul 29, 2021

brooksprumo deleted the iss-v3-snapshot-tests branch July 29, 2021 21:47

brooksprumo mentioned this pull request Jul 29, 2021

Add snapshot_utils::bank_from_latest_snapshot_archives() #18983

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test_bank_forks_incremental_snapshot() #18565

Add test_bank_forks_incremental_snapshot() #18565

brooksprumo commented Jul 9, 2021 •

edited

Loading

codecov bot commented Jul 12, 2021 •

edited

Loading

stale bot commented Jul 20, 2021

brooksprumo commented Jul 24, 2021

carllin Jul 27, 2021

carllin Jul 27, 2021 •

edited

Loading

brooksprumo Jul 28, 2021

carllin Jul 29, 2021

brooksprumo Jul 29, 2021

brooksprumo commented Jul 29, 2021

carllin left a comment

	if let Some(shrink_paths) = shrink_paths {
	deserialized_bank.set_shrink_paths(shrink_paths);
	}

	let deserialized_bank_slot_and_hash = (
	deserialized_bank.slot(),
	deserialized_bank.get_accounts_hash(),
	);

	if deserialized_bank_slot_and_hash
	!= (
	*full_snapshot_archive_info.slot(),
	*full_snapshot_archive_info.hash(),
	)
	{
	error!(
	"Snapshot has mismatch:\narchive: {:?}\ndeserialized: {:?}",
	(
	full_snapshot_archive_info.slot(),
	full_snapshot_archive_info.hash()
	),
	deserialized_bank_slot_and_hash
	);

Add test_bank_forks_incremental_snapshot() #18565

Add test_bank_forks_incremental_snapshot() #18565

Conversation

brooksprumo commented Jul 9, 2021 • edited Loading

codecov bot commented Jul 12, 2021 • edited Loading

Codecov Report

stale bot commented Jul 20, 2021

brooksprumo commented Jul 24, 2021

carllin Jul 27, 2021

Choose a reason for hiding this comment

carllin Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

brooksprumo Jul 28, 2021

Choose a reason for hiding this comment

carllin Jul 29, 2021

Choose a reason for hiding this comment

brooksprumo Jul 29, 2021

Choose a reason for hiding this comment

brooksprumo commented Jul 29, 2021

carllin left a comment

Choose a reason for hiding this comment

brooksprumo commented Jul 9, 2021 •

edited

Loading

codecov bot commented Jul 12, 2021 •

edited

Loading

carllin Jul 27, 2021 •

edited

Loading