-
Notifications
You must be signed in to change notification settings - Fork 12
Simplify coin selector and branch and bound #46
base: master
Are you sure you want to change the base?
Conversation
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.
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..
bdk_core/src/coin_select/bnb2.rs
Outdated
} | ||
|
||
fn consider_adding_to_queue(&mut self, cs: &CoinSelector<'a>, already_scored: bool) { | ||
if let Some(heuristic) = (self.score_fn)(cs, true) { |
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.
maybe heuristic_score
as the name??
bdk_core/src/coin_select/bnb2.rs
Outdated
|
||
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 { |
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.
I think we can also expect
here or a if let
maybe??
bdk_core/src/coin_select/bnb2.rs
Outdated
score_fn, | ||
}; | ||
|
||
iter.consider_adding_to_queue(selector, false); |
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.
Why don't we just score it every time we add a branch?
bdk_core/src/txout_ext.rs
Outdated
|
||
const TXOUT_BASE_WEIGHT: u32 = 4 * size_of::<u64>() as u32; // just the value | ||
|
||
pub trait TxOutExt { |
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.
TFW a wild trait appears!!! 😂😂
bdk_core/src/coin_select/bnb2.rs
Outdated
self.queue.push(Branch { | ||
heuristic_score: heuristic, | ||
selector: cs.clone(), | ||
already_scored, |
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.
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??
bdk_core/src/coin_select/bnb2.rs
Outdated
.take(1634) | ||
.filter_map(|(i, sol)| Some((i, sol?))) | ||
.last() | ||
.expect("it found a solution"); |
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.
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..
bdk_core/src/coin_select/bnb2.rs
Outdated
|
||
let (i, (best, _score)) = solutions | ||
.enumerate() | ||
.take(1634) |
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.
Is this just a big number, or has deeper meaning?
pub fn select(&mut self, index: usize) -> bool { | ||
assert!(index < self.candidates.len()); | ||
self.selected.to_mut().insert(index) |
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.
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.
Update: I've moved the coin selection to its own crate that only depends on 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 @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 Report
@@ 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.
CoinSelector
now only takes abase_weight
parameter. It doesn't store yourtarget_value
,feerate
etc. You can apply policies on these outside.CoinSelector
so that for calls tounselected
they will appear in that order without changing the original order.unselected
anymore.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.