core, params, miner: merge geth v1.17.1 (v1.17.4 sync, milestone 3/6) - #2325
core, params, miner: merge geth v1.17.1 (v1.17.4 sync, milestone 3/6)#2325pratikspatil024 wants to merge 48 commits into
Conversation
Adds `--opcode.count=<file>` flag to `evm t8n` that writes per-opcode execution frequency counts to a JSON file (relative to `--output.basedir`). --------- Co-authored-by: MariusVanDerWijden <m.vanderwijden@live.de> Co-authored-by: Sina Mahmoodi <itz.s1na@gmail.com>
In src/ethereum/forks/amsterdam/vm/interpreter.py:299-304, the caller
address is
only tracked for block level accessList when there's a value transfer:
```python
if message.should_transfer_value and message.value != 0:
# Track value transfer
sender_balance = get_account(state, message.caller).balance
recipient_balance = get_account(state, message.current_target).balance
track_address(message.state_changes, message.caller) # Line 304
```
Since system transactions have should_transfer_value=False and value=0,
this condition is never met, so the caller (SYSTEM_ADDRESS) is not
tracked.
This condition is applied for the syscall in the geth implementation,
aligning with the spec of EIP7928.
---------
Co-authored-by: Felix Lange <fjl@twurst.com>
…33865) Reverts ethereum/go-ethereum#33747. This change suffers an unexpected issue during the sync with `history.chain=postmerge`.
…ncoded packet (#31547) This changes the challenge resend logic again to use the existing `ChallengeData` field of `v5wire.Whoareyou` instead of storing a second copy of the packet in `Whoareyou.Encoded`. It's more correct this way since `ChallengeData` is supposed to be the data that is used by the ID verification procedure. Also adapts the cross-client test to verify this behavior. Follow-up to #31543
The PR exposes the InfuxDB reporting interval as a CLI parameter, which was previously fixed 10s. Default is still kept at 10s. Note that decreasing the interval comes with notable extra traffic and load on InfluxDB.
implements https://github.com/ethereum/execution-apis/pull/710/changes#r2712256529 --------- Co-authored-by: Felix Lange <fjl@twurst.com>
Downgrades beacon syncer reorging from Error to Debug closes ethereum/go-ethereum#29916
Fixes an issue where AuthorizationList wasn't copied over when estimating gas for a user-provided transaction.
Implements the new eth_getStorageValues method. It returns storage values for a list of contracts. Spec: ethereum/execution-apis#756 --------- Co-authored-by: Sina Mahmoodi <itz.s1na@gmail.com>
…790) All five `revert*Request` functions (account, bytecode, storage, trienode heal, bytecode heal) remove the request from the tracked set but never restore the peer to its corresponding idle pool. When a request times out and no response arrives, the peer is permanently lost from the idle pool, preventing new work from being assigned to it. In normal operation mode (snap-sync full state) this bug is masked by pivot movement (which resets idle pools via new Sync() cycles every ~15 minutes) and peer churn (reconnections re-add peers via Register()). However in scenarios like the one I have running my (partial-stateful node)[ethereum/go-ethereum#33764] with long-running sync cycles and few peers, all peers can eventually leak out of the idle pools, stalling sync entirely. Fix: after deleting from the request map, restore the peer to its idle pool if it is still registered (guards against the peer-drop path where Unregister already removed the peer). This mirrors the pattern used in all five On* response handlers. This only seems to manifest in peer-thirstly scenarios as where I find myself when testing snapsync for the partial-statefull node). Still, thought was at least good to raise this point. Unsure if required to discuss or not
The fetcher should not fetch transactions that are already on chain. Until now we were only checking in the txpool, but that does not have the old transaction. This was leading to extra fetches of transactions that were announced by a peer but are already on chain. Here we extend the check to the chain as well.
To align with the latest spec of EIP-7928: ``` # CodeChange: [block_access_index, new_code] CodeChange = [BlockAccessIndex, Bytecode] ```
https://eips.ethereum.org/EIPS/eip-7928 spec: > Precompiled contracts: Precompiles MUST be included when accessed. If a precompile receives value, it is recorded with a balance change. Otherwise, it is included with empty change lists. The precompiled contracts are not explicitly touched when they are invoked since Amsterdam fork.
From the https://eips.ethereum.org/EIPS/eip-7928 > SELFDESTRUCT (in-transaction): Accounts destroyed within a transaction MUST be included in AccountChanges without nonce or code changes. However, if the account had a positive balance pre-transaction, the balance change to zero MUST be recorded. Storage keys within the self-destructed contracts that were modified or read MUST be included as a storage_reads entry. The storage read against the empty contract (zero storage) should also be recorded in the BAL's readlist.
inside tx.GasPrice()/GasFeeCap()/GasTipCap() already new a big.Int.
bench result:
```
goos: darwin
goarch: arm64
pkg: github.com/ethereum/go-ethereum/core
cpu: Apple M4
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
TransactionToMessage-10 240.1n ± 7% 175.1n ± 7% -27.09% (p=0.000 n=10)
│ old.txt │ new.txt │
│ B/op │ B/op vs base │
TransactionToMessage-10 544.0 ± 0% 424.0 ± 0% -22.06% (p=0.000 n=10)
│ old.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
TransactionToMessage-10 17.00 ± 0% 11.00 ± 0% -35.29% (p=0.000 n=10)
```
benchmark code:
```
// Copyright 2025 The go-ethereum Authors
// This file is part of the go-ethereum library.
//
// The go-ethereum library is free software: you can redistribute it and/or modify
// it under the terms of the GNU Lesser General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// The go-ethereum library is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public License
// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.
package core
import (
"math/big"
"testing"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/crypto"
"github.com/ethereum/go-ethereum/params"
)
// BenchmarkTransactionToMessage benchmarks the TransactionToMessage function.
func BenchmarkTransactionToMessage(b *testing.B) {
key, _ := crypto.GenerateKey()
signer := types.LatestSigner(params.TestChainConfig)
to := common.HexToAddress("0x000000000000000000000000000000000000dead")
// Create a DynamicFeeTx transaction
txdata := &types.DynamicFeeTx{
ChainID: big.NewInt(1),
Nonce: 42,
GasTipCap: big.NewInt(1000000000), // 1 gwei
GasFeeCap: big.NewInt(2000000000), // 2 gwei
Gas: 21000,
To: &to,
Value: big.NewInt(1000000000000000000), // 1 ether
Data: []byte{0x12, 0x34, 0x56, 0x78},
AccessList: types.AccessList{
types.AccessTuple{
Address: common.HexToAddress("0x0000000000000000000000000000000000000001"),
StorageKeys: []common.Hash{
common.HexToHash("0x0000000000000000000000000000000000000000000000000000000000000001"),
},
},
},
}
tx, _ := types.SignNewTx(key, signer, txdata)
baseFee := big.NewInt(1500000000) // 1.5 gwei
b.ResetTimer()
b.ReportAllocs()
for i := 0; i < b.N; i++ {
_, err := TransactionToMessage(tx, signer, baseFee)
if err != nil {
b.Fatal(err)
}
}
}
l
```
This pr adds a tool names `inpsect-trie`, aimed to analyze the mpt and its node storage more efficiently. ## Example ./geth db inspect-trie --datadir server/data-seed/ latest 4000 ## Result - MPT shape - Account Trie - Top N Storage Trie ``` +-------+-------+--------------+-------------+--------------+ | - | LEVEL | SHORTNODECNT | FULLNODECNT | VALUENODECNT | +-------+-------+--------------+-------------+--------------+ | - | 0 | 0 | 1 | 0 | | - | 1 | 0 | 16 | 0 | | - | 2 | 76 | 32 | 74 | | - | 3 | 66 | 1 | 66 | | - | 4 | 2 | 0 | 2 | | Total | 144 | 50 | 142 | +-------+-------+--------------+-------------+--------------+ AccountTrie +-------+-------+--------------+-------------+--------------+ | - | LEVEL | SHORTNODECNT | FULLNODECNT | VALUENODECNT | +-------+-------+--------------+-------------+--------------+ | - | 0 | 0 | 1 | 0 | | - | 1 | 0 | 16 | 0 | | - | 2 | 108 | 84 | 104 | | - | 3 | 195 | 5 | 195 | | - | 4 | 10 | 0 | 10 | | Total | 313 | 106 | 309 | +-------+-------+--------------+-------------+--------------+ ContractTrie-0xc874e65ccffb133d9db4ff637e62532ef6ecef3223845d02f522c55786782911 +-------+-------+--------------+-------------+--------------+ | - | LEVEL | SHORTNODECNT | FULLNODECNT | VALUENODECNT | +-------+-------+--------------+-------------+--------------+ | - | 0 | 0 | 1 | 0 | | - | 1 | 0 | 16 | 0 | | - | 2 | 57 | 14 | 56 | | - | 3 | 33 | 0 | 33 | | Total | 90 | 31 | 89 | +-------+-------+--------------+-------------+--------------+ ContractTrie-0x1d7dcb6a0ce5227c5379fc5b0e004561d7833b063355f69bfea3178f08fbaab4 +-------+-------+--------------+-------------+--------------+ | - | LEVEL | SHORTNODECNT | FULLNODECNT | VALUENODECNT | +-------+-------+--------------+-------------+--------------+ | - | 0 | 0 | 1 | 0 | | - | 1 | 5 | 8 | 5 | | - | 2 | 16 | 1 | 16 | | - | 3 | 2 | 0 | 2 | | Total | 23 | 10 | 23 | +-------+-------+--------------+-------------+--------------+ ContractTrie-0xaa8a4783ebbb3bec45d3e804b3c59bfd486edfa39cbeda1d42bf86c08a0ebc0f +-------+-------+--------------+-------------+--------------+ | - | LEVEL | SHORTNODECNT | FULLNODECNT | VALUENODECNT | +-------+-------+--------------+-------------+--------------+ | - | 0 | 0 | 1 | 0 | | - | 1 | 9 | 3 | 9 | | - | 2 | 7 | 1 | 7 | | - | 3 | 2 | 0 | 2 | | Total | 18 | 5 | 18 | +-------+-------+--------------+-------------+--------------+ ContractTrie-0x9d2804d0562391d7cfcfaf0013f0352e176a94403a58577ebf82168a21514441 +-------+-------+--------------+-------------+--------------+ | - | LEVEL | SHORTNODECNT | FULLNODECNT | VALUENODECNT | +-------+-------+--------------+-------------+--------------+ | - | 0 | 0 | 1 | 0 | | - | 1 | 6 | 4 | 6 | | - | 2 | 8 | 0 | 8 | | Total | 14 | 5 | 14 | +-------+-------+--------------+-------------+--------------+ ContractTrie-0x17e3eb95d0e6e92b42c0b3e95c6e75080c9fcd83e706344712e9587375de96e1 +-------+-------+--------------+-------------+--------------+ | - | LEVEL | SHORTNODECNT | FULLNODECNT | VALUENODECNT | +-------+-------+--------------+-------------+--------------+ | - | 0 | 0 | 1 | 0 | | - | 1 | 5 | 3 | 5 | | - | 2 | 7 | 0 | 7 | | Total | 12 | 4 | 12 | +-------+-------+--------------+-------------+--------------+ ContractTrie-0xc017ca90c8aa37693c38f80436bb15bde46d7b30a503aa808cb7814127468a44 Contract Trie, total trie num: 142, ShortNodeCnt: 620, FullNodeCnt: 204, ValueNodeCnt: 615 ``` --------- Co-authored-by: lightclient <lightclient@protonmail.com> Co-authored-by: MariusVanDerWijden <m.vanderwijden@live.de>
Previously, handshake timeouts were recorded as generic peer errors instead of timeout errors. waitForHandshake passed a raw p2p.DiscReadTimeout into markError, but markError classified errors only via errors.Unwrap(err), which returns nil for non-wrapped errors. As a result, the timeoutError meter was never incremented and all such failures fell into the peerError bucket. This change makes markError switch on the base error, using errors.Unwrap(err) when available and falling back to the original error otherwise. With this adjustment, p2p.DiscReadTimeout is correctly mapped to timeoutError, while existing behaviour for the other wrapped sentinel errors remains unchanged --------- Co-authored-by: lightclient <lightclient@protonmail.com>
Co-authored-by: tellabg <249254436+tellabg@users.noreply.github.com> Co-authored-by: lightclient <lightclient@protonmail.com>
Implements the slotnum opcode as specified here: https://eips.ethereum.org/EIPS/eip-7843
The`plucky` and `oracular` have reached end of life. That's why launchpad isn't building them anymore: https://launchpad.net/~ethereum/+archive/ubuntu/ethereum/+packages.
We didn't upgrade to 1.25, so this jumps over one version. I want to upgrade all builds to Go 1.26 soon, but let's start with the Docker build to get a sense of any possible issues.
The endianness was wrong, which means that the code chunks were stored in the wrong location in the tree.
fix the flaky test found in https://ci.appveyor.com/project/ethereum/go-ethereum/builds/53601688/job/af5ccvufpm9usq39 1. increase the timeout from 3+1s to 15s, and use timer instead of sleep(in the CI env, it may need more time to sync the 1024 blocks) 2. add `synced.Load()` to ensure the full async chain is finished Signed-off-by: Delweng <delweng@gmail.com>
…r, needs-wiring, plan)
|
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 (56.01%) 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.0 #2325 +/- ##
===========================================================
+ Coverage 54.23% 54.28% +0.04%
===========================================================
Files 912 915 +3
Lines 164127 165107 +980
===========================================================
+ Hits 89017 89624 +607
- Misses 69546 69866 +320
- Partials 5564 5617 +53
... and 19 files with indirect coverage changes
🚀 New features to boost your workflow:
|
Carries the newTrieReader point-cache fix. This branch had the same two broken calls for the same reason — its cascade hop was verified with go build, which does not compile test files — so here the merge is the fix rather than a formality. 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; #2333's prewalk and read-set tests pass.
…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.
|
codegenie review |
|
@claude review |
🧞 Codegenie ReviewWarning Review incomplete. Some review or verification work did not complete. Treat the results below as partial and consider rerunning.
CoveragePartial review: 60 hunks were not reviewed because budget was exhausted before dispatch.
|
There was a problem hiding this comment.
Pull request overview
Milestone 3/6 of the Bor ← go-ethereum v1.17.4 upstream sync, merging geth v1.17.1 changes into Bor and introducing the dormant (block-scheduled) Amsterdam fork surface (EIP-7843 SLOTNUM, EIP-8024 wiring, and precompile “touch” for BAL), plus related tooling/test updates.
Changes:
- Add Amsterdam fork scheduling to
params.ChainConfig(block-based, nil-by-default) and wire VM/header surfaces forslotNumber/SLOTNUMbehindIsAmsterdam. - Add/extend trie inspection & stats utilities (LevelStats, inspect/summarize tests) and a new
geth db inspect-triecommand. - Update several networking/tooling components (discv5 WHOAREYOU resend caching, request tracker scheduling, HTTP/2 toggles, metrics InfluxDB interval, new
eth_getStorageValuesRPC, internal tablewriter stub).
Reviewed changes
Copilot reviewed 97 out of 102 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| trie/proof.go | Range proof verification: stronger firstKey constraints and error-handling for trie updates. |
| trie/levelstats.go | New per-level trie node statistics collector used by inspection/stats tooling. |
| trie/levelstats_test.go | Tests for LevelStats depth bounds behavior. |
| trie/inspect_test.go | New tests covering trie Inspect/Summarize and contract inspection paths. |
| trie/bintrie/trie.go | Binary trie code-chunk key offset encoding change (endianness). |
| tests/gen_btheader.go | JSON marshal/unmarshal support for SlotNumber in block test headers. |
| tests/block_test_util.go | SlotNumber added to block test header structure and validation. |
| params/config.go | AmsterdamBlock + IsAmsterdam + Rules.IsAmsterdam wiring (dormant by default). |
| p2p/tracker/tracker.go | Tracker clean/schedule safety tweaks and schedule refactor. |
| p2p/discover/v5wire/msg.go | WHOAREYOU struct reshaping and ChallengeData semantics. |
| p2p/discover/v5wire/encoding.go | WHOAREYOU resend encoding via cached ChallengeData; masking refactors. |
| p2p/discover/v5wire/encoding_test.go | Test ensuring WHOAREYOU resend encoding is byte-identical. |
| p2p/discover/v5_udp_test.go | Update test codec to align with new WHOAREYOU resend caching fields. |
| node/rpcstack.go | Add per-server HTTP/2 disable switch for RPC stacks. |
| node/node.go | Disable HTTP/2 on auth/Engine API HTTP servers. |
| miner/worker.go | Populate SlotNumber post-Amsterdam (errors if missing when active). |
| miner/payload_building.go | Include slot number in payload ID; recommit timer reset uses remaining time. |
| metrics/config.go | Add configurable InfluxDB reporting interval to metrics config. |
| internal/web3ext/web3ext.go | Add web3 extension for eth_getStorageValues. |
| internal/tablewriter/database_tablewriter.go | Internal tablewriter stub moved to internal/tablewriter package; API tweaks. |
| internal/tablewriter/database_tablewriter_test.go | Update tests for internal tablewriter stub API changes. |
| internal/ethapi/transaction_args.go | Carry AuthorizationList through tx defaults. |
| internal/ethapi/api.go | Add eth_getStorageValues; include SlotNumber in RPC header marshal. |
| internal/ethapi/api_test.go | Tests for GetStorageValues behavior and request limits. |
| graphql/graphql.go | Expose SlotNumber on GraphQL Block type. |
| go.mod | Bump github.com/ethereum/c-kzg-4844/v2 to v2.1.6; drop olekukonko/tablewriter. |
| go.sum | Update sums for c-kzg bump and tablewriter removal. |
| eth/tracers/native/opcode_counter.go | New native tracer: counts opcode executions. |
| eth/tracers/native/mux.go | Refactor mux tracer construction; add exported NewMuxTracer. |
| eth/protocols/snap/sync.go | Restore peer idlers on request revert when peer still present. |
| docs/upstream-merges/v1.17.4/plan.md | Mark v1.17.1 batches as merged with commit SHAs. |
| docs/upstream-merges/v1.17.4/needs-wiring.md | Track deferred items introduced in this milestone. |
| docs/upstream-merges/v1.17.4/fork-register.md | Document Amsterdam fork surface as block-gated and dormant. |
| core/vm/opcodes.go | Add SLOTNUM opcode constant and string mappings. |
| core/vm/jump_table.go | Add Amsterdam instruction set and dispatch wiring. |
| core/vm/evm.go | SLOTNUM block context + Amsterdam jump table selection; syscall transfer gating; precompile-touch plumbing. |
| core/vm/eips.go | Add 7843 activator and implement SLOTNUM opcode. |
| core/vm/contracts.go | RunPrecompiledContract now optionally “touches” precompile in StateDB post-Amsterdam. |
| core/vm/contracts_test.go | Update precompile tests for new RunPrecompiledContract signature; include IsAmsterdam in parity checklist. |
| core/vm/contracts_fuzz_test.go | Update fuzz harness for new RunPrecompiledContract signature. |
| core/types/transaction.go | Add big.Int fallback for effective tip comparisons when uint256 calc errors. |
| core/types/gen_header_rlp.go | Include SlotNumber in optional RLP header encoding sequence. |
| core/types/gen_header_json.go | Include SlotNumber in header JSON marshal/unmarshal. |
| core/types/block.go | Add SlotNumber field to Header; copy/accessors. |
| core/types/bal/bal.go | Change construction BAL code-change representation to map[txIndex][]byte. |
| core/types/bal/bal_test.go | Adjust tests for new BAL code-change representation/encoding types. |
| core/types/bal/bal_encoding.go | Update BAL encoding to support multiple code changes; add validation/copy logic. |
| core/types/bal/bal_encoding_rlp_generated.go | Regenerated RLP encoding to match BAL encoding structure changes. |
| core/txpool/legacypool/list_test.go | Add tests for price heap comparison across basefee scenarios. |
| core/txpool/blobpool/priority.go | Adjust eviction priority math; add blobfee-specific jump base. |
| core/txpool/blobpool/priority_test.go | Update tests to match new eviction priority behavior. |
| core/txpool/blobpool/evictheap.go | Use blobfee-specific jump calculation; simplify priority clamp. |
| core/txpool/blobpool/evictheap_test.go | Update sorting tests/benchmarks for new blobfee jump behavior. |
| core/txpool/blobpool/blobpool_test.go | Update expected jump constants and test chain fee setup. |
| core/stateless/stats.go | Switch witness leaf depth collection to trie.LevelStats and log/metric reporting changes. |
| core/stateless/stats_test.go | Update witness stats tests; add deep-leaf panic and aggregation coverage. |
| core/state/state_object.go | Ensure storage reader is invoked for destructed objects to record BAL reads. |
| core/state_transition.go | Use tx getters directly when building Message (copies returned by getters). |
| core/state_processor_test.go | Ensure SlotNumber is set in generated headers when Amsterdam is active. |
| core/rawdb/database.go | Switch to internal tablewriter; update constructor call. |
| core/rawdb/accessors_chain.go | Canonical HasBody/HasReceipts behavior changed. |
| core/parallel_state_processor_fork_parity_test.go | Add IsAmsterdam to fork expectations (not state-processor gated). |
| core/genesis.go | Add SlotNumber to genesis struct and header construction post-Amsterdam. |
| core/gen_genesis.go | Add SlotNumber to genesis JSON marshal/unmarshal. |
| core/evm.go | Populate vm.BlockContext.SlotNum from header.SlotNumber. |
| consensus/ethash/consensus.go | Enforce SlotNumber must be nil for ethash headers; panics in SealHash if set. |
| consensus/clique/clique.go | Enforce SlotNumber must be nil for clique headers; panic if present in signature header. |
| consensus/beacon/consensus.go | Enforce SlotNumber presence/absence based on IsAmsterdam. |
| cmd/utils/flags.go | Add --top, --output, and --metrics.influxdb.interval flags; use interval in exporters. |
| cmd/geth/main.go | Wire metrics influxdb interval flag into geth command flags. |
| cmd/geth/dbcmd.go | Add db inspect-trie command and switch metadata table output to internal tablewriter. |
| cmd/geth/config.go | Apply --metrics.influxdb.interval to config. |
| cmd/geth/chaincmd.go | Wire metrics influxdb interval flag into chain subcommands. |
| cmd/evm/testdata/33/exp.json | Normalize logs field to [] instead of null in expected output. |
| cmd/evm/testdata/30/exp.json | Normalize logs field to [] instead of null in expected output. |
| cmd/evm/testdata/3/exp.json | Normalize logs field to [] instead of null in expected output. |
| cmd/evm/testdata/29/exp.json | Normalize logs field to [] instead of null in expected output. |
| cmd/evm/testdata/28/exp.json | Normalize logs field to [] instead of null in expected output. |
| cmd/evm/testdata/25/exp.json | Normalize logs field to [] instead of null in expected output. |
| cmd/evm/testdata/24/exp.json | Normalize logs field to [] instead of null in expected output. |
| cmd/evm/testdata/23/exp.json | Normalize logs field to [] instead of null in expected output. |
| cmd/evm/testdata/13/exp2.json | Normalize logs field to [] instead of null in expected output. |
| cmd/evm/testdata/1/exp.json | Normalize logs field to [] instead of null in expected output. |
| cmd/evm/main.go | Add opcode-count flag to evm t8n command flags. |
| cmd/evm/internal/t8ntool/transition.go | Add opcode counter tracing and optional muxing with existing tracers. |
| cmd/evm/internal/t8ntool/gen_stenv.go | Add SlotNumber to state test env JSON codec. |
| cmd/evm/internal/t8ntool/gen_header.go | Add SlotNumber to header JSON codec. |
| cmd/evm/internal/t8ntool/flags.go | Add --opcode.count flag. |
| cmd/evm/internal/t8ntool/file_tracer.go | Refactor file/result writers to return tracers instead of bare hooks. |
| cmd/evm/internal/t8ntool/execution.go | Add SlotNumber to env/header construction; normalize empty receipt logs to []. |
| cmd/evm/internal/t8ntool/block.go | Add SlotNumber to block/header JSON structures. |
| cmd/devp2p/internal/v5test/discv5tests.go | Update discv5 test to validate WHOAREYOU resend behavior. |
| build/ci.go | Remove EOL Ubuntu distros from CI images list. |
| beacon/engine/types.go | Add PayloadV4 and SlotNumber fields to Engine API payload types. |
| beacon/engine/gen_ed.go | Generated JSON codec updated for ExecutableData SlotNumber. |
| beacon/engine/gen_blockparams.go | Generated JSON codec updated for PayloadAttributes SlotNumber. |
| beacon/blsync/engineclient.go | Suppress specific ForkchoiceUpdated error during blsync reorg skipping. |
| .github/CODEOWNERS | Add CODEOWNERS entry for cmd/keeper. |
Files not reviewed (4)
- beacon/engine/gen_blockparams.go: Generated file
- beacon/engine/gen_ed.go: Generated file
- cmd/evm/internal/t8ntool/gen_header.go: Generated file
- cmd/evm/internal/t8ntool/gen_stenv.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if isCanon(db, number, hash) { | ||
| // Block is in ancient store, but bodies can be pruned. | ||
| // Check if the block number is above the pruning tail. | ||
| tail, _ := db.Tail() | ||
| if number >= tail { | ||
| return true | ||
| } | ||
| return false | ||
| return true | ||
| } |
| if isCanon(db, number, hash) { | ||
| // Block is in ancient store, but receipts can be pruned. | ||
| // Check if the block number is above the pruning tail. | ||
| tail, _ := db.Tail() | ||
| if number >= tail { | ||
| return true | ||
| } | ||
| return false | ||
| return true | ||
| } |
| if tracer != nil { | ||
| // If we have an existing tracer, multiplex with the opcode tracer | ||
| mux, _ := native.NewMuxTracer([]string{"trace", "opcode"}, []*tracers.Tracer{tracer, opcodeTracer}) | ||
| vmConfig.Tracer = mux.Hooks | ||
| } else { | ||
| vmConfig.Tracer = opcodeTracer.Hooks | ||
| } |
| if err.Error() == "beacon syncer reorging" { | ||
| log.Debug("Failed ForkchoiceUpdated", "head", event.Block.Hash(), "error", err) | ||
| continue // ignore beacon syncer reorging errors, this error can occur if the blsync is skipping a block | ||
| } |
| for index, key := range keys { | ||
| _ = tr.Update(key, values[index]) | ||
| if err := tr.Update(key, values[index]); err != nil { | ||
| return false, err | ||
| } | ||
| } |
| Usage: "Print detailed trie information about the structure of account trie and storage tries.", | ||
| Description: `This commands iterates the entrie trie-backed state. If the 'blocknum' is not specified, | ||
| the latest block number will be used by default.`, |
![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. Treat the results below as partial and consider rerunning.
Partial review: 60 hunks were not reviewed because budget was exhausted before dispatch.
Reviewed 155/264 hunks before stopping.
Coverage disclosure:
- Budget stopped review work (token limit reached).
- Verification incomplete for 6 candidates.
- beacon/engine/gen_blockparams.go: generated file
- beacon/engine/gen_ed.go: generated file
- cmd/evm/internal/t8ntool/gen_header.go: generated file
- cmd/evm/internal/t8ntool/gen_stenv.go: generated file
- core/gen_genesis.go: generated file
- core/types/bal/bal_encoding_rlp_generated.go: generated file
- core/types/gen_header_json.go: generated file
- core/types/gen_header_rlp.go: generated file
- go.sum: lockfile
- tests/gen_btheader.go: generated file
- semantic composition skipped; deterministic fallback used
Summary-only findings:
-
⚪ Low: LevelStats.AddLeaf panics on depth >= 16 and the witness-stats caller passes unclamped path length (
trie/levelstats_test.go:36)
Impact: WitnessStats.Add calls trie.LevelStats.AddLeaf(len(path)) with an unclamped node-path length. A witness node path of 16 or more nibbles (a deep account/storage branch, reachable in principle by grinding a long hashed-key prefix collision) indexes s.level[depth] beyond the fixed [16]stat array and panics, aborting the block-processing goroutine instead of degrading the metric. The newly added test locks this panic in as the intended contract rather than requiring a clamp or drop.
A statistics collector should saturate or drop out-of-range samples rather than panic, and encoding the panic as an asserted contract in tests makes the sharp edge durable. Impact is bounded: the only caller is reachable only when both VmConfig.StatelessSelfValidation and VmConfig.EnableWitnessStats are enabled, a debug/self-validation configuration that core/blockchain.go states should never run in production, so this is a robustness/contract-confirmation issue rather than a node-crash risk for default operators.Evidence:
Changed code:
func TestLevelStatsAddLeafPanicsOnDepth16(t *testing.T) { defer func() { if r := recover(); r == nil { t.Fatal("expected panic for depth >= 16") } }() NewLevelStats().AddLeaf(16) }
trie/levelstats.go↗ (Confirmed by exact read of lines 18-95: AddLeaf indexes a fixed [16]stat array with no bounds guard, so depth >= 16 is an unrecovered index-out-of-range panic. The type comment notes tries may reach 64 levels.):const trieStatLevels = 16 type LevelStats struct { level [trieStatLevels]stat } // AddLeaf records a leaf depth. Witness collection reuses the value-node bucket // for leaf accounting. It panics if the depth is outside [0, 15]. func (s *LevelStats) AddLeaf(depth int) { s.level[depth].value.Add(1) }
core/stateless/stats.go↗ (The only production caller passes the raw nibble-path length with no clamp, so any witness node path of 16+ nibbles reaches the unguarded index.):for i, path := range paths { // If current path is a prefix of the next path, it's not a leaf. // The last path is always a leaf. if i == len(paths)-1 || !strings.HasPrefix(paths[i+1], paths[i]) { ownerStat.AddLeaf(len(path)) } }
core/blockchain.go↗ (Bounds the blast radius: the collector is only constructed under the doubly-gated StatelessSelfValidation + EnableWitnessStats debug configuration, so the panic cannot be triggered on a default production node.):// Self validation should *never* run in production, it's more of // a tight integration to enable running *all* consensus tests through the // witness builder/runner ... if witness = statedb.Witness(); witness != nil && bc.cfg.VmConfig.StatelessSelfValidation { ... if bc.cfg.VmConfig.EnableWitnessStats { witnessStats = stateless.NewWitnessStats() }
Suggested fix: Either make AddLeaf tolerant (
if depth < 0 || depth >= trieStatLevels { return }, or clamp into the last bucket) and update both trie/levelstats_test.go and core/stateless/stats_test.go to assert the safe behavior, or explicitly confirm that panicking on depth >= 16 is the intended contract given the caller is debug-gated.Suggested test: If the clamp is adopted, replace TestLevelStatsAddLeafPanicsOnDepth16 with a test asserting AddLeaf(16) is a no-op (or increments the depth-15 bucket) and update core/stateless TestWitnessStatsPanicsOnDeepLeaf to assert WitnessStats.Add tolerates a 20-nibble node path.
-
⚪ Low: inspect-trie: nil header dereference when canonical hash exists but header record is missing (
cmd/geth/dbcmd.go:117)
Impact: Runninggeth db inspect-trie <blocknum>(orlatest) against a datadir where rawdb.ReadCanonicalHash returns a non-zero hash but the corresponding header record is missing or its RLP is corrupt makes rawdb.ReadHeader return nil; the very next statementtrieRoot = blockHeader.Rootdereferences that nil pointer and the CLI panics with a stack trace instead of returning the intended "header not found" error.
This is a new operator-facing debug subcommand whose stated purpose is inspecting state/trie data on databases that may be partially synced, truncated, or damaged — exactly the conditions that produce a canonical-hash entry without a readable header. A nil-pointer panic gives no actionable diagnostic and reads like a crash bug, while the surrounding code already demonstrates the intended error-return style for the adjacent canonical-hash lookup.Evidence:
Changed code:
if number != math.MaxUint64 { hash = rawdb.ReadCanonicalHash(db, number) if hash == (common.Hash{}) { return fmt.Errorf("canonical hash for block %d not found", number) } blockHeader := rawdb.ReadHeader(db, hash, number) trieRoot = blockHeader.Root }
core/rawdb/accessors_chain.go↗ (Decisive helper branch: ReadHeader returns nil both when the header RLP is absent and when it fails to decode, so blockHeader.Root at dbcmd.go:500 can dereference nil.):func ReadHeader(db ethdb.Reader, hash common.Hash, number uint64) *types.Header { data := ReadHeaderRLP(db, hash, number) if len(data) == 0 { return nil } header := new(types.Header) if err := rlp.DecodeBytes(data, header); err != nil { log.Error("Invalid block header RLP", "hash", hash, "err", err) return nil } return header }
Suggested fix: blockHeader := rawdb.ReadHeader(db, hash, number)
if blockHeader == nil {
return fmt.Errorf("header for block %d (%#x) not found", number, hash)
}
trieRoot = blockHeader.RootSuggested test: Add a cmd/geth or rawdb-backed unit test that writes only the canonical hash mapping for a block number (no header record) and asserts inspectTrie returns an error containing "header ... not found" rather than panicking.
-
⚪ Low: TestHandshakeResend drops PONG-after-resend coverage; resp.Node assignment is dead (
cmd/devp2p/internal/v5test/discv5tests.go:200)
Impact: In head,resp.Node = conn.remoteis the last statement of the case and the function returns, so the mutatedrespis never used. The test now only asserts that the second WHOAREYOU repeats the first challenge's Nonce and ChallengeData. Base asserted, via conn.reqresp, that the second PING could actually complete the handshake and yield a valid PONG. A regression in which the resent ChallengeData is echoed correctly but is stale/unusable for deriving session keys (exactly the area touched by switching from Whoareyou.Encoded to ChallengeData) would pass this test.
This is the dedicated conformance test for handshake challenge resend in the devp2p test suite, run against third-party discv5 implementations. Losing the 'answer the resent challenge and get a PONG' assertion narrows the test to a byte-equality check on the challenge and leaves the resend path's usability untested, while the leftover dead assignment signals the follow-up write/PONG check was dropped unintentionally.Evidence:
Changed code:
conn.write(l1, ping2, nil) switch resp := conn.read(l1).(type) { case *v5wire.Whoareyou: if resp.Nonce != challenge1.Nonce { ... } if !bytes.Equal(resp.ChallengeData, challenge1.ChallengeData) { ... } resp.Node = conn.remote default: t.Fatal("expected WHOAREYOU, got", resp) } }
cmd/devp2p/internal/v5test/discv5tests.go↗ (Base version (TestPingHandshakeInterrupted) completed the handshake via conn.reqresp and asserted a valid PONG for ping2; head no longer performs this step, removing the covered boundary.):// Send second PING. ping2 := &v5wire.Ping{ReqID: conn.nextReqID()} switch resp := conn.reqresp(l1, ping2).(type) { case *v5wire.Pong: checkPong(t, resp, ping2, l1)
cmd/devp2p/internal/v5test/discv5tests.go↗ (In TestPingMultiIP the sameresp.Node = ...assignment is immediately followed by writing the handshake packet and checking the PONG, showing the assignment is only meaningful when the challenge is answered.):resp.Node = s.Dest conn.write(l2, ping2, resp) ... case *v5wire.Pong: checkPong(t, resp, ping2, l2)
Suggested fix: Complete the test as the leftover assignment implies:
resp.Node = conn.remote
conn.write(l1, ping2, resp)
}
// Catch the PONG for ping2.
switch resp := conn.read(l1).(type) {
case *v5wire.Pong:
checkPong(t, resp, ping2, l1)
default:
t.Fatal("expected PONG, got", resp)
}
Alternatively, if only the challenge-repeat property is intended, remove the deadresp.Node = conn.remoteline and note in the comment that handshake completion is intentionally out of scope.Suggested test: Extend TestHandshakeResend to answer the resent WHOAREYOU with the handshake packet for ping2 and assert a valid PONG via checkPong, covering that the resent ChallengeData is usable for session establishment.
-
⚪ Low: Unchecked nil header dereference in
geth db inspect-trie(cmd/geth/dbcmd.go:500)
Impact: rawdb.ReadHeader returns a nil *types.Header when no header is stored for (hash, number) or when the stored header RLP fails to decode. The followingblockHeader.Rootthen dereferences nil and panics, sogeth db inspect-trie <blocknum>crashes with a stack trace on an inconsistent or corrupted datadir (canonical-hash marker present without a readable header) instead of returning the clean CLI error used one line earlier for a missing canonical hash.
This is an offline diagnostic command intended to be run against damaged or unusual databases; the exact situation it is used to investigate is the one where it panics instead of reporting the problem. The guard is three lines and matches the surrounding error-handling style.Evidence:
Changed code:
blockHeader := rawdb.ReadHeader(db, hash, number) trieRoot = blockHeader.Root
core/rawdb/accessors_chain.go↗ (Decisive callee branches: ReadHeader returns nil both when no header data is stored and when the stored header RLP fails to decode.):func ReadHeader(db ethdb.Reader, hash common.Hash, number uint64) *types.Header { data := ReadHeaderRLP(db, hash, number) if len(data) == 0 { return nil } header := new(types.Header) if err := rlp.DecodeBytes(data, header); err != nil { log.Error("Invalid block header RLP", "hash", hash, "err", err) return nil } return header }
cmd/geth/dbcmd.go↗ (The immediately preceding lookup establishes the error-return contract for missing data; the header lookup skips the equivalent check.):hash = rawdb.ReadCanonicalHash(db, number) if hash == (common.Hash{}) { return fmt.Errorf("canonical hash for block %d not found", number) }
Suggested fix: blockHeader := rawdb.ReadHeader(db, hash, number)
if blockHeader == nil {
return fmt.Errorf("header for block %d (%s) not found", number, hash)
}
trieRoot = blockHeader.RootSuggested test: Unit/CLI test: seed a test chaindb with WriteCanonicalHash for block N but no header (or a corrupt header RLP), invoke inspectTrie with argument N, and assert it returns a non-nil error rather than panicking.
-
⚪ Low: Re-idling the peer in revert*Request allows a tight assign/send-fail/revert loop on write errors (
eth/protocols/snap/sync.go:1832)
Impact: When a peer's request write fails synchronously (broken/closed connection that has not yet triggered Unregister), the request goroutine calls scheduleRevertAccountRequest immediately. With the new re-idle, revertAccountRequest puts the peer back into s.accountIdlers while it is still present in s.peers, and the next assignAccountTasks pass hands it the same task again, which fails again instantly. This spins assign -> send error -> revert -> re-idle (one goroutine plus one timer plus debug log lines per iteration) until the peer is unregistered. Before the change the peer stayed out of the idle pool, so a failing peer was used at most once per task.
During the window between a peer's write failure and its unregistration, the snap sync event loop can churn through many pointless assign/revert cycles, burning CPU, spamming debug logs and delaying assignment of that task to healthy peers. The timeout path is paced by rates.TargetTimeout() and is far less severe, but it likewise lets a black-hole peer keep re-acquiring tasks.Evidence:
Changed code:
// Remove the request from the tracked set and restore the peer to the // idle pool so it can be reassigned work (skip if peer already left). s.lock.Lock() delete(s.accountReqs, req.id) if _, ok := s.peers[req.peer]; ok { s.accountIdlers[req.peer] = struct{}{} } s.lock.Unlock()
eth/protocols/snap/sync.go↗ (assignAccountTasks (1056-1162) reverts immediately, with no delay/backoff, when the write to the peer fails; each attempt allocates a goroutine and a timer.):req.timeout = time.AfterFunc(s.rates.TargetTimeout(), func() { ... s.scheduleRevertAccountRequest(req) }) s.accountReqs[reqid] = req delete(s.accountIdlers, idle) s.pend.Add(1) go func(root common.Hash) { defer s.pend.Done() ... if err := peer.RequestAccountRange(reqid, root, req.origin, req.limit, uint64(cap)); err != nil { peer.Log().Debug("Failed to request account range", "err", err) s.scheduleRevertAccountRequest(req) } }(s.root)
eth/protocols/snap/sync.go↗ (Event loop (~758-790) re-runs assignment on every iteration, so a peer re-idled by the revert is reassigned the same task immediately.):case req := <-accountReqFails: s.revertAccountRequest(req) // loop top re-runs: s.assignAccountTasks(accountResps, accountReqFails, cancel)
eth/protocols/snap/sync.go↗ (The only filter on idle peers is statelessPeers; a peer whose write failed is not marked stateless, so nothing prevents immediate reselection.):for id := range s.accountIdlers { if _, ok := s.statelessPeers[id]; ok { continue } idlers.ids = append(idlers.ids, id) ... }
eth/protocols/snap/peer.go↗ (The send error is a p2p write error on a broken connection, i.e. persistent rather than transient, so the retry fails again instantly.):func (p *Peer) RequestAccountRange(...) error { ... return p2p.Send(p.rw, GetAccountRangeMsg, &GetAccountRangePacket{...}) }
Suggested fix: Do not re-idle on the synchronous send-failure path, or add a marker/backoff so a peer whose request send failed is not reselected immediately (e.g. mark it stateless/skip until a successful delivery, or signal via s.update instead of re-idling unconditionally). Restrict the re-idle to the timeout path the commit body describes.
Suggested test: In eth/protocols/snap/sync_test.go, register a peer whose RequestAccountRange always returns an error and assert that the number of request attempts made to that peer stays bounded (e.g. a small constant) while sync progresses via other peers.
-
⚪ Low: revertBytecodeRequest re-idles a peer whose response is still outstanding after a timeout (
eth/protocols/snap/sync.go:1878)
Impact: On the timeout revert path the peer is still present in s.peers and still owes a response, so the new unconditional re-add puts it back into s.bytecodeIdlers immediately. assignBytecodeTasks can then dispatch a second bytecode request to a peer that already failed to answer within TargetTimeout, leaving two outstanding requests to one stalled peer and allowing repeated re-issuance to the same slow peer instead of preferring healthy idlers. The guardif _, ok := s.peers[req.peer]only filters the disconnect case (Unregister deletes from s.peers before peerDrop triggers revertRequests).
Snap sync scheduling quality depends on not immediately handing new work to peers that just timed out. The re-idle creates a timeout/reassign churn loop against the same slow peer and doubles its outstanding request count, which can slow state sync progress on peers with degraded links.Evidence:
Changed code:
// Remove the request from the tracked set and restore the peer to the // idle pool so it can be reassigned work (skip if peer already left). s.lock.Lock() delete(s.bytecodeReqs, req.id) if _, ok := s.peers[req.peer]; ok { s.bytecodeIdlers[req.peer] = struct{}{} } s.lock.Unlock()
eth/protocols/snap/sync.go↗ (assignBytecodeTasks (1165-1278) removes the peer from bytecodeIdlers on dispatch and reverts on timeout while the peer is still registered in s.peers, so the new guard does not filter the timeout case; the peer becomes assignable again with a response still outstanding. It also decays the peer's msgrate capacity, which partially mitigates re-preferring the stalled peer.):req.timeout = time.AfterFunc(s.rates.TargetTimeout(), func() { peer.Log().Debug("Bytecode request timed out", "reqid", reqid) s.rates.Update(idle, ByteCodesMsg, 0, 0) s.scheduleRevertBytecodeRequest(req) }) s.bytecodeReqs[reqid] = req delete(s.bytecodeIdlers, idle)
eth/protocols/snap/sync.go↗ (onByteCodes (2827-2932) is the pre-existing idle-restore chokepoint and restores the peer even for a stale/late delivery, so the peer was not permanently excluded before this change; it also discards the stale delivery, bounding the impact of the duplicate outstanding request to scheduling churn rather than data corruption.):defer func() { s.lock.Lock() defer s.lock.Unlock() if _, ok := s.peers[peer.ID()]; ok { s.bytecodeIdlers[peer.ID()] = struct{}{} } ... }() ... req, ok := s.bytecodeReqs[id] if !ok { logger.Warn("Unexpected bytecode packet") ...
Suggested fix: If immediate re-idling is intended, confirm it explicitly (it changes upstream scheduling behavior for timed-out peers). Otherwise, restrict the re-add to revert reasons where the peer is known to be free (e.g. RequestByteCodes send failure / cancellation) and leave a timed-out peer out of the idle pool until its response arrives (onByteCodes already re-idles it, even for stale ids) or it is dropped.
Suggested test: Add a syncer test that dispatches a bytecode request, fires the timeout without any response, and asserts whether the peer reappears in s.bytecodeIdlers and is immediately handed a second bytecode request while the first is still outstanding; mirror it for the send-failure revert path to pin the intended distinction.
-
⚪ Low: AmsterdamBlock is wired into Rules but omitted from CheckConfigForkOrder and checkCompatible (
params/config.go:1938)
Impact: This hunk makes Rules.IsAmsterdam live, derived solely from ChainConfig.AmsterdamBlock. Because amsterdamBlock appears in neither CheckConfigForkOrder's fork list nor checkCompatible: (a) a custom genesis that sets amsterdamBlock earlier than osakaBlock, or sets it while osakaBlock/pragueBlock are nil, passes fork-order validation and yields Rules{IsAmsterdam:true, IsOsaka:false} — an out-of-order fork state the ordering chokepoint exists to reject; and (b) changing or removing an already-passed amsterdamBlock across a restart produces no ConfigCompatError, so the node starts with silently different Rules instead of refusing to start and demanding a rewind.
CheckConfigForkOrder and checkCompatible are the chokepoints that stop nonsensical or silently-mutated fork schedules from reaching Rules and the EVM. Every sibling gate (Cancun, Prague, Osaka, Verkle) is registered in both; Amsterdam is registered in neither, so the new gate is the one fork that can be misordered or retroactively rescheduled without any diagnostic. Impact is bounded today: AmsterdamBlock is nil on every bundled preset, so only operator-authored custom genesis files are affected, and no consumer of Rules.IsAmsterdam was confirmed in this tree — the risk grows as soon as Amsterdam semantics are attached to the flag.Evidence:
Changed code:
IsOsaka: c.IsOsaka(num), IsAmsterdam: c.IsAmsterdam(num), IsEIP4762: c.IsVerkle(num),
params/config.go↗ (Lines 1468-1470: the gate requires only London, not Osaka/Prague, so ordering sanity depends entirely on CheckConfigForkOrder — the check that is missing.):func (c *ChainConfig) IsAmsterdam(num *big.Int) bool { return c.IsLondon(num) && isBlockForked(c.AmsterdamBlock, num) }
params/config.go↗ (CheckConfigForkOrder (1521-1614) fork slice terminates at verkleBlock; amsterdamBlock is absent, so amsterdamBlock set with osakaBlock nil, or amsterdamBlock < osakaBlock, passes validation.):{name: "cancunBlock", block: c.CancunBlock, optional: true}, {name: "pragueBlock", block: c.PragueBlock, optional: true}, {name: "osakaBlock", block: c.OsakaBlock, optional: true}, {name: "verkleBlock", block: c.VerkleBlock, optional: true}, } {params/config.go↗ (checkCompatible (1629-1721) ends here with no AmsterdamBlock clause, so rescheduling/removing an already-passed amsterdamBlock returns nil instead of a ConfigCompatError.):if isForkBlockIncompatible(c.OsakaBlock, newcfg.OsakaBlock, headNumber) { return newBlockCompatError("Osaka fork block", c.OsakaBlock, newcfg.OsakaBlock) } return nil }
params/config.go↗ (Line 857: the field is JSON-settable, so operator-supplied custom genesis files can reach the unvalidated path.):AmsterdamBlock *big.Int `json:"amsterdamBlock,omitempty"` // Amsterdam switch Block (nil = no fork, 0 = already on amsterdam)
Suggested fix: Add
{name: "amsterdamBlock", block: c.AmsterdamBlock, optional: true}to the CheckConfigForkOrder fork slice, positioned consistently with the Osaka/Verkle entries, and add to checkCompatible:if isForkBlockIncompatible(c.AmsterdamBlock, newcfg.AmsterdamBlock, headNumber) {
return newBlockCompatError("Amsterdam fork block", c.AmsterdamBlock, newcfg.AmsterdamBlock)
}mirroring the existing Osaka handling.
Suggested test: In params/config_test.go add a CheckConfigForkOrder case with AmsterdamBlock=1 and OsakaBlock=nil (and one with AmsterdamBlock < OsakaBlock) expecting an ordering error, plus a CheckCompatible case rescheduling AmsterdamBlock past an already-forked head expecting a ConfigCompatError.
🙋 Needs human attention:
- Does lowering testBlockChain basefee 1050→1 and blobfee 105→1 in TestAdd still exercise the pending-vs-queued/underpriced path (e.g. the gapped-nonce TxStatusQueued check and the ErrTxGasPriceTooLow sub-case), or does it now make every seeded tx trivially executable and weaken those assertions?
- Does muxTracer.OnTxStart/OnTxEnd/OnOpcode nil-check each sub-tracer's individual hook functions before invoking them (the fileWritingTracer hooks set every hook, so this is likely fine, but the opcode counter Tracer only defines OnOpcode)?
- Is the new stateDB!=nil precompile touch path (stateDB.Exist(address) in RunPrecompiledContract) exercised by any test, or only by production EVM callers in core/vm/evm.go?
- Do the generated files core/types/gen_header_json.go and core/types/gen_header_rlp.go include the new Header.SlotNumber optional field, and does CopyHeader in core/types/block.go deep-copy it?
- Does trie/proof_test.go (TestRangeProofWithInvalidNonExistentProof, TestOneElementRangeProof) still expect the old error/success behavior for cases where firstKey is greater than keys[0], now that VerifyRangeProof short-circuits with "unexpected key-value pairs preceding the requested range"?
- Additional unresolved notes suppressed: 12
Sorry, this review is incomplete. The allotted max token limit of 8000000 (config
review.maxBudgetTokens) was reached and the review has been degraded. Raise the limit for a complete review.
— codegenie v0.5.5 (58f82a9b2c) · View Workflow Job
| return true | ||
| } | ||
| return false | ||
| return true |
There was a problem hiding this comment.
Impact: After the ancient tail is advanced, ChainFreezerReceiptTable entries below the tail are pruned but ChainFreezerHashTable entries are retained (prunable:false). isCanon therefore still returns true for such canonical blocks, and HasReceipts now unconditionally returns true. Callers that gate on HasReceipts (skipping receipt re-fetch/backfill during sync, or short-circuiting receipt queries) proceed as if receipts exist, while ReadReceiptsRLP's identical isCanon branch reads the pruned receipt table and yields empty data — producing empty receipt responses or skipped repair instead of the previous correct 'not present'.
The removed lines were a deliberate downstream guard ('Block is in ancient store, but receipts can be pruned. Check if the block number is above the pruning tail.'). The upstream merge silently reverted it, re-introducing the pruned-receipt false positive: HasReceipts and ReadReceiptsRLP no longer agree for any canonical block below the ancient pruning tail.
Evidence:
Changed code:
func HasReceipts(db ethdb.Reader, hash common.Hash, number uint64) bool {
if isCanon(db, number, hash) {
return true // removed: tail, _ := db.Tail(); if number >= tail { return true }; return false
}
if has, err := db.Has(blockReceiptsKey(number, hash)); !has || err != nil {
return false
}
return true
}core/rawdb/ancient_scheme.go ↗ (Decisive: the hash table is NOT prunable while the receipt table IS, so tail pruning removes receipts but leaves the canonical hash entries readable.):
var chainFreezerTableConfigs = map[string]freezerTableConfig{
ChainFreezerHeaderTable: {noSnappy: false, prunable: false},
ChainFreezerHashTable: {noSnappy: true, prunable: false},
ChainFreezerBodiesTable: {noSnappy: false, prunable: true},
ChainFreezerReceiptTable: {noSnappy: false, prunable: true},
ChainFreezerDifficultyTable: {noSnappy: true, prunable: true},
}core/rawdb/freezer.go ↗ (Confirms tail truncation only touches prunable tables, so Ancient(ChainFreezerHashTable, number) still succeeds below the tail.):
func (f *Freezer) TruncateTail(tail uint64) (uint64, error) {
...
for _, table := range f.tables {
if table.config.prunable {
if err := table.truncateTail(tail - f.offset.Load()); err != nil {
return 0, err
}
}
}
f.tail.Store(tail)
return old, nil
}core/rawdb/accessors_chain.go ↗ (isCanon consults only the non-prunable hash table; it does not prove receipt data still exists, so it cannot substitute for the removed tail guard.):
func isCanon(reader ethdb.AncientReaderOp, number uint64, hash common.Hash) bool {
h, err := reader.Ancient(ChainFreezerHashTable, number)
if err != nil {
return false
}
return bytes.Equal(h, hash[:])
}core/rawdb/accessors_chain.go ↗ (The read path takes the same isCanon branch and returns empty data for pruned numbers, so HasReceipts==true and ReadReceiptsRLP==empty now disagree.):
func ReadReceiptsRLP(db ethdb.Reader, hash common.Hash, number uint64) rlp.RawValue {
_ = db.ReadAncients(func(reader ethdb.AncientReaderOp) error {
if isCanon(reader, number, hash) {
data, _ = reader.Ancient(ChainFreezerReceiptTable, number)
return nil
}
...Suggested fix: Restore the guard on the ancient branch:
if isCanon(db, number, hash) {
// Block is in ancient store, but receipts can be pruned.
// Check if the block number is above the pruning tail.
tail, _ := db.Tail()
return number >= tail
}
Alternatively, have the ancient branch verify the receipt table entry itself (e.g. read ChainFreezerReceiptTable and treat a read error/empty value as absent). If the drop is intentional, document why HasReceipts may disagree with ReadReceiptsRLP below the tail.
Suggested test: In core/rawdb, freeze N blocks, call TruncateTail past block X, then assert isCanon still resolves X (hash table non-prunable) while HasReceipts(db, hashX, X) == false and ReadReceiptsRLP(db, hashX, X) is empty — i.e. the two accessors agree.
| // AddLeaf records a leaf depth. Witness collection reuses the value-node bucket | ||
| // for leaf accounting. It panics if the depth is outside [0, 15]. | ||
| func (s *LevelStats) AddLeaf(depth int) { | ||
| s.level[depth].value.Add(1) |
There was a problem hiding this comment.
Impact: WitnessStats.Add passes the raw node-path length to trie.LevelStats.AddLeaf, which indexes a fixed [16]stat array with no bounds check or clamp. In base, both the account and storage branches clamped depth to 15 before indexing, so deep paths were merely bucketed into the last slot. In head that clamp is gone and no replacement guard exists on the reachable path (AddLeaf's only non-test caller is WitnessStats.Add), so a witness node path of length >= 16 causes an index-out-of-range panic inside witness statistics collection instead of a clamped counter increment. Node paths are nibble paths bounded by 64 levels, as the new file's own doc comment states.
A metrics-only accounting path becomes a panic source driven by chain data (deep account/storage trie paths) rather than developer input. Base code deliberately tolerated deep paths by clamping; the new helper converts that tolerated case into a crash of the block-execution/witness stats path.
Evidence:
Changed code:
// AddLeaf records a leaf depth. Witness collection reuses the value-node bucket
// for leaf accounting. It panics if the depth is outside [0, 15].
func (s *LevelStats) AddLeaf(depth int) {
s.level[depth].value.Add(1)
}
const trieStatLevels = 16
type LevelStats struct {
level [trieStatLevels]stat
}core/stateless/stats.go ↗ (Base explicitly clamped out-of-range depths into the last bucket before indexing the fixed 16-entry array. This guard is the behavior the new helper drops.):
// base (before this PR)
for i, path := range paths {
if i == len(paths)-1 || !strings.HasPrefix(paths[i+1], paths[i]) {
depth := len(path)
if owner == (common.Hash{}) {
if depth >= len(s.accountTrieLeaves) {
depth = len(s.accountTrieLeaves) - 1
}
s.accountTrieLeaves[depth] += 1
} else {
if depth >= len(s.storageTrieLeaves) {
depth = len(s.storageTrieLeaves) - 1
...core/stateless/stats.go ↗ (Only non-test caller of LevelStats.AddLeaf; passes raw len(path) with no clamp, so the removed guard is not re-enforced anywhere on the reachable path.):
// head (this PR)
for i, path := range paths {
if i == len(paths)-1 || !strings.HasPrefix(paths[i+1], paths[i]) {
ownerStat.AddLeaf(len(path))
}
}trie/levelstats_test.go ↗ (Same-PR test asserts AddLeaf(16) panics, proving the new helper has no bounds handling (it proves the behavior changed, not that it is safe).):
NewLevelStats().AddLeaf(16)Suggested fix: Restore the clamp, either in the helper:
func (s *LevelStats) AddLeaf(depth int) {
if depth < 0 {
return
}
if depth >= trieStatLevels {
depth = trieStatLevels - 1 // overflow bucket, matches previous behavior
}
s.level[depth].value.Add(1)
}
or at the caller in core/stateless/stats.go before calling AddLeaf, and update the doc comment and the panic-asserting test accordingly.
Suggested test: In core/stateless, call WitnessStats.Add with a nodes map containing a path key of length >= 16 and assert it does not panic and that the leaf is counted in the depth-15 overflow bucket (matching pre-PR behavior).
| } | ||
| s.storageTrieLeaves[depth] += 1 | ||
| } | ||
| ownerStat.AddLeaf(len(path)) |
There was a problem hiding this comment.
Impact: Add() feeds raw node-path lengths from witness node maps into trie.LevelStats.AddLeaf, which indexes a [16]stat array without bounds handling. If any collected node path is 16 or more nibbles long, AddLeaf panics with index out of range instead of bucketing the leaf at depth 15 as the base code did, turning a metrics-only accounting step into a panic during state commit on the witness-collection path.
The prior code could never panic because depth was clamped; the refactor moved indexing into a helper whose own comment documents that it panics beyond depth 15, and no caller-side clamp remains. A graceful degradation (over-deep leaves counted in the last bucket) was silently converted into a crash of the block-processing path.
Evidence:
Changed code:
ownerStat := s.accountTrie
if owner != (common.Hash{}) {
ownerStat = s.storageTrie
}
for i, path := range paths {
if i == len(paths)-1 || !strings.HasPrefix(paths[i+1], paths[i]) {
ownerStat.AddLeaf(len(path))
}
}core/stateless/stats.go ↗ (Base version clamped depth to 15 before indexing the fixed 16-slot arrays; the clamp is deleted in head and not reintroduced anywhere on the path.):
OLD:
depth := len(path)
if owner == (common.Hash{}) {
if depth >= len(s.accountTrieLeaves) {
depth = len(s.accountTrieLeaves) - 1
}
s.accountTrieLeaves[depth] += 1
} else {
if depth >= len(s.storageTrieLeaves) {
depth = len(s.storageTrieLeaves) - 1
}
s.storageTrieLeaves[depth] += 1
...trie/levelstats.go ↗ (Complete decisive helper branch: AddLeaf performs an unchecked index into a [16]stat array and its own doc states it panics for depth outside [0,15], so the removed caller-side clamp is not re-enforced by the new helper.):
const trieStatLevels = 16
// Note: theoretically it is possible to have up to 64 trie levels, but
// LevelStats supports exactly 16 levels and panics on deeper paths.
type LevelStats struct {
level [trieStatLevels]stat
}
// AddLeaf records a leaf depth. ... It panics if the depth is outside [0, 15].
func (s *LevelStats) AddLeaf(depth int) {
s.level[depth].value.Add(1)
}core/state/statedb.go ↗ (Shows Add() is reached during state commit with witness node-path maps when witness stats collection is enabled, so the unchecked index is on a live (opt-in) production path.):
if s.witnessStats != nil {
s.witnessStats.Add(witness, obj.addrHash())
}Suggested fix: Restore the clamp at the call site, e.g. ownerStat.AddLeaf(min(len(path), trieStatLevelsMax)), or make trie.LevelStats.AddLeaf clamp (or ignore) depths outside [0, 15] instead of indexing unchecked.
Suggested test: Add a case to TestWitnessStatsAdd with a node key longer than 15 characters (e.g. strings.Repeat("a", 20)) and assert it is counted at depth 15 rather than panicking.
| return true | ||
| } | ||
| return false | ||
| return true |
There was a problem hiding this comment.
Impact: For a canonical block whose body was pruned from the freezer (below the ancient tail) while its hash-table entry remains readable — possible because Freezer.TruncateTail truncates only tables marked prunable — HasBody now returns true unconditionally via isCanon. Callers that gate on HasBody and then call ReadBody/ReadBodyRLP get nil/empty data (e.g. serving eth block-body requests, chain reconstruction/ancestor walks), turning a clean 'not available' into an inconsistent 'present but unreadable'.
The removed lines were a downstream ancient-pruning guard that explicitly documented that ancient bodies can be pruned and required number >= tail before reporting presence. Reverting to the upstream form reintroduces a HasBody/ReadBody divergence in pruned-ancient deployments — exactly the case the guard protected — and does so silently as part of a sync merge with no stated intent to drop it.
Evidence:
Changed code:
func HasBody(db ethdb.Reader, hash common.Hash, number uint64) bool {
if isCanon(db, number, hash) {
return true
}
if has, err := db.Has(blockBodyKey(number, hash)); !has || err != nil {
return false
}
return true
}
// removed by this hunk:
// // Block is in ancient store, but bodies can be pruned.
// // Check if the block number is above the pruning tail.
...core/rawdb/accessors_chain.go ↗ (isCanon only consults ChainFreezerHashTable; it does not prove the bodies table still holds the item, which is what the removed tail guard checked.):
func isCanon(reader ethdb.AncientReaderOp, number uint64, hash common.Hash) bool {
h, err := reader.Ancient(ChainFreezerHashTable, number)
if err != nil {
return false
}
return bytes.Equal(h, hash[:])
}core/rawdb/freezer.go ↗ (Decisive: tail truncation is selective — non-prunable tables (e.g. the hash table) keep entries below the freezer tail, so isCanon can still resolve a hash for a number whose body was pruned.):
// TruncateTail discards all data below the specified threshold. Note that only
// 'prunable' tables will be truncated.
func (f *Freezer) TruncateTail(tail uint64) (uint64, error) {
...
for _, table := range f.tables {
if table.config.prunable {
if err := table.truncateTail(tail - f.offset.Load()); err != nil {
return 0, err
}
}
}
f.tail.Store(tail)core/rawdb/accessors_chain.go ↗ (Read path reads the bodies table, which IS pruned; so HasBody=true while ReadBody returns nil for the same (hash, number).):
// ReadBodyRLP ...
_ = db.ReadAncients(func(reader ethdb.AncientReaderOp) error {
if isCanon(reader, number, hash) {
data, _ = reader.Ancient(ChainFreezerBodiesTable, number)
return nil
}
data, _ = db.Get(blockBodyKey(number, hash))
return nil
})Suggested fix: Restore the fork's tail guard:
if isCanon(db, number, hash) {
// Block is in ancient store, but bodies can be pruned.
// Check if the block number is above the pruning tail.
tail, _ := db.Tail()
return number >= tail
}
Or, if dropping it is intentional in this sync, confirm in the chain-freezer table config that ChainFreezerHashTable is prunable (so isCanon cannot succeed below the tail) and note that in the PR.
Suggested test: In core/rawdb, freeze N canonical blocks, call TruncateTail to prune early bodies, then assert for a pruned number that HasBody(db, hash, number) == false and agrees with ReadBody(db, hash, number) == nil; also assert both are true/non-nil for a number above the tail.
| PragueBlock *big.Int `json:"pragueBlock,omitempty"` // Prague switch Block (nil = no fork, 0 = already on prague) | ||
| VerkleBlock *big.Int `json:"verkleBlock,omitempty"` // Verkle switch Block (nil = no fork, 0 = already on verkle) | ||
| OsakaBlock *big.Int `json:"osakaBlock,omitempty"` // Osaka switch Block (nil = no fork, 0 = already on osaka) | ||
| AmsterdamBlock *big.Int `json:"amsterdamBlock,omitempty"` // Amsterdam switch Block (nil = no fork, 0 = already on amsterdam) |
There was a problem hiding this comment.
Impact: AmsterdamBlock is a user-settable JSON field (amsterdamBlock) that now drives consensus gates (Rules.IsAmsterdam -> amsterdamInstructionSet, EIP-7843 SlotNumber header validation, precompile touch). Because it is absent from CheckConfigForkOrder's fork list and from checkCompatible, (a) a genesis that schedules amsterdamBlock before osaka/prague or without earlier forks passes validation, and (b) editing amsterdamBlock on an already-synced node past that height yields no ConfigCompatError and no rewind, so the node silently applies different consensus rules to already-imported blocks.
Every other block-scheduled fork in this struct is covered by both the ordering check and the compatibility check; omitting the new one removes the two chokepoints that normally turn a mis-specified or mid-chain-changed fork schedule into a startup error instead of a silent consensus divergence. Today it is latent (nil on all presets) but becomes live as soon as any network or test config sets amsterdamBlock.
Evidence:
Changed code:
OsakaBlock *big.Int `json:"osakaBlock,omitempty"` // Osaka switch Block (nil = no fork, 0 = already on osaka)
AmsterdamBlock *big.Int `json:"amsterdamBlock,omitempty"` // Amsterdam switch Block (nil = no fork, 0 = already on amsterdam)params/config.go ↗ (CheckConfigForkOrder's fork list (params/config.go ~1540-1551) has no amsterdamBlock entry, so an amsterdamBlock configured before/at the same height as an earlier fork is never rejected.):
{name: "pragueBlock", block: c.PragueBlock, optional: true},
{name: "osakaBlock", block: c.OsakaBlock, optional: true},
{name: "verkleBlock", block: c.VerkleBlock, optional: true},
} {params/config.go ↗ (checkCompatible (params/config.go ~1717-1721) ends at Osaka; a changed amsterdamBlock across restarts produces no ConfigCompatError and therefore no chain rewind.):
if isForkBlockIncompatible(c.OsakaBlock, newcfg.OsakaBlock, headNumber) {
return newBlockCompatError("Osaka fork block", c.OsakaBlock, newcfg.OsakaBlock)
}
return nil
}params/config.go ↗ (The field is consumed by consensus/EVM gates (Rules.IsAmsterdam, amsterdamInstructionSet, header SlotNumber validation), so a mis-scheduled or changed value has consensus impact once set.):
func (c *ChainConfig) IsAmsterdam(num *big.Int) bool {
return c.IsLondon(num) && isBlockForked(c.AmsterdamBlock, num)
}Suggested fix: Add {name: "amsterdamBlock", block: c.AmsterdamBlock, optional: true} after the osaka/verkle entries in CheckConfigForkOrder, and add an isForkBlockIncompatible(c.AmsterdamBlock, newcfg.AmsterdamBlock, headNumber) check in checkCompatible, mirroring the Osaka handling.
Suggested test: In params/config_test.go, add a TestCheckCompatible case where old cfg has AmsterdamBlock=10 and new cfg has AmsterdamBlock=20 with head 15, expecting a ConfigCompatError; and a CheckConfigForkOrder case with OsakaBlock=20, AmsterdamBlock=10 expecting an ordering error.
| // Remove the request from the tracked set and restore the peer to the | ||
| // idle pool so it can be reassigned work (skip if peer already left). | ||
| s.lock.Lock() | ||
| delete(s.trienodeHealReqs, req.id) | ||
| if _, ok := s.peers[req.peer]; ok { | ||
| s.trienodeHealIdlers[req.peer] = struct{}{} |
There was a problem hiding this comment.
Impact: On the timeout (and failed-send) revert path, revert*Request closes req.stale and immediately re-adds req.peer to the corresponding idle pool. No cancellation is sent to the peer, so its original request may still be outstanding. The scheduler can therefore assign a new request of the same type to the same unresponsive peer right away, and when the late response eventually arrives the response handler marks the peer idle again while that new request is in flight — yielding concurrent duplicate assignments to a peer that just timed out and removing the implicit timeout backoff upstream gets by restoring idleness only in the response handler. The same pattern is applied in revertAccountRequest/revertBytecodeRequest/revertStorageRequest/revertTrienodeHealRequest/revertBytecodeHealRequest.
Snap-sync scheduling uses the idle pools to steer work away from stalled peers and to keep one outstanding request per peer per type. Immediately re-idling a timed-out peer causes repeated assignment to a slow/stalled peer and can produce two concurrent in-flight requests of the same type for that peer, degrading heal/sync throughput and skewing the per-peer msgrate capacity estimates.
Evidence:
Changed code:
// Remove the request from the tracked set and restore the peer to the
// idle pool so it can be reassigned work (skip if peer already left).
s.lock.Lock()
delete(s.trienodeHealReqs, req.id)
if _, ok := s.peers[req.peer]; ok {
s.trienodeHealIdlers[req.peer] = struct{}{}
}
s.lock.Unlock()eth/protocols/snap/sync.go ↗ (The revert path is reached from the request timeout timer. Reverting only closes req.stale; no cancellation is sent to the peer, so the original request can still be answered later while the peer has already been returned to the idle pool.):
req.timeout = time.AfterFunc(s.rates.TargetTimeout(), func() {
peer.Log().Debug("... request timed out", "reqid", reqid)
s.rates.Update(idle, ..., 0, 0)
s.scheduleRevert...Request(req)
})eth/protocols/snap/sync.go ↗ (Idleness is also restored when the late response finally arrives, so the peer can be marked idle again while a newly assigned request is in flight, producing two concurrent same-type requests to one peer.):
// response handler (e.g. OnTrieNodes)
s.trienodeHealIdlers[peer.ID()] = struct{}{}eth/protocols/snap/sync.go ↗ (Refutes the stale-idler concern for the drop path: s.peers and the idler maps are cleared before peerDrop is signalled, so the new s.peers guard correctly suppresses re-insertion for departed peers. The remaining issue is confined to the timeout/failed-send revert path.):
func (s *Syncer) Unregister(id string) error {
s.lock.Lock()
...
delete(s.peers, id)
...
delete(s.trienodeHealIdlers, id)
s.lock.Unlock()
s.peerDrop.Send(id)Suggested fix: Either keep upstream semantics (restore idleness only in the response handler, and instead handle the never-responding case by dropping/expiring the peer), or gate re-idling so a peer with an unresolved outstanding request of the same type is not re-added — e.g. only re-add on the failed-send/drop revert paths, or track outstanding request counts per peer and re-idle when the count reaches zero.
Suggested test: In eth/protocols/snap/sync_test.go, add a test with a peer that never answers a trienode heal request: assert the peer is not re-assigned another heal request before its previous response is resolved (or, if re-assignment is intended, assert that a late response for the stale reqid does not cause a second concurrent request for the same peer).
| return &ret, nil | ||
| } | ||
|
|
||
| func (b *Block) SlotNumber(ctx context.Context) (*hexutil.Uint64, error) { |
There was a problem hiding this comment.
Impact: graphql-go binds resolver methods only to fields declared in the SDL; an extra Go method is ignored. A client issuing { block { slotNumber } } is rejected at query-validation time with an unknown-field error on type Block, and Block.SlotNumber is never invoked. The EIP-7843 slot number is therefore unavailable over GraphQL even though it is available over JSON-RPC, and the newly added resolver is dead code.
The point of adding the resolver during the v1.17.x upstream sync is to expose header.SlotNumber via GraphQL. Without the SDL field the GraphQL API diverges from both upstream geth and this node's own JSON-RPC surface, and the gap is silent (no build or startup error), so it can persist unnoticed.
Evidence:
Changed code:
func (b *Block) SlotNumber(ctx context.Context) (*hexutil.Uint64, error) {
header, err := b.resolveHeader(ctx)
if err != nil {
return nil, err
}
if header.SlotNumber == nil {
return nil, nil
}
ret := hexutil.Uint64(*header.SlotNumber)
return &ret, nil
}graphql/schema.go ↗ (End of the Block SDL type (lines ~275-279). No slotNumber field is declared, and graphql/schema.go has no diff blocks in this PR, so the new resolver has no schema field to bind to.):
# BlobGasUsed is the total amount of gas used by the transactions.
blobGasUsed: Long
# ExcessBlobGas is a running total of blob gas consumed in excess of the target, prior to the block.
excessBlobGas: Long
}internal/ethapi/api.go ↗ (The JSON-RPC block marshaller already exposes slotNumber, showing the intended API surface that GraphQL now fails to match.):
if head.SlotNumber != nil {
result["slotNumber"] = head.SlotNumberSuggested fix: Add the field to the Block type in graphql/schema.go, immediately after excessBlobGas:
SlotNumber is the EIP-7843 slot number of the block, or null for pre-EIP-7843 blocks.
slotNumber: Long
Suggested test: In graphql/graphql_test.go, add a case querying {block { slotNumber }} against a chain configured with EIP-7843 headers (expect the hex slot value) and against a pre-EIP-7843 chain (expect null), which also guards against the schema/resolver pair drifting apart again.
There was a problem hiding this comment.
Beyond the inline fork-order/compat finding, I also checked whether Amsterdam needs its own case in activePrecompiledContracts/ActivePrecompiles (core/vm/contracts.go) — it doesn't add a distinct precompile set versus Osaka (only SLOTNUM + BAL touch instrumentation), so falling through to the IsOsaka case is correct as long as Osaka activates at or before Amsterdam, which is the same ordering guarantee the inline finding shows is currently unenforced.
Extended reasoning...
Verified by reading core/vm/contracts.go: neither activePrecompiledContracts nor ActivePrecompiles has an IsAmsterdam case, and both fall through to IsOsaka. This is fine on its own since Amsterdam (EIP-7843 SLOTNUM, EIP-8024, BAL precompile-touch) doesn't introduce a new precompile, but it means correctness here is coupled to Osaka activating no later than Amsterdam in config — the exact invariant the inline CheckConfigForkOrder/checkCompatible gap leaves unchecked. Not a new/independent bug, so not filed separately, but worth recording as examined.
| return c.IsLondon(num) && isBlockForked(c.OsakaBlock, num) | ||
| } | ||
|
|
||
| // IsAmsterdam returns whether num is either equal to the Amsterdam fork block or greater. | ||
| func (c *ChainConfig) IsAmsterdam(num *big.Int) bool { | ||
| return c.IsLondon(num) && isBlockForked(c.AmsterdamBlock, num) | ||
| } | ||
|
|
||
| // IsVerkleGenesis checks whether the verkle fork is activated at the genesis block. | ||
| // | ||
| // Verkle mode is considered enabled if the verkle fork time is configured, |
There was a problem hiding this comment.
🟡 params/config.go wires AmsterdamBlock/IsAmsterdam block-based-nil like Osaka/Prague/Verkle, but omits it from the two fork-schedule guards every sibling fork participates in: CheckConfigForkOrder's ordered fork list (ends at verkleBlock, no amsterdamBlock entry) never validates its ordering, and checkCompatible never calls isForkBlockIncompatible(c.AmsterdamBlock, newcfg.AmsterdamBlock, headNumber) alongside the Prague/Verkle/Osaka checks right above it.
Extended reasoning...
The bug. params/config.go introduces AmsterdamBlock *big.Int and IsAmsterdam(num) in this PR, explicitly modeled on OsakaBlock/IsOsaka (both defined a few lines apart, IsAmsterdam(num) = IsLondon(num) && isBlockForked(AmsterdamBlock, num)). OsakaBlock participates in two hardcoded per-fork guard lists that exist specifically to catch consensus-config misconfiguration:
CheckConfigForkOrder's ordered fork-name/block slice — used at startup to reject a config where a later fork's block number is lower than an earlier fork's block number.checkCompatible— used when accepting a new chain config against a chain that has already progressed past certain blocks, to reject silently rescheduling a fork the chain already activated (or vice versa).
AmsterdamBlock was added to the struct and to IsAmsterdam/Rules.IsAmsterdam, but was not added to either list. CheckConfigForkOrder's slice ends at {name: "verkleBlock", block: c.VerkleBlock, optional: true} — there is no amsterdamBlock entry even though pragueBlock/osakaBlock/verkleBlock are all present right next to where it should go. checkCompatible calls isForkBlockIncompatible for PragueBlock, VerkleBlock, and OsakaBlock in sequence but never for AmsterdamBlock.
Why existing code doesn't catch this. Both guards are hand-maintained, hardcoded per-fork lists (no reflection or generic iteration over ChainConfig fields), so adding a new block-based field to the struct does nothing to these lists automatically — the wiring has to be added by hand for every new fork, and it was skipped here for Amsterdam specifically. Nothing else in the codebase provides equivalent protection for AmsterdamBlock.
Concrete proof of the gap:
- Fork-order guard: construct a config with
OsakaBlock = 100andAmsterdamBlock = 50(Amsterdam scheduled before Osaka, which itsIsLondon(num) && isBlockForked(...)gating doesn't logically forbid but which no operator would intend, mirroring how e.g.VerkleBlock < OsakaBlockwould be flagged if it were misordered relative to a listed fork). Callconfig.CheckConfigForkOrder(). BecauseamsterdamBlockisn't in the checked slice, the function walks only the listed forks (…,pragueBlock,osakaBlock,verkleBlock) and returnsnil— no error — even though the schedule is nonsensical. Every other listed fork with the same kind of misordering issue would be caught. - Compatibility guard: take a running chain at head block 1000 with
AmsterdamBlock = 2000(not yet activated). An operator pushes a new config withAmsterdamBlock = 500(now already passed). Calloldcfg.checkCompatible(newcfg, big.NewInt(1000), 0). Since there's noisForkBlockIncompatible(c.AmsterdamBlock, newcfg.AmsterdamBlock, headNumber)call, this rescheduling — which for every other fork (Prague/Verkle/Osaka just above it) is exactly the casecheckCompatibleexists to reject — passes silently. This is the "stealth hard fork" class of bug the compatibility check is designed to prevent.
Impact today vs. going forward. AmsterdamBlock is nil on every current Bor preset, so this is currently unreachable — merging as-is causes no observable failure on any network today. But the PR's own stated premise (per its description: "wired block-based-nil like Osaka/Prague/Verkle... Enabling later is just setting AmsterdamBlock (no core-logic changes)") is false as written: the moment a future preset sets AmsterdamBlock, it silently loses the fork-ordering and config-compatibility guardrails every sibling fork gets, unlike Osaka/Prague/Verkle. This is exactly the wiring-completeness gap this repo's .claude/rules/hardfork-rollout.md calls out as required for every new hardfork-shaped diff.
Fix. Two small additions, mirroring the Osaka pattern exactly:
// in CheckConfigForkOrder's slice, after verkleBlock:
{name: "amsterdamBlock", block: c.AmsterdamBlock, optional: true},
// in checkCompatible, after the OsakaBlock check:
if isForkBlockIncompatible(c.AmsterdamBlock, newcfg.AmsterdamBlock, headNumber) {
return newCompatError("Amsterdam fork block", c.AmsterdamBlock, newcfg.AmsterdamBlock)
}…develop-drift cascade)
Important
Reviewer guide — stacked PR 3 of 12. Part of the combined go-ethereum v1.17.4 + v1.17.5 upstream sync, which ships as one stable release. Every PR in the stack merges into the base branch
upstream-merge-v1.17.4; that base merges 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
Milestone 3/6 of the Bor ← go-ethereum v1.17.4 sync: merges upstream
v1.17.0 → v1.17.1(40 first-parent commits) across 2 batches, plus the milestone-chores docs.Introduces the Amsterdam fork surface (EIP-7843 SLOTNUM, EIP-8024, BAL precompile-touch) — wired block-based-nil like Osaka/Prague/Verkle, dormant on every Bor network. Enabling later is just setting
AmsterdamBlock(no core-logic changes). This also corrects a v1.17.0 gap where upstream's Amsterdam gate had been dropped rather than converted to Bor's block-based style.Stacked on #2319 (v1.17.0). Base retargets up-chain as lower PRs merge + their branches are deleted.
9ecb6c4ae18903086616783c167b6175113d(incl. Amsterdam gate wiring)dbae0f4a1(docs)Executed tests
go build ./...rc=0;go vetclean (bar pre-existing//nolintcopylocks).go test ./...— no v1.17.1 regressions. Two failures confirmed pre-existing/flaky:cmd/gethTestCustomBackend(VEBLOP/non-Bor-genesis nil-deref, crash path byte-identical to v1.17.0) andconsensus/bor/heimdallTestFailover_ThreeClients_CascadeToTertiary(flaky, passes 5/5 on re-run).make test-integration(-tags integration -p 1 ./tests/...) — PASS, incl.tests/borconsensus e2e (620s, 77.6% cov, GitCommitb6175113d).govulncheck— 3 called vulns, all pre-existing/develop-inherent (x/text, x/crypto, crypto/tls); c-kzg v2.1.6 added none.TestReinforceMultiClientPreCompilesTest+TestV2ForkParity+TestBorHardforkPrecompileContinuity*.tests/boris green.Rollout notes
AmsterdamBlockis nil on every preset soIsAmsterdamis false everywhere.docs/upstream-merges/v1.17.4/needs-wiring.md): eth/68 drop (#33511, entangled with Bor's forked downloader/wit/state-sync-receipt/PoA propagation), on-chain-tx-check (#33607), testing_buildBlockV1 (#33656).v2.1.5→v2.1.6, olekukonko→internal/tablewritermigration.🤖 Generated with Claude Code