From 3c6af52a71da80d21250f62d190c827dc480b93d Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Thu, 7 Feb 2019 12:14:10 -0700 Subject: [PATCH] Fix pay-to-self Accounts bug (#2682) * Add failing tests * Fix tests * Plumb AccountLoadedTwice error * Fixup budget cancel actions to not depend on duplicate accounts * Use has_duplicates * Update budget-based golden --- programs/native/budget/src/budget_program.rs | 20 +++++++---- sdk/src/budget_transaction.rs | 6 +++- src/accounts.rs | 37 ++++++++++++++++++++ src/bank.rs | 24 +++++++++++++ src/chacha.rs | 2 +- src/rpc.rs | 3 ++ 6 files changed, 84 insertions(+), 8 deletions(-) diff --git a/programs/native/budget/src/budget_program.rs b/programs/native/budget/src/budget_program.rs index 91d96b2a6d..08e3ef2880 100644 --- a/programs/native/budget/src/budget_program.rs +++ b/programs/native/budget/src/budget_program.rs @@ -149,6 +149,14 @@ impl BudgetProgram { } if let Some(payment) = final_payment { + if let Some(key) = keyed_accounts[0].signer_key() { + if &payment.to == key { + self.pending_budget = None; + keyed_accounts[1].account.tokens -= payment.tokens; + keyed_accounts[0].account.tokens += payment.tokens; + return Ok(()); + } + } if &payment.to != keyed_accounts[2].unsigned_key() { trace!("destination missing"); return Err(BudgetError::DestinationMissing); @@ -494,7 +502,7 @@ mod test { // nothing should be changed because apply witness didn't finalize a payment assert_eq!(accounts[from_account].tokens, 0); assert_eq!(accounts[contract_account].tokens, 1); - // this would be the `to.pubkey()` account + // this is the `to.pubkey()` account assert_eq!(accounts[pay_account].tokens, 0); // Now, cancel the transaction. from gets her funds back @@ -505,11 +513,11 @@ mod test { Hash::default(), ); process_transaction(&tx, &mut accounts).unwrap(); - assert_eq!(accounts[from_account].tokens, 0); + assert_eq!(accounts[from_account].tokens, 1); assert_eq!(accounts[contract_account].tokens, 0); - assert_eq!(accounts[pay_account].tokens, 1); + assert_eq!(accounts[pay_account].tokens, 0); - // try to replay the signature contract + // try to replay the cancel contract let tx = BudgetTransaction::new_signature( &from, contract.pubkey(), @@ -520,9 +528,9 @@ mod test { process_transaction(&tx, &mut accounts), Err(BudgetError::ContractNotPending) ); - assert_eq!(accounts[from_account].tokens, 0); + assert_eq!(accounts[from_account].tokens, 1); assert_eq!(accounts[contract_account].tokens, 0); - assert_eq!(accounts[pay_account].tokens, 1); + assert_eq!(accounts[pay_account].tokens, 0); } #[test] diff --git a/sdk/src/budget_transaction.rs b/sdk/src/budget_transaction.rs index 9aa78c011f..7b5c9da044 100644 --- a/sdk/src/budget_transaction.rs +++ b/sdk/src/budget_transaction.rs @@ -85,9 +85,13 @@ impl BudgetTransaction { last_id: Hash, ) -> Transaction { let instruction = Instruction::ApplySignature; + let mut keys = vec![contract]; + if from_keypair.pubkey() != to { + keys.push(to); + } Transaction::new( from_keypair, - &[contract, to], + &keys, budget_program::id(), &instruction, last_id, diff --git a/src/accounts.rs b/src/accounts.rs index cba1198be5..e5d848f3ee 100644 --- a/src/accounts.rs +++ b/src/accounts.rs @@ -1,6 +1,7 @@ use crate::bank::BankError; use crate::bank::Result; use crate::counter::Counter; +use crate::runtime::has_duplicates; use bincode::serialize; use hashbrown::{HashMap, HashSet}; use log::Level; @@ -21,6 +22,7 @@ pub type InstructionLoaders = Vec>; pub struct ErrorCounters { pub account_not_found: usize, pub account_in_use: usize, + pub account_loaded_twice: usize, pub last_id_not_found: usize, pub last_id_too_old: usize, pub reserve_last_id: usize, @@ -137,6 +139,12 @@ impl AccountsDB { if tx.signatures.is_empty() && tx.fee != 0 { Err(BankError::MissingSignatureForFee) } else { + // Check for unique account keys + if has_duplicates(&tx.account_keys) { + error_counters.account_loaded_twice += 1; + return Err(BankError::AccountLoadedTwice); + } + // There is no way to predict what program will execute without an error // If a fee can pay for execution then the program will be scheduled let mut called_accounts: Vec = vec![]; @@ -784,4 +792,33 @@ mod tests { Err(e) => Err(e).unwrap(), } } + + #[test] + fn test_load_account_pay_to_self() { + let mut accounts: Vec<(Pubkey, Account)> = Vec::new(); + let mut error_counters = ErrorCounters::default(); + + let keypair = Keypair::new(); + let pubkey = keypair.pubkey(); + + let account = Account::new(10, 1, Pubkey::default()); + accounts.push((pubkey, account)); + + let instructions = vec![Instruction::new(0, &(), vec![0, 1])]; + // Simulate pay-to-self transaction, which loads the same account twice + let tx = Transaction::new_with_instructions( + &[&keypair], + &[pubkey], + Hash::default(), + 0, + vec![native_loader::id()], + instructions, + ); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.account_loaded_twice, 1); + assert_eq!(loaded_accounts.len(), 1); + loaded_accounts[0].clone().unwrap_err(); + assert_eq!(loaded_accounts[0], Err(BankError::AccountLoadedTwice)); + } } diff --git a/src/bank.rs b/src/bank.rs index df664456af..e02e775e85 100644 --- a/src/bank.rs +++ b/src/bank.rs @@ -46,6 +46,10 @@ pub enum BankError { /// This Pubkey is being processed in another transaction AccountInUse, + /// Pubkey appears twice in the same transaction, typically in a pay-to-self + /// transaction. + AccountLoadedTwice, + /// Attempt to debit from `Pubkey`, but no found no record of a prior credit. AccountNotFound, @@ -643,6 +647,12 @@ impl Bank { error_counters.insufficient_funds ); } + if 0 != error_counters.account_loaded_twice { + inc_new_counter_info!( + "bank-process_transactions-account_loaded_twice", + error_counters.account_loaded_twice + ); + } (loaded_accounts, executed) } @@ -1852,5 +1862,19 @@ mod tests { assert_eq!(bank.get_balance(&pubkey), 1); } + #[test] + fn test_bank_pay_to_self() { + let (genesis_block, mint_keypair) = GenesisBlock::new(1 + BOOTSTRAP_LEADER_TOKENS); + let key1 = Keypair::new(); + let bank = Bank::new(&genesis_block); + bank.transfer(1, &mint_keypair, key1.pubkey(), genesis_block.last_id()) + .unwrap(); + assert_eq!(bank.get_balance(&key1.pubkey()), 1); + let tx = SystemTransaction::new_move(&key1, key1.pubkey(), 1, genesis_block.last_id(), 0); + let res = bank.process_transactions(&vec![tx.clone()]); + assert_eq!(res.len(), 1); + assert_eq!(bank.get_balance(&key1.pubkey()), 1); + res[0].clone().unwrap_err(); + } } diff --git a/src/chacha.rs b/src/chacha.rs index 277eb39e70..b04ba9f0cc 100644 --- a/src/chacha.rs +++ b/src/chacha.rs @@ -166,7 +166,7 @@ mod tests { use bs58; // golden needs to be updated if blob stuff changes.... let golden = Hash::new( - &bs58::decode("BES6jpfVwayNKq9YZbYjbZbyX3GLzFzeQJ7fksm6LifE") + &bs58::decode("nzxMWDQVsftBZbMGA1ika8X6bAKy7vya1jfXnVZSErt") .into_vec() .unwrap(), ); diff --git a/src/rpc.rs b/src/rpc.rs index af9e73e703..b70ae2a6e7 100644 --- a/src/rpc.rs +++ b/src/rpc.rs @@ -120,6 +120,7 @@ impl Metadata for Meta {} #[derive(Copy, Clone, PartialEq, Serialize, Debug)] pub enum RpcSignatureStatus { AccountInUse, + AccountLoadedTwice, Confirmed, GenericFailure, ProgramRuntimeError, @@ -131,6 +132,7 @@ impl FromStr for RpcSignatureStatus { fn from_str(s: &str) -> Result { match s { "AccountInUse" => Ok(RpcSignatureStatus::AccountInUse), + "AccountLoadedTwice" => Ok(RpcSignatureStatus::AccountLoadedTwice), "Confirmed" => Ok(RpcSignatureStatus::Confirmed), "GenericFailure" => Ok(RpcSignatureStatus::GenericFailure), "ProgramRuntimeError" => Ok(RpcSignatureStatus::ProgramRuntimeError), @@ -235,6 +237,7 @@ impl RpcSol for RpcSolImpl { match res.unwrap() { Ok(_) => RpcSignatureStatus::Confirmed, Err(BankError::AccountInUse) => RpcSignatureStatus::AccountInUse, + Err(BankError::AccountLoadedTwice) => RpcSignatureStatus::AccountLoadedTwice, Err(BankError::ProgramError(_, _)) => RpcSignatureStatus::ProgramRuntimeError, Err(err) => { trace!("mapping {:?} to GenericFailure", err);