Add `process_and_select_coins` function #727

csralvall · 2022-08-17T18:17:45Z

Description

There were some common tasks identified in the construction of a coin selection
result that the different algorithms performed separately. Those tasks were
blended inside the algorithms or separated into functions, but the underlying
actions were the same:

Do some preprocessing to the inputs.
Select utxos based in the algorithm criterium.
Decide change.
Build the coin selection result.

This commit intends to create a single function that consumes different
algorithms and perform all the mentioned tasks as a pipeline taking utxos as
input and producing coin selection results at the end.

The new function reduces the work needed to implement other coin selection
algorithms, modularizes the code not related directly to the selection process
and reduces the number of lines to review and understand its logic.

As a side effect of the implementation, the SingleRandomDraw algorithm was
extracted from its host algorithm, BranchAndBound, and introduced as a
FallBack algorithm. But I consider this a feature.

Notes to the reviewers

I'm not totally satisfied with some parts of the code.

For example, I don't like to have "ad-hoc" parameters in the current signature
of coin_select just for the sake of BranchAndBound compatibility. I thought
of some solutions to this, like the addition of a cost_of_change field into the
struct or the recomputation of the available value inside
BranchAndBound::coin_select (duplicating the amount of work), but I would
like to hear other opinions.

Another thing that bothers me is the conversion of target_amount from u64 to
i64 and the other way back. I must be obfuscated with it because I can't see
another way to do it.

This PR will set up the structure needed to integrate the Waste metric (#558)
with the coin selection module.

BREAKING CHANGES: This PR modifies the API of coin_select and introduce a new
function, get_selection, that supersedes its functionality.

Changelog notice

Added process_and_select_coins() function.
Added OutputGroups as a public structure that holds the WeightedUtxo, its
effective value, and its fees.
Changed CoinSelectionAlgorithm::coin_select() signature. The scope of the
selection was reduced to select utxos only from optional_utxos. It only
returns the selected OutputGroups from now on.
Added WeightedScriptPubkey struct.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've ~~added~~ updated the tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

murchandamus

I'm not sure how BDK prefers to structure its commits, but on Bitcoin Core, we like to separate renaming of things and relocating without functional changes from functional changes to separate commits, i.e. it might be easier to review if the relocation of SRD were a separate commit.

src/wallet/coin_selection.rs

csralvall · 2022-08-22T19:53:31Z

I'm not sure how BDK prefers to structure its commits, but on Bitcoin Core, we like to separate renaming of things and relocating without functional changes from functional changes to separate commits, i.e. it might be easier to review if the relocation of SRD were a separate commit.

I thought that squashing things was the way to proceed but I can split this in two commits or more.

danielabrozzoni

I gave a first quick pass, I'm not done reviewing :) (but I chose to publish the review so you can start working on these fixes!)

danielabrozzoni · 2022-08-27T09:25:53Z

src/wallet/mod.rs

+                    .max_satisfaction_weight()
+                    .unwrap()
+            } else {
+                0


This is wrong, you should use miniscript to get the max satisfaction weight here as well

Didn't we consider the drain_to script pubkey as something external? the satisfaction weight for the wallet is zero because it won't spend the created output.

Didn't we consider the drain_to script pubkey as something external?

Yeah

the satisfaction weight for the wallet is zero because it won't spend the created output.

I admit I'm a bit undecided on this... But I think we should still consider the satisfaction weight here. Even tho it's not us spending the output, someone still will have to, so I think it's fair to try to reduce their waste.

Ok, I've to find a way to compute the satisfaction_weight for an arbitrary Script, I've tried before without success.

How is this going? Do you need help?

I'm not advancing in figuring out how to solve this. I've tried to implement a solution using miniscript functions without success. Another idea, which I don't like, is to pass the foreign weight as a parameter of the transaction.
Also, I don't want to implement something to parse the script and compute the weight. It would be pointless because miniscript implements those functions. But there isn't an implementation for an arbitrary scriptpubkey.

src/wallet/coin_selection.rs

danielabrozzoni · 2022-08-28T17:36:12Z

src/wallet/coin_selection.rs

@@ -116,6 +87,9 @@ use std::convert::TryInto;
 pub type DefaultCoinSelectionAlgorithm = BranchAndBoundCoinSelection;
 #[cfg(test)]
 pub type DefaultCoinSelectionAlgorithm = LargestFirstCoinSelection; // make the tests more predictable
+/// Algorithm to use in case of an error in the chosen coin selection algorithm
+// If everything fails, simple random selection should work
+pub type FallbackCoinSelectionAlgorithm = SingleRandomDrawCoinSelection;


First, I agree with Murch that it would have been better to have this split in a different commit - but I understand it's not easy to see when to squash and when not to squash :) If you can, review would be much easier with two commits!

Second, this is a bit suboptimal: not every CS algorithm has a fallback algorithm! Instead, me and @afilini came up with this idea of having a FallbackWrapper algorithm struct (you should be able to copy-paste this code and it should already work):

diff --git a/src/wallet/coin_selection.rs b/src/wallet/coin_selection.rs index 49bafa9..ef6139b 100644 --- a/src/wallet/coin_selection.rs +++ b/src/wallet/coin_selection.rs @@ -84,12 +84,10 @@ use std::convert::TryInto; /// Default coin selection algorithm used by [`TxBuilder`](super::tx_builder::TxBuilder) if not /// overridden #[cfg(not(test))] -pub type DefaultCoinSelectionAlgorithm = BranchAndBoundCoinSelection; +pub type DefaultCoinSelectionAlgorithm = + FallbackWrapper<BranchAndBoundCoinSelection, SingleRandomDrawCoinSelection>; #[cfg(test)] pub type DefaultCoinSelectionAlgorithm = LargestFirstCoinSelection; // make the tests more predictable -/// Algorithm to use in case of an error in the chosen coin selection algorithm -// If everything fails, simple random selection should work -pub type FallbackCoinSelectionAlgorithm = SingleRandomDrawCoinSelection; // Base weight of a Txin, not counting the weight needed for satisfying it. // prev_txid (32 bytes) + prev_vout (4 bytes) + sequence (4 bytes) @@ -250,25 +248,13 @@ pub fn get_selection<D: Database, Cs: CoinSelectionAlgorithm<D>>( } else { // from now on, target_amount can only be positive let target_amount = (target_amount_i64 - required_effective_value) as u64; - let (mut selected_utxos, selected_fees) = match algorithm.coin_select( + let (mut selected_utxos, selected_fees) = algorithm.coin_select( database, optional_utxos.clone(), fee_rate, target_amount, optional_effective_value, - ) { - Ok(selection) => selection, - Err(_err) => { - let fallback_algorithm = FallbackCoinSelectionAlgorithm {}; - fallback_algorithm.coin_select( - database, - optional_utxos, - fee_rate, - target_amount, - optional_effective_value, - )? - } - }; + )?; let selected_effective_value: i64 = selected_utxos.iter().map(|og| og.effective_value).sum(); selected_utxos.append(&mut required_utxos); @@ -344,6 +330,59 @@ pub trait CoinSelectionAlgorithm<D: Database>: std::fmt::Debug { ) -> Result<(Vec<OutputGroup>, u64), Error>; } +#[derive(Debug)] +pub struct FallbackWrapper<M, F> { + main: M, + fallback: F, +} + +impl<M, F> Default for FallbackWrapper<M, F> +where + M: Default, + F: Default, +{ + fn default() -> Self { + FallbackWrapper { + main: M::default(), + fallback: F::default(), + } + } +} + +impl<M, F, D> CoinSelectionAlgorithm<D> for FallbackWrapper<M, F> +where + M: CoinSelectionAlgorithm<D>, + F: CoinSelectionAlgorithm<D>, + D: Database, +{ + fn coin_select( + &self, + database: &D, + optional_utxos: Vec<OutputGroup>, + fee_rate: FeeRate, + target_amount: u64, + available_value: i64, + ) -> Result<(Vec<OutputGroup>, u64), Error> { + self.main + .coin_select( + database, + optional_utxos.clone(), + fee_rate, + target_amount, + available_value, + ) + }) + } +} + /// Simple and dumb coin selection /// /// This coin selection algorithm sorts the available UTXOs by value and then picks them starting

This is basically introducing a FallbackWrapper, which you can build around your favorite CS algorithms. We implemented CoinSelectionAlgorithm on the wrapper, so that you just need to call coin_select, and we automatically take care of calling the fallback in case of errors.

I just noticed that calling BranchAndBounCoinSelection::default() is suboptimal (it's using a default size_of_change)... I'm working on finding a solution...

I may be missing something, but shouldn't be a call to self.callback.coin_select inside the impl of coin_select for FallbackWrapper?

Sorry, I probably copied it wrong 🤦🏻‍♀️ Yeah, it should be:

+impl<M, F, D> CoinSelectionAlgorithm<D> for FallbackWrapper<M, F> +where + M: CoinSelectionAlgorithm<D>, + F: CoinSelectionAlgorithm<D>, + D: Database, +{ + fn coin_select( + &self, + database: &D, + optional_utxos: Vec<OutputGroup>, + fee_rate: FeeRate, + target_amount: u64, + available_value: i64, + ) -> Result<(Vec<OutputGroup>, u64), Error> { + self.main + .coin_select( + database, + optional_utxos.clone(), + fee_rate, + target_amount, + available_value, + ) + .or_else(|_| { + self.fallback.coin_select( + database, + optional_utxos.clone(), + fee_rate, + target_amount, + available_value, + ) + }) + } +} +

I just noticed that calling BranchAndBounCoinSelection::default() is suboptimal (it's using a default size_of_change)... I'm working on finding a solution...

But the user can still pass algorithm as:

DefaultCoinSelectionAlgorithm { main: BranchAndBoundCoinCoinSelection::new(<user_set_size_of_change>), fallback: SingleRandomDrawCoinSelection::default() }

This default is not giving up the flexibility above.

I'll continue working with this in another PR.

src/wallet/coin_selection.rs

rajarshimaitra

Concept ACK..

I made a first shallow pass and for now just have few nit comments and name changing requests.. I will go through the full thing again once more..

rajarshimaitra · 2022-08-27T14:15:05Z

src/wallet/mod.rs

-use coin_selection::DefaultCoinSelectionAlgorithm;
+use coin_selection::{
+    get_selection, DefaultCoinSelectionAlgorithm,
+    Excess::{Change, NoChange},


Naming request: A name like HandleExcess could be more clearer here.. Its a simple enum, that specifies how to handle the change if any.

Not a major issue, a general rust street rule is not to import enum variants directly. Call them with the enum name in code like Excess::Change etc. The name of the enum gives useful context at the usage location. And its always just two item long.

Naming request: A name like HandleExcess could be more clearer here.. Its a simple enum, that specifies how to handle the change if any.

Well, I thought about that before and didn't like it because Excess is just a "thing" and not an action per se. The logic inside create_tx is the one that handles the Excess. That was my reasoning. If I didn't convince you, let me know and I will change it.

Not a major issue, a general rust street rule is not to import enum variants directly. Call them with the enum name in code like Excess::Change etc. The name of the enum gives useful context at the usage location. And its always just two item long.

Good rule! I'll apply it.

src/types.rs

src/wallet/coin_selection.rs

csralvall · 2022-08-30T01:26:51Z

Thanks, @xekyo, @danielabrozzoni, @rajarshimaitra, and @afilini for taking the time to review this. I've several changes ahead. Here is a list to keep you updated:

Split the addition of get_selection from changes to CoinSelectionAlgorithms (SingleRandomDraw).
Create FallbackWrapper (instead of FallbackCoinSelectionAlgorithm). ~~It will probably go in a separate commit together with the split of BranchAndBound.~~ It will included in a separate PR.
Address ~~all~~ the majority of nits.

src/wallet/coin_selection.rs

src/wallet/mod.rs

CHANGELOG.md

danielabrozzoni · 2022-09-22T11:11:10Z

src/wallet/mod.rs

+                    .max_satisfaction_weight()
+                    .unwrap()
+            } else {
+                0


How is this going? Do you need help?

There were some common tasks identified in the construction of a coin selection result that the different algorithms performed separately. Those tasks were blended inside the algorithms or separated into functions, but the underlying actions were the same: - Do some preprocessing to the inputs. - Select utxos based on the algorithm criterium. - Decide change. - Build the coin selection result. This commit intends to create a single function that consumes different algorithms and perform all the mentioned tasks as a pipeline taking utxos as input and producing coin selection results at the end. The new function reduces the work needed to implement other coin selection algorithms, modularizes the code not related directly to the selection process and reduces the number of lines to review and understand its logic.

danielabrozzoni · 2022-09-26T12:43:43Z

I don't think this will get into the milestone 0.23, I'm sorry :( I need some more time to review it and think about the API changes. This will introduce another API break in the coin_selection function, and I'd prefer to avoid further breakage in the future...

evanlinjin

This is an excellent undertaking of trying to streamline coin selection in BDK.

However, there seems to be an attempt of "not changing too much" (which is understandable), but the transaction building model in BDK is inconvenient (and potentially wrong) and should be reworked from scratch (in my opinion).

Let's discuss tomorrow over the call and come up with a "grand plan". Your insights will be very appreciated (based on my judgement of your work).

NACK (for now) 😢

danielabrozzoni · 2022-10-12T09:56:58Z

After discussing this with the team, we came to the conclusion that we have to close this PR, as the bdk_core update makes it obsolete.

Just a reminder: even though this exact code won't be used in bdk, the ideas discovered while writing/reviewing it surely will. Thanks for your work, Cesar :)

murchandamus reviewed Aug 19, 2022

View reviewed changes

src/wallet/coin_selection.rs Show resolved Hide resolved

src/wallet/coin_selection.rs Show resolved Hide resolved

danielabrozzoni added this to the Release 0.22.0 Feature Freeze milestone Aug 22, 2022

notmandatory assigned csralvall Aug 23, 2022

notmandatory mentioned this pull request Aug 25, 2022

W35 BDK Library Team Call bitcoindevkit/.github#20

Closed

27 tasks

danielabrozzoni mentioned this pull request Aug 26, 2022

The branch and bound coin selection should try to minimize the waste #733

Open

danielabrozzoni reviewed Aug 28, 2022

View reviewed changes

rajarshimaitra reviewed Aug 29, 2022

View reviewed changes

danielabrozzoni reviewed Aug 29, 2022

View reviewed changes

src/wallet/coin_selection.rs Outdated Show resolved Hide resolved

notmandatory modified the milestones: Release 0.22.0 Feature Freeze, Release 0.23.0 Feature Freeze Aug 30, 2022

evanlinjin reviewed Sep 5, 2022

View reviewed changes

src/wallet/coin_selection.rs Show resolved Hide resolved

evanlinjin reviewed Sep 5, 2022

View reviewed changes

src/wallet/mod.rs Outdated Show resolved Hide resolved

notmandatory mentioned this pull request Sep 13, 2022

W37 BDK Library Team Call bitcoindevkit/.github#22

Closed

26 tasks

csralvall force-pushed the create_coin_selection_pipeline branch 2 times, most recently from f584370 to 144807b Compare September 14, 2022 13:32

csralvall changed the title ~~Add get_selection function~~ Add process_and_select_coins function Sep 17, 2022

danielabrozzoni reviewed Sep 22, 2022

View reviewed changes

csralvall force-pushed the create_coin_selection_pipeline branch from 144807b to 20ad2a7 Compare September 23, 2022 23:24

danielabrozzoni modified the milestones: Release 0.23.0 Feature Freeze, Release 0.24.0 Feature Freeze Sep 26, 2022

evanlinjin reviewed Oct 4, 2022

View reviewed changes

notmandatory mentioned this pull request Oct 10, 2022

W41 BDK Library Team Call bitcoindevkit/.github#27

Closed

26 tasks

notmandatory removed this from the Release 0.24.0 Feature Freeze milestone Oct 11, 2022

danielabrozzoni closed this Oct 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `process_and_select_coins` function #727

Add `process_and_select_coins` function #727

csralvall commented Aug 17, 2022 •

edited

Loading

murchandamus left a comment •

edited

Loading

csralvall commented Aug 22, 2022

danielabrozzoni left a comment •

edited

Loading

danielabrozzoni Aug 27, 2022

csralvall Aug 30, 2022

danielabrozzoni Aug 30, 2022

csralvall Sep 2, 2022

danielabrozzoni Sep 22, 2022

csralvall Sep 22, 2022

danielabrozzoni Aug 28, 2022

danielabrozzoni Aug 29, 2022

csralvall Aug 30, 2022

danielabrozzoni Aug 30, 2022

csralvall Sep 13, 2022 •

edited

Loading

csralvall Sep 13, 2022

rajarshimaitra left a comment

rajarshimaitra Aug 27, 2022

csralvall Aug 30, 2022

csralvall commented Aug 30, 2022 •

edited

Loading

danielabrozzoni Sep 22, 2022

danielabrozzoni commented Sep 26, 2022

evanlinjin left a comment

danielabrozzoni commented Oct 12, 2022

Add process_and_select_coins function #727

Add process_and_select_coins function #727

Conversation

csralvall commented Aug 17, 2022 • edited Loading

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

murchandamus left a comment • edited Loading

Choose a reason for hiding this comment

csralvall commented Aug 22, 2022

danielabrozzoni left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csralvall Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajarshimaitra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csralvall commented Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

danielabrozzoni commented Sep 26, 2022

evanlinjin left a comment

Choose a reason for hiding this comment

danielabrozzoni commented Oct 12, 2022

Add `process_and_select_coins` function #727

Add `process_and_select_coins` function #727

csralvall commented Aug 17, 2022 •

edited

Loading

murchandamus left a comment •

edited

Loading

danielabrozzoni left a comment •

edited

Loading

csralvall Sep 13, 2022 •

edited

Loading

csralvall commented Aug 30, 2022 •

edited

Loading