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.

Sign up for GitHub

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

Limit number of accounts that a transaction can lock (backport #22201) #22263

Merged
merged 2 commits into from
Jan 4, 2022
Merged

Limit number of accounts that a transaction can lock (backport #22201) #22263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
35f949a
Limit number of accounts that a transaction can lock (#22201)
jstarry Jan 4, 2022
6cfcb25
resolve conflicts
jstarry Jan 4, 2022
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
6 changes: 6 additions & 0 deletions accountsdb-plugin-postgres/src/postgres_client/postgres_client_transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,8 @@ pub enum DbTransactionErrorCode {
WouldExceedMaxBlockCostLimit,
UnsupportedVersion,
InvalidWritableAccount,
WouldExceedMaxAccountDataCostLimit,
TooManyAccountLocks,
}

impl From<&TransactionError> for DbTransactionErrorCode {
Expand Down Expand Up @@ -358,6 +360,10 @@ impl From<&TransactionError> for DbTransactionErrorCode {
TransactionError::WouldExceedMaxBlockCostLimit => Self::WouldExceedMaxBlockCostLimit,
TransactionError::UnsupportedVersion => Self::UnsupportedVersion,
TransactionError::InvalidWritableAccount => Self::InvalidWritableAccount,
TransactionError::WouldExceedMaxAccountDataCostLimit => {
Self::WouldExceedMaxAccountDataCostLimit
}
TransactionError::TooManyAccountLocks => Self::TooManyAccountLocks,
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion rpc/src/transaction_status_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
181 changes: 153 additions & 28 deletions runtime/src/accounts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ use {
pubkey::Pubkey,
system_program,
sysvar::{self, instructions::construct_instructions_data},
transaction::{Result, SanitizedTransaction, TransactionError},
transaction::{Result, SanitizedTransaction, TransactionAccountLocks, TransactionError},
},
std::{
cmp::Reverse,
Expand Down Expand Up @@ -952,12 +952,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]
Expand All @@ -966,20 +965,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()
}
Expand All @@ -994,12 +1006,14 @@ impl Accounts {
let keys: Vec<_> = txs
.zip(results)
.filter_map(|(tx, res)| match res {
Err(TransactionError::AccountInUse) => None,
Err(TransactionError::SanitizeFailure) => None,
Err(TransactionError::AccountLoadedTwice) => None,
Err(TransactionError::WouldExceedMaxBlockCostLimit) => None,
Err(TransactionError::WouldExceedMaxAccountCostLimit) => None,
_ => Some(tx.get_account_locks()),
Err(TransactionError::AccountLoadedTwice)
| Err(TransactionError::AccountInUse)
| Err(TransactionError::SanitizeFailure)
| Err(TransactionError::TooManyAccountLocks)
| Err(TransactionError::WouldExceedMaxBlockCostLimit)
| Err(TransactionError::WouldExceedMaxAccountCostLimit)
| Err(TransactionError::WouldExceedMaxAccountDataCostLimit) => None,
_ => Some(tx.get_account_locks_unchecked()),
})
.collect();
let mut account_locks = self.account_locks.lock().unwrap();
Expand Down Expand Up @@ -1222,12 +1236,12 @@ mod tests {
genesis_config::ClusterType,
hash::Hash,
instruction::{CompiledInstruction, InstructionError},
message::Message,
message::{Message, MessageHeader},
nonce, nonce_account,
rent::Rent,
signature::{keypair_from_seed, signers::Signers, Keypair, Signer},
system_instruction, system_program,
transaction::Transaction,
transaction::{Transaction, MAX_TX_ACCOUNT_LOCKS},
},
std::{
convert::TryFrom,
Expand Down Expand Up @@ -2105,6 +2119,109 @@ mod tests {
accounts.bank_hash_at(1);
}

#[test]
fn test_lock_accounts_with_duplicates() {
let accounts = Accounts::new_with_config_for_tests(
Vec::new(),
&ClusterType::Development,
AccountSecondaryIndexes::default(),
false,
AccountShrinkThreshold::default(),
);

let keypair = Keypair::new();
let message = Message {
header: MessageHeader {
num_required_signatures: 1,
..MessageHeader::default()
},
account_keys: vec![keypair.pubkey(), keypair.pubkey()],
..Message::default()
};

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

#[test]
fn test_lock_accounts_with_too_many_accounts() {
let accounts = Accounts::new_with_config_for_tests(
Vec::new(),
&ClusterType::Development,
AccountSecondaryIndexes::default(),
false,
AccountShrinkThreshold::default(),
);

let keypair = Keypair::new();

// Allow up to MAX_TX_ACCOUNT_LOCKS
{
let num_account_keys = MAX_TX_ACCOUNT_LOCKS;
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());
assert_eq!(results[0], Ok(()));
accounts.unlock_accounts(txs.iter(), &results);
}

// Allow over MAX_TX_ACCOUNT_LOCKS before 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::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());
assert_eq!(results[0], Err(TransactionError::TooManyAccountLocks));
}
}

#[test]
fn test_accounts_locks() {
let keypair0 = Keypair::new();
Expand Down Expand Up @@ -2139,7 +2256,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!(
Expand Down Expand Up @@ -2174,7 +2291,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
Expand All @@ -2201,7 +2318,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
Expand Down Expand Up @@ -2270,7 +2387,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);
Expand All @@ -2285,7 +2404,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));
Expand Down Expand Up @@ -2331,7 +2452,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
Expand Down Expand Up @@ -2422,7 +2543,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()
Expand Down
Loading