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

Refactor transaction account unlocking #103

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Refactor transaction account unlocking #103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
64 changes: 44 additions & 20 deletions accounts-db/src/accounts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,20 @@ impl AccountLocks {
if *count == 0 {
occupied_entry.remove_entry();
}
} else {
debug_assert!(
false,
"Attempted to remove a read-lock for a key that wasn't read-locked"
);
}
}

fn unlock_write(&mut self, key: &Pubkey) {
self.write_locks.remove(key);
let removed = self.write_locks.remove(key);
debug_assert!(
removed,
"Attempted to remove a write-lock for a key that wasn't write-locked"
);
}
}

Expand Down Expand Up @@ -618,14 +627,16 @@ impl Accounts {
#[allow(clippy::needless_collect)]
pub fn unlock_accounts<'a>(
&self,
txs: impl Iterator<Item = &'a SanitizedTransaction>,
results: &[Result<()>],
txs_and_results: impl Iterator<Item = (&'a SanitizedTransaction, &'a Result<()>)>,
) {
let keys: Vec<_> = txs
.zip(results)
let keys: Vec<_> = txs_and_results
.filter(|(_, res)| res.is_ok())
.map(|(tx, _)| tx.get_account_locks_unchecked())
.collect();
if keys.is_empty() {
return;
}

let mut account_locks = self.account_locks.lock().unwrap();
debug!("bank unlock accounts");
keys.into_iter().for_each(|keys| {
Expand Down Expand Up @@ -812,6 +823,7 @@ mod tests {
},
std::{
borrow::Cow,
iter,
sync::atomic::{AtomicBool, AtomicU64, Ordering},
thread, time,
},
Expand Down Expand Up @@ -1099,8 +1111,8 @@ mod tests {

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

// Disallow over MAX_TX_ACCOUNT_LOCKS
Expand Down Expand Up @@ -1156,7 +1168,7 @@ mod tests {
let tx = new_sanitized_tx(&[&keypair0], message, Hash::default());
let results0 = accounts.lock_accounts([tx.clone()].iter(), MAX_TX_ACCOUNT_LOCKS);

assert!(results0[0].is_ok());
assert_eq!(results0, vec![Ok(())]);
assert_eq!(
*accounts
.account_locks
Expand Down Expand Up @@ -1190,9 +1202,13 @@ mod tests {
let tx1 = new_sanitized_tx(&[&keypair1], message, Hash::default());
let txs = vec![tx0, tx1];
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
assert_eq!(
results1,
vec![
Ok(()), // Read-only account (keypair1) can be referenced multiple times
Err(TransactionError::AccountInUse), // Read-only account (keypair1) cannot also be locked as writable
],
);
assert_eq!(
*accounts
.account_locks
Expand All @@ -1204,8 +1220,8 @@ mod tests {
2
);

accounts.unlock_accounts([tx].iter(), &results0);
accounts.unlock_accounts(txs.iter(), &results1);
accounts.unlock_accounts(iter::once(&tx).zip(&results0));
accounts.unlock_accounts(txs.iter().zip(&results1));
let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])];
let message = Message::new_with_compiled_instructions(
1,
Expand All @@ -1217,7 +1233,10 @@ mod tests {
);
let tx = new_sanitized_tx(&[&keypair1], message, Hash::default());
let results2 = accounts.lock_accounts([tx].iter(), MAX_TX_ACCOUNT_LOCKS);
assert!(results2[0].is_ok()); // Now keypair1 account can be locked as writable
assert_eq!(
results2,
vec![Ok(())] // Now keypair1 account can be locked as writable
);

// Check that read-only lock with zero references is deleted
assert!(accounts
Expand Down Expand Up @@ -1285,7 +1304,7 @@ mod tests {
counter_clone.clone().fetch_add(1, Ordering::SeqCst);
}
}
accounts_clone.unlock_accounts(txs.iter(), &results);
accounts_clone.unlock_accounts(txs.iter().zip(&results));
if exit_clone.clone().load(Ordering::Relaxed) {
break;
}
Expand All @@ -1301,7 +1320,7 @@ mod tests {
thread::sleep(time::Duration::from_millis(50));
assert_eq!(counter_value, counter_clone.clone().load(Ordering::SeqCst));
}
accounts_arc.unlock_accounts(txs.iter(), &results);
accounts_arc.unlock_accounts(txs.iter().zip(&results));
thread::sleep(time::Duration::from_millis(50));
}
exit.store(true, Ordering::Relaxed);
Expand Down Expand Up @@ -1442,9 +1461,14 @@ mod tests {
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()
assert!(results[2].is_ok()); // Read-only account (keypair0) can be referenced multiple times
assert_eq!(
results,
vec![
Ok(()), // Read-only account (keypair0) can be referenced multiple times
Err(TransactionError::WouldExceedMaxBlockCostLimit), // is not locked due to !qos_results[1].is_ok()
Ok(()), // Read-only account (keypair0) can be referenced multiple times
],
);

// verify that keypair0 read-only lock twice (for tx0 and tx2)
assert_eq!(
Expand All @@ -1466,7 +1490,7 @@ mod tests {
.get(&keypair2.pubkey())
.is_none());

accounts.unlock_accounts(txs.iter(), &results);
accounts.unlock_accounts(txs.iter().zip(&results));

// check all locks to be removed
assert!(accounts
Expand Down
12 changes: 5 additions & 7 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4376,13 +4376,11 @@ impl Bank {
account_overrides
}

pub fn unlock_accounts(&self, batch: &mut TransactionBatch) {
if batch.needs_unlock() {
batch.set_needs_unlock(false);
self.rc
.accounts
.unlock_accounts(batch.sanitized_transactions().iter(), batch.lock_results())
}
pub fn unlock_accounts<'a>(
&self,
txs_and_results: impl Iterator<Item = (&'a SanitizedTransaction, &'a Result<()>)>,
) {
self.rc.accounts.unlock_accounts(txs_and_results)
}

pub fn remove_unrooted_slots(&self, slots: &[(Slot, BankId)]) {
Expand Down
9 changes: 8 additions & 1 deletion runtime/src/transaction_batch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,14 @@ impl<'a, 'b> TransactionBatch<'a, 'b> {
// Unlock all locked accounts in destructor.
impl<'a, 'b> Drop for TransactionBatch<'a, 'b> {
fn drop(&mut self) {
self.bank.unlock_accounts(self)
if self.needs_unlock() {
self.set_needs_unlock(false);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize we had this previously, but I'm struggling to think why this is necessary. We're in middle of dropping this batch, so it won't get dropped and attempt unlock again, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, it's really not necessary to set to false here

self.bank.unlock_accounts(
self.sanitized_transactions()
.iter()
.zip(self.lock_results()),
)
}
}
}

Expand Down
Loading