Limit number of accounts that a transaction can lock

accountsdb-plugin-postgres/src/postgres_client/postgres_client_transaction.rs

@@ -331,6 +331,7 @@ pub enum DbTransactionErrorCode {
     UnsupportedVersion,
     InvalidWritableAccount,
     WouldExceedMaxAccountDataCostLimit,
+    TooManyAccountLocks,
 }
 
 impl From<&TransactionError> for DbTransactionErrorCode {
@@ -362,6 +363,7 @@ impl From<&TransactionError> for DbTransactionErrorCode {
             TransactionError::WouldExceedMaxAccountDataCostLimit => {
                 Self::WouldExceedMaxAccountDataCostLimit
             }
+            TransactionError::TooManyAccountLocks => Self::TooManyAccountLocks,
         }
     }
 }

rpc/src/transaction_status_service.rs

@@ -123,7 +123,7 @@ impl TransactionStatusService {
                             transaction.message(),
                             lamports_per_signature,
                         );
-                        let tx_account_locks = transaction.get_account_locks();
+                        let tx_account_locks = transaction.get_account_locks_unchecked();
 
                         let inner_instructions = inner_instructions.map(|inner_instructions| {
                             inner_instructions

runtime/src/accounts.rs

@@ -36,7 +36,7 @@ use {
         pubkey::Pubkey,
         system_program,
         sysvar::{self, instructions::construct_instructions_data},
-        transaction::{Result, SanitizedTransaction, TransactionError},
+        transaction::{Result, SanitizedTransaction, TransactionAccountLocks, TransactionError},
         transaction_context::TransactionAccount,
     },
     std::{
@@ -978,12 +978,11 @@ impl Accounts {
     pub fn lock_accounts<'a>(
         &self,
         txs: impl Iterator<Item = &'a SanitizedTransaction>,
+        feature_set: &FeatureSet,
     ) -> Vec<Result<()>> {
-        let keys: Vec<_> = txs.map(|tx| tx.get_account_locks()).collect();
-        let account_locks = &mut self.account_locks.lock().unwrap();
-        keys.into_iter()
-            .map(|keys| self.lock_account(account_locks, keys.writable, keys.readonly))
-            .collect()
+        let tx_account_locks_results: Vec<Result<_>> =
+            txs.map(|tx| tx.get_account_locks(feature_set)).collect();
+        self.lock_accounts_inner(tx_account_locks_results)
     }
 
     #[must_use]
@@ -992,20 +991,33 @@ impl Accounts {
         &self,
         txs: impl Iterator<Item = &'a SanitizedTransaction>,
         results: impl Iterator<Item = Result<()>>,
+        feature_set: &FeatureSet,
     ) -> Vec<Result<()>> {
-        let key_results: Vec<_> = txs
+        let tx_account_locks_results: Vec<Result<_>> = txs
             .zip(results)
             .map(|(tx, result)| match result {
-                Ok(()) => Ok(tx.get_account_locks()),
-                Err(e) => Err(e),
+                Ok(()) => tx.get_account_locks(feature_set),
+                Err(err) => Err(err),
             })
             .collect();
+        self.lock_accounts_inner(tx_account_locks_results)
+    }
+
+    #[must_use]
+    fn lock_accounts_inner(
+        &self,
+        tx_account_locks_results: Vec<Result<TransactionAccountLocks>>,
+    ) -> Vec<Result<()>> {
         let account_locks = &mut self.account_locks.lock().unwrap();
-        key_results
+        tx_account_locks_results
             .into_iter()
-            .map(|key_result| match key_result {
-                Ok(keys) => self.lock_account(account_locks, keys.writable, keys.readonly),
-                Err(e) => Err(e),
+            .map(|tx_account_locks_result| match tx_account_locks_result {
+                Ok(tx_account_locks) => self.lock_account(
+                    account_locks,
+                    tx_account_locks.writable,
+                    tx_account_locks.readonly,
+                ),
+                Err(err) => Err(err),
             })
             .collect()
     }
@@ -1020,13 +1032,14 @@ impl Accounts {
         let keys: Vec<_> = txs
             .zip(results)
             .filter_map(|(tx, res)| match res {
-                Err(TransactionError::AccountInUse)
+                Err(TransactionError::AccountLoadedTwice)
+                | Err(TransactionError::AccountInUse)
                 | Err(TransactionError::SanitizeFailure)
-                | Err(TransactionError::AccountLoadedTwice)
+                | Err(TransactionError::TooManyAccountLocks)
                 | Err(TransactionError::WouldExceedMaxBlockCostLimit)
                 | Err(TransactionError::WouldExceedMaxAccountCostLimit)
                 | Err(TransactionError::WouldExceedMaxAccountDataCostLimit) => None,
-                _ => Some(tx.get_account_locks()),
+                _ => Some(tx.get_account_locks_unchecked()),
             })
             .collect();
         let mut account_locks = self.account_locks.lock().unwrap();
@@ -2170,7 +2183,7 @@ mod tests {
             instructions,
         );
         let tx = new_sanitized_tx(&[&keypair0], message, Hash::default());
-        let results0 = accounts.lock_accounts([tx.clone()].iter());
+        let results0 = accounts.lock_accounts([tx.clone()].iter(), &FeatureSet::all_enabled());
 
         assert!(results0[0].is_ok());
         assert_eq!(
@@ -2205,7 +2218,7 @@ mod tests {
         );
         let tx1 = new_sanitized_tx(&[&keypair1], message, Hash::default());
         let txs = vec![tx0, tx1];
-        let results1 = accounts.lock_accounts(txs.iter());
+        let results1 = accounts.lock_accounts(txs.iter(), &FeatureSet::all_enabled());
 
         assert!(results1[0].is_ok()); // Read-only account (keypair1) can be referenced multiple times
         assert!(results1[1].is_err()); // Read-only account (keypair1) cannot also be locked as writable
@@ -2232,7 +2245,7 @@ mod tests {
             instructions,
         );
         let tx = new_sanitized_tx(&[&keypair1], message, Hash::default());
-        let results2 = accounts.lock_accounts([tx].iter());
+        let results2 = accounts.lock_accounts([tx].iter(), &FeatureSet::all_enabled());
         assert!(results2[0].is_ok()); // Now keypair1 account can be locked as writable
 
         // Check that read-only lock with zero references is deleted
@@ -2301,7 +2314,9 @@ mod tests {
             let exit_clone = exit_clone.clone();
             loop {
                 let txs = vec![writable_tx.clone()];
-                let results = accounts_clone.clone().lock_accounts(txs.iter());
+                let results = accounts_clone
+                    .clone()
+                    .lock_accounts(txs.iter(), &FeatureSet::all_enabled());
                 for result in results.iter() {
                     if result.is_ok() {
                         counter_clone.clone().fetch_add(1, Ordering::SeqCst);
@@ -2316,7 +2331,9 @@ mod tests {
         let counter_clone = counter;
         for _ in 0..5 {
             let txs = vec![readonly_tx.clone()];
-            let results = accounts_arc.clone().lock_accounts(txs.iter());
+            let results = accounts_arc
+                .clone()
+                .lock_accounts(txs.iter(), &FeatureSet::all_enabled());
             if results[0].is_ok() {
                 let counter_value = counter_clone.clone().load(Ordering::SeqCst);
                 thread::sleep(time::Duration::from_millis(50));
@@ -2362,7 +2379,7 @@ mod tests {
             instructions,
         );
         let tx = new_sanitized_tx(&[&keypair0], message, Hash::default());
-        let results0 = accounts.lock_accounts([tx].iter());
+        let results0 = accounts.lock_accounts([tx].iter(), &FeatureSet::all_enabled());
 
         assert!(results0[0].is_ok());
         // Instruction program-id account demoted to readonly
@@ -2453,7 +2470,11 @@ mod tests {
             Ok(()),
         ];
 
-        let results = accounts.lock_accounts_with_results(txs.iter(), qos_results.into_iter());
+        let results = accounts.lock_accounts_with_results(
+            txs.iter(),
+            qos_results.into_iter(),
+            &FeatureSet::all_enabled(),
+        );
 
         assert!(results[0].is_ok()); // Read-only account (keypair0) can be referenced multiple times
         assert!(results[1].is_err()); // is not locked due to !qos_results[1].is_ok()

runtime/src/bank.rs

@@ -3069,7 +3069,10 @@ impl Bank {
             .into_iter()
             .map(SanitizedTransaction::from_transaction_for_tests)
             .collect::<Vec<_>>();
-        let lock_results = self.rc.accounts.lock_accounts(sanitized_txs.iter());
+        let lock_results = self
+            .rc
+            .accounts
+            .lock_accounts(sanitized_txs.iter(), &FeatureSet::all_enabled());
         TransactionBatch::new(lock_results, self, Cow::Owned(sanitized_txs))
     }
 
@@ -3085,7 +3088,10 @@ impl Bank {
                 })
             })
             .collect::<Result<Vec<_>>>()?;
-        let lock_results = self.rc.accounts.lock_accounts(sanitized_txs.iter());
+        let lock_results = self
+            .rc
+            .accounts
+            .lock_accounts(sanitized_txs.iter(), &FeatureSet::all_enabled());
         Ok(TransactionBatch::new(
             lock_results,
             self,
@@ -3098,7 +3104,10 @@ impl Bank {
         &'a self,
         txs: &'b [SanitizedTransaction],
     ) -> TransactionBatch<'a, 'b> {
-        let lock_results = self.rc.accounts.lock_accounts(txs.iter());
+        let lock_results = self
+            .rc
+            .accounts
+            .lock_accounts(txs.iter(), &self.feature_set);
         TransactionBatch::new(lock_results, self, Cow::Borrowed(txs))
     }
 
@@ -3110,10 +3119,11 @@ impl Bank {
         transaction_results: impl Iterator<Item = Result<()>>,
     ) -> TransactionBatch<'a, 'b> {
         // this lock_results could be: Ok, AccountInUse, WouldExceedBlockMaxLimit or WouldExceedAccountMaxLimit
-        let lock_results = self
-            .rc
-            .accounts
-            .lock_accounts_with_results(transactions.iter(), transaction_results);
+        let lock_results = self.rc.accounts.lock_accounts_with_results(
+            transactions.iter(),
+            transaction_results,
+            &self.feature_set,
+        );
         TransactionBatch::new(lock_results, self, Cow::Borrowed(transactions))
     }
 
@@ -3122,7 +3132,9 @@ impl Bank {
         &'a self,
         transaction: SanitizedTransaction,
     ) -> TransactionBatch<'a, '_> {
-        let mut batch = TransactionBatch::new(vec![Ok(())], self, Cow::Owned(vec![transaction]));
+        let lock_result = transaction.get_account_locks(&self.feature_set).map(|_| ());
+        let mut batch =
+            TransactionBatch::new(vec![lock_result], self, Cow::Owned(vec![transaction]));
         batch.needs_unlock = false;
         batch
     }
@@ -6218,6 +6230,7 @@ pub(crate) mod tests {
             system_program,
             sysvar::rewards::Rewards,
             timing::duration_as_s,
+            transaction::MAX_TX_ACCOUNT_LOCKS,
         },
         solana_vote_program::{
             vote_instruction,
@@ -11583,6 +11596,43 @@ pub(crate) mod tests {
         assert_eq!(result, Err(TransactionError::AccountLoadedTwice));
     }
 
+    #[test]
+    fn test_process_transaction_with_too_many_account_locks() {
+        solana_logger::setup();
+        let (genesis_config, mint_keypair) = create_genesis_config(500);
+        let mut bank = Bank::new_for_tests(&genesis_config);
+
+        let from_pubkey = solana_sdk::pubkey::new_rand();
+        let to_pubkey = solana_sdk::pubkey::new_rand();
+
+        let account_metas = vec![
+            AccountMeta::new(from_pubkey, false),
+            AccountMeta::new(to_pubkey, false),
+        ];
+
+        bank.add_builtin(
+            "mock_vote",
+            &solana_vote_program::id(),
+            mock_ok_vote_processor,
+        );
+
+        let instruction =
+            Instruction::new_with_bincode(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(),
+        );
+
+        while tx.message.account_keys.len() <= MAX_TX_ACCOUNT_LOCKS {
+            tx.message.account_keys.push(solana_sdk::pubkey::new_rand());
+        }
+
+        let result = bank.process_transaction(&tx);
+        assert_eq!(result, Err(TransactionError::TooManyAccountLocks));
+    }
+
     #[test]
     fn test_program_id_as_payer() {
         solana_logger::setup();

sdk/program/src/message/sanitized.rs

@@ -30,8 +30,6 @@ pub enum SanitizeMessageError {
     ValueOutOfBounds,
     #[error("invalid value")]
     InvalidValue,
-    #[error("duplicate account key")]
-    DuplicateAccountKey,
 }
 
 impl From<SanitizeError> for SanitizeMessageError {
@@ -48,13 +46,7 @@ impl TryFrom<LegacyMessage> for SanitizedMessage {
     type Error = SanitizeMessageError;
     fn try_from(message: LegacyMessage) -> Result<Self, Self::Error> {
         message.sanitize()?;
-
-        let sanitized_msg = Self::Legacy(message);
-        if sanitized_msg.has_duplicates() {
-            return Err(SanitizeMessageError::DuplicateAccountKey);
-        }
-
-        Ok(sanitized_msg)
+        Ok(Self::Legacy(message))
     }
 }
 
@@ -310,21 +302,6 @@ mod tests {
 
     #[test]
     fn test_try_from_message() {
-        let dupe_key = Pubkey::new_unique();
-        let legacy_message_with_dupes = LegacyMessage {
-            header: MessageHeader {
-                num_required_signatures: 1,
-                ..MessageHeader::default()
-            },
-            account_keys: vec![dupe_key, dupe_key],
-            ..LegacyMessage::default()
-        };
-
-        assert_eq!(
-            SanitizedMessage::try_from(legacy_message_with_dupes).err(),
-            Some(SanitizeMessageError::DuplicateAccountKey),
-        );
-
         let legacy_message_with_no_signers = LegacyMessage {
             account_keys: vec![Pubkey::new_unique()],
             ..LegacyMessage::default()

sdk/src/feature_set.rs

@@ -287,6 +287,10 @@ pub mod cap_accounts_data_len {
     solana_sdk::declare_id!("capRxUrBjNkkCpjrJxPGfPaWijB7q3JoDfsWXAnt46r");
 }
 
+pub mod max_tx_account_locks {
+    solana_sdk::declare_id!("CBkDroRDqm8HwHe6ak9cguPjUomrASEkfmxEaZ5CNNxz");
+}
+
 lazy_static! {
     /// Map of feature identifiers to user-visible description
     pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
@@ -353,6 +357,7 @@ lazy_static! {
         (allow_votes_to_directly_update_vote_state::id(), "enable direct vote state update"),
         (reject_all_elf_rw::id(), "reject all read-write data in program elfs"),
         (cap_accounts_data_len::id(), "cap the accounts data len"),
+        (max_tx_account_locks::id(), "enforce max number of locked accounts per transaction"),
         /*************** ADD NEW FEATURES HERE ***************/
     ]
     .iter()

sdk/src/transaction/error.rs

@@ -105,6 +105,10 @@ pub enum TransactionError {
     /// Transaction would exceed max account data limit within the block
     #[error("Transaction would exceed max account data limit within the block")]
     WouldExceedMaxAccountDataCostLimit,
+
+    /// Transaction locked too many accounts
+    #[error("Transaction locked too many accounts")]
+    TooManyAccountLocks,
 }
 
 impl From<SanitizeError> for TransactionError {
@@ -114,12 +118,7 @@ impl From<SanitizeError> for TransactionError {
 }
 
 impl From<SanitizeMessageError> for TransactionError {
-    fn from(err: SanitizeMessageError) -> Self {
-        match err {
-            SanitizeMessageError::IndexOutOfBounds
-            | SanitizeMessageError::ValueOutOfBounds
-            | SanitizeMessageError::InvalidValue => Self::SanitizeFailure,
-            SanitizeMessageError::DuplicateAccountKey => Self::AccountLoadedTwice,
-        }
+    fn from(_err: SanitizeMessageError) -> Self {
+        Self::SanitizeFailure
     }
 }