Merge bitcoin/bitcoin#30412: MiniMiner: use FeeFrac in AncestorFeerateComparator

0937052 fuzz: mini_miner_selection fixups. (glozow)
de273d5 MiniMiner: use FeeFrac in AncestorFeerateComparator (glozow)

Pull request description:

  Closes #30284. Closes #30367, see bitcoin/bitcoin#30367 (comment)

  Previously, we were only comparing feerates up to 1/1000 precision, since CFeeRate comparison just looks at their respective nSatoshisPerK. This could lead to MiniMiner selecting packages in the wrong order (i.e. by txid) if their feerates were less than 0.001sat/vB different. Fix this by creating + comparing `FeeFrac`s instead.

  Also, `FeeFrac::Mul` doesn't have the overflow problem.

  Also added a few minor fuzzer fixups that caught my eye while I was debugging this.

ACKs for top commit:
  ismaelsadeeq:
    Tested ACK 0937052
  murchandamus:
    ACK 0937052 with nits
  dergoegge:
    tACK 0937052

Tree-SHA512: e5b6d6c3f7289f30cd8280d0a47cd852d0180b83d1b27ff9514f50c97103b0f069484e48cba2ca3a57419beadc1996c1b9dd8d0a0f34bc4f4223d8adaf414ce5

Author: fanquake

src/node/mini_miner.cpp

@@ -174,7 +174,7 @@ MiniMiner::MiniMiner(const std::vector<MiniMinerMempoolEntry>& manual_entries,
     SanityCheck();
 }
 
-// Compare by min(ancestor feerate, individual feerate), then iterator
+// Compare by min(ancestor feerate, individual feerate), then txid
 //
 // Under the ancestor-based mining approach, high-feerate children can pay for parents, but high-feerate
 // parents do not incentive inclusion of their children. Therefore the mining algorithm only considers
@@ -183,21 +183,13 @@ struct AncestorFeerateComparator
 {
     template<typename I>
     bool operator()(const I& a, const I& b) const {
-        auto min_feerate = [](const MiniMinerMempoolEntry& e) -> CFeeRate {
-            const CAmount ancestor_fee{e.GetModFeesWithAncestors()};
-            const int64_t ancestor_size{e.GetSizeWithAncestors()};
-            const CAmount tx_fee{e.GetModifiedFee()};
-            const int64_t tx_size{e.GetTxSize()};
-            // Comparing ancestor feerate with individual feerate:
-            //     ancestor_fee / ancestor_size <= tx_fee / tx_size
-            // Avoid division and possible loss of precision by
-            // multiplying both sides by the sizes:
-            return ancestor_fee * tx_size < tx_fee * ancestor_size ?
-                       CFeeRate(ancestor_fee, ancestor_size) :
-                       CFeeRate(tx_fee, tx_size);
+        auto min_feerate = [](const MiniMinerMempoolEntry& e) -> FeeFrac {
+            FeeFrac self_feerate(e.GetModifiedFee(), e.GetTxSize());
+            FeeFrac ancestor_feerate(e.GetModFeesWithAncestors(), e.GetSizeWithAncestors());
+            return std::min(ancestor_feerate, self_feerate);
         };
-        CFeeRate a_feerate{min_feerate(a->second)};
-        CFeeRate b_feerate{min_feerate(b->second)};
+        FeeFrac a_feerate{min_feerate(a->second)};
+        FeeFrac b_feerate{min_feerate(b->second)};
         if (a_feerate != b_feerate) {
             return a_feerate > b_feerate;
         }

src/test/fuzz/mini_miner.cpp

@@ -188,9 +188,9 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
     auto mock_template_txids = mini_miner.GetMockTemplateTxids();
     // MiniMiner doesn't add a coinbase tx.
     assert(mock_template_txids.count(blocktemplate->block.vtx[0]->GetHash()) == 0);
-    mock_template_txids.emplace(blocktemplate->block.vtx[0]->GetHash());
-    assert(mock_template_txids.size() <= blocktemplate->block.vtx.size());
-    assert(mock_template_txids.size() >= blocktemplate->block.vtx.size());
+    auto [iter, new_entry] = mock_template_txids.emplace(blocktemplate->block.vtx[0]->GetHash());
+    assert(new_entry);
+
     assert(mock_template_txids.size() == blocktemplate->block.vtx.size());
     for (const auto& tx : blocktemplate->block.vtx) {
         assert(mock_template_txids.count(tx->GetHash()));