Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Increase transaction account lock limit from 64 to 128 (backport #27242) #27810

Merged
merged 2 commits into from
Sep 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion programs/bpf/tests/programs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1264,7 +1264,7 @@ fn test_program_bpf_invoke_sanity() {
format!("Program log: invoke {program_lang} program"),
"Program log: Test max account infos exceeded".into(),
"skip".into(), // don't compare compute consumption logs
"Program failed to complete: Invoked an instruction with too many account info's (65 > 64)".into(),
"Program failed to complete: Invoked an instruction with too many account info's (129 > 128)".into(),
format!("Program {invoke_program_id} failed: Program failed to complete"),
]),
);
Expand Down
10 changes: 9 additions & 1 deletion programs/bpf_loader/src/syscalls/cpi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -792,8 +792,16 @@ fn check_account_infos(
.feature_set
.is_active(&feature_set::loosen_cpi_size_restriction::id())
{
let max_cpi_account_infos = if invoke_context
.feature_set
.is_active(&feature_set::increase_tx_account_lock_limit::id())
{
MAX_CPI_ACCOUNT_INFOS
} else {
64
};
let num_account_infos = num_account_infos as u64;
let max_account_infos = MAX_CPI_ACCOUNT_INFOS as u64;
let max_account_infos = max_cpi_account_infos as u64;
if num_account_infos > max_account_infos {
return Err(SyscallError::MaxInstructionAccountInfosExceeded {
num_account_infos,
Expand Down
37 changes: 24 additions & 13 deletions runtime/src/accounts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1139,9 +1139,11 @@ impl Accounts {
pub fn lock_accounts<'a>(
&self,
txs: impl Iterator<Item = &'a SanitizedTransaction>,
tx_account_lock_limit: usize,
) -> Vec<Result<()>> {
let tx_account_locks_results: Vec<Result<_>> =
txs.map(|tx| tx.get_account_locks()).collect();
let tx_account_locks_results: Vec<Result<_>> = txs
.map(|tx| tx.get_account_locks(tx_account_lock_limit))
.collect();
self.lock_accounts_inner(tx_account_locks_results)
}

Expand All @@ -1151,11 +1153,12 @@ impl Accounts {
&self,
txs: impl Iterator<Item = &'a SanitizedTransaction>,
results: impl Iterator<Item = &'a Result<()>>,
tx_account_lock_limit: usize,
) -> Vec<Result<()>> {
let tx_account_locks_results: Vec<Result<_>> = txs
.zip(results)
.map(|(tx, result)| match result {
Ok(()) => tx.get_account_locks(),
Ok(()) => tx.get_account_locks(tx_account_lock_limit),
Err(err) => Err(err.clone()),
})
.collect();
Expand Down Expand Up @@ -2532,7 +2535,7 @@ mod tests {
};

let tx = new_sanitized_tx(&[&keypair], message, Hash::default());
let results = accounts.lock_accounts([tx].iter());
let results = accounts.lock_accounts([tx].iter(), MAX_TX_ACCOUNT_LOCKS);
assert_eq!(results[0], Err(TransactionError::AccountLoadedTwice));
}

Expand Down Expand Up @@ -2565,7 +2568,7 @@ mod tests {
};

let txs = vec![new_sanitized_tx(&[&keypair], message, Hash::default())];
let results = accounts.lock_accounts(txs.iter());
let results = accounts.lock_accounts(txs.iter(), MAX_TX_ACCOUNT_LOCKS);
assert_eq!(results[0], Ok(()));
accounts.unlock_accounts(txs.iter(), &results);
}
Expand All @@ -2587,7 +2590,7 @@ mod tests {
};

let txs = vec![new_sanitized_tx(&[&keypair], message, Hash::default())];
let results = accounts.lock_accounts(txs.iter());
let results = accounts.lock_accounts(txs.iter(), MAX_TX_ACCOUNT_LOCKS);
assert_eq!(results[0], Err(TransactionError::TooManyAccountLocks));
}
}
Expand Down Expand Up @@ -2626,7 +2629,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(), MAX_TX_ACCOUNT_LOCKS);

assert!(results0[0].is_ok());
assert_eq!(
Expand Down Expand Up @@ -2661,7 +2664,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(), MAX_TX_ACCOUNT_LOCKS);

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
Expand All @@ -2688,7 +2691,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(), MAX_TX_ACCOUNT_LOCKS);
assert!(results2[0].is_ok()); // Now keypair1 account can be locked as writable

// Check that read-only lock with zero references is deleted
Expand Down Expand Up @@ -2757,7 +2760,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(), MAX_TX_ACCOUNT_LOCKS);
for result in results.iter() {
if result.is_ok() {
counter_clone.clone().fetch_add(1, Ordering::SeqCst);
Expand All @@ -2772,7 +2777,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(), MAX_TX_ACCOUNT_LOCKS);
if results[0].is_ok() {
let counter_value = counter_clone.clone().load(Ordering::SeqCst);
thread::sleep(time::Duration::from_millis(50));
Expand Down Expand Up @@ -2818,7 +2825,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(), MAX_TX_ACCOUNT_LOCKS);

assert!(results0[0].is_ok());
// Instruction program-id account demoted to readonly
Expand Down Expand Up @@ -2909,7 +2916,11 @@ mod tests {
Ok(()),
];

let results = accounts.lock_accounts_with_results(txs.iter(), qos_results.iter());
let results = accounts.lock_accounts_with_results(
txs.iter(),
qos_results.iter(),
MAX_TX_ACCOUNT_LOCKS,
);

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()
Expand Down
46 changes: 35 additions & 11 deletions runtime/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ use {
timing::years_as_slots,
transaction::{
MessageHash, Result, SanitizedTransaction, Transaction, TransactionError,
TransactionVerificationMode, VersionedTransaction,
TransactionVerificationMode, VersionedTransaction, MAX_TX_ACCOUNT_LOCKS,
},
transaction_context::{
ExecutionRecord, InstructionTrace, TransactionAccount, TransactionContext,
Expand Down Expand Up @@ -3880,13 +3880,28 @@ impl Bank {
}
}

/// Get the max number of accounts that a transaction may lock in this block
pub fn get_transaction_account_lock_limit(&self) -> usize {
if self
.feature_set
.is_active(&feature_set::increase_tx_account_lock_limit::id())
{
MAX_TX_ACCOUNT_LOCKS
} else {
64
}
}

/// Prepare a transaction batch from a list of legacy transactions. Used for tests only.
pub fn prepare_batch_for_tests(&self, txs: Vec<Transaction>) -> TransactionBatch {
let sanitized_txs = txs
.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(),
self.get_transaction_account_lock_limit(),
);
TransactionBatch::new(lock_results, self, Cow::Owned(sanitized_txs))
}

Expand All @@ -3906,7 +3921,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(),
self.get_transaction_account_lock_limit(),
);
Ok(TransactionBatch::new(
lock_results,
self,
Expand All @@ -3919,7 +3937,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.get_transaction_account_lock_limit());
TransactionBatch::new(lock_results, self, Cow::Borrowed(txs))
}

Expand All @@ -3931,10 +3952,11 @@ impl Bank {
transaction_results: impl Iterator<Item = &'b 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.get_transaction_account_lock_limit(),
);
TransactionBatch::new(lock_results, self, Cow::Borrowed(transactions))
}

Expand All @@ -3943,7 +3965,9 @@ impl Bank {
&'a self,
transaction: SanitizedTransaction,
) -> TransactionBatch<'a, '_> {
let lock_result = transaction.get_account_locks().map(|_| ());
let lock_result = transaction
.get_account_locks(self.get_transaction_account_lock_limit())
.map(|_| ());
let mut batch =
TransactionBatch::new(vec![lock_result], self, Cow::Owned(vec![transaction]));
batch.set_needs_unlock(false);
Expand Down Expand Up @@ -8065,7 +8089,6 @@ pub(crate) mod tests {
system_instruction::{self, SystemError, MAX_PERMITTED_DATA_LENGTH},
system_program,
timing::duration_as_s,
transaction::MAX_TX_ACCOUNT_LOCKS,
transaction_context::InstructionContext,
},
solana_vote_program::{
Expand Down Expand Up @@ -14287,7 +14310,8 @@ pub(crate) mod tests {
bank.last_blockhash(),
);

while tx.message.account_keys.len() <= MAX_TX_ACCOUNT_LOCKS {
let transaction_account_lock_limit = bank.get_transaction_account_lock_limit();
while tx.message.account_keys.len() <= transaction_account_lock_limit {
tx.message.account_keys.push(solana_sdk::pubkey::new_rand());
}

Expand Down
4 changes: 2 additions & 2 deletions sdk/bpf/c/inc/sol/cpi.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ static const uint8_t MAX_CPI_INSTRUCTION_ACCOUNTS = 255;
/**
* Maximum number of account info structs that can be used in a single CPI
* invocation. A limit on account info structs is effectively the same as
* limiting the number of unique accounts. 64 was chosen to match the max
* limiting the number of unique accounts. 128 was chosen to match the max
* number of locked accounts per transaction (MAX_TX_ACCOUNT_LOCKS).
*/
static const uint8_t MAX_CPI_ACCOUNT_INFOS = 64;
static const uint16_t MAX_CPI_ACCOUNT_INFOS = 128;

/**
* Account Meta
Expand Down
4 changes: 2 additions & 2 deletions sdk/bpf/c/inc/sol/inc/cpi.inc
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ static const uint8_t MAX_CPI_INSTRUCTION_ACCOUNTS = 255;
/**
* Maximum number of account info structs that can be used in a single CPI
* invocation. A limit on account info structs is effectively the same as
* limiting the number of unique accounts. 64 was chosen to match the max
* limiting the number of unique accounts. 128 was chosen to match the max
* number of locked accounts per transaction (MAX_TX_ACCOUNT_LOCKS).
*/
static const uint8_t MAX_CPI_ACCOUNT_INFOS = 64;
static const uint16_t MAX_CPI_ACCOUNT_INFOS = 128;

/**
* Account Meta
Expand Down
4 changes: 2 additions & 2 deletions sdk/program/src/syscalls/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ pub const MAX_CPI_INSTRUCTION_ACCOUNTS: u8 = u8::MAX;

/// Maximum number of account info structs that can be used in a single CPI
/// invocation. A limit on account info structs is effectively the same as
/// limiting the number of unique accounts. 64 was chosen to match the max
/// limiting the number of unique accounts. 128 was chosen to match the max
/// number of locked accounts per transaction (MAX_TX_ACCOUNT_LOCKS).
pub const MAX_CPI_ACCOUNT_INFOS: usize = 64;
pub const MAX_CPI_ACCOUNT_INFOS: usize = 128;
5 changes: 5 additions & 0 deletions sdk/src/feature_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,10 @@ pub mod return_none_for_zero_lamport_accounts {
solana_sdk::declare_id!("7K5HFrS1WAq6ND7RQbShXZXbtAookyTfaDQPTJNuZpze");
}

pub mod increase_tx_account_lock_limit {
solana_sdk::declare_id!("9LZdXeKGeBV6hRLdxS1rHbHoEUsKqesCC2ZAPTPKJAbK");
}

lazy_static! {
/// Map of feature identifiers to user-visible description
pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
Expand Down Expand Up @@ -629,6 +633,7 @@ lazy_static! {
(incremental_snapshot_only_incremental_hash_calculation::id(), "only hash accounts in incremental snapshot during incremental snapshot creation #26799"),
(vote_state_update_root_fix::id(), "fix root in vote state updates #27361"),
(return_none_for_zero_lamport_accounts::id(), "return none for zero lamport accounts #27800"),
(increase_tx_account_lock_limit::id(), "increase tx account lock limit to 128 #27241"),
/*************** ADD NEW FEATURES HERE ***************/
]
.iter()
Expand Down
13 changes: 8 additions & 5 deletions sdk/src/transaction/sanitized.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ use {
};

/// Maximum number of accounts that a transaction may lock.
/// 64 was chosen because it is roughly twice the previous
/// number of account keys that could fit in a legacy tx.
pub const MAX_TX_ACCOUNT_LOCKS: usize = 64;
/// 128 was chosen because it is the minimum number of accounts
/// needed for the Neon EVM implementation.
pub const MAX_TX_ACCOUNT_LOCKS: usize = 128;

/// Sanitized transaction and the hash of its message
#[derive(Debug, Clone)]
Expand Down Expand Up @@ -208,10 +208,13 @@ impl SanitizedTransaction {
}

/// Validate and return the account keys locked by this transaction
pub fn get_account_locks(&self) -> Result<TransactionAccountLocks> {
pub fn get_account_locks(
&self,
tx_account_lock_limit: usize,
) -> Result<TransactionAccountLocks> {
if self.message.has_duplicates() {
Err(TransactionError::AccountLoadedTwice)
} else if self.message.account_keys().len() > MAX_TX_ACCOUNT_LOCKS {
} else if self.message.account_keys().len() > tx_account_lock_limit {
Err(TransactionError::TooManyAccountLocks)
} else {
Ok(self.get_account_locks_unchecked())
Expand Down