diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 263d9552225bdb..7c1b9a63122056 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -480,8 +480,7 @@ pub struct BankFieldsToSerialize { pub is_delta: bool, pub accounts_data_len: u64, pub versioned_epoch_stakes: HashMap, - // When removing the accounts lt hash featurization code, also remove this Option wrapper - pub accounts_lt_hash: Option, + pub accounts_lt_hash: AccountsLtHash, } // Can't derive PartialEq because RwLock doesn't implement PartialEq @@ -596,8 +595,7 @@ impl PartialEq for Bank { // different Mutexes. && (Arc::ptr_eq(hash_overrides, &other.hash_overrides) || *hash_overrides.lock().unwrap() == *other.hash_overrides.lock().unwrap()) - && !(self.is_accounts_lt_hash_enabled() && other.is_accounts_lt_hash_enabled() - && *accounts_lt_hash.lock().unwrap() != *other.accounts_lt_hash.lock().unwrap()) + && *accounts_lt_hash.lock().unwrap() == *other.accounts_lt_hash.lock().unwrap() && *block_id.read().unwrap() == *other.block_id.read().unwrap() } } @@ -637,7 +635,7 @@ impl BankFieldsToSerialize { is_delta: bool::default(), accounts_data_len: u64::default(), versioned_epoch_stakes: HashMap::default(), - accounts_lt_hash: Some(AccountsLtHash(LtHash([0x7E57; LtHash::NUM_ELEMENTS]))), + accounts_lt_hash: AccountsLtHash(LtHash([0x7E57; LtHash::NUM_ELEMENTS])), } } } @@ -1405,29 +1403,25 @@ impl Bank { .transaction_processor .fill_missing_sysvar_cache_entries(&new)); - let (num_accounts_modified_this_slot, populate_cache_for_accounts_lt_hash_us) = new - .is_accounts_lt_hash_enabled() - .then(|| { - measure_us!({ - // The cache for accounts lt hash needs to be made aware of accounts modified - // before transaction processing begins. Otherwise we may calculate the wrong - // accounts lt hash due to having the wrong initial state of the account. The - // lt hash cache's initial state must always be from an ancestor, and cannot be - // an intermediate state within this Bank's slot. If the lt hash cache has the - // wrong initial account state, we'll mix out the wrong lt hash value, and thus - // have the wrong overall accounts lt hash, and diverge. - let accounts_modified_this_slot = - new.rc.accounts.accounts_db.get_pubkeys_for_slot(slot); - let num_accounts_modified_this_slot = accounts_modified_this_slot.len(); - for pubkey in accounts_modified_this_slot { - new.cache_for_accounts_lt_hash - .entry(pubkey) - .or_insert(AccountsLtHashCacheValue::BankNew); - } - num_accounts_modified_this_slot - }) - }) - .unzip(); + let (num_accounts_modified_this_slot, populate_cache_for_accounts_lt_hash_us) = + measure_us!({ + // The cache for accounts lt hash needs to be made aware of accounts modified + // before transaction processing begins. Otherwise we may calculate the wrong + // accounts lt hash due to having the wrong initial state of the account. The + // lt hash cache's initial state must always be from an ancestor, and cannot be + // an intermediate state within this Bank's slot. If the lt hash cache has the + // wrong initial account state, we'll mix out the wrong lt hash value, and thus + // have the wrong overall accounts lt hash, and diverge. + let accounts_modified_this_slot = + new.rc.accounts.accounts_db.get_pubkeys_for_slot(slot); + let num_accounts_modified_this_slot = accounts_modified_this_slot.len(); + for pubkey in accounts_modified_this_slot { + new.cache_for_accounts_lt_hash + .entry(pubkey) + .or_insert(AccountsLtHashCacheValue::BankNew); + } + num_accounts_modified_this_slot + }); time.stop(); report_new_bank_metrics( @@ -1926,9 +1920,7 @@ impl Bank { is_delta: self.is_delta.load(Relaxed), accounts_data_len: self.load_accounts_data_size(), versioned_epoch_stakes: self.epoch_stakes.clone(), - accounts_lt_hash: self - .is_accounts_lt_hash_enabled() - .then(|| self.accounts_lt_hash.lock().unwrap().clone()), + accounts_lt_hash: self.accounts_lt_hash.lock().unwrap().clone(), } } @@ -2538,11 +2530,9 @@ impl Bank { // freeze is a one-way trip, idempotent self.freeze_started.store(true, Relaxed); - if self.is_accounts_lt_hash_enabled() { - // updating the accounts lt hash must happen *outside* of hash_internal_state() so - // that rehash() can be called and *not* modify self.accounts_lt_hash. - self.update_accounts_lt_hash(); - } + // updating the accounts lt hash must happen *outside* of hash_internal_state() so + // that rehash() can be called and *not* modify self.accounts_lt_hash. + self.update_accounts_lt_hash(); *hash = self.hash_internal_state(); self.rc.accounts.accounts_db.mark_slot_frozen(self.slot()); } @@ -5897,9 +5887,7 @@ impl TransactionProcessingCallback for Bank { } fn inspect_account(&self, address: &Pubkey, account_state: AccountState, is_writable: bool) { - if self.is_accounts_lt_hash_enabled() { - self.inspect_account_for_accounts_lt_hash(address, &account_state, is_writable); - } + self.inspect_account_for_accounts_lt_hash(address, &account_state, is_writable); } } diff --git a/runtime/src/bank/accounts_lt_hash.rs b/runtime/src/bank/accounts_lt_hash.rs index 628c0435edeb5e..8a3be75d1f8267 100644 --- a/runtime/src/bank/accounts_lt_hash.rs +++ b/runtime/src/bank/accounts_lt_hash.rs @@ -17,22 +17,15 @@ use { }; impl Bank { - /// Returns if the accounts lt hash is enabled - pub fn is_accounts_lt_hash_enabled(&self) -> bool { - true - } - /// Returns if snapshots use the accounts lt hash pub fn is_snapshots_lt_hash_enabled(&self) -> bool { - self.is_accounts_lt_hash_enabled() - && (self - .rc - .accounts - .accounts_db - .snapshots_use_experimental_accumulator_hash() - || self - .feature_set - .is_active(&feature_set::snapshots_lt_hash::id())) + self.rc + .accounts + .accounts_db + .snapshots_use_experimental_accumulator_hash() + || self + .feature_set + .is_active(&feature_set::snapshots_lt_hash::id()) } /// Updates the accounts lt hash @@ -44,7 +37,6 @@ impl Bank { /// /// Since this function is non-idempotent, it should only be called once per bank. pub fn update_accounts_lt_hash(&self) { - debug_assert!(self.is_accounts_lt_hash_enabled()); let delta_lt_hash = self.calculate_delta_lt_hash(); let mut accounts_lt_hash = self.accounts_lt_hash.lock().unwrap(); accounts_lt_hash.0.mix_in(&delta_lt_hash); @@ -60,7 +52,6 @@ impl Bank { /// /// This function is idempotent, and may be called more than once. fn calculate_delta_lt_hash(&self) -> LtHash { - debug_assert!(self.is_accounts_lt_hash_enabled()); let measure_total = Measure::start(""); let slot = self.slot(); @@ -308,7 +299,6 @@ impl Bank { account_state: &AccountState, is_writable: bool, ) { - debug_assert!(self.is_accounts_lt_hash_enabled()); if !is_writable { // if the account is not writable, then it cannot be modified; nothing to do here return; @@ -486,9 +476,6 @@ mod tests { genesis_config.fee_rate_governor = FeeRateGovernor::new(0, 0); let (bank, bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); - // ensure the accounts lt hash is enabled, otherwise this test doesn't actually do anything... - assert!(bank.is_accounts_lt_hash_enabled()); - let amount = cmp::max( bank.get_minimum_balance_for_rent_exemption(0), LAMPORTS_PER_SOL, @@ -648,9 +635,6 @@ mod tests { let (genesis_config, mint_keypair) = genesis_config_with(features); let (bank, _bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); - // ensure the accounts lt hash is enabled, otherwise this test doesn't actually do anything... - assert!(bank.is_accounts_lt_hash_enabled()); - // ensure this bank is for slot 0, otherwise this test doesn't actually do anything... assert_eq!(bank.slot(), 0); @@ -677,9 +661,6 @@ mod tests { let (genesis_config, _mint_keypair) = genesis_config_with(features); let (bank, _bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); - // ensure the accounts lt hash is enabled, otherwise this test doesn't actually do anything... - assert!(bank.is_accounts_lt_hash_enabled()); - // the cache should start off empty assert_eq!(bank.cache_for_accounts_lt_hash.len(), 0); @@ -788,9 +769,6 @@ mod tests { let (genesis_config, mint_keypair) = genesis_config_with(features); let (mut bank, bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); - // ensure the accounts lt hash is enabled, otherwise this test doesn't actually do anything... - assert!(bank.is_accounts_lt_hash_enabled()); - let amount = cmp::max( bank.get_minimum_balance_for_rent_exemption(0), LAMPORTS_PER_SOL, @@ -833,9 +811,6 @@ mod tests { let (genesis_config, mint_keypair) = genesis_config_with(features); let (mut bank, bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); - // ensure the accounts lt hash is enabled, otherwise this test doesn't actually do anything... - assert!(bank.is_accounts_lt_hash_enabled()); - let amount = cmp::max( bank.get_minimum_balance_for_rent_exemption(0), LAMPORTS_PER_SOL, @@ -933,9 +908,6 @@ mod tests { genesis_config.fee_rate_governor = FeeRateGovernor::new(0, 0); let (mut bank, bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); - // ensure the accounts lt hash is enabled, otherwise this test doesn't actually do anything... - assert!(bank.is_accounts_lt_hash_enabled()); - let amount = cmp::max( bank.get_minimum_balance_for_rent_exemption(0), LAMPORTS_PER_SOL, @@ -1058,9 +1030,6 @@ mod tests { let (genesis_config, _mint_keypair) = genesis_config_with(features); let (mut bank, bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); - // ensure the accounts lt hash is enabled, otherwise this test doesn't actually do anything... - assert!(bank.is_accounts_lt_hash_enabled()); - let slot = bank.slot() + 1; bank = new_bank_from_parent_with_bank_forks(&bank_forks, bank, &Pubkey::default(), slot); @@ -1096,9 +1065,6 @@ mod tests { let (genesis_config, mint_keypair) = genesis_config_with(features); let (mut bank, bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); - // ensure the accounts lt hash is enabled, otherwise the snapshot lt hash is disabled - assert!(bank.is_accounts_lt_hash_enabled()); - bank.rc .accounts .accounts_db diff --git a/runtime/src/bank/metrics.rs b/runtime/src/bank/metrics.rs index ae0c2b714f7ad0..1e54a385897082 100644 --- a/runtime/src/bank/metrics.rs +++ b/runtime/src/bank/metrics.rs @@ -43,7 +43,7 @@ pub(crate) struct NewBankTimings { pub(crate) cache_preparation_time_us: u64, pub(crate) update_sysvars_time_us: u64, pub(crate) fill_sysvar_cache_time_us: u64, - pub(crate) populate_cache_for_accounts_lt_hash_us: Option, + pub(crate) populate_cache_for_accounts_lt_hash_us: u64, } pub(crate) fn report_new_epoch_metrics( @@ -98,7 +98,7 @@ pub(crate) fn report_new_bank_metrics( slot: Slot, parent_slot: Slot, block_height: u64, - num_accounts_modified_this_slot: Option, + num_accounts_modified_this_slot: usize, timings: NewBankTimings, ) { datapoint_info!( @@ -151,12 +151,12 @@ pub(crate) fn report_new_bank_metrics( ( "num_accounts_modified_this_slot", num_accounts_modified_this_slot, - Option + i64 ), ( "populate_cache_for_accounts_lt_hash_us", timings.populate_cache_for_accounts_lt_hash_us, - Option + i64 ), ); } diff --git a/runtime/src/bank/serde_snapshot.rs b/runtime/src/bank/serde_snapshot.rs index 8dabf41cc52e14..1e165067ffc9db 100644 --- a/runtime/src/bank/serde_snapshot.rs +++ b/runtime/src/bank/serde_snapshot.rs @@ -138,7 +138,7 @@ mod tests { { let mut bank_fields = bank2.get_fields_to_serialize(); let versioned_epoch_stakes = mem::take(&mut bank_fields.versioned_epoch_stakes); - let accounts_lt_hash = bank_fields.accounts_lt_hash.clone().map(Into::into); + let accounts_lt_hash = Some(bank_fields.accounts_lt_hash.clone().into()); serde_snapshot::serialize_bank_snapshot_into( &mut writer, bank_fields, diff --git a/runtime/src/serde_snapshot.rs b/runtime/src/serde_snapshot.rs index e5903d6c8a3e01..05270a5cd1564e 100644 --- a/runtime/src/serde_snapshot.rs +++ b/runtime/src/serde_snapshot.rs @@ -702,7 +702,7 @@ impl Serialize for SerializableBankAndStorage<'_> { let write_version = accounts_db.write_version.load(Ordering::Acquire); let lamports_per_signature = bank_fields.fee_rate_governor.lamports_per_signature; let versioned_epoch_stakes = std::mem::take(&mut bank_fields.versioned_epoch_stakes); - let accounts_lt_hash = bank_fields.accounts_lt_hash.clone().map(Into::into); + let accounts_lt_hash = Some(bank_fields.accounts_lt_hash.clone().into()); let bank_fields_to_serialize = ( SerializableVersionedBank::from(bank_fields), SerializableAccountsDb::<'_> { diff --git a/runtime/src/snapshot_hash.rs b/runtime/src/snapshot_hash.rs index 8a56b08753f3a4..6465d127076952 100644 --- a/runtime/src/snapshot_hash.rs +++ b/runtime/src/snapshot_hash.rs @@ -33,7 +33,7 @@ impl SnapshotHash { #[must_use] pub fn new( merkle_or_lattice_accounts_hash: &MerkleOrLatticeAccountsHash, - accounts_lt_hash_checksum: Option, + accounts_lt_hash_checksum: Option, // option wrapper will be removed next ) -> Self { let accounts_hash = match merkle_or_lattice_accounts_hash { MerkleOrLatticeAccountsHash::Merkle(accounts_hash_kind) => { diff --git a/runtime/src/snapshot_minimizer.rs b/runtime/src/snapshot_minimizer.rs index 418cdaf212ccd4..ae06393ce6eb50 100644 --- a/runtime/src/snapshot_minimizer.rs +++ b/runtime/src/snapshot_minimizer.rs @@ -70,16 +70,14 @@ impl<'a> SnapshotMinimizer<'a> { .bank .set_capitalization_for_tests(minimizer.bank.calculate_capitalization_for_tests()); - if minimizer.bank.is_accounts_lt_hash_enabled() { - // Since the account state has changed, the accounts lt hash must be recalculated - let new_accounts_lt_hash = minimizer - .accounts_db() - .calculate_accounts_lt_hash_at_startup_from_index( - &minimizer.bank.ancestors, - minimizer.bank.slot(), - ); - bank.set_accounts_lt_hash_for_snapshot_minimizer(new_accounts_lt_hash); - } + // Since the account state has changed, the accounts lt hash must be recalculated + let new_accounts_lt_hash = minimizer + .accounts_db() + .calculate_accounts_lt_hash_at_startup_from_index( + &minimizer.bank.ancestors, + minimizer.bank.slot(), + ); + bank.set_accounts_lt_hash_for_snapshot_minimizer(new_accounts_lt_hash); } /// Helper function to measure time and number of accounts added @@ -594,10 +592,6 @@ mod tests { let (bank, bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config_info.genesis_config); - // ensure the accounts lt hash is enabled, otherwise minimization - // doesn't need to recalculate it - assert!(bank.is_accounts_lt_hash_enabled()); - // write to multiple accounts and keep track of one, for minimization later let pubkey_to_keep = Pubkey::new_unique(); let slot = bank.slot() + 1; diff --git a/runtime/src/snapshot_package.rs b/runtime/src/snapshot_package.rs index 47efdfc0518e0a..61a56e0f9de75f 100644 --- a/runtime/src/snapshot_package.rs +++ b/runtime/src/snapshot_package.rs @@ -248,11 +248,13 @@ impl SnapshotPackage { block_height: accounts_package.block_height, hash: SnapshotHash::new( &merkle_or_lattice_accounts_hash, - snapshot_info - .bank_fields_to_serialize - .accounts_lt_hash - .as_ref() - .map(|accounts_lt_hash| accounts_lt_hash.0.checksum()), + Some( + snapshot_info + .bank_fields_to_serialize + .accounts_lt_hash + .0 + .checksum(), + ), ), snapshot_storages: accounts_package.snapshot_storages, status_cache_slot_deltas: snapshot_info.status_cache_slot_deltas, diff --git a/runtime/src/snapshot_utils.rs b/runtime/src/snapshot_utils.rs index bc6936f4602764..8f274191e9bfd9 100644 --- a/runtime/src/snapshot_utils.rs +++ b/runtime/src/snapshot_utils.rs @@ -963,7 +963,7 @@ fn serialize_snapshot( incremental_snapshot_persistence: bank_incremental_snapshot_persistence, obsolete_epoch_accounts_hash: None, versioned_epoch_stakes, - accounts_lt_hash: bank_fields.accounts_lt_hash.clone().map(Into::into), + accounts_lt_hash: Some(bank_fields.accounts_lt_hash.clone().into()), }; serde_snapshot::serialize_bank_snapshot_into( stream,