Repository navigation
fix(evm): make reverted value transfers revertible - #1348
Conversation
MemoryStateDB.Transfer writes the new balances straight into the account cache and journals a transferChange that has no revert, so after a reverted value transfer the in-memory cache and the receipt KV disagree; the two are supposed to stay in step, and only one of them is committed. - transferChange now carries the raw pre-transfer KV of both accounts and restores it on revert, deleting keys that did not exist before. - Create takes its snapshot before the value transfer, so a failed create rolls the attached value back together with the account. - Snapshot.revert unwinds entries in reverse creation order, as its interface contract requires. A successful top-level CREATE with an attached value changes too. Its transfer used to run before any snapshot was open, so addChange dropped it: the balances moved but the receipt never carried the keys, and checkKV rejected the transaction under ForkExecRollback (without that fork the value was committed with no receipt entry). From the fork on the snapshot opens first, so both balance keys reach the receipt and the value is committed through it. Production uses executor.StateDB, which has no Delete, so a key created by a reverted transfer is shadowed with Set(key, nil). The key still lands in GetSetKeys while dropping out of the receipt, so checkKV rejects the attack transaction with ErrNotAllowMemSetKey instead of silently reconciling the balance. All of this is gated behind the new ForkEVMTransferRevert. It is registered at MaxHeight (off), so a chain opts in with a height under [fork.sub.evm]; the pre-fork path keeps the old behaviour. Registering it also means every config with a [fork.sub.evm] section must list the key, because initForkConfig rejects registered dapp forks absent from the file. The bundled templates list it as -1, which resolves to MaxHeight. Co-authored-by: Claude <noreply@anthropic.com>
69b5991 to
8379439
Compare
libangzhu
left a comment
There was a problem hiding this comment.
总结
回滚后的余额被写回了内存缓存,快照也改成了倒序撤销,铸造在缓存里确实补上了。生产用的 executor.StateDB 上还有一个更硬的问题:一次本应成功的交易,只要内部有一笔带回滚的转账,就会在 checkKV 被整笔打成只扣手续费。ForkEVMTransferRevert 在 GetSetKeys 和回执对齐之前不能开。
Blocker:回滚用 Set 记账,成功交易过不了 checkKV
transferChange.revert 把转账前的账户 KV 写回去。生产环境的 executor.StateDB 没有 Delete,新出现的收款地址只能 Set(key, nil)。Set 在交易事务里会把 key 追加进 GetSetKeys(chain33/executor/statedb.go 的 Set:intx 为真时 s.keys = append(...))。
intx 只在 ForkExecRollback 下由 begin() 打开,主网走的就是这条路径。回滚的快照随后被丢掉,GetChangedData 不会带上这些 key。checkKV(chain33/executor/execenv.go)要求每个 Set 过的 key 都出现在回执里,否则返回 ErrNotAllowMemSetKey。
外层调用成功、内层 CALL{value} 回滚时就会撞上。转账先 Set 了双方账户,回滚再 Set 一次;只要某个账户 key 不在存活回执里,整笔交易失败。合约在内层转账失败后继续执行并做 SSTORE,也一样失败。这不是“收款账户原来不存在”的边角:双方账户原来都在、只是这笔转账是本交易里对它们的唯一一次写入,也会失败。
execTx 收到这个错误后回滚缓存,对外返回的是手续费回执和 nil 错误(execenv.go 里 execTxOne 失败则 rollback(),非 API 环境错误仍 return feelog, nil)。交易仍进区块,用户付了手续费,外层已经成功的状态全部丢掉。铸造被挡住了,但 call{value} 失败后继续执行的合约,分叉打开后都会变成这种只扣费的失败。
今天的铸造之所以能落块,是因为被回滚的那次转账的 key,会被外层转账和随后那笔真实花费覆盖进回执,checkKV 只查 key 在不在、不查金额。分叉后第二次花费不再发生,收款地址的 key 留在 GetSetKeys 里、不在回执里,整笔被拒。
两组测试各自成立,合在一起却不是生产行为。TestRevertedCallValueCannotBeSpent 用的是带 Delete 的 goleveldb,Exec 返回 ExecOk,不经过 checkKV。TestRevertedTransferTxRejectedByCheckKV 用的是生产 StateDB,断言的就是整笔被拒。说明里的“分叉后不再铸出余额”只在前一个测试的存储上成立。
回执里塞 {key, nil} 不能当修复。内存缓存把 nil 当成不存在,MAVL 的 Tree.Set 会把空值写成叶子,下一笔 LoadAccount 解码失败会 panic。倒序撤销和 Create 提前打快照可以留,但 revert 必须让 GetSetKeys 和存活回执一致:本帧里新出现的 key 要从 txcache 和 key 列表里摘掉,已有账户要恢复成回执里那份值。这需要 StateDB 提供“撤销本次写入且不再计入 key 列表”的能力,只在 plugin 里 Set 做不到。
Suggestion
capturePreState 在 Get 返回任何错误时都记 existed=false。转账前读收款账户若遇到超时或存储错误,随后 LoadAccount 又读成功并完成转账,回滚会按“原来没有这个 key”删掉真实账户。只有 ErrNotFound 能记成不存在;其他错误应该让这笔转账失败,并打出 key 和错误。
Nit
chain33.para.toml 里新行紧贴着后面的 [fork.sub.blackwhite],能解析,和其他分叉段的空行不一致。
正面
- 分叉默认
MaxHeight,toml 里的-1在initForkConfig会转成MaxHeight,不配的链不会从创世改写历史。 CALL本来就在转账前打快照。Create改到转账前打快照后,失败的带金额CREATE能把金额一起卷回去;成功的带金额CREATE也会把账户 key 写进回执,补上原先currentVer == nil时addChange被丢掉、checkKV直接失败的洞。Snapshot.revert改成倒序是对的。同一帧对同一槽写两次时,正序回滚会留下中间值,父帧还能读到并提交。这个行为和转账回滚绑在同一个分叉上,注释里已经写明。- 预镜像做了拷贝,
coins和paracross的 key 与Transfer/ExecTransfer实际写入的 key 一致。
测试缺口
缺一条走生产 StateDB(没有 Delete,ForkExecRollback 打开)的用例:外层调用成功,内层 CALL 带金额后回滚,断言 checkKV 通过,并且提交后的余额等于回滚后的余额。现在这条路径被写成“应当拒绝”。TransferToToken 也没有对应用例。
transferChange.revert restored the pre-transfer balances with Set. Under ForkExecRollback every Set lands in the transaction key list, and chain33's checkKV requires each of those keys to appear in the surviving receipt. A restored key whose only write was the reverted transfer stays in the list but drops out of the receipt, so the whole transaction is rejected: a contract that sends value, sees the call revert and keeps executing turned into a fee-only failed transaction. The mint shape was rejected by the same check, not by reconciling the balance. chain33 now offers StateDB.RevertSet, which undoes one write of the current transaction and also drops its key-list entry. Use it for each captured key; a key that did not exist before the transfer passes nil. KVs without RevertSet, and calls outside a transaction, keep the direct-write path. capturePreState now treats only ErrNotFound as "the key did not exist". Any other read error fails the transfer and logs the key: recording "absent" from a store timeout would let a revert delete a real account. A failed revert is recorded on MemoryStateDB and innerExec turns it into a failed transaction, since the account cache and the receipt no longer describe the same state; RevertToSnapshot itself has no error channel to the EVM. Depends on 33cn/chain33#1412 (StateDB.RevertSet): this PR cannot merge before chain33 releases a version containing it. The replace to the bysomeone fork at 0ba08b76ebaf is the test-time pin and must be dropped when that release lands. Co-authored-by: Claude <noreply@anthropic.com>
The previous tests either ran on goleveldb (which has Delete) or asserted
that a transaction with a reverted value transfer is rejected. Neither
covers what production does now, and the rejection assertion locked in the
bug.
Add the pair the reviewer asked for, both on the production shape: concrete
executor.StateDB, no Delete, ForkExecRollback open at height 0, Begin()
called, and the receipt checked the way checkKV does.
- Positive: the outer call succeeds, an inner CALL{value} reverts, execution
continues and writes state; the transaction is ExecOk, every Set key is in
the receipt, and the committed balances are the post-revert ones.
- Negative: the mint shape still cannot spend the reverted credit. The
transaction now succeeds instead of being rejected, but no value is
created.
TransferToToken gets the same pair, built on a MemoryStateDB over the
production StateDB with a created token: a legitimate transfer after a
revert passes checkKV, and the reverted credit cannot be spent.
Also pin capturePreState failing the transfer on a non-ErrNotFound read
error, and a failed RevertSet being recorded instead of swallowed.
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
Registering a new system fork makes initForkConfig reject any config whose [fork.system] section does not list it, and the node exits at startup. chain33.para.toml and chain33.fork.toml both carry that section, so every para-chain deployment stopped booting once the fork-gated chain33 was linked in. Write -1 to keep the rule off until a chain schedules it. Co-authored-by: Claude <noreply@anthropic.com>
libangzhu
left a comment
There was a problem hiding this comment.
上次意见
checkKV 那条已经对上。回滚改走 StateDB.RevertSet:每把 key 从本次交易的 GetSetKeys 里摘掉最后一次写入,交易缓存恢复成转账前的值;原来不存在的 key 传 nil,之后读出来是 ErrNotFound。外层调用成功、内层 CALL{value} 回滚时,收款地址不再留在 key 列表里,回执和 checkKV 能对上,后面的状态写入可以提交。
读失败只把 ErrNotFound 当成“原来没有这个 key”,其它错误会让这笔转账失败。chain33.para.toml 的空行也补上了。新增的两组测试走的是生产路径:具体的 executor.StateDB、没有 Delete、ForkExecRollback 打开,一正一反,token 转账同样覆盖了。这两组在当前头上可以跑过。
Blocker:replace 不能合入
go.mod 把整个模块指到 github.com/bysomeone/chain33 v1.72.1-0.20261103141406-9f5c055ab050,require 仍写着 33cn/chain33 v1.72.7-0.20261016082959-e1bde75e25d0。那个发布版本没有 RevertSet。replace 只在 plugin 自己是主模块时生效,下游用发布版 chain33 编这份 plugin 会编译失败。
这个 pin 也不只是 RevertSet。它还带着 chain33#1412 那条信封分支,以及一处没有分叉门的内存池改动:交易组的 nonce 连续性检查从 IsEthSignID 改成只对内嵌了以太坊交易的成员生效(system/mempool/check.go 的 evmGroupTxNonceCheck)。信封校验本身在 ForkEthSignEnvelope 后面,代码默认 MaxHeight,toml 里写 -1,不打开就还是旧的验签。
ForkEthSignEnvelope=-1 要和注册了这个分叉的 chain33 一起发。先把 key 写进配置、二进制还是现在的 1.72.7,开启了分叉检查的节点会在启动时报 system fork 不存在然后退出。
合入顺序:RevertSet 进 33cn/chain33 的发布版本,plugin 把 require 指到那个版本并删掉 replace,两份 fork 配置里的新 key 跟着那次发布走。
A failed revert leaves the account cache and the receipt describing different
state, so innerExec turns it into a failed transaction. Nothing covered that
guard: removing it left every package green. The test drives a call whose
inner CALL{value} reverts through a StateDB whose RevertSet always errors, and
also pins that the next execution starts with the error cleared.
Co-authored-by: Claude <noreply@anthropic.com>
The release contains the RevertSet hook this branch calls and the fork-gated envelope rule, so the module can now be built from the published version instead of a pinned commit. Co-authored-by: Claude <noreply@anthropic.com>
libangzhu
left a comment
There was a problem hiding this comment.
交叉校验评审(head c3f474c)
相对上次关注的 125ab237,本 head 新增:TestInnerExecFailsWhenSnapshotRevertFails、依赖改为正式 chain33 v1.73.0(去掉临时 replace)。
结论
Approve(同意合并)。 上一轮将 chain33.proxyminer.toml 缺 ForkEthSignEnvelope 定为 Blocker——交叉验证后应撤销;其余结论(依赖、回滚修复、测试、发布条件)仍成立。
交叉验证要点
| 项 | 结果 |
|---|---|
go.mod → github.com/33cn/chain33 v1.73.0,无 replace github.com/33cn/chain33 |
✅ tag 含 StateDB.RevertSet |
RevertSet 失败时 innerExec 整笔失败 + 下一笔 ResetRevertError 清错 |
✅ TestInnerExecFailsWhenSnapshotRevertFails(生产 StateDB 形态) |
125ab237 在 chain33.para.toml / chain33.fork.toml 补 ForkEthSignEnvelope=-1 |
✅ 平行链/严格 fork 模板需要;未配会触发 initForkConfig 缺项 |
proxyminer 缺 ForkEthSignEnvelope → CI2 启动 panic |
❌ 不成立 |
proxyminer 说明: plugin/dapp/evm/cmd/ci2/chain33.proxyminer.toml 首行附近为 DisableForkCheck=true。在 v1.73.0 下缺 system fork 项不会 panic;未写入的 ForkEthSignEnvelope 保持代码默认 MaxHeight(关闭),与显式 -1 等价。CI2 docker-compose 使用该文件可以正常起链。
Findings(非 Blocker)
N1 — 配置一致性(可选)
仍建议在 proxyminer.toml 的 [fork.system] 补上 ForkEthSignEnvelope=-1,与 chain33.fork.toml / para.toml 一致,避免日后关闭 DisableForkCheck 时踩坑。
N2 — 发布
ForkEVMTransferRevert 默认关闭(模板 -1)。合 PR 后各链需在 [fork.sub.evm] 配置启用高度(晚于全网升级);bityuan 等部署配置在独立仓库跟进。
N3 — 依赖
合并/发布需以 chain33 v1.73.0 已发布 为前提(含 RevertSet 与 ForkEthSignEnvelope 注册)。
做得好的地方
- 临时 fork
replace已去掉,依赖回到正式版本。 - 回滚失败守卫有执行器级测试,去掉守卫会红。
- 生产
StateDB+checkKV正反用例保留,修复意图清晰。
|
🎉 This PR is included in version 1.76.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
What
MemoryStateDB.Transferwrites the new balances straight into the account cache and then journals atransferChange. That change has norevert, so it inherits the empty one frombaseChange. When an EVM sub-call with value is reverted,RevertToSnapshotdrops the journal entry but the cache keeps the transfer, and the in-memory state and the receipt KV — which are supposed to stay in step — end up disagreeing.Three fixes, all behind a new
ForkEVMTransferRevert:transferChangecarries the raw pre-transfer KV of both accounts and restores it on revert, deleting keys that did not exist before.TransferToTokendoes the same for the token account keys.Createtakes its snapshot before the value transfer instead of after, so a failed create rolls the attached value back together with the account.Snapshot.revertunwinds entries in reverse creation order, as the interface contract at the top of the file requires.The pre-fork path keeps the previous behaviour exactly, so replaying history is unaffected; only the added config lookups differ.
Fork
ForkEVMTransferRevertis a new constant inplugin/dapp/evm/types. It is not registered inInitFork/RegisterDappFork, so a chain that does not configure it resolves it toMaxHeight(effectively off). The height is supplied by the chain config at release time.Verification
go test -count=1 ./plugin/dapp/evm/...— all packages green.TestTransferRevertRestoresBalances— a reverted transfer restores the sender and drops a recipient key that did not exist;TestSnapshotRevertSameSlotReverseOrder— two writes to the same slot in one frame followed by a revert restore the pre-frame value;TestFailedNestedCreateReturnsValue— the caller gets the attached value back;TestRevertedCallValueCannotBeSpent— a transaction-level repro: post-fork no value is created.gofmt -lis clean on the changed files.Notes
checkKVis untouched.TransferToTokenhas no dedicated token-precompile test — the precompile needs a token LocalDB harness — but its revert path is the same code asTransfer's.