From 4f9679a752fae61e0a11b51d2e35f93e5c5c8a69 Mon Sep 17 00:00:00 2001 From: steviez Date: Tue, 20 Aug 2024 10:01:49 -0500 Subject: [PATCH] Make BankHashDetails use a subtype for bank hash mixins (#2644) The option to only record bank hash, and not the items that factor into the bank hash, was added a while ago. So, all of the items that factor into bank hash are optional. Someone is likely to either care about all of the bank hash mixins or none of them, so move the items into a sub type and make it an Option<_> --- ledger-tool/src/main.rs | 19 ++-- runtime/src/bank/bank_hash_details.rs | 136 ++++++++++---------------- 2 files changed, 58 insertions(+), 97 deletions(-) diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 06f17a55e03a2a..d9a3a60d2f4600 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -571,13 +571,13 @@ fn setup_slot_recording( exit(1); }); - let mut include_bank = false; + let mut include_bank_hash_components = false; let mut include_tx = false; if let Some(args) = arg_matches.values_of("record_slots_config") { for arg in args { match arg { "tx" => include_tx = true, - "accounts" => include_bank = true, + "accounts" => include_bank_hash_components = true, _ => unreachable!(), } } @@ -603,16 +603,11 @@ fn setup_slot_recording( let slot_callback = Arc::new({ let slots = Arc::clone(&slot_details); move |bank: &Bank| { - let mut details = if include_bank { - bank_hash_details::SlotDetails::try_from(bank).unwrap() - } else { - bank_hash_details::SlotDetails { - slot: bank.slot(), - bank_hash: bank.hash().to_string(), - ..Default::default() - } - }; - + let mut details = bank_hash_details::SlotDetails::new_from_bank( + bank, + include_bank_hash_components, + ) + .unwrap(); let mut slots = slots.lock().unwrap(); if let Some(recorded_slot) = slots.iter_mut().find(|f| f.slot == details.slot) { diff --git a/runtime/src/bank/bank_hash_details.rs b/runtime/src/bank/bank_hash_details.rs index 5ab13d85c4d89b..6af468a911916e 100644 --- a/runtime/src/bank/bank_hash_details.rs +++ b/runtime/src/bank/bank_hash_details.rs @@ -104,66 +104,28 @@ impl From for TransactionCommitDetails { } } -/// The components that go into a bank hash calculation for a single bank/slot. #[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize, Default)] pub struct SlotDetails { pub slot: Slot, pub bank_hash: String, - #[serde(skip_serializing_if = "String::is_empty")] - #[serde(default)] + #[serde(skip_serializing_if = "Option::is_none", default, flatten)] + pub bank_hash_components: Option, + #[serde(skip_serializing_if = "Vec::is_empty", default)] + pub transactions: Vec, +} + +/// The components that go into a bank hash calculation for a single bank +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize, Default)] +pub struct BankHashComponents { pub parent_bank_hash: String, - #[serde(skip_serializing_if = "String::is_empty")] - #[serde(default)] pub accounts_delta_hash: String, - #[serde(skip_serializing_if = "u64_is_zero")] - #[serde(default)] pub signature_count: u64, - #[serde(skip_serializing_if = "String::is_empty")] - #[serde(default)] pub last_blockhash: String, - #[serde(skip_serializing_if = "accounts_is_empty")] - #[serde(default)] pub accounts: AccountsDetails, - #[serde(skip_serializing_if = "Vec::is_empty")] - #[serde(default)] - pub transactions: Vec, -} - -fn u64_is_zero(val: &u64) -> bool { - *val == 0 -} - -fn accounts_is_empty(accounts: &AccountsDetails) -> bool { - accounts.accounts.is_empty() } impl SlotDetails { - pub fn new( - slot: Slot, - bank_hash: Hash, - parent_bank_hash: Hash, - accounts_delta_hash: Hash, - signature_count: u64, - last_blockhash: Hash, - accounts: AccountsDetails, - ) -> Self { - Self { - slot, - bank_hash: bank_hash.to_string(), - parent_bank_hash: parent_bank_hash.to_string(), - accounts_delta_hash: accounts_delta_hash.to_string(), - signature_count, - last_blockhash: last_blockhash.to_string(), - accounts, - transactions: Vec::new(), - } - } -} - -impl TryFrom<&Bank> for SlotDetails { - type Error = String; - - fn try_from(bank: &Bank) -> Result { + pub fn new_from_bank(bank: &Bank, include_bank_hash_components: bool) -> Result { let slot = bank.slot(); if !bank.is_frozen() { return Err(format!( @@ -171,26 +133,34 @@ impl TryFrom<&Bank> for SlotDetails { )); } - // This bank is frozen; as a result, we know that the state has been - // hashed which means the delta hash is Some(). So, .unwrap() is safe - let AccountsDeltaHash(accounts_delta_hash) = bank - .rc - .accounts - .accounts_db - .get_accounts_delta_hash(slot) - .unwrap(); - - let accounts = bank.get_accounts_for_bank_hash_details(); + let bank_hash_components = if include_bank_hash_components { + // This bank is frozen; as a result, we know that the state has been + // hashed which means the delta hash is Some(). So, .unwrap() is safe + let AccountsDeltaHash(accounts_delta_hash) = bank + .rc + .accounts + .accounts_db + .get_accounts_delta_hash(slot) + .unwrap(); + let accounts = bank.get_accounts_for_bank_hash_details(); + + Some(BankHashComponents { + parent_bank_hash: bank.parent_hash().to_string(), + accounts_delta_hash: accounts_delta_hash.to_string(), + signature_count: bank.signature_count(), + last_blockhash: bank.last_blockhash().to_string(), + accounts: AccountsDetails { accounts }, + }) + } else { + None + }; - Ok(Self::new( + Ok(Self { slot, - bank.hash(), - bank.parent_hash(), - accounts_delta_hash, - bank.signature_count(), - bank.last_blockhash(), - AccountsDetails { accounts }, - )) + bank_hash: bank.hash().to_string(), + bank_hash_components, + transactions: Vec::new(), + }) } } @@ -291,7 +261,7 @@ impl<'de> Deserialize<'de> for AccountsDetails { /// Output the components that comprise the overall bank hash for the supplied `Bank` pub fn write_bank_hash_details_file(bank: &Bank) -> std::result::Result<(), String> { - let slot_details = SlotDetails::try_from(bank)?; + let slot_details = SlotDetails::new_from_bank(bank, /*include_bank_hash_mixins:*/ true)?; let details = BankHashDetails::new(vec![slot_details]); let parent_dir = bank @@ -328,11 +298,9 @@ pub mod tests { use super::*; fn build_details(num_slots: usize) -> BankHashDetails { - use solana_sdk::hash::{hash, hashv}; - let slot_details: Vec<_> = (0..num_slots) .map(|slot| { - let signature_count = 314; + let slot = slot as u64; let account = AccountSharedData::from(Account { lamports: 123_456_789, @@ -342,7 +310,7 @@ pub mod tests { rent_epoch: 123, }); let account_pubkey = Pubkey::new_unique(); - let account_hash = AccountHash(hash("account".as_bytes())); + let account_hash = AccountHash(solana_sdk::hash::hash("account".as_bytes())); let accounts = AccountsDetails { accounts: vec![PubkeyHashAccount { pubkey: account_pubkey, @@ -351,20 +319,18 @@ pub mod tests { }], }; - let bank_hash = hashv(&["bank".as_bytes(), &slot.to_le_bytes()]); - let parent_bank_hash = hash("parent_bank".as_bytes()); - let accounts_delta_hash = hash("accounts_delta".as_bytes()); - let last_blockhash = hash("last_blockhash".as_bytes()); - - SlotDetails::new( - slot as Slot, - bank_hash, - parent_bank_hash, - accounts_delta_hash, - signature_count, - last_blockhash, - accounts, - ) + SlotDetails { + slot, + bank_hash: format!("bank{slot}"), + bank_hash_components: Some(BankHashComponents { + parent_bank_hash: "parent_bank_hash".into(), + accounts_delta_hash: "accounts_delta_hash".into(), + signature_count: slot + 10, + last_blockhash: "last_blockhash".into(), + accounts, + }), + transactions: vec![], + } }) .collect();