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

Fix nonce fee_calculator overwrite #10973

Merged
merged 2 commits into from
Jul 9, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Pass fee_calculator to prepare_if_nonce_account; only overwrite in er…
…ror case
  • Loading branch information
Tyera Eulberg committed Jul 9, 2020
commit f6c6e04ff6ff43e0dcd5e86c2d44a66e1d281a2e
11 changes: 6 additions & 5 deletions runtime/src/accounts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -664,15 +665,15 @@ impl Accounts {
res: &[TransactionProcessResult],
loaded: &mut [(Result<TransactionLoadResult>, Option<HashAgeKind>)],
rent_collector: &RentCollector,
last_blockhash: &Hash,
last_blockhash_with_fee_calculator: &(Hash, FeeCalculator),
) {
let accounts_to_store = self.collect_accounts_to_store(
txs,
txs_iteration_order,
res,
loaded,
rent_collector,
last_blockhash,
last_blockhash_with_fee_calculator,
);
self.accounts_db.store(slot, &accounts_to_store);
}
Expand All @@ -698,7 +699,7 @@ impl Accounts {
res: &'a [TransactionProcessResult],
loaded: &'a mut [(Result<TransactionLoadResult>, Option<HashAgeKind>)],
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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions runtime/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -6681,7 +6681,7 @@ mod tests {
}

#[test]
fn test_nonce_fee_calculator_never_changes() {
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));
Expand Down
84 changes: 51 additions & 33 deletions runtime/src/nonce_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Versions>::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::<Versions>::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(),
Copy link
Contributor

Choose a reason for hiding this comment

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

@CriesofCarrots I think this causes diverged bank hash because patched validator and unpatched validator save different things into the AccountsDB if tx_result.is_err().

So, I guess this needs gated release, actually.

FYI: @jstarry @mvines @t-nelson

Copy link
Contributor Author

@CriesofCarrots CriesofCarrots Jul 13, 2020

Choose a reason for hiding this comment

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

Unless we're seeing any fee fluctuation, I don't think this saves anything different if tx_result.is_err(). I think the issue may actually be with the success case, because the blockhash returned from the blockhashes sysvar in system_instruction_processor (and retained in the nonce account as of this pr) may not be the same as that returned from the bank.last_blockhash_with_fee_calculator. @t-nelson , can you confirm?

Copy link
Contributor

Choose a reason for hiding this comment

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

@CriesofCarrots I believe they should match. The RecentBlockhashes sysvar is updated in Bank::new_from_parent() and we don't touch bank.blockhash_queue until the next slot boundary

Copy link
Contributor Author

@CriesofCarrots CriesofCarrots Jul 13, 2020

Choose a reason for hiding this comment

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

@t-nelson see test at CriesofCarrots@5239ed4
I believe there is one tick at the block boundary where that isn't true

..data.clone()
}));
account.set_state(&new_data).unwrap();
}
}
}
}
Expand Down Expand Up @@ -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 {
Expand All @@ -259,6 +261,9 @@ mod tests {
pre_account,
account,
Hash::new(&[1u8; 32]),
FeeCalculator {
lamports_per_signature: 1234,
},
)
}

Expand All @@ -267,15 +272,15 @@ 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
match maybe_nonce {
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)
Expand All @@ -288,37 +293,39 @@ 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(
&mut post_account,
&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;

Expand All @@ -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
Expand All @@ -346,22 +358,28 @@ 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();
expect_account
.set_state(&Versions::new_current(State::Initialized(
nonce::state::Data {
blockhash: last_blockhash,
fee_calculator: last_fee_calculator.clone(),
..nonce::state::Data::default()
},
)))
Expand All @@ -375,7 +393,7 @@ mod tests {
InstructionError::InvalidArgument,
)),
Some((&pre_account_pubkey, &pre_account)),
&last_blockhash,
&(last_blockhash, last_fee_calculator),
&expect_account,
));
}
Expand Down