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

transaction_status_service: remove get_account_locks_unchecked #2555

Merged
merged 2 commits into from
Aug 20, 2024

Conversation

apfitzge
Copy link

Problem

  • validate_account_locks function got pulled into account_locks from sdk
  • following that change, get_account_locks and get_account_locks_unchecked will be deprecated
  • transaction_status_service uses get_account_locks_unchecked

Summary of Changes

  • Replace use of get_account_locks_unchecked with more direct access
  • Refactor blockstore::write_transaction_status to take a single iterator (simplifies code)

Fixes #

@@ -2868,12 +2868,11 @@ impl Blockstore {
}
}

pub fn write_transaction_status(
pub fn write_transaction_status<'a>(
Copy link
Author

Choose a reason for hiding this comment

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

refactor of this function makes its' internal code slightly simpler

@@ -111,7 +111,6 @@ impl TransactionStatusService {
rent_debits,
..
} = committed_tx;
let tx_account_locks = transaction.get_account_locks_unchecked();
Copy link
Author

Choose a reason for hiding this comment

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

removing this also removes a pair of allocations

@apfitzge apfitzge self-assigned this Aug 12, 2024
@apfitzge apfitzge marked this pull request as ready for review August 12, 2024 16:04
@apfitzge apfitzge requested a review from steviez August 12, 2024 16:04
Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

In general, the code looks good to me and save some temporary allocations as you mentioned.

@CriesofCarrots - In the past, I believe we have been more cautious with adjusting pub function in Blockstore than some other crates, as Blockstore is a little internal facing than some of the other stuff.

Do you know of anything that might be broken specifically if we adjust this, or just trying to be friendly to anyone downstream who might be using Blockstore ?

@@ -171,12 +170,18 @@ impl TransactionStatusService {
.expect("Expect database write to succeed: TransactionMemos");
}

let message = transaction.message();
Copy link

Choose a reason for hiding this comment

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

nit: On line 167, we call the same function. The underlying function just returns a ref so not a big deal, but if message() got refactored to do something more meaningful, would want to avoid duplicate work:

pub fn message(&self) -> &SanitizedMessage {
&self.message
}

Copy link
Author

Choose a reason for hiding this comment

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

eventually message() will go away entirely 😉 . or at least be abstracted by a trait (SVM{Transaction|Message}?)

@CriesofCarrots
Copy link

In the past, I believe we have been more cautious with adjusting pub function in Blockstore than some other crates, as Blockstore is a little internal facing than some of the other stuff.

Do you know of anything that might be broken specifically if we adjust this, or just trying to be friendly to anyone downstream who might be using Blockstore ?

@steviez , I think your first sentence might be missing a word? (Is it more or less internal-facing?)
But I don't know of anything specifically that would be broken by this change. The methods that write to columns are used pretty exclusively by blockstore internals and/or agave validator supplementary metadata services.

@steviez
Copy link

steviez commented Aug 19, 2024

@steviez , I think your first sentence might be missing a word? (Is it more or less internal-facing?)

Dang it yep, I meant as Blockstore is a little more external internal facing than some of the other stuff.

But I don't know of anything specifically that would be broken by this change. The methods that write to columns are used pretty exclusively by blockstore internals and/or agave validator supplementary metadata services.

👍

@apfitzge apfitzge merged commit 925a603 into anza-xyz:master Aug 20, 2024
41 checks passed
@apfitzge apfitzge deleted the blockstore_get_account_locks branch August 20, 2024 13:48
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