v1.17: blockstore: relax parent slot meta check for clear_unconfirmed_slot #67
Conversation
| } else { | ||
| error!( | ||
| "Parent slot meta {} for child {} is missing or cleaned up. | ||
| Falling back to orphan repair to remedy the situation", | ||
| parent_slot, slot | ||
| ); | ||
| } |
There was a problem hiding this comment.
This is a pretty large BP to consider for v1.17 & v1.18; most of the change was a refactor to clean things up. I think we could get away with BP'ing much less, namely, just this bit to print an error instead of doing the following expect that is currently in the v1.17/18 branches:
.expect("Unconfirmed slot should have had parent slot set");There was a problem hiding this comment.
agreed, i think we can just error instead of expect. the atomic write can wait for master
There was a problem hiding this comment.
Right. And thinking out loud, suppose that C is the child of P. If we purge C but can't find P's meta, we'll repopulate a meta for P when C is repaired / reinserted
AshwinSekar
changed the title
AshwinSekar
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v1.17 #67 +/- ##
=======================================
Coverage 81.6% 81.6%
=======================================
Files 806 806
Lines 219092 219097 +5
=======================================
+ Hits 178852 178916 +64
+ Misses 40240 40181 -59 |
There was a problem hiding this comment.
I think this one is good for v1.17 as well, but I think it would be good to get additional buy in before pushing. Again, the reason being is that we're relaxing a panic to an error!() message in a scenario that we might be able to recover from.
In the v1.18 BP, Trent did call out that this diff is significantly different than the original PR. That is probably fair, and to echo a comment from Ashwin, we probably could have done the panic-to-error demotion in one PR and refactor in subsequent PR.
Agree with the general sentiment but here we are; the diff itself is pretty contained and easy to reason about so I don't think we have to abort this one solely for that reason.
For completeness, gonna add Trent and Will to the review
backport of the relevant parts in solana-labs#35124