From 83794393d0faec7cbd845ba299e2c8bab100f18d Mon Sep 17 00:00:00 2001 From: bysomeone <11565373+bysomeone@users.noreply.github.com> Date: Tue, 3 Nov 2026 14:34:16 +0800 Subject: [PATCH 1/7] fix(evm): make reverted value transfers revertible MemoryStateDB.Transfer writes the new balances straight into the account cache and journals a transferChange that has no revert, so after a reverted value transfer the in-memory cache and the receipt KV disagree; the two are supposed to stay in step, and only one of them is committed. - transferChange now carries the raw pre-transfer KV of both accounts and restores it on revert, deleting keys that did not exist before. - Create takes its snapshot before the value transfer, so a failed create rolls the attached value back together with the account. - Snapshot.revert unwinds entries in reverse creation order, as its interface contract requires. A successful top-level CREATE with an attached value changes too. Its transfer used to run before any snapshot was open, so addChange dropped it: the balances moved but the receipt never carried the keys, and checkKV rejected the transaction under ForkExecRollback (without that fork the value was committed with no receipt entry). From the fork on the snapshot opens first, so both balance keys reach the receipt and the value is committed through it. Production uses executor.StateDB, which has no Delete, so a key created by a reverted transfer is shadowed with Set(key, nil). The key still lands in GetSetKeys while dropping out of the receipt, so checkKV rejects the attack transaction with ErrNotAllowMemSetKey instead of silently reconciling the balance. All of this is gated behind the new ForkEVMTransferRevert. It is registered at MaxHeight (off), so a chain opts in with a height under [fork.sub.evm]; the pre-fork path keeps the old behaviour. Registering it also means every config with a [fork.sub.evm] section must list the key, because initForkConfig rejects registered dapp forks absent from the file. The bundled templates list it as -1, which resolves to MaxHeight. Co-authored-by: Claude --- chain33.fork.toml | 2 + chain33.para.toml | 2 + .../dapp/evm/cmd/ci2/chain33.proxyminer.toml | 2 + .../executor/transfer_revert_checkkv_test.go | 134 +++++++++++++ .../transfer_revert_integration_test.go | 186 ++++++++++++++++++ .../executor/vm/runtime/create_revert_test.go | 124 ++++++++++++ plugin/dapp/evm/executor/vm/runtime/evm.go | 13 +- plugin/dapp/evm/executor/vm/state/snapshot.go | 46 +++++ plugin/dapp/evm/executor/vm/state/statedb.go | 62 ++++++ .../executor/vm/state/transfer_revert_test.go | 182 +++++++++++++++++ plugin/dapp/evm/types/evm.go | 6 +- plugin/dapp/evm/types/types.go | 6 + 12 files changed, 763 insertions(+), 2 deletions(-) create mode 100644 plugin/dapp/evm/executor/transfer_revert_checkkv_test.go create mode 100644 plugin/dapp/evm/executor/transfer_revert_integration_test.go create mode 100644 plugin/dapp/evm/executor/vm/runtime/create_revert_test.go create mode 100644 plugin/dapp/evm/executor/vm/state/transfer_revert_test.go diff --git a/chain33.fork.toml b/chain33.fork.toml index 79c79eece7..14c4e22918 100644 --- a/chain33.fork.toml +++ b/chain33.fork.toml @@ -99,6 +99,8 @@ ForkEVMFixOverflow=0 # SELFDESTRUCT pays from the executing account: DELEGATECALL/CALLCODE no longer # spend the borrowed code contract balance ForkEVMSuicidePayer=0 +# Revertible value transfers; -1 keeps the fix off (resolved to MaxHeight) +ForkEVMTransferRevert=-1 [fork.sub.evmxgo] diff --git a/chain33.para.toml b/chain33.para.toml index 00afc4248b..a977f261ef 100644 --- a/chain33.para.toml +++ b/chain33.para.toml @@ -435,6 +435,8 @@ ForkEVMFixOverflow=0 # SELFDESTRUCT pays from the executing account: DELEGATECALL/CALLCODE no longer # spend the borrowed code contract balance ForkEVMSuicidePayer=0 +# Revertible value transfers; -1 keeps the fix off (resolved to MaxHeight) +ForkEVMTransferRevert=-1 [fork.sub.blackwhite] Enable=0 ForkBlackWhiteV2=0 diff --git a/plugin/dapp/evm/cmd/ci2/chain33.proxyminer.toml b/plugin/dapp/evm/cmd/ci2/chain33.proxyminer.toml index 1316dafbbc..316bbd3466 100644 --- a/plugin/dapp/evm/cmd/ci2/chain33.proxyminer.toml +++ b/plugin/dapp/evm/cmd/ci2/chain33.proxyminer.toml @@ -605,6 +605,8 @@ ForkEVMFixOverflow=0 # SELFDESTRUCT pays from the executing account: DELEGATECALL/CALLCODE no longer # spend the borrowed code contract balance ForkEVMSuicidePayer=0 +# Revertible value transfers; -1 keeps the fix off (resolved to MaxHeight) +ForkEVMTransferRevert=-1 [fork.sub.evmxgo] Enable=0 diff --git a/plugin/dapp/evm/executor/transfer_revert_checkkv_test.go b/plugin/dapp/evm/executor/transfer_revert_checkkv_test.go new file mode 100644 index 0000000000..10928cd226 --- /dev/null +++ b/plugin/dapp/evm/executor/transfer_revert_checkkv_test.go @@ -0,0 +1,134 @@ +// Copyright Fuzamei Corp. 2018 All Rights Reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package executor + +// Framework-level repro of the reverted-transfer attack. The state-level tests use goleveldb, +// which has Delete, so the revert can physically remove the key. Production uses +// executor.StateDB, which only has Set: a key that did not exist before the transfer is +// shadowed with Set(key, nil), and that call still appends the key to GetSetKeys. The receipt +// is assembled from snapshots that no longer hold the reverted transfer, so the key is missing +// from it and chain33's checkKV rejects the whole transaction. execTxOne is unexported, so the +// test reproduces its check against the same StateDB type it inspects. + +import ( + "testing" + + apimock "github.com/33cn/chain33/client/mocks" + "github.com/33cn/chain33/common/address" + "github.com/33cn/chain33/common/db" + cexecutor "github.com/33cn/chain33/executor" + "github.com/33cn/chain33/queue" + "github.com/33cn/chain33/system/address/btc" + ctypes "github.com/33cn/chain33/types" + "github.com/33cn/chain33/util" + vcomm "github.com/33cn/plugin/plugin/dapp/evm/executor/vm/common" + evmtypes "github.com/33cn/plugin/plugin/dapp/evm/types" + "github.com/stretchr/testify/require" +) + +// newFrameworkStateDB returns the production StateDB backed by a queue that answers every +// store lookup with ErrNotFound, so reads of absent keys do not block on a store module. +func newFrameworkStateDB(t *testing.T, cfg *ctypes.Chain33Config, localDB db.KVDB) *cexecutor.StateDB { + t.Helper() + q := queue.New("channel") + q.SetConfig(cfg) + + cli := q.Client() + cli.Sub("store") + done := make(chan struct{}) + go func() { + for { + select { + case msg := <-cli.Recv(): + if msg == nil { + return + } + msg.Reply(cli.NewMessage("", ctypes.EventStoreGetReply, &ctypes.StoreReplyValue{Values: [][]byte{nil}})) + case <-done: + return + } + } + }() + t.Cleanup(func() { + close(done) + q.Close() + }) + + return cexecutor.NewStateDB(cli, nil, localDB, nil).(*cexecutor.StateDB) +} + +// checkReceiptCoversSetKeys mirrors chain33 executor's checkKV: every key recorded by +// StateDB.Set must also be carried by the receipt. +func checkReceiptCoversSetKeys(memset []string, kvs []*ctypes.KeyValue) error { + keys := make(map[string]struct{}, len(kvs)) + for _, kv := range kvs { + keys[string(kv.GetKey())] = struct{}{} + } + for _, key := range memset { + if _, ok := keys[key]; !ok { + return ctypes.ErrNotAllowMemSetKey + } + } + return nil +} + +func TestRevertedTransferTxRejectedByCheckKV(t *testing.T) { + const ( + topUp = uint64(1000) + moved = uint64(400) + ) + + // query_test.go replaces the package-wide EVM address driver with the ETH one; the executor + // globals were initialized with the BTC driver, so restore it for the tx envelope. + driver, err := address.LoadDriver(btc.NormalAddressID, -1) + require.NoError(t, err) + vcomm.InitEvmAddressDriver(driver) + + cfg := newTransferRevertConfig(t, 0) + // checkKV only sees Set keys when the framework begins a per-tx transaction, which it does + // under ForkExecRollback. + cfg.SetFork("ForkExecRollback", 0) + require.True(t, cfg.IsFork(0, "ForkExecRollback")) + + dbDir, rawDB, localDB := util.CreateTestDB() + t.Cleanup(func() { util.CloseTestDB(dbDir, rawDB) }) + + st := newFrameworkStateDB(t, cfg, localDB) + api := new(apimock.QueueProtocolAPI) + api.On("GetConfig").Return(cfg) + if !evmInitOnce { + Init(cfg.ExecName(evmtypes.ExecutorName), cfg, nil) + evmInitOnce = true + } + exec := NewEVMExecutor() + exec.SetAPI(api) + exec.SetLocalDB(localDB) + exec.SetStateDB(st) + exec.SetEnv(0, 0, 0) + exec.CheckInit() + + target := vcomm.BytesToAddress([]byte{0xd0}) + forwarder := deployCode(t, cfg, exec, buildForwarderCode(target, moved)) + caller := deployCode(t, cfg, exec, buildCallerCode(*vcomm.StringToAddress(forwarder), moved)) + fundRole(t, exec, cfg, roleLegitUser, int64(topUp)*2) + + // Model executor.begin(): StartTx plus the in-tx mode that records Set keys. + st.Begin() + receipt, err := exec.Exec(makeCallTx(cfg, roleLegitUser, caller, []byte{0x01}, topUp), 0) + require.NoError(t, err) + require.Equal(t, int32(ctypes.ExecOk), receipt.GetTy()) + + forwarderKey := string(exec.GetMStateDB().CoinsAccount.AccountKey(forwarder)) + require.Contains(t, st.GetSetKeys(), forwarderKey, "the reverted transfer must leave its key behind") + + receiptKeys := make(map[string]struct{}, len(receipt.GetKV())) + for _, kv := range receipt.GetKV() { + receiptKeys[string(kv.GetKey())] = struct{}{} + } + require.NotContains(t, receiptKeys, forwarderKey, "the reverted transfer must not reach the receipt") + + require.Equal(t, ctypes.ErrNotAllowMemSetKey, checkReceiptCoversSetKeys(st.GetSetKeys(), receipt.GetKV()), + "checkKV must reject the transaction instead of committing the phantom balance") +} diff --git a/plugin/dapp/evm/executor/transfer_revert_integration_test.go b/plugin/dapp/evm/executor/transfer_revert_integration_test.go new file mode 100644 index 0000000000..769e419cb8 --- /dev/null +++ b/plugin/dapp/evm/executor/transfer_revert_integration_test.go @@ -0,0 +1,186 @@ +// Copyright Fuzamei Corp. 2018 All Rights Reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package executor + +// Transaction-level repro of the reverted-transfer mint: A calls B with value, B reverts, then +// the phantom credit B kept in the account cache is spent by a later call in the same tx. The +// committed state is assembled from the receipt KV only, so before ForkEVMTransferRevert the +// transaction mints the moved amount. From the fork on the first transfer is rolled back and +// the later spend has no balance to take. + +import ( + "strings" + "testing" + + "github.com/33cn/chain33/common/address" + "github.com/33cn/chain33/system/address/btc" + ctypes "github.com/33cn/chain33/types" + vcomm "github.com/33cn/plugin/plugin/dapp/evm/executor/vm/common" + evmtypes "github.com/33cn/plugin/plugin/dapp/evm/types" + "github.com/stretchr/testify/require" +) + +func newTransferRevertConfig(t *testing.T, forkHeight int64) *ctypes.Chain33Config { + t.Helper() + cfgStr := strings.Replace(ctypes.GetDefaultCfgstring(), `Title="local"`, `Title="integration-test"`, 1) + if !strings.Contains(cfgStr, "[exec.sub.evm]") { + cfgStr += "\n[exec.sub.evm]\nethMapFromExecutor=\"coins\"\nethMapFromSymbol=\"bty\"\n" + } + cfg := ctypes.NewChain33Config(cfgStr) + require.False(t, cfg.IsLocal()) + cfg.SetDappFork(evmtypes.ExecutorName, evmtypes.ForkEVMTransferRevert, forkHeight) + require.Equal(t, forkHeight, cfg.GetDappFork(evmtypes.ExecutorName, evmtypes.ForkEVMTransferRevert)) + return cfg +} + +func pushUint64(v uint64) []byte { + if v == 0 { + return []byte{0x60, 0x00} + } + var b []byte + for n := v; n > 0; n >>= 8 { + b = append([]byte{byte(n)}, b...) + } + return append([]byte{0x60 + byte(len(b)-1)}, b...) +} + +// buildCall emits CALL(callee, value) with the given calldata size (memory offset 0). +func buildCall(callee vcomm.Address, value, inSize uint64) []byte { + code := pushUint64(0) // retSize + code = append(code, pushUint64(0)...) // retOffset + code = append(code, pushUint64(inSize)...) // inSize + code = append(code, pushUint64(0)...) // inOffset + code = append(code, pushUint64(value)...) // value + code = append(code, 0x73) + code = append(code, callee.Bytes()...) + code = append(code, pushUint64(0xffff)...) + code = append(code, 0xf1) // CALL + code = append(code, 0x50) // POP + return code +} + +// buildCallerCode calls callee twice: first with value (the callee aborts), then with one byte +// of calldata, which makes the callee forward its balance. +func buildCallerCode(callee vcomm.Address, value uint64) []byte { + code := buildCall(callee, value, 0) + code = append(code, buildCall(callee, 0, 1)...) + code = append(code, 0x00) // STOP + return code +} + +// buildForwarderCode aborts when calldata is empty, otherwise CALLs target with value. +func buildForwarderCode(target vcomm.Address, value uint64) []byte { + code := []byte{ + 0x36, // CALLDATASIZE + 0x60, 0x08, // PUSH1 8 + 0x57, // JUMPI + 0xfe, // INVALID + 0x00, 0x00, 0x00, // padding + 0x5b, // JUMPDEST @8 + } + code = append(code, buildCall(target, value, 0)...) + code = append(code, 0x00) // STOP + return code +} + +func makeDeployCodeTx(cfg *ctypes.Chain33Config, roleIdx int, code []byte) *ctypes.Transaction { + execAddr := address.ExecAddress(cfg.ExecName(evmtypes.ExecutorName)) + action := &evmtypes.EVMContractAction{ + Amount: 0, GasLimit: 0, GasPrice: 0, + Code: code, ContractAddr: execAddr, + } + tx := &ctypes.Transaction{ + ChainID: cfg.GetChainID(), + Execer: []byte(cfg.ExecName(evmtypes.ExecutorName)), + Payload: ctypes.Encode(action), + Fee: 1e6, To: execAddr, Nonce: 0, + } + signTx(tx, roleIdx) + return tx +} + +func applyReceiptKV(t *testing.T, exec *EVMExecutor, receipt *ctypes.Receipt) { + t.Helper() + for _, kv := range receipt.GetKV() { + require.NoError(t, exec.GetStateDB().Set(kv.GetKey(), kv.GetValue())) + } +} + +// wrapInit turns runtime code into CREATE init code that returns it. +func wrapInit(runtime []byte) []byte { + code := []byte{0x60, byte(len(runtime))} // PUSH1 len + offsetAt := len(code) + code = append(code, 0x60, 0x00) // PUSH1 codeOffset (patched below) + code = append(code, 0x60, 0x00) // PUSH1 0 (mem offset) + code = append(code, 0x39) // CODECOPY + code = append(code, 0x60, byte(len(runtime))) // PUSH1 return size + code = append(code, 0x60, 0x00) // PUSH1 0 (return offset) + code = append(code, 0xf3) // RETURN + code[offsetAt+1] = byte(len(code)) + return append(code, runtime...) +} + +func deployCode(t *testing.T, cfg *ctypes.Chain33Config, exec *EVMExecutor, code []byte) string { + t.Helper() + tx := makeDeployCodeTx(cfg, roleDeployer, wrapInit(code)) + receipt, err := exec.Exec(tx, 0) + require.NoError(t, err) + require.Equal(t, int32(ctypes.ExecOk), receipt.GetTy()) + applyReceiptKV(t, exec, receipt) + return vcomm.NewContractAddress(*vcomm.StringToAddress(tx.From()), tx.Hash()).String() +} + +func TestRevertedCallValueCannotBeSpent(t *testing.T) { + const ( + topUp = uint64(1000) + moved = uint64(400) + ) + + // Other tests in this package (query_test.go) replace the package-wide EVM address driver + // with the ETH one. The executor globals were initialized with the default BTC driver, so + // restore it or the tx envelope address no longer matches the EVM exec address. + driver, err := address.LoadDriver(btc.NormalAddressID, -1) + require.NoError(t, err) + vcomm.InitEvmAddressDriver(driver) + + for _, tc := range []struct { + name string + forkHeight int64 + minted uint64 + }{ + {"pre-fork mints the reverted value", ctypes.MaxHeight, moved}, + {"post-fork mints nothing", 0, 0}, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := newTransferRevertConfig(t, tc.forkHeight) + exec := newTestExecutor(t, cfg, 0) + + target := vcomm.BytesToAddress([]byte{0xd0}) + forwarder := deployCode(t, cfg, exec, buildForwarderCode(target, moved)) + caller := deployCode(t, cfg, exec, buildCallerCode(*vcomm.StringToAddress(forwarder), moved)) + + user := addrFromRole(cfg, roleLegitUser) + fundRole(t, exec, cfg, roleLegitUser, int64(topUp)*2) + + sum := func() int64 { + return balanceOf(exec, user) + balanceOf(exec, caller) + + balanceOf(exec, forwarder) + balanceOf(exec, target.String()) + } + before := sum() + + receipt, err := exec.Exec(makeCallTx(cfg, roleLegitUser, caller, []byte{0x01}, topUp), 0) + require.NoError(t, err) + require.Equal(t, int32(ctypes.ExecOk), receipt.GetTy()) + applyReceiptKV(t, exec, receipt) + + require.Equal(t, before+int64(tc.minted), sum(), "committed total must not grow beyond the minted amount") + require.Equal(t, int64(tc.minted), balanceOf(exec, target.String()), "the forwarder must not spend a phantom credit") + }) + } +} + +func balanceOf(exec *EVMExecutor, addr string) int64 { + return exec.GetMStateDB().CoinsAccount.LoadAccount(addr).Balance +} diff --git a/plugin/dapp/evm/executor/vm/runtime/create_revert_test.go b/plugin/dapp/evm/executor/vm/runtime/create_revert_test.go new file mode 100644 index 0000000000..ac65529613 --- /dev/null +++ b/plugin/dapp/evm/executor/vm/runtime/create_revert_test.go @@ -0,0 +1,124 @@ +// Copyright Fuzamei Corp. 2018 All Rights Reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package runtime + +// A failed nested CREATE used to take its attached value from the caller and keep it at the +// child address whose account was rolled back: Create transferred the value before taking the +// snapshot. From ForkEVMTransferRevert on the snapshot is taken first, so the value returns to +// the caller. + +import ( + "bytes" + "strings" + "testing" + + ctypes "github.com/33cn/chain33/types" + evmtypes "github.com/33cn/plugin/plugin/dapp/evm/types" + "github.com/stretchr/testify/require" +) + +func newCreateRevertEnv(t *testing.T, forkHeight int64) *gapEnv { + t.Helper() + cfgStr := strings.Replace(ctypes.GetDefaultCfgstring(), `Title="local"`, `Title="test"`, 1) + if !strings.Contains(cfgStr, "[exec.sub.evm]") { + cfgStr += "\n[exec.sub.evm]\nethMapFromExecutor=\"coins\"\nethMapFromSymbol=\"bty\"\n" + } + cfg := ctypes.NewChain33Config(cfgStr) + require.False(t, cfg.IsLocal(), "a local config would force every fork to height 0") + cfg.SetDappFork(evmtypes.ExecutorName, evmtypes.ForkEVMTransferRevert, forkHeight) + require.Equal(t, forkHeight, cfg.GetDappFork(evmtypes.ExecutorName, evmtypes.ForkEVMTransferRevert)) + require.Equal(t, forkHeight <= gapTestHeight, + cfg.IsDappFork(gapTestHeight, evmtypes.ExecutorName, evmtypes.ForkEVMTransferRevert)) + return newGapEnvWithCfg(t, cfg, "create-revert") +} + +// buildCreateWithValueCode emits CREATE(value, 0, len(init)) with init appended to the code, +// where init is copied into memory with CODECOPY. init is expected to abort, so the create fails. +func buildCreateWithValueCode(value uint64, init []byte) []byte { + code := []byte{0x60, byte(len(init))} // PUSH1 len(init) + offsetAt := len(code) + code = append(code, 0x60, 0x00) // PUSH1 codeOffset (patched below) + code = append(code, 0x60, 0x00) // PUSH1 0 (mem offset) + code = append(code, 0x39) // CODECOPY + code = append(code, 0x60, byte(len(init))) // PUSH1 size + code = append(code, 0x60, 0x00) // PUSH1 0 (mem offset) + code = append(code, pushUint64(value)...) // PUSH value + code = append(code, 0xf0) // CREATE + code = append(code, 0x00) // STOP + code[offsetAt+1] = byte(len(code)) + return append(code, init...) +} + +func TestFailedNestedCreateReturnsValue(t *testing.T) { + const stash = int64(1000) + const attached = uint64(5) + + for _, tc := range []struct { + name string + forkHeight int64 + wantLeft int64 + }{ + {"pre-fork keeps the old behaviour", ctypes.MaxHeight, stash - int64(attached)}, + {"post-fork returns the value", 0, stash}, + } { + t.Run(tc.name, func(t *testing.T) { + env := newCreateRevertEnv(t, tc.forkHeight) + caller, parent := addr(0x31), addr(0x51) + env.fund(parent, stash) + env.deploy(parent, caller, buildCreateWithValueCode(attached, []byte{0xfe})) + + _, _, _, err := env.evm.Call(AccountRef(caller), parent, nil, 5_000_000, 0) + require.NoError(t, err) + require.Equal(t, tc.wantLeft, env.balance(parent)) + }) + } +} + +// TestSuccessfulCreateWithValueCommitsBalances covers a successful top-level CREATE carrying a +// value. With no snapshot open yet, the pre-fork transfer is dropped by addChange: the balances +// move but the receipt never carries the keys, so chain33's checkKV rejects the transaction (or, +// without ForkExecRollback, commits the value without a receipt entry). From the fork on the +// snapshot opens before the transfer and both keys land in the receipt. +func TestSuccessfulCreateWithValueCommitsBalances(t *testing.T) { + const stash = int64(1000) + const attached = uint64(5) + + for _, tc := range []struct { + name string + forkHeight int64 + wantInReceipt bool + }{ + {"pre-fork drops the balance keys", ctypes.MaxHeight, false}, + {"post-fork commits the balance keys", 0, true}, + } { + t.Run(tc.name, func(t *testing.T) { + env := newCreateRevertEnv(t, tc.forkHeight) + caller, contract := addr(0x61), addr(0x71) + env.fund(caller, stash) + + _, snapshot, _, err := env.evm.Create(AccountRef(caller), contract, []byte{0x00}, 5_000_000, "evm.test", "test", attached) + require.NoError(t, err) + require.Equal(t, stash-int64(attached), env.balance(caller)) + require.Equal(t, int64(attached), env.balance(contract)) + + kvs, _ := env.mdb.GetChangedData(snapshot) + for _, key := range [][]byte{ + env.coins.AccountKey(caller.String()), + env.coins.AccountKey(contract.String()), + } { + require.Equal(t, tc.wantInReceipt, hasKV(kvs, key), "receipt key %x", key) + } + }) + } +} + +func hasKV(kvs []*ctypes.KeyValue, key []byte) bool { + for _, kv := range kvs { + if bytes.Equal(kv.Key, key) { + return true + } + } + return false +} diff --git a/plugin/dapp/evm/executor/vm/runtime/evm.go b/plugin/dapp/evm/executor/vm/runtime/evm.go index 57b112ca86..cb916a6d77 100644 --- a/plugin/dapp/evm/executor/vm/runtime/evm.go +++ b/plugin/dapp/evm/executor/vm/runtime/evm.go @@ -539,8 +539,17 @@ func (evm *EVM) Create(caller ContractRef, contractAddr common.Address, code []b // 向合约地址转账 cfg := evm.StateDB.GetConfig() + // From ForkEVMTransferRevert on, the snapshot is taken before the transfer so that a + // failed create also reverts the attached value (see transferChange.revert). + snapshotBeforeTransfer := cfg.IsDappFork(evm.BlockNumber.Int64(), "evm", evmtypes.ForkEVMTransferRevert) + if snapshotBeforeTransfer { + snapshot = evm.StateDB.Snapshot() + } if cfg.IsDappFork(evm.BlockNumber.Int64(), "evm", evmtypes.ForkEVMFixOverflow) { if value > 0 && !evm.Transfer(evm.StateDB, caller.Address(), contractAddr, value) { + if snapshotBeforeTransfer { + evm.StateDB.RevertToSnapshot(snapshot) + } log.Error("evm create transfer failed", "caller", caller.Address().String(), "contractAddr", contractAddr.String(), "value", value) return nil, -1, gas, model.ErrInsufficientBalance } @@ -563,7 +572,9 @@ func (evm *EVM) Create(caller ContractRef, contractAddr common.Address, code []b contract.SetCallCode(&contractAddr, common.ToHash(code), code) // 创建一个新的账户对象（合约账户） - snapshot = evm.StateDB.Snapshot() + if !snapshotBeforeTransfer { + snapshot = evm.StateDB.Snapshot() + } evm.StateDB.CreateAccount(contractAddr.String(), contract.CallerAddress.String(), execName, alias) if EVMDebugOn == evm.VMConfig.Debug && evm.depth == 0 { diff --git a/plugin/dapp/evm/executor/vm/state/snapshot.go b/plugin/dapp/evm/executor/vm/state/snapshot.go index a84c585b77..9a13ed7b45 100644 --- a/plugin/dapp/evm/executor/vm/state/snapshot.go +++ b/plugin/dapp/evm/executor/vm/state/snapshot.go @@ -42,6 +42,15 @@ func (ver *Snapshot) revert() bool { if ver.entries == nil { return true } + // The interface contract requires undoing changes in reverse creation order. Applying two + // writes to the same slot forward would restore the intermediate value instead of the + // pre-frame one. Legacy behaviour is kept before ForkEVMTransferRevert. + if ver.statedb != nil && ver.statedb.isForkActive(evmtypes.ForkEVMTransferRevert) { + for i := len(ver.entries) - 1; i >= 0; i-- { + ver.entries[i].revert(ver.statedb) + } + return true + } for _, entry := range ver.entries { entry.revert(ver.statedb) } @@ -144,6 +153,17 @@ type ( amount int64 data []*types.KeyValue logs []*types.ReceiptLog + // preState carries the raw account KV read before the transfer. It is only populated + // when ForkEVMTransferRevert is active; empty keeps the legacy no-op revert. + preState []kvPreState + } + + // kvPreState is the raw value of one account key before a transfer wrote to it. + // existed=false means the key was absent and revert must remove it again. + kvPreState struct { + key []byte + value []byte + existed bool } // 合约生成日志事件 @@ -324,3 +344,29 @@ func (ch transferChange) getData(mdb *MemoryStateDB) []*types.KeyValue { func (ch transferChange) getLog(mdb *MemoryStateDB) []*types.ReceiptLog { return ch.logs } + +// revert restores the account KV captured before the transfer. Before ForkEVMTransferRevert +// preState is empty, so a reverted transfer keeps its old behaviour and leaves the cache as is. +// +// executor.StateDB, the KV used in production, has no Delete method, so a key that did not +// exist before the transfer is shadowed with Set(key, nil) instead. That still appends the key +// to GetSetKeys(): the reverted transfer's keys therefore drop out of the receipt while +// remaining in the set, and chain33's checkKV rejects the whole transaction with +// ErrNotAllowMemSetKey. The attack is neutralised as a rejected transaction, not as a silent +// reconciliation. +func (ch transferChange) revert(mdb *MemoryStateDB) { + for _, pre := range ch.preState { + var err error + if pre.existed { + err = mdb.StateDB.Set(pre.key, pre.value) + } else if deleter, ok := mdb.StateDB.(interface{ Delete([]byte) error }); ok { + err = deleter.Delete(pre.key) + } else { + // executor.StateDB has no Delete: a nil value shadows the key as absent. + err = mdb.StateDB.Set(pre.key, nil) + } + if err != nil { + log15.Error("transferChange revert failed", "key", string(pre.key), "err", err) + } + } +} diff --git a/plugin/dapp/evm/executor/vm/state/statedb.go b/plugin/dapp/evm/executor/vm/state/statedb.go index cf2775626c..7e589d9330 100644 --- a/plugin/dapp/evm/executor/vm/state/statedb.go +++ b/plugin/dapp/evm/executor/vm/state/statedb.go @@ -524,6 +524,54 @@ const ( Error ) +// isForkActive reports whether fork is active at the current block height. +func (mdb *MemoryStateDB) isForkActive(fork string) bool { + if mdb.api == nil { + return false + } + cfg := mdb.api.GetConfig() + if cfg == nil { + return false + } + return cfg.IsDappFork(mdb.blockHeight, evmtypes.ExecutorName, fork) +} + +// transferPreState captures the raw account KV touched by Transfer, keyed the same way the +// coins account DB stores them. +func (mdb *MemoryStateDB) transferPreState(sender, recipient string) []kvPreState { + conf := types.ConfSub(mdb.api.GetConfig(), evmtypes.ExecutorName) + ethMapFromExecutor := conf.GStr("ethMapFromExecutor") + if bytes.Equal(types.GetRealExecName([]byte(ethMapFromExecutor)), []byte("coins")) { + return mdb.capturePreState([][]byte{ + mdb.CoinsAccount.AccountKey(sender), + mdb.CoinsAccount.AccountKey(recipient), + }) + } + // paracross: balances live in the exec account of the mapped executor + return mdb.capturePreState([][]byte{ + mdb.execAccountKey(sender), + mdb.execAccountKey(recipient), + }) +} + +func (mdb *MemoryStateDB) execAccountKey(addr string) []byte { + return mdb.CoinsAccount.GetExecKVSet(mdb.evmPlatformAddr, &types.Account{Addr: addr})[0].Key +} + +// capturePreState reads the raw values behind keys before a transfer overwrites them. +func (mdb *MemoryStateDB) capturePreState(keys [][]byte) []kvPreState { + states := make([]kvPreState, 0, len(keys)) + for _, key := range keys { + pre := kvPreState{key: append([]byte(nil), key...)} + if value, err := mdb.StateDB.Get(key); err == nil { + pre.value = append([]byte(nil), value...) + pre.existed = true + } + states = append(states, pre) + } + return states +} + // Transfer 借助coins执行器进行转账相关操作 func (mdb *MemoryStateDB) Transfer(sender, recipient string, amount uint64) bool { log15.Debug("transfer from contract to external(contract)", "sender", sender, "recipient", recipient, "amount", amount) @@ -550,6 +598,10 @@ func (mdb *MemoryStateDB) Transfer(sender, recipient string, amount uint64) bool conf := types.ConfSub(mdb.api.GetConfig(), evmtypes.ExecutorName) ethMapFromExecutor := conf.GStr("ethMapFromExecutor") + var preState []kvPreState + if mdb.isForkActive(evmtypes.ForkEVMTransferRevert) { + preState = mdb.transferPreState(sender, recipient) + } if bytes.Equal(types.GetRealExecName([]byte(ethMapFromExecutor)), []byte("coins")) { ret, err = mdb.CoinsAccount.Transfer(sender, recipient, value) } else { //paracross @@ -567,6 +619,7 @@ func (mdb *MemoryStateDB) Transfer(sender, recipient string, amount uint64) bool amount: value, data: ret.KV, logs: ret.Logs, + preState: preState, }) } @@ -604,6 +657,14 @@ func (mdb *MemoryStateDB) TransferToToken(from, recipient, symbol string, amount if recipient == execaddress { return false, errors.New("not allow") } + // Token accounts use the same non-exec account keys as tokendb.Transfer. + var preState []kvPreState + if mdb.isForkActive(evmtypes.ForkEVMTransferRevert) { + preState = mdb.capturePreState([][]byte{ + tokendb.AccountKey(from), + tokendb.AccountKey(recipient), + }) + } receipt, err := tokendb.Transfer(from, recipient, amount) if err != nil { return false, err @@ -614,6 +675,7 @@ func (mdb *MemoryStateDB) TransferToToken(from, recipient, symbol string, amount amount: amount, data: receipt.GetKV(), logs: receipt.GetLogs(), + preState: preState, }) return true, nil diff --git a/plugin/dapp/evm/executor/vm/state/transfer_revert_test.go b/plugin/dapp/evm/executor/vm/state/transfer_revert_test.go new file mode 100644 index 0000000000..3912f00423 --- /dev/null +++ b/plugin/dapp/evm/executor/vm/state/transfer_revert_test.go @@ -0,0 +1,182 @@ +// Copyright Fuzamei Corp. 2018 All Rights Reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package state + +import ( + "strings" + "testing" + + "github.com/33cn/chain33/account" + apimock "github.com/33cn/chain33/client/mocks" + "github.com/33cn/chain33/common/address" + "github.com/33cn/chain33/common/db" + cexecutor "github.com/33cn/chain33/executor" + ctypes "github.com/33cn/chain33/types" + "github.com/33cn/chain33/util" + "github.com/33cn/plugin/plugin/dapp/evm/executor/vm/common" + evmtypes "github.com/33cn/plugin/plugin/dapp/evm/types" + "github.com/stretchr/testify/require" +) + +const transferRevertTestHeight = int64(10) + +// newTransferRevertConfig builds an EVM config whose ForkEVMTransferRevert resolves to +// forkHeight. The config must not be local: Title="local" pins every dapp fork to 0 via +// needSetForkZero, which would make the pre-fork side unreachable. +func newTransferRevertConfig(t *testing.T, forkHeight int64) *ctypes.Chain33Config { + t.Helper() + cfgStr := strings.Replace(ctypes.GetDefaultCfgstring(), `Title="local"`, `Title="evm-transfer-revert-test"`, 1) + if !strings.Contains(cfgStr, "[exec.sub.evm]") { + cfgStr += "\n[exec.sub.evm]\nethMapFromExecutor=\"coins\"\nethMapFromSymbol=\"bty\"\n" + } + cfg := ctypes.NewChain33Config(cfgStr) + require.False(t, cfg.IsLocal()) + cfg.SetDappFork(evmtypes.ExecutorName, evmtypes.ForkEVMTransferRevert, forkHeight) + require.Equal(t, forkHeight, cfg.GetDappFork(evmtypes.ExecutorName, evmtypes.ForkEVMTransferRevert)) + return cfg +} + +// newTransferRevertMDBWith builds a MemoryStateDB over kv, so tests can pick a KV without +// Delete to reach transferChange.revert's fallback branch. +func newTransferRevertMDBWith(t *testing.T, cfg *ctypes.Chain33Config, kv db.KV) *MemoryStateDB { + t.Helper() + api := new(apimock.QueueProtocolAPI) + api.On("GetConfig").Return(cfg) + coinsAccount, err := account.NewAccountDB(cfg, "coins", cfg.GetCoinSymbol(), kv) + require.NoError(t, err) + + mdb := NewMemoryStateDB(kv, nil, coinsAccount, transferRevertTestHeight, api) + mdb.evmPlatformAddr = address.ExecAddress(cfg.ExecName(evmtypes.ExecutorName)) + return mdb +} + +func newTransferRevertMDB(t *testing.T, forkHeight int64) *MemoryStateDB { + t.Helper() + cfg := newTransferRevertConfig(t, forkHeight) + dbDir, stateDB, localDB := util.CreateTestDB() + t.Cleanup(func() { util.CloseTestDB(dbDir, stateDB) }) + + mdb := newTransferRevertMDBWith(t, cfg, stateDB) + mdb.LocalDB = localDB + return mdb +} + +func fundCoinAccount(mdb *MemoryStateDB, addr string, amount int64) { + acc := mdb.CoinsAccount.LoadAccount(addr) + acc.Balance = amount + mdb.CoinsAccount.SaveAccount(acc) +} + +func coinBalance(mdb *MemoryStateDB, addr string) int64 { + return mdb.CoinsAccount.LoadAccount(addr).Balance +} + +// TestTransferRevertRestoresBalances pins both sides of ForkEVMTransferRevert: after the fork a +// reverted transfer leaves both balances untouched, before it the cache keeps the transfer. +func TestTransferRevertRestoresBalances(t *testing.T) { + const ( + sender = "14KEKbY3kNFLfQEGJbNweV4whre7NpqzuB" + recipient = "1PUiGcbsccfxW3zuvHXZBJfznziph5miAo" + initial = int64(1000) + moved = uint64(400) + ) + + t.Run("pre-fork keeps the old behaviour", func(t *testing.T) { + mdb := newTransferRevertMDB(t, ctypes.MaxHeight) + fundCoinAccount(mdb, sender, initial) + + snapshot := mdb.Snapshot() + require.True(t, mdb.Transfer(sender, recipient, moved)) + require.Equal(t, initial-int64(moved), coinBalance(mdb, sender)) + require.Equal(t, int64(moved), coinBalance(mdb, recipient)) + + mdb.RevertToSnapshot(snapshot) + require.Equal(t, initial-int64(moved), coinBalance(mdb, sender), "pre-fork revert must not restore the sender") + require.Equal(t, int64(moved), coinBalance(mdb, recipient), "pre-fork revert must leave the phantom credit") + }) + + t.Run("post-fork restores both balances", func(t *testing.T) { + mdb := newTransferRevertMDB(t, 0) + fundCoinAccount(mdb, sender, initial) + + snapshot := mdb.Snapshot() + require.True(t, mdb.Transfer(sender, recipient, moved)) + require.Equal(t, initial-int64(moved), coinBalance(mdb, sender)) + require.Equal(t, int64(moved), coinBalance(mdb, recipient)) + + mdb.RevertToSnapshot(snapshot) + require.Equal(t, initial, coinBalance(mdb, sender)) + require.Zero(t, coinBalance(mdb, recipient), "recipient key did not exist before the transfer and must be removed") + }) +} + +// TestSnapshotRevertSameSlotReverseOrder pins the reverse-order revert: two SSTOREs to the +// same slot followed by a revert must restore the pre-frame value, not the intermediate one. +func TestSnapshotRevertSameSlotReverseOrder(t *testing.T) { + contract := common.BytesToAddress([]byte{0x42}) + slot := common.BytesToHash([]byte("slot")) + pre := common.BytesToHash([]byte("pre")) + first := common.BytesToHash([]byte("first")) + second := common.BytesToHash([]byte("second")) + + t.Run("pre-fork restores the intermediate value", func(t *testing.T) { + mdb := newTransferRevertMDB(t, ctypes.MaxHeight) + mdb.CreateAccount(contract.String(), "creator", "evm.test", "test") + mdb.SetState(contract.String(), slot, pre) + + snapshot := mdb.Snapshot() + mdb.SetState(contract.String(), slot, first) + mdb.SetState(contract.String(), slot, second) + require.Equal(t, second, mdb.GetState(contract.String(), slot)) + + mdb.RevertToSnapshot(snapshot) + require.Equal(t, first, mdb.GetState(contract.String(), slot), "forward revert restores the intermediate write") + }) + + t.Run("post-fork restores the pre-frame value", func(t *testing.T) { + mdb := newTransferRevertMDB(t, 0) + mdb.CreateAccount(contract.String(), "creator", "evm.test", "test") + mdb.SetState(contract.String(), slot, pre) + + snapshot := mdb.Snapshot() + mdb.SetState(contract.String(), slot, first) + mdb.SetState(contract.String(), slot, second) + require.Equal(t, second, mdb.GetState(contract.String(), slot)) + + mdb.RevertToSnapshot(snapshot) + require.Equal(t, pre, mdb.GetState(contract.String(), slot), "reverse revert restores the pre-frame value") + }) +} + +// TestTransferRevertWithoutDeleteHidesKey covers transferChange.revert's fallback for a KV with +// no Delete: executor.StateDB, the production implementation, only exposes Set, so a key that +// did not exist before the transfer is shadowed with a nil value, which Get reports as absent. +// The goleveldb used by the other tests has Delete and never reaches this branch. +func TestTransferRevertWithoutDeleteHidesKey(t *testing.T) { + const ( + sender = "14KEKbY3kNFLfQEGJbNweV4whre7NpqzuB" + recipient = "1PUiGcbsccfxW3zuvHXZBJfznziph5miAo" + initial = int64(1000) + moved = uint64(400) + ) + + st := cexecutor.NewStateDB(nil, nil, nil, nil).(*cexecutor.StateDB) + _, hasDelete := interface{}(st).(interface{ Delete([]byte) error }) + require.False(t, hasDelete, "executor.StateDB must not expose Delete or the fallback stays untested") + + mdb := newTransferRevertMDBWith(t, newTransferRevertConfig(t, 0), st) + fundCoinAccount(mdb, sender, initial) + recipientKey := mdb.CoinsAccount.AccountKey(recipient) + + snapshot := mdb.Snapshot() + require.True(t, mdb.Transfer(sender, recipient, moved)) + _, err := mdb.StateDB.Get(recipientKey) + require.NoError(t, err, "the transfer must have written the recipient key") + + mdb.RevertToSnapshot(snapshot) + _, err = mdb.StateDB.Get(recipientKey) + require.Equal(t, ctypes.ErrNotFound, err, "the nil Set must shadow the key as absent") + require.Equal(t, initial, coinBalance(mdb, sender)) +} diff --git a/plugin/dapp/evm/types/evm.go b/plugin/dapp/evm/types/evm.go index 69815486ee..8b43b653cf 100644 --- a/plugin/dapp/evm/types/evm.go +++ b/plugin/dapp/evm/types/evm.go @@ -50,7 +50,11 @@ func InitFork(cfg *types.Chain33Config) { cfg.RegisterDappFork(ExecutorName, ForkEVMFixOverflow, 0) // SELFDESTRUCT payer fix: DELEGATECALL/CALLCODE no longer spend the borrowed code's balance cfg.RegisterDappFork(ExecutorName, ForkEVMSuicidePayer, 0) - + // Revertible value transfers are opt-in per chain. Default to MaxHeight (off) because + // registering 0 would enable the consensus change from genesis on every chain that does + // not configure the key, rewriting historical execution. A chain that wants the fix sets + // a concrete height under [fork.sub.evm]; initForkConfig overrides this default. + cfg.RegisterDappFork(ExecutorName, ForkEVMTransferRevert, types.MaxHeight) } // InitExecutor ... diff --git a/plugin/dapp/evm/types/types.go b/plugin/dapp/evm/types/types.go index efd7d98dd0..d831621b8c 100644 --- a/plugin/dapp/evm/types/types.go +++ b/plugin/dapp/evm/types/types.go @@ -60,6 +60,12 @@ const ( // ForkEVMSuicidePayer SELFDESTRUCT pays from the executing account (Address()) instead of // CodeAddr, so that reading the balance, debiting it and suiciding all use the same account ForkEVMSuicidePayer = "ForkEVMSuicidePayer" + // ForkEVMTransferRevert makes value transfers revertible: transferChange restores the + // pre-transfer account KV, a failed CREATE rolls back its value, and Snapshot.revert + // unwinds entries in reverse order. Without it a reverted transfer stays in the account + // cache while its receipt KV is dropped, so a later call in the same tx can spend the + // phantom balance. Registered in InitFork at MaxHeight, so chains must opt in explicitly. + ForkEVMTransferRevert = "ForkEVMTransferRevert" ) var ( From 545f258e43188c879c5c762dda2af3b802a6e31b Mon Sep 17 00:00:00 2001 From: bysomeone <11565373+bysomeone@users.noreply.github.com> Date: Tue, 3 Nov 2026 21:08:09 +0800 Subject: [PATCH 2/7] fix(evm): undo reverted value transfers through StateDB.RevertSet transferChange.revert restored the pre-transfer balances with Set. Under ForkExecRollback every Set lands in the transaction key list, and chain33's checkKV requires each of those keys to appear in the surviving receipt. A restored key whose only write was the reverted transfer stays in the list but drops out of the receipt, so the whole transaction is rejected: a contract that sends value, sees the call revert and keeps executing turned into a fee-only failed transaction. The mint shape was rejected by the same check, not by reconciling the balance. chain33 now offers StateDB.RevertSet, which undoes one write of the current transaction and also drops its key-list entry. Use it for each captured key; a key that did not exist before the transfer passes nil. KVs without RevertSet, and calls outside a transaction, keep the direct-write path. capturePreState now treats only ErrNotFound as "the key did not exist". Any other read error fails the transfer and logs the key: recording "absent" from a store timeout would let a revert delete a real account. A failed revert is recorded on MemoryStateDB and innerExec turns it into a failed transaction, since the account cache and the receipt no longer describe the same state; RevertToSnapshot itself has no error channel to the EVM. Depends on 33cn/chain33#1412 (StateDB.RevertSet): this PR cannot merge before chain33 releases a version containing it. The replace to the bysomeone fork at 0ba08b76ebaf is the test-time pin and must be dropped when that release lands. Co-authored-by: Claude --- chain33.para.toml | 1 + go.mod | 4 + go.sum | 8 +- plugin/dapp/evm/executor/exec.go | 9 ++ plugin/dapp/evm/executor/vm/state/snapshot.go | 105 ++++++++++++------ plugin/dapp/evm/executor/vm/state/statedb.go | 55 +++++++-- 6 files changed, 135 insertions(+), 47 deletions(-) diff --git a/chain33.para.toml b/chain33.para.toml index a977f261ef..5717e2ec85 100644 --- a/chain33.para.toml +++ b/chain33.para.toml @@ -437,6 +437,7 @@ ForkEVMFixOverflow=0 ForkEVMSuicidePayer=0 # Revertible value transfers; -1 keeps the fix off (resolved to MaxHeight) ForkEVMTransferRevert=-1 + [fork.sub.blackwhite] Enable=0 ForkBlackWhiteV2=0 diff --git a/go.mod b/go.mod index 42cf2ce825..f464829300 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,10 @@ toolchain go1.22.12 replace github.com/ava-labs/avalanchego => github.com/33cn/avalanchego v1.10.10-0.20240529041529-ada691598153 +// Temporary test-time pin: the EVM transfer-revert fix calls StateDB.RevertSet, which comes +// from 33cn/chain33#1412. Drop this replace once chain33 releases a version containing it. +replace github.com/33cn/chain33 => github.com/bysomeone/chain33 v1.72.1-0.20261103124951-0ba08b76ebaf + require ( github.com/33cn/chain33 v1.72.7-0.20261016082959-e1bde75e25d0 github.com/BurntSushi/toml v1.3.2 diff --git a/go.sum b/go.sum index ef58f33604..8e4cef180f 100644 --- a/go.sum +++ b/go.sum @@ -46,10 +46,6 @@ dmitri.shuralyov.com/html/belt v0.0.0-20180602232347-f7d459c86be0/go.mod h1:JLBr dmitri.shuralyov.com/service/change v0.0.0-20181023043359-a85b471d5412/go.mod h1:a1inKt/atXimZ4Mv927x+r7UpyzRUf4emIoiiSC2TN4= dmitri.shuralyov.com/state v0.0.0-20180228185332-28bcc343414c/go.mod h1:0PRwlb0D6DFvNNtx+9ybjezNCa8XF0xaYcETyp6rHWU= git.apache.org/thrift.git v0.0.0-20180902110319-2566ecd5d999/go.mod h1:fPE2ZNJGynbRyZ4dJvy6G277gSllfV2HJqblrnkyeyg= -github.com/33cn/chain33 v1.72.0 h1:Nk1CYLSYd5rfvhw1szE2Jt0cJx3A8nZCZZvIEIfTarw= -github.com/33cn/chain33 v1.72.0/go.mod h1:QCdEczXYJI43WTeFuzEpEhNZ/SfEnLOdFaw6r6LvPz8= -github.com/33cn/chain33 v1.72.7-0.20261016082959-e1bde75e25d0 h1:r9UFvcR9Sg9g3UBte5QX//e4e45F9DFxYhm4MnaaRlg= -github.com/33cn/chain33 v1.72.7-0.20261016082959-e1bde75e25d0/go.mod h1:6C5lKl8ejjoXtyhetFXz13t+JV4UQvp9H4t9eLOTNXs= github.com/AndreasBriese/bbloom v0.0.0-20190825152654-46b345b51c96 h1:cTp8I5+VIoKjsnZuH8vjyaysT/ses3EvZeaV/1UkF2M= github.com/AndreasBriese/bbloom v0.0.0-20190825152654-46b345b51c96/go.mod h1:bOvUY6CB00SOBii9/FifXqc0awNKxLFCL/+pkDPuyl8= github.com/Azure/azure-sdk-for-go v41.3.0+incompatible/go.mod h1:9XXNKU+eRnpl9moKnB4QOLf1HestfXbmab5FXxiDBjc= @@ -247,6 +243,8 @@ github.com/btcsuite/websocket v0.0.0-20150119174127-31079b680792 h1:R8vQdOQdZ9Y3 github.com/btcsuite/websocket v0.0.0-20150119174127-31079b680792/go.mod h1:ghJtEyQwv5/p4Mg4C0fgbePVuGr935/5ddU9Z3TmDRY= github.com/btcsuite/winsvc v1.0.0/go.mod h1:jsenWakMcC0zFBFurPLEAyrnc/teJEM1O46fmI40EZs= github.com/buger/jsonparser v0.0.0-20181115193947-bf1c66bbce23/go.mod h1:bbYlZJ7hK1yFx9hf58LP0zeX7UjIGs20ufpu3evjr+s= +github.com/bysomeone/chain33 v1.72.1-0.20261103124951-0ba08b76ebaf h1:rY3/3AOPt21SQT88dUe8y/D2KqZkA0QvwyoeAM0Xpck= +github.com/bysomeone/chain33 v1.72.1-0.20261103124951-0ba08b76ebaf/go.mod h1:6C5lKl8ejjoXtyhetFXz13t+JV4UQvp9H4t9eLOTNXs= github.com/c-bata/go-prompt v0.2.2 h1:uyKRz6Z6DUyj49QVijyM339UJV9yhbr70gESwbNU3e0= github.com/c-bata/go-prompt v0.2.2/go.mod h1:VzqtzE2ksDBcdln8G7mk2RX9QyGjH+OVqOCSiVIqS34= github.com/cactus/go-statsd-client/statsd v0.0.0-20191106001114-12b4e2b38748/go.mod h1:l/bIBLeOl9eX+wxJAzxS4TveKRtAqlyDpHjhkfO0MEI= @@ -1358,8 +1356,6 @@ github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8 github.com/supranational/blst v0.3.11 h1:LyU6FolezeWAhvQk0k6O/d49jqgO52MSDDfYgbeoEm4= github.com/supranational/blst v0.3.11/go.mod h1:jZJtfjgudtNl4en1tzwPIV3KjUnQUvG3/j+w+fVonLw= github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7/go.mod h1:q4W45IWZaF22tdD+VEXcAWRA037jwmWEB5VWYORlTpc= -github.com/syndtr/goleveldb v1.0.1-0.20220614013038-64ee5596c38a h1:1ur3QoCqvE5fl+nylMaIr9PVV1w343YRDtsy+Rwu7XI= -github.com/syndtr/goleveldb v1.0.1-0.20220614013038-64ee5596c38a/go.mod h1:RRCYJbIwD5jmqPI9XoAFR0OcDxqUctll6zUj/+B4S48= github.com/syndtr/goleveldb v1.0.1-0.20220721030215-126854af5e6d h1:vfofYNRScrDdvS342BElfbETmL1Aiz3i2t0zfRj16Hs= github.com/syndtr/goleveldb v1.0.1-0.20220721030215-126854af5e6d/go.mod h1:RRCYJbIwD5jmqPI9XoAFR0OcDxqUctll6zUj/+B4S48= github.com/tarm/serial v0.0.0-20180830185346-98f6abe2eb07/go.mod h1:kDXzergiv9cbyO7IOYJZWg1U88JhDg3PB6klq9Hg2pA= diff --git a/plugin/dapp/evm/executor/exec.go b/plugin/dapp/evm/executor/exec.go index 8a57e37211..d51ac1773f 100644 --- a/plugin/dapp/evm/executor/exec.go +++ b/plugin/dapp/evm/executor/exec.go @@ -58,6 +58,7 @@ func (evm *EVMExecutor) Exec(tx *types.Transaction, index int) (*types.Receipt, func (evm *EVMExecutor) innerExec(msg *common.Message, txHash []byte, sigType int32, index int, txFee uint64, readOnly bool) (receipt *types.Receipt, err error) { cfg := evm.GetAPI().GetConfig() + evm.mStateDB.ResetRevertError() // 获取当前区块的上下文信息构造EVM上下文 context := evm.NewEVMContext(msg, txHash) execAddr := evm.getEvmExecAddress() @@ -191,6 +192,14 @@ func (evm *EVMExecutor) innerExec(msg *common.Message, txHash []byte, sigType in return receipt, vmerr } + // A snapshot revert that failed leaves the account cache and the receipt describing + // different state, which chain33's checkKV can no longer validate. Fail the transaction + // (fee-only receipt, state rolled back by the framework) instead of committing it. + if rerr := evm.mStateDB.RevertError(); rerr != nil { + log.Error("innerExec snapshot revert failed", "error info", rerr) + return receipt, rerr + } + // 计算消耗了多少费用（实际消耗的费用） usedFee, overflow := common.SafeMul(usedGas, uint64(msg.GasPrice())) // 费用消耗溢出，执行失败 diff --git a/plugin/dapp/evm/executor/vm/state/snapshot.go b/plugin/dapp/evm/executor/vm/state/snapshot.go index 9a13ed7b45..8151837a49 100644 --- a/plugin/dapp/evm/executor/vm/state/snapshot.go +++ b/plugin/dapp/evm/executor/vm/state/snapshot.go @@ -5,9 +5,12 @@ package state import ( + "errors" + "fmt" "sort" "github.com/33cn/chain33/common/log/log15" + cexecutor "github.com/33cn/chain33/executor" "github.com/33cn/chain33/types" "github.com/33cn/plugin/plugin/dapp/evm/executor/vm/common" @@ -20,7 +23,7 @@ import ( // 如果合约执行出错，会按生成顺序的倒序，依次调用变更对象的回滚接口进行数据回滚，并同步删除变更对象缓存 // 如果合约执行成功，会按生成顺序的正序，依次调用变更对象的数据和日志变更记录，回传给区块链 type DataChange interface { - revert(mdb *MemoryStateDB) + revert(mdb *MemoryStateDB) error getData(mdb *MemoryStateDB) []*types.KeyValue getLog(mdb *MemoryStateDB) []*types.ReceiptLog } @@ -38,23 +41,27 @@ func (ver *Snapshot) GetID() int { } // 回滚当前版本 -func (ver *Snapshot) revert() bool { +func (ver *Snapshot) revert() error { if ver.entries == nil { - return true + return nil } // The interface contract requires undoing changes in reverse creation order. Applying two // writes to the same slot forward would restore the intermediate value instead of the // pre-frame one. Legacy behaviour is kept before ForkEVMTransferRevert. if ver.statedb != nil && ver.statedb.isForkActive(evmtypes.ForkEVMTransferRevert) { for i := len(ver.entries) - 1; i >= 0; i-- { - ver.entries[i].revert(ver.statedb) + if err := ver.entries[i].revert(ver.statedb); err != nil { + return err + } } - return true + return nil } for _, entry := range ver.entries { - entry.revert(ver.statedb) + if err := entry.revert(ver.statedb); err != nil { + return err + } } - return true + return nil } // 添加变更数据 @@ -181,7 +188,8 @@ type ( ) // 在baseChang中定义三个基本操作，子对象中只需要实现必要的操作 -func (ch baseChange) revert(s *MemoryStateDB) { +func (ch baseChange) revert(s *MemoryStateDB) error { + return nil } func (ch baseChange) getData(s *MemoryStateDB) (kvset []*types.KeyValue) { @@ -193,8 +201,9 @@ func (ch baseChange) getLog(s *MemoryStateDB) (logs []*types.ReceiptLog) { } // 创建账户对象的回滚，需要删除缓存中的账户和变更标记 -func (ch createAccountChange) revert(s *MemoryStateDB) { +func (ch createAccountChange) revert(s *MemoryStateDB) error { delete(s.accounts, ch.account) + return nil } // 创建账户对象的数据集 @@ -208,15 +217,16 @@ func (ch createAccountChange) getData(s *MemoryStateDB) (kvset []*types.KeyValue return nil } -func (ch suicideChange) revert(mdb *MemoryStateDB) { +func (ch suicideChange) revert(mdb *MemoryStateDB) error { // 如果已经自杀过了，不处理 if ch.prev { - return + return nil } acc := mdb.accounts[ch.account] if acc != nil { acc.State.Suicided = ch.prev } + return nil } func (ch suicideChange) getData(mdb *MemoryStateDB) []*types.KeyValue { @@ -231,11 +241,12 @@ func (ch suicideChange) getData(mdb *MemoryStateDB) []*types.KeyValue { return nil } -func (ch nonceChange) revert(mdb *MemoryStateDB) { +func (ch nonceChange) revert(mdb *MemoryStateDB) error { acc := mdb.accounts[ch.account] if acc != nil { acc.State.Nonce = ch.prev } + return nil } func (ch nonceChange) getData(mdb *MemoryStateDB) []*types.KeyValue { @@ -247,12 +258,13 @@ func (ch nonceChange) getData(mdb *MemoryStateDB) []*types.KeyValue { return nil } -func (ch codeChange) revert(mdb *MemoryStateDB) { +func (ch codeChange) revert(mdb *MemoryStateDB) error { acc := mdb.accounts[ch.account] if acc != nil { acc.Data.Code = ch.prevcode acc.Data.CodeHash = ch.prevhash } + return nil } func (ch codeChange) getData(mdb *MemoryStateDB) (kvset []*types.KeyValue) { @@ -265,11 +277,12 @@ func (ch codeChange) getData(mdb *MemoryStateDB) (kvset []*types.KeyValue) { return nil } -func (ch abiChange) revert(mdb *MemoryStateDB) { +func (ch abiChange) revert(mdb *MemoryStateDB) error { acc := mdb.accounts[ch.account] if acc != nil { acc.Data.Abi = ch.prevabi } + return nil } func (ch abiChange) getData(mdb *MemoryStateDB) (kvset []*types.KeyValue) { @@ -281,11 +294,12 @@ func (ch abiChange) getData(mdb *MemoryStateDB) (kvset []*types.KeyValue) { return nil } -func (ch storageChange) revert(mdb *MemoryStateDB) { +func (ch storageChange) revert(mdb *MemoryStateDB) error { acc := mdb.accounts[ch.account] if acc != nil { acc.SetState(ch.key, ch.prevalue) } + return nil } func (ch storageChange) getData(mdb *MemoryStateDB) []*types.KeyValue { @@ -309,11 +323,12 @@ func (ch storageChange) getLog(mdb *MemoryStateDB) []*types.ReceiptLog { return nil } -func (ch refundChange) revert(mdb *MemoryStateDB) { +func (ch refundChange) revert(mdb *MemoryStateDB) error { mdb.refund = ch.prev + return nil } -func (ch addLogChange) revert(mdb *MemoryStateDB) { +func (ch addLogChange) revert(mdb *MemoryStateDB) error { logs := mdb.logs[ch.txhash] if len(logs) == 1 { delete(mdb.logs, ch.txhash) @@ -323,6 +338,7 @@ func (ch addLogChange) revert(mdb *MemoryStateDB) { mdb.logSize-- log15.Info("addLogChange::revert", "mdb.logSize", mdb.logSize) + return nil } func (ch addLogChange) getLog(mdb *MemoryStateDB) []*types.ReceiptLog { @@ -334,8 +350,9 @@ func (ch addLogChange) getLog(mdb *MemoryStateDB) []*types.ReceiptLog { return ch.logs } -func (ch addPreimageChange) revert(mdb *MemoryStateDB) { +func (ch addPreimageChange) revert(mdb *MemoryStateDB) error { delete(mdb.preimages, ch.hash) + return nil } func (ch transferChange) getData(mdb *MemoryStateDB) []*types.KeyValue { @@ -348,25 +365,47 @@ func (ch transferChange) getLog(mdb *MemoryStateDB) []*types.ReceiptLog { // revert restores the account KV captured before the transfer. Before ForkEVMTransferRevert // preState is empty, so a reverted transfer keeps its old behaviour and leaves the cache as is. // -// executor.StateDB, the KV used in production, has no Delete method, so a key that did not -// exist before the transfer is shadowed with Set(key, nil) instead. That still appends the key -// to GetSetKeys(): the reverted transfer's keys therefore drop out of the receipt while -// remaining in the set, and chain33's checkKV rejects the whole transaction with -// ErrNotAllowMemSetKey. The attack is neutralised as a rejected transaction, not as a silent -// reconciliation. -func (ch transferChange) revert(mdb *MemoryStateDB) { +// executor.StateDB, the KV used in production, must undo the write through RevertSet rather +// than Set. Under ForkExecRollback Set records the key in GetSetKeys, and every recorded key +// has to show up in the receipt (chain33's checkKV); a restored write that the surviving +// receipt no longer carries would reject the whole transaction. RevertSet removes the key from +// that list as well. A StateDB without RevertSet (the goleveldb used in some tests) still gets +// the direct writes. +func (ch transferChange) revert(mdb *MemoryStateDB) error { for _, pre := range ch.preState { - var err error + var original []byte if pre.existed { - err = mdb.StateDB.Set(pre.key, pre.value) - } else if deleter, ok := mdb.StateDB.(interface{ Delete([]byte) error }); ok { - err = deleter.Delete(pre.key) - } else { - // executor.StateDB has no Delete: a nil value shadows the key as absent. - err = mdb.StateDB.Set(pre.key, nil) + original = pre.value } - if err != nil { + if err := mdb.revertAccountKey(pre.key, original); err != nil { log15.Error("transferChange revert failed", "key", string(pre.key), "err", err) + return err + } + } + return nil +} + +// revertAccountKey undoes one write made by the transfer. original=nil means the key did not +// exist before the transfer and must read as absent afterwards. +func (mdb *MemoryStateDB) revertAccountKey(key, original []byte) error { + if reverter, ok := mdb.StateDB.(interface { + RevertSet(key []byte, originalValue []byte) error + }); ok { + err := reverter.RevertSet(key, original) + if err == nil { + return nil + } + if !errors.Is(err, cexecutor.ErrStateDBRevertWithoutTx) { + return fmt.Errorf("revert state key %x: %w", key, err) + } + // No transaction is open, so there is no key list to repair: write through as the + // legacy path does (the fork that records Set keys is also off). + } + if original == nil { + if deleter, ok := mdb.StateDB.(interface{ Delete([]byte) error }); ok { + return deleter.Delete(key) } + return mdb.StateDB.Set(key, nil) } + return mdb.StateDB.Set(key, original) } diff --git a/plugin/dapp/evm/executor/vm/state/statedb.go b/plugin/dapp/evm/executor/vm/state/statedb.go index 7e589d9330..2b120d021c 100644 --- a/plugin/dapp/evm/executor/vm/state/statedb.go +++ b/plugin/dapp/evm/executor/vm/state/statedb.go @@ -72,6 +72,11 @@ type MemoryStateDB struct { stateDirty map[string]interface{} dataDirty map[string]interface{} api client.QueueProtocolAPI + + // revertErr records the first error a snapshot revert hit in the current transaction. + // RevertToSnapshot cannot return it to the EVM (its callers are already unwinding an + // error), so innerExec checks RevertError before accepting a successful execution. + revertErr error } // NewMemoryStateDB 基于执行器框架的三个DB构建内存状态机对象 @@ -400,7 +405,10 @@ func (mdb *MemoryStateDB) RevertToSnapshot(version int) { // 从最近版本开始回滚 for index := len(mdb.snapshots) - 1; index >= version; index-- { - mdb.snapshots[index].revert() + if err := mdb.snapshots[index].revert(); err != nil && mdb.revertErr == nil { + log15.Error("RevertToSnapshot failed", "snapshot", index, "err", err) + mdb.revertErr = err + } } // 只保留回滚版本之前的版本数据 @@ -414,6 +422,18 @@ func (mdb *MemoryStateDB) RevertToSnapshot(version int) { } +// RevertError returns the first error a snapshot revert recorded in the current transaction, +// or nil when every revert succeeded. innerExec turns a non-nil value into a failed +// transaction: the account cache and the receipt no longer describe the same state. +func (mdb *MemoryStateDB) RevertError() error { + return mdb.revertErr +} + +// ResetRevertError clears the recorded revert error at the start of an execution. +func (mdb *MemoryStateDB) ResetRevertError() { + mdb.revertErr = nil +} + // Snapshot 对当前的数据状态打快照，并生成快照版本号，方便后面回滚数据 func (mdb *MemoryStateDB) Snapshot() int { id := mdb.versionID @@ -538,7 +558,7 @@ func (mdb *MemoryStateDB) isForkActive(fork string) bool { // transferPreState captures the raw account KV touched by Transfer, keyed the same way the // coins account DB stores them. -func (mdb *MemoryStateDB) transferPreState(sender, recipient string) []kvPreState { +func (mdb *MemoryStateDB) transferPreState(sender, recipient string) ([]kvPreState, error) { conf := types.ConfSub(mdb.api.GetConfig(), evmtypes.ExecutorName) ethMapFromExecutor := conf.GStr("ethMapFromExecutor") if bytes.Equal(types.GetRealExecName([]byte(ethMapFromExecutor)), []byte("coins")) { @@ -558,18 +578,27 @@ func (mdb *MemoryStateDB) execAccountKey(addr string) []byte { return mdb.CoinsAccount.GetExecKVSet(mdb.evmPlatformAddr, &types.Account{Addr: addr})[0].Key } -// capturePreState reads the raw values behind keys before a transfer overwrites them. -func (mdb *MemoryStateDB) capturePreState(keys [][]byte) []kvPreState { +// capturePreState reads the raw values behind keys before a transfer overwrites them. Only +// ErrNotFound means the key did not exist: any other error leaves us without a value to +// restore, so the transfer must fail instead of recording a state that was never read. +func (mdb *MemoryStateDB) capturePreState(keys [][]byte) ([]kvPreState, error) { states := make([]kvPreState, 0, len(keys)) for _, key := range keys { pre := kvPreState{key: append([]byte(nil), key...)} - if value, err := mdb.StateDB.Get(key); err == nil { + value, err := mdb.StateDB.Get(key) + switch { + case err == nil: pre.value = append([]byte(nil), value...) pre.existed = true + case errors.Is(err, types.ErrNotFound): + // The key did not exist yet; revert must make it absent again. + default: + log15.Error("capturePreState failed", "key", string(key), "err", err) + return nil, fmt.Errorf("read state key %x before transfer: %w", key, err) } states = append(states, pre) } - return states + return states, nil } // Transfer 借助coins执行器进行转账相关操作 @@ -600,7 +629,12 @@ func (mdb *MemoryStateDB) Transfer(sender, recipient string, amount uint64) bool ethMapFromExecutor := conf.GStr("ethMapFromExecutor") var preState []kvPreState if mdb.isForkActive(evmtypes.ForkEVMTransferRevert) { - preState = mdb.transferPreState(sender, recipient) + preState, err = mdb.transferPreState(sender, recipient) + if err != nil { + log15.Error("transfer capture pre-state failed", "sender", sender, "recipient", recipient, + "amount", amount, "err", err) + return false + } } if bytes.Equal(types.GetRealExecName([]byte(ethMapFromExecutor)), []byte("coins")) { ret, err = mdb.CoinsAccount.Transfer(sender, recipient, value) @@ -660,10 +694,15 @@ func (mdb *MemoryStateDB) TransferToToken(from, recipient, symbol string, amount // Token accounts use the same non-exec account keys as tokendb.Transfer. var preState []kvPreState if mdb.isForkActive(evmtypes.ForkEVMTransferRevert) { - preState = mdb.capturePreState([][]byte{ + preState, err = mdb.capturePreState([][]byte{ tokendb.AccountKey(from), tokendb.AccountKey(recipient), }) + if err != nil { + log15.Error("TransferToToken capture pre-state failed", "from", from, "recipient", recipient, + "symbol", symbol, "amount", amount, "err", err) + return false, err + } } receipt, err := tokendb.Transfer(from, recipient, amount) if err != nil { From 0ce9c31327871b659225908fc41d67b8b9356768 Mon Sep 17 00:00:00 2001 From: bysomeone <11565373+bysomeone@users.noreply.github.com> Date: Tue, 3 Nov 2026 21:08:25 +0800 Subject: [PATCH 3/7] test(evm): pin the reverted-transfer pair on the production StateDB The previous tests either ran on goleveldb (which has Delete) or asserted that a transaction with a reverted value transfer is rejected. Neither covers what production does now, and the rejection assertion locked in the bug. Add the pair the reviewer asked for, both on the production shape: concrete executor.StateDB, no Delete, ForkExecRollback open at height 0, Begin() called, and the receipt checked the way checkKV does. - Positive: the outer call succeeds, an inner CALL{value} reverts, execution continues and writes state; the transaction is ExecOk, every Set key is in the receipt, and the committed balances are the post-revert ones. - Negative: the mint shape still cannot spend the reverted credit. The transaction now succeeds instead of being rejected, but no value is created. TransferToToken gets the same pair, built on a MemoryStateDB over the production StateDB with a created token: a legitimate transfer after a revert passes checkKV, and the reverted credit cannot be spent. Also pin capturePreState failing the transfer on a non-ErrNotFound read error, and a failed RevertSet being recorded instead of swallowed. Co-authored-by: Claude --- .../executor/transfer_revert_checkkv_test.go | 130 +++++++++++++----- .../transfer_revert_token_checkkv_test.go | 128 +++++++++++++++++ .../executor/vm/state/transfer_revert_test.go | 95 ++++++++++++- 3 files changed, 314 insertions(+), 39 deletions(-) create mode 100644 plugin/dapp/evm/executor/transfer_revert_token_checkkv_test.go diff --git a/plugin/dapp/evm/executor/transfer_revert_checkkv_test.go b/plugin/dapp/evm/executor/transfer_revert_checkkv_test.go index 10928cd226..bd78f298ce 100644 --- a/plugin/dapp/evm/executor/transfer_revert_checkkv_test.go +++ b/plugin/dapp/evm/executor/transfer_revert_checkkv_test.go @@ -4,13 +4,11 @@ package executor -// Framework-level repro of the reverted-transfer attack. The state-level tests use goleveldb, -// which has Delete, so the revert can physically remove the key. Production uses -// executor.StateDB, which only has Set: a key that did not exist before the transfer is -// shadowed with Set(key, nil), and that call still appends the key to GetSetKeys. The receipt -// is assembled from snapshots that no longer hold the reverted transfer, so the key is missing -// from it and chain33's checkKV rejects the whole transaction. execTxOne is unexported, so the -// test reproduces its check against the same StateDB type it inspects. +// Framework-level tests for reverted value transfers on the production StateDB shape: +// a concrete executor.StateDB with no Delete, ForkExecRollback open (Begin records every Set +// key in GetSetKeys) and the receipt/checkKV path that decides whether a transaction is +// accepted. The two shapes are a pair: a legitimate transaction that survives a reverted call +// must commit, and the mint shape must not create value. import ( "testing" @@ -74,18 +72,48 @@ func checkReceiptCoversSetKeys(memset []string, kvs []*ctypes.KeyValue) error { return nil } -func TestRevertedTransferTxRejectedByCheckKV(t *testing.T) { - const ( - topUp = uint64(1000) - moved = uint64(400) - ) +// newProductionExec builds an EVM executor over the production StateDB with the transfer-revert +// fork and ForkExecRollback both open at height 0. +func newProductionExec(t *testing.T, cfg *ctypes.Chain33Config, st *cexecutor.StateDB, localDB db.KVDB) *EVMExecutor { + t.Helper() + api := new(apimock.QueueProtocolAPI) + api.On("GetConfig").Return(cfg) + if !evmInitOnce { + Init(cfg.ExecName(evmtypes.ExecutorName), cfg, nil) + evmInitOnce = true + } + exec := NewEVMExecutor() + exec.SetAPI(api) + exec.SetLocalDB(localDB) + exec.SetStateDB(st) + exec.SetEnv(0, 0, 0) + exec.CheckInit() + return exec +} +func restoreBtcAddressDriver(t *testing.T) { + t.Helper() // query_test.go replaces the package-wide EVM address driver with the ETH one; the executor // globals were initialized with the BTC driver, so restore it for the tx envelope. driver, err := address.LoadDriver(btc.NormalAddressID, -1) require.NoError(t, err) vcomm.InitEvmAddressDriver(driver) +} + +// buildForwardAndStoreCode calls callee with value (the callee aborts) and then writes slot 0, +// so the outer frame continues and changes state after the inner value transfer is reverted. +func buildForwardAndStoreCode(callee vcomm.Address, value uint64) []byte { + code := buildCall(callee, value, 0) + code = append(code, 0x60, 0x01) // PUSH1 1 + code = append(code, 0x60, 0x00) // PUSH1 0 + code = append(code, 0x55) // SSTORE + code = append(code, 0x00) // STOP + return code +} +func productionStateDBFixture(t *testing.T) (*ctypes.Chain33Config, *cexecutor.StateDB, db.KVDB) { + t.Helper() + restoreBtcAddressDriver(t) cfg := newTransferRevertConfig(t, 0) // checkKV only sees Set keys when the framework begins a per-tx transaction, which it does // under ForkExecRollback. @@ -94,41 +122,73 @@ func TestRevertedTransferTxRejectedByCheckKV(t *testing.T) { dbDir, rawDB, localDB := util.CreateTestDB() t.Cleanup(func() { util.CloseTestDB(dbDir, rawDB) }) + return cfg, newFrameworkStateDB(t, cfg, localDB), localDB +} - st := newFrameworkStateDB(t, cfg, localDB) - api := new(apimock.QueueProtocolAPI) - api.On("GetConfig").Return(cfg) - if !evmInitOnce { - Init(cfg.ExecName(evmtypes.ExecutorName), cfg, nil) - evmInitOnce = true - } - exec := NewEVMExecutor() - exec.SetAPI(api) - exec.SetLocalDB(localDB) - exec.SetStateDB(st) - exec.SetEnv(0, 0, 0) - exec.CheckInit() +// TestRevertedCallValueTxSucceedsUnderCheckKV is the reviewer's positive case: the outer call +// succeeds, an inner CALL{value} reverts, execution continues and writes state. Post-fork the +// transaction must be ExecOk, pass checkKV, and commit exactly the post-revert balances. +func TestRevertedCallValueTxSucceedsUnderCheckKV(t *testing.T) { + const ( + topUp = uint64(1000) + moved = uint64(400) + ) + + cfg, st, localDB := productionStateDBFixture(t) + exec := newProductionExec(t, cfg, st, localDB) target := vcomm.BytesToAddress([]byte{0xd0}) forwarder := deployCode(t, cfg, exec, buildForwarderCode(target, moved)) - caller := deployCode(t, cfg, exec, buildCallerCode(*vcomm.StringToAddress(forwarder), moved)) + caller := deployCode(t, cfg, exec, buildForwardAndStoreCode(*vcomm.StringToAddress(forwarder), moved)) fundRole(t, exec, cfg, roleLegitUser, int64(topUp)*2) + user := addrFromRole(cfg, roleLegitUser) + initial := balanceOf(exec, user) // Model executor.begin(): StartTx plus the in-tx mode that records Set keys. st.Begin() receipt, err := exec.Exec(makeCallTx(cfg, roleLegitUser, caller, []byte{0x01}, topUp), 0) require.NoError(t, err) require.Equal(t, int32(ctypes.ExecOk), receipt.GetTy()) + require.NoError(t, checkReceiptCoversSetKeys(st.GetSetKeys(), receipt.GetKV()), + "a reverted inner value transfer must not leave keys the receipt lacks") + + applyReceiptKV(t, exec, receipt) + require.Equal(t, initial-int64(topUp), balanceOf(exec, user)) + require.Equal(t, int64(topUp), balanceOf(exec, caller), "the outer transfer survives") + require.Zero(t, balanceOf(exec, forwarder), "the reverted transfer must not credit the callee") + require.Zero(t, balanceOf(exec, target.String())) + require.Equal(t, vcomm.BytesToHash([]byte{0x01}), exec.GetMStateDB().GetState(caller, vcomm.BytesToHash([]byte{0x00})), + "execution must continue past the reverted call and commit its state write") +} - forwarderKey := string(exec.GetMStateDB().CoinsAccount.AccountKey(forwarder)) - require.Contains(t, st.GetSetKeys(), forwarderKey, "the reverted transfer must leave its key behind") +// TestRevertedCallValueMintTxCreatesNoValue keeps the attack shape under the same production +// StateDB: the phantom credit from a reverted transfer is spent in a later call. The transaction +// now succeeds (it no longer trips checkKV) but must not mint the reverted amount. +func TestRevertedCallValueMintTxCreatesNoValue(t *testing.T) { + const ( + topUp = uint64(1000) + moved = uint64(400) + ) - receiptKeys := make(map[string]struct{}, len(receipt.GetKV())) - for _, kv := range receipt.GetKV() { - receiptKeys[string(kv.GetKey())] = struct{}{} - } - require.NotContains(t, receiptKeys, forwarderKey, "the reverted transfer must not reach the receipt") + cfg, st, localDB := productionStateDBFixture(t) + exec := newProductionExec(t, cfg, st, localDB) + + target := vcomm.BytesToAddress([]byte{0xd0}) + forwarder := deployCode(t, cfg, exec, buildForwarderCode(target, moved)) + caller := deployCode(t, cfg, exec, buildCallerCode(*vcomm.StringToAddress(forwarder), moved)) + fundRole(t, exec, cfg, roleLegitUser, int64(topUp)*2) + user := addrFromRole(cfg, roleLegitUser) + initial := balanceOf(exec, user) + + st.Begin() + receipt, err := exec.Exec(makeCallTx(cfg, roleLegitUser, caller, []byte{0x01}, topUp), 0) + require.NoError(t, err) + require.Equal(t, int32(ctypes.ExecOk), receipt.GetTy()) + require.NoError(t, checkReceiptCoversSetKeys(st.GetSetKeys(), receipt.GetKV())) - require.Equal(t, ctypes.ErrNotAllowMemSetKey, checkReceiptCoversSetKeys(st.GetSetKeys(), receipt.GetKV()), - "checkKV must reject the transaction instead of committing the phantom balance") + applyReceiptKV(t, exec, receipt) + require.Equal(t, initial-int64(topUp), balanceOf(exec, user)) + require.Equal(t, int64(topUp), balanceOf(exec, caller)) + require.Zero(t, balanceOf(exec, forwarder), "the reverted credit must not be spendable") + require.Zero(t, balanceOf(exec, target.String()), "the mint must not reach the target") } diff --git a/plugin/dapp/evm/executor/transfer_revert_token_checkkv_test.go b/plugin/dapp/evm/executor/transfer_revert_token_checkkv_test.go new file mode 100644 index 0000000000..a5c96cdc8f --- /dev/null +++ b/plugin/dapp/evm/executor/transfer_revert_token_checkkv_test.go @@ -0,0 +1,128 @@ +// Copyright Fuzamei Corp. 2018 All Rights Reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package executor + +// TransferToToken goes through the same transferChange revert path as Transfer, so it gets the +// same pair of checks on the production StateDB: a legitimate token transfer that survives a +// reverted one still passes checkKV, and the credit of a reverted transfer cannot be spent. + +import ( + "testing" + + "github.com/33cn/chain33/account" + apimock "github.com/33cn/chain33/client/mocks" + cexecutor "github.com/33cn/chain33/executor" + ctypes "github.com/33cn/chain33/types" + evmstate "github.com/33cn/plugin/plugin/dapp/evm/executor/vm/state" + tokenty "github.com/33cn/plugin/plugin/dapp/token/types" + "github.com/stretchr/testify/require" +) + +const ( + tokenRevertFrom = "14KEKbY3kNFLfQEGJbNweV4whre7NpqzuB" + tokenRevertMiddle = "1PUiGcbsccfxW3zuvHXZBJfznziph5miAo" + tokenRevertSink = "12qyocayNF7Lv6C9qW4avxs2E7U41fKSfv" +) + +type tokenRevertFixture struct { + st *cexecutor.StateDB + mdb *evmstate.MemoryStateDB + tokenDB *account.DB +} + +// newTokenRevertFixture builds a MemoryStateDB over the production StateDB, a created token in +// LocalDB and one funded token account. Funding happens before Begin, as in a real block. +func newTokenRevertFixture(t *testing.T) *tokenRevertFixture { + t.Helper() + cfg, st, localDB := productionStateDBFixture(t) + require.NoError(t, localDB.Set([]byte("LODB-token-create-sto-1-BTY-0"), + ctypes.Encode(&tokenty.LocalToken{Symbol: "BTY", Status: 1}))) + + api := new(apimock.QueueProtocolAPI) + api.On("GetConfig").Return(cfg) + mdb := evmstate.NewMemoryStateDB(st, localDB, nil, 0, api) + + tokenDB, err := account.NewAccountDB(cfg, "token", "BTY", st) + require.NoError(t, err) + acc := tokenDB.LoadAccount(tokenRevertFrom) + acc.Balance = 1000 + tokenDB.SaveAccount(acc) + + return &tokenRevertFixture{st: st, mdb: mdb, tokenDB: tokenDB} +} + +func (f *tokenRevertFixture) balance(t *testing.T, addr string) int64 { + t.Helper() + return f.tokenDB.LoadAccount(addr).Balance +} + +// applyKV commits the receipt KV into the state, completing the checkKV path the way the +// framework does before the transaction is accepted. +func (f *tokenRevertFixture) applyKV(t *testing.T, kvs []*ctypes.KeyValue) { + t.Helper() + for _, kv := range kvs { + require.NoError(t, f.st.Set(kv.GetKey(), kv.GetValue())) + } +} + +func TestTransferToTokenRevertCheckKVPair(t *testing.T) { + t.Run("positive: transfers after a revert pass checkKV", func(t *testing.T) { + f := newTokenRevertFixture(t) + f.st.Begin() + outer := f.mdb.Snapshot() + + ok, err := f.mdb.TransferToToken(tokenRevertFrom, tokenRevertSink, "BTY", 100) + require.NoError(t, err) + require.True(t, ok) + + inner := f.mdb.Snapshot() + ok, err = f.mdb.TransferToToken(tokenRevertFrom, tokenRevertMiddle, "BTY", 400) + require.NoError(t, err) + require.True(t, ok) + f.mdb.RevertToSnapshot(inner) + + // The outer frame keeps executing after the reverted inner transfer. + ok, err = f.mdb.TransferToToken(tokenRevertFrom, tokenRevertSink, "BTY", 50) + require.NoError(t, err) + require.True(t, ok) + + kvs, _ := f.mdb.GetChangedData(outer) + require.NoError(t, checkReceiptCoversSetKeys(f.st.GetSetKeys(), kvs), + "the reverted token transfer must not leave keys the receipt lacks") + f.applyKV(t, kvs) + + require.Equal(t, int64(850), f.balance(t, tokenRevertFrom)) + require.Zero(t, f.balance(t, tokenRevertMiddle), "the reverted transfer must not credit the middle account") + require.Equal(t, int64(150), f.balance(t, tokenRevertSink)) + }) + + t.Run("negative: the reverted credit cannot be spent", func(t *testing.T) { + f := newTokenRevertFixture(t) + f.st.Begin() + outer := f.mdb.Snapshot() + + ok, err := f.mdb.TransferToToken(tokenRevertFrom, tokenRevertSink, "BTY", 100) + require.NoError(t, err) + require.True(t, ok) + + inner := f.mdb.Snapshot() + ok, err = f.mdb.TransferToToken(tokenRevertFrom, tokenRevertMiddle, "BTY", 400) + require.NoError(t, err) + require.True(t, ok) + f.mdb.RevertToSnapshot(inner) + + ok, err = f.mdb.TransferToToken(tokenRevertMiddle, tokenRevertSink, "BTY", 400) + require.False(t, ok, "the phantom credit must not be spendable") + require.ErrorIs(t, err, ctypes.ErrNoBalance) + + kvs, _ := f.mdb.GetChangedData(outer) + require.NoError(t, checkReceiptCoversSetKeys(f.st.GetSetKeys(), kvs)) + f.applyKV(t, kvs) + + require.Equal(t, int64(900), f.balance(t, tokenRevertFrom)) + require.Zero(t, f.balance(t, tokenRevertMiddle)) + require.Equal(t, int64(100), f.balance(t, tokenRevertSink)) + }) +} diff --git a/plugin/dapp/evm/executor/vm/state/transfer_revert_test.go b/plugin/dapp/evm/executor/vm/state/transfer_revert_test.go index 3912f00423..cfb1f85295 100644 --- a/plugin/dapp/evm/executor/vm/state/transfer_revert_test.go +++ b/plugin/dapp/evm/executor/vm/state/transfer_revert_test.go @@ -5,6 +5,8 @@ package state import ( + "bytes" + "errors" "strings" "testing" @@ -150,10 +152,11 @@ func TestSnapshotRevertSameSlotReverseOrder(t *testing.T) { }) } -// TestTransferRevertWithoutDeleteHidesKey covers transferChange.revert's fallback for a KV with -// no Delete: executor.StateDB, the production implementation, only exposes Set, so a key that -// did not exist before the transfer is shadowed with a nil value, which Get reports as absent. -// The goleveldb used by the other tests has Delete and never reaches this branch. +// TestTransferRevertWithoutDeleteHidesKey covers transferChange.revert's fallback when the KV +// has no transaction open: executor.StateDB exposes RevertSet, but outside Begin there is no +// key list to repair, so revert writes directly. A key that did not exist before the transfer +// is then shadowed with a nil value, which Get reports as absent. The goleveldb used by the +// other tests has Delete and never reaches this branch. func TestTransferRevertWithoutDeleteHidesKey(t *testing.T) { const ( sender = "14KEKbY3kNFLfQEGJbNweV4whre7NpqzuB" @@ -165,6 +168,8 @@ func TestTransferRevertWithoutDeleteHidesKey(t *testing.T) { st := cexecutor.NewStateDB(nil, nil, nil, nil).(*cexecutor.StateDB) _, hasDelete := interface{}(st).(interface{ Delete([]byte) error }) require.False(t, hasDelete, "executor.StateDB must not expose Delete or the fallback stays untested") + _, hasRevertSet := interface{}(st).(interface{ RevertSet([]byte, []byte) error }) + require.True(t, hasRevertSet, "executor.StateDB should undo writes through RevertSet inside a tx") mdb := newTransferRevertMDBWith(t, newTransferRevertConfig(t, 0), st) fundCoinAccount(mdb, sender, initial) @@ -180,3 +185,85 @@ func TestTransferRevertWithoutDeleteHidesKey(t *testing.T) { require.Equal(t, ctypes.ErrNotFound, err, "the nil Set must shadow the key as absent") require.Equal(t, initial, coinBalance(mdb, sender)) } + +// faultyKV wraps a KV and fails the read of one key, standing in for a store timeout. +type faultyKV struct { + db.KV + failKey []byte + err error +} + +func (f faultyKV) Get(key []byte) ([]byte, error) { + if bytes.Equal(key, f.failKey) { + return nil, f.err + } + return f.KV.Get(key) +} + +// revertFailingKV is a KV whose RevertSet always fails, so the error path of the producer can +// be observed without a real storage fault. +type revertFailingKV struct { + db.KV + err error +} + +func (f revertFailingKV) RevertSet(key, originalValue []byte) error { + return f.err +} + +// TestRevertErrorIsRecorded pins the propagation of a failed RevertSet: the error reaches the +// MemoryStateDB instead of being swallowed, and the next execution can clear it. +func TestRevertErrorIsRecorded(t *testing.T) { + const ( + sender = "14KEKbY3kNFLfQEGJbNweV4whre7NpqzuB" + recipient = "1PUiGcbsccfxW3zuvHXZBJfznziph5miAo" + initial = int64(1000) + ) + + dbDir, stateDB, _ := util.CreateTestDB() + t.Cleanup(func() { util.CloseTestDB(dbDir, stateDB) }) + revertErr := errors.New("revert failed") + mdb := newTransferRevertMDBWith(t, newTransferRevertConfig(t, 0), revertFailingKV{KV: stateDB, err: revertErr}) + fundCoinAccount(mdb, sender, initial) + + snapshot := mdb.Snapshot() + require.True(t, mdb.Transfer(sender, recipient, 400)) + mdb.RevertToSnapshot(snapshot) + require.ErrorIs(t, mdb.RevertError(), revertErr, "the producer error must reach the caller") + + mdb.ResetRevertError() + require.NoError(t, mdb.RevertError()) +} + +// TestCapturePreStateFailsTransferOnReadError pins the reviewer's Suggestion: only ErrNotFound +// may mean "the key did not exist". Any other read error leaves the pre-transfer value unknown, +// so the transfer must fail instead of recording a state that was never read. +func TestCapturePreStateFailsTransferOnReadError(t *testing.T) { + const ( + sender = "14KEKbY3kNFLfQEGJbNweV4whre7NpqzuB" + recipient = "1PUiGcbsccfxW3zuvHXZBJfznziph5miAo" + initial = int64(1000) + moved = uint64(400) + ) + + cfg := newTransferRevertConfig(t, 0) + dbDir, stateDB, _ := util.CreateTestDB() + t.Cleanup(func() { util.CloseTestDB(dbDir, stateDB) }) + + // The key is built from the same config the coins DB uses. + coinsAccount, err := account.NewAccountDB(cfg, "coins", cfg.GetCoinSymbol(), stateDB) + require.NoError(t, err) + storeErr := errors.New("store read timeout") + mdb := newTransferRevertMDBWith(t, cfg, faultyKV{ + KV: stateDB, + failKey: coinsAccount.AccountKey(recipient), + err: storeErr, + }) + fundCoinAccount(mdb, sender, initial) + + snapshot := mdb.Snapshot() + require.False(t, mdb.Transfer(sender, recipient, moved), "a pre-state read error must fail the transfer") + mdb.RevertToSnapshot(snapshot) + require.Equal(t, initial, coinBalance(mdb, sender)) + require.Zero(t, coinBalance(mdb, recipient)) +} From ea49c27312ec3ab65ddd9bc34f41556f8b8020d0 Mon Sep 17 00:00:00 2001 From: bysomeone <11565373+bysomeone@users.noreply.github.com> Date: Tue, 3 Nov 2026 22:43:39 +0800 Subject: [PATCH 4/7] chore(deps): pin chain33 at the fork-gated envelope fix Co-authored-by: Claude --- go.mod | 2 +- go.sum | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index f464829300..0181d45f3f 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,7 @@ replace github.com/ava-labs/avalanchego => github.com/33cn/avalanchego v1.10.10- // Temporary test-time pin: the EVM transfer-revert fix calls StateDB.RevertSet, which comes // from 33cn/chain33#1412. Drop this replace once chain33 releases a version containing it. -replace github.com/33cn/chain33 => github.com/bysomeone/chain33 v1.72.1-0.20261103124951-0ba08b76ebaf +replace github.com/33cn/chain33 => github.com/bysomeone/chain33 v1.72.1-0.20261103141406-9f5c055ab050 require ( github.com/33cn/chain33 v1.72.7-0.20261016082959-e1bde75e25d0 diff --git a/go.sum b/go.sum index 8e4cef180f..d35ceca8a2 100644 --- a/go.sum +++ b/go.sum @@ -245,6 +245,8 @@ github.com/btcsuite/winsvc v1.0.0/go.mod h1:jsenWakMcC0zFBFurPLEAyrnc/teJEM1O46f github.com/buger/jsonparser v0.0.0-20181115193947-bf1c66bbce23/go.mod h1:bbYlZJ7hK1yFx9hf58LP0zeX7UjIGs20ufpu3evjr+s= github.com/bysomeone/chain33 v1.72.1-0.20261103124951-0ba08b76ebaf h1:rY3/3AOPt21SQT88dUe8y/D2KqZkA0QvwyoeAM0Xpck= github.com/bysomeone/chain33 v1.72.1-0.20261103124951-0ba08b76ebaf/go.mod h1:6C5lKl8ejjoXtyhetFXz13t+JV4UQvp9H4t9eLOTNXs= +github.com/bysomeone/chain33 v1.72.1-0.20261103141406-9f5c055ab050 h1:puInZbRE5coLWDn5e04xwtInUXFtY5ODuLhO1Ae2CfQ= +github.com/bysomeone/chain33 v1.72.1-0.20261103141406-9f5c055ab050/go.mod h1:6C5lKl8ejjoXtyhetFXz13t+JV4UQvp9H4t9eLOTNXs= github.com/c-bata/go-prompt v0.2.2 h1:uyKRz6Z6DUyj49QVijyM339UJV9yhbr70gESwbNU3e0= github.com/c-bata/go-prompt v0.2.2/go.mod h1:VzqtzE2ksDBcdln8G7mk2RX9QyGjH+OVqOCSiVIqS34= github.com/cactus/go-statsd-client/statsd v0.0.0-20191106001114-12b4e2b38748/go.mod h1:l/bIBLeOl9eX+wxJAzxS4TveKRtAqlyDpHjhkfO0MEI= From 125ab2379f6121b77a58d226f1a652e24487713d Mon Sep 17 00:00:00 2001 From: bysomeone <11565373+bysomeone@users.noreply.github.com> Date: Tue, 3 Nov 2026 23:03:09 +0800 Subject: [PATCH 5/7] fix(config): register ForkEthSignEnvelope in the shipped fork configs Registering a new system fork makes initForkConfig reject any config whose [fork.system] section does not list it, and the node exits at startup. chain33.para.toml and chain33.fork.toml both carry that section, so every para-chain deployment stopped booting once the fork-gated chain33 was linked in. Write -1 to keep the rule off until a chain schedules it. Co-authored-by: Claude --- chain33.fork.toml | 2 ++ chain33.para.toml | 2 ++ 2 files changed, 4 insertions(+) diff --git a/chain33.fork.toml b/chain33.fork.toml index 14c4e22918..a855a35a63 100644 --- a/chain33.fork.toml +++ b/chain33.fork.toml @@ -36,6 +36,8 @@ ForkEthAddressFormat=0 ForkTxChainIDStrict=-1 # 创世动作(Genesis)执行高度限制：启用后框架统一拒绝 height != 0 的 Exec_Genesis，防止执行器漏写 height 校验被创世重入；-1 表示关闭 ForkGenesisOnlyAtHeightZero=0 +# eth签名交易的chain33信封校验启用高度；-1 表示关闭 +ForkEthSignEnvelope=-1 [fork.sub.none] ForkUseTimeDelay=0 diff --git a/chain33.para.toml b/chain33.para.toml index 5717e2ec85..a6c8915cb2 100644 --- a/chain33.para.toml +++ b/chain33.para.toml @@ -344,6 +344,8 @@ ForkEthAddressFormat=0 ForkTxChainIDStrict=-1 # 创世动作(Genesis)执行高度限制：启用后框架统一拒绝 height != 0 的 Exec_Genesis，防止执行器漏写 height 校验被创世重入；-1 表示关闭 ForkGenesisOnlyAtHeightZero=0 +# eth签名交易的chain33信封校验启用高度；-1 表示关闭 +ForkEthSignEnvelope=-1 [fork.sub.none] ForkUseTimeDelay=0 From ec09a7104afd61c707b4c63aca73e88ab4f491c1 Mon Sep 17 00:00:00 2001 From: bysomeone <11565373+bysomeone@users.noreply.github.com> Date: Wed, 4 Nov 2026 11:22:28 +0800 Subject: [PATCH 6/7] test(evm): fail the transaction when a snapshot revert fails A failed revert leaves the account cache and the receipt describing different state, so innerExec turns it into a failed transaction. Nothing covered that guard: removing it left every package green. The test drives a call whose inner CALL{value} reverts through a StateDB whose RevertSet always errors, and also pins that the next execution starts with the error cleared. Co-authored-by: Claude --- .../executor/snapshot_revert_failure_test.go | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 plugin/dapp/evm/executor/snapshot_revert_failure_test.go diff --git a/plugin/dapp/evm/executor/snapshot_revert_failure_test.go b/plugin/dapp/evm/executor/snapshot_revert_failure_test.go new file mode 100644 index 0000000000..883efb7310 --- /dev/null +++ b/plugin/dapp/evm/executor/snapshot_revert_failure_test.go @@ -0,0 +1,88 @@ +// Copyright Fuzamei Corp. 2018 All Rights Reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package executor + +// Executor-level test for the guard in innerExec that turns a failed snapshot revert into a +// failed transaction. A failed revert leaves the account cache and the receipt describing +// different state, so the transaction must not be committed. + +import ( + "errors" + "testing" + + "github.com/33cn/chain33/common/db" + ctypes "github.com/33cn/chain33/types" + vcomm "github.com/33cn/plugin/plugin/dapp/evm/executor/vm/common" + "github.com/stretchr/testify/require" +) + +// failingRevertKV wraps the production StateDB and fails every RevertSet, standing in for a +// store fault during the undo of a reverted value transfer. +type failingRevertKV struct { + db.KV + err error +} + +func (f failingRevertKV) RevertSet(key, originalValue []byte) error { + return f.err +} + +// TestInnerExecFailsWhenSnapshotRevertFails drives the same CALL{value} revert shape as the +// transfer-revert integration tests, but with a KV whose RevertSet always fails. The inner call +// is popped by the caller contract, so the outer frame reports success; only the RevertError +// guard can fail the transaction. The next execution must still succeed: ResetRevertError clears +// the recorded error at the start of every innerExec call. +func TestInnerExecFailsWhenSnapshotRevertFails(t *testing.T) { + const ( + topUp = uint64(1000) + moved = uint64(400) + ) + + cfg, st, localDB := productionStateDBFixture(t) + revertErr := errors.New("injected RevertSet failure") + + // Wrap the production StateDB so the snapshot revert of the value transfer fails. SetStateDB + // also points the coins account at the wrapper, and CheckInit rebuilds the MemoryStateDB. + exec := newProductionExec(t, cfg, st, localDB) + exec.SetStateDB(failingRevertKV{KV: st, err: revertErr}) + exec.CheckInit() + + target := vcomm.BytesToAddress([]byte{0xd0}) + forwarder := deployCode(t, cfg, exec, buildForwarderCode(target, moved)) + caller := deployCode(t, cfg, exec, buildCallerCode(*vcomm.StringToAddress(forwarder), moved)) + noop := deployCode(t, cfg, exec, []byte{0x00}) + + user := addrFromRole(cfg, roleLegitUser) + fundRole(t, exec, cfg, roleLegitUser, int64(topUp)*2) + initial := balanceOf(exec, user) + + // First transaction: the forwarder aborts on the value call, the reverted transfer fails to + // restore its account keys, and the outer contract continues to success. + st.Begin() + receipt, err := exec.Exec(makeCallTx(cfg, roleLegitUser, caller, []byte{0x01}, topUp), 0) + require.ErrorIs(t, err, revertErr, "innerExec must fail the transaction on a failed snapshot revert") + require.Empty(t, receipt.GetKV(), "the failed transaction must carry no state to commit") + require.ErrorIs(t, exec.GetMStateDB().RevertError(), revertErr, + "the guard must observe the recorded revert error") + st.Rollback() + + require.Equal(t, initial, balanceOf(exec, user), "no balance may move out of the user") + require.Zero(t, balanceOf(exec, forwarder), "the reverted credit must not be committed") + require.Zero(t, balanceOf(exec, target.String()), "the phantom credit must not be committed") + + // Second execution on the same MemoryStateDB (Exec would recreate it through CheckInit). + // Nothing reverts here, so a stale error would fail at the guard; ResetRevertError at the + // start of innerExec must clear it before execution starts. + tx := makeCallTx(cfg, roleLegitUser, noop, nil, 0) + msg, err := exec.GetMessage(tx, 0, nil) + require.NoError(t, err) + + st.Begin() + receipt, err = exec.innerExec(msg, tx.Hash(), tx.GetSignature().GetTy(), 0, msg.GasLimit(), false) + require.NoError(t, err, "a failed revert must not poison the next execution") + require.Equal(t, int32(ctypes.ExecOk), receipt.GetTy()) + require.NoError(t, exec.GetMStateDB().RevertError(), "ResetRevertError must clear the recorded error") + st.Rollback() +} From c3f474c8398ad18f0b402764fcafd8e201d19b02 Mon Sep 17 00:00:00 2001 From: bysomeone <11565373+bysomeone@users.noreply.github.com> Date: Wed, 4 Nov 2026 13:55:56 +0800 Subject: [PATCH 7/7] chore(deps): require chain33 v1.73.0 and drop the temporary replace The release contains the RevertSet hook this branch calls and the fork-gated envelope rule, so the module can now be built from the published version instead of a pinned commit. Co-authored-by: Claude --- go.mod | 6 +----- go.sum | 2 ++ 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 0181d45f3f..4e25a1194d 100644 --- a/go.mod +++ b/go.mod @@ -6,12 +6,8 @@ toolchain go1.22.12 replace github.com/ava-labs/avalanchego => github.com/33cn/avalanchego v1.10.10-0.20240529041529-ada691598153 -// Temporary test-time pin: the EVM transfer-revert fix calls StateDB.RevertSet, which comes -// from 33cn/chain33#1412. Drop this replace once chain33 releases a version containing it. -replace github.com/33cn/chain33 => github.com/bysomeone/chain33 v1.72.1-0.20261103141406-9f5c055ab050 - require ( - github.com/33cn/chain33 v1.72.7-0.20261016082959-e1bde75e25d0 + github.com/33cn/chain33 v1.73.0 github.com/BurntSushi/toml v1.3.2 github.com/NebulousLabs/Sia v1.3.7 github.com/bitly/go-simplejson v0.5.0 diff --git a/go.sum b/go.sum index d35ceca8a2..93817c8c58 100644 --- a/go.sum +++ b/go.sum @@ -46,6 +46,8 @@ dmitri.shuralyov.com/html/belt v0.0.0-20180602232347-f7d459c86be0/go.mod h1:JLBr dmitri.shuralyov.com/service/change v0.0.0-20181023043359-a85b471d5412/go.mod h1:a1inKt/atXimZ4Mv927x+r7UpyzRUf4emIoiiSC2TN4= dmitri.shuralyov.com/state v0.0.0-20180228185332-28bcc343414c/go.mod h1:0PRwlb0D6DFvNNtx+9ybjezNCa8XF0xaYcETyp6rHWU= git.apache.org/thrift.git v0.0.0-20180902110319-2566ecd5d999/go.mod h1:fPE2ZNJGynbRyZ4dJvy6G277gSllfV2HJqblrnkyeyg= +github.com/33cn/chain33 v1.73.0 h1:Pp+rKZnw84oeRue4FoOMbHmIKzgk/qNjQKDWhhB5i0U= +github.com/33cn/chain33 v1.73.0/go.mod h1:6C5lKl8ejjoXtyhetFXz13t+JV4UQvp9H4t9eLOTNXs= github.com/AndreasBriese/bbloom v0.0.0-20190825152654-46b345b51c96 h1:cTp8I5+VIoKjsnZuH8vjyaysT/ses3EvZeaV/1UkF2M= github.com/AndreasBriese/bbloom v0.0.0-20190825152654-46b345b51c96/go.mod h1:bOvUY6CB00SOBii9/FifXqc0awNKxLFCL/+pkDPuyl8= github.com/Azure/azure-sdk-for-go v41.3.0+incompatible/go.mod h1:9XXNKU+eRnpl9moKnB4QOLf1HestfXbmab5FXxiDBjc=