Avoid explicitly computing diagram; compare based on chunks

src/policy/rbf.cpp

@@ -190,14 +190,14 @@ std::optional<std::pair<DiagramCheckError, std::string>> ImprovesFeerateDiagram(
                                                 CAmount replacement_fees,
                                                 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)};
+    // Require that the replacement strictly improve the mempool's feerate diagram.
+    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

@@ -108,17 +108,6 @@ BOOST_AUTO_TEST_CASE(build_diagram_test)
     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());
@@ -149,31 +138,33 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
 
     // 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)};
+    int32_t replacement_vsize = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(1, 1000000);
+    auto calc_results{pool.CalculateChunksForRBF(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);
+        // 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};
+        int32_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);
+        assert((first_sum - FeeFrac{replaced_fee, replaced_size} + FeeFrac{replacement_fees, replacement_vsize}) == second_sum);
     }
 
     // If internals report error, wrapper should too