Merge #35: Fix: allow zero-fee transactions in LowestFee metric

67a8698 fix: allow zero-fee transactions in LowestFee metric (志宇)
92adec1 test: Upgrade `rand` and `proptest` (志宇)
575964f chore: make clippy happy (志宇)

Pull request description:

  Change assertion from fee > 0 to fee >= 0 to handle valid zero-fee transactions. Add test case to verify the metric works correctly when target fee rate is zero.

  Also make clippy happy and upgrade test dependencies.

Top commit has no ACKs.

Tree-SHA512: 0bbfa373f82d8f67a463d579eaa981b09ec7eae0f876d447b88421d9e6c18bf6bf4ecff2a17940eb07413cbf33475d07a8d6d543a046a617f883755fa741c238

Author: evanlinjin

Cargo.toml

@@ -21,6 +21,6 @@ default = ["std"]
 std = []
 
 [dev-dependencies]
-rand = "0.8"
-proptest = "1.4"
+rand = "0.9"
+proptest = "1.7"
 bitcoin = "0.32"

src/coin_selector.rs

@@ -2,7 +2,7 @@ use super::*;
 #[allow(unused)] // some bug in <= 1.48.0 sees this as unused when it isn't
 use crate::float::FloatExt;
 use crate::{bnb::BnbMetric, float::Ordf32, ChangePolicy, FeeRate, Target};
-use alloc::{borrow::Cow, collections::BTreeSet, vec::Vec};
+use alloc::{borrow::Cow, collections::BTreeSet};
 
 /// [`CoinSelector`] selects/deselects coins from a set of canididate coins.
 ///
@@ -16,7 +16,7 @@ pub struct CoinSelector<'a> {
     candidates: &'a [Candidate],
     selected: Cow<'a, BTreeSet<usize>>,
     banned: Cow<'a, BTreeSet<usize>>,
-    candidate_order: Cow<'a, Vec<usize>>,
+    candidate_order: Cow<'a, [usize]>,
 }
 
 impl<'a> CoinSelector<'a> {
@@ -578,6 +578,8 @@ impl<'a> CoinSelector<'a> {
     }
 }
 
+// Allow this for now due to MSRV
+#[allow(clippy::uninlined_format_args)]
 impl core::fmt::Display for CoinSelector<'_> {
     fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
         write!(f, "[")?;

src/metrics/lowest_fee.rs

@@ -32,8 +32,9 @@ impl BnbMetric for LowestFee {
             let drain = cs.drain(self.target, self.change_policy);
             let fee_for_the_tx = cs.fee(self.target.value(), drain.value);
             assert!(
-                fee_for_the_tx > 0,
-                "must not be called unless selection has met target"
+                fee_for_the_tx >= 0,
+                "must not be called unless selection has met target: fee={}",
+                fee_for_the_tx
             );
             // `spend_fee` rounds up here. We could use floats but I felt it was just better to
             // accept the extra 1 sat penality to having a change output

tests/bnb.rs

@@ -10,12 +10,12 @@ use proptest::{prelude::*, proptest, test_runner::*};
 
 fn test_wv(mut rng: impl RngCore) -> impl Iterator<Item = Candidate> {
     core::iter::repeat_with(move || {
-        let value = rng.gen_range(0..1_000);
+        let value = rng.random_range(0..1_000);
         let mut candidate = Candidate {
             value,
             weight: 100,
-            input_count: rng.gen_range(1..2),
-            is_segwit: rng.gen_bool(0.5),
+            input_count: rng.random_range(1..2),
+            is_segwit: rng.random_bool(0.5),
         };
         // HACK: set is_segwit = true for all these tests because you can't actually lower bound
         // things easily with how segwit inputs interfere with their weights. We can't modify the
@@ -100,7 +100,7 @@ fn bnb_finds_an_exact_solution_in_n_iter() {
         .last()
         .expect("it found a solution");
 
-    assert_eq!(rounds, 3150);
+    assert_eq!(rounds, 3194);
     assert_eq!(best.input_weight(), solution_weight);
     assert_eq!(best.selected_value(), target_value, "score={:?}", score);
 }
@@ -134,9 +134,9 @@ fn bnb_finds_solution_if_possible_in_n_iter() {
         .last()
         .expect("found a solution");
 
-    assert_eq!(rounds, 193);
+    assert_eq!(rounds, 164);
     let excess = sol.excess(target, Drain::NONE);
-    assert_eq!(excess, 1);
+    assert_eq!(excess, 0);
 }
 
 proptest! {

tests/changeless.rs

@@ -11,11 +11,11 @@ use rand::{prelude::IteratorRandom, Rng, RngCore};
 
 fn test_wv(mut rng: impl RngCore) -> impl Iterator<Item = Candidate> {
     core::iter::repeat_with(move || {
-        let value = rng.gen_range(0..1_000);
+        let value = rng.random_range(0..1_000);
         Candidate {
             value,
-            weight: rng.gen_range(0..100),
-            input_count: rng.gen_range(1..2),
+            weight: rng.random_range(0..100),
+            input_count: rng.random_range(1..2),
             is_segwit: false,
         }
     })

tests/common.rs

@@ -243,10 +243,10 @@ impl StrategyParams {
 pub fn gen_candidates(n: usize) -> Vec<Candidate> {
     let mut rng = TestRng::deterministic_rng(RngAlgorithm::ChaCha);
     core::iter::repeat_with(move || {
-        let value = rng.gen_range(1..500_001);
-        let weight = rng.gen_range(1..2001);
-        let input_count = rng.gen_range(1..3);
-        let is_segwit = rng.gen_bool(0.01);
+        let value = rng.random_range(1..500_001);
+        let weight = rng.random_range(1..2001);
+        let input_count = rng.random_range(1..3);
+        let is_segwit = rng.random_bool(0.01);
 
         Candidate {
             value,
@@ -465,11 +465,11 @@ pub fn compare_against_benchmarks<M: BnbMetric + Clone>(
 }
 
 #[allow(unused)]
-fn randomly_satisfy_target<'a>(
+fn randomly_satisfy_target<'a, R: rand::Rng>(
     cs: &CoinSelector<'a>,
     target: Target,
     change_policy: ChangePolicy,
-    rng: &mut impl RngCore,
+    rng: &mut R,
     mut metric: impl BnbMetric,
 ) -> CoinSelector<'a> {
     let mut cs = cs.clone();

tests/lowest_fee.rs

@@ -267,3 +267,55 @@ fn adding_another_input_to_remove_change() {
     assert!(score <= best_solution_score);
     assert_eq!(cs.selected_indices(), best_solution.selected_indices());
 }
+
+#[test]
+fn zero_fee_tx() {
+    let target_feerate = FeeRate::ZERO;
+    let long_term_feerate = FeeRate::DEFAULT_MIN_RELAY;
+
+    let target = Target {
+        fee: TargetFee {
+            rate: target_feerate,
+            replace: None,
+        },
+        outputs: TargetOutputs {
+            value_sum: 99_870,
+            weight_sum: 200 - TX_FIXED_FIELD_WEIGHT - 1,
+            n_outputs: 1,
+        },
+    };
+
+    let candidates = vec![
+        Candidate {
+            value: 100_000,
+            weight: 100,
+            input_count: 1,
+            is_segwit: true,
+        },
+        Candidate {
+            value: 50_000,
+            weight: 100,
+            input_count: 1,
+            is_segwit: true,
+        },
+    ];
+
+    let drain_weights = DrainWeights {
+        output_weight: 100,
+        spend_weight: 1_000,
+        n_outputs: 1,
+    };
+
+    let mut cs = CoinSelector::new(&candidates);
+    let metric = LowestFee {
+        target,
+        long_term_feerate,
+        change_policy: ChangePolicy::min_value_and_waste(
+            drain_weights,
+            50,
+            target_feerate,
+            long_term_feerate,
+        ),
+    };
+    let (_score, _rounds) = common::bnb_search(&mut cs, metric, 1000).expect("must find solution");
+}