-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace 'Drain' with 'Change' for clearer terminology #54
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
30063aa
Update FIFO selection to prioritize OutputGroups with None sequence
BEULAHEVANJALIN a817c21
Prioritize OutputGroups with sequence numbers
BEULAHEVANJALIN 0dbdc20
Fix Clippy warning by using instead of
BEULAHEVANJALIN 13ee339
Merge branch 'Bitshala-Incubator:main' into main
BEULAHEVANJALIN 91b8e90
Merge branch 'Bitshala-Incubator:main' into main
BEULAHEVANJALIN 23f0aaf
Check inputs with creation_sequence are sorted
BEULAHEVANJALIN 83e11b6
Merge branch 'Bitshala-Incubator:main' into main
BEULAHEVANJALIN 54f6905
Rename 'drain' to 'change' for output clarity
BEULAHEVANJALIN ef8429a
fix: applied review corrections
BEULAHEVANJALIN File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,20 +47,20 @@ 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, | ||
|
||
/// 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, | ||
|
@@ -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. | ||
|
@@ -279,7 +279,7 @@ pub fn select_coin_knapsack( | |
options: CoinSelectionOpt, | ||
) -> Result<SelectionOutput, SelectionError> { | ||
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<usize> = 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); | ||
|
@@ -667,12 +667,12 @@ 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #54 Please change this reference to ToDrain if options.excess_strategy != ExcessStrategy::ToDrain {
// 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 { |
||
} else { | ||
// Change is created if excess strategy is set to ToDrain. Hence 'excess' should be set to 0 | ||
waste += options.drain_cost; | ||
// Change is created if excess strategy is set to ToChange. Hence 'excess' should be set to 0 | ||
waste += options.change_cost; | ||
} | ||
waste | ||
} | ||
|
@@ -852,31 +852,31 @@ 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, | ||
excess_strategy: ExcessStrategy::ToDrain, | ||
min_change_value: 500, | ||
excess_strategy: ExcessStrategy::ToChange, | ||
} | ||
} | ||
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, | ||
excess_strategy: ExcessStrategy::ToDrain, | ||
min_change_value, | ||
excess_strategy: ExcessStrategy::ToChange, | ||
} | ||
} | ||
fn knapsack_setup_output_groups( | ||
|
@@ -927,12 +927,12 @@ 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, | ||
excess_strategy: ExcessStrategy::ToDrain, | ||
min_change_value: 500, | ||
excess_strategy: ExcessStrategy::ToChange, | ||
} | ||
} | ||
|
||
|
@@ -1328,12 +1328,12 @@ 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. | ||
excess_strategy: ExcessStrategy::ToDrain, | ||
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::ToChange, | ||
}; | ||
if let Ok(result) = select_coin_knapsack(&inputs, options) { | ||
// Chekcing if knapsack selects exactly 2 inputs | ||
|
@@ -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, | ||
}; | ||
|
||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#54
Please replace
ToDrain
in excess strategy withToChange
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.