[Merged by Bors] - Initial Commit of Retrospective OTB Verification #3372
Conversation
paulhauner
added
work-in-progress
There was a problem hiding this comment.
This is super clean, it was a pleasure to read! 🎉
This was a fiddly task done in a short time and you've done really well IMO! I've made a few suggestions, you'll see each suggestion is accompanied by a commit in this branch: https://github.com/paulhauner/lighthouse/tree/rOTBv-2
You can just merge rOTBv-2 into this branch, if you're happy with my suggestions. There's a couple of extra commits on that branch that don't show up in comments:
- paulhauner@ab00945: Fix some merge conflicts
- paulhauner@3d56603: Tidy some comments and add a couple of paranoid checks in tests
| for otb in unfinalized_canonical_otbs { | ||
| match chain.get_block(otb.root()).await { | ||
| Ok(Some(block)) => { | ||
| match validate_merge_block(chain, block.message()).await { |
There was a problem hiding this comment.
validate_merge_block will return Ok(()) when it's past SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY and the EL is still syncing. In this scenario we're declare that the OTB is fully valid, when we actually just optimistically verified it again. It would be an OOTB!
I've suggest a fix and regression test for this in paulhauner@a030175.
There was a problem hiding this comment.
Great catch! 🙏
I knew this would be called recursively, which was why I added this check but somehow just didn't realize that it returns Ok(()) after attempting to re-insert 🤦♂️
I've merged your fix in. I wonder if we should remove the double insert check? I'm not even sure if leveldb returns an error if the entry already exists but that actually would've been better in this case.
There was a problem hiding this comment.
I wonder if we should remove the double insert check? I'm not even sure if leveldb returns an error if the entry already exists but that actually would've been better in this case.
I don't think the check is strictly necessary, but I don't mind leaving it there. I don't think it'll have any functional impact.
paulhauner
added
waiting-on-author
ethDreamer
added
ready-for-review
paulhauner
added
ready-for-merge
|
bors r+ |
## Issue Addressed * #2983 ## Proposed Changes Basically followed the [instructions laid out here](#2983 (comment)) Co-authored-by: Paul Hauner <[email protected]> Co-authored-by: ethDreamer <[email protected]>
|
This PR was included in a batch that timed out, it will be automatically retried |
|
bors r- |
|
Canceled. |
|
bors r+ |
## Issue Addressed * #2983 ## Proposed Changes Basically followed the [instructions laid out here](#2983 (comment)) Co-authored-by: Paul Hauner <[email protected]> Co-authored-by: ethDreamer <[email protected]>
bors
bot
changed the title
Issue Addressed
Proposed Changes
Basically followed the instructions laid out here