-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: enforced batch mode #68
base: main
Are you sure you want to change the base?
Conversation
8612778
to
aee35b9
Compare
aee35b9
to
d851e8e
Compare
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.
LGTM!
with the new batch design this will need to change again but the main mechanism of
- track time of latest finalized batch
- if too much time elapsed allow anyone to commit batch + proof and disallow sequencer to submit via usual methods
- only allow security council to deactivate this enforced/permissionless mode
should stay the same and just the interface of what/how a batch needs to be passed should change.
@@ -46,6 +46,15 @@ library BatchHeaderV3Codec { | |||
} | |||
} | |||
|
|||
/// @notice Get the blob versioned hash of the batch header. |
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.
wrong comment
@@ -172,6 +197,11 @@ contract ScrollChain is OwnableUpgradeable, PausableUpgradeable, IScrollChain { | |||
_; | |||
} | |||
|
|||
modifier whenEnforcedBatchNotEnable() { |
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.
modifier whenEnforcedBatchNotEnable() { | |
modifier whenEnforcedBatchNotEnabled() { |
} | ||
} | ||
// initialize enforcedBatchParameters | ||
enforcedBatchParameters.maxDelayEnterEnforcedMode = _maxDelayEnterEnforcedMode; |
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.
Any way to update this?
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.
I assume it won't be updated when we set it. Sure I can add a method to update it.
* Structs * | ||
***********/ | ||
|
||
struct EnforcedBatchParameters { |
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.
Would this add +1 SSTORE cost for normal commit batch and finalize bundle txs?
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.
my guess is about 4600
gas.
// explicit set enforce batch enable | ||
parameters.enforcedModeEnabled = true; | ||
// reset `lastCommittedBatchIndex` | ||
parameters.lastCommittedBatchIndex = uint64(lastFinalizedBatchIndex); |
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.
I think we've discussed this before but I forgot the conclusion; doesn't this make "fast L2 finality" impossible? @jonastheis
In the future with the infallible chain derivation, we could just have a permissionless fallback for batch commitment and finalization (but not need to revert unfinalized batches).
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.
Overall looks good, but this will have a lot of conflicts with other changes planned for this upgrade. Do you want to still submit them as separate PRs and then merge them into a single unified PR that we share with the auditors?
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.
yes, I will create a branch and all related PR will merged into this branch.
No description provided.