Rm finality blocks and refactor checkpoint_submitter to be resilient to edge cases #2836
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## v3 #2836 +/- ##
==========================================
- Coverage 62.53% 62.43% -0.10%
==========================================
Files 99 99
Lines 1017 1017
Branches 106 106
==========================================
- Hits 636 635 -1
Misses 344 344
- Partials 37 38 +1
|
| // | ||
| // tree.index() will panic if the tree is empty, so we use tree.count() instead | ||
| // and convert the correctness_checkpoint.index to a count by adding 1. | ||
| while correctness_checkpoint.index + 1 > tree.count() as u32 { |
There was a problem hiding this comment.
can you use tree_exceeds_checkpoint here?
There was a problem hiding this comment.
See the comment above on tree.index(), it panics if the count is 0 so this is the unfortunate workaround
And I don't think so because tree_exceeds_checkpoint returns true if tree.index > checkpoint.index (so doing !tree_exceeds_checkpoint would be if tree.index <= checkpoint.index) and we want the condition here to be checkpoint.index > tree.index
| // small sleep before signing next checkpoint to avoid rate limiting | ||
| sleep(Duration::from_millis(100)).await; |
There was a problem hiding this comment.
did you encounter this? assume this is specific to KMS so might be nice not to have it here but not critical
There was a problem hiding this comment.
No I just copied this from the existing logic, figured this was from that weird KMS thing too so didn't want to change
### Description Deploy to include #2836 ### Drive-by changes n/a ### Related issues n/a ### Backward compatibility yes ### Testing deployed
Description
finality_blocksinstead ofreorg_periodin validator #532 wasn't actually fully closed out - finality_blocks was used still for indexing. Sadly testnet4 infra wasn't configuring finality blocks anymore, so we weren't indexing only finalized blocks. Moved fully to reorg_periodcheckpoint_submitterin light of races revealed by the above ^ problem. It used to assume that message indexing and thelatest_checkpoint()call would align with one another, and wasn't resilient to the tree already being past the correctness checkpoint. A couple situations were possible before that aren't now:a. Indexing is ahead of the latest_checkpoint() call, which will result in tree ingesting the new indexed messages and the tree being ahead of the correctness checkpoint :(
b. It's possible for the tree() call that constructs the tree initially to be made against a block that's after the next latest_checkpoint() call, which would result in the tree being ahead of the correctness checkpoint from the very beginning :(
Drive-by changes
removed a function that wasn't being used anymore
Related issues
#532
Backward compatibility
Removes finality blocks entirely
Testing
Builds, e2e