core, consensus/beacon, eth: merge geth v1.17.3 batch 1/7 (v1.17.4 sync, milestone 5/6 part 1) - #2340
Draft
pratikspatil024 wants to merge 24 commits into
Draft
core, consensus/beacon, eth: merge geth v1.17.3 batch 1/7 (v1.17.4 sync, milestone 5/6 part 1)#2340pratikspatil024 wants to merge 24 commits into
pratikspatil024 wants to merge 24 commits into
Conversation
In this PR, we add support for protocol version eth/70, defined by EIP-7975. Overall changes: - Each response is buffered in the peer’s receipt buffer when the `lastBlockIncomplete` field is true. - Continued request uses the same request id of its original request(`RequestPartialReceipts`). - Partial responses are verified in `validateLastBlockReceipt`. - Even if all receipts for partial blocks of the request are collected, those partial results are not sinked to the downloader, to avoid complexity. This assumes that partial response and buffering occur only in exceptional cases. --------- Co-authored-by: Gary Rong <garyrong0905@gmail.com> Co-authored-by: Felix Lange <fjl@twurst.com>
Add persistent storage for Block Access Lists (BALs) in `core/rawdb/`. This provides read/write/delete accessors for BALs in the active key-value store. --------- Co-authored-by: Jared Wasinger <j-wasinger@hotmail.com> Co-authored-by: Gary Rong <garyrong0905@gmail.com>
…110) Add missing `StorageUpdated` and `StorageDeleted` counter increments in the binary trie fast path of `IntermediateRoot()`.
This is a breaking change in the opcode (structLog) tracer. Several fields will have a slight formatting difference to conform to the newly established spec at: ethereum/execution-apis#762. The differences include: - `memory`: words will have the 0x prefix. Also last word of memory will be padded to 32-bytes. - `storage`: keys and values will have the 0x prefix. --------- Co-authored-by: Sina M <1591639+s1na@users.noreply.github.com>
This PR changes the blsync checkpoint init logic so that even if the initialization fails with a certain server and an error log message is printed, the server goes back to its initial state and is allowed to retry initialization after the failure delay period. The previous logic had an `ssDone` server state that did put the server in a permanently unusable state once the checkpoint init failed for an apparently permanent reason. This was not the correct behavior because different servers behave differently in case of overload and sometimes the response to a permanently missing item is not clearly distinguishable from an overload response. A safer logic is to never assume anything to be permanent and always give a chance to retry. The failure delay formula is also fixed; now it is properly capped at `maxFailureDelay`. The previous formula did allow the delay to grow unlimited if a retry was attempted immediately after each delay period.
Block overrides were to a great extent ignored by the gasestimator. This PR fixes that.
This PR implements the missing functionality for archive nodes by pruning stale index data. The current mechanism is relatively simple but sufficient for now: it periodically iterates over index entries and deletes outdated data on a per-block basis. The pruning process is triggered every 90,000 new blocks (approximately every 12 days), and the iteration typically takes ~30 minutes on a mainnet node. This mechanism is only applied with `gcmode=archive` enabled, having no impact on normal full node.
In this PR, the Database interface in `core/state` has been extended with one more function: ```go // Iteratee returns a state iteratee associated with the specified state root, // through which the account iterator and storage iterator can be created. Iteratee(root common.Hash) (Iteratee, error) ``` With this additional abstraction layer, the implementation details can be hidden behind the interface. For example, state traversal can now operate directly on the flat state for Verkle or binary trees, which do not natively support traversal. Moreover, state dumping will now prefer using the flat state iterator as the primary option, offering better efficiency. Edit: this PR also fixes a tiny issue in the state dump, marshalling the next field in the correct way.
Implement the snap/2 wire protocol with BAL serving --------- Co-authored-by: Gary Rong <garyrong0905@gmail.com>
…(#34649) This PR adds Bytes field back to GetAccesListsPacket
The trienode history indexing progress is also exposed via an RPC endpoint and contributes to the eth_syncing status.
👋
This PR makes it possible to run "Amsterdam" in statetests. I'm aware
that they'll be failing and not in consensus with other clients, yet,
but it's nice to be able to run tests and see what works and what
doesn't
Before the change:
```
$ go run ./cmd/evm statetest ./amsterdam.json
[
{
"name": "00000019-mixed-1",
"pass": false,
"fork": "Amsterdam",
"error": "unexpected error: unsupported fork \"Amsterdam\""
}
]
```
After
```
$ go run ./cmd/evm statetest ./amsterdam.json
{"stateRoot": "0x25b78260b76493a783c77c513125c8b0c5d24e058b4e87130bbe06f1d8b9419e"}
[
{
"name": "00000019-mixed-1",
"pass": false,
"stateRoot": "0x25b78260b76493a783c77c513125c8b0c5d24e058b4e87130bbe06f1d8b9419e",
"fork": "Amsterdam",
"error": "post state root mismatch: got 25b78260b76493a783c77c513125c8b0c5d24e058b4e87130bbe06f1d8b9419e, want 0000000000000000000000000000000000000000000000000000000000000000"
}
]
```
…ared pipeline into triedb/internal (#34654) This PR adds `GenerateTrie(db, scheme, root)` to the `triedb` package, which rebuilds all tries from flat snapshot KV data. This is needed by snap/2 sync so it can rebuild the trie after downloading the flat state. The shared trie generation pipeline from `pathdb/verifier.go` was moved into `triedb/internal/conversion.go` so both `GenerateTrie` and `VerifyState` reuse the same code.
This PR refactors the encoding rules for `AccessListsPacket` in the wire protocol. Specifically: - The response is now encoded as a list of `rlp.RawValue` - `rlp.EmptyString` is used as a placeholder for unavailable BAL objects
PathDB keys diff layers by state root, not by block hash. That means a side-chain block can legitimately collide with an existing canonical diff layer when both blocks produce the same post-state (for example same parent, same coinbase, no txs). Today `layerTree.add` blindly inserts that second layer. If the root already exists, this overwrites `tree.layers[root]` and appends the same root to the mutation lookup again. Later account/storage lookups resolve that root to the wrong diff layer, which can corrupt reads for descendant canonical states. At runtime, the corruption is silent: no error is logged and no invariant check fires. State reads against affected descendants simply return stale data from the wrong diff layer (for example, an account balance that reflects one fewer block reward), which can propagate into RPC responses and block validation. This change makes duplicate-root inserts idempotent. A second layer with the same state root does not add any new retrievable state to a tree that is already keyed by root; keeping the original layer preserves the existing parent chain and avoids polluting the lookup history with duplicate roots. The regression test imports a canonical chain of two layers followed by a fork layer at height 1 with the same state root but a different block hash. Before the fix, account and storage lookups at the head resolve the fork layer instead of the canonical one. After the fix, the duplicate insert is skipped and lookups remain correct.
Co-authored-by: Felix Lange <fjl@twurst.com>
…33884) Changes JSON serialization of FilterCriteria to exclude "address" when it is empty.
ProcessBeaconBlockRoot (EIP-4788) and processRequestsSystemCall (EIP-7002/7251) do not merge the EVM access events into the state after execution. ProcessParentBlockHash (EIP-2935) already does this correctly at line 290-291. Without this merge, the Verkle witness will be missing the storage accesses from the beacon root and request system calls, leading to incomplete witnesses and potential consensus issues when Verkle activates.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (44.17%) 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-corevm-catchup #2340 +/- ##
=========================================================
+ Coverage 54.27% 54.33% +0.05%
=========================================================
Files 919 923 +4
Lines 165821 166349 +528
=========================================================
+ Hits 90003 90380 +377
- Misses 70143 70260 +117
- Partials 5675 5709 +34
... and 19 files with indirect coverage changes
🚀 New features to boost your workflow:
|
The batch-20 merge kept Bor's snapshot import, which upstream dropped in #33102, and explained why in a comment placed directly above it. goimports treats a comment inside an import group as a group separator, so the block no longer looked sorted and lint failed. Separating the commented import into its own group keeps the explanation next to the import it explains. gofmt does not check import grouping, which is why the batch's formatting gate passed while CI's golangci-lint run did not.
Ancestry only. The newTrieReader point-cache fix this carries was already present here, 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Merges go-ethereum up to
04e40995d— batch 1 of 7 for the v1.17.3 milestoneof the ongoing v1.17.4 upstream sync. 20 upstream first-parent commits, 30
conflicts, 58 files, +2926/−672.
This is part 1 of the v1.17.3 milestone. The milestone is split because
EIP-7975 / eth/70, deferred in this batch, is adopted as its own stacked PR
immediately after it (see the stack below), and batches 21–26 follow on top of
that. Splitting keeps each PR one reviewable unit: this one is purely an upstream
merge, the next is hand-written Bor code.
Branch cut from the
core/vmcatch-up tip (#2337) rather than fromppatil-upstream-v1.17.2, because batch 21 consumes the fourgas*Intrinsicfunctions that catch-up introduced.
Consensus surface
consensus/beacon/consensus.go(#34064) — upstream added aBlockAccessListHashexistence check next to the existing EIP-7843
SlotNumbercheck. Combined:upstream's two-field body on Bor's block-based
IsAmsterdam(header.Number)gate, so both fields activate together when Amsterdam is enabled. Amsterdam is
dormant (
AmsterdamBlocknil), so the branch asserts both fields are nil, whichis what every Bor header carries today. No behaviour change.
Two whole features deferred
Both are tracked in
docs/upstream-merges/v1.17.4/needs-wiring.mdwith thewiring a future adoption would need. Together they account for 18 of the 30
conflicts.
next PR in the stack, so this is a deferral of days, not of releases. Bor's
receipt code is structurally diverged (a
ReceiptListinterface withper-version implementations, where upstream has one struct), so it needed its
own branch, tests and review rather than a merge resolution. Full 31-file
footprint reverted to Bor HEAD, including auto-merged companions.
lists, which Bor cannot produce while Amsterdam is dormant, so adopting it buys
nothing today and costs a rewrite of Bor's diverged snap serving path. Upstream
itself ships the dispatch commented out (
//case SNAP2:). Tied to theAmsterdam/BAL enable decision.
Other resolutions worth a look
version/version.go— kept Bor'sPatch=0/unstable, declining upstream'sv1.17.3 release bump. This is a recurring auto-merge trap in this sync.
eth/gasestimator(#34081) — took upstream's restructure and re-added Bor'sMadhugiri gate. Bor's side of the conflict referenced a struct field that had
already been dropped by auto-merge, so keeping it would not have compiled.
core/state/{database,database_history}.go— kept Bor'sSnapshot()and itsremoval of
Commit(Bor usesCommitWithUpdate), and re-added upstream'sIteratee()implementations that a hunk-level take-ours had silentlydropped while the interface still required them.
core/blockchain_reader.go— split decision. Reverting the file for #34083'sdeferral had also discarded #34633's adopted
StateIndexProgresssignaturechange; only #34083's part is dropped.
eth/tracers(#33102) — upstream removed thereexecparameter fromStateAtBlock/StateAtTransaction. This broke Bor-only code that raised noconflict at all (the Parity tracer, six files) and surfaced only at build
time; adapted to the new signature rather than reverted.
tests/init.go(#34671) — kept Bor's deletion. Upstream's Amsterdam statetestskey on
AmsterdamTimewith a BPO4 blob schedule, fields Bor lacks. Recorded asa coverage gap.
Full per-file reasoning is in
docs/upstream-merges/v1.17.4/ledger.md. Note thatthis PR carries no doc updates — the v1.17.3 documentation lands in the next
PR in the stack, which is where the batch-20 rows were written.
Executed tests
Beyond CI's standard gates:
go build ./...,go vetovercore,eth,tests,consensus,internal— clean apart from the two pre-existing
//nolintcopylocks.gofmt -lclean.go testgreen oncore/state,core/rawdb,core/types,core/types/bal,consensus/beacon, alleth/tracers/...,eth/protocols/{eth,snap,wit},eth/downloader,core(172.9 s),eth(47.9 s),internal/ethapi.Three defects were caught by build/vet during verification and fixed rather
than shipped: the missing
Iterateeimplementations, the over-broadblockchain_reader.gorevert, and the Parity tracer's dependence on the removedreexecparameter.Rollout notes
dormant: Amsterdam stays nil on all Bor presets, so EIP-7928's header check,
storage prefix, and BAL serving are all inert. No fork gate was flipped.
docs/upstream-merges/v1.17.4/fork-register.md.Stacked PR — do not squash
This is part of a stack. Merge order matters and squash-merging any PR in it
breaks every PR above it, because squashing rewrites commits into new SHAs and
the PRs above would then re-show all of this PR's changes and conflict against
their base. Team standard for upstream syncs is a merge commit, never squash —
preserving upstream's per-commit history and authorship is the whole point.
Reviews are deliberately not being requested yet — the sync is mid-flight and
several decisions are still open. This will be marked ready once all six
milestones are complete.