From 25228ca95705cce122774501141bd32c81126a91 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Thu, 9 Jul 2020 14:22:21 -0600 Subject: [PATCH] Fix nonce fee_calculator overwrite (#10973) * Add failing test * Pass fee_calculator to prepare_if_nonce_account; only overwrite in error case --- runtime/src/accounts.rs | 11 ++--- runtime/src/bank.rs | 66 +++++++++++++++++++++++++++++- runtime/src/nonce_utils.rs | 84 +++++++++++++++++++++++--------------- 3 files changed, 122 insertions(+), 39 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 6b8342821d..76bd9b093a 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -17,6 +17,7 @@ use rayon::slice::ParallelSliceMut; use solana_sdk::{ account::Account, clock::Slot, + fee_calculator::FeeCalculator, hash::Hash, message::Message, native_loader, nonce, @@ -664,7 +665,7 @@ impl Accounts { res: &[TransactionProcessResult], loaded: &mut [(Result, Option)], rent_collector: &RentCollector, - last_blockhash: &Hash, + last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), ) { let accounts_to_store = self.collect_accounts_to_store( txs, @@ -672,7 +673,7 @@ impl Accounts { res, loaded, rent_collector, - last_blockhash, + last_blockhash_with_fee_calculator, ); self.accounts_db.store(slot, &accounts_to_store); } @@ -698,7 +699,7 @@ impl Accounts { res: &'a [TransactionProcessResult], loaded: &'a mut [(Result, Option)], rent_collector: &RentCollector, - last_blockhash: &Hash, + last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), ) -> Vec<(&'a Pubkey, &'a Account)> { let mut accounts = Vec::with_capacity(loaded.len()); for (i, ((raccs, _hash_age_kind), tx)) in loaded @@ -734,7 +735,7 @@ impl Accounts { key, res, maybe_nonce, - last_blockhash, + last_blockhash_with_fee_calculator, ); if message.is_writable(i) { if account.rent_epoch == 0 { @@ -1689,7 +1690,7 @@ mod tests { &loaders, &mut loaded, &rent_collector, - &Hash::default(), + &(Hash::default(), FeeCalculator::default()), ); assert_eq!(collected_accounts.len(), 2); assert!(collected_accounts diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 062edfd9f5..915c3b00e2 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -1578,7 +1578,7 @@ impl Bank { executed, loaded_accounts, &self.rent_collector, - &self.last_blockhash(), + &self.last_blockhash_with_fee_calculator(), ); self.collect_rent(executed, loaded_accounts); @@ -6680,6 +6680,70 @@ mod tests { ); } + #[test] + fn test_nonce_fee_calculator_updates() { + let (mut genesis_config, mint_keypair) = create_genesis_config(1_000_000); + genesis_config.rent.lamports_per_byte_year = 0; + let mut bank = Arc::new(Bank::new(&genesis_config)); + + // Deliberately use bank 0 to initialize nonce account, so that nonce account fee_calculator indicates 0 fees + let (custodian_keypair, nonce_keypair) = + nonce_setup(&mut bank, &mint_keypair, 500_000, 100_000, None).unwrap(); + let custodian_pubkey = custodian_keypair.pubkey(); + let nonce_pubkey = nonce_keypair.pubkey(); + + // Grab the hash and fee_calculator stored in the nonce account + let (stored_nonce_hash, stored_fee_calculator) = bank + .get_account(&nonce_pubkey) + .and_then(|acc| { + let state = + StateMut::::state(&acc).map(|v| v.convert_to_current()); + match state { + Ok(nonce::State::Initialized(ref data)) => { + Some((data.blockhash, data.fee_calculator.clone())) + } + _ => None, + } + }) + .unwrap(); + + // Kick nonce hash off the blockhash_queue + for _ in 0..MAX_RECENT_BLOCKHASHES + 1 { + goto_end_of_slot(Arc::get_mut(&mut bank).unwrap()); + bank = Arc::new(new_from_parent(&bank)); + } + + // Durable Nonce transfer + let nonce_tx = Transaction::new_signed_with_payer( + &[ + system_instruction::advance_nonce_account(&nonce_pubkey, &nonce_pubkey), + system_instruction::transfer(&custodian_pubkey, &Pubkey::new_rand(), 100_000), + ], + Some(&custodian_pubkey), + &[&custodian_keypair, &nonce_keypair], + stored_nonce_hash, + ); + bank.process_transaction(&nonce_tx).unwrap(); + + // Grab the new hash and fee_calculator; both should be updated + let (nonce_hash, fee_calculator) = bank + .get_account(&nonce_pubkey) + .and_then(|acc| { + let state = + StateMut::::state(&acc).map(|v| v.convert_to_current()); + match state { + Ok(nonce::State::Initialized(ref data)) => { + Some((data.blockhash, data.fee_calculator.clone())) + } + _ => None, + } + }) + .unwrap(); + + assert_ne!(stored_nonce_hash, nonce_hash); + assert_ne!(stored_fee_calculator, fee_calculator); + } + #[test] fn test_collect_balances() { let (genesis_config, _mint_keypair) = create_genesis_config(500); diff --git a/runtime/src/nonce_utils.rs b/runtime/src/nonce_utils.rs index 04823ac73d..4fc813e836 100644 --- a/runtime/src/nonce_utils.rs +++ b/runtime/src/nonce_utils.rs @@ -51,25 +51,26 @@ pub fn prepare_if_nonce_account( account_pubkey: &Pubkey, tx_result: &transaction::Result<()>, maybe_nonce: Option<(&Pubkey, &Account)>, - last_blockhash: &Hash, + last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), ) { if let Some((nonce_key, nonce_acc)) = maybe_nonce { if account_pubkey == nonce_key { // Nonce TX failed with an InstructionError. Roll back // its account state if tx_result.is_err() { - *account = nonce_acc.clone() - } - // Since hash_age_kind is DurableNonce, unwrap is safe here - let state = StateMut::::state(nonce_acc) - .unwrap() - .convert_to_current(); - if let State::Initialized(ref data) = state { - let new_data = Versions::new_current(State::Initialized(nonce::state::Data { - blockhash: *last_blockhash, - ..data.clone() - })); - account.set_state(&new_data).unwrap(); + *account = nonce_acc.clone(); + // Since hash_age_kind is DurableNonce, unwrap is safe here + let state = StateMut::::state(nonce_acc) + .unwrap() + .convert_to_current(); + if let State::Initialized(ref data) = state { + let new_data = Versions::new_current(State::Initialized(nonce::state::Data { + blockhash: last_blockhash_with_fee_calculator.0, + fee_calculator: last_blockhash_with_fee_calculator.1.clone(), + ..data.clone() + })); + account.set_state(&new_data).unwrap(); + } } } } @@ -247,7 +248,8 @@ mod tests { }); } - fn create_accounts_prepare_if_nonce_account() -> (Pubkey, Account, Account, Hash) { + fn create_accounts_prepare_if_nonce_account() -> (Pubkey, Account, Account, Hash, FeeCalculator) + { let data = Versions::new_current(State::Initialized(nonce::state::Data::default())); let account = Account::new_data(42, &data, &system_program::id()).unwrap(); let pre_account = Account { @@ -259,6 +261,9 @@ mod tests { pre_account, account, Hash::new(&[1u8; 32]), + FeeCalculator { + lamports_per_signature: 1234, + }, ) } @@ -267,7 +272,7 @@ mod tests { account_pubkey: &Pubkey, tx_result: &transaction::Result<()>, maybe_nonce: Option<(&Pubkey, &Account)>, - last_blockhash: &Hash, + last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), expect_account: &Account, ) -> bool { // Verify expect_account's relationship @@ -275,7 +280,7 @@ mod tests { Some((nonce_pubkey, _nonce_account)) if nonce_pubkey == account_pubkey && tx_result.is_ok() => { - assert_ne!(expect_account, account) + assert_eq!(expect_account, account) // Account update occurs in system_instruction_processor } Some((nonce_pubkey, nonce_account)) if nonce_pubkey == account_pubkey => { assert_ne!(expect_account, nonce_account) @@ -288,22 +293,24 @@ mod tests { account_pubkey, tx_result, maybe_nonce, - last_blockhash, + last_blockhash_with_fee_calculator, ); expect_account == account } #[test] fn test_prepare_if_nonce_account_expected() { - let (pre_account_pubkey, pre_account, mut post_account, last_blockhash) = - create_accounts_prepare_if_nonce_account(); + let ( + pre_account_pubkey, + pre_account, + mut post_account, + last_blockhash, + last_fee_calculator, + ) = create_accounts_prepare_if_nonce_account(); let post_account_pubkey = pre_account_pubkey; let mut expect_account = post_account.clone(); - let data = Versions::new_current(State::Initialized(nonce::state::Data { - blockhash: last_blockhash, - ..nonce::state::Data::default() - })); + let data = Versions::new_current(State::Initialized(nonce::state::Data::default())); expect_account.set_state(&data).unwrap(); assert!(run_prepare_if_nonce_account_test( @@ -311,14 +318,14 @@ mod tests { &post_account_pubkey, &Ok(()), Some((&pre_account_pubkey, &pre_account)), - &last_blockhash, + &(last_blockhash, last_fee_calculator), &expect_account, )); } #[test] fn test_prepare_if_nonce_account_not_nonce_tx() { - let (pre_account_pubkey, _pre_account, _post_account, last_blockhash) = + let (pre_account_pubkey, _pre_account, _post_account, last_blockhash, last_fee_calculator) = create_accounts_prepare_if_nonce_account(); let post_account_pubkey = pre_account_pubkey; @@ -329,15 +336,20 @@ mod tests { &post_account_pubkey, &Ok(()), None, - &last_blockhash, + &(last_blockhash, last_fee_calculator), &expect_account, )); } #[test] - fn test_prepare_if_nonce_naccount_ot_nonce_pubkey() { - let (pre_account_pubkey, pre_account, mut post_account, last_blockhash) = - create_accounts_prepare_if_nonce_account(); + fn test_prepare_if_nonce_account_not_nonce_pubkey() { + let ( + pre_account_pubkey, + pre_account, + mut post_account, + last_blockhash, + last_fee_calculator, + ) = create_accounts_prepare_if_nonce_account(); let expect_account = post_account.clone(); // Wrong key @@ -346,15 +358,20 @@ mod tests { &Pubkey::new(&[1u8; 32]), &Ok(()), Some((&pre_account_pubkey, &pre_account)), - &last_blockhash, + &(last_blockhash, last_fee_calculator), &expect_account, )); } #[test] fn test_prepare_if_nonce_account_tx_error() { - let (pre_account_pubkey, pre_account, mut post_account, last_blockhash) = - create_accounts_prepare_if_nonce_account(); + let ( + pre_account_pubkey, + pre_account, + mut post_account, + last_blockhash, + last_fee_calculator, + ) = create_accounts_prepare_if_nonce_account(); let post_account_pubkey = pre_account_pubkey; let mut expect_account = pre_account.clone(); @@ -362,6 +379,7 @@ mod tests { .set_state(&Versions::new_current(State::Initialized( nonce::state::Data { blockhash: last_blockhash, + fee_calculator: last_fee_calculator.clone(), ..nonce::state::Data::default() }, ))) @@ -375,7 +393,7 @@ mod tests { InstructionError::InvalidArgument, )), Some((&pre_account_pubkey, &pre_account)), - &last_blockhash, + &(last_blockhash, last_fee_calculator), &expect_account, )); }