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 test_bank_forks_incremental_snapshot() #18565

Merged
merged 2 commits into from
Jul 29, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
295 changes: 275 additions & 20 deletions core/tests/snapshots.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ macro_rules! DEFINE_SNAPSHOT_VERSION_PARAMETERIZED_TEST_FUNCTIONS {
fn test_slots_to_snapshot() {
run_test_slots_to_snapshot(SNAPSHOT_VERSION, CLUSTER_TYPE)
}

#[test]
fn test_bank_forks_incremental_snapshot_n() {
run_test_bank_forks_incremental_snapshot_n(SNAPSHOT_VERSION, CLUSTER_TYPE)
}
}
};
}
Expand All @@ -39,6 +44,7 @@ mod tests {
use crossbeam_channel::unbounded;
use fs_extra::dir::CopyOptions;
use itertools::Itertools;
use log::{info, trace};
use solana_core::snapshot_packager_service::{PendingSnapshotPackage, SnapshotPackagerService};
use solana_gossip::{cluster_info::ClusterInfo, contact_info::ContactInfo};
use solana_runtime::{
Expand All @@ -57,7 +63,7 @@ mod tests {
use solana_sdk::{
clock::Slot,
genesis_config::{ClusterType, GenesisConfig},
hash::hashv,
hash::{hashv, Hash},
pubkey::Pubkey,
signature::{Keypair, Signer},
system_transaction,
Expand All @@ -66,6 +72,7 @@ mod tests {
use std::{
collections::HashSet,
fs,
io::{Error, ErrorKind},
path::PathBuf,
sync::{
atomic::{AtomicBool, Ordering},
Expand All @@ -83,8 +90,8 @@ mod tests {

struct SnapshotTestConfig {
accounts_dir: TempDir,
snapshot_dir: TempDir,
_snapshot_output_path: TempDir,
bank_snapshots_dir: TempDir,
snapshot_archives_dir: TempDir,
snapshot_config: SnapshotConfig,
bank_forks: BankForks,
genesis_config_info: GenesisConfigInfo,
Expand All @@ -94,11 +101,12 @@ mod tests {
fn new(
snapshot_version: SnapshotVersion,
cluster_type: ClusterType,
snapshot_interval_slots: u64,
accounts_hash_interval_slots: Slot,
snapshot_interval_slots: Slot,
) -> SnapshotTestConfig {
let accounts_dir = TempDir::new().unwrap();
let snapshot_dir = TempDir::new().unwrap();
let snapshot_output_path = TempDir::new().unwrap();
let bank_snapshots_dir = TempDir::new().unwrap();
let snapshot_archives_dir = TempDir::new().unwrap();
let mut genesis_config_info = create_genesis_config(10_000);
genesis_config_info.genesis_config.cluster_type = cluster_type;
let bank0 = Bank::new_with_paths(
Expand All @@ -114,21 +122,21 @@ mod tests {
);
bank0.freeze();
let mut bank_forks = BankForks::new(bank0);
bank_forks.accounts_hash_interval_slots = snapshot_interval_slots;
bank_forks.accounts_hash_interval_slots = accounts_hash_interval_slots;

let snapshot_config = SnapshotConfig {
snapshot_interval_slots,
snapshot_package_output_path: PathBuf::from(snapshot_output_path.path()),
snapshot_path: PathBuf::from(snapshot_dir.path()),
snapshot_package_output_path: snapshot_archives_dir.path().to_path_buf(),
snapshot_path: bank_snapshots_dir.path().to_path_buf(),
archive_format: ArchiveFormat::TarBzip2,
snapshot_version,
maximum_snapshots_to_retain: DEFAULT_MAX_FULL_SNAPSHOT_ARCHIVES_TO_RETAIN,
};
bank_forks.set_snapshot_config(Some(snapshot_config.clone()));
SnapshotTestConfig {
accounts_dir,
snapshot_dir,
_snapshot_output_path: snapshot_output_path,
bank_snapshots_dir,
snapshot_archives_dir,
snapshot_config,
bank_forks,
genesis_config_info,
Expand Down Expand Up @@ -185,9 +193,9 @@ mod tests {
.clone();
assert_eq!(*bank, deserialized_bank);

let bank_snapshot_infos = snapshot_utils::get_bank_snapshots(&snapshot_path);
let bank_snapshots = snapshot_utils::get_bank_snapshots(&snapshot_path);

for p in bank_snapshot_infos {
for p in bank_snapshots {
snapshot_utils::remove_bank_snapshot(p.slot, &snapshot_path).unwrap();
}
}
Expand All @@ -207,7 +215,12 @@ mod tests {
{
solana_logger::setup();
// Set up snapshotting config
let mut snapshot_test_config = SnapshotTestConfig::new(snapshot_version, cluster_type, 1);
let mut snapshot_test_config = SnapshotTestConfig::new(
snapshot_version,
cluster_type,
set_root_interval,
set_root_interval,
);

let bank_forks = &mut snapshot_test_config.bank_forks;
let mint_keypair = &snapshot_test_config.genesis_config_info.mint_keypair;
Expand Down Expand Up @@ -316,10 +329,11 @@ mod tests {
solana_logger::setup();

// Set up snapshotting config
let mut snapshot_test_config = SnapshotTestConfig::new(snapshot_version, cluster_type, 1);
let mut snapshot_test_config =
SnapshotTestConfig::new(snapshot_version, cluster_type, 1, 1);

let bank_forks = &mut snapshot_test_config.bank_forks;
let snapshots_dir = &snapshot_test_config.snapshot_dir;
let bank_snapshots_dir = &snapshot_test_config.bank_snapshots_dir;
let snapshot_config = &snapshot_test_config.snapshot_config;
let snapshot_path = &snapshot_config.snapshot_path;
let snapshot_package_output_path = &snapshot_config.snapshot_package_output_path;
Expand Down Expand Up @@ -438,9 +452,9 @@ mod tests {
// currently sitting in the channel
snapshot_utils::purge_old_bank_snapshots(snapshot_path);

let mut bank_snapshot_infos = snapshot_utils::get_bank_snapshots(&snapshots_dir);
bank_snapshot_infos.sort_unstable();
assert!(bank_snapshot_infos
let mut bank_snapshots = snapshot_utils::get_bank_snapshots(&bank_snapshots_dir);
bank_snapshots.sort_unstable();
assert!(bank_snapshots
.into_iter()
.map(|path| path.slot)
.eq(3..=snapshot_utils::MAX_BANK_SNAPSHOTS as u64 + 2));
Expand Down Expand Up @@ -541,7 +555,8 @@ mod tests {
let mut snapshot_test_config = SnapshotTestConfig::new(
snapshot_version,
cluster_type,
(*add_root_interval * num_set_roots * 2) as u64,
(*add_root_interval * num_set_roots * 2) as Slot,
(*add_root_interval * num_set_roots * 2) as Slot,
);
let mut current_bank = snapshot_test_config.bank_forks[0].clone();
let request_sender = AbsRequestSender::new(Some(snapshot_sender));
Expand Down Expand Up @@ -610,4 +625,244 @@ mod tests {
);
}
}

fn run_test_bank_forks_incremental_snapshot_n(
snapshot_version: SnapshotVersion,
cluster_type: ClusterType,
) {
solana_logger::setup();

const SET_ROOT_INTERVAL: Slot = 2;
const INCREMENTAL_SNAPSHOT_INTERVAL_SLOTS: Slot = SET_ROOT_INTERVAL * 2;
const FULL_SNAPSHOT_INTERVAL_SLOTS: Slot = INCREMENTAL_SNAPSHOT_INTERVAL_SLOTS * 5;
const LAST_SLOT: Slot = FULL_SNAPSHOT_INTERVAL_SLOTS * 2 - 1;

info!("Running bank forks incremental snapshot test, full snapshot interval: {} slots, incremental snapshot interval: {} slots, last slot: {}, set root interval: {} slots",
FULL_SNAPSHOT_INTERVAL_SLOTS, INCREMENTAL_SNAPSHOT_INTERVAL_SLOTS, LAST_SLOT, SET_ROOT_INTERVAL);

let mut snapshot_test_config = SnapshotTestConfig::new(
snapshot_version,
cluster_type,
SET_ROOT_INTERVAL,
FULL_SNAPSHOT_INTERVAL_SLOTS,
);
trace!("SnapshotTestConfig:\naccounts_dir: {}\nbank_snapshots_dir: {}\nsnapshot_archives_dir: {}", snapshot_test_config.accounts_dir.path().display(), snapshot_test_config.bank_snapshots_dir.path().display(), snapshot_test_config.snapshot_archives_dir.path().display());

let bank_forks = &mut snapshot_test_config.bank_forks;
let mint_keypair = &snapshot_test_config.genesis_config_info.mint_keypair;

let (snapshot_request_sender, snapshot_request_receiver) = unbounded();
let (accounts_package_sender, _accounts_package_receiver) = channel();
let request_sender = AbsRequestSender::new(Some(snapshot_request_sender));
let snapshot_request_handler = SnapshotRequestHandler {
snapshot_config: snapshot_test_config.snapshot_config.clone(),
snapshot_request_receiver,
accounts_package_sender,
};

let mut last_full_snapshot_slot = None;
for slot in 1..=LAST_SLOT {
// Make a new bank and perform some transactions
let bank = {
let bank = Bank::new_from_parent(&bank_forks[slot - 1], &Pubkey::default(), slot);

let key = Keypair::new().pubkey();
let tx = system_transaction::transfer(mint_keypair, &key, 1, bank.last_blockhash());
assert_eq!(bank.process_transaction(&tx), Ok(()));

let key = Keypair::new().pubkey();
let tx = system_transaction::transfer(mint_keypair, &key, 0, bank.last_blockhash());
assert_eq!(bank.process_transaction(&tx), Ok(()));

while !bank.is_complete() {
bank.register_tick(&Hash::new_unique());
}

bank_forks.insert(bank)
};

// Set root to make sure we don't end up with too many account storage entries
// and to allow snapshotting of bank and the purging logic on status_cache to
// kick in
if slot % SET_ROOT_INTERVAL == 0 {
// set_root sends a snapshot request
bank_forks.set_root(bank.slot(), &request_sender, None);
bank.update_accounts_hash();
snapshot_request_handler.handle_snapshot_requests(false, false, false, 0);
}

// Since AccountsBackgroundService isn't running, manually make a full snapshot archive
// at the right interval
if slot % FULL_SNAPSHOT_INTERVAL_SLOTS == 0 {
make_full_snapshot_archive(&bank, &snapshot_test_config.snapshot_config).unwrap();
last_full_snapshot_slot = Some(slot);
}
// Similarly, make an incremental snapshot archive at the right interval, but only if
// there's been at least one full snapshot first, and a full snapshot wasn't already
// taken at this slot.
//
// Then, after making an incremental snapshot, restore the bank and verify it is correct
else if slot % INCREMENTAL_SNAPSHOT_INTERVAL_SLOTS == 0
&& last_full_snapshot_slot.is_some()
&& slot != last_full_snapshot_slot.unwrap()
{
make_incremental_snapshot_archive(
&bank,
last_full_snapshot_slot.unwrap(),
&snapshot_test_config.snapshot_config,
)
.unwrap();

restore_from_incremental_snapshot_and_check_banks_are_equal(
&bank,
last_full_snapshot_slot.unwrap(),
&snapshot_test_config.snapshot_config,
snapshot_test_config.accounts_dir.path().to_path_buf(),
&snapshot_test_config.genesis_config_info.genesis_config,
)
.unwrap();
}
}
}

fn make_full_snapshot_archive(
bank: &Bank,
snapshot_config: &SnapshotConfig,
) -> snapshot_utils::Result<()> {
let slot = bank.slot();
info!("Making full snapshot archive from bank at slot: {}", slot);
let bank_snapshots = snapshot_utils::get_bank_snapshots(&snapshot_config.snapshot_path)
.into_iter()
.find(|elem| elem.slot == slot)
.ok_or_else(|| Error::new(ErrorKind::Other, "did not find snapshot with this path"))?;
let snapshot_package = snapshot_utils::package_full_snapshot(
bank,
&bank_snapshots,
&snapshot_config.snapshot_path,
bank.src.slot_deltas(&bank.src.roots()),
&snapshot_config.snapshot_package_output_path,
bank.get_snapshot_storages(),
snapshot_config.archive_format,
snapshot_config.snapshot_version,
None,
)?;
let snapshot_package = snapshot_utils::process_accounts_package_pre(
snapshot_package,
Some(bank.get_thread_pool()),
None,
);
snapshot_utils::archive_snapshot_package(
&snapshot_package,
snapshot_config.maximum_snapshots_to_retain,
)
}

fn make_incremental_snapshot_archive(
bank: &Bank,
incremental_snapshot_base_slot: Slot,
snapshot_config: &SnapshotConfig,
) -> snapshot_utils::Result<()> {
let slot = bank.slot();
info!(
"Making incremental snapshot archive from bank at slot: {}, and base slot: {}",
slot, incremental_snapshot_base_slot,
);
let bank_snapshots = snapshot_utils::get_bank_snapshots(&snapshot_config.snapshot_path)
.into_iter()
.find(|elem| elem.slot == slot)
.ok_or_else(|| Error::new(ErrorKind::Other, "did not find snapshot with this path"))?;
let incremental_snapshot_package = snapshot_utils::package_incremental_snapshot(
bank,
incremental_snapshot_base_slot,
&bank_snapshots,
&snapshot_config.snapshot_path,
bank.src.slot_deltas(&bank.src.roots()),
&snapshot_config.snapshot_package_output_path,
bank.get_incremental_snapshot_storages(incremental_snapshot_base_slot),
snapshot_config.archive_format,
snapshot_config.snapshot_version,
None,
)?;
let incremental_snapshot_package = snapshot_utils::process_accounts_package_pre(
incremental_snapshot_package,
Some(bank.get_thread_pool()),
Some(incremental_snapshot_base_slot),
);
snapshot_utils::archive_snapshot_package(
&incremental_snapshot_package,
snapshot_config.maximum_snapshots_to_retain,
)
}
brooksprumo marked this conversation as resolved.
Show resolved Hide resolved

fn restore_from_incremental_snapshot_and_check_banks_are_equal(
bank: &Bank,
last_full_snapshot_slot: Slot,
snapshot_config: &SnapshotConfig,
accounts_dir: PathBuf,
genesis_config: &GenesisConfig,
) -> snapshot_utils::Result<()> {
let (
full_snapshot_archive_slot,
(incremental_snapshot_archive_base_slot, incremental_snapshot_archive_slot),
deserialized_bank,
) = restore_from_incremental_snapshot(snapshot_config, accounts_dir, genesis_config)?;

assert_eq!(
full_snapshot_archive_slot,
incremental_snapshot_archive_base_slot
);
assert_eq!(full_snapshot_archive_slot, last_full_snapshot_slot);
assert_eq!(incremental_snapshot_archive_slot, bank.slot(),);
assert_eq!(*bank, deserialized_bank);

Ok(())
}

fn restore_from_incremental_snapshot(
snapshot_config: &SnapshotConfig,
accounts_dir: PathBuf,
genesis_config: &GenesisConfig,
) -> snapshot_utils::Result<(Slot, (Slot, Slot), Bank)> {
let full_snapshot_archive_info = snapshot_utils::get_highest_full_snapshot_archive_info(
&snapshot_config.snapshot_package_output_path,
)
.ok_or_else(|| Error::new(ErrorKind::Other, "no full snapshot"))?;

let incremental_snapshot_archive_info =
snapshot_utils::get_highest_incremental_snapshot_archive_info(
&snapshot_config.snapshot_package_output_path,
*full_snapshot_archive_info.slot(),
)
.ok_or_else(|| Error::new(ErrorKind::Other, "no incremental snapshot"))?;

info!("Restoring bank from full snapshot slot: {}, and incremental snapshot slot: {} (with base slot: {})",
full_snapshot_archive_info.slot(), incremental_snapshot_archive_info.slot(), incremental_snapshot_archive_info.base_slot());

let (deserialized_bank, _) = snapshot_utils::bank_from_snapshot_archives(
&[accounts_dir],
&[],
&snapshot_config.snapshot_path,
full_snapshot_archive_info.path(),
Some(incremental_snapshot_archive_info.path()),
snapshot_config.archive_format,
genesis_config,
None,
None,
AccountSecondaryIndexes::default(),
false,
None,
accounts_db::AccountShrinkThreshold::default(),
false,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@carllin carllin Jul 27, 2021

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

false,
)?;

Ok((
*full_snapshot_archive_info.slot(),
(
*incremental_snapshot_archive_info.base_slot(),
*incremental_snapshot_archive_info.slot(),
),
deserialized_bank,
))
}
}
Loading