Simplify coin selector and branch and bound by LLFourn · Pull Request #46 · LLFourn/bdk_core_staging

LLFourn · 2022-10-18T09:26:58Z

CoinSelector now only takes a base_weight parameter. It doesn't store your target_value, feerate etc. You can apply policies on these outside.
You can sort the candidates inside CoinSelector so that for calls to unselected they will appear in that order without changing the original order.
You can ban candidates in the coin selector so that they won't appear in unselected anymore.
"branch and bound" is implemented generically 🎉 -- you can pass in your own scoring function. The scoring function is passed the current selection and a bool to indicate whether it should return the score of the current selection or the lower bound score for any child selection of the current one (i.e. the best it could do by adding more inputs). Note textbook branch and bound would have two separate functions but because of difficulties of rust closures they had to be one.
I show how to minimize waste metric on top of branch and bound implementation.

This is a WIP but I think that it's good enough for me to explore how (1) affects the API in BDK.

Early impressions appreciated.

rajarshimaitra

I just made a first pass over the new bnb code, and I won't pretend I understood it fully, but it does look structurally very different from the existing bnb or any other bnb codes I have seen so far..

I "kinda" get what its trying to do inside the BnbIter.. But I am happy that I am now finally motivated to go through Murch's thesis to weigh on this code.. 😅

One generic question I had.. Does a "generic branch and bound" means we can use the same algorithm to optimize for different "policies"?? If so, what kind of different policies are we talking about?? Is it (theoretically) possible to combine various existing CS algos into this generic model??

Will make another thorough pass after this..

Below are some more questions and comments..

rajarshimaitra · 2022-10-18T14:47:12Z

bdk_core/src/coin_select/bnb2.rs

+    }
+
+    fn consider_adding_to_queue(&mut self, cs: &CoinSelector<'a>, already_scored: bool) {
+        if let Some(heuristic) = (self.score_fn)(cs, true) {


maybe heuristic_score as the name??

rajarshimaitra · 2022-10-18T14:48:13Z

bdk_core/src/coin_select/bnb2.rs

+
+    fn consider_adding_to_queue(&mut self, cs: &CoinSelector<'a>, already_scored: bool) {
+        if let Some(heuristic) = (self.score_fn)(cs, true) {
+            if self.best.is_none() || self.best.as_ref().unwrap() > &heuristic {


I think we can also expect here or a if let maybe??

rajarshimaitra · 2022-10-18T14:53:43Z

bdk_core/src/coin_select/bnb2.rs

+            score_fn,
+        };
+
+        iter.consider_adding_to_queue(selector, false);


Why don't we just score it every time we add a branch?

rajarshimaitra · 2022-10-18T15:03:27Z

bdk_core/src/txout_ext.rs

+
+const TXOUT_BASE_WEIGHT: u32 = 4 * size_of::<u64>() as u32; // just the value
+
+pub trait TxOutExt {


TFW a wild trait appears!!! 😂😂

rajarshimaitra · 2022-10-18T17:35:23Z

bdk_core/src/coin_select/bnb2.rs

+                self.queue.push(Branch {
+                    heuristic_score: heuristic,
+                    selector: cs.clone(),
+                    already_scored,


For some time I was stuck on this thinking if its about "already calculated the score".. Instead its like "if this branch is already considered for best score".. Is that correct thinking??

rajarshimaitra · 2022-10-18T17:38:20Z

bdk_core/src/coin_select/bnb2.rs

+            .take(1634)
+            .filter_map(|(i, sol)| Some((i, sol?)))
+            .last()
+            .expect("it found a solution");


I am getting a

running 1 test thread 'coin_select::bnb2::test::finds_a_solution_in_n_iter' panicked at 'it found a solution', bdk_core/src/coin_select/bnb2.rs:198:14

error here.. Not sure if that's what it's suppose to do..

rajarshimaitra · 2022-10-18T17:49:12Z

bdk_core/src/coin_select/bnb2.rs

+
+        let (i, (best, _score)) = solutions
+            .enumerate()
+            .take(1634)


Is this just a big number, or has deeper meaning?

rajarshimaitra · 2022-10-18T18:01:39Z

bdk_core/src/coin_select/coin_selector.rs

+    pub fn select(&mut self, index: usize) -> bool {
+        assert!(index < self.candidates.len());
+        self.selected.to_mut().insert(index)


I don't know if I am reading wrong, but it seems the index here is the index of the item in the candidate list?? Where as the value of the stuff in that index is the actual index of the actual utxo (or whatever we are selecting)?? If so, Why keep so many indexes? Cant we just directly select the utxo index in the selected list?

Inspired by wikipedia description: https://en.wikipedia.org/wiki/Branch_and_bound

TODO add Weight type?

To put the individual coin selection metrics in. I'm not sure this actually makes sense after I've been through it.

(for now)

LLFourn · 2022-10-19T09:30:33Z

Update: I've moved the coin selection to its own crate that only depends on rust-bitcoin as requested in wizardsardine/liana#51 (comment). It's also easier like this to start putting it into bdk.

I've tried to design branch and bound algorithms so they can be composed e.g. if you want to find a "changeless" solution but then tiebreak by the waste metric i hoped you could easily create a scoring function that returns (has_change, waste_metric(..)). Since this is Ord it should "just work". However, this is subtly very tricky. That works for the score of a particular selection but not the heuristic (or the "bound"). How can we get the lower bound of the boolean indicating whether change was used? You could always return false which would work but it would be worse than if we created a combined heuristic targeted to do the job properly.

@rajarshimaitra thanks for the initial review. I'll try and respond to comments tomorrow.

I was missing some more incredibly subtle logic on both sides of the rate_diff branch.

codecov-commenter · 2022-11-14T04:47:15Z

Codecov Report

❗ No coverage uploaded for pull request base (master@6bcec82). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #46   +/-   ##
=========================================
  Coverage          ?   71.06%           
=========================================
  Files             ?        3           
  Lines             ?      318           
  Branches          ?        0           
=========================================
  Hits              ?      226           
  Misses            ?       92           
  Partials          ?        0

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

because we can and why not

It seems some cases are taking a while but still do find a solution.

LLFourn marked this pull request as draft October 18, 2022 09:27

rajarshimaitra reviewed Oct 18, 2022

View reviewed changes

LLFourn force-pushed the bnb3 branch from b7606fb to 15fc7dc Compare October 19, 2022 00:35

LLFourn added 6 commits October 19, 2022 16:59

[coin select] Add flexbile BnB algo

210fe3b

Inspired by wikipedia description: https://en.wikipedia.org/wiki/Branch_and_bound

[coin select] Simplify CoinSelector

fb1a90b

[coins select] Favor inclusion branch in BnB

f9a90cd

[coin select] Add iterator for candidates

9c43895

[coin select] Remove old bnb implementation

204c505

Add FeeRate type

1910f6c

TODO add Weight type?

LLFourn force-pushed the bnb3 branch from c114403 to 8fcec73 Compare October 19, 2022 09:17

LLFourn added 2 commits October 19, 2022 17:17

[cs] Add metrics module

673d1df

To put the individual coin selection metrics in. I'm not sure this actually makes sense after I've been through it.

[cs] Move coin selection into its own crate

600757d

(for now)

LLFourn force-pushed the bnb3 branch from 8fcec73 to 600757d Compare October 19, 2022 09:17

LLFourn added 3 commits October 21, 2022 15:54

[cs] Make waste metric work

92e8a77

[cs] Fix test assertion when solution not found

95a2fda

Add float wrappers that implement Ord

d201399

LLFourn force-pushed the bnb3 branch from 2bd44d0 to d201399 Compare October 21, 2022 07:56

LLFourn added 4 commits October 21, 2022 16:23

impl BnBMetric for Tuples

dc624c2

[cs] Make cs zero dependency crate

1232a3f

[cs] WIP further bugs found and fixed

6cd5a8d

I was missing some more incredibly subtle logic on both sides of the rate_diff branch.

[cs] All waste metric tests passing

8771d10

LLFourn force-pushed the bnb3 branch from b9aa19a to 8771d10 Compare November 14, 2022 04:41

LLFourn added 6 commits November 15, 2022 10:45

[cs] Remove select_while in favor of iterators

b9c68a1

Make feerate Ord

e98dc93

because we can and why not

[cs] rename to requires_ordering_by_descending_value_pwu

d9c047a

[cs] Move tests to /tests

23dffcc

[cs] Make the change policy actually set the value of the drain

262a433

[cs] Move waste metric to its own module

4adfd97

LLFourn added 4 commits November 15, 2022 17:22

[cs] fix bug in waste metric when rate_diff was low

ef8cf88

[cs] increase timeout on waste proptest

06515fc

It seems some cases are taking a while but still do find a solution.

[cs] better printing in tests

3dd8860

[cs] Add shorthand sort_candidates_by_descending_value_pwu

e0efc81

LLFourn force-pushed the bnb3 branch from 25b3f26 to e0efc81 Compare November 16, 2022 08:50

[cs] commit regressions

0331b05

LLFourn mentioned this pull request Nov 25, 2022

Implement transaction builder #40

Open

LLFourn mentioned this pull request Jan 10, 2023

Improve signature of CoinSelectorOpt::fund_outputs #33

Closed

LLFourn mentioned this pull request Mar 23, 2023

Introduce new bdk_coin_select implementation bitcoindevkit/bdk#924

Closed

evanlinjin mentioned this pull request Aug 9, 2023

Introduce new bdk_coin_select implementation bitcoindevkit/bdk#1072

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify coin selector and branch and bound#46

Simplify coin selector and branch and bound#46
LLFourn wants to merge 26 commits intomasterfrom
bnb3

LLFourn commented Oct 18, 2022

Uh oh!

rajarshimaitra left a comment

Uh oh!

rajarshimaitra Oct 18, 2022

Uh oh!

rajarshimaitra Oct 18, 2022

Uh oh!

rajarshimaitra Oct 18, 2022

Uh oh!

rajarshimaitra Oct 18, 2022

Uh oh!

rajarshimaitra Oct 18, 2022

Uh oh!

rajarshimaitra Oct 18, 2022

Uh oh!

rajarshimaitra Oct 18, 2022

Uh oh!

rajarshimaitra Oct 18, 2022

Uh oh!

LLFourn commented Oct 19, 2022

Uh oh!

codecov-commenter commented Nov 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		const TXOUT_BASE_WEIGHT: u32 = 4 * size_of::<u64>() as u32; // just the value

		pub trait TxOutExt {

Conversation

LLFourn commented Oct 18, 2022

Uh oh!

rajarshimaitra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LLFourn commented Oct 19, 2022

Uh oh!

codecov-commenter commented Nov 14, 2022

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants