Repository navigation
fix(address): reject the 25-byte checksum error below the base58 fork - #1406
Merged
Merged
Conversation
The gate below ForkBase58AddressCheck tolerated ErrCheckChecksum as well as ErrAddressChecksum, which accepts transactions the chain packed as failures. Block 101641 carries one: a coins transfer to the 25-byte address 1Di16bUjPJnvZ8Hrf4vuQDffzkv9jC5Jp whose checksum does not match, executed as ExecPack with "Address Checksum error". A replay that accepts it runs the transfer -- the coins transfer path no longer validates the recipient's checksum -- so the computed state root stops matching and a node syncing from scratch stalls at 101641. Tolerating ErrCheckChecksum was meant to keep 25-byte addresses from flipping from "sometimes accepted" to "always rejected" once the drivers were visited in a fixed order. That flip cannot happen at the heights below ForkMultiSignAddress where this gate matters: only the btc and utxo drivers are enabled there, and neither reports ErrCheckVersion, so the legacy form was always rejected. From ForkMultiSignAddress up, ErrCheckVersion is what the gate above already tolerates, which is what the on-chain blocks relied on. 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 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]>
33cn
approved these changes
Oct 8, 2026
Owner
|
🎉 This PR is included in version 1.72.5 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
DriverBase.CheckAddresstolerates some address errors below the fork heights so historicalblocks still replay. #1401 added
ErrCheckChecksumto theForkBase58AddressCheckgate:That tolerance costs a replay divergence. bityuan mainnet block 101641 contains a coins transfer
to the 25-byte address
1Di16bUjPJnvZ8Hrf4vuQDffzkv9jC5Jp, whose checksum does not match; thechain packed it as
ExecPackwithAddress Checksum error. A node syncing bityuan from genesison v1.72.3 stalls there: the replay accepts a transfer the chain executed as a failure, so the
state root diverges.
Why that tolerance does not apply here
#1401 added it because a 25-byte address could come back as
ErrCheckVersionfrom a driver thatexpects another version byte — tolerated below
ForkMultiSignAddress— and would otherwise flipfrom "sometimes accepted" to "always rejected" once the driver walk became deterministic. That
requires an enabled driver reporting
ErrCheckVersionfor the address. On bityuan, belowForkMultiSignAddress,btcMultiSignis not enabled:btcMultiSignenable heightForkMultiSignAddressMeasured on block 101641's address, 20,000 driver walks below the fork: two possible answers,
ErrCheckChecksum(37.6%) andErrAddressType(62.4%) —ErrCheckVersionnever appeared. So thetolerance protects no block on this chain; its only effect is to accept the one transaction the
chain rejected.
What this changes for other chains
For a chain that does enable
btcMultiSignbelowForkMultiSignAddress— the chain33 defaultshape, where the window is
[0, 1298600)— the tolerated verdict was never stable to begin with:Go re-randomises the map walk on every call, so a block minted between v1.67.0 and v1.72.2 that
depended on it recorded whichever branch that one draw produced. Measured on that shape, 18.8% of
walks accept and 81.2% reject. This PR moves such chains from "sometimes accepted" to
"deterministically rejected", which is the majority branch; the alternative matches the minority.
No chain is known to have a block that depends on the accepting branch.
Once the walk is deterministic a 25-byte address can only report
ErrCheckChecksum:CheckBase58Addresscannot reportErrAddressChecksumfor one, because both checksumcomputations are byte-equivalent sha256d and the 25-byte branch returns first.
Changes
system/dapp/driver.go— dropErrCheckChecksumfrom theForkBase58AddressCheckgate, andreplace the comment explaining the old tolerance with the reasoning above.
system/dapp/driver_test.go— the test that asserted the 25-byte case was tolerated below thefork now asserts it is rejected, using the address from the on-chain block instead of a
synthetically corrupted one.
Verification
A node running this change synced bityuan mainnet from genesis past the fork heights without
diverging. The
ExecPacktransaction above is the one that previously forced the replay todiverge at that block; with the change it replays as the failure the chain recorded.
Relationship to #1401
This reverts the
system/dapp/driver.gohalf of #1401 (566304371, released in v1.72.3). Therest of that PR — the deterministic driver walk and the address cache key — is what makes a
from-genesis replay work at all, and is untouched here. A chain on v1.72.3 or later needs this
change to replay bityuan from genesis; bityuan itself pins chain33 v1.72.2 today, so the
regression only bites once that dependency moves.