[wallet] Add branch and bound coin selection by danielabrozzoni · Pull Request #148 · bitcoindevkit/bdk

danielabrozzoni · 2020-10-27T10:05:26Z

Fixes #48

murchandamus

This is coming along nicely! I think the tests with two UTXO are a bit too easy to actually uncover some of the issues around Branch and Bound. I would like to suggest that you increase the available UTXO at least into the range of 10 to see some interesting behaviors. Also, since the target window is feerate dependent, you might want to add a couple more tests with hier feerates.

Personally, I would prefer if a PR this large was composed of multiple commits. A few spots could use a bit more explanation, I think it may be hard to follow along in a few spots if one isn't as familiar with the intended outcome as myself.

murchandamus · 2020-10-27T20:47:48Z

+
+// Calculated as:
+// prev_txid (32 bytes) + prev_vout (4 bytes) + script_len (1 bytes) + script len (0 bytes) + sequence (4 bytes)
+const TXIN_DEFAULT_WEIGHT: usize = 32 + 4 + 1 + 4;


From what I read this only supports native segwit inputs. Is that the intention?

Not exactly. This should be the weight needed to build the txin without the satisfaction weight, i.e., should be standard for every input. One thing this comment made me notice is that I shouldn't add script_len here, as that's part of the satisfaction_weight (and it's currently counted in every other CS implemented, I'll fix this in a separate commit) :)

murchandamus · 2020-10-27T22:15:35Z

+                while let Some(false) = current_selection.last() {
+                    current_selection.pop();
+                    curr_available_value +=
+                        may_use_utxos[current_selection.len()].effective_value as u64;


Is the current_selection.len() right here? If you are exploring a branch that has omitted multiple UTXO, I don't think the selection's length needs to match the position in the available UTXO anymore.

current_selection is a vector of booleans, and current_selection[i] contains true if we're using may_use_utxos[i], false otherwise. (This is not very idiomatic Rust, I'm currently looking at a way to refactor it).
So current_selection.len() is always <= than may_use_utxos.len(), but current_selection[i] matches may_use_utxos[i] for i in 0..current_selection.len()
For example:

current_selection: [true, false, false] may_use_utxos (only the effective_values): [1, 2, 3, 4, 5] if you current_selection.pop() you are removing current_selection[2], and current_selection.len() becomes 2; at this point you have to add may_use_utxos[2]

murchandamus · 2020-10-27T22:44:39Z

+        assert_eq!(result.txin.len(), 2);
+        assert_eq!(result.selected_amount, 300_000);
+        assert_eq!(result.fee_amount, 186.0);


The BnB algorithm never allows change to be created. In the case that no changeless solution is found, it is supposed to fallback to another selection method.
Since the two available inputs are 100,000 and 200,000 satoshis, this should result in a failure for BnB. Since 50,000 sats is much larger than the fee for two inputs at 1 sat/vB and the permitted excess.

Yeah at the moment what's happening is that BnB is giving back TotalTriesExceeded (even if this is not exactly true: BnB tried everything and didn't find a suitable solution, it's not that there weren't enough tries!), single random draw is correctly giving back 300_000.

I have updated the tests so they test the functions bnb and single_random_draw (instead of testing only coin_select)

murchandamus · 2020-10-27T22:44:58Z

+        assert_eq!(result.txin.len(), 2);
+        assert_eq!(result.selected_amount, 300_000);
+        assert_eq!(result.fee_amount, 186.0);


murchandamus · 2020-10-27T22:58:56Z

+    fn test_bnb_exact_match() {
+        let seed = [0; 32];
+        let mut rng: StdRng = SeedableRng::from_seed(seed);
+        let database = MemoryDatabase::default();
+
+        for _i in 0..200 {
+            let mut may_use_utxos = generate_random_utxos(&mut rng, 16);
+            let target_amount = sum_random_utxos(&mut rng, &mut may_use_utxos);
+            let result = BranchAndBoundCoinSelection::new(0)
+                .coin_select(
+                    &database,
+                    vec![],
+                    may_use_utxos,
+                    FeeRate::from_sat_per_vb(0.0),
+                    target_amount,
+                    0.0,
+                )
+                .unwrap();
+            assert_eq!(result.selected_amount, target_amount);
+        }
+    }


This is a good sanity check. I would like to propose another. Generate 200 random UTXOs and make the target the sum of the effective value of two of them. You should only get a results with one or two imputs from that. I think that might uncover at least one bug in the above code (I think there is an issue in the backtrack). :)

Alternatively, I would like to suggest a test with a known set of available UTXOs where it e.g. uses the first and fifth largest UTXO of the set to construct the target.

I wrote the first test you suggested and it's unsurprisingly failing (TotalTriesExceeded), but I don't get why it should pass. 200 UTXOs are really a lot (with values ranging from 0 sats to 2BTC), how could the bnb find the exact match with only 100_000 tries?

What is the complexity of the BnB approach? O(n^2)? (If so, it should definitely pass :))

The whole search space of BnB is O(2^n), but the bounding essentially makes it go to linear complexity for some parts. Finding an exact match with two inputs can be done in less than n^2 tries though. I think we will need a few more bounding mechanisms. :)

danielabrozzoni · 2020-10-31T15:59:39Z

I've updated the code, should be little bit better now. As I was saying in the comment, the test with 200 UTXOs is currently failing (throws TotalTriesExceeded). I'll dig into it :)

danielabrozzoni · 2020-11-03T17:46:13Z

I updated the PR so it's not a draft anymore.
I would say: the fact that bnb can't handle more than 40/45 utxos is an issue... but maybe we shouldn't focus too much on it now. I added a TODO: so we don't forget that in the future some optimizations will be needed :)

murchandamus · 2020-11-03T18:46:13Z

Okay, I'll review it again. :)

afilini

Even though there are a few things that could probably be expressed in a more "rust-like" style, I would go ahead and merge this so that we can prepare a tag for the next beta release, and then work on more improvements in separate PRs.

Do you have any more comments @xekyo before I go ahead and merge this?

LLFourn · 2020-11-11T03:39:54Z

I've made a smaller PR here that will conflict a bit with this. All in all it looks easier to merge that one first and then this one. I don't mind updating this PR if that works. Not a big issue either way.

As a side note I think it would be good if the B&B coin selection algorithm go in its own file in a coin_selection directory. Presumably there could be more than one coin selection algorithm going forward.

danielabrozzoni · 2020-11-12T18:41:53Z

I'm cool with merging #159 before this one :)
I like the idea of having a coin_selection directory with different files for the different algos, but I would prefer to do this refactoring in a different PR, so it's easier to review

LLFourn · 2020-11-12T23:10:17Z

@danielabrozzoni, @afilini it looks like there are some complications to that PR that requires some further thinking so I'd say go for this one :)

Replace all the occurences of `serialize(&txin)` with TXIN_DEFAULT_WEIGHT.

murchandamus · 2020-11-19T16:13:31Z

+struct OutputGroup {
+    utxo: UTXO,
+    // weight needed to satisfy the UTXO, as described in `Descriptor::max_satisfaction_weight`
+    satisfaction_weight: usize,
+    // Amount of fees for spending a certain utxo, calculated using a certain FeeRate
+    fee: f32,
+    // The effective value of the UTXO, i.e., the utxo value minus the fee for spending it
+    effective_value: i64,
+}


Just a bit background info: OutputGroups in Bitcoin Core collect all unspents associated with one address. They are used to prevent spending only one UTXO if there are multiple associated with the same address, and to rather only spend all or none of them, so that there is no additional privacy leak.

murchandamus · 2020-11-19T18:22:19Z

+            .collect();
+
+        // Mapping every (UTXO, usize) to an output group.
+        // Filtering UTXOs with an effective_value < 0, as the fee paid for


Nit: Strictly speaking, I think you're filtering effective_value <= 0.

murchandamus · 2020-11-20T23:41:18Z

+        let mut rng: StdRng = SeedableRng::from_seed(seed);
+        let fee_rate = FeeRate::from_sat_per_vb(0.0);
+
+        for _ in 0..200 {


Am I understanding correctly, that you're performing this test 200 times? Would that perhaps be the reason why it timed out with a larger number of UTXOs? If I understand that correctly, have you tried running it with ~500 UTXOs just once?

murchandamus · 2020-11-20T23:52:26Z

One optimization that I had added was to skip a UTXO's inclusion branch when it exactly matches the one that was just backtracked. E.g.

A = 100
B = 90
C = 90
D = 80
…

Would then have the exploration: { [A], [AB], [ABC], [ABCD], …, [ABC_…], [AB__…], A__D…]. I.e. once B gets omitted, C gets skipped, because the prefix [AB…] and [AC…] would be equivalent.

The observation was that people often send round values and this was an easy way to skip a bunch of equivalent solutions.

notmandatory mentioned this pull request Oct 27, 2020

Update coin selection terminology #146

Merged

murchandamus suggested changes Oct 27, 2020

View reviewed changes

danielabrozzoni force-pushed the bnb_coin_selection branch 2 times, most recently from 009349b to 6151657 Compare October 31, 2020 15:54

danielabrozzoni force-pushed the bnb_coin_selection branch 3 times, most recently from 5711f08 to 3702fdc Compare November 3, 2020 17:42

danielabrozzoni marked this pull request as ready for review November 3, 2020 17:45

danielabrozzoni force-pushed the bnb_coin_selection branch 2 times, most recently from 84d883d to 4d7257d Compare November 8, 2020 15:18

afilini approved these changes Nov 9, 2020

View reviewed changes

danielabrozzoni force-pushed the bnb_coin_selection branch from 4d7257d to 84a8988 Compare November 12, 2020 18:55

danielabrozzoni force-pushed the bnb_coin_selection branch from 84a8988 to 5ebda55 Compare November 13, 2020 11:40

danielabrozzoni added 6 commits November 13, 2020 12:42

[wallet] Use TXIN_DEFAULT_WEIGHT constant in coin selection

a86706d

Replace all the occurences of `serialize(&txin)` with TXIN_DEFAULT_WEIGHT.

[wallet] Add Branch and Bound coin selection

99060c5

[wallet] Add tests for BranchAndBoundCoinSelection::coin_select

be91997

[wallet] Add tests for BranchAndBoundCoinSelection::bnb

2382432

[wallet] Add tests for BranchAndBoundCoinSelection::single_random_draw

c43f201

[wallet] Replace must_use with required in coin selection

9f31ad1

danielabrozzoni force-pushed the bnb_coin_selection branch from 5ebda55 to 9f31ad1 Compare November 13, 2020 11:42

afilini merged commit 9f31ad1 into bitcoindevkit:master Nov 13, 2020

murchandamus reviewed Nov 20, 2020

View reviewed changes

danielabrozzoni deleted the bnb_coin_selection branch August 16, 2022 17:06

Conversation

danielabrozzoni commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murchandamus 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!

Uh oh!

Uh oh!

Uh oh!

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!

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!

danielabrozzoni commented Oct 31, 2020

Uh oh!

danielabrozzoni commented Nov 3, 2020

Uh oh!

murchandamus commented Nov 3, 2020

Uh oh!

afilini left a comment

Choose a reason for hiding this comment

Uh oh!

LLFourn commented Nov 11, 2020

Uh oh!

danielabrozzoni commented Nov 12, 2020

Uh oh!

LLFourn commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

murchandamus commented Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danielabrozzoni commented Oct 27, 2020 •

edited

Loading

LLFourn commented Nov 12, 2020 •

edited

Loading

murchandamus commented Nov 20, 2020 •

edited

Loading