chainparams, test: store pow_limit in internal byte order - #398
Conversation
|
Amended: added a negative control for the zero hash. Every case in the test passed an all-zero hash, which clears the hash-vs-target comparison so the The new arm uses the same chain and the same Worth noting what the test already did, since it is stronger than it looks:
Re-confirmed that the whole test still fails at line 104 against the old byte order, so the new arm has not masked the original guard. 78/78. |
8740ae4 to
a23cc9d
Compare
edtubbs
left a comment
There was a problem hiding this comment.
ACK, great catch and test to prove the check.
check_pow() rejects a target above the chain's pow_limit via
uint256_cmp(target_bytes, params->pow_limit). uint256_cmp() scans from
index 31 down and returns on the first differing byte, so it reads its
operands as internal (little-endian) byte arrays, and target_bytes comes
from arith_uint256.pn[], which is little-endian.
pow_limit was stored in display order. Mainnet held
{0x00, 0x00, 0x0f, 0xff, ... 0xff}
which uint256_cmp() read as 0xffffff..ff0f0000 -- within one byte of the
256-bit maximum. No representable target exceeds that, so the comparison
was false for every input and the bound rejected nothing. Testnet carried
the same literal and regtest the equivalent 0x7f-prefixed one.
This is the only 32-byte field in dogecoin_chainparams stored that way.
genesisblockhash, genesisblockchainwork and minimumchainwork are all in
internal order; reversing genesisblockhash yields the value Core asserts
in chainparams.cpp, and pow_limit was the outlier.
Reverse the three literals. Each now reverses to the value Core sets for
the corresponding chain:
main/test 0x00000fffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
regtest 0x7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
This changes validation behavior: a header whose nBits decode above the
chain's pow_limit is now rejected rather than accepted. No block on any
chain has such a target, and the existing suite is unaffected -- the
genesis chainwork vector and the spv header chains decode well under the
limit and pass unchanged.
Add test_check_pow_limit_bound() to pin it. nbits 0x1e100000 decodes to
0x0000100000..00, one byte above the mainnet limit, and is paired with a
zero hash so it clears the hash-vs-target comparison that follows: the
pow_limit bound is the only condition left that can reject it, and a true
return means the bound is not enforced. The test fails on the parent
commit and passes here. It also asserts the same target is accepted on
regtest, whose limit is higher, and that 0x1e0fffff -- the largest target
that does not exceed the mainnet limit -- is accepted, so the bound is
pinned from both sides rather than as a blanket rejection.
a23cc9d to
0ac16fb
Compare
Found while investigating the log output @edtubbs flagged (see #397). The
diagnostic fix there is what made this visible: once
above_pow_limitcouldactually be reported, it became clear it was never firing.
pow_limitwas the only 32-byte chainparams field stored big-endian.genesis_hash,genesis_chainworkandminimumchainworkare all little-endian(internal) order.
check_powcompares against it in internal order, so thecomparison was against a byte-reversed value and the bound never fired — the
check was dead.
Verified against Core
Each new value reverses to exactly what Core sets in
chainparams.cpp:0x00000fffffffffff…ff0x00000fffffffffff…ff0x7fffffffffffffff…ffThe guard is discriminating
test_check_pow_limit_bound()fails at line 104 on the parent commit andpasses here. It pins from both sides rather than only asserting the happy path:
test would catch a fix that simply rejected everything
0x1e0fffff, the largest non-exceeding mainnet target, is acceptedConsensus-visible, and worth saying plainly
This takes the bound from dead to live. No chain has ever had a target above
powLimit, so nothing real is affected — genesisbits0x1e0ffff0decodesbelow the limit either way, and the passing-test set is identical to baseline.
But it is a validation behaviour change and should be reviewed as one.
Kept off #397 deliberately so the diagnostic fix is not blocked behind this
review. They do not conflict: #397 touches
pow.candvalidation.c, thistouches
chainparams.candblock_tests.c.78/78.
Left alone, worth separate issues
check_powcallsswap_byteson the caller's hash in place and does notswap back on the success path, so the caller's buffer is silently mutated.
Harmless today because every call site passes a local, but a landmine for
anyone reusing a hash across calls.
strerror(errno)pattern inblock.c's deserialize paths — same defect aspow, validation: report the condition that actually failed #397 fixed in
check_auxpow, larger diff.