Guardian now produces rollup based on interval time and available bat… #1536
Conversation
There was a problem hiding this comment.
logic looks good.
A couple of small cleanup things .
@BedrockSquirrel , could you have a look as well?
go/host/enclave/guardian.go
Outdated
| @@ -608,3 +646,40 @@ func (g *Guardian) streamEnclaveData() { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func (g *Guardian) getBatchesAfterSize(seqNo uint64) (uint64, error) { | |||
There was a problem hiding this comment.
Find this method name a bit confusing. Maybe could be something like getSizeOfBatchesAfter() (maybe since instead of after)
There was a problem hiding this comment.
Yeah, I found that confusing as well
There was a problem hiding this comment.
Third here, hate the name. WDYT ofcalculateUnrolledupBatchesSize ?
| if !g.state.IsUpToDate() { | ||
| // if we're behind the L1, we don't want to produce rollups | ||
| g.logger.Debug("skipping rollup production because L1 is not up to date", "state", g.state) | ||
| continue | ||
| } | ||
|
|
||
| fromBatch, err := g.getLastestBatchNo() | ||
| fromBatch, err := g.getLatestBatchNo() |
There was a problem hiding this comment.
I think we should have another skip here that's like:
if time.Since(lastSuccessfulRollup) < minRollupInterval {
continue // only just produced a rollup, allow time for enclave to consume
}
where minRollupInterval can probably just be like 1*blocktime (it's mostly just incase the timer fires soon after the previous run, not sure how the timer works and if that's possible but I assume it is)
There was a problem hiding this comment.
Yeah, I like to have a minRollupInterval as a performance improvement, perhaps at a later stage?
Mainly due to the not being clear what the minRollupInterval should be.
Should be ok to not have this check for now because:
- This is an awaited method. Once the rollup is created, it will wait for the receipt, which guarantees one block spacing.
- This whole process then is called after the 1*l1blocktime duration which is AFTER the success tx mint. So after the await of the receipt (step above) it waits again another block to check if it should rollup.
Would it make sense to add a few timers/logs so that it's possible to monitor this behavior? (for future improvements?)
There was a problem hiding this comment.
Could be slightly confusing when the timer fires straightaway but shouldn't be a problem, like you say probably just optimisation at worst.
There was a problem hiding this comment.
Yeah as discussed on discord the race condition I was worried about isn't an issue any more because you're using the L1 data directly to decide how far we have rolled up. Disregard this!
…ches size
Why this change is needed
https://github.com/obscuronet/obscuro-internal/issues/2153
availableBatchesSize>=maxRollupSizeseqNomaxRollupSizeas a flagWhat changes were made as part of this PR
Please provide a high level list of the changes made
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks