From 5f3e1ac709ed7765e5ab7761740ba7f134e074b3 Mon Sep 17 00:00:00 2001 From: king Date: Wed, 4 Nov 2026 09:04:56 +0000 Subject: [PATCH 1/2] fix(evm): route EVM state writes through a journaled KV buffer A value transfer inside a reverted EVM sub-call was removed from the receipt but left in the coins account cache, so a later transfer in the same transaction could spend the refunded balance and commit a credit that no debit backed (native-coin mint). #1348 fixed this by undoing each transfer by hand (preState capture plus StateDB.RevertSet), gated behind ForkEVMTransferRevert. This replaces that with automatic rollback: MemoryStateDB wraps the executor StateDB in journalKV, a write buffer with nested rollback points aligned with Snapshot versions, in the spirit of chain33's txcache. The coins account is a copy bound to the buffer, so coins and token transfers, SELFDESTRUCT payouts and the paracross ExecTransfer path are all undone by RevertToSnapshot without per-path revert code. The surviving writes go into the receipt and back to the StateDB in GetChangedData; nothing reaches the executor StateDB during execution, so checkKV holds by construction. Snapshot.revert unwinds entries in reverse order and Create snapshots before the value transfer, now unconditionally. Not fork-gated: history was checked for exploitation of the mint. ForkEVMTransferRevert is no longer read; its registration and config keys stay because initForkConfig panics on an unregistered key in shipped configs. Removed with the #1348 machinery: transferChange.preState, revertAccountKey, RevertError/ResetRevertError and their tests. The behaviour tests from #1348 (production-StateDB checkKV, token precompile, failed and successful CREATE, transaction-level mint) now run without the fork and pass. Co-Authored-By: Claude Fable 5 --- chain33.fork.toml | 2 +- chain33.para.toml | 2 +- .../dapp/evm/cmd/ci2/chain33.proxyminer.toml | 2 +- .../evm/executor/attack_integration_test.go | 5 +- plugin/dapp/evm/executor/exec.go | 9 - .../executor/snapshot_revert_failure_test.go | 88 ------- .../executor/transfer_revert_checkkv_test.go | 8 +- .../transfer_revert_integration_test.go | 66 +++--- .../executor/vm/runtime/blacklist_gap_test.go | 3 +- .../executor/vm/runtime/create_revert_test.go | 89 ++----- plugin/dapp/evm/executor/vm/runtime/evm.go | 15 +- .../vm/runtime/revert_mint_poc_test.go | 88 +++++++ .../dapp/evm/executor/vm/state/journal_kv.go | 103 ++++++++ .../evm/executor/vm/state/journal_kv_test.go | 102 ++++++++ plugin/dapp/evm/executor/vm/state/snapshot.go | 128 ++-------- plugin/dapp/evm/executor/vm/state/statedb.go | 132 ++--------- .../executor/vm/state/transfer_revert_test.go | 220 ++---------------- plugin/dapp/evm/types/evm.go | 5 +- plugin/dapp/evm/types/types.go | 8 +- 19 files changed, 430 insertions(+), 645 deletions(-) delete mode 100644 plugin/dapp/evm/executor/snapshot_revert_failure_test.go create mode 100644 plugin/dapp/evm/executor/vm/runtime/revert_mint_poc_test.go create mode 100644 plugin/dapp/evm/executor/vm/state/journal_kv.go create mode 100644 plugin/dapp/evm/executor/vm/state/journal_kv_test.go diff --git a/chain33.fork.toml b/chain33.fork.toml index a855a35a6..4830d5506 100644 --- a/chain33.fork.toml +++ b/chain33.fork.toml @@ -101,7 +101,7 @@ 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) +# 已废弃，代码不再读取；保留该 key 以兼容旧配置 ForkEVMTransferRevert=-1 diff --git a/chain33.para.toml b/chain33.para.toml index a6c8915cb..0a9f79d07 100644 --- a/chain33.para.toml +++ b/chain33.para.toml @@ -437,7 +437,7 @@ 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) +# 已废弃，代码不再读取；保留该 key 以兼容旧配置 ForkEVMTransferRevert=-1 [fork.sub.blackwhite] diff --git a/plugin/dapp/evm/cmd/ci2/chain33.proxyminer.toml b/plugin/dapp/evm/cmd/ci2/chain33.proxyminer.toml index 316bbd346..82be203af 100644 --- a/plugin/dapp/evm/cmd/ci2/chain33.proxyminer.toml +++ b/plugin/dapp/evm/cmd/ci2/chain33.proxyminer.toml @@ -605,7 +605,7 @@ 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) +# 已废弃，代码不再读取；保留该 key 以兼容旧配置 ForkEVMTransferRevert=-1 [fork.sub.evmxgo] diff --git a/plugin/dapp/evm/executor/attack_integration_test.go b/plugin/dapp/evm/executor/attack_integration_test.go index f316a38f5..c411228b6 100644 --- a/plugin/dapp/evm/executor/attack_integration_test.go +++ b/plugin/dapp/evm/executor/attack_integration_test.go @@ -82,9 +82,10 @@ func privKey(idx int) crypto.PrivKey { return util.TestPrivkeyList[idx] } func fundAddr(t *testing.T, exec *EVMExecutor, addr string, amount int64) { t.Helper() - acc := exec.mStateDB.CoinsAccount.LoadAccount(addr) + // 写到执行器的 StateDB：mStateDB 的写缓冲每笔交易重建，写进去的余额不会留到下一次 Exec + acc := exec.GetCoinsAccount().LoadAccount(addr) acc.Balance = amount - exec.mStateDB.CoinsAccount.SaveAccount(acc) + exec.GetCoinsAccount().SaveAccount(acc) } func addrFromRole(cfg *ctypes.Chain33Config, roleIdx int) string { diff --git a/plugin/dapp/evm/executor/exec.go b/plugin/dapp/evm/executor/exec.go index d51ac1773..8a57e3721 100644 --- a/plugin/dapp/evm/executor/exec.go +++ b/plugin/dapp/evm/executor/exec.go @@ -58,7 +58,6 @@ 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() @@ -192,14 +191,6 @@ 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/snapshot_revert_failure_test.go b/plugin/dapp/evm/executor/snapshot_revert_failure_test.go deleted file mode 100644 index 883efb731..000000000 --- a/plugin/dapp/evm/executor/snapshot_revert_failure_test.go +++ /dev/null @@ -1,88 +0,0 @@ -// 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() -} diff --git a/plugin/dapp/evm/executor/transfer_revert_checkkv_test.go b/plugin/dapp/evm/executor/transfer_revert_checkkv_test.go index bd78f298c..1a1fadf56 100644 --- a/plugin/dapp/evm/executor/transfer_revert_checkkv_test.go +++ b/plugin/dapp/evm/executor/transfer_revert_checkkv_test.go @@ -72,8 +72,8 @@ func checkReceiptCoversSetKeys(memset []string, kvs []*ctypes.KeyValue) error { return nil } -// newProductionExec builds an EVM executor over the production StateDB with the transfer-revert -// fork and ForkExecRollback both open at height 0. +// newProductionExec builds an EVM executor over the production StateDB with ForkExecRollback +// 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) @@ -114,7 +114,7 @@ func buildForwardAndStoreCode(callee vcomm.Address, value uint64) []byte { func productionStateDBFixture(t *testing.T) (*ctypes.Chain33Config, *cexecutor.StateDB, db.KVDB) { t.Helper() restoreBtcAddressDriver(t) - cfg := newTransferRevertConfig(t, 0) + cfg := newTransferRevertConfig(t) // checkKV only sees Set keys when the framework begins a per-tx transaction, which it does // under ForkExecRollback. cfg.SetFork("ForkExecRollback", 0) @@ -126,7 +126,7 @@ func productionStateDBFixture(t *testing.T) (*ctypes.Chain33Config, *cexecutor.S } // TestRevertedCallValueTxSucceedsUnderCheckKV is the reviewer's positive case: the outer call -// succeeds, an inner CALL{value} reverts, execution continues and writes state. Post-fork the +// succeeds, an inner CALL{value} reverts, execution continues and writes state. The // transaction must be ExecOk, pass checkKV, and commit exactly the post-revert balances. func TestRevertedCallValueTxSucceedsUnderCheckKV(t *testing.T) { const ( diff --git a/plugin/dapp/evm/executor/transfer_revert_integration_test.go b/plugin/dapp/evm/executor/transfer_revert_integration_test.go index 769e419cb..bf1795ec6 100644 --- a/plugin/dapp/evm/executor/transfer_revert_integration_test.go +++ b/plugin/dapp/evm/executor/transfer_revert_integration_test.go @@ -5,10 +5,9 @@ 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. +// the phantom credit B kept in the account cache was spent by a later call in the same tx, +// minting the moved amount. The reverted transfer is now rolled back by journalKV, so the later +// spend has no balance to take. import ( "strings" @@ -22,7 +21,7 @@ import ( "github.com/stretchr/testify/require" ) -func newTransferRevertConfig(t *testing.T, forkHeight int64) *ctypes.Chain33Config { +func newTransferRevertConfig(t *testing.T) *ctypes.Chain33Config { t.Helper() cfgStr := strings.Replace(ctypes.GetDefaultCfgstring(), `Title="local"`, `Title="integration-test"`, 1) if !strings.Contains(cfgStr, "[exec.sub.evm]") { @@ -30,8 +29,6 @@ func newTransferRevertConfig(t *testing.T, forkHeight int64) *ctypes.Chain33Conf } 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 } @@ -145,40 +142,29 @@ func TestRevertedCallValueCannotBeSpent(t *testing.T) { 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") - }) + cfg := newTransferRevertConfig(t) + 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, sum(), "committed total must not grow") + require.Zero(t, balanceOf(exec, target.String()), "the forwarder must not spend a phantom credit") } func balanceOf(exec *EVMExecutor, addr string) int64 { diff --git a/plugin/dapp/evm/executor/vm/runtime/blacklist_gap_test.go b/plugin/dapp/evm/executor/vm/runtime/blacklist_gap_test.go index b81d07f4c..77f4a37dd 100644 --- a/plugin/dapp/evm/executor/vm/runtime/blacklist_gap_test.go +++ b/plugin/dapp/evm/executor/vm/runtime/blacklist_gap_test.go @@ -89,8 +89,9 @@ func (e *gapEnv) fund(addr vmcommon.Address, amount int64) { e.coins.SaveAccount(acc) } +// balance 经由 mdb 读取：执行期间的转账只在 mdb 的写缓冲中，直接读 StateDB 看不到 func (e *gapEnv) balance(addr vmcommon.Address) int64 { - return e.coins.LoadAccount(addr.String()).Balance + return e.mdb.CoinsAccount.LoadAccount(addr.String()).Balance } func (e *gapEnv) deploy(addr, creator vmcommon.Address, code []byte) { diff --git a/plugin/dapp/evm/executor/vm/runtime/create_revert_test.go b/plugin/dapp/evm/executor/vm/runtime/create_revert_test.go index ac6552961..cbc7116ba 100644 --- a/plugin/dapp/evm/executor/vm/runtime/create_revert_test.go +++ b/plugin/dapp/evm/executor/vm/runtime/create_revert_test.go @@ -6,34 +6,16 @@ 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. +// snapshot. The snapshot is now 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 { @@ -55,62 +37,39 @@ 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})) + env := newGapEnv(t, nil) + 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)) - }) - } + _, _, _, err := env.evm.Call(AccountRef(caller), parent, nil, 5_000_000, 0) + require.NoError(t, err) + require.Equal(t, stash, 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. +// value. With no snapshot open yet, the old code dropped the transfer in addChange: the balances +// moved but the receipt never carried the keys, so chain33's checkKV rejected the transaction. +// Both keys must now 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) + env := newGapEnv(t, nil) + 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)) + _, 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) - } - }) + kvs, _ := env.mdb.GetChangedData(snapshot) + for _, key := range [][]byte{ + env.coins.AccountKey(caller.String()), + env.coins.AccountKey(contract.String()), + } { + require.True(t, hasKV(kvs, key), "receipt key %x", key) } } diff --git a/plugin/dapp/evm/executor/vm/runtime/evm.go b/plugin/dapp/evm/executor/vm/runtime/evm.go index cb916a6d7..acec89577 100644 --- a/plugin/dapp/evm/executor/vm/runtime/evm.go +++ b/plugin/dapp/evm/executor/vm/runtime/evm.go @@ -537,20 +537,14 @@ func (evm *EVM) Create(caller ContractRef, contractAddr common.Address, code []b return nil, -1, gas, berr } + // 先打快照再转账，创建失败时转入的金额随合约账户一起回滚 + snapshot = evm.StateDB.Snapshot() // 向合约地址转账 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) + evm.StateDB.RevertToSnapshot(snapshot) return nil, -1, gas, model.ErrInsufficientBalance } } else { @@ -572,9 +566,6 @@ func (evm *EVM) Create(caller ContractRef, contractAddr common.Address, code []b contract.SetCallCode(&contractAddr, common.ToHash(code), code) // 创建一个新的账户对象（合约账户） - 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/runtime/revert_mint_poc_test.go b/plugin/dapp/evm/executor/vm/runtime/revert_mint_poc_test.go new file mode 100644 index 000000000..68ea8f50d --- /dev/null +++ b/plugin/dapp/evm/executor/vm/runtime/revert_mint_poc_test.go @@ -0,0 +1,88 @@ +package runtime + +// 复现安全报告：回滚子调用的转账留在 coins 缓存里，被同一交易后续转账提交，造成增发。 +// 提交态 = 交易前状态 + 回执 KV（GetChangedData），与 execTxOne/ExecKVMemSet 一致。 + +import ( + "math/big" + "testing" + + ctypes "github.com/33cn/chain33/types" + vmcommon "github.com/33cn/plugin/plugin/dapp/evm/executor/vm/common" + "github.com/stretchr/testify/require" +) + +func zeroArgs4() []byte { return []byte{0x60, 0x00, 0x60, 0x00, 0x60, 0x00, 0x60, 0x00} } + +// Q：把收到的 value 原样 CALL 回 caller，然后 STOP。 +func buildRefundCode() []byte { + code := zeroArgs4() + return append(code, 0x34, 0x33, 0x5a, 0xf1, 0x00) // CALLVALUE CALLER GAS CALL STOP +} + +// P：无 calldata 则 REVERT；有 calldata 则先把 value 转给 q，再把同额转给 payout。 +func buildTrapCode(q, payout vmcommon.Address) []byte { + code := []byte{0x36, 0x60, 0x09, 0x57, 0x60, 0x00, 0x60, 0x00, 0xfd, 0x5b} + code = append(code, zeroArgs4()...) + code = append(code, 0x34, 0x73) + code = append(code, q.Bytes()...) + code = append(code, 0x5a, 0xf1, 0x50) + code = append(code, zeroArgs4()...) + code = append(code, 0x34, 0x73) + code = append(code, payout.Bytes()...) + return append(code, 0x5a, 0xf1, 0x00) +} + +func TestRevertedValueTransferMints(t *testing.T) { + env := newGapEnv(t, nil) + attacker, p, q, payout := addr(0xa1), addr(0xa2), addr(0xa3), addr(0xa4) + env.deploy(q, attacker, buildRefundCode()) + env.deploy(p, attacker, buildTrapCode(q, payout)) + const value = 100 * 1e8 + env.fund(attacker, 1000*1e8) + + addrs := []vmcommon.Address{attacker, p, q, payout} + committed := map[string]int64{} + var preSum int64 + for _, a := range addrs { + committed[string(env.coins.AccountKey(a.String()))] = env.balance(a) + preSum += env.balance(a) + } + + _, _, _, err := env.evm.Call(AccountRef(attacker), p, []byte{0x01}, 5_000_000, value) + require.NoError(t, err) + + kvs, _ := env.mdb.GetChangedData(env.mdb.GetLastSnapshot().GetID()) + for _, kv := range kvs { + if _, ok := committed[string(kv.Key)]; ok { + var acc ctypes.Account + require.NoError(t, ctypes.Decode(kv.Value, &acc)) + committed[string(kv.Key)] = acc.Balance + } + } + var postSum int64 + for _, a := range addrs { + b := committed[string(env.coins.AccountKey(a.String()))] + t.Logf("%s committed=%d cache=%d", a, b, env.balance(a)) + postSum += b + } + require.Equal(t, preSum, postSum, "coin supply changed: minted %d", postSum-preSum) +} + +// 同一帧内两次 SSTORE 同一槽位后 REVERT：Snapshot.revert 正序重放 prevalue， +// 槽位停在第一次写入的值而不是原值，父帧能读到被回滚帧写过的数据。 +func TestRevertedDoubleSStoreLeaks(t *testing.T) { + env := newGapEnv(t, nil) + user, s := addr(0xb1), addr(0xb2) + code := []byte{0x36, 0x60, 0x19, 0x57, // 有 calldata 跳到内层 + 0x60, 0x00, 0x60, 0x00, 0x60, 0x01, 0x60, 0x00, 0x60, 0x00, 0x30, 0x5a, 0xf1, 0x50, // CALL self(1 字节 calldata) + 0x60, 0x00, 0x54, 0x60, 0x01, 0x55, 0x00, // slot1 = slot0 + 0x5b, 0x60, 0x01, 0x60, 0x00, 0x55, 0x60, 0x02, 0x60, 0x00, 0x55, 0x60, 0x00, 0x60, 0x00, 0xfd} // slot0=1; slot0=2; REVERT + env.deploy(s, user, code) + _, _, _, err := env.evm.Call(AccountRef(user), s, nil, 5_000_000, 0) + require.NoError(t, err) + slot0 := env.mdb.GetState(s.String(), vmcommon.Hash{}) + slot1 := env.mdb.GetState(s.String(), vmcommon.BigToHash(new(big.Int).SetInt64(1))) + t.Logf("slot0=%x slot1=%x", slot0.Big(), slot1.Big()) + require.Equal(t, vmcommon.Hash{}, slot1, "parent frame observed a value written by a reverted frame") +} diff --git a/plugin/dapp/evm/executor/vm/state/journal_kv.go b/plugin/dapp/evm/executor/vm/state/journal_kv.go new file mode 100644 index 000000000..a9db1ffbc --- /dev/null +++ b/plugin/dapp/evm/executor/vm/state/journal_kv.go @@ -0,0 +1,103 @@ +// 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 ( + "sort" + + "github.com/33cn/chain33/common/db" + "github.com/33cn/chain33/types" +) + +// journalKV 是 MemoryStateDB 与执行器 StateDB 之间的写缓冲，作用等同 chain33 的 txcache， +// 但支持与 Snapshot 版本一一对应的嵌套回滚。 +// +// EVM 执行期间所有经由 MemoryStateDB 的状态写入（coins/token 账户等）只进入本缓冲： +// 回滚某个快照时，按 undo 日志倒序恢复写之前的值，调用方无需为每种写入单独实现 revert。 +// 执行成功后由 GetChangedData 把留下的写入一次性交给回执，并写回底层 StateDB。 +type journalKV struct { + parent db.KV + dirty map[string][]byte // nil 值表示该 key 在本交易内不存在 + undo []kvUndo + marks []int // marks[v] 为快照 v 创建时 undo 的长度 +} + +type kvUndo struct { + key string + prev []byte + hadPrev bool // 写之前 dirty 中是否已有该 key +} + +func newJournalKV(parent db.KV) *journalKV { + return &journalKV{parent: parent, dirty: make(map[string][]byte)} +} + +// Get 先读缓冲，再读底层 StateDB +func (j *journalKV) Get(key []byte) ([]byte, error) { + if v, ok := j.dirty[string(key)]; ok { + if v == nil { + return nil, types.ErrNotFound + } + return v, nil + } + return j.parent.Get(key) +} + +// Set 只写缓冲，并记录 undo +func (j *journalKV) Set(key []byte, value []byte) error { + k := string(key) + prev, had := j.dirty[k] + j.undo = append(j.undo, kvUndo{key: k, prev: prev, hadPrev: had}) + if value != nil { + value = append([]byte(nil), value...) + } + j.dirty[k] = value + return nil +} + +// Begin/Commit/Rollback 满足 db.KV 接口；交易级的提交与回滚由执行器框架负责 +func (j *journalKV) Begin() {} +func (j *journalKV) Commit() error { return nil } +func (j *journalKV) Rollback() { j.revert(0) } + +// snapshot 记录快照 version 的回滚点，version 必须等于当前快照数 +func (j *journalKV) snapshot(version int) { + j.marks = append(j.marks[:version], len(j.undo)) +} + +// revertTo 把缓冲恢复到快照 version 创建时的状态 +func (j *journalKV) revertTo(version int) { + if version < 0 || version >= len(j.marks) { + return + } + j.revert(j.marks[version]) + j.marks = j.marks[:version] +} + +func (j *journalKV) revert(n int) { + for i := len(j.undo) - 1; i >= n; i-- { + u := j.undo[i] + if u.hadPrev { + j.dirty[u.key] = u.prev + } else { + delete(j.dirty, u.key) + } + } + j.undo = j.undo[:n] +} + +// changes 返回缓冲中的全部写入，按 key 排序保证各节点一致 +func (j *journalKV) changes() []*types.KeyValue { + keys := make([]string, 0, len(j.dirty)) + for k := range j.dirty { + keys = append(keys, k) + } + sort.Strings(keys) + kvs := make([]*types.KeyValue, 0, len(keys)) + for _, k := range keys { + kvs = append(kvs, &types.KeyValue{Key: []byte(k), Value: j.dirty[k]}) + } + return kvs +} diff --git a/plugin/dapp/evm/executor/vm/state/journal_kv_test.go b/plugin/dapp/evm/executor/vm/state/journal_kv_test.go new file mode 100644 index 000000000..a54e1a7ab --- /dev/null +++ b/plugin/dapp/evm/executor/vm/state/journal_kv_test.go @@ -0,0 +1,102 @@ +package state + +import ( + "strings" + "testing" + + "github.com/33cn/chain33/account" + apimock "github.com/33cn/chain33/client/mocks" + "github.com/33cn/chain33/common/db" + ctypes "github.com/33cn/chain33/types" + "github.com/33cn/chain33/util" + "github.com/stretchr/testify/require" +) + +// recordKV 记录写入底层 StateDB 的 key，模拟 chain33 StateDB.GetSetKeys +type recordKV struct { + db.KV + keys []string +} + +func (r *recordKV) Set(key, value []byte) error { + r.keys = append(r.keys, string(key)) + return r.KV.Set(key, value) +} + +func newJournalTestDB(t *testing.T) (*MemoryStateDB, *recordKV, *account.DB) { + cfgStr := ctypes.GetDefaultCfgstring() + if !strings.Contains(cfgStr, "[exec.sub.evm]") { + cfgStr += "\n[exec.sub.evm]\nethMapFromExecutor=\"coins\"\nethMapFromSymbol=\"bty\"\n" + } + cfg := ctypes.NewChain33Config(cfgStr) + api := new(apimock.QueueProtocolAPI) + api.On("GetConfig").Return(cfg) + + dbDir, stateDB, localDB := util.CreateTestDB() + t.Cleanup(func() { util.CloseTestDB(dbDir, stateDB) }) + + raw, err := account.NewAccountDB(cfg, "coins", cfg.GetCoinSymbol(), stateDB) + require.NoError(t, err) + rec := &recordKV{KV: stateDB} + coins, err := account.NewAccountDB(cfg, "coins", cfg.GetCoinSymbol(), rec) + require.NoError(t, err) + return NewMemoryStateDB(rec, localDB, coins, 1, api), rec, raw +} + +// 被回滚的转账：缓冲恢复原值，不写底层 StateDB，也不进回执； +// 写入底层的 key 必须都在回执里（chain33 checkKV 的约束）。 +func TestJournalKVRevertedTransfer(t *testing.T) { + mdb, rec, raw := newJournalTestDB(t) + a, b, c := "1JRNjdEqp4LJ5fqycUBm9ayCKSeeskgMKR", "1NLHPEcbTWWxxU3dGUZBhayjrCHD3psX7k", "1MCftFynyvG2F4ED5mdHYgziDxx6vDrScs" + acc := raw.LoadAccount(a) + acc.Balance = 1000 + raw.SaveAccount(acc) + rec.keys = nil + + mdb.Snapshot() + v := mdb.Snapshot() + require.True(t, mdb.Transfer(a, b, 600)) + require.EqualValues(t, 600, mdb.GetBalance(b)) + mdb.RevertToSnapshot(v) + require.EqualValues(t, 1000, mdb.GetBalance(a)) + require.EqualValues(t, 0, mdb.GetBalance(b)) + require.Empty(t, rec.keys, "reverted transfer leaked into the underlying StateDB") + + // 回滚后余额已恢复，后续转账不能再花被回滚的钱 + require.False(t, mdb.Transfer(b, c, 600)) + require.True(t, mdb.Transfer(a, c, 300)) + + kvs, _ := mdb.GetChangedData(mdb.GetLastSnapshot().GetID()) + inReceipt := map[string]bool{} + for _, kv := range kvs { + inReceipt[string(kv.Key)] = true + } + require.NotContains(t, inReceipt, string(raw.AccountKey(b))) + for _, k := range rec.keys { + require.True(t, inReceipt[k], "key %s written to StateDB but missing from receipt", k) + } + require.EqualValues(t, 700, raw.LoadAccount(a).Balance) + require.EqualValues(t, 0, raw.LoadAccount(b).Balance) + require.EqualValues(t, 300, raw.LoadAccount(c).Balance) +} + +// 嵌套快照：内层回滚只撤销内层写入，外层回滚撤销全部 +func TestJournalKVNestedRevert(t *testing.T) { + mem, err := db.NewGoMemDB("t", "", 0) + require.NoError(t, err) + j := newJournalKV(mem) + k := []byte("k") + j.snapshot(0) + require.NoError(t, j.Set(k, []byte("1"))) + j.snapshot(1) + require.NoError(t, j.Set(k, []byte("2"))) + require.NoError(t, j.Set(k, []byte("3"))) + j.revertTo(1) + v, err := j.Get(k) + require.NoError(t, err) + require.Equal(t, "1", string(v)) + j.revertTo(0) + _, err = j.Get(k) + require.Equal(t, ctypes.ErrNotFound, err) + require.Empty(t, j.changes()) +} diff --git a/plugin/dapp/evm/executor/vm/state/snapshot.go b/plugin/dapp/evm/executor/vm/state/snapshot.go index 8151837a4..3ff93c14d 100644 --- a/plugin/dapp/evm/executor/vm/state/snapshot.go +++ b/plugin/dapp/evm/executor/vm/state/snapshot.go @@ -5,12 +5,9 @@ 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" @@ -23,7 +20,7 @@ import ( // 如果合约执行出错，会按生成顺序的倒序，依次调用变更对象的回滚接口进行数据回滚，并同步删除变更对象缓存 // 如果合约执行成功，会按生成顺序的正序，依次调用变更对象的数据和日志变更记录，回传给区块链 type DataChange interface { - revert(mdb *MemoryStateDB) error + revert(mdb *MemoryStateDB) getData(mdb *MemoryStateDB) []*types.KeyValue getLog(mdb *MemoryStateDB) []*types.ReceiptLog } @@ -41,27 +38,15 @@ func (ver *Snapshot) GetID() int { } // 回滚当前版本 -func (ver *Snapshot) revert() error { +func (ver *Snapshot) revert() bool { if ver.entries == nil { - return 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-- { - if err := ver.entries[i].revert(ver.statedb); err != nil { - return err - } - } - return nil + // 按生成顺序倒序回滚：同一版本内多次修改同一数据时，才能恢复到最早的值 + for i := len(ver.entries) - 1; i >= 0; i-- { + ver.entries[i].revert(ver.statedb) } - for _, entry := range ver.entries { - if err := entry.revert(ver.statedb); err != nil { - return err - } - } - return nil + return true } // 添加变更数据 @@ -153,24 +138,12 @@ type ( prev uint64 } - // 转账事件 - // 合约转账动作不执行回滚，失败后数据不会写入区块 + // 转账事件，只记录日志 + // 账户余额的 KV 写入 journalKV，回滚与回执数据都由 journalKV 负责 transferChange struct { baseChange 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 } // 合约生成日志事件 @@ -188,8 +161,7 @@ type ( ) // 在baseChang中定义三个基本操作，子对象中只需要实现必要的操作 -func (ch baseChange) revert(s *MemoryStateDB) error { - return nil +func (ch baseChange) revert(s *MemoryStateDB) { } func (ch baseChange) getData(s *MemoryStateDB) (kvset []*types.KeyValue) { @@ -201,9 +173,8 @@ func (ch baseChange) getLog(s *MemoryStateDB) (logs []*types.ReceiptLog) { } // 创建账户对象的回滚，需要删除缓存中的账户和变更标记 -func (ch createAccountChange) revert(s *MemoryStateDB) error { +func (ch createAccountChange) revert(s *MemoryStateDB) { delete(s.accounts, ch.account) - return nil } // 创建账户对象的数据集 @@ -217,16 +188,15 @@ func (ch createAccountChange) getData(s *MemoryStateDB) (kvset []*types.KeyValue return nil } -func (ch suicideChange) revert(mdb *MemoryStateDB) error { +func (ch suicideChange) revert(mdb *MemoryStateDB) { // 如果已经自杀过了，不处理 if ch.prev { - return nil + return } acc := mdb.accounts[ch.account] if acc != nil { acc.State.Suicided = ch.prev } - return nil } func (ch suicideChange) getData(mdb *MemoryStateDB) []*types.KeyValue { @@ -241,12 +211,11 @@ func (ch suicideChange) getData(mdb *MemoryStateDB) []*types.KeyValue { return nil } -func (ch nonceChange) revert(mdb *MemoryStateDB) error { +func (ch nonceChange) revert(mdb *MemoryStateDB) { acc := mdb.accounts[ch.account] if acc != nil { acc.State.Nonce = ch.prev } - return nil } func (ch nonceChange) getData(mdb *MemoryStateDB) []*types.KeyValue { @@ -258,13 +227,12 @@ func (ch nonceChange) getData(mdb *MemoryStateDB) []*types.KeyValue { return nil } -func (ch codeChange) revert(mdb *MemoryStateDB) error { +func (ch codeChange) revert(mdb *MemoryStateDB) { 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) { @@ -277,12 +245,11 @@ func (ch codeChange) getData(mdb *MemoryStateDB) (kvset []*types.KeyValue) { return nil } -func (ch abiChange) revert(mdb *MemoryStateDB) error { +func (ch abiChange) revert(mdb *MemoryStateDB) { acc := mdb.accounts[ch.account] if acc != nil { acc.Data.Abi = ch.prevabi } - return nil } func (ch abiChange) getData(mdb *MemoryStateDB) (kvset []*types.KeyValue) { @@ -294,12 +261,11 @@ func (ch abiChange) getData(mdb *MemoryStateDB) (kvset []*types.KeyValue) { return nil } -func (ch storageChange) revert(mdb *MemoryStateDB) error { +func (ch storageChange) revert(mdb *MemoryStateDB) { acc := mdb.accounts[ch.account] if acc != nil { acc.SetState(ch.key, ch.prevalue) } - return nil } func (ch storageChange) getData(mdb *MemoryStateDB) []*types.KeyValue { @@ -323,12 +289,11 @@ func (ch storageChange) getLog(mdb *MemoryStateDB) []*types.ReceiptLog { return nil } -func (ch refundChange) revert(mdb *MemoryStateDB) error { +func (ch refundChange) revert(mdb *MemoryStateDB) { mdb.refund = ch.prev - return nil } -func (ch addLogChange) revert(mdb *MemoryStateDB) error { +func (ch addLogChange) revert(mdb *MemoryStateDB) { logs := mdb.logs[ch.txhash] if len(logs) == 1 { delete(mdb.logs, ch.txhash) @@ -338,7 +303,6 @@ func (ch addLogChange) revert(mdb *MemoryStateDB) error { mdb.logSize-- log15.Info("addLogChange::revert", "mdb.logSize", mdb.logSize) - return nil } func (ch addLogChange) getLog(mdb *MemoryStateDB) []*types.ReceiptLog { @@ -350,62 +314,10 @@ func (ch addLogChange) getLog(mdb *MemoryStateDB) []*types.ReceiptLog { return ch.logs } -func (ch addPreimageChange) revert(mdb *MemoryStateDB) error { +func (ch addPreimageChange) revert(mdb *MemoryStateDB) { delete(mdb.preimages, ch.hash) - return nil } -func (ch transferChange) getData(mdb *MemoryStateDB) []*types.KeyValue { - return ch.data -} 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, 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 original []byte - if pre.existed { - original = pre.value - } - 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 2b120d021..552fe8a20 100644 --- a/plugin/dapp/evm/executor/vm/state/statedb.go +++ b/plugin/dapp/evm/executor/vm/state/statedb.go @@ -33,13 +33,15 @@ import ( // 执行器的Exec阶段会返回：交易收据、合约账户（包含合约地址、合约代码、合约存储信息） // 执行器的ExecLocal阶段会返回：合约创建人和合约的关联信息 type MemoryStateDB struct { - // StateDB 状态DB，从执行器框架传入 + // StateDB 状态DB：执行器框架传入的 StateDB 外包一层 journalKV， + // 执行期间的写入只进缓冲，随快照自动回滚 StateDB db.KV + kv *journalKV // LocalDB 本地DB，从执行器框架传入 LocalDB db.KVDB - // CoinsAccount Coins账户操作对象，从执行器框架传入 + // CoinsAccount Coins账户操作对象：执行器框架传入对象的副本，读写经由 journalKV CoinsAccount *account.DB //evm 平台账户地址 @@ -72,19 +74,21 @@ 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构建内存状态机对象 // 此对象的生命周期对应一个区块，在同一个区块内的多个交易执行时共享同一个DB对象 // 开始执行下一个区块时（执行器框架调用setEnv设置的区块高度发生变更时），会重新创建此DB对象 func NewMemoryStateDB(StateDB db.KV, LocalDB db.KVDB, CoinsAccount *account.DB, blockHeight int64, api client.QueueProtocolAPI) *MemoryStateDB { + kv := newJournalKV(StateDB) + if CoinsAccount != nil { + // 复制一份再换 DB，不改动执行器框架共享的 coins 账户对象 + coins := *CoinsAccount + CoinsAccount = coins.SetDB(kv) + } mdb := &MemoryStateDB{ - StateDB: StateDB, + StateDB: kv, + kv: kv, LocalDB: LocalDB, CoinsAccount: CoinsAccount, evmPlatformAddr: address.ExecAddress(api.GetConfig().ExecName("evm")), @@ -405,12 +409,11 @@ func (mdb *MemoryStateDB) RevertToSnapshot(version int) { // 从最近版本开始回滚 for index := len(mdb.snapshots) - 1; index >= version; index-- { - if err := mdb.snapshots[index].revert(); err != nil && mdb.revertErr == nil { - log15.Error("RevertToSnapshot failed", "snapshot", index, "err", err) - mdb.revertErr = err - } + mdb.snapshots[index].revert() } + mdb.kv.revertTo(version) + // 只保留回滚版本之前的版本数据 mdb.snapshots = mdb.snapshots[:version] mdb.versionID = version @@ -422,24 +425,13 @@ 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 mdb.versionID++ mdb.currentVer = &Snapshot{id: id, statedb: mdb} mdb.snapshots = append(mdb.snapshots, mdb.currentVer) + mdb.kv.snapshot(id) log15.Debug("MemoryStateDB::Snapshot", "mdb.versionID", mdb.versionID) return id } @@ -475,6 +467,9 @@ func (mdb *MemoryStateDB) GetReceiptLogs(addr string) (logs []*types.ReceiptLog) // 因为目前执行器每次执行都是一个新的MemoryStateDB，所以，所有的快照都是从0开始的， // 这里获取的应该是从0到目前快照的所有变更； // 另外，因为合约内部会调用其它合约，也会产生数据变更，所以这里返回的数据，不止是一个合约的数据。 +// +// coins/token 等经由 journalKV 的写入取缓冲中的最终值，同时写回底层 StateDB， +// 使同一区块内后续交易可见；本方法只应在执行成功、不再回滚时调用。 func (mdb *MemoryStateDB) GetChangedData(version int) (kvSet []*types.KeyValue, logs []*types.ReceiptLog) { if version < 0 || version >= len(mdb.snapshots) { return @@ -490,6 +485,12 @@ func (mdb *MemoryStateDB) GetChangedData(version int) (kvSet []*types.KeyValue, } } + for _, kv := range mdb.kv.changes() { + if err := mdb.kv.parent.Set(kv.Key, kv.Value); err != nil { + panic(err) + } + kvSet = append(kvSet, kv) + } return } @@ -544,63 +545,6 @@ 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, error) { - 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. 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...)} - 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, nil -} - // 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) @@ -627,15 +571,6 @@ 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, 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) } else { //paracross @@ -651,9 +586,7 @@ func (mdb *MemoryStateDB) Transfer(sender, recipient string, amount uint64) bool mdb.addChange(transferChange{ baseChange: baseChange{}, amount: value, - data: ret.KV, logs: ret.Logs, - preState: preState, }) } @@ -691,19 +624,6 @@ 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, 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 { return false, err @@ -712,9 +632,7 @@ func (mdb *MemoryStateDB) TransferToToken(from, recipient, symbol string, amount mdb.addChange(transferChange{ baseChange: baseChange{}, 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 index cfb1f8529..3e94e3eec 100644 --- a/plugin/dapp/evm/executor/vm/state/transfer_revert_test.go +++ b/plugin/dapp/evm/executor/vm/state/transfer_revert_test.go @@ -5,16 +5,12 @@ package state import ( - "bytes" - "errors" "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" @@ -24,44 +20,23 @@ import ( 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 { +func newTransferRevertMDB(t *testing.T) *MemoryStateDB { t.Helper() - cfgStr := strings.Replace(ctypes.GetDefaultCfgstring(), `Title="local"`, `Title="evm-transfer-revert-test"`, 1) + cfgStr := ctypes.GetDefaultCfgstring() 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) }) + coinsAccount, err := account.NewAccountDB(cfg, "coins", cfg.GetCoinSymbol(), stateDB) + require.NoError(t, err) - mdb := newTransferRevertMDBWith(t, cfg, stateDB) - mdb.LocalDB = localDB + mdb := NewMemoryStateDB(stateDB, localDB, coinsAccount, transferRevertTestHeight, api) + mdb.evmPlatformAddr = address.ExecAddress(cfg.ExecName(evmtypes.ExecutorName)) return mdb } @@ -75,8 +50,7 @@ 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. +// TestTransferRevertRestoresBalances: a reverted transfer leaves both balances untouched. func TestTransferRevertRestoresBalances(t *testing.T) { const ( sender = "14KEKbY3kNFLfQEGJbNweV4whre7NpqzuB" @@ -85,33 +59,17 @@ func TestTransferRevertRestoresBalances(t *testing.T) { 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) + mdb := newTransferRevertMDB(t) + 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)) + 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") - }) + 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 @@ -123,147 +81,15 @@ func TestSnapshotRevertSameSlotReverseOrder(t *testing.T) { 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 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" - 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") - _, 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) - recipientKey := mdb.CoinsAccount.AccountKey(recipient) + mdb := newTransferRevertMDB(t) + mdb.CreateAccount(contract.String(), "creator", "evm.test", "test") + mdb.SetState(contract.String(), slot, pre) 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.SetState(contract.String(), slot, first) + mdb.SetState(contract.String(), slot, second) + require.Equal(t, second, mdb.GetState(contract.String(), slot)) 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)) -} - -// 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)) + require.Equal(t, pre, mdb.GetState(contract.String(), slot), "reverse revert restores the pre-frame value") } diff --git a/plugin/dapp/evm/types/evm.go b/plugin/dapp/evm/types/evm.go index 8b43b653c..5899b84c2 100644 --- a/plugin/dapp/evm/types/evm.go +++ b/plugin/dapp/evm/types/evm.go @@ -50,10 +50,7 @@ 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. + // 已废弃，仅为兼容已发布配置中的 key 保留注册，见 ForkEVMTransferRevert 注释 cfg.RegisterDappFork(ExecutorName, ForkEVMTransferRevert, types.MaxHeight) } diff --git a/plugin/dapp/evm/types/types.go b/plugin/dapp/evm/types/types.go index d831621b8..7e7abe630 100644 --- a/plugin/dapp/evm/types/types.go +++ b/plugin/dapp/evm/types/types.go @@ -60,11 +60,9 @@ 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 已废弃，代码不再读取。 + // 1.76.4 用它门控手写的转账回滚；之后 EVM 写入统一经由 journalKV 随快照自动回滚，不再分叉。 + // 已发布的配置文件里带有这个 key，initForkConfig 遇到未注册的 key 会 panic，所以保留注册。 ForkEVMTransferRevert = "ForkEVMTransferRevert" ) From 1cfcdb0d551d0dbea07bfb61572a3dc0f992dc75 Mon Sep 17 00:00:00 2001 From: king Date: Wed, 4 Nov 2026 10:26:17 +0000 Subject: [PATCH 2/2] fix(evm): gate the journaled KV buffer behind ForkEVMTransferRevert Below ForkEVMTransferRevert MemoryStateDB writes straight to the executor StateDB, transferChange carries the receipt KV, Snapshot.revert runs forward and Create transfers before taking its snapshot, exactly as before #1348, so historical blocks replay unchanged. From the fork on writes go through journalKV, Snapshot.revert runs in reverse and Create snapshots first. The fork keeps the registration shipped in 1.76.4 (MaxHeight in code, -1 in the configs); the activation height is set at release time. The #1348 tests that pin both sides of the fork are restored and pass against the buffer. Co-Authored-By: Claude Fable 5 --- chain33.fork.toml | 2 +- chain33.para.toml | 2 +- .../dapp/evm/cmd/ci2/chain33.proxyminer.toml | 2 +- .../executor/transfer_revert_checkkv_test.go | 8 +- .../transfer_revert_integration_test.go | 66 +++++++---- .../executor/vm/runtime/create_revert_test.go | 89 ++++++++++---- plugin/dapp/evm/executor/vm/runtime/evm.go | 15 ++- plugin/dapp/evm/executor/vm/state/snapshot.go | 22 +++- plugin/dapp/evm/executor/vm/state/statedb.go | 46 +++++-- .../executor/vm/state/transfer_revert_test.go | 112 +++++++++++++----- plugin/dapp/evm/types/evm.go | 5 +- plugin/dapp/evm/types/types.go | 9 +- 12 files changed, 268 insertions(+), 110 deletions(-) diff --git a/chain33.fork.toml b/chain33.fork.toml index 4830d5506..a855a35a6 100644 --- a/chain33.fork.toml +++ b/chain33.fork.toml @@ -101,7 +101,7 @@ ForkEVMFixOverflow=0 # SELFDESTRUCT pays from the executing account: DELEGATECALL/CALLCODE no longer # spend the borrowed code contract balance ForkEVMSuicidePayer=0 -# 已废弃，代码不再读取；保留该 key 以兼容旧配置 +# Revertible value transfers; -1 keeps the fix off (resolved to MaxHeight) ForkEVMTransferRevert=-1 diff --git a/chain33.para.toml b/chain33.para.toml index 0a9f79d07..a6c8915cb 100644 --- a/chain33.para.toml +++ b/chain33.para.toml @@ -437,7 +437,7 @@ ForkEVMFixOverflow=0 # SELFDESTRUCT pays from the executing account: DELEGATECALL/CALLCODE no longer # spend the borrowed code contract balance ForkEVMSuicidePayer=0 -# 已废弃，代码不再读取；保留该 key 以兼容旧配置 +# Revertible value transfers; -1 keeps the fix off (resolved to MaxHeight) ForkEVMTransferRevert=-1 [fork.sub.blackwhite] diff --git a/plugin/dapp/evm/cmd/ci2/chain33.proxyminer.toml b/plugin/dapp/evm/cmd/ci2/chain33.proxyminer.toml index 82be203af..316bbd346 100644 --- a/plugin/dapp/evm/cmd/ci2/chain33.proxyminer.toml +++ b/plugin/dapp/evm/cmd/ci2/chain33.proxyminer.toml @@ -605,7 +605,7 @@ ForkEVMFixOverflow=0 # SELFDESTRUCT pays from the executing account: DELEGATECALL/CALLCODE no longer # spend the borrowed code contract balance ForkEVMSuicidePayer=0 -# 已废弃，代码不再读取；保留该 key 以兼容旧配置 +# Revertible value transfers; -1 keeps the fix off (resolved to MaxHeight) ForkEVMTransferRevert=-1 [fork.sub.evmxgo] diff --git a/plugin/dapp/evm/executor/transfer_revert_checkkv_test.go b/plugin/dapp/evm/executor/transfer_revert_checkkv_test.go index 1a1fadf56..bd78f298c 100644 --- a/plugin/dapp/evm/executor/transfer_revert_checkkv_test.go +++ b/plugin/dapp/evm/executor/transfer_revert_checkkv_test.go @@ -72,8 +72,8 @@ func checkReceiptCoversSetKeys(memset []string, kvs []*ctypes.KeyValue) error { return nil } -// newProductionExec builds an EVM executor over the production StateDB with ForkExecRollback -// open at height 0. +// 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) @@ -114,7 +114,7 @@ func buildForwardAndStoreCode(callee vcomm.Address, value uint64) []byte { func productionStateDBFixture(t *testing.T) (*ctypes.Chain33Config, *cexecutor.StateDB, db.KVDB) { t.Helper() restoreBtcAddressDriver(t) - cfg := newTransferRevertConfig(t) + 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) @@ -126,7 +126,7 @@ func productionStateDBFixture(t *testing.T) (*ctypes.Chain33Config, *cexecutor.S } // TestRevertedCallValueTxSucceedsUnderCheckKV is the reviewer's positive case: the outer call -// succeeds, an inner CALL{value} reverts, execution continues and writes state. The +// 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 ( diff --git a/plugin/dapp/evm/executor/transfer_revert_integration_test.go b/plugin/dapp/evm/executor/transfer_revert_integration_test.go index bf1795ec6..769e419cb 100644 --- a/plugin/dapp/evm/executor/transfer_revert_integration_test.go +++ b/plugin/dapp/evm/executor/transfer_revert_integration_test.go @@ -5,9 +5,10 @@ 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 was spent by a later call in the same tx, -// minting the moved amount. The reverted transfer is now rolled back by journalKV, so the later -// spend has no balance to take. +// 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" @@ -21,7 +22,7 @@ import ( "github.com/stretchr/testify/require" ) -func newTransferRevertConfig(t *testing.T) *ctypes.Chain33Config { +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]") { @@ -29,6 +30,8 @@ func newTransferRevertConfig(t *testing.T) *ctypes.Chain33Config { } 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 } @@ -142,29 +145,40 @@ func TestRevertedCallValueCannotBeSpent(t *testing.T) { require.NoError(t, err) vcomm.InitEvmAddressDriver(driver) - cfg := newTransferRevertConfig(t) - 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()) + 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") + }) } - 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, sum(), "committed total must not grow") - require.Zero(t, balanceOf(exec, target.String()), "the forwarder must not spend a phantom credit") } func balanceOf(exec *EVMExecutor, addr string) int64 { diff --git a/plugin/dapp/evm/executor/vm/runtime/create_revert_test.go b/plugin/dapp/evm/executor/vm/runtime/create_revert_test.go index cbc7116ba..ac6552961 100644 --- a/plugin/dapp/evm/executor/vm/runtime/create_revert_test.go +++ b/plugin/dapp/evm/executor/vm/runtime/create_revert_test.go @@ -6,16 +6,34 @@ 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. The snapshot is now taken first, so the value returns to the caller. +// 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 { @@ -37,39 +55,62 @@ func TestFailedNestedCreateReturnsValue(t *testing.T) { const stash = int64(1000) const attached = uint64(5) - env := newGapEnv(t, nil) - caller, parent := addr(0x31), addr(0x51) - env.fund(parent, stash) - env.deploy(parent, caller, buildCreateWithValueCode(attached, []byte{0xfe})) + 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, stash, env.balance(parent)) + _, _, _, 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 old code dropped the transfer in addChange: the balances -// moved but the receipt never carried the keys, so chain33's checkKV rejected the transaction. -// Both keys must now land in the receipt. +// 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) - env := newGapEnv(t, nil) - caller, contract := addr(0x61), addr(0x71) - env.fund(caller, stash) + 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)) + _, 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.True(t, hasKV(kvs, key), "receipt key %x", key) + 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) + } + }) } } diff --git a/plugin/dapp/evm/executor/vm/runtime/evm.go b/plugin/dapp/evm/executor/vm/runtime/evm.go index acec89577..cb916a6d7 100644 --- a/plugin/dapp/evm/executor/vm/runtime/evm.go +++ b/plugin/dapp/evm/executor/vm/runtime/evm.go @@ -537,14 +537,20 @@ func (evm *EVM) Create(caller ContractRef, contractAddr common.Address, code []b return nil, -1, gas, berr } - // 先打快照再转账，创建失败时转入的金额随合约账户一起回滚 - snapshot = evm.StateDB.Snapshot() // 向合约地址转账 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) - evm.StateDB.RevertToSnapshot(snapshot) return nil, -1, gas, model.ErrInsufficientBalance } } else { @@ -566,6 +572,9 @@ func (evm *EVM) Create(caller ContractRef, contractAddr common.Address, code []b contract.SetCallCode(&contractAddr, common.ToHash(code), code) // 创建一个新的账户对象（合约账户） + 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 3ff93c14d..b940c141c 100644 --- a/plugin/dapp/evm/executor/vm/state/snapshot.go +++ b/plugin/dapp/evm/executor/vm/state/snapshot.go @@ -42,9 +42,16 @@ func (ver *Snapshot) revert() bool { if ver.entries == nil { return true } - // 按生成顺序倒序回滚：同一版本内多次修改同一数据时，才能恢复到最早的值 - for i := len(ver.entries) - 1; i >= 0; i-- { - ver.entries[i].revert(ver.statedb) + // ForkEVMTransferRevert 之后按生成顺序倒序回滚：同一版本内多次修改同一数据时，才能恢复到最早的值。 + // 分叉之前保留正序，历史区块按原结果重放。 + if ver.statedb.kv != nil { + 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) } return true } @@ -138,11 +145,13 @@ type ( prev uint64 } - // 转账事件，只记录日志 - // 账户余额的 KV 写入 journalKV，回滚与回执数据都由 journalKV 负责 + // 转账事件 + // ForkEVMTransferRevert 之前：data 携带回执 KV，回滚时不处理（缓存中的转账不会撤销）； + // 之后：data 为空，账户余额的回滚与回执数据都由 journalKV 负责，这里只记录日志 transferChange struct { baseChange amount int64 + data []*types.KeyValue logs []*types.ReceiptLog } @@ -318,6 +327,9 @@ func (ch addPreimageChange) revert(mdb *MemoryStateDB) { delete(mdb.preimages, ch.hash) } +func (ch transferChange) getData(mdb *MemoryStateDB) []*types.KeyValue { + return ch.data +} func (ch transferChange) getLog(mdb *MemoryStateDB) []*types.ReceiptLog { return ch.logs } diff --git a/plugin/dapp/evm/executor/vm/state/statedb.go b/plugin/dapp/evm/executor/vm/state/statedb.go index 552fe8a20..f8aa0fc70 100644 --- a/plugin/dapp/evm/executor/vm/state/statedb.go +++ b/plugin/dapp/evm/executor/vm/state/statedb.go @@ -33,15 +33,16 @@ import ( // 执行器的Exec阶段会返回：交易收据、合约账户（包含合约地址、合约代码、合约存储信息） // 执行器的ExecLocal阶段会返回：合约创建人和合约的关联信息 type MemoryStateDB struct { - // StateDB 状态DB：执行器框架传入的 StateDB 外包一层 journalKV， + // StateDB 状态DB，从执行器框架传入；ForkEVMTransferRevert 之后外包一层 journalKV， // 执行期间的写入只进缓冲，随快照自动回滚 StateDB db.KV - kv *journalKV + // kv 为 nil 表示分叉之前的旧行为：写入直达 StateDB，transferChange 自带回执 KV + kv *journalKV // LocalDB 本地DB，从执行器框架传入 LocalDB db.KVDB - // CoinsAccount Coins账户操作对象：执行器框架传入对象的副本，读写经由 journalKV + // CoinsAccount Coins账户操作对象，从执行器框架传入；分叉之后为绑定 journalKV 的副本 CoinsAccount *account.DB //evm 平台账户地址 @@ -80,14 +81,18 @@ type MemoryStateDB struct { // 此对象的生命周期对应一个区块，在同一个区块内的多个交易执行时共享同一个DB对象 // 开始执行下一个区块时（执行器框架调用setEnv设置的区块高度发生变更时），会重新创建此DB对象 func NewMemoryStateDB(StateDB db.KV, LocalDB db.KVDB, CoinsAccount *account.DB, blockHeight int64, api client.QueueProtocolAPI) *MemoryStateDB { - kv := newJournalKV(StateDB) - if CoinsAccount != nil { - // 复制一份再换 DB，不改动执行器框架共享的 coins 账户对象 - coins := *CoinsAccount - CoinsAccount = coins.SetDB(kv) + var kv *journalKV + if api.GetConfig().IsDappFork(blockHeight, evmtypes.ExecutorName, evmtypes.ForkEVMTransferRevert) { + kv = newJournalKV(StateDB) + StateDB = kv + if CoinsAccount != nil { + // 复制一份再换 DB，不改动执行器框架共享的 coins 账户对象 + coins := *CoinsAccount + CoinsAccount = coins.SetDB(kv) + } } mdb := &MemoryStateDB{ - StateDB: kv, + StateDB: StateDB, kv: kv, LocalDB: LocalDB, CoinsAccount: CoinsAccount, @@ -412,7 +417,9 @@ func (mdb *MemoryStateDB) RevertToSnapshot(version int) { mdb.snapshots[index].revert() } - mdb.kv.revertTo(version) + if mdb.kv != nil { + mdb.kv.revertTo(version) + } // 只保留回滚版本之前的版本数据 mdb.snapshots = mdb.snapshots[:version] @@ -431,7 +438,9 @@ func (mdb *MemoryStateDB) Snapshot() int { mdb.versionID++ mdb.currentVer = &Snapshot{id: id, statedb: mdb} mdb.snapshots = append(mdb.snapshots, mdb.currentVer) - mdb.kv.snapshot(id) + if mdb.kv != nil { + mdb.kv.snapshot(id) + } log15.Debug("MemoryStateDB::Snapshot", "mdb.versionID", mdb.versionID) return id } @@ -468,7 +477,7 @@ func (mdb *MemoryStateDB) GetReceiptLogs(addr string) (logs []*types.ReceiptLog) // 这里获取的应该是从0到目前快照的所有变更； // 另外，因为合约内部会调用其它合约，也会产生数据变更，所以这里返回的数据，不止是一个合约的数据。 // -// coins/token 等经由 journalKV 的写入取缓冲中的最终值，同时写回底层 StateDB， +// 分叉之后，coins/token 等经由 journalKV 的写入取缓冲中的最终值，同时写回底层 StateDB， // 使同一区块内后续交易可见；本方法只应在执行成功、不再回滚时调用。 func (mdb *MemoryStateDB) GetChangedData(version int) (kvSet []*types.KeyValue, logs []*types.ReceiptLog) { if version < 0 || version >= len(mdb.snapshots) { @@ -485,6 +494,9 @@ func (mdb *MemoryStateDB) GetChangedData(version int) (kvSet []*types.KeyValue, } } + if mdb.kv == nil { + return + } for _, kv := range mdb.kv.changes() { if err := mdb.kv.parent.Set(kv.Key, kv.Value); err != nil { panic(err) @@ -545,6 +557,14 @@ const ( Error ) +// legacyTransferKV 分叉之前转账的回执 KV 由 transferChange 携带；分叉之后由 journalKV 提供 +func (mdb *MemoryStateDB) legacyTransferKV(kvs []*types.KeyValue) []*types.KeyValue { + if mdb.kv != nil { + return nil + } + return kvs +} + // 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) @@ -586,6 +606,7 @@ func (mdb *MemoryStateDB) Transfer(sender, recipient string, amount uint64) bool mdb.addChange(transferChange{ baseChange: baseChange{}, amount: value, + data: mdb.legacyTransferKV(ret.KV), logs: ret.Logs, }) } @@ -632,6 +653,7 @@ func (mdb *MemoryStateDB) TransferToToken(from, recipient, symbol string, amount mdb.addChange(transferChange{ baseChange: baseChange{}, amount: amount, + data: mdb.legacyTransferKV(receipt.GetKV()), logs: receipt.GetLogs(), }) 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 index 3e94e3eec..e6023584b 100644 --- a/plugin/dapp/evm/executor/vm/state/transfer_revert_test.go +++ b/plugin/dapp/evm/executor/vm/state/transfer_revert_test.go @@ -11,6 +11,7 @@ import ( "github.com/33cn/chain33/account" apimock "github.com/33cn/chain33/client/mocks" "github.com/33cn/chain33/common/address" + "github.com/33cn/chain33/common/db" ctypes "github.com/33cn/chain33/types" "github.com/33cn/chain33/util" "github.com/33cn/plugin/plugin/dapp/evm/executor/vm/common" @@ -20,23 +21,43 @@ import ( const transferRevertTestHeight = int64(10) -func newTransferRevertMDB(t *testing.T) *MemoryStateDB { +// 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 := ctypes.GetDefaultCfgstring() + 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. +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) }) - coinsAccount, err := account.NewAccountDB(cfg, "coins", cfg.GetCoinSymbol(), stateDB) - require.NoError(t, err) - mdb := NewMemoryStateDB(stateDB, localDB, coinsAccount, transferRevertTestHeight, api) - mdb.evmPlatformAddr = address.ExecAddress(cfg.ExecName(evmtypes.ExecutorName)) + mdb := newTransferRevertMDBWith(t, cfg, stateDB) + mdb.LocalDB = localDB return mdb } @@ -50,7 +71,8 @@ func coinBalance(mdb *MemoryStateDB, addr string) int64 { return mdb.CoinsAccount.LoadAccount(addr).Balance } -// TestTransferRevertRestoresBalances: a reverted transfer leaves both balances untouched. +// 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" @@ -59,17 +81,33 @@ func TestTransferRevertRestoresBalances(t *testing.T) { moved = uint64(400) ) - mdb := newTransferRevertMDB(t) - 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") + 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 @@ -81,15 +119,31 @@ func TestSnapshotRevertSameSlotReverseOrder(t *testing.T) { first := common.BytesToHash([]byte("first")) second := common.BytesToHash([]byte("second")) - mdb := newTransferRevertMDB(t) - 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") + 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") + }) } diff --git a/plugin/dapp/evm/types/evm.go b/plugin/dapp/evm/types/evm.go index 5899b84c2..8b43b653c 100644 --- a/plugin/dapp/evm/types/evm.go +++ b/plugin/dapp/evm/types/evm.go @@ -50,7 +50,10 @@ 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) - // 已废弃，仅为兼容已发布配置中的 key 保留注册，见 ForkEVMTransferRevert 注释 + // 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) } diff --git a/plugin/dapp/evm/types/types.go b/plugin/dapp/evm/types/types.go index 7e7abe630..f86e8efd7 100644 --- a/plugin/dapp/evm/types/types.go +++ b/plugin/dapp/evm/types/types.go @@ -60,9 +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 已废弃，代码不再读取。 - // 1.76.4 用它门控手写的转账回滚；之后 EVM 写入统一经由 journalKV 随快照自动回滚，不再分叉。 - // 已发布的配置文件里带有这个 key，initForkConfig 遇到未注册的 key 会 panic，所以保留注册。 + // ForkEVMTransferRevert makes EVM state writes revertible: MemoryStateDB buffers them in + // journalKV, which rolls back with each snapshot, 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" )