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

[nebojsa94]

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_simulateV1 → simulator.processBlock → FinalizeAndAssemble → CommitStates 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 (FetchAndCommitSpan → CommitSpan, and CommitStates → CommitState).

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 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]

codegenie review

[github-actions]

🧞 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.

Coverage

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

⚠️ Findings

🔵 Medium: systemTxVMConfig nils the tracer when Finalize is given a plain *state.StateDB (parallel processor path)

File: consensus/bor/bor.go:1784 ↗
Confidence: medium

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.

🙋 Needs Human Attention

• Are there tests asserting that system-transaction tracing is preserved on canonical import and suppressed for FinalizeAndAssemble/eth_simulateV1?
  • Files: consensus/bor/bor_test.go
  • Symbols: TestCommitStates_WithIndore, TestFinalize_SprintBlockWithoutHeimdall, systemTxVMConfig
  • Reason: Packet reviewer could not resolve this question from the reviewed context.

Stats

• 🤖 Model: anthropic claude-opus-5 high
• 🧞 Codegenie: v0.5.5 (58f82a9b2c)
• Elapsed time: 2m 48s
• Git: 0xPolygon/bor from master to fix/system-tx-live-tracer-leak (47d243e54e)
• Posting: 1 inline
• Review completeness: complete.
• Usage: model calls 30, tokens 553955, cost $2.5861.
• Effective caps: tokens 8000000.
• Local context pressure: 4 degraded tool results, 2 degraded hunks.

— View Workflow Job

[Copilot]

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]

🧞 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

[github-actions]

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.