Skip to content

Refactor transaction account unlocking#103

Merged
jstarry merged 1 commit intoanza-xyz:masterfrom
jstarry:refactor/accounts-unlock
Mar 7, 2024
Merged

Refactor transaction account unlocking#103
jstarry merged 1 commit intoanza-xyz:masterfrom
jstarry:refactor/accounts-unlock

Conversation

@jstarry
Copy link
Copy Markdown

@jstarry jstarry commented Mar 6, 2024

Problem

Current transaction account unlocking APIs aren't ideal for implementing partial transaction batch unlocking. Partial unlocking is needed for solana-labs#34825 by implementing a process of

  1. lock txs
  2. check qos
  3. unlock txs that fail qos

Summary of Changes

  • Changed Accounts::unlock_accounts and Bank::unlock_accounts to operate over a zipped iterator of transactions and lock results so that it's easier to implement a partial unlock of a transaction batch.
  • Added debug assertions to ensure that no tests are relying on double unlocking transactions
  • Added an is_empty check inside Accounts::unlock_accounts to avoid taking the accounts lock if it's not needed

Fixes #

@jstarry jstarry requested review from apfitzge and tao-stones March 6, 2024 07:19
@jstarry jstarry marked this pull request as draft March 6, 2024 13:35
@jstarry jstarry force-pushed the refactor/accounts-unlock branch from 7445684 to e702fa6 Compare March 6, 2024 13:54
@jstarry jstarry marked this pull request as ready for review March 6, 2024 13:56
Copy link
Copy Markdown

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

Neat!

Copy link
Copy Markdown

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

lgtm

fn drop(&mut self) {
self.bank.unlock_accounts(self)
if self.needs_unlock() {
self.set_needs_unlock(false);
Copy link
Copy Markdown

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
Copy Markdown
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

@jstarry jstarry merged commit 9cc5534 into anza-xyz:master Mar 7, 2024
codebender828 pushed a commit to codebender828/agave that referenced this pull request Oct 3, 2024
OliverNChalk pushed a commit to OliverNChalk/agave that referenced this pull request Nov 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants