Introduce new `bdk_coin_select` implementation by LLFourn · Pull Request #924 · bitcoindevkit/bdk

LLFourn · 2023-03-23T06:24:13Z

This is porting over LLFourn/bdk_core_staging#46

I made it compile on 1.48.0 and added a script that we can use in CI to check that all the crates that should be able to compile on 1.48.0 can.

TODO:

finish documentation
update examples to use this

I've had a request to do a release of the separately before it is used in bdk itself so that other projects can try it out.

cc @darosior

and add some tooling to enforce this

notmandatory · 2023-03-26T04:40:33Z

The script approach looks good to me. I tried my sub-directory idea (cargo +1.48.0 build --manifest-path msrv-1.48.0/Cargo.toml) and it doesn't work.

evanlinjin · 2023-05-31T08:04:11Z

+#[derive(Debug, Clone)]
+pub struct CoinSelector<'a> {
+    base_weight: u32,
+    candidates: &'a [Candidate],


I think it will be much more helpful if we had:

pub struct CoinSelector<'c, C> { candidates: &'c [C], /* ... */ } impl<'c, C: AsRef<Candidate>> CoinSelector<'c, C> {}

This way C can contain everything required to complete the rest of tx building.

You can even do:

impl AsRef<Candidate> for Candidate { fn as_ref(&self) -> &Candidate { self } }

I think to do this properly we'd have to make Candidate a trait. I'd leave it like this for now until we see how tx building turns out and make it more flexible where we find pain points.

on the other hand there's no real harm to doing what you suggested now so maybe I can give it a shot.

darosior

Impressive work, thanks for making it a separate crate.

We started poking at using this in Liana (see wizardsardine/liana#560), so i'm curious what's the state of this PR?

darosior · 2023-06-20T12:18:55Z

+
+/// The weight of a TXOUT without the `scriptPubkey` (and script pubkey length field).
+/// Just the weight of the value field.
+pub const TXOUT_BASE_WEIGHT: u32 = 4 * core::mem::size_of::<u64>() as u32; // just the value


nit: Technically it's an i64, but the size is the same anyways.

Suggested change

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

pub const TXOUT_BASE_WEIGHT: u32 = 4 * core::mem::size_of::<i64>() as u32; // just the value

I actually didn't know that bitcoin core treated these at i64 internally. I think from a consensus perspective either is correct? i.e. if we changed the definition to be u64 the protocol doesn't change at all.

darosior · 2023-06-20T12:21:05Z

+
+    pub fn new_tr_keyspend() -> Self {
+        Self {
+            weight: TXOUT_BASE_WEIGHT + TR_SPK_WEIGHT,


You are missing the scriptPubKey length (and therefore 4 weight units)?

Yep well spotted. I added a 1 byte spk length to TXOUT_BASE_WEIGHT. All this weight stuff was added on a whim while I was trying to use this and is not very well tested. I'm not sure if it's worth having since you should probably get the numbers from rust bitcoin and rust miniscript which do a better job of telling you this stuff now.

it's inconsistent to not include it and I confused myself.

LLFourn · 2023-06-21T04:48:19Z

Hey @darosior. Thanks for taking a stab at using this that was really helpful. I think the status is waiting for @evanlinjin to help me finish it. Here's a list of TODOs:

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.

I wonder if @evanlinjin can put this as priority after #1002.

darosior · 2023-06-27T10:43:52Z

+    fn default() -> Self {
+        Self {
+            feerate: FeeRate::default_min_relay_fee(),
+            min_fee: 0, // TODO figure out what the actual network rule is for this


There is no explicit standardness limit for absolute fee (aside from replacement rules). It's implicitly bounded by the minimum feerate rule since transactions can't be empty.

Yep. I'll leave removing all these TODOs around the code for @evanlinjin who I think created most of them. Things that need addressing should that when this is merged it's TODO free.

darosior · 2023-06-27T10:46:15Z

+    /// The minimum fee the selection must have
+    pub min_fee: u64,


Since this only needs be specified for RBF, how about making it Optional?

This would mean Some(0) and None have the same semantics which I think outweighs the better intuition provided by having it as an Option. Code docs should for sure explain that the only time you need to use this is to comply with RBF rules.

darosior · 2023-06-27T11:49:42Z

+        let witness_header_extra_weight = self
+            .selected()
+            .find(|(_, wv)| wv.is_segwit)
+            .map(|_| 2)
+            .unwrap_or(0);
+        let vin_count_varint_extra_weight = {
+            let input_count = self.selected().map(|(_, wv)| wv.input_count).sum::<usize>();
+            (varint_size(input_count) - 1) * 4
+        };
+
+        self.selected_weight() + witness_header_extra_weight + vin_count_varint_extra_weight


For transaction that have at least one Segwit input, you also need to consider the added witness stack size for all inputs that are not spending from a Segwit Script. This is always 1 per legacy input since for them this stack is always empty.

More information about this here: https://github.com/bitcoin/bitcoin/pull/26567/files#diff-6e06b309cd494ef5da4e78aa0929a980767edd12342137f268b9219167064d13R62-R69.

Thanks. I fixed this and added some tests 4e0c907

Interestingly doing this properly broke some prop tests. The prop tester would find edge cases where the branch and bound algorithm found what looked like a good solution with lots of legacy inputs but then it had to add one segwit input and made the weight blow up for the existing inputs. This could probably be fixed but I don't think it's worth the effort and code complexity just to have slightly better selections when you mix legacy and segwit for waste metric etc.

see: bitcoindevkit#924 (comment) Was a PITA since branch and bound is hard to do with this interference between segiwt and legacy weights. It would find solutions that looked good until you add the final input which was segwit and then the solution would be suboptimal and fail the test.

darosior · 2023-08-07T14:17:39Z

So #1002 was replaced by #1034 which was merged. @evanlinjin do you still have this as a priority?

evanlinjin · 2023-08-07T14:36:32Z

@darosior thanks for asking. I'm literally planning to start work on this tomorrow. This will be a priority. I should have a rough timeline of when I think I can get this completed by the end of the week.

Also note that I also need to look into a couple of bugs as well. Will keep you posted.

darosior · 2023-08-07T14:41:31Z

That's great news! Thanks, please do.

see: bitcoindevkit#924 (comment) Was a PITA since branch and bound is hard to do with this interference between segiwt and legacy weights. It would find solutions that looked good until you add the final input which was segwit and then the solution would be suboptimal and fail the test.

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

This is replaced by #1072.

see: bitcoindevkit#924 (comment) Was a PITA since branch and bound is hard to do with this interference between segiwt and legacy weights. It would find solutions that looked good until you add the final input which was segwit and then the solution would be suboptimal and fail the test.

LLFourn added 3 commits March 23, 2023 11:41

Move over from old PR

19d193e

Make bdk_coin_select work on 1.48.0

ee196e8

and add some tooling to enforce this

Introduce new coin selection implementation

948b09c

LLFourn mentioned this pull request Mar 23, 2023

Support for MSRV shipped with Debian stable for default features #331

Closed

LLFourn added 2 commits May 23, 2023 12:04

tmp

f48f558

tmp

9129b43

evanlinjin reviewed May 31, 2023

View reviewed changes

notmandatory assigned LLFourn Jun 19, 2023

darosior reviewed Jun 20, 2023

View reviewed changes

LLFourn added 2 commits June 21, 2023 12:24

cargo fmt

a86fed1

Include the SPK length field weight in TXOUT_BASE_weight

02f72c8

it's inconsistent to not include it and I confused myself.

darosior reviewed Jun 27, 2023

View reviewed changes

nondiremanuel mentioned this pull request Aug 8, 2023

W33 Lib Team Call bitcoindevkit/.github#72

Closed

26 tasks

evanlinjin mentioned this pull request Aug 9, 2023

Introduce new bdk_coin_select implementation #1072

Closed

14 tasks

notmandatory closed this Aug 16, 2023

notmandatory mentioned this pull request Aug 29, 2023

W35 Lib Team Call bitcoindevkit/.github#74

Closed

30 tasks

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

		/// The minimum fee the selection must have
		pub min_fee: u64,

Conversation

LLFourn commented Mar 23, 2023

Uh oh!

notmandatory commented Mar 26, 2023

Uh oh!

evanlinjin May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LLFourn Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

LLFourn commented Jun 21, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

darosior Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darosior commented Aug 7, 2023

Uh oh!

evanlinjin commented Aug 7, 2023

Uh oh!

darosior commented Aug 7, 2023

Uh oh!

notmandatory commented Aug 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

evanlinjin May 31, 2023 •

edited

Loading

LLFourn Jun 7, 2023 •

edited

Loading

LLFourn commented Jun 21, 2023 •

edited

Loading

darosior Jun 27, 2023 •

edited

Loading