fuzz: mini_miner_selection: ASSERT: mock_template_txids.size() <= blocktemplate->block.vtx.size()

[maflcko]

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=69957

bitcoin/src/test/fuzz/mini_miner.cpp

Line 192 in 2f6dca4

assert(mock_template_txids.size() <= blocktemplate->block.vtx.size());

[maflcko]

$ echo 'AyABAAAAACAg/wAAAAAA/yD/AQAAAAAg/yCu/w==' | base64 --decode > /tmp/oss-fuzz-crash 
$ FUZZ=mini_miner_selection ./src/test/fuzz/fuzz /tmp/oss-fuzz-crash

[dergoegge]

cc @glozow @murchandamus (in case you missed it) would be nice to have this fixed and also have an explanation of the actual bug, since it is somewhat of a mystery why it's so hard to find

[glozow]

Ooh fun, this found a difference between MiniMiner and CTxMemPool's comparators , which boils down to a lack of precision in CFeeRate.

Debugging, the 3 entries in mempool are

A <- B <- C
A: 1sat/264vB
B: 0sat/178vB
C:1sat/178vB

The minimum feerate, i.e. target_feerate or blockMinFeeRate is 1sat/vB.

BlockAssembler selects A. MiniMiner selects ABC. They are both wrong to do so, since all transactions have a feerate below 1sat/vB. This is because CFeeRate rounds minfeerate.GetFee(packagesize) up to 1sat, and the if condition succeeds. But this isn't the bug that was hit by the fuzzer.

The actual bug is in the comparators:
CompareTxMemPoolentryByAncestorFee in CTxMemPool casts everything to doubles and doesn't compare using CFeeRate objects. It can see that A's feerate is 1/264=0.0038sat/vB, while C's ancestor feerate is 2/260=0.0032sat/vB, and orders A before C.

CFeeRate only stores satoshis per KvB, so CFeerate(1, 264) and CFeeRate(2, 260) both become CFeerate(nSatoshisPerK=3), i.e. 0.003sat/vB.

The AncestorFeerateComparator in MiniMiner compares CFeeRates and basically sees that 3 == 3, tiebreaks using txid, and selects package ABC first. You can observe that the fuzz input in this issue succeeds if you multiply all fees by 10 before passing them to the CFeeRate constructors in AncestorFeerateComparator (then it's comparing 32 and 38).

[glozow]

I can open a PR to change MiniMiner to store/compare FeeFracs, which by itself would fix the imprecision bug, though I'm not sure if that'll make it identical to txmempool.

Changing txmempool seems less important imo since there is no current imprecision bug, and perhaps cluster mempool plans to do this anyway.