Skip to content

Add waste metric for coin selection #435

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.

Sign up for GitHub

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

Closed
benthecarman wants to merge 1 commit into from
Closed

Add waste metric for coin selection #435

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 151 additions & 0 deletions src/wallet/coin_selection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,49 @@ impl BranchAndBoundCoinSelection {
}
}

struct WasteMetric;
Copy link
Copy Markdown
Member

@notmandatory notmandatory Sep 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good direction for improving coin selection, I haven't looked deeply into the code/algorithm but I do have a couple rust style suggestions.

  1. Unless you plan on adding additional fields to the WasteMetric struct I think you can do without it.
  2. And if the idea for calculate_waste is as a helper function to implementations of the CoinSelectionAlgorithm trait then it may be better to make calculate_waste a "default implementation" on the CoinSelectionAlgorithm trait.

If you take this approach then possibly CoinSelectionResult could include a waste metric field.

Copy link
Copy Markdown
Contributor Author

@benthecarman benthecarman Sep 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The waste metric requires a long term fee rate (expected average fee rate of the wallet) so idk if it can be directly be added to CoinSelectionResult.

Copy link
Copy Markdown
Contributor

@murchandamus murchandamus May 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, the "Long Term Feerate Estimate" (LTFRE) is not the average feerate on the wallet, it's more of a minimum feerate that you expect to be able to spend at in the future. The thinking is that if you expect to be able to spend an input at a lower feerate in the future, you should delay spending it to then, hence, anything above that feerate is considered "wasteful". Since you always need to use at least one input, two solutions that each use one input of the same type will see their score increase by the same amount, so it doesn't affect their prioritization. However, if you e.g. have different output types or compare solutions with multiple inputs, you'll prefer the smaller input weight per this heuristic.


impl WasteMetric {
fn calculate_waste(
selected: Vec<WeightedUtxo>,
cost_of_change: Option<u64>,
amount_needed: u64,
fee_rate: FeeRate,
long_term_fee_rate: FeeRate,
) -> i64 {
Comment on lines +539 to +546
Copy link
Copy Markdown
Contributor

@rajarshimaitra rajarshimaitra Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of returning i64 why not return just return Self? And have i64 or whatever as member of WasteMetric. If we are defining a struct anyway, lets use it. :)

In this way we can also use this inside CoinSelectionResult later. This seems to me like a very nice tool to compare different selection algorithm and we can expose a simple API to users with some default configs (like long term fee rate etc).

// Always consider the cost of spending an input now vs in the future.
let utxo_groups: Vec<OutputGroup> = selected
.into_iter()
.map(|u| OutputGroup::new(u, fee_rate))
.collect();

let waste: 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);

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 0
waste + change_cost as i64
}
None => {
// When we are not making change (change_cost == 0), consider the excess we are throwing away to fees
waste + selected_effective_value - amount_needed as i64
}
}
}
}

#[cfg(test)]
mod test {
use std::str::FromStr;
Expand Down Expand Up @@ -620,6 +663,26 @@ mod test {
vec![utxo; utxos_number]
}

fn generate_utxos_of_values(utxos_values: Vec<u64>) -> Vec<WeightedUtxo> {
utxos_values
.into_iter()
.map(|value| WeightedUtxo {
satisfaction_weight: P2WPKH_WITNESS_SIZE,
utxo: Utxo::Local(LocalUtxo {
outpoint: OutPoint::from_str(
"ebd9813ecebc57ff8f30797de7c205e3c7498ca950ea4341ee51a685ff2fa30a:0",
)
.unwrap(),
txout: TxOut {
value: value,
script_pubkey: Script::new(),
},
keychain: KeychainKind::External,
}),
})
.collect()
}

fn sum_random_utxos(mut rng: &mut StdRng, utxos: &mut Vec<WeightedUtxo>) -> u64 {
let utxos_picked_len = rng.gen_range(2, utxos.len() / 2);
utxos.shuffle(&mut rng);
Expand Down Expand Up @@ -1056,4 +1119,92 @@ mod test {
assert!(result.selected_amount() > target_amount);
assert_eq!(result.fee_amount, (50 + result.selected.len() * 68) as u64);
}

#[test]
fn test_calculate_waste1() {
let fee_rate = FeeRate::from_sat_per_vb(11.0);
let long_term_fee_rate = FeeRate::from_sat_per_vb(10.0);
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()
.map(|u| OutputGroup::new(u, fee_rate))
.collect();

let size_of_change = 31;
let cost_of_change: u64 = (size_of_change as f32 * fee_rate.as_sat_vb()) as u64;

let utxo_fee_diffs: Vec<u64> = utxos
.into_iter()
.map(|utxo| {
let fee_rate_diff = fee_rate - long_term_fee_rate;

let fee =
fee_rate_diff.fee_wu(TXIN_BASE_WEIGHT + utxo.weighted_utxo.satisfaction_weight);

fee
})
.collect();

let utxo_fee_diff: u64 = utxo_fee_diffs.iter().sum();

// Waste with change is the change cost and difference between fee and long term fee
let waste = WasteMetric::calculate_waste(
selected,
Some(cost_of_change),
amount_needed,
fee_rate,
long_term_fee_rate,
);

assert_eq!(waste, (utxo_fee_diff + cost_of_change) as i64);
}

#[test]
fn test_calculate_waste2() {
let fee_rate = FeeRate::from_sat_per_vb(11.0);
let long_term_fee_rate = FeeRate::from_sat_per_vb(10.0);
let utxo_values = vec![100_000_000, 200_000_000];
let selected = generate_utxos_of_values(utxo_values.clone());
let in_amt: u64 = utxo_values.iter().sum();
let amount_needed = 200_000_000;

let utxos: Vec<OutputGroup> = selected
.clone()
.into_iter()
.map(|u| OutputGroup::new(u, fee_rate))
.collect();

let utxos_fee: u64 = utxos
.clone()
.into_iter()
.map(|utxo| fee_rate.fee_wu(TXIN_BASE_WEIGHT + utxo.weighted_utxo.satisfaction_weight))
.collect::<Vec<u64>>()
.iter()
.sum();

let utxo_fee_diffs: Vec<u64> = utxos
.into_iter()
.map(|utxo| {
let fee_rate_diff = fee_rate - long_term_fee_rate;

let fee =
fee_rate_diff.fee_wu(TXIN_BASE_WEIGHT + utxo.weighted_utxo.satisfaction_weight);

fee
})
.collect();

let utxo_fee_diff: u64 = utxo_fee_diffs.iter().sum();

let excess = in_amt - utxos_fee - amount_needed;

// Waste without change is the excess and difference between fee and long term fee
let waste =
WasteMetric::calculate_waste(selected, None, 200_000_000, fee_rate, long_term_fee_rate);

assert_eq!(waste, (utxo_fee_diff + excess) as i64);
}
}