feat: implement eip-7702#196
Conversation
WalkthroughMakeSigner and receipt APIs become time-aware (blockTime passed/propagated). EIP‑7702 SetCode authorization data is exposed and wired into IntrinsicGas, tx-pool, RPC and clients. A Hooks-based tracing system with state-change reasons and widespread VM/state/rawdb/test updates is introduced. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant RPC
participant TxPool
participant Chain
participant StateTx as StateTransition
participant EVM
participant VM
Client->>RPC: submit tx (SetCode or normal)
RPC->>TxPool: validate (IntrinsicGas with authList)
TxPool-->>RPC: accept/reject
Note right of Chain: Block processing uses blockTime (header.Time)
Chain->>StateTx: apply tx (signer := MakeSigner(cfg, blockNum, blockTime))
StateTx->>StateTx: pre-checks & apply authorizations (if any)
StateTx->>EVM: execute message (hooked StateDB if tracer present)
EVM->>VM: resolveCode(addr) (Morph300 lookup if enabled)
VM-->>EVM: code (possibly delegated)
EVM-->>StateTx: result (logs, gas, l1DataFee)
StateTx->>Chain: create receipt (DeriveFields uses blockTime, baseFee)
sequenceDiagram
autonumber
participant Reader as ReceiptReader
participant DB
participant HeaderStore
participant Types as Receipts.DeriveFields
Reader->>DB: ReadHeader(hash, number)
DB-->>Reader: header (time, baseFee)
Reader->>DB: ReadReceipts(hash, number, header.Time, cfg)
DB-->>Reader: raw receipts
Reader->>Types: DeriveFields(cfg, hash, number, header.Time, header.BaseFee, txs)
Types-->>Reader: enriched receipts
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
ethclient/gethclient/gethclient.go (1)
95-95: Fix JSON tag typo: keccakodeHash → keccakCodeHashThis breaks decoding of GetProof results.
- KeccakCodeHash common.Hash `json:"keccakodeHash"` + KeccakCodeHash common.Hash `json:"keccakCodeHash"`core/types/tx_blob.go (2)
177-179: Potential nil deref in rawSignatureValues.
If V/R/S are nil (e.g., partially constructed/decoded tx), ToBig() will panic.-func (tx *BlobTx) rawSignatureValues() (v, r, s *big.Int) { - return tx.V.ToBig(), tx.R.ToBig(), tx.S.ToBig() -} +func (tx *BlobTx) rawSignatureValues() (v, r, s *big.Int) { + if tx.V == nil || tx.R == nil || tx.S == nil { + return new(big.Int), new(big.Int), new(big.Int) + } + return tx.V.ToBig(), tx.R.ToBig(), tx.S.ToBig() +}
181-186: setSignatureValues assumes non-nil fields.
ChainID/V/R/S may be nil; SetFromBig on nil panics. Allocate defensively.func (tx *BlobTx) setSignatureValues(chainID, v, r, s *big.Int) { - tx.ChainID.SetFromBig(chainID) - tx.V.SetFromBig(v) - tx.R.SetFromBig(r) - tx.S.SetFromBig(s) + if tx.ChainID == nil { tx.ChainID = new(uint256.Int) } + if tx.V == nil { tx.V = new(uint256.Int) } + if tx.R == nil { tx.R = new(uint256.Int) } + if tx.S == nil { tx.S = new(uint256.Int) } + tx.ChainID.SetFromBig(chainID) + tx.V.SetFromBig(v) + tx.R.SetFromBig(r) + tx.S.SetFromBig(s) }les/downloader/queue_test.go (1)
50-56: Fix incorrect timestamp passed to MakeSigner in makeChainparent.Time()+10 is only correct for the first generated block; later blocks get the wrong signer when time-gated. Pass each block’s actual timestamp.
Preferred (if available on BlockGen):
- signer := types.MakeSigner(params.TestChainConfig, block.Number(), parent.Time()+10) + signer := types.MakeSigner(params.TestChainConfig, block.Number(), block.Header().Time)Fallback (matching GenerateChain’s 10s step):
- signer := types.MakeSigner(params.TestChainConfig, block.Number(), parent.Time()+10) + signer := types.MakeSigner(params.TestChainConfig, block.Number(), parent.Time()+uint64((i+1)*10))eth/fetcher/block_fetcher_test.go (1)
54-61: Replace allparent.Time()+10withblock.Time()in MakeSigner calls across testsThe following tests still use a constant timestamp; update each to use the actual block timestamp:
- les/fetcher/block_fetcher_test.go:55
- les/downloader/queue_test.go:50
- les/downloader/testchain_test.go:129
- eth/downloader/queue_test.go:51
- eth/downloader/testchain_test.go:165
- eth/fetcher/block_fetcher_test.go:56
Preferred replacement:
- signer := types.MakeSigner(params.TestChainConfig, block.Number(), parent.Time()+10) + signer := types.MakeSigner(params.TestChainConfig, block.Number(), block.Time())Fallback (if
block.Time()isn’t exposed):- signer := types.MakeSigner(params.TestChainConfig, block.Number(), parent.Time()+10) + signer := types.MakeSigner(params.TestChainConfig, block.Number(), block.PrevBlock(0).Time()+10)rollup/tracing/tracing.go (2)
281-286: Use block hash, not tx root, in tx context.BlockHash should be the block’s hash; TxHash is different (tx Merkle root). Fix to avoid misleading consumers.
Apply:
- txctx := &Context{ - BlockHash: block.TxHash(), + txctx := &Context{ + BlockHash: block.Hash(), TxIndex: index, TxHash: tx.Hash(), }
451-456: Avoid method call on possibly missing map entry (nil-safe ZkTrieTracer access).env.ZkTrieTracer[addrStr].Available() may read a zero value; guard with ok check.
Apply:
- if zktrieTracer.Available() && !env.ZkTrieTracer[addrStr].Available() { - env.ZkTrieTracer[addrStr] = state.NewProofTracer(trie) - } + if zktrieTracer.Available() { + if v, ok := env.ZkTrieTracer[addrStr]; !ok || !v.Available() { + env.ZkTrieTracer[addrStr] = state.NewProofTracer(trie) + } + }Also applies to: 459-461
params/config.go (2)
858-924: Add Morph300 to compatibility checks.Without this, config mismatches for Morph300 won’t be detected, risking consensus splits.
Apply:
if isForkTimestampIncompatible(c.Morph203Time, newcfg.Morph203Time, headTimestamp) { return newTimestampCompatError("Morph203Time fork timestamp", c.Morph203Time, newcfg.Morph203Time) } + if isForkTimestampIncompatible(c.Morph300Time, newcfg.Morph300Time, headTimestamp) { + return newTimestampCompatError("Morph300Time fork timestamp", c.Morph300Time, newcfg.Morph300Time) + } return nil
791-856: Include Morph300 in fork-order validation.Prevents invalid scheduling in configs.
Apply:
for _, cur := range []fork{ @@ - {name: "morph203Time", timestamp: c.Morph203Time, optional: true}, + {name: "morph203Time", timestamp: c.Morph203Time, optional: true}, + {name: "morph300Time", timestamp: c.Morph300Time, optional: true}, } {core/rawdb/accessors_chain.go (1)
686-687: Fix log fields: missing value for "hash"The structured log omits the actual hash value.
- log.Error("Invalid receipt array RLP", "hash", "err", err) + log.Error("Invalid receipt array RLP", "hash", hash, "err", err)internal/ethapi/transaction_args.go (1)
80-185: Fail-fast validations for SetCode in RPC and fix gas estimation undercharge
- Return errors early if AuthorizationList is used with contract creation (nil To), with legacy gasPrice, or before Morph300 activation.
- Include AuthorizationList in DoEstimateGas to account for intrinsic auth gas.
func (args *TransactionArgs) setDefaults(ctx context.Context, b Backend) error { + // Validate SetCode preconditions early + head := b.CurrentHeader() + if args.AuthorizationList != nil { + if args.To == nil { + return errors.New("authorizationList requires 'to' (SetCode tx cannot create contracts)") + } + if args.GasPrice != nil { + return errors.New("authorizationList cannot be combined with gasPrice; use maxFeePerGas/maxPriorityFeePerGas") + } + if !b.ChainConfig().IsMorph300(head.Number, head.Time) { + return errors.New("authorizationList not supported before Morph300") + } + } - // After curie, default to 1559 unless gasPrice is set - head := b.CurrentHeader() + // After curie, default to 1559 unless gasPrice is set + // (head already fetched) @@ if args.Gas == nil { @@ - callArgs := TransactionArgs{ + callArgs := TransactionArgs{ From: args.From, To: args.To, GasPrice: args.GasPrice, MaxFeePerGas: args.MaxFeePerGas, MaxPriorityFeePerGas: args.MaxPriorityFeePerGas, Value: args.Value, Data: (*hexutil.Bytes)(&data), AccessList: args.AccessList, + AuthorizationList: args.AuthorizationList, }
🧹 Nitpick comments (46)
core/tx_list.go (1)
293-294: Docstring nit: tighten wording and spacingUse standard Go doc style and remove the extra space.
Apply this diff:
-// Contains returns whether the list contains a transaction -// with the provided nonce. +// Contains reports whether the list contains a transaction with the given nonce.core/vm/contracts_test.go (1)
191-193: Avoid double-formatting in ErrorfPass format and args to Errorf directly; Sprintf is redundant.
- bench.Errorf(fmt.Sprintf("Expected %v, got %v", test.Expected, common.Bytes2Hex(res))) + bench.Errorf("Expected %v, got %v", test.Expected, common.Bytes2Hex(res))core/types/legacy_tx.go (1)
107-109: Nil-safety in effectiveGasPrice.Guard against a nil GasPrice to avoid a potential panic.
func (tx *LegacyTx) effectiveGasPrice(dst *big.Int, baseFee *big.Int) *big.Int { - return dst.Set(tx.GasPrice) + if tx.GasPrice == nil { + return dst.SetUint64(0) + } + return dst.Set(tx.GasPrice) }light/txpool.go (1)
97-105: Make TxPool.signer update on new heads
Currentlypool.signeris set once viatypes.LatestSigner(config)in NewTxPool and never updated (see lines 98–105 and 311–318). To ensure the signer reflects time-based fork rules, assignpool.signer = types.MakeSigner(pool.config, head.Number, head.Time)inside setNewHead.
core/vm/eips.go (1)
276-282: 7702 gas hooks wired to CALL-family opcodes.Looks correct provided gasCallEIP7702* implement the intended semantics. Please ensure a targeted test verifies CALL/CALLCODE/STATICCALL/DELEGATECALL gas under Morph300.
core/types/l1_message_tx.go (1)
70-72: Panic on sigHash; add guardrails or doc.If this method is accidentally reached, it will crash tests/nodes. Either add a comment asserting it's unreachable, or return a zero hash and let callers gate by type. At minimum, add a clarifying comment.
-func (tx *L1MessageTx) sigHash(chainID *big.Int) common.Hash { - panic("sigHash called on L1MessageTx") -} +// sigHash is undefined for L1MessageTx; L1 messages are not user-signed. +// Callers must not invoke this for L1MessageTx. +func (tx *L1MessageTx) sigHash(chainID *big.Int) common.Hash { + panic("sigHash called on L1MessageTx (unreachable)") +}les/downloader/testchain_test.go (1)
129-131: Prefer block.Time() for signer to avoid header-time driftUsing parent.Time()+10 risks divergence if BlockGen adjusts the current block timestamp (e.g., via OffsetTime). Using block.Time() ties the signer gating to the actual header time.
Apply:
- signer := types.MakeSigner(params.TestChainConfig, block.Number(), parent.Time()+10) + signer := types.MakeSigner(params.TestChainConfig, block.Number(), block.Time())eth/gasprice/gasprice.go (1)
256-260: Docstring name is staleComment says getBlockPrices but function is getBlockValues. Rename for clarity.
-// getBlockPrices calculates the lowest transaction gas price in a given block +// getBlockValues calculates effective gas tips for a given blockcore/types/tx_blob.go (1)
46-48: Typo in comment ("sigining").
Fix spelling.-// is used to create a transaction for sigining. +// is used to create a transaction for signing.core/vm/operations_acl.go (1)
227-285: Avoid potential double-charge on self-delegation; add fast-path to skip full code fetch
- If a contract delegates to itself, charging an extra warm read (100) may double-count the same address. Skip the extra charge when target == addr.
- Micro perf: Check code size (== 23) before fetching full code to parse the delegation stub.
Please add unit tests covering: warm/cold target, cold caller+warm target, and self-delegation.
Apply this minimal diff to guard resolution and self-delegation:
@@ - // Check if code is a delegation and if so, charge for resolution. - if target, ok := types.ParseDelegation(evm.StateDB.GetCode(addr)); ok { + // Check if code is a delegation and if so, charge for resolution. + // Fast-path: avoid allocating code for non-7702 accounts + if evm.StateDB.GetCodeSize(addr) == 23 { + if target, ok := types.ParseDelegation(evm.StateDB.GetCode(addr)); ok && target != addr { var cost uint64 - if evm.StateDB.AddressInAccessList(target) { - cost = params.WarmStorageReadCostEIP2929 - } else { - evm.StateDB.AddAddressToAccessList(target) - cost = params.ColdAccountAccessCostEIP2929 - } - if !contract.UseGas(cost) { - return 0, ErrOutOfGas - } - total += cost - } + if evm.StateDB.AddressInAccessList(target) { + cost = params.WarmStorageReadCostEIP2929 + } else { + evm.StateDB.AddAddressToAccessList(target) + cost = params.ColdAccountAccessCostEIP2929 + } + if !contract.UseGas(cost) { + return 0, ErrOutOfGas + } + total += cost + } + }Optional readability nit (rename accumulator to avoid shadowing on return):
@@ - total uint64 // total dynamic gas used + extra uint64 // extra dynamic gas used for EIP-7702 resolution & cold access @@ - total += coldCost + extra += coldCost @@ - total += cost + extra += cost @@ - contract.Gas += total + contract.Gas += extra @@ - if total, overflow = math.SafeAdd(old, total); overflow { + if extra, overflow = math.SafeAdd(old, extra); overflow { return 0, ErrGasUintOverflow } - return total, nil + return extra, nilcore/blockchain_test.go (1)
687-687: Avoid shadowing the time package; rename local var.Minor readability fix; prevents accidental package shadowing in-loop.
- num, hash, time := blocks[i].NumberU64(), blocks[i].Hash(), blocks[i].Time() + num, hash, blockTime := blocks[i].NumberU64(), blocks[i].Hash(), blocks[i].Time() @@ - freceipts := rawdb.ReadReceipts(fastDb, hash, num, time, fast.Config()) - anreceipts := rawdb.ReadReceipts(ancientDb, hash, num, time, fast.Config()) - areceipts := rawdb.ReadReceipts(archiveDb, hash, num, time, fast.Config()) + freceipts := rawdb.ReadReceipts(fastDb, hash, num, blockTime, fast.Config()) + anreceipts := rawdb.ReadReceipts(ancientDb, hash, num, blockTime, fast.Config()) + areceipts := rawdb.ReadReceipts(archiveDb, hash, num, blockTime, fast.Config())Also applies to: 710-713
miner/worker.go (5)
320-321: Consider handling Sender() errors to skip malformed txs early.Ignoring the error can waste cycles and spam logs. If derivation fails, drop the tx and continue.
Apply:
- from, _ := types.Sender(env.signer, tx) + from, errSender := types.Sender(env.signer, tx) + if errSender != nil { + log.Trace("Skipping tx with unresolvable sender", "hash", tx.Hash(), "err", errSender) + txs.Pop() + continue + }
324-324: Fix typo in log message: “reply” → “replay”.- log.Trace("Ignoring reply protected transaction", "hash", tx.Hash(), "eip155", miner.chainConfig.EIP155Block) + log.Trace("Ignoring replay-protected transaction", "hash", tx.Hash(), "eip155", miner.chainConfig.EIP155Block)
355-355: Fix typo in log message: “hight” → “high”.- log.Trace("Skipping account with hight nonce", "sender", from, "nonce", tx.Nonce()) + log.Trace("Skipping account with high nonce", "sender", from, "nonce", tx.Nonce())
81-81: Spelling nit: “timstamp” → “timestamp”.- timestamp uint64 // The timstamp for sealing task + timestamp uint64 // The timestamp for sealing task
432-432: Comment clarity nit.- // Giving up involving L2 transactions if it is simulation for L1Messages + // Skip L2 transactions when simulating L1 messagestests/state_test_util.go (1)
358-360: Add coverage for non-nil AuthorizationList (EIP-7702).Include a test vector with a minimal valid SetCode authorization to exercise NewMessage plumbing through ApplyMessage.
I can draft a table-driven test that builds a Message with a single authorization and asserts state changes and gas accounting—want me to add it?
core/types/access_list_tx.go (1)
111-114: Optional: document unused baseFee param to avoid confusion.-func (tx *AccessListTx) effectiveGasPrice(dst *big.Int, baseFee *big.Int) *big.Int { +// For EIP-2930 tx, effective gas price equals GasPrice; baseFee is ignored. +func (tx *AccessListTx) effectiveGasPrice(dst *big.Int, baseFee *big.Int) *big.Int {core/types/tx_setcode.go (2)
108-114: Name the 0x05 domain prefix to avoid magic numbers.-func (a *SetCodeAuthorization) SigHash() common.Hash { - return prefixedRlpHash(0x05, []any{ +func (a *SetCodeAuthorization) SigHash() common.Hash { + return prefixedRlpHash(AuthorizationSigHashPrefix, []any{Add this constant near the transaction type declarations:
// AuthorizationSigHashPrefix is the typed-data prefix for SetCode authorizations (EIP-7702). const AuthorizationSigHashPrefix = byte(0x05)
108-114: Optional: unit test SigHash/Authority roundtrip.Add tests that:
- build an auth, sign with a known key via SignSetCode,
- assert a.SigHash() matches a known vector,
- assert Authority() recovers the signer.
I can contribute a test using the dev key (0x...deadbeef) and a fixed chainId/nonce to pin the hash.
core/vm/jump_table.go (1)
69-73: Morph300 inherits Curie and enables 7702 — OK.Minor: consider a brief comment noting which semantics 7702 toggles here for quick grepability.
light/odr_util.go (1)
171-182: Avoid reassembling the block just to compute receipt fields.Minor perf: fetch header/body directly and pass their fields to DeriveFields to skip NewBlock/WithBody.
Apply:
- block, err := GetBlock(ctx, odr, hash, number) + header, err := GetHeaderByNumber(ctx, odr, number) if err != nil { return nil, err } - genesis := rawdb.ReadCanonicalHash(odr.Database(), 0) - config := rawdb.ReadChainConfig(odr.Database(), genesis) - - if err := receipts.DeriveFields(config, block.Hash(), block.NumberU64(), block.Time(), block.BaseFee(), block.Transactions()); err != nil { + body, err := GetBody(ctx, odr, hash, number) + if err != nil { + return nil, err + } + genesis := rawdb.ReadCanonicalHash(odr.Database(), 0) + config := rawdb.ReadChainConfig(odr.Database(), genesis) + if err := receipts.DeriveFields(config, header.Hash(), header.Number.Uint64(), header.Time, header.BaseFee, body.Transactions); err != nil { return nil, err } rawdb.WriteReceipts(odr.Database(), hash, number, receipts)core/rawdb/accessors_indexes.go (1)
133-139: Header requirement before ReadReceipts may change behaviorEarly-returning when header is missing tightens invariants but could regress lookups if receipts exist while headers are pruned/corrupt. Consider a graceful fallback (time=0 via ReadReceipts) or log at warn-level and continue, if compatible with your storage guarantees.
- header := ReadHeader(db, blockHash, *blockNumber) - if header == nil { - return nil, common.Hash{}, 0, 0 - } - receipts := ReadReceipts(db, blockHash, *blockNumber, header.Time, config) + if header := ReadHeader(db, blockHash, *blockNumber); header != nil { + receipts := ReadReceipts(db, blockHash, *blockNumber, header.Time, config) + ... + } else { + // Optional: fallback if you want legacy tolerance; otherwise keep current behavior. + // receipts := ReadReceipts(db, blockHash, *blockNumber, 0, config) + return nil, common.Hash{}, 0, 0 + }les/fetcher/block_fetcher_test.go (1)
55-56: Prefer block.Time() over parent.Time()+10Using the child block’s timestamp avoids edge cases around fork-time thresholds and reads clearer in tests.
- signer := types.MakeSigner(params.TestChainConfig, block.Number(), parent.Time()+10) + signer := types.MakeSigner(params.TestChainConfig, block.Number(), block.Time())cmd/evm/internal/t8ntool/transaction.go (1)
116-116: Use actual block timestamp when creating signer
Replace the hardcoded0at cmd/evm/internal/t8ntool/transaction.go:116 with the real block timestamp (from CLI or input) to ensure the correct signer is chosen around Morph300/EIP-7702.rollup/tracing/tracing.go (1)
546-563: Fix typos in log messages (“intrinstic” → “intrinsic”).Keeps logs professional and searchable.
Apply:
- log.Error("Proof for intrinstic address not available", "error", err, "address", addr) + log.Error("Proof for intrinsic address not available", "error", err, "address", addr) @@ - log.Error("Storage proof for intrinstic address not available", "error", err, "address", addr) + log.Error("Storage proof for intrinsic address not available", "error", err, "address", addr) @@ - log.Error("Get storage proof for intrinstic address failed", "error", err, "address", addr, "slot", slot) + log.Error("Get storage proof for intrinsic address failed", "error", err, "address", addr, "slot", slot)params/config.go (2)
544-544: Fix comment typo for Morph300.“Morph303Time” should be “Morph300Time”.
Apply:
- Morph300Time *uint64 `json:"morph300Time,omitempty"` // Morph303Time switch time (nil = no fork, 0 = already on morph303) + Morph300Time *uint64 `json:"morph300Time,omitempty"` // Morph300Time switch time (nil = no fork, 0 = already on morph300)
625-660: Consider printing Morph300Time in ChainConfig.String().Helps diagnostics when debugging signer and fork selection.
Apply:
return fmt.Sprintf("{ChainID: %v Homestead: %v DAO: %v DAOSupport: %v EIP150: %v EIP155: %v EIP158: %v Byzantium: %v Constantinople: %v Petersburg: %v Istanbul: %v, Muir Glacier: %v, Berlin: %v, London: %v, Arrow Glacier: %v, Archimedes: %v, Shanghai: %v, Bernoulli: %v, Curie: %v, Morph203: %v, Engine: %v, Morph config: %v}", @@ - c.Morph203Time, + c.Morph203Time, + /* Morph300: */ c.Morph300Time,core/vm/evm.go (1)
231-240: Use of resolveCode/resolveCodeHash in call paths — nice; consider minor perf tweakThis centralizes delegation logic. To shave a state read on Morph300 non-delegated accounts, resolveCodeHash can avoid fetching full code by first checking code size. See diff below.
Also applies to: 299-300, 342-343, 398-399
core/blockchain.go (2)
1403-1404: Partition sender recovery by signer to handle time-gated forksUsing a single signer built from chain[0]’s time can misrecover senders when the batch crosses a fork boundary gated by time (e.g., Morph300). Consider partitioning the batch by signer changes and invoking recovery per segment.
- senderCacher.recoverFromBlocks(types.MakeSigner(bc.chainConfig, chain[0].Number(), chain[0].Time()), chain) + { + start := 0 + for start < len(chain) { + s := types.MakeSigner(bc.chainConfig, chain[start].Number(), chain[start].Time()) + end := start + 1 + for end < len(chain) { + s2 := types.MakeSigner(bc.chainConfig, chain[end].Number(), chain[end].Time()) + if reflect.TypeOf(s) != reflect.TypeOf(s2) { + break + } + end++ + } + senderCacher.recoverFromBlocks(s, chain[start:end]) + start = end + } + }Add import (outside this hunk):
import "reflect"
1845-1849: Avoid redundant header reads for receipts derivationReadReceipts re-reads the header internally to fetch BaseFee; you already have header here. Consider a helper that accepts header to avoid an extra DB hit on reorg hot path.
core/types/dynamic_fee_tx.go (3)
39-41: Recheck JSON/gen-code tags removal for V/R/SDropping JSON/gencodec tags may alter RPC serialization behavior. If RPC never marshals DynamicFeeTx directly, fine; otherwise restore tags to avoid breaking clients.
- V *big.Int - R *big.Int - S *big.Int + V *big.Int `json:"v" gencodec:"required"` + R *big.Int `json:"r" gencodec:"required"` + S *big.Int `json:"s" gencodec:"required"`
99-108: Clamp negative tip for robustness when baseFee > feeCapWhile included blocks satisfy baseFee <= feeCap, calls/sims may not. Clamping avoids negative tip intermediates and matches min(max(feeCap-baseFee,0), tipCap)+baseFee.
func (tx *DynamicFeeTx) effectiveGasPrice(dst *big.Int, baseFee *big.Int) *big.Int { if baseFee == nil { return dst.Set(tx.GasFeeCap) } - tip := dst.Sub(tx.GasFeeCap, baseFee) + tip := dst.Sub(tx.GasFeeCap, baseFee) + if tip.Sign() < 0 { + tip.SetUint64(0) + } if tip.Cmp(tx.GasTipCap) > 0 { tip.Set(tx.GasTipCap) } return tip.Add(tip, baseFee) }
126-140: Defensively align chainID with tx payloadPrevent accidental domain mismatch by preferring tx.ChainID when provided; fall back to param if needed.
func (tx *DynamicFeeTx) sigHash(chainID *big.Int) common.Hash { + if tx.ChainID != nil { + if chainID == nil || tx.ChainID.Cmp(chainID) != 0 { + chainID = tx.ChainID + } + } return prefixedRlpHash( DynamicFeeTxType, []any{ chainID, tx.Nonce, tx.GasTipCap, tx.GasFeeCap, tx.Gas, tx.To, tx.Value, tx.Data, tx.AccessList, }) }core/tx_pool.go (3)
719-723: Use the canonical error for empty auth-list.Prefer the shared error (defined in core/error.go) to keep error handling consistent.
Apply:
- if len(tx.SetCodeAuthorizations()) == 0 { - return errors.New("set code tx must have at least one authorization tuple") - } + if len(tx.SetCodeAuthorizations()) == 0 { + return ErrEmptyAuthList + }
732-753: Delegation limit logic: looks fine, but add tests for corner cases.Constraints for delegated/pending-delegation accounts and allowance for replacement by nonce are sensible. Please add unit tests covering:
- delegated sender with no pending tx -> only next-nonce allowed;
- replacement path (same nonce) succeeds;
- non-delegated sender unaffected.
I can draft targeted txpool tests for these scenarios if helpful.
1845-1855: Authority tracking implementation is reasonable; minor perf note.Slice-based tracking with slices.Contains/Index is OK given the “≤1 in-flight per authority” policy bounds list sizes. If this grows later, consider a small set structure to avoid O(n) scans.
Also applies to: 2007-2052
core/types/transaction_signing.go (3)
71-89: LatestSigner(nil ChainID) returns nil SignerConsider returning HomesteadSigner when ChainID is nil to avoid nil deref at call sites expecting a non-nil signer.
- } - return signer + } + if signer == nil { + signer = HomesteadSigner{} + } + return signer
265-281: Typed tx: accepting chainID=0 — confirm spec intentSignatureValues allows chainID==0 (treated as “unspecified”). For EIP-2930/1559/4844, chainID is typically mandatory. If this exception is only for SetCode, gate it by tx type.
- if tx.inner.chainID().Sign() != 0 && tx.inner.chainID().Cmp(s.chainID) != 0 { + if tt != SetCodeTxType && tx.inner.chainID().Sign() == 0 { + return nil, nil, nil, fmt.Errorf("%w: missing chain id", ErrInvalidChainId) + } + if tx.inner.chainID().Sign() != 0 && tx.inner.chainID().Cmp(s.chainID) != 0 {
346-360: Consistent error details for chain ID mismatchEIP155Signer.Sender returns ErrInvalidChainId without “have/want” context. Mirror modernSigner’s detailed message for easier debugging.
- if tx.ChainId().Cmp(s.chainId) != 0 { - return common.Address{}, ErrInvalidChainId - } + if tx.ChainId().Cmp(s.chainId) != 0 { + return common.Address{}, fmt.Errorf("%w: have %d want %d", ErrInvalidChainId, tx.ChainId(), s.chainId) + }core/state_transition.go (3)
331-339: Precheck for SetCode: no creation and non-empty listSolid. Consider mirroring this validation in RPC (setDefaults) to fail fast.
424-442: Authorization application: errors ignored and warming of final delegationIf per-spec invalid auths must be ignored (not revert), this is correct. Please confirm; otherwise, log at TRACE for diagnosability.
- // Note errors are ignored, we simply skip invalid authorizations here. - st.applyAuthorization(&auth) + if err := st.applyAuthorization(&auth); err != nil { + log.Trace("Skipped invalid EIP-7702 authorization", "err", err) + }
518-544: Refund math relies on EIP-3529 cap; confirm net gas matches intended scheduleThe refund of (CallNewAccountGas - TxAuthTupleGas) for existing accounts is fine if intrinsic charged “max upfront.” Ensure the final net cost matches the intended tuple/new-account economics under refund caps.
I can add unit tests covering: existing vs new authority, invalid signature, and duplicate tuples to verify gas deltas and state changes.
internal/ethapi/transaction_args.go (1)
57-59: Make AuthorizationList omitemptyAvoids emitting an empty array by default in JSON-RPC responses.
- AuthorizationList []types.SetCodeAuthorization `json:"authorizationList"` + AuthorizationList []types.SetCodeAuthorization `json:"authorizationList,omitempty"`core/types/transaction.go (2)
492-514: Minor cleanup: use struct{} set and pre-size map.Saves a few bytes per entry and avoids bool allocations. Behavior unchanged.
Apply:
- var ( - marks = make(map[common.Address]bool) - auths = make([]common.Address, 0, len(setcodetx.AuthList)) - ) + var ( + marks = make(map[common.Address]struct{}, len(setcodetx.AuthList)) + auths = make([]common.Address, 0, len(setcodetx.AuthList)) + ) for _, auth := range setcodetx.AuthList { if addr, err := auth.Authority(); err == nil { - if marks[addr] { + if _, seen := marks[addr]; seen { continue } - marks[addr] = true + marks[addr] = struct{}{} auths = append(auths, addr) } }
774-787: Prefer centralized gas price derivation to avoid drift.Since TxData has effectiveGasPrice now, reuse it to compute msg.gasPrice with baseFee.
Apply:
// If baseFee provided, set gasPrice to effectiveGasPrice. - if baseFee != nil { - msg.gasPrice = math.BigMin(msg.gasPrice.Add(msg.gasTipCap, baseFee), msg.gasFeeCap) - } + if baseFee != nil { + msg.gasPrice = tx.inner.effectiveGasPrice(msg.gasPrice, baseFee) + }Also applies to: 789-791
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (73)
accounts/abi/bind/backends/simulated.go(4 hunks)accounts/external/backend.go(1 hunks)cmd/evm/internal/t8ntool/execution.go(1 hunks)cmd/evm/internal/t8ntool/transaction.go(2 hunks)cmd/evm/internal/t8ntool/transition.go(1 hunks)core/bench_test.go(3 hunks)core/blockchain.go(2 hunks)core/blockchain_l2.go(1 hunks)core/blockchain_reader.go(1 hunks)core/blockchain_test.go(2 hunks)core/error.go(1 hunks)core/rawdb/accessors_chain.go(4 hunks)core/rawdb/accessors_chain_test.go(5 hunks)core/rawdb/accessors_indexes.go(1 hunks)core/state/journal.go(2 hunks)core/state/state_object.go(1 hunks)core/state_prefetcher.go(1 hunks)core/state_processor.go(2 hunks)core/state_transition.go(8 hunks)core/tx_list.go(1 hunks)core/tx_pool.go(10 hunks)core/types/access_list_tx.go(2 hunks)core/types/dynamic_fee_tx.go(3 hunks)core/types/l1_message_tx.go(2 hunks)core/types/l2trace.go(1 hunks)core/types/legacy_tx.go(2 hunks)core/types/receipt.go(4 hunks)core/types/receipt_test.go(1 hunks)core/types/transaction.go(3 hunks)core/types/transaction_signing.go(5 hunks)core/types/tx_blob.go(2 hunks)core/types/tx_blob_test.go(1 hunks)core/types/tx_setcode.go(3 hunks)core/vm/contracts_test.go(1 hunks)core/vm/eips.go(2 hunks)core/vm/evm.go(6 hunks)core/vm/interpreter.go(1 hunks)core/vm/jump_table.go(1 hunks)core/vm/operations_acl.go(2 hunks)eth/catalyst/api.go(1 hunks)eth/downloader/queue_test.go(1 hunks)eth/downloader/testchain_test.go(1 hunks)eth/fetcher/block_fetcher_test.go(1 hunks)eth/filters/filter_system_test.go(1 hunks)eth/gasprice/gasprice.go(4 hunks)eth/state_accessor.go(1 hunks)eth/tracers/api.go(5 hunks)eth/tracers/api_test.go(1 hunks)eth/tracers/internal/tracetest/calltrace_test.go(2 hunks)eth/tracers/js/tracer.go(1 hunks)eth/tracers/native/prestate.go(2 hunks)ethclient/ethclient.go(1 hunks)ethclient/gethclient/gethclient.go(1 hunks)graphql/graphql.go(2 hunks)interfaces.go(1 hunks)internal/ethapi/api.go(11 hunks)internal/ethapi/transaction_args.go(4 hunks)les/downloader/queue_test.go(1 hunks)les/downloader/testchain_test.go(1 hunks)les/fetcher/block_fetcher_test.go(1 hunks)les/handler_test.go(1 hunks)les/odr_test.go(3 hunks)les/state_accessor.go(1 hunks)light/odr_test.go(2 hunks)light/odr_util.go(1 hunks)light/txpool.go(2 hunks)miner/worker.go(1 hunks)params/config.go(4 hunks)params/forks/forks.go(1 hunks)params/protocol_params.go(1 hunks)rollup/tracing/tracing.go(2 hunks)tests/state_test_util.go(1 hunks)tests/transaction_test_util.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (58)
eth/tracers/native/prestate.go (2)
params/config.go (1)
ChainConfig(515-556)core/types/tx_setcode.go (1)
ParseDelegation(37-42)
core/vm/contracts_test.go (1)
common/bytes.go (1)
Bytes2Hex(71-73)
core/types/l2trace.go (1)
core/types/transaction_signing.go (1)
MakeSigner(42-61)
graphql/graphql.go (1)
core/types/transaction.go (2)
DynamicFeeTxType(52-52)SetCodeTxType(54-54)
accounts/external/backend.go (1)
core/types/transaction.go (2)
DynamicFeeTxType(52-52)SetCodeTxType(54-54)
interfaces.go (1)
core/types/tx_setcode.go (1)
SetCodeAuthorization(72-79)
core/types/tx_blob_test.go (1)
core/types/transaction_signing.go (1)
NewCurieSigner(300-302)
core/vm/eips.go (2)
core/vm/jump_table.go (1)
JumpTable(67-67)core/vm/opcodes.go (4)
CALL(217-217)CALLCODE(218-218)STATICCALL(223-223)DELEGATECALL(220-220)
core/blockchain_reader.go (1)
core/rawdb/accessors_chain.go (2)
ReadHeader(317-328)ReadReceipts(565-589)
cmd/evm/internal/t8ntool/execution.go (1)
core/types/transaction_signing.go (1)
MakeSigner(42-61)
eth/tracers/js/tracer.go (1)
core/state_transition.go (1)
IntrinsicGas(133-183)
miner/worker.go (1)
core/types/transaction_signing.go (1)
MakeSigner(42-61)
eth/filters/filter_system_test.go (1)
core/rawdb/accessors_chain.go (2)
ReadHeader(317-328)ReadReceipts(565-589)
core/vm/evm.go (2)
core/vm/interface.go (1)
StateDB(28-88)core/types/tx_setcode.go (1)
ParseDelegation(37-42)
eth/state_accessor.go (1)
core/types/transaction_signing.go (1)
MakeSigner(42-61)
eth/fetcher/block_fetcher_test.go (2)
core/types/transaction_signing.go (1)
MakeSigner(42-61)params/config.go (1)
TestChainConfig(398-427)
core/types/receipt.go (2)
core/types/transaction.go (6)
DynamicFeeTxType(52-52)AccessListTxType(51-51)BlobTxType(53-53)L1MessageTxType(56-56)SetCodeTxType(54-54)Transactions(575-575)core/types/transaction_signing.go (1)
MakeSigner(42-61)
eth/catalyst/api.go (1)
core/types/transaction_signing.go (1)
MakeSigner(42-61)
core/state_processor.go (1)
core/types/transaction_signing.go (1)
MakeSigner(42-61)
core/types/access_list_tx.go (1)
core/types/transaction.go (1)
AccessListTxType(51-51)
core/blockchain_test.go (1)
core/rawdb/accessors_chain.go (1)
ReadReceipts(565-589)
eth/tracers/internal/tracetest/calltrace_test.go (1)
core/types/transaction_signing.go (1)
MakeSigner(42-61)
eth/tracers/api_test.go (1)
core/types/transaction_signing.go (1)
MakeSigner(42-61)
eth/downloader/queue_test.go (2)
core/types/transaction_signing.go (1)
MakeSigner(42-61)params/config.go (1)
TestChainConfig(398-427)
core/types/tx_blob.go (1)
core/types/transaction.go (1)
BlobTxType(53-53)
cmd/evm/internal/t8ntool/transition.go (1)
core/types/transaction_signing.go (1)
MakeSigner(42-61)
eth/downloader/testchain_test.go (2)
core/types/transaction_signing.go (1)
MakeSigner(42-61)params/config.go (1)
TestChainConfig(398-427)
les/state_accessor.go (1)
core/types/transaction_signing.go (1)
MakeSigner(42-61)
core/rawdb/accessors_indexes.go (1)
core/rawdb/accessors_chain.go (2)
ReadHeader(317-328)ReadReceipts(565-589)
core/types/tx_setcode.go (1)
core/types/gen_authorization.go (2)
SetCodeAuthorization(18-25)SetCodeAuthorization(38-45)
core/rawdb/accessors_chain.go (4)
ethdb/database.go (1)
Reader(128-131)params/config.go (1)
ChainConfig(515-556)core/types/receipt.go (1)
Receipts(418-418)rlp/decode.go (1)
DecodeBytes(92-106)
les/fetcher/block_fetcher_test.go (2)
core/types/transaction_signing.go (1)
MakeSigner(42-61)params/config.go (1)
TestChainConfig(398-427)
les/downloader/queue_test.go (2)
core/types/transaction_signing.go (1)
MakeSigner(42-61)params/config.go (1)
TestChainConfig(398-427)
rollup/tracing/tracing.go (2)
core/types/transaction_signing.go (1)
MakeSigner(42-61)core/types/l2trace.go (1)
NewTransactionData(150-180)
core/types/dynamic_fee_tx.go (2)
core/types/transaction.go (1)
DynamicFeeTxType(52-52)core/types/access_list_tx.go (1)
AccessList(30-30)
light/txpool.go (1)
core/state_transition.go (1)
IntrinsicGas(133-183)
core/state_prefetcher.go (1)
core/types/transaction_signing.go (1)
MakeSigner(42-61)
core/blockchain.go (2)
core/types/transaction_signing.go (1)
MakeSigner(42-61)core/rawdb/accessors_chain.go (1)
ReadReceipts(565-589)
les/handler_test.go (1)
core/rawdb/accessors_chain.go (1)
ReadReceipts(565-589)
internal/ethapi/transaction_args.go (2)
core/types/tx_setcode.go (2)
SetCodeAuthorization(72-79)SetCodeTx(51-67)core/types/transaction.go (6)
NewMessage(754-770)LegacyTxType(50-50)SetCodeTxType(54-54)DynamicFeeTxType(52-52)AccessListTxType(51-51)TxData(80-111)
cmd/evm/internal/t8ntool/transaction.go (2)
core/types/transaction_signing.go (1)
MakeSigner(42-61)core/state_transition.go (1)
IntrinsicGas(133-183)
core/tx_pool.go (5)
core/types/transaction.go (3)
SetCodeTxType(54-54)ErrTxTypeNotSupported(39-39)Transaction(60-68)core/error.go (5)
ErrTxTypeNotSupported(89-89)ErrIntrinsicGas(85-85)ErrOutOfOrderTxFromDelegated(122-122)ErrInflightTxLimitReached(118-118)ErrAuthorityReserved(127-127)core/state_transition.go (1)
IntrinsicGas(133-183)core/types/transaction_signing.go (1)
Sender(146-162)crypto/codehash/codehash.go (1)
EmptyKeccakCodeHash(10-10)
les/downloader/testchain_test.go (2)
core/types/transaction_signing.go (1)
MakeSigner(42-61)params/config.go (1)
TestChainConfig(398-427)
eth/gasprice/gasprice.go (2)
core/types/transaction_signing.go (1)
MakeSigner(42-61)params/config.go (1)
ChainConfig(515-556)
eth/tracers/api.go (2)
core/types/transaction_signing.go (1)
MakeSigner(42-61)params/config.go (1)
ChainConfig(515-556)
core/state_transition.go (3)
core/types/tx_setcode.go (3)
SetCodeAuthorization(72-79)ParseDelegation(37-42)AddressToDelegation(45-47)params/protocol_params.go (2)
CallNewAccountGas(31-31)TxAuthTupleGas(93-93)core/error.go (8)
ErrSenderNoEOA(108-108)ErrSetCodeTxCreate(114-114)ErrEmptyAuthList(113-113)ErrAuthorizationWrongChainID(133-133)ErrAuthorizationNonceOverflow(134-134)ErrAuthorizationInvalidSignature(135-135)ErrAuthorizationDestinationHasCode(136-136)ErrAuthorizationNonceMismatch(137-137)
core/vm/operations_acl.go (7)
core/vm/evm.go (1)
EVM(107-133)core/vm/interface.go (1)
StateDB(28-88)params/protocol_params.go (2)
ColdAccountAccessCostEIP2929(63-63)WarmStorageReadCostEIP2929(65-65)core/vm/errors.go (1)
ErrOutOfGas(26-26)core/types/tx_setcode.go (1)
ParseDelegation(37-42)common/math/integer.go (1)
SafeAdd(89-92)core/error.go (1)
ErrGasUintOverflow(81-81)
core/types/receipt_test.go (1)
core/types/transaction_signing.go (1)
MakeSigner(42-61)
core/types/transaction_signing.go (4)
params/config.go (1)
ChainConfig(515-556)params/forks/forks.go (7)
Fork(20-20)SpuriousDragon(28-28)Homestead(25-25)Berlin(34-34)London(35-35)Curie(40-40)Morph300(42-42)core/types/transaction.go (7)
LegacyTxType(50-50)AccessListTxType(51-51)DynamicFeeTxType(52-52)BlobTxType(53-53)SetCodeTxType(54-54)Transaction(60-68)ErrTxTypeNotSupported(39-39)cmd/evm/internal/t8ntool/transaction.go (1)
Transaction(68-184)
internal/ethapi/api.go (3)
core/types/transaction_signing.go (1)
MakeSigner(42-61)core/types/tx_setcode.go (1)
SetCodeAuthorization(72-79)core/types/transaction.go (2)
Transaction(60-68)SetCodeTxType(54-54)
core/bench_test.go (3)
core/state_transition.go (1)
IntrinsicGas(133-183)core/types/transaction_signing.go (1)
MakeSigner(42-61)core/rawdb/accessors_chain.go (1)
ReadReceipts(565-589)
light/odr_util.go (1)
core/types/transaction.go (1)
Transactions(575-575)
les/odr_test.go (3)
core/rawdb/accessors_chain.go (2)
ReadHeader(317-328)ReadReceipts(565-589)core/types/transaction.go (1)
NewMessage(754-770)core/types/transaction_signing.go (1)
MakeSigner(42-61)
light/odr_test.go (4)
core/rawdb/accessors_chain.go (2)
ReadHeader(317-328)ReadReceipts(565-589)core/types/transaction.go (1)
NewMessage(754-770)core/evm.go (1)
NewEVMBlockContext(40-68)core/types/transaction_signing.go (1)
MakeSigner(42-61)
core/types/transaction.go (3)
core/types/tx_setcode.go (1)
SetCodeAuthorization(72-79)core/types/gen_authorization.go (2)
SetCodeAuthorization(18-25)SetCodeAuthorization(38-45)core/state_transition.go (1)
Message(78-94)
core/rawdb/accessors_chain_test.go (2)
core/rawdb/accessors_chain.go (2)
ReadReceipts(565-589)WriteReceipts(592-606)core/types/receipt.go (1)
Receipts(418-418)
tests/transaction_test_util.go (2)
core/state_transition.go (1)
IntrinsicGas(133-183)core/types/access_list_tx.go (1)
AccessList(30-30)
accounts/abi/bind/backends/simulated.go (4)
core/types/transaction_signing.go (1)
MakeSigner(42-61)core/types/tx_setcode.go (1)
SetCodeAuthorization(72-79)interfaces.go (1)
CallMsg(129-147)core/rawdb/accessors_chain.go (2)
ReadHeader(317-328)ReadReceipts(565-589)
| case types.DynamicFeeTxType, types.SetCodeTxType: | ||
| args.MaxFeePerGas = (*hexutil.Big)(tx.GasFeeCap()) | ||
| args.MaxPriorityFeePerGas = (*hexutil.Big)(tx.GasTipCap()) | ||
| default: |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
SetCodeTxType gas handling OK; also propagate AuthorizationList to signer args
Dynamic-fee handling for SetCode is good. But the authorization list is not forwarded to clef, risking data loss when signing SetCode txs.
@@ func (api *ExternalSigner) SignTx(account accounts.Account, tx *types.Transaction, chainID *big.Int) (*types.Transaction, error) {
- accessList := tx.AccessList()
- args.AccessList = &accessList
+ accessList := tx.AccessList()
+ args.AccessList = &accessList
+ // For EIP-7702 SetCodeTxType, forward authorizations to the external signer (requires apitypes support).
+ if tx.Type() == types.SetCodeTxType {
+ auth := tx.SetCodeAuthorizations()
+ if len(auth) > 0 {
+ args.AuthorizationList = &auth
+ }
+ }Verification:
🏁 Script executed:
#!/bin/bash
# Check if apitypes.SendTxArgs supports AuthorizationList; if not, add it.
rg -nC2 'type\s+SendTxArgs\s+struct' signer/core/apitypes | sed -n '1,200p'
rg -n 'AuthorizationList' --type=go signer/core/apitypesLength of output: 563
🏁 Script executed:
#!/bin/bash
# Extract the full SendTxArgs struct definition to verify AuthorizationList support
sed -n '/type SendTxArgs struct {/,/^}/p' signer/core/apitypes/types.goLength of output: 1056
Add AuthorizationList to SendTxArgs and forward SetCode authorizations
The SendTxArgs struct in signer/core/apitypes/types.go doesn’t include an AuthorizationList field. Add it just above ChainID:
type SendTxArgs struct {
…
AccessList *types.AccessList `json:"accessList,omitempty"`
+ // For EIP-7702 SetCode transactions
+ AuthorizationList *[]types.Authorization `json:"authorizationList,omitempty"`
ChainID *hexutil.Big `json:"chainId,omitempty"`
}Then in accounts/external/backend.go inside SignTx, propagate the authorizations for SetCodeTxType:
accessList := tx.AccessList()
args.AccessList = &accessList
+ if tx.Type() == types.SetCodeTxType {
+ auth := tx.SetCodeAuthorizations()
+ if len(auth) > 0 {
+ args.AuthorizationList = &auth
+ }
+ }This ensures the external signer (e.g., clef) receives the authorization list when signing SetCode transactions.
| header := ReadHeader(db, hash, number) | ||
|
|
||
| var baseFee *big.Int | ||
| if header == nil { | ||
| baseFee = big.NewInt(0) | ||
| } else { | ||
| baseFee = header.BaseFee | ||
| } | ||
| if err := receipts.DeriveFields(config, hash, number, time, baseFee, body.Transactions); err != nil { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Base fee nil vs zero: preserve pre-London semantics
Passing big.NewInt(0) when the header is missing subtly changes semantics for effective gas price (nil typically means “pre-London”). Use nil unless header.BaseFee exists.
- var baseFee *big.Int
- if header == nil {
- baseFee = big.NewInt(0)
- } else {
- baseFee = header.BaseFee
- }
+ var baseFee *big.Int
+ if header != nil {
+ baseFee = header.BaseFee // may be nil pre-London
+ }🤖 Prompt for AI Agents
In core/rawdb/accessors_chain.go around lines 576 to 584, the code currently
sets baseFee to big.NewInt(0) when the header is missing which changes
semantics; instead preserve pre‑London semantics by setting baseFee to nil when
header == nil and only assign header.BaseFee when header exists, so
receipts.DeriveFields receives nil for pre‑London blocks rather than a zero
big.Int.
| blockContext = NewEVMBlockContext(header, p.bc, p.config, nil) | ||
| evm = vm.NewEVM(blockContext, vm.TxContext{}, statedb, p.config, cfg) | ||
| signer = types.MakeSigner(p.config, header.Number) | ||
| signer = types.MakeSigner(p.config, header.Number, header.Time) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Time-aware signer: confirm all call sites updated and consistent with header time
Looks correct. Please verify there are no remaining two-arg MakeSigner calls and that all callers provide the correct block time (not 0 or nil-equivalent).
Run:
🏁 Script executed:
#!/bin/bash
# Find legacy 2-arg MakeSigner calls (should return nothing)
rg -nP --glob '!**/vendor/**' 'MakeSigner\s*\([^,]+,\s*[^,]+\s*\)'
# List all 3-arg calls for a quick scan
rg -nP --glob '!**/vendor/**' 'MakeSigner\s*\([^)]*,\s*[^)]*,\s*[^)]+\)'
# Flag suspicious time arg (0 or nil)
rg -nP --glob '!**/vendor/**' 'MakeSigner\([^,]+,\s*[^,]+,\s*(0|nil)\s*\)'Length of output: 4884
Replace zero block time in MakeSigner call
In cmd/evm/internal/t8ntool/transaction.go:116 the call
signer := types.MakeSigner(chainConfig, new(big.Int), 0)passes 0 for the block time (nil-equivalent), defeating time‐aware signing. Supply the actual timestamp (e.g. prestate.Env.Timestamp) to this and any similar callers.
🤖 Prompt for AI Agents
core/state_prefetcher.go around line 58: the MakeSigner call must not receive a
zero/nil block time; replace any hardcoded 0 arguments (e.g. in
cmd/evm/internal/t8ntool/transaction.go:116) with the actual block timestamp
(use prestate.Env.Timestamp or the header.Time available in context), converting
to the expected type if necessary, and audit other MakeSigner callers to pass
the real timestamp instead of 0.
| // For symmetry, allow at most one in-flight tx for any authority with a | ||
| // pending transaction. | ||
| if auths := tx.SetCodeAuthorities(); len(auths) > 0 { | ||
| for _, auth := range auths { | ||
| var count int | ||
| if pending := pool.pending[auth]; pending != nil { | ||
| count += pending.Len() | ||
| } | ||
| if queue := pool.queue[auth]; queue != nil { | ||
| count += queue.Len() | ||
| } | ||
| if count > 1 { | ||
| return ErrAuthorityReserved | ||
| } | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Authority reservation check is incorrect source; may under/over-enforce.
Current logic counts txs by “from = authority” in pending/queue, but the constraint is about transactions that reference an authority, regardless of the sender. This can:
- miss conflicts from other senders referencing the same authority, and
- falsely block when the authority just has unrelated txs.
Use the txLookup auth index to detect real conflicts and allow same-sender/same-nonce replacement.
Apply:
- // For symmetry, allow at most one in-flight tx for any authority with a
- // pending transaction.
- if auths := tx.SetCodeAuthorities(); len(auths) > 0 {
- for _, auth := range auths {
- var count int
- if pending := pool.pending[auth]; pending != nil {
- count += pending.Len()
- }
- if queue := pool.queue[auth]; queue != nil {
- count += queue.Len()
- }
- if count > 1 {
- return ErrAuthorityReserved
- }
- }
- }
+ // For symmetry, allow at most one in-flight tx per authority across the pool,
+ // but permit replacement by the same sender+nonce.
+ from, _ := types.Sender(pool.signer, tx) // already validated
+ if auths := tx.SetCodeAuthorities(); len(auths) > 0 {
+ for _, auth := range auths {
+ for _, h := range pool.all.AuthTxHashes(auth) {
+ if other := pool.all.Get(h); other != nil {
+ ofrom, _ := types.Sender(pool.signer, other)
+ if ofrom != from || other.Nonce() != tx.Nonce() {
+ return ErrAuthorityReserved
+ }
+ }
+ }
+ }
+ }Add this helper to txLookup (outside this hunk):
// AuthTxHashes returns a copy of tx hashes that reference the given authority.
func (t *txLookup) AuthTxHashes(addr common.Address) []common.Hash {
t.lock.RLock()
defer t.lock.RUnlock()
list := t.auths[addr]
if len(list) == 0 {
return nil
}
out := make([]common.Hash, len(list))
copy(out, list)
return out
}| func (tx *L1MessageTx) effectiveGasPrice(dst *big.Int, baseFee *big.Int) *big.Int { | ||
| return new(big.Int) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid allocation; honor dst parameter.
Return dst set to 0 instead of allocating a new big.Int to match the TxData contract used by other tx types.
func (tx *L1MessageTx) effectiveGasPrice(dst *big.Int, baseFee *big.Int) *big.Int {
- return new(big.Int)
+ return dst.SetUint64(0)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (tx *L1MessageTx) effectiveGasPrice(dst *big.Int, baseFee *big.Int) *big.Int { | |
| return new(big.Int) | |
| } | |
| func (tx *L1MessageTx) effectiveGasPrice(dst *big.Int, baseFee *big.Int) *big.Int { | |
| return dst.SetUint64(0) | |
| } |
🤖 Prompt for AI Agents
In core/types/l1_message_tx.go around lines 50-53, the effectiveGasPrice
implementation incorrectly allocates a new big.Int instead of using the provided
dst; change it to set dst to zero and return dst (e.g., dst.SetInt64(0); return
dst) so the function honors the dst parameter and avoids allocation to match the
TxData contract used by other tx types; if your codebase allows dst to be nil,
guard for nil and allocate only in that edge case before setting to zero.
| case types.SetCodeTxType: | ||
| al := tx.AccessList() | ||
| yparity := hexutil.Uint64(v.Sign()) | ||
| result.Accesses = &al | ||
| result.ChainID = (*hexutil.Big)(tx.ChainId()) | ||
| result.YParity = &yparity | ||
| result.GasFeeCap = (*hexutil.Big)(tx.GasFeeCap()) | ||
| result.GasTipCap = (*hexutil.Big)(tx.GasTipCap()) | ||
| // if the transaction has been mined, compute the effective gas price | ||
| if baseFee != nil && blockHash != (common.Hash{}) { | ||
| price := math.BigMin(new(big.Int).Add(tx.GasTipCap(), baseFee), tx.GasFeeCap()) | ||
| result.GasPrice = (*hexutil.Big)(price) | ||
| } else { | ||
| result.GasPrice = (*hexutil.Big)(tx.GasFeeCap()) | ||
| } | ||
| result.AuthorizationList = tx.SetCodeAuthorizations() | ||
| } |
There was a problem hiding this comment.
Fix yParity derivation; v.Sign() is not robust.
v.Sign() yields 1 for any non-zero v, collapsing 1/27/28 into 1. Derive parity from the least-significant bit.
Apply:
- yparity := hexutil.Uint64(v.Sign())
+ y := v.Uint64()
+ yparity := hexutil.Uint64(y & 1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case types.SetCodeTxType: | |
| al := tx.AccessList() | |
| yparity := hexutil.Uint64(v.Sign()) | |
| result.Accesses = &al | |
| result.ChainID = (*hexutil.Big)(tx.ChainId()) | |
| result.YParity = &yparity | |
| result.GasFeeCap = (*hexutil.Big)(tx.GasFeeCap()) | |
| result.GasTipCap = (*hexutil.Big)(tx.GasTipCap()) | |
| // if the transaction has been mined, compute the effective gas price | |
| if baseFee != nil && blockHash != (common.Hash{}) { | |
| price := math.BigMin(new(big.Int).Add(tx.GasTipCap(), baseFee), tx.GasFeeCap()) | |
| result.GasPrice = (*hexutil.Big)(price) | |
| } else { | |
| result.GasPrice = (*hexutil.Big)(tx.GasFeeCap()) | |
| } | |
| result.AuthorizationList = tx.SetCodeAuthorizations() | |
| } | |
| case types.SetCodeTxType: | |
| al := tx.AccessList() | |
| // derive yParity from the least-significant bit of v | |
| y := v.Uint64() | |
| yparity := hexutil.Uint64(y & 1) | |
| result.Accesses = &al | |
| result.ChainID = (*hexutil.Big)(tx.ChainId()) | |
| result.YParity = &yparity | |
| result.GasFeeCap = (*hexutil.Big)(tx.GasFeeCap()) | |
| result.GasTipCap = (*hexutil.Big)(tx.GasTipCap()) | |
| // if the transaction has been mined, compute the effective gas price | |
| if baseFee != nil && blockHash != (common.Hash{}) { | |
| price := math.BigMin(new(big.Int).Add(tx.GasTipCap(), baseFee), tx.GasFeeCap()) | |
| result.GasPrice = (*hexutil.Big)(price) | |
| } else { | |
| result.GasPrice = (*hexutil.Big)(tx.GasFeeCap()) | |
| } | |
| result.AuthorizationList = tx.SetCodeAuthorizations() | |
| } |
🤖 Prompt for AI Agents
internal/ethapi/api.go around lines 1442 to 1458: the code uses v.Sign() to set
yParity which collapses any non-zero v to 1; instead derive parity from v's
least-significant bit. Replace the yparity assignment to compute parity as
v.Bit(0) (or v.Uint64() & 1) so it yields 0 or 1 correctly, and use that value
to set yParity while keeping the rest of the case block unchanged.
| var forkToString = map[Fork]string{ | ||
| Frontier: "Frontier", | ||
| FrontierThawing: "Frontier Thawing", | ||
| Homestead: "Homestead", | ||
| DAO: "DAO", | ||
| TangerineWhistle: "Tangerine Whistle", | ||
| SpuriousDragon: "Spurious Dragon", | ||
| Byzantium: "Byzantium", | ||
| Constantinople: "Constantinople", | ||
| Petersburg: "Petersburg", | ||
| Istanbul: "Istanbul", | ||
| MuirGlacier: "Muir Glacier", | ||
| Berlin: "Berlin", | ||
| London: "London", | ||
| ArrowGlacier: "Arrow Glacier", | ||
| Archimedes: "Archimedes", | ||
| Shanghai: "Shanghai", | ||
| Bernoulli: "Bernoulli", | ||
| Curie: "Curie", | ||
| Morph203: "Morph203", | ||
| } |
There was a problem hiding this comment.
Missing String() mapping for Morph300.
String() returns "Unknown fork" for Morph300; impacts logs/metrics/tests.
var forkToString = map[Fork]string{
@@
Curie: "Curie",
Morph203: "Morph203",
+ Morph300: "Morph300",
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var forkToString = map[Fork]string{ | |
| Frontier: "Frontier", | |
| FrontierThawing: "Frontier Thawing", | |
| Homestead: "Homestead", | |
| DAO: "DAO", | |
| TangerineWhistle: "Tangerine Whistle", | |
| SpuriousDragon: "Spurious Dragon", | |
| Byzantium: "Byzantium", | |
| Constantinople: "Constantinople", | |
| Petersburg: "Petersburg", | |
| Istanbul: "Istanbul", | |
| MuirGlacier: "Muir Glacier", | |
| Berlin: "Berlin", | |
| London: "London", | |
| ArrowGlacier: "Arrow Glacier", | |
| Archimedes: "Archimedes", | |
| Shanghai: "Shanghai", | |
| Bernoulli: "Bernoulli", | |
| Curie: "Curie", | |
| Morph203: "Morph203", | |
| } | |
| var forkToString = map[Fork]string{ | |
| Frontier: "Frontier", | |
| FrontierThawing: "Frontier Thawing", | |
| Homestead: "Homestead", | |
| DAO: "DAO", | |
| TangerineWhistle: "Tangerine Whistle", | |
| SpuriousDragon: "Spurious Dragon", | |
| Byzantium: "Byzantium", | |
| Constantinople: "Constantinople", | |
| Petersburg: "Petersburg", | |
| Istanbul: "Istanbul", | |
| MuirGlacier: "Muir Glacier", | |
| Berlin: "Berlin", | |
| London: "London", | |
| ArrowGlacier: "Arrow Glacier", | |
| Archimedes: "Archimedes", | |
| Shanghai: "Shanghai", | |
| Bernoulli: "Bernoulli", | |
| Curie: "Curie", | |
| Morph203: "Morph203", | |
| Morph300: "Morph300", | |
| } |
🤖 Prompt for AI Agents
In params/forks/forks.go around lines 54 to 74, the forkToString map is missing
an entry for Morph300 which causes String() to return "Unknown fork"; add a new
map entry mapping Morph300 to "Morph300" (i.e. forkToString[Morph300] =
"Morph300") in the same style as the other entries, then run tests/lint to
ensure logs/metrics/tests pick up the new string.
* refactor tracer * fix getBlockTrace * fix nil pointer * fix tracer api * use origin evm.StateDB to applyMessage
There was a problem hiding this comment.
Actionable comments posted: 40
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
eth/tracers/api_blocktrace.go (2)
86-92: Default logger.Config is built but never used.You create
&logger.Config{...}on nil input, but nothing in this function (or the wrapper call below) consumes it. As-is, these toggles (DisableStorage/Stack, EnableMemory/ReturnData) have no effect on the produced trace.Two options:
- Wire the config through: plumb
*TraceConfig(or*logger.Config) into the tracing path and have the wrapper honor it.- Or, if config is intentionally deprecated here, remove this block to avoid a misleading no-op and document the new source of truth for tracer settings.
111-113: Tracer config not propagated to execution wrapper.
CreateTraceEnvAndGetBlockTrace(...)receives no config, so any user-supplied trace options (except Reexec, handled above) are ignored. This is likely an unintended regression vs. the previous VM LogConfig plumbing.Proposed interface/threading change within this file (wrapper implementations must be updated accordingly):
- type morphTracerWrapper interface { - CreateTraceEnvAndGetBlockTrace(*params.ChainConfig, core.ChainContext, consensus.Engine, ethdb.Database, *state.StateDB, *types.Block, *types.Block, bool) (*types.BlockTrace, error) - } + type morphTracerWrapper interface { + CreateTraceEnvAndGetBlockTrace(*params.ChainConfig, core.ChainContext, consensus.Engine, ethdb.Database, *state.StateDB, *types.Block, *types.Block, bool, *TraceConfig) (*types.BlockTrace, error) + } @@ - return api.morphTracerWrapper.CreateTraceEnvAndGetBlockTrace(api.backend.ChainConfig(), api.chainContext(ctx), api.backend.Engine(), chaindb, statedb, parent, block, true) + return api.morphTracerWrapper.CreateTraceEnvAndGetBlockTrace(api.backend.ChainConfig(), api.chainContext(ctx), api.backend.Engine(), chaindb, statedb, parent, block, true, config)If widening the interface is not desirable, consider constructing the concrete tracer from
config.Confighere and attaching it to the env the wrapper uses.consensus/ethash/consensus.go (1)
660-671: Miner gets underpaid: final credit uses r (last per-uncle share) instead of total reward.After the uncles loop, the miner should receive
reward(blockReward plus 1/32 per uncle). Current code credits onlyr(which ends as blockReward/32, or 0 with no uncles).Apply:
- state.AddBalance(uncle.Coinbase, r, tracing.BalanceIncreaseRewardMineUncle) + state.AddBalance(uncle.Coinbase, r, tracing.BalanceIncreaseRewardMineUncle) @@ - state.AddBalance(header.Coinbase, r, tracing.BalanceIncreaseRewardMineBlock) + state.AddBalance(header.Coinbase, reward, tracing.BalanceIncreaseRewardMineBlock)Optional clarity (avoid reuse of r):
- r := new(big.Int) + r := new(big.Int) // per-uncle reward (temporary)Please add/adjust tests covering 0, 1, 2 uncles to prevent regressions.
I can draft tests asserting miner and uncle payouts across these cases.
core/tx_pool_test.go (2)
1083-1088: Fix test assertion message: expected count is wrongThe code checks queued != 2 but the message says “want 3”. Make the “want” match 2.
Apply:
- if queued != 2 { - t.Fatalf("queued transactions mismatched: have %d, want %d", queued, 3) - } + if queued != 2 { + t.Fatalf("queued transactions mismatched: have %d, want %d", queued, 2) + }
1979-1982: Fix expected total tx count: should be 20, not 10GlobalSlots=10 and GlobalQueue=10 ⇒ pending+queued should be 20.
Apply:
- if pending+queued != 20 { - t.Fatalf("transaction count mismatch: have %d, want %d", pending+queued, 10) - } + if pending+queued != 20 { + t.Fatalf("transaction count mismatch: have %d, want %d", pending+queued, 20) + }core/state/statedb_test.go (1)
178-181: Bug: updating the wrong state object in copy-of-copy testccopy updates with copyObj instead of ccopyObj, breaking the test’s intent.
Apply:
- ccopy.updateStateObject(copyObj) + ccopy.updateStateObject(ccopyObj)eth/tracers/api_test.go (1)
493-497: Unexported field prevents JSON population — test silently ignores returnValue.encoding/json ignores unexported fields; both have and want will default to empty and the assertion won't check returnValue. Export the field (and add tags) to ensure it’s validated.
- type res struct { - Gas int - Failed bool - returnValue string - } + type res struct { + Gas int `json:"gas"` + Failed bool `json:"failed"` + ReturnValue string `json:"returnValue"` + }core/state_processor.go (1)
174-179: Add ReturnValue field to types.Receipt
state_processor.go sets receipt.ReturnValue but core/types/receipt.go’s Receipt struct (and its JSON/RLP marshaler) lacks this field—define ReturnValue (e.g. []byte with appropriatejson/rlptags) to prevent compile errors and ensure correct serialization.eth/tracers/api.go (1)
895-899: Nil deref risk: guard StateOverrides.
config.StateOverridesmay be nil; calling.Applywill panic.- if config != nil { - if err := config.StateOverrides.Apply(statedb); err != nil { - return nil, err - } - } + if config != nil && config.StateOverrides != nil { + if err := config.StateOverrides.Apply(statedb); err != nil { + return nil, err + } + }
♻️ Duplicate comments (2)
core/vm/evm.go (2)
535-547: Update fork name in comments; add self-delegation guard.Comment still says “Prague”; here it’s gated by
IsMorph300. Also avoid pointless self-delegation lookup.-// resolveCode returns the code associated with the provided account. After -// Prague, it can also resolve code pointed to by a delegation designator. +// resolveCode returns the code associated with the provided account. +// On Morph300, it can also resolve code pointed to by a delegation designator. func (evm *EVM) resolveCode(addr common.Address) []byte { - code := evm.StateDB.GetCode(addr) - if !evm.chainRules.IsMorph300 { - return code - } - if target, ok := types.ParseDelegation(code); ok { - // Note we only follow one level of delegation. - return evm.StateDB.GetCode(target) - } - return code + code := evm.StateDB.GetCode(addr) + if !evm.chainRules.IsMorph300 { + return code + } + if target, ok := types.ParseDelegation(code); ok { + if target == addr { // avoid self-delegation loop + return code + } + // Note we only follow one level of delegation. + return evm.StateDB.GetCode(target) + } + return code }
549-562: Avoid full code fetch on common path; handle self-delegation.Check size first (23-byte designator), then fetch code. Guard self-delegation.
-// resolveCodeHash returns the code hash associated with the provided address. -// After Prague, it can also resolve code hash of the account pointed to by a -// delegation designator. Although this is not accessible in the EVM it is used -// internally to associate jumpdest analysis to code. +// resolveCodeHash returns the code hash associated with the provided address. +// On Morph300, it can also resolve the code hash of a delegated target. func (evm *EVM) resolveCodeHash(addr common.Address) common.Hash { - if evm.chainRules.IsMorph300 { - code := evm.StateDB.GetCode(addr) - if target, ok := types.ParseDelegation(code); ok { - // Note we only follow one level of delegation. - return evm.StateDB.GetKeccakCodeHash(target) - } - } - return evm.StateDB.GetKeccakCodeHash(addr) + if evm.chainRules.IsMorph300 { + // Quick size check to avoid fetching full code in the common case. + if evm.StateDB.GetCodeSize(addr) == 23 { + code := evm.StateDB.GetCode(addr) + if target, ok := types.ParseDelegation(code); ok { + if target == addr { + return evm.StateDB.GetKeccakCodeHash(addr) + } + // Note we only follow one level of delegation. + return evm.StateDB.GetKeccakCodeHash(target) + } + } + } + return evm.StateDB.GetKeccakCodeHash(addr) }
🧹 Nitpick comments (97)
eth/tracers/internal/tracetest/testdata/call_tracer_legacy/throw.json (1)
48-48: Genesis timestamp change — confirm header/hash consistency.Changing the timestamp without updating the adjacent header fields (notably "hash" at Line 42) can make this fixture internally inconsistent. If the harness reads/validates these fields, recompute the hash or drop unused/derived fields from the fixture to avoid brittle tests.
eth/tracers/internal/tracetest/testdata/call_tracer_legacy/inner_instafail.json (1)
66-66: Remove stray whitespace in JSON field.There's an extra space after the colon in "error": "internal failure". Normalize to avoid noisy diffs.
- "error": "internal failure", + "error": "internal failure",eth/tracers/internal/tracetest/testdata/call_tracer_flat/result_output.json (2)
62-62: Clarify tx type assumptions (legacy vs 1559)Input RLP appears legacy (no maxFee fields). If any signer/time-aware changes conditionally affect decoding, note it near the fixture or include a README hint to prevent future mismatches.
1-107: Add a lightweight schema/invariant check to prevent future fixture rotAutomate validation that parent/child counts and basic keys exist; catches silent schema drifts early.
Proposed script (safe, read‑only):
#!/bin/bash set -euo pipefail f='eth/tracers/internal/tracetest/testdata/call_tracer_flat/result_output.json' # 1) Required keys on each trace row jq -e ' .result as $r | all($r[]; has("type") and has("action") and has("result") and has("traceAddress") and has("subtraces") and has("transactionHash") and has("blockNumber") and has("blockHash")) ' "$f" >/dev/null # 2) Parent/child subtrace count consistency for traceAddress [ ] jq -e ' .result as $r | ($r | map(select(.traceAddress|length==0)) | length) == 1 and ($r[] | select(.traceAddress|length==0).subtraces) == ($r | map(select(.traceAddress|length==1 and .traceAddress[0]==0)) | length) ' "$f" >/dev/null # 3) Context vs genesis numbering: context.number == genesis.number + 1 jq -e '((.context.number|tonumber) - (.genesis.number|tonumber))==1' "$f" >/dev/null echo "Schema/invariants OK"If you’d like, I can wire this into a small go:generate or a make target.
eth/tracers/internal/tracetest/testdata/call_tracer_flat/delegatecall_parent_value.json (1)
79-86: Delegatecall action.value — confirm schema intentMost “flat” trace schemas omit value on DELEGATECALL or set it to 0 since no value is transferred. If this fixture intentionally surfaces msg.value for delegatecall (parent value), please confirm the tracer was updated accordingly and align other fixtures/docs. If not intentional, drop the value field.
Proposed fix if omission is desired:
- "to": "0x6ab9dd83108698b9ca8d03af3c7eb91c0e54c3fc", - "value": "0x8ac7230489e80000" + "to": "0x6ab9dd83108698b9ca8d03af3c7eb91c0e54c3fc"eth/tracers/internal/tracetest/testdata/call_tracer_flat/include_precompiled.json (4)
73-75: Align file name with config key (“includePrecompiles”).The file is named include_precompiled.json but the config flag is includePrecompiles. Recommend renaming the file to include_precompiles.json for consistency and easier grepping.
3-13: Normalize number encodings for header fields.Mixed decimal/hex encodings (e.g., difficulty/timestamp as decimal strings vs nonce/mixHash/extraData as hex). Prefer a consistent convention used by other tracer fixtures in this repo to reduce parser surprises.
14-25: Account nonce format may be inconsistent with other fixtures.alloc[0x877b…].nonce is a decimal string ("77947"). Most fixtures use hex (e.g., "0x130bb"). Consider normalizing to hex for consistency.
76-76: Document how to regenerate this fixture.Given the size and specificity of input data, add a short README.md next to this file with the exact command(s) to regenerate it from the tracer (client/build, block/time, tx crafting). This keeps the fixture from drifting.
I can draft the README with commands if you share the exact generation steps.
eth/tracers/internal/tracetest/testdata/call_tracer_flat/oog.json (1)
2-8: Normalize numeric types for consistency (optional).Minor: context.number/timestamp are strings while blockNumber is numeric. Aligning them to numbers improves consistency and avoids accidental string/number comparisons in future tooling.
Apply if desired:
- "number": "2294631", - "timestamp": "1513675366" + "number": 2294631, + "timestamp": 1513675366 @@ - "number": "2294630", + "number": 2294630, @@ - "timestamp": "1513675347" + "timestamp": 1513675347Also applies to: 44-47, 59-59
eth/tracers/internal/tracetest/makeTest.js (4)
21-24: Add basic input validation for tx/block lookupsGuard against missing transactions/blocks to fail fast with actionable errors instead of opaque exceptions from downstream calls.
- var block = eth.getBlock(eth.getTransaction(tx).blockHash); - var genesis = eth.getBlock(block.parentHash); + var txObj = eth.getTransaction(tx); + if (!txObj) { + throw new Error("Transaction not found: " + tx); + } + var block = eth.getBlock(txObj.blockHash); + if (!block) { + throw new Error("Block not found for tx: " + tx); + } + var genesis = eth.getBlock(block.parentHash); + if (!genesis) { + throw new Error("Parent block not found for block: " + block.hash); + }
25-36: Strip Cancun/Prague-era header fields tooTo keep the synthetic genesis minimal and forward-compatible, also remove fields introduced by EIP-4844/EIP-4788 where present.
delete genesis.withdrawals; delete genesis.withdrawalsRoot; delete genesis.baseFeePerGas; + // Post-Cancun / Proto-Danksharding and beacon-root fields + delete genesis.blobGasUsed; + delete genesis.excessBlobGas; + delete genesis.parentBeaconBlockRoot;Please verify on your target chain that these keys exist and that removing them matches the expectations of the tracer test harness.
42-48: Normalize alloc fields and harden iterationPrestate balances can surface as non-strings on some networks/console builds; also guard against prototype keys.
- for (var key in genesis.alloc) { - var nonce = genesis.alloc[key].nonce; - if (nonce) { - genesis.alloc[key].nonce = nonce.toString(); - } - } + for (var key in genesis.alloc) { + if (!Object.prototype.hasOwnProperty.call(genesis.alloc, key)) continue; + var acct = genesis.alloc[key]; + if (acct.nonce != null) { + acct.nonce = acct.nonce.toString(); + } + if (acct.balance != null && typeof acct.balance !== "string") { + acct.balance = acct.balance.toString(); + } + }
77-77: Terminate the function assignment with a semicolonMinor ASI nit to avoid edge cases when concatenating scripts.
-} +};eth/tracers/internal/tracetest/README.md (2)
3-4: Tighten wording and punctuationMinor grammar/punctuation improvement.
-To fill test cases for the built-in tracers, the `makeTest.js` script can be used. Given a transaction on a dev/test network, `makeTest.js` will fetch its prestate and then traces with the given configuration. -In the Geth console do: +To fill test cases for the built-in tracers, use the `makeTest.js` script. Given a transaction on a dev/test network, `makeTest.js` fetches its prestate and then traces it with the given configuration. +In the Geth console, run:
6-10: Add a short note on prerequisitesMention the required modules to avoid confusion when running from a restricted console.
```terminal let tx = '0x...' loadScript('makeTest.js') makeTest(tx, { tracer: 'callTracer' })+Prerequisites: start the console with
adminanddebugAPIs enabled (e.g.,--http.api admin,debug,eth,net,web3if using HTTP) so thatadmin.nodeInfoanddebug.traceTransactionare available.</blockquote></details> <details> <summary>eth/ethconfig/config.go (1)</summary><blockquote> `187-190`: **Clarify semantics and add TOML omitempty tags for new fields** "Enables VM tracing" suggests a boolean, but `VMTrace` is a string. Clarify that a non-empty tracer name enables tracing, and avoid persisting empty strings in generated TOML. ```diff - // Enables VM tracing - VMTrace string - VMTraceConfig string + // VMTrace selects a VM tracer by name; if non-empty, VM tracing is enabled (e.g., "callTracer"). + VMTrace string `toml:",omitempty"` + // VMTraceConfig is a JSON/TOML string passed to the tracer. + VMTraceConfig string `toml:",omitempty"`eth/tracers/internal/tracetest/testdata/call_tracer/simple.json (1)
55-55: Extend header-time alignment checks to all tracer fixtures. Verified that simple.json passes (genesis timestamp ≤ context timestamp, block number increment by 1); apply the same validation across every JSON fixture under eth/tracers/internal/tracetest/testdata to catch any misalignments and prevent flaky time-aware tracing.eth/tracers/internal/tracetest/testdata/call_tracer_flat/selfdestruct.json (3)
67-67: transactionPosition likely should be 0 for single-tx blockUnless this fixture intentionally mirrors a real block’s 15th tx, set to 0 for consistency with common testdata.
Apply:
- "transactionPosition": 14, + "transactionPosition": 0,And similarly at Line 96.
Also applies to: 96-96
16-16: Use uniform hex encoding for account noncesOther quantities here are hex. Consider encoding nonces as hex for consistency and to match typical genesis parsing.
- "nonce": "0", + "nonce": "0x0" - "nonce": "260140", + "nonce": "0x3f82c",Ensure the tracetest loader accepts hex for nonce in alloc accounts.
Also applies to: 22-22
61-63: Clarify empty code on create resultReturning
"code": "0x"is fine if the init selfdestructs; if the harness treats absence differently, consider omitting the field instead.Compare with adjacent call_tracer_flat fixtures for consistency.
eth/tracers/internal/tracetest/testdata/call_tracer_flat/suicide.json (1)
16-16: Use hex for alloc nonce for consistency with other fields.Minor consistency tweak; avoids ambiguity in parsers expecting hex.
Apply:
- "nonce": "701", + "nonce": "0x2bd",eth/tracers/internal/tracetest/testdata/call_tracer_flat/nested_create2_action_gas.json (1)
1-94: Add a companion EIP-7702 tracer fixture to guard recent signer/state-change refactors.Given this PR introduces EIP-7702 behaviors, consider adding a small flat-call tracer fixture that includes a SetCode authorization path and validates any new fields (e.g., state change reasons) to prevent regressions.
I can propose a minimal tx/state and expected trace JSON if you’d like.
eth/tracers/internal/tracetest/testdata/call_tracer_flat/inner_precompiled_wrong_gas.json (3)
14-29: Nonce formatting inconsistency in alloc.Account nonces are decimal strings (e.g., "77947" and "1"). Most fixtures use hex (0x-prefixed). Consider normalizing for consistency to avoid parser surprises across tools.
38-49: Precompile accounts in alloc are likely unnecessary.Precompiles are built-ins and don’t require alloc entries. Keeping them is harmless, but you can drop 0x000...0002 and 0x000...0004 to reduce noise, unless a past bug relied on their explicit presence.
1-218: Add a brief rationale in the directory README for “wrong_gas”.Optional: a one-liner in a README.md next to these fixtures noting the specific inner-precompile gas bug this guards against will aid future maintainers.
Happy to draft the README snippet if you want.
eth/tracers/internal/tracetest/prestate_test.go (5)
106-109: Populate TxContext with message-derived fields for accurate tracing.Empty TxContext may produce incomplete trace metadata (Origin/To/GasPrice).
- evm := vm.NewEVM(context, vm.TxContext{}, state.StateDB, test.Genesis.Config, vm.Config{Tracer: tracer.Hooks}) + txctx := vm.TxContext{ + Origin: msg.From(), + To: msg.To(), + GasPrice: msg.GasPrice(), + } + evm := vm.NewEVM(context, txctx, state.StateDB, test.Genesis.Config, vm.Config{Tracer: tracer.Hooks})
97-101: Ensure tracer cleanup to avoid goroutine/resource leaks.Call Stop after use.
tracer, err := tracers.DefaultDirectory.New(tracerName, new(tracers.Context), test.TracerConfig, test.Genesis.Config) if err != nil { t.Fatalf("failed to create call tracer: %v", err) } + defer func() { + if tracer.Stop != nil { + tracer.Stop(nil) + } + }()Also applies to: 112-116
120-126: Handle (un)marshal errors during legacy normalization.Avoid ignoring JSON errors; fail fast for clearer signal.
if strings.HasSuffix(dirPath, "_legacy") { // This is a tweak to make it deterministic. Can be removed when // we remove the legacy tracer. var x prestateTrace - json.Unmarshal(res, &x) - res, _ = json.Marshal(x) + if err := json.Unmarshal(res, &x); err != nil { + t.Fatalf("failed to decode legacy result: %v", err) + } + if res, err = json.Marshal(x); err != nil { + t.Fatalf("failed to normalize legacy result: %v", err) + } }
97-101: Error message nit: say “prestate tracer” instead of “call tracer”.The created tracer here is the prestate tracer.
- t.Fatalf("failed to create call tracer: %v", err) + t.Fatalf("failed to create prestate tracer: %v", err)
102-113: Optional: add a nil-check guard before invoking tracer hooks.Defensive check helps catch mis-registered tracers early.
- tracer.OnTxStart(evm.GetVMContext(), tx, msg.From()) + if tracer.Hooks == nil || tracer.OnTxStart == nil || tracer.OnTxEnd == nil { + t.Fatalf("tracer hooks not initialized") + } + tracer.OnTxStart(evm.GetVMContext(), tx, msg.From())eth/tracers/internal/tracetest/testdata/call_tracer/inner_instafail.json (1)
59-68: Schema consistency: add explicit top-level "output" (if still expected) and confirm inner gasUsed semantics
- The inner CALL frame looks correct (2300 stipend shown as gas 0x8fc, EOA target, and “insufficient balance for transfer” error).
- If consumers still expect an explicit top-level
"output": "0x"on success-with-no-return, add it for consistency; otherwise ensure downstream readers/tests were updated to handle its absence.- Double-check that reporting
gasUsed: "0x0"for a precheck-failed value transfer is the intended tracer behavior.Apply if needed:
@@ - "input": "0x2e1a7d4d00000000000000000000000000000000000000000000000014d1120d7b160000", + "input": "0x2e1a7d4d00000000000000000000000000000000000000000000000014d1120d7b160000", + "output": "0x", "calls": [{eth/tracers/internal/tracetest/testdata/call_tracer/delegatecall.json (1)
88-95: Top-level CALL missingoutput— consider adding"output": "0x"for schema consistency.Most callTracer frames include
outputeven if empty. Omitting it can break strict schema validations. Suggest adding an explicit empty output.Apply:
"input": "0x7065cb480000000000000000000000001523e55a1ca4efbae03355775ae89f8d7699ad9e", "to": "0x269296dddce321a6bcbaa2f0181127593d732cba", "type": "CALL", + "output": "0x", "value": "0x0"eth/tracers/internal/tracetest/testdata/call_tracer/inner_throw_outer_revert.md (2)
1-2: Tighten wording.“ This test tests out …” is redundant. Prefer concise phrasing.
Apply:
-This test tests out the trace generated by the deployment of this contract: +This test validates the trace generated by deploying the following contract:
18-19: Improve grammar and clarity.Use “top-level call” and avoid “as well as”.
Apply:
-The trace should show a revert, with the revert reason for both the top-call as well -as the inner call. +The trace should show a revert, including the revert reason for both the top-level call +and the inner call.core/vm/errors.go (2)
98-107: Avoid double-wrapping VMErrorIf err is already *VMError, return it as-is to keep code stable and error chains shallow.
func VMErrorFromErr(err error) error { if err == nil { return nil } - return &VMError{ - error: err, - code: vmErrorCodeFromErr(err), - } + if v, ok := err.(*VMError); ok { + return v + } + return &VMError{error: err, code: vmErrorCodeFromErr(err)} }
143-146: Reserve a small sentinel for VMErrorCodeUnknown
math.MaxInt-1 can surprise consumers expecting error codes in a small, fixed-width range (e.g. JSON-RPC). Use 0 or a smaller high-value (e.g. 1<<30) instead and verify downstream serialization/RPC layers handle it correctly.core/vm/contracts_test.go (2)
343-350: Use os.ReadFile; ioutil is deprecatedModernize file reads in tests.
-import "io/ioutil" +import "os" @@ - data, err := ioutil.ReadFile(fmt.Sprintf("testdata/precompiles/%v.json", name)) + data, err := os.ReadFile(fmt.Sprintf("testdata/precompiles/%v.json", name)) @@ - data, err := ioutil.ReadFile(fmt.Sprintf("testdata/precompiles/fail-%v.json", name)) + data, err := os.ReadFile(fmt.Sprintf("testdata/precompiles/fail-%v.json", name))Also applies to: 352-360
227-235: Typo: "identiy" → "identity"Minor comment fix.
-// Benchmarks the sample inputs from the identiy precompile. +// Benchmarks the sample inputs from the identity precompile.eth/tracers/api_blocktrace.go (2)
93-96: Silently discarding config.Tracer is a breaking API behavior change.Setting
config.Tracer = niland just warning can lead to surprising results for API clients. Prefer returning a 400-style error to the caller when an unsupported param is supplied, or explicitly mark the field deprecated in the type’s doc and ignore only if it’s empty.If you keep ignoring it, please add a test asserting that providing
Tracercauses a deterministic error response.
95-95: Minor wording nit in warning.“Tracer params is unsupported” → “Tracer param is unsupported” or “Tracer params are unsupported”.
eth/api_test.go (1)
82-83: Reason value looks acceptable; consider a more specific reason if available.
tracing.BalanceChangeUnspecifiedcompiles (per snippet), but if there’s a dedicated reason for state-test initialization, prefer that for clearer traces.cmd/evm/staterunner.go (1)
70-71: tracerFromFlags must preserve prior Debug/Machine semantics.Switching to
Tracer: tracerFromFlags(ctx)is fine iftracerFromFlagsmaps the old flags to an equivalent tracer. Verify that:
--debugstill produces per-op traces (previously viavm.Config.Debug+ StructLogger).--json --machineoutput remains unchanged.If you intend to deprecate
vm.Config.Debug, update help text/docs and add a fallback intracerFromFlagsto construct a StructLogger when--debugis set but no explicit tracer is requested.core/vm/runtime/runtime.go (3)
128-130: Include GasPrice when faking the tx for OnTxStart to avoid nil access in tracers.Some tracers read GasPrice; keep it non-nil.
- cfg.EVMConfig.Tracer.OnTxStart(vmenv.GetVMContext(), types.NewTx(&types.LegacyTx{To: &address, Data: input, Value: cfg.Value, Gas: cfg.GasLimit}), cfg.Origin) + cfg.EVMConfig.Tracer.OnTxStart( + vmenv.GetVMContext(), + types.NewTx(&types.LegacyTx{ + To: &address, + Data: input, + Value: cfg.Value, + Gas: cfg.GasLimit, + GasPrice: cfg.GasPrice, + }), + cfg.Origin, + )
165-167: Creation path: populate GasPrice in the synthetic LegacyTx too.Consistency with Execute and to avoid tracer assumptions.
- cfg.EVMConfig.Tracer.OnTxStart(vmenv.GetVMContext(), types.NewTx(&types.LegacyTx{Data: input, Value: cfg.Value, Gas: cfg.GasLimit}), cfg.Origin) + cfg.EVMConfig.Tracer.OnTxStart( + vmenv.GetVMContext(), + types.NewTx(&types.LegacyTx{ + Data: input, + Value: cfg.Value, + Gas: cfg.GasLimit, + GasPrice: cfg.GasPrice, + }), + cfg.Origin, + )
197-199: Call path: include GasPrice in the synthetic tx passed to the tracer.Prevents nil deref risks in tracer code paths that inspect price.
- cfg.EVMConfig.Tracer.OnTxStart(vmenv.GetVMContext(), types.NewTx(&types.LegacyTx{To: &address, Data: input, Value: cfg.Value, Gas: cfg.GasLimit}), cfg.Origin) + cfg.EVMConfig.Tracer.OnTxStart( + vmenv.GetVMContext(), + types.NewTx(&types.LegacyTx{ + To: &address, + Data: input, + Value: cfg.Value, + Gas: cfg.GasLimit, + GasPrice: cfg.GasPrice, + }), + cfg.Origin, + )eth/tracers/internal/tracetest/testdata/call_tracer_flat/nested_create_inerror.json (1)
79-86: Normalize “init” for the failing CREATE.Given the CREATE fails with “insufficient balance,” consider using an empty init field ("0x") to avoid implying a 32-byte payload of zeros.
- "init": "0x0000000000000000000000000000000000000000000000000000000000000000", + "init": "0x",core/vm/contracts.go (1)
47-49: Fix docstring typo (“RequiredPrice” → “RequiredGas”).- RequiredGas(input []byte) uint64 // RequiredPrice calculates the contract gas use + RequiredGas(input []byte) uint64 // RequiredGas calculates the contract gas usecore/vm/contract.go (2)
180-190: Add GoDoc and keep overflow invariant in mind.Consider a short comment documenting when RefundGas is used and that gas is uint64-bounded by EVM invariants.
-// RefundGas refunds gas to the contract +// RefundGas refunds gas to the contract and emits a gas-change event (if enabled). +// Note: EVM invariants guarantee c.Gas+gas does not overflow uint64. func (c *Contract) RefundGas(gas uint64, logger *tracing.Hooks, reason tracing.GasChangeReason) {
173-187: Optional: factor guard into a tiny helper to reduce repetition.Minor DRY: a private helper like emitGasChange(logger, old, new, reason) could centralize the nil/reason checks.
core/vm/interface.go (2)
41-41: SetCode now returns []byte (previous code?). Document semantics.Clarify whether the return is old or new code and whether callers may mutate it.
51-51: SetState now returns common.Hash (previous value?). Document semantics.Add brief comments to the interface to prevent misuse.
core/vm/operations_acl.go (3)
249-262: Micro-opt: check code size before fetching code bytesParseDelegation only succeeds on 23-byte payloads. Avoid fetching full code unless size matches.
- if target, ok := types.ParseDelegation(evm.StateDB.GetCode(addr)); ok { + if evm.StateDB.GetCodeSize(addr) == 23 { + if target, ok := types.ParseDelegation(evm.StateDB.GetCode(addr)); ok { ... - } + } + }
249-262: Use a distinct GasChange reason for delegation resolutionReusing GasChangeCallStorageColdAccess for delegation target costs may confuse tracers. Consider adding GasChangeCallDelegationResolution (warm/cold) to disambiguate.
Happy to add the new enum and thread it through tracing.Hooks if desired.
274-285: Rename shadowed variable for clarityShadowing total in SafeAdd makes the final return less readable. Use sum to avoid confusion.
- var overflow bool - if total, overflow = math.SafeAdd(old, total); overflow { + var ( + sum uint64 + overflow bool + ) + if sum, overflow = math.SafeAdd(old, total); overflow { return 0, ErrGasUintOverflow } - return total, nil + return sum, nileth/tracers/internal/tracetest/flat_calltrace_test.go (2)
166-174: Parallel subtests: capture loop variable to avoid flakiness on older Got.Run closures can capture the loop variable. Defensive copy avoids intermittent failures on Go versions before 1.22.
- t.Run(camel(strings.TrimSuffix(file.Name(), ".json")), func(t *testing.T) { + name := file.Name() + t.Run(camel(strings.TrimSuffix(name, ".json")), func(t *testing.T) { t.Parallel() - - err := flatCallTracerTestRunner(tracerName, file.Name(), dirPath, t) + err := flatCallTracerTestRunner(tracerName, name, dirPath, t) if err != nil { t.Fatal(err) } })
104-121: Consider deferring tracer termination if the implementation requires itSome tracers may buffer and flush on Stop. If the tracer returned by DefaultDirectory.New sets Tracer.Stop, consider deferring it to ensure resources are released.
- tracer, err := tracers.DefaultDirectory.New(tracerName, new(tracers.Context), test.TracerConfig, test.Genesis.Config) + tracer, err := tracers.DefaultDirectory.New(tracerName, new(tracers.Context), test.TracerConfig, test.Genesis.Config) if err != nil { return fmt.Errorf("failed to create call tracer: %v", err) } + if tracer.Stop != nil { + defer tracer.Stop(nil) + }cmd/evm/internal/t8ntool/transition.go (2)
221-245: Tracer setup path: ensure tracer context isn’t required (nil passed)You pass nil for the tracers.Context. If any tracer expects non-nil contextual inputs, this may panic. Consider passing &tracers.Context{} for safety, mirroring the test harness.
- tracer, err := tracers.DefaultDirectory.New(ctx.String(TraceTracerFlag.Name), - nil, config, chainConfig) + tracer, err := tracers.DefaultDirectory.New(ctx.String(TraceTracerFlag.Name), + new(tracers.Context), config, chainConfig)
361-371: Replace deprecated ioutil.WriteFileUse os.WriteFile to avoid deprecated API and redundant import.
- if err = ioutil.WriteFile(location, b, 0644); err != nil { + if err = os.WriteFile(location, b, 0644); err != nil {And remove the ioutil import.
core/state/statedb_hooked.go (4)
37-44: Guard against nil StateDB in constructorAvoid latent panics from nil receivers; validate input.
func NewHookedState(stateDb *StateDB, hooks *tracing.Hooks) *hookedStateDB { - s := &hookedStateDB{stateDb, hooks} + if stateDb == nil { + panic("NewHookedState: stateDb is nil") + } + s := &hookedStateDB{stateDb, hooks} if s.hooks == nil { s.hooks = new(tracing.Hooks) } return s }
170-180: Avoid aliasing code slices passed to hooksHooks should not observe future mutations; pass copies.
func (s *hookedStateDB) SetCode(address common.Address, code []byte) []byte { prev := s.inner.SetCode(address, code) if s.hooks.OnCodeChange != nil { prevHash := codehash.EmptyKeccakCodeHash if len(prev) != 0 { prevHash = crypto.Keccak256Hash(prev) } - s.hooks.OnCodeChange(address, prevHash, prev, crypto.Keccak256Hash(code), code) + prevCopy := append([]byte(nil), prev...) + codeCopy := append([]byte(nil), code...) + s.hooks.OnCodeChange(address, prevHash, prevCopy, crypto.Keccak256Hash(codeCopy), codeCopy) } return prev }
210-224: Event ordering of selfdestruct-burn vs FinaliseYou emit BalanceDecreaseSelfdestructBurn before inner.Finalise runs (due to defer). Verify consumers expect the event pre-commit. If not, move the hook after Finalise by snapshotting addresses first.
142-149: Current balance‐hook logic is correct; optional future-proof refactorThe existing
AddBalance/SubBalanceimplementations return the previous balance and compute the post‐state viaprev ± amount, which matches the actual new value (they allocate newbig.Intinstances and don’t mutate the old one in place, and the zero‐amount branch is skipped by the hook). No changes are required for correctness.If you’d like to guard against any future in-place mutations or clamping semantics, you can optionally clone both values and re-read the post-state before invoking the hook:
prev := s.inner.SubBalance(addr, amount, reason) if s.hooks.OnBalanceChange != nil && amount.Sign() != 0 { prevCopy := new(big.Int).Set(prev) newBalance := new(big.Int).Set(s.inner.GetBalance(addr)) s.hooks.OnBalanceChange(addr, prevCopy, newBalance, reason) } return preveth/tracers/dir.go (3)
38-45: Comment mismatch: Tracer is a struct, not an interfaceTighten the doc to avoid confusion.
-// Tracer interface extends vm.EVMLogger and additionally -// allows collecting the tracing result. +// Tracer aggregates tracing hooks and result/control functions +// for collecting and controlling a trace. type Tracer struct {
92-101: Consider jsEval presence in IsJSUnknown name should only be “JS” if an evaluator exists.
func (d *directory) IsJS(name string) bool { if elem, ok := d.elems[name]; ok { return elem.isJS } - // JS eval will execute JS code - return true + return d.jsEval != nil }
55-76: Registry not concurrency-safeIf tracers are registered at init() only, you’re fine. Otherwise add a RWMutex around elems/jsEval reads/writes.
I can send a follow-up diff adding sync.RWMutex if you expect concurrent Register/New calls.
Also applies to: 66-71, 78-90, 92-101
eth/tracers/api_test.go (2)
255-268: Result schema change acknowledged; consider normalizing l1DataFee expectation.Some cases omit l1DataFee while others expect "0x0". To avoid flakiness from omitempty behavior, prefer asserting essential fields only or include l1DataFee consistently.
335-337: Prefer errors.Is over string equality for error checks.String matching is brittle and locale/format dependent. Use errors.Is when possible.
- if !reflect.DeepEqual(err.Error(), testspec.expectErr.Error()) { + if !errors.Is(err, testspec.expectErr) {core/state/state_object.go (2)
451-460: Document SetBalance return value explicitly (previous balance).The behavior is fine, but the doc should state “returns the previous balance” to avoid misuse.
-// SetBalance sets the balance for the object, and returns the previous balance. +// SetBalance sets the balance for the object and returns the previous balance.
513-521: SetCode returns previous code — OK; add doc for clarity.- func (s *stateObject) SetCode(code []byte) []byte { + // SetCode sets the code and returns the previous code bytes. + func (s *stateObject) SetCode(code []byte) []byte {core/state_processor.go (1)
188-195: Receipt fields population (logs, bloom, indices, L1Fee) looks correct.Consider whether EffectiveGasPrice is set here or derived later via types.DeriveFields; ensure consistency across readers.
cmd/evm/internal/t8ntool/flags.go (2)
34-41: Consistent flag style and validation.
- Mixed pointer/value usage for flags (pointers here; values elsewhere). Consider standardizing for readability.
- If
--trace.jsonconfigmust be JSON, validate early (flag parse time) and fail fast with a clear error.
54-57: Document callframes interplay with other trace flags.Note when
trace.callframesis effective (which tracers honor it) and how it interacts with stack/memory/returndata toggles.eth/tracers/internal/tracetest/calltrace_test.go (4)
133-134: Guard optional hooks to avoid nil-call panics.Some tracers may not provide
OnTxStart.- tracer.OnTxStart(evm.GetVMContext(), tx, msg.From()) + if tracer.OnTxStart != nil { + tracer.OnTxStart(evm.GetVMContext(), tx, msg.From()) + }
138-139: Symmetric guard for OnTxEnd.Avoid nil-call panics and keep symmetry with start.
- tracer.OnTxEnd(&types.Receipt{GasUsed: vmRet.UsedGas}, nil) + if tracer.OnTxEnd != nil { + tracer.OnTxEnd(&types.Receipt{GasUsed: vmRet.UsedGas}, nil) + }
205-205: Prefer time-aware signer for determinism across fork rules.Align with the repo-wide change to a time-aware signer.
- signer := types.MakeSigner(test.Genesis.Config, new(big.Int).SetUint64(uint64(test.Context.Number)), uint64(test.Context.Time)) + signer := types.MakeSigner(test.Genesis.Config, new(big.Int).SetUint64(uint64(test.Context.Number)), uint64(test.Context.Time))Note: Already correct here; this is just a confirmation. If you reintroduce
LatestSignerelsewhere, preferMakeSigner.
248-249: Use MakeSigner with block/time in TestInternals.Keeps tests consistent with time-aware signer logic.
- signer = types.LatestSigner(config) + signer = types.MakeSigner(config, context.BlockNumber, context.Time.Uint64())cmd/evm/main.go (2)
206-218: Remove or wire up unused traceFlags.
traceFlagsisn’t referenced in this file. Drop it or use it to group flags where needed.
289-291: Avoid os.Exit in helper; bubble up error instead.Returning an error keeps CLI wiring testable and avoids abrupt exits from callers.
- fmt.Fprintf(os.Stderr, "unknown trace format: %q\n", format) - os.Exit(1) - return nil + panic(fmt.Errorf("unknown trace format: %q", format))Or change signature to return
(*tracing.Hooks, error)and handle upstream.core/state/statedb.go (5)
416-423: Remove unreachable nil-check; consider reason handling.
GetOrNewStateObjectnever returns nil; the guard is dead code. Also, thereasonarg is unused here — if hooks aren’t injected elsewhere, this drops tracing semantics.Apply:
func (s *StateDB) AddBalance(addr common.Address, amount *big.Int, reason tracing.BalanceChangeReason) *big.Int { - stateObject := s.GetOrNewStateObject(addr) - if stateObject == nil { - return new(big.Int) - } - return stateObject.AddBalance(amount) + stateObject := s.GetOrNewStateObject(addr) + return stateObject.AddBalance(amount) }Please confirm whether balance-change hooks are fired in
stateObjector in a hooked wrapper (e.g., statedb_hooked.go). If not, we need to wirereasonto the hook path.
426-435: Same here: remove unreachable nil-check; early return on zero is fine.This simplifies the control flow and avoids redundant branches.
func (s *StateDB) SubBalance(addr common.Address, amount *big.Int, reason tracing.BalanceChangeReason) *big.Int { - stateObject := s.GetOrNewStateObject(addr) - if stateObject == nil { - return new(big.Int) - } + stateObject := s.GetOrNewStateObject(addr) if amount.Sign() == 0 { return stateObject.Balance() } return stateObject.SubBalance(amount) }
437-442: Nonce/balance reasons are ignored here.
SetBalancedoes not propagatereason. If the hook wiring isn’t in a wrapper, the tracer will miss cause attribution.Would you like me to patch these setters to call the appropriate
logger.OnBalanceChange/OnNonceChangehooks if present?
444-449: Nonce change reason is unused.Same concern as above; please confirm where the reason is consumed.
451-457: Return value OK, but confirm hook path for code changes.If code-change events should be observable, ensure
OnCodeChangeis emitted somewhere (either here or instateObject.SetCodeor the hooked wrapper).core/vm/evm.go (1)
312-317: Remove stale commented debug tracer block.Dead code comments add noise and can diverge from actual behavior.
- // if evm.Config.Debug { - // evm.Config.Tracer.CaptureEnter(DELEGATECALL, caller.Address(), addr, input, gas, nil) - // defer func(startGas uint64) { - // evm.Config.Tracer.CaptureExit(ret, startGas-gas, err) - // }(gas) - // }core/state_transition.go (2)
134-185: Charge gas for auth list — add overflow check for symmetry.Mirror the overflow guards used for data and access lists.
- if authList != nil { - gas += uint64(len(authList)) * params.CallNewAccountGas - } + if authList != nil { + if (math.MaxUint64-gas)/params.CallNewAccountGas < uint64(len(authList)) { + return 0, ErrGasUintOverflow + } + gas += uint64(len(authList)) * params.CallNewAccountGas + }
302-306: EOA check: avoid loading full code unless needed.Use
GetCodeSize(from)first; only fetch code to parse delegation if size > 0.- code := st.state.GetCode(st.msg.From()) - _, delegated := types.ParseDelegation(code) - if len(code) > 0 && !delegated { + if st.state.GetCodeSize(st.msg.From()) > 0 { + code := st.state.GetCode(st.msg.From()) + if _, delegated := types.ParseDelegation(code); !delegated { return fmt.Errorf("%w: address %v, len(code): %d", ErrSenderNoEOA, st.msg.From().Hex(), len(code)) - } + } + }core/vm/interpreter.go (1)
276-281: Gas handling: propagate dynamic-gas errors as-is.Wrapping every dynamic-gas error as
ErrOutOfGasmay mask different error causes. Prefer returning the original error unless it's specifically an OOG condition.- if err != nil { - return nil, fmt.Errorf("%w: %v", ErrOutOfGas, err) - } + if err != nil { + return nil, err + }Also applies to: 299-315
eth/tracers/api.go (2)
771-779: Use modernoshelpers; drop deprecatedioutil.Minor cleanup.
- "io/ioutil" + // (no new import needed; already importing "os") @@ - blob, err := ioutil.ReadFile(file) + blob, err := os.ReadFile(file) @@ - dump, err = ioutil.TempFile(os.TempDir(), prefix) + dump, err = os.CreateTemp(os.TempDir(), prefix)Also applies to: 482-486, 19-32
300-309: TraceChain error flow: continue vs. break semantics.On L1 fee calc or tracing error you
break, skipping remaining txs in the block. If partial results are acceptable, considercontinueand record the error at this index.core/tracing/hooks.go (7)
131-135: Doc mismatch: mentions td parameter that no longer existsComment references td but BlockStartHook only takes BlockEvent. Clarify or remove the TD mention.
Apply:
-// `td` is the total difficulty prior to `block`. +// Provides block and its consensus context via BlockEvent.
161-164: Fix OnSystemCallEndHook wordingIt says “about to be executed” for the end hook.
-// this hook is invoked when the EIP-4788 system call is about to be executed to set the -// beacon block root. +// this hook is invoked after the EIP-4788 system call finishes executing to set the +// beacon block root.
324-325: Minor grammar: “can be check”-// performed can be check by `OnOpcode` handling. +// performed can be checked via `OnOpcode` handling.
244-245: Minor grammar: BalanceDecreaseGasBuy-// BalanceDecreaseGasBuy is spent to purchase gas for execution a transaction. -// Part of this gas will be burnt as per EIP-1559 rules. +// BalanceDecreaseGasBuy is spent to purchase gas for executing a transaction. +// Part of this gas will be burned as per EIP-1559 rules.
170-175: Deprecate v1 nonce hook in docs to steer new integrationsGiven V2 exists, mark V1 as deprecated to reduce ambiguity.
-// NonceChangeHook is called when the nonce of an account changes. +// NonceChangeHook is called when the nonce of an account changes. +// Deprecated: use NonceChangeHookV2 to receive a reason.
146-156: Explicitly deprecate OnSystemCallStartHook in favor of V2V2 provides VM context; mark V1 deprecated.
-// OnSystemCallStartHook is called when a system call is about to be executed. Today, +// OnSystemCallStartHook is called when a system call is about to be executed. Today, // this hook is invoked when the EIP-4788 system call is about to be executed to set the // beacon block root. +// Deprecated: use OnSystemCallStartHookV2 to access VM context.Also applies to: 157-160
98-99: Use a strongly-typed call kind instead of byteImproves clarity and type-safety for EnterHook.
-EnterHook = func(depth int, typ byte, from common.Address, to common.Address, input []byte, gas uint64, value *big.Int) +// CallKind enumerates the kind of call/create. +type CallKind byte +const ( + Call CallKind = iota + CallCode + DelegateCall + StaticCall + Create + Create2 +) +EnterHook = func(depth int, typ CallKind, from common.Address, to common.Address, input []byte, gas uint64, value *big.Int)
| func (l *fileWritingTracer) Write(p []byte) (n int, err error) { | ||
| if l.destination != nil { | ||
| return l.destination.Write(p) | ||
| } | ||
| log.Warn("Tracer wrote to non-existing output") | ||
| // It is tempting to return an error here, however, the json encoder | ||
| // will no retry writing to an io.Writer once it has returned an error once. | ||
| // Therefore, we must squash the error. | ||
| return n, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return len(p) when dropping writes to avoid Writer contract ambiguity.
Returning 0, nil can confuse callers; signal that bytes were “accepted” even if ignored.
- return n, nil
+ return len(p), nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (l *fileWritingTracer) Write(p []byte) (n int, err error) { | |
| if l.destination != nil { | |
| return l.destination.Write(p) | |
| } | |
| log.Warn("Tracer wrote to non-existing output") | |
| // It is tempting to return an error here, however, the json encoder | |
| // will no retry writing to an io.Writer once it has returned an error once. | |
| // Therefore, we must squash the error. | |
| return n, nil | |
| } | |
| func (l *fileWritingTracer) Write(p []byte) (n int, err error) { | |
| if l.destination != nil { | |
| return l.destination.Write(p) | |
| } | |
| log.Warn("Tracer wrote to non-existing output") | |
| // It is tempting to return an error here, however, the json encoder | |
| // will no retry writing to an io.Writer once it has returned an error once. | |
| // Therefore, we must squash the error. | |
| return len(p), nil | |
| } |
🤖 Prompt for AI Agents
In cmd/evm/internal/t8ntool/file_tracer.go around lines 48 to 57, the Write
method currently returns the zero-valued n when dropping writes which can
violate io.Writer expectations; change the return so it reports the number of
bytes accepted by returning len(p), nil (or set n = len(p) before returning)
when l.destination is nil to clearly signal the bytes were "accepted" even
though they are ignored.
| // Open a new file, or print a warning log if it's failed | ||
| fname := filepath.Join(l.baseDir, fmt.Sprintf("trace-%d-%v.%v", l.txIndex, tx.Hash().String(), l.suffix)) | ||
| traceFile, err := os.Create(fname) | ||
| if err != nil { | ||
| log.Warn("Failed creating trace-file", "err", err) | ||
| } else { | ||
| log.Info("Created tracing-file", "path", fname) | ||
| l.destination = traceFile | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Create baseDir before file create; minor filename formatting.
Ensure directory exists to avoid silent drops; use %s for hash.
- fname := filepath.Join(l.baseDir, fmt.Sprintf("trace-%d-%v.%v", l.txIndex, tx.Hash().String(), l.suffix))
- traceFile, err := os.Create(fname)
+ fname := filepath.Join(l.baseDir, fmt.Sprintf("trace-%d-%s.%s", l.txIndex, tx.Hash(), l.suffix))
+ if mkErr := os.MkdirAll(l.baseDir, 0o755); mkErr != nil {
+ log.Warn("Failed ensuring trace directory", "dir", l.baseDir, "err", mkErr)
+ }
+ traceFile, err := os.Create(fname)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Open a new file, or print a warning log if it's failed | |
| fname := filepath.Join(l.baseDir, fmt.Sprintf("trace-%d-%v.%v", l.txIndex, tx.Hash().String(), l.suffix)) | |
| traceFile, err := os.Create(fname) | |
| if err != nil { | |
| log.Warn("Failed creating trace-file", "err", err) | |
| } else { | |
| log.Info("Created tracing-file", "path", fname) | |
| l.destination = traceFile | |
| } | |
| // Open a new file, or print a warning log if it fails | |
| fname := filepath.Join(l.baseDir, fmt.Sprintf("trace-%d-%s.%s", l.txIndex, tx.Hash(), l.suffix)) | |
| if mkErr := os.MkdirAll(l.baseDir, 0o755); mkErr != nil { | |
| log.Warn("Failed ensuring trace directory", "dir", l.baseDir, "err", mkErr) | |
| } | |
| traceFile, err := os.Create(fname) | |
| if err != nil { | |
| log.Warn("Failed creating trace-file", "err", err) | |
| } else { | |
| log.Info("Created tracing-file", "path", fname) | |
| l.destination = traceFile | |
| } |
🤖 Prompt for AI Agents
In cmd/evm/internal/t8ntool/file_tracer.go around lines 85 to 93, the code
creates a trace file without ensuring the baseDir exists and formats the
filename using %v for the tx hash; update it to: ensure l.baseDir exists by
calling os.MkdirAll(l.baseDir, 0o755) and handling its error before calling
os.Create, change fmt.Sprintf to use "%s" for tx.Hash().String(), and preserve
logging on both MkdirAll and os.Create failures so the trace isn't silently
dropped.
| func (l *fileWritingTracer) OnTxEnd(receipt *types.Receipt, err error) { | ||
| if l.inner != nil && l.inner.OnTxEnd != nil { | ||
| l.inner.OnTxEnd(receipt, err) | ||
| } | ||
| if l.getResult != nil && l.destination != nil { | ||
| if result, err := l.getResult(); result != nil { | ||
| json.NewEncoder(l.destination).Encode(result) | ||
| } else { | ||
| log.Warn("Error obtaining tracer result", "err", err) | ||
| } | ||
| l.destination.Close() | ||
| l.destination = nil | ||
| } | ||
| l.txIndex++ | ||
| } |
There was a problem hiding this comment.
Always close the per-tx file; current logic leaks FDs when getResult is nil.
When using newFileWriter (jsonl), getResult is nil, so the file never closes. This leaks descriptors and can truncate subsequent writes.
Apply this diff to always close and to handle result/encoding errors:
func (l *fileWritingTracer) OnTxEnd(receipt *types.Receipt, err error) {
if l.inner != nil && l.inner.OnTxEnd != nil {
l.inner.OnTxEnd(receipt, err)
}
- if l.getResult != nil && l.destination != nil {
- if result, err := l.getResult(); result != nil {
- json.NewEncoder(l.destination).Encode(result)
- } else {
- log.Warn("Error obtaining tracer result", "err", err)
- }
- l.destination.Close()
- l.destination = nil
- }
+ if l.destination != nil {
+ if l.getResult != nil {
+ if result, rerr := l.getResult(); rerr != nil {
+ log.Warn("Error obtaining tracer result", "err", rerr)
+ } else if result != nil {
+ if encErr := json.NewEncoder(l.destination).Encode(result); encErr != nil {
+ log.Warn("Error writing tracer result", "err", encErr)
+ }
+ }
+ }
+ if cerr := l.destination.Close(); cerr != nil {
+ log.Warn("Failed closing trace-file", "err", cerr)
+ }
+ l.destination = nil
+ }
l.txIndex++
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (l *fileWritingTracer) OnTxEnd(receipt *types.Receipt, err error) { | |
| if l.inner != nil && l.inner.OnTxEnd != nil { | |
| l.inner.OnTxEnd(receipt, err) | |
| } | |
| if l.getResult != nil && l.destination != nil { | |
| if result, err := l.getResult(); result != nil { | |
| json.NewEncoder(l.destination).Encode(result) | |
| } else { | |
| log.Warn("Error obtaining tracer result", "err", err) | |
| } | |
| l.destination.Close() | |
| l.destination = nil | |
| } | |
| l.txIndex++ | |
| } | |
| func (l *fileWritingTracer) OnTxEnd(receipt *types.Receipt, err error) { | |
| if l.inner != nil && l.inner.OnTxEnd != nil { | |
| l.inner.OnTxEnd(receipt, err) | |
| } | |
| if l.destination != nil { | |
| if l.getResult != nil { | |
| if result, rerr := l.getResult(); rerr != nil { | |
| log.Warn("Error obtaining tracer result", "err", rerr) | |
| } else if result != nil { | |
| if encErr := json.NewEncoder(l.destination).Encode(result); encErr != nil { | |
| log.Warn("Error writing tracer result", "err", encErr) | |
| } | |
| } | |
| } | |
| if cerr := l.destination.Close(); cerr != nil { | |
| log.Warn("Failed closing trace-file", "err", cerr) | |
| } | |
| l.destination = nil | |
| } | |
| l.txIndex++ | |
| } |
🤖 Prompt for AI Agents
In cmd/evm/internal/t8ntool/file_tracer.go around lines 101 to 115, the current
OnTxEnd logic skips closing the per-transaction file when getResult is nil
causing FD leaks; always close and nil out l.destination regardless of
getResult, and avoid shadowing the outer err variable. Modify the block so it:
1) checks l.destination != nil and defers or calls l.destination.Close() and
sets l.destination = nil immediately; 2) calls l.getResult() if non-nil and
captures its error into a new local variable name (not err) so it doesn’t shadow
the method parameter; 3) if a result is returned, attempt json.Encode and log
any encoding error; 4) if getResult returns nil or an error, log the error with
context. Ensure closing happens even when encoding or getResult fails.
| func (l *fileWritingTracer) hooks() *tracing.Hooks { | ||
| return &tracing.Hooks{ | ||
| OnTxStart: l.OnTxStart, | ||
| OnTxEnd: l.OnTxEnd, | ||
| OnEnter: func(depth int, typ byte, from common.Address, to common.Address, input []byte, gas uint64, value *big.Int) { | ||
| if l.inner != nil && l.inner.OnEnter != nil { | ||
| l.inner.OnEnter(depth, typ, from, to, input, gas, value) | ||
| } | ||
| }, | ||
| OnExit: func(depth int, output []byte, gasUsed uint64, err error, reverted bool) { | ||
| if l.inner != nil && l.inner.OnExit != nil { | ||
| l.inner.OnExit(depth, output, gasUsed, err, reverted) | ||
| } | ||
| }, | ||
| OnOpcode: func(pc uint64, op byte, gas, cost uint64, scope tracing.OpContext, rData []byte, depth int, err error) { | ||
| if l.inner != nil && l.inner.OnOpcode != nil { | ||
| l.inner.OnOpcode(pc, op, gas, cost, scope, rData, depth, err) | ||
| } | ||
| }, | ||
| OnFault: func(pc uint64, op byte, gas, cost uint64, scope tracing.OpContext, depth int, err error) { | ||
| if l.inner != nil && l.inner.OnFault != nil { | ||
| l.inner.OnFault(pc, op, gas, cost, scope, depth, err) | ||
| } | ||
| }, | ||
| OnSystemCallStart: func() { | ||
| if l.inner != nil && l.inner.OnSystemCallStart != nil { | ||
| l.inner.OnSystemCallStart() | ||
| } | ||
| }, | ||
| OnSystemCallEnd: func() { | ||
| if l.inner != nil && l.inner.OnSystemCallEnd != nil { | ||
| l.inner.OnSystemCallEnd() | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pass through all inner hooks; current wrapper drops many (GasChange, Block, State, etc.).**
Only a subset is forwarded, causing lost events when using this wrapper.
Would you like me to add passthroughs for the remaining hooks (OnGasChange, OnBlockchainInit, OnClose, OnBlockStart/End/Skipped/Genesis, OnSystemCallStartV2, OnBalanceChange, OnNonceChange/OnNonceChangeV2, OnCodeChange, OnStorageChange, OnLog, OnBlockHashRead)? I can provide a patch once you confirm desired coverage.
🤖 Prompt for AI Agents
In cmd/evm/internal/t8ntool/file_tracer.go around lines 117 to 151, the hooks
wrapper only forwards a subset of tracing.Hooks and drops many events
(GasChange, Blockchain/Block lifecycle hooks, State changes, SystemCallStartV2,
Balance/Nonce/Code/Storage changes, Log, BlockHashRead, OnClose, etc.); update
the hooks() function to passthrough every hook field from l.inner when non-nil
by adding forwarding lambdas for OnGasChange, OnBlockchainInit, OnClose,
OnBlockStart, OnBlockEnd, OnBlockSkipped, OnBlockGenesis, OnSystemCallStartV2,
OnBalanceChange, OnNonceChange, OnNonceChangeV2, OnCodeChange, OnStorageChange,
OnLog, OnBlockHashRead (and any other missing fields present on tracing.Hooks)
following the same pattern used for existing hooks so no events are dropped.
| "stateRoot": "0xff403612573d76dfdaf4fea2429b77dbe9764021ae0e38dc8ac79a3cf551179e", | ||
| "timestamp": "1513681246", | ||
| "totalDifficulty": "7162347056825919" | ||
| "timestamp": "1513681246" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Genesis timestamp changed — confirm intent and cross-fixture consistency.
Parent block timestamp is now "1513681246". Verify this was intentional and remains consistent with any precomputed header fields and sibling fixtures. Unintended timestamp drift can invalidate deterministic trace expectations.
Run to sanity-check timestamps across all tracer fixtures and ensure no ordering violations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) No lingering totalDifficulty fields after cleanup
if rg -n '"totalDifficulty"' eth/tracers/internal/tracetest/testdata/**/*.json; then
echo "Found totalDifficulty occurrences above"; exit 1
else
echo "OK: no totalDifficulty in fixtures"
fi
# 2) Parent (genesis) timestamp must be <= context timestamp
fd -e json eth/tracers/internal/tracetest/testdata | while read -r f; do
gt=$(jq -r '.genesis.timestamp // empty' "$f")
ct=$(jq -r '.context.timestamp // empty' "$f")
if [[ -n "${gt}" && -n "${ct}" ]]; then
if (( gt > ct )); then
echo "Timestamp ordering violated in $f: genesis=$gt > context=$ct"
exit 1
fi
fi
done
echo "OK: timestamp ordering valid"Length of output: 8755
Remove lingering totalDifficulty fields from tracer fixtures
The following fixtures still contain "totalDifficulty" and must be cleaned up before validating timestamps:
- eth/tracers/internal/tracetest/testdata/prestate_tracer_with_diff_mode/simple_disable_code_and_storage.json:54
- eth/tracers/internal/tracetest/testdata/prestate_tracer_with_diff_mode/inner_create_disable_code_and_storage.json:13
- eth/tracers/internal/tracetest/testdata/prestate_tracer_with_diff_mode/create_disable_code.json:13
- eth/tracers/internal/tracetest/testdata/prestate_tracer_with_diff_mode/create_disable_storage.json:13
- eth/tracers/internal/tracetest/testdata/prestate_tracer/disable_storage.json:54
- eth/tracers/internal/tracetest/testdata/prestate_tracer/disable_code_and_storage.json:54
- eth/tracers/internal/tracetest/testdata/prestate_tracer/disable_code.json:54
- eth/tracers/internal/tracetest/testdata/call_tracer_flat/create_oog_parity.json:13
After removing these fields, re-run the sanity-check script and confirm that each fixture’s genesis timestamp ≤ context timestamp.
| "timestamp": "1513676127" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Genesis timestamp changed but genesis hash not updated — inconsistency risk
You modified "genesis.timestamp" (Line 44) without updating "genesis.hash" (Line 38). For genesis files, the hash is derived from header fields including the timestamp; changing the timestamp requires recomputing and updating the hash. Otherwise the fixture can become non-deterministic or rejected by tools that validate the genesis hash.
If the timestamp change is intentional, please recompute and replace "genesis.hash" accordingly; otherwise revert the timestamp to match the existing hash.
🏁 Script executed:
#!/bin/bash
# Display the genesis.hash and genesis.timestamp fields to verify consistency
grep -n '"genesis"' -A6 eth/tracers/internal/tracetest/testdata/call_tracer/create.jsonLength of output: 299
Recompute genesis.hash to match updated timestamp
File eth/tracers/internal/tracetest/testdata/call_tracer/create.json (lines 38 & 44): you modified genesis.timestamp without updating the derived genesis.hash; regenerate the hash to reflect the new timestamp or revert the timestamp to maintain fixture consistency.
🤖 Prompt for AI Agents
In eth/tracers/internal/tracetest/testdata/call_tracer/create.json around lines
38 and 44, the genesis.timestamp was changed but genesis.hash was not updated;
regenerate the genesis.hash to reflect the modified timestamp (or revert the
timestamp to its previous value) so the fixture remains consistent—compute the
new genesis hash using the same algorithm/serialization used for other fixtures
and replace the existing genesis.hash value in the file.
| ```solidity | ||
| contract Revertor { | ||
| function run() public pure { | ||
| require(2 > 3, "This called failed"); |
There was a problem hiding this comment.
Fix typo in revert message.
“This called failed” → “This call failed”.
Apply:
- require(2 > 3, "This called failed");
+ require(2 > 3, "This call failed");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require(2 > 3, "This called failed"); | |
| require(2 > 3, "This call failed"); |
🤖 Prompt for AI Agents
In
eth/tracers/internal/tracetest/testdata/call_tracer/inner_throw_outer_revert.md
around line 6, the revert message contains a typo "This called failed"; update
the string to "This call failed" so the require message is grammatically correct
and consistent.
| "gas": "0xca1d", | ||
| "gasUsed": "0xca1d", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM: gas/gasUsed now match the tx gas (0xca1d).
The updated expectations align with the encoded tx field (…82ca1d…). This should stabilize OOG tracing with the revised intrinsic gas rules.
Recommend running the tracer tests to ensure no other fixtures need updates:
🏁 Script executed:
#!/bin/bash
go test ./eth/tracers/internal/tracetest -run CallTracerLength of output: 16566
Restore tracer tests by aligning gas serialization and fixtures
- CallTracerNativeWithLog tests are failing: actual
gasUsedremains0x7160but fixtures expect0x1a9e5—update the OOG and related JSON fixtures undereth/tracers/internal/tracetestto the correct values. - FlatCallTracerNative tests are failing with unmarshal errors (
non-string into hexutil.Uint64): adjust the flat tracer output to emit numeric gas fields or update the Go struct to accept hex strings.
Re-run go test ./eth/tracers/internal/tracetest until all tracer tests pass.
🤖 Prompt for AI Agents
In eth/tracers/internal/tracetest/testdata/call_tracer/oog.json around lines
52-53, update the fixture gas and gasUsed fields from the expected 0x1a9e5 to
the actual 0x7160 so the JSON matches runtime output; additionally, fix the
FlatCallTracer output to emit gas fields as JSON numeric values (not hex
strings) so Go unmarshalling into hexutil.Uint64 succeeds (alternatively, modify
the Go struct to accept hex strings), then re-run go test
./eth/tracers/internal/tracetest until all tracer tests pass.
| "stateRoot": "0xc7f10f352bff82fac3c2999d3085093d12652e19c7fd32591de49dc5d91b4f1f", | ||
| "timestamp": "1513601261", | ||
| "totalDifficulty": "7143276353481064" | ||
| "timestamp": "1513601261" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm removal of genesis.totalDifficulty across fixtures and loader tolerance.
Only genesis.timestamp is shown as changed here, but the file no longer contains genesis.totalDifficulty. Ensure all fixtures are updated consistently and that any loader/schema logic tolerates the missing field.
Run to sanity-check all tracer fixtures:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find any remaining uses of genesis.totalDifficulty
fd -t f -e json 'eth/tracers/internal/tracetest/testdata' \
| xargs -I{} sh -c 'jq -e ".genesis.totalDifficulty" "{}" >/dev/null 2>&1 && echo "HAS totalDifficulty: {}" || true'
# 2) Ensure context.timestamp >= genesis.timestamp everywhere (numeric compare)
fd -t f -e json 'eth/tracers/internal/tracetest/testdata' \
| xargs -I{} jq -r '"{} "+((.context.timestamp|tonumber) >= (.genesis.timestamp|tonumber) | tostring)' \
| awk '$2!="true"{print "Timestamp order check failed for", $1}'Length of output: 658
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find any remaining uses of genesis.totalDifficulty
find eth/tracers/internal/tracetest/testdata -type f -name '*.json' -print0 \
| xargs -0 -I{} sh -c 'jq -e ".genesis.totalDifficulty" "{}" >/dev/null 2>&1 && echo "HAS totalDifficulty: {}"'
# 2) Ensure context.timestamp >= genesis.timestamp everywhere
find eth/tracers/internal/tracetest/testdata -type f -name '*.json' -print0 \
| xargs -0 -I{} sh -c 'jq -e "(.context.timestamp|tonumber) >= (.genesis.timestamp|tonumber)" "{}" >/dev/null || echo "Timestamp order check failed for {}"'Length of output: 1113
Remove genesis.totalDifficulty from tracer fixtures and update loader tolerance
- The following fixtures still include
genesis.totalDifficultyand need the field removed:- eth/tracers/internal/tracetest/testdata/call_tracer_flat/create_oog_parity.json
- eth/tracers/internal/tracetest/testdata/prestate_tracer/disable_code.json
- eth/tracers/internal/tracetest/testdata/prestate_tracer/disable_code_and_storage.json
- eth/tracers/internal/tracetest/testdata/prestate_tracer/disable_storage.json
- eth/tracers/internal/tracetest/testdata/prestate_tracer_with_diff_mode/create_disable_code.json
- eth/tracers/internal/tracetest/testdata/prestate_tracer_with_diff_mode/create_disable_storage.json
- eth/tracers/internal/tracetest/testdata/prestate_tracer_with_diff_mode/inner_create_disable_code_and_storage.json
- eth/tracers/internal/tracetest/testdata/prestate_tracer_with_diff_mode/simple_disable_code_and_storage.json
- Ensure the fixture loader/schema logic tolerates a missing
genesis.totalDifficulty(e.g., default to zero or treat as optional). - Verified that every fixture’s
context.timestamp≥genesis.timestamp. [run_scripts verification]
🤖 Prompt for AI Agents
In eth/tracers/internal/tracetest/testdata/call_tracer/selfdestruct.json around
line 50, the fixture still includes a genesis.totalDifficulty field that must be
removed and the test fixture loader/schema must be made tolerant of its absence:
delete genesis.totalDifficulty from this and the other listed fixture files, and
update the fixture loader/schema to treat genesis.totalDifficulty as optional
(default to zero or equivalent) so parsing succeeds when the field is missing;
no changes to timestamps are needed since context.timestamp ≥ genesis.timestamp
has been verified.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
eth/ethconfig/config.go (1)
187-190: Regenerate TOML codec and wire flags for VM tracingVMTrace and VMTraceConfig were added here, but they’re absent from gen_config.go. Regenerate the TOML encoder/decoder so these fields persist in config files; also ensure CLI flags and tracer wiring are in place (per earlier feedback).
🧹 Nitpick comments (1)
eth/gasprice/gasprice_test.go (1)
101-111: Create the signer after mutating ChainConfig to avoid subtle fork-gate mismatchesMinor ordering nit:
signeris constructed before you mutateconfig(London/Shanghai/etc. and Morph gates). Constructing it after the config edits is safer and more future-proof for time-gated forks.Apply this minimal diff:
@@ - gspec = &core.Genesis{ + gspec = &core.Genesis{ Config: &config, Alloc: core.GenesisAlloc{addr: {Balance: big.NewInt(math.MaxInt64)}}, } - signer = types.LatestSigner(gspec.Config) + signer types.Signer ) @@ config.Morph203Time = nil config.Morph300Time = nil + signer = types.LatestSigner(gspec.Config)Also applies to: 118-119
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
cmd/geth/chaincmd.go(2 hunks)cmd/geth/config.go(1 hunks)cmd/geth/main.go(1 hunks)cmd/utils/flags.go(1 hunks)core/genesis.go(4 hunks)core/state_processor_test.go(1 hunks)core/vm/runtime/runtime.go(5 hunks)eth/backend.go(1 hunks)eth/ethconfig/config.go(2 hunks)eth/ethconfig/gen_config.go(2 hunks)eth/gasprice/gasprice_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/vm/runtime/runtime.go
🧰 Additional context used
🧬 Code graph analysis (4)
cmd/geth/main.go (1)
cmd/utils/flags.go (1)
OverrideMorph300TimeFlag(274-277)
cmd/geth/chaincmd.go (1)
cmd/utils/flags.go (1)
OverrideMorph300TimeFlag(274-277)
cmd/geth/config.go (1)
cmd/utils/flags.go (1)
OverrideMorph300TimeFlag(274-277)
core/genesis.go (1)
core/tracing/hooks.go (2)
BalanceIncreaseGenesisBalance(239-239)NonceChangeGenesis(359-359)
🔇 Additional comments (11)
eth/gasprice/gasprice_test.go (1)
117-119: Explicitly disabling Morph300 in this test — LGTMSetting
config.Morph300Time = nil(alongsideMorph203Time = nil) makes the pre-Morph300 behavior explicit and avoids future defaults affecting these gasprice tests.eth/backend.go (1)
137-139: Genesis override wiring for Morph300 looks correct.Mirrors Morph203 handling and cleanly feeds core.SetupGenesisBlockWithOverride.
core/state_processor_test.go (1)
62-63: Verify TestStateProcessorErrors under Morph300 activation
SettingMorph300Time = new(uint64)enables Morph300 from genesis, causingMakeSignerto selectMorph300Signer. Confirm the test’s intent and update thewanterror strings incore/state_processor_test.go(around the dynamic‐fee cases) to reflect any behavior changes.cmd/geth/main.go (1)
75-75: Flag exposure LGTM.
Override.morph300timeis properly added next to Morph203 for discoverability.cmd/utils/flags.go (1)
274-277: New flag definition is consistent with existing overrides.Name/usage align with Morph203 counterpart; pointer type matches repo conventions.
cmd/geth/config.go (1)
165-168: Morph300 override plumbing is correctly wired end-to-end.
ConfirmedOverrideMorph300Timeflows throughcmd/geth/config,ChainOverrides.apply,SetupGenesisBlockWithOverride, and client/backend initialization.eth/ethconfig/config.go (1)
223-224: LGTM: Morph300Time override field mirrors Morph203TimeField shape and tag look correct; just ensure gen_config.go is regenerated so both marshal and unmarshal paths include it. The init CLI flag is already wired in cmd/geth/chaincmd.go.
cmd/geth/chaincmd.go (2)
54-55: LGTM: Exposes --override.morph300time on initFlag addition is consistent with Morph203Time and follows existing utils flag patterns.
218-221: LGTM: Applies Morph300Time override during genesis initOverride assignment mirrors Morph203Time and correctly passes through to SetupGenesisBlockWithOverride.
core/genesis.go (2)
149-151: LGTM: ChainOverrides extended with Morph300Time and appliedOverride propagation and fork-order validation via CheckConfigForkOrder look correct.
Also applies to: 161-163
35-35: All AddBalance/SetNonce invocations now include tracing reasons — no two-argument calls remain across the codebase.
| OverrideMorph300Time *uint64 `toml:",omitempty"` | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix TOML round-trip: OverrideMorph300Time is not decoded in UnmarshalTOML
You added OverrideMorph300Time to the marshaler and assigned enc.OverrideMorph300Time, but the UnmarshalTOML path doesn’t populate it. Users setting this in TOML won’t get the value applied.
Action:
- Add the field to the UnmarshalTOML dec struct and copy it into c in the decode path.
- Since this is a generated file, prefer re-running codegen rather than hand-editing.
Example (non-diff, since the additions are outside the shown range):
// In UnmarshalTOML's inner Config struct:
OverrideMorph300Time *uint64 `toml:",omitempty"`
// After decoding:
if dec.OverrideMorph300Time != nil {
c.OverrideMorph300Time = dec.OverrideMorph300Time
}Also, VMTrace and VMTraceConfig recently added to eth/ethconfig/config.go are still missing in this generated file. Please run:
- go generate ./eth/ethconfig
Also applies to: 116-116
🏁 Script executed:
#!/bin/bash
set -e
# Locate the generated config file
fd gen_config.go
# Show UnmarshalTOML definition
grep -R "func UnmarshalTOML" -n eth/ethconfig/gen_config.go || true
# Check if OverrideMorph300Time is decoded
grep -R "OverrideMorph300Time" -n eth/ethconfig/gen_config.go || true
# Check VMTrace and VMTraceConfig presence
grep -R "VMTrace" -n eth/ethconfig/gen_config.go || trueLength of output: 354
🏁 Script executed:
#!/bin/bash
set -e
# Check for UnmarshalTOML method
grep -R "UnmarshalTOML" -n eth/ethconfig/gen_config.go || echo "no UnmarshalTOML"
# Any unmarshal or decode references
grep -R "Unmarshal" -n eth/ethconfig/gen_config.go | head -n 20
# Inspect top of file to locate decoding logic
sed -n '1,200p' eth/ethconfig/gen_config.goLength of output: 8207
🏁 Script executed:
#!/bin/bash
set -e
# Locate config.go
fd -e go eth/ethconfig/config.go
# Check for VMTrace in config.go
grep -R "VMTrace" -n eth/ethconfig/config.go || echo "no VMTrace"
# Check for VMTraceConfig in config.go
grep -R "VMTraceConfig" -n eth/ethconfig/config.go || echo "no VMTraceConfig"Length of output: 217
Regenerate eth/ethconfig/gen_config.go
- UnmarshalTOML’s dec struct omits OverrideMorph300Time, so it isn’t applied
- VMTrace and VMTraceConfig fields are absent from the generated file
Rerungo generate ./eth/ethconfigto include these mappings.
🤖 Prompt for AI Agents
In eth/ethconfig/gen_config.go around lines 67-68, the generated dec/unmarshal
struct is missing OverrideMorph300Time and the VMTrace and VMTraceConfig fields
so TOML/unmarshal ignores them; re-run the generator to regenerate mappings by
executing `go generate ./eth/ethconfig` (or the package-specific generator
command), verify the generated file now includes OverrideMorph300Time *uint64
and fields for VMTrace and VMTraceConfig in the dec struct and unmarshal logic,
run `go vet`/`go test` to ensure no build breaks, and commit the updated
gen_config.go.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
cmd/evm/internal/t8ntool/execution.go (2)
209-209: Resolved: receipt now carries L1 data fee.
Addresses prior feedback to persist L1 fee into receipt.
225-227: Resolved: paired OnTxEnd after receipt append.
Start/End parity achieved for successful txs.core/state/state_object.go (2)
427-437: Fix inconsistent AddBalance return (prev vs current). Always return the updated balance.For amount > 0, you currently return the previous balance via SetBalance, while for amount == 0 you return the current balance. This inconsistency will cause subtle bugs in callers.
Apply:
func (s *stateObject) AddBalance(amount *big.Int) *big.Int { // EIP161: We must check emptiness for the objects such that the account // clearing (0,0,0 objects) can take effect. if amount.Sign() == 0 { if s.empty() { s.touch() } - return new(big.Int).Set(s.Balance()) + return new(big.Int).Set(s.Balance()) } - return s.SetBalance(new(big.Int).Add(s.Balance(), amount)) + newBal := new(big.Int).Add(s.Balance(), amount) + _ = s.SetBalance(newBal) // journal prev, update balance + return new(big.Int).Set(newBal) }Run to find callsites that might assume the old semantics:
#!/bin/bash rg -nP -C2 '\.AddBalance\(' --type go
441-446: Make SubBalance return semantics consistent and copy on zero.For amount == 0 you return a pointer to the internal balance; for > 0 you return the previous balance via SetBalance. Always return the updated balance and avoid exposing the internal pointer.
func (s *stateObject) SubBalance(amount *big.Int) *big.Int { if amount.Sign() == 0 { - return s.Balance() + return new(big.Int).Set(s.Balance()) } - return s.SetBalance(new(big.Int).Sub(s.Balance(), amount)) + newBal := new(big.Int).Sub(s.Balance(), amount) + _ = s.SetBalance(newBal) + return new(big.Int).Set(newBal) }
🧹 Nitpick comments (5)
cmd/evm/internal/t8ntool/execution.go (1)
167-171: Optional: fire OnTxEnd on error paths for start/end parity.
Today OnTxEnd runs only on success; consider invoking it with err and nil receipt on early rejections.Apply:
l1DataFee, err := fees.CalculateL1DataFee(tx, statedb, chainConfig, new(big.Int).SetUint64(pre.Env.Number)) if err != nil { log.Info("rejected tx due to fees.CalculateL1DataFee", "index", i, "hash", tx.Hash(), "from", msg.From(), "error", err) rejectedTxs = append(rejectedTxs, &rejectedTx{i, err.Error()}) + if evm.Config.Tracer != nil && evm.Config.Tracer.OnTxEnd != nil { + evm.Config.Tracer.OnTxEnd(nil, err) + } continue } @@ msgResult, err := core.ApplyMessage(evm, msg, gaspool, l1DataFee) if err != nil { statedb.RevertToSnapshot(snapshot) log.Info("rejected tx", "index", i, "hash", tx.Hash(), "from", msg.From(), "error", err) rejectedTxs = append(rejectedTxs, &rejectedTx{i, err.Error()}) + if evm.Config.Tracer != nil && evm.Config.Tracer.OnTxEnd != nil { + evm.Config.Tracer.OnTxEnd(nil, err) + } continue }Confirm OnTxEnd(receipt, err) signature matches Hooks.
Also applies to: 178-183
core/state/state_object.go (1)
448-457: SetBalance returns previous balance — verify all callers expect this.Returning the previous value is fine for journaling, but easy to misuse. Given Add/Sub will now ignore this return, confirm no other callers expect “new/current”.
If confusion persists, consider renaming to
SetBalancePrev(internal) and introducing a helper that returns the new/current value.core/state/statedb.go (3)
407-414: Remove unreachable nil-guard after GetOrNewStateObject in AddBalance.
GetOrNewStateObjectnever returns nil. The guard adds noise.func (s *StateDB) AddBalance(addr common.Address, amount *big.Int, reason tracing.BalanceChangeReason) *big.Int { - stateObject := s.GetOrNewStateObject(addr) - if stateObject == nil { - return new(big.Int) - } - return stateObject.AddBalance(amount) + return s.GetOrNewStateObject(addr).AddBalance(amount) }
417-426: Same here: drop unreachable nil-guard in SubBalance.Also, once stateObject.SubBalance is fixed to return the updated balance, this API will be consistent across zero/non-zero amounts.
func (s *StateDB) SubBalance(addr common.Address, amount *big.Int, reason tracing.BalanceChangeReason) *big.Int { - stateObject := s.GetOrNewStateObject(addr) - if stateObject == nil { - return new(big.Int) - } - if amount.Sign() == 0 { - return stateObject.Balance() - } - return stateObject.SubBalance(amount) + stateObject := s.GetOrNewStateObject(addr) + if amount.Sign() == 0 { + return stateObject.Balance() + } + return stateObject.SubBalance(amount) }
442-448: Simplify SetCode: nil-guard is unreachable after GetOrNewStateObject.func (s *StateDB) SetCode(addr common.Address, code []byte) []byte { - stateObject := s.GetOrNewStateObject(addr) - if stateObject != nil { - return stateObject.SetCode(code) - } - return nil + return s.GetOrNewStateObject(addr).SetCode(code) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
cmd/evm/internal/t8ntool/execution.go(8 hunks)core/state/state_object.go(2 hunks)core/state/statedb.go(2 hunks)core/tracing/hooks.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/tracing/hooks.go
🧰 Additional context used
🧬 Code graph analysis (3)
core/state/statedb.go (2)
core/tracing/hooks.go (3)
StateDB(49-60)BalanceChangeReason(224-224)NonceChangeReason(351-351)core/vm/interface.go (1)
StateDB(29-89)
core/state/state_object.go (1)
crypto/codehash/codehash.go (1)
KeccakCodeHash(16-18)
cmd/evm/internal/t8ntool/execution.go (6)
core/vm/interpreter.go (1)
Config(33-42)core/state/statedb.go (1)
StateDB(63-127)core/tracing/hooks.go (5)
StateDB(49-60)BalanceIncreaseRewardMineUncle(233-233)BalanceIncreaseRewardMineBlock(235-235)NonceChangeGenesis(359-359)BalanceIncreaseGenesisBalance(239-239)core/vm/interface.go (1)
StateDB(29-89)core/state_transition.go (1)
ExecutionResult(99-104)core/types/transaction_signing.go (1)
MakeSigner(42-61)
🔇 Additional comments (10)
cmd/evm/internal/t8ntool/execution.go (6)
33-33: Good: tracing import is warranted by new reasoned state mutations.
118-118: Signer now time-aware: correct param used.
Passing pre.Env.Timestamp matches MakeSigner(config, blockNumber, blockTime).
173-175: Good: OnTxStart hook before execution.
Ensures trace context has VM, tx, sender.
254-256: Good: reward attributions annotated with tracing reasons.
Improves post-hoc analysis for miner and ommer rewards.
282-283: Good: genesis nonce/balance set with explicit reasons.
Consistent with reasoned StateDB API.
101-101: All Prestate.Apply call sites have been updated to match the new signature; no remaining callers pass the old tracer/factory arguments.core/state/state_object.go (2)
510-518: SetCode journaling and return of prevCode look correct.
520-527: Hash/size updates in setCode are correct and ordered.core/state/statedb.go (2)
428-433: Reason parameter currently unused here — ensure hooks consume it (wrapper or tracer).If
statedb_hooked.goor a wrapper wiresreasonto tracing hooks, all good; otherwise you lose valuable context.
435-440: Ditto for SetNonce: verifyreasonis propagated to tracing.
| func (s *StateDB) SetState(addr common.Address, key, value common.Hash) common.Hash { | ||
| stateObject := s.GetOrNewStateObject(addr) | ||
| if stateObject != nil { | ||
| stateObject.SetState(s.db, key, value) | ||
| } | ||
| return common.Hash{} | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
SetState discards the required return value; return the previous slot value.
The interface returns common.Hash. Currently always zero. Return the previous value for callers/tracers.
func (s *StateDB) SetState(addr common.Address, key, value common.Hash) common.Hash {
- stateObject := s.GetOrNewStateObject(addr)
- if stateObject != nil {
- stateObject.SetState(s.db, key, value)
- }
- return common.Hash{}
+ stateObject := s.GetOrNewStateObject(addr)
+ prev := stateObject.GetState(s.db, key)
+ stateObject.SetState(s.db, key, value)
+ return prev
}🏁 Script executed:
#!/bin/bash
# Grep potential callsites relying on SetState's return
rg -nP -C2 '\.SetState\(' --type goLength of output: 21396
🏁 Script executed:
#!/bin/bash
# Locate hookedStateDB SetState definition and context
rg -nP 'func\s+\(s \*hookedStateDB\)\s+SetState' -n --type go
rg -nP -C3 'func\s+\(s \*hookedStateDB\)\s+SetState' -n --type goLength of output: 779
Return previous storage slot value in StateDB.SetState
Modify core/state/statedb.go so that SetState fetches and returns the old value (with a nil-check) instead of always returning a zero hash:
func (s *StateDB) SetState(addr common.Address, key, value common.Hash) common.Hash {
- stateObject := s.GetOrNewStateObject(addr)
- if stateObject != nil {
- stateObject.SetState(s.db, key, value)
- }
- return common.Hash{}
+ stateObject := s.GetOrNewStateObject(addr)
+ if stateObject == nil {
+ return common.Hash{}
+ }
+ prev := stateObject.GetState(s.db, key)
+ stateObject.SetState(s.db, key, value)
+ return prev
}HookedStateDB already propagates this value to OnStorageChange (core/state/statedb_hooked.go:183–185), and existing call sites ignore the return so this change is backwards-compatible. (blog.csdn.net)
🤖 Prompt for AI Agents
In core/state/statedb.go around lines 450–456, change SetState to fetch and
return the previous storage slot value instead of always returning a zero hash:
if stateObject is nil return common.Hash{}; otherwise call
stateObject.GetState(s.db, key) (or the appropriate getter on StateObject with
the db and key) to capture the old value, then call stateObject.SetState(s.db,
key, value) and return the captured old value (preserving nil-checks and
existing behavior).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/types/l2trace.go (3)
129-147: Document the new EIP‑7702 field; consider JSON omission semantics.
- Add a short comment tying AuthorizationList to EIP‑7702 for clarity.
- Note: with
omitempty, non-nil empty slices still marshal as[]. If you want the field omitted when empty, ensure it isnilat construction time (see suggestion below on Lines 161-180).type TransactionData struct { @@ - AuthorizationList []SetCodeAuthorization `json:"authorizationList,omitempty"` + // EIP-7702: per-tx SetCode authorizations (present on typed tx that carry them). + AuthorizationList []SetCodeAuthorization `json:"authorizationList,omitempty"`
153-155: Don’t silently discard Sender() errors; make intent explicit.If signature recovery fails, traces will show zero-address. Keep behavior, but document it to avoid confusion during debugging.
-from, _ := Sender(signer, tx) +from, err := Sender(signer, tx) +if err != nil { + // Intentionally ignore to keep trace emission resilient; zero address denotes + // an unrecoverable sender for this tx in trace outputs. +}
161-180: EnsureauthorizationListtruly omits on empty.Normalize to
nilwhen there are no authorizations soomitemptysuppresses the field rather than emitting[].result := &TransactionData{ @@ - AccessList: tx.AccessList(), - AuthorizationList: tx.SetCodeAuthorizations(), + AccessList: tx.AccessList(), + // Normalize empty -> nil so `omitempty` suppresses the field. + AuthorizationList: func() []SetCodeAuthorization { + al := tx.SetCodeAuthorizations() + if len(al) == 0 { + return nil + } + return al + }(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
core/types/l2trace.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/types/l2trace.go (2)
core/types/tx_setcode.go (1)
SetCodeAuthorization(72-79)core/types/transaction_signing.go (1)
MakeSigner(42-61)
🔇 Additional comments (1)
core/types/l2trace.go (1)
151-153: All call sites updated. Verified there are no remaining 3-argNewTransactionDatacalls.
(Optional) Clarify in the doc comment thatblockTimeis the block header timestamp (seconds since Unix epoch):// NewTransactionData returns a transaction that will serialize to the trace // representation, with the given location metadata set (if available). +// blockTime must be the block header timestamp (seconds since Unix epoch).
* add logs for debug * fix sender * convert auth data * revert log --------- Co-authored-by: fletcher.fan <fletcher.fan@bitget.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/types/transaction_signing.go (1)
41-61: MakeSigner may panic when ChainID is nil on Berlin+ forksnewModernSigner panics for nil/≤0 chainIDs. If a config with Berlin/London/Curie/Morph300 is used without ChainID set (tests, tools), this path will panic. Guard and fall back to Homestead/Frontier when ChainID is nil.
func MakeSigner(config *params.ChainConfig, blockNumber *big.Int, blockTime uint64) Signer { - var signer Signer - switch { + // If ChainID is nil, avoid modern signers that require it. + if config.ChainID == nil { + if config.IsHomestead(blockNumber) { + return HomesteadSigner{} + } + return FrontierSigner{} + } + var signer Signer + switch { case config.IsMorph300(blockNumber, blockTime): signer = NewMorph300Signer(config.ChainID) case config.IsCurie(blockNumber): signer = NewCurieSigner(config.ChainID) case config.IsLondon(blockNumber): signer = NewLondonSigner(config.ChainID) case config.IsBerlin(blockNumber): signer = NewEIP2930Signer(config.ChainID)core/types/l2trace.go (1)
7-11: Use hexutil.U256 for JSON hex encoding; drop unused uint256 importuint256.Int has no JSON marshaler; it will serialize as {}. Use hexutil.U256 to get 0x-prefixed hex. Then remove the uint256 import to avoid compile errors.
-import ( - "encoding/json" - "math/big" - - "github.com/holiman/uint256" +import ( + "encoding/json" + "math/big"
🧹 Nitpick comments (4)
core/types/transaction_signing.go (3)
234-237: maps.Equal OK but see toolchain noteEquality logic is sound; dependency caveat covered above.
If you prefer avoiding maps.Equal, a tiny length+key loop would decouple from Go version.
248-267: Sender() chainID check stricter than SignatureValues() — align semanticsSender rejects typed txs when tx.ChainId()==0, but SignatureValues allows zero. This inconsistency can surprise signing flows (sign ok, recover fails). Consider accepting zero here too (as “unspecified”) or tightening SignatureValues.
- if tx.ChainId().Cmp(s.chainID) != 0 { + if tx.ChainId().Sign() != 0 && tx.ChainId().Cmp(s.chainID) != 0 { return common.Address{}, fmt.Errorf("%w: have %d want %d", ErrInvalidChainId, tx.ChainId(), s.chainID) }
269-285: SignatureValues: OK; consider explicit length checkdecodeSignature panics on wrong length; if you prefer error returns over panic at this layer, validate sig len first.
- R, S, _ = decodeSignature(sig) + if len(sig) != crypto.SignatureLength { + return nil, nil, nil, ErrInvalidSig + } + R, S, _ = decodeSignature(sig)core/types/l2trace.go (1)
161-192: NewTransactionData: signer construction looks correctPassing blockNumber and blockTime into MakeSigner aligns with the new API. The rest of the mapping is consistent, and AuthorizationList uses omitempty (empty slice omitted).
Minor: avoid allocating an empty slice by returning nil when len(auths)==0 inside convertToAuthorizationData to reduce JSON clutter/allocs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/types/l2trace.go(3 hunks)core/types/transaction_signing.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
core/types/l2trace.go (3)
internal/ethapi/api.go (1)
AccessList(1536-1632)core/types/transaction_signing.go (1)
MakeSigner(42-61)core/types/tx_setcode.go (1)
SetCodeAuthorization(72-79)
core/types/transaction_signing.go (4)
params/config.go (1)
ChainConfig(519-560)params/forks/forks.go (7)
Fork(20-20)SpuriousDragon(28-28)Homestead(25-25)Berlin(34-34)London(35-35)Curie(40-40)Morph300(42-42)core/types/transaction.go (8)
LegacyTxType(50-50)L1MessageTxType(56-56)AccessListTxType(51-51)DynamicFeeTxType(52-52)BlobTxType(53-53)SetCodeTxType(54-54)Transaction(60-68)ErrTxTypeNotSupported(39-39)cmd/evm/internal/t8ntool/transaction.go (1)
Transaction(68-184)
🔇 Additional comments (4)
core/types/transaction_signing.go (4)
99-106: Guard against zero/negative chainIDsLatestSignerForChainID forwards chainID to newModernSigner which panics on ≤0. Either validate here or document the precondition.
func LatestSignerForChainID(chainID *big.Int) Signer { var signer Signer - if chainID != nil { + if chainID != nil && chainID.Sign() > 0 { signer = NewMorph300Signer(chainID) } else { signer = HomesteadSigner{} } return signer }
195-213: Legacy signer selection looks goodFork-based legacy fallback (Frontier/Homestead/EIP155) is correct and makes modernSigner safe for LegacyTxType.
287-321: Per-fork constructors are consistentThe new constructors correctly parameterize modernSigner by fork thresholds.
19-30: Go 1.21+ dependency via maps.Equal — go.mod OK; verify CI images
- go.mod declares "go 1.21" (line 3) — stdlib maps (maps.Equal) is supported.
- CI runner images / Dockerfiles referencing Go were not found in the repo; ensure CI workflows or Dockerfiles are pinned to Go ≥1.21 or switch to golang.org/x/exp/maps / a manual equality loop.
| type SetCodeAuthorizationData struct { | ||
| ChainID uint256.Int `json:"chainId" gencodec:"required"` | ||
| Address common.Address `json:"address" gencodec:"required"` | ||
| Nonce uint64 `json:"nonce" gencodec:"required"` | ||
| V hexutil.Uint64 `json:"yParity" gencodec:"required"` | ||
| R hexutil.U256 `json:"r" gencodec:"required"` | ||
| S hexutil.U256 `json:"s" gencodec:"required"` | ||
| } |
There was a problem hiding this comment.
Fix ChainID JSON encoding in SetCodeAuthorizationData
Switch ChainID to hexutil.U256 for correct wire format.
type SetCodeAuthorizationData struct {
- ChainID uint256.Int `json:"chainId" gencodec:"required"`
+ ChainID hexutil.U256 `json:"chainId" gencodec:"required"`
Address common.Address `json:"address" gencodec:"required"`
Nonce uint64 `json:"nonce" gencodec:"required"`
V hexutil.Uint64 `json:"yParity" gencodec:"required"`
R hexutil.U256 `json:"r" gencodec:"required"`
S hexutil.U256 `json:"s" gencodec:"required"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type SetCodeAuthorizationData struct { | |
| ChainID uint256.Int `json:"chainId" gencodec:"required"` | |
| Address common.Address `json:"address" gencodec:"required"` | |
| Nonce uint64 `json:"nonce" gencodec:"required"` | |
| V hexutil.Uint64 `json:"yParity" gencodec:"required"` | |
| R hexutil.U256 `json:"r" gencodec:"required"` | |
| S hexutil.U256 `json:"s" gencodec:"required"` | |
| } | |
| type SetCodeAuthorizationData struct { | |
| ChainID hexutil.U256 `json:"chainId" gencodec:"required"` | |
| Address common.Address `json:"address" gencodec:"required"` | |
| Nonce uint64 `json:"nonce" gencodec:"required"` | |
| V hexutil.Uint64 `json:"yParity" gencodec:"required"` | |
| R hexutil.U256 `json:"r" gencodec:"required"` | |
| S hexutil.U256 `json:"s" gencodec:"required"` | |
| } |
🤖 Prompt for AI Agents
In core/types/l2trace.go around lines 150 to 157, the ChainID field in
SetCodeAuthorizationData uses uint256.Int which produces the wrong wire JSON
format; change the ChainID field type to hexutil.U256 (maintain the
`json:"chainId" gencodec:"required"` tag) so it serializes as the expected
hex-encoded unsigned 256-bit value, and update imports accordingly (remove or
replace uint256 import if no longer used and ensure hexutil is imported).
| func convertToAuthorizationData(auths []SetCodeAuthorization) []SetCodeAuthorizationData { | ||
| result := make([]SetCodeAuthorizationData, len(auths)) | ||
| for i, auth := range auths { | ||
| result[i] = SetCodeAuthorizationData{ | ||
| ChainID: auth.ChainID, | ||
| Address: auth.Address, | ||
| Nonce: auth.Nonce, | ||
| V: hexutil.Uint64(auth.V), | ||
| R: hexutil.U256(auth.R), | ||
| S: hexutil.U256(auth.S), | ||
| } | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Map auth.ChainID to hexutil.U256
Update the converter to match the struct change above.
func convertToAuthorizationData(auths []SetCodeAuthorization) []SetCodeAuthorizationData {
result := make([]SetCodeAuthorizationData, len(auths))
for i, auth := range auths {
result[i] = SetCodeAuthorizationData{
- ChainID: auth.ChainID,
+ ChainID: hexutil.U256(auth.ChainID),
Address: auth.Address,
Nonce: auth.Nonce,
V: hexutil.Uint64(auth.V),
R: hexutil.U256(auth.R),
S: hexutil.U256(auth.S),
}
}
return result
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func convertToAuthorizationData(auths []SetCodeAuthorization) []SetCodeAuthorizationData { | |
| result := make([]SetCodeAuthorizationData, len(auths)) | |
| for i, auth := range auths { | |
| result[i] = SetCodeAuthorizationData{ | |
| ChainID: auth.ChainID, | |
| Address: auth.Address, | |
| Nonce: auth.Nonce, | |
| V: hexutil.Uint64(auth.V), | |
| R: hexutil.U256(auth.R), | |
| S: hexutil.U256(auth.S), | |
| } | |
| } | |
| return result | |
| } | |
| func convertToAuthorizationData(auths []SetCodeAuthorization) []SetCodeAuthorizationData { | |
| result := make([]SetCodeAuthorizationData, len(auths)) | |
| for i, auth := range auths { | |
| result[i] = SetCodeAuthorizationData{ | |
| ChainID: hexutil.U256(auth.ChainID), | |
| Address: auth.Address, | |
| Nonce: auth.Nonce, | |
| V: hexutil.Uint64(auth.V), | |
| R: hexutil.U256(auth.R), | |
| S: hexutil.U256(auth.S), | |
| } | |
| } | |
| return result | |
| } |
🤖 Prompt for AI Agents
In core/types/l2trace.go around lines 194 to 207, the converter currently
assigns ChainID directly but the SetCodeAuthorizationData struct now expects
hexutil.U256; update the assignment to convert auth.ChainID to hexutil.U256
(e.g., ChainID: hexutil.U256(auth.ChainID)) while keeping the existing
conversions for V, R, S unchanged so the result slice uses the correct hexutil
types.
| func LatestSigner(config *params.ChainConfig) Signer { | ||
| var signer Signer | ||
| if config.ChainID != nil { | ||
| if config.LondonBlock != nil { | ||
| return NewLondonSignerWithEIP4844(config.ChainID) | ||
| } | ||
| if config.BerlinBlock != nil { | ||
| return NewEIP2930Signer(config.ChainID) | ||
| } | ||
| if config.EIP155Block != nil { | ||
| return NewEIP155Signer(config.ChainID) | ||
| switch { | ||
| case config.Morph300Time != nil: | ||
| signer = NewMorph300Signer(config.ChainID) | ||
| case config.CurieBlock != nil: | ||
| signer = NewCurieSigner(config.ChainID) | ||
| case config.LondonBlock != nil: | ||
| signer = NewLondonSigner(config.ChainID) | ||
| case config.BerlinBlock != nil: | ||
| signer = NewEIP2930Signer(config.ChainID) | ||
| case config.EIP155Block != nil: | ||
| signer = NewEIP155Signer(config.ChainID) | ||
| default: | ||
| signer = HomesteadSigner{} | ||
| } | ||
| } | ||
| return HomesteadSigner{} | ||
| return signer | ||
| } |
There was a problem hiding this comment.
LatestSigner returns nil when ChainID is nil
Current logic can return a nil Signer, causing NPEs at call sites. Default to Homestead when ChainID is nil.
func LatestSigner(config *params.ChainConfig) Signer {
- var signer Signer
- if config.ChainID != nil {
+ var signer Signer
+ if config.ChainID != nil {
switch {
case config.Morph300Time != nil:
signer = NewMorph300Signer(config.ChainID)
case config.CurieBlock != nil:
signer = NewCurieSigner(config.ChainID)
case config.LondonBlock != nil:
signer = NewLondonSigner(config.ChainID)
case config.BerlinBlock != nil:
signer = NewEIP2930Signer(config.ChainID)
case config.EIP155Block != nil:
signer = NewEIP155Signer(config.ChainID)
default:
signer = HomesteadSigner{}
}
- }
- return signer
+ } else {
+ signer = HomesteadSigner{}
+ }
+ return signer
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func LatestSigner(config *params.ChainConfig) Signer { | |
| var signer Signer | |
| if config.ChainID != nil { | |
| if config.LondonBlock != nil { | |
| return NewLondonSignerWithEIP4844(config.ChainID) | |
| } | |
| if config.BerlinBlock != nil { | |
| return NewEIP2930Signer(config.ChainID) | |
| } | |
| if config.EIP155Block != nil { | |
| return NewEIP155Signer(config.ChainID) | |
| switch { | |
| case config.Morph300Time != nil: | |
| signer = NewMorph300Signer(config.ChainID) | |
| case config.CurieBlock != nil: | |
| signer = NewCurieSigner(config.ChainID) | |
| case config.LondonBlock != nil: | |
| signer = NewLondonSigner(config.ChainID) | |
| case config.BerlinBlock != nil: | |
| signer = NewEIP2930Signer(config.ChainID) | |
| case config.EIP155Block != nil: | |
| signer = NewEIP155Signer(config.ChainID) | |
| default: | |
| signer = HomesteadSigner{} | |
| } | |
| } | |
| return HomesteadSigner{} | |
| return signer | |
| } | |
| func LatestSigner(config *params.ChainConfig) Signer { | |
| var signer Signer | |
| if config.ChainID != nil { | |
| switch { | |
| case config.Morph300Time != nil: | |
| signer = NewMorph300Signer(config.ChainID) | |
| case config.CurieBlock != nil: | |
| signer = NewCurieSigner(config.ChainID) | |
| case config.LondonBlock != nil: | |
| signer = NewLondonSigner(config.ChainID) | |
| case config.BerlinBlock != nil: | |
| signer = NewEIP2930Signer(config.ChainID) | |
| case config.EIP155Block != nil: | |
| signer = NewEIP155Signer(config.ChainID) | |
| default: | |
| signer = HomesteadSigner{} | |
| } | |
| } else { | |
| signer = HomesteadSigner{} | |
| } | |
| return signer | |
| } |
🤖 Prompt for AI Agents
In core/types/transaction_signing.go around lines 70 to 89, LatestSigner can
return a nil Signer when config.ChainID is nil; change the function to default
to returning HomesteadSigner instead of nil. Specifically, initialize signer to
HomesteadSigner (or set signer = HomesteadSigner{} in the outer else branch when
ChainID is nil) so that callers never receive a nil Signer while preserving the
existing switch logic when ChainID is non-nil.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
core/genesis.go (1)
149-151: Doc nit: update ChainOverrides comment and new field naming context.Comment mentions BSC; this repo is Morph. Also, please add a brief note that ViridianTime gates the Morph300/Viridian instruction set and tracing behavior.
-// Typically, these modifications involve hardforks that are not enabled on the BSC mainnet, intended for testing purposes. +// Typically, these modifications involve hardforks that may not be enabled on Morph mainnet, intended for testing purposes. +// ViridianTime gates the Viridian/Morph300 fork (EIP‑7702 enablement and tracing changes).core/vm/runtime/runtime.go (1)
129-131: Tx-start tracing added; consider also emitting OnTxEnd.OnTxStart is now invoked in Execute/Create/Call. If tracers rely on paired lifecycle hooks, add a matching OnTxEnd call after execution completes.
If OnTxEnd’s signature differs (e.g., needs status/gasUsed), I can adapt the call sites once you confirm the hook shape.
Also applies to: 167-168, 198-200
core/vm/evm.go (1)
312-316: Remove stale commented-out tracer code.These commented debug hooks add noise; they’re superseded by captureBegin/captureEnd.
- // if evm.Config.Debug { - // evm.Config.Tracer.CaptureEnter(DELEGATECALL, caller.Address(), addr, input, gas, nil) - // defer func(startGas uint64) { - // evm.Config.Tracer.CaptureExit(ret, startGas-gas, err) - // }(gas) - // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
cmd/geth/chaincmd.go(2 hunks)cmd/geth/config.go(1 hunks)cmd/geth/main.go(1 hunks)cmd/utils/flags.go(1 hunks)core/genesis.go(4 hunks)core/state_processor_test.go(1 hunks)core/tx_pool.go(10 hunks)core/types/transaction_signing.go(5 hunks)core/vm/evm.go(18 hunks)core/vm/interpreter.go(7 hunks)core/vm/jump_table.go(2 hunks)core/vm/runtime/runtime.go(5 hunks)eth/backend.go(1 hunks)eth/ethconfig/config.go(2 hunks)eth/ethconfig/gen_config.go(2 hunks)eth/gasprice/gasprice_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/geth/main.go
- cmd/utils/flags.go
- eth/backend.go
- eth/ethconfig/gen_config.go
🧰 Additional context used
🧬 Code graph analysis (8)
cmd/geth/chaincmd.go (1)
cmd/utils/flags.go (1)
OverrideViridianTimeFlag(274-277)
cmd/geth/config.go (1)
cmd/utils/flags.go (1)
OverrideViridianTimeFlag(274-277)
core/types/transaction_signing.go (4)
params/config.go (1)
ChainConfig(519-560)params/forks/forks.go (7)
Fork(20-20)SpuriousDragon(28-28)Homestead(25-25)Berlin(34-34)London(35-35)Curie(40-40)Viridian(42-42)core/types/transaction.go (8)
LegacyTxType(50-50)L1MessageTxType(56-56)AccessListTxType(51-51)DynamicFeeTxType(52-52)BlobTxType(53-53)SetCodeTxType(54-54)Transaction(60-68)ErrTxTypeNotSupported(39-39)cmd/evm/internal/t8ntool/transaction.go (1)
Transaction(68-184)
core/vm/runtime/runtime.go (2)
core/types/transaction.go (1)
NewTx(71-75)core/types/legacy_tx.go (1)
LegacyTx(27-35)
core/tx_pool.go (5)
core/types/transaction.go (3)
SetCodeTxType(54-54)ErrTxTypeNotSupported(39-39)Transaction(60-68)core/error.go (5)
ErrTxTypeNotSupported(89-89)ErrIntrinsicGas(85-85)ErrOutOfOrderTxFromDelegated(122-122)ErrInflightTxLimitReached(118-118)ErrAuthorityReserved(127-127)core/state_transition.go (1)
IntrinsicGas(134-184)core/types/transaction_signing.go (1)
Sender(146-162)crypto/codehash/codehash.go (1)
EmptyKeccakCodeHash(10-10)
core/vm/evm.go (6)
core/vm/interpreter.go (1)
Config(33-42)core/vm/interface.go (1)
StateDB(29-89)core/vm/contracts.go (1)
RunPrecompiledContract(216-227)core/vm/contract.go (4)
NewContract(68-85)AccountRef(40-40)Contract(47-65)ContractRef(29-31)core/tracing/hooks.go (7)
GasChangeCallFailedExecution(332-332)BalanceChangeTouchAccount(261-261)NonceChangeNewContract(368-368)GasChangeCallCodeStorage(323-323)GasChangeCallInitialBalance(309-309)GasChangeCallLeftOverReturned(313-313)VMContext(63-70)core/types/tx_setcode.go (1)
ParseDelegation(37-42)
core/vm/interpreter.go (6)
eth/tracers/dir.go (1)
Tracer(40-45)core/tracing/hooks.go (2)
Hooks(192-220)GasChangeCallOpCode(326-326)core/vm/memory.go (1)
Memory(26-29)core/vm/stack.go (1)
Stack(35-37)core/vm/contract.go (1)
Contract(47-65)core/vm/errors.go (2)
VMErrorFromErr(98-107)ErrOutOfGas(27-27)
core/genesis.go (1)
core/tracing/hooks.go (2)
BalanceIncreaseGenesisBalance(239-239)NonceChangeGenesis(359-359)
🔇 Additional comments (19)
core/state_processor_test.go (1)
62-63: Include Viridian fork in test config.Setting
ViridianTimealongside the other zero-time forks keeps these “recent chain” tests aligned with the updated configuration shape, ensuring LatestSigner and fork gating see the new activation.core/types/transaction_signing.go (1)
70-89: Ensure LatestSigner never returns nilReturning nil when ChainID is unset leaves callers like the txpool with a nil signer, and the first call to
types.Senderwill panic. This regression shows up immediately when bootstrapping with a chain config that omits ChainID (common in tests/private devnets).func LatestSigner(config *params.ChainConfig) Signer { - var signer Signer - if config.ChainID != nil { + signer := Signer(HomesteadSigner{}) + if config.ChainID != nil { switch { case config.ViridianTime != nil: signer = NewViridianSigner(config.ChainID) @@ - default: - signer = HomesteadSigner{} } } return signer }core/genesis.go (2)
161-163: Override wiring for ViridianTime looks correct.Nil-safe receiver on apply avoids panics when overrides is nil; fork-order check is preserved.
Please confirm ChainConfig has ViridianTime in CheckConfigForkOrder() ordering.
346-349: Good: genesis mutations emit tracing reasons.AddBalance(..., BalanceIncreaseGenesisBalance) and SetNonce(..., NonceChangeGenesis) align with the new Hooks plumbing.
Do you want a small test ensuring OnBalanceChange/OnNonceChange fire during ToBlock() for alloc entries? I can draft it.
core/vm/runtime/runtime.go (1)
77-78: Defaulting ViridianTime to 0 enables the fork by default in runtime.This mirrors other forks in setDefaults, but it activates Viridian for runtime executions at time 0. Confirm this is intentional for t8n/evm usage.
core/vm/jump_table.go (2)
59-60: Viridian instruction set registration matches interpreter selection.Interpreter prefers viridianInstructionSet when IsViridian; good cohesion.
65-69: Initialize Viridian from Curie and enable 7702.Looks right. Please ensure enable7702 is idempotent and only toggles the intended opcodes.
core/vm/evm.go (6)
182-188: Call tracing begin/end wrapper is correct and defers will see final gas.Using the named result (leftOverGas) in the deferred captureEnd is fine because named returns are assigned before defers run.
Confirm that tracers expect GasChangeCallInitialBalance at begin and GasChangeCallLeftOverReturned when leftOverGas > 0.
535-563: Delegation resolution: update fork name in comments and guard self-delegation.Prevent a redundant self lookup and align comments with the Viridian fork name.
-// resolveCode returns the code associated with the provided account. After -// Prague, it can also resolve code pointed to by a delegation designator. +// resolveCode returns the code associated with the provided account. +// On Viridian, it can also resolve code pointed to by a delegation designator. func (evm *EVM) resolveCode(addr common.Address) []byte { code := evm.StateDB.GetCode(addr) if !evm.chainRules.IsViridian { return code } if target, ok := types.ParseDelegation(code); ok { - // Note we only follow one level of delegation. - return evm.StateDB.GetCode(target) + if target == addr { // avoid self-delegation loop + return code + } + // Note we only follow one level of delegation. + return evm.StateDB.GetCode(target) } return code } -// resolveCodeHash returns the code hash associated with the provided address. -// After Prague, it can also resolve code hash of the account pointed to by a +// resolveCodeHash returns the code hash associated with the provided address. +// On Viridian, it can also resolve code hash of the account pointed to by a // delegation designator. Although this is not accessible in the EVM it is used // internally to associate jumpdest analysis to code. func (evm *EVM) resolveCodeHash(addr common.Address) common.Hash { if evm.chainRules.IsViridian { code := evm.StateDB.GetCode(addr) if target, ok := types.ParseDelegation(code); ok { - // Note we only follow one level of delegation. - return evm.StateDB.GetKeccakCodeHash(target) + if target == addr { // avoid self-delegation loop + return evm.StateDB.GetKeccakCodeHash(addr) + } + // Note we only follow one level of delegation. + return evm.StateDB.GetKeccakCodeHash(target) } } return evm.StateDB.GetKeccakCodeHash(addr) }
572-610: Tracer helpers and VM context exposure: LGTM.captureBegin/captureEnd and GetVMContext provide the needed signals and context.
389-399: Gas-change on failed execution looks consistent.On failure (non-revert), GasChangeCallFailedExecution is emitted and gas zeroed.
460-470: Nonce change reasons during create are tagged correctly.Incrementing caller nonce and setting created-account nonce with NonceChangeNewContract is consistent with tracing expectations.
210-211: Precompile execution now traces gas deltas.Passing tracer through RunPrecompiledContract enables GasChangeCallPrecompiledContract emissions.
Also applies to: 278-279, 336-337, 390-391, 500-501, 513-514
core/vm/interpreter.go (6)
34-42: Tracer API switch to tracing.Hooks is good; Debug flag removal aligns with hook presence.This reduces legacy logger surface area; jump table remains configurable via Config.
52-95: ScopeContext accessors: useful for tracers; ensure read-only semantics.Returning underlying slices without copy is fine for internal tracers; callers must not mutate.
Confirm external tracers never mutate MemoryData/StackData.
124-126: Viridian jump table selection precedence.Viridian first, then Curie et al. Matches the new fork. Ensure Rules(..).IsViridian toggles with ViridianTime.
299-307: Dynamic gas error wrapping.Returning fmt.Errorf("%w: %v", ErrOutOfGas, err) keeps error class while preserving detail; good for VMErrorFromErr classification.
316-325: Opcode-level tracing order and gas-change emission look correct.OnGasChange before OnOpcode with (prevGas, prevGas-cost) and pre-expansion memory snapshot is appropriate.
333-342: STOP token normalization.Mapping errStopToken to nil maintains legacy behavior.
| // Allow at most one in-flight tx for delegated accounts or those with a | ||
| // pending authorization. | ||
| if err := pool.checkDelegationLimit(tx); err != nil { | ||
| return err | ||
| } | ||
| // For symmetry, allow at most one in-flight tx for any authority with a | ||
| // pending transaction. | ||
| if auths := tx.SetCodeAuthorities(); len(auths) > 0 { | ||
| for _, auth := range auths { | ||
| var count int | ||
| if pending := pool.pending[auth]; pending != nil { | ||
| count += pending.Len() | ||
| } | ||
| if queue := pool.queue[auth]; queue != nil { | ||
| count += queue.Len() | ||
| } | ||
| if count > 1 { | ||
| return ErrAuthorityReserved | ||
| } | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Authority reservation check misses cross-sender conflicts
pool.pending/pool.queue are keyed by sender, so this path only notices transactions whose sender equals the authority. It lets multiple SetCode txs from different senders reserve the same authority concurrently, defeating the single-in-flight guarantee the EIP requires. Please drive the check off the authority index you already maintain in txLookup so we examine the transactions that actually reference the authority and only allow same-sender/same-nonce replacements.
func (pool *TxPool) validateAuth(tx *types.Transaction) error {
@@
- if auths := tx.SetCodeAuthorities(); len(auths) > 0 {
- for _, auth := range auths {
- var count int
- if pending := pool.pending[auth]; pending != nil {
- count += pending.Len()
- }
- if queue := pool.queue[auth]; queue != nil {
- count += queue.Len()
- }
- if count > 1 {
- return ErrAuthorityReserved
- }
- }
+ if auths := tx.SetCodeAuthorities(); len(auths) > 0 {
+ from, _ := types.Sender(pool.signer, tx)
+ for _, auth := range auths {
+ for _, hash := range pool.all.AuthTxHashes(auth) {
+ if other := pool.all.Get(hash); other != nil {
+ ofrom, _ := types.Sender(pool.signer, other)
+ if ofrom != from || other.Nonce() != tx.Nonce() {
+ return ErrAuthorityReserved
+ }
+ }
+ }
+ }
}
return nil
}Drop this helper next to hasAuth so the validation can query the authority index:
func (t *txLookup) AuthTxHashes(addr common.Address) []common.Hash {
t.lock.RLock()
defer t.lock.RUnlock()
hashes := t.auths[addr]
if len(hashes) == 0 {
return nil
}
out := make([]common.Hash, len(hashes))
copy(out, hashes)
return out
}| pool.eip1559 = pool.chainconfig.IsCurie(next) | ||
| pool.shanghai = pool.chainconfig.IsShanghai(next) | ||
| pool.eip7702 = pool.chainconfig.IsViridian(next, newHead.Time) | ||
|
|
There was a problem hiding this comment.
Enable SetCode as soon as the fork time is reachable
Basing pool.eip7702 on newHead.Time delays the switch until after the first Viridian block lands, so the txpool rejects every SetCode tx during the fork block and miners cannot include them. Compute the flag from the earliest legal timestamp for the next block (parent time + 1, clamped to time.Now()), so we accept SetCode txs as soon as the fork timestamp is attainable.
- pool.eip7702 = pool.chainconfig.IsViridian(next, newHead.Time)
+ nextTime := newHead.Time + 1
+ if unix := uint64(time.Now().Unix()); unix > nextTime {
+ nextTime = unix
+ }
+ pool.eip7702 = pool.chainconfig.IsViridian(next, nextTime)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pool.eip1559 = pool.chainconfig.IsCurie(next) | |
| pool.shanghai = pool.chainconfig.IsShanghai(next) | |
| pool.eip7702 = pool.chainconfig.IsViridian(next, newHead.Time) | |
| pool.eip1559 = pool.chainconfig.IsCurie(next) | |
| pool.shanghai = pool.chainconfig.IsShanghai(next) | |
| nextTime := newHead.Time + 1 | |
| if unix := uint64(time.Now().Unix()); unix > nextTime { | |
| nextTime = unix | |
| } | |
| pool.eip7702 = pool.chainconfig.IsViridian(next, nextTime) |
🤖 Prompt for AI Agents
In core/tx_pool.go around lines 1452-1455, pool.eip7702 is computed using
newHead.Time which delays enabling SetCode until after the fork block; instead
compute the earliest legal timestamp for the next block as parent.Time + 1 and
clamp it to now (i.e. use max(parent.Time+1, time.Now())), then pass that
earliest timestamp into chainconfig.IsViridian(next, earliestTimestamp) so
pool.eip7702 becomes true as soon as the fork timestamp is attainable.
fix: set tx context before hooks tx start
fix: Address fee mismatch issue in L1DataFee settlement
fix: return err when failed to query block in GetBlockReceipts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/state_transition.go (1)
530-535: Refund path for existing authority accounts.Note: This refund also counts towards the global refund cap. If the intent is to guarantee net TxAuthTupleGas for existing accounts irrespective of cap, this will not hold. See earlier gas comment.
🧹 Nitpick comments (3)
core/state_transition.go (3)
133-135: Auth tuple gas via refund is capped by EIP‑3529; confirm economics.Current model:
- Intrinsic: len(authList) * CallNewAccountGas.
- Runtime: if account exists, refund (CallNewAccountGas - TxAuthTupleGas).
This makes net cost for existing accounts subject to the refund cap (gasUsed/5 post‑3529), potentially overcharging txs with many authorizations. Is this intentional per EIP‑7702? If not, consider a non‑refund approach (e.g., charge TxAuthTupleGas intrinsically and debit the delta only when creating a new authority account).
Also applies to: 180-183
429-434: Invalid authorizations are skipped silently; confirm spec/UX.You ignore errors from applyAuthorization. If this is spec‑intended (invalid tuples are no‑ops), fine. Otherwise consider surfacing a reason via tracer/logs to aid debugging.
508-510: Typo in comment.“exisiting” → “existing”.
- // 1) doesn't have code or has exisiting delegation + // 1) doesn't have code or has existing delegation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/state_processor.go(4 hunks)core/state_transition.go(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
core/state_transition.go (6)
core/types/tx_setcode.go (3)
SetCodeAuthorization(72-79)ParseDelegation(37-42)AddressToDelegation(45-47)internal/ethapi/api.go (1)
AccessList(1536-1632)params/protocol_params.go (2)
CallNewAccountGas(31-31)TxAuthTupleGas(93-93)core/vm/interpreter.go (1)
Config(33-42)core/tracing/hooks.go (9)
GasChangeTxInitialBalance(294-294)BalanceDecreaseGasBuy(246-246)GasChangeTxIntrinsicGas(297-297)NonceChangeEoACall(362-362)BalanceIncreaseRewardTransactionFee(243-243)NonceChangeAuthorization(371-371)GasChangeTxRefunds(300-300)BalanceIncreaseGasReturn(248-248)GasChangeTxLeftOverReturned(305-305)core/error.go (8)
ErrSenderNoEOA(108-108)ErrSetCodeTxCreate(114-114)ErrEmptyAuthList(113-113)ErrAuthorizationWrongChainID(133-133)ErrAuthorizationNonceOverflow(134-134)ErrAuthorizationInvalidSignature(135-135)ErrAuthorizationDestinationHasCode(136-136)ErrAuthorizationNonceMismatch(137-137)
core/state_processor.go (8)
core/vm/interface.go (1)
StateDB(29-89)eth/tracers/dir.go (1)
Tracer(40-45)core/state/statedb_hooked.go (1)
NewHookedState(38-44)core/evm.go (2)
NewEVMBlockContext(41-69)NewEVMTxContext(72-78)core/vm/evm.go (3)
NewEVM(138-149)TxContext(92-97)EVM(108-134)core/types/transaction.go (1)
Message(738-752)core/types/transaction_signing.go (1)
MakeSigner(42-61)core/vm/interpreter.go (1)
Config(33-42)
🔇 Additional comments (7)
core/state_transition.go (3)
302-306: EOA check correctly permits delegated code.Allowing EOAs that have delegation code installed is aligned with EIP‑7702 semantics. LGTM.
337-345: Good shape checks for 7702.Rejecting contract creation and empty auth lists when authorizations are present is correct.
94-95: No issues found—all Message interface implementers define SetCodeAuthorizations().The verification confirms that all three implementations of
core.Messageproperly define theSetCodeAuthorizations()method:
Transaction.SetCodeAuthorizations()at core/types/transaction.go:484Message.SetCodeAuthorizations()at core/types/transaction.go:809callMsg.SetCodeAuthorizations()at accounts/abi/bind/backends/simulated.go:918All call sites throughout the codebase (tests, tx pool, API, tracers, state transition) successfully resolve to these implementations with consistent signatures. No missing implementations detected.
core/state_processor.go (4)
93-100: Tracing StateDB wrapping looks good.Hooking the statedb only when a tracer exists is clean; passing the hooked StateDB into the EVM is correct.
99-100: Single EVM reused across tx in a block; verify no per‑tx state leakage.EVM holds mutable fields (e.g., depth, callGasTemp) and caches. You reset TxContext per tx, but please confirm interpreter/EVM state is fully per‑call and safe to reuse across transactions.
Also applies to: 123-131
103-104: Time‑aware signer propagation.Switching to MakeSigner(config, header.Number, header.Time) at both call sites is consistent with the new API.
Also applies to: 203-211
131-138: Tx lifecycle tracing and receipt construction LGTM.OnTxStart/OnTxEnd hooks, L1 fee calculation and wiring into receipts, and Byzantium‑aware state root handling look correct.
Also applies to: 148-156, 172-196
| // Perform convenience warming of sender's delegation target. Although the | ||
| // sender is already warmed in Prepare(..), it's possible a delegation to | ||
| // the account was deployed during this transaction. To handle correctly, | ||
| // simply wait until the final state of delegations is determined before | ||
| // performing the resolution and warming. | ||
| if addr, ok := types.ParseDelegation(st.state.GetCode(*msg.To())); ok { | ||
| st.state.AddAddressToAccessList(addr) | ||
| } | ||
|
|
There was a problem hiding this comment.
Comment/code mismatch (“sender” vs “recipient”) in warming block.
Code warms delegation target of msg.To(), but comment says “sender’s delegation target”. Update the comment (or change the code if warming the sender was intended).
Apply this diff to fix the comment:
- // Perform convenience warming of sender's delegation target. Although the
- // sender is already warmed in Prepare(..), it's possible a delegation to
+ // Perform convenience warming of the recipient's delegation target. Although the
+ // recipient is already warmed in Prepare(..), it's possible a delegation to
- // the account was deployed during this transaction. To handle correctly,
+ // the account was deployed during this transaction. To handle correctly,🤖 Prompt for AI Agents
In core/state_transition.go around lines 436 to 444, the comment refers to
"sender's delegation target" but the code actually warms the delegation target
of msg.To() (the recipient); update the comment to say it warms the recipient's
(msg.To()) delegation target or otherwise change wording to "recipient" /
"msg.To()" so the comment matches the code, keeping the existing logic that
parses delegation from st.state.GetCode(*msg.To()) and adds the address to the
access list.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/geth/chaincmd.go(2 hunks)cmd/geth/main.go(1 hunks)cmd/utils/flags.go(1 hunks)core/genesis.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/utils/flags.go
- cmd/geth/chaincmd.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/geth/main.go (1)
cmd/utils/flags.go (1)
OverrideViridianTimeFlag(278-281)
core/genesis.go (1)
core/tracing/hooks.go (2)
BalanceIncreaseGenesisBalance(239-239)NonceChangeGenesis(359-359)
🔇 Additional comments (4)
core/genesis.go (4)
34-34: LGTM!The tracing import is correctly added to support the genesis-level tracing constants used in lines 348 and 350.
161-163: LGTM!The override application logic correctly follows the established pattern for fork time configuration, with appropriate nil checks and config validation.
348-350: LGTM!The tracing parameters are correctly added to enable genesis-level state mutations to be recorded with specific reasons. The constants
BalanceIncreaseGenesisBalanceandNonceChangeGenesisare properly defined incore/tracing/hooks.go.
150-150: No issues found. TheViridianTimefield is properly defined inparams/config.go(line 579) with the correct type*uint64, matching the pattern ofMorph203Time. The code change atcore/genesis.go:150will not produce compilation errors.
| utils.SmartCardDaemonPathFlag, | ||
| utils.OverrideArrowGlacierFlag, | ||
| utils.OverrideMorph203TimeFlag, | ||
| utils.OverrideViridianTimeFlag, |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! Flag addition follows the established pattern.
The addition of utils.OverrideViridianTimeFlag to the nodeFlags slice is correct and consistent with the existing OverrideMorph203TimeFlag on line 74. The flag enables CLI-level control of the Viridian fork timestamp, which aligns with the EIP-7702 implementation's time-aware fork handling.
Run the following script to verify the flag is properly consumed by the configuration logic:
🏁 Script executed:
#!/bin/bash
# Description: Verify OverrideViridianTimeFlag usage across the codebase
# Search for references to OverrideViridianTimeFlag
echo "=== Searching for OverrideViridianTimeFlag references ==="
rg -n -C3 'OverrideViridianTimeFlag' --type=go
# Search for OverrideViridianTime (the config field that should consume this flag)
echo -e "\n=== Searching for OverrideViridianTime config field ==="
rg -n -C3 'OverrideViridianTime' --type=go -g '!*_test.go'
# Search for Viridian fork/time handling
echo -e "\n=== Searching for Viridian fork references ==="
rg -n -C2 '\bViridian\b' --type=go -g '!*_test.go' | head -50Length of output: 7881
[skip_cloning: false]
🏁 Script executed:
#!/bin/bash
# Verify OverrideMorph300Time vs OverrideViridianTime discrepancy
echo "=== Searching for OverrideMorph300Time ==="
rg -n 'OverrideMorph300Time|Morph300' --type=go -C2
echo -e "\n=== Checking gen_config.go context ==="
rg -n 'enc\.' eth/ethconfig/gen_config.go -A1 -B1 | grep -E '(OverrideViridian|OverrideMorph|Morph300)' -C1Length of output: 891
🏁 Script executed:
#!/bin/bash
# Verify the config struct definition to understand the naming scheme
echo "=== Checking eth/ethconfig/config.go struct definition ==="
rg -n 'type Config struct' eth/ethconfig/config.go -A 100 | head -150
echo -e "\n=== Confirming no OverrideMorph300Time in main Config ==="
rg -n 'OverrideMorph300Time' eth/ethconfig/config.go
echo -e "\n=== Checking UnmarshalTOML to see inverse mapping ==="
rg -n 'func.*UnmarshalTOML' eth/ethconfig/gen_config.go -A 50 | grep -E '(OverrideViridian|OverrideMorph300)' -B2 -A2Length of output: 4351
🏁 Script executed:
#!/bin/bash
# Verify the config struct definition and understand the naming scheme
echo "=== Checking eth/ethconfig/config.go struct definition ==="
sed -n '/^type Config struct/,/^}/p' eth/ethconfig/config.go | tail -30
echo -e "\n=== Confirming OverrideMorph300Time location ==="
rg -n 'OverrideMorph300Time' eth/ethconfig/
echo -e "\n=== Check UnmarshalTOML for reverse mapping ==="
rg -n 'func.*UnmarshalTOML' eth/ethconfig/gen_config.go -A 60 | grep -E '(Viridian|Morph300)' -B1 -A1Length of output: 1237
🏁 Script executed:
#!/bin/bash
# Verify the config struct definition and understand the naming scheme
echo "=== Checking eth/ethconfig/config.go struct definition ==="
rg -n 'OverrideViridianTime|OverrideMorph203Time' eth/ethconfig/config.go -B2 -A2
echo -e "\n=== Confirming OverrideMorph300Time only in gen_config.go ==="
rg -l 'OverrideMorph300Time' --type=go
echo -e "\n=== Check UnmarshalTOML for reverse mapping ==="
ast-grep --pattern $'func ($_ *Config) UnmarshalTOML($_) $_ {
$$$
}'Length of output: 12435
🏁 Script executed:
#!/bin/bash
# Verify the config struct definition and understand the naming scheme
echo "=== Config struct fields related to overrides ==="
rg -n 'Override.*Time' eth/ethconfig/config.go
echo -e "\n=== Files containing OverrideMorph300Time ==="
rg -l 'OverrideMorph300Time' --type=go
echo -e "\n=== UnmarshalTOML function mapping ==="
rg -n 'UnmarshalTOML' eth/ethconfig/gen_config.go -A 80 | tail -50Length of output: 1940
🏁 Script executed:
#!/bin/bash
# Verify config structure and naming
echo "=== Config.go Override fields ==="
grep -n 'Override.*Time' eth/ethconfig/config.go
echo -e "\n=== Files with OverrideMorph300Time ==="
grep -r 'OverrideMorph300Time' --include='*.go' .
echo -e "\n=== UnmarshalTOML mapping logic ==="
sed -n '/func (c \*Config) UnmarshalTOML/,/^}/p' eth/ethconfig/gen_config.go | grep -E '(Viridian|Morph300|Marshal)' -B1 -A1Length of output: 384
🏁 Script executed:
#!/bin/bash
# Direct check on UnmarshalTOML logic
echo "=== UnmarshalTOML full function from gen_config.go ==="
awk '/func.*\(c \*Config\) UnmarshalTOML/,/^}$/{print NR": "$0}' eth/ethconfig/gen_config.go | head -100Length of output: 4125
🏁 Script executed:
#!/bin/bash
# Examine the full UnmarshalTOML to understand the bidirectional mapping
cat eth/ethconfig/gen_config.go | sed -n '/func.*UnmarshalTOML/,/^}/p' | grep -n '.'Length of output: 6535
🏁 Script executed:
#!/bin/bash
# Check file header and full UnmarshalTOML function
echo "=== File header (check for code generation marker) ==="
head -10 eth/ethconfig/gen_config.go
echo -e "\n=== Full UnmarshalTOML function ==="
grep -n 'func.*UnmarshalTOML' eth/ethconfig/gen_config.go
# Get line number and extract that section
START_LINE=$(grep -n 'func.*UnmarshalTOML' eth/ethconfig/gen_config.go | cut -d: -f1)
if [ ! -z "$START_LINE" ]; then
echo -e "\n=== UnmarshalTOML body starting at line $START_LINE ==="
sed -n "${START_LINE},$((START_LINE+100))p" eth/ethconfig/gen_config.go
fiLength of output: 3995
🏁 Script executed:
#!/bin/bash
# Check file header and get UnmarshalTOML function with proper line context
echo "=== File header (first 15 lines) ==="
head -15 eth/ethconfig/gen_config.go
echo -e "\n=== Locate and extract UnmarshalTOML ==="
awk '/func \(c \*Config\) UnmarshalTOML/,/^}/ {print NR": "$0}' eth/ethconfig/gen_config.go | head -120Length of output: 5221
🏁 Script executed:
#!/bin/bash
cat eth/ethconfig/gen_config.go | head -20
echo "---"
python3 << 'PYEOF'
with open('eth/ethconfig/gen_config.go', 'r') as f:
lines = f.readlines()
in_func = False
for i, line in enumerate(lines, 1):
if 'UnmarshalTOML' in line:
in_func = True
if in_func:
print(f"{i}: {line}", end='')
if line.strip() == '}' and i > 100:
break
PYEOFLength of output: 3230
🏁 Script executed:
#!/bin/bash
python3 << 'PYEOF'
import re
# Read the file
with open('eth/ethconfig/gen_config.go', 'r') as f:
content = f.read()
# Check file header
lines = content.split('\n')
print("=== File header ===")
for i, line in enumerate(lines[:15]):
print(f"{i+1}: {line}")
# Find UnmarshalTOML function
match = re.search(r'(func \(c \*Config\) UnmarshalTOML.*?\n})', content, re.DOTALL)
if match:
func_lines = match.group(1).split('\n')
print("\n=== UnmarshalTOML function ===")
start = next(i for i, l in enumerate(lines) if 'UnmarshalTOML' in l)
for i, line in enumerate(func_lines[:80]):
print(f"{start+i+1}: {line}")
PYEOFLength of output: 3998
🏁 Script executed:
#!/bin/bash
python3 << 'PYEOF'
with open('eth/ethconfig/gen_config.go', 'r') as f:
lines = f.readlines()
# Show first 20 lines
print("=== File Header ===")
print(''.join(lines[:20]))
# Find and show UnmarshalTOML
print("\n=== UnmarshalTOML Function ===")
in_func = False
for i, line in enumerate(lines):
if 'func (c *Config) UnmarshalTOML' in line:
in_func = True
start_line = i
if in_func:
print(f"{i+1}: {line.rstrip()}")
if line.strip() == '}' and i > start_line:
break
PYEOFLength of output: 3235
🏁 Script executed:
#!/bin/bash
python3 << 'PYEOF'
with open('eth/ethconfig/gen_config.go', 'r') as f:
lines = f.readlines()
print("=== First 20 lines ===")
for i in range(20):
print(f"{i+1}: {lines[i].rstrip()}")
print("\n=== UnmarshalTOML function ===")
for i in range(len(lines)):
if 'UnmarshalTOML' in lines[i]:
for j in range(i, min(i+120, len(lines))):
print(f"{j+1}: {lines[j].rstrip()}")
if j > i and lines[j].strip() == '}':
break
break
PYEOFLength of output: 3352
🏁 Script executed:
#!/bin/bash
python3 << 'EOF'
try:
with open('eth/ethconfig/gen_config.go', 'r') as f:
content = f.read()
# Find UnmarshalTOML
idx = content.find('UnmarshalTOML')
if idx > 0:
# Get lines around it
start = content.rfind('\n', max(0, idx-200), idx)
end = content.find('\n}\n', idx) + 3
section = content[start:end]
print(section)
else:
print("UnmarshalTOML not found")
except Exception as e:
print(f"Error: {e}")
EOFLength of output: 5912
🏁 Script executed:
#!/bin/bash
# Simple approach: show lines 130-200 which should contain UnmarshalTOML
sed -n '125,200p' eth/ethconfig/gen_config.goLength of output: 2881
🏁 Script executed:
#!/bin/bash
# Direct file view using sed - get context around UnmarshalTOML
echo "=== MarshalTOML (lines 105-120) ==="
sed -n '105,120p' eth/ethconfig/gen_config.go
echo -e "\n=== UnmarshalTOML location ==="
grep -n 'func.*UnmarshalTOML' eth/ethconfig/gen_config.go
echo -e "\n=== UnmarshalTOML function (lines 120-200) ==="
sed -n '120,200p' eth/ethconfig/gen_config.goLength of output: 3860
🏁 Script executed:
#!/bin/bash
sed -n '1,250p' eth/ethconfig/gen_config.go | tail -120Length of output: 3997
🏁 Script executed:
#!/bin/bash
cat eth/ethconfig/gen_config.go | tail -60Length of output: 1357
🏁 Script executed:
#!/bin/bash
wc -l eth/ethconfig/gen_config.go
tail -100 eth/ethconfig/gen_config.goLength of output: 2539
🏁 Script executed:
#!/bin/bash
# Get file line count and show full MarshalTOML and UnmarshalTOML sections
echo "=== File stats ==="
wc -l eth/ethconfig/gen_config.go
echo -e "\n=== Lines 60-120 (covering both functions) ==="
sed -n '60,120p' eth/ethconfig/gen_config.go
echo -e "\n=== Lines 120-180 (UnmarshalTOML) ==="
sed -n '120,180p' eth/ethconfig/gen_config.goLength of output: 5083
🏁 Script executed:
#!/bin/bash
# Show the entire file content
cat eth/ethconfig/gen_config.goLength of output: 10593
Critical serialization bug in generated code must be fixed.
The addition of utils.OverrideViridianTimeFlag in main.go is correct, but verification reveals a one-way serialization bug in eth/ethconfig/gen_config.go:
MarshalTOML (line 116) serializes OverrideViridianTime → OverrideMorph300Time for TOML output, but UnmarshalTOML (lines 120–312) never deserializes OverrideMorph300Time back. The internal Config struct in UnmarshalTOML only includes OverrideMorph203Time and terminates at line 312 without handling OverrideMorph300Time. This causes configuration files to lose the Viridian override value when read.
The generated codec file needs to be regenerated or manually patched to include bidirectional mapping for OverrideMorph300Time ↔ OverrideViridianTime in UnmarshalTOML.
🤖 Prompt for AI Agents
In cmd/geth/main.go around line 75 the new utils.OverrideViridianTimeFlag was
added but the generated codec eth/ethconfig/gen_config.go contains a one-way
serialization bug: MarshalTOML writes OverrideViridianTime as
OverrideMorph300Time but UnmarshalTOML never reads OverrideMorph300Time back.
Fix by regenerating the codec or manually patching eth/ethconfig/gen_config.go
UnmarshalTOML to include OverrideMorph300Time in the internal Config struct and
map it back into the exported OverrideViridianTime field (i.e., add the
OverrideMorph300Time field to the temporary struct used for TOML unmarshalling
and assign its value to cfg.OverrideViridianTime), ensuring symmetric TOML
marshal/unmarshal behavior.
3 participants
1. Purpose or design rationale of this PR
This PR implements EIP-7702 (Account Delegation) following the specification at
https://eips.ethereum.org/EIPS/eip-7702. The implementation is based entirely on Ethereum's go-ethereum reference implementation.2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Improvements
Chores