Fix bank hash not changing when no internal state has changed (#7052)
* Fix bank hash not changing when no internal state has changed * Fix unnecessary call to hash_internal_state * Add blockhash into the bank_hash * Add blockhash into the bank_hash and update tests * Refactor accounts_db slot_hashes * More clarity in comments * Add clippy suggestion * Grammar * Fix compile after clippy made me break it * Schooled by clippy
This commit is contained in:
parent
d2ed921bc6
commit
42da1ce4e2
|
@ -454,12 +454,7 @@ impl Accounts {
|
||||||
|
|
||||||
pub fn hash_internal_state(&self, slot_id: Slot) -> Option<BankHash> {
|
pub fn hash_internal_state(&self, slot_id: Slot) -> Option<BankHash> {
|
||||||
let slot_hashes = self.accounts_db.slot_hashes.read().unwrap();
|
let slot_hashes = self.accounts_db.slot_hashes.read().unwrap();
|
||||||
let slot_hash = slot_hashes.get(&slot_id)?;
|
slot_hashes.get(&slot_id).cloned()
|
||||||
if slot_hash.0 {
|
|
||||||
Some(slot_hash.1)
|
|
||||||
} else {
|
|
||||||
None
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// This function will prevent multiple threads from modifying the same account state at the
|
/// This function will prevent multiple threads from modifying the same account state at the
|
||||||
|
|
|
@ -387,7 +387,7 @@ pub struct AccountsDB {
|
||||||
min_num_stores: usize,
|
min_num_stores: usize,
|
||||||
|
|
||||||
/// slot to BankHash and a status flag to indicate if the hash has been initialized or not
|
/// slot to BankHash and a status flag to indicate if the hash has been initialized or not
|
||||||
pub slot_hashes: RwLock<HashMap<Slot, (bool, BankHash)>>,
|
pub slot_hashes: RwLock<HashMap<Slot, BankHash>>,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl Default for AccountsDB {
|
impl Default for AccountsDB {
|
||||||
|
@ -512,7 +512,7 @@ impl AccountsDB {
|
||||||
let version: u64 = deserialize_from(&mut stream)
|
let version: u64 = deserialize_from(&mut stream)
|
||||||
.map_err(|_| AccountsDB::get_io_error("write version deserialize error"))?;
|
.map_err(|_| AccountsDB::get_io_error("write version deserialize error"))?;
|
||||||
|
|
||||||
let slot_hash: (Slot, (bool, BankHash)) = deserialize_from(&mut stream)
|
let slot_hash: (Slot, BankHash) = deserialize_from(&mut stream)
|
||||||
.map_err(|_| AccountsDB::get_io_error("slot hashes deserialize error"))?;
|
.map_err(|_| AccountsDB::get_io_error("slot hashes deserialize error"))?;
|
||||||
self.slot_hashes
|
self.slot_hashes
|
||||||
.write()
|
.write()
|
||||||
|
@ -647,7 +647,7 @@ impl AccountsDB {
|
||||||
let hash = *slot_hashes
|
let hash = *slot_hashes
|
||||||
.get(&parent_slot)
|
.get(&parent_slot)
|
||||||
.expect("accounts_db::set_hash::no parent slot");
|
.expect("accounts_db::set_hash::no parent slot");
|
||||||
slot_hashes.insert(slot, (false, hash.1));
|
slot_hashes.insert(slot, hash);
|
||||||
}
|
}
|
||||||
|
|
||||||
pub fn load(
|
pub fn load(
|
||||||
|
@ -859,7 +859,7 @@ impl AccountsDB {
|
||||||
hash_state.xor(hash);
|
hash_state.xor(hash);
|
||||||
}
|
}
|
||||||
let slot_hashes = self.slot_hashes.read().unwrap();
|
let slot_hashes = self.slot_hashes.read().unwrap();
|
||||||
if let Some((_, state)) = slot_hashes.get(&slot) {
|
if let Some(state) = slot_hashes.get(&slot) {
|
||||||
hash_state == *state
|
hash_state == *state
|
||||||
} else {
|
} else {
|
||||||
false
|
false
|
||||||
|
@ -868,11 +868,8 @@ impl AccountsDB {
|
||||||
|
|
||||||
pub fn xor_in_hash_state(&self, slot_id: Slot, hash: BankHash) {
|
pub fn xor_in_hash_state(&self, slot_id: Slot, hash: BankHash) {
|
||||||
let mut slot_hashes = self.slot_hashes.write().unwrap();
|
let mut slot_hashes = self.slot_hashes.write().unwrap();
|
||||||
let slot_hash_state = slot_hashes
|
let slot_hash_state = slot_hashes.entry(slot_id).or_insert_with(BankHash::default);
|
||||||
.entry(slot_id)
|
slot_hash_state.xor(hash);
|
||||||
.or_insert((false, BankHash::default()));
|
|
||||||
slot_hash_state.1.xor(hash);
|
|
||||||
slot_hash_state.0 = true;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
fn update_index(
|
fn update_index(
|
||||||
|
|
|
@ -1358,20 +1358,20 @@ impl Bank {
|
||||||
/// Hash the `accounts` HashMap. This represents a validator's interpretation
|
/// Hash the `accounts` HashMap. This represents a validator's interpretation
|
||||||
/// of the delta of the ledger since the last vote and up to now
|
/// of the delta of the ledger since the last vote and up to now
|
||||||
fn hash_internal_state(&self) -> Hash {
|
fn hash_internal_state(&self) -> Hash {
|
||||||
// If there are no accounts, return the same hash as we did before
|
// If there are no accounts, return the hash of the previous state and the latest blockhash
|
||||||
// checkpointing.
|
let accounts_delta_hash = self
|
||||||
if let Some(accounts_delta_hash) = self.rc.accounts.hash_internal_state(self.slot()) {
|
.rc
|
||||||
let mut signature_count_buf = [0u8; 8];
|
.accounts
|
||||||
LittleEndian::write_u64(&mut signature_count_buf[..], self.signature_count() as u64);
|
.hash_internal_state(self.slot())
|
||||||
|
.expect("No accounts delta was found for this bank, that should not be possible");
|
||||||
hashv(&[
|
let mut signature_count_buf = [0u8; 8];
|
||||||
&self.parent_hash.as_ref(),
|
LittleEndian::write_u64(&mut signature_count_buf[..], self.signature_count() as u64);
|
||||||
&accounts_delta_hash.as_ref(),
|
hashv(&[
|
||||||
&signature_count_buf,
|
self.parent_hash.as_ref(),
|
||||||
])
|
accounts_delta_hash.as_ref(),
|
||||||
} else {
|
&signature_count_buf,
|
||||||
self.parent_hash
|
self.last_blockhash().as_ref(),
|
||||||
}
|
])
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Recalculate the hash_internal_state from the account stores. Would be used to verify a
|
/// Recalculate the hash_internal_state from the account stores. Would be used to verify a
|
||||||
|
@ -2542,9 +2542,9 @@ mod tests {
|
||||||
bank1.transfer(1_000, &mint_keypair, &pubkey).unwrap();
|
bank1.transfer(1_000, &mint_keypair, &pubkey).unwrap();
|
||||||
assert_eq!(bank0.hash_internal_state(), bank1.hash_internal_state());
|
assert_eq!(bank0.hash_internal_state(), bank1.hash_internal_state());
|
||||||
|
|
||||||
// Checkpointing should not change its state
|
// Checkpointing should always result in a new state
|
||||||
let bank2 = new_from_parent(&Arc::new(bank1));
|
let bank2 = new_from_parent(&Arc::new(bank1));
|
||||||
assert_eq!(bank0.hash_internal_state(), bank2.hash_internal_state());
|
assert_ne!(bank0.hash_internal_state(), bank2.hash_internal_state());
|
||||||
|
|
||||||
let pubkey2 = Pubkey::new_rand();
|
let pubkey2 = Pubkey::new_rand();
|
||||||
info!("transfer 2 {}", pubkey2);
|
info!("transfer 2 {}", pubkey2);
|
||||||
|
@ -2563,9 +2563,12 @@ mod tests {
|
||||||
bank0.transfer(1_000, &mint_keypair, &pubkey).unwrap();
|
bank0.transfer(1_000, &mint_keypair, &pubkey).unwrap();
|
||||||
|
|
||||||
let bank0_state = bank0.hash_internal_state();
|
let bank0_state = bank0.hash_internal_state();
|
||||||
// Checkpointing should not change its state
|
let bank0 = Arc::new(bank0);
|
||||||
let bank2 = new_from_parent(&Arc::new(bank0));
|
// Checkpointing should result in a new state
|
||||||
assert_eq!(bank0_state, bank2.hash_internal_state());
|
let bank2 = new_from_parent(&bank0);
|
||||||
|
assert_ne!(bank0_state, bank2.hash_internal_state());
|
||||||
|
// Checkpointing should never modify the checkpoint's state
|
||||||
|
assert_eq!(bank0_state, bank0.hash_internal_state());
|
||||||
|
|
||||||
let pubkey2 = Pubkey::new_rand();
|
let pubkey2 = Pubkey::new_rand();
|
||||||
info!("transfer 2 {}", pubkey2);
|
info!("transfer 2 {}", pubkey2);
|
||||||
|
@ -2581,7 +2584,7 @@ mod tests {
|
||||||
let bank0 = Arc::new(Bank::new(&genesis_config));
|
let bank0 = Arc::new(Bank::new(&genesis_config));
|
||||||
let initial_state = bank0.hash_internal_state();
|
let initial_state = bank0.hash_internal_state();
|
||||||
let bank1 = Bank::new_from_parent(&bank0.clone(), &Pubkey::default(), 1);
|
let bank1 = Bank::new_from_parent(&bank0.clone(), &Pubkey::default(), 1);
|
||||||
assert_eq!(bank1.hash_internal_state(), initial_state);
|
assert_ne!(bank1.hash_internal_state(), initial_state);
|
||||||
|
|
||||||
info!("transfer bank1");
|
info!("transfer bank1");
|
||||||
let pubkey = Pubkey::new_rand();
|
let pubkey = Pubkey::new_rand();
|
||||||
|
@ -2651,16 +2654,12 @@ mod tests {
|
||||||
|
|
||||||
let bank1 = Bank::new_from_parent(&bank0, &collector_id, 1);
|
let bank1 = Bank::new_from_parent(&bank0, &collector_id, 1);
|
||||||
|
|
||||||
// no delta in bank1, hashes match
|
// no delta in bank1, hashes should always update
|
||||||
assert_eq!(hash0, bank1.hash_internal_state());
|
assert_ne!(hash0, bank1.hash_internal_state());
|
||||||
|
|
||||||
// remove parent
|
// remove parent
|
||||||
bank1.squash();
|
bank1.squash();
|
||||||
assert!(bank1.parents().is_empty());
|
assert!(bank1.parents().is_empty());
|
||||||
|
|
||||||
// hash should still match,
|
|
||||||
// can't use hash_internal_state() after a freeze()...
|
|
||||||
assert_eq!(hash0, bank1.hash());
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Verifies that last ids and accounts are correctly referenced from parent
|
/// Verifies that last ids and accounts are correctly referenced from parent
|
||||||
|
@ -2972,11 +2971,11 @@ mod tests {
|
||||||
|
|
||||||
let bank1 = new_from_parent(&bank);
|
let bank1 = new_from_parent(&bank);
|
||||||
assert_eq!(bank1.is_delta.load(Ordering::Relaxed), false);
|
assert_eq!(bank1.is_delta.load(Ordering::Relaxed), false);
|
||||||
assert_eq!(bank1.hash_internal_state(), bank.hash());
|
assert_ne!(bank1.hash_internal_state(), bank.hash());
|
||||||
// ticks don't make a bank into a delta
|
// ticks don't make a bank into a delta
|
||||||
bank1.register_tick(&Hash::default());
|
bank1.register_tick(&Hash::default());
|
||||||
assert_eq!(bank1.is_delta.load(Ordering::Relaxed), false);
|
assert_eq!(bank1.is_delta.load(Ordering::Relaxed), false);
|
||||||
assert_eq!(bank1.hash_internal_state(), bank.hash());
|
assert_ne!(bank1.hash_internal_state(), bank.hash());
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
@ -3401,4 +3400,16 @@ mod tests {
|
||||||
let last_ts = bank1.last_vote_sync.load(Ordering::Relaxed);
|
let last_ts = bank1.last_vote_sync.load(Ordering::Relaxed);
|
||||||
assert_eq!(last_ts, 1);
|
assert_eq!(last_ts, 1);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_hash_internal_state_unchanged() {
|
||||||
|
let (genesis_config, _) = create_genesis_config(500);
|
||||||
|
let bank0 = Arc::new(Bank::new(&genesis_config));
|
||||||
|
bank0.freeze();
|
||||||
|
let bank0_hash = bank0.hash();
|
||||||
|
let bank1 = Bank::new_from_parent(&bank0, &Pubkey::default(), 1);
|
||||||
|
bank1.freeze();
|
||||||
|
let bank1_hash = bank1.hash();
|
||||||
|
assert_ne!(bank0_hash, bank1_hash);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
Loading…
Reference in New Issue