From 30063aa3ca5f0a014ef2cef29938f3965d651429 Mon Sep 17 00:00:00 2001 From: BEULAHEVANJALIN Date: Wed, 28 Aug 2024 17:06:25 +0530 Subject: [PATCH 1/6] Update FIFO selection to prioritize OutputGroups with None sequence --- src/lib.rs | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index f428bfd..27a7d1a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -306,10 +306,23 @@ pub fn select_coin_fifo( let mut estimated_fees: u64 = 0; // Sorting the inputs vector based on creation_sequence + let mut inputs_with_sequence: Vec<_> = inputs + .iter() + .enumerate() + .filter(|(_, og)| og.creation_sequence.is_some()) + .collect(); - let mut sorted_inputs: Vec<_> = inputs.iter().enumerate().collect(); + let mut inputs_without_sequence: Vec<_> = inputs + .iter() + .enumerate() + .filter(|(_, og)| og.creation_sequence.is_none()) + .collect(); + + inputs_with_sequence.sort_by_key(|(_, a)| a.creation_sequence); - sorted_inputs.sort_by_key(|(_, a)| a.creation_sequence); + let mut sorted_inputs = Vec::new(); + sorted_inputs.append(&mut inputs_without_sequence); + sorted_inputs.append(&mut inputs_with_sequence); for (index, inputs) in sorted_inputs { estimated_fees = calculate_fee(accumulated_weight, options.target_feerate); @@ -579,6 +592,13 @@ mod test { is_segwit: false, creation_sequence: Some(1001), }, + OutputGroup { + value: 1500, + weight: 150, + input_count: 1, + is_segwit: false, + creation_sequence: None, + }, ] } fn setup_lowestlarger_output_groups() -> Vec { From a817c210ee5a91cb2722f12d64ba592e572ae49b Mon Sep 17 00:00:00 2001 From: BEULAHEVANJALIN Date: Sun, 1 Sep 2024 15:36:58 +0530 Subject: [PATCH 2/6] Prioritize OutputGroups with sequence numbers --- src/lib.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 27a7d1a..248fe1d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -306,7 +306,7 @@ pub fn select_coin_fifo( let mut estimated_fees: u64 = 0; // Sorting the inputs vector based on creation_sequence - let mut inputs_with_sequence: Vec<_> = inputs + let mut sorted_inputs: Vec<_> = inputs .iter() .enumerate() .filter(|(_, og)| og.creation_sequence.is_some()) @@ -318,11 +318,7 @@ pub fn select_coin_fifo( .filter(|(_, og)| og.creation_sequence.is_none()) .collect(); - inputs_with_sequence.sort_by_key(|(_, a)| a.creation_sequence); - - let mut sorted_inputs = Vec::new(); - sorted_inputs.append(&mut inputs_without_sequence); - sorted_inputs.append(&mut inputs_with_sequence); + sorted_inputs.extend(inputs_without_sequence); for (index, inputs) in sorted_inputs { estimated_fees = calculate_fee(accumulated_weight, options.target_feerate); From 0dbdc202a2ffa51af4b15f4b039ed7e5487f96f1 Mon Sep 17 00:00:00 2001 From: BEULAHEVANJALIN Date: Sun, 1 Sep 2024 15:41:36 +0530 Subject: [PATCH 3/6] Fix Clippy warning by using instead of --- src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 248fe1d..988c57c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1144,10 +1144,10 @@ mod test { let mut selected_input_2: Vec = Vec::new(); for j in 0..RUN_TESTS { if let Ok(result) = select_coin_knapsack(&inputs, options) { - selected_input_1 = result.selected_inputs.clone(); + selected_input_1.clone_from(&result.selected_inputs); } if let Ok(result) = select_coin_knapsack(&inputs, options) { - selected_input_2 = result.selected_inputs.clone(); + selected_input_2.clone_from(&result.selected_inputs); } // Checking if the selected inputs, in two consequtive calls of the knapsack function are not the same assert_ne!(selected_input_1, selected_input_2); @@ -1175,10 +1175,10 @@ mod test { for k in 0..RUN_TESTS { for l in 0..RANDOM_REPEATS { if let Ok(result) = select_coin_knapsack(&inputs, options) { - selected_input_1 = result.selected_inputs.clone(); + selected_input_1.clone_from(&result.selected_inputs); } if let Ok(result) = select_coin_knapsack(&inputs, options) { - selected_input_2 = result.selected_inputs.clone(); + selected_input_2.clone_from(&result.selected_inputs); } if selected_input_1 == selected_input_2 { fails += 1; From 23f0aaf4543ef556f61cff48aa7e7277de6c6710 Mon Sep 17 00:00:00 2001 From: BEULAHEVANJALIN Date: Mon, 9 Sep 2024 21:29:34 +0530 Subject: [PATCH 4/6] Check inputs with creation_sequence are sorted --- src/lib.rs | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index 6f0cb98..1039d8f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -316,6 +316,8 @@ pub fn select_coin_fifo( .filter(|(_, og)| og.creation_sequence.is_some()) .collect(); + sorted_inputs.sort_by(|a, b| a.1.creation_sequence.cmp(&b.1.creation_sequence)); + let mut inputs_without_sequence: Vec<_> = inputs .iter() .enumerate() @@ -1213,6 +1215,72 @@ mod test { test_insufficient_funds(); } + #[test] + fn test_fifo_with_none_and_sequence() { + let inputs = vec![ + OutputGroup { + value: 1000, + weight: 100, + input_count: 1, + is_segwit: false, + creation_sequence: Some(1), + }, + OutputGroup { + value: 2000, + weight: 200, + input_count: 1, + is_segwit: false, + creation_sequence: None, // No sequence + }, + OutputGroup { + value: 3000, + weight: 300, + input_count: 1, + is_segwit: false, + creation_sequence: Some(0), // Oldest UTXO + }, + OutputGroup { + value: 1500, + weight: 150, + input_count: 1, + is_segwit: false, + creation_sequence: None, // No sequence + }, + OutputGroup { + value: 2500, + weight: 250, + input_count: 1, + is_segwit: false, + creation_sequence: Some(5), // Newer UTXO + }, + ]; + + let options = CoinSelectionOpt { + target_value: 4500, + target_feerate: 0.1, + long_term_feerate: None, + min_absolute_fee: 10, + base_weight: 100, + drain_weight: 50, + drain_cost: 0, + cost_per_input: 10, + cost_per_output: 5, + min_drain_value: 0, + excess_strategy: ExcessStrategy::ToFee, + }; + + let result = select_coin_fifo(&inputs, options); + + assert!(result.is_ok()); + + let selection_output = result.unwrap(); + // It should prioritize the ones with sequences: (3000, 1000, 2500) and then fall back on inputs without sequences + assert_eq!(selection_output.selected_inputs.len(), 3); // Should select 3 inputs + assert_eq!(selection_output.selected_inputs, vec![2, 0, 4]); // These are inputs with sequences + + assert!(selection_output.waste.0 < 10000); // Check waste does not exceed an arbitrary limit. + } + #[test] fn test_lowestlarger_successful() { let mut inputs = setup_lowestlarger_output_groups(); From 54f6905959a14eea50cbf456ea6f1613bcdbaf81 Mon Sep 17 00:00:00 2001 From: BEULAHEVANJALIN Date: Tue, 1 Oct 2024 12:53:49 +0530 Subject: [PATCH 5/6] Rename 'drain' to 'change' for output clarity --- src/lib.rs | 62 +++++++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 538634e..ce657f0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -47,11 +47,11 @@ pub struct CoinSelectionOpt { /// The weight of the template transaction, including fixed fields and outputs. pub base_weight: u32, - /// Additional weight if we include the drain (change) output. - pub drain_weight: u32, + /// Additional weight if we include the change output. + pub change_weight: u32, - /// Weight of spending the drain (change) output in the future. - pub drain_cost: u64, + /// Weight of spending the change output in the future. + pub change_cost: u64, /// Estimate of cost of spending an input. pub cost_per_input: u64, @@ -59,8 +59,8 @@ pub struct CoinSelectionOpt { /// Estimate of cost of spending the output. pub cost_per_output: u64, - /// Minimum value allowed for a drain (change) output. - pub min_drain_value: u64, + /// Minimum value allowed for a change output. + pub min_change_value: u64, /// Strategy to use the excess value other than fee and target pub excess_strategy: ExcessStrategy, @@ -279,7 +279,7 @@ pub fn select_coin_knapsack( options: CoinSelectionOpt, ) -> Result { let mut adjusted_target = options.target_value - + options.min_drain_value + + options.min_change_value + calculate_fee(options.base_weight, options.target_feerate); let mut smaller_coins = inputs .iter() @@ -397,7 +397,7 @@ pub fn select_coin_lowestlarger( let mut accumulated_weight: u32 = 0; let mut selected_inputs: Vec = Vec::new(); let mut estimated_fees: u64 = 0; - let target = options.target_value + options.min_drain_value; + let target = options.target_value + options.min_change_value; let mut sorted_inputs: Vec<_> = inputs.iter().enumerate().collect(); sorted_inputs.sort_by_key(|(_, input)| effective_value(input, options.target_feerate)); @@ -482,7 +482,7 @@ pub fn select_coin_fifo( if accumulated_value >= (options.target_value + estimated_fees.max(options.min_absolute_fee) - + options.min_drain_value) + + options.min_change_value) { break; } @@ -493,7 +493,7 @@ pub fn select_coin_fifo( if accumulated_value < (options.target_value + estimated_fees.max(options.min_absolute_fee) - + options.min_drain_value) + + options.min_change_value) { Err(SelectionError::InsufficientFunds) } else { @@ -537,7 +537,7 @@ pub fn select_coin_srd( let mut input_counts = 0; let necessary_target = options.target_value - + options.min_drain_value + + options.min_change_value + calculate_fee(options.base_weight, options.target_feerate); for (index, input) in randomized_inputs { @@ -550,7 +550,7 @@ pub fn select_coin_srd( if accumulated_value >= options.target_value - + options.min_drain_value + + options.min_change_value + estimated_fee.max(options.min_absolute_fee) { break; @@ -559,7 +559,7 @@ pub fn select_coin_srd( if accumulated_value < options.target_value - + options.min_drain_value + + options.min_change_value + estimated_fee.max(options.min_absolute_fee) { return Err(SelectionError::InsufficientFunds); @@ -672,7 +672,7 @@ fn calculate_waste( waste += (accumulated_value - (options.target_value + estimated_fee)); } else { // Change is created if excess strategy is set to ToDrain. Hence 'excess' should be set to 0 - waste += options.drain_cost; + waste += options.change_cost; } waste } @@ -852,30 +852,30 @@ mod test { long_term_feerate: Some(0.4), min_absolute_fee: 0, base_weight: 10, - drain_weight: 50, - drain_cost: 10, + change_weight: 50, + change_cost: 10, cost_per_input: 20, cost_per_output: 10, - min_drain_value: 500, + min_change_value: 500, excess_strategy: ExcessStrategy::ToDrain, } } fn knapsack_setup_options(adjusted_target: u64, target_feerate: f32) -> CoinSelectionOpt { - let min_drain_value = 500; + let min_change_value = 500; let base_weight = 10; let target_value = - adjusted_target - min_drain_value - calculate_fee(base_weight, target_feerate); + adjusted_target - min_change_value - calculate_fee(base_weight, target_feerate); CoinSelectionOpt { target_value, target_feerate, // Simplified feerate long_term_feerate: Some(0.4), min_absolute_fee: 0, base_weight, - drain_weight: 50, - drain_cost: 10, + change_weight: 50, + change_cost: 10, cost_per_input: 20, cost_per_output: 10, - min_drain_value, + min_change_value, excess_strategy: ExcessStrategy::ToDrain, } } @@ -927,11 +927,11 @@ mod test { long_term_feerate: None, min_absolute_fee: 0, base_weight: 10, - drain_weight: 50, - drain_cost: 10, + change_weight: 50, + change_cost: 10, cost_per_input: 20, cost_per_output: 10, - min_drain_value: 500, + min_change_value: 500, excess_strategy: ExcessStrategy::ToDrain, } } @@ -1328,11 +1328,11 @@ mod test { long_term_feerate: Some(0.4), min_absolute_fee: 0, base_weight: 10, - drain_weight: 50, - drain_cost: 10, + change_weight: 50, + change_cost: 10, cost_per_input: 20, cost_per_output: 10, - min_drain_value: (0.05 * CENT).round() as u64, // Setting minimum drain value = 0.05 CENT. This will make the algorithm to avoid creating small change. + min_change_value: (0.05 * CENT).round() as u64, // Setting minimum change value = 0.05 CENT. This will make the algorithm to avoid creating small change. excess_strategy: ExcessStrategy::ToDrain, }; if let Ok(result) = select_coin_knapsack(&inputs, options) { @@ -1520,11 +1520,11 @@ mod test { long_term_feerate: None, min_absolute_fee: 10, base_weight: 100, - drain_weight: 50, - drain_cost: 0, + change_weight: 50, + change_cost: 0, cost_per_input: 10, cost_per_output: 5, - min_drain_value: 0, + min_change_value: 0, excess_strategy: ExcessStrategy::ToFee, }; From ef8429ad4a5fdf664d9be99dc93a26374a48fb08 Mon Sep 17 00:00:00 2001 From: BEULAHEVANJALIN Date: Mon, 7 Oct 2024 19:23:52 +0530 Subject: [PATCH 6/6] fix: applied review corrections --- src/lib.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index ce657f0..b8fc4da 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -71,7 +71,7 @@ pub struct CoinSelectionOpt { pub enum ExcessStrategy { ToFee, ToRecipient, - ToDrain, + ToChange, } /// Error Describing failure of a selection attempt, on any subset of inputs. @@ -667,11 +667,11 @@ fn calculate_waste( waste = (accumulated_weight as f32 * (options.target_feerate - long_term_feerate)).ceil() as u64; } - if options.excess_strategy != ExcessStrategy::ToDrain { + if options.excess_strategy != ExcessStrategy::ToChange { // Change is not created if excess strategy is ToFee or ToRecipient. Hence cost of change is added waste += (accumulated_value - (options.target_value + estimated_fee)); } else { - // Change is created if excess strategy is set to ToDrain. Hence 'excess' should be set to 0 + // Change is created if excess strategy is set to ToChange. Hence 'excess' should be set to 0 waste += options.change_cost; } waste @@ -857,7 +857,7 @@ mod test { cost_per_input: 20, cost_per_output: 10, min_change_value: 500, - excess_strategy: ExcessStrategy::ToDrain, + excess_strategy: ExcessStrategy::ToChange, } } fn knapsack_setup_options(adjusted_target: u64, target_feerate: f32) -> CoinSelectionOpt { @@ -876,7 +876,7 @@ mod test { cost_per_input: 20, cost_per_output: 10, min_change_value, - excess_strategy: ExcessStrategy::ToDrain, + excess_strategy: ExcessStrategy::ToChange, } } fn knapsack_setup_output_groups( @@ -932,7 +932,7 @@ mod test { cost_per_input: 20, cost_per_output: 10, min_change_value: 500, - excess_strategy: ExcessStrategy::ToDrain, + excess_strategy: ExcessStrategy::ToChange, } } @@ -1333,7 +1333,7 @@ mod test { cost_per_input: 20, cost_per_output: 10, min_change_value: (0.05 * CENT).round() as u64, // Setting minimum change value = 0.05 CENT. This will make the algorithm to avoid creating small change. - excess_strategy: ExcessStrategy::ToDrain, + excess_strategy: ExcessStrategy::ToChange, }; if let Ok(result) = select_coin_knapsack(&inputs, options) { // Chekcing if knapsack selects exactly 2 inputs