Put empty accounts in the accounts list on load (#9840)

Indexing into accounts array does not match account_keys otherwise.
Also enforce program accounts not at index 0
Enforce at least 1 Read-write signing fee-payer account.

runtime/src/accounts.rs

@@ -151,50 +151,68 @@ impl Accounts {
 
             // 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 accounts: TransactionAccounts = Vec::with_capacity(message.account_keys.len());
+            let mut payer_index = None;
             let mut tx_rent: TransactionRent = 0;
-            for (i, key) in message
+            let mut accounts: Vec<_> = message
                 .account_keys
                 .iter()
                 .enumerate()
-                .filter(|(i, key)| Self::is_non_loader_key(message, key, *i))
-            {
-                let (account, rent) = AccountsDB::load(storage, ancestors, accounts_index, key)
-                    .and_then(|(mut account, _)| {
-                        if message.is_writable(i) && !account.executable {
-                            let rent_due = rent_collector.update(&key, &mut account);
-                            Some((account, rent_due))
-                        } else {
-                            Some((account, 0))
+                .map(|(i, key)| {
+                    if Self::is_non_loader_key(message, key, i) {
+                        if payer_index.is_none() {
+                            payer_index = Some(i);
                         }
-                    })
-                    .unwrap_or_default();
+                        let (account, rent) =
+                            AccountsDB::load(storage, ancestors, accounts_index, key)
+                                .and_then(|(mut account, _)| {
+                                    if message.is_writable(i) && !account.executable {
+                                        let rent_due = rent_collector.update(&key, &mut account);
+                                        Some((account, rent_due))
+                                    } else {
+                                        Some((account, 0))
+                                    }
+                                })
+                                .unwrap_or_default();
 
-                accounts.push(account);
-                tx_rent += rent;
-            }
+                        tx_rent += rent;
+                        account
+                    } else {
+                        // Fill in an empty account for the program slots.
+                        Account::default()
+                    }
+                })
+                .collect();
 
-            if accounts.is_empty() || accounts[0].lamports == 0 {
+            if let Some(payer_index) = payer_index {
+                if payer_index != 0 {
+                    warn!("Payer index should be 0! {:?}", tx);
+                }
+                if accounts[payer_index].lamports == 0 {
+                    error_counters.account_not_found += 1;
+                    Err(TransactionError::AccountNotFound)
+                } else {
+                    let min_balance = match get_system_account_kind(&accounts[payer_index])
+                        .ok_or_else(|| {
+                            error_counters.invalid_account_for_fee += 1;
+                            TransactionError::InvalidAccountForFee
+                        })? {
+                        SystemAccountKind::System => 0,
+                        SystemAccountKind::Nonce => {
+                            rent_collector.rent.minimum_balance(nonce::State::size())
+                        }
+                    };
+
+                    if accounts[payer_index].lamports < fee + min_balance {
+                        error_counters.insufficient_funds += 1;
+                        Err(TransactionError::InsufficientFundsForFee)
+                    } else {
+                        accounts[payer_index].lamports -= fee;
+                        Ok((accounts, tx_rent))
+                    }
+                }
+            } else {
                 error_counters.account_not_found += 1;
                 Err(TransactionError::AccountNotFound)
-            } else {
-                let min_balance = match get_system_account_kind(&accounts[0]).ok_or_else(|| {
-                    error_counters.invalid_account_for_fee += 1;
-                    TransactionError::InvalidAccountForFee
-                })? {
-                    SystemAccountKind::System => 0,
-                    SystemAccountKind::Nonce => {
-                        rent_collector.rent.minimum_balance(nonce::State::size())
-                    }
-                };
-
-                if accounts[0].lamports < fee + min_balance {
-                    error_counters.insufficient_funds += 1;
-                    Err(TransactionError::InsufficientFundsForFee)
-                } else {
-                    accounts[0].lamports -= fee;
-                    Ok((accounts, tx_rent))
-                }
             }
         }
     }
@@ -651,8 +669,8 @@ impl Accounts {
                 .account_keys
                 .iter()
                 .enumerate()
-                .filter(|(i, key)| Self::is_non_loader_key(message, key, *i))
                 .zip(acc.0.iter_mut())
+                .filter(|((i, key), _account)| Self::is_non_loader_key(message, key, *i))
             {
                 nonce_utils::prepare_if_nonce_account(
                     account,
@@ -1050,7 +1068,7 @@ mod tests {
                 Ok((transaction_accounts, transaction_loaders, _transaction_rents)),
                 _hash_age_kind,
             ) => {
-                assert_eq!(transaction_accounts.len(), 2);
+                assert_eq!(transaction_accounts.len(), 3);
                 assert_eq!(transaction_accounts[0], accounts[0].1);
                 assert_eq!(transaction_loaders.len(), 1);
                 assert_eq!(transaction_loaders[0].len(), 0);
@@ -1288,7 +1306,7 @@ mod tests {
                 Ok((transaction_accounts, transaction_loaders, _transaction_rents)),
                 _hash_age_kind,
             ) => {
-                assert_eq!(transaction_accounts.len(), 1);
+                assert_eq!(transaction_accounts.len(), 3);
                 assert_eq!(transaction_accounts[0], accounts[0].1);
                 assert_eq!(transaction_loaders.len(), 2);
                 assert_eq!(transaction_loaders[0].len(), 1);

runtime/src/bank.rs

@@ -4603,7 +4603,10 @@ mod tests {
             bank.last_blockhash(),
         );
 
-        assert_eq!(bank.process_transaction(&tx), Ok(()));
+        assert_eq!(
+            bank.process_transaction(&tx),
+            Err(TransactionError::InvalidAccountIndex)
+        );
         assert_eq!(bank.get_balance(&key.pubkey()), 0);
     }
 
@@ -5840,15 +5843,11 @@ mod tests {
 
         tx.message.account_keys.push(Pubkey::new_rand());
 
-        fn mock_vote_processor(
-            _pubkey: &Pubkey,
-            _ka: &[KeyedAccount],
-            _data: &[u8],
-        ) -> std::result::Result<(), InstructionError> {
-            Ok(())
-        }
-
-        bank.add_static_program("mock_vote", solana_vote_program::id(), mock_vote_processor);
+        bank.add_static_program(
+            "mock_vote",
+            solana_vote_program::id(),
+            mock_ok_vote_processor,
+        );
         let result = bank.process_transaction(&tx);
         assert_eq!(result, Ok(()));
         let account = bank.get_account(&solana_vote_program::id()).unwrap();
@@ -5883,4 +5882,96 @@ mod tests {
         // capitalization
         assert_eq!(bank.capitalization(), pre_capitalization - burn_amount);
     }
+
+    #[test]
+    fn test_program_id_as_payer() {
+        solana_logger::setup();
+        let (genesis_config, mint_keypair) = create_genesis_config(500);
+        let mut bank = Bank::new(&genesis_config);
+
+        let from_pubkey = Pubkey::new_rand();
+        let to_pubkey = Pubkey::new_rand();
+
+        let account_metas = vec![
+            AccountMeta::new(from_pubkey, false),
+            AccountMeta::new(to_pubkey, false),
+        ];
+
+        bank.add_static_program(
+            "mock_vote",
+            solana_vote_program::id(),
+            mock_ok_vote_processor,
+        );
+
+        let instruction = Instruction::new(solana_vote_program::id(), &10, account_metas);
+        let mut tx = Transaction::new_signed_with_payer(
+            &[instruction],
+            Some(&mint_keypair.pubkey()),
+            &[&mint_keypair],
+            bank.last_blockhash(),
+        );
+
+        info!(
+            "mint: {} account keys: {:?}",
+            mint_keypair.pubkey(),
+            tx.message.account_keys
+        );
+        assert_eq!(tx.message.account_keys.len(), 4);
+        tx.message.account_keys.clear();
+        tx.message.account_keys.push(solana_vote_program::id());
+        tx.message.account_keys.push(mint_keypair.pubkey());
+        tx.message.account_keys.push(from_pubkey);
+        tx.message.account_keys.push(to_pubkey);
+        tx.message.instructions[0].program_id_index = 0;
+        tx.message.instructions[0].accounts.clear();
+        tx.message.instructions[0].accounts.push(2);
+        tx.message.instructions[0].accounts.push(3);
+
+        let result = bank.process_transaction(&tx);
+        assert_eq!(result, Err(TransactionError::InvalidAccountIndex));
+    }
+
+    fn mock_ok_vote_processor(
+        _pubkey: &Pubkey,
+        _ka: &[KeyedAccount],
+        _data: &[u8],
+    ) -> std::result::Result<(), InstructionError> {
+        Ok(())
+    }
+
+    #[test]
+    fn test_ref_account_key_after_program_id() {
+        let (genesis_config, mint_keypair) = create_genesis_config(500);
+        let mut bank = Bank::new(&genesis_config);
+
+        let from_pubkey = Pubkey::new_rand();
+        let to_pubkey = Pubkey::new_rand();
+
+        let account_metas = vec![
+            AccountMeta::new(from_pubkey, false),
+            AccountMeta::new(to_pubkey, false),
+        ];
+
+        bank.add_static_program(
+            "mock_vote",
+            solana_vote_program::id(),
+            mock_ok_vote_processor,
+        );
+
+        let instruction = Instruction::new(solana_vote_program::id(), &10, account_metas);
+        let mut tx = Transaction::new_signed_with_payer(
+            &[instruction],
+            Some(&mint_keypair.pubkey()),
+            &[&mint_keypair],
+            bank.last_blockhash(),
+        );
+
+        tx.message.account_keys.push(Pubkey::new_rand());
+        assert_eq!(tx.message.account_keys.len(), 5);
+        tx.message.instructions[0].accounts.remove(0);
+        tx.message.instructions[0].accounts.push(4);
+
+        let result = bank.process_transaction(&tx);
+        assert_eq!(result, Ok(()));
+    }
 }

runtime/tests/stake.rs

@@ -137,14 +137,17 @@ fn test_stake_create_and_split_single_signature() {
         Pubkey::create_with_seed(&staker_pubkey, "split_stake", &solana_stake_program::id())
             .unwrap();
     // Test split
-    let message = Message::new(&stake_instruction::split_with_seed(
-        &stake_address, // original
-        &staker_pubkey, // authorized
-        lamports / 2,
-        &split_stake_address, // new address
-        &staker_pubkey,       // base
-        "split_stake",        // seed
-    ));
+    let message = Message::new_with_payer(
+        &stake_instruction::split_with_seed(
+            &stake_address, // original
+            &staker_pubkey, // authorized
+            lamports / 2,
+            &split_stake_address, // new address
+            &staker_pubkey,       // base
+            "split_stake",        // seed
+        ),
+        Some(&staker_keypair.pubkey()),
+    );
 
     assert!(bank_client
         .send_message(&[&staker_keypair], message)

sdk/src/message.rs

@@ -165,19 +165,27 @@ pub struct Message {
 
 impl Sanitize for Message {
     fn sanitize(&self) -> std::result::Result<(), SanitizeError> {
-        if self.header.num_required_signatures as usize > self.account_keys.len() {
-            return Err(SanitizeError::IndexOutOfBounds);
-        }
-        if self.header.num_readonly_unsigned_accounts as usize
-            + self.header.num_readonly_signed_accounts as usize
+        // signing area and read-only non-signing area should not overlap
+        if self.header.num_required_signatures as usize
+            + self.header.num_readonly_unsigned_accounts as usize
             > self.account_keys.len()
         {
             return Err(SanitizeError::IndexOutOfBounds);
         }
+
+        // there should be at least 1 RW fee-payer account.
+        if self.header.num_readonly_signed_accounts >= self.header.num_required_signatures {
+            return Err(SanitizeError::IndexOutOfBounds);
+        }
+
         for ci in &self.instructions {
             if ci.program_id_index as usize >= self.account_keys.len() {
                 return Err(SanitizeError::IndexOutOfBounds);
             }
+            // A program cannot be a payer.
+            if ci.program_id_index == 0 {
+                return Err(SanitizeError::IndexOutOfBounds);
+            }
             for ai in &ci.accounts {
                 if *ai as usize >= self.account_keys.len() {
                     return Err(SanitizeError::IndexOutOfBounds);

sdk/src/transaction.rs

@@ -510,6 +510,21 @@ mod tests {
         tx = o.clone();
         tx.message.instructions[0].accounts[0] = 3;
         assert_eq!(tx.sanitize(), Err(SanitizeError::IndexOutOfBounds));
+
+        tx = o.clone();
+        tx.message.instructions[0].program_id_index = 0;
+        assert_eq!(tx.sanitize(), Err(SanitizeError::IndexOutOfBounds));
+
+        tx = o.clone();
+        tx.message.header.num_readonly_signed_accounts = 2;
+        tx.message.header.num_readonly_unsigned_accounts = 3;
+        tx.message.account_keys.resize(4, Pubkey::default());
+        assert_eq!(tx.sanitize(), Err(SanitizeError::IndexOutOfBounds));
+
+        tx = o.clone();
+        tx.message.header.num_readonly_signed_accounts = 2;
+        tx.message.header.num_required_signatures = 1;
+        assert_eq!(tx.sanitize(), Err(SanitizeError::IndexOutOfBounds));
     }
 
     fn create_sample_transaction() -> Transaction {