Merge bitcoin/bitcoin#29242: Mempool util: Add RBF diagram checks for single chunks against clusters of size 2

7295986 Unit tests for CalculateFeerateDiagramsForRBF (Greg Sanders)
b767e6b test: unit test for ImprovesFeerateDiagram (Greg Sanders)
7e89b65 Add fuzz test for FeeFrac (Greg Sanders)
4d6528a fuzz: fuzz diagram creation and comparison (Greg Sanders)
e9c5aeb test: Add tests for CompareFeerateDiagram and CheckConflictTopology (Greg Sanders)
588a98d fuzz: Add fuzz target for ImprovesFeerateDiagram (Greg Sanders)
2079b80 Implement ImprovesFeerateDiagram (Greg Sanders)
66d966d Add FeeFrac unit tests (Greg Sanders)
ce8e225 Add FeeFrac utils (Greg Sanders)

Pull request description:

  This is a smaller piece of bitcoin/bitcoin#28984 broken off for easier review.

  Up to date explanation of diagram checks are here: https://delvingbitcoin.org/t/mempool-incentive-compatibility/553

  This infrastructure has two near term applications prior to cluster mempool:
  1) Limited Package RBF(bitcoin/bitcoin#28984): We want to allow package RBF only when we know it improves the mempool. This narrowly scoped functionality allows use with v3-like topologies, and will be expanded at some point post-cluster mempool when diagram checks can be done efficiently against bounded cluster sizes.
  2) Replacement for single tx RBF(in a cluster size of up to two) against conflicts of up to cluster size two. `ImprovesFeerateDiagram` interface will have to change for this use-case, which is a future direction to solve certain pins and improve mempool incentive compatibility: https://delvingbitcoin.org/t/ephemeral-anchors-and-mev/383#diagram-checks-fix-this-3

  And longer-term, this would be the proposed way we would compute incentive compatibility for all conflicts, post-cluster mempool.

ACKs for top commit:
  sipa:
    utACK 7295986
  glozow:
    code review ACK 7295986
  murchandamus:
    utACK 7295986
  ismaelsadeeq:
    Re-ACK bitcoin/bitcoin@7295986
  willcl-ark:
    crACK 7295986
  sdaftuar:
    ACK 7295986

Tree-SHA512: 79593e5a087801c06f06cc8b73aa3e7b96ab938d3b90f5d229c4e4bfca887a77b447605c49aa5eb7ddcead85706c534ac5eb6146ae2396af678f4beaaa5bea8e

Author: glozow

src/Makefile.am

@@ -302,6 +302,7 @@ BITCOIN_CORE_H = \
   util/error.h \
   util/exception.h \
   util/fastrange.h \
+  util/feefrac.h \
   util/fees.h \
   util/fs.h \
   util/fs_helpers.h \
@@ -741,6 +742,7 @@ libbitcoin_util_a_SOURCES = \
   util/check.cpp \
   util/error.cpp \
   util/exception.cpp \
+  util/feefrac.cpp \
   util/fees.cpp \
   util/fs.cpp \
   util/fs_helpers.cpp \
@@ -983,6 +985,7 @@ libbitcoinkernel_la_SOURCES = \
   util/batchpriority.cpp \
   util/chaintype.cpp \
   util/check.cpp \
+  util/feefrac.cpp \
   util/fs.cpp \
   util/fs_helpers.cpp \
   util/hasher.cpp \

src/Makefile.test.include

@@ -93,6 +93,7 @@ BITCOIN_TESTS =\
   test/denialofservice_tests.cpp \
   test/descriptor_tests.cpp \
   test/disconnected_transactions.cpp \
+  test/feefrac_tests.cpp \
   test/flatfile_tests.cpp \
   test/fs_tests.cpp \
   test/getarg_tests.cpp \
@@ -313,7 +314,9 @@ test_fuzz_fuzz_SOURCES = \
  test/fuzz/descriptor_parse.cpp \
  test/fuzz/deserialize.cpp \
  test/fuzz/eval_script.cpp \
+ test/fuzz/feefrac.cpp \
  test/fuzz/fee_rate.cpp \
+ test/fuzz/feeratediagram.cpp \
  test/fuzz/fees.cpp \
  test/fuzz/flatfile.cpp \
  test/fuzz/float.cpp \

src/policy/rbf.cpp

@@ -19,6 +19,8 @@
 #include <limits>
 #include <vector>
 
+#include <compare>
+
 RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
 {
     AssertLockHeld(pool.cs);
@@ -181,3 +183,24 @@ std::optional<std::string> PaysForRBF(CAmount original_fees,
     }
     return std::nullopt;
 }
+
+std::optional<std::pair<DiagramCheckError, std::string>> ImprovesFeerateDiagram(CTxMemPool& pool,
+                                                const CTxMemPool::setEntries& direct_conflicts,
+                                                const CTxMemPool::setEntries& all_conflicts,
+                                                CAmount replacement_fees,
+                                                int64_t replacement_vsize)
+{
+    // Require that the replacement strictly improve the mempool's feerate diagram.
+    std::vector<FeeFrac> old_diagram, new_diagram;
+
+    const auto diagram_results{pool.CalculateFeerateDiagramsForRBF(replacement_fees, replacement_vsize, direct_conflicts, all_conflicts)};
+
+    if (!diagram_results.has_value()) {
+        return std::make_pair(DiagramCheckError::UNCALCULABLE, util::ErrorString(diagram_results).original);
+    }
+
+    if (!std::is_gt(CompareFeerateDiagram(diagram_results.value().second, diagram_results.value().first))) {
+        return std::make_pair(DiagramCheckError::FAILURE, "insufficient feerate: does not improve feerate diagram");
+    }
+    return std::nullopt;
+}

src/policy/rbf.h

@@ -9,7 +9,9 @@
 #include <primitives/transaction.h>
 #include <threadsafety.h>
 #include <txmempool.h>
+#include <util/feefrac.h>
 
+#include <compare>
 #include <cstddef>
 #include <cstdint>
 #include <optional>
@@ -33,6 +35,13 @@ enum class RBFTransactionState {
     FINAL,
 };
 
+enum class DiagramCheckError {
+    /** Unable to calculate due to topology or other reason */
+    UNCALCULABLE,
+    /** New diagram wasn't strictly superior  */
+    FAILURE,
+};
+
 /**
  * Determine whether an unconfirmed transaction is signaling opt-in to RBF
  * according to BIP 125
@@ -106,4 +115,21 @@ std::optional<std::string> PaysForRBF(CAmount original_fees,
                                       CFeeRate relay_fee,
                                       const uint256& txid);
 
+/**
+ * The replacement transaction must improve the feerate diagram of the mempool.
+ * @param[in]   pool                The mempool.
+ * @param[in]   direct_conflicts    Set of in-mempool txids corresponding to the direct conflicts i.e.
+ *                                  input double-spends with the proposed transaction
+ * @param[in]   all_conflicts       Set of mempool entries corresponding to all transactions to be evicted
+ * @param[in]   replacement_fees    Fees of proposed replacement package
+ * @param[in]   replacement_vsize   Size of proposed replacement package
+ * @returns error type and string if mempool diagram doesn't improve, otherwise std::nullopt.
+ */
+std::optional<std::pair<DiagramCheckError, std::string>> ImprovesFeerateDiagram(CTxMemPool& pool,
+                                                const CTxMemPool::setEntries& direct_conflicts,
+                                                const CTxMemPool::setEntries& all_conflicts,
+                                                CAmount replacement_fees,
+                                                int64_t replacement_vsize)
+                                                EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
+
 #endif // BITCOIN_POLICY_RBF_H

src/test/feefrac_tests.cpp

@@ -0,0 +1,124 @@
+// Copyright (c) 2024-present The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <util/feefrac.h>
+#include <random.h>
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_AUTO_TEST_SUITE(feefrac_tests)
+
+BOOST_AUTO_TEST_CASE(feefrac_operators)
+{
+    FeeFrac p1{1000, 100}, p2{500, 300};
+    FeeFrac sum{1500, 400};
+    FeeFrac diff{500, -200};
+    FeeFrac empty{0, 0};
+    FeeFrac zero_fee{0, 1}; // zero-fee allowed
+
+    BOOST_CHECK(empty == FeeFrac{}); // same as no-args
+
+    BOOST_CHECK(p1 == p1);
+    BOOST_CHECK(p1 + p2 == sum);
+    BOOST_CHECK(p1 - p2 == diff);
+
+    FeeFrac p3{2000, 200};
+    BOOST_CHECK(p1 != p3); // feefracs only equal if both fee and size are same
+    BOOST_CHECK(p2 != p3);
+
+    FeeFrac p4{3000, 300};
+    BOOST_CHECK(p1 == p4-p3);
+    BOOST_CHECK(p1 + p3 == p4);
+
+    // Fee-rate comparison
+    BOOST_CHECK(p1 > p2);
+    BOOST_CHECK(p1 >= p2);
+    BOOST_CHECK(p1 >= p4-p3);
+    BOOST_CHECK(!(p1 >> p3)); // not strictly better
+    BOOST_CHECK(p1 >> p2); // strictly greater feerate
+
+    BOOST_CHECK(p2 < p1);
+    BOOST_CHECK(p2 <= p1);
+    BOOST_CHECK(p1 <= p4-p3);
+    BOOST_CHECK(!(p3 << p1)); // not strictly worse
+    BOOST_CHECK(p2 << p1); // strictly lower feerate
+
+    // "empty" comparisons
+    BOOST_CHECK(!(p1 >> empty)); // << will always result in false
+    BOOST_CHECK(!(p1 << empty));
+    BOOST_CHECK(!(empty >> empty));
+    BOOST_CHECK(!(empty << empty));
+
+    // empty is always bigger than everything else
+    BOOST_CHECK(empty > p1);
+    BOOST_CHECK(empty > p2);
+    BOOST_CHECK(empty > p3);
+    BOOST_CHECK(empty >= p1);
+    BOOST_CHECK(empty >= p2);
+    BOOST_CHECK(empty >= p3);
+
+    // check "max" values for comparison
+    FeeFrac oversized_1{4611686000000, 4000000};
+    FeeFrac oversized_2{184467440000000, 100000};
+
+    BOOST_CHECK(oversized_1 < oversized_2);
+    BOOST_CHECK(oversized_1 <= oversized_2);
+    BOOST_CHECK(oversized_1 << oversized_2);
+    BOOST_CHECK(oversized_1 != oversized_2);
+
+    // Tests paths that use double arithmetic
+    FeeFrac busted{(static_cast<int64_t>(INT32_MAX)) + 1, INT32_MAX};
+    BOOST_CHECK(!(busted < busted));
+
+    FeeFrac max_fee{2100000000000000, INT32_MAX};
+    BOOST_CHECK(!(max_fee < max_fee));
+    BOOST_CHECK(!(max_fee > max_fee));
+    BOOST_CHECK(max_fee <= max_fee);
+    BOOST_CHECK(max_fee >= max_fee);
+
+    FeeFrac max_fee2{1, 1};
+    BOOST_CHECK(max_fee >= max_fee2);
+
+}
+
+BOOST_AUTO_TEST_CASE(build_diagram_test)
+{
+    FeeFrac p1{1000, 100};
+    FeeFrac empty{0, 0};
+    FeeFrac zero_fee{0, 1};
+    FeeFrac oversized_1{4611686000000, 4000000};
+    FeeFrac oversized_2{184467440000000, 100000};
+
+    // Diagram-building will reorder chunks
+    std::vector<FeeFrac> chunks;
+    chunks.push_back(p1);
+    chunks.push_back(zero_fee);
+    chunks.push_back(empty);
+    chunks.push_back(oversized_1);
+    chunks.push_back(oversized_2);
+
+    // Caller in charge of sorting chunks in case chunk size limit
+    // differs from cluster size limit
+    std::sort(chunks.begin(), chunks.end(), [](const FeeFrac& a, const FeeFrac& b) { return a > b; });
+
+    // Chunks are now sorted in reverse order (largest first)
+    BOOST_CHECK(chunks[0] == empty); // empty is considered "highest" fee
+    BOOST_CHECK(chunks[1] == oversized_2);
+    BOOST_CHECK(chunks[2] == oversized_1);
+    BOOST_CHECK(chunks[3] == p1);
+    BOOST_CHECK(chunks[4] == zero_fee);
+
+    std::vector<FeeFrac> generated_diagram{BuildDiagramFromChunks(chunks)};
+    BOOST_CHECK(generated_diagram.size() == 1 + chunks.size());
+
+    // Prepended with an empty, then the chunks summed in order as above
+    BOOST_CHECK(generated_diagram[0] == empty);
+    BOOST_CHECK(generated_diagram[1] == empty);
+    BOOST_CHECK(generated_diagram[2] == oversized_2);
+    BOOST_CHECK(generated_diagram[3] == oversized_2 + oversized_1);
+    BOOST_CHECK(generated_diagram[4] == oversized_2 + oversized_1 + p1);
+    BOOST_CHECK(generated_diagram[5] == oversized_2 + oversized_1 + p1 + zero_fee);
+}
+
+BOOST_AUTO_TEST_SUITE_END()

src/test/fuzz/feefrac.cpp

@@ -0,0 +1,123 @@
+// Copyright (c) 2024 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <util/feefrac.h>
+#include <test/fuzz/FuzzedDataProvider.h>
+#include <test/fuzz/fuzz.h>
+#include <test/fuzz/util.h>
+
+#include <compare>
+#include <cstdint>
+#include <iostream>
+
+namespace {
+
+/** Compute a * b, represented in 4x32 bits, highest limb first. */
+std::array<uint32_t, 4> Mul128(uint64_t a, uint64_t b)
+{
+    std::array<uint32_t, 4> ret{0, 0, 0, 0};
+
+    /** Perform ret += v << (32 * pos), at 128-bit precision. */
+    auto add_fn = [&](uint64_t v, int pos) {
+        uint64_t accum{0};
+        for (int i = 0; i + pos < 4; ++i) {
+            // Add current value at limb pos in ret.
+            accum += ret[3 - pos - i];
+            // Add low or high half of v.
+            if (i == 0) accum += v & 0xffffffff;
+            if (i == 1) accum += v >> 32;
+            // Store lower half of result in limb pos in ret.
+            ret[3 - pos - i] = accum & 0xffffffff;
+            // Leave carry in accum.
+            accum >>= 32;
+        }
+        // Make sure no overflow.
+        assert(accum == 0);
+    };
+
+    // Multiply the 4 individual limbs (schoolbook multiply, with base 2^32).
+    add_fn((a & 0xffffffff) * (b & 0xffffffff), 0);
+    add_fn((a >> 32) * (b & 0xffffffff), 1);
+    add_fn((a & 0xffffffff) * (b >> 32), 1);
+    add_fn((a >> 32) * (b >> 32), 2);
+    return ret;
+}
+
+/* comparison helper for std::array */
+std::strong_ordering compare_arrays(const std::array<uint32_t, 4>& a, const std::array<uint32_t, 4>& b) {
+    for (size_t i = 0; i < a.size(); ++i) {
+        if (a[i] != b[i]) return a[i] <=> b[i];
+    }
+    return std::strong_ordering::equal;
+}
+
+std::strong_ordering MulCompare(int64_t a1, int64_t a2, int64_t b1, int64_t b2)
+{
+    // Compute and compare signs.
+    int sign_a = (a1 == 0 ? 0 : a1 < 0 ? -1 : 1) * (a2 == 0 ? 0 : a2 < 0 ? -1 : 1);
+    int sign_b = (b1 == 0 ? 0 : b1 < 0 ? -1 : 1) * (b2 == 0 ? 0 : b2 < 0 ? -1 : 1);
+    if (sign_a != sign_b) return sign_a <=> sign_b;
+
+    // Compute absolute values.
+    uint64_t abs_a1 = static_cast<uint64_t>(a1), abs_a2 = static_cast<uint64_t>(a2);
+    uint64_t abs_b1 = static_cast<uint64_t>(b1), abs_b2 = static_cast<uint64_t>(b2);
+    // Use (~x + 1) instead of the equivalent (-x) to silence the linter; mod 2^64 behavior is
+    // intentional here.
+    if (a1 < 0) abs_a1 = ~abs_a1 + 1;
+    if (a2 < 0) abs_a2 = ~abs_a2 + 1;
+    if (b1 < 0) abs_b1 = ~abs_b1 + 1;
+    if (b2 < 0) abs_b2 = ~abs_b2 + 1;
+
+    // Compute products of absolute values.
+    auto mul_abs_a = Mul128(abs_a1, abs_a2);
+    auto mul_abs_b = Mul128(abs_b1, abs_b2);
+    if (sign_a < 0) {
+        return compare_arrays(mul_abs_b, mul_abs_a);
+    } else {
+        return compare_arrays(mul_abs_a, mul_abs_b);
+    }
+}
+
+} // namespace
+
+FUZZ_TARGET(feefrac)
+{
+    FuzzedDataProvider provider(buffer.data(), buffer.size());
+
+    int64_t f1 = provider.ConsumeIntegral<int64_t>();
+    int32_t s1 = provider.ConsumeIntegral<int32_t>();
+    if (s1 == 0) f1 = 0;
+    FeeFrac fr1(f1, s1);
+    assert(fr1.IsEmpty() == (s1 == 0));
+
+    int64_t f2 = provider.ConsumeIntegral<int64_t>();
+    int32_t s2 = provider.ConsumeIntegral<int32_t>();
+    if (s2 == 0) f2 = 0;
+    FeeFrac fr2(f2, s2);
+    assert(fr2.IsEmpty() == (s2 == 0));
+
+    // Feerate comparisons
+    auto cmp_feerate = MulCompare(f1, s2, f2, s1);
+    assert(FeeRateCompare(fr1, fr2) == cmp_feerate);
+    assert((fr1 << fr2) == std::is_lt(cmp_feerate));
+    assert((fr1 >> fr2) == std::is_gt(cmp_feerate));
+
+    // Compare with manual invocation of FeeFrac::Mul.
+    auto cmp_mul = FeeFrac::Mul(f1, s2) <=> FeeFrac::Mul(f2, s1);
+    assert(cmp_mul == cmp_feerate);
+
+    // Same, but using FeeFrac::MulFallback.
+    auto cmp_fallback = FeeFrac::MulFallback(f1, s2) <=> FeeFrac::MulFallback(f2, s1);
+    assert(cmp_fallback == cmp_feerate);
+
+    // Total order comparisons
+    auto cmp_total = std::is_eq(cmp_feerate) ? (s2 <=> s1) : cmp_feerate;
+    assert((fr1 <=> fr2) == cmp_total);
+    assert((fr1 < fr2) == std::is_lt(cmp_total));
+    assert((fr1 > fr2) == std::is_gt(cmp_total));
+    assert((fr1 <= fr2) == std::is_lteq(cmp_total));
+    assert((fr1 >= fr2) == std::is_gteq(cmp_total));
+    assert((fr1 == fr2) == std::is_eq(cmp_total));
+    assert((fr1 != fr2) == std::is_neq(cmp_total));
+}