core, params, miner: merge geth v1.17.1 (v1.17.4 sync, milestone 3/6)

[pratikspatil024]

Important

Reviewer guide — stacked PR 3 of 12. Part of the combined go-ethereum v1.17.4 + v1.17.5 upstream sync, which ships as one stable release. Every PR in the stack merges into the base branch upstream-merge-v1.17.4; that base merges into develop once, at the very end — not per-PR.

Merge-commit only — never squash. Squashing rewrites a branch's SHAs and breaks every PR stacked above it.

Review bottom-up: #2308 → #2319 → #2325 → #2328 → #2337 → #2340 → #2341 → #2342 → #2343 → #2345 → #2346 → #2354. Start at #2308 / #2319 — every PR above inherits them, so reviewing top-down means re-reviewing.

Expected-red / flaky checks (not code blockers): Quality metrics (diffguard — skipped by team decision; it also mis-scopes across a stacked diff, comparing against the bottom of the stack), and codecov/project (repo-wide coverage threshold; per-PR patch coverage is green). Kurtosis e2e occasionally flakes (~1-in-5, devtools-owned) and is re-run by hand. Full per-batch conflict-resolution reasoning is in docs/upstream-merges/.

---

Summary

Milestone 3/6 of the Bor ← go-ethereum v1.17.4 sync: merges upstream v1.17.0 → v1.17.1 (40 first-parent commits) across 2 batches, plus the milestone-chores docs.

Introduces the Amsterdam fork surface (EIP-7843 SLOTNUM, EIP-8024, BAL precompile-touch) — wired block-based-nil like Osaka/Prague/Verkle, dormant on every Bor network. Enabling later is just setting AmsterdamBlock (no core-logic changes). This also corrects a v1.17.0 gap where upstream's Amsterdam gate had been dropped rather than converted to Bor's block-based style.

Stacked on #2319 (v1.17.0). Base retargets up-chain as lower PRs merge + their branches are deleted.

Batch	Upstream boundary	Merge commit
14 (1/2)	9ecb6c4ae	189030866
15 (2/2)	16783c167	b6175113d (incl. Amsterdam gate wiring)
chores	—	dbae0f4a1 (docs)

Executed tests

• go build ./... rc=0; go vet clean (bar pre-existing //nolint copylocks).
• Full go test ./... — no v1.17.1 regressions. Two failures confirmed pre-existing/flaky: cmd/geth TestCustomBackend (VEBLOP/non-Bor-genesis nil-deref, crash path byte-identical to v1.17.0) and consensus/bor/heimdall TestFailover_ThreeClients_CascadeToTertiary (flaky, passes 5/5 on re-run).
• make test-integration (-tags integration -p 1 ./tests/...) — PASS, incl. tests/bor consensus e2e (620s, 77.6% cov, GitCommit b6175113d).
• govulncheck — 3 called vulns, all pre-existing/develop-inherent (x/text, x/crypto, crypto/tls); c-kzg v2.1.6 added none.
• Amsterdam dormancy verified via TestReinforceMultiClientPreCompilesTest + TestV2ForkParity + TestBorHardforkPrecompileContinuity*.
• Kurtosis live devnet skipped this milestone: Amsterdam is dormant so v1.17.1 is behaviorally identical to v1.17.0 on every Bor network (v1.17.0 passed 8/8 probes), and tests/bor is green.

Rollout notes

• No consensus behavior changes on any Bor network — the Amsterdam fork (and its SLOTNUM/EIP-8024/BAL surfaces) is merged dormant; AmsterdamBlock is nil on every preset so IsAmsterdam is false everywhere.
• Deferred (tracked in docs/upstream-merges/v1.17.4/needs-wiring.md): eth/68 drop (#33511, entangled with Bor's forked downloader/wit/state-sync-receipt/PoA propagation), on-chain-tx-check (#33607), testing_buildBlockV1 (#33656).
• Adopted dep/tooling: c-kzg v2.1.5→v2.1.6, olekukonko→internal/tablewriter migration.
• Merge, never squash — squashing rewrites SHAs and breaks every stacked PR above this one.
• diffguard (mutation) + the soak ladder are CI/operator gates, not run here.

🤖 Generated with Claude Code

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[pratikspatil024]

codegenie review

[pratikspatil024]

@claude review

[github-actions]

🧞 Codegenie Review

Warning

Review incomplete. Some review or verification work did not complete. Treat the results below as partial and consider rerunning.

Sorry, this review is incomplete. The allotted max token limit of 8000000 (config review.maxBudgetTokens) was reached and the review has been degraded. Raise the limit for a complete review.

⚠️ Found 14 verified issues.

Coverage

Partial review: 60 hunks were not reviewed because budget was exhausted before dispatch.
Reviewed 155/264 hunks before stopping.
Incomplete work: skipped 49, failed 60, verification incomplete 6.
Coverage levels: deep 32, normal 178, light 5, skip 49.
Unreviewed hunks by file:

• beacon/engine/types.go: 2 hunks (budget stopped before dispatch)
• build/checksums.txt: 1 hunk (budget stopped before dispatch)
• cmd/devp2p/internal/v5test/discv5tests.go: 1 hunk (budget stopped before dispatch)
• cmd/evm/internal/t8ntool/block.go: 3 hunks (budget stopped before dispatch)
• cmd/evm/internal/t8ntool/execution.go: 2 hunks (budget stopped before dispatch)
• cmd/evm/internal/t8ntool/transition.go: 1 hunk (budget stopped before dispatch)
• cmd/evm/main.go: 1 hunk (budget stopped before dispatch)
• cmd/geth/chaincmd.go: 1 hunk (budget stopped before dispatch)
• cmd/geth/dbcmd.go: 1 hunk (budget stopped before dispatch)
• cmd/geth/main.go: 1 hunk (budget stopped before dispatch)
• cmd/utils/flags.go: 1 hunk (budget stopped before dispatch)
• core/genesis.go: 2 hunks (budget stopped before dispatch)
• 20 additional files omitted.
• Budget stopped review work (token limit reached).
• Verification incomplete for 6 candidates.
• beacon/engine/gen_blockparams.go: generated file
• beacon/engine/gen_ed.go: generated file
• cmd/evm/internal/t8ntool/gen_header.go: generated file
• cmd/evm/internal/t8ntool/gen_stenv.go: generated file
• core/gen_genesis.go: generated file
• core/types/bal/bal_encoding_rlp_generated.go: generated file
• core/types/gen_header_json.go: generated file
• core/types/gen_header_rlp.go: generated file
• go.sum: lockfile
• tests/gen_btheader.go: generated file
• semantic composition skipped; deterministic fallback used

⚠️ Findings

🔵 Medium: HasReceipts drops ancient pruning-tail check, reports pruned receipts as present

File: core/rawdb/accessors_chain.go:681 ↗
Confidence: high

Impact: After the ancient tail is advanced, ChainFreezerReceiptTable entries below the tail are pruned but ChainFreezerHashTable entries are retained (prunable:false). isCanon therefore still returns true for such canonical blocks, and HasReceipts now unconditionally returns true. Callers that gate on HasReceipts (skipping receipt re-fetch/backfill during sync, or short-circuiting receipt queries) proceed as if receipts exist, while ReadReceiptsRLP's identical isCanon branch reads the pruned receipt table and yields empty data — producing empty receipt responses or skipped repair instead of the previous correct 'not present'.
The removed lines were a deliberate downstream guard ('Block is in ancient store, but receipts can be pruned. Check if the block number is above the pruning tail.'). The upstream merge silently reverted it, re-introducing the pruned-receipt false positive: HasReceipts and ReadReceiptsRLP no longer agree for any canonical block below the ancient pruning tail.

Evidence:

Changed code:

func HasReceipts(db ethdb.Reader, hash common.Hash, number uint64) bool {
	if isCanon(db, number, hash) {
 return true // removed: tail, _ := db.Tail(); if number >= tail { return true }; return false
	}
	if has, err := db.Has(blockReceiptsKey(number, hash)); !has || err != nil {
 return false
	}
	return true
}

core/rawdb/ancient_scheme.go ↗ (Decisive: the hash table is NOT prunable while the receipt table IS, so tail pruning removes receipts but leaves the canonical hash entries readable.):

var chainFreezerTableConfigs = map[string]freezerTableConfig{
	ChainFreezerHeaderTable: {noSnappy: false, prunable: false},
	ChainFreezerHashTable: {noSnappy: true, prunable: false},
	ChainFreezerBodiesTable: {noSnappy: false, prunable: true},
	ChainFreezerReceiptTable: {noSnappy: false, prunable: true},
	ChainFreezerDifficultyTable: {noSnappy: true, prunable: true},
}

core/rawdb/freezer.go ↗ (Confirms tail truncation only touches prunable tables, so Ancient(ChainFreezerHashTable, number) still succeeds below the tail.):

func (f *Freezer) TruncateTail(tail uint64) (uint64, error) {
	...
	for _, table := range f.tables {
 if table.config.prunable {
 if err := table.truncateTail(tail - f.offset.Load()); err != nil {
 return 0, err
 }
 }
	}
	f.tail.Store(tail)
	return old, nil
}

core/rawdb/accessors_chain.go ↗ (isCanon consults only the non-prunable hash table; it does not prove receipt data still exists, so it cannot substitute for the removed tail guard.):

func isCanon(reader ethdb.AncientReaderOp, number uint64, hash common.Hash) bool {
	h, err := reader.Ancient(ChainFreezerHashTable, number)
	if err != nil {
 return false
	}
	return bytes.Equal(h, hash[:])
}

core/rawdb/accessors_chain.go ↗ (The read path takes the same isCanon branch and returns empty data for pruned numbers, so HasReceipts==true and ReadReceiptsRLP==empty now disagree.):

func ReadReceiptsRLP(db ethdb.Reader, hash common.Hash, number uint64) rlp.RawValue {
	_ = db.ReadAncients(func(reader ethdb.AncientReaderOp) error {
 if isCanon(reader, number, hash) {
 data, _ = reader.Ancient(ChainFreezerReceiptTable, number)
 return nil
 }
 ...

Suggested fix: Restore the guard on the ancient branch:
if isCanon(db, number, hash) {
// Block is in ancient store, but receipts can be pruned.
// Check if the block number is above the pruning tail.
tail, _ := db.Tail()
return number >= tail
}
Alternatively, have the ancient branch verify the receipt table entry itself (e.g. read ChainFreezerReceiptTable and treat a read error/empty value as absent). If the drop is intentional, document why HasReceipts may disagree with ReadReceiptsRLP below the tail.

Suggested test: In core/rawdb, freeze N blocks, call TruncateTail past block X, then assert isCanon still resolves X (hash table non-prunable) while HasReceipts(db, hashX, X) == false and ReadReceiptsRLP(db, hashX, X) is empty — i.e. the two accessors agree.

🔵 Medium: Witness stats depth clamp removed: LevelStats.AddLeaf panics for trie paths deeper than 15

File: trie/levelstats.go:83 ↗
Confidence: medium

Impact: WitnessStats.Add passes the raw node-path length to trie.LevelStats.AddLeaf, which indexes a fixed [16]stat array with no bounds check or clamp. In base, both the account and storage branches clamped depth to 15 before indexing, so deep paths were merely bucketed into the last slot. In head that clamp is gone and no replacement guard exists on the reachable path (AddLeaf's only non-test caller is WitnessStats.Add), so a witness node path of length >= 16 causes an index-out-of-range panic inside witness statistics collection instead of a clamped counter increment. Node paths are nibble paths bounded by 64 levels, as the new file's own doc comment states.
A metrics-only accounting path becomes a panic source driven by chain data (deep account/storage trie paths) rather than developer input. Base code deliberately tolerated deep paths by clamping; the new helper converts that tolerated case into a crash of the block-execution/witness stats path.

Evidence:

Changed code:

// AddLeaf records a leaf depth. Witness collection reuses the value-node bucket
// for leaf accounting. It panics if the depth is outside [0, 15].
func (s *LevelStats) AddLeaf(depth int) {
	s.level[depth].value.Add(1)
}
const trieStatLevels = 16
type LevelStats struct {
	level [trieStatLevels]stat
}

core/stateless/stats.go ↗ (Base explicitly clamped out-of-range depths into the last bucket before indexing the fixed 16-entry array. This guard is the behavior the new helper drops.):

// base (before this PR)
for i, path := range paths {
	if i == len(paths)-1 || !strings.HasPrefix(paths[i+1], paths[i]) {
 depth := len(path)
 if owner == (common.Hash{}) {
 if depth >= len(s.accountTrieLeaves) {
 depth = len(s.accountTrieLeaves) - 1
 }
 s.accountTrieLeaves[depth] += 1
 } else {
 if depth >= len(s.storageTrieLeaves) {
 depth = len(s.storageTrieLeaves) - 1
...

core/stateless/stats.go ↗ (Only non-test caller of LevelStats.AddLeaf; passes raw len(path) with no clamp, so the removed guard is not re-enforced anywhere on the reachable path.):

// head (this PR)
for i, path := range paths {
	if i == len(paths)-1 || !strings.HasPrefix(paths[i+1], paths[i]) {
 ownerStat.AddLeaf(len(path))
	}
}

trie/levelstats_test.go ↗ (Same-PR test asserts AddLeaf(16) panics, proving the new helper has no bounds handling (it proves the behavior changed, not that it is safe).):

	NewLevelStats().AddLeaf(16)

Suggested fix: Restore the clamp, either in the helper:

func (s *LevelStats) AddLeaf(depth int) {
if depth < 0 {
return
}
if depth >= trieStatLevels {
depth = trieStatLevels - 1 // overflow bucket, matches previous behavior
}
s.level[depth].value.Add(1)
}

or at the caller in core/stateless/stats.go before calling AddLeaf, and update the doc comment and the panic-asserting test accordingly.

Suggested test: In core/stateless, call WitnessStats.Add with a nodes map containing a path key of length >= 16 and assert it does not panic and that the leaf is counted in the depth-15 overflow bucket (matching pre-PR behavior).

🔵 Medium: Removed depth clamp: witness stats Add() can panic on trie paths deeper than 15

File: core/stateless/stats.go:85 ↗
Confidence: medium

Impact: Add() feeds raw node-path lengths from witness node maps into trie.LevelStats.AddLeaf, which indexes a [16]stat array without bounds handling. If any collected node path is 16 or more nibbles long, AddLeaf panics with index out of range instead of bucketing the leaf at depth 15 as the base code did, turning a metrics-only accounting step into a panic during state commit on the witness-collection path.
The prior code could never panic because depth was clamped; the refactor moved indexing into a helper whose own comment documents that it panics beyond depth 15, and no caller-side clamp remains. A graceful degradation (over-deep leaves counted in the last bucket) was silently converted into a crash of the block-processing path.

Evidence:

Changed code:

ownerStat := s.accountTrie
if owner != (common.Hash{}) {
	ownerStat = s.storageTrie
}
for i, path := range paths {
	if i == len(paths)-1 || !strings.HasPrefix(paths[i+1], paths[i]) {
		ownerStat.AddLeaf(len(path))
	}
}

core/stateless/stats.go ↗ (Base version clamped depth to 15 before indexing the fixed 16-slot arrays; the clamp is deleted in head and not reintroduced anywhere on the path.):

OLD:
			depth := len(path)
			if owner == (common.Hash{}) {
				if depth >= len(s.accountTrieLeaves) {
					depth = len(s.accountTrieLeaves) - 1
				}
				s.accountTrieLeaves[depth] += 1
			} else {
				if depth >= len(s.storageTrieLeaves) {
					depth = len(s.storageTrieLeaves) - 1
				}
				s.storageTrieLeaves[depth] += 1
...

trie/levelstats.go ↗ (Complete decisive helper branch: AddLeaf performs an unchecked index into a [16]stat array and its own doc states it panics for depth outside [0,15], so the removed caller-side clamp is not re-enforced by the new helper.):

const trieStatLevels = 16
// Note: theoretically it is possible to have up to 64 trie levels, but
// LevelStats supports exactly 16 levels and panics on deeper paths.
type LevelStats struct {
	level [trieStatLevels]stat
}
// AddLeaf records a leaf depth. ... It panics if the depth is outside [0, 15].
func (s *LevelStats) AddLeaf(depth int) {
	s.level[depth].value.Add(1)
}

core/state/statedb.go ↗ (Shows Add() is reached during state commit with witness node-path maps when witness stats collection is enabled, so the unchecked index is on a live (opt-in) production path.):

if s.witnessStats != nil {
	s.witnessStats.Add(witness, obj.addrHash())
}

Suggested fix: Restore the clamp at the call site, e.g. ownerStat.AddLeaf(min(len(path), trieStatLevelsMax)), or make trie.LevelStats.AddLeaf clamp (or ignore) depths outside [0, 15] instead of indexing unchecked.

Suggested test: Add a case to TestWitnessStatsAdd with a node key longer than 15 characters (e.g. strings.Repeat("a", 20)) and assert it is counted at depth 15 rather than panicking.

🔵 Medium: HasBody drops ancient-tail check, may report pruned bodies as present

File: core/rawdb/accessors_chain.go:583 ↗
Confidence: medium

Impact: For a canonical block whose body was pruned from the freezer (below the ancient tail) while its hash-table entry remains readable — possible because Freezer.TruncateTail truncates only tables marked prunable — HasBody now returns true unconditionally via isCanon. Callers that gate on HasBody and then call ReadBody/ReadBodyRLP get nil/empty data (e.g. serving eth block-body requests, chain reconstruction/ancestor walks), turning a clean 'not available' into an inconsistent 'present but unreadable'.
The removed lines were a downstream ancient-pruning guard that explicitly documented that ancient bodies can be pruned and required number >= tail before reporting presence. Reverting to the upstream form reintroduces a HasBody/ReadBody divergence in pruned-ancient deployments — exactly the case the guard protected — and does so silently as part of a sync merge with no stated intent to drop it.

Evidence:

Changed code:

func HasBody(db ethdb.Reader, hash common.Hash, number uint64) bool {
	if isCanon(db, number, hash) {
 return true
	}
	if has, err := db.Has(blockBodyKey(number, hash)); !has || err != nil {
 return false
	}
	return true
}
// removed by this hunk:
// // Block is in ancient store, but bodies can be pruned.
// // Check if the block number is above the pruning tail.
...

core/rawdb/accessors_chain.go ↗ (isCanon only consults ChainFreezerHashTable; it does not prove the bodies table still holds the item, which is what the removed tail guard checked.):

func isCanon(reader ethdb.AncientReaderOp, number uint64, hash common.Hash) bool {
	h, err := reader.Ancient(ChainFreezerHashTable, number)
	if err != nil {
 return false
	}
	return bytes.Equal(h, hash[:])
}

core/rawdb/freezer.go ↗ (Decisive: tail truncation is selective — non-prunable tables (e.g. the hash table) keep entries below the freezer tail, so isCanon can still resolve a hash for a number whose body was pruned.):

// TruncateTail discards all data below the specified threshold. Note that only
// 'prunable' tables will be truncated.
func (f *Freezer) TruncateTail(tail uint64) (uint64, error) {
	...
	for _, table := range f.tables {
 if table.config.prunable {
 if err := table.truncateTail(tail - f.offset.Load()); err != nil {
 return 0, err
 }
 }
	}
	f.tail.Store(tail)

core/rawdb/accessors_chain.go ↗ (Read path reads the bodies table, which IS pruned; so HasBody=true while ReadBody returns nil for the same (hash, number).):

// ReadBodyRLP ...
	_ = db.ReadAncients(func(reader ethdb.AncientReaderOp) error {
 if isCanon(reader, number, hash) {
 data, _ = reader.Ancient(ChainFreezerBodiesTable, number)
 return nil
 }
 data, _ = db.Get(blockBodyKey(number, hash))
 return nil
	})

Suggested fix: Restore the fork's tail guard:

if isCanon(db, number, hash) {
// Block is in ancient store, but bodies can be pruned.
// Check if the block number is above the pruning tail.
tail, _ := db.Tail()
return number >= tail
}

Or, if dropping it is intentional in this sync, confirm in the chain-freezer table config that ChainFreezerHashTable is prunable (so isCanon cannot succeed below the tail) and note that in the PR.

Suggested test: In core/rawdb, freeze N canonical blocks, call TruncateTail to prune early bodies, then assert for a pruned number that HasBody(db, hash, number) == false and agrees with ReadBody(db, hash, number) == nil; also assert both are true/non-nil for a number above the tail.

🔵 Medium: New AmsterdamBlock fork field is not wired into CheckConfigForkOrder or checkCompatible

File: params/config.go:857 ↗
Confidence: medium

Impact: AmsterdamBlock is a user-settable JSON field (amsterdamBlock) that now drives consensus gates (Rules.IsAmsterdam -> amsterdamInstructionSet, EIP-7843 SlotNumber header validation, precompile touch). Because it is absent from CheckConfigForkOrder's fork list and from checkCompatible, (a) a genesis that schedules amsterdamBlock before osaka/prague or without earlier forks passes validation, and (b) editing amsterdamBlock on an already-synced node past that height yields no ConfigCompatError and no rewind, so the node silently applies different consensus rules to already-imported blocks.
Every other block-scheduled fork in this struct is covered by both the ordering check and the compatibility check; omitting the new one removes the two chokepoints that normally turn a mis-specified or mid-chain-changed fork schedule into a startup error instead of a silent consensus divergence. Today it is latent (nil on all presets) but becomes live as soon as any network or test config sets amsterdamBlock.

Evidence:

Changed code:

	OsakaBlock *big.Int `json:"osakaBlock,omitempty"` // Osaka switch Block (nil = no fork, 0 = already on osaka)
	AmsterdamBlock *big.Int `json:"amsterdamBlock,omitempty"` // Amsterdam switch Block (nil = no fork, 0 = already on amsterdam)

params/config.go ↗ (CheckConfigForkOrder's fork list (params/config.go ~1540-1551) has no amsterdamBlock entry, so an amsterdamBlock configured before/at the same height as an earlier fork is never rejected.):

 {name: "pragueBlock", block: c.PragueBlock, optional: true},
 {name: "osakaBlock", block: c.OsakaBlock, optional: true},
 {name: "verkleBlock", block: c.VerkleBlock, optional: true},
	} {

params/config.go ↗ (checkCompatible (params/config.go ~1717-1721) ends at Osaka; a changed amsterdamBlock across restarts produces no ConfigCompatError and therefore no chain rewind.):

	if isForkBlockIncompatible(c.OsakaBlock, newcfg.OsakaBlock, headNumber) {
 return newBlockCompatError("Osaka fork block", c.OsakaBlock, newcfg.OsakaBlock)
	}
	return nil
}

params/config.go ↗ (The field is consumed by consensus/EVM gates (Rules.IsAmsterdam, amsterdamInstructionSet, header SlotNumber validation), so a mis-scheduled or changed value has consensus impact once set.):

func (c *ChainConfig) IsAmsterdam(num *big.Int) bool {
	return c.IsLondon(num) && isBlockForked(c.AmsterdamBlock, num)
}

Suggested fix: Add {name: "amsterdamBlock", block: c.AmsterdamBlock, optional: true} after the osaka/verkle entries in CheckConfigForkOrder, and add an isForkBlockIncompatible(c.AmsterdamBlock, newcfg.AmsterdamBlock, headNumber) check in checkCompatible, mirroring the Osaka handling.

Suggested test: In params/config_test.go, add a TestCheckCompatible case where old cfg has AmsterdamBlock=10 and new cfg has AmsterdamBlock=20 with head 15, expecting a ConfigCompatError; and a CheckConfigForkOrder case with OsakaBlock=20, AmsterdamBlock=10 expecting an ordering error.

🔵 Medium: Reverting a timed-out request re-marks the peer idle while its response may still arrive, allowing concurrent duplicate assignment

File: eth/protocols/snap/sync.go:1974 ↗
Confidence: medium

Impact: On the timeout (and failed-send) revert path, revert*Request closes req.stale and immediately re-adds req.peer to the corresponding idle pool. No cancellation is sent to the peer, so its original request may still be outstanding. The scheduler can therefore assign a new request of the same type to the same unresponsive peer right away, and when the late response eventually arrives the response handler marks the peer idle again while that new request is in flight — yielding concurrent duplicate assignments to a peer that just timed out and removing the implicit timeout backoff upstream gets by restoring idleness only in the response handler. The same pattern is applied in revertAccountRequest/revertBytecodeRequest/revertStorageRequest/revertTrienodeHealRequest/revertBytecodeHealRequest.
Snap-sync scheduling uses the idle pools to steer work away from stalled peers and to keep one outstanding request per peer per type. Immediately re-idling a timed-out peer causes repeated assignment to a slow/stalled peer and can produce two concurrent in-flight requests of the same type for that peer, degrading heal/sync throughput and skewing the per-peer msgrate capacity estimates.

Evidence:

Changed code:

	// Remove the request from the tracked set and restore the peer to the
	// idle pool so it can be reassigned work (skip if peer already left).
	s.lock.Lock()
	delete(s.trienodeHealReqs, req.id)
	if _, ok := s.peers[req.peer]; ok {
 s.trienodeHealIdlers[req.peer] = struct{}{}
	}
	s.lock.Unlock()

eth/protocols/snap/sync.go ↗ (The revert path is reached from the request timeout timer. Reverting only closes req.stale; no cancellation is sent to the peer, so the original request can still be answered later while the peer has already been returned to the idle pool.):

req.timeout = time.AfterFunc(s.rates.TargetTimeout(), func() {
	peer.Log().Debug("... request timed out", "reqid", reqid)
	s.rates.Update(idle, ..., 0, 0)
	s.scheduleRevert...Request(req)
})

eth/protocols/snap/sync.go ↗ (Idleness is also restored when the late response finally arrives, so the peer can be marked idle again while a newly assigned request is in flight, producing two concurrent same-type requests to one peer.):

// response handler (e.g. OnTrieNodes)
	s.trienodeHealIdlers[peer.ID()] = struct{}{}

eth/protocols/snap/sync.go ↗ (Refutes the stale-idler concern for the drop path: s.peers and the idler maps are cleared before peerDrop is signalled, so the new s.peers guard correctly suppresses re-insertion for departed peers. The remaining issue is confined to the timeout/failed-send revert path.):

func (s *Syncer) Unregister(id string) error {
	s.lock.Lock()
	...
	delete(s.peers, id)
	...
	delete(s.trienodeHealIdlers, id)
	s.lock.Unlock()
	s.peerDrop.Send(id)

Suggested fix: Either keep upstream semantics (restore idleness only in the response handler, and instead handle the never-responding case by dropping/expiring the peer), or gate re-idling so a peer with an unresolved outstanding request of the same type is not re-added — e.g. only re-add on the failed-send/drop revert paths, or track outstanding request counts per peer and re-idle when the count reaches zero.

Suggested test: In eth/protocols/snap/sync_test.go, add a test with a peer that never answers a trienode heal request: assert the peer is not re-assigned another heal request before its previous response is resolved (or, if re-assignment is intended, assert that a late response for the stale reqid does not cause a second concurrent request for the same peer).

⚪ Low: New GraphQL Block.SlotNumber resolver is unreachable: slotNumber missing from graphql/schema.go SDL

File: graphql/graphql.go:1096 ↗
Confidence: high

Impact: graphql-go binds resolver methods only to fields declared in the SDL; an extra Go method is ignored. A client issuing { block { slotNumber } } is rejected at query-validation time with an unknown-field error on type Block, and Block.SlotNumber is never invoked. The EIP-7843 slot number is therefore unavailable over GraphQL even though it is available over JSON-RPC, and the newly added resolver is dead code.
The point of adding the resolver during the v1.17.x upstream sync is to expose header.SlotNumber via GraphQL. Without the SDL field the GraphQL API diverges from both upstream geth and this node's own JSON-RPC surface, and the gap is silent (no build or startup error), so it can persist unnoticed.

Evidence:

Changed code:

func (b *Block) SlotNumber(ctx context.Context) (*hexutil.Uint64, error) {
	header, err := b.resolveHeader(ctx)
	if err != nil {
 return nil, err
	}
	if header.SlotNumber == nil {
 return nil, nil
	}
	ret := hexutil.Uint64(*header.SlotNumber)
	return &ret, nil
}

graphql/schema.go ↗ (End of the Block SDL type (lines ~275-279). No slotNumber field is declared, and graphql/schema.go has no diff blocks in this PR, so the new resolver has no schema field to bind to.):

 # BlobGasUsed is the total amount of gas used by the transactions.
 blobGasUsed: Long
 # ExcessBlobGas is a running total of blob gas consumed in excess of the target, prior to the block.
 excessBlobGas: Long
 }

internal/ethapi/api.go ↗ (The JSON-RPC block marshaller already exposes slotNumber, showing the intended API surface that GraphQL now fails to match.):

	if head.SlotNumber != nil {
 result["slotNumber"] = head.SlotNumber

Suggested fix: Add the field to the Block type in graphql/schema.go, immediately after excessBlobGas:

SlotNumber is the EIP-7843 slot number of the block, or null for pre-EIP-7843 blocks.

slotNumber: Long

Suggested test: In graphql/graphql_test.go, add a case querying {block { slotNumber }} against a chain configured with EIP-7843 headers (expect the hex slot value) and against a pre-EIP-7843 chain (expect null), which also guards against the schema/resolver pair drifting apart again.

Summary-Only Findings

⚪ Low: LevelStats.AddLeaf panics on depth >= 16 and the witness-stats caller passes unclamped path length

File: trie/levelstats_test.go:36 ↗
Confidence: medium

Impact: WitnessStats.Add calls trie.LevelStats.AddLeaf(len(path)) with an unclamped node-path length. A witness node path of 16 or more nibbles (a deep account/storage branch, reachable in principle by grinding a long hashed-key prefix collision) indexes s.level[depth] beyond the fixed [16]stat array and panics, aborting the block-processing goroutine instead of degrading the metric. The newly added test locks this panic in as the intended contract rather than requiring a clamp or drop.
A statistics collector should saturate or drop out-of-range samples rather than panic, and encoding the panic as an asserted contract in tests makes the sharp edge durable. Impact is bounded: the only caller is reachable only when both VmConfig.StatelessSelfValidation and VmConfig.EnableWitnessStats are enabled, a debug/self-validation configuration that core/blockchain.go states should never run in production, so this is a robustness/contract-confirmation issue rather than a node-crash risk for default operators.

Evidence:

Changed code:

func TestLevelStatsAddLeafPanicsOnDepth16(t *testing.T) {
	defer func() {
 if r := recover(); r == nil {
 t.Fatal("expected panic for depth >= 16")
 }
	}()
	NewLevelStats().AddLeaf(16)
}

trie/levelstats.go ↗ (Confirmed by exact read of lines 18-95: AddLeaf indexes a fixed [16]stat array with no bounds guard, so depth >= 16 is an unrecovered index-out-of-range panic. The type comment notes tries may reach 64 levels.):

const trieStatLevels = 16
type LevelStats struct {
	level [trieStatLevels]stat
}
// AddLeaf records a leaf depth. Witness collection reuses the value-node bucket
// for leaf accounting. It panics if the depth is outside [0, 15].
func (s *LevelStats) AddLeaf(depth int) {
	s.level[depth].value.Add(1)
}

core/stateless/stats.go ↗ (The only production caller passes the raw nibble-path length with no clamp, so any witness node path of 16+ nibbles reaches the unguarded index.):

	for i, path := range paths {
 // If current path is a prefix of the next path, it's not a leaf.
 // The last path is always a leaf.
 if i == len(paths)-1 || !strings.HasPrefix(paths[i+1], paths[i]) {
 ownerStat.AddLeaf(len(path))
 }
	}

core/blockchain.go ↗ (Bounds the blast radius: the collector is only constructed under the doubly-gated StatelessSelfValidation + EnableWitnessStats debug configuration, so the panic cannot be triggered on a default production node.):

// Self validation should *never* run in production, it's more of
// a tight integration to enable running *all* consensus tests through the
// witness builder/runner ...
if witness = statedb.Witness(); witness != nil && bc.cfg.VmConfig.StatelessSelfValidation {
	...
	if bc.cfg.VmConfig.EnableWitnessStats {
 witnessStats = stateless.NewWitnessStats()
	}

Suggested fix: Either make AddLeaf tolerant (if depth < 0 || depth >= trieStatLevels { return }, or clamp into the last bucket) and update both trie/levelstats_test.go and core/stateless/stats_test.go to assert the safe behavior, or explicitly confirm that panicking on depth >= 16 is the intended contract given the caller is debug-gated.

Suggested test: If the clamp is adopted, replace TestLevelStatsAddLeafPanicsOnDepth16 with a test asserting AddLeaf(16) is a no-op (or increments the depth-15 bucket) and update core/stateless TestWitnessStatsPanicsOnDeepLeaf to assert WitnessStats.Add tolerates a 20-nibble node path.

⚪ Low: inspect-trie: nil header dereference when canonical hash exists but header record is missing

File: cmd/geth/dbcmd.go:117 ↗
Confidence: medium

Impact: Running geth db inspect-trie <blocknum> (or latest) against a datadir where rawdb.ReadCanonicalHash returns a non-zero hash but the corresponding header record is missing or its RLP is corrupt makes rawdb.ReadHeader return nil; the very next statement trieRoot = blockHeader.Root dereferences that nil pointer and the CLI panics with a stack trace instead of returning the intended "header not found" error.
This is a new operator-facing debug subcommand whose stated purpose is inspecting state/trie data on databases that may be partially synced, truncated, or damaged — exactly the conditions that produce a canonical-hash entry without a readable header. A nil-pointer panic gives no actionable diagnostic and reads like a crash bug, while the surrounding code already demonstrates the intended error-return style for the adjacent canonical-hash lookup.

Evidence:

Changed code:

if number != math.MaxUint64 {
	hash = rawdb.ReadCanonicalHash(db, number)
	if hash == (common.Hash{}) {
 return fmt.Errorf("canonical hash for block %d not found", number)
	}
	blockHeader := rawdb.ReadHeader(db, hash, number)
	trieRoot = blockHeader.Root
}

core/rawdb/accessors_chain.go ↗ (Decisive helper branch: ReadHeader returns nil both when the header RLP is absent and when it fails to decode, so blockHeader.Root at dbcmd.go:500 can dereference nil.):

func ReadHeader(db ethdb.Reader, hash common.Hash, number uint64) *types.Header {
	data := ReadHeaderRLP(db, hash, number)
	if len(data) == 0 {
 return nil
	}
	header := new(types.Header)
	if err := rlp.DecodeBytes(data, header); err != nil {
 log.Error("Invalid block header RLP", "hash", hash, "err", err)
 return nil
	}
	return header
}

Suggested fix: blockHeader := rawdb.ReadHeader(db, hash, number)
if blockHeader == nil {
return fmt.Errorf("header for block %d (%#x) not found", number, hash)
}
trieRoot = blockHeader.Root

Suggested test: Add a cmd/geth or rawdb-backed unit test that writes only the canonical hash mapping for a block number (no header record) and asserts inspectTrie returns an error containing "header ... not found" rather than panicking.

⚪ Low: TestHandshakeResend drops PONG-after-resend coverage; resp.Node assignment is dead

File: cmd/devp2p/internal/v5test/discv5tests.go:200 ↗
Confidence: medium

Impact: In head, resp.Node = conn.remote is the last statement of the case and the function returns, so the mutated resp is never used. The test now only asserts that the second WHOAREYOU repeats the first challenge's Nonce and ChallengeData. Base asserted, via conn.reqresp, that the second PING could actually complete the handshake and yield a valid PONG. A regression in which the resent ChallengeData is echoed correctly but is stale/unusable for deriving session keys (exactly the area touched by switching from Whoareyou.Encoded to ChallengeData) would pass this test.
This is the dedicated conformance test for handshake challenge resend in the devp2p test suite, run against third-party discv5 implementations. Losing the 'answer the resent challenge and get a PONG' assertion narrows the test to a byte-equality check on the challenge and leaves the resend path's usability untested, while the leftover dead assignment signals the follow-up write/PONG check was dropped unintentionally.

Evidence:

Changed code:

	conn.write(l1, ping2, nil)
	switch resp := conn.read(l1).(type) {
	case *v5wire.Whoareyou:
 if resp.Nonce != challenge1.Nonce { ... }
 if !bytes.Equal(resp.ChallengeData, challenge1.ChallengeData) { ... }
 resp.Node = conn.remote
	default:
 t.Fatal("expected WHOAREYOU, got", resp)
	}
}

cmd/devp2p/internal/v5test/discv5tests.go ↗ (Base version (TestPingHandshakeInterrupted) completed the handshake via conn.reqresp and asserted a valid PONG for ping2; head no longer performs this step, removing the covered boundary.):

	// Send second PING.
	ping2 := &v5wire.Ping{ReqID: conn.nextReqID()}
	switch resp := conn.reqresp(l1, ping2).(type) {
	case *v5wire.Pong:
 checkPong(t, resp, ping2, l1)

cmd/devp2p/internal/v5test/discv5tests.go ↗ (In TestPingMultiIP the same resp.Node = ... assignment is immediately followed by writing the handshake packet and checking the PONG, showing the assignment is only meaningful when the challenge is answered.):

 resp.Node = s.Dest
 conn.write(l2, ping2, resp)
	...
	case *v5wire.Pong:
 checkPong(t, resp, ping2, l2)

Suggested fix: Complete the test as the leftover assignment implies:
resp.Node = conn.remote
conn.write(l1, ping2, resp)
}
// Catch the PONG for ping2.
switch resp := conn.read(l1).(type) {
case *v5wire.Pong:
checkPong(t, resp, ping2, l1)
default:
t.Fatal("expected PONG, got", resp)
}
Alternatively, if only the challenge-repeat property is intended, remove the dead resp.Node = conn.remote line and note in the comment that handshake completion is intentionally out of scope.

Suggested test: Extend TestHandshakeResend to answer the resent WHOAREYOU with the handshake packet for ping2 and assert a valid PONG via checkPong, covering that the resent ChallengeData is usable for session establishment.

⚪ Low: Unchecked nil header dereference in geth db inspect-trie

File: cmd/geth/dbcmd.go:500 ↗
Confidence: medium

Impact: rawdb.ReadHeader returns a nil *types.Header when no header is stored for (hash, number) or when the stored header RLP fails to decode. The following blockHeader.Root then dereferences nil and panics, so geth db inspect-trie <blocknum> crashes with a stack trace on an inconsistent or corrupted datadir (canonical-hash marker present without a readable header) instead of returning the clean CLI error used one line earlier for a missing canonical hash.
This is an offline diagnostic command intended to be run against damaged or unusual databases; the exact situation it is used to investigate is the one where it panics instead of reporting the problem. The guard is three lines and matches the surrounding error-handling style.

Evidence:

Changed code:

 blockHeader := rawdb.ReadHeader(db, hash, number)
 trieRoot = blockHeader.Root

core/rawdb/accessors_chain.go ↗ (Decisive callee branches: ReadHeader returns nil both when no header data is stored and when the stored header RLP fails to decode.):

func ReadHeader(db ethdb.Reader, hash common.Hash, number uint64) *types.Header {
	data := ReadHeaderRLP(db, hash, number)
	if len(data) == 0 {
 return nil
	}
	header := new(types.Header)
	if err := rlp.DecodeBytes(data, header); err != nil {
 log.Error("Invalid block header RLP", "hash", hash, "err", err)
 return nil
	}
	return header
}

cmd/geth/dbcmd.go ↗ (The immediately preceding lookup establishes the error-return contract for missing data; the header lookup skips the equivalent check.):

 hash = rawdb.ReadCanonicalHash(db, number)
 if hash == (common.Hash{}) {
 return fmt.Errorf("canonical hash for block %d not found", number)
 }

Suggested fix: blockHeader := rawdb.ReadHeader(db, hash, number)
if blockHeader == nil {
return fmt.Errorf("header for block %d (%s) not found", number, hash)
}
trieRoot = blockHeader.Root

Suggested test: Unit/CLI test: seed a test chaindb with WriteCanonicalHash for block N but no header (or a corrupt header RLP), invoke inspectTrie with argument N, and assert it returns a non-nil error rather than panicking.

⚪ Low: Re-idling the peer in revert*Request allows a tight assign/send-fail/revert loop on write errors

File: eth/protocols/snap/sync.go:1832 ↗
Confidence: medium

Impact: When a peer's request write fails synchronously (broken/closed connection that has not yet triggered Unregister), the request goroutine calls scheduleRevertAccountRequest immediately. With the new re-idle, revertAccountRequest puts the peer back into s.accountIdlers while it is still present in s.peers, and the next assignAccountTasks pass hands it the same task again, which fails again instantly. This spins assign -> send error -> revert -> re-idle (one goroutine plus one timer plus debug log lines per iteration) until the peer is unregistered. Before the change the peer stayed out of the idle pool, so a failing peer was used at most once per task.
During the window between a peer's write failure and its unregistration, the snap sync event loop can churn through many pointless assign/revert cycles, burning CPU, spamming debug logs and delaying assignment of that task to healthy peers. The timeout path is paced by rates.TargetTimeout() and is far less severe, but it likewise lets a black-hole peer keep re-acquiring tasks.

Evidence:

Changed code:

	// Remove the request from the tracked set and restore the peer to the
	// idle pool so it can be reassigned work (skip if peer already left).
	s.lock.Lock()
	delete(s.accountReqs, req.id)
	if _, ok := s.peers[req.peer]; ok {
 s.accountIdlers[req.peer] = struct{}{}
	}
	s.lock.Unlock()

eth/protocols/snap/sync.go ↗ (assignAccountTasks (1056-1162) reverts immediately, with no delay/backoff, when the write to the peer fails; each attempt allocates a goroutine and a timer.):

req.timeout = time.AfterFunc(s.rates.TargetTimeout(), func() { ... s.scheduleRevertAccountRequest(req) })
s.accountReqs[reqid] = req
delete(s.accountIdlers, idle)
s.pend.Add(1)
go func(root common.Hash) {
	defer s.pend.Done()
	...
	if err := peer.RequestAccountRange(reqid, root, req.origin, req.limit, uint64(cap)); err != nil {
 peer.Log().Debug("Failed to request account range", "err", err)
 s.scheduleRevertAccountRequest(req)
	}
}(s.root)

eth/protocols/snap/sync.go ↗ (Event loop (~758-790) re-runs assignment on every iteration, so a peer re-idled by the revert is reassigned the same task immediately.):

case req := <-accountReqFails:
	s.revertAccountRequest(req)
// loop top re-runs: s.assignAccountTasks(accountResps, accountReqFails, cancel)

eth/protocols/snap/sync.go ↗ (The only filter on idle peers is statelessPeers; a peer whose write failed is not marked stateless, so nothing prevents immediate reselection.):

for id := range s.accountIdlers {
	if _, ok := s.statelessPeers[id]; ok {
 continue
	}
	idlers.ids = append(idlers.ids, id)
	...
}

eth/protocols/snap/peer.go ↗ (The send error is a p2p write error on a broken connection, i.e. persistent rather than transient, so the retry fails again instantly.):

func (p *Peer) RequestAccountRange(...) error {
	...
	return p2p.Send(p.rw, GetAccountRangeMsg, &GetAccountRangePacket{...})
}

Suggested fix: Do not re-idle on the synchronous send-failure path, or add a marker/backoff so a peer whose request send failed is not reselected immediately (e.g. mark it stateless/skip until a successful delivery, or signal via s.update instead of re-idling unconditionally). Restrict the re-idle to the timeout path the commit body describes.

Suggested test: In eth/protocols/snap/sync_test.go, register a peer whose RequestAccountRange always returns an error and assert that the number of request attempts made to that peer stays bounded (e.g. a small constant) while sync progresses via other peers.

⚪ Low: revertBytecodeRequest re-idles a peer whose response is still outstanding after a timeout

File: eth/protocols/snap/sync.go:1878 ↗
Confidence: medium

Impact: On the timeout revert path the peer is still present in s.peers and still owes a response, so the new unconditional re-add puts it back into s.bytecodeIdlers immediately. assignBytecodeTasks can then dispatch a second bytecode request to a peer that already failed to answer within TargetTimeout, leaving two outstanding requests to one stalled peer and allowing repeated re-issuance to the same slow peer instead of preferring healthy idlers. The guard if _, ok := s.peers[req.peer] only filters the disconnect case (Unregister deletes from s.peers before peerDrop triggers revertRequests).
Snap sync scheduling quality depends on not immediately handing new work to peers that just timed out. The re-idle creates a timeout/reassign churn loop against the same slow peer and doubles its outstanding request count, which can slow state sync progress on peers with degraded links.

Evidence:

Changed code:

	// Remove the request from the tracked set and restore the peer to the
	// idle pool so it can be reassigned work (skip if peer already left).
	s.lock.Lock()
	delete(s.bytecodeReqs, req.id)
	if _, ok := s.peers[req.peer]; ok {
 s.bytecodeIdlers[req.peer] = struct{}{}
	}
	s.lock.Unlock()

eth/protocols/snap/sync.go ↗ (assignBytecodeTasks (1165-1278) removes the peer from bytecodeIdlers on dispatch and reverts on timeout while the peer is still registered in s.peers, so the new guard does not filter the timeout case; the peer becomes assignable again with a response still outstanding. It also decays the peer's msgrate capacity, which partially mitigates re-preferring the stalled peer.):

req.timeout = time.AfterFunc(s.rates.TargetTimeout(), func() {
	peer.Log().Debug("Bytecode request timed out", "reqid", reqid)
	s.rates.Update(idle, ByteCodesMsg, 0, 0)
	s.scheduleRevertBytecodeRequest(req)
})
s.bytecodeReqs[reqid] = req
delete(s.bytecodeIdlers, idle)

eth/protocols/snap/sync.go ↗ (onByteCodes (2827-2932) is the pre-existing idle-restore chokepoint and restores the peer even for a stale/late delivery, so the peer was not permanently excluded before this change; it also discards the stale delivery, bounding the impact of the duplicate outstanding request to scheduling churn rather than data corruption.):

defer func() {
	s.lock.Lock()
	defer s.lock.Unlock()
	if _, ok := s.peers[peer.ID()]; ok {
 s.bytecodeIdlers[peer.ID()] = struct{}{}
	}
	...
}()
...
req, ok := s.bytecodeReqs[id]
if !ok {
	logger.Warn("Unexpected bytecode packet")
...

Suggested fix: If immediate re-idling is intended, confirm it explicitly (it changes upstream scheduling behavior for timed-out peers). Otherwise, restrict the re-add to revert reasons where the peer is known to be free (e.g. RequestByteCodes send failure / cancellation) and leave a timed-out peer out of the idle pool until its response arrives (onByteCodes already re-idles it, even for stale ids) or it is dropped.

Suggested test: Add a syncer test that dispatches a bytecode request, fires the timeout without any response, and asserts whether the peer reappears in s.bytecodeIdlers and is immediately handed a second bytecode request while the first is still outstanding; mirror it for the send-failure revert path to pin the intended distinction.

⚪ Low: AmsterdamBlock is wired into Rules but omitted from CheckConfigForkOrder and checkCompatible

File: params/config.go:1938 ↗
Confidence: medium

Impact: This hunk makes Rules.IsAmsterdam live, derived solely from ChainConfig.AmsterdamBlock. Because amsterdamBlock appears in neither CheckConfigForkOrder's fork list nor checkCompatible: (a) a custom genesis that sets amsterdamBlock earlier than osakaBlock, or sets it while osakaBlock/pragueBlock are nil, passes fork-order validation and yields Rules{IsAmsterdam:true, IsOsaka:false} — an out-of-order fork state the ordering chokepoint exists to reject; and (b) changing or removing an already-passed amsterdamBlock across a restart produces no ConfigCompatError, so the node starts with silently different Rules instead of refusing to start and demanding a rewind.
CheckConfigForkOrder and checkCompatible are the chokepoints that stop nonsensical or silently-mutated fork schedules from reaching Rules and the EVM. Every sibling gate (Cancun, Prague, Osaka, Verkle) is registered in both; Amsterdam is registered in neither, so the new gate is the one fork that can be misordered or retroactively rescheduled without any diagnostic. Impact is bounded today: AmsterdamBlock is nil on every bundled preset, so only operator-authored custom genesis files are affected, and no consumer of Rules.IsAmsterdam was confirmed in this tree — the risk grows as soon as Amsterdam semantics are attached to the flag.

Evidence:

Changed code:

 IsOsaka: c.IsOsaka(num),
 IsAmsterdam: c.IsAmsterdam(num),
 IsEIP4762: c.IsVerkle(num),

params/config.go ↗ (Lines 1468-1470: the gate requires only London, not Osaka/Prague, so ordering sanity depends entirely on CheckConfigForkOrder — the check that is missing.):

func (c *ChainConfig) IsAmsterdam(num *big.Int) bool {
	return c.IsLondon(num) && isBlockForked(c.AmsterdamBlock, num)
}

params/config.go ↗ (CheckConfigForkOrder (1521-1614) fork slice terminates at verkleBlock; amsterdamBlock is absent, so amsterdamBlock set with osakaBlock nil, or amsterdamBlock < osakaBlock, passes validation.):

 {name: "cancunBlock", block: c.CancunBlock, optional: true},
 {name: "pragueBlock", block: c.PragueBlock, optional: true},
 {name: "osakaBlock", block: c.OsakaBlock, optional: true},
 {name: "verkleBlock", block: c.VerkleBlock, optional: true},
	} {

params/config.go ↗ (checkCompatible (1629-1721) ends here with no AmsterdamBlock clause, so rescheduling/removing an already-passed amsterdamBlock returns nil instead of a ConfigCompatError.):

	if isForkBlockIncompatible(c.OsakaBlock, newcfg.OsakaBlock, headNumber) {
 return newBlockCompatError("Osaka fork block", c.OsakaBlock, newcfg.OsakaBlock)
	}
	return nil
}

params/config.go ↗ (Line 857: the field is JSON-settable, so operator-supplied custom genesis files can reach the unvalidated path.):

	AmsterdamBlock *big.Int `json:"amsterdamBlock,omitempty"` // Amsterdam switch Block (nil = no fork, 0 = already on amsterdam)

Suggested fix: Add {name: "amsterdamBlock", block: c.AmsterdamBlock, optional: true} to the CheckConfigForkOrder fork slice, positioned consistently with the Osaka/Verkle entries, and add to checkCompatible:

if isForkBlockIncompatible(c.AmsterdamBlock, newcfg.AmsterdamBlock, headNumber) {

return newBlockCompatError("Amsterdam fork block", c.AmsterdamBlock, newcfg.AmsterdamBlock)
}

mirroring the existing Osaka handling.

Suggested test: In params/config_test.go add a CheckConfigForkOrder case with AmsterdamBlock=1 and OsakaBlock=nil (and one with AmsterdamBlock < OsakaBlock) expecting an ordering error, plus a CheckCompatible case rescheduling AmsterdamBlock past an already-forked head expecting a ConfigCompatError.

🙋 Needs Human Attention

• Does lowering testBlockChain basefee 1050→1 and blobfee 105→1 in TestAdd still exercise the pending-vs-queued/underpriced path (e.g. the gapped-nonce TxStatusQueued check and the ErrTxGasPriceTooLow sub-case), or does it now make every seeded tx trivially executable and weaken those assertions?

  • Files: core/txpool/blobpool/blobpool.go, core/txpool/blobpool/blobpool_test.go, core/txpool/blobpool/evictheap.go, core/txpool/blobpool/evictheap_test.go, core/txpool/blobpool/priority.go, core/txpool/blobpool/priority_test.go
  • Symbols: (*BlobPool).Reset, (*BlobPool).add, BlobPool.Add, BlobPool.Init, BlobPool.Status, BlobPool.add
  • Reason: The only changed lines lower the simulated market fees used by every TestAdd sub-case; sub-cases assert TxStatusQueued for gapped nonces and ErrTxGasPriceTooLow for a zero blob fee cap, whose outcome depends on chain basefee/blobfee. Confirm the fee-threshold suppression logic added in this sync is still covered rather than bypassed. Related reasons: Add no longer collects successfully added txs; announcement now depends entirely on add's isAnnouncable gate (blobpool.go:1753-1756). If the suppression threshold is not recomputed/released on head update, low-fee txs could stay permanently unannounced. Announcement ordering uses txs[i-1].announced; the recompute loop at 508 breaks on the first non-announcable tx, so nonce-ordered release semantics need confirmation. Grouped from 13 related hints across 10 packets.

• Does muxTracer.OnTxStart/OnTxEnd/OnOpcode nil-check each sub-tracer's individual hook functions before invoking them (the fileWritingTracer hooks set every hook, so this is likely fine, but the opcode counter Tracer only defines OnOpcode)?

  • Files: cmd/evm/internal/t8ntool/file_tracer.go, cmd/evm/internal/t8ntool/flags.go, cmd/evm/internal/t8ntool/transition.go, cmd/evm/main.go, eth/tracers/native/mux.go
  • Symbols: NewMuxTracer, NewOpcodeCounter, OpcodeCountFlag, Transition, muxTracer.GetResult, muxTracer.OnTxEnd
  • Reason: NewOpcodeCounter returns a Tracer whose Hooks only define OnOpcode; if muxTracer dispatches to t.tracers[i].Hooks.OnTxStart without a per-hook nil check, evm t8n --opcodecount combined with tracing would nil-panic at tx start. Related reasons: Packet reviewer could not resolve this question from the reviewed context. The new opcode-counter branch is gated on OpcodeCountFlag; if the flag is not wired into the t8n command, the added output path is dead code and the feature silently never emits counts. Grouped from 4 related hints across 3 packets.

• Is the new stateDB!=nil precompile touch path (stateDB.Exist(address) in RunPrecompiledContract) exercised by any test, or only by production EVM callers in core/vm/evm.go?

  • Files: core/vm/contracts.go, core/vm/contracts_test.go, core/vm/evm.go
  • Symbols: RunPrecompiledContract, runEcrecoverWithCache, runPrecompile
  • Reason: All precompile tests/benchmarks pass nil stateDB, so the newly added EIP-7928 BAL touch branch has no direct test coverage in this file; a verifier should confirm coverage exists elsewhere (e.g. state transition / BAL tests). Related reasons: Packet reviewer could not resolve this question from the reviewed context. Grouped from 2 related hints across 2 packets.

• Do the generated files core/types/gen_header_json.go and core/types/gen_header_rlp.go include the new Header.SlotNumber optional field, and does CopyHeader in core/types/block.go deep-copy it?

  • Files: core/types/block.go, core/types/gen_header_json.go, core/types/gen_header_rlp.go
  • Symbols: CopyHeader, DecodeRLP, EncodeRLP, Header, MarshalJSON, SlotNumber
  • Reason: Header adds SlotNumber *uint64 with rlp:"optional" and a headerMarshaling override (*hexutil.Uint64). If the rlpgen/gencodec outputs were not regenerated, JSON round-trip and header hashing would silently drop slotNumber; if CopyHeader misses it, copied headers would lose the field and hash differently. The tool budget ran out before these generated files and CopyHeader could be read.

• Does trie/proof_test.go (TestRangeProofWithInvalidNonExistentProof, TestOneElementRangeProof) still expect the old error/success behavior for cases where firstKey is greater than keys[0], now that VerifyRangeProof short-circuits with "unexpected key-value pairs preceding the requested range"?

  • Files: trie/proof.go, trie/proof_test.go
  • Symbols: TestOneElementRangeProof, TestRangeProofWithInvalidNonExistentProof, VerifyRangeProof
  • Reason: The new guard at trie/proof.go:575-577 rejects inputs that previously fell through to the two-edge-path logic (or to the "correct proof but invalid key" branch). Upstream geth changed the tests alongside this guard; this packet shows no test changes, so a test expecting the old error string/outcome could now fail or a behavior boundary could be untested.

Additional unresolved notes suppressed: 12.

Stats

• 🤖 Model: anthropic claude-opus-5 high
• 🧞 Codegenie: v0.5.5 (58f82a9b2c)
• Elapsed time: 15m 18s
• Git: 0xPolygon/bor from ppatil-upstream-v1.17.0 to ppatil-upstream-v1.17.1 (427f20c9b3)
• Planner coverage: submitted 88, accepted 87 entries / 87 unique hunks, rejected 1 unknown hunk
• Posting: 7 inline
• Review completeness: partial.
• Usage: model calls 385, tokens 8071287, cost $34.2010.
• Effective caps: tokens 8000000.
• Budget overruns: stage 9 tokens 8012749/8000000.
• Budget dispatch blocks: stage 7 tokens 6807234/8000000; stage 7 tokens 6802359/8000000; stage 7 tokens 6803386/8000000; stage 7 tokens 6804787/8000000; stage 7 tokens 6801780/8000000; stage 7 tokens 6811440/8000000; stage 7 tokens 6821373/8000000; stage 7 tokens 6826035/8000000; stage 7 tokens 6833758/8000000; stage 7 tokens 6858887/8000000; stage 9 tokens 8004104/8000000; stage 9 tokens 8001678/8000000; stage 9 tokens 8006555/8000000; stage 9 tokens 8003678/8000000; stage 9 tokens 8000767/8000000; stage 9 tokens 8003770/8000000; stage 9 tokens 8008834/8000000; stage 9 tokens 8012928/8000000; stage 9 tokens 8017868/8000000; stage 10 tokens 8071287/8000000.
• Local context pressure: 29 tool-budget rejections, 105 degraded tool results, 36 degraded hunks, 12 unresolved notes suppressed.

— View Workflow Job

[Copilot]

Pull request overview

Milestone 3/6 of the Bor ← go-ethereum v1.17.4 upstream sync, merging geth v1.17.1 changes into Bor and introducing the dormant (block-scheduled) Amsterdam fork surface (EIP-7843 SLOTNUM, EIP-8024 wiring, and precompile “touch” for BAL), plus related tooling/test updates.

Changes:

• Add Amsterdam fork scheduling to params.ChainConfig (block-based, nil-by-default) and wire VM/header surfaces for slotNumber/SLOTNUM behind IsAmsterdam.
• Add/extend trie inspection & stats utilities (LevelStats, inspect/summarize tests) and a new geth db inspect-trie command.
• Update several networking/tooling components (discv5 WHOAREYOU resend caching, request tracker scheduling, HTTP/2 toggles, metrics InfluxDB interval, new eth_getStorageValues RPC, internal tablewriter stub).

Reviewed changes

Copilot reviewed 97 out of 102 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
trie/proof.go	Range proof verification: stronger firstKey constraints and error-handling for trie updates.
trie/levelstats.go	New per-level trie node statistics collector used by inspection/stats tooling.
trie/levelstats_test.go	Tests for LevelStats depth bounds behavior.
trie/inspect_test.go	New tests covering trie Inspect/Summarize and contract inspection paths.
trie/bintrie/trie.go	Binary trie code-chunk key offset encoding change (endianness).
tests/gen_btheader.go	JSON marshal/unmarshal support for SlotNumber in block test headers.
tests/block_test_util.go	SlotNumber added to block test header structure and validation.
params/config.go	AmsterdamBlock + IsAmsterdam + Rules.IsAmsterdam wiring (dormant by default).
p2p/tracker/tracker.go	Tracker clean/schedule safety tweaks and schedule refactor.
p2p/discover/v5wire/msg.go	WHOAREYOU struct reshaping and ChallengeData semantics.
p2p/discover/v5wire/encoding.go	WHOAREYOU resend encoding via cached ChallengeData; masking refactors.
p2p/discover/v5wire/encoding_test.go	Test ensuring WHOAREYOU resend encoding is byte-identical.
p2p/discover/v5_udp_test.go	Update test codec to align with new WHOAREYOU resend caching fields.
node/rpcstack.go	Add per-server HTTP/2 disable switch for RPC stacks.
node/node.go	Disable HTTP/2 on auth/Engine API HTTP servers.
miner/worker.go	Populate SlotNumber post-Amsterdam (errors if missing when active).
miner/payload_building.go	Include slot number in payload ID; recommit timer reset uses remaining time.
metrics/config.go	Add configurable InfluxDB reporting interval to metrics config.
internal/web3ext/web3ext.go	Add web3 extension for eth_getStorageValues.
internal/tablewriter/database_tablewriter.go	Internal tablewriter stub moved to internal/tablewriter package; API tweaks.
internal/tablewriter/database_tablewriter_test.go	Update tests for internal tablewriter stub API changes.
internal/ethapi/transaction_args.go	Carry AuthorizationList through tx defaults.
internal/ethapi/api.go	Add eth_getStorageValues; include SlotNumber in RPC header marshal.
internal/ethapi/api_test.go	Tests for GetStorageValues behavior and request limits.
graphql/graphql.go	Expose SlotNumber on GraphQL Block type.
go.mod	Bump github.com/ethereum/c-kzg-4844/v2 to v2.1.6; drop olekukonko/tablewriter.
go.sum	Update sums for c-kzg bump and tablewriter removal.
eth/tracers/native/opcode_counter.go	New native tracer: counts opcode executions.
eth/tracers/native/mux.go	Refactor mux tracer construction; add exported NewMuxTracer.
eth/protocols/snap/sync.go	Restore peer idlers on request revert when peer still present.
docs/upstream-merges/v1.17.4/plan.md	Mark v1.17.1 batches as merged with commit SHAs.
docs/upstream-merges/v1.17.4/needs-wiring.md	Track deferred items introduced in this milestone.
docs/upstream-merges/v1.17.4/fork-register.md	Document Amsterdam fork surface as block-gated and dormant.
core/vm/opcodes.go	Add SLOTNUM opcode constant and string mappings.
core/vm/jump_table.go	Add Amsterdam instruction set and dispatch wiring.
core/vm/evm.go	SLOTNUM block context + Amsterdam jump table selection; syscall transfer gating; precompile-touch plumbing.
core/vm/eips.go	Add 7843 activator and implement SLOTNUM opcode.
core/vm/contracts.go	RunPrecompiledContract now optionally “touches” precompile in StateDB post-Amsterdam.
core/vm/contracts_test.go	Update precompile tests for new RunPrecompiledContract signature; include IsAmsterdam in parity checklist.
core/vm/contracts_fuzz_test.go	Update fuzz harness for new RunPrecompiledContract signature.
core/types/transaction.go	Add big.Int fallback for effective tip comparisons when uint256 calc errors.
core/types/gen_header_rlp.go	Include SlotNumber in optional RLP header encoding sequence.
core/types/gen_header_json.go	Include SlotNumber in header JSON marshal/unmarshal.
core/types/block.go	Add SlotNumber field to Header; copy/accessors.
core/types/bal/bal.go	Change construction BAL code-change representation to map[txIndex][]byte.
core/types/bal/bal_test.go	Adjust tests for new BAL code-change representation/encoding types.
core/types/bal/bal_encoding.go	Update BAL encoding to support multiple code changes; add validation/copy logic.
core/types/bal/bal_encoding_rlp_generated.go	Regenerated RLP encoding to match BAL encoding structure changes.
core/txpool/legacypool/list_test.go	Add tests for price heap comparison across basefee scenarios.
core/txpool/blobpool/priority.go	Adjust eviction priority math; add blobfee-specific jump base.
core/txpool/blobpool/priority_test.go	Update tests to match new eviction priority behavior.
core/txpool/blobpool/evictheap.go	Use blobfee-specific jump calculation; simplify priority clamp.
core/txpool/blobpool/evictheap_test.go	Update sorting tests/benchmarks for new blobfee jump behavior.
core/txpool/blobpool/blobpool_test.go	Update expected jump constants and test chain fee setup.
core/stateless/stats.go	Switch witness leaf depth collection to trie.LevelStats and log/metric reporting changes.
core/stateless/stats_test.go	Update witness stats tests; add deep-leaf panic and aggregation coverage.
core/state/state_object.go	Ensure storage reader is invoked for destructed objects to record BAL reads.
core/state_transition.go	Use tx getters directly when building Message (copies returned by getters).
core/state_processor_test.go	Ensure SlotNumber is set in generated headers when Amsterdam is active.
core/rawdb/database.go	Switch to internal tablewriter; update constructor call.
core/rawdb/accessors_chain.go	Canonical HasBody/HasReceipts behavior changed.
core/parallel_state_processor_fork_parity_test.go	Add IsAmsterdam to fork expectations (not state-processor gated).
core/genesis.go	Add SlotNumber to genesis struct and header construction post-Amsterdam.
core/gen_genesis.go	Add SlotNumber to genesis JSON marshal/unmarshal.
core/evm.go	Populate vm.BlockContext.SlotNum from header.SlotNumber.
consensus/ethash/consensus.go	Enforce SlotNumber must be nil for ethash headers; panics in SealHash if set.
consensus/clique/clique.go	Enforce SlotNumber must be nil for clique headers; panic if present in signature header.
consensus/beacon/consensus.go	Enforce SlotNumber presence/absence based on IsAmsterdam.
cmd/utils/flags.go	Add --top, --output, and --metrics.influxdb.interval flags; use interval in exporters.
cmd/geth/main.go	Wire metrics influxdb interval flag into geth command flags.
cmd/geth/dbcmd.go	Add db inspect-trie command and switch metadata table output to internal tablewriter.
cmd/geth/config.go	Apply --metrics.influxdb.interval to config.
cmd/geth/chaincmd.go	Wire metrics influxdb interval flag into chain subcommands.
cmd/evm/testdata/33/exp.json	Normalize logs field to [] instead of null in expected output.
cmd/evm/testdata/30/exp.json	Normalize logs field to [] instead of null in expected output.
cmd/evm/testdata/3/exp.json	Normalize logs field to [] instead of null in expected output.
cmd/evm/testdata/29/exp.json	Normalize logs field to [] instead of null in expected output.
cmd/evm/testdata/28/exp.json	Normalize logs field to [] instead of null in expected output.
cmd/evm/testdata/25/exp.json	Normalize logs field to [] instead of null in expected output.
cmd/evm/testdata/24/exp.json	Normalize logs field to [] instead of null in expected output.
cmd/evm/testdata/23/exp.json	Normalize logs field to [] instead of null in expected output.
cmd/evm/testdata/13/exp2.json	Normalize logs field to [] instead of null in expected output.
cmd/evm/testdata/1/exp.json	Normalize logs field to [] instead of null in expected output.
cmd/evm/main.go	Add opcode-count flag to evm t8n command flags.
cmd/evm/internal/t8ntool/transition.go	Add opcode counter tracing and optional muxing with existing tracers.
cmd/evm/internal/t8ntool/gen_stenv.go	Add SlotNumber to state test env JSON codec.
cmd/evm/internal/t8ntool/gen_header.go	Add SlotNumber to header JSON codec.
cmd/evm/internal/t8ntool/flags.go	Add --opcode.count flag.
cmd/evm/internal/t8ntool/file_tracer.go	Refactor file/result writers to return tracers instead of bare hooks.
cmd/evm/internal/t8ntool/execution.go	Add SlotNumber to env/header construction; normalize empty receipt logs to [].
cmd/evm/internal/t8ntool/block.go	Add SlotNumber to block/header JSON structures.
cmd/devp2p/internal/v5test/discv5tests.go	Update discv5 test to validate WHOAREYOU resend behavior.
build/ci.go	Remove EOL Ubuntu distros from CI images list.
beacon/engine/types.go	Add PayloadV4 and SlotNumber fields to Engine API payload types.
beacon/engine/gen_ed.go	Generated JSON codec updated for ExecutableData SlotNumber.
beacon/engine/gen_blockparams.go	Generated JSON codec updated for PayloadAttributes SlotNumber.
beacon/blsync/engineclient.go	Suppress specific ForkchoiceUpdated error during blsync reorg skipping.
.github/CODEOWNERS	Add CODEOWNERS entry for cmd/keeper.

Files not reviewed (4)

• beacon/engine/gen_blockparams.go: Generated file
• beacon/engine/gen_ed.go: Generated file
• cmd/evm/internal/t8ntool/gen_header.go: Generated file
• cmd/evm/internal/t8ntool/gen_stenv.go: Generated file

---

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

[github-actions]

🧞 Codegenie Review

Warning

Review incomplete. Some review or verification work did not complete. Treat the results below as partial and consider rerunning.

⚠️ Found 14 verified issues.

Partial review: 60 hunks were not reviewed because budget was exhausted before dispatch.
Reviewed 155/264 hunks before stopping.

Coverage disclosure:

• Budget stopped review work (token limit reached).
• Verification incomplete for 6 candidates.
• beacon/engine/gen_blockparams.go: generated file
• beacon/engine/gen_ed.go: generated file
• cmd/evm/internal/t8ntool/gen_header.go: generated file
• cmd/evm/internal/t8ntool/gen_stenv.go: generated file
• core/gen_genesis.go: generated file
• core/types/bal/bal_encoding_rlp_generated.go: generated file
• core/types/gen_header_json.go: generated file
• core/types/gen_header_rlp.go: generated file
• go.sum: lockfile
• tests/gen_btheader.go: generated file
• semantic composition skipped; deterministic fallback used

Summary-only findings:

• ⚪ Low: LevelStats.AddLeaf panics on depth >= 16 and the witness-stats caller passes unclamped path length (trie/levelstats_test.go:36)
  Impact: WitnessStats.Add calls trie.LevelStats.AddLeaf(len(path)) with an unclamped node-path length. A witness node path of 16 or more nibbles (a deep account/storage branch, reachable in principle by grinding a long hashed-key prefix collision) indexes s.level[depth] beyond the fixed [16]stat array and panics, aborting the block-processing goroutine instead of degrading the metric. The newly added test locks this panic in as the intended contract rather than requiring a clamp or drop.
  A statistics collector should saturate or drop out-of-range samples rather than panic, and encoding the panic as an asserted contract in tests makes the sharp edge durable. Impact is bounded: the only caller is reachable only when both VmConfig.StatelessSelfValidation and VmConfig.EnableWitnessStats are enabled, a debug/self-validation configuration that core/blockchain.go states should never run in production, so this is a robustness/contract-confirmation issue rather than a node-crash risk for default operators.

  Evidence:

  Changed code:

  func TestLevelStatsAddLeafPanicsOnDepth16(t *testing.T) {
  	defer func() {
   if r := recover(); r == nil {
   t.Fatal("expected panic for depth >= 16")
   }
  	}()
  	NewLevelStats().AddLeaf(16)
  }

  trie/levelstats.go ↗ (Confirmed by exact read of lines 18-95: AddLeaf indexes a fixed [16]stat array with no bounds guard, so depth >= 16 is an unrecovered index-out-of-range panic. The type comment notes tries may reach 64 levels.):

  const trieStatLevels = 16
  type LevelStats struct {
  	level [trieStatLevels]stat
  }
  // AddLeaf records a leaf depth. Witness collection reuses the value-node bucket
  // for leaf accounting. It panics if the depth is outside [0, 15].
  func (s *LevelStats) AddLeaf(depth int) {
  	s.level[depth].value.Add(1)
  }

  core/stateless/stats.go ↗ (The only production caller passes the raw nibble-path length with no clamp, so any witness node path of 16+ nibbles reaches the unguarded index.):

  	for i, path := range paths {
   // If current path is a prefix of the next path, it's not a leaf.
   // The last path is always a leaf.
   if i == len(paths)-1 || !strings.HasPrefix(paths[i+1], paths[i]) {
   ownerStat.AddLeaf(len(path))
   }
  	}

  core/blockchain.go ↗ (Bounds the blast radius: the collector is only constructed under the doubly-gated StatelessSelfValidation + EnableWitnessStats debug configuration, so the panic cannot be triggered on a default production node.):

  // Self validation should *never* run in production, it's more of
  // a tight integration to enable running *all* consensus tests through the
  // witness builder/runner ...
  if witness = statedb.Witness(); witness != nil && bc.cfg.VmConfig.StatelessSelfValidation {
  	...
  	if bc.cfg.VmConfig.EnableWitnessStats {
   witnessStats = stateless.NewWitnessStats()
  	}

  Suggested fix: Either make AddLeaf tolerant (if depth < 0 || depth >= trieStatLevels { return }, or clamp into the last bucket) and update both trie/levelstats_test.go and core/stateless/stats_test.go to assert the safe behavior, or explicitly confirm that panicking on depth >= 16 is the intended contract given the caller is debug-gated.

  Suggested test: If the clamp is adopted, replace TestLevelStatsAddLeafPanicsOnDepth16 with a test asserting AddLeaf(16) is a no-op (or increments the depth-15 bucket) and update core/stateless TestWitnessStatsPanicsOnDeepLeaf to assert WitnessStats.Add tolerates a 20-nibble node path.

• ⚪ Low: inspect-trie: nil header dereference when canonical hash exists but header record is missing (cmd/geth/dbcmd.go:117)
  Impact: Running geth db inspect-trie <blocknum> (or latest) against a datadir where rawdb.ReadCanonicalHash returns a non-zero hash but the corresponding header record is missing or its RLP is corrupt makes rawdb.ReadHeader return nil; the very next statement trieRoot = blockHeader.Root dereferences that nil pointer and the CLI panics with a stack trace instead of returning the intended "header not found" error.
  This is a new operator-facing debug subcommand whose stated purpose is inspecting state/trie data on databases that may be partially synced, truncated, or damaged — exactly the conditions that produce a canonical-hash entry without a readable header. A nil-pointer panic gives no actionable diagnostic and reads like a crash bug, while the surrounding code already demonstrates the intended error-return style for the adjacent canonical-hash lookup.

  Evidence:

  Changed code:

  if number != math.MaxUint64 {
  	hash = rawdb.ReadCanonicalHash(db, number)
  	if hash == (common.Hash{}) {
   return fmt.Errorf("canonical hash for block %d not found", number)
  	}
  	blockHeader := rawdb.ReadHeader(db, hash, number)
  	trieRoot = blockHeader.Root
  }

  core/rawdb/accessors_chain.go ↗ (Decisive helper branch: ReadHeader returns nil both when the header RLP is absent and when it fails to decode, so blockHeader.Root at dbcmd.go:500 can dereference nil.):

  func ReadHeader(db ethdb.Reader, hash common.Hash, number uint64) *types.Header {
  	data := ReadHeaderRLP(db, hash, number)
  	if len(data) == 0 {
   return nil
  	}
  	header := new(types.Header)
  	if err := rlp.DecodeBytes(data, header); err != nil {
   log.Error("Invalid block header RLP", "hash", hash, "err", err)
   return nil
  	}
  	return header
  }

  Suggested fix: blockHeader := rawdb.ReadHeader(db, hash, number)
  if blockHeader == nil {
  return fmt.Errorf("header for block %d (%#x) not found", number, hash)
  }
  trieRoot = blockHeader.Root

  Suggested test: Add a cmd/geth or rawdb-backed unit test that writes only the canonical hash mapping for a block number (no header record) and asserts inspectTrie returns an error containing "header ... not found" rather than panicking.

• ⚪ Low: TestHandshakeResend drops PONG-after-resend coverage; resp.Node assignment is dead (cmd/devp2p/internal/v5test/discv5tests.go:200)
  Impact: In head, resp.Node = conn.remote is the last statement of the case and the function returns, so the mutated resp is never used. The test now only asserts that the second WHOAREYOU repeats the first challenge's Nonce and ChallengeData. Base asserted, via conn.reqresp, that the second PING could actually complete the handshake and yield a valid PONG. A regression in which the resent ChallengeData is echoed correctly but is stale/unusable for deriving session keys (exactly the area touched by switching from Whoareyou.Encoded to ChallengeData) would pass this test.
  This is the dedicated conformance test for handshake challenge resend in the devp2p test suite, run against third-party discv5 implementations. Losing the 'answer the resent challenge and get a PONG' assertion narrows the test to a byte-equality check on the challenge and leaves the resend path's usability untested, while the leftover dead assignment signals the follow-up write/PONG check was dropped unintentionally.

  Evidence:

  Changed code:

  	conn.write(l1, ping2, nil)
  	switch resp := conn.read(l1).(type) {
  	case *v5wire.Whoareyou:
   if resp.Nonce != challenge1.Nonce { ... }
   if !bytes.Equal(resp.ChallengeData, challenge1.ChallengeData) { ... }
   resp.Node = conn.remote
  	default:
   t.Fatal("expected WHOAREYOU, got", resp)
  	}
  }

  cmd/devp2p/internal/v5test/discv5tests.go ↗ (Base version (TestPingHandshakeInterrupted) completed the handshake via conn.reqresp and asserted a valid PONG for ping2; head no longer performs this step, removing the covered boundary.):

  	// Send second PING.
  	ping2 := &v5wire.Ping{ReqID: conn.nextReqID()}
  	switch resp := conn.reqresp(l1, ping2).(type) {
  	case *v5wire.Pong:
   checkPong(t, resp, ping2, l1)

  cmd/devp2p/internal/v5test/discv5tests.go ↗ (In TestPingMultiIP the same resp.Node = ... assignment is immediately followed by writing the handshake packet and checking the PONG, showing the assignment is only meaningful when the challenge is answered.):

   resp.Node = s.Dest
   conn.write(l2, ping2, resp)
  	...
  	case *v5wire.Pong:
   checkPong(t, resp, ping2, l2)

  Suggested fix: Complete the test as the leftover assignment implies:
  resp.Node = conn.remote
  conn.write(l1, ping2, resp)
  }
  // Catch the PONG for ping2.
  switch resp := conn.read(l1).(type) {
  case *v5wire.Pong:
  checkPong(t, resp, ping2, l1)
  default:
  t.Fatal("expected PONG, got", resp)
  }
  Alternatively, if only the challenge-repeat property is intended, remove the dead resp.Node = conn.remote line and note in the comment that handshake completion is intentionally out of scope.

  Suggested test: Extend TestHandshakeResend to answer the resent WHOAREYOU with the handshake packet for ping2 and assert a valid PONG via checkPong, covering that the resent ChallengeData is usable for session establishment.

• ⚪ Low: Unchecked nil header dereference in geth db inspect-trie (cmd/geth/dbcmd.go:500)
  Impact: rawdb.ReadHeader returns a nil *types.Header when no header is stored for (hash, number) or when the stored header RLP fails to decode. The following blockHeader.Root then dereferences nil and panics, so geth db inspect-trie <blocknum> crashes with a stack trace on an inconsistent or corrupted datadir (canonical-hash marker present without a readable header) instead of returning the clean CLI error used one line earlier for a missing canonical hash.
  This is an offline diagnostic command intended to be run against damaged or unusual databases; the exact situation it is used to investigate is the one where it panics instead of reporting the problem. The guard is three lines and matches the surrounding error-handling style.

  Evidence:

  Changed code:

   blockHeader := rawdb.ReadHeader(db, hash, number)
   trieRoot = blockHeader.Root

  core/rawdb/accessors_chain.go ↗ (Decisive callee branches: ReadHeader returns nil both when no header data is stored and when the stored header RLP fails to decode.):

  func ReadHeader(db ethdb.Reader, hash common.Hash, number uint64) *types.Header {
  	data := ReadHeaderRLP(db, hash, number)
  	if len(data) == 0 {
   return nil
  	}
  	header := new(types.Header)
  	if err := rlp.DecodeBytes(data, header); err != nil {
   log.Error("Invalid block header RLP", "hash", hash, "err", err)
   return nil
  	}
  	return header
  }

  cmd/geth/dbcmd.go ↗ (The immediately preceding lookup establishes the error-return contract for missing data; the header lookup skips the equivalent check.):

   hash = rawdb.ReadCanonicalHash(db, number)
   if hash == (common.Hash{}) {
   return fmt.Errorf("canonical hash for block %d not found", number)
   }

  Suggested fix: blockHeader := rawdb.ReadHeader(db, hash, number)
  if blockHeader == nil {
  return fmt.Errorf("header for block %d (%s) not found", number, hash)
  }
  trieRoot = blockHeader.Root

  Suggested test: Unit/CLI test: seed a test chaindb with WriteCanonicalHash for block N but no header (or a corrupt header RLP), invoke inspectTrie with argument N, and assert it returns a non-nil error rather than panicking.

• ⚪ Low: Re-idling the peer in revert*Request allows a tight assign/send-fail/revert loop on write errors (eth/protocols/snap/sync.go:1832)
  Impact: When a peer's request write fails synchronously (broken/closed connection that has not yet triggered Unregister), the request goroutine calls scheduleRevertAccountRequest immediately. With the new re-idle, revertAccountRequest puts the peer back into s.accountIdlers while it is still present in s.peers, and the next assignAccountTasks pass hands it the same task again, which fails again instantly. This spins assign -> send error -> revert -> re-idle (one goroutine plus one timer plus debug log lines per iteration) until the peer is unregistered. Before the change the peer stayed out of the idle pool, so a failing peer was used at most once per task.
  During the window between a peer's write failure and its unregistration, the snap sync event loop can churn through many pointless assign/revert cycles, burning CPU, spamming debug logs and delaying assignment of that task to healthy peers. The timeout path is paced by rates.TargetTimeout() and is far less severe, but it likewise lets a black-hole peer keep re-acquiring tasks.

  Evidence:

  Changed code:

  	// Remove the request from the tracked set and restore the peer to the
  	// idle pool so it can be reassigned work (skip if peer already left).
  	s.lock.Lock()
  	delete(s.accountReqs, req.id)
  	if _, ok := s.peers[req.peer]; ok {
   s.accountIdlers[req.peer] = struct{}{}
  	}
  	s.lock.Unlock()

  eth/protocols/snap/sync.go ↗ (assignAccountTasks (1056-1162) reverts immediately, with no delay/backoff, when the write to the peer fails; each attempt allocates a goroutine and a timer.):

  req.timeout = time.AfterFunc(s.rates.TargetTimeout(), func() { ... s.scheduleRevertAccountRequest(req) })
  s.accountReqs[reqid] = req
  delete(s.accountIdlers, idle)
  s.pend.Add(1)
  go func(root common.Hash) {
  	defer s.pend.Done()
  	...
  	if err := peer.RequestAccountRange(reqid, root, req.origin, req.limit, uint64(cap)); err != nil {
   peer.Log().Debug("Failed to request account range", "err", err)
   s.scheduleRevertAccountRequest(req)
  	}
  }(s.root)

  eth/protocols/snap/sync.go ↗ (Event loop (~758-790) re-runs assignment on every iteration, so a peer re-idled by the revert is reassigned the same task immediately.):

  case req := <-accountReqFails:
  	s.revertAccountRequest(req)
  // loop top re-runs: s.assignAccountTasks(accountResps, accountReqFails, cancel)

  eth/protocols/snap/sync.go ↗ (The only filter on idle peers is statelessPeers; a peer whose write failed is not marked stateless, so nothing prevents immediate reselection.):

  for id := range s.accountIdlers {
  	if _, ok := s.statelessPeers[id]; ok {
   continue
  	}
  	idlers.ids = append(idlers.ids, id)
  	...
  }

  eth/protocols/snap/peer.go ↗ (The send error is a p2p write error on a broken connection, i.e. persistent rather than transient, so the retry fails again instantly.):

  func (p *Peer) RequestAccountRange(...) error {
  	...
  	return p2p.Send(p.rw, GetAccountRangeMsg, &GetAccountRangePacket{...})
  }

  Suggested fix: Do not re-idle on the synchronous send-failure path, or add a marker/backoff so a peer whose request send failed is not reselected immediately (e.g. mark it stateless/skip until a successful delivery, or signal via s.update instead of re-idling unconditionally). Restrict the re-idle to the timeout path the commit body describes.

  Suggested test: In eth/protocols/snap/sync_test.go, register a peer whose RequestAccountRange always returns an error and assert that the number of request attempts made to that peer stays bounded (e.g. a small constant) while sync progresses via other peers.

• ⚪ Low: revertBytecodeRequest re-idles a peer whose response is still outstanding after a timeout (eth/protocols/snap/sync.go:1878)
  Impact: On the timeout revert path the peer is still present in s.peers and still owes a response, so the new unconditional re-add puts it back into s.bytecodeIdlers immediately. assignBytecodeTasks can then dispatch a second bytecode request to a peer that already failed to answer within TargetTimeout, leaving two outstanding requests to one stalled peer and allowing repeated re-issuance to the same slow peer instead of preferring healthy idlers. The guard if _, ok := s.peers[req.peer] only filters the disconnect case (Unregister deletes from s.peers before peerDrop triggers revertRequests).
  Snap sync scheduling quality depends on not immediately handing new work to peers that just timed out. The re-idle creates a timeout/reassign churn loop against the same slow peer and doubles its outstanding request count, which can slow state sync progress on peers with degraded links.

  Evidence:

  Changed code:

  	// Remove the request from the tracked set and restore the peer to the
  	// idle pool so it can be reassigned work (skip if peer already left).
  	s.lock.Lock()
  	delete(s.bytecodeReqs, req.id)
  	if _, ok := s.peers[req.peer]; ok {
   s.bytecodeIdlers[req.peer] = struct{}{}
  	}
  	s.lock.Unlock()

  eth/protocols/snap/sync.go ↗ (assignBytecodeTasks (1165-1278) removes the peer from bytecodeIdlers on dispatch and reverts on timeout while the peer is still registered in s.peers, so the new guard does not filter the timeout case; the peer becomes assignable again with a response still outstanding. It also decays the peer's msgrate capacity, which partially mitigates re-preferring the stalled peer.):

  req.timeout = time.AfterFunc(s.rates.TargetTimeout(), func() {
  	peer.Log().Debug("Bytecode request timed out", "reqid", reqid)
  	s.rates.Update(idle, ByteCodesMsg, 0, 0)
  	s.scheduleRevertBytecodeRequest(req)
  })
  s.bytecodeReqs[reqid] = req
  delete(s.bytecodeIdlers, idle)

  eth/protocols/snap/sync.go ↗ (onByteCodes (2827-2932) is the pre-existing idle-restore chokepoint and restores the peer even for a stale/late delivery, so the peer was not permanently excluded before this change; it also discards the stale delivery, bounding the impact of the duplicate outstanding request to scheduling churn rather than data corruption.):

  defer func() {
  	s.lock.Lock()
  	defer s.lock.Unlock()
  	if _, ok := s.peers[peer.ID()]; ok {
   s.bytecodeIdlers[peer.ID()] = struct{}{}
  	}
  	...
  }()
  ...
  req, ok := s.bytecodeReqs[id]
  if !ok {
  	logger.Warn("Unexpected bytecode packet")
  ...

  Suggested fix: If immediate re-idling is intended, confirm it explicitly (it changes upstream scheduling behavior for timed-out peers). Otherwise, restrict the re-add to revert reasons where the peer is known to be free (e.g. RequestByteCodes send failure / cancellation) and leave a timed-out peer out of the idle pool until its response arrives (onByteCodes already re-idles it, even for stale ids) or it is dropped.

  Suggested test: Add a syncer test that dispatches a bytecode request, fires the timeout without any response, and asserts whether the peer reappears in s.bytecodeIdlers and is immediately handed a second bytecode request while the first is still outstanding; mirror it for the send-failure revert path to pin the intended distinction.

• ⚪ Low: AmsterdamBlock is wired into Rules but omitted from CheckConfigForkOrder and checkCompatible (params/config.go:1938)
  Impact: This hunk makes Rules.IsAmsterdam live, derived solely from ChainConfig.AmsterdamBlock. Because amsterdamBlock appears in neither CheckConfigForkOrder's fork list nor checkCompatible: (a) a custom genesis that sets amsterdamBlock earlier than osakaBlock, or sets it while osakaBlock/pragueBlock are nil, passes fork-order validation and yields Rules{IsAmsterdam:true, IsOsaka:false} — an out-of-order fork state the ordering chokepoint exists to reject; and (b) changing or removing an already-passed amsterdamBlock across a restart produces no ConfigCompatError, so the node starts with silently different Rules instead of refusing to start and demanding a rewind.
  CheckConfigForkOrder and checkCompatible are the chokepoints that stop nonsensical or silently-mutated fork schedules from reaching Rules and the EVM. Every sibling gate (Cancun, Prague, Osaka, Verkle) is registered in both; Amsterdam is registered in neither, so the new gate is the one fork that can be misordered or retroactively rescheduled without any diagnostic. Impact is bounded today: AmsterdamBlock is nil on every bundled preset, so only operator-authored custom genesis files are affected, and no consumer of Rules.IsAmsterdam was confirmed in this tree — the risk grows as soon as Amsterdam semantics are attached to the flag.

  Evidence:

  Changed code:

   IsOsaka: c.IsOsaka(num),
   IsAmsterdam: c.IsAmsterdam(num),
   IsEIP4762: c.IsVerkle(num),

  params/config.go ↗ (Lines 1468-1470: the gate requires only London, not Osaka/Prague, so ordering sanity depends entirely on CheckConfigForkOrder — the check that is missing.):

  func (c *ChainConfig) IsAmsterdam(num *big.Int) bool {
  	return c.IsLondon(num) && isBlockForked(c.AmsterdamBlock, num)
  }

  params/config.go ↗ (CheckConfigForkOrder (1521-1614) fork slice terminates at verkleBlock; amsterdamBlock is absent, so amsterdamBlock set with osakaBlock nil, or amsterdamBlock < osakaBlock, passes validation.):

   {name: "cancunBlock", block: c.CancunBlock, optional: true},
   {name: "pragueBlock", block: c.PragueBlock, optional: true},
   {name: "osakaBlock", block: c.OsakaBlock, optional: true},
   {name: "verkleBlock", block: c.VerkleBlock, optional: true},
  	} {

  params/config.go ↗ (checkCompatible (1629-1721) ends here with no AmsterdamBlock clause, so rescheduling/removing an already-passed amsterdamBlock returns nil instead of a ConfigCompatError.):

  	if isForkBlockIncompatible(c.OsakaBlock, newcfg.OsakaBlock, headNumber) {
   return newBlockCompatError("Osaka fork block", c.OsakaBlock, newcfg.OsakaBlock)
  	}
  	return nil
  }

  params/config.go ↗ (Line 857: the field is JSON-settable, so operator-supplied custom genesis files can reach the unvalidated path.):

  	AmsterdamBlock *big.Int `json:"amsterdamBlock,omitempty"` // Amsterdam switch Block (nil = no fork, 0 = already on amsterdam)

  Suggested fix: Add {name: "amsterdamBlock", block: c.AmsterdamBlock, optional: true} to the CheckConfigForkOrder fork slice, positioned consistently with the Osaka/Verkle entries, and add to checkCompatible:

  if isForkBlockIncompatible(c.AmsterdamBlock, newcfg.AmsterdamBlock, headNumber) {
  return newBlockCompatError("Amsterdam fork block", c.AmsterdamBlock, newcfg.AmsterdamBlock)
  }

  mirroring the existing Osaka handling.

  Suggested test: In params/config_test.go add a CheckConfigForkOrder case with AmsterdamBlock=1 and OsakaBlock=nil (and one with AmsterdamBlock < OsakaBlock) expecting an ordering error, plus a CheckCompatible case rescheduling AmsterdamBlock past an already-forked head expecting a ConfigCompatError.

🙋 Needs human attention:

• Does lowering testBlockChain basefee 1050→1 and blobfee 105→1 in TestAdd still exercise the pending-vs-queued/underpriced path (e.g. the gapped-nonce TxStatusQueued check and the ErrTxGasPriceTooLow sub-case), or does it now make every seeded tx trivially executable and weaken those assertions?
• Does muxTracer.OnTxStart/OnTxEnd/OnOpcode nil-check each sub-tracer's individual hook functions before invoking them (the fileWritingTracer hooks set every hook, so this is likely fine, but the opcode counter Tracer only defines OnOpcode)?
• Is the new stateDB!=nil precompile touch path (stateDB.Exist(address) in RunPrecompiledContract) exercised by any test, or only by production EVM callers in core/vm/evm.go?
• Do the generated files core/types/gen_header_json.go and core/types/gen_header_rlp.go include the new Header.SlotNumber optional field, and does CopyHeader in core/types/block.go deep-copy it?
• Does trie/proof_test.go (TestRangeProofWithInvalidNonExistentProof, TestOneElementRangeProof) still expect the old error/success behavior for cases where firstKey is greater than keys[0], now that VerifyRangeProof short-circuits with "unexpected key-value pairs preceding the requested range"?
• Additional unresolved notes suppressed: 12

Sorry, this review is incomplete. The allotted max token limit of 8000000 (config review.maxBudgetTokens) was reached and the review has been degraded. Raise the limit for a complete review.

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

[claude]

Beyond the inline fork-order/compat finding, I also checked whether Amsterdam needs its own case in activePrecompiledContracts/ActivePrecompiles (core/vm/contracts.go) — it doesn't add a distinct precompile set versus Osaka (only SLOTNUM + BAL touch instrumentation), so falling through to the IsOsaka case is correct as long as Osaka activates at or before Amsterdam, which is the same ordering guarantee the inline finding shows is currently unenforced.

Extended reasoning...

Verified by reading core/vm/contracts.go: neither activePrecompiledContracts nor ActivePrecompiles has an IsAmsterdam case, and both fall through to IsOsaka. This is fine on its own since Amsterdam (EIP-7843 SLOTNUM, EIP-8024, BAL precompile-touch) doesn't introduce a new precompile, but it means correctness here is coupled to Osaka activating no later than Amsterdam in config — the exact invariant the inline CheckConfigForkOrder/checkCompatible gap leaves unchecked. Not a new/independent bug, so not filed separately, but worth recording as examined.

[lucca30]

quick question on this one — the touch we added for self destructed accounts (s.db.reader.Storage(...) in core/state/state_object.go) runs unconditionally, no IsAmsterdam check. compare that to the precompile touch in core/vm/evm.go which is gated behind evm.chainRules.IsAmsterdam and stays a no-op til the fork is scheduled.

looks like this bit came straight from upstream (the TODO comment about extending the reader with Touch is rjl's own), so maybe they just don't care about gating it on their side. but since amsterdam is dormant on every bor network right now, this means we're doing an extra disk read every time on self destructed account storage access for basically no reason yet.

im wondering if we should keep with this check since its amsterdam, or if we should remove/gate it until the fork actually gets scheduled, just to be consistent with how we gated the other touch and the "no behavior change" claim in the description.

[pratikspatil024]

Good catch — and I think the reason to gate it is stronger than the one you landed on.

Everything you describe checks out: the read is ungated, the precompile touch in core/vm/evm.go is gated, and the hunk is byte-identical to upstream v1.17.1 (core/state/state_object.go:197-211), rjl's TODO and all.

The wasted disk read is real but small — post-EIP-6780 stateObjectsDestruct only fills on same-transaction create-and-destruct, so it's rare rather than every time. What actually matters is that this read lands in the witness. NewTrieOnly exists specifically to force reads through the MPT "because the witness captures trie nodes during the walk" (statedb.go:214-217), and that's the reader the producer path uses.

That gives the asymmetry we already know is dangerous:

• new producer → old consumer: witness is a superset, fine
• old producer → new consumer: the consumer reads nodes the producer never recorded → reader errors → setError → dbErr → commit aborts at statedb.go:2177 → block import fails

And it's cheap to reach: create-and-selfdestruct in one tx, then read that address's storage in a later tx of the same block. With most mainnet validators running stateless and gating milestone voting, a mixed-version window on that is exactly what we built Hampi for.

There's a policy angle too — fork-register.md defines verified-dormant as the gate being nil/unset and the behaviour being gate-guarded, "not ungated / unconditional". So an ungated Amsterdam surface fails invariant 9 on its own terms. EIP-7928 also had no register row at all, which was its own gap.

Fixed in 4570596: StateDB gains an amsterdam flag set from rules.IsAmsterdam in Prepare (which every message execution goes through) and carried through Copy, with the read behind it. TestDestructedSlotReadIsAmsterdamGated asserts no reader access pre-fork, exactly one post-fork, and that Copy preserves the flag — it fails on the first subtest with the gate removed, so it can't pass vacuously. Added the missing EIP-7928 row to fork-register.md and a correction note in the ledger.

Deliberate divergence from upstream, which doesn't gate it — recorded as such so a future sync doesn't quietly drop it.

One caveat I put in the commit message and the ledger rather than hiding: Prepare is per-transaction, so a path reaching GetCommittedState without preparing a tx — system calls, state-sync in Finalize, the pooled StateDBs behind SafeBase — leaves the flag false and skips the read. That's correct while Amsterdam is dormant, but it has to be revisited when the fork is actually scheduled or BAL will under-record on those paths.

Separately, your question is the second time we've reasoned our way through witness skew on a whiteboard with no test that could have caught it (PIP-88/Hampi was the first). We're going to look at wiring old-producer/new-consumer coverage into the kurtosis/e2e setup so this stops being a review-time catch.

[lucca30]

Verified the fix for the destructed-account read gating (4570596, 343241f, 6b4820b): built core/state and ran TestDestructedSlotReadIsAmsterdamGated + TestDestructedReadWitnessSkew locally against this branch, both pass and confirm the witness-skew claim (1 node gated-off vs 5 gated-on). Good catch on the producer/consumer skew, and the boundary test + measured witness test go beyond what was asked.

No other blockers from my side. Approving.