From a18debc34a4919c91ec5b7b007afc8d4ab497949 Mon Sep 17 00:00:00 2001 From: "Jeff Washington (jwash)" Date: Fri, 27 Oct 2023 07:14:05 -0700 Subject: [PATCH] allow test feature to skip rewrites (#33851) * allow test feature to skip rewrites * hook up cli arg for test skip rewrites, update tests * fix sanity checker * add account hash to abi to fix a test * reviews * use hashmap to collect skip_rewrites. exclude skip_rewrites from dirty pubkey set * accumulate skipped_rewrite in reduce * mutex * fmt * skip hash verify for this test flag * add skipped rewrites num stat * skip bank hash verify not account hash verify * reviews --------- Co-authored-by: HaoranYi --- accounts-db/src/accounts_db.rs | 42 +++++++++++++++++--- accounts-db/src/accounts_hash.rs | 2 +- ledger-tool/src/args.rs | 2 + ledger-tool/src/main.rs | 13 ++++++ runtime/src/bank.rs | 68 +++++++++++++++++++++++++++----- validator/src/cli.rs | 6 +++ validator/src/main.rs | 2 + 7 files changed, 120 insertions(+), 15 deletions(-) diff --git a/accounts-db/src/accounts_db.rs b/accounts-db/src/accounts_db.rs index f080abcb85556f..92c144ac0cbe59 100644 --- a/accounts-db/src/accounts_db.rs +++ b/accounts-db/src/accounts_db.rs @@ -486,6 +486,7 @@ pub const ACCOUNTS_DB_CONFIG_FOR_TESTING: AccountsDbConfig = AccountsDbConfig { exhaustively_verify_refcounts: false, create_ancient_storage: CreateAncientStorage::Pack, test_partitioned_epoch_rewards: TestPartitionedEpochRewards::CompareResults, + test_skip_rewrites_but_include_in_bank_hash: false, }; pub const ACCOUNTS_DB_CONFIG_FOR_BENCHMARKS: AccountsDbConfig = AccountsDbConfig { index: Some(ACCOUNTS_INDEX_CONFIG_FOR_BENCHMARKS), @@ -498,6 +499,7 @@ pub const ACCOUNTS_DB_CONFIG_FOR_BENCHMARKS: AccountsDbConfig = AccountsDbConfig exhaustively_verify_refcounts: false, create_ancient_storage: CreateAncientStorage::Pack, test_partitioned_epoch_rewards: TestPartitionedEpochRewards::None, + test_skip_rewrites_but_include_in_bank_hash: false, }; pub type BinnedHashData = Vec>; @@ -557,6 +559,7 @@ pub struct AccountsDbConfig { /// if None, ancient append vecs are set to ANCIENT_APPEND_VEC_DEFAULT_OFFSET /// Some(offset) means include slots up to (max_slot - (slots_per_epoch - 'offset')) pub ancient_append_vec_offset: Option, + pub test_skip_rewrites_but_include_in_bank_hash: bool, pub skip_initial_hash_calc: bool, pub exhaustively_verify_refcounts: bool, /// how to create ancient storages @@ -1440,6 +1443,9 @@ pub struct AccountsDb { /// from AccountsDbConfig create_ancient_storage: CreateAncientStorage, + /// true if this client should skip rewrites but still include those rewrites in the bank hash as if rewrites had occurred. + pub test_skip_rewrites_but_include_in_bank_hash: bool, + pub accounts_cache: AccountsCache, write_cache_limit_bytes: Option, @@ -1573,6 +1579,7 @@ pub struct AccountsStats { delta_hash_scan_time_total_us: AtomicU64, delta_hash_accumulate_time_total_us: AtomicU64, delta_hash_num: AtomicU64, + skipped_rewrites_num: AtomicUsize, last_store_report: AtomicInterval, store_hash_accounts: AtomicU64, @@ -2547,6 +2554,7 @@ impl AccountsDb { exhaustively_verify_refcounts: false, partitioned_epoch_rewards_config: PartitionedEpochRewardsConfig::default(), epoch_accounts_hash_manager: EpochAccountsHashManager::new_invalid(), + test_skip_rewrites_but_include_in_bank_hash: false, } } @@ -2622,6 +2630,11 @@ impl AccountsDb { .map(|config| config.test_partitioned_epoch_rewards) .unwrap_or_default(); + let test_skip_rewrites_but_include_in_bank_hash = accounts_db_config + .as_ref() + .map(|config| config.test_skip_rewrites_but_include_in_bank_hash) + .unwrap_or_default(); + let partitioned_epoch_rewards_config: PartitionedEpochRewardsConfig = PartitionedEpochRewardsConfig::new(test_partitioned_epoch_rewards); @@ -2647,6 +2660,7 @@ impl AccountsDb { .and_then(|x| x.write_cache_limit_bytes), partitioned_epoch_rewards_config, exhaustively_verify_refcounts, + test_skip_rewrites_but_include_in_bank_hash, ..Self::default_with_accounts_index( accounts_index, base_working_path, @@ -6944,6 +6958,11 @@ impl AccountsDb { .swap(0, Ordering::Relaxed), i64 ), + ( + "skipped_rewrites_num", + self.stats.skipped_rewrites_num.swap(0, Ordering::Relaxed), + i64 + ), ); } @@ -7908,7 +7927,6 @@ impl AccountsDb { slot: Slot, ) -> (Vec<(Pubkey, AccountHash)>, u64, Measure) { let mut scan = Measure::start("scan"); - let scan_result: ScanStorageResult<(Pubkey, AccountHash), DashMap> = self.scan_account_storage( slot, @@ -7928,6 +7946,7 @@ impl AccountsDb { ScanStorageResult::Cached(cached_result) => cached_result, ScanStorageResult::Stored(stored_result) => stored_result.into_iter().collect(), }; + (hashes, scan.as_us(), accumulate) } @@ -7968,12 +7987,12 @@ impl AccountsDb { } } - /// Calculate accounts delta hash for `slot` + /// Wrapper function to calculate accounts delta hash for `slot` (only used for testing and benchmarking.) /// /// As part of calculating the accounts delta hash, get a list of accounts modified this slot /// (aka dirty pubkeys) and add them to `self.uncleaned_pubkeys` for future cleaning. pub fn calculate_accounts_delta_hash(&self, slot: Slot) -> AccountsDeltaHash { - self.calculate_accounts_delta_hash_internal(slot, None) + self.calculate_accounts_delta_hash_internal(slot, None, HashMap::default()) } /// Calculate accounts delta hash for `slot` @@ -7984,9 +8003,20 @@ impl AccountsDb { &self, slot: Slot, ignore: Option, + mut skipped_rewrites: HashMap, ) -> AccountsDeltaHash { let (mut hashes, scan_us, mut accumulate) = self.get_pubkey_hash_for_slot(slot); let dirty_keys = hashes.iter().map(|(pubkey, _hash)| *pubkey).collect(); + + hashes.iter().for_each(|(k, _h)| { + skipped_rewrites.remove(k); + }); + + let num_skipped_rewrites = skipped_rewrites.len(); + hashes.extend(skipped_rewrites); + + info!("skipped rewrite hashes {} {}", slot, num_skipped_rewrites); + if let Some(ignore) = ignore { hashes.retain(|k| k.0 != ignore); } @@ -8015,6 +8045,10 @@ impl AccountsDb { .delta_hash_accumulate_time_total_us .fetch_add(accumulate.as_us(), Ordering::Relaxed); self.stats.delta_hash_num.fetch_add(1, Ordering::Relaxed); + self.stats + .skipped_rewrites_num + .fetch_add(num_skipped_rewrites, Ordering::Relaxed); + accounts_delta_hash } @@ -15020,7 +15054,6 @@ pub mod tests { db.store_uncached(1, &[(&account_key1, &account2)]); db.calculate_accounts_delta_hash(0); db.calculate_accounts_delta_hash(1); - db.print_accounts_stats("pre-clean1"); // clean accounts - no accounts should be cleaned, since no rooted slots @@ -15042,7 +15075,6 @@ pub mod tests { db.store_uncached(2, &[(&account_key2, &account3)]); db.store_uncached(2, &[(&account_key1, &account3)]); db.calculate_accounts_delta_hash(2); - db.clean_accounts_for_tests(); db.print_accounts_stats("post-clean2"); diff --git a/accounts-db/src/accounts_hash.rs b/accounts-db/src/accounts_hash.rs index 6b853895d7b790..7631ea694635b8 100644 --- a/accounts-db/src/accounts_hash.rs +++ b/accounts-db/src/accounts_hash.rs @@ -1243,7 +1243,7 @@ pub enum ZeroLamportAccounts { /// Hash of an account #[repr(transparent)] -#[derive(Debug, Copy, Clone, Eq, PartialEq, Pod, Zeroable)] +#[derive(Debug, Copy, Clone, Eq, PartialEq, Pod, Zeroable, AbiExample)] pub struct AccountHash(pub Hash); // Ensure the newtype wrapper never changes size from the underlying Hash diff --git a/ledger-tool/src/args.rs b/ledger-tool/src/args.rs index c11954a56780ab..0bb28e4a2779ca 100644 --- a/ledger-tool/src/args.rs +++ b/ledger-tool/src/args.rs @@ -83,6 +83,8 @@ pub fn get_accounts_db_config( exhaustively_verify_refcounts: arg_matches.is_present("accounts_db_verify_refcounts"), skip_initial_hash_calc: arg_matches.is_present("accounts_db_skip_initial_hash_calculation"), test_partitioned_epoch_rewards, + test_skip_rewrites_but_include_in_bank_hash: arg_matches + .is_present("accounts_db_test_skip_rewrites"), ..AccountsDbConfig::default() } } diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 697199981b26f8..33031e9d14a0a5 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -1128,6 +1128,12 @@ fn main() { "Debug option to scan all AppendVecs and verify account index refcounts prior to clean", ) .hidden(hidden_unless_forced()); + let accounts_db_test_skip_rewrites_but_include_in_bank_hash = Arg::with_name("accounts_db_test_skip_rewrites") + .long("accounts-db-test-skip-rewrites") + .help( + "Debug option to skip rewrites for rent-exempt accounts but still add them in bank delta hash calculation", + ) + .hidden(hidden_unless_forced()); let accounts_filler_count = Arg::with_name("accounts_filler_count") .long("accounts-filler-count") .value_name("COUNT") @@ -1556,6 +1562,7 @@ fn main() { .arg(&disable_disk_index) .arg(&accountsdb_verify_refcounts) .arg(&accounts_db_skip_initial_hash_calc_arg) + .arg(&accounts_db_test_skip_rewrites_but_include_in_bank_hash) ) .subcommand( SubCommand::with_name("shred-meta") @@ -1573,6 +1580,7 @@ fn main() { .arg(&disable_disk_index) .arg(&accountsdb_verify_refcounts) .arg(&accounts_db_skip_initial_hash_calc_arg) + .arg(&accounts_db_test_skip_rewrites_but_include_in_bank_hash) ) .subcommand( SubCommand::with_name("bounds") @@ -1608,6 +1616,7 @@ fn main() { .arg(&disable_disk_index) .arg(&accountsdb_skip_shrink) .arg(&accountsdb_verify_refcounts) + .arg(&accounts_db_test_skip_rewrites_but_include_in_bank_hash) .arg(&accounts_filler_count) .arg(&accounts_filler_size) .arg(&verify_index_arg) @@ -1688,6 +1697,7 @@ fn main() { .arg(&accounts_index_limit) .arg(&disable_disk_index) .arg(&accountsdb_verify_refcounts) + .arg(&accounts_db_test_skip_rewrites_but_include_in_bank_hash) .arg(&accounts_db_skip_initial_hash_calc_arg) .arg(&halt_at_slot_arg) .arg(&hard_forks_arg) @@ -1724,6 +1734,7 @@ fn main() { .arg(&accounts_index_limit) .arg(&disable_disk_index) .arg(&accountsdb_verify_refcounts) + .arg(&accounts_db_test_skip_rewrites_but_include_in_bank_hash) .arg(&accounts_db_skip_initial_hash_calc_arg) .arg(&accountsdb_skip_shrink) .arg(&ancient_append_vecs) @@ -1918,6 +1929,7 @@ fn main() { .arg(&accounts_index_limit) .arg(&disable_disk_index) .arg(&accountsdb_verify_refcounts) + .arg(&accounts_db_test_skip_rewrites_but_include_in_bank_hash) .arg(&accounts_db_skip_initial_hash_calc_arg) .arg(&halt_at_slot_arg) .arg(&hard_forks_arg) @@ -1952,6 +1964,7 @@ fn main() { .arg(&accounts_index_limit) .arg(&disable_disk_index) .arg(&accountsdb_verify_refcounts) + .arg(&accounts_db_test_skip_rewrites_but_include_in_bank_hash) .arg(&accounts_db_skip_initial_hash_calc_arg) .arg(&halt_at_slot_arg) .arg(&hard_forks_arg) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index edc2c26bc4a3e9..5c63f05e693b17 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -75,11 +75,13 @@ use { TransactionLoadResult, }, accounts_db::{ - AccountShrinkThreshold, AccountStorageEntry, AccountsDbConfig, + AccountShrinkThreshold, AccountStorageEntry, AccountsDb, AccountsDbConfig, CalcAccountsHashDataSource, VerifyAccountsHashAndLamportsConfig, ACCOUNTS_DB_CONFIG_FOR_BENCHMARKS, ACCOUNTS_DB_CONFIG_FOR_TESTING, }, - accounts_hash::{AccountsHash, CalcAccountsHashConfig, HashStats, IncrementalAccountsHash}, + accounts_hash::{ + AccountHash, AccountsHash, CalcAccountsHashConfig, HashStats, IncrementalAccountsHash, + }, accounts_index::{AccountSecondaryIndexes, IndexKey, ScanConfig, ScanResult, ZeroLamport}, accounts_partition::{self, Partition, PartitionIndex}, accounts_update_notifier_interface::AccountsUpdateNotifier, @@ -195,7 +197,7 @@ use { AtomicBool, AtomicI64, AtomicU64, AtomicUsize, Ordering::{self, AcqRel, Acquire, Relaxed}, }, - Arc, LockResult, RwLock, RwLockReadGuard, RwLockWriteGuard, + Arc, LockResult, Mutex, RwLock, RwLockReadGuard, RwLockWriteGuard, }, thread::Builder, time::{Duration, Instant}, @@ -507,6 +509,7 @@ impl PartialEq for Bank { return true; } let Self { + skipped_rewrites: _, rc: _, status_cache: _, blockhash_queue, @@ -815,6 +818,10 @@ pub struct Bank { /// The change to accounts data size in this Bank, due to off-chain events (i.e. rent collection) accounts_data_size_delta_off_chain: AtomicI64, + /// until the skipped rewrites feature is activated, it is possible to skip rewrites and still include + /// the account hash of the accounts that would have been rewritten as bank hash expects. + skipped_rewrites: Mutex>, + /// Transaction fee structure pub fee_structure: FeeStructure, @@ -1012,6 +1019,7 @@ impl Bank { fn default_with_accounts(accounts: Accounts) -> Self { let mut bank = Self { + skipped_rewrites: Mutex::default(), incremental_snapshot_persistence: None, rc: BankRc::new(accounts, Slot::default()), status_cache: Arc::>::default(), @@ -1343,6 +1351,7 @@ impl Bank { let accounts_data_size_initial = parent.load_accounts_data_size(); let mut new = Self { + skipped_rewrites: Mutex::default(), incremental_snapshot_persistence: None, rc, status_cache, @@ -1797,6 +1806,7 @@ impl Bank { ); let stakes_accounts_load_duration = now.elapsed(); let mut bank = Self { + skipped_rewrites: Mutex::default(), incremental_snapshot_persistence: fields.incremental_snapshot_persistence, rc: bank_rc, status_cache: Arc::>::default(), @@ -6010,9 +6020,16 @@ impl Bank { let mut time_collecting_rent_us = 0; let mut time_storing_accounts_us = 0; let can_skip_rewrites = self.bank_hash_skips_rent_rewrites(); + let test_skip_rewrites_but_include_hash_in_bank_hash = !can_skip_rewrites + && self + .rc + .accounts + .accounts_db + .test_skip_rewrites_but_include_in_bank_hash; let set_exempt_rent_epoch_max: bool = self .feature_set .is_active(&solana_sdk::feature_set::set_exempt_rent_epoch_max::id()); + let mut skipped_rewrites = Vec::default(); for (pubkey, account, _loaded_slot) in accounts.iter_mut() { let (rent_collected_info, measure) = measure!(self.rent_collector.collect_from_existing_account( @@ -6028,7 +6045,9 @@ impl Bank { // Also, there's another subtle side-effect from rewrites: this // ensures we verify the whole on-chain state (= all accounts) // via the bank delta hash slowly once per an epoch. - if !can_skip_rewrites || !Self::skip_rewrite(rent_collected_info.rent_amount, account) { + if (!can_skip_rewrites && !test_skip_rewrites_but_include_hash_in_bank_hash) + || !Self::skip_rewrite(rent_collected_info.rent_amount, account) + { if rent_collected_info.rent_amount > 0 { if let Some(rent_paying_pubkeys) = rent_paying_pubkeys { if !rent_paying_pubkeys.contains(pubkey) { @@ -6058,6 +6077,13 @@ impl Bank { } total_rent_collected_info += rent_collected_info; accounts_to_store.push((pubkey, account)); + } else if test_skip_rewrites_but_include_hash_in_bank_hash { + // include rewrites that we skipped in the accounts delta hash. + // This is what consensus requires prior to activation of bank_hash_skips_rent_rewrites. + // This code path exists to allow us to test the long term effects on validators when the skipped rewrites + // feature is enabled. + let hash = AccountsDb::hash_account(account, pubkey); + skipped_rewrites.push((*pubkey, hash)); } rent_debits.insert(pubkey, rent_collected_info.rent_amount, account.lamports()); } @@ -6071,6 +6097,7 @@ impl Bank { } CollectRentFromAccountsInfo { + skipped_rewrites, rent_collected_info: total_rent_collected_info, rent_rewards: rent_debits.into_unordered_rewards_iter().collect(), time_collecting_rent_us, @@ -6173,6 +6200,11 @@ impl Bank { CollectRentInPartitionInfo::reduce, ); + self.skipped_rewrites + .lock() + .unwrap() + .extend(&mut results.skipped_rewrites.into_iter()); + // We cannot assert here that we collected from all expected keys. // Some accounts may have been topped off or may have had all funds removed and gone to 0 lamports. @@ -7066,7 +7098,11 @@ impl Bank { .rc .accounts .accounts_db - .calculate_accounts_delta_hash_internal(slot, ignore); + .calculate_accounts_delta_hash_internal( + slot, + ignore, + std::mem::take(&mut self.skipped_rewrites.lock().unwrap()), + ); let mut signature_count_buf = [0u8; 8]; LittleEndian::write_u64(&mut signature_count_buf[..], self.signature_count()); @@ -7635,10 +7671,20 @@ impl Bank { }); let (verified_bank, verify_bank_time_us) = measure_us!({ - info!("Verifying bank..."); - let verified = self.verify_hash(); - info!("Verifying bank... Done."); - verified + let should_verify_bank = !self + .rc + .accounts + .accounts_db + .test_skip_rewrites_but_include_in_bank_hash; + if should_verify_bank { + info!("Verifying bank..."); + let verified = self.verify_hash(); + info!("Verifying bank... Done."); + verified + } else { + info!("Verifying bank... Skipped."); + true + } }); datapoint_info!( @@ -8337,6 +8383,7 @@ enum ApplyFeatureActivationsCaller { /// process later. #[derive(Debug, Default)] struct CollectRentFromAccountsInfo { + skipped_rewrites: Vec<(Pubkey, AccountHash)>, rent_collected_info: CollectedInfo, rent_rewards: Vec<(Pubkey, RewardInfo)>, time_collecting_rent_us: u64, @@ -8348,6 +8395,7 @@ struct CollectRentFromAccountsInfo { /// `collect_rent_in_partition()`—and then perform a reduce on all of them. #[derive(Debug, Default)] struct CollectRentInPartitionInfo { + skipped_rewrites: Vec<(Pubkey, AccountHash)>, rent_collected: u64, accounts_data_size_reclaimed: u64, rent_rewards: Vec<(Pubkey, RewardInfo)>, @@ -8363,6 +8411,7 @@ impl CollectRentInPartitionInfo { #[must_use] fn new(info: CollectRentFromAccountsInfo, time_loading_accounts: Duration) -> Self { Self { + skipped_rewrites: info.skipped_rewrites, rent_collected: info.rent_collected_info.rent_amount, accounts_data_size_reclaimed: info.rent_collected_info.account_data_len_reclaimed, rent_rewards: info.rent_rewards, @@ -8380,6 +8429,7 @@ impl CollectRentInPartitionInfo { #[must_use] fn reduce(lhs: Self, rhs: Self) -> Self { Self { + skipped_rewrites: [lhs.skipped_rewrites, rhs.skipped_rewrites].concat(), rent_collected: lhs.rent_collected.saturating_add(rhs.rent_collected), accounts_data_size_reclaimed: lhs .accounts_data_size_reclaimed diff --git a/validator/src/cli.rs b/validator/src/cli.rs index bd82c0a4ac2727..9aa1c466f8e336 100644 --- a/validator/src/cli.rs +++ b/validator/src/cli.rs @@ -1194,6 +1194,12 @@ pub fn app<'a>(version: &'a str, default_args: &'a DefaultArgs) -> App<'a, 'a> { .help("Debug option to scan all append vecs and verify account index refcounts prior to clean") .hidden(hidden_unless_forced()) ) + .arg( + Arg::with_name("accounts_db_test_skip_rewrites") + .long("accounts-db-test-skip-rewrites") + .help("Debug option to skip rewrites for rent-exempt accounts but still add them in bank delta hash calculation") + .hidden(hidden_unless_forced()) + ) .arg( Arg::with_name("no_skip_initial_accounts_db_clean") .long("no-skip-initial-accounts-db-clean") diff --git a/validator/src/main.rs b/validator/src/main.rs index 38bb9813ab3a70..4c247c9a9977a2 100644 --- a/validator/src/main.rs +++ b/validator/src/main.rs @@ -1206,6 +1206,8 @@ pub fn main() { .then_some(CreateAncientStorage::Pack) .unwrap_or_default(), test_partitioned_epoch_rewards, + test_skip_rewrites_but_include_in_bank_hash: matches + .is_present("accounts_db_test_skip_rewrites"), ..AccountsDbConfig::default() };