feat(derivation): batch verification, L1 reorg detection, and rollback-based recovery#907
feat(derivation): batch verification, L1 reorg detection, and rollback-based recovery#907
Conversation
Replace the challenge mechanism with a batch-data-as-source-of-truth model: - When local L2 blocks don't match L1 batch data, rollback and re-derive from L1 instead of issuing a state challenge - Add L1 reorg detection for non-finalized confirmation modes (latest/safe) by tracking L1 block hashes and comparing on each derivation loop - L1 reorg only triggers DB cleanup and re-derivation; L2 rollback is only triggered when batch data comparison actually fails Key changes: - Remove validator/challenge dependency from derivation - Add verifyBlockContext() and verifyBatchRoots() for batch data comparison - Add detectReorg() with configurable check depth (default 64 blocks) - Add rollbackLocalChain() stub (TODO: geth SetHead API integration) - Add L1 block hash tracking in DB for reorg detection - Add metrics: l1_reorg_detected_total, l2_rollback_total, block_mismatch_total - Add --derivation.reorgCheckDepth CLI flag Made-with: Cursor
Bug fixes: - Fix detectReorg traversal direction: iterate oldest-to-newest to find the earliest divergence point, not the latest - Make rollbackLocalChain return error instead of nil to prevent silent fall-through to NewSafeL2Block on an already-existing block number - Handle edge case in handleL1Reorg when reorgAtL1Height <= startHeight Optimizations: - Skip recordL1Blocks in finalized mode (reorg detection is disabled, recording L1 hashes is unnecessary overhead) Cleanup: - Remove unused BatchIndex/L2EndBlock fields from DerivationL1Block - Add batch-internal tx count consistency check in verifyBlockContext - Use Info instead of Error for L1 reorg detection logs (expected in latest mode) - Update DERIVATION_REFACTOR.md with review feedback changes Made-with: Cursor
- Handle startHeight==0 edge case in handleL1Reorg by writing 0 instead of skipping the reset - Change recordL1Blocks to return on first failure instead of continue, preventing gaps in L1 block hash tracking that could cause missed reorgs - Return immediately after L1 reorg handling instead of continuing the same derivation loop, avoiding recording unstable L1 hashes during ongoing reorgs - Clarify verifyBlockContext tx count check is batch-internal consistency, not local-vs-L1 comparison (local-vs-L1 covered by state root in verifyBatchRoots) Made-with: Cursor
…cording fails recordL1Blocks now returns error. If any L1 header fetch fails mid-range, derivationBlock returns early without calling WriteLatestDerivationL1Height. This prevents permanent gaps in L1 block hash tracking that would make reorgs in the gap range undetectable. Made-with: Cursor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves validator/challenge wiring from derivation initialization; adds persistent L1 block tracking and reorg detection with CRUD DB operations; introduces L1-batch vs L2 verification and rollback scaffolding, ReorgCheckDepth config/flag, and new derivation metrics. Changes
Sequence Diagram(s)sequenceDiagram
participant Derivation as Derivation Loop
participant DB as Store (DB)
participant L1 as L1 Chain
participant Metrics as Metrics
Derivation->>Derivation: Start derivation cycle
alt Non-finalized mode: check for L1 reorg
Derivation->>DB: ReadDerivationL1BlockRange / detectReorg()
DB-->>Derivation: stored L1 headers
Derivation->>L1: Fetch current L1 headers
L1-->>Derivation: current L1 headers
Derivation->>Derivation: compare stored vs current hashes
alt Reorg detected
Derivation->>Metrics: IncReorgCount()
Derivation->>DB: DeleteDerivationL1BlocksFrom(height)
DB-->>Derivation: cleaned state
Derivation->>Derivation: reset derivation pointer, exit cycle
end
end
Derivation->>L1: Fetch CommitBatch events / batch data
L1-->>Derivation: batch events
loop For each batch
Derivation->>Derivation: verifyBlockContext(header, batchData)
alt Context mismatch
Derivation->>Metrics: IncBlockMismatchCount()
end
Derivation->>Derivation: verifyBatchRoots(batchInfo, lastHeader)
alt Root mismatch
Derivation->>Metrics: IncRollbackCount()
Derivation->>Derivation: rollbackLocalChain(target) [stub]
Derivation->>Derivation: re-derive / halt on failure
end
end
Derivation->>DB: WriteDerivationL1Block(...) (recordL1Blocks)
DB-->>Derivation: stored
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
node/cmd/node/main.go (1)
76-110:⚠️ Potential issue | 🟡 MinorRemove the orphaned
node/validatorpackage.The validator package is completely unused. No code in the repository imports or instantiates
validator.NewValidator()or usesvalidator.Config. Thenode/cmd/node/main.gochecks the--validatorflag but never calls the validator package. The entirenode/validatordirectory (config.go, validator.go, validator_test.go) is dead code and should be deleted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/cmd/node/main.go` around lines 76 - 110, The validator package is unused; delete the orphaned package (all files defining validator.Config, validator.NewValidator, and validator_test.go) and remove any vestigial references in the CLI (the --validator flag, the isValidator variable and the conditional block in main.go that checks it) and clean up imports and unused vars; after removing the files, run go mod tidy/build to ensure no remaining references to validator.* exist.
🧹 Nitpick comments (3)
node/db/store.go (1)
190-199: Consider logging or returning gap information inReadDerivationL1BlockRange.The function silently skips
nilblocks when iterating, which could mask gaps in the L1 block data. If gaps exist (e.g., due to switching between finalized and non-finalized modes), callers have no visibility into missing entries.Depending on how this is used, you may want to either return a map keyed by height or log when gaps are detected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/db/store.go` around lines 190 - 199, ReadDerivationL1BlockRange currently drops nil results silently which hides gaps in L1 data; update it to either (a) return a structure that preserves missing heights (e.g., map[uint64]*DerivationL1Block or a slice of structs containing height+block) or (b) log detected gaps as you iterate; locate the function ReadDerivationL1BlockRange and the call to s.ReadDerivationL1Block(h) and implement one of these approaches so callers can detect missing heights (include the height in logs or as keys in the returned map).node/derivation/DERIVATION_REFACTOR.md (1)
53-67: Consider adding language specifiers to fenced code blocks.The ASCII flowcharts lack language specifiers, which triggers markdown linting warnings. While this is minor, adding
textorplaintextas the language identifier would satisfy linters and improve rendering consistency.📝 Proposed fix
-``` +```text derivationBlock() loop start │🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/derivation/DERIVATION_REFACTOR.md` around lines 53 - 67, The fenced ASCII flowchart in DERIVATION_REFACTOR.md should include a language specifier to satisfy markdown linters and improve rendering; update the triple-backtick fences around the block that documents derivationBlock(), detectReorg(), handleL1Reorg(), DeleteDerivationL1BlocksFrom, WriteLatestDerivationL1Height, derive(), verifyBatchRoots(), and recordL1Blocks to use ```text (or ```plaintext) instead of bare ``` so the entire diagram is treated as plain text.node/derivation/derivation.go (1)
369-369: Consider a helper to wrap HeaderByNumber with nil-safety.The
HeaderByNumbernil-check issue appears in three places:detectReorg(line 369),recordL1Blocks(line 501), andderive(line 796). A small helper would reduce duplication and ensure consistent handling.♻️ Suggested helper function
// safeHeaderByNumber fetches an L1 header and returns an error if nil. func (d *Derivation) safeL1HeaderByNumber(ctx context.Context, number uint64) (*eth.Header, error) { header, err := d.l1Client.HeaderByNumber(ctx, big.NewInt(int64(number))) if err != nil { return nil, fmt.Errorf("failed to get L1 header at %d: %w", number, err) } if header == nil { return nil, fmt.Errorf("L1 header at %d not found", number) } return header, nil }Then use it in
detectReorg,recordL1Blocks, and similar locations. For the L2 client call inderive, a similar helper or inline check would be appropriate.Also applies to: 501-501, 796-796
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/derivation/derivation.go` at line 369, Introduce a small helper on Derivation (e.g., safeL1HeaderByNumber(ctx, number uint64) (*eth.Header, error)) that wraps d.l1Client.HeaderByNumber, checks for errors and nil header, and returns a descriptive error if nil; replace direct calls to d.l1Client.HeaderByNumber in detectReorg, recordL1Blocks, and any other sites (and add a similar safe helper or nil-check for the L2 client usage in derive) to eliminate duplicated nil-safety logic and ensure consistent error messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@node/db/store.go`:
- Around line 201-216: DeleteDerivationL1BlocksFrom currently stops at the first
missing key which can leave later L1 block records orphaned when gaps exist;
change the deletion strategy in DeleteDerivationL1BlocksFrom to perform a
bounded scan instead of breaking on the first missing key (e.g., iterate from
height up to height + reorgCheckDepth or another configured bound), attempt to
delete each existing key within that range using the same batch.Delete and then
write the batch as before; reference the DeleteDerivationL1BlocksFrom function
and use the reorgCheckDepth (or a new configurable bound) to limit the scan so
gaps do not prematurely terminate deletion.
In `@node/derivation/config.go`:
- Around line 119-121: The ReorgCheckDepth field (set via
ctx.GlobalUint64(flags.DerivationReorgCheckDepth.Name) into c.ReorgCheckDepth)
lacks the same non‑zero validation used for PollInterval and FetchBlockRange;
after the block that sets c.ReorgCheckDepth add a validation that returns an
error when c.ReorgCheckDepth == 0 (e.g. return errors.New("invalid
reorgCheckDepth")) to keep behavior consistent, or alternatively add a clear
comment/docstring on the Derivation config and/or the ReorgCheckDepth field
explaining that zero is intentionally allowed to disable historical reorg
checking if you choose to permit zero instead of rejecting it.
In `@node/derivation/derivation.go`:
- Around line 499-504: In recordL1Blocks, HeaderByNumber may return (nil, nil)
causing header.Hash() to panic; update the loop in recordL1Blocks to check for a
nil header after the call to d.l1Client.HeaderByNumber (same pattern used in
detectReorg) and handle it gracefully—return a descriptive error (or
skip/continue as appropriate) when header == nil before calling header.Hash(),
ensuring you reference d.l1Client.HeaderByNumber and the header variable in your
change.
- Around line 796-817: The call to d.l2Client.HeaderByNumber may return (nil,
nil) so add an explicit nil-check after the HeaderByNumber call (the symbol to
locate is the localHeader, err := d.l2Client.HeaderByNumber(...) line) and
return a clear error (or handle it the same way as detectReorg/recordL1Blocks)
when localHeader == nil to avoid a nil pointer dereference before calling
d.verifyBlockContext; also update the misleading comment that says "Fall through
to NewSafeL2Block below to re-execute" (near the rollbackLocalChain(...) call)
to reflect actual control flow — remove or change it to indicate that
rollbackLocalChain returns on error and that code will continue to the
NewSafeL2Block path only when rollback succeeds.
- Around line 486-492: blockData.SafeL2Data is guaranteed allocated during batch
parsing, so remove the inconsistent nil guard around the tx-count check: delete
the surrounding "if blockData.SafeL2Data != nil { ... }" and always compare
len(blockData.SafeL2Data.Transactions) with blockData.txsNum, keeping the
existing fmt.Errorf using blockData.Number, blockData.txsNum and the computed
batchTxCount; this makes behavior consistent with the other direct uses of
SafeL2Data at references where SafeL2Data.Transactions is accessed (see usages
around the code that currently access SafeL2Data at the other locations).
- Around line 401-410: The code writes 0 to the DB when a reorg happens at or
before genesis, causing ReadLatestDerivationL1Height()/latestDerivation to
return &0 and skip block 0; change the reorg handling in derivation.go so that
when d.startHeight == 0 you delete the stored latest derivation height instead
of writing 0: add DeleteLatestDerivationL1Height() to the DB interface and store
implementation, call d.db.DeleteLatestDerivationL1Height() in the else branch
where you currently call WriteLatestDerivationL1Height(0), and keep existing
behavior of calling WriteLatestDerivationL1Height(d.startHeight - 1) when
d.startHeight > 0 so subsequent derivationBlock() logic (which uses
ReadLatestDerivationL1Height() to compute start) will default to d.startHeight.
---
Outside diff comments:
In `@node/cmd/node/main.go`:
- Around line 76-110: The validator package is unused; delete the orphaned
package (all files defining validator.Config, validator.NewValidator, and
validator_test.go) and remove any vestigial references in the CLI (the
--validator flag, the isValidator variable and the conditional block in main.go
that checks it) and clean up imports and unused vars; after removing the files,
run go mod tidy/build to ensure no remaining references to validator.* exist.
---
Nitpick comments:
In `@node/db/store.go`:
- Around line 190-199: ReadDerivationL1BlockRange currently drops nil results
silently which hides gaps in L1 data; update it to either (a) return a structure
that preserves missing heights (e.g., map[uint64]*DerivationL1Block or a slice
of structs containing height+block) or (b) log detected gaps as you iterate;
locate the function ReadDerivationL1BlockRange and the call to
s.ReadDerivationL1Block(h) and implement one of these approaches so callers can
detect missing heights (include the height in logs or as keys in the returned
map).
In `@node/derivation/DERIVATION_REFACTOR.md`:
- Around line 53-67: The fenced ASCII flowchart in DERIVATION_REFACTOR.md should
include a language specifier to satisfy markdown linters and improve rendering;
update the triple-backtick fences around the block that documents
derivationBlock(), detectReorg(), handleL1Reorg(), DeleteDerivationL1BlocksFrom,
WriteLatestDerivationL1Height, derive(), verifyBatchRoots(), and recordL1Blocks
to use ```text (or ```plaintext) instead of bare ``` so the entire diagram is
treated as plain text.
In `@node/derivation/derivation.go`:
- Line 369: Introduce a small helper on Derivation (e.g.,
safeL1HeaderByNumber(ctx, number uint64) (*eth.Header, error)) that wraps
d.l1Client.HeaderByNumber, checks for errors and nil header, and returns a
descriptive error if nil; replace direct calls to d.l1Client.HeaderByNumber in
detectReorg, recordL1Blocks, and any other sites (and add a similar safe helper
or nil-check for the L2 client usage in derive) to eliminate duplicated
nil-safety logic and ensure consistent error messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00c37ccd-184c-4341-8064-a88ceb8e8935
📒 Files selected for processing (9)
node/cmd/node/main.gonode/db/keys.gonode/db/store.gonode/derivation/DERIVATION_REFACTOR.mdnode/derivation/config.gonode/derivation/database.gonode/derivation/derivation.gonode/derivation/metrics.gonode/flags/flags.go
| func (s *Store) DeleteDerivationL1BlocksFrom(height uint64) { | ||
| batch := s.db.NewBatch() | ||
| for h := height; ; h++ { | ||
| key := DerivationL1BlockKey(h) | ||
| has, err := s.db.Has(key) | ||
| if err != nil || !has { | ||
| break | ||
| } | ||
| if err := batch.Delete(key); err != nil { | ||
| panic(fmt.Sprintf("failed to delete DerivationL1Block at %d, err: %v", h, err)) | ||
| } | ||
| } | ||
| if err := batch.Write(); err != nil { | ||
| panic(fmt.Sprintf("failed to write batch delete for DerivationL1Blocks, err: %v", err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
DeleteDerivationL1BlocksFrom may leave orphaned records if gaps exist.
The loop breaks at the first missing key (line 206-207), assuming contiguous data. However, per context snippet 2, recordL1Blocks is only called in non-finalized mode. If the node switches between finalized and non-finalized modes, gaps can form in the recorded L1 blocks, leaving records after the gap undeleted.
Consider whether this is acceptable or if you need a bounded scan (e.g., up to height + reorgCheckDepth) to handle potential gaps.
🔧 Potential approach for bounded deletion
-func (s *Store) DeleteDerivationL1BlocksFrom(height uint64) {
+func (s *Store) DeleteDerivationL1BlocksFrom(height uint64, maxLookahead uint64) {
batch := s.db.NewBatch()
- for h := height; ; h++ {
+ upperBound := height + maxLookahead
+ for h := height; h <= upperBound; h++ {
key := DerivationL1BlockKey(h)
has, err := s.db.Has(key)
- if err != nil || !has {
- break
- }
- if err := batch.Delete(key); err != nil {
+ if err != nil {
+ break // DB error, stop
+ }
+ if !has {
+ continue // Skip gaps
+ }
+ if err := batch.Delete(key); err != nil {
panic(fmt.Sprintf("failed to delete DerivationL1Block at %d, err: %v", h, err))
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@node/db/store.go` around lines 201 - 216, DeleteDerivationL1BlocksFrom
currently stops at the first missing key which can leave later L1 block records
orphaned when gaps exist; change the deletion strategy in
DeleteDerivationL1BlocksFrom to perform a bounded scan instead of breaking on
the first missing key (e.g., iterate from height up to height + reorgCheckDepth
or another configured bound), attempt to delete each existing key within that
range using the same batch.Delete and then write the batch as before; reference
the DeleteDerivationL1BlocksFrom function and use the reorgCheckDepth (or a new
configurable bound) to limit the scan so gaps do not prematurely terminate
deletion.
There was a problem hiding this comment.
The orphaned records are harmless in practice:
- After
handleL1Reorg(150)resetslatestDerivationL1Heightto 149,detectReorgonly scans[checkFrom, 149]— the orphans at 301-400 are never read. - When derivation catches up and
recordL1Blocksreaches 301-400,WriteDerivationL1Blockoverwrites them viadb.Put. - In the interim they waste a few KB of DB space with no functional impact.
Adding a maxLookahead parameter to handle this edge case introduces interface complexity that outweighs the benefit. No change needed.
There was a problem hiding this comment.
@curryxbo, that's a clear and well-reasoned explanation. The combination of bounded scan scope in detectReorg, idempotent overwrite semantics from db.Put in recordL1Blocks, and the negligible interim storage cost all confirm that the gap-stop behavior is safe. Agreed — no change needed.
node/derivation/derivation.go
Outdated
| if reorgAtL1Height > d.startHeight { | ||
| d.db.WriteLatestDerivationL1Height(reorgAtL1Height - 1) | ||
| } else { | ||
| // Reorg at or before startHeight — reset so next loop starts from startHeight. | ||
| if d.startHeight > 0 { | ||
| d.db.WriteLatestDerivationL1Height(d.startHeight - 1) | ||
| } else { | ||
| d.db.WriteLatestDerivationL1Height(0) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if DeleteLatestDerivationL1Height exists or how the DB handles 0 vs nil
ast-grep --pattern 'func ($_ *$_) ReadLatestDerivationL1Height() $_ {
$$$
}'
# Also check how nil is distinguished from 0 in the DB layer
rg -n "ReadLatestDerivationL1Height" --type go -A 5Repository: morph-l2/morph
Length of output: 2363
🏁 Script executed:
# Find the derivationBlock function to see the exact logic for handling latestDerivation
rg -n "func.*derivationBlock" --type go -A 30 node/derivation/derivation.go | head -80
# Check for DeleteLatestDerivationL1Height
rg -n "DeleteLatestDerivationL1Height" --type goRepository: morph-l2/morph
Length of output: 1309
🏁 Script executed:
# Read the specific section where start is determined based on latestDerivation
sed -n '245,260p' node/derivation/derivation.go
# Also check WriteLatestDerivationL1Height to see if there's a delete variant
rg -n "WriteLatestDerivationL1Height\|DeleteLatestDerivationL1Height" --type go -B 2 -A 5Repository: morph-l2/morph
Length of output: 507
🏁 Script executed:
# Look for delete patterns in the DB or similar reset methods
rg -n "func.*Delete\|func.*Clear\|func.*Reset" node/db/store.go -A 3
# Check if there are other WriteLatest methods that handle nil/empty cases
rg -n "WriteLatest" --type go node/db/store.go -A 5Repository: morph-l2/morph
Length of output: 584
🏁 Script executed:
# Look at the complete handleL1Reorg function to understand the full context
sed -n '386,413p' node/derivation/derivation.goRepository: morph-l2/morph
Length of output: 1249
Edge case: Writing height 0 causes block 0 to be skipped after reorg at genesis.
When startHeight == 0 and reorg occurs at or before genesis, line 408 writes 0 to the DB. On the next derivationBlock call:
- Line 239:
latestDerivationreturns&0(pointer to 0, not nil) - Line 247:
start = *latestDerivation + 1 = 1 - Block 0 is skipped despite the intent to reset to
startHeight
When startHeight > 0, the logic works correctly because WriteLatestDerivationL1Height(startHeight - 1) allows the next loop to compute start = (startHeight - 1) + 1 = startHeight. However, for startHeight == 0, writing 0 produces an off-by-one error.
The proper fix is to delete the DB key instead, so ReadLatestDerivationL1Height() returns nil and the next loop defaults to start = d.startHeight:
Suggested fix
if reorgAtL1Height > d.startHeight {
d.db.WriteLatestDerivationL1Height(reorgAtL1Height - 1)
} else {
- // Reorg at or before startHeight — reset so next loop starts from startHeight.
- if d.startHeight > 0 {
- d.db.WriteLatestDerivationL1Height(d.startHeight - 1)
- } else {
- d.db.WriteLatestDerivationL1Height(0)
- }
+ // Reorg at or before startHeight — delete the key so next loop starts from startHeight.
+ d.db.DeleteLatestDerivationL1Height()
}This requires adding DeleteLatestDerivationL1Height() to the database interface and implementing it in the store.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@node/derivation/derivation.go` around lines 401 - 410, The code writes 0 to
the DB when a reorg happens at or before genesis, causing
ReadLatestDerivationL1Height()/latestDerivation to return &0 and skip block 0;
change the reorg handling in derivation.go so that when d.startHeight == 0 you
delete the stored latest derivation height instead of writing 0: add
DeleteLatestDerivationL1Height() to the DB interface and store implementation,
call d.db.DeleteLatestDerivationL1Height() in the else branch where you
currently call WriteLatestDerivationL1Height(0), and keep existing behavior of
calling WriteLatestDerivationL1Height(d.startHeight - 1) when d.startHeight > 0
so subsequent derivationBlock() logic (which uses ReadLatestDerivationL1Height()
to compute start) will default to d.startHeight.
There was a problem hiding this comment.
firstBlockNumber is parsed from L1 Rollup contract's CommitBatch calldata — the contract rejects batches starting at block 0. Even hypothetically, the underflowed value would be passed to rollbackLocalChain which returns an error (currently a stub), triggering halt. No silent corruption is possible. Not worth guarding against an impossible upstream state.
There was a problem hiding this comment.
@curryxbo, understood — thanks for the context. Since the L1 Rollup contract rejects batches starting at block 0, startHeight is always > 0 in practice, making the d.startHeight == 0 branch in handleL1Reorg effectively dead code. And even in a hypothetical scenario, the stub rollbackLocalChain would return an error and trigger halt, preventing any silent corruption. That's a reasonable justification for leaving it as-is.
✏️ Learnings added
Learnt from: curryxbo
Repo: morph-l2/morph PR: 907
File: node/derivation/derivation.go:435-444
Timestamp: 2026-03-10T14:34:34.008Z
Learning: In `node/derivation/derivation.go` (morph-l2/morph), `startHeight` is always > 0 in practice because the L1 Rollup contract rejects CommitBatch transactions starting at block 0. The `else` branch in `handleL1Reorg` that handles `startHeight == 0` is therefore dead code and does not need a special-case guard. Do not flag the `WriteLatestDerivationL1Height(0)` call in that branch as an off-by-one edge case.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: curryxbo
Repo: morph-l2/morph PR: 907
File: node/derivation/config.go:119-121
Timestamp: 2026-03-10T14:16:02.526Z
Learning: In `node/derivation/config.go` (morph-l2/morph), `ReorgCheckDepth == 0` is intentionally valid and not a bug. When set to zero, `detectReorg` computes `checkFrom` equal to `*latestDerivation`, `ReadDerivationL1BlockRange` returns an empty slice, and the function returns nil — effectively disabling historical L1 reorg checking with no errors or side effects. Do not flag missing zero-rejection validation for this field.
…reorg check, fix baseFee nil handling
1. Add `halted` flag: when rollback stub fails on batch mismatch, derivation
stops instead of infinitely retrying the same batch with wasted L1 RPCs.
2. Optimize detectReorg: check newest saved block first — if it matches,
skip the full scan (1 RPC instead of 64 in the common no-reorg case).
3. Fix verifyBlockContext BaseFee: explicitly error when one side is nil
and the other is not, instead of silently skipping the comparison.
4. Fix doc: DerivationL1Block field list now matches code ({Number, Hash}).
Made-with: Cursor
Expose morphnode_derivation_halted gauge (0/1) so operators can set up alerts when derivation halts due to unrecoverable batch mismatch. All three code paths that set d.halted=true now also call metrics.SetHalted(). Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
node/derivation/DERIVATION_REFACTOR.md (1)
53-67: Add language specifier to fenced code blocks for lint compliance.Static analysis flags the flow diagram code blocks at lines 53, 83, and 98 as missing language specifiers. Using
textorplaintextsatisfies linters while preserving readability.📝 Proposed fix for line 53
-``` +```text derivationBlock() loop start │Apply the same pattern to the other flow diagram blocks at lines 83 and 98.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/derivation/DERIVATION_REFACTOR.md` around lines 53 - 67, The fenced flow-diagram code blocks in DERIVATION_REFACTOR.md (the one that documents derivationBlock() and the other two blocks) are missing a language specifier; update each triple-backtick fence for those diagrams to include a plain text specifier (e.g., ```text or ```plaintext) so the linter stops flagging them—specifically change the fences surrounding the derivationBlock() diagram and the other two flow diagrams referenced in the file to use ```text (or ```plaintext) while leaving the diagram content unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@node/derivation/derivation.go`:
- Around line 315-321: The code computes rollbackTarget :=
batchInfo.firstBlockNumber - 1 which underflows when batchInfo.firstBlockNumber
== 0; before calling d.rollbackLocalChain validate batchInfo.firstBlockNumber >
0 (or handle zero explicitly) and bail with a clear error/log and set d.halted
if necessary; update the block around rollbackTarget, d.rollbackLocalChain call,
and the d.logger.Error path to handle the zero case (e.g., log "cannot rollback
from block 0" with batchIndex and avoid computing rollbackTarget) so you never
pass a wrapped uint64 to d.rollbackLocalChain.
- Around line 837-841: The code computes rollbackTarget :=
blockData.SafeL2Data.Number - 1 which will underflow when SafeL2Data.Number ==
0; add a guard before computing rollbackTarget in the same block (in
derivation.go near where rollbackTarget and d.rollbackLocalChain are used) that
checks if blockData.SafeL2Data.Number == 0 and, if so, set d.halted = true and
return a clear error (e.g., "cannot rollback: SafeL2Data.Number is 0") instead
of performing the subtraction; otherwise compute rollbackTarget = number-1 and
call d.rollbackLocalChain as before.
- Around line 380-398: The code calls d.l1Client.HeaderByNumber and immediately
uses newestHeader.Hash() / header.Hash() which can panic if HeaderByNumber
returns (nil, nil); update the fast path (where newestHeader is set from
HeaderByNumber) and the slow path loop (where header is set) to check for nil
returns and handle them (e.g., return a descriptive error like "L1 header not
found at %d" wrapped with %w or treat as a reorg) before calling .Hash();
reference the symbols savedBlocks, newest, newestHeader, header, and the
d.l1Client.HeaderByNumber calls to locate and modify the checks.
---
Nitpick comments:
In `@node/derivation/DERIVATION_REFACTOR.md`:
- Around line 53-67: The fenced flow-diagram code blocks in
DERIVATION_REFACTOR.md (the one that documents derivationBlock() and the other
two blocks) are missing a language specifier; update each triple-backtick fence
for those diagrams to include a plain text specifier (e.g., ```text or
```plaintext) so the linter stops flagging them—specifically change the fences
surrounding the derivationBlock() diagram and the other two flow diagrams
referenced in the file to use ```text (or ```plaintext) while leaving the
diagram content unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10026e22-da23-4c79-b5d4-b26740384916
📒 Files selected for processing (2)
node/derivation/DERIVATION_REFACTOR.mdnode/derivation/derivation.go
| rollbackTarget := blockData.SafeL2Data.Number - 1 | ||
| if err := d.rollbackLocalChain(rollbackTarget); err != nil { | ||
| d.halted = true | ||
| return nil, fmt.Errorf("rollback to %d failed (derivation halted): %v", rollbackTarget, err) | ||
| } |
There was a problem hiding this comment.
Potential underflow if SafeL2Data.Number is 0.
Same issue as in derivationBlock: if blockData.SafeL2Data.Number is 0, rollbackTarget underflows to max uint64.
🛡️ Proposed fix
+ if blockData.SafeL2Data.Number == 0 {
+ d.halted = true
+ return nil, fmt.Errorf("cannot rollback before genesis block at block %d", blockData.Number)
+ }
rollbackTarget := blockData.SafeL2Data.Number - 1
if err := d.rollbackLocalChain(rollbackTarget); err != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rollbackTarget := blockData.SafeL2Data.Number - 1 | |
| if err := d.rollbackLocalChain(rollbackTarget); err != nil { | |
| d.halted = true | |
| return nil, fmt.Errorf("rollback to %d failed (derivation halted): %v", rollbackTarget, err) | |
| } | |
| if blockData.SafeL2Data.Number == 0 { | |
| d.halted = true | |
| return nil, fmt.Errorf("cannot rollback before genesis block at block %d", blockData.Number) | |
| } | |
| rollbackTarget := blockData.SafeL2Data.Number - 1 | |
| if err := d.rollbackLocalChain(rollbackTarget); err != nil { | |
| d.halted = true | |
| return nil, fmt.Errorf("rollback to %d failed (derivation halted): %v", rollbackTarget, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@node/derivation/derivation.go` around lines 837 - 841, The code computes
rollbackTarget := blockData.SafeL2Data.Number - 1 which will underflow when
SafeL2Data.Number == 0; add a guard before computing rollbackTarget in the same
block (in derivation.go near where rollbackTarget and d.rollbackLocalChain are
used) that checks if blockData.SafeL2Data.Number == 0 and, if so, set d.halted =
true and return a clear error (e.g., "cannot rollback: SafeL2Data.Number is 0")
instead of performing the subtraction; otherwise compute rollbackTarget =
number-1 and call d.rollbackLocalChain as before.
There was a problem hiding this comment.
Same reasoning as the firstBlockNumber - 1 underflow discussion: SafeL2Data.Number comes from L1 batch parsing, and genesis block (0) is never included in a batch. Even hypothetically, the underflowed value goes to rollbackLocalChain which returns error → halt. No silent corruption possible.
There was a problem hiding this comment.
@curryxbo, understood — the same invariant applies here: SafeL2Data.Number is derived from L1 batch data, which structurally never contains the genesis block, so zero is not a reachable value. And even in a hypothetical adversarial case, the underflowed target lands in rollbackLocalChain, which currently returns an error and sets d.halted = true, preventing any silent corruption. I'll withdraw this concern.
✏️ Learnings added
Learnt from: curryxbo
Repo: morph-l2/morph PR: 907
File: node/derivation/derivation.go:847-852
Timestamp: 2026-03-10T14:34:22.656Z
Learning: In `node/derivation/derivation.go` (morph-l2/morph PR `#907`), `blockData.SafeL2Data.Number` underflow to max uint64 when computing `rollbackTarget := blockData.SafeL2Data.Number - 1` is not a real concern. `SafeL2Data.Number` is parsed from L1 batch data and the genesis block (number 0) is never included in any committed batch. Additionally, even if underflow hypothetically occurred, `rollbackLocalChain` would return an error causing `d.halted = true`, with no silent corruption. Do not flag uint64 underflow for this expression.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: curryxbo
Repo: morph-l2/morph PR: 907
File: node/derivation/config.go:119-121
Timestamp: 2026-03-10T14:16:02.526Z
Learning: In `node/derivation/config.go` (morph-l2/morph), `ReorgCheckDepth == 0` is intentionally valid and not a bug. When set to zero, `detectReorg` computes `checkFrom` equal to `*latestDerivation`, `ReadDerivationL1BlockRange` returns an empty slice, and the function returns nil — effectively disabling historical L1 reorg checking with no errors or side effects. Do not flag missing zero-rejection validation for this field.
There was a problem hiding this comment.
♻️ Duplicate comments (5)
node/derivation/derivation.go (5)
427-436:⚠️ Potential issue | 🟡 MinorEdge case: Writing height 0 causes block 0 to be skipped after reorg at genesis.
When
d.startHeight == 0and a reorg occurs at genesis, line 434 writes0to the DB. On the nextderivationBlockcall:
latestDerivationreturns&0(pointer to 0, not nil)start = *latestDerivation + 1 = 1- Block 0 is skipped
The fix requires deleting the DB key so
ReadLatestDerivationL1Height()returns nil, causing the next loop to default tostart = d.startHeight.🛡️ Proposed fix
if reorgAtL1Height > d.startHeight { d.db.WriteLatestDerivationL1Height(reorgAtL1Height - 1) } else { - // Reorg at or before startHeight — reset so next loop starts from startHeight. - if d.startHeight > 0 { - d.db.WriteLatestDerivationL1Height(d.startHeight - 1) - } else { - d.db.WriteLatestDerivationL1Height(0) - } + // Reorg at or before startHeight — delete the key so next loop starts from startHeight. + d.db.DeleteLatestDerivationL1Height() }This requires adding
DeleteLatestDerivationL1Height()to the database interface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/derivation/derivation.go` around lines 427 - 436, The code writes 0 into the DB when a reorg occurs at genesis which causes ReadLatestDerivationL1Height() to return &0 and skip block 0; instead, when d.startHeight == 0 you must delete the stored key so ReadLatestDerivationL1Height() returns nil and the next loop uses start = d.startHeight. Add DeleteLatestDerivationL1Height() to the DB interface and implementations, and replace the branch that currently calls d.db.WriteLatestDerivationL1Height(0) with d.db.DeleteLatestDerivationL1Height(); keep existing WriteLatestDerivationL1Height(reorgAtL1Height-1) for reorgAtL1Height > d.startHeight and WriteLatestDerivationL1Height(d.startHeight-1) for d.startHeight > 0.
532-548:⚠️ Potential issue | 🟠 MajorPotential nil pointer dereference when fetching L1 headers.
Same issue as in
detectReorg:HeaderByNumbermay return(nil, nil)for a block that doesn't exist. Line 540 callsheader.Hash()without a nil check, which would panic.🐛 Proposed fix
func (d *Derivation) recordL1Blocks(ctx context.Context, from, to uint64) error { for h := from; h <= to; h++ { header, err := d.l1Client.HeaderByNumber(ctx, big.NewInt(int64(h))) if err != nil { return fmt.Errorf("failed to get L1 header at %d: %w", h, err) } + if header == nil { + return fmt.Errorf("L1 header at %d not found (possible reorg)", h) + } var hashBytes [32]byte copy(hashBytes[:], header.Hash().Bytes())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/derivation/derivation.go` around lines 532 - 548, In recordL1Blocks, HeaderByNumber may return (nil, nil) causing a panic when calling header.Hash(); update the loop to check if header == nil after the call to d.l1Client.HeaderByNumber and return a descriptive error (or skip/handle as appropriate) instead of dereferencing; ensure the check is placed before using header.Hash() and before copying into hashBytes, and keep the rest of the logic that calls d.db.WriteDerivationL1Block intact (referencing recordL1Blocks, HeaderByNumber, header.Hash, and WriteDerivationL1Block to locate the change).
315-321:⚠️ Potential issue | 🟡 MinorPotential underflow if
firstBlockNumberis 0.The computation
rollbackTarget := batchInfo.firstBlockNumber - 1will underflow to^uint64(0)(max uint64) ifbatchInfo.firstBlockNumberis 0. While genesis block mismatches are rare, this should be guarded defensively.🛡️ Proposed fix
+ if batchInfo.firstBlockNumber == 0 { + d.logger.Error("cannot rollback before genesis block", "batchIndex", batchInfo.batchIndex) + d.halted = true + d.metrics.SetHalted() + return + } rollbackTarget := batchInfo.firstBlockNumber - 1 if err := d.rollbackLocalChain(rollbackTarget); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/derivation/derivation.go` around lines 315 - 321, The subtraction can underflow when batchInfo.firstBlockNumber is 0; before computing rollbackTarget do an explicit check for batchInfo.firstBlockNumber == 0 and handle it safely (e.g., log an error with context via d.logger.Error, set d.halted = true and d.metrics.SetHalted(), and return) instead of performing batchInfo.firstBlockNumber - 1; only compute rollbackTarget and call d.rollbackLocalChain(rollbackTarget) when firstBlockNumber > 0 to avoid the uint64 underflow.
382-408:⚠️ Potential issue | 🟠 MajorPotential nil pointer dereference when fetching L1 headers.
HeaderByNumbermay return(nil, nil)for a block that doesn't exist (e.g., due to a concurrent reorg). Both the fast path (line 388:newestHeader.Hash()) and slow path (line 400:header.Hash()) access the hash without nil checks, which would panic.🐛 Proposed fix
// Fast path: check the newest block first. If it matches, no reorg occurred. newest := savedBlocks[len(savedBlocks)-1] newestHeader, err := d.l1Client.HeaderByNumber(ctx, big.NewInt(int64(newest.Number))) if err != nil { return nil, fmt.Errorf("failed to get L1 header at %d: %w", newest.Number, err) } + if newestHeader == nil { + return nil, fmt.Errorf("L1 header at %d not found (possible reorg)", newest.Number) + } if newestHeader.Hash() == common.BytesToHash(newest.Hash[:]) { return nil, nil } // Slow path: reorg detected. Scan oldest-to-newest to find the earliest divergence. for i := 0; i < len(savedBlocks); i++ { block := savedBlocks[i] header, err := d.l1Client.HeaderByNumber(ctx, big.NewInt(int64(block.Number))) if err != nil { return nil, fmt.Errorf("failed to get L1 header at %d: %w", block.Number, err) } + if header == nil { + return nil, fmt.Errorf("L1 header at %d not found (possible reorg)", block.Number) + } savedHash := common.BytesToHash(block.Hash[:])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/derivation/derivation.go` around lines 382 - 408, The code calls d.l1Client.HeaderByNumber and immediately uses newestHeader.Hash() and header.Hash() which can panic if HeaderByNumber returns (nil, nil); update both the fast-path (variable newestHeader) and slow-path (variable header inside the savedBlocks loop) to check if the returned header is nil and return a clear error (e.g., fmt.Errorf("no header returned for L1 block %d") or treat as divergence) before accessing .Hash(); ensure modifications reference d.l1Client.HeaderByNumber, newestHeader, and header so the nil-checks are applied consistently in both places and include the block number in the error message.
829-844:⚠️ Potential issue | 🟠 MajorMultiple issues in block verification path.
Nil pointer dereference (line 829):
HeaderByNumbermay return(nil, nil), causingverifyBlockContext(localHeader, blockData)at line 834 to panic.Potential underflow (line 839): If
blockData.SafeL2Data.Numberis 0,rollbackTargetunderflows to max uint64.🐛 Proposed fix
if blockData.SafeL2Data.Number <= latestBlockNumber { // Block already exists locally - verify it matches the batch data localHeader, err := d.l2Client.HeaderByNumber(d.ctx, big.NewInt(int64(blockData.SafeL2Data.Number))) if err != nil { return nil, fmt.Errorf("query header by number error:%v", err) } + if localHeader == nil { + return nil, fmt.Errorf("local header at %d not found", blockData.SafeL2Data.Number) + } if err := d.verifyBlockContext(localHeader, blockData); err != nil { d.logger.Error("block context mismatch with L1 batch data, rollback required", "blockNumber", blockData.Number, "error", err) d.metrics.IncBlockMismatchCount() + if blockData.SafeL2Data.Number == 0 { + d.halted = true + d.metrics.SetHalted() + return nil, fmt.Errorf("cannot rollback before genesis block %d", blockData.Number) + } rollbackTarget := blockData.SafeL2Data.Number - 1 if err := d.rollbackLocalChain(rollbackTarget); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/derivation/derivation.go` around lines 829 - 844, HeaderByNumber can return (nil, nil) and the subsequent call to verifyBlockContext(localHeader, blockData) can panic, and computing rollbackTarget := blockData.SafeL2Data.Number - 1 can underflow when Number == 0; update the code around the HeaderByNumber call to explicitly handle a nil header even when err == nil (treat as verification failure: log via d.logger, increment d.metrics.IncBlockMismatchCount(), set d.halted and d.metrics.SetHalted() as appropriate, and return a clear error) and before subtracting check if blockData.SafeL2Data.Number == 0 (handle as a special case: do not subtract, return a clear error or set rollbackTarget to 0 and avoid calling rollbackLocalChain, or bail out with halted state), locating changes around d.l2Client.HeaderByNumber, verifyBlockContext, and rollbackLocalChain to ensure no nil deref or uint underflow occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@node/derivation/derivation.go`:
- Around line 427-436: The code writes 0 into the DB when a reorg occurs at
genesis which causes ReadLatestDerivationL1Height() to return &0 and skip block
0; instead, when d.startHeight == 0 you must delete the stored key so
ReadLatestDerivationL1Height() returns nil and the next loop uses start =
d.startHeight. Add DeleteLatestDerivationL1Height() to the DB interface and
implementations, and replace the branch that currently calls
d.db.WriteLatestDerivationL1Height(0) with
d.db.DeleteLatestDerivationL1Height(); keep existing
WriteLatestDerivationL1Height(reorgAtL1Height-1) for reorgAtL1Height >
d.startHeight and WriteLatestDerivationL1Height(d.startHeight-1) for
d.startHeight > 0.
- Around line 532-548: In recordL1Blocks, HeaderByNumber may return (nil, nil)
causing a panic when calling header.Hash(); update the loop to check if header
== nil after the call to d.l1Client.HeaderByNumber and return a descriptive
error (or skip/handle as appropriate) instead of dereferencing; ensure the check
is placed before using header.Hash() and before copying into hashBytes, and keep
the rest of the logic that calls d.db.WriteDerivationL1Block intact (referencing
recordL1Blocks, HeaderByNumber, header.Hash, and WriteDerivationL1Block to
locate the change).
- Around line 315-321: The subtraction can underflow when
batchInfo.firstBlockNumber is 0; before computing rollbackTarget do an explicit
check for batchInfo.firstBlockNumber == 0 and handle it safely (e.g., log an
error with context via d.logger.Error, set d.halted = true and
d.metrics.SetHalted(), and return) instead of performing
batchInfo.firstBlockNumber - 1; only compute rollbackTarget and call
d.rollbackLocalChain(rollbackTarget) when firstBlockNumber > 0 to avoid the
uint64 underflow.
- Around line 382-408: The code calls d.l1Client.HeaderByNumber and immediately
uses newestHeader.Hash() and header.Hash() which can panic if HeaderByNumber
returns (nil, nil); update both the fast-path (variable newestHeader) and
slow-path (variable header inside the savedBlocks loop) to check if the returned
header is nil and return a clear error (e.g., fmt.Errorf("no header returned for
L1 block %d") or treat as divergence) before accessing .Hash(); ensure
modifications reference d.l1Client.HeaderByNumber, newestHeader, and header so
the nil-checks are applied consistently in both places and include the block
number in the error message.
- Around line 829-844: HeaderByNumber can return (nil, nil) and the subsequent
call to verifyBlockContext(localHeader, blockData) can panic, and computing
rollbackTarget := blockData.SafeL2Data.Number - 1 can underflow when Number ==
0; update the code around the HeaderByNumber call to explicitly handle a nil
header even when err == nil (treat as verification failure: log via d.logger,
increment d.metrics.IncBlockMismatchCount(), set d.halted and
d.metrics.SetHalted() as appropriate, and return a clear error) and before
subtracting check if blockData.SafeL2Data.Number == 0 (handle as a special case:
do not subtract, return a clear error or set rollbackTarget to 0 and avoid
calling rollbackLocalChain, or bail out with halted state), locating changes
around d.l2Client.HeaderByNumber, verifyBlockContext, and rollbackLocalChain to
ensure no nil deref or uint underflow occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 762a067e-8ed5-4d0e-8d55-3082dc8b099c
📒 Files selected for processing (2)
node/derivation/derivation.gonode/derivation/metrics.go
🚧 Files skipped from review as they are similar to previous changes (1)
- node/derivation/metrics.go
…ix doc env var 1. Add nil check for lastHeader after derive() returns — if blockContexts is empty, skip the batch instead of panicking on lastHeader.Number. 2. Fix DERIVATION_REFACTOR.md: env var is MORPH_NODE_DERIVATION_REORG_CHECK_DEPTH (was missing NODE_ prefix). Made-with: Cursor
…ment halted metric Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
node/derivation/derivation.go (3)
390-408:⚠️ Potential issue | 🟠 MajorNil-check every fetched header before using it.
These
HeaderByNumberresults are dereferenced immediately. If either client returns(nil, nil)during a reorg or missing-height race,.Hash()orverifyBlockContext()will panic before the recovery path runs.Suggested fix
newestHeader, err := d.l1Client.HeaderByNumber(ctx, big.NewInt(int64(newest.Number))) if err != nil { return nil, fmt.Errorf("failed to get L1 header at %d: %w", newest.Number, err) } + if newestHeader == nil { + return nil, fmt.Errorf("L1 header at %d not found", newest.Number) + } if newestHeader.Hash() == common.BytesToHash(newest.Hash[:]) { return nil, nil } @@ header, err := d.l1Client.HeaderByNumber(ctx, big.NewInt(int64(block.Number))) if err != nil { return nil, fmt.Errorf("failed to get L1 header at %d: %w", block.Number, err) } + if header == nil { + return nil, fmt.Errorf("L1 header at %d not found", block.Number) + } savedHash := common.BytesToHash(block.Hash[:]) @@ header, err := d.l1Client.HeaderByNumber(ctx, big.NewInt(int64(h))) if err != nil { return fmt.Errorf("failed to get L1 header at %d: %w", h, err) } + if header == nil { + return fmt.Errorf("L1 header at %d not found", h) + } var hashBytes [32]byte @@ localHeader, err := d.l2Client.HeaderByNumber(d.ctx, big.NewInt(int64(blockData.SafeL2Data.Number))) if err != nil { return nil, fmt.Errorf("query header by number error:%v", err) } + if localHeader == nil { + return nil, fmt.Errorf("local header at %d not found", blockData.SafeL2Data.Number) + } if err := d.verifyBlockContext(localHeader, blockData); err != nil {Also applies to: 541-548, 837-842
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/derivation/derivation.go` around lines 390 - 408, The code calls d.l1Client.HeaderByNumber and immediately uses the returned header (e.g., newestHeader.Hash() and header.Hash()) which can be (nil, nil) and cause a panic; update every place that calls HeaderByNumber (including use sites around newestHeader, the loop's header variable, and the verifyBlockContext call sites) to nil-check the returned header and return a clear error (or handle missing header) before dereferencing; reference d.l1Client.HeaderByNumber, newestHeader, header, savedHash and verifyBlockContext when adding the nil checks so each call path guards against a nil header prior to calling .Hash() or passing into verifyBlockContext.
435-443:⚠️ Potential issue | 🟡 MinorResetting to
0still skips genesis after a reorg.When
d.startHeight == 0, writing0here makes the next loop start from1becausederivationBlock()always adds one to a stored height. Delete/reset the key instead of persisting0.Suggested fix
if reorgAtL1Height > d.startHeight { d.db.WriteLatestDerivationL1Height(reorgAtL1Height - 1) } else { - // Reorg at or before startHeight — reset so next loop starts from startHeight. - if d.startHeight > 0 { - d.db.WriteLatestDerivationL1Height(d.startHeight - 1) - } else { - d.db.WriteLatestDerivationL1Height(0) - } + // Reorg at or before startHeight — reset so next loop starts from startHeight. + if d.startHeight > 0 { + d.db.WriteLatestDerivationL1Height(d.startHeight - 1) + } else { + d.db.DeleteLatestDerivationL1Height() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/derivation/derivation.go` around lines 435 - 443, The current branch writes 0 when d.startHeight == 0 which causes derivationBlock() to start at height 1 (skipping genesis); instead, when startHeight is 0 unset/delete the persisted latest derivation L1 height rather than writing 0. Replace the d.db.WriteLatestDerivationL1Height(0) call with a removal/clear call on the DB (e.g., d.db.DeleteLatestDerivationL1Height() or the appropriate "clear" method provided by the DB), leaving higher-case branch unchanged so reorgAtL1Height > d.startHeight still writes reorgAtL1Height-1.
319-320:⚠️ Potential issue | 🟡 MinorProtect both rollback targets against block-0 underflow.
Both paths compute
n - 1on auint64without guarding0. If the mismatch happens on the first derived block, the rollback target becomesmath.MaxUint64and the node halts on a nonsensical rewind height.Suggested fix
- rollbackTarget := batchInfo.firstBlockNumber - 1 + if batchInfo.firstBlockNumber == 0 { + d.halted = true + d.metrics.SetHalted() + d.logger.Error("cannot rollback before genesis block", "batchIndex", batchInfo.batchIndex) + return + } + rollbackTarget := batchInfo.firstBlockNumber - 1- rollbackTarget := blockData.SafeL2Data.Number - 1 + if blockData.SafeL2Data.Number == 0 { + d.halted = true + d.metrics.SetHalted() + return nil, fmt.Errorf("cannot rollback before genesis block at block %d", blockData.Number) + } + rollbackTarget := blockData.SafeL2Data.Number - 1Also applies to: 847-848
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/derivation/derivation.go` around lines 319 - 320, The rollback target calculation subtracts 1 from uint64 values (e.g., batchInfo.firstBlockNumber and the other occurrence around lines 847-848) without guarding against 0, which underflows to math.MaxUint64; update the code that computes rollbackTarget (and the duplicate site) to check if the source is 0 and handle it explicitly (e.g., skip rollback, set target to 0, or return an error) before doing n-1, and call d.rollbackLocalChain only with a validated/guarded rollbackTarget.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@node/derivation/DERIVATION_REFACTOR.md`:
- Around line 53-67: The fenced code blocks in DERIVATION_REFACTOR.md (the
blocks starting with "derivationBlock() loop start", "block.Number <=
latestBlockNumber?", and "After derive(batchInfo) completes:") are missing a
language and trigger markdownlint MD040; update each opening triple-backtick to
include a language token (e.g., change ``` to ```text) so the examples are
fenced as text, and apply the same change to the other occurrences referenced in
the comment (the similar blocks around the other ranges).
In `@node/derivation/derivation.go`:
- Around line 550-553: Update the Writer interface to have
WriteDerivationL1Block(...) return error, change the store implementation of
WriteDerivationL1Block to return errors instead of panicking on RLP or DB
failures (propagate underlying error from encoding/db ops), and then modify
recordL1Blocks to check the returned error for each call to
d.db.WriteDerivationL1Block and return that error immediately so failures are
propagated to the caller (preventing subsequent WriteLatestDerivationL1Height
from advancing on partial writes). Ensure all other call sites of
WriteDerivationL1Block are updated to handle the returned error.
---
Duplicate comments:
In `@node/derivation/derivation.go`:
- Around line 390-408: The code calls d.l1Client.HeaderByNumber and immediately
uses the returned header (e.g., newestHeader.Hash() and header.Hash()) which can
be (nil, nil) and cause a panic; update every place that calls HeaderByNumber
(including use sites around newestHeader, the loop's header variable, and the
verifyBlockContext call sites) to nil-check the returned header and return a
clear error (or handle missing header) before dereferencing; reference
d.l1Client.HeaderByNumber, newestHeader, header, savedHash and
verifyBlockContext when adding the nil checks so each call path guards against a
nil header prior to calling .Hash() or passing into verifyBlockContext.
- Around line 435-443: The current branch writes 0 when d.startHeight == 0 which
causes derivationBlock() to start at height 1 (skipping genesis); instead, when
startHeight is 0 unset/delete the persisted latest derivation L1 height rather
than writing 0. Replace the d.db.WriteLatestDerivationL1Height(0) call with a
removal/clear call on the DB (e.g., d.db.DeleteLatestDerivationL1Height() or the
appropriate "clear" method provided by the DB), leaving higher-case branch
unchanged so reorgAtL1Height > d.startHeight still writes reorgAtL1Height-1.
- Around line 319-320: The rollback target calculation subtracts 1 from uint64
values (e.g., batchInfo.firstBlockNumber and the other occurrence around lines
847-848) without guarding against 0, which underflows to math.MaxUint64; update
the code that computes rollbackTarget (and the duplicate site) to check if the
source is 0 and handle it explicitly (e.g., skip rollback, set target to 0, or
return an error) before doing n-1, and call d.rollbackLocalChain only with a
validated/guarded rollbackTarget.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a7746c5-078f-45fd-8ba3-1ffd34b4c57a
📒 Files selected for processing (2)
node/derivation/DERIVATION_REFACTOR.mdnode/derivation/derivation.go
node/derivation/derivation.go
Outdated
| d.db.WriteDerivationL1Block(&db.DerivationL1Block{ | ||
| Number: h, | ||
| Hash: hashBytes, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file mentioned in the review
find . -name "derivation.go" -path "*/node/*" | head -5Repository: morph-l2/morph
Length of output: 89
🏁 Script executed:
# Get basic file structure
ls -la node/derivation/ 2>/dev/null || echo "Path not found, searching..."
fd "derivation" --type fRepository: morph-l2/morph
Length of output: 1122
🏁 Script executed:
# Look at the specific file and lines mentioned
if [ -f "node/derivation/derivation.go" ]; then
wc -l node/derivation/derivation.go
fiRepository: morph-l2/morph
Length of output: 91
🏁 Script executed:
# Read the specific lines mentioned in the review (550-553) plus context
sed -n '530,580p' node/derivation/derivation.go | cat -nRepository: morph-l2/morph
Length of output: 1992
🏁 Script executed:
# Also get the function signature to understand recordL1Blocks
rg "func.*recordL1Blocks" -A 5 node/derivation/derivation.goRepository: morph-l2/morph
Length of output: 328
🏁 Script executed:
# Check if WriteDerivationL1Block and WriteLatestDerivationL1Height are called in this function
rg "WriteLatestDerivationL1Height|WriteDerivationL1Block" -B 2 -A 2 node/derivation/derivation.goRepository: morph-l2/morph
Length of output: 686
🏁 Script executed:
# Look at the database interface definition
cat node/derivation/database.goRepository: morph-l2/morph
Length of output: 559
🏁 Script executed:
# Find the database implementation (likely in another directory)
fd "database\|store" --type f | grep -E "\.(go)$" | head -20Repository: morph-l2/morph
Length of output: 40
🏁 Script executed:
# Search for WriteDerivationL1Block implementation
rg "func.*WriteDerivationL1Block" -A 10Repository: morph-l2/morph
Length of output: 677
🏁 Script executed:
# Find where recordL1Blocks is called to see error handling
rg "recordL1Blocks" -B 3 -A 3Repository: morph-l2/morph
Length of output: 2560
recordL1Blocks() cannot propagate database write errors — silent failures can leave permanent gaps in reorg tracking.
The WriteDerivationL1Block() method in the Writer interface does not return an error. The store implementation panics on RLP encoding or database failures instead of returning them, so recordL1Blocks() cannot detect or propagate write failures. If a DB write fails mid-loop, the function returns nil anyway, allowing the caller to proceed with WriteLatestDerivationL1Height(end), creating a permanent gap in L1 block tracking. This violates the function's documented contract: "Returns an error if any header fetch fails — the caller must not advance derivation height to avoid permanent gaps."
Fix requires:
- Modify the
Writerinterface to returnerrorfromWriteDerivationL1Block() - Update the store implementation to return errors instead of panicking
- Check and propagate errors in
recordL1Blocks()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@node/derivation/derivation.go` around lines 550 - 553, Update the Writer
interface to have WriteDerivationL1Block(...) return error, change the store
implementation of WriteDerivationL1Block to return errors instead of panicking
on RLP or DB failures (propagate underlying error from encoding/db ops), and
then modify recordL1Blocks to check the returned error for each call to
d.db.WriteDerivationL1Block and return that error immediately so failures are
propagated to the caller (preventing subsequent WriteLatestDerivationL1Height
from advancing on partial writes). Ensure all other call sites of
WriteDerivationL1Block are updated to handle the returned error.
There was a problem hiding this comment.
The premise is incorrect — WriteDerivationL1Block panics on DB write failure (see store.go:168-172), it doesn't silently return. A panic terminates the process; there is no code path where a DB write fails mid-loop and execution continues to WriteLatestDerivationL1Height. The "permanent gap" scenario described cannot occur.
The panic-on-error pattern is consistent with all other DB methods in this codebase (WriteSyncedL1Messages, WriteLatestDerivationL1Height, etc.). Changing this to error-return would be a DB-layer-wide refactor — existing technical debt, not in scope for this PR.
There was a problem hiding this comment.
@curryxbo, you're right — I apologize for the incorrect analysis. Since WriteDerivationL1Block panics on DB write failure, the process terminates immediately; execution never continues to WriteLatestDerivationL1Height, so the "permanent gap" scenario I described cannot occur. The panic-on-error pattern is the established DB-layer convention here, and refactoring it to return errors would be out of scope for this PR.
✏️ Learnings added
Learnt from: curryxbo
Repo: morph-l2/morph PR: 907
File: node/derivation/derivation.go:550-553
Timestamp: 2026-03-10T14:34:23.835Z
Learning: In the morph-l2/morph codebase (Go, node/db/store.go), all DB write methods (`WriteDerivationL1Block`, `WriteSyncedL1Messages`, `WriteLatestDerivationL1Height`, etc.) use a panic-on-error pattern rather than returning errors. A panic terminates the process immediately on DB write failure, so there is no silent failure path. Do not flag missing error-return propagation for these DB write methods — the panic behavior is intentional and consistent across the codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: curryxbo
Repo: morph-l2/morph PR: 907
File: node/derivation/config.go:119-121
Timestamp: 2026-03-10T14:16:02.526Z
Learning: In `node/derivation/config.go` (morph-l2/morph), `ReorgCheckDepth == 0` is intentionally valid and not a bug. When set to zero, `detectReorg` computes `checkFrom` equal to `*latestDerivation`, `ReadDerivationL1BlockRange` returns an empty slice, and the function returns nil — effectively disabling historical L1 reorg checking with no errors or side effects. Do not flag missing zero-rejection validation for this field.
Made-with: Cursor
Extract newly added functions into dedicated files for clarity: - verify.go: rollbackLocalChain, verifyBatchRoots, verifyBlockContext - reorg.go: detectReorg, handleL1Reorg, recordL1Blocks Existing batch parsing code stays in derivation.go to keep the diff scoped to this PR's changes only. No logic changes — pure file split. Made-with: Cursor
1 participant
Summary
Refactors the derivation module to support
latest-mode L1 batch fetching with L1 reorg detection, and replaces the challenge mechanism with a rollback-and-re-derive recovery model.Key changes
validatordependency andChallengeState/ChallengeEnableremoved. Divergence triggers rollback + re-derive instead of challenge.verifyBlockContextfor timestamp/gasLimit/baseFee/txCount,verifyBatchRootsfor stateRoot/withdrawalRoot).rollbackLocalChain()stub: Returns error until gethSetHeadAPI is exposed.New components
DerivationL1Block(db){Number, Hash}for reorg detectiondetectReorg()handleL1Reorg()verifyBlockContext()verifyBatchRoots()recordL1Blocks()ReorgCheckDepthconfig--derivation.reorgCheckDepth(default: 64)l1_reorg_detected_total,l2_rollback_total,block_mismatch_totalDesign decisions
derivationBlockreturns immediately. This avoids recording potentially unstable L1 hashes while the chain is still reorging.recordL1Blocksfails mid-range,WriteLatestDerivationL1Heightis skipped entirely, so the next loop retries the full range. This prevents permanent gaps in L1 hash tracking.recordL1BlocksanddetectReorgare both skipped whenconfirmations == FinalizedBlockNumber, since finalized blocks don't reorg.Modified files
node/derivation/derivation.gonode/derivation/database.goReader/Writerinterfaces for L1 block hash trackingnode/derivation/config.goReorgCheckDepthconfig fieldnode/derivation/metrics.gonode/db/keys.goderivationL1BlockPrefixandDerivationL1BlockKey()node/db/store.goDerivationL1Blockstruct and 4 CRUD methodsnode/flags/flags.goDerivationReorgCheckDepthCLI flagnode/cmd/node/main.govalidatordependency fromNewDerivationClientcallnode/derivation/DERIVATION_REFACTOR.mdFollow-up work (not in this PR)
rollbackLocalChain()implementation — ExposeSetHeadvia geth engine API chain (l2_api.go→authclient→retryable_client).BlockByNumberRPC onRetryableClient.Test plan
go build ./node/...passesgo test ./node/derivation/...passes--derivation.confirmations -2): reorg detection activates, L1 block hashes trackedSummary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation