Merge bitcoin/bitcoin#29306: policy: enable sibling eviction for v3 transactions

1342a31 [functional test] sibling eviction (glozow)
5fbab37 [unit test] sibling not returned from SingleV3Checks if 1p2c or 3gen (glozow)
1703067 [policy] sibling eviction for v3 transactions (glozow)
b5d15f7 [refactor] return pair from SingleV3Checks (glozow)

Pull request description:

  When we receive a v3 transaction that would bust a mempool transaction's descendant limit, instead of rejecting the new tx, consider replacing the other descendant if it is much higher feerate (using existing RBF criteria to assess that it's more incentive compatible and to avoid DoS).

  Delving post with more background and motivation: https://delvingbitcoin.org/t/sibling-eviction-for-v3-transactions/472

ACKs for top commit:
  sdaftuar:
    ACK 1342a31
  achow101:
    ACK 1342a31
  instagibbs:
    ACK bitcoin/bitcoin@1342a31

Tree-SHA512: dd957d49e51db78758f566c49bddc579b72478e371275c592d3d5ba097d20de47a6c81952045021b99d82a787f5b799baf16dd0ee0e6de90ba12e21e275352be

Author: achow101

src/policy/v3_policy.cpp

@@ -130,10 +130,11 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v
             }
 
             // It shouldn't be possible to have any mempool siblings at this point. SingleV3Checks
-            // catches mempool siblings. Also, if the package consists of connected transactions,
+            // catches mempool siblings and sibling eviction is not extended to packages. Also, if the package consists of connected transactions,
             // any tx having a mempool ancestor would mean the package exceeds ancestor limits.
             if (!Assume(!parent_info.m_has_mempool_descendant)) {
-                return strprintf("tx %u would exceed descendant count limit", parent_info.m_wtxid.ToString());
+                return strprintf("tx %s (wtxid=%s) would exceed descendant count limit",
+                                parent_info.m_txid.ToString(), parent_info.m_wtxid.ToString());
             }
         }
     } else {
@@ -158,21 +159,23 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v
     return std::nullopt;
 }
 
-std::optional<std::string> SingleV3Checks(const CTransactionRef& ptx,
+std::optional<std::pair<std::string, CTransactionRef>> SingleV3Checks(const CTransactionRef& ptx,
                                           const CTxMemPool::setEntries& mempool_ancestors,
                                           const std::set<Txid>& direct_conflicts,
                                           int64_t vsize)
 {
     // Check v3 and non-v3 inheritance.
     for (const auto& entry : mempool_ancestors) {
         if (ptx->nVersion != 3 && entry->GetTx().nVersion == 3) {
-            return strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 tx %s (wtxid=%s)",
+            return std::make_pair(strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 tx %s (wtxid=%s)",
                              ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(),
-                             entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString());
+                             entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString()),
+                nullptr);
         } else if (ptx->nVersion == 3 && entry->GetTx().nVersion != 3) {
-            return strprintf("v3 tx %s (wtxid=%s) cannot spend from non-v3 tx %s (wtxid=%s)",
+            return std::make_pair(strprintf("v3 tx %s (wtxid=%s) cannot spend from non-v3 tx %s (wtxid=%s)",
                              ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(),
-                             entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString());
+                             entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString()),
+                nullptr);
         }
     }
 
@@ -185,16 +188,18 @@ std::optional<std::string> SingleV3Checks(const CTransactionRef& ptx,
 
     // Check that V3_ANCESTOR_LIMIT would not be violated.
     if (mempool_ancestors.size() + 1 > V3_ANCESTOR_LIMIT) {
-        return strprintf("tx %s (wtxid=%s) would have too many ancestors",
-                         ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString());
+        return std::make_pair(strprintf("tx %s (wtxid=%s) would have too many ancestors",
+                         ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()),
+            nullptr);
     }
 
     // Remaining checks only pertain to transactions with unconfirmed ancestors.
     if (mempool_ancestors.size() > 0) {
         // If this transaction spends V3 parents, it cannot be too large.
         if (vsize > V3_CHILD_MAX_VSIZE) {
-            return strprintf("v3 child tx %s (wtxid=%s) is too big: %u > %u virtual bytes",
-                             ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, V3_CHILD_MAX_VSIZE);
+            return std::make_pair(strprintf("v3 child tx %s (wtxid=%s) is too big: %u > %u virtual bytes",
+                             ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, V3_CHILD_MAX_VSIZE),
+                nullptr);
         }
 
         // Check the descendant counts of in-mempool ancestors.
@@ -210,9 +215,20 @@ std::optional<std::string> SingleV3Checks(const CTransactionRef& ptx,
             std::any_of(children.cbegin(), children.cend(),
                 [&direct_conflicts](const CTxMemPoolEntry& child){return direct_conflicts.count(child.GetTx().GetHash()) > 0;});
         if (parent_entry->GetCountWithDescendants() + 1 > V3_DESCENDANT_LIMIT && !child_will_be_replaced) {
-            return strprintf("tx %u (wtxid=%s) would exceed descendant count limit",
-                             parent_entry->GetSharedTx()->GetHash().ToString(),
-                             parent_entry->GetSharedTx()->GetWitnessHash().ToString());
+            // Allow sibling eviction for v3 transaction: if another child already exists, even if
+            // we don't conflict inputs with it, consider evicting it under RBF rules. We rely on v3 rules
+            // only permitting 1 descendant, as otherwise we would need to have logic for deciding
+            // which descendant to evict. Skip if this isn't true, e.g. if the transaction has
+            // multiple children or the sibling also has descendants due to a reorg.
+            const bool consider_sibling_eviction{parent_entry->GetCountWithDescendants() == 2 &&
+                children.begin()->get().GetCountWithAncestors() == 2};
+
+            // Return the sibling if its eviction can be considered. Provide the "descendant count
+            // limit" string either way, as the caller may decide not to do sibling eviction.
+            return std::make_pair(strprintf("tx %u (wtxid=%s) would exceed descendant count limit",
+                                            parent_entry->GetSharedTx()->GetHash().ToString(),
+                                            parent_entry->GetSharedTx()->GetWitnessHash().ToString()),
+                                  consider_sibling_eviction ?  children.begin()->get().GetSharedTx() : nullptr);
         }
     }
     return std::nullopt;

src/policy/v3_policy.h

@@ -48,9 +48,15 @@ static_assert(V3_CHILD_MAX_VSIZE + MAX_STANDARD_TX_WEIGHT / WITNESS_SCALE_FACTOR
  *                                      count of in-mempool ancestors.
  * @param[in]   vsize                   The sigop-adjusted virtual size of ptx.
  *
- * @returns debug string if an error occurs, std::nullopt otherwise.
+ * @returns 3 possibilities:
+ * - std::nullopt if all v3 checks were applied successfully
+ * - debug string + pointer to a mempool sibling if this transaction would be the second child in a
+ *   1-parent-1-child cluster; the caller may consider evicting the specified sibling or return an
+ *   error with the debug string.
+ * - debug string + nullptr if this transaction violates some v3 rule and sibling eviction is not
+ *   applicable.
  */
-std::optional<std::string> SingleV3Checks(const CTransactionRef& ptx,
+std::optional<std::pair<std::string, CTransactionRef>> SingleV3Checks(const CTransactionRef& ptx,
                                           const CTxMemPool::setEntries& mempool_ancestors,
                                           const std::set<Txid>& direct_conflicts,
                                           int64_t vsize);