Fix race between setting tick height and calculating accounts hash (#14101)

Co-authored-by: Carl Lin <carl@solana.com>
This commit is contained in:
carllin 2020-12-15 12:45:40 -08:00 committed by GitHub
parent 582418de5e
commit 75e9e321de
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 58 additions and 49 deletions

View File

@ -2558,10 +2558,10 @@ pub(crate) mod tests {
#[test]
fn test_replay_commitment_cache() {
fn leader_vote(bank: &Arc<Bank>, pubkey: &Pubkey) {
fn leader_vote(vote_slot: Slot, bank: &Arc<Bank>, pubkey: &Pubkey) {
let mut leader_vote_account = bank.get_account(&pubkey).unwrap();
let mut vote_state = VoteState::from(&leader_vote_account).unwrap();
vote_state.process_slot_vote_unchecked(bank.slot());
vote_state.process_slot_vote_unchecked(vote_slot);
let versioned = VoteStateVersions::Current(Box::new(vote_state));
VoteState::to(&versioned, &mut leader_vote_account).unwrap();
bank.store_account(&pubkey, &leader_vote_account);
@ -2581,10 +2581,7 @@ pub(crate) mod tests {
}
bank0.freeze();
let arc_bank0 = Arc::new(bank0);
let bank_forks = Arc::new(RwLock::new(BankForks::new_from_banks(
&[arc_bank0.clone()],
0,
)));
let bank_forks = Arc::new(RwLock::new(BankForks::new_from_banks(&[arc_bank0], 0)));
let exit = Arc::new(AtomicBool::new(false));
let block_commitment_cache = Arc::new(RwLock::new(BlockCommitmentCache::default()));
@ -2608,44 +2605,33 @@ pub(crate) mod tests {
.get_block_commitment(1)
.is_none());
let bank1 = Bank::new_from_parent(&arc_bank0, &Pubkey::default(), arc_bank0.slot() + 1);
let _res = bank1.transfer(
10,
&genesis_config_info.mint_keypair,
&solana_sdk::pubkey::new_rand(),
);
for _ in 0..genesis_config.ticks_per_slot {
bank1.register_tick(&Hash::default());
for i in 1..=3 {
let prev_bank = bank_forks.read().unwrap().get(i - 1).unwrap().clone();
let bank = Bank::new_from_parent(&prev_bank, &Pubkey::default(), prev_bank.slot() + 1);
let _res = bank.transfer(
10,
&genesis_config_info.mint_keypair,
&solana_sdk::pubkey::new_rand(),
);
for _ in 0..genesis_config.ticks_per_slot {
bank.register_tick(&Hash::default());
}
bank_forks.write().unwrap().insert(bank);
let arc_bank = bank_forks.read().unwrap().get(i).unwrap().clone();
leader_vote(i - 1, &arc_bank, &leader_voting_pubkey);
ReplayStage::update_commitment_cache(
arc_bank.clone(),
0,
leader_lamports,
&lockouts_sender,
);
arc_bank.freeze();
}
bank1.freeze();
bank_forks.write().unwrap().insert(bank1);
let arc_bank1 = bank_forks.read().unwrap().get(1).unwrap().clone();
leader_vote(&arc_bank1, &leader_voting_pubkey);
ReplayStage::update_commitment_cache(
arc_bank1.clone(),
0,
leader_lamports,
&lockouts_sender,
);
let bank2 = Bank::new_from_parent(&arc_bank1, &Pubkey::default(), arc_bank1.slot() + 1);
let _res = bank2.transfer(
10,
&genesis_config_info.mint_keypair,
&solana_sdk::pubkey::new_rand(),
);
for _ in 0..genesis_config.ticks_per_slot {
bank2.register_tick(&Hash::default());
}
bank2.freeze();
bank_forks.write().unwrap().insert(bank2);
let arc_bank2 = bank_forks.read().unwrap().get(2).unwrap().clone();
leader_vote(&arc_bank2, &leader_voting_pubkey);
ReplayStage::update_commitment_cache(arc_bank2, 0, leader_lamports, &lockouts_sender);
thread::sleep(Duration::from_millis(200));
let mut expected0 = BlockCommitment::default();
expected0.increase_confirmation_stake(2, leader_lamports);
expected0.increase_confirmation_stake(3, leader_lamports);
assert_eq!(
block_commitment_cache
.read()

View File

@ -867,6 +867,8 @@ pub struct Bank {
pub feature_set: Arc<FeatureSet>,
pub drop_callback: RwLock<OptionalDropCallback>,
pub freeze_started: AtomicBool,
}
impl Default for BlockhashQueue {
@ -1022,6 +1024,7 @@ impl Bank {
.as_ref()
.map(|drop_callback| drop_callback.clone_box()),
)),
freeze_started: AtomicBool::new(false),
};
datapoint_info!(
@ -1145,6 +1148,7 @@ impl Bank {
transaction_log_collector: new(),
feature_set: new(),
drop_callback: RwLock::new(OptionalDropCallback(None)),
freeze_started: AtomicBool::new(fields.hash != Hash::default()),
};
bank.finish_init(genesis_config, additional_builtins);
@ -1248,6 +1252,10 @@ impl Bank {
*self.hash.read().unwrap() != Hash::default()
}
pub fn freeze_started(&self) -> bool {
self.freeze_started.load(Relaxed)
}
pub fn status_cache_ancestors(&self) -> Vec<u64> {
let mut roots = self.src.status_cache.read().unwrap().roots().clone();
let min = roots.iter().min().cloned().unwrap_or(0);
@ -1941,6 +1949,17 @@ impl Bank {
}
pub fn freeze(&self) {
// This lock prevents any new commits from BankingStage
// `process_and_record_transactions_locked()` from coming
// in after the last tick is observed. This is because in
// BankingStage, any transaction successfully recorded in
// `record_transactions()` is recorded after this `hash` lock
// is grabbed. At the time of the successful record,
// this means the PoH has not yet reached the last tick,
// so this means freeze() hasn't been called yet. And because
// BankingStage doesn't release this hash lock until both
// record and commit are finished, those transactions will be
// committed before this write lock can be obtained here.
let mut hash = self.hash.write().unwrap();
if *hash == Hash::default() {
@ -1952,6 +1971,7 @@ impl Bank {
self.run_incinerator();
// freeze is a one-way trip, idempotent
self.freeze_started.store(true, Relaxed);
*hash = self.hash_internal_state();
}
}
@ -2145,7 +2165,7 @@ impl Bank {
}
assert!(
!self.is_frozen(),
!self.freeze_started(),
"Can't change frozen bank by adding not-existing new native program ({}, {}). \
Maybe, inconsistent program activation is detected on snapshot restore?",
name,
@ -2272,22 +2292,24 @@ impl Bank {
/// bank will reject transactions using that `hash`.
pub fn register_tick(&self, hash: &Hash) {
assert!(
!self.is_frozen(),
"register_tick() working on a frozen bank!"
!self.freeze_started(),
"register_tick() working on a bank that is already frozen or is undergoing freezing!"
);
inc_new_counter_debug!("bank-register_tick-registered", 1);
// Grab blockhash lock before incrementing tick height so that replay stage does
// not attempt to freeze after observing the last tick and before blockhash is
// updated
let mut w_blockhash_queue = self.blockhash_queue.write().unwrap();
let current_tick_height = self.tick_height.fetch_add(1, Relaxed) as u64;
if self.is_block_boundary(current_tick_height + 1) {
if self.is_block_boundary(self.tick_height.load(Relaxed) + 1) {
w_blockhash_queue.register_hash(hash, &self.fee_calculator);
if self.fix_recent_blockhashes_sysvar_delay() {
self.update_recent_blockhashes_locked(&w_blockhash_queue);
}
}
// ReplayStage will start computing the accounts delta hash when it
// detects the tick height has reached the boundary, so the system
// needs to guarantee all account updates for the slot have been
// committed before this tick height is incremented (like the blockhash
// sysvar above)
self.tick_height.fetch_add(1, Relaxed);
}
pub fn is_complete(&self) -> bool {
@ -3053,8 +3075,8 @@ impl Bank {
signature_count: u64,
) -> TransactionResults {
assert!(
!self.is_frozen(),
"commit_transactions() working on a frozen bank!"
!self.freeze_started(),
"commit_transactions() working on a bank that is already frozen or is undergoing freezing!"
);
self.increment_transaction_count(tx_count);
@ -3768,6 +3790,7 @@ impl Bank {
}
pub fn store_account(&self, pubkey: &Pubkey, account: &Account) {
assert!(!self.freeze_started());
self.rc.accounts.store_slow(self.slot(), pubkey, account);
if Stakes::is_stake(account) {