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

@@ -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]

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,

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<Vec<(Pubkey, Account)>>;
 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<Account> = 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));
+    }
 }

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();
+    }
 }

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(),
         );

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<RpcSignatureStatus> {
         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);