Introduce new `bdk_coin_select` implementation #1072

evanlinjin · 2023-08-09T14:12:05Z

Description

This replaces #924 (which in turn, ports over LLFourn/bdk_core_staging#46).

As stated by @LLFourn in #924 (comment), we will complete the following in this PR:

Add minimize fee metric (it's a better default that waste see Apply coin selection for spend wizardsardine/liana#560 (comment))
Allow easy weighted combination of different metrics (see same link above)
Document everything including nice README.md exmaples
Make DWIM branch and bound helper function (Apply coin selection for spend wizardsardine/liana#560 (comment))
Make DrainWeight (Apply coin selection for spend wizardsardine/liana#560 (comment))
Add CoinSelector::fund_outputs (see Apply coin selection for spend wizardsardine/liana#560 (comment)) -- another point is that in general we need to know the number of outputs to get the varint for the outputs correct which I think we are cheating on atm.
Test cases against real world transactions to see if coin selections have precisely the real world feerate that was expected. (EDIT: I've just done a few of these against the feerate shown mempool.space).
Rewrite examples to use this crate.
Remove all TODO in the new crate and make issues for further work.

Notes to the reviewers

~

Changelog notice

Introduce new bdk_coin_select module

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 tests for the new feature
I've added docs for the new feature

notmandatory · 2023-08-10T01:07:03Z

This looks like something worthy of a 2.0 release milestone. Will current 1.0 coin selection be at least on par with pre-1.0 BDK coin selection?

LLFourn · 2023-08-10T03:01:35Z

This looks like something worthy of a 2.0 release milestone. Will current 1.0 coin selection be at least on par with pre-1.0 BDK coin selection?

The plan for this PR is to release the crate on crates.io. Gutting BDK wallet coin selection and replacing it with this is another problem altogether. Using it in bdk wallet would require the planning module #1068 is first ready to be released. I think the plan is to have a crate bdk_tx_builder or something that will put all these things together and can be used without Wallet.

notmandatory · 2023-08-10T15:49:14Z

This looks like something worthy of a 2.0 release milestone. Will current 1.0 coin selection be at least on par with pre-1.0 BDK coin selection?

The plan for this PR is to release the crate on crates.io. Gutting BDK wallet coin selection and replacing it with this is another problem altogether. Using it in bdk wallet would require the planning module #1068 is first ready to be released. I think the plan is to have a crate bdk_tx_builder or something that will put all these things together and can be used without Wallet.

Got it, I won't mark this for any BDK major release milestones. But eventually integrating this into BDK along with #1068 sounds like a good goals for a future 2.0 milestone.

notmandatory · 2023-08-16T13:05:59Z

Per discussion in bitcoindevkit/.github#72 this will be stand-alone framework, not to be integrate with BDK wallet until a future 2.0 milestone.

nondiremanuel · 2023-08-18T14:11:48Z

Even if it will be ready together with alpha.3, being that it won't be integrated until a future 2.0 version, I would assign it to the 2.0 milestone. In general I propose to use milestones to signal which will be the release that will include a specific PR, regardless of the timing of development (so even if it is ready earlier), but maybe I didn't understand correctly the topic here

LLFourn · 2023-08-21T02:51:16Z

Depending on how things go we could replace the existing coin selection with this one without doing the full redesign of tx building in bdk and put in alpha.3. For example, instead of:

tx_builder = tx_builder.coin_selection::<SomeCoinSelectionAlgo>();

This could be improved to:

tx_builder = tx_builder.coin_selection(|coin_selector| {
    /*This will be called once we've got all the candidates. Do your coin selection with coin_selector and return it*/
});

Internally tx_builder would store a where for<'a> F: FnOnce(CoinSelector<'a>) -> Result<CoinSelector<'a>, E> which would have a sensible default (F and E would become type parameters on TxBuilder). The benefit would be getting the flexibility and power and correctness of the new coin selection code. This would likely fix #1066 which arises mainly from the current architecture of coin selection where each coin selection algorithm can choose to return coin selections that violate the constraints given to it (in this case absolute fee).

@evanlinjin would be a better judge of whether this is a good idea.

evanlinjin · 2023-08-23T01:25:03Z

@darosior I'll give a guesstimate of two more weeks before this is in a good state.

darosior · 2023-08-23T07:37:49Z

Cool, thanks for keeping me up to date.

…

-------- Original Message --------

On Aug 23, 2023, 3:25 AM, 志宇 wrote: ***@***.***(https://github.com/darosior) I'll give a guesstimate of two more weeks before this is in a good state. — Reply to this email directly, [view it on GitHub](#1072 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3FYAYDDJ4OCD32JJYP3XWVLXVANCNFSM6AAAAAA3KCJUKY). You are receiving this because you were mentioned.Message ID: ***@***.***>

LLFourn · 2023-11-15T05:00:58Z

crates/bdk/src/wallet/coin_selection.rs

@@ -836,7 +836,7 @@ mod test {
        let drain_script = ScriptBuf::default();
        let target_amount = 250_000 + FEE_AMOUNT;

-        let result = LargestFirstCoinSelection::default()
+        let result = LargestFirstCoinSelection


huh why are we changing this file?

LLFourn · 2023-11-15T05:11:53Z

build-msrv-crates.sh

+}
+
+cp Cargo.toml Cargo.tmp.toml
+cp Cargo.1.48.0.toml Cargo.toml


I think we should instead just put this code in a separate repo in the bdk org and have our own MSRV for it.

LLFourn · 2023-11-15T05:18:50Z

nursery/coin_select/README.md

+use bitcoin::{ Address, Network, Transaction, TxIn, TxOut };
+
+// You should use miniscript to figure out the satisfaction weight for your coins!
+const TR_SATISFACTION_WEIGHT: u32 = 66;


We actually have this as a const in bdk_coin_select. Keep the comment saying something "In general, you can use miniscript to figure out the satisfaction weight of your inputs".

LLFourn · 2023-11-15T05:19:47Z

nursery/coin_select/README.md

+        input_count: 1,
+        // the value of the input
+        value: 1_000_000,
+        // the total weight of the input(s). This doesn't include


What doesn't it include (I know this was probably my fault)?. Maybe just remove that.

LLFourn · 2023-11-15T05:21:43Z

nursery/coin_select/README.md

+    lock_time: bitcoin::absolute::LockTime::from_height(0).unwrap(),
+    version: 0x02,
+};
+let base_weight = base_tx.weight().to_wu() as u32;


since rust bitcoin uses u64 here should we also? It can be u32 internally but we can cast it.

LLFourn · 2023-11-15T05:41:29Z

nursery/coin_select/src/coin_selector.rs

+        &mut self,
+        metric: M,
+        max_rounds: usize,
+    ) -> Result<Ordf32, NoBnbSolution> {


When i was suggesting this method I was thinking that it would fall back to select_until_target_met if it can't find anything. This would mean you don't need this error type -- rather you can just have InsufficientFunds.

Do you think it's worth having run_bnb_with_fallback() that does that?

Also another sneaky request -- if #[cfg(feature = "std")] can you add a method that lets you pass in a std::time::Duration and checks the time passed every 1_000 rounds and stops after the time has been exceeded.

LLFourn · 2023-11-15T05:43:51Z

nursery/coin_select/src/feerate.rs

+/// Fee rate
+#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
+// Internally stored as satoshi/weight unit
+pub struct FeeRate(Ordf32);


might be worth manually implementing Debug so the format prints sats per vb in the first place like: 8 sat/vb (2 sat/wu)

LLFourn · 2023-11-15T05:45:53Z

nursery/coin_select/src/feerate.rs

+    }
+
+    /// Return the value as satoshi/wu.
+    pub fn spwu(&self) -> f32 {


Suggested change

pub fn spwu(&self) -> f32 {

pub fn sat_per_wu(&self) -> f32 {

we can keep fn spwu as pub(crate) but in the API it should be more descriptive.

LLFourn · 2023-11-15T05:59:29Z

nursery/coin_select/src/metrics/lowest_fee.rs

+        match drain_weights {
+            // with change
+            Some(drain_weights) => {
+                (cs.input_weight() + drain_weights.output_weight) as f32


Probably worth throwing in the base_weight in here too to make the metric match the result.

I had a helper method on CoinSelector for this: implied_fee -- might be more elegant to make it public and use it. You're not taking into account the min_fee which seems like it could cause the wrong answer.

LLFourn · 2023-11-15T06:08:38Z

nursery/coin_select/src/metrics/lowest_fee.rs

+                Some(_) => {
+                    let selected_value = cs.selected_value();
+                    assert!(selected_value >= self.target.value);
+                    (cs.selected_value() - self.target.value) as f32


what's going on here? When you do have a change output why do you add this extra input minus target to the value 😕.

Should this rather be in the None case for the excess? I've just encountered one scenario where I get the same score for two different changeless selections, but one of these selections has greater excess.

darosior · 2023-11-20T18:27:50Z

Another API consideration: for RBF the minimum absolute fee of a replacement transaction depends on the size of itself (since by rule 4 the fee must be at least the sum of the replaced transactions' fees plus 1sat/vb worth of the replacement transaction's size).

Therefore when selecting coins for the replacement transaction the caller can't set min_fee to the right value, since it depends on the result of the coin selection it's trying to get. For now i think our best bet is to try again with min_fee = prev_min_fee + replacement_tx_size until it either errors or succeeds, but it's meh.

h/t @jp1ac4.

LLFourn · 2023-11-21T04:50:18Z

Another API consideration: for RBF the minimum absolute fee of a replacement transaction depends on the size of itself (since by rule 4 the fee must be at least the sum of the replaced transactions' fees plus 1sat/vb worth of the replacement transaction's size).

Therefore when selecting coins for the replacement transaction the caller can't set min_fee to the right value, since it depends on the result of the coin selection it's trying to get. For now i think our best bet is to try again with min_fee = prev_min_fee + replacement_tx_size until it either errors or succeeds, but it's meh.

h/t @jp1ac4.

The min_fee is for rule 3. You use the previous feerate + 1 s/vb to satisfy rule 4. At least this is what i had in mind let me know if I missed something!

darosior · 2023-11-21T09:53:56Z

The min_fee is for rule 3. You use the previous feerate + 1 s/vb to satisfy rule 4. At least this is what i had in mind let me know if I missed something!

Unfortunately it's not that simple. Rule 4 is about absolute fees. Consider this situation (Tx (fee, vsize)). We try replacing the following transaction chain: A (100, 100) <- B (100, 100). We set min_fee to 200 and feerate to 2 s/vb (ie previous feerate + 1 s/vb). We get a replacement A' (200, 100).

This fails the Rule 4 check since A' should not only have a higher feerate than conflicts (6) and a higher fee than the sum of the conflicts' fees (3), but it should also have 1 s/vb * its size additional fees (4). A' should have a fee of 300.

Of course in this trivial example we could have set min_fee to 300. But what if the coin selection makes it so A' is 200 vbytes large instead? Failure again. It's really something which can't be done outside the coin selection.

LLFourn · 2023-11-22T00:19:02Z

@darosior thanks for the correction. I always simplified this rule to "higher feerate" in my mind. TIL. I don't blame myself too much since this is a very bizarre rule. It obviates the need for rule 3 (satisfying rule 4 will always satisfy rule 3). I do believe that higher abs fee + 1svb higher feerate than tx being replaced is far more intuitive and simpler.

I feel as if there was some discussion about simplifying these rules led by @glozow. I wonder if there was any progress.

So if I understand correctly we should add another constraint to Target that handles RBF constraints which handles the abs fee of what is being replaced and has a "inremental relay" feerate. I think we should do this in a follow up PR @evanlinjin.

Once @evanlinjin responds to my comments above we're going to:

Move this crate to a new repo because it is so nicely decoupled from everything else
Release it
Make this PR just update the examples and use the released version (luckily we didn't impl RBF in the examples yet!)

darosior · 2023-11-23T09:22:36Z

I do believe that higher abs fee + 1svb higher feerate than tx being replaced is far more intuitive and simpler.

And doesn't achieve the purpose of this rule, which is to prevent free relay/churn. For instance with your rule i can replace txA(100_000,100_000) hundreds (almost a thousand) of times without paying any more fee by every time crafting a smaller txA'(100_000,50_000), txA'(100_000,33_333), etc.. replacement.

I feel as if there was some discussion about simplifying these rules led by @glozow. I wonder if there was any progress.

There's been plenty of discussions around improving RBF, but it was mostly in the context of solving pinning (transaction v3 and ephemeral anchors). There's also been work into trying to patch some holes in the current RBF and make it more incentive compatible along with other mempool operations (for instance, correctly considering the ancestor feerate) but it turns out it's very complicated to do without a whole rewrite of the mempool. This culminated in the Cluster Mempool proposal, which should make for a more approachable interface for wallets.

TL;DR: there is many issues with RBF. People are aware. It's hard. :)

LLFourn · 2023-11-27T23:56:49Z

I do believe that higher abs fee + 1svb higher feerate than tx being replaced is far more intuitive and simpler.

And doesn't achieve the purpose of this rule, which is to prevent free relay/churn. For instance with your rule i can replace txA(100_000,100_000) hundreds (almost a thousand) of times without paying any more fee by every time crafting a smaller txA'(100_000,50_000), txA'(100_000,33_333), etc.. replacement.

My claim was that transaction fees (abs fee, and feerate) should just be enforced to be monotonically increasing which is contradiction to "I can replace hundreds of times without paying any more fee". Your response sounds like you thought I was suggesting that enforcing only a greater feerate was sufficient. If my understanding is wrong please make another attempt to clean it up!

There's been plenty of discussions around improving RBF, but it was mostly in the context of solving pinning (transaction v3 and ephemeral anchors). There's also been work into trying to patch some holes in the current RBF and make it more incentive compatible along with other mempool operations (for instance, correctly considering the ancestor feerate) but it turns out it's very complicated to do without a whole rewrite of the mempool. This culminated in the Cluster Mempool proposal, which should make for a more approachable interface for wallets.

TL;DR: there is many issues with RBF. People are aware. It's hard. :)

Thanks the cluster mempool work looks like it could be a great starting point to go deeper into mempool theory.

darosior · 2023-11-28T17:41:41Z

(I'm happy to move this discussion elsewhere btw, it looks like we are slowly shifting OT of the code review here)

I do believe that higher abs fee + 1svb higher feerate than tx being replaced is far more intuitive and simpler.

And doesn't achieve the purpose of this rule, which is to prevent free relay/churn. For instance with your rule i can replace txA(100_000,100_000) hundreds (almost a thousand) of times without paying any more fee by every time crafting a smaller txA'(100_000,50_000), txA'(100_000,33_333), etc.. replacement.

My claim was that transaction fees (abs fee, and feerate) should just be enforced to be monotonically increasing which is contradiction to "I can replace hundreds of times without paying any more fee".

My bad then i indeed misunderstood "higher abs fee + 1svb" for "higher or equal abs fee + 1svb". But i'm still confused because it was in the context of your criticism of rule 4. Are you saying as long as it's paying as small as 1 satoshi more in abs fees we should accept it? That's not free, but still pretty cheap churn+relay. And that's precisely the rationale given for rule 4:

Try to prevent DoS attacks where an attacker causes the network to repeatedly relay transactions each paying a tiny additional amount in fees, e.g. just 1 satoshi.

LLFourn · 2023-11-29T00:46:26Z

(I'm happy to move this discussion elsewhere btw, it looks like we are slowly shifting OT of the code review here)

All good this discussion will be useful when we come to fix this.

My bad then i indeed misunderstood "higher abs fee + 1svb" for "higher or equal abs fee + 1svb". But i'm still confused because it was in the context of your criticism of rule 4. Are you saying as long as it's paying as small as 1 satoshi more in abs fees we should accept it? That's not free, but still pretty cheap churn+relay. And that's precisely the rationale given for rule 4:

No my bad looking back at it I used "+" to mean logical conjunction. Whoops.

Right I am suggesting that 1 satoshi more in abs fee is good enough for a small tx replacing a bigger tx (since that's the scenario where the abs fee would be stricter than the feerate rule). Or if 1 sat was not a big enough step up have a minimum like you do for feerate e.g. 100 sats.

After thinking about this my misunderstanding is that this would be simpler -- but actually it's not. Providing the previous tx fee and the prev feerate is actually equivalent to providing the previous transaction fee and weight. I don't know how I got stuck on this. I am still confused as to why there is rule 3 when rule 4 suffices for everything.

~~So I propose we do something like this for the target:~~

enum TargetFee {
    Rate(FeeRate),
    Replacement {
        prev_fee: u64,
        prev_weight: u32,
        incremental_relay_feerate: FeeRate,
    }
}

No this doesn't make sense. It should be:

struct TargetFee {
    feerate: FeeRate,
    replacement: Option<Replacement>,
}

struct Replacement {
    prev_fee: u64,
    prev_weight: u32,
    incremental_relay_feerate: FeeRate,
}

Feerate is orthogonal to whether we are replacing. It's two different constraints. So I'd say get rid of min_fee and do this.

Discussion: bitcoindevkit/bdk#1072 (comment)

evanlinjin · 2023-12-11T14:21:38Z

bdk_coin_select now exists in a separate repository so we can close this PR.

notmandatory · 2023-12-11T17:57:14Z

@evanlinjin should we have an issue for bdk to switch to using this new crate?

evanlinjin force-pushed the new_bdk_coin_select branch 2 times, most recently from ea80a62 to df70b71 Compare August 9, 2023 14:27

notmandatory assigned evanlinjin Aug 9, 2023

evanlinjin force-pushed the new_bdk_coin_select branch from df70b71 to e2eb208 Compare August 10, 2023 08:15

evanlinjin force-pushed the new_bdk_coin_select branch from e2eb208 to 22c6d71 Compare August 11, 2023 05:09

nondiremanuel mentioned this pull request Aug 15, 2023

W33 Lib Team Call bitcoindevkit/.github#72

Closed

26 tasks

nondiremanuel modified the milestones: 2.0.0-alpha.0, 1.0.0-alpha.3 Aug 15, 2023

evanlinjin force-pushed the new_bdk_coin_select branch from be29c34 to c1a9dec Compare August 16, 2023 03:25

notmandatory mentioned this pull request Aug 16, 2023

Introduce new bdk_coin_select implementation #924

Closed

evanlinjin force-pushed the new_bdk_coin_select branch 3 times, most recently from 758d854 to b317c43 Compare August 18, 2023 09:30

evanlinjin force-pushed the new_bdk_coin_select branch from b317c43 to 055510e Compare August 19, 2023 15:58

evanlinjin force-pushed the new_bdk_coin_select branch 5 times, most recently from a3bdfb9 to 03a7f1a Compare August 22, 2023 07:19

nondiremanuel mentioned this pull request Aug 22, 2023

W35 Lib Team Call bitcoindevkit/.github#74

Closed

30 tasks

evanlinjin force-pushed the new_bdk_coin_select branch from 52263fa to 991db08 Compare August 24, 2023 11:10

docs(coin_select): update README

476bc87

evanlinjin marked this pull request as ready for review November 14, 2023 21:04

chore(coin_select): update Cargo.toml

d620dc6

LLFourn reviewed Nov 15, 2023

View reviewed changes

nondiremanuel mentioned this pull request Nov 19, 2023

W47 Lib Team Call bitcoindevkit/.github#86

Closed

54 tasks

nondiremanuel modified the milestones: 2.0.0-alpha.0, 1.0.0-alpha.4 Nov 20, 2023

evanlinjin added a commit to bitcoindevkit/coin-select that referenced this pull request Nov 29, 2023

fix: remove minimum absolute fee

a0e9250

Discussion: bitcoindevkit/bdk#1072 (comment)

nondiremanuel mentioned this pull request Dec 2, 2023

W49 Lib Team Call bitcoindevkit/.github#87

Closed

55 tasks

evanlinjin closed this Dec 11, 2023

This was referenced Dec 12, 2023

fix: add excess to changeless LowestFee score bitcoindevkit/coin-select#11

Closed

Allow to target suitable transaction fee for RBF bitcoindevkit/coin-select#12

Closed

nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 6, 2024

notmandatory removed this from the 1.0.0 milestone Jan 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce new `bdk_coin_select` implementation #1072

Introduce new `bdk_coin_select` implementation #1072

evanlinjin commented Aug 9, 2023 •

edited

Loading

notmandatory commented Aug 10, 2023 •

edited

Loading

LLFourn commented Aug 10, 2023

notmandatory commented Aug 10, 2023

notmandatory commented Aug 16, 2023

nondiremanuel commented Aug 18, 2023

LLFourn commented Aug 21, 2023 •

edited

Loading

evanlinjin commented Aug 23, 2023

darosior commented Aug 23, 2023 via email

LLFourn Nov 15, 2023

LLFourn Nov 15, 2023

LLFourn Nov 15, 2023

LLFourn Nov 15, 2023

LLFourn Nov 15, 2023 •

edited

Loading

LLFourn Nov 15, 2023

LLFourn Nov 15, 2023

LLFourn Nov 15, 2023

LLFourn Nov 15, 2023

LLFourn Nov 15, 2023 •

edited

Loading

jp1ac4 Nov 28, 2023

darosior commented Nov 20, 2023

LLFourn commented Nov 21, 2023

darosior commented Nov 21, 2023

LLFourn commented Nov 22, 2023

darosior commented Nov 23, 2023 •

edited

Loading

LLFourn commented Nov 27, 2023

darosior commented Nov 28, 2023

LLFourn commented Nov 29, 2023 •

edited

Loading

evanlinjin commented Dec 11, 2023

notmandatory commented Dec 11, 2023

	pub fn spwu(&self) -> f32 {
	pub fn sat_per_wu(&self) -> f32 {

Introduce new bdk_coin_select implementation #1072

Introduce new bdk_coin_select implementation #1072

Conversation

evanlinjin commented Aug 9, 2023 • edited Loading

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

notmandatory commented Aug 10, 2023 • edited Loading

LLFourn commented Aug 10, 2023

notmandatory commented Aug 10, 2023

notmandatory commented Aug 16, 2023

nondiremanuel commented Aug 18, 2023

LLFourn commented Aug 21, 2023 • edited Loading

evanlinjin commented Aug 23, 2023

darosior commented Aug 23, 2023 via email

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

LLFourn Nov 15, 2023 • 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

LLFourn Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darosior commented Nov 20, 2023

LLFourn commented Nov 21, 2023

darosior commented Nov 21, 2023

LLFourn commented Nov 22, 2023

darosior commented Nov 23, 2023 • edited Loading

LLFourn commented Nov 27, 2023

darosior commented Nov 28, 2023

LLFourn commented Nov 29, 2023 • edited Loading

evanlinjin commented Dec 11, 2023

notmandatory commented Dec 11, 2023

Introduce new `bdk_coin_select` implementation #1072

Introduce new `bdk_coin_select` implementation #1072

evanlinjin commented Aug 9, 2023 •

edited

Loading

notmandatory commented Aug 10, 2023 •

edited

Loading

LLFourn commented Aug 21, 2023 •

edited

Loading

LLFourn Nov 15, 2023 •

edited

Loading

LLFourn Nov 15, 2023 •

edited

Loading

darosior commented Nov 23, 2023 •

edited

Loading

LLFourn commented Nov 29, 2023 •

edited

Loading