From fd00e5cb35d5977aa05aff198850a747177a45ca Mon Sep 17 00:00:00 2001 From: Trent Nelson Date: Thu, 5 Mar 2020 07:40:26 -0700 Subject: [PATCH] Store FeeCalculator with blockhash in nonce accounts (#8650) * Copy current state version to v0 * Add `FeeCalculator` to nonce state * fixup compile * Dump v0 handling... Since we new account data is all zeros, new `Current` versioned accounts look like v0. We could hack around this with some data size checks, but the `account_utils::*State` traits are applied to `Account`, not the state data, so we're kind SOL... * Create more representative test `RecentBlockhashes` * Improve CLI nonce account display Co-Authored-By: Michael Vines * Fix that last bank test... * clippy/fmt Co-authored-by: Michael Vines --- cli/src/cli.rs | 2 + cli/src/nonce.rs | 9 ++++ core/src/transaction_status_service.rs | 20 +++++---- runtime/src/accounts.rs | 31 ++++++++------ runtime/src/bank.rs | 46 ++++++++++++--------- runtime/src/lib.rs | 2 +- runtime/src/nonce_utils.rs | 22 ++++++---- runtime/src/system_instruction_processor.rs | 36 ++++++++-------- sdk/src/nonce/account.rs | 28 +++++++++---- sdk/src/nonce/state/current.rs | 7 ++-- sdk/src/sysvar/recent_blockhashes.rs | 11 +++-- 11 files changed, 133 insertions(+), 81 deletions(-) diff --git a/cli/src/cli.rs b/cli/src/cli.rs index 42ef1f843..6a2e8bf82 100644 --- a/cli/src/cli.rs +++ b/cli/src/cli.rs @@ -3269,6 +3269,7 @@ mod tests { nonce::state::Versions::new_current(nonce::State::Initialized(nonce::state::Data { authority: config.signers[0].pubkey(), blockhash, + fee_calculator: FeeCalculator::default(), })); let nonce_response = json!(Response { context: RpcResponseContext { slot: 1 }, @@ -3297,6 +3298,7 @@ mod tests { nonce::state::Versions::new_current(nonce::State::Initialized(nonce::state::Data { authority: bob_pubkey, blockhash, + fee_calculator: FeeCalculator::default(), })); let nonce_authority_response = json!(Response { context: RpcResponseContext { slot: 1 }, diff --git a/cli/src/nonce.rs b/cli/src/nonce.rs index 807ce12d0..c21cb58ed 100644 --- a/cli/src/nonce.rs +++ b/cli/src/nonce.rs @@ -569,10 +569,15 @@ pub fn process_show_nonce_account( match data { Some(ref data) => { println!("Nonce: {}", data.blockhash); + println!( + "Fee: {} lamports per signature", + data.fee_calculator.lamports_per_signature + ); println!("Authority: {}", data.authority); } None => { println!("Nonce: uninitialized"); + println!("Fees: uninitialized"); println!("Authority: uninitialized"); } } @@ -626,6 +631,7 @@ mod tests { use crate::cli::{app, parse_command}; use solana_sdk::{ account::Account, + fee_calculator::FeeCalculator, hash::hash, nonce::{self, State}, signature::{read_keypair_file, write_keypair, Keypair, Signer}, @@ -906,6 +912,7 @@ mod tests { let data = Versions::new_current(State::Initialized(nonce::state::Data { authority: nonce_pubkey, blockhash, + fee_calculator: FeeCalculator::default(), })); let valid = Account::new_data(1, &data, &system_program::ID); assert!(check_nonce_account(&valid.unwrap(), &nonce_pubkey, &blockhash).is_ok()); @@ -929,6 +936,7 @@ mod tests { let data = Versions::new_current(State::Initialized(nonce::state::Data { authority: nonce_pubkey, blockhash: hash(b"invalid"), + fee_calculator: FeeCalculator::default(), })); let invalid_hash = Account::new_data(1, &data, &system_program::ID); assert_eq!( @@ -939,6 +947,7 @@ mod tests { let data = Versions::new_current(State::Initialized(nonce::state::Data { authority: Pubkey::new_rand(), blockhash, + fee_calculator: FeeCalculator::default(), })); let invalid_authority = Account::new_data(1, &data, &system_program::ID); assert_eq!( diff --git a/core/src/transaction_status_service.rs b/core/src/transaction_status_service.rs index 0e9e16f90..2b6f4749e 100644 --- a/core/src/transaction_status_service.rs +++ b/core/src/transaction_status_service.rs @@ -1,7 +1,10 @@ use crossbeam_channel::{Receiver, RecvTimeoutError}; use solana_client::rpc_response::RpcTransactionStatus; use solana_ledger::{blockstore::Blockstore, blockstore_processor::TransactionStatusBatch}; -use solana_runtime::bank::{Bank, HashAgeKind}; +use solana_runtime::{ + bank::{Bank, HashAgeKind}, + nonce_utils, +}; use std::{ sync::{ atomic::{AtomicBool, Ordering}, @@ -59,14 +62,13 @@ impl TransactionStatusService { .zip(balances.post_balances) { if Bank::can_commit(&status) && !transaction.signatures.is_empty() { - let fee_hash = if let Some(HashAgeKind::DurableNonce(_, _)) = hash_age_kind { - bank.last_blockhash() - } else { - transaction.message().recent_blockhash - }; - let fee_calculator = bank - .get_fee_calculator(&fee_hash) - .expect("FeeCalculator must exist"); + let fee_calculator = match hash_age_kind { + Some(HashAgeKind::DurableNonce(_, account)) => { + nonce_utils::fee_calculator_of(&account) + } + _ => bank.get_fee_calculator(&transaction.message().recent_blockhash), + } + .expect("FeeCalculator must exist"); let fee = fee_calculator.calculate_fee(transaction.message()); blockstore .write_transaction_status( diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 8413bb01e..62b1230fe 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -6,7 +6,7 @@ use crate::{ append_vec::StoredAccount, bank::{HashAgeKind, TransactionProcessResult}, blockhash_queue::BlockhashQueue, - nonce_utils::prepare_if_nonce_account, + nonce_utils, rent_collector::RentCollector, system_instruction_processor::{get_system_account_kind, SystemAccountKind}, transaction_utils::OrderedIterator, @@ -265,13 +265,15 @@ impl Accounts { .zip(lock_results.into_iter()) .map(|etx| match etx { (tx, (Ok(()), hash_age_kind)) => { - let fee_hash = if let Some(HashAgeKind::DurableNonce(_, _)) = hash_age_kind { - hash_queue.last_hash() - } else { - tx.message().recent_blockhash + let fee_calculator = match hash_age_kind.as_ref() { + Some(HashAgeKind::DurableNonce(_, account)) => { + nonce_utils::fee_calculator_of(account) + } + _ => hash_queue + .get_fee_calculator(&tx.message().recent_blockhash) + .cloned(), }; - let fee = if let Some(fee_calculator) = hash_queue.get_fee_calculator(&fee_hash) - { + let fee = if let Some(fee_calculator) = fee_calculator { fee_calculator.calculate_fee(tx.message()) } else { return (Err(TransactionError::BlockhashNotFound), hash_age_kind); @@ -630,7 +632,13 @@ impl Accounts { .enumerate() .zip(acc.0.iter_mut()) { - prepare_if_nonce_account(account, key, res, maybe_nonce, last_blockhash); + nonce_utils::prepare_if_nonce_account( + account, + key, + res, + maybe_nonce, + last_blockhash, + ); if message.is_writable(i) { if account.rent_epoch == 0 { account.rent_epoch = rent_collector.epoch; @@ -921,10 +929,9 @@ mod tests { nonce.pubkey(), Account::new_data( min_balance * 2, - &nonce::State::Initialized(nonce::state::Data { - authority: Pubkey::default(), - blockhash: Hash::default(), - }), + &nonce::state::Versions::new_current(nonce::State::Initialized( + nonce::state::Data::default(), + )), &system_program::id(), ) .unwrap(), diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index eb754ed7f..cd9b1f66c 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -1409,19 +1409,20 @@ impl Bank { let results = OrderedIterator::new(txs, iteration_order) .zip(executed.iter()) .map(|(tx, (res, hash_age_kind))| { - let is_durable_nonce = hash_age_kind - .as_ref() - .map(|hash_age_kind| hash_age_kind.is_durable_nonce()) - .unwrap_or(false); - let fee_hash = if is_durable_nonce { - self.last_blockhash() - } else { - tx.message().recent_blockhash + let (fee_calculator, is_durable_nonce) = match hash_age_kind { + Some(HashAgeKind::DurableNonce(_, account)) => { + (nonce_utils::fee_calculator_of(account), true) + } + _ => ( + hash_queue + .get_fee_calculator(&tx.message().recent_blockhash) + .cloned(), + false, + ), }; - let fee = hash_queue - .get_fee_calculator(&fee_hash) - .ok_or(TransactionError::BlockhashNotFound)? - .calculate_fee(tx.message()); + let fee_calculator = fee_calculator.ok_or(TransactionError::BlockhashNotFound)?; + + let fee = fee_calculator.calculate_fee(tx.message()); let message = tx.message(); match *res { @@ -3417,10 +3418,9 @@ mod tests { let nonce = Keypair::new(); let nonce_account = Account::new_data( min_balance + 42, - &nonce::State::Initialized(nonce::state::Data { - authority: Pubkey::default(), - blockhash: Hash::default(), - }), + &nonce::state::Versions::new_current(nonce::State::Initialized( + nonce::state::Data::default(), + )), &system_program::id(), ) .unwrap(); @@ -5153,6 +5153,13 @@ mod tests { genesis_cfg_fn(&mut genesis_config); let mut bank = Arc::new(Bank::new(&genesis_config)); + // Banks 0 and 1 have no fees, wait two blocks before + // initializing our nonce accounts + for _ in 0..2 { + goto_end_of_slot(Arc::get_mut(&mut bank).unwrap()); + bank = Arc::new(new_from_parent(&bank)); + } + let (custodian_keypair, nonce_keypair) = nonce_setup( &mut bank, &mint_keypair, @@ -5276,10 +5283,9 @@ mod tests { let nonce = Keypair::new(); let nonce_account = Account::new_data( 42424242, - &nonce::State::Initialized(nonce::state::Data { - authority: Pubkey::default(), - blockhash: Hash::default(), - }), + &nonce::state::Versions::new_current(nonce::State::Initialized( + nonce::state::Data::default(), + )), &system_program::id(), ) .unwrap(); diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 197c3f27c..45ae90ce8 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -10,7 +10,7 @@ pub mod genesis_utils; pub mod loader_utils; pub mod message_processor; mod native_loader; -mod nonce_utils; +pub mod nonce_utils; pub mod rent_collector; mod serde_utils; pub mod stakes; diff --git a/runtime/src/nonce_utils.rs b/runtime/src/nonce_utils.rs index 7218c3fd4..f8cbcfb99 100644 --- a/runtime/src/nonce_utils.rs +++ b/runtime/src/nonce_utils.rs @@ -1,6 +1,7 @@ use solana_sdk::{ account::Account, account_utils::StateMut, + fee_calculator::FeeCalculator, hash::Hash, instruction::CompiledInstruction, nonce::{self, state::Versions, State}, @@ -66,7 +67,7 @@ pub fn prepare_if_nonce_account( if let State::Initialized(ref data) = state { let new_data = Versions::new_current(State::Initialized(nonce::state::Data { blockhash: *last_blockhash, - ..*data + ..data.clone() })); account.set_state(&new_data).unwrap(); } @@ -74,6 +75,16 @@ pub fn prepare_if_nonce_account( } } +pub fn fee_calculator_of(account: &Account) -> Option { + let state = StateMut::::state(account) + .ok()? + .convert_to_current(); + match state { + State::Initialized(data) => Some(data.fee_calculator), + _ => None, + } +} + #[cfg(test)] mod tests { use super::*; @@ -244,10 +255,7 @@ mod tests { } fn create_accounts_prepare_if_nonce_account() -> (Pubkey, Account, Account, Hash) { - let data = Versions::new_current(State::Initialized(nonce::state::Data { - authority: Pubkey::default(), - blockhash: Hash::default(), - })); + 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 { lamports: 43, @@ -300,8 +308,8 @@ mod tests { let mut expect_account = post_account.clone(); let data = Versions::new_current(State::Initialized(nonce::state::Data { - authority: Pubkey::default(), blockhash: last_blockhash, + ..nonce::state::Data::default() })); expect_account.set_state(&data).unwrap(); @@ -360,8 +368,8 @@ mod tests { expect_account .set_state(&Versions::new_current(State::Initialized( nonce::state::Data { - authority: Pubkey::default(), blockhash: last_blockhash, + ..nonce::state::Data::default() }, ))) .unwrap(); diff --git a/runtime/src/system_instruction_processor.rs b/runtime/src/system_instruction_processor.rs index 7c7df51e9..945a11d11 100644 --- a/runtime/src/system_instruction_processor.rs +++ b/runtime/src/system_instruction_processor.rs @@ -306,8 +306,13 @@ pub fn get_system_account_kind(account: &Account) -> Option { if system_program::check_id(&account.owner) { if account.data.is_empty() { Some(SystemAccountKind::System) - } else if let Ok(nonce::State::Initialized(_)) = account.state() { - Some(SystemAccountKind::Nonce) + } else if account.data.len() == nonce::State::size() { + match account.state().ok()? { + nonce::state::Versions::Current(state) => match *state { + nonce::State::Initialized(_) => Some(SystemAccountKind::Nonce), + _ => None, + }, + } } else { None } @@ -735,10 +740,9 @@ mod tests { let nonce = Pubkey::new_rand(); let nonce_account = Account::new_ref_data( 42, - &nonce::State::Initialized(nonce::state::Data { - authority: Pubkey::default(), - blockhash: Hash::default(), - }), + &nonce::state::Versions::new_current(nonce::State::Initialized( + nonce::state::Data::default(), + )), &system_program::id(), ) .unwrap(); @@ -877,10 +881,10 @@ mod tests { let from = Pubkey::new_rand(); let from_account = Account::new_ref_data( 100, - &nonce::State::Initialized(nonce::state::Data { + &nonce::state::Versions::new_current(nonce::State::Initialized(nonce::state::Data { authority: from, - blockhash: Hash::default(), - }), + ..nonce::state::Data::default() + })), &system_program::id(), ) .unwrap(); @@ -1382,10 +1386,9 @@ mod tests { fn test_get_system_account_kind_nonce_ok() { let nonce_account = Account::new_data( 42, - &nonce::State::Initialized(nonce::state::Data { - authority: Pubkey::default(), - blockhash: Hash::default(), - }), + &nonce::state::Versions::new_current(nonce::State::Initialized( + nonce::state::Data::default(), + )), &system_program::id(), ) .unwrap(); @@ -1413,10 +1416,9 @@ mod tests { fn test_get_system_account_kind_nonsystem_owner_with_nonce_data_fail() { let nonce_account = Account::new_data( 42, - &nonce::State::Initialized(nonce::state::Data { - authority: Pubkey::default(), - blockhash: Hash::default(), - }), + &nonce::state::Versions::new_current(nonce::State::Initialized( + nonce::state::Data::default(), + )), &Pubkey::new_rand(), ) .unwrap(); diff --git a/sdk/src/nonce/account.rs b/sdk/src/nonce/account.rs index 1ecbc5a03..7caea2db2 100644 --- a/sdk/src/nonce/account.rs +++ b/sdk/src/nonce/account.rs @@ -60,6 +60,7 @@ impl<'a> Account for KeyedAccount<'a> { let new_data = nonce::state::Data { blockhash: recent_blockhash, + fee_calculator: recent_blockhashes[0].fee_calculator.clone(), ..data }; self.set_state(&Versions::new_current(State::Initialized(new_data))) @@ -127,6 +128,7 @@ impl<'a> Account for KeyedAccount<'a> { let data = nonce::state::Data { authority: *nonce_authority, blockhash: recent_blockhashes[0].blockhash, + fee_calculator: recent_blockhashes[0].fee_calculator.clone(), }; self.set_state(&Versions::new_current(State::Initialized(data))) } @@ -159,7 +161,7 @@ pub fn create_account(lamports: u64) -> RefCell { RefCell::new( account::Account::new_data_with_space( lamports, - &State::Uninitialized, + &Versions::new_current(State::Uninitialized), State::size(), &system_program::id(), ) @@ -225,10 +227,11 @@ mod test { .convert_to_current(); let data = nonce::state::Data { blockhash: recent_blockhashes[0].blockhash, - ..data + fee_calculator: recent_blockhashes[0].fee_calculator.clone(), + ..data.clone() }; // First nonce instruction drives state from Uninitialized to Initialized - assert_eq!(state, State::Initialized(data)); + assert_eq!(state, State::Initialized(data.clone())); let recent_blockhashes = create_test_recent_blockhashes(63); keyed_account .advance_nonce_account(&recent_blockhashes, &signers) @@ -238,10 +241,11 @@ mod test { .convert_to_current(); let data = nonce::state::Data { blockhash: recent_blockhashes[0].blockhash, - ..data + fee_calculator: recent_blockhashes[0].fee_calculator.clone(), + ..data.clone() }; // Second nonce instruction consumes and replaces stored nonce - assert_eq!(state, State::Initialized(data)); + assert_eq!(state, State::Initialized(data.clone())); let recent_blockhashes = create_test_recent_blockhashes(31); keyed_account .advance_nonce_account(&recent_blockhashes, &signers) @@ -251,10 +255,11 @@ mod test { .convert_to_current(); let data = nonce::state::Data { blockhash: recent_blockhashes[0].blockhash, - ..data + fee_calculator: recent_blockhashes[0].fee_calculator.clone(), + ..data.clone() }; // Third nonce instruction for fun and profit - assert_eq!(state, State::Initialized(data)); + assert_eq!(state, State::Initialized(data.clone())); with_test_keyed_account(42, false, |to_keyed| { let recent_blockhashes = create_test_recent_blockhashes(0); let withdraw_lamports = keyed_account.account.borrow().lamports; @@ -302,6 +307,7 @@ mod test { let data = nonce::state::Data { authority, blockhash: recent_blockhashes[0].blockhash, + fee_calculator: recent_blockhashes[0].fee_calculator.clone(), }; assert_eq!(state, State::Initialized(data)); let signers = HashSet::new(); @@ -590,8 +596,9 @@ mod test { let data = nonce::state::Data { authority, blockhash: recent_blockhashes[0].blockhash, + fee_calculator: recent_blockhashes[0].fee_calculator.clone(), }; - assert_eq!(state, State::Initialized(data)); + assert_eq!(state, State::Initialized(data.clone())); with_test_keyed_account(42, false, |to_keyed| { let withdraw_lamports = nonce_keyed.account.borrow().lamports - min_lamports; let nonce_expect_lamports = @@ -611,7 +618,8 @@ mod test { .convert_to_current(); let data = nonce::state::Data { blockhash: recent_blockhashes[0].blockhash, - ..data + fee_calculator: recent_blockhashes[0].fee_calculator.clone(), + ..data.clone() }; assert_eq!(state, State::Initialized(data)); assert_eq!(nonce_keyed.account.borrow().lamports, nonce_expect_lamports); @@ -746,6 +754,7 @@ mod test { let data = nonce::state::Data { authority, blockhash: recent_blockhashes[0].blockhash, + fee_calculator: recent_blockhashes[0].fee_calculator.clone(), }; assert_eq!(result, Ok(())); let state = AccountUtilsState::::state(keyed_account) @@ -828,6 +837,7 @@ mod test { let data = nonce::state::Data { authority, blockhash: recent_blockhashes[0].blockhash, + fee_calculator: recent_blockhashes[0].fee_calculator.clone(), }; let result = nonce_account.authorize_nonce_account(&Pubkey::default(), &signers); assert_eq!(result, Ok(())); diff --git a/sdk/src/nonce/state/current.rs b/sdk/src/nonce/state/current.rs index f4a1c32de..3b3178909 100644 --- a/sdk/src/nonce/state/current.rs +++ b/sdk/src/nonce/state/current.rs @@ -1,14 +1,15 @@ use super::Versions; -use crate::{hash::Hash, pubkey::Pubkey}; +use crate::{fee_calculator::FeeCalculator, hash::Hash, pubkey::Pubkey}; use serde_derive::{Deserialize, Serialize}; -#[derive(Debug, Default, Serialize, Deserialize, PartialEq, Clone, Copy)] +#[derive(Debug, Default, Serialize, Deserialize, PartialEq, Clone)] pub struct Data { pub authority: Pubkey, pub blockhash: Hash, + pub fee_calculator: FeeCalculator, } -#[derive(Debug, Serialize, Deserialize, PartialEq, Clone, Copy)] +#[derive(Debug, Serialize, Deserialize, PartialEq, Clone)] pub enum State { Uninitialized, Initialized(Data), diff --git a/sdk/src/sysvar/recent_blockhashes.rs b/sdk/src/sysvar/recent_blockhashes.rs index d72ae0859..c2a8b26d0 100644 --- a/sdk/src/sysvar/recent_blockhashes.rs +++ b/sdk/src/sysvar/recent_blockhashes.rs @@ -144,12 +144,17 @@ where pub fn create_test_recent_blockhashes(start: usize) -> RecentBlockhashes { let blocks: Vec<_> = (start..start + MAX_ENTRIES) - .map(|i| (i as u64, hash(&bincode::serialize(&i).unwrap()))) + .map(|i| { + ( + i as u64, + hash(&bincode::serialize(&i).unwrap()), + FeeCalculator::new(i as u64 * 100), + ) + }) .collect(); - let def_fees = FeeCalculator::default(); let bhq: Vec<_> = blocks .iter() - .map(|(i, hash)| IterItem(*i, hash, &def_fees)) + .map(|(i, hash, fee_calc)| IterItem(*i, hash, fee_calc)) .collect(); RecentBlockhashes::from_iter(bhq.into_iter()) }