Conversation
Adds L2JSON json.RawMessage + L2 L2Config to Config, mirroring the BorJSON/Bor contract: chainspec JSON round-trips the "l2" key into L2JSON via the existing plain json.Decode, and the registering L2 package unmarshals it into L2 at spec-registration time. L2Config is minimal for now (Name() string only); ResolveRules lands in a later PR.
Rules gains L2Version; L2Config.ResolveRules is consulted at the single per-block Rules choke point (BlockContext.Rules) so an L2 stack can flip EVM-fork booleans off its own version ladder instead of L1 time/number. MakeSignerFromRules mirrors MakeSigner's fork cascade for Rules-driven signer gating. BlockContext.L2Version population arrives with the engine-hook PR.
BlockContext.Rules folds Bhilai into IsPrague, so a Rules-driven signer checking IsPrague first would enable blob transactions on Bor chains at Bhilai, diverging from MakeSigner's blob=false gating.
…ngineReader Adds StartTx, GasCharging, ComputeRefund and AmendBlockContext to rules.EngineReader, installed onto BlockContext by NewEVMBlockContext with the same nil-engine fallback shape as GetTransferFunc/GetPostApplyMessageFunc. TxnExecutor.Execute consults them at the top of the transition, in the gas-purchase path, and in the refund ladder; nil hooks leave the existing behavior byte-identical. ExecutionResult gains an opaque L2 field as the execution-to-receipt bridge these hooks fill.
There was a problem hiding this comment.
Pull request overview
This PR extends Erigon’s execution “rules engine” integration to support L2 transaction lifecycle interception by introducing per-tx hook functions on evmtypes.BlockContext (installed via NewEVMBlockContext) and consuming them inside TxnExecutor.Execute. It also adds an engine-provided AmendBlockContext hook to populate engine-owned context fields (e.g., L2Version), and introduces an opaque ExecutionResult.L2 slot for L2-specific data to flow into the execution→receipt bridge.
Changes:
- Add
StartTx/GasCharging/ComputeRefundhooks (and supporting types) toevmtypes.BlockContext, plusRefundResultand anevmtypes.Messageinterface for hook inputs. - Wire hooks into
TxnExecutor.Execute(transition entry, post-gas-purchase path, refund ladder override) and add tests covering short-circuit, abort, override, and nil-hook golden-path. - Extend
rules.EngineReaderwith hook accessors +AmendBlockContext, implement no-ops for ethash/aura/bor, and forward in merge / rpcdaemon remote engine wrapper.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
polygon/bor/bor.go |
Implements new EngineReader hook methods as no-ops for Bor. |
execution/vm/evmtypes/evmtypes.go |
Adds hook fields to BlockContext, defines hook typedefs, adds RefundResult, evmtypes.Message, and ExecutionResult.L2. |
execution/protocol/txn_executor.go |
Invokes lifecycle hooks at tx entry, gas charging, and refund computation. |
execution/protocol/txn_executor_test.go |
Adds tests validating hook behavior and ensuring nil hooks preserve existing behavior. |
execution/protocol/rules/rules.go |
Extends EngineReader interface with hook accessors and AmendBlockContext. |
execution/protocol/rules/merge/merge.go |
Forwards new hook methods to wrapped eth1 engine. |
execution/protocol/rules/ethash/ethash.go |
Implements new EngineReader hook methods as no-ops for ethash. |
execution/protocol/rules/aura/aura.go |
Implements new EngineReader hook methods as no-ops for AuRa. |
execution/protocol/evm.go |
Installs engine-provided hooks into BlockContext and calls AmendBlockContext. |
execution/protocol/evm_test.go |
Tests that AmendBlockContext is invoked and L2Version reaches Rules resolution. |
cmd/rpcdaemon/cli/config.go |
Adds hook methods + AmendBlockContext to the remote engine wrapper with readiness validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A StartTx short-circuit with neither result nor error, and a ComputeRefund override claiming more gas used than the tx limit (which would underflow the sender refund), now fail the transaction instead of propagating. Also align the StartTx doc comments with the done-flag contract.
…budget A GasCharging hook that returns adjustedGasRemaining above the pre-hook budget let the EVM run past msg.Gas(), so txnGasUsed could exceed the limit and underflow refundGas(). Reject it, mirroring the existing ComputeRefund guard.
|
192 files changed - probably something wrong with base branch |
…fecycle-hooks" This reverts commit f061c4d.
…ooks # Conflicts: # execution/chain/chain_config.go # execution/chain/chain_config_test.go # execution/protocol/txn_executor.go # execution/protocol/txn_executor_test.go # execution/types/transaction_signing_test.go # execution/vm/evmtypes/evmtypes.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
execution/vm/evmtypes/evmtypes.go:167
evmtypes.Messageduplicatesprotocol.Message(see execution/protocol/txn_executor.go:110) with an identical method set. This creates a maintenance hazard: any future method change needs to be kept in sync across two packages, and a mismatch will surface as a compile error at hook call sites. Consider makingprotocol.Messagea type alias ofevmtypes.Message(or moving the shared interface to a lower/common package) so there is a single source of truth.
// Message is the subset of protocol.Message the lifecycle hooks read; its
// method set must stay a subset of protocol.Message so that type satisfies
// this interface structurally without evmtypes importing protocol.
type Message interface {
yperbasis
left a comment
There was a problem hiding this comment.
Requesting changes for three correctness issues:
-
GasCharging tip redirection is not preserved through delayed-fee parallel execution. Serial execution credits the redirected local recipient, but parallel finalization credits the original TxResult.Coinbase because ExecutionResult carries only FeeTipped. This can make serial and parallel state roots diverge.
-
A GasCharging deduction cannot be passed safely into ComputeRefund. Default accounting omits the pre-EVM deduction, while ComputeRefund receives neither the message, state, nor the charged delta. Since the hooks are stored in one per-block context and reused across parallel transactions, engine closure state is not transaction-local. Please thread the charge or transaction-local lifecycle context explicitly.
-
remoteRulesEngine.init never selects a registered L2 engine, and the raw database config restores only L2JSON. The added forwarding methods therefore delegate to built-in no-op hooks in external rpcdaemon mode, causing L2 calls and traces to execute with L1 semantics.
# Conflicts: # execution/protocol/txn_executor.go # execution/protocol/txn_executor_test.go # execution/vm/evmtypes/evmtypes.go # polygon/bor/bor.go
…Context pointer BlockContext is embedded by value in EVM, which sits exactly on the 448-byte malloc size class. Three inline func fields pushed it to 472, so every EVM allocation would have moved up a class. One *L2 pointer replaces them and the existing L2Version, leaving the struct at 192 bytes. AmendBlockContext installs it, so GetStartTxFunc/GetGasChargingFunc/ GetComputeRefundFunc drop off EngineReader along with their no-op implementations and forwards.
c418b17 to
7123933
Compare
|
Reviewed at the merged head. Build and vet are clean and the protocol, vm and rpcdaemon test packages pass. Two of the three changes requested earlier are still live, and the last commit left something behind that I should own. Still blockingThe tip redirect is dropped under parallel execution. External rpcdaemon still resolves no L2 engine. The forwarding surface shrank to one method in the last commit, but the substance is unchanged: NewThe short-circuit charges nothing against the block. When
A shared Mine to clean up
SmallerThe
|
|
Two correctness findings, both on the serial path, both new (not in the existing round or Copilot's). 1.
Failure: tx N does Fix: hoist 2.
Failure: a contract doing Fix: keep Dead weight.
if a, ok := engine.(interface{ AmendBlockContext(*evmtypes.BlockContext, *types.Header) }); ok {
a.AmendBlockContext(&blockContext, header)
}That is the right shape for an optional extension point with no in-tree implementer. Nit: |
yperbasis
left a comment
There was a problem hiding this comment.
Re-reviewed at 712393392fec63372b986e9e50b7eaf909961f2e after reading all comments and review threads. Requesting changes for three correctness issues:
-
P1: Preserve the redirected tip recipient through delayed fee processing. txn_executor.go:648–649 changes only the local recipient. Serial execution credits that account. With delayed fees, the executor returns
FeeTippedwithout the recipient; TxTask.Execute keeps the original block coinbase, and parallel fee processing credits that address. This can produce different balances and state roots between execution modes. A focused reproduction pays 21,000 wei to the redirected account in serial mode, while the delayed-fee result still names the original coinbase. Please carry the recipient alongside the tip amount and use it during delayed fee processing. -
P1: Include GasCharging deductions in transaction gas accounting. txn_executor.go:647 reduces the gas budget but discards the charged delta. Later accounting totals only authority, top-level and EVM frame usage, so the hook's deduction is refunded to the sender. Reproduction: a simple transfer with a 100,000-gas limit, gas price 1 wei and a hook that subtracts 10,000 execution gas still charges only 21,000 gas. A sender starting with 1,000,000 wei ends with 979,000 instead of 969,000.
ComputeRefundalso receives neither the charged delta nor the message or transaction state, so it cannot recover a variable per-transaction charge from its arguments. Please keep this charge in transaction-local accounting and pass it into refund calculation; mutable state captured by the shared per-block hooks is not a safe substitute. -
P2: Keep block coinbase separate from the fee recipient during Prepare. The assignment at txn_executor.go:649 changes the address passed to
Prepareat line 678.Preparewarms that address under Shanghai, whileCOINBASEstill returnsevm.Context.Coinbase. A contract executingCOINBASE; BALANCE; POP; STOPuses 21,104 gas without a redirect and 23,604 with one. The original coinbase can therefore be charged as cold. Please keep the original coinbase forPrepareand use a separate recipient for fee payment. The same argument also controls the implicit coinbase BAL access.
Validation: go test ./execution/protocol ./execution/vm/evmtypes -count=1 passes. Three temporary focused tests fail for the findings above. The delayed-fee reproduction checks TxTask.Execute's handoff; the finalizer's destination was confirmed by code inspection.
Correction to the latest transient-storage comment: the stated sequence of tx N, a short-circuited N+1, and a normal N+2 does clear transient storage and access-list warmth when N+2 calls Prepare. A reproduction with TSTORE, a short-circuited transaction, then TLOAD returns zero. StartTx itself still runs before Prepare, so a stateful hook must account for that ordering.
The standalone RPC engine-selection defect remains and is already tracked as a separate prerequisite in #22193. The new forwarding method alone does not provide standalone L2 calls and traces.
…m the block coinbase, drop dead hook surface
|
@yperbasis all three reproduce. Fixed in 3cf40ac.
RPC engine selection stays with #22193. @AskAlexSharov's dead-weight list applied: six no-op |
There was a problem hiding this comment.
🟡 Changes recommended
Serial and parallel callbacks can receive different coinbase addresses, and advertised context and receipt contracts remain incomplete.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Balanced
| if a, ok := engine.(interface { | ||
| AmendBlockContext(*evmtypes.BlockContext, *types.Header) | ||
| }); ok { | ||
| a.AmendBlockContext(&blockContext, header) |
| if !tipRecipient.IsNil() { | ||
| if st.noFeeBurnAndTip { | ||
| return nil, fmt.Errorf("%w: GasCharging tip redirect is unsupported under delayed fee processing", ErrTxnExecutionFailed) | ||
| } | ||
| coinbase = tipRecipient |
| // RefundResult is what ComputeRefundFunc produces in place of the refund | ||
| // ladder: the final per-tx gas-used values TxnExecutor.Execute needs to | ||
| // charge the block gas pool and pay tips/burn the base fee. | ||
| type RefundResult struct { |
yperbasis
left a comment
There was a problem hiding this comment.
Requesting changes for one gas accounting issue at 3cf40aca00: state charges that spill into the execution reservoir are assigned to the wrong gas dimension. The inline comment includes the reproduction and suggested fix.
Validation: go test ./execution/protocol ./execution/vm/evmtypes ./execution/protocol/rules/merge ./execution/vm/runtime passes. A temporary regression test using mdgas.Consume fails with empty and partially filled state reservoirs; the same test passes when the state reservoir covers the whole charge.
| hookChargedExecution = st.gasRemaining.Execution - adjustedGasRemaining.Execution | ||
| hookChargedState = st.gasRemaining.State - adjustedGasRemaining.State |
There was a problem hiding this comment.
[P2] Preserve the gas dimension when a state charge spills into execution gas
Under Amsterdam, mdgas.Consume(&remaining, &used, charge, mdgas.StateGas) can pay a state charge from the execution reservoir. These subtractions classify that portion as execution usage, although it must still count as state usage.
Reproduced with a plain transfer and a hook charging 10,000 state gas via mdgas.Consume:
- Gas limit 100,000: reports 0 state gas and adds 10,000 execution gas.
- Gas limit
params.MaxTxnGasLimit + 5_000: reports 5,000 state gas and adds 5,000 execution gas. - Gas limit
params.MaxTxnGasLimit + 10_000: correctly reports 10,000 state gas and leaves execution usage unchanged.
The first two cases debit the wrong block gas pool and pass incorrect dimensions to ComputeRefund. Please return explicit gas usage, including state spill, from the hook instead of inferring the charge dimension from remaining balances. Add coverage for empty and partially filled state reservoirs; the current state-charge test covers only a sufficient reservoir.
|
why are we making these changes? there is no need for them. |
taratorio
left a comment
There was a problem hiding this comment.
#22200 (comment) - i dont think we should be merging this blindly without having a real use case that we are working towards. we have other priorities to chase at the moment
…he dimension the hook reports
|
@taratorio They are handy for arbitrum and BSC support in general but this thread activated on my yesterday's pass on old prs update |
|
not supposed to merge it today actually |
Arbitrum is a dead story. Time to move on. BSC - @sudeepdino008 will add if he needs to. Right now there is no use case for this and there is no point adding useless abstractions that no one uses to the codebase. |
An L2 needs to intercept the transaction lifecycle — short-circuit system/deposit txs before the standard checks, charge costs in its own fee dimensions out of the tx's gas budget, and apply its own refund policy. The nitro-erigon fork spliced this inline into the state transition behind chain checks and re-wired the hook separately at the executor and three RPC sites.
rules.EngineReaderalready installs per-block funcs (GetTransferFunc/GetPostApplyMessageFunc) throughNewEVMBlockContext, inherited by every executor and RPC call site — this extends that mechanism instead.Changes
StartTxFunc/GasChargingFunc/ComputeRefundFunctypedefs andBlockContextfields inevmtypes; installed byNewEVMBlockContextwith the existing nil-engine fallback shape; consulted byTxnExecutor.Executeat transition entry, in the gas-purchase path, and as an override of the refund ladder. Nil hooks leave behavior byte-identical (no call-site changes anywhere).AmendBlockContext(bc, header)onrules.EngineReader— the engine populates context values it owns (e.g.BlockContext.L2Versionfeeding the fork oracle).ExecutionResult.L2opaque slot — the execution-to-receipt bridge the hooks fill.mergeforwards all four to the wrapped engine.Stacked on #22195. Part of #22193.