blockstore: Allow fallback for AddressSignature index() #34440
Conversation
A change landed somewhat recently in master that changed the key format of the transaction metadata columns. A compatibility backport was introduced to allow a blockstore that had been populated with this newer version to still be readable by v1.17 (backwards software compat). However, there was an oversight in the backport. Namely, the index() function for AddressSignatures column did a regular unwrap() on the try_current_index() result. try_current_index() can fail if a key with an unknown size is encountered. This would be exactly the case for encountering a key that was populated by the newer software version with the different key format. So, use .unwrap_or_else() in the index() implementation for AddressSignatures; this will now be consistent with the implementation of index() for TransactionStatus column.
|
@CriesofCarrots - I'm going to try to manually reproduce this panic, but just throwing on your radar incase you want to mull on it as well. One related item that I was thinking about... By returning For v1.17, I think that is fine given that v1.17 can't read the key/value pair anyways. But, some potential weirdness if someone is toggling back and forth between v1.17 and v1.18 (master). I think this is fine because that shouldn't be the common use case, but mentioning for completeness. I think the only way to avoid that would be backporting a lot more code to v1.17 in order to be able to read the new columns, which is probably not the way we want to go. |
steviez
changed the title
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## v1.17 #34440 +/- ##
=========================================
- Coverage 81.8% 81.8% -0.1%
=========================================
Files 803 803
Lines 218166 218167 +1
=========================================
- Hits 178515 178504 -11
- Misses 39651 39663 +12 |
|
Ah nuts, I can't believe we (I) messed this up for AddressSignatures 🤕
I also tend to think this is fine, although if we care about this potential data loss, we could extend the v1.17 version of the ColumnIndexDeprecation trait, I guess. |
Emphasis on "we"; I missed this too 😢 I was able to reproduce the panic right after the And then after restart with my patch applied, confirmed that the node was able to restart:
Cool, think we're on the same page here with "do nothing further" than this fix |
Ha, good point. Think I should let the v1.17 version update land before merging ? |
Up to you; I don't think it really matters much. I do sometimes scan through commit lists and use those bump commits to determine what's in the previous release, but that's obv an imperfect technique already :) |
I do the same, so I'll hold off until my manual bump goes through in #34451 |
|
It'd probably be more proper to rebase this and re-run CI, but I think it should be fine and I will clean up the mess if something goes wrong |
Problem
A change landed somewhat recently in master that changed the key format of the transaction metadata columns. A compatibility backport was introduced to allow a blockstore that had been populated with this newer version to still be readable by v1.17 (backwards software compat).
However, there was an oversight in the backport. Namely, the
index()function for AddressSignatures column did a regularunwrap()intry_current_index().try_current_index()can fail if a key with an unknown size is encountered. This would be exactly the case for encountering a key that was populated by the newer software version with the different key format.Namely, the following panic was observed when downgrading from recent tip of master to v1.17 with
--enable-rpc-transaction-historyflag:And here is the line for v1.17.9 mentioned in the panic message:
solana/ledger/src/blockstore_db.rs
Line 878 in 8b2c1a5
Summary of Changes
So, use
.unwrap_or_else()in theindex()implementation for AddressSignatures; this will now be consistent with the implementation of index() for TransactionStatus column.