fix: remove deprecated max_satisfaction_weight by storopoli · Pull Request #1345 · bitcoindevkit/bdk

storopoli · 2024-02-15T11:52:49Z

Description

Continuation of #1115.
Closes #1036.

Change deprecated max_satisfaction_weight to max_weight_to_satisfy
Remove #[allow(deprecated)] flags

Notes to the reviewers

I've changed all max_satisfaction_weight() to max_weight_to_satisfy() in Wallet.get_available_utxo() and Wallet.build_fee_bump(). Checking the docs on the miniscript crate for max_weight_to_satisfy has the following note:

We are testing if the underlying descriptor is.segwit() or .is_taproot,
then adding 4WU if true or leaving as it is otherwise.

Another thing, we are not testing in BDK tests for legacy (pre-segwit) descriptors.
Should I also add them to this PR?

Changelog notice

Fixed

Replace the deprecated max_satisfaction_weight from rust-miniscript to max_weight_to_satisfy.

Checklists

All Submissions:

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

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

evanlinjin

Thank you for having a go at this, however the approach here is incorrect.

First off, it's incorrect to add 4WU to only segwit inputs.

Looking at the docs of max_weight_to_satisfy, we need to change the definition of our TX_IN_BASE_WEIGHT to be TxIn::default().segwit_weight().

After that, to make the tests pass, we need to change P2WPKH_SATISFACTION_SIZE to take in to account the scriptSigLen and witnessLen.

I think it's okay to assume all transactions are segwit transactions for now? Unless if someone can figure out how to take away the witness len elegantly (for non-segwit txs).

evanlinjin · 2024-02-29T01:24:53Z

crates/bdk/src/wallet/mod.rs

+                    let segwit_add = match is_segwit {
+                        true => 4,
+                        false => 0,
+                    };


Is the intention here to include the scriptSigLen weight? If so, please name the variable appropriately.

However, non-segwit inputs also have this field. Wouldn't it be correct to always have this addition?

Ignore the comment above. Just remove this and add the extra weights to TX_IN_BASE_WEIGHT.

Great, sorry for the missundestanding.
I think I've fixed this in 52d6283

crates/bdk/src/wallet/coin_selection.rs

evanlinjin · 2024-03-01T09:28:59Z

Can I please have some input on my statement here? Thanks!

I think it's okay to assume all transactions are segwit transactions for now? Unless if someone can figure out how to take away the witness len elegantly (for non-segwit txs).

storopoli · 2024-03-01T10:22:09Z

I think it's okay to assume all transactions are segwit transactions for now? Unless if someone can figure out how to take away the witness len elegantly (for non-segwit txs).

I agree. We currently don't test any non-segwit tx anyways...

evanlinjin · 2024-03-01T19:07:21Z

I think it's okay to assume all transactions are segwit transactions for now? Unless if someone can figure out how to take away the witness len elegantly (for non-segwit txs).

Because we already assume this:

https://github.com/bitcoindevkit/bdk/blob/master/crates%2Fbdk%2Fsrc%2Fwallet%2Fmod.rs#L1505-L1513

So I think it is okay.

evanlinjin · 2024-03-01T19:09:36Z

@storopoli can we please rebase the two commits into one and I'll ACK it.

storopoli · 2024-03-02T10:01:11Z

@storopoli can we please rebase the two commits into one and I'll ACK it.

Done, thanks mate!

evanlinjin · 2024-03-03T01:34:49Z

crates/bdk/src/wallet/tx_builder.rs

+                let satisfaction_weight = {
+                    let is_segwit = wallet
+                        .get_descriptor_for_keychain(utxo.keychain)
+                        .is_witness()
+                        || wallet
+                            .get_descriptor_for_keychain(utxo.keychain)
+                            .is_taproot();
+                    let segwit_add = match is_segwit {
+                        true => 4,
+                        false => 0,
+                    };
+                    descriptor.max_weight_to_satisfy().unwrap() + segwit_add
+                };


This is still wrong.

Sorry about that. Should be good now.

ValuedMammal

Approach ACK.

For simplicity I think it's fair to assume segwit weight everywhere. Unless someone is doing legacy transactions with hundreds of inputs (which is certainly possible), I don't think you'd notice a difference. We could consider carving out a special case for legacy-only transactions in a future iteration of tx building.

To the note on testing legacy descriptors I don't have an immediate comment on it.

crates/bdk/src/wallet/coin_selection.rs

crates/bdk/src/wallet/mod.rs

evanlinjin · 2024-03-22T04:41:47Z

@storopoli sorry can you do a rebase on master? thanks

storopoli · 2024-03-22T07:48:01Z

@storopoli sorry can you do a rebase on master? thanks

Don't worry man. Sure, rebased to master :)

- Change deprecated `max_satisfaction_weight` to `max_weight_to_satisfy` - Remove `#[allow(deprecated)]` flags - updates the calculations in TXIN_BASE_WEIGHT and P2WPKH_SATISFACTION_SIZE Update crates/bdk/src/wallet/coin_selection.rs Co-authored-by: ValuedMammal <valuedmammal@protonmail.com>

evanlinjin

ACK 798ed8c

LLFourn requested a review from danielabrozzoni February 16, 2024 05:50

This comment was marked as spam.

Sign in to view

nondiremanuel added this to the 1.0.0-alpha milestone Feb 27, 2024

nondiremanuel assigned storopoli Feb 27, 2024

evanlinjin requested changes Feb 29, 2024

View reviewed changes

storopoli force-pushed the js/max-weight-deprecation branch from e397896 to 8d6764d Compare March 1, 2024 08:29

storopoli commented Mar 1, 2024

View reviewed changes

crates/bdk/src/wallet/coin_selection.rs Outdated Show resolved Hide resolved

storopoli force-pushed the js/max-weight-deprecation branch 2 times, most recently from 8e91886 to 52d6283 Compare March 1, 2024 10:20

storopoli force-pushed the js/max-weight-deprecation branch from 52d6283 to 6d0f282 Compare March 2, 2024 10:01

evanlinjin requested changes Mar 3, 2024

View reviewed changes

storopoli force-pushed the js/max-weight-deprecation branch from 6d0f282 to 482e16c Compare March 3, 2024 07:21

storopoli requested a review from evanlinjin March 3, 2024 07:21

storopoli force-pushed the js/max-weight-deprecation branch from 482e16c to 9078957 Compare March 5, 2024 11:06

notmandatory added module-wallet api A breaking API change labels Mar 16, 2024

ValuedMammal reviewed Mar 19, 2024

View reviewed changes

crates/bdk/src/wallet/coin_selection.rs Outdated Show resolved Hide resolved

crates/bdk/src/wallet/mod.rs Show resolved Hide resolved

storopoli force-pushed the js/max-weight-deprecation branch from a02230c to 9f05e63 Compare March 19, 2024 18:00

storopoli force-pushed the js/max-weight-deprecation branch 2 times, most recently from 446acd2 to 73aacda Compare March 22, 2024 07:45

storopoli force-pushed the js/max-weight-deprecation branch 2 times, most recently from 1bf5bcf to d2b8e66 Compare March 27, 2024 06:55

storopoli force-pushed the js/max-weight-deprecation branch from d2b8e66 to 798ed8c Compare March 29, 2024 09:27

evanlinjin approved these changes Apr 2, 2024

View reviewed changes

evanlinjin merged commit 2bb6540 into bitcoindevkit:master Apr 2, 2024

storopoli deleted the js/max-weight-deprecation branch April 2, 2024 11:28

notmandatory mentioned this pull request Apr 12, 2024

Release 1.0.0-alpha.9 #1399

Closed

25 tasks

Conversation

storopoli commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Changelog notice

Fixed

Checklists

All Submissions:

Bugfixes:

Uh oh!

This comment was marked as spam.

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

evanlinjin Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

evanlinjin Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

storopoli Mar 1, 2024 • 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!

Uh oh!

evanlinjin commented Mar 1, 2024

Uh oh!

storopoli commented Mar 1, 2024

Uh oh!

evanlinjin commented Mar 1, 2024

Uh oh!

evanlinjin commented Mar 1, 2024

Uh oh!

storopoli commented Mar 2, 2024

Uh oh!

evanlinjin Mar 3, 2024

Choose a reason for hiding this comment

Uh oh!

storopoli Mar 3, 2024

Choose a reason for hiding this comment

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

evanlinjin commented Mar 22, 2024

Uh oh!

storopoli commented Mar 22, 2024

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

storopoli commented Feb 15, 2024 •

edited

Loading

storopoli Mar 1, 2024 •

edited

Loading