Skip to content

consensus/bor: don't leak the live tracer into non-import system transactions - #2353

Open
nebojsa94 wants to merge 3 commits into
0xPolygon:masterfrom
nebojsa94:fix/system-tx-live-tracer-leak
Open

consensus/bor: don't leak the live tracer into non-import system transactions#2353
nebojsa94 wants to merge 3 commits into
0xPolygon:masterfrom
nebojsa94:fix/system-tx-live-tracer-leak

Conversation

@nebojsa94

Copy link
Copy Markdown
Contributor

Problem

Bor system transactions (span commits and state-sync events) are always executed with c.vmConfig — the vm.Config the consensus engine captured at startup. When a live tracer is configured (--vmtrace), that config carries the node-wide singleton tracer hooks, so every caller of Finalize / FinalizeAndAssemble fires them, not just canonical block import:

  • eth_simulateV1simulator.processBlockFinalizeAndAssembleCommitStates runs on an RPC goroutine whenever the simulated block number is a sprint start.
  • debug_trace* historical state regeneration replays ancestor blocks via StateProcessor.Process(..., vm.Config{}, ...)Finalize, which still commits system transactions with the live tracer despite the caller's empty config.

Live tracer hooks are stateful and single-threaded by contract (they are invoked serially during import — see also the concurrency note on WrapStateSyncHooks). Invoking them concurrently from RPC goroutines corrupts the tracer and the tracing journal.

Observed in production (bor v2.9.0, Polygon mainnet full nodes serving eth_simulateV1 traffic with a live tracer): within milliseconds at a sprint-start block,

  1. the simulate goroutine panics in the shared tracing journal — RPC method eth_simulateV1 crashed: runtime error: slice bounds out of range [:-1] in journal.popRevision (recovered by the RPC layer, leaving the shared tracer corrupted), then
  2. the import goroutine panics inside the live tracer on the corrupted state and takes the node down.

All observed crash blocks were sprint starts (number % 16 == 0), matching the IsSprintStart gate in front of checkAndCommitSpan/CommitStates. Identically configured nodes receiving no eth_simulateV1 traffic never crashed.

Fix

Derive the system-transaction tracer from the state being mutated instead of c.vmConfig:

  • Canonical import passes a hooked state (state.NewHookedState) when a live tracer is configured — system transactions there keep being traced with the same hooks as the rest of the block.
  • Plain states — the miner and eth_simulateV1 via FinalizeAndAssemble (which takes a concrete *state.StateDB), and historical state regeneration via Finalize — now run system transactions untraced, matching the (empty) config those callers execute the rest of the block with.

This adds an exported Hooks() accessor on state.hookedStateDB and a systemTxVMConfig helper on Bor, used at the two c.vmConfig call sites (FetchAndCommitSpanCommitSpan, and CommitStatesCommitState).

Testing

  • TestSystemTxVMConfig verifies the tracer is stripped for plain states (with the rest of the config preserved) and taken from the hooked state during traced import.
  • go test ./consensus/bor/ ./core/state/ passes.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@pratikspatil024

Copy link
Copy Markdown
Member

codegenie review

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown

🧞 Codegenie Review

✅ No credible findings.

Coverage

Reviewed 9/9 hunks.
Coverage levels: deep 4, normal 5, light 0, skip 0.

Stats

  • 🤖 Model: anthropic claude-opus-5 high
  • 🧞 Codegenie: v0.5.5 (58f82a9b2c)
  • Elapsed time: 3m 41s
  • Git: 0xPolygon/bor from master to fix/system-tx-live-tracer-leak (88f0286658)
  • Review completeness: complete.
  • Usage: model calls 34, tokens 773212, cost $3.3430.
  • Effective caps: tokens 8000000.
  • Local context pressure: 2 tool-budget rejections, 8 degraded tool results, 5 degraded hunks.

✅ No Findings

No credible findings were found. Everything looks good.

View Workflow Job

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a concurrency/crash hazard where Bor system transactions (span commits and state-sync events) could inadvertently use the node-wide live tracer (--vmtrace) in non-import contexts (e.g., eth_simulateV1, historical state regeneration), by deriving the tracer from the state being mutated rather than c.vmConfig.

Changes:

  • Add a Hooks() accessor on hooked state wrappers so callers can detect tracing hooks from the state itself.
  • Introduce Bor.systemTxVMConfig(state) and use it for system-transaction execution to prevent leaking the live tracer into plain-state callers.
  • Add a focused unit test validating tracer stripping/preservation behavior for plain vs hooked states.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
core/state/statedb_hooked.go Exposes tracing hooks from hookedStateDB via Hooks() for downstream detection.
consensus/bor/bor.go Routes system tx execution through systemTxVMConfig(state) so tracer selection depends on the provided state wrapper.
consensus/bor/vmconfig_test.go Adds TestSystemTxVMConfig to ensure tracers are only used when the state itself is hooked.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧞 Codegenie Review

Reviewed all 6 hunks (1 deep, 5 normal); no hunks skipped or failed. One verified issue: the new (*Bor).systemTxVMConfig derives the tracer solely from the passed-in state, so the parallel state processor path — which hands Finalize an unwrapped *state.StateDB — loses live-tracer coverage for bor system transactions. This is an intentional contract change per the PR/commit body, but the parallel-path consequence looks broader than intended and needs caller confirmation. Also worth noting for the author: the only added test is consensus/bor/vmconfig_test.go (TestSystemTxVMConfig), which exercises the helper in isolation; there is no test asserting that system-transaction tracing is preserved on canonical import and suppressed for FinalizeAndAssemble/eth_simulateV1.

Reviewed 6/6 hunks.
Coverage levels: deep 1, normal 5, light 0, skip 0.

🙋 Needs human attention:

  • Are there tests asserting that system-transaction tracing is preserved on canonical import and suppressed for FinalizeAndAssemble/eth_simulateV1?

— codegenie v0.5.5 (58f82a9b2c) · View Workflow Job

Comment thread consensus/bor/bor.go
}

return c.spanner.CommitSpan(ctx, minSpan, validators, producers, state, header, chain, c.vmConfig)
return c.spanner.CommitSpan(ctx, minSpan, validators, producers, state, header, chain, c.systemTxVMConfig(state))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

systemTxVMConfig sets cfg.Tracer = nil whenever the passed-in state does not implement stateTracingHooks, and the parallel state processor passes an unwrapped *state.StateDB to Finalize. With --vmtrace enabled, bor span-commit and state-sync system transactions therefore execute with no tracer on the parallel execution path.

Impact: Observability only — no state, receipt, or consensus output changes — but live-tracer streams lose bor system transactions on the parallel-processor path with no error or warning, making the gap data-dependent and hard to detect downstream by consumers building external indexes/state feeds.

Evidence — the helper drops the tracer rather than falling back to the engine config:

// consensus/bor/bor.go:1651-1660
func (c *Bor) systemTxVMConfig(state vm.StateDB) vm.Config {
	cfg := c.vmConfig
	if hooked, ok := state.(stateTracingHooks); ok {
 cfg.Tracer = hooked.Hooks()
	} else {
 cfg.Tracer = nil
	}

	return cfg
}

Only *hookedStateDB satisfies stateTracingHooks (core/state/statedb_hooked.go:314), and the two import paths differ:

// core/state_processor.go — serial path wraps the state, tracing preserved
var tracingStateDB = vm.StateDB(statedb)
if hooks := cfg.Tracer; hooks != nil {
	tracingStateDB = state.NewHookedState(statedb, hooks)
}
receipts, err = p.chain.Engine().Finalize(p.chain, header, tracingStateDB, block.Body(), receipts)
// core/parallel_state_processor.go:431 — plain statedb, else-branch taken, tracer nil
receipts, err = p.chain.Engine().Finalize(p.bc.hc, header, statedb, block.Body(), receipts)

The PR and commit bodies declare an intentional change away from c.vmConfig for system transactions, so the contract change itself is deliberate; what needs confirmation is whether dropping tracing on the parallel import path is also intended, since canonical import was meant to keep tracer coverage.

Suggested fix: either wrap the state in core/parallel_state_processor.go before calling Finalize, mirroring the serial path's state.NewHookedState(statedb, hooks), or fall back to the engine-configured tracer instead of nil:

cfg := c.vmConfig
if hooked, ok := state.(stateTracingHooks); ok {
	cfg.Tracer = hooked.Hooks()
}
// otherwise keep cfg.Tracer from c.vmConfig
return cfg

Suggested test: import a block through the parallel state processor with a live tracer configured and assert that OnTxStart/OnEnter fire for the bor span-commit and state-sync system transactions.

Relatedly, the added consensus/bor/vmconfig_test.go (TestSystemTxVMConfig) only covers the helper in isolation; consider adding coverage asserting that system-transaction tracing is preserved on canonical import and suppressed for FinalizeAndAssemble/eth_simulateV1.

@pratikspatil024

Copy link
Copy Markdown
Member

please resolve the conflicts and address the comments

…sactions

Bor system transactions (span commits and state-sync events) were always
executed with c.vmConfig, the vm.Config captured by the consensus engine at
startup. When a live tracer is configured (--vmtrace), that config carries the
node-wide singleton tracer hooks, so every caller of Finalize /
FinalizeAndAssemble fired them — not just canonical block import:

  * eth_simulateV1 -> simulator.processBlock -> FinalizeAndAssemble ->
    CommitStates runs on an RPC goroutine whenever the simulated block number
    is a sprint start, and
  * debug_trace* historical state regeneration replays ancestor blocks via
    StateProcessor.Process(vm.Config{}) -> Finalize, which still committed
    system transactions with the live tracer despite the empty config.

Live tracer hooks are stateful and single-threaded by contract (hooks are
invoked serially during import). Invoking them concurrently from RPC
goroutines corrupts the tracer and the tracing journal, producing
'slice bounds out of range' panics in journal.popRevision (recovered by the
RPC layer) and corrupted-state panics on the import goroutine that crash the
node. Observed in production on nodes serving eth_simulateV1 traffic with a
live tracer enabled: both effects fired within milliseconds at sprint-start
blocks.

Derive the system-transaction vm.Config tracer from the state being mutated
instead: canonical import passes a hooked state (state.NewHookedState) when a
live tracer is configured, so system transactions there keep being traced with
the same hooks. Plain states — miner and eth_simulateV1 via
FinalizeAndAssemble, state regeneration via Finalize — now run system
transactions untraced.
…l path, add behavioral test

The parallel state processor handed Finalize an unwrapped *state.StateDB, so
with the new state-derived tracer selection, bor system transactions would run
untraced on that path even when the caller configured a tracer. Mirror the
serial processor: wrap the state with state.NewHookedState when cfg.Tracer is
set, and fire the state-sync transaction's OnTxStart/OnTxEnd window around
Finalize so state-sync events are traced inside a well-formed tx scope.
(Live tracing and ParallelEVM are currently mutually exclusive at startup, so
this is parity/future-proofing rather than a behavior change today.)

Also add TestCommitStates_SystemTxTracer, exercising CommitStates end to end:
system transactions receive the hooks tracing the state they mutate on
canonical import, and no tracer for plain states — never the node-wide live
tracer stored in c.vmConfig.
@nebojsa94
nebojsa94 force-pushed the fix/system-tx-live-tracer-leak branch from 47d243e to 7cd8fe5 Compare August 17, 2026 12:19
@nebojsa94

Copy link
Copy Markdown
Contributor Author

Rebased onto current master (v2.10.0) and addressed the review:

Codegenie finding (parallel processor loses system-tx tracing): fixed by mirroring the serial processor rather than falling back to c.vmConfig (a fallback would reintroduce the leak for FinalizeAndAssemble / state regeneration, which also pass plain states). ParallelStateProcessor.Process now wraps the state with state.NewHookedState when cfg.Tracer is set before calling Finalize, and fires the state-sync transaction's OnTxStart/OnTxEnd window around it so state-sync events are traced inside a well-formed tx scope — same as the serial path. Note live tracing and ParallelEVM are currently mutually exclusive at startup (eth/backend.go refuses --vmtrace with ParallelEVM enabled), so this is parity/future-proofing rather than a behavior change today.

Test coverage: added TestCommitStates_SystemTxTracer, which exercises CommitStates end to end with a capturing GenesisContract: with a node-wide tracer set in c.vmConfig, system transactions applied over a plain state receive no tracer (the FinalizeAndAssemble/eth_simulateV1/regeneration case, with the rest of the config preserved), and over a hooked state they receive exactly the hooks tracing that state (the canonical-import case). The existing TestSystemTxVMConfig keeps covering the helper in isolation.

go test ./consensus/bor/ ./core/state/ ./core/ all pass locally.

@pratikspatil024

Copy link
Copy Markdown
Member

@claude review

@pratikspatil024

Copy link
Copy Markdown
Member

codegenie review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧞 Codegenie Review

Reviewed all 8 hunks (6 deep, 2 normal); no verification gaps. The core change — deriving the system-tx vm.Config from the state handed to Finalize instead of the engine-captured c.vmConfig — is sound and matches the stated intent, and the state-sync path is pinned by a new behavioral test. Two low-severity issues remain: the span-commit sink of the new state-derived config has no test assertion because fakeSpanner.CommitSpan discards its vm.Config argument, and on the parallel path a recovered panic runs the newly added OnTxEnd defer with (nil, nil), emitting a success-looking state-sync trace for a block that failed to process.

Reviewed 8/8 hunks.
Coverage levels: deep 6, normal 2, light 0, skip 0.

— codegenie v0.5.5 (58f82a9b2c) · View Workflow Job

// for plain states (miner and eth_simulateV1 via FinalizeAndAssemble,
// historical state regeneration via Finalize) — never the node-wide live
// tracer stored in c.vmConfig.
func TestCommitStates_SystemTxTracer(t *testing.T) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The span-commit sink of the new state-derived vm.Config has no test assertion. TestCommitStates_SystemTxTracer only captures and asserts the state-sync (CommitState) sink; the only spanner double discards the config it receives:

// consensus/bor/bor_test.go:72
func (s *fakeSpanner) CommitSpan(ctx context.Context, _ borTypes.Span, _ []stakeTypes.MinimalVal, _ []stakeTypes.MinimalVal, _ vm.StateDB, _ *types.Header, _ core.ChainContext, _ vm.Config) error {
	if s.shouldFailCommit {
 return errors.New("span commit failed")

The changed call site is behavior-bearing — base consensus/bor/bor.go:1747 passed c.vmConfig, head consensus/bor/bor.go:1775 passes the state-derived config:

return c.spanner.CommitSpan(ctx, minSpan, validators, producers, state, header, chain, c.systemTxVMConfig(state))

A repo-wide search over consensus/bor found no assertion on the vm.Config given to CommitSpan.

Impact: No incorrect behavior today — production wiring is correct and systemTxVMConfig is unit-tested directly. But a future edit or bad merge restoring c.vmConfig at consensus/bor/bor.go:1775, or a state wrapper that stops implementing stateTracingHooks on the span path, would keep the whole consensus/bor suite green while re-introducing exactly the defect this PR fixes: the node-wide live tracer being invoked for span-commit system txs on plain states from the miner, eth_simulateV1 (via FinalizeAndAssemble) and historical regeneration (via Finalize). This PR establishes that both system-tx sinks derive their tracer from the state they mutate; only one half is pinned.

Suggested fix: Give fakeSpanner a capturedVMConfigs []vm.Config field, append the argument in CommitSpan, and add a sibling test:

func TestCommitSpan_SystemTxTracer(t *testing.T) {
	b.vmConfig = vm.Config{Tracer: &tracing.Hooks{}, NoBaseFee: true}
	// (a) plain state: tracer dropped, other fields preserved
	_ = b.FetchAndCommitSpan(/* pre-Rio header */)
	require.Nil(t, sp.capturedVMConfigs[0].Tracer)
	require.True(t, sp.capturedVMConfigs[0].NoBaseFee)
	// (b) hooked state: tracer taken from the state
	_ = b.FetchAndCommitSpan(/* with state.NewHookedState(plain, importHooks) */)
	require.Same(t, importHooks, sp.capturedVMConfigs[0].Tracer)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining for now to keep the PR focused: both system-tx sinks route through the single systemTxVMConfig helper, whose plain-state and hooked-state behavior is pinned directly by TestSystemTxVMConfig, and the state-sync sink is additionally covered end-to-end by TestCommitStates_SystemTxTracer. A call-site regression at CommitSpan would have to bypass the helper entirely, which review would catch on a one-line diff. If maintainers would like the span-side capture as an extra pin, I'm happy to add it here or in a follow-up.

if hooks := cfg.Tracer; hooks != nil && hasStateSyncTx && hooks.OnTxStart != nil && hooks.OnTxEnd != nil {
hooks.OnTxStart(vmenv.GetVMContext(), txs[len(txs)-1], params.BorSystemAddress)
defer func() {
hooks.OnTxEnd(stateSyncReceipt, stateSyncEndErr)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the parallel path, a recovered panic fires the newly added OnTxEnd(nil, nil), reporting the synthetic state-sync tx as successfully ended while Process returns an error.

The new hook scope in core/parallel_state_processor.go:

if hooks := cfg.Tracer; hooks != nil && hasStateSyncTx && hooks.OnTxStart != nil && hooks.OnTxEnd != nil {
	hooks.OnTxStart(vmenv.GetVMContext(), txs[len(txs)-1], params.BorSystemAddress)
	defer func() {
 hooks.OnTxEnd(stateSyncReceipt, stateSyncEndErr)
	}()
}

is registered at head lines 425-427, after the function-level recover defer at lines 265-272:

func (p *ParallelStateProcessor) Process(...) (processResult *ProcessResult, err error) {
	defer func() {
 if r := recover(); r != nil {
 log.Error("recovered from panic during parallel execution", "err", r)
 processResult = nil
 err = fmt.Errorf("panic during parallel execution: %v", r)
 }
	}()

Defers run LIFO, so on a panic inside p.chain.Engine().Finalize (bor CommitSpan/CommitStates) or the receipt/log handling that follows, OnTxEnd runs first with the named locals stateSyncReceipt and stateSyncEndErr still nil, and only then does recover set err. The explicit error returns in the same hunk (lines 434-437 and 443-445) do set stateSyncEndErr, so the recovered-panic window is the only path that emits OnTxEnd(nil, nil). The serial processor being mirrored in core/state_processor.go has no recover, so it does not have this window.

Impact: Bounded to tracer output — no state, receipt, or consensus value is affected. Any consumer reconstructing tx outcomes from OnTxStart/OnTxEnd pairs records a successful state-sync transaction for a block that never completed, and live/streaming tracers emit that trace. This is a new-behavior hunk (the parallel path now hands Finalize a hooked state so bor system transactions are traced); please confirm the intended hook semantics on the recovered-panic path.

Suggested fix: Make the panic case observable to the hook:

completed := false
if hooks := cfg.Tracer; hooks != nil && hasStateSyncTx && hooks.OnTxStart != nil && hooks.OnTxEnd != nil {
	hooks.OnTxStart(vmenv.GetVMContext(), txs[len(txs)-1], params.BorSystemAddress)
	defer func() {
 if !completed && stateSyncEndErr == nil {
 stateSyncEndErr = errors.New("panic during parallel execution")
 }
 hooks.OnTxEnd(stateSyncReceipt, stateSyncEndErr)
	}()
}

with completed = true set immediately before the successful return. Alternatively, register the OnTxEnd defer before the recover defer so recover can populate stateSyncEndErr first.

Suggested test: Run ParallelStateProcessor.Process on a post-Madhugiri block whose last tx is a state-sync tx, with a stub tracing.Hooks recording OnTxStart/OnTxEnd and an engine whose Finalize panics; assert Process returns an error and the recorded OnTxEnd carries a non-nil error rather than (nil, nil).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining this one deliberately: the deferred OnTxEnd firing with a nil receipt during panic unwinding is the established hook contract in this codebase — ApplyTransactionWithEVM's deferred hooks.OnTxEnd(receipt, err) behaves identically on the serial path, so tracers must already tolerate nil receipts. Special-casing the parallel copy with a completed flag would make it diverge from the serial block it mirrors (the drift concern raised in the other thread), for a path that is unreachable today (eth/backend.go refuses --vmtrace with ParallelEVM enabled) and where the process already surfaces the failure as an error. If the hook semantics on panic paths should change, that's better done consistently for both processors in a follow-up.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This automated review found only minor nits (both noted as latent/non-behavioral given ParallelEVM and live tracing are mutually exclusive today). No functional or security-impacting issues were found in the tracer-routing fix itself.

Beyond the inline findings, I also checked whether Finalize panicking would make the deferred OnTxEnd fire with misleading nil-success args ahead of the outer panic recovery in ParallelStateProcessor.Process. It does (the recover-defer is registered before the OnTxEnd-defer, so LIFO ordering runs OnTxEnd(nil, nil) first), but since the process then aborts and returns an error/panic to the caller, this doesn't lead to any consensus or state impact — a trace-only cosmetic gap in an already-exceptional path.

Extended reasoning...

Verified the panic-ordering concern from the ruled-out candidates list directly against the code: the outer defer recover() in ParallelStateProcessor.Process is registered near the top of the function (~line 266), while the OnTxEnd defer is registered later, right before the Finalize call (~line 424). Go runs deferred functions LIFO, so if Finalize panics, the OnTxEnd defer fires first with stateSyncReceipt/stateSyncEndErr still at their zero values, before the outer recover converts the panic into a returned error. This confirms the candidate issue is real as a mechanical fact, but it is inert in practice: a panic here already means the whole block-import attempt is aborted and surfaced as an error, so a single spurious nil/nil tracer callback in an already-fatal codepath has no consensus or state consequence, and matches why the verifiers ruled it out as not a real bug.

Comment thread core/parallel_state_processor.go Outdated
Comment on lines +407 to +428
// Mirror the serial processor: when the caller configured a tracer, hand
// Finalize a hooked state so bor system transactions (span commits and
// state-sync events) are traced with the caller's hooks.
var tracingStateDB = vm.StateDB(statedb)
if hooks := cfg.Tracer; hooks != nil {
tracingStateDB = state.NewHookedState(statedb, hooks)
}

// State-sync transactions (post-Madhugiri) are the last tx in body. In order to produce
// accurate traces for state-sync transactions, we fire the OnTxStart and OnTxEnd hooks
// here before calling Finalize, mirroring the serial processor.
var (
hasStateSyncTx = len(txs) > 0 && txs[len(txs)-1].Type() == types.StateSyncTxType
stateSyncReceipt *types.Receipt
stateSyncEndErr error
)
if hooks := cfg.Tracer; hooks != nil && hasStateSyncTx && hooks.OnTxStart != nil && hooks.OnTxEnd != nil {
hooks.OnTxStart(vmenv.GetVMContext(), txs[len(txs)-1], params.BorSystemAddress)
defer func() {
hooks.OnTxEnd(stateSyncReceipt, stateSyncEndErr)
}()
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 core/parallel_state_processor.go:407-428 duplicates the tracingStateDB construction and the hasStateSyncTx/OnTxStart+deferred-OnTxEnd trace-scope wiring that core/state_processor.go:92-171 already implements near-verbatim (added earlier on this same PR branch). This is a real maintenance hazard — see bug_001's vmenv/tracingStateDB divergence, which is exactly the kind of drift this duplication invites — and would be fixed by extracting a small shared helper (e.g. openStateSyncTxTraceScope(cfg, tracingStateDB, txs, vmCtx) returning a finish func) used by both Process implementations.

Extended reasoning...

What the bug is: core/parallel_state_processor.go (lines 407-428) duplicates a chunk of logic that was added to core/state_processor.go (lines 92-96 and 156-171) earlier in this same PR branch: constructing tracingStateDB from the caller's cfg.Tracer, computing hasStateSyncTx, declaring stateSyncReceipt/stateSyncEndErr, and wiring the OnTxStart/deferred-OnTxEnd scope around the synthetic state-sync transaction before calling Finalize. The two blocks are near copy-paste of each other, differing only in incidental details (e.g. evm.GetVMContext() on the serial path vs vmenv.GetVMContext() on the parallel path).

Where it manifests: Anyone touching this trace-scope logic — e.g. fixing how the synthetic state-sync tx's VM context is derived, or changing when OnTxEnd fires relative to Finalize errors — has to find and edit both core/state_processor.go and core/parallel_state_processor.go and keep them byte-for-byte equivalent by hand. There is no test asserting parity between the two copies, so a partial fix silently leaves the other path behind.

Why this isn't just theoretical: it already happened. bug_001 (filed separately against this same PR) is exactly this class of drift: the parallel path builds the OnTxStart VM context from vmenv (built over the plain statedb) while intending to trace over tracingStateDB (the hooked wrapper), whereas the serial path derives its equivalent EVM context consistently from the traced state. That divergence crept in during this very duplication.

Why existing code doesn't prevent it: both Process implementations live in package core and take structurally similar inputs (cfg vm.Config, the state, the block's transactions, a VM context), so there's no technical barrier to sharing this logic — it just wasn't factored out when it was copied over during the same branch's development.

Impact: Purely a maintainability/code-quality concern — no incorrect behavior is introduced by the duplication itself (the concrete divergence it enabled is tracked separately as bug_001). It does raise the ongoing cost of the state-sync tracing feature: every future change to this scope-opening logic is a two-file, hand-synced edit with no safety net.

Suggested fix: extract a small shared helper in package core, e.g. func openStateSyncTxTraceScope(cfg vm.Config, tracingStateDB vm.StateDB, txs types.Transactions, vmCtx *tracing.VMContext) (finish func(*types.Receipt, error)), and call it from both StateProcessor.Process and ParallelStateProcessor.Process. A single fix (including the bug_001 vmenv/tracingStateDB fix) would then apply to both execution paths simultaneously, and the helper becomes a natural place to add a unit test asserting the two paths behave identically.

Proof of the duplication (step by step):

  1. Read core/state_processor.go:92-96: it builds tracingStateDB from statedb and cfg.Tracer.
  2. Read core/state_processor.go:156-171: it declares hasStateSyncTx/stateSyncReceipt/stateSyncEndErr and wires OnTxStart + a deferred OnTxEnd guarded by the same hasStateSyncTx && hooks.OnTxStart != nil && hooks.OnTxEnd != nil condition.
  3. Read core/parallel_state_processor.go:407-428 (this PR's diff): the identical variable names, identical guard condition, and identical OnTxStart/deferred-OnTxEnd shape reappear, differing only in using vmenv.GetVMContext() instead of the serial path's traced-EVM context.
  4. Cross-reference bug_001: it independently found that this exact difference (which VM context/state backs the OnTxStart call) is inconsistent between the two copies — direct evidence that hand-syncing duplicated logic across these two files has already failed once on this branch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged, but I'd prefer to keep the extraction out of this PR: a shared scope helper would need to carry mutable receipt/error state across Finalize, which means restructuring the serial processor's flow — the consensus-critical path this fix PR deliberately avoids touching. With 88f0286 the parallel block is now a faithful mirror of the serial one (same wrap point, same EVM construction), which removes the drift that motivated this finding. Happy to do the openStateSyncTxTraceScope extraction as a focused follow-up PR if maintainers want it.

Comment thread core/parallel_state_processor.go
…state

Review follow-up: vmenv was constructed over the plain statedb while Finalize
received the hooked wrapper, so the state-sync OnTxStart hook reported a
VMContext.StateDB different from the state the traced system transactions
mutate. Construct the tracing wrapper first and build vmenv on it — this also
gives the pre-execution system calls (beacon root, EIP-2935) the same hooked
state the serial processor uses.
@nebojsa94

Copy link
Copy Markdown
Contributor Author

Addressed the latest review round in 88f0286: vmenv in the parallel processor is now built over the hooked state, so the state-sync OnTxStart's VMContext.StateDB matches the state Finalize mutates (and the pre-execution system calls get the same serial-path parity).

The three remaining suggestions I've declined with rationale in their threads, in short: the span-side test duplicates coverage the systemTxVMConfig unit test and the CommitStates end-to-end test already pin; the OnTxEnd-on-panic tweak would diverge from the serial processor's (and geth's) established nil-receipt hook semantics on a path that's unreachable today; and the shared-helper extraction would restructure the serial processor inside a fix PR — offered as a follow-up instead. Happy to revisit any of these if you disagree.

@pratikspatil024

Copy link
Copy Markdown
Member

codegenie review

@pratikspatil024

Copy link
Copy Markdown
Member

@claude review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-reviewed this PR after the latest changes. No new bugs were found this run. The previously flagged issue (parallel processor's vmenv built over the plain state instead of the hooked/tracing state) was fixed in 88f0286 and now mirrors the serial processor correctly.

What was reviewed: the systemTxVMConfig tracer-derivation helper and its two call sites (FetchAndCommitSpan/CommitSpan, CommitStates/CommitState); the parallel processor's hooked-state wiring for pre-execution system calls and the state-sync tx trace scope; and the new/updated tests in vmconfig_test.go. A candidate issue about regular (non-system) parallel tx tasks sharing a tracer singleton across goroutines was investigated and ruled out — eth/backend.go never constructs a live tracer when ParallelEVM is enabled (it only logs a warning and skips tracer creation), so cfg.Tracer is nil on that path in production and the concern is not currently reachable.

Given this PR touches consensus-critical code (consensus/bor, the parallel state processor's Finalize/tracing wiring) and has gone through several rounds of substantive fixes, a human review is still worthwhile before merging.

Extended reasoning...

No bugs were found by the bug hunting system this run. This is a re-review triggered by a new commit (88f0286) that addresses a previously flagged concern (the parallel processor's vmenv being built over the plain statedb instead of the hooked/tracing state used for Finalize), plus a fresh "@claude review" request.

Overview

The PR changes how the tracer is selected for bor system transactions (span commits, state-sync events): instead of always using the engine's startup vmConfig (which carries a node-wide singleton live tracer), it now derives the tracer from the state instance being mutated via a new systemTxVMConfig helper. Canonical import passes a hooked state so system txs keep being traced; plain states (miner, eth_simulateV1, historical regeneration) get no tracer. The parallel state processor was updated in this same PR to build its pre-execution system calls and Finalize call over a hooked state when a tracer is configured, mirroring the serial processor, and to fire OnTxStart/OnTxEnd around the state-sync transaction.

Security risks

No exploitable security risk: this affects observability (live tracer hook firing) only, not state transitions, receipts, or consensus output. The original production bug being fixed (concurrent invocation of a singleton live tracer corrupting itself and crashing the node) is a real availability concern, but the fix itself does not introduce new attack surface. The confirmed dead-code path (regular parallel tx tasks sharing a tracer instance across goroutines) is gated off by eth/backend.go refusing to construct a live tracer when ParallelEVM is enabled.

Level of scrutiny

This warrants continued human scrutiny: it touches consensus/bor/bor.go (system transaction application) and core/parallel_state_processor.go (block finalization on the parallel execution path), both consensus/production-critical. The PR has already gone through multiple review rounds (codegenie, human, and this bot) surfacing real ordering bugs that were fixed, which suggests the tracer-wiring plumbing here is subtle enough to warrant a final human pass, even though no further bugs surfaced this run.

Other factors

The three codegenie suggestions from the prior round (span-side test coverage, OnTxEnd-on-panic semantics, shared-helper extraction) were explicitly declined by the author with reasoning tied to keeping the fix PR focused and mirroring existing serial-processor/geth hook contracts; those are judgment calls for maintainers rather than correctness bugs. Test coverage for the helper and the CommitStates end-to-end path is solid; the CommitSpan sink remains untested per the author's own acknowledgment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants