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

Remove dummy entries in Blockstore special columns (part 2) #33649

Merged
merged 4 commits into from
Oct 11, 2023

Conversation

steviez
Copy link
Contributor

@steviez steviez commented Oct 11, 2023

Problem

#33511 previously attempted to make Blockstore::open() code remove dummy entries to enable the special case check that #33534 added. However, this change was flawed:

  • The lines to remove the dummy entries was added to Blockstore::initialize_transaction_status_index()
  • Blockstore::initialize_transaction_status_index() would only get called if the TransactionStatusIndex entries are not populated
  • The previous code populated the TransactionStatusIndex entries and the special columns dummy entries at the same time

Thus, the logic to cleanup the dummy entries would only get called if TransactionStatusIndex entries are absent. But, the case where we need to clean up dummy entries is the case where TransactionStatusIndex` entries are present.

Summary of Changes

  • Rename the initialization function and ensure it is always called at open
  • Stop populating TransactionStatusIndex entries
    • These are now unused, and if the ledger is opened with an old software version that uses these entries, it will re-populate them
    • For now, I left any existing TransactionStatusIndex entries untouched as @CriesofCarrots previously expressed an idea to me of being able to use values in these entries to detect when the ledger is fully cleaned of old-key formats

@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Merging #33649 (a4c7cf9) into master (4042079) will decrease coverage by 0.1%.
Report is 1075 commits behind head on master.
The diff coverage is 87.5%.

@@            Coverage Diff            @@
##           master   #33649     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         766      806     +40     
  Lines      209084   217450   +8366     
=========================================
+ Hits       171145   177992   +6847     
- Misses      37939    39458   +1519     

@steviez steviez merged commit 6009d49 into solana-labs:master Oct 11, 2023
16 checks passed
@steviez steviez deleted the bstore_init_tx_status_idx branch October 11, 2023 18:13
steviez pushed a commit to steviez/solana that referenced this pull request Oct 18, 2023
Versions v1.16 and v1.17 expect these entries to be populated; there is
code that calls .unwrap() on the Option returned by .get(). A change
that is currently in master (and should land in v1.18) will make it such
that these entries are no longer read.

Until the TransactionStatusIndexMeta's are no longer read, they should
continue to be populated to allow for software downgrade scenarios. So,
add back code that solana-labs#33649 overzealously removed to populate these
entries.
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