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

Promotes accounts hash to a strong type #28930

Merged
merged 1 commit into from
Nov 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion accounts-bench/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ fn main() {
}
println!(
"hash,{},{},{},{}%",
results.0,
results.0 .0,
Copy link
Contributor

Choose a reason for hiding this comment

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

fmt here? Not sure what formatter is doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yah, fmt adds a space when there are multiple tuple indirections. Always makes me look twice...

time,
time_store,
(time_store.as_us() as f64 / time.as_us() as f64 * 100.0f64) as u32
Expand Down
18 changes: 9 additions & 9 deletions core/src/accounts_hash_verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use {
solana_gossip::cluster_info::{ClusterInfo, MAX_SNAPSHOT_HASHES},
solana_measure::{measure, measure::Measure},
solana_runtime::{
accounts_hash::{CalcAccountsHashConfig, HashStats},
accounts_hash::{AccountsHash, CalcAccountsHashConfig, HashStats},
epoch_accounts_hash::EpochAccountsHash,
snapshot_config::SnapshotConfig,
snapshot_package::{
Expand Down Expand Up @@ -210,7 +210,7 @@ impl AccountsHashVerifier {
}

/// returns calculated accounts hash
fn calculate_and_verify_accounts_hash(accounts_package: &AccountsPackage) -> Hash {
fn calculate_and_verify_accounts_hash(accounts_package: &AccountsPackage) -> AccountsHash {
let mut measure_hash = Measure::start("hash");
let mut sort_time = Measure::start("sort_storages");
let sorted_storages = SortedStorages::new(&accounts_package.snapshot_storages);
Expand Down Expand Up @@ -313,13 +313,13 @@ impl AccountsHashVerifier {
accounts_hash
}

fn save_epoch_accounts_hash(accounts_package: &AccountsPackage, accounts_hash: Hash) {
fn save_epoch_accounts_hash(accounts_package: &AccountsPackage, accounts_hash: AccountsHash) {
if accounts_package.package_type == AccountsPackageType::EpochAccountsHash {
info!(
"saving epoch accounts hash, slot: {}, hash: {}",
accounts_package.slot, accounts_hash
accounts_package.slot, accounts_hash.0,
);
let epoch_accounts_hash = EpochAccountsHash::new(accounts_hash);
let epoch_accounts_hash = EpochAccountsHash::from(accounts_hash);
accounts_package
.accounts
.accounts_db
Expand All @@ -346,17 +346,17 @@ impl AccountsHashVerifier {
hashes: &mut Vec<(Slot, Hash)>,
exit: &Arc<AtomicBool>,
fault_injection_rate_slots: u64,
accounts_hash: Hash,
accounts_hash: AccountsHash,
) {
if fault_injection_rate_slots != 0
&& accounts_package.slot % fault_injection_rate_slots == 0
{
// For testing, publish an invalid hash to gossip.
let fault_hash = Self::generate_fault_hash(&accounts_hash);
let fault_hash = Self::generate_fault_hash(&accounts_hash.0);
warn!("inserting fault at slot: {}", accounts_package.slot);
hashes.push((accounts_package.slot, fault_hash));
} else {
hashes.push((accounts_package.slot, accounts_hash));
hashes.push((accounts_package.slot, accounts_hash.0));
}

retain_max_n_elements(hashes, MAX_SNAPSHOT_HASHES);
Expand All @@ -378,7 +378,7 @@ impl AccountsHashVerifier {
accounts_package: AccountsPackage,
pending_snapshot_package: Option<&PendingSnapshotPackage>,
snapshot_config: Option<&SnapshotConfig>,
accounts_hash: Hash,
accounts_hash: AccountsHash,
) {
if pending_snapshot_package.is_none()
|| !snapshot_config
Expand Down
2 changes: 1 addition & 1 deletion core/tests/epoch_accounts_hash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ fn test_epoch_accounts_hash_basic(test_environment: TestEnvironment) {
},
)
.unwrap();
expected_epoch_accounts_hash = Some(EpochAccountsHash::new(accounts_hash));
expected_epoch_accounts_hash = Some(EpochAccountsHash::from(accounts_hash));
debug!(
"slot {}, expected epoch accounts hash: {:?}",
bank.slot(),
Expand Down
12 changes: 7 additions & 5 deletions core/tests/snapshots.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use {
PrunedBanksRequestHandler, SnapshotRequestHandler,
},
accounts_db::{self, ACCOUNTS_DB_CONFIG_FOR_TESTING},
accounts_hash::AccountsHash,
accounts_index::AccountSecondaryIndexes,
bank::{Bank, BankSlotDelta},
bank_forks::BankForks,
Expand Down Expand Up @@ -277,13 +278,14 @@ fn run_bank_forks_snapshot_n<F>(
None,
)
.unwrap();
let accounts_hash = last_bank.get_accounts_hash();
solana_runtime::serde_snapshot::reserialize_bank_with_new_accounts_hash(
accounts_package.snapshot_links_dir(),
accounts_package.slot,
&last_bank.get_accounts_hash(),
&accounts_hash,
None,
);
let snapshot_package = SnapshotPackage::new(accounts_package, last_bank.get_accounts_hash());
let snapshot_package = SnapshotPackage::new(accounts_package, accounts_hash);
snapshot_utils::archive_snapshot_package(
&snapshot_package,
&snapshot_config.full_snapshot_archives_dir,
Expand Down Expand Up @@ -527,10 +529,10 @@ fn test_concurrent_snapshot_packaging(
solana_runtime::serde_snapshot::reserialize_bank_with_new_accounts_hash(
accounts_package.snapshot_links_dir(),
accounts_package.slot,
&Hash::default(),
&AccountsHash::default(),
None,
);
let snapshot_package = SnapshotPackage::new(accounts_package, Hash::default());
let snapshot_package = SnapshotPackage::new(accounts_package, AccountsHash::default());
pending_snapshot_package
.lock()
.unwrap()
Expand Down Expand Up @@ -571,7 +573,7 @@ fn test_concurrent_snapshot_packaging(
solana_runtime::serde_snapshot::reserialize_bank_with_new_accounts_hash(
saved_snapshots_dir.path(),
saved_slot,
&Hash::default(),
&AccountsHash::default(),
None,
);

Expand Down
96 changes: 49 additions & 47 deletions runtime/src/accounts_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ use {
accounts_background_service::{DroppedSlotsSender, SendDroppedBankCallback},
accounts_cache::{AccountsCache, CachedAccount, SlotCache},
accounts_hash::{
AccountsHasher, CalcAccountsHashConfig, CalculateHashIntermediate, HashStats,
PreviousPass,
AccountsHash, AccountsHasher, CalcAccountsHashConfig, CalculateHashIntermediate,
HashStats, PreviousPass,
},
accounts_index::{
AccountIndexGetResult, AccountSecondaryIndexes, AccountsIndex, AccountsIndexConfig,
Expand Down Expand Up @@ -1155,7 +1155,7 @@ impl BankHashStats {
#[derive(Clone, Default, Debug, Serialize, Deserialize, PartialEq, Eq, AbiExample)]
pub struct BankHashInfo {
pub accounts_delta_hash: Hash,
pub accounts_hash: Hash,
pub accounts_hash: AccountsHash,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The heart of the change.

pub stats: BankHashStats,
}

Expand Down Expand Up @@ -4987,11 +4987,7 @@ impl AccountsDb {
return;
}

let new_hash_info = BankHashInfo {
accounts_delta_hash: Hash::default(),
accounts_hash: Hash::default(),
stats: BankHashStats::default(),
};
let new_hash_info = BankHashInfo::default();
bank_hashes.insert(slot, new_hash_info);
}

Expand Down Expand Up @@ -6931,7 +6927,7 @@ impl AccountsDb {
&self,
max_slot: Slot,
config: &CalcAccountsHashConfig<'_>,
) -> Result<(Hash, u64), BankHashVerificationError> {
) -> Result<(AccountsHash, u64), BankHashVerificationError> {
use BankHashVerificationError::*;
let mut collect = Measure::start("collect");
let keys: Vec<_> = self
Expand Down Expand Up @@ -7056,10 +7052,11 @@ impl AccountsDb {
);
self.assert_safe_squashing_accounts_hash(max_slot, config.epoch_schedule);

Ok((accumulated_hash, total_lamports))
let accounts_hash = AccountsHash(accumulated_hash);
Ok((accounts_hash, total_lamports))
}

pub fn get_accounts_hash(&self, slot: Slot) -> Hash {
pub fn get_accounts_hash(&self, slot: Slot) -> AccountsHash {
let bank_hashes = self.bank_hashes.read().unwrap();
let bank_hash_info = bank_hashes.get(&slot).unwrap();
bank_hash_info.accounts_hash
Expand All @@ -7071,7 +7068,7 @@ impl AccountsDb {
ancestors: &Ancestors,
debug_verify: bool,
is_startup: bool,
) -> (Hash, u64) {
) -> (AccountsHash, u64) {
self.update_accounts_hash(
CalcAccountsHashDataSource::IndexForTests,
debug_verify,
Expand Down Expand Up @@ -7403,7 +7400,7 @@ impl AccountsDb {
data_source: CalcAccountsHashDataSource,
slot: Slot,
config: &CalcAccountsHashConfig<'_>,
) -> Result<(Hash, u64), BankHashVerificationError> {
) -> Result<(AccountsHash, u64), BankHashVerificationError> {
match data_source {
CalcAccountsHashDataSource::Storages => {
if self.accounts_cache.contains_any_slots(slot) {
Expand Down Expand Up @@ -7448,23 +7445,24 @@ impl AccountsDb {
slot: Slot,
config: CalcAccountsHashConfig<'_>,
expected_capitalization: Option<u64>,
) -> Result<(Hash, u64), BankHashVerificationError> {
let (hash, total_lamports) = self.calculate_accounts_hash(data_source, slot, &config)?;
) -> Result<(AccountsHash, u64), BankHashVerificationError> {
let (accounts_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::IndexForTests => CalcAccountsHashDataSource::Storages,
CalcAccountsHashDataSource::Storages => CalcAccountsHashDataSource::IndexForTests,
};
let (hash_other, total_lamports_other) =
let (accounts_hash_other, total_lamports_other) =
self.calculate_accounts_hash(data_source_other, slot, &config)?;

let success = hash == hash_other
let success = accounts_hash == accounts_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: {:?}, data source: {:?}, slot: {}", hash, hash_other, total_lamports, total_lamports_other, expected_capitalization, data_source, slot);
assert!(success, "calculate_accounts_hash_with_verify mismatch. hashes: {}, {}; lamports: {}, {}; expected lamports: {:?}, data source: {:?}, slot: {}", accounts_hash.0, accounts_hash_other.0, total_lamports, total_lamports_other, expected_capitalization, data_source, slot);
}
Ok((hash, total_lamports))
Ok((accounts_hash, total_lamports))
}

#[allow(clippy::too_many_arguments)]
Expand All @@ -7478,9 +7476,9 @@ impl AccountsDb {
epoch_schedule: &EpochSchedule,
rent_collector: &RentCollector,
is_startup: bool,
) -> (Hash, u64) {
) -> (AccountsHash, u64) {
let check_hash = false;
let (hash, total_lamports) = self
let (accounts_hash, total_lamports) = self
.calculate_accounts_hash_with_verify(
data_source,
debug_verify,
Expand All @@ -7497,15 +7495,15 @@ impl AccountsDb {
expected_capitalization,
)
.unwrap(); // unwrap here will never fail since check_hash = false
self.set_accounts_hash(slot, hash);
(hash, total_lamports)
self.set_accounts_hash(slot, accounts_hash);
(accounts_hash, total_lamports)
}

/// update hash for this slot in the 'bank_hashes' map
pub(crate) fn set_accounts_hash(&self, slot: Slot, hash: Hash) {
pub(crate) fn set_accounts_hash(&self, slot: Slot, accounts_hash: AccountsHash) {
let mut bank_hashes = self.bank_hashes.write().unwrap();
let mut bank_hash_info = bank_hashes.get_mut(&slot).unwrap();
bank_hash_info.accounts_hash = hash;
bank_hash_info.accounts_hash = accounts_hash;
}

/// scan 'storage', return a vec of 'CacheHashDataFile', one per pass
Expand Down Expand Up @@ -7625,7 +7623,7 @@ impl AccountsDb {
config: &CalcAccountsHashConfig<'_>,
storages: &SortedStorages<'_>,
mut stats: HashStats,
) -> Result<(Hash, u64), BankHashVerificationError> {
) -> Result<(AccountsHash, u64), BankHashVerificationError> {
let _guard = self.active_stats.activate(ActiveStatItem::Hash);
stats.oldest_root = storages.range().start;

Expand Down Expand Up @@ -7694,6 +7692,7 @@ impl AccountsDb {
storages.max_slot_inclusive(),
final_result
);
let final_result = (AccountsHash(final_result.0), final_result.1);
Ok(final_result)
};

Expand Down Expand Up @@ -7777,21 +7776,22 @@ impl AccountsDb {
use BankHashVerificationError::*;

let check_hash = false; // this will not be supported anymore
let (calculated_hash, calculated_lamports) = self.calculate_accounts_hash_with_verify(
CalcAccountsHashDataSource::Storages,
test_hash_calculation,
slot,
CalcAccountsHashConfig {
use_bg_thread_pool,
check_hash,
ancestors: Some(ancestors),
epoch_schedule,
rent_collector,
store_detailed_debug_info_on_failure: store_hash_raw_data_for_debug,
full_snapshot: None,
},
None,
)?;
let (calculated_accounts_hash, calculated_lamports) = self
.calculate_accounts_hash_with_verify(
CalcAccountsHashDataSource::Storages,
test_hash_calculation,
slot,
CalcAccountsHashConfig {
use_bg_thread_pool,
check_hash,
ancestors: Some(ancestors),
epoch_schedule,
rent_collector,
store_detailed_debug_info_on_failure: store_hash_raw_data_for_debug,
full_snapshot: None,
},
None,
)?;

if calculated_lamports != total_lamports {
warn!(
Expand All @@ -7806,12 +7806,12 @@ impl AccountsDb {
} else {
let bank_hashes = self.bank_hashes.read().unwrap();
if let Some(found_hash_info) = bank_hashes.get(&slot) {
if calculated_hash == found_hash_info.accounts_hash {
if calculated_accounts_hash == found_hash_info.accounts_hash {
Ok(())
} else {
warn!(
"mismatched bank hash for slot {}: {} (calculated) != {} (expected)",
slot, calculated_hash, found_hash_info.accounts_hash
"mismatched bank hash for slot {}: {:?} (calculated) != {:?} (expected)",
slot, calculated_accounts_hash, found_hash_info.accounts_hash,
);
Err(MismatchedBankHash)
}
Expand Down Expand Up @@ -10624,7 +10624,8 @@ pub mod tests {
)
.unwrap();
let expected_hash = Hash::from_str("GKot5hBsd81kMupNCXHaqbhv3huEbxAFMLnpcX2hniwn").unwrap();
assert_eq!(result, (expected_hash, 0));
let expected_accounts_hash = AccountsHash(expected_hash);
assert_eq!(result, (expected_accounts_hash, 0));
}

#[test]
Expand All @@ -10646,7 +10647,8 @@ pub mod tests {
)
.unwrap();

assert_eq!(result, (expected_hash, sum));
let expected_accounts_hash = AccountsHash(expected_hash);
assert_eq!(result, (expected_accounts_hash, sum));
}

fn sample_storage() -> (SnapshotStorages, usize, Slot) {
Expand Down Expand Up @@ -12934,7 +12936,7 @@ pub mod tests {
let some_bank_hash = Hash::new(&[0xca; HASH_BYTES]);
let bank_hash_info = BankHashInfo {
accounts_delta_hash: some_bank_hash,
accounts_hash: Hash::new(&[0xca; HASH_BYTES]),
accounts_hash: AccountsHash(Hash::new(&[0xca; HASH_BYTES])),
stats: BankHashStats::default(),
};
db.bank_hashes
Expand Down
4 changes: 4 additions & 0 deletions runtime/src/accounts_hash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1079,6 +1079,10 @@ impl AccountsHasher {
}
}

/// Hash of all the accounts
#[derive(Debug, Default, Copy, Clone, Eq, PartialEq, Serialize, Deserialize, AbiExample)]
pub struct AccountsHash(pub Hash);

#[cfg(test)]
pub mod tests {
use {super::*, std::str::FromStr};
Expand Down
Loading