Repository navigation
fix: make from-genesis replay work again (GenesisInit amount bound, exec-time address check) - #1401
Conversation
…dy were GenesisInit has to reject a negative amount -- safeAdd only guards overflow, so a negative amount is persisted as a negative balance -- but it must not apply the rest of CheckAmount. CheckAmount also rejects amount == 0 and everything at or above MaxCoin*coinPrecision, a tighter upper bound than the MaxTokenBalance that safeAdd enforces. GenesisInit is not a genesis-only entry point: the token executor calls it from every tokenFinishCreate with the token's total, which the pre-create step validated against MaxTokenBalance. A token whose total falls between the two bounds was accepted by the blocks that are already on the chain, so the tighter bound makes those blocks unreplayable -- a node syncing from scratch fails on them with ErrCheckStateHash. The genesis path keeps the validation it already had: Coins.Exec_Genesis calls CheckAmount at the executor entry before it reaches GenesisInit. Co-authored-by: Claude <[email protected]>
Verification only -- this branch must not be merged. Adds 33cn/chain33#1401's GenesisInit fix plus the probe that skips the two exec-time tx.To address checks, so the from-genesis replay can get past bityuan block 546820 and report what else it hits. Co-authored-by: Claude <[email protected]>
CheckAddress ranged over the driver map and kept the last error it saw, so for an address that several drivers reject the error returned depended on Go's randomized map iteration -- measured on one affected address through system/dapp.CheckAddress: 5x ErrInvalidEthAddr, 1x ErrAddressType, 2x tolerated. Callers decide whether a legacy address format may be tolerated by comparing that error against specific values: system/dapp.CheckAddress tolerates ErrCheckVersion before ForkMultiSignAddress and ErrAddressChecksum before ForkBase58AddressCheck. An arbitrary error therefore defeated those fork gates at random, and a from-genesis replay diverged wherever it landed on the rejecting side. Visit the drivers in ascending id order, the legacy base58 drivers first, and report the first failure. An address that only fails the legacy checks now reports ErrCheckVersion or ErrAddressChecksum, which is what the gates match on. Co-authored-by: Claude <[email protected]>
execCheckTx validated tx.To with the raw address.CheckAddress, which has no fork tolerance, while Exec validated the same address with the dapp wrapper that tolerates the legacy formats below ForkMultiSignAddress / ForkBase58AddressCheck. The two paths disagreed about the same address, and the strict one runs first: a transaction to a legacy address that its block was accepted with is rejected here, its state root no longer matches, and a from-genesis sync stops at that block with ErrCheckStateHash (bityuan: 546820). Check with the same wrapper Exec uses. Below those fork heights the legacy formats are tolerated, as the configuration intends and as the blocks that are already on the chain were executed; at or above them the address is still rejected, now deterministically. Co-authored-by: Claude <[email protected]>
bysomeone
changed the title
Review(审查对象
|
execCheckTx is not on the path a block is replayed through, so this change contributed nothing to the 546820 fix: EventCheckTx has exactly one sender, system/mempool/check.go, and the mempool checks the same address earlier with the raw address.CheckAddress, which is stricter than the wrapper. Anything that reaches execCheckTx had already passed the stricter check, so the wrapper could not change the outcome there. The replay is fixed by visiting the address drivers in a fixed order, which makes Exec's check -- the one that runs during a replay -- report the legacy errors the fork gates tolerate. Keeping the execCheckTx change would be a consensus-path edit with no causal evidence and no test. Co-authored-by: Claude <[email protected]>
The cached answer depends on the height -- isEnable enables every driver when the height is negative -- but the cache was keyed by the address alone, and the callers that pass -1 (rpc/client, rpc/jrpchandler, wallet, the CLI commands) run in the same process as the ones that pass a real height. One of those queries could therefore decide a later check at a real height: the mempool check in system/mempool/check.go passes a real height, would take the cached nil, and accept a transaction the other nodes reject. That transaction can be mined, and the nodes that did not answer such a query reject the block -- the same class of divergence this change set is about, with the trigger moved from the driver map's iteration order to the call order. Include the drivers enabled at the height in the key. ValidateAddr does not take a height, so the address and the enabled set are the whole input, and a set is also what a later address.Init would invalidate. Co-authored-by: Claude <[email protected]>
CheckBase58Address reports ErrCheckChecksum for a 25-byte address whose checksum fails (the older check, kept for compatibility) and ErrAddressChecksum for longer ones, and both are the same legacy form. Before the drivers were visited in a fixed order, such an address could come back as ErrCheckVersion instead -- from a driver that expects another version byte -- which the gate above already tolerates. Now it reports ErrCheckChecksum every time, so leaving the gate as it was would flip these addresses from "sometimes accepted" to "always rejected" below the fork, where the blocks on the chain were executed. Tolerate both errors below ForkBase58AddressCheck. At and above it nothing changes, so this only affects the replay of heights below the fork. Co-authored-by: Claude <[email protected]>
The upper-bound test rejected MaxTokenBalance+1 on an account that already held MaxCoin*coinPrecision, so it passed on safeAdd's sum check rather than on the amount, and the boundary a replay actually depends on was not covered: the token executor's preCreate allows total == MaxTokenBalance, so a fresh account initialized with exactly that has to succeed. Cover the three cases separately -- between the two bounds, exactly MaxTokenBalance on an empty account, and the sum bound -- and cut the comment in GenesisInit down to the part a reader of this repository can check, pointing at the caller in 33cn/plugin by path. Co-authored-by: Claude <[email protected]>
|
感谢 review，逐条回一下。1/2/3/5 都改了，注释与描述那两条也一起处理了，改动分 4 个提交追加在本 PR 上（ 问题 2：
|
The deterministic driver ordering built a fresh id slice, sorted it and composed a string cache key on every CheckAddress call, which took the cache hit path from 40ns/0 allocs to 500ns/264B/7 allocs -- and that path runs for every transaction (executor/execenv.go, system/mempool/check.go). RegisterDriver now maintains driverOrder, the registered drivers with ascending id, rebuilt with a plain scan (ids run 0..MaxID, and there are at most MaxID+1 drivers). CheckAddress walks it directly, so no per-call slice or sort is left. The cache keeps the address as its key and stores the enabled driver set in the entry, as a bitmask over driverOrder, comparing it on the hit path instead of composing it into the key. Reuse is still refused when the enabled set differs, so a check at -1 still cannot decide a later one at a real height; an entry whose set differs is replaced, which costs the callers that alternate between -1 and a real height a recomputation rather than correctness. Measured with a temporary benchmark (not committed), cache hit path: upstream 40ns/0 allocs, before this commit 500ns/7 allocs, after 45ns/0 allocs. Co-authored-by: Claude <[email protected]>
|
自查后又推了一个提交，改动集中在 1. 把确定性排序引入的热路径分配退回去。 前一版每次调用都会新建并排序一份 id 切片、再拼字符串缓存 key，命中路径从 40ns/0 alloc 变成 500ns/264B/7 alloc（每笔交易都走 2. 按你的"排除升级窗口分叉"这条口径重新核过一遍 3. 窗口内的一处行为变更改为披露。 在 重启验证（从创世重放）跑的就是放宽后的规则； |
|
🎉 This PR is included in version 1.72.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
The gate's two fork heights are separate knobs and are equal only on chains that say so -- bityuan sets both to 2270000, the chain33 defaults are 1298600 and 1800000 -- so a test that sets them to the same value cannot see the window between them. Inside [ForkMultiSignAddress, ForkBase58AddressCheck) the version error stops being tolerated while the checksum error still is, so the legacy form at 546820 is rejected there: that is the behaviour change 33cn#1401 disclosed and left in place. The 25-byte checksum failure does not depend on ForkMultiSignAddress and is rejected at every height. The 25-byte assertions fail if ErrCheckChecksum is tolerated again below the fork. Co-authored-by: Claude <[email protected]>
The gate's two fork heights are separate knobs and are equal only on chains that say so -- bityuan sets both to 2270000, the chain33 defaults are 1298600 and 1800000 -- so a test that sets them to the same value cannot see the window between them. Inside [ForkMultiSignAddress, ForkBase58AddressCheck) the version error stops being tolerated while the checksum error still is, so the legacy form at 546820 is rejected there: that is the behaviour change #1401 disclosed and left in place. The 25-byte checksum failure does not depend on ForkMultiSignAddress and is rejected at every height. The 25-byte assertions fail if ErrCheckChecksum is tolerated again below the fork. Co-authored-by: Claude <[email protected]>
3 participants
Fixes #1400 and #1403.
A node that syncs from genesis stops on historical blocks with
ErrCheckStateHash. There are two independent causes, both checks that were added years after those blocks were accepted, and this PR fixes both.1.
GenesisInitamount bound --account/genesis.goGenesisInitnow rejects only a negative amount. #1378 added the fullCheckAmountvalidation, whose upper bound (MaxCoin*coinPrecision, 1e17) is tighter than theMaxTokenBalance(9e18) thatsafeAddalready enforces — and that a token's pre-create validates its total against.GenesisInitis not genesis-only: the token executor calls it from everytokenFinishCreatewith the token's total. On bityuan, 18 tokens havetotal >= 1e17, at heights 394223 / 595494 / 688446 / 1473857. With the tighter bound, a node syncing from genesis stops on each of them withErrCheckStateHash, because the state root it computes no longer matches the one in the block header.The negative case — what #1378 was actually about — is still rejected, and it is the only guard left.
amount == 0is accepted again, as are amounts between the two upper bounds, which is the rule those blocks were executed with: before #1378,GenesisInithad no validation at all beyondsafeAdd. Reverting #1378's lower bound as well would introduce a new rejection, and a new rejection is the failure mode this PR exists to fix — a node replaying a block that calledgenesis_init(addr, 0)would stop on it.GenesisInithas three callers, and the zero amount is reachable from two of them:Coins.Exec_Genesis(system/dapp/coins/executor) runs only atheight == 0and validates the amount at its own entry, so the genesis path is unaffected either way.TestGenesisZeroAmountandTestGenesisNegativeAmountthere still pass.tokenFinishCreate, with the token's total, which is where the upper bound matters.genesis_init, which a deployed contract can call at any height and which has no amount guard of its own. This is the path an earlier version of this description missed: the zero amount is not covered one level up here.This relaxation is not fork-gated, and it applies at every height. So during the upgrade itself, a token created with a total in
[1e17, 9e18]executes on the upgraded nodes and fails withErrAmounton the ones that have not been upgraded yet, which is a state divergence rather than a replay-only difference: the whole network has to be upgraded together. For bityuan that is a coordinated upgrade, not a rolling one.That is also the whole newly-accepted set on bityuan. The
amount == 0case cannot reachGenesisInitthere:Coins.Exec_Genesisonly runs atheight == 0and rejects a zero amount at its own entry, the token executor's pre-create already rejectstotal <= 0(tokendb.go:163), and the js executor — the one caller with no amount guard, reachable at any height — is not linked into bityuan's binary at all (plugin/dapp/init/init.goregisters nojs). So the zero amount is a disclosure, not an exposure, for the chain this is written for.A fork height is not the knob for this one. A gate only applies from some height H upward, and every block the change restores is historical: a gate above those blocks leaves them unreplayable, which is the bug, and a gate below them is indistinguishable from no gate. The strict rule is also the newer one — it is what a fresh sync computes its state root with today, and it is what disagrees with the chain.
Test changes in
account/genesis_checkamount_test.go:amount == 0is now expected to succeed, with the reason documentedTestGenesisInitAmountBoundariesreplaces the previous upper-bound test, which rejectedMaxTokenBalance+1on an account that already heldMaxCoin*coinPrecision— i.e. it passed onsafeAdd's sum check rather than on the amount. It now covers the three boundaries separately, including the one a replay depends on: a fresh account initialized with exactlyMaxTokenBalance, which is what the token pre-create allows.2. The address check --
common/address/address.go,system/dapp/driver.goCheckAddressranged over the driver map and kept the last error it saw, so the error returned for an address that several drivers reject depended on Go's randomized map iteration. Callers decide whether a legacy address format may be tolerated by comparing that error againstErrCheckVersion/ErrAddressChecksum, so the same block could be accepted on one node and rejected on another.Measured over the 57 addresses that differ from a valid one only in the last character (so the length and the version byte still check out), on upstream code:
ErrInvalidEthAddrx35,ErrAddressTypex8,ErrCheckChecksumx7,ErrCheckVersionx7 — four different answers for the same address, and the fork gate treats two of them differently from the other two. Visiting the drivers in ascending id order and returning the first failure makes all 57 answerErrCheckChecksum.That is what fixes bityuan's 546820, a
coinstransfer to a legacy malformed address: the deterministic answer is the error the fork gate tolerates, so the transaction is accepted during a replay exactly as it was when its block was first executed.Two consequences of the same measurement are handled here as well:
ErrCheckChecksumtoo (system/dapp/driver.go).CheckBase58Addressreports it for a 25-byte address andErrAddressChecksumfor longer ones, and both are the same legacy form. Previously such a 25-byte address could come back asErrCheckVersion— from a driver that expects another version byte — which the gate tolerates belowForkMultiSignAddress; now it always reportsErrCheckChecksum, so leaving the gate alone would flip these addresses from "sometimes accepted" to "always rejected" below the fork. Both errors are tolerated belowForkBase58AddressCheckonly, so nothing changes at or above it.common/address/address.go). The answer depends on the height — a negative height enables every driver, including the ones the config disables — but the cache was keyed by the address alone, and the callers that pass -1 (rpc/client,rpc/jrpchandler,wallet, the CLI commands) share the process with the ones that pass a real height. A query could therefore decide a later check at a real height: the mempool check insystem/mempool/check.gopasses a real height, would take the cachednil, and accept a transaction the other nodes reject.ValidateAddrdoes not take a height, so the address and the enabled set are the whole input.One behaviour change is disclosed rather than fixed: inside the window between the two fork heights,
[ForkMultiSignAddress, ForkBase58AddressCheck), the deterministic answer for such an address isErrCheckVersion, and that error stops being tolerated atForkMultiSignAddress. So inside that window an address that only fails the legacy checks flips from "rejected unless the arbitrary visiting order happened to put a checksum-error driver first" to "always rejected". Extending theErrCheckVersiontolerance toForkBase58AddressCheckwould restore the old acceptance, but that is a fork-semantics decision and this PR does not make it. The window is empty on the chains I know of — the chain33 defaults are 1298600/1800000 and bityuan sets both to 2270000 — so bityuan cannot reach it, and its affected block (546820) is far below both.No new fork height is needed.
ForkMultiSignAddress/ForkBase58AddressCheckalready express "tolerate these formats below this height, reject them at or above it". At or above both heights the address is still rejected — now deterministically — and current heights are far above them, so live behaviour there is unchanged. For a chain whose fork heights are still in the future, this makes the check obey the configured tolerance instead of bypassing it; if stricter behaviour is wanted there, the fork height is the knob to move.This PR originally also changed
executor/execenv.goto use the wrapper at theEventCheckTxcall site. That call site is not on the path a block is replayed through:EventCheckTxhas exactly one sender (system/mempool/check.go), and the mempool checks the same address earlier with the raw function, which is stricter — anything that reachesexecCheckTxhas already passed it, so the wrapper cannot change the outcome there. The change is dropped rather than kept as a consensus-path edit with no causal evidence; the fix is the deterministic ordering, which applies to the checkExecruns during a replay.system/mempool/check.goalso calls the raw function. The mempool is not consensus-critical and at current heights the outcome is identical, so it is left unchanged rather than widened in the same PR.Verification
A bityuan mainnet node was rebuilt from genesis with this branch's chain33 (via a temporary
replacein a test branch of our fork — not part of this PR). It replayed every height that used to fail — 394223, 595494, 688446, 1473857 (GenesisInit) and 546820 (address check) — crossed the 2270000 fork boundary, and reached 9,719,794 blocks with zeroErrCheckStateHashand no panic. Block hashes at 2270000 and 6000000 match a healthy node on the same chain byte for byte.The replay was not repeated after the last four commits, and does not need to be: neither the cache key (identical in a process that never calls
CheckAddresswith -1, which a replay is), nor the gate tolerance (below the fork only, and it can only accept more, never reject more), nor the removedexecCheckTxedit (not on that path) can change a replay's outcome. That run also answers the question a replay can answer here — a clean process, with no -1 call to poison the cache, replayed the whole range, so no block in it depends on a poisoned cache entry to be accepted.Tests
Added, each verified to fail before the change it covers and pass after:
common/address:TestCheckAddressErrorIsDeterministic(the 57 variants above must all return the same error),TestCheckAddressCacheFollowsHeight(CheckAddress(ethAddr, -1)must not decideCheckAddress(ethAddr, 0))system/dapp:TestCheckAddressToleratesLegacyFormBelowFork,TestCheckAddressToleratesLegacyChecksumBelowForkaccount:TestGenesisInitAmountBoundariesgo test ./common/address/ ./system/address/... ./system/dapp/ ./system/dapp/coins/executor/ ./account/ ./executor/ -count=1passes, andgo vetis clean on the touched packages.The cache hit path was measured with a temporary benchmark, not committed: 40ns/0 allocs on upstream, 500ns/264B/7 allocs for the first version of the deterministic ordering, 45ns/0 allocs after the last commit, which keeps the driver order in a slice and the enabled driver set in the cache entry.
🤖 Generated with Claude Code