feat(derivation): SPEC-005 — validator cleanup + Path A/B verification mode + derivation-driven tag management#951
Open
curryxbo wants to merge 20 commits into
Open
Conversation
Replace the internal codename "bitget" with the neutral term "polyrepo" in build context references, variable names, and container paths.
Cover the rest of the file with one-line docstrings to satisfy CodeRabbit's docstring coverage threshold.
…g Dockerfile.l2-geth Move the morph-el-0 build: section from docker-compose-4nodes.yml into a dedicated docker-compose-geth-build.yml, included only when EXECUTION_CLIENT=geth. The reth overlay now sees no build: on morph-el-0, eliminating the risk of docker compose up building geth code and tagging it as the reth image when the reth image is absent.
Keep the base devnet compose file self-contained for geth while using the reth overlay to explicitly reset inherited geth build definitions. Constraint: Do not include the devnet execution-client test file in this commit Rejected: Keep a separate geth build compose file | changes direct base compose usage Confidence: high Scope-risk: narrow
[codex] support reth execution client in devnet
- Delete node/validator package (config.go, validator.go, validator_test.go) - Drop validator wiring from node/cmd/node/main.go and derivation.NewDerivationClient signature - Drop validator field from Derivation struct - Drop ChallengeEnable/ChallengeState invocation in derivation rollback path - Remove validator.challengeEnable / validator.privateKey CLI flags - Remove MORPH_NODE_VALIDATOR_PRIVATE_KEY env from docker compose files Refs: morph-l2/morph-specs SPEC-005 §4.1 Equivalent in intent to PR #948 commit 3e49457, but applied directly to main without the Phase A reorg/halted/rollback context that 3e49457 brought along.
Same as the merge previously applied to PR #948's branch — needed because PR #935 (multi_batch / Batch Header v2) hasn't landed in main yet, but SPEC-005 narrow scope's Path B implementation depends on common/batch + common/blob packages introduced by it. Resolutions: - ops/docker/docker-compose-4nodes.yml: take origin/feat/multi_batch's TX_SUBMITTER_PRIORITY_ROLLUP=true and drop TX_SUBMITTER_SEAL_BATCH; that env block is owned by tx-submitter, which feat/derivation-batch-verify should not be modifying. - go-ethereum submodule: keep at 045be0fd (v2.2.2). Multi_batch's 53aee9b internal merge accidentally regressed the pointer to 1d460577; that regression is unrelated to multi_batch's design intent and should not propagate to feat/derivation-batch-verify.
Pull the inline state-root / withdrawal-root mismatch check out of
derivationBlock into a standalone verifyBatchRoots(batchInfo, lastHeader)
function in a new node/derivation/verify.go.
Both roots are read from L1 calldata at parse time, so verifyBatchRoots
is independent of blob data — this is the SPEC-005 §3.4 invariant that
later allows Path B (local-rebuild verification) to reuse this same
check without modification.
No behavior change: the main loop still logs + returns on mismatch, sets
stateException on the metric, and continues otherwise. Only the location
and error-message format change.
Refs: morph-l2/morph-specs SPEC-005 §4.2 / §6 ("两种模式共享同一份 verifyBatchRoots")
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR removes validator and blocktag services, introduces SPEC-005 Path B batch verification with a finalizer subcomponent and tag advancer, abstracts the L2 execution layer naming from geth to a generic "el" service, and refactors devnet/Makefile to support pluggable execution clients (geth/reth). ChangesDerivation Path B Verification and Safe/Finalized Tag Management
Service Architecture Removal
Execution Client Abstraction and Devnet Reth Support
Execution Service Rebranding (geth → el)
Supporting Changes
Sequence Diagram(s)sequenceDiagram
participant L1
participant Node as Derivation<br/>Client
participant TagAdv as TagAdvancer
participant Final as Finalizer
participant L2 as L2 Engine
autonumber
rect rgba(100, 150, 255, 0.5)
Note over Node,Final: Batch Verification & Safe Advancement (Per Batch)
Node->>Node: Check verifyMode
alt pathB Mode
Node->>L1: fetchBatchInfoPathB<br/>(L1 commitBatch tx)
L1-->>Node: BatchInfo with blobHashes
Node->>Node: verifyBatchContentPathB<br/>(reconstruct & compare)
Node->>Node: fetchLocalLastHeader
else pathA Mode
Node->>L1: fetchRollupDataByTxHash
L1-->>Node: RollupData
Node->>Node: derive (legacy path)
end
Node->>TagAdv: advanceSafe(header, batchIndex)
TagAdv->>L2: SetBlockTags (safe)
L2-->>TagAdv: OK
end
rect rgba(150, 200, 100, 0.5)
Note over Final,L2: Finalization Poll (Every 30s)
Final->>L1: Read L1 finalized header
L1-->>Final: blockNumber
Final->>L1: Query LastCommittedBatchIndex @ L1 finalized
L1-->>Final: batchIndex
Final->>Node: BatchDataStore lookup<br/>candidate batch → lastL2Block
Node-->>Final: L2 block number
Final->>Node: Fetch L2 header @ lastL2Block
Node-->>Final: Header
Final->>TagAdv: advanceFinalized(header, batchIndex)
TagAdv->>TagAdv: Check finalized ≤ safe
TagAdv->>L2: SetBlockTags (finalized)
L2-->>TagAdv: OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
6 tasks
Introduces Path B verification mode (SPEC-005 §4) as a startup-time
mutually-exclusive alternative to Path A. Selected via
`--derivation.verify-mode` (default "pathA", preserves current
behaviour). The two modes do not interact: under no circumstance does
the node fall back from one to the other; switching requires changing
the config and restarting.
Path B mode:
- Skips beacon-side blob fetch entirely.
- Reads L1 commitBatch tx calldata + tx.BlobHashes() only.
- Reads local L2 blocks for the batch range, replays the sequencer's
encoding (parsingTxs + buildBlockContext + BatchData) to rebuild
the blob bytes.
- Compresses + builds a BlobTxSidecar; compares its versioned hashes
against the values from L1 calldata.
- Reuses the shared verifyBatchRoots for state/withdrawal root
verification (independent of blob).
Touches:
- common/batch: export ParsingTxs / BuildBlockContext (rename from
package-private). No semantic change for tx-submitter.
- node/derivation/batch_info.go: add BatchInfo.blobHashes field;
add ParseBatchMetadataOnly (calldata-only parse, no blob).
- node/derivation/config.go + flags: add VerifyMode + flag with
fail-fast validation on unknown values.
- node/derivation/derivation.go: dispatch in main loop based on
verifyMode; populate blobHashes in Path A's fetch helper too so
BatchInfo is consistent across modes.
- node/derivation/verify_path_b.go: fetchBatchInfoPathB +
verifyBatchContentPathB + fetchLocalLastHeader.
- node/derivation/metrics.go: path_b_triggered_total /
path_b_failed_total counters.
Verified clean: `go build ./node/... ./common/...`, `go vet`,
test compile.
Refs: morph-l2/morph-specs SPEC-005 §4 / §5 / §6
Resolves the WIP item flagged in PR #951.
curryxbo
changed the title
2 tasks
…e cleanup Completes the remaining SPEC-005 work after Path B (commit 9f01c53): Target A residual -- delete validator node role: - Remove ValidatorEnable flag (--validator / MORPH_NODE_VALIDATOR) - Collapse cmd/node/main.go isValidator branch into a single linear assembly path; all non-mock nodes now run consensus + derivation simultaneously, sharing a single store + syncer instance - Remove validator-el / validator_node containers from docker-compose-4nodes.yml and the docker-compose-reth.yml override - Delete node/ops-morph/docker-compose-validator.yml Target C -- delete blocktag, derivation drives safe/finalized heads: - Delete node/blocktag/ package and BlockTagSafeConfirmations flag - Add tag_advance.go: tagAdvancer with advanceSafe / advanceFinalized, finalized<=safe<=unsafe invariant assertion, L1-reorg reset hook, and dedup of redundant SetBlockTags calls - Add finalizer.go: in-process subcomponent that polls L1 finalized -> Rollup.LastCommittedBatchIndex(@Finalized) and forwards to tagAdvancer.advanceFinalized; lifecycle joined to derivation.Stop - Wire derivation main loop: advanceSafe after verifyBatchRoots passes; finalizer goroutine started in Start() and joined in Stop() - Add 6 prometheus metrics (safe/finalized advance + l2 number gauges + l1 reorg reset + invariant violation counters) - Add --derivation.finalizer-interval flag (default 30s) Tests: - 7 tag_advance behavioural tests (safe/finalized advance, dedup, monotonicity, invariant skip, L1 reorg reset, BlockNumber failure) - 3 static-scan tests guarding against validator/blocktag residue and common/blob package drift in Path B Note on SPEC-005 section 4.3 [must]: tagAdvancer.reset() infrastructure is in place but is not yet invoked from production code; the trigger (derivation main loop's L1 reorg detection) is deferred per tech-design section 8. The blocktag-side independent polling path is fully removed as required. Refs: morph-l2/morph-specs SPEC-005 sections 4.1, 4.3 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
curryxbo
changed the title
Add 9 unit-test cases covering SPEC-005 §5.1 entries that don't require the full L1 / beacon / authclient mock stack: - config_test.go (3): TestVerifyMode_DefaultIsPathA / _AcceptsPathB / _RejectsUnknown -- exercise validateAndDefaultVerifyMode and assert DefaultConfig().VerifyMode pins to pathA. - verify_path_b_test.go (6): round-trip OK V1 + V2 (using empty L2 blocks against the same encoder pipeline as the L1 oracle), versioned hash mismatch, local block missing, local block read error, three invalid-input branches (firstBlock=0, last<first, empty blobHashes). Two small refactors enable the tests without constructing a Derivation: - verify_path_b.go: extract pathBBlockReader interface and free function verifyPathBContent; the Derivation method becomes a 1-line delegate so behaviour and error messages are unchanged. - config.go: extract validateAndDefaultVerifyMode from the inline switch in SetCliContext so the validation runs without a cli.Context. Tests deferred to devnet (§5.2): TestPathA_*, TestTagAdvance_FinalizedFromL1, TestTagAdvance_PathBVsPathAParity -- these require L1 client / beacon client / Rollup contract / RetryableClient fakes that add ~300-400 lines of mock scaffolding for limited unit-test value above devnet integration coverage. Refs: morph-l2/morph-specs SPEC-005 §5.1 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Brings in 6 commits from base feat/multi_batch since the previous merge (09a2c72) so PR #951 stops reporting CONFLICTING: - 4be9cda update challenge handler dep - c6f7cc2 add v0/v1 blob check - 7ec11cf Merge branch 'feat/multi_batch' of github.com:morph-l2/morph - fa9f4e8 fix submitter replay batch with config max blob count - 1fe0d74 add rustc version desc - fbd1bee chore: align go-ethereum submodule with origin/main Single content conflict in common/batch/batch_cache.go: base added a new sealEffectiveBlobCount function in the same hunk where SPEC-005 exported BuildBlockContext (PR #935-derived rename). Resolved by keeping both: sealEffectiveBlobCount stays where base placed it; the exported BuildBlockContext (with SPEC-005 docstring) follows. base's lowercase "buildBlockContext" comment was dropped (now stale -- we renamed it). All other touched files auto-merged: contracts/Rollup.sol, BlobVersionedHashLegacy.t.sol (added by base), prover/Cargo.{toml,lock}, prover/{,bin/challenge/}rust-toolchain (deleted by base), tx-submitter/mock/rollup.go, common/batch/batch_restart_test.go. go build ./common/batch/ ./common/blob/ ./node/... ./tx-submitter/... ./oracle/... -- clean. go test ./node/derivation/... -- 22 tests PASS (incl. 9 added in b9ad14e). common/batch tests hang on a pre-existing txSenderCacher goroutine leak unrelated to this merge (reproducible on origin/feat/multi_batch in isolation); not introduced here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
CI golangci-lint flagged two categories on the SPEC-005 derivation files: - gofmt: metrics.go, static_scan_test.go, tag_advance_test.go, verify_path_b.go -- alignment / spacing drift. Auto-fixed with gofmt -w; no semantic changes. - misspell (US English): four "behaviour" -> "behavior" sites in config.go (added in b9ad14e), finalizer.go (a73639d), tag_advance.go (a73639d), and verify_path_b.go (b9ad14e). go test ./node/derivation/... still passes 22 cases (incl. the 9 added in b9ad14e). No symbols renamed; comments only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
node/derivation/metrics.go (1)
20-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun
gofmton this file to unblock CI.The current pipeline is failing due to formatting in this file; apply
gofmtand re-run checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@node/derivation/metrics.go` around lines 20 - 42, The file's formatting is out of sync with gofmt; run gofmt on the Metrics struct file (e.g., the Metrics type and its fields like L1SyncHeight, PathBTriggered, SafeAdvanceTotal, etc.) to normalize spacing and alignment, then re-add the formatted file to your commit; you can run `gofmt -w node/derivation/metrics.go` (or `gofmt -w .` at repo root) and re-run CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@node/cmd/node/main.go`:
- Around line 122-129: The Syncer has unsynchronized concurrent reads/writes to
the uint64 field latestSynced (accessed by fetchL1Messages(), the background
goroutine started in Start(), and readers via LatestSynced()), causing a data
race; fix by protecting all accesses to latestSynced: either add a sync.Mutex
(e.g., s.mu) and wrap every read/write in fetchL1Messages() and LatestSynced()
with s.mu.Lock()/Unlock(), or convert latestSynced to a uint64 and use
sync/atomic functions (atomic.LoadUint64/atomic.StoreUint64) in
fetchL1Messages(), Start() and LatestSynced() so all reads/writes are atomic and
race-free. Ensure you update any logging that reads latestSynced to use the
protected/atomic read.
---
Outside diff comments:
In `@node/derivation/metrics.go`:
- Around line 20-42: The file's formatting is out of sync with gofmt; run gofmt
on the Metrics struct file (e.g., the Metrics type and its fields like
L1SyncHeight, PathBTriggered, SafeAdvanceTotal, etc.) to normalize spacing and
alignment, then re-add the formatted file to your commit; you can run `gofmt -w
node/derivation/metrics.go` (or `gofmt -w .` at repo root) and re-run CI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4791e23f-6353-4395-8d21-d6788540fd53
📒 Files selected for processing (35)
Makefilecommon/batch/batch_cache.gonode/blocktag/config.gonode/blocktag/service.gonode/cmd/node/main.gonode/derivation/batch_info.gonode/derivation/config.gonode/derivation/config_test.gonode/derivation/derivation.gonode/derivation/finalizer.gonode/derivation/metrics.gonode/derivation/static_scan_test.gonode/derivation/tag_advance.gonode/derivation/tag_advance_test.gonode/derivation/verify.gonode/derivation/verify_path_b.gonode/derivation/verify_path_b_test.gonode/flags/flags.gonode/ops-morph/docker-compose-validator.ymlnode/ops-morph/docker-compose.ymlnode/ops-morph/testnet/docker-compose.ymlnode/ops-morph/testnet/static-nodes.jsonnode/validator/config.gonode/validator/validator.gonode/validator/validator_test.goops/devnet-morph/devnet/__init__.pyops/docker-sequencer-test/Dockerfile.l2-geth-testops/docker-sequencer-test/Dockerfile.l2-node-testops/docker-sequencer-test/docker-compose.override.ymlops/docker-sequencer-test/run-test.shops/docker-sequencer-test/scripts/tx-generator.shops/docker/docker-compose-4nodes.ymlops/docker/docker-compose-reth.ymlops/docker/static-nodes.jsonoracle/docker-compose.yml
💤 Files with no reviewable changes (6)
- node/validator/config.go
- node/validator/validator_test.go
- node/validator/validator.go
- node/blocktag/config.go
- node/blocktag/service.go
- node/ops-morph/docker-compose-validator.yml
Comment on lines
+122
to
+129
| // Reuse the shared syncer instance -- DevSequencer mode is the only path | ||
| // that pulls a syncer out of NewExecutor, so we hand back the same one | ||
| // rather than letting NewExecutor open a second store + syncer. | ||
| newSyncerFunc := func() (*sync.Syncer, error) { return syncer, nil } | ||
| executor, err = node.NewExecutor(newSyncerFunc, nodeConfig, pubKey) | ||
| if err != nil { | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if sync.Syncer has internal locking or concurrency documentation
# Check for mutex/lock fields in the Syncer struct
rg -nP --type=go -A 20 'type Syncer struct' node/sync/
# Check for concurrent access documentation or warnings
rg -nP --type=go -C 5 'concurrent|thread.?safe|goroutine' node/sync/syncer.goRepository: morph-l2/morph
Length of output: 1102
🏁 Script executed:
# Check how syncer is used in executor initialization
rg -nP --type=go -B 5 -A 10 'NewExecutor' node/cmd/node/main.go
# Check executor's use of syncer
fd -e go -path "*/executor*" | head -20
# Check derivation client initialization and syncer usage
rg -nP --type=go -B 3 -A 10 'derivation|Derivation' node/cmd/node/main.go | head -50Repository: morph-l2/morph
Length of output: 2505
🏁 Script executed:
# Find where derivation is initialized with syncer
rg -nP --type=go 'derivation\.' node/cmd/node/main.go | head -30
# Check derivation initialization signature
fd -name derivation.go | xargs rg -nP 'func New.*Derivation' -A 5
# Check if derivation stores and uses syncer
fd -name "*.go" -path "*/derivation/*" -type f
# Check how executor and derivation are used (concurrent or sequential)
rg -nP --type=go 'go ' node/cmd/node/main.go | grep -E '(executor|derivation|dvNode)'Repository: morph-l2/morph
Length of output: 1357
🏁 Script executed:
# Check if dvNode and executor run concurrently
rg -nP --type=go 'go dvNode|go executor|dvNode\.Start|executor\.' node/cmd/node/main.go
# Check syncer methods to see if they have internal synchronization
rg -nP --type=go 'func.*Syncer.*\(' node/sync/syncer.go | head -20
# Check Syncer's actual method implementations for locks/synchronization
rg -nP --type=go 'sync\.Mutex|sync\.RWMutex|\bLock\(\)|\bUnlock\(' node/sync/
# Look at how derivation uses syncer
rg -nP --type=go 'syncer\.' node/derivation/derivation.go | head -20Repository: morph-l2/morph
Length of output: 678
🏁 Script executed:
# Check Syncer.Start() implementation to see if it uses goroutines
rg -nP --type=go -A 20 'func.*Start.*\{' node/sync/syncer.go | grep -A 20 'func (s \*Syncer) Start'
# Check fetchL1Messages for goroutines
rg -nP --type=go -A 15 'func.*fetchL1Messages' node/sync/syncer.go
# Check how latestSynced is modified
rg -nP --type=go 'latestSynced\s*=' node/sync/syncer.go
# Check executor's use of syncer
rg -nP --type=go -B 3 -A 10 'newSyncerFunc\|syncer\.' node/core/ | head -40Repository: morph-l2/morph
Length of output: 1322
🏁 Script executed:
# Check if executor actually uses syncer after initialization
rg -nP --type=go -B 5 -A 15 'type Executor' node/core/executor.go | head -50
# Search for syncer usage in executor
rg -nP --type=go 'syncer\.' node/core/executor.go | head -20
# Check what NewExecutor does with the syncer function
rg -nP --type=go -A 30 'func NewExecutor' node/core/executor.go | head -50Repository: morph-l2/morph
Length of output: 1704
🏁 Script executed:
# Verify when syncer is started
rg -nP --type=go 'syncer.*Start\|syncer.*New' node/cmd/node/main.go
# Check the full flow: when is syncer created vs when is dvNode started
sed -n '77,151p' node/cmd/node/main.go
# Check if derivation.Start() is async (goroutine)
rg -nP --type=go -B 2 -A 10 'func.*Derivation.*Start' node/derivation/derivation.go | head -30Repository: morph-l2/morph
Length of output: 3244
🏁 Script executed:
# Check if executor calls newSyncerFunc or uses syncer
rg -nP --type=go 'newSyncerFunc\|e\.syncer' node/core/executor.go
# Check the full Start method of Derivation to confirm syncer.Start() timing
rg -nP --type=go -A 30 'func.*Derivation.*Start' node/derivation/derivation.goRepository: morph-l2/morph
Length of output: 875
🏁 Script executed:
# Check for atomic operations on latestSynced
rg -nP --type=go 'atomic\.' node/sync/syncer.go
# Check if there are any channels or other sync mechanisms
rg -nP --type=go 'chan\|sync\.' node/sync/syncer.go | grep -v 'package\|import'
# Verify latestSynced access patterns
rg -nP --type=go 'latestSynced' node/sync/syncer.go
# Check derivation's use of syncer - does it call LatestSynced or ReadL1MessagesInRange?
rg -nP --type=go 's\.latestSynced\|LatestSynced\|ReadL1MessagesInRange' node/derivation/derivation.goRepository: morph-l2/morph
Length of output: 733
🏁 Script executed:
# Check if there are any other synchronization mechanisms (channels, etc)
cat node/sync/syncer.go | head -100
# Check how ReadL1MessagesInRange accesses latestSynced
rg -nP --type=go -B 5 -A 10 'ReadL1MessagesInRange' node/sync/syncer.goRepository: morph-l2/morph
Length of output: 3258
Syncer requires synchronization for concurrent access to latestSynced field.
The sync.Syncer struct shares the latestSynced field (uint64) between the background polling goroutine spawned by Start() and external readers (e.g., LatestSynced()). The fetchL1Messages() method reads and writes this field without any mutex or atomic protection, creating a data race when the syncer is concurrently accessed by the derivation client.
The latestSynced field is read at line 131 (loop initialization) and lines 136-137 (logging), and written at line 167, all within fetchL1Messages() which runs in a background goroutine. This unsynchronized concurrent access can cause memory corruption or unpredictable behavior.
Add sync.Mutex to protect latestSynced or use sync/atomic for the uint64 operations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@node/cmd/node/main.go` around lines 122 - 129, The Syncer has unsynchronized
concurrent reads/writes to the uint64 field latestSynced (accessed by
fetchL1Messages(), the background goroutine started in Start(), and readers via
LatestSynced()), causing a data race; fix by protecting all accesses to
latestSynced: either add a sync.Mutex (e.g., s.mu) and wrap every read/write in
fetchL1Messages() and LatestSynced() with s.mu.Lock()/Unlock(), or convert
latestSynced to a uint64 and use sync/atomic functions
(atomic.LoadUint64/atomic.StoreUint64) in fetchL1Messages(), Start() and
LatestSynced() so all reads/writes are atomic and race-free. Ensure you update
any logging that reads latestSynced to use the protected/atomic read.
The shared *Syncer is wired through node/cmd/node/main.go to two
independent start sites:
Path 1: Derivation.Start spawns a goroutine that calls d.syncer.Start
(node/derivation/derivation.go:166, since the very first commit).
Path 2: Executor.updateSequencerSet calls e.syncer.Start (or `go
e.syncer.Start`) on the !isSequencer -> isSequencer transition
(node/core/sequencers.go:86,88). On a real Tendermint sequencer
node, e.isSequencer starts at false (zero value), so the first
updateSequencerSet call always fires this branch -- producing
a second Start on the syncer Derivation already started.
The previous Start was not idempotent: it (a) re-ran the blocking
initial fetchL1Messages, (b) spawned a duplicate poller racing on
s.latestSynced and s.metrics, and (c) double-closed s.stop on shutdown
(panic: close of closed channel). On verifier-only nodes only Path 1
fires, masking the issue; the mock-sequencer devnet path avoids
Tendermint and also masks it; real Tendermint sequencer nodes hit the
race.
This bug predates SPEC-005 (Path 1 was added in 7f2f887, the initial
commit, 2024-01-12), but the SPEC-005 PR consolidates how derivation
drives shared services and is the natural place to retire the
duplicate-load path. Wrapping Start in s.startOnce.Do() keeps both call
sites correct without changing ownership semantics: the second caller
returns immediately, the first poller continues, shutdown closes
s.stop exactly once.
go build ./node/sync/ ./node/derivation/ ./node/core/ -- clean.
go test ./node/derivation/... -- 22 tests PASS.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
Implements the three targets of morph-l2/morph-specs SPEC-005 (tracking issue):
c82a1456): Deletenode/validator/plus all call sites / flags / docker-compose entries / monitoring panels.9f01c530): Add--derivation.verify-mode={pathA,pathB}(defaultpathA), bound at startup and not switchable at runtime. Path B does not pull blobs from beacon — it rebuilds versioned hashes locally from L2 blocks using the sequencer-consistent encoding and compares them against the values recorded on L1.a73639de): Delete the entirenode/blocktag/package; the safe / finalized heads are advanced solely by the derivation main loop + the in-process finalizer subcomponent; thefinalized ≤ safe ≤ unsafeinvariant is explicitly asserted; theSetBlockTagsengine RPC signature is unchanged.Design doc: tech-design.md
Base dependency
Base =
feat/multi_batch(PR #935). Reusescommon/batch.{ParsingTxs, BuildBlockContext, BatchData}+common/blob.{CompressBatchBytes, MakeBlobTxSidecar}. This PR will need a rebase once #935 lands onmain.Breaking change
The external semantics of
eth_getBlockByNumber("safe")change from "L1 N confirmations" → "locally derivation-verified". The driver offinalizedmoves from blocktag to the derivation-internal finalizer. Downstream consumers (wallets / indexers / bridge relayers) must be informed via changelog; the morph repository itself has 0 business call sites (only EL protocol-layer references), so external consumers will be notified separately. See tech-design §6 / §7.Test coverage
static_scan_test.go(3):TestNoValidatorReferences/TestNoBlocktagReferences/TestPathBUsesCommonBlobPackagetag_advance_test.go(7): safe / dedup / invariant ×2 / monotonic / l1-reorg-reset / block-number-errorTestVerifierDispatch_*(3) /TestPathA_*(2) /TestPathB_*(4) /TestTagAdvance_FinalizedFromL1/TestTagAdvance_PathBVsPathAParityDevnet verification
Pending (7 scenarios listed in spec §5.2, including an L1 reorg triggering a one-shot safe rewind while finalized stays put).
Rollback
Hard revert via
git revert+ redeploy (no feature-flag dual-run window; rationale in tech-design §7).Code review
@coderabbitai reviewto force a run, or wait until feat: multi blob #935 merges and this PR is rebased ontomain.BlockNumberinfinalizer.tick" have been applied).Refs
Summary by CodeRabbit
New Features
Bug Fixes & Updates
Configuration
derivation.verify-modeandderivation.finalizer-intervalEXECUTION_CLIENTselection with Reth build parameters