derive: iterator-style Deriver replacing batch-mode PureDerive#19303
derive: iterator-style Deriver replacing batch-mode PureDerive#19303sebastianst wants to merge 15 commits intodevelopfrom
Conversation
sebastianst
left a comment
There was a problem hiding this comment.
Also list all checks that this implementation skips compared to the legacy derivation pipeline.
op-core/pure/types.go
Outdated
| Hash common.Hash | ||
| Number uint64 | ||
| Timestamp uint64 | ||
| BaseFee *big.Int | ||
| BlobBaseFee *big.Int | ||
| ParentHash common.Hash | ||
| MixDigest common.Hash // prevrandao |
There was a problem hiding this comment.
Why did we add all header field individually to the L1Input instead of just adding a Header *types.Header field? I imagine that the caller will probably have the L1 header at hand.
op-core/pure/types.go
Outdated
|
|
||
| func (i *l1InputInfo) BlobGasUsed() *uint64 { return nil } | ||
|
|
||
| func (i *l1InputInfo) HeaderRLP() ([]byte, error) { |
There was a problem hiding this comment.
Is this function used?
op-core/pure/types.go
Outdated
| return rlp.EncodeToBytes(h) | ||
| } | ||
|
|
||
| func (i *l1InputInfo) Header() *types.Header { |
There was a problem hiding this comment.
Is this function used?
op-core/pure/batches.go
Outdated
|
|
||
| // validateBatch checks whether a singular batch is valid given the current | ||
| // derivation cursor and known L1 origins. | ||
| func validateBatch(batch *derive.SingularBatch, cursor l2Cursor, l1Origins []eth.L1BlockRef, cfg *rollup.Config) bool { |
There was a problem hiding this comment.
Ideally we reuse the existing batch validation functions from op-node/rollup/derive/batches.go. Since we have the no-overlap mod, expand the existing checks to be stricter about overlaps and reject them as invalid batches, if the current hardfork is Karst. The latest hardfork Karst is available on latest develop branch, which is already fetched.
op-core/pure/batches.go
Outdated
| ) ([]*derive.SingularBatch, error) { | ||
| spec := rollup.NewChainSpec(cfg) | ||
| maxRLP := spec.MaxRLPBytesPerChannel(cursor.Timestamp) | ||
| isFjord := cfg.IsFjord(cursor.Timestamp) |
There was a problem hiding this comment.
let's actually check IsKarst (which implies Fjord because forks are strictly ordered) and return an error here if it's not Karst because this pure DP only supports Karst because we reject overlapping span batches.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Refactor L1Input to embed *types.Header instead of individual fields - Replace l1InputInfo adapter with l1BlockInfoAdapter wrapping eth.HeaderBlockInfo - Add Karst fork gate: PureDerive requires Karst to be active - Add span batch overlap rejection under Karst - Move needsEmptyBatch to l2Cursor method - Optimize findL1Origin with map-based O(1) index - Return error instead of fallback when L1 block is missing - Validate l1Blocks start from safe head L1 origin - Add channelAssembler comment explaining design vs upstream - Add Jovian network upgrade transactions in attributes.go - Add comprehensive doc comment on PureDerive listing skipped checks - Add batch validation comments referencing upstream functions - Add JovianTime and KarstTime to test rollup config - Fix all lint issues (bigs.Uint64Strict, goimports) - Add new tests: RejectsPreKarst, ValidatesL1BlockRange, EmptyL1Blocks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ac0c820 to
73a2125
Compare
- Add full batch validation matching upstream checkSingularBatch: sequence window, epoch advancement (current/next only), timestamp >= L1 origin, max sequencer drift, fork activation blocks, and transaction validation (no empty txs, no deposits) - Remove Jovian NUTs (pre-Karst, can never be activation block) - Remove unused makeEmptyBatch - Remove isFjord variable (always true under Karst) - Simplify channel timeout integration test - Fix test L1 block times for realistic epoch advancement Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
559d2e9 to
223092d
Compare
- types.go: use eth.BlockRefFromHeader and eth.HeaderBlockID - batches.go: export CheckSpanBatchPrefix with Karst overlap rejection and nil l2Fetcher support, call from pure code - Remove inline span batch overlap check in favor of upstream Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove l1BlockInfoAdapter — pass the L1 chain config as an explicit dependency to PureDerive and thread it through to L1InfoDeposit, which needs it for BlobBaseFee computation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
op-core/pure/derive.go
Outdated
| // Validate that l1Blocks start from the safe head's L1 origin. | ||
| firstL1Num := bigs.Uint64Strict(l1Blocks[0].Header.Number) | ||
| if firstL1Num > safeHead.L1Origin.Number { | ||
| return nil, fmt.Errorf("l1Blocks start at %d but safe head L1 origin is %d", firstL1Num, safeHead.L1Origin.Number) | ||
| } |
There was a problem hiding this comment.
I realize we need to go back to the old check and require that the first l1 num is a channel timeout before the safe head origin
There was a problem hiding this comment.
Done — now requires l1Blocks to start at least ChannelTimeoutBedrock before the safe head's L1 origin. Uses underflow-safe subtraction.
op-core/pure/batches.go
Outdated
|
|
||
| validity, _ := derive.CheckSpanBatchPrefix( | ||
| context.Background(), cfg, | ||
| log.NewLogger(log.DiscardHandler()), |
There was a problem hiding this comment.
let's actually pass a logger to PureDerive and pass it around so we can log at key points during derivation. debug log for progress through the stages, warn log for invalid data.
There was a problem hiding this comment.
Done — PureDerive now takes a log.Logger and threads it through to decodeBatches and validateBatch. Debug logs for progress (channel ready), warn logs for issues (parse failures, invalid batches, timeouts).
op-core/pure/batches.go
Outdated
| if validity != derive.BatchAccept { | ||
| return nil, fmt.Errorf("span batch prefix check failed (validity=%d)", validity) | ||
| } |
There was a problem hiding this comment.
I think that's not correct. note that we need to collect batches and return all we got so far if we hit an invalid batch. then the channel has to be flushed. also compare this behavior to the reference impl.
also, if the validity is BatchPast the batch is dropped but we just continue with the next, not stop and flush the channel.
There was a problem hiding this comment.
Done — decodeBatches now handles batch validity correctly:
BatchPast→ skip the span batch, continue with next- Any other non-
BatchAccept→ return batches collected so far (channel is flushed)
In the derive loop, validateBatch failure now does break (flush remaining channel batches) instead of continue.
…h validity
Address review feedback:
- Pass log.Logger through PureDerive → decodeBatches → validateBatch
- Replace map-based L1 lookup with O(1) index arithmetic
- Require l1Blocks to start ChannelTimeoutBedrock before safe head origin
- decodeBatches no longer returns error; bad input is logged and returns
partial results
- Restructure EOF handling: if err == io.EOF { break } else if err != nil
- Span batch: BatchPast → skip, other non-Accept → return collected batches
- Invalid batch in derive loop → break (flush channel) instead of continue
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
op-core/pure/derive.go
Outdated
| // Require l1Blocks to start at least ChannelTimeoutBedrock before the safe | ||
| // head's L1 origin so that channels opened before the safe head are available. | ||
| requiredStart := safeHead.L1Origin.Number | ||
| if requiredStart > cfg.ChannelTimeoutBedrock { | ||
| requiredStart -= cfg.ChannelTimeoutBedrock |
There was a problem hiding this comment.
must not use the bedrock channel timeout, but the channel timeout of the L1 origin. construct a spec from the rollup.Config to do this, then call ChannelTimeout on it.
There was a problem hiding this comment.
Done. Now using spec.ChannelTimeout(safeHead.Time) for the initial L1 range check, where spec is constructed from rollup.NewChainSpec(cfg).
op-core/pure/derive.go
Outdated
| } | ||
| } | ||
|
|
||
| assembler.checkTimeout(l1Ref, cfg.ChannelTimeoutBedrock) |
There was a problem hiding this comment.
use correct channel timeout of current L1
There was a problem hiding this comment.
Done. Using spec.ChannelTimeout(l1Ref.Time) per L1 block in the main loop.
op-core/pure/derive.go
Outdated
| if !validateBatch(lgr, batch, cursor, l1Origins, cfg, l1Ref.Number) { | ||
| lgr.Warn("invalid batch, flushing channel", | ||
| "timestamp", batch.Timestamp, "epoch", batch.EpochNum, "l1_block", l1Ref.Number) | ||
| break | ||
| } |
There was a problem hiding this comment.
there needs to be a distinction between BatchPast and BatchDrop. For past batches, we'd continue because we skip over it, not break and flush the channel like for dropped batches.
also, I'm wondering if just breaking here really is the same as a channel flush? can't an l1 block contain multiple channels in theory? check this and adapt the implementation if needed.
There was a problem hiding this comment.
Good catch on both points.
BatchPast vs BatchDrop: Now properly distinguished. BatchPast → continue (skip batch, keep processing channel). Non-accept (including BatchDrop) → break (flush remaining batches from this channel).
Multiple channels per L1 block: The break only exits the inner for _, batch := range batches loop. The outer loops (for _, txData and for _, frame) continue processing other batcher transactions and frames in the same L1 block. So yes, an L1 block can contain multiple channels and they are each processed independently.
op-core/pure/derive.go
Outdated
| Timestamp: nextTimestamp, | ||
| } | ||
|
|
||
| epochL1 := findL1(newOrigin.Number) |
There was a problem hiding this comment.
isn't this the same as nextL1?
There was a problem hiding this comment.
Yes, it was redundant. Removed it — now we compute epochL1 once from cursor.L1Origin.Number and reassign to nextL1 when the epoch advances.
- Use spec.ChannelTimeout(l1Ref.Time) instead of cfg.ChannelTimeoutBedrock for both the initial L1 range check and per-block timeout check. - Distinguish BatchPast (continue) from BatchDrop (break/flush channel) in the main derivation loop. - Add Jovian and Interop fork activation block checks alongside Karst. - Add SetCode (EIP-7702) transaction rejection before Isthmus. - validateBatch returns derive.BatchValidity instead of bool. - Remove redundant findL1 call in empty batch epoch advancement. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Documents the objective, architecture, and a check-by-check comparison between validateBatch and upstream checkSingularBatch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pass l1Ref (where the channel completed) instead of ready.openBlock (where the channel opened) to decodeBatches. The inclusion block is used by CheckSpanBatchPrefix for the sequence window check and must be the L1 block where the batch became available for derivation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduces a Deriver iterator that produces one PayloadAttributes at a time from incrementally-added L1 blocks, replacing the batch-mode PureDerive function. Key changes: - NewDeriver/AddL1Block/Next/Reset API for incremental derivation - Upstream CheckBatch for batch validation (including parent hash checks) - ErrNeedL1Data/ErrReorg sentinels for iterator control flow - Empty batch generation extracted as pure makeEmptyBatch function - Package renamed from op-core/pure to op-core/derive (upstream aliased as opderive) - Removed PureDerive, validateBatch, and DerivedBlock Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Deriveriterator that produces onePayloadAttributesat a time from incrementally-added L1 blocks, replacing the batch-modePureDerivefunctionCheckBatchfor batch validation including parent hash checks, removing the customvalidateBatchErrNeedL1Data/ErrReorgsentinels, empty batch generation as puremakeEmptyBatch, andResetfor L1 reorg handlingop-core/puretoop-core/derive(upstream aliased asopderive)API
Test plan
go test ./op-core/derive/... -v -count=1)go test ./op-node/rollup/derive/... -count=1)go build ./op-core/derive/...)