Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix nonce fee_calculator overwrite (bp #10973) #10976

Merged
merged 1 commit into from
Jul 9, 2020
Merged

Fix nonce fee_calculator overwrite (bp #10973) #10976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 6 additions & 5 deletions runtime/src/accounts.rs
View file
Open in desktop
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
66 changes: 65 additions & 1 deletion runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1548,7 +1548,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 @@ -6635,6 +6635,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::<nonce::state::Versions>::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::<nonce::state::Versions>::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);
Expand Down
84 changes: 51 additions & 33 deletions runtime/src/nonce_utils.rs
View file
Open in desktop
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(),
..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