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

ledger-tool: Adjust logic to obtain TransactionStatusService Blockstore #34646

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

steviez
Copy link
Contributor

@steviez steviez commented Jan 4, 2024

Problem

TransactionStatusService needs Primary access in order to write
transaction status into the Blockstore if enable_rpc_transaction_history
is set to True. The current logic attempts to get Primary access for the
service.

Summary of Changes

However, in the event that this function had been called with a
Blockstore that already had Primary access, this second attempt to get
Primary access would fail. So, only attempt to open with Primary access
when necessary AND when the current access level is not sufficient.

Note that this function is currently called with Secondary (read only) blockstore access. So, we would not currently run into this error with the default. Someone setting several flags in a particular manner could technically encounter this error.

Regardless, there is no requirement about the access type of the Blockstore passed into load_and_process_ledger(). So, this change is more defensive in the event that we start calling this function with Primary access, or if someone happens to provide the exact combination of args to hit the error that this change prevents

TransactionStatusService needs Primary access in order to write
transaction status into the Blockstore if enable_rpc_transaction_history
is set to True. The current logic attempts to get Primary access for the
service.

However, in the event that this function had been called with a
Blockstore that already had Primary access, this second attempt to get
Primary access would fail. So, only attempt to open with Primary access
when necessary AND when the current access level is not sufficient.
{
// Need Primary (R/W) access to insert transaction data;
// obtain Primary access if we do not already have it
let tss_blockstore = if enable_rpc_transaction_history && !blockstore.is_primary_access() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only logic change was the addition of && !blockstore.is_primary_access() to this line (first commit). It is unintuitive to me that this change would make the linter want to adjust lines for the if statement that encloses this line (and other stuff), but that is what cargo fmt wanted so 🤷

Copy link

codecov bot commented Jan 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0b49d82) 81.8% compared to head (d5a1f5c) 81.8%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #34646   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         824      824           
  Lines      222393   222393           
=======================================
+ Hits       181954   181972   +18     
+ Misses      40439    40421   -18     

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Ah cool... I was wondering about this reopening when I reviewed the arg_matches PR, but didn't take the time to trace the code.
Fix lgtm!

@steviez
Copy link
Contributor Author

steviez commented Jan 4, 2024

Ah cool... I was wondering about this reopening when I reviewed the arg_matches PR, but didn't take the time to trace the code.

Ha yup, your comment there was the only reason I noticed this

@steviez steviez merged commit 9fe2037 into solana-labs:master Jan 4, 2024
35 checks passed
@steviez steviez deleted the lt_tss_primary branch January 4, 2024 16:31
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.

2 participants