Skip to content

core, eth, trie: merge geth v1.17.0 (v1.17.4 sync, milestone 2/6) - #2319

Open
pratikspatil024 wants to merge 247 commits into
upstream-merge-v1.17.4from
ppatil-upstream-v1.17.0
Open

core, eth, trie: merge geth v1.17.0 (v1.17.4 sync, milestone 2/6)#2319
pratikspatil024 wants to merge 247 commits into
upstream-merge-v1.17.4from
ppatil-upstream-v1.17.0

Conversation

@pratikspatil024

@pratikspatil024 pratikspatil024 commented Jul 24, 2026

Copy link
Copy Markdown
Member

Important

Reviewer guide — stacked PR 2 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 2/6 of the Bor ← go-ethereum v1.17.4 upstream sync: merges
go-ethereum v1.17.0 (upstream range v1.16.9-tip → v1.17.0 stable,
0cf3d3ba4) into Bor across 12 review-sized merge batches (~20 upstream
first-parent commits each) plus one standalone go-verkle-removal commit and a
milestone-chores commit.

Stacked on #2308 (v1.16.9, milestone 1/6). Base is ppatil-upstream-v1.16.9
for now; it retargets to upstream-merge-v1.17.4 when #2308 merges.

⚠️ Merge, never squash. Every batch is a real 2-parent merge commit so the
merge base advances and upstream history/authorship is preserved. Squash-merging
this PR (or any PR below it in the stack) rewrites SHAs and breaks every stacked
PR above it. Use a merge commit.

What landed

Most of v1.17.0 is bugfix/tooling/test sweeps that adopted cleanly. The
consensus-relevant decisions, per-batch, are in
docs/upstream-merges/v1.17.4/ledger.md; fork decisions in fork-register.md;
the deferred backlog (with adopt-steps) in needs-wiring.md.

  • No fork/EIP activated. Every v1.17.0 fork/EIP surface is merged dormant
    on Bor networks (EIP-8024 ExtraEips-only; Verkle gate nil). No precompile-set,
    gas-schedule, or activation-block change on any Bor preset. params.Rules.ChainID
    retained (declined upstream removal) to satisfy Bor's precompile-parity guard.
  • Adopted (consensus-relevant): crypto/keccak vendoring (#33323); eip4844
    latestBlobConfig(BlobConfig,error) keeping Bor's block-based gating + no-config
    return 0; opKeccak256 Keccak256Cache adapted to upstream's hasher; trienode-history
    (#32621/#33551/#33584) + NodeFullValueCheckpoint (#32727), both dormant by default;
    eth_getProofs-for-history; callTracer log index (goldens regenerated preserving Bor's
    0x1010 fee logs); pathdb history-index mutex combined with upstream's iterator
    refactor; trie decode CountValues error check; freezer fd-leak fix.
  • go-verkle scaffolding removed (mirrors upstream #33461; kept the dormant Verkle
    gate + binary-tree + stateless.Witness).
  • Deferred / declined (reverted to Bor HEAD, tracked in needs-wiring.md with
    adopt-steps — nothing dropped silently): the state code-read metrics line, core/vm
    write-protection + selfdestruct cluster, OpenTelemetry line (#33452/#33484/#33521/#33780;
    Bor has its own server-side OTel), delayed-p2p-decoding (#33835), EraE format (#32157),
    syncModer, slow-block stats, tx-announcement validation, snap-sync locking, legacypool
    alloc, --miner.maxblobs. These clash with Bor divergences (BlockSTM state, SafePool,
    ExcludeStateSyncReceipt, TD-in-DB, forked downloader, per-fork signer chain).
  • Milestone chores: regenerated eth/ethconfig/gen_config.go (TrienodeHistory +
    NodeFullValueCheckpoint + NoSnapServing TOML); exempted DumpBinTrieLeaves in the
    StateDB↔ParallelStateDB parity guard.

Executed tests

Beyond the standard CI gates:

  • go build ./... — clean. go vet ./... — clean except two pre-existing
    //nolint:govet copylocks (identical to develop). go mod tidy — no drift
    (this milestone adds no dependencies).
  • Full go test ./... — green after fixing the only two merge artifacts it
    surfaced (ethclient rpc.NewServer signature; internal/ethapi blockTimestamp
    goldens). Remaining failures are pre-existing and byte-identical to develop
    (VEBLOP miner nil-deref on non-Bor-network test harnesses in cmd/geth/cmd/devp2p/ethtest;
    t8n golden drift; a core/vm interrupt-race flake).
  • make test-integration (-tags integration ./tests/...) — pass, zero failures,
    including the tests/bor consensus e2e suite (621s).
  • govulncheck ./... — no merge-introduced vulnerabilities (3 called vulns are all
    pre-existing/develop-inherent dependency/toolchain versions).

Not run in-session (operator/CI release gates): diffguard mutation, kurtosis devnet,
and the devnet→testnet soak ladder. Kurtosis marginal value is low here — tests/bor
(consensus e2e) is green and the only observed panics are non-Bor-network paths.

Rollout notes

  • Not consensus-affecting on Bor networks. No fork activated, no precompile/gas/
    activation-height change; all adopted features either dormant-by-default or
    non-consensus (tooling/RPC/metrics/tests). Deferred upstream features were reverted
    to Bor HEAD.
  • Backward-compatible; no coordinated upgrade required for this milestone.
  • Operator-visible additions: blockTimestamp field on eth_getTransactionBy* RPC
    responses (additive); download progress-bar behavior; HTTP/2 JSON-RPC.
  • Follow-ups for the team (pre-existing, outside this sync): nil-guard the VEBLOP miner
    timeout (miner/worker.go:774) so non-Bor-network tests stop panicking; dependency
    bumps (x/text→v0.39.0, cascade develop's x/crypto v0.52.0, Go→1.26.5).

🤖 Generated with Claude Code

fjl and others added 30 commits November 4, 2025 13:28
Because the map iteration is unstable, we need to order logs by tx index
and keep the same order with receipts and their logs, so we can still
get the same `LogsHash` across runs.

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: rjl493456442 <garyrong0905@gmail.com>
The version check incorrectly used `&&` instead of `||`, causing
versions like v1.0.x through v1.4.x to be allowed when they should be
rejected. These versions don't support EIP-712 signing which was
introduced in firmware v1.5.0.
Removes an unnecessary todo. This case is handled, the comment was an
artifact from Kev's refactor
…r (#33098)

failed in 32bit:

```
--- FAIL: TestDecodeSingleCorruptedData (0.00s)
panic: runtime error: slice bounds out of range [:-1501805520] [recovered, repanicked]

goroutine 38872 [running]:
testing.tRunner.func1.2({0x838db20, 0xa355620})
	/opt/actions-runner/_work/_tool/go/1.25.3/x64/src/testing/testing.go:1872 +0x29b
testing.tRunner.func1()
	/opt/actions-runner/_work/_tool/go/1.25.3/x64/src/testing/testing.go:1875 +0x414
panic({0x838db20, 0xa355620})
	/opt/actions-runner/_work/_tool/go/1.25.3/x64/src/runtime/panic.go:783 +0x103
github.com/ethereum/go-ethereum/triedb/pathdb.decodeSingle({0x9e57500, 0x1432, 0x1432}, 0x0)
	/opt/actions-runner/_work/go-ethereum/go-ethereum/triedb/pathdb/history_trienode.go:399 +0x18d6
github.com/ethereum/go-ethereum/triedb/pathdb.TestDecodeSingleCorruptedData(0xa2db9e8)
	/opt/actions-runner/_work/go-ethereum/go-ethereum/triedb/pathdb/history_trienode_test.go:698 +0x180
testing.tRunner(0xa2db9e8, 0x83c86e8)
	/opt/actions-runner/_work/_tool/go/1.25.3/x64/src/testing/testing.go:1934 +0x114
created by testing.(*T).Run in goroutine 1
	/opt/actions-runner/_work/_tool/go/1.25.3/x64/src/testing/testing.go:1997 +0x4b4
FAIL	github.com/ethereum/go-ethereum/triedb/pathdb	41.453s
?   	github.com/ethereum/go-ethereum/version	[no test files]
FAIL
```

Found in
https://github.com/ethereum/go-ethereum/actions/runs/18912701345/job/53990136071?pr=33052
- Replace hardcoded DigestLength 
- Add hash length validation
Rollups may want to use these to dynamically adjust blobs posted after
BPO forks.
The periodic sealing loop failed to reset its timer when sealBlock
returned an error, causing the timer to never fire again and effectively
halting block production in developer periodic mode after the first
failure. This is a bug because the loop relies on the timer to trigger
subsequent sealing attempts, and transient errors (e.g., pool races or
chain rewinds) should not permanently stop the loop. The change moves
timer.Reset after the sealing attempt unconditionally, ensuring the loop
continues ticking and retrying even when sealing fails, which matches
how other periodic timers in the codebase behave and preserves forward
progress.
[powdr](github.com/powdr-labs/powdr) has tested keeper in their womir
system and managed to get it to work. This PR adds wasm as a keeper
target. There's another plan by the zkevm team to support wasm with wasi
as well, so these PR adds both targets.

These currently uses the `example` tag, as there is no precompile
intefrace defined for either target yet. Nonetheless, this is useful for
testing these zkvms so it makes sense to support these experimental
targets already.
While updating to latest Geth, I noticed `OnCodeChangeV2` was not
properly handled in `SelfDestruct/6780`, this PR fixes this and bring a
unit test. Let me know if it's deemed more approriate to merge the tests
with the other one.
Context from Cloudflare blog:
https://blog.cloudflare.com/go-and-enhance-your-calm/#reading-bodies-in-go-can-be-unintuitive

We were able to reproduce the same issue discussed by Cloudflare in
their recent blog post above using the `ethclient`.
Add BlobTxs flag to filter.

Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Add nil check before calling tx.Type() to prevent panic when transaction
is not found.
The iterator loop in findTxInBlockBody returned the outer-scoped err
when iter.Err() was non-nil, which could incorrectly propagate a nil or
stale error and hide actual RLP decoding issues. This patch returns
iter.Err() as intended by the rlp list iterator API, matching
established patterns elsewhere in the codebase and improving diagnostics
when encountering malformed transaction entries.
…3186)

Co-authored-by: Gary Rong <garyrong0905@gmail.com>
…T, and move the transition tree to its own package (#32445)

This is broken off of #31730 to only focus on testing networks that
start with verkle at genesis.

The PR has seen a lot of work since its creation, and it now targets
creating and re-executing tests for a binary tree testnet without the
transition (so it starts at genesis). The transition tree has been moved
to its own package. It also replaces verkle with the binary tree for
this specific application.

---------

Co-authored-by: Gary Rong <garyrong0905@gmail.com>
Show the actual gas used in the block limit error so RPC clients see
useful numbers.
Fixes a potential panic in `randomDuration` when `min == max` by
handling the edge case explicitly.
`StateDB` lacks recording functionality, so it has been replaced with
`tractStateDB` and advanced
Develop-drift cascade: carries develop's #2347 (Kurtosis e2e and
stateless-e2e on every PR base) and #2333 (complete witnesses under
BlockSTM v2) one hop further up the stack.

Clean at this hop; this branch still predates upstream #34106.
The develop-drift cascade brought in #2333's prewalk and read-set tests,
which call newTrieReader with the verkle point-cache argument develop still
has. Upstream dropped that parameter within the v1.17.0 range, so the calls
carry one argument too many from this branch upward and the core/state test
package no longer typechecks. Same two-line adaptation the merge into
ppatil-upstream-v1.17.2 already carried; this branch and the one above it
were missed.

The reason they were missed is worth recording, because the failure mode is
easy to repeat: the cascade's lower hops were verified with go build, and
go build does not compile test files. Nothing was wrong with the merge — it
was clean and the production tree built — but a test package that cannot
typecheck fails both the lint and unit-tests jobs, which is exactly what CI
reported. go vet ./... catches this and go build ./... never will.

Verified: build clean; full-tree go vet clean apart from the two
pre-existing lock-copy findings in core/parallel_state_processor.go and
trie/secure_trie.go; make lint reports 0 issues; all seven of #2333's
prewalk and read-set tests pass.
@pratikspatil024
pratikspatil024 requested a lite review from Copilot August 12, 2026 09:20
@pratikspatil024

Copy link
Copy Markdown
Member Author

codegenie review

@pratikspatil024

Copy link
Copy Markdown
Member Author

@claude review

@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown

🧞 Codegenie Review

Warning

Review incomplete. Some review or verification work did not complete. Planning also fell back to deterministic default coverage. 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.

Review completed under a partial-coverage budget: only 15 of 1023 hunks in this chunked projection were actually reviewed (865 hunks were never dispatched, planning was degraded), so this is not a full-diff review of the go-ethereum v1.17.4/v1.17.5 sync.

Three actionable findings, all in code adopted from the upstream merge:

  1. core/state/access_events.goKeys() truncates the 32-byte tree index to 31 bytes, so main-storage slots whose key begins with 0xFF (tree index >= 2^248) yield a witness key derived from a wrong/aliased tree index. Needs a caller/spec confirmation on whether the narrowed key derivation is intended.
  2. core/txpool/blobpool/blobpool.go — the discoverFeed/insertFeed NewTxsEvent fan-out moved from Add() (lock released, one batched event) into addLocked() (called with p.lock held, one event per tx, plus more per gapped promotion), turning subscriber backpressure into a pool-wide write-lock stall and changing event granularity for callers.
  3. core/txpool/blobpool/blobpool.gomaxGapped (documented as a total-transaction cap) is applied to len(p.gapped), which counts accounts, so the gapped buffer can hold several times the intended number of blob txs; the promotion/eviction half of the gapped queue also has no test coverage (only the buffering branch is exercised by TestAdd's eve case).

Open follow-ups worth resolving before merge: whether the gappedSource orphan path is reachable when the inner addLocked fails during promotion, whether all NewTxsEvent subscribers are safe against a locked Send, whether decodePair's EIP-8024 immediate mapping matches upstream, and whether the post-Osaka v0 sidecar path still has a conversion site after conversionQueue removal.

Coverage

Partial review: 865 hunks were not reviewed because budget was exhausted before dispatch.
Reviewed 15/1023 hunks before stopping.
Incomplete work: skipped 143, failed 865.
Coverage levels: deep 416, normal 261, light 203, skip 143.
Planning was degraded and deterministic fallbacks were used.
Unreviewed hunks by file:

  • accounts/abi/bind/v2/dep_tree_test.go: 1 hunk (budget stopped before dispatch)
  • accounts/accounts.go: 2 hunks (budget stopped before dispatch)
  • accounts/keystore/keystore.go: 2 hunks (budget stopped before dispatch)
  • accounts/keystore/presale.go: 1 hunk (budget stopped before dispatch)
  • accounts/scwallet/securechannel.go: 1 hunk (budget stopped before dispatch)
  • accounts/usbwallet/hub.go: 3 hunks (budget stopped before dispatch)
  • accounts/usbwallet/ledger.go: 1 hunk (budget stopped before dispatch)
  • accounts/usbwallet/wallet.go: 1 hunk (budget stopped before dispatch)
  • beacon/blsync/engineclient.go: 3 hunks (budget stopped before dispatch)
  • beacon/engine/types.go: 4 hunks (budget stopped before dispatch)
  • beacon/light/canonical.go: 1 hunk (budget stopped before dispatch)
  • beacon/light/sync/head_sync.go: 1 hunk (budget stopped before dispatch)
  • 288 additional files omitted.
  • Budget stopped review work (token limit reached).
  • planner degraded; deterministic default plan used
  • Dossier is chunk 1 of 93 (accounts/abi..accounts/usbwallet) from a 326-file, 997-hunk upstream sync; only the 12 hunks across 8 accounts/* files are in scope here.; This dossier is chunk 3 of 93 for a very large 326-file upstream sync (997 hunks total); only the beacon/params and beacon/types fork-configuration files are routed here.; Planner dossier is chunk 7 of 93 for this 326-file, 997-hunk upstream sync; only the 13 hunks across 6 cmd/ files in this routing chunk are planned here.; Dossier is chunk 8 of 93 for this upstream-sync PR; only the cmd/geth vulnerability-check removal and snapshot CLI hunks are in scope here.; Planner dossier is chunk 9 of 93; only 6 deletion hunks in cmd/geth (version-check removal and verkle scaffolding removal) are visible in this projection out of 997 total hunks across 315 files.; The dossier is chunk 12 of 93 (chunkRoot cmd/workload) from a 326-file, 997-hunk upstream sync. Only the 10 hunks across 5 cmd/workload files are in scope here; the rest of the v1.17.0 merge is covered by other chunks.; Dossier is a chunked routing projection (chunk 13 of 93) rooted at common/bitutil/bitutil_test.go; only this file's 7 hunks are visible here out of 997 total hunks across 315 kept files in the full v1.17.0 upstream sync.; Dossier is chunk 14 of 93 for this PR; only common/bitutil/bitutil.go and common/size.go are in scope here. The remaining ~313 changed files of the v1.17.0 sync are covered by other chunks.; Dossier is chunk 15 of 93 for this PR; only common/types.go and the three consensus engine files are in scope here, while the full PR touches 326 files / 997 hunks.; Dossier is chunk 20 of 93 for a 326-file, 997-hunk upstream sync; only the two core/rawdb tablewriter files (13 hunks) are in scope here.; Planner dossier is chunk 26 of 93 for a 326-file upstream sync; only core/state/dump.go hunks are in scope here.; Dossier is one chunk of a large stacked sync PR; only core/txpool/legacypool/legacypool.go is in scope here, while the PR spans hundreds of files and hunks.; Planner dossier is chunk 41 of 93 (chunkRoot core/vm/instructions.go..core/vm/runtime/env.go); this plan covers only the 10 hunks across 5 core/vm files visible in this chunk, not the full 997-hunk / 326-file merge.; The planner dossier is chunk 44 of 93; it projects only 6 of the 326 changed files (9 of 997 hunks). Coverage is planned solely for this slice.; Planner dossier is chunk 50 of 93 for a 326-file, 997-hunk upstream sync; only the three callTracer withLog golden fixtures are in scope here.; Dossier is a chunked projection (chunk 52 of 93) covering only eth/tracers/internal/tracetest/testdata/call_tracer_withLog/multi_contracts.json; the remaining changed files of the v1.17.0 merge are routed in other chunks.; Planner dossier is chunk 53 of 93; only one changed file (a callTracer golden fixture) is projected into this chunk out of the full sync PR's changed files. The remaining Go source changes are planned in other chunks.; Planner dossier is chunk 55 of 93 for a 326-file, 997-hunk upstream sync; only the ethclient/gethclient files in this chunk are planned here.; Planner dossier is a chunked projection (chunk 59 of 93) limited to internal/ethapi/api.go; the other 325 changed files of this upstream sync are planned in other chunks.; Dossier is chunk 61 of 93 for a very large upstream merge (326 files, 997 hunks); only the 11 hunks in this chunk's file range are planned here.; Planner dossier is a chunked projection (chunk 64/93) of a 326-file, 997-hunk upstream merge; only the 11 hunks across 5 files in this chunk are schedulable here.; This dossier is chunk 69 of 93 and exposes only the trie/bintrie hashed-node file pair; the remaining ~991 hunks of the v1.17.0 sync are routed in other chunks.; Planner dossier is a chunked routing projection (chunk 76 of 93) covering only the trie/trie_test.go and trie/utils verkle files; the full 326-file, 997-hunk v1.17.0 merge is planned in other chunks.; Dossier is chunk 79 of 93 for a 326-file, 997-hunk upstream sync; only triedb/pathdb/database.go is in scope for this plan.; Planner dossier is chunk 88 of 93 of a very large upstream sync (326 files, 997 hunks); this plan only schedules the two triedb/pathdb history files present in this chunk.; Planner dossier is chunk 93 of 93 of a 326-file/997-hunk upstream merge; only 10 hunks across 5 files are projected here, so coverage is scoped to that tail.
  • beacon/engine/gen_ed.go: generated file
  • cmd/keeper/go.sum: lockfile
  • core/tracing/gen_balance_change_reason_stringer.go: generated file
  • core/tracing/gen_code_change_reason_stringer.go: generated file
  • core/tracing/gen_nonce_change_reason_stringer.go: generated file
  • crypto/keccak/keccakf_amd64.s: generated file
  • crypto/keccak/testdata/keccakKats.json.deflate: binary file
  • eth/ethconfig/gen_config.go: generated file
  • ethclient/gethclient/gen_callframe_json.go: generated file
  • ethclient/gethclient/gen_calllog_json.go: generated file

⚠️ Findings

🔵 Medium: Keys() drops the most-significant tree-index byte when building the binary tree key

File: core/state/access_events.go:77
Confidence: medium

AccessEvents.Keys() reconstructs the leaf offset from only 31 of the 32 bytes of the tree index, so the emitted key does not correspond to the tree index that was actually touched whenever the index is >= 2^248.

for chunk := range ae.chunks {
	var offset [32]byte
	treeIndexBytes := chunk.treeIndex.Bytes32()
	copy(offset[:31], treeIndexBytes[1:]) // treeIndexBytes[0] is dropped
	offset[31] = chunk.leafKey
	key := bintrie.GetBinaryTreeKey(chunk.addr, offset[:])
	keys = append(keys, key)
}

The dropped byte is reachable. bintrie.StorageIndex maps main storage to (slot >> 8) + 2^240:

mainStorageOffsetLshVerkleNodeWidth = new(uint256.Int).Lsh(uint256.NewInt(1), 248-uint(verkleNodeWidthLog2))
...
key.Rsh(&key, uint(verkleNodeWidthLog2))
key.Add(&key, mainStorageOffsetLshVerkleNodeWidth)
return &key, suffix

For a slot whose most-significant byte is 0xFF, slot >> 8 >= 2^248 - 2^240, so the addition carries past bit 247 and Bytes32()[0] is non-zero. SlotGas stores that value verbatim into ae.chunks:

treeIndex, subIndex := bintrie.StorageIndex(slot.Bytes())
_, expected := ae.touchAddressAndChargeGas(addr, *treeIndex, subIndex, isWrite, availableGas)

And bintrie.GetBinaryTreeKey only hashes key[:31] as the stem with key[31] as suffix, so a tree index that needs 32 bytes has no representation in this call:

func GetBinaryTreeKey(addr common.Address, key []byte) []byte {
	hasher := sha256.New()
	hasher.Write(zeroHash[:12])
	hasher.Write(addr[:])
	hasher.Write(key[:31])
	hasher.Write([]byte{0})
	k := hasher.Sum(nil)
	k[31] = key[31]
	return k
}

Impact: for main-storage accesses whose slot key begins with 0xFF (~1/256 of a keccak-uniform slot space), Keys() reports a key computed from treeIndex - 2^248. That key both misidentifies the touched slot and aliases the distinct lower tree index, so a witness/access list built from Keys() cannot be validated against the key the binary trie actually uses. Gas accounting and state writes are unaffected — only the key list published by Keys(). The previous implementation passed the full uint256 tree index to utils.GetTreeKeyWithEvaluatedAddress and had no truncation.

This is a contract change to the reported access-key set that arrived with the upstream key-derivation switch (verkle PointCachebintrie.GetBinaryTreeKey); please confirm against the intended binary-tree key spec and the Keys() consumers whether the narrowed derivation is deliberate.

Suggested fix: do not narrow the tree index. Either derive the key through a helper that accepts the full uint256 tree index (matching what GetBinaryTreeKeyStorageSlot produces for the same slot), or fail loudly instead of truncating:

treeIndexBytes := chunk.treeIndex.Bytes32()
if treeIndexBytes[0] != 0 {
	// cannot be represented by a 31-byte stem - use full-width derivation
}

Worth confirming separately that the non-carry case is byte-for-byte identical to GetBinaryTreeKeyStorageSlot, whose stem is {0x01, key[0..29]} rather than a shift of the uint256 tree index.

Suggested test: in core/state/access_events_test.go, call SlotGas with common.HexToHash("0xff00...00") and assert Keys()[0] == bintrie.GetBinaryTreeKeyStorageSlot(addr, slot[:]), plus that two slots differing only in that high region produce distinct keys.

🔵 Medium: NewTxsEvent feeds are now sent per-tx while the blobpool write lock is held

File: core/txpool/blobpool/blobpool.go:1683
Confidence: medium

The blobpool's NewTxsEvent fan-out moved from Add() (lock released, one batched event per call) into addLocked(), which runs with p.lock held, and now emits one event per transaction:

// core/txpool/blobpool/blobpool.go - inside addLocked
// Notify all listeners of the new arrival
p.discoverFeed.Send(core.NewTxsEvent{Txs: []*types.Transaction{tx.WithoutBlobTxSidecar()}})
p.insertFeed.Send(core.NewTxsEvent{Txs: []*types.Transaction{tx.WithoutBlobTxSidecar()}})

addLocked is reached with the pool write lock held:

func (p *BlobPool) add(tx *types.Transaction) (err error) {
	waitStart := time.Now()
	p.lock.Lock()
	addwaitHist.Update(time.Since(waitStart).Nanoseconds())
	defer p.lock.Unlock()
	...
	return p.addLocked(tx, true)
}

Base performed the fan-out after p.add() returned and the lock was released, batched into a single event:

for i, tx := range txs {
	if errs[i] = p.preCheck(tx); errs[i] != nil {
 continue
	}
	if errs[i] = p.add(tx); errs[i] == nil {
 adds = append(adds, tx.WithoutBlobTxSidecar())
	}
}
if len(adds) > 0 {
	p.discoverFeed.Send(core.NewTxsEvent{Txs: adds})
	p.insertFeed.Send(core.NewTxsEvent{Txs: adds})
}
return errs

Head Add() still accumulates adds but never sends it, which suggests the send site was relocated during conflict resolution rather than redesigned. The gapped-promotion path re-enters addLocked inside the same critical section, so each promoted transaction triggers two more locked Send calls:

if tx.Nonce() <= firstgap {
	if err := p.addLocked(tx, false); err == nil {

Impact: event.Feed.Send blocks until every subscriber accepts the value, so a slow or full NewTxsEvent subscriber now stalls the blobpool write lock and with it all concurrent Get/Has/Pending/Add/reset callers. Per-transaction granularity fills subscriber buffers faster than the previous batched event. If any subscriber must acquire p.lock before draining its channel, this is a deadlock rather than just latency. The caller-visible event granularity also changed from one batched event per Add() call to one event per transaction.

This is a contract change relative to base (and to legacypool, which also fans out outside the pool mutex) that arrived with the upstream merge; please confirm the intended dispatch point with the subscriber owners. Subscriber buffer sizes and re-entrancy were not verified, so the deadlock leg is unconfirmed — the demonstrated effect is lock-hold amplification.

Suggested fix: restore the base ordering — have addLocked record accepted/promoted transactions (append to a pool field or return them) and perform the discoverFeed/insertFeed sends in Add() after p.lock is released, reusing the adds slice that is currently built but unused. If the sends must stay in addLocked, document and verify that every subscriber uses a sufficiently buffered channel and never acquires p.lock before draining.

Suggested test: subscribe to the pool's transaction feed with a never-drained channel, call Add for a valid blob tx, and assert from another goroutine that Get/Has (which take p.lock) complete within a timeout; extend the case to the gapped-promotion path so the recursive addLocked sends are covered.

⚪ Low: maxGapped bounds the number of gapped accounts, not queued gapped transactions

File: core/txpool/blobpool/blobpool.go:1532
Confidence: medium

maxGapped is documented as a cap on gapped transactions, but the admission guard applies it to the number of gapped accounts:

allowance := p.gappedAllowance(from)
if allowance >= 1 && len(p.gapped) < maxGapped {
	p.gapped[from] = append(p.gapped[from], tx)
	p.gappedSource[tx.Hash()] = from

p.gapped is keyed by sender, so len(p.gapped) is an account count:

gapped       map[common.Address][]*types.Transaction // Transactions that are currently gapped (nonce too high)
gappedSource map[common.Hash]common.Address          // Source of gapped transactions to allow rechecking on inclusion

// maxGappedTxs is the maximum number of gapped transactions kept overall.
// This is a safety limit to avoid DoS vectors.
maxGapped = 128

Per-account depth can exceed 1:

func (p *BlobPool) gappedAllowance(addr common.Address) int {
	nonce := p.state.GetNonce(addr)
	allowance := int(math.Log10(float64(nonce + 1)))
	return min(allowance, maxTxsPerAccount-len(p.index[addr])) - len(p.gapped[addr])
}

So up to 128 accounts may each buffer several full blob transactions (sidecars included, ~128KB per blob). There is no global transaction or byte counter on this path; evictGapped only trims per account by gappedLifetime and stale nonce. Realistically depth 2 needs on-chain nonce >= 100 and depth 5 needs >= 10^5, and entries expire after one minute, so the overshoot is a bounded single-digit multiplier of the documented memory headroom rather than an unbounded DoS — but the enforced limit is not the documented one, which will misdirect any future tuning of maxGapped.

Suggested fix: either track a running total of buffered gapped transactions (or sum queue lengths) and compare that against maxGapped, or rename the constant to maxGappedAccounts, update its doc comment, and add a separate explicit total-transaction/byte cap.

Separately, the promotion and cleanup half of this feature is untested. The only test touching the gapped queue is TestAdd's eve case, which exercises the buffering branch and stops there:

"eve": {balance: 21100 + blobSize, nonce: 10}, // High nonce to test gapped acceptance
...
{ // Old account, 10 txs in chain: 0 pending: accept nonce 11 as gapped
	from: "eve",
	tx:   makeUnsignedTx(11, 1, 1, 1),
	err:  nil,
	check: func(pool *BlobPool, tx *types.Transaction) bool {
		return pool.Status(tx.Hash()) == txpool.TxStatusQueued
	},
},

No test ever adds the gap-filling nonce, so the promotion loop, the stale tx.Nonce() < stateNonce drop branch, delete(p.gappedSource, ...) and the p.gapped[from] = gtxs write-back never execute:

if gtxs, ok := p.gapped[from]; checkGapped && ok && len(gtxs) > 0 {
	sort.SliceStable(gtxs, func(i, j int) bool { return gtxs[i].Nonce() < gtxs[j].Nonce() })
	for len(gtxs) > 0 {
		stateNonce := p.state.GetNonce(from)
		firstgap := stateNonce + uint64(len(p.index[from]))
		if gtxs[0].Nonce() > firstgap { break }
		tx := gtxs[0]; gtxs[0] = nil; gtxs = gtxs[1:]
		delete(p.gappedSource, tx.Hash())
		if tx.Nonce() < stateNonce { continue }
		if tx.Nonce() <= firstgap {
			if err := p.addLocked(tx, false); err == nil { ... } else { ... }
		}
	}
	if len(gtxs) == 0 { delete(p.gapped, from) } else { p.gapped[from] = gtxs }
}

A defect here — the write-back clobbering entries appended by the nested addLocked(tx, false), an off-by-one in the firstgap comparison, or a leaked p.gappedSource entry — would not be caught by CI, and would surface as gap-filled blob txs silently expiring instead of becoming pending.

Suggested test: func TestGappedPromotion(t *testing.T) — seed an account at state nonce 10 with sufficient balance; pool.Add(tx(nonce=11)) and assert err == nil, Status == txpool.TxStatusQueued, len(pool.gapped[addr]) == 1; then pool.Add(tx(nonce=10)) and assert Status(tx11) == txpool.TxStatusPending, pool.gapped has no entry for addr, pool.gappedSource has no entry for tx11.Hash(), and verifyPoolInternals(t, pool) passes. Add companion cases for the stale-drop branch and for evictGapped after gappedLifetime. Also add a case that fills maxGapped accounts, each with a state nonce high enough for allowance > 1, and asserts the summed len(p.gapped[addr]) stays within the intended global bound.

🙋 Needs Human Attention

  • Does decodePair's else-branch mapping (r+1, 29-q) match the EIP-8024 specification's EXCHANGE immediate encoding, and are the excluded immediate ranges (x>90&&x<128 for DUPN/SWAPN, x>79&&x<128 for EXCHANGE) exactly the upstream go-ethereum v1.17.0 values?
    • Files: core/vm/instructions.go, core/vm/instructions_test.go
    • Symbols: TestEIP8024_Execution, codeBitmap, decodePair, decodeSingle, opDupN, opExchange
    • Reason: decodePair is a bijection onto 208 pairs and is self-consistent, and the one test vector (imm=0x01 -> (1,2)) only exercises the q<r branch; the (r+1, 29-q) branch has no inspected test vector, and a mismatch with the spec would be consensus-splitting. Related reasons: Packet reviewer could not resolve this question from the reviewed context. Grouped from 2 related hints across 1 packet.

Stats

  • 🤖 Model: anthropic claude-opus-5 high
  • 🧞 Codegenie: v0.5.5 (58f82a9b2c)
  • Elapsed time: 1h 12m 49s
  • Git: 0xPolygon/bor from ppatil-upstream-v1.16.9 to ppatil-upstream-v1.17.0 (127e7eae0c)
  • Posting: 3 inline
  • Review completeness: partial.
  • Usage: model calls 173, tokens 7713839, cost $50.4916.
  • Effective caps: tokens 8000000.
  • Budget dispatch blocks: stage 7 tokens 6806936/8000000; stage 7 tokens 6819545/8000000; stage 7 tokens 6819545/8000000; stage 7 tokens 6830723/8000000; stage 7 tokens 6886157/8000000; stage 7 tokens 6917754/8000000.
  • Local context pressure: 77 degraded tool results, 177 degraded hunks.

View Workflow Job

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

Comment on lines 72 to 83
// TODO: consider if parallelizing this is worth it, probably depending on len(ae.chunks).
keys := make([][]byte, 0, len(ae.chunks))
for chunk := range ae.chunks {
basePoint := ae.pointCache.Get(chunk.addr[:])
key := utils.GetTreeKeyWithEvaluatedAddress(basePoint, &chunk.treeIndex, chunk.leafKey)
var offset [32]byte
treeIndexBytes := chunk.treeIndex.Bytes32()
copy(offset[:31], treeIndexBytes[1:])
offset[31] = chunk.leafKey
key := bintrie.GetBinaryTreeKey(chunk.addr, offset[:])
keys = append(keys, key)
}
return keys
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 AccessEvents.Keys() (core/state/access_events.go:71-83, new in this PR) reconstructs the binary-trie key for a storage slot from bintrie.StorageIndex()'s (treeIndex, leafKey) pair by taking treeIndex.Bytes32()[1:] as the 31-byte tree-index. This does not match trie/bintrie.GetBinaryTreeKeyStorageSlot() (used by BinaryTrie.GetStorage/UpdateStorage/DeleteStorage to actually read/write the slot), which builds its tree-index via a naive byte-copy that drops slot byte 30 instead of shifting right by 8 bits. For essentially any real (keccak-derived) storage slot the two derivations diverge, so the witness/access-key set built by Keys() points at the wrong trie location. Fix by making GetBinaryTreeKeyStorageSlot use the same arithmetic right-shift-by-8 as StorageIndex (or vice versa, whichever matches the EIP-6800 spec) so both paths agree. Currently unreachable since the binary-trie/EIP-4762 fork is dormant on all Bor networks, so this does not block merge, but must be fixed before that fork could ever activate.

Extended reasoning...

What the bug is: AccessEvents.Keys() (core/state/access_events.go:71-83) is new code in this PR that reconstructs a binary-trie key from a chunkAccessKey (an (addr, treeIndex, leafKey) triple recorded by SlotGas/bintrie.StorageIndex()). It does this by taking chunk.treeIndex.Bytes32()[1:] as the 31-byte tree-index portion of the key offset and chunk.leafKey as the final suffix byte, then calling bintrie.GetBinaryTreeKey(addr, offset).

The problem is that this reconstruction does not match the key actually used to store and retrieve the same storage slot in the trie. trie/bintrie.GetBinaryTreeKeyStorageSlot() — the function BinaryTrie.GetStorage, UpdateStorage, and DeleteStorage (trie/bintrie/trie.go:239, 282, 305) actually call to touch storage — builds its 31-byte tree-index stem via a naive byte-copy: k[0]=1; copy(k[1:31], slot[0:30]); k[31]=slot[31]. This silently drops slot[30] and effectively computes tree_index = 2^240 + (slot >> 16).

bintrie.StorageIndex() (the function that feeds Keys() via SlotGas), on the other hand, does the spec-correct EIP-6800 arithmetic: tree_index = 2^240 + (slot >> 8), extracting the suffix as slot[31].

Why they diverge: the two derivations only agree when slot < 256 (i.e., when byte 30 and everything above it in slot >> 8 is zero and shifting by 8 vs 16 makes no observable difference). For any storage slot at or above 256 — which covers virtually every real Solidity mapping/array slot, since those are keccak256 outputs — the two functions produce completely different 32-byte trie keys. This was verified concretely by multiple independent checks: e.g. for slot 0x10000, the direct GetBinaryTreeKeyStorageSlot key and the Keys()-style reconstruction via StorageIndex differ entirely, and one verifier found the exact off-by-one-byte signature (the storage key for slot 0x10000 equals the witness key for slot 0x100).

Why nothing currently catches this: AccessEvents was switched in this PR from the old verkle-tree utils.StorageIndex/PointCache derivation (which was presumably consistent with the verkle trie's own key derivation) to the new bintrie package. The pre-existing GetBinaryTreeKeyStorageSlot function was not updated to match, and no test in this PR exercises Keys() against the trie's actual storage locations for a realistic (large) slot value — the only test that exists is a narrowly-scoped unit test, and slots below 256 happen to accidentally agree.

Impact: Under EIP-4762/binary-trie execution, AccessEvents.Keys() is used to build the state-access witness key set (used for gas metering and stateless-witness verification). With this bug, it would report a bogus key for essentially every non-trivial storage slot access instead of the slot's real trie location, corrupting witness generation for any block that touches ordinary contract storage.

Concrete proof (byte-level walkthrough) for slot = 0x10000:

  1. StorageIndex(0x10000) computes treeIndex = 2^240 + (0x10000 >> 8) = 2^240 + 0x100. As a 32-byte big-endian value, this has 0x01 at byte index 29 (counting from 0), matching 0x100 sitting two bytes from the end.
  2. Keys() takes treeIndex.Bytes32()[1:] (bytes 1..31, i.e. effectively re-indexing bytes 1..30 of the original 32-byte value onto offset[0..29]) — the 0x01 lands at offset index 29, hash input becomes [..., 0x01, 0x00] with leafKey (slot[31]=0x00) appended as offset[31].
  3. GetBinaryTreeKeyStorageSlot(addr, 0x10000) instead copies slot[0:30] directly into k[1:31] and overwrites k[31]=slot[31]. For slot 0x10000 (32-byte big-endian: zeros except byte 29 = 0x01), this puts 0x01 at byte index 30 of the key offset (not 29), because it never shifts — it's a literal byte copy of the original slot bytes.
  4. The two 32-byte offset values passed into GetBinaryTreeKey(addr, offset) therefore differ (0x01 at position 29 vs. position 30), producing two entirely different sha256-derived 32-byte trie keys.

Fix: Align the two derivations — either make GetBinaryTreeKeyStorageSlot perform the same slot >> 8 arithmetic shift that StorageIndex uses (the EIP-6800-correct approach), or make Keys()/StorageIndex mirror whatever GetBinaryTreeKeyStorageSlot actually does. Either way, the witness-key derivation path and the actual trie storage/read path must agree bit-for-bit.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and thanks for the byte-level walkthrough — but not something this PR will change.

trie/bintrie/key_encoding.go is byte-identical to upstream v1.17.0 here, so the mismatch is upstream's, arriving via the sync rather than authored by us. It's also unreachable on Bor today: the binary-trie / EIP-4762 path is gated behind VerkleBlock, which is nil on every Bor preset, so Keys() never runs.

Patching upstream code inside a sync merge means re-resolving that divergence on every future sync, for no runtime effect. Recording it in docs/upstream-merges/.../needs-wiring.md instead, so it's a blocker on ever enabling the fork rather than a silent gap, and raising it upstream.

Comment thread core/vm/instructions.go
Comment on lines +1025 to +1050
func opDupN(pc *uint64, evm *EVM, scope *ScopeContext) ([]byte, error) {
code := scope.Contract.Code
i := *pc + 1

// If the immediate byte is missing, treat as 0x00 (same convention as PUSHn).
var x byte
if i < uint64(len(code)) {
x = code[i]
}

// This range is excluded to preserve compatibility with existing opcodes.
if x > 90 && x < 128 {
return nil, &ErrInvalidOpCode{opcode: OpCode(x)}
}
n := decodeSingle(x)

// DUPN duplicates the n'th stack item, so the stack must contain at least n elements.
if scope.Stack.len() < n {
return nil, &ErrStackUnderflow{stackLen: scope.Stack.len(), required: n}
}

//The n‘th stack item is duplicated at the top of the stack.
scope.Stack.push(scope.Stack.Back(n - 1))
*pc += 1
return nil, nil
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 opDupN, opSwapN, and opExchange (core/vm/instructions.go, added for EIP-8024) each re-implement the identical snippet for reading the opcode's immediate byte with an implicit-zero default (code := scope.Contract.Code; i := *pc + 1; var x byte; if i < uint64(len(code)) { x = code[i] }), including the same explanatory comment. Extracting this into a small helper (e.g. immediateByte(pc *uint64, scope *ScopeContext) byte) would remove the triplication and keep the PUSHn-style boundary convention in one place for future fixes.

Extended reasoning...

What the duplication looks like

core/vm/instructions.go adds three new opcode handlers for EIP-8024: opDupN (~lines 1025-1050), opSwapN (~lines 1052-1079), and opExchange (~lines 1081-1111). Each begins with the byte-for-byte identical block:

code := scope.Contract.Code
i := *pc + 1

// If the immediate byte is missing, treat as 0x00 (same convention as PUSHn).
var x byte
if i < uint64(len(code)) {
    x = code[i]
}

This is the "read the immediate operand byte following the opcode, defaulting to 0x00 if the code buffer runs out" convention borrowed from the existing PUSHn opcodes, but here it's freshly copy-pasted three times in the same PR rather than factored into a shared helper.

Why this is worth flagging

Each of the three handlers also carries its own opcode-specific range check (x > 90 && x < 128 for DUPN/SWAPN vs x > 79 && x < 128 for EXCHANGE) and its own *pc += 1 increment, which legitimately differ per-opcode and are not part of this finding. Only the immediate-byte-read snippet itself — five identical lines plus an identical comment — is duplicated three times. Because the read logic and its boundary convention now exist in three copies, any future correction to that convention (e.g. a boundary/off-by-one fix, or a change in how out-of-bounds immediates are handled) has to be applied identically in three places, and it's easy to update one copy and miss the others.

Suggested fix

Extract the read into a small helper, e.g.:

// immediateByte returns the opcode's immediate byte at pc+1, or 0x00 if the
// code buffer doesn't extend that far (same convention as PUSHn).
func immediateByte(pc *uint64, scope *ScopeContext) byte {
    code := scope.Contract.Code
    i := *pc + 1
    if i < uint64(len(code)) {
        return code[i]
    }
    return 0
}

and call x := immediateByte(pc, scope) at the top of each of opDupN, opSwapN, and opExchange, leaving each opcode's own range check and *pc += 1 untouched. This is a pure refactor — the read logic and its behavior are unchanged, only its location — so there's no risk of altering opcode semantics. Since this is a tiny, inlinable helper on a hot execution path (the EVM interpreter loop), there's no meaningful performance concern either; the Go compiler will happily inline it.

Step-by-step verification

  1. Open core/vm/instructions.go and locate opDupN: it contains code := scope.Contract.Code; i := *pc + 1; var x byte; if i < uint64(len(code)) { x = code[i] } with the comment "If the immediate byte is missing, treat as 0x00 (same convention as PUSHn)."
  2. Locate opSwapN immediately below it: the identical five-line block and identical comment appear again, verbatim.
  3. Locate opExchange immediately below that: the same five-line block and comment appear a third time.
  4. Diff the three blocks against each other — they are character-for-character identical, confirming genuine triplication rather than superficially similar code.
  5. Confirm the surrounding code (range checks, *pc += 1) differs per-opcode, so only the immediate-byte-read logic itself is the duplicated unit, and it is exactly what a single immediateByte helper would replace.

This is scoped purely to code quality — reuse and maintainability — with no functional or security implication, so it should not block merging this PR.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this as-is.

The three handlers are upstream code (EIP-8024 arrives with this merge), so factoring a helper out of them would be a Bor-only edit to a file upstream keeps touching — a conflict we'd re-resolve every sync.

It also runs against our own style guidance, which is to leave three similar call sites alone until a fourth arrives or all three have to change together. Neither applies yet.

Comment thread core/txpool/blobpool/blobpool.go
@pratikspatil024
pratikspatil024 marked this pull request as ready for review August 13, 2026 06:55

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

codegenie review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧞 Codegenie Review

Warning

Review incomplete. Some review or verification work did not complete. Planning also fell back to deterministic default coverage. Treat the results below as partial and consider rerunning.

Review completed under a partial-coverage budget: only 15 of 1023 hunks in this chunked projection were actually reviewed (865 hunks were never dispatched, planning was degraded), so this is not a full-diff review of the go-ethereum v1.17.4/v1.17.5 sync.

Three actionable findings, all in code adopted from the upstream merge:

  1. core/state/access_events.goKeys() truncates the 32-byte tree index to 31 bytes, so main-storage slots whose key begins with 0xFF (tree index >= 2^248) yield a witness key derived from a wrong/aliased tree index. Needs a caller/spec confirmation on whether the narrowed key derivation is intended.
  2. core/txpool/blobpool/blobpool.go — the discoverFeed/insertFeed NewTxsEvent fan-out moved from Add() (lock released, one batched event) into addLocked() (called with p.lock held, one event per tx, plus more per gapped promotion), turning subscriber backpressure into a pool-wide write-lock stall and changing event granularity for callers.
  3. core/txpool/blobpool/blobpool.gomaxGapped (documented as a total-transaction cap) is applied to len(p.gapped), which counts accounts, so the gapped buffer can hold several times the intended number of blob txs; the promotion/eviction half of the gapped queue also has no test coverage (only the buffering branch is exercised by TestAdd's eve case).

Open follow-ups worth resolving before merge: whether the gappedSource orphan path is reachable when the inner addLocked fails during promotion, whether all NewTxsEvent subscribers are safe against a locked Send, whether decodePair's EIP-8024 immediate mapping matches upstream, and whether the post-Osaka v0 sidecar path still has a conversion site after conversionQueue removal.

Partial review: 865 hunks were not reviewed because budget was exhausted before dispatch.
Reviewed 15/1023 hunks before stopping.

Coverage disclosure:

  • Budget stopped review work (token limit reached).
  • planner degraded; deterministic default plan used
  • Dossier is chunk 1 of 93 (accounts/abi..accounts/usbwallet) from a 326-file, 997-hunk upstream sync; only the 12 hunks across 8 accounts/* files are in scope here.; This dossier is chunk 3 of 93 for a very large 326-file upstream sync (997 hunks total); only the beacon/params and beacon/types fork-configuration files are routed here.; Planner dossier is chunk 7 of 93 for this 326-file, 997-hunk upstream sync; only the 13 hunks across 6 cmd/ files in this routing chunk are planned here.; Dossier is chunk 8 of 93 for this upstream-sync PR; only the cmd/geth vulnerability-check removal and snapshot CLI hunks are in scope here.; Planner dossier is chunk 9 of 93; only 6 deletion hunks in cmd/geth (version-check removal and verkle scaffolding removal) are visible in this projection out of 997 total hunks across 315 files.; The dossier is chunk 12 of 93 (chunkRoot cmd/workload) from a 326-file, 997-hunk upstream sync. Only the 10 hunks across 5 cmd/workload files are in scope here; the rest of the v1.17.0 merge is covered by other chunks.; Dossier is a chunked routing projection (chunk 13 of 93) rooted at common/bitutil/bitutil_test.go; only this file's 7 hunks are visible here out of 997 total hunks across 315 kept files in the full v1.17.0 upstream sync.; Dossier is chunk 14 of 93 for this PR; only common/bitutil/bitutil.go and common/size.go are in scope here. The remaining ~313 changed files of the v1.17.0 sync are covered by other chunks.; Dossier is chunk 15 of 93 for this PR; only common/types.go and the three consensus engine files are in scope here, while the full PR touches 326 files / 997 hunks.; Dossier is chunk 20 of 93 for a 326-file, 997-hunk upstream sync; only the two core/rawdb tablewriter files (13 hunks) are in scope here.; Planner dossier is chunk 26 of 93 for a 326-file upstream sync; only core/state/dump.go hunks are in scope here.; Dossier is one chunk of a large stacked sync PR; only core/txpool/legacypool/legacypool.go is in scope here, while the PR spans hundreds of files and hunks.; Planner dossier is chunk 41 of 93 (chunkRoot core/vm/instructions.go..core/vm/runtime/env.go); this plan covers only the 10 hunks across 5 core/vm files visible in this chunk, not the full 997-hunk / 326-file merge.; The planner dossier is chunk 44 of 93; it projects only 6 of the 326 changed files (9 of 997 hunks). Coverage is planned solely for this slice.; Planner dossier is chunk 50 of 93 for a 326-file, 997-hunk upstream sync; only the three callTracer withLog golden fixtures are in scope here.; Dossier is a chunked projection (chunk 52 of 93) covering only eth/tracers/internal/tracetest/testdata/call_tracer_withLog/multi_contracts.json; the remaining changed files of the v1.17.0 merge are routed in other chunks.; Planner dossier is chunk 53 of 93; only one changed file (a callTracer golden fixture) is projected into this chunk out of the full sync PR's changed files. The remaining Go source changes are planned in other chunks.; Planner dossier is chunk 55 of 93 for a 326-file, 997-hunk upstream sync; only the ethclient/gethclient files in this chunk are planned here.; Planner dossier is a chunked projection (chunk 59 of 93) limited to internal/ethapi/api.go; the other 325 changed files of this upstream sync are planned in other chunks.; Dossier is chunk 61 of 93 for a very large upstream merge (326 files, 997 hunks); only the 11 hunks in this chunk's file range are planned here.; Planner dossier is a chunked projection (chunk 64/93) of a 326-file, 997-hunk upstream merge; only the 11 hunks across 5 files in this chunk are schedulable here.; This dossier is chunk 69 of 93 and exposes only the trie/bintrie hashed-node file pair; the remaining ~991 hunks of the v1.17.0 sync are routed in other chunks.; Planner dossier is a chunked routing projection (chunk 76 of 93) covering only the trie/trie_test.go and trie/utils verkle files; the full 326-file, 997-hunk v1.17.0 merge is planned in other chunks.; Dossier is chunk 79 of 93 for a 326-file, 997-hunk upstream sync; only triedb/pathdb/database.go is in scope for this plan.; Planner dossier is chunk 88 of 93 of a very large upstream sync (326 files, 997 hunks); this plan only schedules the two triedb/pathdb history files present in this chunk.; Planner dossier is chunk 93 of 93 of a 326-file/997-hunk upstream merge; only 10 hunks across 5 files are projected here, so coverage is scoped to that tail.
  • beacon/engine/gen_ed.go: generated file
  • cmd/keeper/go.sum: lockfile
  • core/tracing/gen_balance_change_reason_stringer.go: generated file
  • core/tracing/gen_code_change_reason_stringer.go: generated file
  • core/tracing/gen_nonce_change_reason_stringer.go: generated file
  • crypto/keccak/keccakf_amd64.s: generated file
  • crypto/keccak/testdata/keccakKats.json.deflate: binary file
  • eth/ethconfig/gen_config.go: generated file
  • ethclient/gethclient/gen_callframe_json.go: generated file
  • ethclient/gethclient/gen_calllog_json.go: generated file

🙋 Needs human attention:

  • Does decodePair's else-branch mapping (r+1, 29-q) match the EIP-8024 specification's EXCHANGE immediate encoding, and are the excluded immediate ranges (x>90&&x<128 for DUPN/SWAPN, x>79&&x<128 for EXCHANGE) exactly the upstream go-ethereum v1.17.0 values?

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

Comment on lines +75 to +77
var offset [32]byte
treeIndexBytes := chunk.treeIndex.Bytes32()
copy(offset[:31], treeIndexBytes[1:])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AccessEvents.Keys() reconstructs the leaf offset from only 31 of the 32 bytes of the tree index, so the emitted key does not correspond to the tree index that was actually touched whenever the index is >= 2^248.

for chunk := range ae.chunks {
	var offset [32]byte
	treeIndexBytes := chunk.treeIndex.Bytes32()
	copy(offset[:31], treeIndexBytes[1:]) // treeIndexBytes[0] is dropped
	offset[31] = chunk.leafKey
	key := bintrie.GetBinaryTreeKey(chunk.addr, offset[:])
	keys = append(keys, key)
}

The dropped byte is reachable. bintrie.StorageIndex maps main storage to (slot >> 8) + 2^240:

mainStorageOffsetLshVerkleNodeWidth = new(uint256.Int).Lsh(uint256.NewInt(1), 248-uint(verkleNodeWidthLog2))
...
key.Rsh(&key, uint(verkleNodeWidthLog2))
key.Add(&key, mainStorageOffsetLshVerkleNodeWidth)
return &key, suffix

For a slot whose most-significant byte is 0xFF, slot >> 8 >= 2^248 - 2^240, so the addition carries past bit 247 and Bytes32()[0] is non-zero. SlotGas stores that value verbatim into ae.chunks:

treeIndex, subIndex := bintrie.StorageIndex(slot.Bytes())
_, expected := ae.touchAddressAndChargeGas(addr, *treeIndex, subIndex, isWrite, availableGas)

And bintrie.GetBinaryTreeKey only hashes key[:31] as the stem with key[31] as suffix, so a tree index that needs 32 bytes has no representation in this call:

func GetBinaryTreeKey(addr common.Address, key []byte) []byte {
	hasher := sha256.New()
	hasher.Write(zeroHash[:12])
	hasher.Write(addr[:])
	hasher.Write(key[:31])
	hasher.Write([]byte{0})
	k := hasher.Sum(nil)
	k[31] = key[31]
	return k
}

Impact: for main-storage accesses whose slot key begins with 0xFF (~1/256 of a keccak-uniform slot space), Keys() reports a key computed from treeIndex - 2^248. That key both misidentifies the touched slot and aliases the distinct lower tree index, so a witness/access list built from Keys() cannot be validated against the key the binary trie actually uses. Gas accounting and state writes are unaffected — only the key list published by Keys(). The previous implementation passed the full uint256 tree index to utils.GetTreeKeyWithEvaluatedAddress and had no truncation.

This is a contract change to the reported access-key set that arrived with the upstream key-derivation switch (verkle PointCachebintrie.GetBinaryTreeKey); please confirm against the intended binary-tree key spec and the Keys() consumers whether the narrowed derivation is deliberate.

Suggested fix: do not narrow the tree index. Either derive the key through a helper that accepts the full uint256 tree index (matching what GetBinaryTreeKeyStorageSlot produces for the same slot), or fail loudly instead of truncating:

treeIndexBytes := chunk.treeIndex.Bytes32()
if treeIndexBytes[0] != 0 {
	// cannot be represented by a 31-byte stem - use full-width derivation
}

Worth confirming separately that the non-carry case is byte-for-byte identical to GetBinaryTreeKeyStorageSlot, whose stem is {0x01, key[0..29]} rather than a shift of the uint256 tree index.

Suggested test: in core/state/access_events_test.go, call SlotGas with common.HexToHash("0xff00...00") and assert Keys()[0] == bintrie.GetBinaryTreeKeyStorageSlot(addr, slot[:]), plus that two slots differing only in that high region produce distinct keys.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and thanks for the byte-level walkthrough — but not something this PR will change.

trie/bintrie/key_encoding.go is byte-identical to upstream v1.17.0 here, so the mismatch is upstream's, arriving via the sync rather than authored by us. It's also unreachable on Bor today: the binary-trie / EIP-4762 path is gated behind VerkleBlock, which is nil on every Bor preset, so Keys() never runs.

Patching upstream code inside a sync merge means re-resolving that divergence on every future sync, for no runtime effect. Recording it in docs/upstream-merges/.../needs-wiring.md instead, so it's a blocker on ever enabling the fork rather than a silent gap, and raising it upstream.

addValidMeter.Mark(1)

// Notify all listeners of the new arrival
p.discoverFeed.Send(core.NewTxsEvent{Txs: []*types.Transaction{tx.WithoutBlobTxSidecar()}})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The blobpool's NewTxsEvent fan-out moved from Add() (lock released, one batched event per call) into addLocked(), which runs with p.lock held, and now emits one event per transaction:

// core/txpool/blobpool/blobpool.go - inside addLocked
// Notify all listeners of the new arrival
p.discoverFeed.Send(core.NewTxsEvent{Txs: []*types.Transaction{tx.WithoutBlobTxSidecar()}})
p.insertFeed.Send(core.NewTxsEvent{Txs: []*types.Transaction{tx.WithoutBlobTxSidecar()}})

addLocked is reached with the pool write lock held:

func (p *BlobPool) add(tx *types.Transaction) (err error) {
	waitStart := time.Now()
	p.lock.Lock()
	addwaitHist.Update(time.Since(waitStart).Nanoseconds())
	defer p.lock.Unlock()
	...
	return p.addLocked(tx, true)
}

Base performed the fan-out after p.add() returned and the lock was released, batched into a single event:

for i, tx := range txs {
	if errs[i] = p.preCheck(tx); errs[i] != nil {
 continue
	}
	if errs[i] = p.add(tx); errs[i] == nil {
 adds = append(adds, tx.WithoutBlobTxSidecar())
	}
}
if len(adds) > 0 {
	p.discoverFeed.Send(core.NewTxsEvent{Txs: adds})
	p.insertFeed.Send(core.NewTxsEvent{Txs: adds})
}
return errs

Head Add() still accumulates adds but never sends it, which suggests the send site was relocated during conflict resolution rather than redesigned. The gapped-promotion path re-enters addLocked inside the same critical section, so each promoted transaction triggers two more locked Send calls:

if tx.Nonce() <= firstgap {
	if err := p.addLocked(tx, false); err == nil {

Impact: event.Feed.Send blocks until every subscriber accepts the value, so a slow or full NewTxsEvent subscriber now stalls the blobpool write lock and with it all concurrent Get/Has/Pending/Add/reset callers. Per-transaction granularity fills subscriber buffers faster than the previous batched event. If any subscriber must acquire p.lock before draining its channel, this is a deadlock rather than just latency. The caller-visible event granularity also changed from one batched event per Add() call to one event per transaction.

This is a contract change relative to base (and to legacypool, which also fans out outside the pool mutex) that arrived with the upstream merge; please confirm the intended dispatch point with the subscriber owners. Subscriber buffer sizes and re-entrancy were not verified, so the deadlock leg is unconfirmed — the demonstrated effect is lock-hold amplification.

Suggested fix: restore the base ordering — have addLocked record accepted/promoted transactions (append to a pool field or return them) and perform the discoverFeed/insertFeed sends in Add() after p.lock is released, reusing the adds slice that is currently built but unused. If the sends must stay in addLocked, document and verify that every subscriber uses a sufficiently buffered channel and never acquires p.lock before draining.

Suggested test: subscribe to the pool's transaction feed with a never-drained channel, call Add for a valid blob tx, and assert from another goroutine that Get/Has (which take p.lock) complete within a timeout; extend the case to the gapped-promotion path so the recursive addLocked sends are covered.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this one specifically, since "moved into addLocked" would be a merge-resolution error if it were ours — it isn't. Upstream v1.17.0 has both sends inside addLocked at the same lines (1682-1683), so the structure is upstream's own.

And as with the other blobpool findings: Bor never constructs a blobpool (blobpool.New( has no non-test callers; eth/backend.go registers only legacyPool), so this path doesn't execute here.

Not changing it in a sync PR, for the usual divergence reason.

// Store the tx in memory, and revalidate later
from, _ := types.Sender(p.signer, tx)
allowance := p.gappedAllowance(from)
if allowance >= 1 && len(p.gapped) < maxGapped {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxGapped is documented as a cap on gapped transactions, but the admission guard applies it to the number of gapped accounts:

allowance := p.gappedAllowance(from)
if allowance >= 1 && len(p.gapped) < maxGapped {
	p.gapped[from] = append(p.gapped[from], tx)
	p.gappedSource[tx.Hash()] = from

p.gapped is keyed by sender, so len(p.gapped) is an account count:

gapped       map[common.Address][]*types.Transaction // Transactions that are currently gapped (nonce too high)
gappedSource map[common.Hash]common.Address          // Source of gapped transactions to allow rechecking on inclusion

// maxGappedTxs is the maximum number of gapped transactions kept overall.
// This is a safety limit to avoid DoS vectors.
maxGapped = 128

Per-account depth can exceed 1:

func (p *BlobPool) gappedAllowance(addr common.Address) int {
	nonce := p.state.GetNonce(addr)
	allowance := int(math.Log10(float64(nonce + 1)))
	return min(allowance, maxTxsPerAccount-len(p.index[addr])) - len(p.gapped[addr])
}

So up to 128 accounts may each buffer several full blob transactions (sidecars included, ~128KB per blob). There is no global transaction or byte counter on this path; evictGapped only trims per account by gappedLifetime and stale nonce. Realistically depth 2 needs on-chain nonce >= 100 and depth 5 needs >= 10^5, and entries expire after one minute, so the overshoot is a bounded single-digit multiplier of the documented memory headroom rather than an unbounded DoS — but the enforced limit is not the documented one, which will misdirect any future tuning of maxGapped.

Suggested fix: either track a running total of buffered gapped transactions (or sum queue lengths) and compare that against maxGapped, or rename the constant to maxGappedAccounts, update its doc comment, and add a separate explicit total-transaction/byte cap.

Separately, the promotion and cleanup half of this feature is untested. The only test touching the gapped queue is TestAdd's eve case, which exercises the buffering branch and stops there:

"eve": {balance: 21100 + blobSize, nonce: 10}, // High nonce to test gapped acceptance
...
{ // Old account, 10 txs in chain: 0 pending: accept nonce 11 as gapped
	from: "eve",
	tx:   makeUnsignedTx(11, 1, 1, 1),
	err:  nil,
	check: func(pool *BlobPool, tx *types.Transaction) bool {
		return pool.Status(tx.Hash()) == txpool.TxStatusQueued
	},
},

No test ever adds the gap-filling nonce, so the promotion loop, the stale tx.Nonce() < stateNonce drop branch, delete(p.gappedSource, ...) and the p.gapped[from] = gtxs write-back never execute:

if gtxs, ok := p.gapped[from]; checkGapped && ok && len(gtxs) > 0 {
	sort.SliceStable(gtxs, func(i, j int) bool { return gtxs[i].Nonce() < gtxs[j].Nonce() })
	for len(gtxs) > 0 {
		stateNonce := p.state.GetNonce(from)
		firstgap := stateNonce + uint64(len(p.index[from]))
		if gtxs[0].Nonce() > firstgap { break }
		tx := gtxs[0]; gtxs[0] = nil; gtxs = gtxs[1:]
		delete(p.gappedSource, tx.Hash())
		if tx.Nonce() < stateNonce { continue }
		if tx.Nonce() <= firstgap {
			if err := p.addLocked(tx, false); err == nil { ... } else { ... }
		}
	}
	if len(gtxs) == 0 { delete(p.gapped, from) } else { p.gapped[from] = gtxs }
}

A defect here — the write-back clobbering entries appended by the nested addLocked(tx, false), an off-by-one in the firstgap comparison, or a leaked p.gappedSource entry — would not be caught by CI, and would surface as gap-filled blob txs silently expiring instead of becoming pending.

Suggested test: func TestGappedPromotion(t *testing.T) — seed an account at state nonce 10 with sufficient balance; pool.Add(tx(nonce=11)) and assert err == nil, Status == txpool.TxStatusQueued, len(pool.gapped[addr]) == 1; then pool.Add(tx(nonce=10)) and assert Status(tx11) == txpool.TxStatusPending, pool.gapped has no entry for addr, pool.gappedSource has no entry for tx11.Hash(), and verifyPoolInternals(t, pool) passes. Add companion cases for the stale-drop branch and for evictGapped after gappedLifetime. Also add a case that fills maxGapped accounts, each with a state nonce high enough for allowance > 1, and asserts the summed len(p.gapped[addr]) stays within the intended global bound.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accurate reading of the code — len(p.gapped) is an account count and the constant's comment promises a transaction count — but it isn't reachable on Bor.

Bor never constructs a blobpool: blobpool.New( has no non-test callers, and eth/backend.go registers only []txpool.SubPool{legacyPool}. The whole package is inherited-but-unwired, so no blob transaction reaches addLocked. The code is also upstream's (geth #32717), not ours.

So not fixed here — patching upstream code mid-sync creates permanent divergence. Recording it in needs-wiring.md so it's caught if blobs are ever wired up, and it's worth an upstream report.

Base automatically changed from ppatil-upstream-v1.16.9 to upstream-merge-v1.17.4 August 17, 2026 03:47
pratikspatil024 added a commit that referenced this pull request Aug 20, 2026
Both were surfaced by AI review on #2319 and verified: the binary-trie witness
key derivation disagrees with the storage read/write path for any slot >= 256,
and the blobpool's gapped-transaction cap bounds sender addresses rather than
transactions. Neither is ours — both files are byte-identical to upstream
v1.17.0 — and neither is reachable on Bor today, since VerkleBlock is nil on
every preset and the blobpool is never constructed. Recording them so they are
gates on enabling those paths rather than silent gaps, instead of patching
upstream code inside a sync merge.
@cffls

cffls commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Any idea why e2e test failed?

@pratikspatil024

pratikspatil024 commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

@cffls somehow GitHub is malfunctioning here. The CI on the PR is not showing the latest run.
If you check the CI against the latest commit on this PR, the tests are passing.
Screenshot 2026-08-24 at 9 47 35 AM

Comment thread core/vm/instructions.go
}
size.SetBytes32(h[:])
return nil, nil
hash = cached.(common.Hash)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this used to reuse a hasher off the EVM struct (no allocs), but now it calls crypto.Keccak256Hash which allocates a bit each time it's not a cache hit. just mentioning as this is probably one of the opcodes we use the most, but might not a real issue at our throughput. wdyt?

Comment thread rpc/http.go
ctx = context.WithValue(ctx, peerInfoContextKey{}, connInfo)

// Extract trace context from incoming headers.
ctx = otel.GetTextMapPropagator().Extract(ctx, propagation.HeaderCarrier(r.Header))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ledger declines otel tracing for the other rpc files (client/handler/server/service.go) but this still parses otel headers off every incoming RPC request. probably not an issue today since nothing consumes the span, but flagging it to know if there's a reason to keep it here specifically?

Comment thread core/vm/eips.go
4762: enable4762,
7702: enable7702,
7939: enable7939,
8024: enable8024,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have no tests for this, right? I mean nothing would fail in the CI if enable8024 (or others) get wired into an active preset. Maybe worth having a table test.

TxIndex int
Incarnation int // bumped on re-execution for validation
base *SafeBase // thread-safe pre-block state reads
rawBase *StateDB // raw base for PointCache/Witness only

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't find this PointCache anymore, maybe we want to update the comment here

Comment thread node/rpcstack.go
h.server = &http.Server{Handler: h}
h.server.Protocols = new(http.Protocols)
h.server.Protocols.SetHTTP1(true)
h.server.Protocols.SetUnencryptedHTTP2(true)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had agents flagging this:

This turns on HTTP/2 without encryption for our RPC/engine-api endpoints. If whatever sits in front of these (nginx, etc.) doesn't handle that combination carefully, there's a known way for someone to sneak a second hidden request past the proxy straight to bor. No idea if that actually applies to how we've got things set up, just flagging since I haven't seen anyone check it. Might be totally fine.

Maybe worth a check

Comment thread core/state/database.go
return trie.NewVerkleTrie(root, db.triedb, db.pointCache)
// Use BinaryTrie instead of VerkleTrie when IsVerkle is set
// (IsVerkle actually means Binary Trie mode in this codebase)
return bintrie.NewBinaryTrie(root, db.triedb)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this returns a *bintrie.BinaryTrie now, but mustCopyTrie below still doesn't handle BinaryTrie, right? I fear once this path is active, the StateDB.Copy() can panic. Shall we add the BinaryTrie.Copy() case too?

Comment thread internal/ethapi/api.go
// SendRawTransactionSync will add the signed transaction to the transaction pool
// and wait until the transaction has been included in a block and return the receipt, or the timeout.
func (api *TransactionAPI) SendRawTransactionSync(ctx context.Context, input hexutil.Bytes, timeoutMs *hexutil.Uint64) (map[string]interface{}, error) {
func (api *TransactionAPI) SendRawTransactionSync(ctx context.Context, input hexutil.Bytes, timeoutMs *uint64) (map[string]interface{}, error) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this change intentional? I mean callers now can pass a normal quantity string for the timeout and hit invalid params

Comment thread core/evm.go
ctx := vm.TxContext{
Origin: msg.From,
GasPrice: new(big.Int).Set(msg.GasPrice),
GasPrice: uint256.MustFromBig(msg.GasPrice),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this panic before the normal fee-cap sanity errors? The block import builds the tx context before preCheck, where the relative errors are returned, so an oversized gas price from a bad block might panic the node instead of rejecting cleanly, no?

// Internal shuffle shouldn't touch the lookup set.
pool.enqueueTx(hash, tx, false)
}
pool.priced.Removed(len(olds) + len(drops))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should conditional removals be included too? FilterTxConditional removes txs from pending, but doesn’t subtract them from list.totalcost, so ExistingExpenditure can stay inflated after a conditional tx expires.

@cffls cffls left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from my side! One minor question about state db change.

Comment thread core/state_processor.go
// Mutate the block and state according to any hard-fork specs
if config.DAOForkSupport && config.DAOForkBlock != nil && config.DAOForkBlock.Cmp(block.Number()) == 0 {
misc.ApplyDAOHardFork(statedb)
misc.ApplyDAOHardFork(tracingStateDB)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do the same change for parallel_state_processor?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.