-
Notifications
You must be signed in to change notification settings - Fork 602
consensus/bor: don't leak the live tracer into non-import system transactions #2353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
b16d7a5
7cd8fe5
88f0286
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| package bor | ||
|
|
||
| import ( | ||
| "math/big" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/ethereum/go-ethereum/common" | ||
| "github.com/ethereum/go-ethereum/consensus/bor/clerk" | ||
| "github.com/ethereum/go-ethereum/consensus/bor/statefull" | ||
| "github.com/ethereum/go-ethereum/consensus/bor/valset" | ||
| "github.com/ethereum/go-ethereum/core/state" | ||
| "github.com/ethereum/go-ethereum/core/tracing" | ||
| "github.com/ethereum/go-ethereum/core/types" | ||
| "github.com/ethereum/go-ethereum/core/vm" | ||
| ) | ||
|
|
||
| // TestSystemTxVMConfig verifies that system transactions (span commits and | ||
| // state-sync events) are traced only when the caller traces the state they | ||
| // are applied to. The node-wide live tracer stored in c.vmConfig must never | ||
| // leak into contexts that pass a plain state (miner and eth_simulateV1 via | ||
| // FinalizeAndAssemble, historical state regeneration via Finalize): those run | ||
| // outside the import goroutine, and invoking the singleton live tracer | ||
| // concurrently corrupts it. | ||
| func TestSystemTxVMConfig(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| liveTracer := &tracing.Hooks{ | ||
| OnEnter: func(depth int, typ byte, from, to common.Address, input []byte, gas uint64, value *big.Int) { | ||
| t.Error("live tracer must not be invoked for untraced states") | ||
| }, | ||
| } | ||
| c := &Bor{vmConfig: vm.Config{Tracer: liveTracer, NoBaseFee: true}} | ||
|
|
||
| plainState, err := state.New(types.EmptyRootHash, state.NewDatabaseForTesting()) | ||
| require.NoError(t, err) | ||
|
|
||
| // A plain state (regeneration, miner, eth_simulateV1) must not get the | ||
| // live tracer, but keeps the rest of the config. | ||
| cfg := c.systemTxVMConfig(plainState) | ||
| require.Nil(t, cfg.Tracer) | ||
| require.True(t, cfg.NoBaseFee) | ||
|
|
||
| // A hooked state (canonical import with a live tracer) keeps being traced | ||
| // with the hooks that trace the state itself. | ||
| importHooks := &tracing.Hooks{} | ||
| cfg = c.systemTxVMConfig(state.NewHookedState(plainState, importHooks)) | ||
| require.Same(t, importHooks, cfg.Tracer) | ||
| } | ||
|
|
||
| // capturingGenesisContract records the vm.Config each CommitState call receives. | ||
| type capturingGenesisContract struct { | ||
| lastStateID uint64 | ||
| captured []vm.Config | ||
| } | ||
|
|
||
| func (m *capturingGenesisContract) CommitState(event *clerk.EventRecordWithTime, state vm.StateDB, header *types.Header, chCtx statefull.ChainContext, vmCfg vm.Config) (uint64, error) { | ||
| m.captured = append(m.captured, vmCfg) | ||
| return 0, nil | ||
| } | ||
|
|
||
| func (m *capturingGenesisContract) LastStateId(st *state.StateDB, number uint64, hash common.Hash) (*big.Int, error) { | ||
| return big.NewInt(int64(m.lastStateID)), nil | ||
| } | ||
|
|
||
| // TestCommitStates_SystemTxTracer asserts end to end through CommitStates that | ||
| // state-sync system transactions receive the tracer of the state they mutate: | ||
| // the hooks tracing the state during canonical import, and no tracer at all | ||
| // 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) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The span-commit sink of the new state-derived // 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 return c.spanner.CommitSpan(ctx, minSpan, validators, producers, state, header, chain, c.systemTxVMConfig(state))A repo-wide search over Impact: No incorrect behavior today — production wiring is correct and Suggested fix: Give 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)
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| t.Parallel() | ||
|
|
||
| addr1 := common.HexToAddress("0x1") | ||
| sp := &fakeSpanner{vals: []*valset.Validator{{Address: addr1, VotingPower: 1}}} | ||
|
|
||
| now := time.Now() | ||
| chain, b := newChainAndBorForTest(t, sp, indoreBorConfig(), true, addr1, uint64(now.Unix())) | ||
|
|
||
| // The node-wide live tracer configured at startup: must never reach system txs. | ||
| b.vmConfig = vm.Config{Tracer: &tracing.Hooks{}, NoBaseFee: true} | ||
|
|
||
| gc := &capturingGenesisContract{lastStateID: 0} | ||
| b.GenesisContractsClient = gc | ||
| b.SetHeimdallClient(&mockHeimdallClient{ | ||
| events: []*clerk.EventRecordWithTime{ | ||
| { | ||
| EventRecord: clerk.EventRecord{ID: 1, Contract: common.HexToAddress("0x1001"), Data: []byte{0x01}, ChainID: "1"}, | ||
| Time: now.Add(-10 * time.Second), | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| genesis := chain.HeaderChain().GetHeaderByNumber(0) | ||
| header := &types.Header{Number: big.NewInt(16), ParentHash: genesis.Hash(), Time: uint64(now.Unix())} | ||
| cx := statefull.ChainContext{Chain: chain.HeaderChain(), Bor: b} | ||
|
|
||
| // Plain state (regeneration, miner, eth_simulateV1): no tracer, rest of the | ||
| // engine config preserved. | ||
| _, err := b.CommitStates(newStateDBForTest(t, genesis.Root), header, cx) | ||
| require.NoError(t, err) | ||
| require.Len(t, gc.captured, 1) | ||
| require.Nil(t, gc.captured[0].Tracer) | ||
| require.True(t, gc.captured[0].NoBaseFee) | ||
|
|
||
| // Hooked state (canonical import with a live tracer): traced with the hooks | ||
| // tracing the state itself. | ||
| gc.captured = nil | ||
| importHooks := &tracing.Hooks{} | ||
| _, err = b.CommitStates(state.NewHookedState(newStateDBForTest(t, genesis.Root), importHooks), header, cx) | ||
| require.NoError(t, err) | ||
| require.Len(t, gc.captured, 1) | ||
| require.Same(t, importHooks, gc.captured[0].Tracer) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -301,7 +301,17 @@ func (p *ParallelStateProcessor) Process(block *types.Block, statedb *state.Stat | |
|
|
||
| context := NewEVMBlockContext(header, p.bc.hc, author) | ||
|
|
||
| vmenv := vm.NewEVM(context, statedb, config, cfg) | ||
| // Mirror the serial processor: when the caller configured a tracer, run the | ||
| // pre-execution system calls and bor system transactions (span commits and | ||
| // state-sync events, via Finalize below) over a hooked state so they are | ||
| // traced with the caller's hooks and the tracer's VMContext reports the | ||
| // same state instance those calls mutate. | ||
| var tracingStateDB = vm.StateDB(statedb) | ||
| if hooks := cfg.Tracer; hooks != nil { | ||
| tracingStateDB = state.NewHookedState(statedb, hooks) | ||
| } | ||
|
|
||
| vmenv := vm.NewEVM(context, tracingStateDB, config, cfg) | ||
|
|
||
| if beaconRoot := block.BeaconRoot(); beaconRoot != nil { | ||
| ProcessBeaconBlockRoot(*beaconRoot, vmenv) | ||
|
|
@@ -404,11 +414,27 @@ func (p *ParallelStateProcessor) Process(block *types.Block, statedb *state.Stat | |
| // Polygon/bor: EIP-6110, EIP-7002, and EIP-7251 are not supported | ||
| var requests [][]byte | ||
|
|
||
| // 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) | ||
|
claude[bot] marked this conversation as resolved.
|
||
| defer func() { | ||
| hooks.OnTxEnd(stateSyncReceipt, stateSyncEndErr) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the parallel path, a recovered panic fires the newly added The new hook scope in 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 Impact: Bounded to tracer output — no state, receipt, or consensus value is affected. Any consumer reconstructing tx outcomes from 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 Suggested test: Run
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Declining this one deliberately: the deferred |
||
| }() | ||
| } | ||
|
|
||
| // Finalize the block, applying any consensus engine specific extras (e.g. block rewards), apply | ||
| // state sync event (if any), and append the receipt. | ||
| receiptsCountBeforeFinalize := len(receipts) | ||
| receipts, err = p.chain.Engine().Finalize(p.bc.hc, header, statedb, block.Body(), receipts) | ||
| receipts, err = p.chain.Engine().Finalize(p.bc.hc, header, tracingStateDB, block.Body(), receipts) | ||
| if err != nil { | ||
| stateSyncEndErr = err | ||
| return nil, err | ||
| } | ||
|
|
||
|
|
@@ -417,12 +443,14 @@ func (p *ParallelStateProcessor) Process(block *types.Block, statedb *state.Stat | |
| // Defense-in-depth: if insertStateSyncTransactionAndCalculateReceipt silently failed | ||
| // to add the receipt, the count will be off. | ||
| if len(block.Transactions()) != len(receipts) { | ||
| return nil, fmt.Errorf("%w: receipt count mismatch, txs=%d receipts=%d", ErrStateSyncMismatch, len(block.Transactions()), len(receipts)) | ||
| stateSyncEndErr = fmt.Errorf("%w: receipt count mismatch, txs=%d receipts=%d", ErrStateSyncMismatch, len(block.Transactions()), len(receipts)) | ||
| return nil, stateSyncEndErr | ||
| } | ||
| appliedNewStateSyncReceipt := receiptsCountBeforeFinalize+1 == len(receipts) | ||
|
|
||
| if appliedNewStateSyncReceipt { | ||
| allLogs = append(allLogs, receipts[len(receipts)-1].Logs...) | ||
| stateSyncReceipt = receipts[len(receipts)-1] | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
systemTxVMConfigsetscfg.Tracer = nilwhenever the passed-in state does not implementstateTracingHooks, and the parallel state processor passes an unwrapped*state.StateDBtoFinalize. With--vmtraceenabled, 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:
Only
*hookedStateDBsatisfiesstateTracingHooks(core/state/statedb_hooked.go:314), and the two import paths differ:The PR and commit bodies declare an intentional change away from
c.vmConfigfor 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.gobefore callingFinalize, mirroring the serial path'sstate.NewHookedState(statedb, hooks), or fall back to the engine-configured tracer instead ofnil:Suggested test: import a block through the parallel state processor with a live tracer configured and assert that
OnTxStart/OnEnterfire 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 forFinalizeAndAssemble/eth_simulateV1.