Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

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
55 changes: 17 additions & 38 deletions runtime/src/accounts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1112,10 +1112,11 @@ impl Accounts {
pub fn lock_accounts<'a>(
&self,
txs: impl Iterator<Item = &'a SanitizedTransaction>,
feature_set: &FeatureSet,
tx_account_lock_limit: usize,
) -> Vec<Result<()>> {
let tx_account_locks_results: Vec<Result<_>> =
txs.map(|tx| tx.get_account_locks(feature_set)).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 @@ -1125,12 +1126,12 @@ impl Accounts {
&self,
txs: impl Iterator<Item = &'a SanitizedTransaction>,
results: impl Iterator<Item = &'a Result<()>>,
feature_set: &FeatureSet,
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(feature_set),
Ok(()) => tx.get_account_locks(tx_account_lock_limit),
Err(err) => Err(err.clone()),
})
.collect();
Expand Down Expand Up @@ -2507,7 +2508,7 @@ mod tests {
};

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

Expand Down Expand Up @@ -2540,12 +2541,12 @@ mod tests {
};

let txs = vec![new_sanitized_tx(&[&keypair], message, Hash::default())];
let results = accounts.lock_accounts(txs.iter(), &FeatureSet::all_enabled());
let results = accounts.lock_accounts(txs.iter(), MAX_TX_ACCOUNT_LOCKS);
assert_eq!(results[0], Ok(()));
accounts.unlock_accounts(txs.iter(), &results);
}

// Allow over MAX_TX_ACCOUNT_LOCKS before feature activation
// Disallow over MAX_TX_ACCOUNT_LOCKS
{
let num_account_keys = MAX_TX_ACCOUNT_LOCKS + 1;
let mut account_keys: Vec<_> = (0..num_account_keys)
Expand All @@ -2562,29 +2563,7 @@ mod tests {
};

let txs = vec![new_sanitized_tx(&[&keypair], message, Hash::default())];
let results = accounts.lock_accounts(txs.iter(), &FeatureSet::default());
assert_eq!(results[0], Ok(()));
accounts.unlock_accounts(txs.iter(), &results);
}

// Disallow over MAX_TX_ACCOUNT_LOCKS after feature activation
{
let num_account_keys = MAX_TX_ACCOUNT_LOCKS + 1;
let mut account_keys: Vec<_> = (0..num_account_keys)
.map(|_| Pubkey::new_unique())
.collect();
account_keys[0] = keypair.pubkey();
let message = Message {
header: MessageHeader {
num_required_signatures: 1,
..MessageHeader::default()
},
account_keys,
..Message::default()
};

let txs = vec![new_sanitized_tx(&[&keypair], message, Hash::default())];
let results = accounts.lock_accounts(txs.iter(), &FeatureSet::all_enabled());
let results = accounts.lock_accounts(txs.iter(), MAX_TX_ACCOUNT_LOCKS);
assert_eq!(results[0], Err(TransactionError::TooManyAccountLocks));
}
}
Expand Down Expand Up @@ -2623,7 +2602,7 @@ mod tests {
instructions,
);
let tx = new_sanitized_tx(&[&keypair0], message, Hash::default());
let results0 = accounts.lock_accounts([tx.clone()].iter(), &FeatureSet::all_enabled());
let results0 = accounts.lock_accounts([tx.clone()].iter(), MAX_TX_ACCOUNT_LOCKS);

assert!(results0[0].is_ok());
assert_eq!(
Expand Down Expand Up @@ -2658,7 +2637,7 @@ mod tests {
);
let tx1 = new_sanitized_tx(&[&keypair1], message, Hash::default());
let txs = vec![tx0, tx1];
let results1 = accounts.lock_accounts(txs.iter(), &FeatureSet::all_enabled());
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 @@ -2685,7 +2664,7 @@ mod tests {
instructions,
);
let tx = new_sanitized_tx(&[&keypair1], message, Hash::default());
let results2 = accounts.lock_accounts([tx].iter(), &FeatureSet::all_enabled());
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 @@ -2756,7 +2735,7 @@ mod tests {
let txs = vec![writable_tx.clone()];
let results = accounts_clone
.clone()
.lock_accounts(txs.iter(), &FeatureSet::all_enabled());
.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 @@ -2773,7 +2752,7 @@ mod tests {
let txs = vec![readonly_tx.clone()];
let results = accounts_arc
.clone()
.lock_accounts(txs.iter(), &FeatureSet::all_enabled());
.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 @@ -2819,7 +2798,7 @@ mod tests {
instructions,
);
let tx = new_sanitized_tx(&[&keypair0], message, Hash::default());
let results0 = accounts.lock_accounts([tx].iter(), &FeatureSet::all_enabled());
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 @@ -2913,7 +2892,7 @@ mod tests {
let results = accounts.lock_accounts_with_results(
txs.iter(),
qos_results.iter(),
&FeatureSet::all_enabled(),
MAX_TX_ACCOUNT_LOCKS,
);

assert!(results[0].is_ok()); // Read-only account (keypair0) can be referenced multiple times
Expand Down
42 changes: 28 additions & 14 deletions runtime/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ use {
timing::years_as_slots,
transaction::{
MessageHash, Result, SanitizedTransaction, Transaction, TransactionError,
TransactionVerificationMode, VersionedTransaction,
TransactionVerificationMode, VersionedTransaction, MAX_TX_ACCOUNT_LOCKS,
},
transaction_context::{InstructionTrace, TransactionAccount, TransactionContext},
},
Expand Down Expand Up @@ -3510,16 +3510,28 @@ impl Bank {
tick_height % self.ticks_per_slot == 0
}

/// 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(), &FeatureSet::all_enabled());
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 @@ -3539,10 +3551,10 @@ impl Bank {
)
})
.collect::<Result<Vec<_>>>()?;
let lock_results = self
.rc
.accounts
.lock_accounts(sanitized_txs.iter(), &FeatureSet::all_enabled());
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 @@ -3558,7 +3570,7 @@ impl Bank {
let lock_results = self
.rc
.accounts
.lock_accounts(txs.iter(), &self.feature_set);
.lock_accounts(txs.iter(), self.get_transaction_account_lock_limit());
TransactionBatch::new(lock_results, self, Cow::Borrowed(txs))
}

Expand All @@ -3573,7 +3585,7 @@ impl Bank {
let lock_results = self.rc.accounts.lock_accounts_with_results(
transactions.iter(),
transaction_results,
&self.feature_set,
self.get_transaction_account_lock_limit(),
);
TransactionBatch::new(lock_results, self, Cow::Borrowed(transactions))
}
Expand All @@ -3583,7 +3595,9 @@ impl Bank {
&'a self,
transaction: SanitizedTransaction,
) -> TransactionBatch<'a, '_> {
let lock_result = transaction.get_account_locks(&self.feature_set).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 @@ -7007,7 +7021,6 @@ pub(crate) mod tests {
system_program,
sysvar::rewards::Rewards,
timing::duration_as_s,
transaction::MAX_TX_ACCOUNT_LOCKS,
transaction_context::InstructionContext,
},
solana_vote_program::{
Expand Down Expand Up @@ -13060,7 +13073,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
5 changes: 5 additions & 0 deletions sdk/src/feature_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,10 @@ pub mod check_ping_ancestor_requests {
solana_sdk::declare_id!("AXLB87anNaUQtqBSsxkm4gvNzYY985aLtNtpJC94uWLJ");
}

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 @@ -523,6 +527,7 @@ lazy_static! {
(sign_repair_requests::id(), "sign repair requests #26834"),
(check_ping_ancestor_requests::id(), "ancestor hash repair socket ping/pong support #26963"),
(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
12 changes: 5 additions & 7 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 @@ -210,13 +210,11 @@ impl SanitizedTransaction {
/// Validate and return the account keys locked by this transaction
pub fn get_account_locks(
&self,
feature_set: &feature_set::FeatureSet,
tx_account_lock_limit: usize,
) -> Result<TransactionAccountLocks> {
if self.message.has_duplicates() {
Err(TransactionError::AccountLoadedTwice)
} else if feature_set.is_active(&feature_set::max_tx_account_locks::id())
&& 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