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

7295986778 Unit tests for CalculateFeerateDiagramsForRBF (Greg Sanders)
b767e6bd47 test: unit test for ImprovesFeerateDiagram (Greg Sanders)
7e89b659e1 Add fuzz test for FeeFrac (Greg Sanders)
4d6528a3d6 fuzz: fuzz diagram creation and comparison (Greg Sanders)
e9c5aeb11d test: Add tests for CompareFeerateDiagram and CheckConflictTopology (Greg Sanders)
588a98dccc fuzz: Add fuzz target for ImprovesFeerateDiagram (Greg Sanders)
2079b80854 Implement ImprovesFeerateDiagram (Greg Sanders)
66d966dcfa Add FeeFrac unit tests (Greg Sanders)
ce8e22542e Add FeeFrac utils (Greg Sanders)

Pull request description:

  This is a smaller piece of https://github.com/bitcoin/bitcoin/pull/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(https://github.com/bitcoin/bitcoin/pull/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 7295986778
  glozow:
    code review ACK 7295986778
  murchandamus:
    utACK 7295986778
  ismaelsadeeq:
    Re-ACK 7295986778
  willcl-ark:
    crACK 7295986778
  sdaftuar:
    ACK 7295986778

Tree-SHA512: 79593e5a087801c06f06cc8b73aa3e7b96ab938d3b90f5d229c4e4bfca887a77b447605c49aa5eb7ddcead85706c534ac5eb6146ae2396af678f4beaaa5bea8e

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));
+}

src/test/fuzz/feeratediagram.cpp

@@ -0,0 +1,119 @@
+// Copyright (c) 2023 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 <stdint.h>
+
+#include <vector>
+
+#include <util/feefrac.h>
+#include <policy/rbf.h>
+
+#include <test/fuzz/fuzz.h>
+#include <test/fuzz/util.h>
+
+#include <assert.h>
+
+namespace {
+
+/** Evaluate a diagram at a specific size, returning the fee as a fraction.
+ *
+ * Fees in diagram cannot exceed 2^32, as the returned evaluation could overflow
+ * the FeeFrac::fee field in the result. */
+FeeFrac EvaluateDiagram(int32_t size, Span<const FeeFrac> diagram)
+{
+    assert(diagram.size() > 0);
+    unsigned not_above = 0;
+    unsigned not_below = diagram.size() - 1;
+    // If outside the range of diagram, extend begin/end.
+    if (size < diagram[not_above].size) return {diagram[not_above].fee, 1};
+    if (size > diagram[not_below].size) return {diagram[not_below].fee, 1};
+    // Perform bisection search to locate the diagram segment that size is in.
+    while (not_below > not_above + 1) {
+        unsigned mid = (not_below + not_above) / 2;
+        if (diagram[mid].size <= size) not_above = mid;
+        if (diagram[mid].size >= size) not_below = mid;
+    }
+    // If the size matches a transition point between segments, return its fee.
+    if (not_below == not_above) return {diagram[not_below].fee, 1};
+    // Otherwise, interpolate.
+    auto dir_coef = diagram[not_below] - diagram[not_above];
+    assert(dir_coef.size > 0);
+    // Let A = diagram[not_above] and B = diagram[not_below]
+    const auto& point_a = diagram[not_above];
+    // We want to return:
+    //     A.fee + (B.fee - A.fee) / (B.size - A.size) * (size - A.size)
+    //   = A.fee + dir_coef.fee / dir_coef.size * (size - A.size)
+    //   = (A.fee * dir_coef.size + dir_coef.fee * (size - A.size)) / dir_coef.size
+    assert(size >= point_a.size);
+    return {point_a.fee * dir_coef.size + dir_coef.fee * (size - point_a.size), dir_coef.size};
+}
+
+std::weak_ordering CompareFeeFracWithDiagram(const FeeFrac& ff, Span<const FeeFrac> diagram)
+{
+    return FeeRateCompare(FeeFrac{ff.fee, 1}, EvaluateDiagram(ff.size, diagram));
+}
+
+std::partial_ordering CompareDiagrams(Span<const FeeFrac> dia1, Span<const FeeFrac> dia2)
+{
+    bool all_ge = true;
+    bool all_le = true;
+    for (const auto p1 : dia1) {
+        auto cmp = CompareFeeFracWithDiagram(p1, dia2);
+        if (std::is_lt(cmp)) all_ge = false;
+        if (std::is_gt(cmp)) all_le = false;
+    }
+    for (const auto p2 : dia2) {
+        auto cmp = CompareFeeFracWithDiagram(p2, dia1);
+        if (std::is_lt(cmp)) all_le = false;
+        if (std::is_gt(cmp)) all_ge = false;
+    }
+    if (all_ge && all_le) return std::partial_ordering::equivalent;
+    if (all_ge && !all_le) return std::partial_ordering::greater;
+    if (!all_ge && all_le) return std::partial_ordering::less;
+    return std::partial_ordering::unordered;
+}
+
+void PopulateChunks(FuzzedDataProvider& fuzzed_data_provider, std::vector<FeeFrac>& chunks)
+{
+    chunks.clear();
+
+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 50)
+    {
+        chunks.emplace_back(fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(INT32_MIN>>1, INT32_MAX>>1), fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(1, 1000000));
+    }
+    return;
+}
+
+} // namespace
+
+FUZZ_TARGET(build_and_compare_feerate_diagram)
+{
+    // Generate a random set of chunks
+    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
+    std::vector<FeeFrac> chunks1, chunks2;
+    FeeFrac empty{0, 0};
+
+    PopulateChunks(fuzzed_data_provider, chunks1);
+    PopulateChunks(fuzzed_data_provider, chunks2);
+
+    std::vector<FeeFrac> diagram1{BuildDiagramFromChunks(chunks1)};
+    std::vector<FeeFrac> diagram2{BuildDiagramFromChunks(chunks2)};
+
+    assert(diagram1.front() == empty);
+    assert(diagram2.front() == empty);
+
+    auto real = CompareFeerateDiagram(diagram1, diagram2);
+    auto sim = CompareDiagrams(diagram1, diagram2);
+    assert(real == sim);
+
+    // Do explicit evaluation at up to 1000 points, and verify consistency with the result.
+    LIMITED_WHILE(fuzzed_data_provider.remaining_bytes(), 1000) {
+        int32_t size = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(0, diagram2.back().size);
+        auto eval1 = EvaluateDiagram(size, diagram1);
+        auto eval2 = EvaluateDiagram(size, diagram2);
+        auto cmp = FeeRateCompare(eval1, eval2);
+        if (std::is_lt(cmp)) assert(!std::is_gt(real));
+        if (std::is_gt(cmp)) assert(!std::is_lt(real));
+    }
+}

src/test/fuzz/rbf.cpp

@@ -23,12 +23,30 @@ namespace {
 const BasicTestingSetup* g_setup;
 } // namespace
 
+const int NUM_ITERS = 10000;
+
+std::vector<COutPoint> g_outpoints;
+
 void initialize_rbf()
 {
     static const auto testing_setup = MakeNoLogFileContext<>();
     g_setup = testing_setup.get();
 }
 
+void initialize_package_rbf()
+{
+    static const auto testing_setup = MakeNoLogFileContext<>();
+    g_setup = testing_setup.get();
+
+    // Create a fixed set of unique "UTXOs" to source parents from
+    // to avoid fuzzer giving circular references
+    for (int i = 0; i < NUM_ITERS; ++i) {
+        g_outpoints.emplace_back();
+        g_outpoints.back().n = i;
+    }
+
+}
+
 FUZZ_TARGET(rbf, .init = initialize_rbf)
 {
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
@@ -40,7 +58,7 @@ FUZZ_TARGET(rbf, .init = initialize_rbf)
 
     CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)};
 
-    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000)
+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), NUM_ITERS)
     {
         const std::optional<CMutableTransaction> another_mtx = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider, TX_WITH_WITNESS);
         if (!another_mtx) {
@@ -63,3 +81,98 @@ FUZZ_TARGET(rbf, .init = initialize_rbf)
         (void)IsRBFOptIn(tx, pool);
     }
 }
+
+void CheckDiagramConcave(std::vector<FeeFrac>& diagram)
+{
+    // Diagrams are in monotonically-decreasing feerate order.
+    FeeFrac last_chunk = diagram.front();
+    for (size_t i = 1; i<diagram.size(); ++i) {
+        FeeFrac next_chunk = diagram[i] - diagram[i-1];
+        assert(next_chunk <= last_chunk);
+        last_chunk = next_chunk;
+    }
+}
+
+FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
+{
+    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
+    SetMockTime(ConsumeTime(fuzzed_data_provider));
+
+    std::optional<CMutableTransaction> child = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider, TX_WITH_WITNESS);
+    if (!child) return;
+
+    CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)};
+
+    // Add a bunch of parent-child pairs to the mempool, and remember them.
+    std::vector<CTransaction> mempool_txs;
+    size_t iter{0};
+
+    LOCK2(cs_main, pool.cs);
+
+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), NUM_ITERS)
+    {
+        // Make sure txns only have one input, and that a unique input is given to avoid circular references
+        std::optional<CMutableTransaction> parent = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider, TX_WITH_WITNESS);
+        if (!parent) {
+            continue;
+        }
+        assert(iter <= g_outpoints.size());
+        parent->vin.resize(1);
+        parent->vin[0].prevout = g_outpoints[iter++];
+
+        mempool_txs.emplace_back(*parent);
+        pool.addUnchecked(ConsumeTxMemPoolEntry(fuzzed_data_provider, mempool_txs.back()));
+        if (fuzzed_data_provider.ConsumeBool() && !child->vin.empty()) {
+            child->vin[0].prevout = COutPoint{mempool_txs.back().GetHash(), 0};
+        }
+        mempool_txs.emplace_back(*child);
+        pool.addUnchecked(ConsumeTxMemPoolEntry(fuzzed_data_provider, mempool_txs.back()));
+    }
+
+    // Pick some transactions at random to be the direct conflicts
+    CTxMemPool::setEntries direct_conflicts;
+    for (auto& tx : mempool_txs) {
+        if (fuzzed_data_provider.ConsumeBool()) {
+            direct_conflicts.insert(*pool.GetIter(tx.GetHash()));
+        }
+    }
+
+    // Calculate all conflicts:
+    CTxMemPool::setEntries all_conflicts;
+    for (auto& txiter : direct_conflicts) {
+        pool.CalculateDescendants(txiter, all_conflicts);
+    }
+
+    // Calculate the feerate diagrams for a replacement.
+    CAmount replacement_fees = ConsumeMoney(fuzzed_data_provider);
+    int64_t replacement_vsize = fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(1, 1000000);
+    auto calc_results{pool.CalculateFeerateDiagramsForRBF(replacement_fees, replacement_vsize, direct_conflicts, all_conflicts)};
+
+    if (calc_results.has_value()) {
+        // Sanity checks on the diagrams.
+
+        // Diagrams start at 0.
+        assert(calc_results->first.front().size == 0);
+        assert(calc_results->first.front().fee == 0);
+        assert(calc_results->second.front().size == 0);
+        assert(calc_results->second.front().fee == 0);
+
+        CheckDiagramConcave(calc_results->first);
+        CheckDiagramConcave(calc_results->second);
+
+        CAmount replaced_fee{0};
+        int64_t replaced_size{0};
+        for (auto txiter : all_conflicts) {
+            replaced_fee += txiter->GetModifiedFee();
+            replaced_size += txiter->GetTxSize();
+        }
+        // The total fee of the new diagram should be the total fee of the old
+        // diagram - replaced_fee + replacement_fees
+        assert(calc_results->first.back().fee - replaced_fee + replacement_fees == calc_results->second.back().fee);
+        assert(calc_results->first.back().size - replaced_size + replacement_vsize == calc_results->second.back().size);
+    }
+
+    // If internals report error, wrapper should too
+    auto err_tuple{ImprovesFeerateDiagram(pool, direct_conflicts, all_conflicts, replacement_fees, replacement_vsize)};
+    if (!calc_results.has_value()) assert(err_tuple.has_value());
+}