Refactor calculate_waste inside Waste struct

Author: csralvall

src/wallet/coin_selection.rs

@@ -66,16 +66,13 @@
 //!             });
 //!         }
 //!
-//!         let calculated_waste = Some(
-//!             calculate_waste(
-//!                 &all_utxos_selected,
-//!                 None,
-//!                 amount_needed,
-//!                 fee_rate,
-//!                 FeeRate::from_sat_per_vb(10.0),
-//!             )
-//!             .unwrap(),
-//!         );
+//!         let calculated_waste = Waste::calculate(
+//!             &all_utxos_selected,
+//!             None,
+//!             amount_needed,
+//!             fee_rate,
+//!             FeeRate::from_sat_per_vb(10.0),
+//!         )?;
 //!
 //!         Ok(CoinSelectionResult::new(
 //!             all_utxos_selected.into_iter().map(|u| u.utxo).collect(),
@@ -109,7 +106,6 @@ use rand::seq::SliceRandom;
 use rand::thread_rng;
 #[cfg(test)]
 use rand::{rngs::StdRng, SeedableRng};
-use std::cell::Cell;
 use std::convert::TryInto;
 
 /// Default coin selection algorithm used by [`TxBuilder`](super::tx_builder::TxBuilder) if not
@@ -123,7 +119,6 @@ pub type DefaultCoinSelectionAlgorithm = LargestFirstCoinSelection; // make the
 // prev_txid (32 bytes) + prev_vout (4 bytes) + sequence (4 bytes) + script_len (1 bytes)
 pub(crate) const TXIN_BASE_WEIGHT: usize = (32 + 4 + 4 + 1) * 4;
 
-pub type Waste = i64;
 /// Result of a successful coin selection
 #[derive(Debug)]
 pub struct CoinSelectionResult {
@@ -132,16 +127,16 @@ pub struct CoinSelectionResult {
     /// Total fee amount in satoshi
     pub fee_amount: u64,
     /// Waste value of current coin selection
-    waste: Cell<Option<Waste>>,
+    pub waste: Waste,
 }
 
 impl CoinSelectionResult {
     /// Create new CoinSelectionResult
-    pub fn new(selected_utxos: Vec<Utxo>, fee_amount: u64, selection_waste: Option<Waste>) -> Self {
+    pub fn new(selected_utxos: Vec<Utxo>, fee_amount: u64, selection_waste: Waste) -> Self {
         CoinSelectionResult {
             selected: selected_utxos,
             fee_amount,
-            waste: Cell::new(selection_waste),
+            waste: selection_waste,
         }
     }
 
@@ -160,78 +155,59 @@ impl CoinSelectionResult {
             })
             .sum()
     }
+}
 
-    pub fn get_waste(
-        &self,
+#[derive(Debug, PartialEq, Eq, PartialOrd, Ord)]
+pub struct Waste(pub i64);
+
+impl Waste {
+    pub fn calculate(
         selected: &[WeightedUtxo],
         cost_of_change: Option<u64>,
         target: u64,
         fee_rate: FeeRate,
+        // NOTE: The value of long_term_fee_rate should be set to 10sat/vb.
         long_term_fee_rate: FeeRate,
-    ) -> Waste {
-        match self.waste.get() {
-            Some(waste) => waste,
-            None => {
-                let calculated_waste = calculate_waste(
-                    selected,
-                    cost_of_change,
-                    target,
-                    fee_rate,
-                    long_term_fee_rate,
-                )
-                .unwrap();
-                self.waste.set(Some(calculated_waste));
-                calculated_waste
-            }
-        }
-    }
-}
+    ) -> Result<Waste, Error> {
+        // Always consider the cost of spending an input now vs in the future.
+        let utxo_groups: Vec<_> = selected
+            .iter()
+            .map(|u| OutputGroup::new(u, fee_rate))
+            .collect();
+
+        // If fee_rate < long_term_fee_rate timing cost can be negative
+        let timing_cost: i64 = utxo_groups.iter().fold(0, |acc, utxo| {
+            let fee: i64 = utxo.fee as i64;
+            let long_term_fee: i64 = long_term_fee_rate
+                .fee_wu(TXIN_BASE_WEIGHT + utxo.weighted_utxo.satisfaction_weight)
+                as i64;
 
-pub fn calculate_waste(
-    selected: &[WeightedUtxo],
-    cost_of_change: Option<u64>,
-    target: u64,
-    fee_rate: FeeRate,
-    long_term_fee_rate: FeeRate,
-) -> Result<Waste, Error> {
-    // Always consider the cost of spending an input now vs in the future.
-    let utxo_groups: Vec<_> = selected
-        .iter()
-        .map(|u| OutputGroup::new(u, fee_rate))
-        .collect();
-
-    // If fee_rate < long_term_fee_rate timing cost can be negative
-    let timing_cost: i64 = utxo_groups.iter().fold(0, |acc, utxo| {
-        let fee: i64 = utxo.fee as i64;
-        let long_term_fee: i64 = long_term_fee_rate
-            .fee_wu(TXIN_BASE_WEIGHT + utxo.weighted_utxo.satisfaction_weight)
-            as i64;
-
-        acc + fee - long_term_fee
-    });
-
-    let selected_effective_value: i64 = utxo_groups
-        .iter()
-        .fold(0, |acc, utxo| acc + utxo.effective_value);
-
-    let creation_cost = match cost_of_change {
-        Some(change_cost) => {
-            // Consider the cost of making change and spending it in the future
-            // If we aren't making change, the caller should've set change_cost to None
-            if change_cost == 0 {
-                return Err(Error::Generic(
-                    "if there is not cost_of_change, set to None, not zero".to_string(),
-                ));
+            acc + fee - long_term_fee
+        });
+
+        let selected_effective_value: i64 = utxo_groups
+            .iter()
+            .fold(0, |acc, utxo| acc + utxo.effective_value);
+
+        let creation_cost = match cost_of_change {
+            Some(change_cost) => {
+                // Consider the cost of making change and spending it in the future
+                // If we aren't making change, the caller should've set cost_of_change to None
+                if change_cost == 0 {
+                    return Err(Error::Generic(
+                        "if there is not cost_of_change, set to None, not zero".to_string(),
+                    ));
+                }
+                change_cost as i64
             }
-            change_cost as i64
-        }
-        None => {
-            // When we are not making change, consider the excess we are throwing away to fees
-            selected_effective_value - target as i64
-        }
-    };
+            None => {
+                // When we are not making change, consider the excess we are throwing away to fees
+                selected_effective_value - target as i64
+            }
+        };
 
-    Ok(timing_cost + creation_cost)
+        Ok(Waste(timing_cost + creation_cost))
+    }
 }
 
 /// Trait for generalized coin selection algorithms
@@ -332,7 +308,8 @@ impl<D: Database> CoinSelectionAlgorithm<D> for LargestFirstCoinSelection {
             });
         }
 
-        Ok(CoinSelectionResult::new(selected, fee_amount, None))
+        // TODO: Calculate waste metric for selected coins
+        Ok(CoinSelectionResult::new(selected, fee_amount, Waste(0)))
     }
 }
 
@@ -621,7 +598,8 @@ impl BranchAndBoundCoinSelection {
             .map(|u| u.weighted_utxo.utxo.clone())
             .collect::<Vec<_>>();
 
-        CoinSelectionResult::new(selected, fee_amount, None)
+        // TODO: Calculate waste metric for selected coins
+        CoinSelectionResult::new(selected, fee_amount, Waste(0))
     }
 }
 
@@ -1026,7 +1004,8 @@ mod test {
     #[should_panic(expected = "BnBNoExactMatch")]
     fn test_bnb_function_no_exact_match() {
         let fee_rate = FeeRate::from_sat_per_vb(10.0);
-        let utxos: Vec<OutputGroup<'_>> = get_test_utxos()
+        let test_utxos = get_test_utxos();
+        let utxos: Vec<OutputGroup<'_>> = test_utxos
             .iter()
             .map(|u| OutputGroup::new(u, fee_rate))
             .collect();
@@ -1052,7 +1031,8 @@ mod test {
     #[should_panic(expected = "BnBTotalTriesExceeded")]
     fn test_bnb_function_tries_exceeded() {
         let fee_rate = FeeRate::from_sat_per_vb(10.0);
-        let utxos: Vec<OutputGroup> = generate_same_value_utxos(100_000, 100_000)
+        let same_value_utxos = generate_same_value_utxos(100_000, 100_000);
+        let utxos: Vec<OutputGroup> = same_value_utxos
             .iter()
             .map(|u| OutputGroup::new(u, fee_rate))
             .collect();
@@ -1082,7 +1062,8 @@ mod test {
         let size_of_change = 31;
         let cost_of_change = size_of_change as f32 * fee_rate.as_sat_vb();
 
-        let utxos: Vec<_> = generate_same_value_utxos(50_000, 10)
+        let same_value_utxos = generate_same_value_utxos(50_000, 10);
+        let utxos: Vec<_> = same_value_utxos
             .iter()
             .map(|u| OutputGroup::new(u, fee_rate))
             .collect();
@@ -1118,7 +1099,8 @@ mod test {
         let fee_rate = FeeRate::from_sat_per_vb(0.0);
 
         for _ in 0..200 {
-            let optional_utxos: Vec<_> = generate_random_utxos(&mut rng, 40)
+            let random_utxos = generate_random_utxos(&mut rng, 40);
+            let optional_utxos: Vec<_> = random_utxos
                 .iter()
                 .map(|u| OutputGroup::new(u, fee_rate))
                 .collect();
@@ -1155,8 +1137,8 @@ mod test {
         let target_amount = sum_random_utxos(&mut rng, &mut utxos);
 
         let fee_rate = FeeRate::from_sat_per_vb(1.0);
-        let utxos: Vec<OutputGroup> = utxos
-            .into_iter()
+        let utxos: Vec<OutputGroup<'_>> = utxos
+            .iter()
             .map(|u| OutputGroup::new(u, fee_rate))
             .collect();
 
@@ -1179,9 +1161,8 @@ mod test {
         let selected = generate_utxos_of_values(vec![100_000_000, 200_000_000]);
         let amount_needed = 200_000_000;
 
-        let utxos: Vec<OutputGroup> = selected
-            .clone()
-            .into_iter()
+        let utxos: Vec<OutputGroup<'_>> = selected
+            .iter()
             .map(|u| OutputGroup::new(u, fee_rate))
             .collect();
 
@@ -1198,15 +1179,15 @@ mod test {
         });
 
         // Waste with change is the change cost and difference between fee and long term fee
-        let waste = calculate_waste(
-            selected,
+        let waste = Waste::calculate(
+            &selected,
             Some(cost_of_change),
             amount_needed,
             fee_rate,
             long_term_fee_rate,
         );
 
-        assert_eq!(waste.unwrap(), (utxo_fee_diff + cost_of_change as i64));
+        assert_eq!(waste.unwrap(), Waste(utxo_fee_diff + cost_of_change as i64));
     }
 
     #[test]
@@ -1218,9 +1199,8 @@ mod test {
         let available_value: u64 = utxo_values.iter().sum();
         let amount_needed = 200_000_000;
 
-        let utxos: Vec<OutputGroup> = selected
-            .clone()
-            .into_iter()
+        let utxos: Vec<OutputGroup<'_>> = selected
+            .iter()
             .map(|u| OutputGroup::new(u, fee_rate))
             .collect();
 
@@ -1238,9 +1218,9 @@ mod test {
         let excess = available_value - utxos_fee - amount_needed;
 
         // Waste without change is the excess and difference between fee and long term fee
-        let waste = calculate_waste(selected, None, amount_needed, fee_rate, long_term_fee_rate);
+        let waste = Waste::calculate(&selected, None, amount_needed, fee_rate, long_term_fee_rate);
 
-        assert_eq!(waste.unwrap(), (utxo_fee_diff + excess as i64));
+        assert_eq!(waste.unwrap(), Waste(utxo_fee_diff + excess as i64));
     }
 
     #[test]
@@ -1252,8 +1232,8 @@ mod test {
         let amount_needed = 200_000_000;
 
         // Waste without change is the excess and difference between fee and long term fee
-        let waste = calculate_waste(
-            selected,
+        let waste = Waste::calculate(
+            &selected,
             Some(0),
             amount_needed,
             fee_rate,
@@ -1283,9 +1263,8 @@ mod test {
         let available_value: u64 = utxo_values.iter().sum();
         let amount_needed = 200_000_000;
 
-        let utxos: Vec<OutputGroup> = selected
-            .clone()
-            .into_iter()
+        let utxos: Vec<OutputGroup<'_>> = selected
+            .iter()
             .map(|u| OutputGroup::new(u, fee_rate))
             .collect();
 
@@ -1303,9 +1282,9 @@ mod test {
         let excess = available_value - utxos_fee - amount_needed;
 
         // Waste without change is the excess and difference between fee and long term fee
-        let waste = calculate_waste(selected, None, amount_needed, fee_rate, long_term_fee_rate);
+        let waste = Waste::calculate(&selected, None, amount_needed, fee_rate, long_term_fee_rate);
 
-        assert_eq!(waste.unwrap(), (utxo_fee_diff + excess as i64));
+        assert_eq!(waste.unwrap(), Waste(utxo_fee_diff + excess as i64));
     }
 
     #[test]
@@ -1321,10 +1300,10 @@ mod test {
         let amount_needed = utxo_values[0] - utxos_fee;
 
         // Waste without change is the excess and difference between fee and long term fee
-        let waste = calculate_waste(selected, None, amount_needed, fee_rate, long_term_fee_rate);
+        let waste = Waste::calculate(&selected, None, amount_needed, fee_rate, long_term_fee_rate);
 
         // There shouldn't be any waste if there is no
         // timing_cost nor creation_cost
-        assert_eq!(waste.unwrap(), 0);
+        assert_eq!(waste.unwrap(), Waste(0));
     }
 }