Merge bitcoin/bitcoin#28762: MiniMiner changes for package linearization

d9cc99d [test] MiniMiner::Linearize and manual construction (glozow)
dfd6a37 [refactor] unify fee amounts in miniminer_tests (glozow)
f4b1b24 [MiniMiner] track inclusion order and add Linearize() function (glozow)
0040759 [test] add case for MiniMiner working with negative fee txns (glozow)
fe6332c [MiniMiner] make target_feerate optional (glozow)
5a83f55 [MiniMiner] allow manual construction with non-mempool txns (glozow)
e3b2e63 [refactor] change MiniMinerMempoolEntry ctor to take values, update includes (glozow)
4aa98b7 [lint] update expected boost includes (glozow)

Pull request description:

  This is part of #27463. It splits off the `MiniMiner`-specific changes from #26711 for ease of review, as suggested in bitcoin/bitcoin#26711 (comment).

  - Allow using `MiniMiner` on transactions that aren't in the mempool.
  - Make `target_feerate` param of `BuildMockTemplate` optional, meaning "don't stop building the template until all the transactions have been selected."
    - Add clarification for how this is different from `target_feerate=0` (bitcoin/bitcoin#26711 (comment))
  - Track the order in which transactions are included in the template to get the "linearization order" of the transactions.
  - Tests

  Reviewers can take a look at #26711 to see how these functions are used to linearize the `AncestorPackage` there.

ACKs for top commit:
  TheCharlatan:
    ACK d9cc99d
  kevkevinpal:
    reACK [d9cc99d](bitcoin/bitcoin@d9cc99d)
  achow101:
    re-ACK d9cc99d

Tree-SHA512: 32b80064b6679536ac573d674825c5ca0cd6245e49c2fd5eaf260dc535335a57683c74ddd7ce1f249b5b12b2683de4362a7b0f1fc0814c3b3b9f14c682665583

Author: achow101

src/node/mini_miner.cpp

@@ -4,9 +4,14 @@
 
 #include <node/mini_miner.h>
 
+#include <boost/multi_index/detail/hash_index_iterator.hpp>
+#include <boost/operators.hpp>
 #include <consensus/amount.h>
 #include <policy/feerate.h>
 #include <primitives/transaction.h>
+#include <sync.h>
+#include <txmempool.h>
+#include <uint256.h>
 #include <util/check.h>
 
 #include <algorithm>
@@ -72,7 +77,12 @@ MiniMiner::MiniMiner(const CTxMemPool& mempool, const std::vector<COutPoint>& ou
     // Add every entry to m_entries_by_txid and m_entries, except the ones that will be replaced.
     for (const auto& txiter : cluster) {
         if (!m_to_be_replaced.count(txiter->GetTx().GetHash())) {
-            auto [mapiter, success] = m_entries_by_txid.emplace(txiter->GetTx().GetHash(), MiniMinerMempoolEntry(txiter));
+            auto [mapiter, success] = m_entries_by_txid.emplace(txiter->GetTx().GetHash(),
+                MiniMinerMempoolEntry{/*fee_self=*/txiter->GetModifiedFee(),
+                                      /*fee_ancestor=*/txiter->GetModFeesWithAncestors(),
+                                      /*vsize_self=*/txiter->GetTxSize(),
+                                      /*vsize_ancestor=*/txiter->GetSizeWithAncestors(),
+                                      /*tx_in=*/txiter->GetSharedTx()});
             m_entries.push_back(mapiter);
         } else {
             auto outpoints_it = m_requested_outpoints_by_txid.find(txiter->GetTx().GetHash());
@@ -122,6 +132,48 @@ MiniMiner::MiniMiner(const CTxMemPool& mempool, const std::vector<COutPoint>& ou
     SanityCheck();
 }
 
+MiniMiner::MiniMiner(const std::vector<MiniMinerMempoolEntry>& manual_entries,
+                     const std::map<Txid, std::set<Txid>>& descendant_caches)
+{
+    for (const auto& entry : manual_entries) {
+        const auto& txid = entry.GetTx().GetHash();
+        // We need to know the descendant set of every transaction.
+        if (!Assume(descendant_caches.count(txid) > 0)) {
+            m_ready_to_calculate = false;
+            return;
+        }
+        // Just forward these args onto MiniMinerMempoolEntry
+        auto [mapiter, success] = m_entries_by_txid.emplace(txid, entry);
+        // Txids must be unique; this txid shouldn't already be an entry in m_entries_by_txid
+        if (Assume(success)) m_entries.push_back(mapiter);
+    }
+    // Descendant cache is already built, but we need to translate them to m_entries_by_txid iters.
+    for (const auto& [txid, desc_txids] : descendant_caches) {
+        // Descendant cache should include at least the tx itself.
+        if (!Assume(!desc_txids.empty())) {
+            m_ready_to_calculate = false;
+            return;
+        }
+        std::vector<MockEntryMap::iterator> cached_descendants;
+        for (const auto& desc_txid : desc_txids) {
+            auto desc_it{m_entries_by_txid.find(desc_txid)};
+            // Descendants should only include transactions with corresponding entries.
+            if (!Assume(desc_it != m_entries_by_txid.end())) {
+                m_ready_to_calculate = false;
+                return;
+            } else {
+                cached_descendants.emplace_back(desc_it);
+            }
+        }
+        m_descendant_set_by_txid.emplace(txid, cached_descendants);
+    }
+    Assume(m_to_be_replaced.empty());
+    Assume(m_requested_outpoints_by_txid.empty());
+    Assume(m_bump_fees.empty());
+    Assume(m_inclusion_order.empty());
+    SanityCheck();
+}
+
 // Compare by min(ancestor feerate, individual feerate), then iterator
 //
 // Under the ancestor-based mining approach, high-feerate children can pay for parents, but high-feerate
@@ -201,8 +253,10 @@ void MiniMiner::SanityCheck() const
         [&](const auto& txid){return m_entries_by_txid.find(txid) == m_entries_by_txid.end();}));
 }
 
-void MiniMiner::BuildMockTemplate(const CFeeRate& target_feerate)
+void MiniMiner::BuildMockTemplate(std::optional<CFeeRate> target_feerate)
 {
+    const auto num_txns{m_entries_by_txid.size()};
+    uint32_t sequence_num{0};
     while (!m_entries_by_txid.empty()) {
         // Sort again, since transaction removal may change some m_entries' ancestor feerates.
         std::sort(m_entries.begin(), m_entries.end(), AncestorFeerateComparator());
@@ -213,7 +267,8 @@ void MiniMiner::BuildMockTemplate(const CFeeRate& target_feerate)
         const auto ancestor_package_size = (*best_iter)->second.GetSizeWithAncestors();
         const auto ancestor_package_fee = (*best_iter)->second.GetModFeesWithAncestors();
         // Stop here. Everything that didn't "make it into the block" has bumpfee.
-        if (ancestor_package_fee < target_feerate.GetFee(ancestor_package_size)) {
+        if (target_feerate.has_value() &&
+            ancestor_package_fee < target_feerate->GetFee(ancestor_package_size)) {
             break;
         }
 
@@ -237,14 +292,32 @@ void MiniMiner::BuildMockTemplate(const CFeeRate& target_feerate)
                 to_process.erase(iter);
             }
         }
+        // Track the order in which transactions were selected.
+        for (const auto& ancestor : ancestors) {
+            m_inclusion_order.emplace(Txid::FromUint256(ancestor->first), sequence_num);
+        }
         DeleteAncestorPackage(ancestors);
         SanityCheck();
+        ++sequence_num;
+    }
+    if (!target_feerate.has_value()) {
+        Assume(m_in_block.size() == num_txns);
+    } else {
+        Assume(m_in_block.empty() || m_total_fees >= target_feerate->GetFee(m_total_vsize));
     }
-    Assume(m_in_block.empty() || m_total_fees >= target_feerate.GetFee(m_total_vsize));
+    Assume(m_in_block.empty() || sequence_num > 0);
+    Assume(m_in_block.size() == m_inclusion_order.size());
     // Do not try to continue building the block template with a different feerate.
     m_ready_to_calculate = false;
 }
 
+
+std::map<Txid, uint32_t> MiniMiner::Linearize()
+{
+    BuildMockTemplate(std::nullopt);
+    return m_inclusion_order;
+}
+
 std::map<COutPoint, CAmount> MiniMiner::CalculateBumpFees(const CFeeRate& target_feerate)
 {
     if (!m_ready_to_calculate) return {};

src/node/mini_miner.h

@@ -5,33 +5,45 @@
 #ifndef BITCOIN_NODE_MINI_MINER_H
 #define BITCOIN_NODE_MINI_MINER_H
 
-#include <txmempool.h>
+#include <consensus/amount.h>
+#include <primitives/transaction.h>
+#include <uint256.h>
 
+#include <map>
 #include <memory>
 #include <optional>
+#include <set>
 #include <stdint.h>
+#include <vector>
+
+class CFeeRate;
+class CTxMemPool;
 
 namespace node {
 
 // Container for tracking updates to ancestor feerate as we include ancestors in the "block"
 class MiniMinerMempoolEntry
 {
-    const CAmount fee_individual;
     const CTransactionRef tx;
     const int64_t vsize_individual;
-    CAmount fee_with_ancestors;
     int64_t vsize_with_ancestors;
+    const CAmount fee_individual;
+    CAmount fee_with_ancestors;
 
 // This class must be constructed while holding mempool.cs. After construction, the object's
 // methods can be called without holding that lock.
 
 public:
-    explicit MiniMinerMempoolEntry(CTxMemPool::txiter entry) :
-        fee_individual{entry->GetModifiedFee()},
-        tx{entry->GetSharedTx()},
-        vsize_individual(entry->GetTxSize()),
-        fee_with_ancestors{entry->GetModFeesWithAncestors()},
-        vsize_with_ancestors(entry->GetSizeWithAncestors())
+    explicit MiniMinerMempoolEntry(CAmount fee_self,
+                                   CAmount fee_ancestor,
+                                   int64_t vsize_self,
+                                   int64_t vsize_ancestor,
+                                   const CTransactionRef& tx_in):
+        tx{tx_in},
+        vsize_individual{vsize_self},
+        vsize_with_ancestors{vsize_ancestor},
+        fee_individual{fee_self},
+        fee_with_ancestors{fee_ancestor}
     { }
 
     CAmount GetModifiedFee() const { return fee_individual; }
@@ -55,8 +67,14 @@ struct IteratorComparator
     }
 };
 
-/** A minimal version of BlockAssembler. Allows us to run the mining algorithm on a subset of
- * mempool transactions, ignoring consensus rules, to calculate mining scores. */
+/** A minimal version of BlockAssembler, using the same ancestor set scoring algorithm. Allows us to
+ * run this algorithm on a limited set of transactions (e.g. subset of mempool or transactions that
+ * are not yet in mempool) instead of the entire mempool, ignoring consensus rules.
+ * Callers may use this to:
+ * - Calculate the "bump fee" needed to spend an unconfirmed UTXO at a given feerate
+ * - "Linearize" a list of transactions to see the order in which they would be selected for
+ *   inclusion in a block
+ */
 class MiniMiner
 {
     // When true, a caller may use CalculateBumpFees(). Becomes false if we failed to retrieve
@@ -72,7 +90,11 @@ class MiniMiner
     // the same tx will have the same bumpfee. Excludes non-mempool transactions.
     std::map<uint256, std::vector<COutPoint>> m_requested_outpoints_by_txid;
 
-    // What we're trying to calculate.
+    // Txid to a number representing the order in which this transaction was included (smaller
+    // number = included earlier).  Transactions included in an ancestor set together have the same
+    // sequence number.
+    std::map<Txid, uint32_t> m_inclusion_order;
+    // What we're trying to calculate. Outpoint to the fee needed to bring the transaction to the target feerate.
     std::map<COutPoint, CAmount> m_bump_fees;
 
     // The constructed block template
@@ -102,14 +124,32 @@ class MiniMiner
     /** Returns true if CalculateBumpFees may be called, false if not. */
     bool IsReadyToCalculate() const { return m_ready_to_calculate; }
 
-    /** Build a block template until the target feerate is hit. */
-    void BuildMockTemplate(const CFeeRate& target_feerate);
+    /** Build a block template until the target feerate is hit. If target_feerate is not given,
+     * builds a block template until all transactions have been selected. */
+    void BuildMockTemplate(std::optional<CFeeRate> target_feerate);
 
     /** Returns set of txids in the block template if one has been constructed. */
     std::set<uint256> GetMockTemplateTxids() const { return m_in_block; }
 
+    /** Constructor that takes a list of outpoints that may or may not belong to transactions in the
+     * mempool. Copies out information about the relevant transactions in the mempool into
+     * MiniMinerMempoolEntrys.
+    */
     MiniMiner(const CTxMemPool& mempool, const std::vector<COutPoint>& outpoints);
 
+    /** Constructor in which the MiniMinerMempoolEntry entries have been constructed manually,
+     * presumably because these transactions are not in the mempool (yet). It is assumed that all
+     * entries are unique and their values are correct, otherwise results computed by MiniMiner may
+     * be incorrect. Callers should check IsReadyToCalculate() after construction.
+     * @param[in] descendant_caches A map from each transaction to the set of txids of this
+     *                              transaction's descendant set, including itself. Each tx in
+     *                              manual_entries must have a corresponding entry in this map, and
+     *                              all of the txids in a descendant set must correspond to a tx in
+     *                              manual_entries.
+     */
+    MiniMiner(const std::vector<MiniMinerMempoolEntry>& manual_entries,
+              const std::map<Txid, std::set<Txid>>& descendant_caches);
+
     /** Construct a new block template and, for each outpoint corresponding to a transaction that
      * did not make it into the block, calculate the cost of bumping those transactions (and their
      * ancestors) to the minimum feerate. Returns a map from outpoint to bump fee, or an empty map
@@ -120,6 +160,12 @@ class MiniMiner
      * not make it into the block to the target feerate. Returns the total bump fee, or std::nullopt
      * if it cannot be calculated. */
     std::optional<CAmount> CalculateTotalBumpFees(const CFeeRate& target_feerate);
+
+    /** Construct a new block template with all of the transactions and calculate the order in which
+     * they are selected. Returns the sequence number (lower = selected earlier) with which each
+     * transaction was selected, indexed by txid, or an empty map if it cannot be calculated.
+     */
+    std::map<Txid, uint32_t> Linearize();
 };
 } // namespace node