diff --git a/src/cluster_linearize.h b/src/cluster_linearize.h index 774bc61734f..1bf3d475f10 100644 --- a/src/cluster_linearize.h +++ b/src/cluster_linearize.h @@ -432,7 +432,7 @@ std::vector> ChunkLinearizationInfo(const DepGraph& de /** The new chunk to be added, initially a singleton. */ SetInfo new_chunk(depgraph, i); // As long as the new chunk has a higher feerate than the last chunk so far, absorb it. - while (!ret.empty() && new_chunk.feerate >> ret.back().feerate) { + while (!ret.empty() && ByRatio{new_chunk.feerate} > ByRatio{ret.back().feerate}) { new_chunk |= ret.back(); ret.pop_back(); } @@ -452,7 +452,7 @@ std::vector ChunkLinearization(const DepGraph& depgraph, std:: /** The new chunk to be added, initially a singleton. */ auto new_chunk = depgraph.FeeRate(i); // As long as the new chunk has a higher feerate than the last chunk so far, absorb it. - while (!ret.empty() && new_chunk >> ret.back()) { + while (!ret.empty() && ByRatio{new_chunk} > ByRatio{ret.back()}) { new_chunk += ret.back(); ret.pop_back(); } @@ -1027,8 +1027,8 @@ private: auto& reached_chunk_info = m_set_info[reached_chunk_idx]; todo -= reached_chunk_info.transactions; // See if it has an acceptable feerate. - auto cmp = DownWard ? FeeRateCompare(best_other_chunk_feerate, reached_chunk_info.feerate) - : FeeRateCompare(reached_chunk_info.feerate, best_other_chunk_feerate); + auto cmp = DownWard ? ByRatio{best_other_chunk_feerate} <=> ByRatio{reached_chunk_info.feerate} + : ByRatio{reached_chunk_info.feerate} <=> ByRatio{best_other_chunk_feerate}; if (cmp > 0) continue; uint64_t tiebreak = m_rng.rand64(); if (cmp < 0 || tiebreak >= best_other_chunk_tiebreak) { @@ -1150,7 +1150,7 @@ private: auto& dep_top_info = m_set_info[tx_data.dep_top_idx[child_idx]]; // Skip if this dependency is ineligible (the top chunk that would be created // does not have higher feerate than the chunk it is currently part of). - auto cmp = FeeRateCompare(dep_top_info.feerate, chunk_info.feerate); + auto cmp = ByRatio{dep_top_info.feerate} <=> ByRatio{chunk_info.feerate}; if (cmp <= 0) continue; // Generate a random tiebreak for this dependency, and reject it if its tiebreak // is worse than the best so far. This means that among all eligible @@ -1377,7 +1377,7 @@ public: // Skip if this dependency does not have equal top and bottom set feerates. Note // that the top cannot have higher feerate than the bottom, or OptimizeSteps would // have dealt with it. - if (dep_top_info.feerate << chunk_info.feerate) continue; + if (ByRatio{dep_top_info.feerate} < ByRatio{chunk_info.feerate}) continue; have_any = true; // Skip if this dependency does not have pivot in the right place. if (move_pivot_down == dep_top_info.transactions[pivot_idx]) continue; @@ -1506,7 +1506,7 @@ public: // First sort by increasing transaction feerate. auto& a_feerate = m_depgraph.FeeRate(a); auto& b_feerate = m_depgraph.FeeRate(b); - auto feerate_cmp = FeeRateCompare(a_feerate, b_feerate); + auto feerate_cmp = ByRatio{a_feerate} <=> ByRatio{b_feerate}; if (feerate_cmp != 0) return feerate_cmp < 0; // Then by decreasing transaction size. if (a_feerate.size != b_feerate.size) { @@ -1528,7 +1528,7 @@ public: // First sort by increasing chunk feerate. auto& chunk_feerate_a = m_set_info[a.first].feerate; auto& chunk_feerate_b = m_set_info[b.first].feerate; - auto feerate_cmp = FeeRateCompare(chunk_feerate_a, chunk_feerate_b); + auto feerate_cmp = ByRatio{chunk_feerate_a} <=> ByRatio{chunk_feerate_b}; if (feerate_cmp != 0) return feerate_cmp < 0; // Then by decreasing chunk size. if (chunk_feerate_a.size != chunk_feerate_b.size) { @@ -1618,7 +1618,7 @@ public: for (auto chunk_idx : m_chunk_idxs) { ret.push_back(m_set_info[chunk_idx].feerate); } - std::sort(ret.begin(), ret.end(), std::greater{}); + std::sort(ret.begin(), ret.end(), std::greater>{}); return ret; } @@ -1972,7 +1972,7 @@ void PostLinearize(const DepGraph& depgraph, std::span l DepGraphIndex next_group = SENTINEL; // We inserted at the end, so next group is sentinel. DepGraphIndex prev_group = entries[cur_group].prev_group; // Continue as long as the current group has higher feerate than the previous one. - while (entries[cur_group].feerate >> entries[prev_group].feerate) { + while (ByRatio{entries[cur_group].feerate} > ByRatio{entries[prev_group].feerate}) { // prev_group/cur_group/next_group refer to (the last transactions of) 3 // consecutive entries in groups list. Assume(cur_group == entries[next_group].prev_group); diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 7ea8d10b730..836b828b5c4 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -295,7 +295,7 @@ void BlockAssembler::addChunks() while (selected_transactions.size() > 0) { // Check to see if min fee rate is still respected. - if (chunk_feerate_vsize << m_options.blockMinFeeRate.GetFeePerVSize()) { + if (ByRatio{chunk_feerate_vsize} < ByRatio{m_options.blockMinFeeRate.GetFeePerVSize()}) { // Everything else we might consider has a lower feerate return; } diff --git a/src/node/mini_miner.cpp b/src/node/mini_miner.cpp index 9d85db7ded0..59ca1b556d6 100644 --- a/src/node/mini_miner.cpp +++ b/src/node/mini_miner.cpp @@ -184,12 +184,12 @@ struct AncestorFeerateComparator auto min_feerate = [](const MiniMinerMempoolEntry& e) -> FeeFrac { FeeFrac self_feerate(e.GetModifiedFee(), e.GetTxSize()); FeeFrac ancestor_feerate(e.GetModFeesWithAncestors(), e.GetSizeWithAncestors()); - return std::min(ancestor_feerate, self_feerate); + return std::min>(ancestor_feerate, self_feerate); }; FeeFrac a_feerate{min_feerate(a->second)}; FeeFrac b_feerate{min_feerate(b->second)}; if (a_feerate != b_feerate) { - return a_feerate > b_feerate; + return ByRatioNegSize{a_feerate} > ByRatioNegSize{b_feerate}; } // Use txid as tiebreaker for stable sorting return a->first < b->first; diff --git a/src/node/txorphanage.cpp b/src/node/txorphanage.cpp index ca7eb20470a..32608e05769 100644 --- a/src/node/txorphanage.cpp +++ b/src/node/txorphanage.cpp @@ -164,7 +164,7 @@ class TxOrphanageImpl final : public TxOrphanage { assert(max_peer_memory > 0); const FeeFrac latency_score(m_total_latency_score, max_peer_latency_score); const FeeFrac mem_score(m_total_usage, max_peer_memory); - return std::max(latency_score, mem_score); + return std::max>(latency_score, mem_score); } }; /** Store per-peer statistics. Used to determine each peer's DoS score. The size of this map is used to determine the @@ -457,12 +457,18 @@ void TxOrphanageImpl::LimitOrphans() for (const auto& [nodeid, entry] : m_peer_orphanage_info) { // Performance optimization: only consider peers with a DoS score > 1. const auto dos_score = entry.GetDosScore(max_lat, max_mem); - if (dos_score >> FeeFrac{1, 1}) { + if (ByRatio{dos_score} > ByRatio{FeeFrac{1, 1}}) { heap_peer_dos.emplace_back(nodeid, dos_score); } } static constexpr auto compare_score = [](const auto& left, const auto& right) { - if (left.second != right.second) return left.second < right.second; + if (left.second != right.second) { + // Note: if ratios are the same, this tiebreaks by denominator. In practice, since the + // latency denominator (number of announcements and inputs) is always lower, this means + // that a peer with only high latency scores will be targeted before a peer using a lot + // of memory, even if they have the same ratios. + return ByRatioNegSize{left.second} < ByRatioNegSize{right.second}; + } // Tiebreak by considering the more recent peer (higher NodeId) to be worse. return left.first < right.first; }; @@ -472,9 +478,6 @@ void TxOrphanageImpl::LimitOrphans() // This outer loop finds the peer with the highest DoS score, which is a fraction of memory and latency scores // over the respective allowances. We continue until the orphanage is within global limits. That means some peers // might still have a DoS score > 1 at the end. - // Note: if ratios are the same, FeeFrac tiebreaks by denominator. In practice, since the latency denominator (number of - // announcements and inputs) is always lower, this means that a peer with only high latency scores will be targeted - // before a peer using a lot of memory, even if they have the same ratios. do { Assume(!heap_peer_dos.empty()); // This is a max-heap, so the worst peer is at the front. pop_heap() @@ -484,7 +487,7 @@ void TxOrphanageImpl::LimitOrphans() heap_peer_dos.pop_back(); // If needs trim, then at least one peer has a DoS score higher than 1. - Assume(dos_score >> (FeeFrac{1, 1})); + Assume(ByRatio{dos_score} > ByRatio{FeeFrac(1, 1)}); auto it_worst_peer = m_peer_orphanage_info.find(worst_peer); @@ -506,7 +509,8 @@ void TxOrphanageImpl::LimitOrphans() // If we erased the last orphan from this peer, it_worst_peer will be invalidated. it_worst_peer = m_peer_orphanage_info.find(worst_peer); - if (it_worst_peer == m_peer_orphanage_info.end() || it_worst_peer->second.GetDosScore(max_lat, max_mem) <= dos_threshold) break; + if (it_worst_peer == m_peer_orphanage_info.end() || + ByRatioNegSize{it_worst_peer->second.GetDosScore(max_lat, max_mem)} <= ByRatioNegSize{dos_threshold}) break; } LogDebug(BCLog::TXPACKAGES, "peer=%d orphanage overflow, removed %u of %u announcements\n", worst_peer, num_erased_this_round, starting_num_ann); diff --git a/src/policy/feerate.h b/src/policy/feerate.h index f6b49a1465b..8f13f8d0dca 100644 --- a/src/policy/feerate.h +++ b/src/policy/feerate.h @@ -60,13 +60,13 @@ public: * Return the fee in satoshis for a vsize of 1000 vbytes */ CAmount GetFeePerK() const { return CAmount(m_feerate.EvaluateFeeDown(1000)); } - friend std::weak_ordering operator<=>(const CFeeRate& a, const CFeeRate& b) noexcept + friend std::strong_ordering operator<=>(const CFeeRate& a, const CFeeRate& b) noexcept { - return FeeRateCompare(a.m_feerate, b.m_feerate); + return ByRatio{a.m_feerate} <=> ByRatio{b.m_feerate}; } friend bool operator==(const CFeeRate& a, const CFeeRate& b) noexcept { - return FeeRateCompare(a.m_feerate, b.m_feerate) == std::weak_ordering::equivalent; + return ByRatio{a.m_feerate} == ByRatio{b.m_feerate}; } CFeeRate& operator+=(const CFeeRate& a) { m_feerate = FeePerVSize(GetFeePerK() + a.GetFeePerK(), 1000); diff --git a/src/test/feefrac_tests.cpp b/src/test/feefrac_tests.cpp index ff95fc9efb8..3d5fe0cb88c 100644 --- a/src/test/feefrac_tests.cpp +++ b/src/test/feefrac_tests.cpp @@ -70,40 +70,40 @@ BOOST_AUTO_TEST_CASE(feefrac_operators) 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(ByRatioNegSize{p1} > ByRatioNegSize{p2}); + BOOST_CHECK(ByRatioNegSize{p1} >= ByRatioNegSize{p2}); + BOOST_CHECK(ByRatioNegSize{p1} >= ByRatioNegSize{p4-p3}); + BOOST_CHECK(!(ByRatio{p1} > ByRatio{p3})); // not strictly better + BOOST_CHECK(ByRatio{p1} > ByRatio{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 + BOOST_CHECK(ByRatioNegSize{p2} < ByRatioNegSize{p1}); + BOOST_CHECK(ByRatioNegSize{p2} <= ByRatioNegSize{p1}); + BOOST_CHECK(ByRatioNegSize{p1} <= ByRatioNegSize{p4-p3}); + BOOST_CHECK(!(ByRatio{p3} < ByRatio{p1})); // not strictly worse + BOOST_CHECK(ByRatio{p2} < ByRatio{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)); + BOOST_CHECK(!(ByRatio{p1} > ByRatio{empty})); // << will always result in false + BOOST_CHECK(!(ByRatio{p1} < ByRatio{empty})); + BOOST_CHECK(!(ByRatio{empty} > ByRatio{empty})); + BOOST_CHECK(!(ByRatio{empty} < ByRatio{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); + BOOST_CHECK(ByRatioNegSize{empty} > ByRatioNegSize{p1}); + BOOST_CHECK(ByRatioNegSize{empty} > ByRatioNegSize{p2}); + BOOST_CHECK(ByRatioNegSize{empty} > ByRatioNegSize{p3}); + BOOST_CHECK(ByRatioNegSize{empty} >= ByRatioNegSize{p1}); + BOOST_CHECK(ByRatioNegSize{empty} >= ByRatioNegSize{p2}); + BOOST_CHECK(ByRatioNegSize{empty} >= ByRatioNegSize{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); + BOOST_CHECK(ByRatioNegSize{oversized_1} < ByRatioNegSize{oversized_2}); + BOOST_CHECK(ByRatioNegSize{oversized_1} <= ByRatioNegSize{oversized_2}); + BOOST_CHECK(ByRatio{oversized_1} < ByRatio{oversized_2}); + BOOST_CHECK(ByRatioNegSize{oversized_1} != ByRatioNegSize{oversized_2}); BOOST_CHECK_EQUAL(oversized_1.EvaluateFeeDown(0), 0); BOOST_CHECK_EQUAL(oversized_1.EvaluateFeeDown(1), 1152921); @@ -124,13 +124,13 @@ BOOST_AUTO_TEST_CASE(feefrac_operators) // Tests paths that use double arithmetic FeeFrac busted{(static_cast(INT32_MAX)) + 1, INT32_MAX}; - BOOST_CHECK(!(busted < busted)); + BOOST_CHECK(!(ByRatioNegSize{busted} < ByRatioNegSize{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); + BOOST_CHECK(!(ByRatioNegSize{max_fee} < ByRatioNegSize{max_fee})); + BOOST_CHECK(!(ByRatioNegSize{max_fee} > ByRatioNegSize{max_fee})); + BOOST_CHECK(ByRatioNegSize{max_fee} <= ByRatioNegSize{max_fee}); + BOOST_CHECK(ByRatioNegSize{max_fee} >= ByRatioNegSize{max_fee}); BOOST_CHECK_EQUAL(max_fee.EvaluateFeeDown(0), 0); BOOST_CHECK_EQUAL(max_fee.EvaluateFeeDown(1), 977888); @@ -146,7 +146,7 @@ BOOST_AUTO_TEST_CASE(feefrac_operators) BOOST_CHECK_EQUAL(max_fee.EvaluateFeeUp(INT32_MAX), 2100000000000000); FeeFrac max_fee2{1, 1}; - BOOST_CHECK(max_fee >= max_fee2); + BOOST_CHECK(ByRatioNegSize{max_fee} >= ByRatioNegSize{max_fee2}); // Test for integer overflow issue (https://github.com/bitcoin/bitcoin/issues/32294) BOOST_CHECK_EQUAL((FeeFrac{0x7ffffffdfffffffb, 0x7ffffffd}.EvaluateFeeDown(0x7fffffff)), 0x7fffffffffffffff); diff --git a/src/test/fuzz/cluster_linearize.cpp b/src/test/fuzz/cluster_linearize.cpp index b735940100a..e529078c65d 100644 --- a/src/test/fuzz/cluster_linearize.cpp +++ b/src/test/fuzz/cluster_linearize.cpp @@ -126,7 +126,7 @@ public: // Add a queue entry with split excluded. queue.emplace_back(inc, und - m_depgraph.Descendants(split)); // Update statistics to account for the candidate new_inc. - if (new_inc.feerate > best.feerate) best = new_inc; + if (ByRatioNegSize{new_inc.feerate} > ByRatioNegSize{best.feerate}) best = new_inc; break; } } @@ -181,7 +181,7 @@ public: x_shifted >>= 1; } SetInfo cur(m_depgraph, txn & m_todo); - if (cur.feerate > best.feerate) best = cur; + if (ByRatioNegSize{cur.feerate} > ByRatioNegSize{best.feerate}) best = cur; } return best; } @@ -737,7 +737,7 @@ FUZZ_TARGET(clusterlin_chunking) // Verify that chunk feerates are monotonically non-increasing. for (size_t i = 1; i < chunking.size(); ++i) { - assert(!(chunking[i] >> chunking[i - 1])); + assert(ByRatio{chunking[i]} <= ByRatio{chunking[i - 1]}); } // Naively recompute the chunks (each is the highest-feerate prefix of what remains). @@ -748,7 +748,7 @@ FUZZ_TARGET(clusterlin_chunking) for (DepGraphIndex idx : linearization) { if (todo[idx]) { accumulator.Set(depgraph, idx); - if (best.feerate.IsEmpty() || accumulator.feerate >> best.feerate) { + if (best.feerate.IsEmpty() || ByRatio{accumulator.feerate} > ByRatio{best.feerate}) { best = accumulator; } } @@ -825,7 +825,7 @@ FUZZ_TARGET(clusterlin_simple_finder) // Compare with a non-empty topological set read from the fuzz input (comparing with an // empty set is not interesting). auto read_topo = ReadTopologicalSet(depgraph, todo, reader, /*non_empty=*/true); - assert(found.feerate >= depgraph.FeeRate(read_topo)); + assert(ByRatioNegSize{found.feerate} >= ByRatioNegSize{depgraph.FeeRate(read_topo)}); } // Find a non-empty topologically valid subset of transactions to remove from the graph. @@ -1110,9 +1110,9 @@ FUZZ_TARGET(clusterlin_linearize) // Check whether tx2 only depends on transactions that precede tx1. if ((depgraph.Ancestors(tx2) - done).Count() == 1) { // tx2 could take position pos1. - // Verify that individual transaction feerate is decreasing (note that >= - // tie-breaks by size). - assert(depgraph.FeeRate(tx1) >= depgraph.FeeRate(tx2)); + // Verify that individual transaction feerate is decreasing (tie-breaking by + // size). + assert(ByRatioNegSize{depgraph.FeeRate(tx1)} >= ByRatioNegSize{depgraph.FeeRate(tx2)}); // If feerate and size are equal, compare by DepGraphIndex. if (depgraph.FeeRate(tx1) == depgraph.FeeRate(tx2)) { assert(tx1 < tx2); @@ -1139,8 +1139,8 @@ FUZZ_TARGET(clusterlin_linearize) // Check whether chunk2 only depends on transactions that precede chunk1. if ((chunk2_ancestors - done).IsSubsetOf(chunk2.transactions)) { // chunk2 could take position chunk_num1. - // Verify that chunk feerate is decreasing (note that >= tie-breaks by size). - assert(chunk1.feerate >= chunk2.feerate); + // Verify that chunk feerate is decreasing (tie-breaking by size). + assert(ByRatioNegSize{chunk1.feerate} >= ByRatioNegSize{chunk2.feerate}); // If feerate and size are equal, compare by maximum DepGraphIndex element. if (chunk1.feerate == chunk2.feerate) { assert(chunk1.transactions.Last() < chunk2.transactions.Last()); diff --git a/src/test/fuzz/feefrac.cpp b/src/test/fuzz/feefrac.cpp index d52a46582c3..6b6a348387f 100644 --- a/src/test/fuzz/feefrac.cpp +++ b/src/test/fuzz/feefrac.cpp @@ -84,9 +84,9 @@ FUZZ_TARGET(feefrac) // 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)); + assert((ByRatio{fr1} <=> ByRatio{fr2}) == cmp_feerate); + assert((ByRatio{fr1} < ByRatio{fr2}) == std::is_lt(cmp_feerate)); + assert((ByRatio{fr1} > ByRatio{fr2}) == std::is_gt(cmp_feerate)); // Compare with manual invocation of FeeFrac::Mul. auto cmp_mul = FeeFrac::Mul(f1, s2) <=> FeeFrac::Mul(f2, s1); @@ -98,13 +98,13 @@ FUZZ_TARGET(feefrac) // 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)); + assert((ByRatioNegSize{fr1} <=> ByRatioNegSize{fr2}) == cmp_total); + assert((ByRatioNegSize{fr1} < ByRatioNegSize{fr2}) == std::is_lt(cmp_total)); + assert((ByRatioNegSize{fr1} > ByRatioNegSize{fr2}) == std::is_gt(cmp_total)); + assert((ByRatioNegSize{fr1} <= ByRatioNegSize{fr2}) == std::is_lteq(cmp_total)); + assert((ByRatioNegSize{fr1} >= ByRatioNegSize{fr2}) == std::is_gteq(cmp_total)); + assert((ByRatioNegSize{fr1} == ByRatioNegSize{fr2}) == std::is_eq(cmp_total)); + assert((ByRatioNegSize{fr1} != ByRatioNegSize{fr2}) == std::is_neq(cmp_total)); } FUZZ_TARGET(feefrac_div_fallback) diff --git a/src/test/fuzz/feeratediagram.cpp b/src/test/fuzz/feeratediagram.cpp index 6dcc63e91e2..3a2d4e4ae26 100644 --- a/src/test/fuzz/feeratediagram.cpp +++ b/src/test/fuzz/feeratediagram.cpp @@ -63,9 +63,9 @@ FeeFrac EvaluateDiagram(int32_t size, std::span diagram) return {point_a.fee * dir_coef.size + dir_coef.fee * (size - point_a.size), dir_coef.size}; } -std::weak_ordering CompareFeeFracWithDiagram(const FeeFrac& ff, std::span diagram) +std::strong_ordering CompareFeeFracWithDiagram(const FeeFrac& ff, std::span diagram) { - return FeeRateCompare(FeeFrac{ff.fee, 1}, EvaluateDiagram(ff.size, diagram)); + return ByRatio{FeeFrac{ff.fee, 1}} <=> ByRatio{EvaluateDiagram(ff.size, diagram)}; } std::partial_ordering CompareDiagrams(std::span dia1, std::span dia2) @@ -126,7 +126,7 @@ FUZZ_TARGET(build_and_compare_feerate_diagram) int32_t size = fuzzed_data_provider.ConsumeIntegralInRange(0, diagram2.back().size); auto eval1 = EvaluateDiagram(size, diagram1); auto eval2 = EvaluateDiagram(size, diagram2); - auto cmp = FeeRateCompare(eval1, eval2); + auto cmp = ByRatio{eval1} <=> ByRatio{eval2}; if (std::is_lt(cmp)) assert(!std::is_gt(real)); if (std::is_gt(cmp)) assert(!std::is_lt(real)); } diff --git a/src/test/fuzz/rbf.cpp b/src/test/fuzz/rbf.cpp index 023fe7a42b1..bfa74bfb74b 100644 --- a/src/test/fuzz/rbf.cpp +++ b/src/test/fuzz/rbf.cpp @@ -210,12 +210,12 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf) 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])); + if (i) assert(ByRatio{calc_results->first[i - 1]} >= ByRatio{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])); + if (i) assert(ByRatio{calc_results->second[i - 1]} >= ByRatio{calc_results->second[i]}); } FeeFrac replaced; diff --git a/src/test/fuzz/txgraph.cpp b/src/test/fuzz/txgraph.cpp index 0fc4e047814..e3eb0dd18fe 100644 --- a/src/test/fuzz/txgraph.cpp +++ b/src/test/fuzz/txgraph.cpp @@ -433,7 +433,7 @@ FUZZ_TARGET(txgraph) assert(num_tx == sim.GetTransactionCount()); // Sort by feerate only, since violating topological constraints within same-feerate // chunks won't affect diagram comparisons. - std::sort(chunk_feerates.begin(), chunk_feerates.end(), std::greater{}); + std::sort(chunk_feerates.begin(), chunk_feerates.end(), std::greater>{}); return chunk_feerates; }; @@ -806,10 +806,10 @@ FUZZ_TARGET(txgraph) assert(sim_gain == real_gain); // Check that the feerates in each diagram are monotonically decreasing. for (size_t i = 1; i < real_main_diagram.size(); ++i) { - assert(FeeRateCompare(real_main_diagram[i], real_main_diagram[i - 1]) <= 0); + assert(ByRatio{real_main_diagram[i]} <= ByRatio{real_main_diagram[i - 1]}); } for (size_t i = 1; i < real_staged_diagram.size(); ++i) { - assert(FeeRateCompare(real_staged_diagram[i], real_staged_diagram[i - 1]) <= 0); + assert(ByRatio{real_staged_diagram[i]} <= ByRatio{real_staged_diagram[i - 1]}); } break; } else if (block_builders.size() < 4 && !main_sim.IsOversized() && command-- == 0) { @@ -829,7 +829,7 @@ FUZZ_TARGET(txgraph) if (chunk) { // Chunk feerates must be monotonously decreasing. if (!builder_data.last_feerate.IsEmpty()) { - assert(!(chunk->second >> builder_data.last_feerate)); + assert(ByRatio{chunk->second} <= ByRatio{builder_data.last_feerate}); } builder_data.last_feerate = chunk->second; // Verify the contents of GetCurrentChunk. @@ -1118,7 +1118,7 @@ FUZZ_TARGET(txgraph) std::pair max_chunk_tiebreak{0, 0}; for (const auto& chunk : real_chunking) { // If this is the first chunk with a strictly lower feerate, reset. - if (chunk.feerate << last_chunk_feerate) { + if (ByRatio{chunk.feerate} < ByRatio{last_chunk_feerate}) { comp_prefix_sizes.clear(); max_chunk_tiebreak = {0, 0}; } @@ -1212,12 +1212,12 @@ FUZZ_TARGET(txgraph) if (pos > 0) { size_t before = rng.randrange(pos); auto before_feerate = real->GetMainChunkFeerate(*sims[0].GetRef(vec1[before])); - assert(FeeRateCompare(before_feerate, pos_feerate) >= 0); + assert(ByRatio{before_feerate} >= ByRatio{pos_feerate}); } if (pos + 1 < vec1.size()) { size_t after = pos + 1 + rng.randrange(vec1.size() - 1 - pos); auto after_feerate = real->GetMainChunkFeerate(*sims[0].GetRef(vec1[after])); - assert(FeeRateCompare(after_feerate, pos_feerate) <= 0); + assert(ByRatio{after_feerate} <= ByRatio{pos_feerate}); } } @@ -1265,17 +1265,17 @@ FUZZ_TARGET(txgraph) auto [main_cmp_diagram, stage_cmp_diagram] = real->GetMainStagingDiagrams(); // Check that the feerates in each diagram are monotonically decreasing. for (size_t i = 1; i < main_cmp_diagram.size(); ++i) { - assert(FeeRateCompare(main_cmp_diagram[i], main_cmp_diagram[i - 1]) <= 0); + assert(ByRatio{main_cmp_diagram[i]} <= ByRatio{main_cmp_diagram[i - 1]}); } for (size_t i = 1; i < stage_cmp_diagram.size(); ++i) { - assert(FeeRateCompare(stage_cmp_diagram[i], stage_cmp_diagram[i - 1]) <= 0); + assert(ByRatio{stage_cmp_diagram[i]} <= ByRatio{stage_cmp_diagram[i - 1]}); } // Treat the diagrams as sets of chunk feerates, and sort them in the same way so that // std::set_difference can be used on them below. The exact ordering does not matter // here, but it has to be consistent with the one used in main_real_diagram and // stage_real_diagram). - std::sort(main_cmp_diagram.begin(), main_cmp_diagram.end(), std::greater{}); - std::sort(stage_cmp_diagram.begin(), stage_cmp_diagram.end(), std::greater{}); + std::sort(main_cmp_diagram.begin(), main_cmp_diagram.end(), std::greater>{}); + std::sort(stage_cmp_diagram.begin(), stage_cmp_diagram.end(), std::greater>{}); // Find the chunks that appear in main_diagram but are missing from main_cmp_diagram. // This is allowed, because GetMainStagingDiagrams omits clusters in main unaffected // by staging. @@ -1283,7 +1283,7 @@ FUZZ_TARGET(txgraph) std::set_difference(main_real_diagram.begin(), main_real_diagram.end(), main_cmp_diagram.begin(), main_cmp_diagram.end(), std::inserter(missing_main_cmp, missing_main_cmp.end()), - std::greater{}); + std::greater>{}); assert(main_cmp_diagram.size() + missing_main_cmp.size() == main_real_diagram.size()); // Do the same for chunks in stage_diagram missing from stage_cmp_diagram. auto stage_real_diagram = get_diagram_fn(TxGraph::Level::TOP); @@ -1291,7 +1291,7 @@ FUZZ_TARGET(txgraph) std::set_difference(stage_real_diagram.begin(), stage_real_diagram.end(), stage_cmp_diagram.begin(), stage_cmp_diagram.end(), std::inserter(missing_stage_cmp, missing_stage_cmp.end()), - std::greater{}); + std::greater>{}); assert(stage_cmp_diagram.size() + missing_stage_cmp.size() == stage_real_diagram.size()); // The missing chunks must be equal across main & staging (otherwise they couldn't have // been omitted). diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index f6720ffb897..9ba6c3a4fe9 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -554,7 +554,7 @@ FUZZ_TARGET(txorphanage_sim) count += 1 + (txn[ann.tx]->vin.size() / 10); usage += GetTransactionWeight(*txn[ann.tx]); } - return std::max(FeeFrac{count, max_count}, FeeFrac{usage, max_usage}); + return std::max>(FeeFrac{count, max_count}, FeeFrac{usage, max_usage}); }; // @@ -706,13 +706,13 @@ FUZZ_TARGET(txorphanage_sim) auto dos_score = dos_score_fn(peer, max_ann, max_mem); // Use >= so that the more recent peer (higher NodeId) wins in case of // ties. - if (dos_score >= worst_dos_score) { + if (ByRatioNegSize{dos_score} >= ByRatioNegSize{worst_dos_score}) { worst_dos_score = dos_score; worst_peer = peer; } } assert(worst_peer != unsigned(-1)); - assert(worst_dos_score >> FeeFrac(1, 1)); + assert(ByRatio{worst_dos_score} > ByRatio{FeeFrac(1, 1)}); // Find oldest announcement from worst_peer, preferring non-reconsiderable ones. bool done{false}; for (int reconsider = 0; reconsider < 2; ++reconsider) { diff --git a/src/txgraph.cpp b/src/txgraph.cpp index ca12bca6f49..8993947a9fe 100644 --- a/src/txgraph.cpp +++ b/src/txgraph.cpp @@ -496,9 +496,8 @@ private: const auto& entry_a = m_entries[a]; const auto& entry_b = m_entries[b]; // Compare chunk feerates, and return result if it differs. - auto feerate_cmp = FeeRateCompare(entry_b.m_main_chunk_feerate, entry_a.m_main_chunk_feerate); - if (feerate_cmp < 0) return std::strong_ordering::less; - if (feerate_cmp > 0) return std::strong_ordering::greater; + auto feerate_cmp = ByRatio{entry_b.m_main_chunk_feerate} <=> ByRatio{entry_a.m_main_chunk_feerate}; + if (feerate_cmp != 0) return feerate_cmp; // Compare equal-feerate chunk prefix size for comparing equal chunk feerates. This does two // things: it distinguishes equal-feerate chunks within the same cluster (because later // ones will always have a higher prefix size), and it may distinguish equal-feerate chunks @@ -1100,7 +1099,7 @@ void GenericClusterImpl::Updated(TxGraphImpl& graph, int level, bool rename) noe Assume(chunk_count > 0); // Update equal_feerate_chunk_feerate to include this chunk, starting over when the // feerate changed. - if (chunk.feerate << equal_feerate_chunk_feerate) { + if (ByRatio{chunk.feerate} < ByRatio{equal_feerate_chunk_feerate}) { equal_feerate_chunk_feerate = chunk.feerate; } else { // Note that this is adding fees to fees, and sizes to sizes, so the overall @@ -2828,8 +2827,8 @@ std::pair, std::vector> TxGraphImpl::GetMainStagin } } // Sort both by decreasing feerate to obtain diagrams, and return them. - std::sort(main_feerates.begin(), main_feerates.end(), [](auto& a, auto& b) { return a > b; }); - std::sort(staging_feerates.begin(), staging_feerates.end(), [](auto& a, auto& b) { return a > b; }); + std::sort(main_feerates.begin(), main_feerates.end(), std::greater>{}); + std::sort(staging_feerates.begin(), staging_feerates.end(), std::greater>{}); return std::make_pair(std::move(main_feerates), std::move(staging_feerates)); } @@ -2875,10 +2874,10 @@ void GenericClusterImpl::SanityCheck(const TxGraphImpl& graph, int level) const ++chunk_num; assert(chunk_num < linchunking.size()); chunk_pos = 0; - if (linchunking[chunk_num].feerate << equal_feerate_prefix) { + if (ByRatio{linchunking[chunk_num].feerate} < ByRatio{equal_feerate_prefix}) { equal_feerate_prefix = linchunking[chunk_num].feerate; } else { - assert(!(linchunking[chunk_num].feerate >> equal_feerate_prefix)); + assert(ByRatio{linchunking[chunk_num].feerate} == ByRatio{equal_feerate_prefix}); equal_feerate_prefix += linchunking[chunk_num].feerate; } } @@ -3103,7 +3102,7 @@ void TxGraphImpl::SanityCheck() const actual_chunkindex.insert(idx); auto chunk_feerate = m_entries[idx].m_main_chunk_feerate; if (!last_chunk_feerate.IsEmpty()) { - assert(FeeRateCompare(last_chunk_feerate, chunk_feerate) >= 0); + assert(ByRatio{last_chunk_feerate} >= ByRatio{FeeFrac{chunk_feerate}}); } last_chunk_feerate = chunk_feerate; } @@ -3338,7 +3337,7 @@ std::vector TxGraphImpl::Trim() noexcept // We do not need to sort by cluster or within clusters, because due to the implicit // dependency between consecutive linearization elements, no two transactions from the // same Cluster will ever simultaneously be in the heap. - return a->m_chunk_feerate < b->m_chunk_feerate; + return ByRatioNegSize{a->m_chunk_feerate} < ByRatioNegSize{b->m_chunk_feerate}; }; /** Given a TrimTxData entry, find the representative of the partition it is in. */ diff --git a/src/util/feefrac.cpp b/src/util/feefrac.cpp index 68ba2b6665a..6982626accb 100644 --- a/src/util/feefrac.cpp +++ b/src/util/feefrac.cpp @@ -42,17 +42,17 @@ std::partial_ordering CompareChunks(std::span chunks0, std::span< const auto slope_ap = point_p - point_a; Assume(slope_ap.size > 0); - std::weak_ordering cmp = std::weak_ordering::equivalent; + auto cmp = std::strong_ordering::equivalent; if (done_0 || done_1) { // If a single side has no points left, act as if AB has slope tail_feerate(of 0). Assume(!(done_0 && done_1)); - cmp = FeeRateCompare(slope_ap, FeeFrac(0, 1)); + cmp = ByRatio{slope_ap} <=> ByRatio{FeeFrac(0, 1)}; } else { // If both sides have points left, compute B, and the slope of AB explicitly. const FeeFrac& point_b = next_point(!unproc_side); const auto slope_ab = point_b - point_a; Assume(slope_ab.size >= slope_ap.size); - cmp = FeeRateCompare(slope_ap, slope_ab); + cmp = ByRatio{slope_ap} <=> ByRatio{slope_ab}; // If B and P have the same size, B can be marked as processed (in addition to P, see // below), as we've already performed a comparison at this size. diff --git a/src/util/feefrac.h b/src/util/feefrac.h index 7577107e8c2..7dac2f1366b 100644 --- a/src/util/feefrac.h +++ b/src/util/feefrac.h @@ -12,29 +12,9 @@ #include #include -/** Data structure storing a fee and size, ordered by increasing fee/size. +/** Data structure storing a fee and size. * * The size of a FeeFrac cannot be zero unless the fee is also zero. - * - * FeeFracs have a total ordering, first by increasing feerate (ratio of fee over size), and then - * by decreasing size. The empty FeeFrac (fee and size both 0) sorts last. So for example, the - * following FeeFracs are in sorted order: - * - * - fee=0 size=1 (feerate 0) - * - fee=1 size=2 (feerate 0.5) - * - fee=2 size=3 (feerate 0.667...) - * - fee=2 size=2 (feerate 1) - * - fee=1 size=1 (feerate 1) - * - fee=3 size=2 (feerate 1.5) - * - fee=2 size=1 (feerate 2) - * - fee=0 size=0 (undefined feerate) - * - * A FeeFrac is considered "better" if it sorts after another, by this ordering. All standard - * comparison operators (<=>, ==, !=, >, <, >=, <=) respect this ordering. - * - * The FeeRateCompare, and >> and << operators only compare feerate and treat equal feerate but - * different size as equivalent. The empty FeeFrac is neither lower or higher in feerate than any - * other. */ struct FeeFrac { @@ -153,35 +133,6 @@ struct FeeFrac return a.fee == b.fee && a.size == b.size; } - /** Compare two FeeFracs just by feerate. */ - friend inline std::weak_ordering FeeRateCompare(const FeeFrac& a, const FeeFrac& b) noexcept - { - auto cross_a = Mul(a.fee, b.size), cross_b = Mul(b.fee, a.size); - return cross_a <=> cross_b; - } - - /** Check if a FeeFrac object has strictly lower feerate than another. */ - friend inline bool operator<<(const FeeFrac& a, const FeeFrac& b) noexcept - { - auto cross_a = Mul(a.fee, b.size), cross_b = Mul(b.fee, a.size); - return cross_a < cross_b; - } - - /** Check if a FeeFrac object has strictly higher feerate than another. */ - friend inline bool operator>>(const FeeFrac& a, const FeeFrac& b) noexcept - { - auto cross_a = Mul(a.fee, b.size), cross_b = Mul(b.fee, a.size); - return cross_a > cross_b; - } - - /** Compare two FeeFracs. <, >, <=, and >= are auto-generated from this. */ - friend inline std::strong_ordering operator<=>(const FeeFrac& a, const FeeFrac& b) noexcept - { - auto cross_a = Mul(a.fee, b.size), cross_b = Mul(b.fee, a.size); - if (cross_a == cross_b) return b.size <=> a.size; - return cross_a <=> cross_b; - } - /** Swap two FeeFracs. */ friend inline void swap(FeeFrac& a, FeeFrac& b) noexcept { @@ -255,4 +206,107 @@ using FeePerVSize = FeePerUnit; struct WeightTag {}; using FeePerWeight = FeePerUnit; +/** Wrapper around FeeFrac & derived types, which adds a feerate-based ordering which treats + * equal-feerate but distinct-size FeeFracs as equals. + * + * This is not included inside FeeFrac itself, because it is not a total ordering (as would be + * expected for built-in comparison operators). + */ +template T> +class ByRatio +{ + const T& m_feefrac; + +public: + constexpr ByRatio(const T& feefrac) noexcept : m_feefrac{feefrac} {} + + friend bool operator==(const ByRatio& a, const ByRatio& b) noexcept + { + auto cross_a = T::Mul(a.m_feefrac.fee, b.m_feefrac.size); + auto cross_b = T::Mul(b.m_feefrac.fee, a.m_feefrac.size); + return cross_a == cross_b; + } + + // Note that we can use std::strong_ordering here, because even though FeeFrac{1,2} and + // FeeFrac{2,4} are distinct as FeeFracs, they are indistinguishable from ByRatio's perspective + // (operator== also treats them as equal). + friend std::strong_ordering operator<=>(const ByRatio& a, const ByRatio& b) noexcept + { + auto cross_a = T::Mul(a.m_feefrac.fee, b.m_feefrac.size); + auto cross_b = T::Mul(b.m_feefrac.fee, a.m_feefrac.size); + return cross_a <=> cross_b; + } + + // Specialized versions for efficiency. GCC 15+ and Clang 11+ produce operator<=>-derived + // versions that are equally efficient as this at -O2, but earlier versions do not. + friend bool operator<(const ByRatio& a, const ByRatio& b) noexcept + { + auto cross_a = T::Mul(a.m_feefrac.fee, b.m_feefrac.size); + auto cross_b = T::Mul(b.m_feefrac.fee, a.m_feefrac.size); + return cross_a < cross_b; + } + friend bool operator>(const ByRatio& a, const ByRatio& b) noexcept + { + auto cross_a = T::Mul(a.m_feefrac.fee, b.m_feefrac.size); + auto cross_b = T::Mul(b.m_feefrac.fee, a.m_feefrac.size); + return cross_a > cross_b; + } + friend bool operator<=(const ByRatio& a, const ByRatio& b) noexcept + { + auto cross_a = T::Mul(a.m_feefrac.fee, b.m_feefrac.size); + auto cross_b = T::Mul(b.m_feefrac.fee, a.m_feefrac.size); + return cross_a <= cross_b; + } + friend bool operator>=(const ByRatio& a, const ByRatio& b) noexcept + { + auto cross_a = T::Mul(a.m_feefrac.fee, b.m_feefrac.size); + auto cross_b = T::Mul(b.m_feefrac.fee, a.m_feefrac.size); + return cross_a >= cross_b; + } +}; + +/** Wrapper around FeeFrac & derived types, which adds a total ordering which first sorts by feerate + * and then by reversed size (i.e., larger sizes come first). + * + * This is not included inside FeeFrac itself, because it is not the most natural behavior, so it + * is better to make code using it invoke this explicitly. + * + * The empty FeeFrac (fee and size both 0) sorts last. So for example, the following FeeFracs are + * in sorted order: + * + * - fee=0 size=1 (feerate 0) + * - fee=1 size=2 (feerate 0.5) + * - fee=2 size=3 (feerate 0.667...) + * - fee=2 size=2 (feerate 1) + * - fee=1 size=1 (feerate 1) + * - fee=3 size=2 (feerate 1.5) + * - fee=2 size=1 (feerate 2) + * - fee=0 size=0 (undefined feerate) + */ +template T> +class ByRatioNegSize +{ + const T& m_feefrac; + +public: + constexpr ByRatioNegSize(const T& feefrac) noexcept : m_feefrac{feefrac} {} + + friend bool operator==(const ByRatioNegSize& a, const ByRatioNegSize& b) noexcept + { + return a.m_feefrac == b.m_feefrac; + } + + friend std::strong_ordering operator<=>(const ByRatioNegSize& a, const ByRatioNegSize& b) noexcept + { + auto cross_a = T::Mul(a.m_feefrac.fee, b.m_feefrac.size); + auto cross_b = T::Mul(b.m_feefrac.fee, a.m_feefrac.size); + auto cmp = cross_a <=> cross_b; + if (cmp != 0) return cmp; + return b.m_feefrac.size <=> a.m_feefrac.size; + } + + // Support conversion back to underlying FeeFrac, which allows using std::max(). + operator const T&() const noexcept { return m_feefrac; } +}; + #endif // BITCOIN_UTIL_FEEFRAC_H