-
Notifications
You must be signed in to change notification settings - Fork 24
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
Wait for a minimum amount of time since parent assertion was created to post a new assertion #714
base: main
Are you sure you want to change the base?
Conversation
…to post a new assertion
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.
Lookin nice! thanks for putting this together
Would just fix up the default for the time since parent and then can merge |
Need to think about how it affects overflow assertions
assertions/poster.go
Outdated
@@ -140,6 +139,18 @@ func (m *Manager) PostAssertionBasedOnParent( | |||
) (protocol.Assertion, error), | |||
) (option.Option[*protocol.AssertionCreatedInfo], error) { | |||
none := option.None[*protocol.AssertionCreatedInfo]() | |||
if m.times.minGapToParent != 0 { |
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.
This would indeed prevent us from posting overflow assertions immediately. The max number of blocks an assertion can consume is 2^26. If there are more than 2^26 blocks between assertions, first, an assertion that consumes 2^26 will be posted, then we should immediately post another assertion based on it that consumes the remaining messages. To know if the parent assertion overflowed, we can check: protocol.GoGlobalStateFromSolidity(parentCreationInfo.AfterState).PosInBatch > 0
, then we should allow posting its child immediately. We shouldn't wait in that case and should skip this conditional
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.
Sorry to bring this up so late. But, I don't think it's sufficient to check that the parentCreationInfo.AfterState PosInBatch > 0.
Imagine that instead of 2^26 the max challenge edge height were just 10.
Imagine you have three batches with 5 messages each when the previous assertion is created.
The previous assertion will say that you need to handle all three batches, but, you'll process the first 10 messages and be at the end of the second block. PosInBatch == 0 but in the 3rd batch. The one you haven't processed yet.
You're still in an overflow assertion because you should be able to immediately post the last 5 messages.
I think the better check is the one that the contracts make:
https://github.com/OffchainLabs/nitro-contracts/blob/bold-merge/src/rollup/RollupCore.sol#L481
Essentially, the machine needs to have reached the expected message from the previous parent assertion.
Earlier if assertion manager fails to create an assertion, it'd wait an hour before trying again. This PR fixes this by allowing immediate retries but inside the assertion creation we enable waiting for a minimum amount of time (configurable) since the parent assertion was created.
Corresponding nitro PR- OffchainLabs/nitro#2825
Part of NIT-2961