Allow custom fallback algorithm for bnb

[evanlinjin]

Description

This allows the caller to set a custom fallback algorithm when using BranchAndBoundCoinSelection. Previously, you were forced into using SingleRandomDraw.

Signature of CoinSelectionAlgorithm::coin_select has been changed to take in a &mut RangCore. This allows us to pass the random number generator directly to the cs algorithm.

Single random draw is now it's own type SingleRandomDraw and impls CoinSelectionAlgorithm.

BranchAndBoundCoinSelection now handles it's own fallback algorithm internally, and a generic type parameter is added to specify the fallback algorithm.

coin_selection::Error is renamed to InsufficientFunds and the BnB error variants are removed. The BnB error variants are no longer needed since those cases are handled internally by BranchAndBoundCoinSelection (via calling the fallback algorithm).

Notes to the reviewers

This is breaking change. Not sure how useful this is for our users. If it's deemed useful, consider including in beta.

Changelog notice

• Changed CoinSelectionAlgorithm::coin_select to take in an additional &mut RangCore variable. This allows us to pass a random number generator directly to the cs algorithm.
• Added SingeRandomDraw type which impls CoinSelectAlgorithm.
• Changed BranchAndBoundCoinSelection to call the fallback internally. An additional generic parameter is added set this.
• Changed coin_selection::Error to coin_selection::InsufficientFunds (which is now a struct) and therefore removing bnb error variants.

Checklists

All Submissions:

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

[ValuedMammal]

Concept ACK. I want to take a closer look at it. It would also be nice to try and fix the ignored tests in coin_selection module

[ValuedMammal]

Simple test suggestion

    #[test]
    fn test_bnb_fallback_algorithm() {
        // utxo value
        // 120k + 80k + 300k
        let optional_utxos = get_oldest_first_test_utxos();
        let feerate = FeeRate::BROADCAST_MIN;
        let target_amount = 190_000;
        let drain_script = ScriptBuf::new();
        // bnb won't find exact match and should select oldest first
        let res = BranchAndBoundCoinSelection::<OldestFirstCoinSelection>::default()
            .coin_select(
                vec![],
                optional_utxos,
                feerate,
                target_amount,
                &drain_script,
                &mut thread_rng(),
            )
            .unwrap();
        assert_eq!(res.selected_amount(), 200_000);
    }

[evanlinjin]

Thanks @ValuedMammal feel free to push a commit to introduce the test (if you wish).

[oleonardolima]

Concept ACK 36f7d39
Overall looks good! It's getting late, I'll do another review and test it tomorrow :)

[ValuedMammal]

My main gripe is making the RNG required in order to implement CoinSelectionAlgorithm when previously it wasn't and technically none of our other algos require randomness. But there may be a way to keep the new struct SingleRandomDraw and implement the trait feature-gated behind "std" so that we can use thread_rng. Then BnB can fallback to single random draw if "std" feature is enabled, otherwise a user-provided algorithm. I haven't fully fleshed it out in code yet.

[oleonardolima]

[...] It would also be nice to try and fix the ignored tests in coin_selection module

AFAICT it only needs to removes the #[ignore = ...] from lines 1148 and 1193, as SRD is the fallback algorithm now. At least those two worked just fine testing locally.

Maybe renaming the test fn to mention it relies on fallback is good too.

edit: I still haven't figured out the problem with the other one from line 1236, as it's been ignored for a while.

[notmandatory]

My main gripe is making the RNG required in order to implement CoinSelectionAlgorithm when previously it wasn't and technically none of our other algos require randomness. But there may be a way to keep the new struct SingleRandomDraw and implement the trait feature-gated behind "std" so that we can use thread_rng. Then BnB can fallback to single random draw if "std" feature is enabled, otherwise a user-provided algorithm. I haven't fully fleshed it out in code yet.

I also took a stab at encapsulating the RNG for SingleRandomDraw and it's cleaner in some ways but breaks the ability to use DefaultCoinSelectionAlgorithm in the no_std mode. So for now I think the way it is here is best.

[notmandatory]

@evanlinjin feel free to squash 89e619e which has @ValuedMammal's suggestions in it and I think this one also needs a rebase.

[notmandatory]

ACK 89e619e

[notmandatory]

I've rebased this PR and incorporated @ValuedMammal 's docs and new bnb fallback test.

[notmandatory]

I think this PR still requires a few changes. If someone can take over this PR (and review and come to consensus on the following suggestions), that will be absolutely wonderful.

1. We currently use `.sort_unstable_by_key` to sort UTXOs for our coin selection algorithm impls. This is fine, but to make things deterministic (i.e. equal-key elements are ordered the same each time), we should add something like `OutPoint` at the end of the key used to sort.

I added a test to check if deterministic coin selection algos were always returning the same utxos and even without changing .sort_unstable_by_key to .sort_by_key I'm getting deterministic results. I still changed to using .sort_by_key just to be safe.

2. In bnb, we use `.reverse`. However, to make things consistent with the way we use `OutPoint` as part of the sort-key, consider using [`Reverse(effective_value)`](https://doc.rust-lang.org/std/cmp/struct.Reverse.html) as the first tuple element of the sort-key.

I didn't have to change .reverse in bnb to get deterministic results.

I also did some test cleanup and added back ignored tests that work as long as we calculate the correct target amount that bnb needs to select the expected utxos.

[ValuedMammal]

Regarding utxo sorting I think we're also saved by the fact that list_unspent calls index.outpoints() that returns an ordered set of ((K, u32), OutPoint) - it seems like we can rely on this sorting. Also isn't it true that no two WeightedUtxos should be the same because it already contains the outpoint?

[evanlinjin]

I think I pushed everyone onto the wrong rabbit-hole with regards to .sort_unstable_by_key. The "unstable" part only means that equal elements may be reordered. There is no randomness here however, so with the same UTXO set, the sorted order will still be the same (as you probably found in your tests @notmandatory).

I think we should revert back to using .sort_unstable_by_key as it is more performant and there is no benefit for keeping equal elements ordered the same.

[notmandatory]

I think we should revert back to using .sort_unstable_by_key as it is more performant and there is no benefit for keeping equal elements ordered the same.

Makes sense, reverted.

[oleonardolima]

utACK 65be4ea

Cool, the new deterministic UTXO selection tests look like a great addition!

[evanlinjin]

self-ACK 65be4ea

[ValuedMammal]

@@ -402,7 +402,7 @@ pub struct BranchAndBoundCoinSelection<Cs = SingleRandomDraw> {
     fallback_algorithm: Cs,
 }
 
-/// Error returned by branch and bond coin selection.
+/// Error returned by branch and bound coin selection.
 #[derive(Debug)]
 enum BnbError {
     /// Branch and bound coin selection tries to avoid needing a change by finding the right inputs for
@@ -521,8 +521,8 @@ impl<Cs: CoinSelectionAlgorithm> CoinSelectionAlgorithm for BranchAndBoundCoinSe
         }
 
         match self.bnb(
-            required_ogs.clone(),
-            optional_ogs.clone(),
+            required_ogs,
+            optional_ogs,
             curr_value,
             curr_available_value,
             signed_target_amount,
@@ -1383,7 +1383,7 @@ mod test {
 
         let drain_script = ScriptBuf::default();
         let target_amount = 20_000 + FEE_AMOUNT;
-        BranchAndBoundCoinSelection::<SingleRandomDraw>::default()
+        BranchAndBoundCoinSelection::new(size_of_change, SingleRandomDraw)
             .bnb(
                 vec![],
                 utxos,
@@ -1414,7 +1414,7 @@ mod test {
 
         let drain_script = ScriptBuf::default();
 
-        BranchAndBoundCoinSelection::<SingleRandomDraw>::default()
+        BranchAndBoundCoinSelection::new(size_of_change, SingleRandomDraw)
             .bnb(
                 vec![],
                 utxos,
@@ -1450,7 +1450,7 @@ mod test {
 
         let drain_script = ScriptBuf::default();
 
-        let result = BranchAndBoundCoinSelection::<SingleRandomDraw>::default()
+        let result = BranchAndBoundCoinSelection::new(size_of_change, SingleRandomDraw)
             .bnb(
                 vec![],
                 utxos,

[notmandatory]

I'm working on a follow-up PR. I also think the tests need to be improved and there's a bigger issue with SingleRandomDraw in that if used on its own it doesn't fail when there's not enough sats to meet the required amount.

[notmandatory]

Fixed in #1605