core, core/state, core/vm, eth/tracers: adopt the deferred core/vm upstream changes (#32919, #33450, #33281, #33637, #33648) - #2337
Draft
pratikspatil024 wants to merge 7 commits into
Conversation
…ework (#32919)
First of five upstream core/vm PRs being un-deferred ahead of the v1.17.3
milestone. All five were merged during the v1.17.4 sync (batches 8 and 18) and
then had their effects reverted at conflict resolution because they clashed with
Bor's BlockSTM StateDB. Milestone 5 batch 21 changes the signature of the four
gas*Intrinsic functions that #33648 introduces, so that batch cannot be merged
until this chain is adopted; #32919 is the root of it.
What upstream changed:
- StateDB.SelfDestruct becomes void and no longer clears the balance. The
balance arithmetic moves into the two EVM opcode handlers, which now do an
explicit SubBalance.
- SelfDestruct6780 is deleted; the "created in this same transaction" check is
exposed separately as StateDB.IsNewContract.
- The code and nonce tracing hooks fire at transaction finalisation rather
than when the opcode executes. This is the PR's actual purpose; the
signature changes are a means to it.
Bor-side work beyond the upstream diff:
- ParallelStateDB is Bor-only and appears in none of the five PRs, so its
adaptation is authored here: SelfDestruct is balance-neutral, and
IsNewContract reads the tx-local newContract map. No MVHashMap read is
recorded for it, because EIP-6780's same-transaction condition can only be
satisfied by this transaction's own writes.
- The MVWrite(BalancePath) inside SelfDestruct is removed along with the
balance handling it accompanied. Conflict detection is unaffected:
SubBalance and AddBalance already record a BalancePath write and force a
balance read, and the opcode handler already performs an MVRead via
GetBalance before either.
- The EIP-7708 selfdestruct branch added in sync batch 19 read
SelfDestruct6780's second return value as a stand-in for IsNewContract,
precisely because this PR was deferred. It now uses IsNewContract directly,
which retires that workaround and matches upstream byte for byte.
- hookedStateDB.Finalise takes upstream's current version, which sorts the
self-destructed addresses before invoking hooks. That ordering is a later
upstream refinement than #32919 itself, and it keeps hook emission
deterministic.
An invariant that reviewers should focus on:
Callers of SelfDestruct now own clearing the balance. On Bor this is
load-bearing for V1/V2 parity rather than a matter of tidiness, because
ParallelStateDB.Exist treats a non-zero balance on an address destructed by an
earlier transaction as an implicit recreation. A leftover balance therefore
makes V2 report the account as existing while the serial path, which deletes
the object outright at Finalise, reports it gone.
All five non-test callers were checked and honour it: both opcode handlers
SubBalance immediately beforehand; the V1 write-set settle path in
statedb.go settles BalancePath in the same loop; the V2 settleAccountSet runs
after settleBalanceOpsAndLogs; hookedStateDB forwards. The invariant is
documented on both SelfDestruct implementations, and the stale
"This clears the account balance" comment is dropped since it is no longer
true.
Four Bor V2 tests were asserting the old contract by calling SelfDestruct
without the handler's SubBalance. They now mirror the real sequence, including
that the handler subtracts the entire balance rather than a fixed amount --
the base-prefunded read-equivalence case only agrees once it does.
Adopting this also removes a latent ordering coupling in the V1 write-set
settle path, where BalancePath and SuicidePath were handled as independent
cases and SelfDestruct's zeroing could clobber a settled balance depending on
which ran last. That interaction disappears now that SelfDestruct is
balance-neutral.
Tests: upstream's selfdestruct state-tracer suite is adopted along with its
seven yul fixtures (661 lines, 12 subtests). Two Bor adaptations were needed --
InsertChain takes Bor's extra makeWitnesses argument, and the beacon-path
subtests need TerminalTotalDifficulty zero plus SetPoS in the generator, since
Bor's validator rejects an ethash difficulty there. Bor's own V1/V2
differential, fuzz and coverage suites are migrated from SelfDestruct6780 to
the IsNewContract-gated shape.
Verified: build and vet clean apart from the two pre-existing nolint copylocks;
gofmt clean; core, core/state/..., eth/tracers/... all pass. core/vm shows only
the documented TestAbortDuringJump interrupt-timing flake. A BlockSTM V2
throughput comparison against BenchmarkV2AllBlocks runs once the full chain of
five is in place; none of the five PRs touch core/evm.go, so the RecordTransfer
V2 fast path is out of scope.
…fordable (#33450) Second of five upstream core/vm PRs being un-deferred ahead of the v1.17.3 milestone. Deferred in sync batch 8 because it sat behind #32919, which is now adopted. makeSelfdestructGasFn charges the EIP-2929 cold-account cost and then probes state -- StateDB.Empty on the beneficiary and GetBalance on the contract -- to decide whether the CreateBySelfdestructGas surcharge applies. If the caller cannot even cover the cold-access cost, those probes are pointless: the call is already doomed. This returns ErrOutOfGas before them. The outcome is unchanged either way, since a caller short of gas ends in out-of-gas with all gas consumed regardless of which check trips first. What changes is that the doomed path no longer reads state, which is the point: EIP-7928 brings state reads into consensus via block access lists, so a call that cannot pay must not record accesses. Bor-relevant consequence: skipping those two reads shrinks the witness and the BlockSTM MVHashMap read set for the out-of-gas case. Deterministic, so it is not a split risk between nodes, and it can only reduce V2 conflict surface rather than grow it. Worth noting explicitly because it is a change to a consensus-critical gas path that is not fork-gated -- it applies on every fork. The early return uses upstream's current form (return 0, ErrOutOfGas) rather than what #33450 originally landed (return gas, nil); a later upstream change tightened it, and taking the current shape keeps the file converged. Bor's readOnly write-protection check is deliberately left in the opcode handler here. Relocating it into the gas handlers is #33281, adopted in the next commit. Verified: build, vet and gofmt clean; core passes, including the fork-parity and precompile-continuity guards. core/vm shows only the documented TestAbortDuringJump interrupt-timing flake.
Third of five upstream core/vm PRs being un-deferred ahead of the v1.17.3
milestone. Both were deferred in sync batch 8 behind #32919.
These two are deliberately combined into one commit because #33637 reverts part
of #33281. Applying them separately would remove the opcode-level write
protection in one commit and re-add it in the next, leaving an intermediate
state on a consensus-critical path that is weaker than either endpoint. The net
effect is what lands here.
#33281 moves the read-only check into the gas handlers so that a write opcode in
a static context terminates during gas metering, before any state is read. The
motivation is EIP-7928: an SSTORE or CALL that is going to fail write protection
must not record state accesses in the block access list.
#33637 then re-adds the checks at the opcode level, keeping the gas-handler ones
in place. Upstream's rationale, worth restating because it drives the Bor-side
finding below: read-only protection should always be enforced at the opcode
level regardless of whether gas metering already checked, so that it acts as a
gatekeeper -- otherwise it is easy to introduce errors by adding new gas
measurement logic without consistently applying the protection.
So the end state is defence in depth: checks in both layers.
What lands:
- gas_table.go: gates in gasSStore, gasSStoreEIP2200, gasCall (value-transfer
only) and gasSelfdestruct.
- operations_acl.go: gates in makeGasSStoreFunc and makeSelfdestructGasFn,
plus the gasCallEIP7702 wrapper that rejects a value-transferring call in a
static context before EIP-7702 delegation resolution loads the target's
code, which would otherwise record the target in the access list for a call
that then fails.
- instructions.go: unchanged. Bor never lost the opcode-level checks, since
#33281 was declined, and the selfdestruct handlers already took upstream's
post-#33637 shape in the first commit of this series.
Bor-specific: makeGasSStoreFuncPIP88 gets the gate too.
This is Bor's own SSTORE gas variant for the PIP-88 cold-storage repricing
(Chicago); upstream has no counterpart, so no upstream diff covers it. Without
the gate, SSTORE under PIP-88 would be the one path where write protection is
enforced at the opcode layer but not during gas metering -- precisely the
inconsistency #33637's rationale warns about, and it would have gone unnoticed
because the opcode-level check still makes the observable behaviour correct.
Bor's other PIP-88 gas function, gasSLoadPIP88, deliberately does not get a
gate: SLOAD is a read, and upstream's gasSLoadEIP2929 has none either.
Convergence check on gate counts against upstream at be4dc0c: gas_table.go
4 and 4, instructions.go 7 and 7, operations_acl.go 4 against upstream's 3 --
the difference being exactly the PIP-88 gate above.
Verified: build, vet and gofmt clean; core and core/vm/... pass, including the
fork-parity and precompile-continuity guards.
…33648)
Last of five upstream core/vm PRs being un-deferred ahead of the v1.17.3
milestone. Deferred in sync batch 8 behind #32919, which is now adopted.
This is the prerequisite the chain existed for. Milestone 5 batch 21 changes the
signature of gasCallIntrinsic, gasCallCodeIntrinsic, gasDelegateCallIntrinsic
and gasStaticCallIntrinsic from (uint64, error) to a gas vector; those four
functions are introduced here. Without this commit batch 21 would be editing
functions Bor does not have, and the only way through would be to decline that
milestone's headline feature.
What upstream changed:
- Each call variant's inner gas calculation is split into a stateless half
(memory expansion, value transfer, EIP-2929 cold access) and a stateful
half (the Empty / Exist probe that decides CallNewAccountGas).
- The stateless half is checked against the remaining gas first, and the
handler returns ErrOutOfGas before performing any state read if it is not
covered. Same motivation as #33450: EIP-7928 brings state reads into
consensus via block access lists, so a call that cannot pay must not
record accesses.
- The 63/64ths split (callGas) moves out of the four gas functions into a
shared makeCallVariantGasCost wrapper, so gasCall, gasCallCode,
gasDelegateCall and gasStaticCall become composed vars rather than
functions. The EIP-7702 wrapper takes the intrinsic functions directly and
applies callGas itself, since it has to interleave its own charges.
Gas equivalence: the total returned is unchanged on every path. In the plain
case it was state + transfer + memory + callGasTemp and is now
intrinsic + callGasTemp with intrinsic being the same three terms. In the
EIP-7702 case it was coldCost + delegationCost + (intrinsic + callGasTemp) and
is now the same four terms added explicitly, with callGas still evaluated after
both UseGas calls so the available-gas base is identical.
The only behavioural delta is the two new early returns, and they can only fire
where the call was already doomed: if the remaining gas is below the intrinsic
cost then the dynamic gas eventually returned exceeds it, so the interpreter's
UseGas fails. Both ErrOutOfGas and the ErrGasUintOverflow that callGas could
previously produce from the same input are non-revert errors, so all gas is
consumed either way and the observable state change is the same. What the early
return removes is the state read on the way there.
Bor-relevant consequences:
- The condition is derived only from contract.Gas, memorySize, the stack and
the chain rules -- nothing state-derived -- so it cannot diverge between
nodes or between the serial and BlockSTM paths.
- For an out-of-gas CALL, the Empty / Exist probe and, on the EIP-7702 path,
the GetCode delegation probe no longer happen. That shrinks the witness and
the V2 MVHashMap read set for exactly the transactions that were going to
abort. Monotone: it can only reduce V2 conflict surface.
- The EIP-2929 and EIP-4762 wrappers still compose the full gasCall, so they
pick up the new shape without changes. operations_verkle.go is already
byte-identical to upstream, so that composition is converged rather than
coincidental.
- The pre-existing underflow in callGas when base exceeds availableGas is
left as upstream leaves it. The CALL early return no longer covers it
(intrinsic includes CallNewAccountGas, which the stateless check does not),
and the outcome is out-of-gas either way.
No PIP-88 counterpart to adjust: Bor's PIP-88 divergence is in SSTORE and SLOAD
gas, not the call variants.
Convergence: core/vm/gas_table.go is now identical to upstream at be4dc0c
except for blank lines, and core/vm/operations_acl.go contains every upstream
line plus Bor's PIP-88 additions -- no upstream line is missing from either.
Verified: build, vet and gofmt clean; core and core/vm/... pass. core/vm shows
only TestAbortDuringJump, re-baselined at the parent commit in a separate
worktree with an identical 12/12 subtest failure count, so it is the documented
fast-vs-slow dispatch timing flake and not a regression from this change.
Five backlog rows move from deferred to adopted, now that the coordinated core/vm catch-up is complete. Follows the #33461 precedent: status flipped, adopting commit recorded, and the "to adopt" cell rewritten to say what was actually done rather than what would need doing. - #32919 selfdestruct rework -> 9cea8e9 - #33644 deterministic Finalise ordering -> 9cea8e9 - #33450 cold-access early return -> d8ad732 - #33281 + #33637 write protection -> 403feb6 - #33648 call-variant gas split -> d292039 Five rather than the four that were planned. #33644 came along with #32919 because upstream's current hookedStateDB.Finalise already carries its sort; the two share the function, so taking one takes the other. #33490's hook infrastructure is not included and stays deferred on its own row. The rows are not deleted. Why each change was originally deferred is the part a reviewer needs in order to judge whether adopting it was right, and that context only exists here. Each rewritten cell records the deviations from a plain upstream port, so they are findable without reading four commit messages: the #33281/#33637 pair is deliberately one commit (#33637 reverts part of #33281), makeGasSStoreFuncPIP88 needed a Bor-only readOnly gate with no upstream counterpart, #33450 takes upstream's current form rather than what the PR originally landed, #32919 removes MVWrite(BalancePath) from SelfDestruct and documents the callers-clear-the-balance invariant that ParallelStateDB.Exist depends on, and #33648 was adopted ahead of the EIP-7928/BAL decision rather than after it because batch 21 changes the signature of the four functions it introduces.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (38.26%) 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.2 #2337 +/- ##
==========================================================
Coverage ? 54.27%
==========================================================
Files ? 919
Lines ? 165821
Branches ? 0
==========================================================
Hits ? 90003
Misses ? 70143
Partials ? 5675
🚀 New features to boost your workflow:
|
This was referenced Aug 3, 2026
core, consensus/beacon, eth: merge geth v1.17.3 batch 1/7 (v1.17.4 sync, milestone 5/6 part 1)
#2340
Draft
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
1 participant
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
Adopts five upstream
core/vmPRs that were merged during the v1.17.4 sync and then hadtheir effects reverted at conflict resolution because they clashed with Bor's BlockSTM
StateDB. All five are already merge-base ancestors of
ppatil-upstream-v1.17.2(they camein with batches 8 and 18), so this is not a merge — each diff was re-applied by hand
against upstream's commit as the reference.
9cea8e922715bf8e81)StateDB.SelfDestructbecomes void and no longer clears the balance,SelfDestruct6780is deleted,IsNewContractis added, and the code/nonce tracing hooks fire at finalisation. Carries #33644 (2eb1ccc6c) with it.d8ad73267b6fb79cdf)ErrOutOfGasfrom the selfdestruct gas handler before probing state, when the caller cannot cover the EIP-2929 cold-account cost.403feb69323c349883) + #33637 (500931bc8)d292039f2fd859638b)gas*Intrinsicfunctions.8b71184d4needs-wiring.mdrows fromdeferredtoadopted.Why now, and why on the stack rather than on
develop. Milestone 5's batch 21 changesthe signature of
gasCallIntrinsic,gasCallCodeIntrinsic,gasDelegateCallIntrinsicandgasStaticCallIntrinsicfrom(uint64, error)to a gas vector. Those four functions areintroduced by #33648. Without this branch, batch 21 would be editing functions Bor does not
have, and the only way through would be to decline milestone 5's headline feature. Doing the
work here rather than as a
develop-side refactor means each change is authored againstupstream's own diff instead of reinvented, and milestone 5 gets cut from this tip so batch 21
merges against the converged shape.
The common thread upstream is EIP-7928 (block access lists), which brings state reads into
consensus: a call or opcode that cannot pay must not record state accesses. BAL is dormant on
Bor, so none of this changes observable behaviour here — but the shapes have to match for the
remaining batches to merge.
The invariant reviewers should focus on
SelfDestructno longer clears the balance; its callers do, via an explicitSubBalance.On Bor this is load-bearing for V1/V2 parity rather than a matter of tidiness:
ParallelStateDB.Existtreats a non-zero balance on an address destructed by an earliertransaction as an implicit recreation. A leftover balance therefore makes V2 report the
account as existing while the serial path, which deletes the object outright at
Finalise,reports it gone.
All five non-test callers were checked and honour it — both opcode handlers
SubBalanceimmediately beforehand, the V1 write-set settle path settles
BalancePathin the same loop,V2's
settleAccountSetruns aftersettleBalanceOpsAndLogs, andhookedStateDBforwards.The invariant is documented on both
SelfDestructimplementations.MVWrite(BalancePath)was removed fromSelfDestructalong with the balance handling.Conflict detection is unaffected:
SubBalance/AddBalancealready record aBalancePathwrite and force a balance read, and the opcode handler already performs an
MVReadviaGetBalancebefore either.Deviations from a plain upstream port
them separately would remove the opcode-level write protection in one commit and re-add it
in the next, leaving an intermediate state on a consensus-critical gas path that is weaker
than either endpoint.
makeGasSStoreFuncPIP88gets areadOnlygate with no upstream counterpart. It isBor's own SSTORE gas variant for the PIP-88 cold-storage repricing. Without the gate,
SSTORE under PIP-88 would be the one path enforced at the opcode layer but not during gas
metering — exactly the inconsistency #33637's rationale warns about, and invisible because
the opcode check keeps observable behaviour correct.
gasSLoadPIP88deliberately does notget one: SLOAD is a read, and upstream's
gasSLoadEIP2929has none either.return 0, ErrOutOfGas) rather than what the PRoriginally landed (
return gas, nil); a later upstream change tightened it, and taking thecurrent shape keeps the file converged.
hookedStateDB.Finaliseis taken wholesale, which is how #33644's deterministicordering arrives. #33490's hook infrastructure is not included and remains deferred.
SelfDestruct6780's second returnvalue as a stand-in for
IsNewContractprecisely because #32919 was deferred; it now usesIsNewContractand matches upstream byte for byte.ParallelStateDBis authored here, not ported. It is Bor-only and appears in none ofthe five upstream diffs.
IsNewContractreads the tx-localnewContractmap and recordsno MVHashMap read, because EIP-6780's same-transaction condition can only be satisfied by
this transaction's own writes.
Convergence
Measured by diffing against upstream
be4dc0c4b:core/vm/gas_table.gocore/vm/operations_acl.gogas_table.gois now identical to upstream except for blank lines, andoperations_acl.gois a strict superset. Gate counts:
gas_table.go4 vs 4,instructions.go7 vs 7,operations_acl.go4 vs upstream's 3 — the extra being the PIP-88 gate above.The EIP-2929 and EIP-4762 wrappers needed no adaptation: both still compose the full
gasCallexactly as upstream does, andoperations_verkle.gois already byte-identical toupstream, so that composition is converged rather than coincidental.
Executed tests
Beyond the standard CI gates:
TestV2BlockSTMAllBlocksmake test-integrationtests/borok, 619.8 s, coverage 77.6 %make lintgo test ./core/govulncheck ./...google.golang.org/grpc@v1.79.3), inherited fromdevelopat an untouched pin — nogo.mod/go.sumchanges in this branchUpstream's selfdestruct state-tracer suite is adopted along with its seven yul fixtures
(661 lines, 12 subtests). Two Bor adaptations were needed:
InsertChaintakes Bor's extramakeWitnessesargument, and the beacon-path subtests needTerminalTotalDifficultyzeroplus
SetPoSin the generator, since Bor's validator rejects an ethash difficulty there.Four Bor V2 tests were asserting the old
SelfDestructcontract and now mirror the realhandler sequence, including that it subtracts the entire balance — the base-prefunded
read-equivalence case only agrees once it does.
BlockSTM V2 throughput
The requirement was not to give back the throughput the newer BlockSTM gained. Three genuine
runs per side of
TestV2BlockSTMAllBlocksover the same 241 mainnet blocks, alternatingbase and branch so load drift hits both equally:
d06fab0e2Medians move +0.7 % on vfails and +0.2 % on wall clock. The branch's minimum vfail count
(9338) is below the base's minimum (9455), against a base spread of 122 — the delta is
inside the jitter floor in both directions. Using
docs/blockstm-v2.md's own conversion(one percentage point of vfail rate ≈ 1 ms per block), a 0.1–0.3 point median move is
0.1–0.3 ms on an 85–160 ms block.
Direction of travel supports this: #33648 and #33450 remove state reads on doomed calls,
which shrinks the witness and the MVHashMap read set. The effect on V2 conflict surface is
monotone downward.
Known gap, stated rather than glossed: there is no
mgas/sfigure across worker counts.BenchmarkV2AllBlocksallocates roughly 11–12 GB per V2 iteration, which thrashed swap onthe measuring machine — one
V2/8witeration took 231 s against a 21 s neighbour, and V2/4wspread ±25 %. Those numbers were discarded rather than reported with a disclaimer. The
benchmark should be re-run on a host that can hold that working set before this PR is marked
ready. Note also that the benchmark is the harness
docs/blockstm-v2.mddescribes asreporting 0–6 validation failures per block against production's 50–60; the differential test
used above is not in that regime (per-block vfails: min 0, median 34, max 127, mean ≈ 39), so
it is the better instrument for a conflict-rate comparison.
Two pre-existing failures were re-baselined rather than assumed:
TestAbortDuringJumpfails12 of 12 subtests both here and at the parent commit in a clean worktree (a fast-vs-slow
dispatch timing race —
runWithAbortsleeps 5 ms before settingevm.abort, and on thatmachine the fast path burns its 10 M gas first), and
go vetreports the same two documented//nolintcopylocks.Rollout notes
path. The only behavioural delta is the new early returns, and they can only fire where the
call was already doomed: if remaining gas is below the intrinsic cost then the dynamic gas
eventually returned exceeds it and the interpreter's
UseGasfails. BothErrOutOfGasandthe
ErrGasUintOverflowthatcallGascould previously produce from the same input arenon-revert errors, so all gas is consumed either way and the observable state change is the
same. What the early return removes is the state read on the way there.
change rather than a feature. The early-out conditions derive only from
contract.Gas,memorySize, the stack and the chain rules; nothing state-derived, so they cannot divergebetween nodes or between the serial and V2 paths.
only shrink the V2 conflict surface.
other client to match.
callGasunderflow whenbase > availableGasis left exactly as upstreamleaves it. The CALL early return does not cover it, since
intrinsicincludesCallNewAccountGaswhile the stateless check does not, and the outcome is out-of-gaseither way.
Full session report, including the raw measurement artifacts and the swap-thrashing evidence,
lives workspace-side under
runs/pos-merge-upstream/2026-07-31T12-15-00Z-claude-corevm-catchup-step4/.