Skip to content

Commit

Permalink
Uses enum for data source with calc_accounts_hash() (solana-labs#28584)
Browse files Browse the repository at this point in the history
  • Loading branch information
@brooksprumo
brooksprumo authored Oct 26, 2022
1 parent 252d7f6 commit 27269d8
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 55 deletions.
4 changes: 2 additions & 2 deletions accounts-bench/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use {
test_utils::{create_test_accounts, update_accounts_bench},
Accounts,
},
accounts_db::AccountShrinkThreshold,
accounts_db::{AccountShrinkThreshold, CalcAccountsHashDataSource},
accounts_index::AccountSecondaryIndexes,
ancestors::Ancestors,
rent_collector::RentCollector,
Expand Down Expand Up @@ -126,7 +126,7 @@ fn main() {
time.stop();
let mut time_store = Measure::start("hash using store");
let results_store = accounts.accounts_db.update_accounts_hash(
false,
CalcAccountsHashDataSource::Storages,
false,
solana_sdk::clock::Slot::default(),
&ancestors,
Expand Down
7 changes: 3 additions & 4 deletions runtime/src/accounts.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ use {
account_rent_state::{check_rent_state_with_account, RentState},
accounts_db::{
AccountShrinkThreshold, AccountsAddRootTiming, AccountsDb, AccountsDbConfig,
BankHashInfo, IncludeSlotInHash, LoadHint, LoadedAccount, ScanStorageResult,
ACCOUNTS_DB_CONFIG_FOR_BENCHMARKS, ACCOUNTS_DB_CONFIG_FOR_TESTING,
BankHashInfo, CalcAccountsHashDataSource, IncludeSlotInHash, LoadHint, LoadedAccount,
ScanStorageResult, ACCOUNTS_DB_CONFIG_FOR_BENCHMARKS, ACCOUNTS_DB_CONFIG_FOR_TESTING,
},
accounts_index::{
AccountSecondaryIndexes, IndexKey, ScanConfig, ScanError, ScanResult, ZeroLamport,
Expand Down Expand Up @@ -789,14 +789,13 @@ impl Accounts {
epoch_schedule: &EpochSchedule,
rent_collector: &RentCollector,
) -> u64 {
let use_index = false;
let is_startup = true;
self.accounts_db
.verify_accounts_hash_in_bg
.wait_for_complete();
self.accounts_db
.update_accounts_hash(
use_index,
CalcAccountsHashDataSource::Storages,
debug_verify,
slot,
ancestors,
Expand Down
10 changes: 7 additions & 3 deletions runtime/src/accounts_background_service.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
mod stats;
use {
crate::{
accounts_db::CalcAccountsHashDataSource,
accounts_hash::CalcAccountsHashConfig,
bank::{Bank, BankSlotDelta, DropCallback},
bank_forks::BankForks,
Expand Down Expand Up @@ -296,7 +297,11 @@ impl SnapshotRequestHandler {
let previous_hash = if test_hash_calculation {
// We have to use the index version here.
// We cannot calculate the non-index way because cache has not been flushed and stores don't match reality. This comment is out of date and can be re-evaluated.
snapshot_root_bank.update_accounts_hash_with_index_option(true, false, false)
snapshot_root_bank.update_accounts_hash_with_index_option(
CalcAccountsHashDataSource::Index,
false,
false,
)
} else {
Hash::default()
};
Expand Down Expand Up @@ -330,14 +335,13 @@ impl SnapshotRequestHandler {
flush_accounts_cache_time.stop();

let hash_for_testing = if test_hash_calculation {
let use_index_hash_calculation = false;
let check_hash = false;

let (this_hash, capitalization) = snapshot_root_bank
.accounts()
.accounts_db
.calculate_accounts_hash(
use_index_hash_calculation,
CalcAccountsHashDataSource::Storages,
snapshot_root_bank.slot(),
&CalcAccountsHashConfig {
use_bg_thread_pool: true,
Expand Down
97 changes: 57 additions & 40 deletions runtime/src/accounts_db.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6923,7 +6923,7 @@ impl AccountsDb {
is_startup: bool,
) -> (Hash, u64) {
self.update_accounts_hash(
true,
CalcAccountsHashDataSource::Index,
debug_verify,
slot,
ancestors,
Expand Down Expand Up @@ -7227,69 +7227,77 @@ impl AccountsDb {

pub(crate) fn calculate_accounts_hash(
&self,
use_index: bool,
data_source: CalcAccountsHashDataSource,
slot: Slot,
config: &CalcAccountsHashConfig<'_>,
) -> Result<(Hash, u64), BankHashVerificationError> {
if !use_index {
if self.accounts_cache.contains_any_slots(slot) {
// this indicates a race condition
inc_new_counter_info!("accounts_hash_items_in_write_cache", 1);
}

let mut collect_time = Measure::start("collect");
let (combined_maps, slots) = self.get_snapshot_storages(slot, None, config.ancestors);
collect_time.stop();
match data_source {
CalcAccountsHashDataSource::Storages => {
if self.accounts_cache.contains_any_slots(slot) {
// this indicates a race condition
inc_new_counter_info!("accounts_hash_items_in_write_cache", 1);
}

let mut sort_time = Measure::start("sort_storages");
let min_root = self.accounts_index.min_alive_root();
let storages = SortedStorages::new_with_slots(
combined_maps.iter().zip(slots.into_iter()),
min_root,
Some(slot),
);
sort_time.stop();
let mut collect_time = Measure::start("collect");
let (combined_maps, slots) =
self.get_snapshot_storages(slot, None, config.ancestors);
collect_time.stop();

let mut sort_time = Measure::start("sort_storages");
let min_root = self.accounts_index.min_alive_root();
let storages = SortedStorages::new_with_slots(
combined_maps.iter().zip(slots.into_iter()),
min_root,
Some(slot),
);
sort_time.stop();

let mut timings = HashStats {
collect_snapshots_us: collect_time.as_us(),
storage_sort_us: sort_time.as_us(),
..HashStats::default()
};
timings.calc_storage_size_quartiles(&combined_maps);
let mut timings = HashStats {
collect_snapshots_us: collect_time.as_us(),
storage_sort_us: sort_time.as_us(),
..HashStats::default()
};
timings.calc_storage_size_quartiles(&combined_maps);

self.calculate_accounts_hash_from_storages(config, &storages, timings)
} else {
self.calculate_accounts_hash_from_index(slot, config)
self.calculate_accounts_hash_from_storages(config, &storages, timings)
}
CalcAccountsHashDataSource::Index => {
self.calculate_accounts_hash_from_index(slot, config)
}
}
}

#[allow(clippy::too_many_arguments)]
fn calculate_accounts_hash_with_verify(
&self,
use_index: bool,
data_source: CalcAccountsHashDataSource,
debug_verify: bool,
slot: Slot,
config: CalcAccountsHashConfig<'_>,
expected_capitalization: Option<u64>,
) -> Result<(Hash, u64), BankHashVerificationError> {
let (hash, total_lamports) = self.calculate_accounts_hash(use_index, slot, &config)?;
let (hash, total_lamports) = self.calculate_accounts_hash(data_source, slot, &config)?;
if debug_verify {
// calculate the other way (store or non-store) and verify results match.
let data_source_other = match data_source {
CalcAccountsHashDataSource::Index => CalcAccountsHashDataSource::Storages,
CalcAccountsHashDataSource::Storages => CalcAccountsHashDataSource::Index,
};
let (hash_other, total_lamports_other) =
self.calculate_accounts_hash(!use_index, slot, &config)?;
self.calculate_accounts_hash(data_source_other, slot, &config)?;

let success = hash == hash_other
&& total_lamports == total_lamports_other
&& total_lamports == expected_capitalization.unwrap_or(total_lamports);
assert!(success, "calculate_accounts_hash_with_verify mismatch. hashes: {}, {}; lamports: {}, {}; expected lamports: {:?}, using index: {}, slot: {}", hash, hash_other, total_lamports, total_lamports_other, expected_capitalization, use_index, slot);
assert!(success, "calculate_accounts_hash_with_verify mismatch. hashes: {}, {}; lamports: {}, {}; expected lamports: {:?}, data source: {:?}, slot: {}", hash, hash_other, total_lamports, total_lamports_other, expected_capitalization, data_source, slot);
}
Ok((hash, total_lamports))
}

#[allow(clippy::too_many_arguments)]
pub fn update_accounts_hash(
&self,
use_index: bool,
data_source: CalcAccountsHashDataSource,
debug_verify: bool,
slot: Slot,
ancestors: &Ancestors,
Expand All @@ -7301,7 +7309,7 @@ impl AccountsDb {
let check_hash = false;
let (hash, total_lamports) = self
.calculate_accounts_hash_with_verify(
use_index,
data_source,
debug_verify,
slot,
CalcAccountsHashConfig {
Expand Down Expand Up @@ -7595,10 +7603,9 @@ impl AccountsDb {
) -> Result<(), BankHashVerificationError> {
use BankHashVerificationError::*;

let use_index = false;
let check_hash = false; // this will not be supported anymore
let (calculated_hash, calculated_lamports) = self.calculate_accounts_hash_with_verify(
use_index,
CalcAccountsHashDataSource::Storages,
test_hash_calculation,
slot,
CalcAccountsHashConfig {
Expand Down Expand Up @@ -9390,6 +9397,13 @@ impl AccountsDb {
}
}

/// Specify the source of the accounts data when calculating the accounts hash
#[derive(Debug, Copy, Clone, Eq, PartialEq)]
pub enum CalcAccountsHashDataSource {
Index,
Storages,
}

#[cfg(test)]
impl AccountsDb {
pub fn new(paths: Vec<PathBuf>, cluster_type: &ClusterType) -> Self {
Expand Down Expand Up @@ -12344,10 +12358,13 @@ pub mod tests {
);
db.add_root(some_slot);
let check_hash = true;
for use_index in [true, false] {
for data_source in [
CalcAccountsHashDataSource::Index,
CalcAccountsHashDataSource::Storages,
] {
assert!(db
.calculate_accounts_hash(
use_index,
data_source,
some_slot,
&CalcAccountsHashConfig {
use_bg_thread_pool: true, // is_startup used to be false
Expand Down Expand Up @@ -12399,7 +12416,7 @@ pub mod tests {
let check_hash = true;
assert_eq!(
db.calculate_accounts_hash(
false,
CalcAccountsHashDataSource::Storages,
some_slot,
&CalcAccountsHashConfig {
use_bg_thread_pool: true, // is_startup used to be false
Expand All @@ -12410,7 +12427,7 @@ pub mod tests {
)
.unwrap(),
db.calculate_accounts_hash(
true,
CalcAccountsHashDataSource::Index,
some_slot,
&CalcAccountsHashConfig {
use_bg_thread_pool: true, // is_startup used to be false
Expand Down
13 changes: 7 additions & 6 deletions runtime/src/bank.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,9 @@ use {
TransactionLoadResult,
},
accounts_db::{
AccountShrinkThreshold, AccountsDbConfig, IncludeSlotInHash, SnapshotStorages,
ACCOUNTS_DB_CONFIG_FOR_BENCHMARKS, ACCOUNTS_DB_CONFIG_FOR_TESTING,
AccountShrinkThreshold, AccountsDbConfig, CalcAccountsHashDataSource,
IncludeSlotInHash, SnapshotStorages, ACCOUNTS_DB_CONFIG_FOR_BENCHMARKS,
ACCOUNTS_DB_CONFIG_FOR_TESTING,
},
accounts_index::{AccountSecondaryIndexes, IndexKey, ScanConfig, ScanResult, ZeroLamport},
accounts_update_notifier_interface::AccountsUpdateNotifier,
Expand Down Expand Up @@ -6980,12 +6981,12 @@ impl Bank {

pub fn update_accounts_hash_with_index_option(
&self,
use_index: bool,
data_source: CalcAccountsHashDataSource,
mut debug_verify: bool,
is_startup: bool,
) -> Hash {
let (hash, total_lamports) = self.rc.accounts.accounts_db.update_accounts_hash(
use_index,
data_source,
debug_verify,
self.slot(),
&self.ancestors,
Expand All @@ -7007,7 +7008,7 @@ impl Bank {
// Run both versions of the calculation to attempt to get more info.
debug_verify = true;
self.rc.accounts.accounts_db.update_accounts_hash(
use_index,
data_source,
debug_verify,
self.slot(),
&self.ancestors,
Expand All @@ -7029,7 +7030,7 @@ impl Bank {
}

pub fn update_accounts_hash(&self) -> Hash {
self.update_accounts_hash_with_index_option(true, false, false)
self.update_accounts_hash_with_index_option(CalcAccountsHashDataSource::Index, false, false)
}

/// A snapshot bank should be purged of 0 lamport accounts which are not part of the hash
Expand Down

0 comments on commit 27269d8

Please sign in to comment.