Merge bitcoin/bitcoin#26152: Bump unconfirmed ancestor transactions to target feerate

f18f9ef Amend bumpfee for inputs with overlapping ancestry (Murch)
2e35e94 Bump unconfirmed parent txs to target feerate (Murch)
3e3e052 coinselection: Move GetSelectionWaste into SelectionResult (Andrew Chow)
c57889d [node] interface to get bump fees (glozow)
c24851b Make MiniMinerMempoolEntry fields private (Murch)
ac6030e Remove unused imports (Murch)
d2f90c3 Fix calculation of ancestor set feerates in test (Murch)
a1f7d98 Match tx names to index in miniminer overlap test (Murch)

Pull request description:

  Includes some commits to address follow-ups from #27021: bitcoin/bitcoin#27021 (comment)

  Reduces the effective value of unconfirmed UTXOs by the fees necessary to bump their ancestor transactions to the same feerate.

  While the individual UTXOs always account for their full ancestry before coin-selection, we can correct potential overestimates with a second pass where we establish the ancestry and bump fee for the whole input set collectively.

  Fixes #9645
  Fixes #9864
  Fixes #15553

ACKs for top commit:
  S3RK:
    ACK f18f9ef
  ismaelsadeeq:
    ACK f18f9ef
  achow101:
    ACK f18f9ef
  brunoerg:
    crACK f18f9ef
  t-bast:
    ACK bitcoin/bitcoin@f18f9ef, I reviewed the latest changes and run e2e tests against eclair, everything looks good 👍

Tree-SHA512: b65180c4243b1f9d13c311ada7a1c9f2f055d530d6c533b78c2068b50b8c29ac1321e89e85675b15515760d4f1b653ebd9da77b37c7be52d9bc565a3538f0aa6

Author: achow101

src/bench/coin_selection.cpp

@@ -79,7 +79,7 @@ static void CoinSelection(benchmark::Bench& bench)
     };
     auto group = wallet::GroupOutputs(wallet, available_coins, coin_selection_params, {{filter_standard}})[filter_standard];
     bench.run([&] {
-        auto result = AttemptSelection(1003 * COIN, group, coin_selection_params, /*allow_mixed_output_types=*/true);
+        auto result = AttemptSelection(wallet.chain(), 1003 * COIN, group, coin_selection_params, /*allow_mixed_output_types=*/true);
         assert(result);
         assert(result->GetSelectedValue() == 1003 * COIN);
         assert(result->GetInputSet().size() == 2);

src/interfaces/chain.h

@@ -216,6 +216,43 @@ class Chain
     //! Calculate mempool ancestor and descendant counts for the given transaction.
     virtual void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants, size_t* ancestorsize = nullptr, CAmount* ancestorfees = nullptr) = 0;
 
+    //! For each outpoint, calculate the fee-bumping cost to spend this outpoint at the specified
+    //  feerate, including bumping its ancestors. For example, if the target feerate is 10sat/vbyte
+    //  and this outpoint refers to a mempool transaction at 3sat/vbyte, the bump fee includes the
+    //  cost to bump the mempool transaction to 10sat/vbyte (i.e. 7 * mempooltx.vsize). If that
+    //  transaction also has, say, an unconfirmed parent with a feerate of 1sat/vbyte, the bump fee
+    //  includes the cost to bump the parent (i.e. 9 * parentmempooltx.vsize).
+    //
+    //  If the outpoint comes from an unconfirmed transaction that is already above the target
+    //  feerate or bumped by its descendant(s) already, it does not need to be bumped. Its bump fee
+    //  is 0. Likewise, if any of the transaction's ancestors are already bumped by a transaction
+    //  in our mempool, they are not included in the transaction's bump fee.
+    //
+    //  Also supported is bump-fee calculation in the case of replacements. If an outpoint
+    //  conflicts with another transaction in the mempool, it is assumed that the goal is to replace
+    //  that transaction. As such, the calculation will exclude the to-be-replaced transaction, but
+    //  will include the fee-bumping cost. If bump fees of descendants of the to-be-replaced
+    //  transaction are requested, the value will be 0. Fee-related RBF rules are not included as
+    //  they are logically distinct.
+    //
+    //  Any outpoints that are otherwise unavailable from the mempool (e.g. UTXOs from confirmed
+    //  transactions or transactions not yet broadcast by the wallet) are given a bump fee of 0.
+    //
+    //  If multiple outpoints come from the same transaction (which would be very rare because
+    //  it means that one transaction has multiple change outputs or paid the same wallet using multiple
+    //  outputs in the same transaction) or have shared ancestry, the bump fees are calculated
+    //  independently, i.e. as if only one of them is spent. This may result in double-fee-bumping. This
+    //  caveat can be rectified per use of the sister-function CalculateCombinedBumpFee(…).
+    virtual std::map<COutPoint, CAmount> CalculateIndividualBumpFees(const std::vector<COutPoint>& outpoints, const CFeeRate& target_feerate) = 0;
+
+    //! Calculate the combined bump fee for an input set per the same strategy
+    //  as in CalculateIndividualBumpFees(…).
+    //  Unlike CalculateIndividualBumpFees(…), this does not return individual
+    //  bump fees per outpoint, but a single bump fee for the shared ancestry.
+    //  The combined bump fee may be used to correct overestimation due to
+    //  shared ancestry by multiple UTXOs after coin selection.
+    virtual std::optional<CAmount> CalculateCombinedBumpFee(const std::vector<COutPoint>& outpoints, const CFeeRate& target_feerate) = 0;
+
     //! Get the node's package limits.
     //! Currently only returns the ancestor and descendant count limits, but could be enhanced to
     //! return more policy settings.

src/node/interfaces.cpp

@@ -28,6 +28,7 @@
 #include <node/coin.h>
 #include <node/context.h>
 #include <node/interface_ui.h>
+#include <node/mini_miner.h>
 #include <node/transaction.h>
 #include <policy/feerate.h>
 #include <policy/fees.h>
@@ -665,6 +666,26 @@ class ChainImpl : public Chain
         if (!m_node.mempool) return;
         m_node.mempool->GetTransactionAncestry(txid, ancestors, descendants, ancestorsize, ancestorfees);
     }
+
+    std::map<COutPoint, CAmount> CalculateIndividualBumpFees(const std::vector<COutPoint>& outpoints, const CFeeRate& target_feerate) override
+    {
+        if (!m_node.mempool) {
+            std::map<COutPoint, CAmount> bump_fees;
+            for (const auto& outpoint : outpoints) {
+                bump_fees.emplace(std::make_pair(outpoint, 0));
+            }
+            return bump_fees;
+        }
+        return MiniMiner(*m_node.mempool, outpoints).CalculateBumpFees(target_feerate);
+    }
+
+    std::optional<CAmount> CalculateCombinedBumpFee(const std::vector<COutPoint>& outpoints, const CFeeRate& target_feerate) override
+    {
+        if (!m_node.mempool) {
+            return 0;
+        }
+        return MiniMiner(*m_node.mempool, outpoints).CalculateTotalBumpFees(target_feerate);
+    }
     void getPackageLimits(unsigned int& limit_ancestor_count, unsigned int& limit_descendant_count) override
     {
         const CTxMemPool::Limits default_limits{};

src/node/mini_miner.cpp

@@ -7,9 +7,7 @@
 #include <consensus/amount.h>
 #include <policy/feerate.h>
 #include <primitives/transaction.h>
-#include <timedata.h>
 #include <util/check.h>
-#include <util/moneystr.h>
 
 #include <algorithm>
 #include <numeric>
@@ -171,9 +169,8 @@ void MiniMiner::DeleteAncestorPackage(const std::set<MockEntryMap::iterator, Ite
         for (auto& descendant : it->second) {
             // If these fail, we must be double-deducting.
             Assume(descendant->second.GetModFeesWithAncestors() >= anc->second.GetModifiedFee());
-            Assume(descendant->second.vsize_with_ancestors >= anc->second.GetTxSize());
-            descendant->second.fee_with_ancestors -= anc->second.GetModifiedFee();
-            descendant->second.vsize_with_ancestors -= anc->second.GetTxSize();
+            Assume(descendant->second.GetSizeWithAncestors() >= anc->second.GetTxSize());
+            descendant->second.UpdateAncestorState(-anc->second.GetTxSize(), -anc->second.GetModifiedFee());
         }
     }
     // Delete these entries.

src/node/mini_miner.h

@@ -19,12 +19,13 @@ class MiniMinerMempoolEntry
     const CAmount fee_individual;
     const CTransactionRef tx;
     const int64_t vsize_individual;
+    CAmount fee_with_ancestors;
+    int64_t vsize_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:
-    CAmount fee_with_ancestors;
-    int64_t vsize_with_ancestors;
     explicit MiniMinerMempoolEntry(CTxMemPool::txiter entry) :
         fee_individual{entry->GetModifiedFee()},
         tx{entry->GetSharedTx()},
@@ -38,6 +39,10 @@ class MiniMinerMempoolEntry
     int64_t GetTxSize() const { return vsize_individual; }
     int64_t GetSizeWithAncestors() const { return vsize_with_ancestors; }
     const CTransaction& GetTx() const LIFETIMEBOUND { return *tx; }
+    void UpdateAncestorState(int64_t vsize_change, CAmount fee_change) {
+        vsize_with_ancestors += vsize_change;
+        fee_with_ancestors += fee_change;
+    }
 };
 
 // Comparator needed for std::set<MockEntryMap::iterator>