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

Make Blockstore populate TransactionStatusIndex entries #33756

Merged
merged 2 commits into from
Oct 19, 2023

Conversation

steviez
Copy link
Contributor

@steviez steviez commented Oct 18, 2023

Problem

A previous change removed logic that populated the
TransactionStatusIndex entries at each of the legacy primary index keys
(0 and 1). While these entries will not be read or written in the
future, these entries are necessary for backwards compatibility. Namely,
branches <= v1.17 expect these entries to be present and .unwrap()'s
could fail if they are not.

So, add the initialization of these entries back into Blockstore logic.
We can remove initialization of these entries once our stable and beta
branches are both versions that do not expect these entries to be
present (should be v1.18).

I encountered this issue originally by:

  • Creating a Blockstore with master (which does not insert these entries) AND
  • Attempting to open the Blockstore with solana-ledger-tool which opens in Secondary (read-only mode)

@steviez steviez changed the title Make Blockstore populate TransactionStatusIndex for backwards compat Make Blockstore populate TransactionStatusIndex entries Oct 19, 2023
A previous change removed logic that populated the
TransactionStatusIndex entries at each of the legacy primary index keys
(0 and 1). While these entries will not be read or written in the
future, these entries are necessary for backwards compatibility. Namely,
branches <= v1.17 expect these entries to be present and .unwrap()'s
could fail if they are not.

So, add the initialization of these entries back into Blockstore logic.
We can remove initialization of these entries once our stable and beta
branches are both versions that do not expect these entries to be
present (should be v1.18).
@steviez steviez marked this pull request as ready for review October 19, 2023 20:07
@@ -2152,7 +2163,7 @@ impl Blockstore {
highest_primary_index_slot = Some(meta.max_slot);
}
}
if highest_primary_index_slot.is_some() {
if highest_primary_index_slot.is_some_and(|slot| slot != 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels a little dirty but couldn't think of a better way to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, really my fault for all the clean_slot_0 stuff, though. This wfm!

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Merging #33756 (2cf244b) into master (e96678b) will increase coverage by 0.0%.
Report is 19 commits behind head on master.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #33756   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         806      806           
  Lines      217913   217919    +6     
=======================================
+ Hits       178286   178295    +9     
+ Misses      39627    39624    -3     

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.

Thank you, and sorry for not catching this in review!

@@ -2152,7 +2163,7 @@ impl Blockstore {
highest_primary_index_slot = Some(meta.max_slot);
}
}
if highest_primary_index_slot.is_some() {
if highest_primary_index_slot.is_some_and(|slot| slot != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, really my fault for all the clean_slot_0 stuff, though. This wfm!

@steviez steviez merged commit 383aef2 into solana-labs:master Oct 19, 2023
16 checks passed
@steviez steviez deleted the bstore_clean_up_my_mess branch October 19, 2023 21:28
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