execution/vm: disable the value transfer in syscall#19277
execution/vm: disable the value transfer in syscall#19277yperbasis merged 9 commits intoerigontech:mainfrom
Conversation
|
Thank you. Looks good |
|
Putting this on hold until #18848 is done. |
mh0lt
left a comment
There was a problem hiding this comment.
I think this change is better handling of this than we currently have in the code which removed these from the bal when it is created here:
erigon/execution/stagedsync/bal_create.go
Line 192 in b39064e
func isSystemBALAddress(addr accounts.Address) bool {
If it is the case that this removed the need to include this check on BAL creation we should think about taking the check out as it potentially masks other issues. We will need to review and retest here.
We also need to review the changes in: #19265 to see if they need to be reverted or adjusted once this change takes effect.
5d94762 to
105b733
Compare
|
@mh0lt can you please take a look when you get a chance? |
|
@yperbasis , @mh0lt will take over this. Some tests which are failing is not resolved yet. Until those are fixed, running the CLI again and again will just waste resources since it will keep failing. @mh0lt will look into it when he gets a chance. |
yperbasis
left a comment
There was a problem hiding this comment.
TestEmptySystemAccountCreation fails
Head branch was pushed to by a user without write access
b09a677 to
6d2043b
Compare
f175490 to
116f183
Compare
0e9f13e to
ea04650
Compare
yperbasis
left a comment
There was a problem hiding this comment.
- Potentially-redundant BAL filter not addressed
As @mh0lt flagged, execution/state/versionedio.go:1174 currently filters SystemAddress out of the BAL when it has no actual account changes:
if account.changes.Address == params.SystemAddress && !hasAccountChanges(account.changes) && !account.nonRevertableUserAccess {
continue
}
If this PR already prevents SystemAddress from entering the BAL for zero-value syscalls, this post-hoc filter is masking whatever residual paths still write it. Worth a separate
investigation (either remove the filter, or document why it's still needed).
- Test coverage gaps
The new TestSystemCallZeroValueSkipsTransferChecks only asserts that the hooks are not invoked. It does not assert:
- SYSTEM_ADDRESS was touched (positive check on the caller-side empty-account creation).
- SYSTEM_ADDRESS is absent from the BAL after the call — that is the actual goal of the PR.
- TestEmptySystemAccountCreation in execution/protocol/rules/aura/aura_test.go:149 still passes (Gnosis regression).
At minimum, add an assertion that reads the caller account after the call and asserts Empty() == true.
- Confusing inline comment (evm.go:346)
} else {
// Calling Transfer is required even for zero-value transfers to
// ensure the usual touch/state-clearing behavior is applied.
The else branch is taken for all non-syscall calls (including non-zero-value ones), not just zero-value transfers. Reword to: "Normal (non-syscall) calls always go through Transfer —
this handles both value movement and the zero-balance touch required for state clearing."
- Dead-ish guard in SubBalance (intra_block_state.go:1149)
if amount.IsZero() {
if addr == params.SystemAddress {
return sdb.TouchAccount(addr)
}
return nil
}
Since evm.call() now avoids Transfer entirely for zero-value syscalls, SubBalance(SystemAddress, 0) from the CALL path is no longer reachable. The branch only matters for other
callers (AuRa engine code, consensus callbacks). That is fine as defense-in-depth, but the comment "Gnosis keeps an empty system account even after Spurious Dragon." should ideally
reference the same PR-5645 / issue-18276 context that the original comment did so the invariant doesn't lose its provenance.
Minor
- syscall := isSystemCall(caller) is computed before typ == CALL || typ == CALLCODE, but is now only used inside the typ == CALL block. Moving it inside the block would tighten scope
— not worth a round-trip though. - The isSystemCall helper is one line; inlining is defensible, but naming aids readability.
aa57f65 to
78d5fe8
Compare
Thanks for the detailed review, addressed everything below: BAL filter ( Test coverage — added
Confusing comment (evm.go:346) — reworded to: "Normal (non-syscall) calls always go through Transfer — this handles both value movement and the zero-balance touch required for state clearing." SubBalance comment — added PR 5645 / Issue 18276 references and a note that this branch is defense-in-depth since Minor (syscall scope) — left as-is per your suggestion. |
yperbasis
left a comment
There was a problem hiding this comment.
CanTransfer guard is now correct but the comment is misleading (execution/vm/evm.go:305-307). The comment was copied from an earlier iteration and says "Calling CanTransfer for
zero-value calls (e.g. system calls) creates spurious balance reads…" — after the PR the value-transfer split has moved downstream, and the only thing the !value.IsZero() guard really
enforces is the geth-style short-circuit. The "pollute the BAL" motivation should live next to the syscall && value.IsZero() → TouchAccount branch, where it now actually applies.
78d5fe8 to
686facb
Compare
Fixed — moved the BAL pollution rationale to the TouchAccount branch and simplified the CanTransfer comment to just reference the geth short-circuit. |
…d regression test
d75889f to
cb4e0cc
Compare
…tics #19277 added `&& !syscall` to the Spurious Dragon zero-value short-circuit in evm.call, so system calls to a non-deployed target now run CreateAccount(addr, false) instead of returning a no-op. That breaks EIP-4788's rule that the beacon-root syscall is a no-op when the contract code is empty — specifically at the fork-transition block, where the beacon-root contract is deployed later in the same block by a CREATE tx. The pre-tx CreateAccount diverts state enough that a later verifier tx underuses gas and the block is rejected with a gas mismatch. Surfaced once #20606 (bal-devnet-4) flipped ExperimentalBAL + Exec3Parallel on by default: eest_blockchain's cancun/eip4788_beacon_root/test_beacon_root_contract_deploy[deploy_on_cancun] goes red with "gas used by execution: 52529, in header: 116552" on block 2. Dropping the `!syscall` clause restores the no-op short-circuit. The rest of #19277 (skip CanTransfer on zero-value calls, TouchAccount(caller) instead of Transfer for syscalls, intra_block_state refactor) is preserved. TestSystemCallZeroValueSkipsTransferChecks still passes (its target already exists, so this path isn't exercised). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tics #19277 added `&& !syscall` to the Spurious Dragon zero-value short-circuit in evm.call, so system calls to a non-deployed target now run CreateAccount(addr, false) instead of returning a no-op. That breaks EIP-4788's rule that the beacon-root syscall is a no-op when the contract code is empty — specifically at the fork-transition block, where the beacon-root contract is deployed later in the same block by a CREATE tx. The pre-tx CreateAccount diverts state enough that a later verifier tx underuses gas and the block is rejected with a gas mismatch. Surfaced once #20606 (bal-devnet-4) flipped ExperimentalBAL + Exec3Parallel on by default: eest_blockchain's cancun/eip4788_beacon_root/test_beacon_root_contract_deploy[deploy_on_cancun] goes red with "gas used by execution: 52529, in header: 116552" on block 2. Dropping the `!syscall` clause restores the no-op short-circuit. The rest of #19277 (skip CanTransfer on zero-value calls, TouchAccount(caller) instead of Transfer for syscalls, intra_block_state refactor) is preserved. TestSystemCallZeroValueSkipsTransferChecks still passes (its target already exists, so this path isn't exercised). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tics (erigontech#20691) ## Summary erigontech#19277 added `&& !syscall` to the Spurious Dragon zero-value short-circuit in `evm.call`, causing system calls to a non-deployed target to run `CreateAccount(addr, false)` instead of returning a no-op. That violates EIP-4788's rule that *"if the contract code is empty, the system call is a no-op"*, specifically at the fork-transition block where the beacon-root contract is deployed later in the same block by a CREATE tx. ## Repro On erigontech#20606 (`bal-devnet-4`), which flips `ExperimentalBAL + Exec3Parallel` on by default, the eest_blockchain suite fails: ``` --- FAIL: TestExecutionSpecBlockchain/cancun/eip4788_beacon_root/ test_beacon_root_contract_deploy.json/.../deploy_on_cancun block_test.go:62: block #[2 0 0 0] insertion into chain failed: BadBlock err: updateForkChoice: [4/6 Execution] invalid block, block=2, gas used by execution: 52529, in header: 116552 ``` Block 1 is the first Cancun block (timestamp=15000, fork point). The EIP-4788 pre-block syscall fires *before* the beacon-root contract is deployed later in block 1's tx 0. Post-erigontech#19277, the syscall calls `CreateAccount(0x000f3df6…, false)` at the beacon-root address, diverting state so block 2's verifier tx underuses gas by ~64k. Reverting only the `!syscall` clause from the short-circuit — while keeping the rest of erigontech#19277 — makes the test green without regressing `TestSystemCallZeroValueSkipsTransferChecks` (its target already exists, so this branch isn't exercised). ## Change `execution/vm/evm.go`, one hunk: ```diff if !exist { - if !isPrecompile && evm.chainRules.IsSpuriousDragon && value.IsZero() && !syscall { + // EIP-4788/6110/7002/7251 system calls to a non-deployed target are + // no-ops ... short-circuit to preserve that at the fork-transition block. + if !isPrecompile && evm.chainRules.IsSpuriousDragon && value.IsZero() { return nil, gas, nil } evm.intraBlockState.CreateAccount(addr, false) } ``` Everything else from erigontech#19277 is preserved: the zero-value `CanTransfer` skip, `TouchAccount(caller)` instead of `Transfer` for syscalls, and the `intra_block_state.go` refactor. ## Validation Local: - [x] `cancun/eip4788_beacon_root/*` — all pass (8.5s) - [x] Full `cancun` eest_blockchain suite — pass (167s) - [x] Full `prague` eest_blockchain suite — pass (85s) - [x] `TestSystemCallZeroValueSkipsTransferChecks` — pass (still green) - [x] `execution/state` + `execution/stagedsync` unit tests — pass - [x] `make lint` — clean ## Test plan - [ ] CI Gate green on this branch - [ ] Rebase erigontech#20606 onto this and confirm `race-tests / execution-eest-blockchain` turns green there ## Related - Introduced by: erigontech#19277 (cherry-pick of geth PR 33741) - Surfaced by: erigontech#20606 (bal-devnet-4 defaults) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: info@weblogix.biz <admin@10gbps.weblogix.it>
In src/ethereum/forks/amsterdam/vm/interpreter.py (lines 299–304), the caller’s address is currently added to the block-level access list only when there is an actual ETH value transfer. This check depends on
should_transfer_valuebeing true and value beingnon-zero. For system transactions, both of these are set to false and zero respectively, so this condition never passes. As a result, the caller address (SYSTEM_ADDRESS) is not tracked in the access list during system calls. This PR implements the same syscall behavior in Erigon, aligning it with Geth and the EIP-7928 specification.