all: sync with go-ethereum v1.17.2 (upstream merge 4/6) - #2328
all: sync with go-ethereum v1.17.2 (upstream merge 4/6)#2328pratikspatil024 wants to merge 87 commits into
Conversation
ethereum/go-ethereum#33916 + cmd/keeper go mod tidy --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
In `buildPayload()`, the background goroutine uses a `select` to wait on the recommit timer, the stop channel, and the end timer. When both `timer.C` and `payload.stop` are ready simultaneously, Go's `select` picks a case non-deterministically. This means the loop can enter the `timer.C` case and perform an unnecessary `generateWork` call even after the payload has been resolved. Add a non-blocking check of `payload.stop` at the top of the `timer.C` case to exit immediately when the payload has already been delivered.
Return the Amsterdam instruction set from `LookupInstructionSet` when `IsAmsterdam` is true, so Amsterdam rules no longer fall through to the Osaka jump table. --------- Co-authored-by: rjl493456442 <garyrong0905@gmail.com>
For bal-devnet-3 we need to update the EIP-8024 implementation to the latest spec changes: ethereum/EIPs#11306 > Note: I deleted tests not specified in the EIP bc maintaining them through EIP changes is too error prone.
Pebble maintains a batch pool to recycle the batch object. Unfortunately batch object must be explicitly returned via `batch.Close` function. This PR extends the batch interface by adding the close function and also invoke batch.Close in some critical code paths. Memory allocation must be measured before merging this change. What's more, it's an open question that whether we should apply batch.Close as much as possible in every invocation.
Implements https://eips.ethereum.org/EIPS/eip-7778 --------- Co-authored-by: Gary Rong <garyrong0905@gmail.com>
Mainnet was already overriding --cache to 4096. This PR just makes this the default.
…#33927) The BatchSpanProcessor queue size was incorrectly set to DefaultMaxExportBatchSize (512) instead of DefaultMaxQueueSize (2048). I noticed the issue on bloatnet when analyzing the block building traces. During a particular run, the miner was including 1000 transactions in a single block. When telemetry is enabled, the miner creates a span for each transaction added to the block. With the queue capped at 512, spans were silently dropped when production outpaced the span export, resulting in incomplete traces with orphaned spans. While this doesn't eliminate the possibility of drops under extreme load, using the correct default restores the 4x buffer between queue capacity and export batch size that the SDK was designed around.
…789) closes #32741
Fixes a regression in #33593 where a block gas limit > gasCap resulted in more execution than the gas cap.
Eth currently has a flaky test, related to the tx fetcher. The issue seems to happen when Unsubscribe is called while sub is nil. It seems that chain.Stop() may be invoked before the loop starts in some tests, but the exact cause is still under investigation through repeated runs. I think this change will at least prevent the error.
The computation of `MAIN_STORAGE_OFFSET` was incorrect, causing the last byte of the stem to be dropped. This means that there would be a collision in the hash computation (at the preimage level, not a hash collision of course) if two keys were only differing at byte 31.
…le tree (#33961) This is an optimization that existed for verkle and the MPT, but that got dropped during the rebase. Mark the nodes that were modified as needing recomputation, and skip the hash computation if this is not needed. Otherwise, the whole tree is hashed, which kills performance.
`GenerateChain` commits trie nodes asynchronously, and it can happen that some nodes aren't making it to the db in time for `GenerateChain` to open it and find the data it is looking for.
Add nil checks to prevent potential panics when keystore backend is unavailable in the Clef signer API.
Reduce allocations in calculation of tx cost. --------- Co-authored-by: weixie.cui <weixie.cui@okg.com> Co-authored-by: Sina M <1591639+s1na@users.noreply.github.com>
Updates go-eth-kzg to https://github.com/crate-crypto/go-eth-kzg/releases/tag/v1.5.0 Significantly reduces the allocations in VerifyCellProofBatch which is around ~5% of all allocations on my node --------- Co-authored-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com>
I observed failing tests in Hive `engine-withdrawals`: - https://hive.ethpandaops.io/#/test/generic/1772351960-ad3e3e460605c670efe1b4f4178eb422?testnumber=146 - https://hive.ethpandaops.io/#/test/generic/1772351960-ad3e3e460605c670efe1b4f4178eb422?testnumber=147 ```shell DEBUG (Withdrawals Fork on Block 2): NextPayloadID before getPayloadV2: id=0x01487547e54e8abe version=1 >> engine_getPayloadV2("0x01487547e54e8abe") << error: {"code":-38005,"message":"Unsupported fork"} FAIL: Expected no error on EngineGetPayloadV2: error=Unsupported fork ``` The same failure pattern occurred for Block 3. Per Shanghai engine_getPayloadV2 spec, pre-Shanghai payloads should be accepted via V2 and returned as ExecutionPayloadV1: - executionPayload: ExecutionPayloadV1 | ExecutionPayloadV2 - ExecutionPayloadV1 MUST be returned if payload timestamp < Shanghai timestamp - ExecutionPayloadV2 MUST be returned if payload timestamp >= Shanghai timestamp Reference: - https://github.com/ethereum/execution-apis/blob/main/src/engine/shanghai.md#engine_getpayloadv2 Current implementation only allows GetPayloadV2 on the Shanghai fork window (`[]forks.Fork{forks.Shanghai}`), so pre-Shanghai payloads are rejected with Unsupported fork. If my interpretation of the spec is incorrect, please let me know and I can adjust accordingly. --------- Co-authored-by: muzry.li <muzry.li1@ambergroup.io>
This PR fixes a regression introduced in https://github.com/ethereum/go-ethereum/pull/33836/changes Before PR 33836, running mainnet would automatically bump the cache size to 4GB and trigger a cache re-calculation, specifically setting the key-value database cache to 2GB. After PR 33836, this logic was removed, and the cache value is no longer recomputed if no command line flags are specified. The default key-value database cache is 512MB. This PR bumps the default key-value database cache size alongside the default cache size for other components (such as snapshot) accordingly.
We got a report for a bug in the tracing journal which has the responsibility to emit events for all state that must be reverted. The edge case is as follows: on CREATE operations the nonce is incremented. When a create frame reverts, the nonce increment associated with it does **not** revert. This works fine on master. Now one step further: if the parent frame reverts tho, the nonce **should** revert and there is the bug.
This PR contains two changes: Firstly, the finalized header will be resolved from local chain if it's not recently announced via the `engine_newPayload`. What's more importantly is, in the downloader, originally there are two code paths to push forward the pivot point block, one in the beacon header fetcher (`fetchHeaders`), and another one is in the snap content processer (`processSnapSyncContent`). Usually if there are new blocks and local pivot block becomes stale, it will firstly be detected by the `fetchHeaders`. `processSnapSyncContent` is fully driven by the beacon headers and will only detect the stale pivot block after synchronizing the corresponding chain segment. I think the detection here is redundant and useless.
…3984) The `--remove.chain` flag incorrectly described itself as selecting "state data" for removal, which could mislead operators into removing the wrong data category. This corrects the description to accurately reflect that the flag targets chain data (block bodies and receipts).
This PR allows users to prune their nodes up to the Prague fork. It indirectly depends on #32157 and can't really be merged before eraE files are widely available for download. The `--history.chain` flag becomes mandatory for `prune-history` command. Here I've listed all the edge cases that can happen and how we behave: ## prune-history Behavior | From | To | Result | |-------------|--------------|--------------------------| | full | postmerge | ✅ prunes | | full | postprague | ✅ prunes | | postmerge | postprague | ✅ prunes further | | postprague | postmerge | ❌ can't unprune | | any | all | ❌ use import-history | ## Node Startup Behavior | DB State | Flag | Result | |-------------|--------------|----------------------------------------------------------------| | fresh | postprague | ✅ syncs from Prague | | full | postprague | ❌ "run prune-history first" | | postmerge | postprague | ❌ "run prune-history first" | | postprague | postmerge | ❌ "can't unprune, use import-history or fix flag" | | pruned | all | ✅ accepts known prune points |
This PR adds a cmd tool fetchpayload which connects to a node and gets all the information in order to create a serialized payload that can then be passed to the zkvm.
…r, needs-wiring, plan) Batch 18/19 ledger sections, the EIP-7708 fork-register row, the milestone-wide upstream-PR triage (77 commits) and the full-gate results. Triage surfaced one item needing a team decision: upstream's default cache bump (#33836/#33975) merges clean but never reaches bor server, which computes its caches from internal/cli/server's own Cache: 1024 split. Recorded in needs-wiring.md rather than taken -- it would quadruple the default per-node memory footprint.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (54.84%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## ppatil-upstream-v1.17.1 #2328 +/- ##
===========================================================
+ Coverage 54.28% 54.30% +0.02%
===========================================================
Files 915 919 +4
Lines 165107 165809 +702
===========================================================
+ Hits 89624 90049 +425
- Misses 69866 70102 +236
- Partials 5617 5658 +41
... and 23 files with indirect coverage changes
🚀 New features to boost your workflow:
|
Develop-drift cascade: carries develop's #2347 (Kurtosis e2e and stateless-e2e on every PR base) and #2333 (complete witnesses under BlockSTM v2) up the stack. This is the first hop that carries upstream #34106, so it is where develop's witness code needs adapting; the hops above inherit these resolutions. Witness surface — one conflict and two silent API breaks. StateDB's witness field block conflicted. The merge base carried both `witness` and `witnessStats`; upstream #34106 relocated witness statistics out of StateDB and into `stateless.Witness` (gated by `VmConfig.EnableWitnessStats`, reported via `Witness.ReportMetrics`), so this side had already dropped `witnessStats` and swept its ~25 call sites. develop kept the field and added `witnessPrewalkStop`. Resolved as the plain three-way answer: this side's removal plus develop's addition. Keeping `witnessStats` was considered and rejected. Post-#34106 `StartPrefetcher` no longer takes a stats argument, so the field would have had no writer and no reader — dead weight rather than preserved behaviour. The metric itself is unaffected: `AddState(nodes, owner)` receives exactly the owner develop passed to `witnessStats.Add`, so attribution is identical, and because the relocated collector counts inside `AddState` it also covers the prewalk path that develop's design left uninstrumented. Consequence worth knowing when comparing dashboards across branches: the witness-stats counters read higher here than on develop, because prewalk-collected nodes are now counted. Two further breaks were invisible to git and only the compiler found them, both in develop's new tests: `stateless.NewWitness` gained the `enableStats` parameter (passed false — this test regenerates and compares witness content, which stats do not affect), and `newTrieReader` lost its point-cache parameter (the `nil` argument dropped). Verified: build clean; `go vet ./core ./core/state` clean apart from the pre-existing `parallel_state_processor.go:341` lock-copy finding; gofmt clean. All seven of #2333's prewalk and read-set tests pass, including TestCollectStateWitnessIncludesFlatServedReads. Both witness regeneration tests pass, with the full sweep round-tripping 241/241 real mainnet blocks and no skips — so the fix is verified working here, not merely present.
Ancestry only. The newTrieReader point-cache fix this carries was already present here — it was made at this hop originally, and the two branches below have only now caught up — so the merge records the relationship without changing a byte. That is the reason it exists. Without it this branch would not contain its predecessor, and a stacked pull request whose head does not contain its base misreports its own diff and turns an eventual merge into an argument. Deliberately not re-verified, because there is nothing new to verify: the merge result's tree is identical to this branch's previous tree, which is the tree that already passed build, full-tree vet, #2333's prewalk and read-set tests, and CI. A merge with no tree delta cannot break what that tree established.
…develop-drift cascade)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
|
@claude review |
|
codegenie review |
🧞 Codegenie ReviewWarning 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.
CoveragePartial review: 189 hunks were not reviewed because budget was exhausted before dispatch.
|
There was a problem hiding this comment.
Pull request overview
Upstream-merge milestone syncing Bor’s fork with go-ethereum v1.17.2, bringing in a large set of upstream changes across trie/pathdb history handling, freezer/ancients behavior, EVM gas accounting plumbing (Amsterdam-gated), p2p dialing/discovery improvements, and related test/CLI adjustments.
Changes:
- Introduces new/updated primitives for gas accounting (
core.GasPool), receipt cumulative gas tracking, and Amsterdam-gated behaviors (incl. EIP-7708 surfaces, EIP-7954 limits). - Expands and hardens storage/history subsystems: pathdb history repair/indexing, freezer tail truncation behavior, batch resource closing, and verkle/bintrie hashing improvements.
- Updates p2p and RPC/user-facing tooling: avoid outbound dials to pending inbound peers, resolve bootnode hostnames, add RPC limits and error behaviors, and extend pruning history modes.
Reviewed changes
Copilot reviewed 154 out of 155 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| triedb/pathdb/reader.go | Historic reader error handling change |
| triedb/pathdb/history.go | History repair truncation logic |
| triedb/pathdb/history_indexer_test.go | Indexer test updated for new args |
| triedb/pathdb/history_indexer_state.go | New sync-state tracker for indexer |
| triedb/pathdb/database.go | Pass new index-delay config |
| triedb/pathdb/database_test.go | Enable no-delay indexing in tests |
| triedb/pathdb/config.go | Add NoHistoryIndexDelay config |
| triedb/pathdb/buffer.go | Close batch after flush |
| trie/trie_test.go | Add Close to batch test stub |
| trie/levelstats.go | Add LevelStats deep copy |
| trie/bintrie/trie.go | Deserialize node with provided hash |
| trie/bintrie/stem_node.go | Cache hash + pooled sha256 usage |
| trie/bintrie/stem_node_test.go | Ensure recompute flag in hash test |
| trie/bintrie/key_encoding.go | Rework key derivation + overflow handling |
| trie/bintrie/iterator.go | Handle nil resolved nodes + backtracking |
| trie/bintrie/internal_node.go | Cached hash + parallel hashing |
| trie/bintrie/internal_node_test.go | Recompute flag adjustments in tests |
| trie/bintrie/hasher.go | New sha256 hasher pool |
| trie/bintrie/hashed_node.go | Deserialize with hash parameter |
| trie/bintrie/empty.go | Ensure new stem nodes marked dirty |
| trie/bintrie/binary_node.go | Add DeserializeNodeWithHash API |
| tests/state_test_util.go | Use NewGasPool in state tests |
| tests/bor/helper.go | Adjust ApplyTransaction signature + gaspool |
| signer/core/uiapi.go | Avoid extra allocs; keystore nil check |
| rlp/raw.go | Add RawList.AppendList |
| rlp/raw_test.go | Tests for RawList.AppendList |
| rlp/encode_test.go | Add EncoderBuffer.Size test |
| rlp/encbuffer.go | Implement EncoderBuffer.Size |
| params/protocol_params.go | Add Amsterdam code size + system log topics |
| p2p/server.go | Track pending inbound IDs for dialer |
| p2p/discover/table.go | Resolve bootnode hostnames via DNS |
| p2p/discover/table_test.go | Test DNS hostname fallback resolution |
| p2p/dial.go | Avoid dialing nodes with pending inbound |
| p2p/dial_test.go | Test pending-inbound dial suppression |
| miner/worker.go | GasPool API updates; prefetcher signature updates |
| miner/worker_test.go | Update gaspool init |
| miner/worker_prefetch_unit_test.go | Update gaspool init |
| miner/payload_building.go | Prefer stop signal before timer work |
| internal/ethapi/simulate.go | Use NewGasPool + cumulative gas tracking |
| internal/ethapi/errors.go | Map initcode error to vm package |
| internal/ethapi/bor_api_test.go | Update witness constructor signature |
| internal/ethapi/api.go | Add getProof key cap; gaspool nil handling; slotNumber encoding |
| go.sum | Dependency checksum updates |
| go.mod | Bump go-eth-kzg and karalabe/hid |
| ethdb/pebble/pebble.go | Implement batch.Close |
| ethdb/memorydb/memorydb.go | Implement batch.Close (noop) |
| ethdb/leveldb/leveldb.go | Implement batch.Close (noop) |
| ethdb/batch.go | Add Close() to Batch interface |
| eth/tracers/tracers_test.go | ApplyMessage now accepts nil gaspool |
| eth/tracers/logger/access_list_tracer.go | Stable/safe storageKeys marshaling |
| eth/tracers/internal/tracetest/prestate_test.go | ApplyMessage nil gaspool update |
| eth/tracers/internal/tracetest/flat_calltrace_test.go | ApplyMessage nil gaspool update |
| eth/tracers/internal/tracetest/erc7562_tracer_test.go | ApplyMessage nil gaspool update |
| eth/tracers/internal/tracetest/calltrace_test.go | ApplyMessage nil gaspool update |
| eth/tracers/api.go | Use nil gaspool; traceTx usedGas via GasPool |
| eth/tracers/api_test.go | ApplyMessage nil gaspool update |
| eth/state_accessor.go | ApplyMessage nil gaspool update |
| eth/protocols/wit/peer_test.go | Update witness constructor signature |
| eth/peer_test.go | Update witness APIs + AddState signature |
| eth/handler_wit_test.go | Update witness constructor signature |
| eth/gasestimator/gasestimator.go | ApplyMessage nil gaspool update |
| eth/filters/filter.go | Error on inverted block range |
| eth/filters/filter_test.go | Update test expectation for range error |
| eth/filters/api.go | Subscription setup cleanup; pruned history guard |
| eth/fetcher/witness_manager_test.go | Update witness constructor signature |
| eth/fetcher/block_fetcher_test.go | Update witness constructor signature |
| eth/fetcher/block_fetcher_race_test.go | Update witness constructor signature |
| eth/ethconfig/config.go | Update cache split defaults |
| eth/catalyst/simulated_beacon.go | Amsterdam payload version + slot number |
| eth/catalyst/api.go | Fetch unknown forkchoice head; finalized fallback |
| eth/catalyst/api_test.go | Allow GetPayloadV2 pre-Shanghai |
| docs/upstream-merges/v1.17.4/plan.md | Mark v1.17.2 batches merged |
| docs/upstream-merges/v1.17.4/fork-register.md | Record new fork/EIP surfaces |
| core/vm/jump_table_export.go | Add Amsterdam instruction set export |
| core/vm/interpreter_test.go | Transfer signature includes rules |
| core/vm/interface.go | Add EmitLogsForBurnAccounts to StateDB |
| core/vm/instructions.go | EIP-7708 selfdestruct logs; EIP-8024 decoding changes |
| core/vm/instructions_test.go | Update EIP-8024 execution tests |
| core/vm/gas_table.go | Use initcode-size helper check |
| core/vm/gas_table_test.go | Update Transfer signature; config selection |
| core/vm/evm.go | Transfer now receives rules |
| core/vm/dispatch_test.go | Update Transfer signature in tests |
| core/vm/dispatch_bench_test.go | Update Transfer signature in benches |
| core/vm/common.go | Add CheckMax(Code |
| core/v2_witness_regen_test.go | Update witness constructor signature |
| core/v2_serial_parity_fuzz_test.go | GasPool API updates |
| core/v2_selfdestruct_self_beneficiary_test.go | GasPool API updates |
| core/v2_pre_exec_system_call_test.go | GasPool API updates |
| core/v2_metamorphic_parity_test.go | GasPool API updates |
| core/v2_blockstm_test.go | GasPool API updates |
| core/v1_differential_test.go | GasPool API updates |
| core/types/transaction.go | Reduce allocations in Cost() |
| core/types/log.go | Add EIP-7708 log constructors |
| core/txpool/validation.go | Use vm initcode-size helper |
| core/txpool/legacypool/legacypool.go | Simplify Get; use types.Sender helper |
| core/tracing/journal.go | Fix nonce journaling across frame reverts |
| core/tracing/journal_test.go | Add parent-revert nonce test |
| core/tracing/hooks.go | Comment/name adjustments |
| core/stateless/witness.go | Optional stats + AddState(owner) |
| core/stateless/stats.go | Add WitnessStats copy |
| core/stateless/encoding.go | Export FromExtWitness |
| core/stateless/database_test.go | Update witness constructor signature |
| core/state/statedb_hooked.go | Forward EmitLogsForBurnAccounts |
| core/state/state_object.go | Skip redundant verkle trie commits |
| core/state/reader.go | Hash inputs via slices; witness owner passed |
| core/state/parallel_statedb.go | Implement EmitLogsForBurnAccounts |
| core/state_transition.go | Nil gaspool allowed; return gas via GasPool.ReturnGas; burn-log hook |
| core/state_processor.go | Receipt cumulative gas via GasPool; ApplyTransaction signature |
| core/state_prefetcher.go | GasPool API updates |
| core/state_prefetcher_intermediate_root_test.go | GasPool API updates |
| core/rawdb/table.go | Batch.Close passthrough |
| core/rawdb/freezer.go | Track head vs tail; tail-over-head handling |
| core/rawdb/freezer_utils.go | Atomic rename + reset helper; spelling fix |
| core/rawdb/freezer_utils_windows.go | Windows syncDir no-op |
| core/rawdb/freezer_utils_unix.go | Unix syncDir implementation |
| core/rawdb/freezer_table.go | Tail truncation reset + sync refactor |
| core/rawdb/freezer_table_test.go | Add tail-over-head randomized op + test |
| core/rawdb/freezer_resettable.go | Align head field rename usage |
| core/rawdb/freezer_memory.go | Tail-over-head reset behavior |
| core/rawdb/database.go | Tighten key-length checks in inspector |
| core/rawdb/ancienttest/testsuite.go | Add tail-over-head write test |
| core/parallel_state_processor.go | GasPool API + prefetcher signature update |
| core/parallel_state_processor_review_test.go | Prefetcher signature update |
| core/mainnet_witness_benchmark_test.go | Witness/gaspool API updates |
| core/history/historymode.go | Add postprague mode + prune-point lookup |
| core/genesis_test.go | Update verkle genesis root expectation |
| core/gaspool.go | New GasPool struct + APIs |
| core/evm.go | Transfer emits EIP-7708 log under rules |
| core/error.go | Add ErrGasLimitOverflow; remove initcode error |
| core/chain_makers.go | GasPool API updates; triedb lifetime fix |
| core/blockchain_test.go | Update witness constructor signature |
| console/console.go | Allow digit ‘0’ in autocomplete parsing |
| consensus/bor/bor.go | Prefetcher signature update |
| cmd/utils/flags.go | Cache default to 4096; history mode string update |
| cmd/keeper/stubs.go | Update build tags for womir |
| cmd/keeper/getpayload_womir.go | Add WOMIR wasm input reader |
| cmd/keeper/getpayload_wasm.go | Exclude womir from wasm build |
| cmd/keeper/getpayload_example.go | Add legacy +build line |
| cmd/geth/main.go | Remove mainnet cache bump logic |
| cmd/geth/dbcmd.go | Fix remove.chain usage text |
| cmd/geth/chaincmd.go | Extend prune-history to select mode |
| cmd/evm/internal/t8ntool/transaction.go | Use vm initcode-size helper |
| cmd/evm/internal/t8ntool/execution.go | GasPool API updates; remove request prefix stripping |
| cmd/devp2p/internal/v5test/framework.go | Decode using stable remoteAddr |
| build/ci.go | Add womir build; fix flags/dirs |
| accounts/abi/bind/backends/simulated.go | Use NewGasPool for contract calls |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| // CumulativeUsed returns the amount of cumulative consumed gas (refunded included). | ||
| func (gp *GasPool) CumulativeUsed() uint64 { | ||
| return gp.cumulativeUsed | ||
| } |
| meta, err := readTrienodeMetadata(db.trienodeFreezer, *id+1) | ||
| if err != nil { | ||
| return nil, err // e.g., the referred trienode history has been pruned | ||
| return nil, fmt.Errorf("state %#x is not available", root) // e.g., the referred trienode history has been pruned | ||
| } | ||
| if meta.parent != root { | ||
| return nil, fmt.Errorf("state %#x is not canonincal", root) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧞 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.
Partial review: 189 hunks were not reviewed because budget was exhausted before dispatch.
Reviewed 195/386 hunks before stopping.
Coverage disclosure:
- Budget stopped review work (token limit reached).
- Verification incomplete for 13 candidates.
- planner degraded; deterministic default plan used
- go.sum: lockfile
- semantic composition skipped; deterministic fallback used
🙋 Needs human attention:
- Does every path that calls dialsched.inboundPending(id) in p2p/server.go guarantee a matching inboundCompleted(id) call (including error/early-return and panic paths), so pendingInbound entries cannot leak and permanently block dials to that node via checkDial's errPendingInbound?
- Does EIP-8024's EXCHANGE immediate encoding really permit imm bytes 0x50 and 0x51 (which decodePair maps to pairs (14,16) and (14,15)), and is the missing pair (15,16) (would require imm 0x60, still rejected) intentional?
- Does pruneHistoryCommand (cmd/geth/chaincmd.go:206) register utils.ChainHistoryFlag in its Flags list, and does utils.ChainHistoryFlag exist with name "history.chain"?
- In this fork, is f.tail stored as an absolute (offset-inclusive) item number by repair()/validate() at open time, the same way f.head is (freezer.head.Add(offset) in NewFreezer)? If repair sets tail relative while TruncateTail stores an absolute tail, the new 'if f.head.Load() < tail { f.head.Store(tail) }' guard compares/stores mixed-basis values and could push head forward by up to
offsetitems. - Once initerState reaches stateStalled, update() never re-evaluates progress (the ticker branch continues on stateStalled), so canExit() in indexIniter.run stays permanently true even if sync later resumes. Is that the intended terminal state in upstream v1.17.x?
- Additional unresolved notes suppressed: 35
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
| } | ||
|
|
||
| // Encode payload as RLP (shared by "rlp" and "hex" formats). | ||
| rlpBytes, err := rlp.EncodeToBytes(payload) |
There was a problem hiding this comment.
Impact: The tool advertises writing a payload "in the format consumed by cmd/keeper", but rlp.EncodeToBytes(payload) dispatches to Witness.EncodeRLP, which emits the 3-field BorWitness form (context, headers, state) and never writes w.Codes. cmd/keeper decodes that same Payload with rlp.DecodeBytes; DecodeRLP's BorWitness branch sets w.Codes to an empty map, then hands the witness to core.ExecuteStateless. If stateless execution needs contract bytecode from Codes, keeper fails (exit 10) or produces a wrong root; the failure appears at consumption time, far from this tool. The json output is unaffected because it serializes ExtWitness with Codes populated, so the three formats produced by the same invocation carry different data.
A debugging/fixture tool that silently produces content-incomplete payload files wastes triage time: the rlp and hex artifacts differ from the json artifact for the same block, and the loss is only observable when keeper executes them. Either the tool should serialize an encoding that preserves codes, or the omission should be documented so users know rlp/hex payloads are not self-contained.
Evidence:
Changed code:
payload := Payload{
ChainID: chainID.Uint64(),
Block: block,
Witness: witness,
}
// Encode payload as RLP (shared by "rlp" and "hex" formats).
rlpBytes, err := rlp.EncodeToBytes(payload)core/stateless/encoding.go ↗ (Payload embeds *stateless.Witness, so rlp.EncodeToBytes dispatches to this custom encoder, which serializes only w.State and never w.Codes.):
// EncodeRLP serializes a witness as RLP using the canonical BorWitness 3-field
// format. Only state trie nodes are encoded; contract bytecodes are not
// included in the wire format.
func (w *Witness) EncodeRLP(wr io.Writer) error {
w.lock.RLock()
defer w.lock.RUnlock()
bw := &BorWitness{
Context: w.context,
Headers: w.Headers,
State: make([][]byte, 0, len(w.State)),
}
for node := range w.State {
...core/stateless/encoding.go ↗ (Confirms the round trip is lossy: a consumer re-decoding the .rlp/.hex file gets Codes == empty map, not the codes returned by debug_executionWitness.):
// When decoding BorWitness, State items are placed into w.State and w.Codes
// is left empty, since codes are not part of the BorWitness wire format.
func (w *Witness) DecodeRLP(s *rlp.Stream) error {
...
var bw BorWitness
if err := rlp.DecodeBytes(raw, &bw); err == nil {
w.context = bw.Context
w.Headers = bw.Headers
w.Codes = make(map[string]struct{})
...cmd/keeper/main.go ↗ (The named consumer decodes this exact RLP Payload and feeds the (Codes-empty) witness straight into ExecuteStateless.):
var payload Payload
rlp.DecodeBytes(input, &payload)
...
crossStateRoot, crossReceiptRoot, _, _, err := core.ExecuteStateless(chainConfig, vmConfig, payload.Block, payload.Witness, nil, nil, nil)cmd/fetchpayload/main.go ↗ (The json format serializes ExtWitness (which has a Codes field populated from the RPC response), so the three advertised formats are not equivalent in content.):
case "json":
data, err = marshalJSONPayload(chainID, block, &extWitness)Suggested fix: Confirm whether core.ExecuteStateless requires Witness.Codes for this Bor witness format. If it does, serialize a codes-preserving encoding for the rlp/hex outputs (e.g. encode the ExtWitness form, which DecodeRLP already accepts via its 5-field fallback) so the file round-trips. If codes are genuinely redundant because State already contains them, reconcile the contradictory BorWitness/EncodeRLP doc comments and note in the fetchpayload package doc that rlp/hex payloads omit the separate Codes list.
Suggested test: Round-trip test in cmd/fetchpayload: build a Payload whose Witness has a non-empty Codes set, run rlp.EncodeToBytes then rlp.DecodeBytes into a fresh Payload, and assert on Codes — either that it survives (after the fix) or, if the loss is intentional, assert it is empty and document the invariant so the asymmetry with the json output is pinned by a test.
| id := trie.StorageTrieID(root, common.BytesToHash(accIter.Key), acc.Root) | ||
|
|
||
| storageTrie, err := trie.NewStateTrie(id, triedb) | ||
| err := traverseStorage(trie.StorageTrieID(root, common.BytesToHash(accIter.Key), acc.Root), triedb, false, false) |
There was a problem hiding this comment.
Impact: Running geth snapshot traverse-state (no --account) still prints the "Traversing state"/"State is complete" summary lines, but slots is now permanently 0 because the extracted traverseStorage keeps its slot count local and returns only an error. Operators using the command to verify state completeness lose the storage-slot count and may read slots=0 as an empty/broken state.
The command's caller-visible output silently degrades: a previously meaningful diagnostic counter is now a constant zero, which can mislead pruning/state verification workflows. It is purely diagnostic (no state correctness impact), hence low severity, but it is an unintended regression of the refactor.
Evidence:
Changed code:
if acc.Root != types.EmptyRootHash {
err := traverseStorage(trie.StorageTrieID(root, common.BytesToHash(accIter.Key), acc.Root), triedb, false, false)
if err != nil {
return err
}
}cmd/geth/snapshot.go ↗ (Head-side read of traverseState (lines ~464-506) confirms slots is declared and printed in both progress and completion logs but is no longer incremented anywhere after the inlined storage loop was replaced by traverseStorage.):
var (
accounts int
slots int
codes int
...
)
...
log.Info("Traversing state", "accounts", accounts, "slots", slots, "codes", codes, ...)
...
log.Info("State is complete", "accounts", accounts, "slots", slots, "codes", codes, ...)cmd/geth/snapshot.go ↗ (traverseStorage counts slots in a function-local variable and returns only error, so the per-account slot count is not propagated back to traverseState.):
func traverseStorage(id *trie.ID, db *triedb.Database, report bool, detail bool) error {
...
slots += 1
...
return nil
}cmd/geth/snapshot.go ↗ (Base-side read shows the removed loop was the only place the outer slots counter was incremented.):
storageIter := trie.NewIterator(storageIt)
for storageIter.Next() {
slots += 1
...
}Suggested fix: Have traverseStorage return the slot (and optionally node) count, e.g. func traverseStorage(...) (int, error), and accumulate it in traverseState: n, err := traverseStorage(...); slots += n. Alternatively drop slots from traverseState's log lines so the output does not advertise a stale zero.
Suggested test: Add a cmd/geth test (or manual run) over a small state with at least one contract account holding storage, asserting the "State is complete" log line reports a non-zero slots value.
| } | ||
| for key, origin := range obj.uncommittedStorage { | ||
| value, exist := obj.pendingStorage[key] | ||
| if value == origin || !exist { |
There was a problem hiding this comment.
Impact: In verkle/binary-trie mode, IntermediateRoot now applies storage diffs inline instead of via (*stateObject).updateTrie. A key present in uncommittedStorage but absent from pendingStorage is silently skipped: the !exist case is folded into the same continue as the noop case, so the upstream log.Error("Storage slot is not found in pending area", ...) internal-consistency diagnostic never fires. The same loop also never increments s.StorageUpdated / s.StorageDeleted, so storageUpdatedMeter and storageDeletedMeter report zero storage activity for all verkle-mode blocks.
Control flow for the !exist case is identical (both implementations continue), so no root or storage value changes; the concrete impact is bounded to observability. Still, if a state-diff bug ever leaves a slot out of the pending area, verkle mode loses the only signal that would attribute the resulting root mismatch, and storage update/delete meters silently read zero for verkle nodes, which can mislead capacity and regression analysis.
Evidence:
Changed code:
for key, origin := range obj.uncommittedStorage {
value, exist := obj.pendingStorage[key]
if value == origin || !exist {
continue
}
if (value != common.Hash{}) {
if err := s.trie.UpdateStorage(addr, key[:], common.TrimLeftZeroes(value[:])); err != nil {
s.setError(err)
}
} else {
if err := s.trie.DeleteStorage(addr, key[:]); err != nil {
s.setError(err)
...core/state/state_object.go ↗ (Decisive callee branch (read in full, lines 344-390 of head): the bypassed implementation logs the missing-pending-slot invariant violation and increments StorageUpdated/StorageDeleted. The inlined verkle loop does neither.):
for key, origin := range s.uncommittedStorage {
value, exist := s.pendingStorage[key]
if value == origin {
continue
}
if !exist {
log.Error("Storage slot is not found in pending area", "address", s.address, "slot", key)
continue
}
if (value != common.Hash{}) {
if err := tr.UpdateStorage(s.address, key[:], common.TrimLeftZeroes(value[:])); err != nil { ... }
s.db.StorageUpdated.Add(1)
...core/state/statedb.go ↗ (Shows the dropped counters are reported to metrics meters (statedb.go:2085-2087), so the omission is an observability regression in verkle/binary-trie mode rather than dead state.):
storageUpdatedMeter.Mark(s.StorageUpdated.Load())
accountDeletedMeter.Mark(int64(s.AccountDeleted))
storageDeletedMeter.Mark(s.StorageDeleted.Load())Suggested fix: Restore parity with updateTrie() on the inlined path: split the !exist case back out with its log.Error("Storage slot is not found in pending area", "address", addr, "slot", key), and call s.StorageUpdated.Add(1) / s.StorageDeleted.Add(1) alongside the corresponding UpdateStorage/DeleteStorage calls.
Suggested test: Add a verkle-mode StateDB test that sets and clears storage slots on an account, runs IntermediateRoot, and asserts s.StorageUpdated and s.StorageDeleted match the number of non-zero writes and deletions (matching the counts the non-verkle updateTrie path produces for the same mutations).
| if db.RecordTransfer(sender, recipient, amount) { | ||
| db.SubBalance(sender, amount, tracing.BalanceChangeTransfer) | ||
| db.AddBalance(recipient, amount, tracing.BalanceChangeTransfer) | ||
| emitEthTransferLog(db, sender, recipient, amount, rules) | ||
|
|
||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
🔴 core.Transfer's V2 BlockSTM settlement path orders the EIP-7708 transfer log after Bor's 0x1010 LogTransfer, while the serial path (core/evm.go:213-224) orders them the other way — a receipt-root divergence between V1 and V2 execution for every non-self, non-zero value transfer once Amsterdam activates. This defeats the PR's own 'adaptation 1' claim that V1/V2 log order is preserved, and should be fixed alongside this PR's EIP-7708 wiring (e.g. by flushing s.logs[tr.LogIdx] in ParallelStateDB.tryEmitTransferAt before emitting the Bor log, or changing the flush boundary to <= tr.LogIdx) rather than being discovered at Amsterdam enablement.
Extended reasoning...
What the bug is. core.Transfer (core/evm.go:196-231) calls db.RecordTransfer(sender, recipient, amount) first. On the serial StateDB this returns false and falls through to the pre-existing snapshot-based path, which calls emitEthTransferLog (the new EIP-7708 log) before AddTransferLog (Bor's existing 0x1010 LogTransfer). So the serial per-tx log order is [EIP-7708, Bor]. On ParallelStateDB (V2 BlockSTM), RecordTransfer returns true and short-circuits, but before doing so it captures TransferRecord.LogIdx = len(s.logs) — a snapshot taken before emitEthTransferLog appends anything. emitEthTransferLog is then called immediately after and appends the EIP-7708 log at exactly s.logs[LogIdx].\n\nThe code path that triggers it. At settlement, ParallelStateDB.tryEmitTransferAt (core/state/parallel_statedb_settle.go, pre-existing/untouched by this PR) flushes execution logs with index strictly less than tr.LogIdx into the final DB, then immediately calls emitTransferLog → TransferLogFn → AddTransferLog, emitting the Bor 0x1010 log. Only afterward — on the next transfer's preceding-log flush, or the trailing catch-all loop — does s.logs[tr.LogIdx] (the EIP-7708 log) get flushed into final.logs. So the V2 per-tx log order ends up [Bor, EIP-7708] — the reverse of serial.\n\nWhy existing code/tests don't catch it. The PR's own core/eth_transfer_logs_test.go only drives the serial GenerateChain path; nothing in this PR (or the pre-existing test suite) exercises V2 BlockSTM settlement with EIP-7708 enabled, so the divergence is invisible to CI. tryEmitTransferAt's flush boundary (< tr.LogIdx) was written before EIP-7708 existed and correctly assumed nothing was appended to s.logs between RecordTransfer and the balance ops; emitEthTransferLog breaks that assumption by inserting a log at exactly LogIdx while still logically 'part of' the same transfer as the deferred Bor log.\n\nWhy this matters. Receipts are built from finalDB.GetLogs(tx.Hash()) after settlement (core/parallel_state_processor.go), so the interleaved AddLog order during settlement is the per-tx receipt log order — it directly determines each log's Index, the receipt's bloom filter, and the RLP-encoded receipt, which feeds the receipt root. A serial node and a BlockSTM node would therefore compute different receipt roots for the same block once IsAmsterdam activates, for every non-self, non-zero value transfer through core.Transfer on any Bor-configured chain — this is a consensus split, the exact failure class state-security.md calls out ('parallel and sequential output must be byte-identical'). It is currently dormant only because AmsterdamBlock is nil on every shipped preset; nothing prevents it from firing the moment a preset sets that block.\n\nWhy the PR's stated safeguard doesn't hold. The PR description's 'adaptation 1' explicitly claims emission was placed 'at the same point relative to the balance change' on every transfer path specifically to keep V1/V2 log order in agreement, because the author correctly identified that V2 defers the Bor log to settlement. The flaw is that 'the same point relative to the balance change' is not the same as 'the same point relative to tryEmitTransferAt's flush boundary' — RecordTransfer captures LogIdx before the balance change and before emitEthTransferLog runs, so the newly-appended EIP-7708 log always lands exactly at the tr.LogIdx boundary that tryEmitTransferAt was written to exclude from the pre-Bor-log flush.\n\nHow to fix it. Either (a) change tryEmitTransferAt's pre-flush condition from < tr.LogIdx to <= tr.LogIdx so the EIP-7708 log flushes before the Bor log is emitted, or (b) explicitly flush s.logs[tr.LogIdx] immediately before calling emitTransferLog when Amsterdam is active. Either restores order parity with the serial path.\n\nStep-by-step proof. Consider a Bor chain with Amsterdam active and a plain transfer A→B for amount N (A≠B, N>0), processed via V2 BlockSTM: (1) core.Transfer calls db.RecordTransfer(A,B,N) on the ParallelStateDB; this captures LogIdx = len(s.logs) (say, 5) and returns true. (2) SubBalance(A,N) / AddBalance(B,N) run (no logs appended). (3) emitEthTransferLog runs, appending the EIP-7708 log at s.logs[5]. (4) At settlement, tryEmitTransferAt flushes s.logs[0..4] (strictly < 5) into final.logs, then calls emitTransferLog → AddTransferLog, which appends the Bor 0x1010 log directly into final.logs at the next index. (5) Only later (next transfer's flush, or the trailing loop) is s.logs[5] (the EIP-7708 log) flushed into final.logs, landing after the Bor log. Final receipt order: [..., Bor(0x1010), EIP-7708]. Now run the identical transaction through the serial StateDB: RecordTransfer returns false, so Transfer falls through the plain path: SubBalance/AddBalance, then emitEthTransferLog (appends EIP-7708 directly to s.logs), then AddTransferLog (appends Bor 0x1010 directly to s.logs). Receipt order: [..., EIP-7708, Bor(0x1010)]. The two receipts for the identical transaction have swapped log order, hence different log.Index values, different bloom filters, and different RLP encodings — different receipt roots.
| func readBytes() []byte { | ||
| hintInput() | ||
| byteLen := readWord() | ||
| numWords := (byteLen + 3) / 4 | ||
| data := make([]byte, numWords*4) | ||
| hintBuffer(unsafe.Pointer(&data[0]), numWords) | ||
| return data[:byteLen] | ||
| } |
There was a problem hiding this comment.
🟡 In cmd/keeper/getpayload_womir.go, readBytes() panics with an index-out-of-range on &data[0] when the WOMIR hint stream reports a zero-length item, because hintBuffer(unsafe.Pointer(&data[0]), numWords) evaluates data[0] unconditionally even though numWords==0 makes the call itself a no-op. This is new code added by this PR (womir keeper target, upstream #34079); guard the numWords == 0 case (e.g. return data[:byteLen] early) and skip the hintBuffer call. Low impact: reachable only via a malformed/empty hint item from the (trusted) proving host in this WASM guest tool, not on any bor node/consensus/RPC path.
Extended reasoning...
readBytes() in cmd/keeper/getpayload_womir.go:37-44 reads a length-prefixed item from the WOMIR hint stream:
func readBytes() []byte {
hintInput()
byteLen := readWord()
numWords := (byteLen + 3) / 4
data := make([]byte, numWords*4)
hintBuffer(unsafe.Pointer(&data[0]), numWords)
return data[:byteLen]
}When the hint stream reports byteLen == 0, numWords becomes 0, so data := make([]byte, 0) is an empty slice. The very next line, hintBuffer(unsafe.Pointer(&data[0]), numWords), evaluates data[0] as part of constructing the argument — and in Go, indexing a zero-length slice always performs a bounds check and panics with index out of range [0] with length 0, regardless of whether the resulting pointer is ever dereferenced by the callee. Taking the address of an out-of-bounds element does not sidestep the check.
The panic happens before hintBuffer is even called, and it happens for no benefit: with numWords == 0, hintBuffer would read zero words and be a complete no-op even if the call succeeded. So the &data[0] expression is simultaneously unnecessary (the callee does nothing with it) and unsafe (it panics on the empty-slice case).
readBytes()'s only caller in this file is getInput(), which is the entrypoint the womir-tagged cmd/keeper build uses to read the single RLP-encoded Payload (ChainID + Block + Witness) from the hint stream. In correct operation that payload is never zero-length, so the panic is not reachable via the intended protocol; it would only trigger if the (trusted) proving host ever fed an empty/malformed hint item. This is a WASM guest binary used by an offline proving tool (//go:build womir), not the bor node, not consensus, and not network-facing, so there is no node-crash or consensus-safety implication.
Step-by-step proof:
- Host writes a hint item with
byteLen = 0(e.g. malformed input, or an off-protocol call toreadBytes()). readWord()returns0, sonumWords := (0 + 3) / 4 == 0.data := make([]byte, 0*4)produces a zero-length, zero-capacity slice.- Evaluating the call
hintBuffer(unsafe.Pointer(&data[0]), 0)requires computing&data[0]first; Go's runtime bounds-check on slice indexing fires becauselen(data) == 0, and the goroutine panics withindex out of range [0] with length 0—hintBufferis never reached.
Fix: guard the empty case before touching data[0], e.g.:
func readBytes() []byte {
hintInput()
byteLen := readWord()
numWords := (byteLen + 3) / 4
data := make([]byte, numWords*4)
if numWords > 0 {
hintBuffer(unsafe.Pointer(&data[0]), numWords)
}
return data[:byteLen]
}This is a one-line, purely defensive fix; it doesn't change behavior for any non-empty item.
| ) | ||
| } | ||
| } | ||
|
|
||
| if rules.IsAmsterdam { | ||
| st.evm.StateDB.EmitLogsForBurnAccounts() | ||
| } | ||
| return &ExecutionResult{ | ||
| UsedGas: st.gasUsed(), | ||
| MaxUsedGas: peakGasUsed, |
There was a problem hiding this comment.
🟡 EmitLogsForBurnAccounts (state_transition.go:687-689, gated only on rules.IsAmsterdam) runs before the deferred Coinbase/burnt-contract fee credit on the serial engine but after that same credit on the V2 BlockSTM engine, because the two engines set noFeeBurnAndTip differently (ApplyMessageNoFeeBurnOrTip vs ApplyMessageNoFeeLog). If Coinbase or the burnt-contract address is itself a same-tx-created self-destructing contract, V2 can emit an EthBurnLog that serial does not, diverging receipts once Amsterdam activates. Fix by running EmitLogsForBurnAccounts at the same point relative to fee crediting on both engines (e.g. move the external serial-side credit in ApplyTransactionWithEVM to occur before the call to execute() finishes emitting logs, or defer EmitLogsForBurnAccounts symmetrically on V2).
Extended reasoning...
The bug. stateTransition.execute() credits the block's fee tip to Coinbase and the burn amount to the Bor burnt-contract address inside a if !st.noFeeBurnAndTip block (core/state_transition.go:651-658), then unconditionally calls st.evm.StateDB.EmitLogsForBurnAccounts() right afterward (core/state_transition.go:687-689), gated only on rules.IsAmsterdam. EmitLogsForBurnAccounts walks the tx's dirty/destructed accounts and emits an EIP-7708 EthBurnLog for any that still hold a non-zero balance — i.e. an account that self-destructed earlier in the tx but then received funds afterward.
Why the two engines disagree. Bor's serial processor (core/state_processor.go StateProcessor.Process → ApplyTransactionWithEVM) always calls ApplyMessageNoFeeBurnOrTip, which sets st.noFeeBurnAndTip = true (state_transition.go:250). That skips the crediting block inside execute() entirely — the credit is instead applied externally, after ApplyMessage returns, via evm.StateDB.AddBalance(result.BurntContractAddress, ...) and evm.StateDB.AddBalance(evm.Context.Coinbase, ...) in state_processor.go:248 and :251. So on the serial path, EmitLogsForBurnAccounts runs strictly before the credit lands.
The V2 BlockSTM path (core/parallel_state_processor.go v2Env.applyMessage → ApplyMessageNoFeeLog) sets only st.noFeeLog = true, leaving noFeeBurnAndTip = false (state_transition.go:242-246). That means the crediting block runs inline inside execute(), before EmitLogsForBurnAccounts is reached in the same call. FeeData.BalancesApplied = true (parallel_state_processor.go:759) confirms this inline credit is authoritative — settlement does not re-apply it.
Concrete walkthrough. Suppose a transaction pays a non-zero tip, and the block's Coinbase (or Bor's configured burnt-contract address) happens to be a contract that was created earlier in the same transaction and then self-destructs (EIP-6780 same-tx-creation semantics) before the tx ends:
- Serial:
execute()skips the credit block (noFeeBurnAndTip=true) →EmitLogsForBurnAccountssees the account's balance still zero → no burn log emitted.ApplyTransactionWithEVMcredits the tip/burn afterward, with no further log emission. - V2:
execute()runs the credit block inline (noFeeBurnAndTip=false) → the account's balance is now non-zero →EmitLogsForBurnAccountsemits anEthBurnLogfor it. - Result: V2's receipt has one more log than serial's receipt for the identical transaction — a receipt/log-root divergence between the two execution engines.
Why nothing catches this today. The PR was otherwise careful about this exact class of bug — its own description calls out placing emitEthTransferLog at the same point relative to the balance change on all three transfer paths specifically to avoid a V1/V2 log-order mismatch. EmitLogsForBurnAccounts just wasn't checked against the pre-existing noFeeBurnAndTip serial/V2 split when it was added, so it inherited the asymmetry.
Reachability — narrower than a first look suggests. A verifier raised a plausible refutation: EmitLogsForBurnAccounts only fires for an account that is selfDestructed with residual balance, and neither Coinbase nor the burnt-contract address can normally be a self-destructing contract. That's correct for the burnt-contract address — it's always a fixed, pre-existing deployed contract (params.BorMainnetChainConfig.Bor.BurntContract), never something created in the paying transaction, so that leg is realistically unreachable (would need a ~2^160 address preimage).
For Coinbase, however, the refutation's premise ("Coinbase is the validator's own signer EOA") is only true pre-Rio or when BorConfig.Coinbase is unset. Post-Rio, core/evm.go:53-57 (NewEVMBlockContext) resolves the fee-crediting beneficiary via chain.Config().Bor.CalculateCoinbase(header.Number.Uint64()), which returns a fixed, block-height-keyed governance config value (params.BorMainnetChainConfig.Bor.Coinbase, currently 0x7Ee41D8A25641000661B1EF5E6AE8A00400466B0 since block 77414656), not the block's actual author — falling back to the validator's own address only if that config value is the zero address. So on live mainnet/Amoy today the Coinbase address is also a long-lived, pre-existing address with real transaction history, and can't satisfy EIP-6780's "created in this very transaction" requirement either. The scenario is therefore not "a malicious validator points Coinbase at an address they control" (Coinbase isn't chosen per-block by the validator at all on Bor); it would require a future governance change of the Coinbase config value to a brand-new address whose CREATE2 preimage was pre-computed by whoever proposed that change, colluding to self-destruct a contract there in the very transaction that first uses it as coinbase. That is an even more contrived path to exploitation than the verifiers described, reinforcing (rather than fully refuting) the "essentially unreachable today" conclusion, without eliminating the underlying code defect.
The fix. Run EmitLogsForBurnAccounts at the same point relative to fee/burn crediting on both engines — e.g., have the serial path's external credit in ApplyTransactionWithEVM happen (or be accounted for) before log emission, or symmetrically defer EmitLogsForBurnAccounts on the V2 path to align with serial's externally-applied credit. Since IsAmsterdam is nil on every shipped preset, this has no effect on any current chain and isn't a merge blocker, but it should be fixed (or at least tracked in fork-register.md's Amsterdam enable-time checklist) before Amsterdam is ever activated, given the state-security invariant that serial and V2 BlockSTM output must be byte-identical.
| if isBorInternalCall(ctx) && isBorSystemTx(b.ChainConfig().Bor, args.To) { | ||
| globalGasCap = 0 | ||
| } | ||
| gp := new(core.GasPool) | ||
| gp := core.NewGasPool(globalGasCap) | ||
| if globalGasCap == 0 { | ||
| gp.AddGas(gomath.MaxUint64) | ||
| } else { | ||
| gp.AddGas(globalGasCap) | ||
| gp = core.NewGasPool(gomath.MaxUint64) | ||
| } | ||
| return applyMessage(ctx, b, args, state, header, timeout, globalGasCap, gp, &blockCtx, &vm.Config{NoBaseFee: true}, precompiles) | ||
| } |
There was a problem hiding this comment.
🟡 sweep:core.NewGasPool(globalGasCap)\s*\n\s*if globalGasCap == 0 — in doCall (internal/ethapi/api.go:963-966), a GasPool is unconditionally built from globalGasCap and then immediately discarded and rebuilt with gomath.MaxUint64 whenever globalGasCap == 0. Collapse to a single conditional: cap := globalGasCap; if cap == 0 { cap = gomath.MaxUint64 }; gp := core.NewGasPool(cap).
Extended reasoning...
What it is. doCall() in internal/ethapi/api.go (lines 963-966) always allocates a *GasPool from globalGasCap, then immediately checks if globalGasCap == 0 and, if true, throws that pool away and allocates a second one with gomath.MaxUint64. The first core.NewGasPool(globalGasCap) call is pure waste on that path — its result is never read before being overwritten.
Why it happens now. This is a mechanical side effect of the v1.17.2 GasPool refactor bundled in this PR (core/gaspool.go): the old GasPool was a uint64 built via new(core.GasPool) + AddGas(amount), a single allocation regardless of branching. The new NewGasPool(amount uint64) *GasPool constructor takes the amount up front, so code that used to conditionally call AddGas on one pool now conditionally constructs two pools, and the pre-conditional construction becomes dead work whenever the branch is taken.
When the branch is taken. globalGasCap is forced to 0 when isBorInternalCall(ctx) && isBorSystemTx(b.ChainConfig().Bor, args.To) — i.e. Bor-internal system-contract calls — or when the RPC gas cap is configured as unlimited (--rpc.gascap=0). By default geth/Bor ships a non-zero RPCGasCap, so in the common eth_call/eth_estimateGas case globalGasCap != 0 and the first allocation is actually the one used; the discard specifically hits the bor-internal-system-tx path and any deployment with an explicit unlimited gas cap. eth_estimateGas calls doCall repeatedly during its binary search, so on that uncommon-but-real path the redundant allocation is paid many times per RPC call.
Impact. GasPool is a small (3×uint64) heap-allocated struct, so the cost is a handful of bytes and one allocation — not performance-significant on its own. This is a code-quality/cleanup finding, not a correctness or security issue: no behavior changes, and gas accounting is identical either way.
Proof. Concrete walk-through: (1) globalGasCap arrives as 0 (either explicitly configured or forced by the bor-internal-system-tx check two lines above). (2) gp := core.NewGasPool(globalGasCap) executes, heap-allocating a *GasPool{remaining: 0, initial: 0}. (3) The if globalGasCap == 0 branch is entered. (4) gp = core.NewGasPool(gomath.MaxUint64) executes, heap-allocating a second *GasPool{remaining: MaxUint64, initial: MaxUint64} and overwriting the local variable gp — the first allocation from step 2 is now unreferenced and becomes garbage with no reader ever having observed it.
Fix. Compute the effective cap once, then construct the pool a single time:
cap := globalGasCap
if cap == 0 {
cap = gomath.MaxUint64
}
gp := core.NewGasPool(cap)This is behavior-identical (same resulting pool contents on every path) and removes the wasted allocation.
…develop-drift cascade)
Important
Reviewer guide — stacked PR 4 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 intodeveloponce, 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), andcodecov/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 indocs/upstream-merges/.Summary
Fourth milestone of the Bor ← go-ethereum sync to v1.17.4: takes upstream from v1.17.1 → v1.17.2 (77 first-parent commits, 4 batches of ~20). Stacked on #2325 (v1.17.1) — base is
ppatil-upstream-v1.17.1, so this PR's diff is exactly this milestone.Upstream release notes: https://github.com/ethereum/go-ethereum/releases/tag/v1.17.2
The headline is EIP-7708 (ETH transfers as logs), the first Amsterdam EIP whose content had to be reshaped rather than copied — Bor's transfer, selfdestruct and parallel-state paths all diverge from upstream's. Like every other upstream fork/EIP in this sync it is merged wired but dormant:
AmsterdamBlockis nil on every shipped preset, so no behavior changes on mainnet or Amoy.16783c167..be4dc0c4b(77 first-parent commits)Commits
00540f94609c78485177e7e5ad10a83ed542e23b0cbc21abb57b8fbe4dc0c4b682b4c3802bc495ff9Reviewing this PR
docs/upstream-merges/v1.17.4/ledger.mdis the review map — every non-trivial resolution is recorded there with its rationale. The sections added by this PR are the four## v1.17.2 batch N/4blocks, the milestone triage table, and the full-gate results.fork-register.mdcarries one row per fork surface;needs-wiring.mdcarries everything deliberately deferred.EIP-7708 — the three adaptations worth reviewer attention
Log ordering across Bor's two transfer paths. Upstream appends the log right after the balance change in
Transfer. Bor has two transfer functions and an early-return V2 BlockSTM fast path whose 0x1010LogTransferis generated later, at settlement. The emission was factored intoemitEthTransferLogand called at the same point relative to the balance change on all three paths (serial, V2 fast path,EthereumTransfer). Placing it afterAddTransferLogon the serial path — the natural reading of the diff — would order the pair one way under V1 and the other under V2, i.e. a receipt-root divergence the day Amsterdam is enabled.Selfdestruct without #32919. Upstream's burn/transfer branch reads
StateDB.IsNewContract, which arrives with the selfdestruct rework Bor declined. Rather than drop the branch, the signal is taken fromSelfDestruct6780's existing second return value (wasNewContract), which Bor's opcode handler was discarding. Verified across all four cases. Dropping it would not have been covered by the adoptedEmitLogsForBurnAccountssafety net — Bor zeroes the balance insideSelfDestruct6780, so the tx-boundary sweep skips the account and the burn log would vanish silently.ParallelStateDB.EmitLogsForBurnAccounts. Newvm.StateDBmethod; implemented against the parallel executor'sdestructedmap, address-sorted to match the serial executor's ordering.Enable-time consequence to note now: once Amsterdam activates, a plain value transfer on Bor emits both the EIP-7708 system log and Bor's existing 0x1010
LogTransfer. Bloom/receipt-size change, not a correctness problem — but a product decision that should be taken deliberately, not discovered at activation. Recorded infork-register.md.Executed tests
Full per-milestone gate at
682b4c380:go build ./...clean;go vet ./...clean bar the two pre-existing//nolintcopylocks.make lint(golangci-lint v2.11.4, repo config): 0 issues.gofmt -lclean;go mod tidyno-op.go test ./...: 148 packages pass. 4 fail, all pre-existing with no new test names —cmd/geth+cmd/devp2p/internal/ethtest(VEBLOP/non-Bor-genesis nil-deref),cmd/evm(t8n golden drift),core/vm(TestAbortDuringJumpinterrupt-timing flake). The latter two were re-baselined on a detached worktree at1abb57b8fand fail identically there.make test-integration:tests/borok, 628 s, 77.6% coverage. (Caveat recorded in the ledger: the./testsconsensus-fixture suite is a no-op in this repo —tests/testdatais absent and isn't a submodule, so those cases skip rather than run. Pre-existing.)smallpreset on an image built from this tip): 8/8 checks pass — ~1 s block production with validator/RPC in lockstep, 45–100 txs/block, 29StateSyncedlogs, span 3 active, checkpoint 6 submitted, zero errors in either client.EIP-7708 dormancy, proven on a live chain
The unit test proves the emission works with the gate forced on. The devnet proves it stays off. Over 50 blocks at 45–100 txs/block:
0xfffffffffffffffffffffffffffffffffffffffe(EIP-7708SystemAddress)0x0000000000000000000000000000000000001010(BorLogTransfer)The 2598 is what makes the 0 meaningful: thousands of value transfers ran through
core.Transferand none emitted a system log. A leaked gate, or a misplaced emission on any of the three transfer paths, would show a non-zero count.Milestone triage — one item needs a team decision
Beyond conflicts, all 77 upstream PRs were triaged for Bor wiring (a PR can merge perfectly clean and still leave Bor un-wired). Result: 1 needs wiring, 5 consensus-relevant (all dormant, in the register), 7 operator-visible but no wiring, 12 deferred, 52 inert.
The one: upstream's default cache bump 1024 → 4096 MB (#33836, #33975) merged clean but has no effect on
bor server.internal/cli/server/config.gocarries its ownCache: 1024with a 50/15/25/10 split whosecalcPercoverwrites all fourethconfigfields — and that split against 1024 MB reproduces geth's old defaults exactly (512/154/256/102), which is evidently how it was tuned. Socmd/gethand tests get upstream's new values while production Bor stays 4× below them.Parity is a one-line change (
Cache: 4096), deliberately not taken here — it quadruples the default per-node memory footprint, which is a PoS/devops call rather than a merge decision. pos-ops pinscacheper host in each BP'sbor/config.toml, so deployed BPs are unaffected either way; this only moves the default for operators who don't set it. Filed as a row inneeds-wiring.md.Deferred this milestone
Six whole-feature declines, each with rationale and a re-adoption path in
needs-wiring.md: #33931 (ExecuteConfig), #33816 (codedb/cachingDB), #33773 (miner OTel ctx-threading), #33648 (call-variant gas rework), #34036 (history-pruning policy), #33894 (history-import batching). Plus dependent declines that re-apply automatically when their parent lands: #34011, #34062, #34094, #33950, #33955, #33150.core/vm/{gas_table,operations_acl}.gonow carries four deferred upstream changes (#33281, #33637, #33450, #33648) plus blocked #32919, and batch 19 added a fifth entanglement — the EIP-7708 selfdestruct adaptation readsSelfDestruct6780's return value because #32919 is declined, so adopting #32919 later means revisiting that code rather than replacing it. Milestones 5 and 6 bring the gas-vector rework and gas budget into the same files. Worth a team discussion before v1.17.3.Rollout notes
IsAmsterdam, which is nil on every shipped preset. No fork block,params.Rulesfield, forkid input, chain preset or genesis file changed.eth_getProofkeys andeth_simulateV1blocks/calls, a newMaxUsedGasfield in theeth_simulateV1response,eth_getLogsnow erroring on an inverted block range instead of returning empty, andeth_createAccessListreturning[]rather thannullfor emptyStorageKeys.🤖 Generated with Claude Code