The assumeutxo data for the fuzz target could change and invalidate the hash silently, preventing the fuzz target from reaching some code paths. Fix this by introducing a unit test which would break if the snapshot data the fuzz target relies on were to change.
In implementing this i noticed the height used for coins in the fuzz target is actually off-by-one (as if the first block in the created chain was the genesis but it’s block 1), so fix that too.
The chain starts at block 1, not genesis.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31910.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Ignored review | dergoegge |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/37498360174
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
843+{
844+ const auto snapshot_height{200};
845+
846+ // Mine the same 200 blocks as the `utxo_snapshot` fuzz target.
847+ for (auto& block: CreateBlockChain(snapshot_height, m_node.chainman->GetParams())) {
848+ MineBlock(m_node, block);
I don’t understand this pull request and I don’t think it is possible, but I may be missing something obvious.
My understanding is that the blockchain in the fuzzing binary may always be different, because it will have a weaker pow check, or is there something that ensures the pow check in the fuzzing binary in this fuzz target behaves identical to the one in the unit tests?
Actually valid PoW will pass the reduced fuzz PoW check as well. But you’re right I think the block hashes in the chain produced in the fuzz test and the unit test will be different.
We could move the assertion to the init function in the fuzz test, that should fail right now (i think).
hash[31] & 0x80 == 0 is equivalent to the pow check against regtest min difficulty used in the unit test. If one passes the other necessarily does. This mean both will mine the same chain. You can check this as they both arrive to the same block hash at height 200, with the same utxo set hash.
The fuzz mock
hash[31] & 0x80 == 0is equivalent to the pow check against regtest min difficulty used in the unit test
Just for reference, I don’t think this is true. It simply happens to pass because it is the most likely outcome, given the number of regtest blocks, but that is not guaranteed. Block hashes do exist where this is not true. For example:
0
1{
2 const auto consensus = CreateChainParams(*m_node.args, ChainType::REGTEST)->GetConsensus();
3 unsigned int nBits{0x207fffff};
4 uint256 hash;
5 hash = uint256{"7fffff0000000000000000000000000000000000000000000000000000000000"};
6 Assert(CheckProofOfWork(hash, nBits, consensus));
7 hash = uint256{"7fffff0000000000000000000000000000000000000000000000000000000001"};
8 Assert(CheckProofOfWork(hash, nBits, consensus)); // fails in the unit test, passes in the fuzz test
9}
0 hash = uint256{"7fffff0000000000000000000000000000000000000000000000000000000001"}; 1 Assert(CheckProofOfWork(hash, nBits, consensus)); // fails in the unit test, passes in the fuzz test
I don’t think this actually passes in the fuzz test. The hex representation is in reversed byte order. hash[31] here is 0x7f. If it had its highest bit set (that is hash[31] & 0x80 != 0 then it would be necessarily be higher than the regtest max target.
All values with the highest bit set would be higher than the regtest maximum target (which is 7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff). These are 8000000000000000000000000000000000000000000000000000000000000000 to ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff. Therefore i think my original point stands.
Yes, the high bit is always unset, otherwise the check would fail on both tests.
However that does not mean all remaining bits are always the same in the block hash on regtest and “fuzznet”. “fuzznet” doesn’t care about the remaining bits at all. regtest does check them.
You can copy-paste the c++ code above and check it for yourself locally.
You are right, i missed how the genesis block’s nBits does not correspond to the powLimit of 7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff but instead encodes a rounded down target of 7fffff0000000000000000000000000000000000000000000000000000000000. This means that all values from 7fffff0000000000000000000000000000000000000000000000000000000001 to 7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff would have the highest bit unset but also fail in the unit test. If the nBits didn’t round down the maximum target the two would match exactly.
Thanks for the patience.
The assumeutxo data for the fuzz target could change and invalidate the hash silently, preventing
the fuzz target from reaching some code paths.
Fix this by sanity checking the snapshot values during initialization.
Just pushed an update to have the sanity check in the fuzz target initialization instead. Despite the previous version being technically correct it was so for non-obvious reasons and it makes sense to have the sanity check next to where the value will be used.
You can check it serves its intended purpose with the following diff for instance:
0diff --git a/src/test/util/mining.cpp b/src/test/util/mining.cpp
1index e006a2da629..443254ca503 100644
2--- a/src/test/util/mining.cpp
3+++ b/src/test/util/mining.cpp
4@@ -43,6 +43,7 @@ std::vector<std::shared_ptr<CBlock>> CreateBlockChain(size_t total_height, const
5 coinbase_tx.vout.resize(1);
6 coinbase_tx.vout[0].scriptPubKey = P2WSH_OP_TRUE;
7 coinbase_tx.vout[0].nValue = GetBlockSubsidy(height + 1, params.GetConsensus());
8+ coinbase_tx.nLockTime = height;
9 coinbase_tx.vin[0].scriptSig = CScript() << (height + 1) << OP_0;
10 block.vtx = {MakeTransactionRef(std::move(coinbase_tx))};
Running the fuzz target with this diff would make the invalidation of the snapshot value in chainparams explicit:
0test/fuzz/utxo_snapshot.cpp:65 sanity_check_snapshot: Assertion `stats.hashBlock == cp_au_data.blockhash' failed.
1Aborted
Whereas if you apply the same diff on master the target will happily run and never be able to find a valid snapshot.
Also, did you check the coverage is not decreased?
I did not generate a full coverage report but did check it was still possible for the target to find a valid snapshot and also check the cov: value reported by libfuzzer was the same on master and this branch (4200 without sanitizers).
I did not generate a full coverage report but did check it was still possible for the target to find a valid snapshot and also check the cov: value reported by libfuzzer was the same on master and this branch (4200 without sanitizers).
More strictly, I’d expect that a fuzz seed that generated a valid snapshot before this change would also generate one with the change - the loop iteration that isn’t done anymore doesn’t make use of fuzzed_data_provider. So existing seeds shouldn’t be invalidated.
91@@ -92,6 +92,11 @@ COutPoint MineBlock(const NodeContext& node, std::shared_ptr<CBlock>& block)
92 assert(block->nNonce);
93 }
94
95+ return ConnectBlock(node, block);
96+}
97+
98+COutPoint ConnectBlock(const NodeContext& node, const std::shared_ptr<CBlock>& block)
ProcessNewBlock some other name (e.g. ProcessBlock() like net_processing does) to avoid confusion with the existing ConnectBlock in validation. The added function doesn’t necessarily attempt to connect the block, for example if a conflicting block arrived / was connected in between mining and calling ProcessNewBlock the real ConnectBlock may never be called.