Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Allow bank hash file to contain details for multiple slots #34516

Merged
merged 7 commits into from
Dec 27, 2023
Merged

Allow bank hash file to contain details for multiple slots #34516

Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6d900be
Minor modifications to several comments
Dec 19, 2023
39e81ee
Rename BankHashDetails ==> BankHashSlotDetails
Dec 19, 2023
0bece87
Allow BankHashDetails to include details for multiple slots
Dec 19, 2023
05e3f3b
Shift the logic for picking filename to BankHashDetails
Dec 19, 2023
b859e8c
Move test logic to helper function
Dec 19, 2023
21e7a82
Update unit test to build outer type
Dec 19, 2023
7ac8858
Remove custom serde rename and tweak some comments
Dec 21, 2023
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
145 changes: 97 additions & 48 deletions runtime/src/bank/bank_hash_details.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,51 @@ use {

#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
pub(crate) struct BankHashDetails {
/// client version
/// The client version
pub version: String,
/// The encoding format for account data buffers
pub account_data_encoding: String,
#[serde(rename = "banks")]
pub bank_hash_details: Vec<BankHashSlotDetails>,
}

impl BankHashDetails {
Copy link
Contributor

@gregcusack gregcusack Dec 20, 2023

Choose a reason for hiding this comment

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

I'd maybe rename this to something like BankHashDetailsSet or something that indicates we are looking at a set (or really a vector) of bank hashes/slots. From the name, it's not totally clear that this holds more than one bank hash

Copy link
Contributor Author

@steviez steviez Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm generally hesitant to use Set or any other specific container name in the type name unless the container is purely that. Ie, something like:

type BankHashDetailsMap = HashMap<Slot, BankHashDetails>

In this instance, we have additional contents.

We are providing (plural) details about each bank, so having BankHashDetails with an inner Vec<BankHashDetail> doesn't fit either (ie the inner Details also needs to be plural).

Both you and @seanyoung had suggestions so maybe we should adjust the name. I see your points but I'm not in love with the alternatives that we've come up with either. I'm just now offering my rebuttal so I'll give you and Sean the chance to read/think/respond. But, if we don't agree on something that we all like, I think we should push forward as-is. A reader could also go inspect the inner type if they want more information. I'll double check comments as well

This type name will not show up in the produced json, and it is only used within this source file at the moment. So, it will be very easy to update down the road, and pushing now will allow subsequent work (potentially by both Greg and Sean, and maybe me too) to build on top.

Thoughts?

Copy link
Contributor

@gregcusack gregcusack Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ya totally get that and agree. your update looks good to me. ship it

Copy link
Contributor

@t-nelson t-nelson Dec 22, 2023

Choose a reason for hiding this comment

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

BankHashDetailsSummary?

Copy link
Contributor Author

@steviez steviez Dec 22, 2023

Choose a reason for hiding this comment

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

BankHashDetailsSummary?

Use this name for the outer, and then use BankHashSlotDetails or BankHashDetails for the inner ?

pub fn new(bank_hash_details: Vec<BankHashSlotDetails>) -> Self {
Self {
version: solana_version::version!().to_string(),
account_data_encoding: "base64".to_string(),
bank_hash_details,
}
}

/// Determines a filename given the currently held bank details
pub fn filename(&self) -> Result<String, String> {
if self.bank_hash_details.is_empty() {
return Err("BankHashDetails does not contains details for any banks".to_string());
}
// From here on, .unwrap() on .first() and .second() is safe as
// self.bank_hash_details is known to be non-empty
let (first_slot, first_hash) = {
let details = self.bank_hash_details.first().unwrap();
(details.slot, &details.bank_hash)
};

let filename = if self.bank_hash_details.len() == 1 {
format!("{first_slot}-{first_hash}.json")
} else {
let (last_slot, last_hash) = {
let details = self.bank_hash_details.last().unwrap();
(details.slot, &details.bank_hash)
};
format!("{first_slot}-{first_hash}_{last_slot}-{last_hash}.json")
};
Ok(filename)
}
}

/// The components that go into a bank hash calculation
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
pub(crate) struct BankHashSlotDetails {
pub slot: Slot,
pub bank_hash: String,
pub parent_bank_hash: String,
Expand All @@ -35,7 +77,7 @@ pub(crate) struct BankHashDetails {
pub accounts: BankHashAccounts,
}

impl BankHashDetails {
impl BankHashSlotDetails {
pub fn new(
slot: Slot,
bank_hash: Hash,
Expand All @@ -46,8 +88,6 @@ impl BankHashDetails {
accounts: BankHashAccounts,
) -> Self {
Self {
version: solana_version::version!().to_string(),
account_data_encoding: "base64".to_string(),
slot,
bank_hash: bank_hash.to_string(),
parent_bank_hash: parent_bank_hash.to_string(),
Expand All @@ -59,7 +99,7 @@ impl BankHashDetails {
}
}

impl TryFrom<&Bank> for BankHashDetails {
impl TryFrom<&Bank> for BankHashSlotDetails {
type Error = String;

fn try_from(bank: &Bank) -> Result<Self, Self::Error> {
Expand Down Expand Up @@ -99,15 +139,16 @@ impl TryFrom<&Bank> for BankHashDetails {
}
}

// Wrap the Vec<...> so we can implement custom Serialize/Deserialize traits on the wrapper type
/// Wrapper around a Vec<_> to facilitate custom Serialize/Deserialize trait
/// implementations.
#[derive(Clone, Debug, Eq, PartialEq)]
pub(crate) struct BankHashAccounts {
pub accounts: Vec<PubkeyHashAccount>,
}

#[derive(Deserialize, Serialize)]
/// Used as an intermediate for serializing and deserializing account fields
/// into a human readable format.
#[derive(Deserialize, Serialize)]
struct SerdeAccount {
pubkey: String,
hash: String,
Expand Down Expand Up @@ -193,24 +234,22 @@ impl<'de> Deserialize<'de> for BankHashAccounts {
}
}

/// Output the components that comprise bank hash
/// 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 details = BankHashDetails::try_from(bank)?;
let slot_details = BankHashSlotDetails::try_from(bank)?;
let details = BankHashDetails::new(vec![slot_details]);

let slot = details.slot;
let hash = &details.bank_hash;
let file_name = format!("{slot}-{hash}.json");
let parent_dir = bank
.rc
.accounts
.accounts_db
.get_base_working_path()
.join("bank_hash_details");
let path = parent_dir.join(file_name);
let path = parent_dir.join(details.filename()?);
// A file with the same name implies the same hash for this slot. Skip
// rewriting a duplicate file in this scenario
if !path.exists() {
info!("writing details of bank {} to {}", slot, path.display());
info!("writing bank hash details file: {}", path.display());

// std::fs::write may fail (depending on platform) if the full directory
// path does not exist. So, call std::fs_create_dir_all first.
Expand All @@ -228,44 +267,54 @@ pub fn write_bank_hash_details_file(bank: &Bank) -> std::result::Result<(), Stri
pub mod tests {
use super::*;

#[test]
fn test_serde_bank_hash_details() {
use solana_sdk::hash::hash;
fn build_details(num_slots: usize) -> BankHashDetails {
use solana_sdk::hash::{hash, hashv};

let slot = 123_456_789;
let signature_count = 314;
let slot_details: Vec<_> = (0..num_slots)
.map(|slot| {
let signature_count = 314;

let account = AccountSharedData::from(Account {
lamports: 123_456_789,
data: vec![0, 9, 1, 8, 2, 7, 3, 6, 4, 5],
owner: Pubkey::new_unique(),
executable: true,
rent_epoch: 123,
});
let account_pubkey = Pubkey::new_unique();
let account_hash = AccountHash(hash("account".as_bytes()));
let accounts = BankHashAccounts {
accounts: vec![PubkeyHashAccount {
pubkey: account_pubkey,
hash: account_hash,
account,
}],
};
let account = AccountSharedData::from(Account {
lamports: 123_456_789,
data: vec![0, 9, 1, 8, 2, 7, 3, 6, 4, 5],
owner: Pubkey::new_unique(),
executable: true,
rent_epoch: 123,
});
let account_pubkey = Pubkey::new_unique();
let account_hash = AccountHash(hash("account".as_bytes()));
let accounts = BankHashAccounts {
accounts: vec![PubkeyHashAccount {
pubkey: account_pubkey,
hash: account_hash,
account,
}],
};

let bank_hash = hash("bank".as_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());
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());

let bank_hash_details = BankHashDetails::new(
slot,
bank_hash,
parent_bank_hash,
accounts_delta_hash,
signature_count,
last_blockhash,
accounts,
);
BankHashSlotDetails::new(
slot as Slot,
bank_hash,
parent_bank_hash,
accounts_delta_hash,
signature_count,
last_blockhash,
accounts,
)
})
.collect();

BankHashDetails::new(slot_details)
}

#[test]
fn test_serde_bank_hash_details() {
let num_slots = 10;
let bank_hash_details = build_details(num_slots);

let serialized_bytes = serde_json::to_vec(&bank_hash_details).unwrap();
let deserialized_bank_hash_details: BankHashDetails =
Expand Down