Merge bitcoin/bitcoin#29757: feefrac: avoid explicitly computing diagram; compare based on chunks

b22901d Avoid explicitly computing diagram; compare based on chunks (Pieter Wuille)

Pull request description:

  This merges the `BuildDiagramFromChunks` and `CompareFeeRateDiagram` introduced in #29242 into a single `CompareChunks` function, which operates on sorted chunk data rather than diagrams, instead computing the diagram on the fly.

  This avoids the need for the construction of an intermediary diagram object, and removes the slightly arbitrary "all diagrams must start at (0, 0)" requirement.

  Not a big deal, but I think the result is a bit cleaner and not really more complicated.

ACKs for top commit:
  glozow:
    reACK b22901d
  instagibbs:
    reACK bitcoin/bitcoin@b22901d

Tree-SHA512: ca37bdf61d9a9cb5435f4da73e97ead33bf65828ad9af49b87336b1ece70db8ced1c21f517fc6eb6d616311c91f3da75ecae6b9bd42547133e3a3c5320b7816d

Author: glozow

src/policy/rbf.cpp

@@ -191,13 +191,13 @@ std::optional<std::pair<DiagramCheckError, std::string>> ImprovesFeerateDiagram(
                                                 int64_t replacement_vsize)
 {
     // Require that the replacement strictly improves the mempool's feerate diagram.
-    const auto diagram_results{pool.CalculateFeerateDiagramsForRBF(replacement_fees, replacement_vsize, direct_conflicts, all_conflicts)};
+    const auto chunk_results{pool.CalculateChunksForRBF(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 (!chunk_results.has_value()) {
+        return std::make_pair(DiagramCheckError::UNCALCULABLE, util::ErrorString(chunk_results).original);
     }
 
-    if (!std::is_gt(CompareFeerateDiagram(diagram_results.value().second, diagram_results.value().first))) {
+    if (!std::is_gt(CompareChunks(chunk_results.value().second, chunk_results.value().first))) {
         return std::make_pair(DiagramCheckError::FAILURE, "insufficient feerate: does not improve feerate diagram");
     }
     return std::nullopt;

src/test/feefrac_tests.cpp

@@ -82,43 +82,4 @@ BOOST_AUTO_TEST_CASE(feefrac_operators)
 
 }
 
-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/feeratediagram.cpp

@@ -16,6 +16,20 @@
 
 namespace {
 
+/** Takes the pre-computed and topologically-valid chunks and generates a fee diagram which starts at FeeFrac of (0, 0) */
+std::vector<FeeFrac> BuildDiagramFromChunks(const Span<const FeeFrac> chunks)
+{
+    std::vector<FeeFrac> diagram;
+    diagram.reserve(chunks.size() + 1);
+
+    diagram.emplace_back(0, 0);
+    for (auto& chunk : chunks) {
+        diagram.emplace_back(diagram.back() + chunk);
+    }
+    return diagram;
+}
+
+
 /** 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
@@ -103,7 +117,7 @@ FUZZ_TARGET(build_and_compare_feerate_diagram)
     assert(diagram1.front() == empty);
     assert(diagram2.front() == empty);
 
-    auto real = CompareFeerateDiagram(diagram1, diagram2);
+    auto real = CompareChunks(chunks1, chunks2);
     auto sim = CompareDiagrams(diagram1, diagram2);
     assert(real == sim);
 

src/test/fuzz/rbf.cpp

@@ -82,17 +82,6 @@ FUZZ_TARGET(rbf, .init = initialize_rbf)
     }
 }
 
-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());
@@ -107,7 +96,7 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
     std::vector<CTransaction> mempool_txs;
     size_t iter{0};
 
-    int64_t replacement_vsize = fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(1, 1000000);
+    int32_t replacement_vsize = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(1, 1000000);
 
     // Keep track of the total vsize of CTxMemPoolEntry's being added to the mempool to avoid overflow
     // Add replacement_vsize since this is added to new diagram during RBF check
@@ -167,32 +156,33 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
         pool.CalculateDescendants(txiter, all_conflicts);
     }
 
-    // Calculate the feerate diagrams for a replacement.
+    // Calculate the chunks for a replacement.
     CAmount replacement_fees = ConsumeMoney(fuzzed_data_provider);
-    auto calc_results{pool.CalculateFeerateDiagramsForRBF(replacement_fees, replacement_vsize, direct_conflicts, all_conflicts)};
+    auto calc_results{pool.CalculateChunksForRBF(replacement_fees, replacement_vsize, direct_conflicts, all_conflicts)};
 
     if (calc_results.has_value()) {
-        // Sanity checks on the diagrams.
+        // Sanity checks on the chunks.
 
-        // 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);
+        // Feerates are monotonically decreasing.
+        FeeFrac first_sum;
+        for (size_t i = 0; i < calc_results->first.size(); ++i) {
+            first_sum += calc_results->first[i];
+            if (i) assert(!(calc_results->first[i - 1] << calc_results->first[i]));
+        }
+        FeeFrac second_sum;
+        for (size_t i = 0; i < calc_results->second.size(); ++i) {
+            second_sum += calc_results->second[i];
+            if (i) assert(!(calc_results->second[i - 1] << calc_results->second[i]));
+        }
 
-        CAmount replaced_fee{0};
-        int64_t replaced_size{0};
+        FeeFrac replaced;
         for (auto txiter : all_conflicts) {
-            replaced_fee += txiter->GetModifiedFee();
-            replaced_size += txiter->GetTxSize();
+            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);
+        // The total fee & size of the new diagram minus replaced fee & size should be the total
+        // fee & size of the old diagram minus replacement fee & size.
+        assert((first_sum - replaced) == (second_sum - FeeFrac{replacement_fees, replacement_vsize}));
     }
 
     // If internals report error, wrapper should too
@@ -201,10 +191,12 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
          assert(err_tuple.value().first == DiagramCheckError::UNCALCULABLE);
     } else {
         // Diagram check succeeded
+        auto old_sum = std::accumulate(calc_results->first.begin(), calc_results->first.end(), FeeFrac{});
+        auto new_sum = std::accumulate(calc_results->second.begin(), calc_results->second.end(), FeeFrac{});
         if (!err_tuple.has_value()) {
             // New diagram's final fee should always match or exceed old diagram's
-            assert(calc_results->first.back().fee <= calc_results->second.back().fee);
-        } else if (calc_results->first.back().fee > calc_results->second.back().fee) {
+            assert(old_sum.fee <= new_sum.fee);
+        } else if (old_sum.fee > new_sum.fee) {
             // Or it failed, and if old diagram had higher fees, it should be a failure
             assert(err_tuple.value().first == DiagramCheckError::FAILURE);
         }