-
Notifications
You must be signed in to change notification settings - Fork 9
Emergency commit #353
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
base: compatible
Are you sure you want to change the base?
Emergency commit #353
Conversation
d60163f to
86a0934
Compare
86a0934 to
548fb07
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces an important emergency commit mechanism to handle sequencer downtime, which is a crucial feature for the network's liveness. The implementation is well-structured, especially the refactoring in rule_commit.ml to accommodate the new emergency logic while reusing existing components. The addition of the count_commits folder and the Verify_emergency_folders wrapper to handle multiple recursive proofs is a good use of the existing patterns. The documentation and spec have also been updated to reflect these changes.
I've found a few minor issues, mostly in the specification document regarding pseudocode correctness and clarity, and a suggestion to improve configuration readability by avoiding a magic number. Overall, this is a solid contribution.
Until we have decentralized sequencing, we need some kind of mechanism to do emergency commit in case the sequencer goes offline. This PR implements the simple commit in case there has been no commits for some period of time.
The original commit rule stays the same.
The new emergency commit rule reuses the original rule, and checks that there has been no commit for max_sequencer_inactivity slots.
The main drawback of this approach is that the outer action state precondition can be set to the last 5 values, therefore the sequencer has to maintain commits in 5 distinct slots in the max_sequencer_inactivity window.
More detailed explanation is in the spec and rollup explanation doc.
Follwing is left to do: