Skip to content

Commit 6165b9c

Browse files
execution/vm: disable the value transfer in syscall (#19277)
In [src/ethereum/forks/amsterdam/vm/interpreter.py (lines 299–304)](https://github.com/ethereum/execution-specs/blob/d22139bf438649d9dbc139ab685b9f9075980253/src/ethereum/forks/amsterdam/vm/interpreter.py#L299), 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_value` being true and value being `non-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](ethereum/go-ethereum#33741) and the EIP-7928 specification. --------- Co-authored-by: Andrew Ashikhmin <34320705+yperbasis@users.noreply.github.com>
1 parent 6a2b0e7 commit 6165b9c

File tree

4 files changed

+196
-27
lines changed

4 files changed

+196
-27
lines changed

execution/state/intra_block_state.go

Lines changed: 33 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -955,22 +955,7 @@ func (sdb *IntraBlockState) AddBalance(addr accounts.Address, amount uint256.Int
955955
// EIP161: We must check emptiness for the objects such that the account
956956
// clearing (0,0,0 objects) can take effect.
957957
if amount.IsZero() {
958-
stateObject, err := sdb.GetOrNewStateObject(addr)
959-
if err != nil {
960-
return err
961-
}
962-
963-
if stateObject.data.Empty() {
964-
versionWritten(sdb, addr, BalancePath, accounts.NilKey, uint256.Int{})
965-
if _, ok := sdb.journal.dirties[addr]; !ok {
966-
if dbg.TraceTransactionIO && (sdb.trace || dbg.TraceAccount(addr.Handle())) {
967-
fmt.Printf("%d (%d.%d) Touch %x\n", sdb.blockNum, sdb.txIndex, sdb.version, addr)
968-
}
969-
sdb.touchAccount(addr)
970-
}
971-
}
972-
973-
return nil
958+
return sdb.TouchAccount(addr)
974959
}
975960

976961
prev, wasCommited, _ := sdb.getBalance(addr)
@@ -1010,6 +995,27 @@ func (sdb *IntraBlockState) touchAccount(addr accounts.Address) {
1010995
}
1011996
}
1012997

998+
// TouchAccount materializes an empty account and records the zero-balance touch
999+
// needed for state clearing and trie consistency.
1000+
func (sdb *IntraBlockState) TouchAccount(addr accounts.Address) error {
1001+
stateObject, err := sdb.GetOrNewStateObject(addr)
1002+
if err != nil {
1003+
return err
1004+
}
1005+
1006+
if stateObject.data.Empty() {
1007+
versionWritten(sdb, addr, BalancePath, accounts.NilKey, uint256.Int{})
1008+
if _, ok := sdb.journal.dirties[addr]; !ok {
1009+
if dbg.TraceTransactionIO && (sdb.trace || dbg.TraceAccount(addr.Handle())) {
1010+
fmt.Printf("%d (%d.%d) Touch %x\n", sdb.blockNum, sdb.txIndex, sdb.version, addr)
1011+
}
1012+
sdb.touchAccount(addr)
1013+
}
1014+
}
1015+
1016+
return nil
1017+
}
1018+
10131019
func (sdb *IntraBlockState) getVersionedAccount(addr accounts.Address, readStorage bool) (*accounts.Account, ReadSource, Version, error) {
10141020
if sdb.versionMap == nil {
10151021
return nil, UnknownSource, UnknownVersion, nil
@@ -1140,10 +1146,17 @@ func (sdb *IntraBlockState) refreshVersionedAccount(addr accounts.Address, readA
11401146
// SubBalance subtracts amount from the account associated with addr.
11411147
// DESCRIBED: docs/programmers_guide/guide.md#address---identifier-of-an-account
11421148
func (sdb *IntraBlockState) SubBalance(addr accounts.Address, amount uint256.Int, reason tracing.BalanceChangeReason) error {
1143-
if amount.IsZero() && addr != params.SystemAddress {
1144-
// We skip this early exit if the sender is the system address
1145-
// because Gnosis has a special logic to create an empty system account
1146-
// even after Spurious Dragon (see PR 5645 and Issue 18276).
1149+
if amount.IsZero() {
1150+
if addr == params.SystemAddress {
1151+
// Gnosis/AuRa keeps an empty system account even after
1152+
// Spurious Dragon (see PR 5645 and Issue 18276).
1153+
//
1154+
// The primary syscall path in evm.call() handles this via
1155+
// TouchAccount directly; this branch is retained as
1156+
// defense-in-depth for other callers (AuRa engine,
1157+
// consensus callbacks).
1158+
return sdb.TouchAccount(addr)
1159+
}
11471160
return nil
11481161
}
11491162

execution/vm/evm.go

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,10 @@ func (evm *EVM) SetCallGasTemp(gas uint64) {
252252
evm.callGasTemp = gas
253253
}
254254

255+
func isSystemCall(caller accounts.Address) bool {
256+
return caller == params.SystemAddress
257+
}
258+
255259
// SetPrecompiles sets the precompiles for the EVM
256260
func (evm *EVM) SetPrecompiles(precompiles PrecompiledContracts) {
257261
evm.precompiles = precompiles
@@ -299,12 +303,11 @@ func (evm *EVM) call(typ OpCode, caller accounts.Address, callerAddress accounts
299303
if depth > int(params.CallCreateDepth) {
300304
return nil, gas, ErrDepth
301305
}
306+
syscall := isSystemCall(caller)
307+
302308
if typ == CALL || typ == CALLCODE {
303309
// Fail if we're trying to transfer more than the available balance.
304-
// Only check when value is non-zero — matching geth's short-circuit
305-
// behavior. Calling CanTransfer for zero-value calls (e.g. system
306-
// calls) creates spurious balance reads on the caller that pollute
307-
// the Block Access List (EIP-7928).
310+
// Skip the check for zero-value calls, matching geth's short-circuit.
308311
if !value.IsZero() {
309312
canTransfer, err := evm.Context.CanTransfer(evm.intraBlockState, caller, value)
310313
if err != nil {
@@ -325,12 +328,27 @@ func (evm *EVM) call(typ OpCode, caller accounts.Address, callerAddress accounts
325328
return nil, mdgas.MdGas{}, fmt.Errorf("%w: %w", ErrIntraBlockStateFailed, err)
326329
}
327330
if !exist {
328-
if !isPrecompile && evm.chainRules.IsSpuriousDragon && value.IsZero() {
331+
if !isPrecompile && evm.chainRules.IsSpuriousDragon && value.IsZero() && !syscall {
329332
return nil, gas, nil
330333
}
331334
evm.intraBlockState.CreateAccount(addr, false)
332335
}
333-
evm.Context.Transfer(evm.intraBlockState, caller, addr, value, bailout, evm.chainRules)
336+
// System calls use TouchAccount instead of Transfer to avoid
337+
// spurious balance reads on the caller that would pollute the
338+
// Block Access List (EIP-7928). The touch is still needed so
339+
// AuRa/Gnosis keeps the empty system account in the PMT.
340+
if syscall && value.IsZero() {
341+
if err := evm.intraBlockState.TouchAccount(caller); err != nil {
342+
return nil, mdgas.MdGas{}, fmt.Errorf("%w: %w", ErrIntraBlockStateFailed, err)
343+
}
344+
} else {
345+
// Normal (non-syscall) calls always go through Transfer —
346+
// this handles both value movement and the zero-balance touch
347+
// required for state clearing.
348+
if err := evm.Context.Transfer(evm.intraBlockState, caller, addr, value, bailout, evm.chainRules); err != nil {
349+
return nil, mdgas.MdGas{}, fmt.Errorf("%w: %w", ErrIntraBlockStateFailed, err)
350+
}
351+
}
334352
} else if typ == STATICCALL {
335353
// We do an AddBalance of zero here, just in order to trigger a touch.
336354
// This doesn't matter on Mainnet, where all empties are gone at the time of Byzantium,
@@ -556,7 +574,9 @@ func (evm *EVM) create(caller accounts.Address, codeAndHash *codeAndHash, gasRem
556574
if evm.chainRules.IsSpuriousDragon {
557575
evm.intraBlockState.SetNonce(address, 1)
558576
}
559-
evm.Context.Transfer(evm.intraBlockState, caller, address, value, bailout, evm.chainRules)
577+
if err := evm.Context.Transfer(evm.intraBlockState, caller, address, value, bailout, evm.chainRules); err != nil {
578+
return nil, accounts.NilAddress, mdgas.MdGas{}, fmt.Errorf("%w: %w", ErrIntraBlockStateFailed, err)
579+
}
560580

561581
// Initialise a new contract and set the code that is to be used by the EVM.
562582
// The contract is a scoped environment for this execution context only.

execution/vm/gas_table_test.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import (
3939
"github.com/erigontech/erigon/db/state/execctx"
4040
"github.com/erigontech/erigon/execution/chain"
4141
"github.com/erigontech/erigon/execution/protocol/mdgas"
42+
"github.com/erigontech/erigon/execution/protocol/params"
4243
"github.com/erigontech/erigon/execution/state"
4344
"github.com/erigontech/erigon/execution/tests/testutil"
4445
"github.com/erigontech/erigon/execution/types/accounts"
@@ -220,3 +221,41 @@ func TestCreateGas(t *testing.T) {
220221
}
221222
tx.Rollback()
222223
}
224+
225+
// TestSystemCallZeroValueSkipsTransferChecks verifies that zero-value system
226+
// calls do not invoke CanTransfer or Transfer.
227+
func TestSystemCallZeroValueSkipsTransferChecks(t *testing.T) {
228+
t.Parallel()
229+
230+
tx, sd := testTemporalTxSD(t)
231+
txNum, _, err := sd.SeekCommitment(t.Context(), tx)
232+
require.NoError(t, err)
233+
234+
r, w := state.NewReaderV3(sd.AsGetter(tx)), state.NewWriter(sd.AsPutDel(tx), nil, txNum)
235+
s := state.New(r)
236+
237+
address := accounts.InternAddress(common.BytesToAddress([]byte("callee")))
238+
239+
canTransferCalled := false
240+
transferCalled := false
241+
canTransferErr := errors.New("can transfer should not be called")
242+
transferErr := errors.New("transfer should not be called")
243+
244+
vmctx := evmtypes.BlockContext{
245+
CanTransfer: func(evmtypes.IntraBlockState, accounts.Address, uint256.Int) (bool, error) {
246+
canTransferCalled = true
247+
return false, canTransferErr
248+
},
249+
Transfer: func(evmtypes.IntraBlockState, accounts.Address, accounts.Address, uint256.Int, bool, *chain.Rules) error {
250+
transferCalled = true
251+
return transferErr
252+
},
253+
}
254+
_ = s.CommitBlock(vmctx.Rules(chain.TestChainBerlinConfig), w)
255+
256+
vmenv := vm.NewEVM(vmctx, evmtypes.TxContext{}, s, chain.TestChainBerlinConfig, vm.Config{})
257+
_, _, err = vmenv.Call(params.SystemAddress, address, nil, mdgas.MdGas{Regular: math.MaxUint64}, uint256.Int{}, false /* bailout */)
258+
require.NoError(t, err)
259+
require.False(t, canTransferCalled, "CanTransfer should be skipped for zero-value system calls")
260+
require.False(t, transferCalled, "Transfer should be skipped for zero-value system calls")
261+
}

execution/vm/runtime/runtime_test.go

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import (
3838
"github.com/erigontech/erigon/execution/chain"
3939
"github.com/erigontech/erigon/execution/protocol"
4040
"github.com/erigontech/erigon/execution/protocol/mdgas"
41+
"github.com/erigontech/erigon/execution/protocol/params"
4142
"github.com/erigontech/erigon/execution/protocol/rules"
4243
"github.com/erigontech/erigon/execution/state"
4344
"github.com/erigontech/erigon/execution/tests/testutil"
@@ -890,3 +891,99 @@ func TestGasTracingNoUnderflowOnStateGas(t *testing.T) {
890891
}
891892
require.True(t, found, "expected at least one GasChangeCallOpCode event from SSTORE")
892893
}
894+
895+
// TestSystemCallZeroValueSkipsTransferChecks verifies that a system call
896+
// (caller = SystemAddress, value = 0) executes successfully without triggering
897+
// CanTransfer or Transfer balance-change hooks on the caller. It also asserts:
898+
// - SYSTEM_ADDRESS was touched and exists after the call (positive check on the
899+
// caller-side empty-account creation for Gnosis/AuRa; see PR 5645, Issue 18276).
900+
// - SYSTEM_ADDRESS remains an empty account after the call.
901+
// - SYSTEM_ADDRESS is absent from the BAL produced by the call's TxIO.
902+
// - No balance-change tracer events fire for SYSTEM_ADDRESS as a result of
903+
// the zero-value transfer path.
904+
func TestSystemCallZeroValueSkipsTransferChecks(t *testing.T) {
905+
t.Parallel()
906+
907+
db := testutil.TemporalDB(t)
908+
tx, domains := testutil.TemporalTxSD(t, db)
909+
statedb := state.New(state.NewReaderV3(domains.AsGetter(tx)))
910+
911+
systemAddr := params.SystemAddress
912+
target := accounts.InternAddress(common.HexToAddress("0xbeef"))
913+
914+
// Deploy a trivial contract at the target that returns 0x42.
915+
statedb.CreateAccount(target, true)
916+
statedb.SetCode(target, []byte{
917+
byte(vm.PUSH1), 0x42,
918+
byte(vm.PUSH1), 0,
919+
byte(vm.MSTORE),
920+
byte(vm.PUSH1), 32,
921+
byte(vm.PUSH1), 0,
922+
byte(vm.RETURN),
923+
})
924+
925+
// Track balance-change events on SYSTEM_ADDRESS.
926+
type balChange struct {
927+
addr accounts.Address
928+
oldBal uint256.Int
929+
newBal uint256.Int
930+
reason tracing.BalanceChangeReason
931+
}
932+
var balChanges []balChange
933+
934+
hooks := &tracing.Hooks{
935+
OnBalanceChange: func(addr accounts.Address, prev, newBal uint256.Int, reason tracing.BalanceChangeReason) {
936+
if addr == systemAddr {
937+
balChanges = append(balChanges, balChange{addr, prev, newBal, reason})
938+
}
939+
},
940+
}
941+
942+
cfg := &Config{
943+
State: statedb,
944+
Origin: systemAddr,
945+
EVMConfig: vm.Config{Tracer: hooks},
946+
GasLimit: 10_000_000,
947+
}
948+
setDefaults(cfg)
949+
950+
vmenv := NewEnv(cfg)
951+
rules := vmenv.ChainRules()
952+
statedb.Prepare(rules, systemAddr, cfg.Coinbase, target, vm.ActivePrecompiles(rules), nil, nil)
953+
954+
ret, _, err := vmenv.Call(
955+
systemAddr,
956+
target,
957+
nil,
958+
mdgas.SplitTxnGasLimit(cfg.GasLimit, mdgas.MdGas{}, rules),
959+
uint256.Int{}, // value = 0
960+
false,
961+
)
962+
require.NoError(t, err)
963+
964+
// The contract should have returned 0x42.
965+
require.Equal(t, 32, len(ret))
966+
require.Equal(t, byte(0x42), ret[31])
967+
968+
// Positive check: SYSTEM_ADDRESS must exist (Gnosis/AuRa invariant).
969+
exists, err := statedb.Exist(systemAddr)
970+
require.NoError(t, err)
971+
require.True(t, exists, "SYSTEM_ADDRESS should exist after a zero-value syscall")
972+
973+
// SYSTEM_ADDRESS must remain empty after the touch.
974+
empty, err := statedb.Empty(systemAddr)
975+
require.NoError(t, err)
976+
require.True(t, empty, "SYSTEM_ADDRESS should remain empty after a zero-value syscall")
977+
978+
// The call-level BAL must not include SYSTEM_ADDRESS when the syscall only
979+
// performs the sender-side touch and no actual account access.
980+
bal := statedb.TxIO().AsBlockAccessList()
981+
for _, accountChanges := range bal {
982+
require.NotEqual(t, systemAddr, accountChanges.Address,
983+
"SYSTEM_ADDRESS should be absent from the BAL after a zero-value syscall")
984+
}
985+
986+
// No balance-change events should have fired for SYSTEM_ADDRESS
987+
// from the zero-value call path.
988+
require.Empty(t, balChanges, "no balance-change events expected for SYSTEM_ADDRESS on zero-value syscall, got %v", balChanges)
989+
}

0 commit comments

Comments
 (0)