bump(deps): upgrade rust `bitcoin` to `0.32.0`, `miniscript` to `0.12.0` and others #1448

oleonardolima · 2024-05-22T23:08:56Z

Description

This PR focuses on upgrading the rust-bitcoin and miniscript versions, to 0.32.0 and 0.12.0, respectively. It also bumps the versions of other BDK ecosystem crates that also rely on both rust-bitcoin and miniscript, being:

electrum-client chore(deps): upgrade rust-bitcoin to 0.32.0 rust-electrum-client#133
esplora-client chore(deps): upgrade rust-bitcoin to 0.32.0 rust-esplora-client#85
hwi chore(deps): upgrade rust-bitcoin to 0.32.0 rust-hwi#99

I structured the PR in multiple commits, with closed scope, being one for each BDK crate being upgraded, and one for each kind of fix and upgrade required, it seems like a lot of commits (that should be squashed before merging), but I think it'll make it easier during review phase.

In summary I can already mention some of the main changes:

using compute_txid() instead of deprecated txid()
using minimal_non_dust() instead of dust_value()
using the renamed signature and sighash_type fields
using proper sighash::P2wpkhError, sighash::TaprootError instead of older sighash::Error
conversion from Network to new expected NetworkKind feat: use new NetworkKind throughout the BDK codebase #1465
conversion from the new Weight type to current expected usize feat: use new Weight type throughout the BDK codebase, instead of usize for weight units #1466
using .into() to convert from AbsLockTime and RelLockTime to absolute::LockTime and relative::LockTime
using Message::from_digest() instead of relying on deprecated ThirtyTwoByteHash trait.
updating the miniscript policy and dsl to proper expect and handle new Threshold type, instead of the previous two parameters.

Notes to the reviewers

Again, I structured the PR in multiple commits, with closed scope, being one for each BDK crate being upgraded, and one for each kind of fix and upgrade required, it seems like a lot of commits (that should be squashed before merging), but I think it'll make it easier during review phase.

It should definitely be carefully reviewed, especially the last commits for the wallet crate scope, the ones with the semantic fix(wallet).

I would also appreciate if @tcharding reviewed it, as he gave a try in the past (#1400 ), and I relied on some of it for the policy part of it, other rust-bitcoin maintainers reviews are a definitely welcome 😁

Changelog notice

Use compute_txid() instead of deprecated txid()
Use minimal_non_dust() instead of dust_value()
Use signature and sighash_type fields, instead of previous sig and hash_type
Use sighash::P2wpkhError, and sighash::TaprootError instead of older sighash::Error
Converts from Network to NetworkKind, where expected
Converts from Weight type to current used usize
Use .into() to convert from AbsLockTime and RelLockTime to absolute::LockTime and relative::LockTime
Remove use of deprecated ThirtyTwoByteHash trait, use Message::from_digest()
Update the miniscript policy and dsl macros to proper expect and handle new Threshold type, instead of the previous two parameters.

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

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

oleonardolima · 2024-06-06T23:18:06Z

NOTE: related to the mentioned todo's on the description, I'll open a new PR to add more scope/context to this one with all the changes required due to the new NetworkKind and new Weight.

ValuedMammal · 2024-06-07T11:02:59Z

crates/wallet/src/descriptor/error.rs

@@ -37,7 +37,7 @@ pub enum Error {
    /// Error during base58 decoding
    Base58(bitcoin::base58::Error),
    /// Key-related error
-    Pk(bitcoin::key::Error),
+    Pk(bitcoin::key::ParsePublicKeyError),


I'm undecided if we need this Pk variant

What are your thoughts about it?

afaik it's not being used anywhere

crates/wallet/src/descriptor/policy.rs

crates/wallet/src/wallet/mod.rs

crates/wallet/src/wallet/signer.rs

ValuedMammal · 2024-06-07T11:19:29Z

crates/wallet/src/wallet/signer.rs

+    /// Error while computing the hash to sign a Taproot input.
+    SighashTaproot(sighash::TaprootError),
+    /// Error while computing the hash, out of bounds access on the transaction inputs.
+    InputsIndexError(transaction::InputsIndexError),


📌 Considering we already have a InputIndexOutOfRange error, I recommend consolidating these into one variant if possible.

@ValuedMammal Oh, thanks! Yes, that's much better, I'll fix it.

@ValuedMammal I did a deep-dive in rust-bitcoin, and now I'm wondering if we should have two variants, one for Psbt and another one for Transaction, as they have two in rust-bitcoin crate:

bitcoin::psbt::IndexOutOfBoundsError

bitcoin::transaction::InputsIndexError & bitcoin::transaction::OutputsIndexError

I updated it to be TxInputsIndexError instead, I also think for the current InputIndexOutOfRange it should be PsbtInputIndexOutOfRange(psbt::IndexOutOfBoundsError) instead, let me know what you think.

A simple solution would be to make SignerError::InputIndexOutOfRange into a struct with fields for index and length (essentially reimplementing IndexOutOfBoundsError) and then we have

impl From<transaction::InputsIndexError> for SignerError {}

similar to what you have here, but it shouldn't be a blocker to this PR. Accessing the outputs of a tx or psbt doesn't appear to be a concern in this module.

ValuedMammal · 2024-06-07T11:27:41Z

@tcharding Do the changes related to the new Threshold still look ok to you?

notmandatory

I have a couple small suggestions but otherwise this looks almost ready. Great work!

I also created #1469 to remove tmp_plan, should it be done with this PR?

notmandatory · 2024-06-08T01:27:10Z

crates/chain/src/descriptor_ext.rs

@@ -31,7 +31,7 @@ impl DescriptorExt for Descriptor<DescriptorPublicKey> {
        self.at_derivation_index(0)
            .expect("descriptor can't have hardened derivation")
            .script_pubkey()
-            .dust_value()
+            .minimal_non_dust()
            .to_sat()


📌 since this is a public API should we change the DescriptorExt trait to make this an Amount instead of u64?

@notmandatory Yes, I do agree. I'll update it as well.

@notmandatory I'll address it in another PR, it ends up breaking and requiring changes on the CoinSelectorOpt, and probably will also raise some discussions, which is outside the scope of this PR.

crates/esplora/Cargo.toml

crates/wallet/src/descriptor/policy.rs

notmandatory · 2024-06-08T02:17:09Z

crates/wallet/src/types.rs

@@ -72,7 +72,7 @@ pub struct WeightedUtxo {
    /// properly maintain the feerate when adding this input to a transaction during coin selection.
    ///
    /// [weight units]: https://en.bitcoin.it/wiki/Weight_units
-    pub satisfaction_weight: usize,
+    pub satisfaction_weight: usize, // FIXME: (@leonardo) I think it should expect the new bitcoin-units `Weight` type instead of usize.


👍 agreed, now that there's a Weight type we should use it. If you think it's too big a change to add to this PR feel free to make an issue to fix it in a future PR.

@notmandatory I wasn't so sure about, but I think it adds unrelated scope/context, I'm addressing this one in #1468

crates/wallet/src/wallet/mod.rs

crates/esplora/Cargo.toml

tcharding · 2024-06-11T21:53:19Z

crates/esplora/Cargo.toml

-# use these dependencies if you need to enable their /no-std features
-bitcoin = { version = "0.31.0", optional = true, default-features = false }
-miniscript = { version = "11.0.0", optional = true, default-features = false }
+# The `no-std` feature it's implied when the `std` feature is disabled.


This comment is not really correct, there is no no-std feature anymore in rust-bitcoin. Its in another place also.

Good point! I think it's best to just remove the comments altogether then.
fyi @notmandatory

tcharding · 2024-06-11T22:11:52Z

@tcharding Do the changes related to the new Threshold still look ok to you?

Handwavey response, if it builds ship it.

Take everything I saw with a pinch of salt, I'm not that good with miniscript.

I'm not sure, I don't exactly know what you are aiming for:

Just make it build - then CI can answer that.
Make it build and not be obviously wrong - seems like the usage is ok to me.
Fully utilise miniscript's new Threshold type - definitely currently not doing this.

I didn't grok the fragment macro totally but was surprised to see expect called if inputs to the macro are invalid thresholds. Passing the closure is a bit hacky but I totally feel your pain, patching macros is an epic PITA.

Sorry I can't be much more help than that.

ValuedMammal · 2024-06-12T11:47:02Z

The summary of changes in the description certainly helps, thanks for that. In terms of review, the individual commits are less useful since realistically this should be a single commit - the one that finally builds with tests passing.

ValuedMammal · 2024-06-12T11:48:35Z

I'm not sure, I don't exactly know what you are aiming for:

1. Just make it build - then CI can answer that.

2. Make it build and not be obviously wrong - seems like the usage is ok to me.

3. Fully utilise `miniscript`'s new `Threshold` type - definitely currently not doing this.

Mostly number 2 for now, ha. All good, appreciate you @tcharding !

crates/wallet/src/descriptor/policy.rs

crates/wallet/src/wallet/signer.rs

oleonardolima · 2024-06-12T13:26:53Z

The summary of changes in the description certainly helps, thanks for that. In terms of review, the individual commits are less useful since realistically this should be a single commit - the one that finally builds with tests passing.

Nice, thanks for the feedback! I'll squash the commits into a single one.

deps(chain): bump `bitcoin` to `0.32.0`, miniscript to `12.0.0` fix(chain): use `minimal_non_dust()` instead of `dust_value()` fix(chain): use `compute_txid()` instead of `txid` deps(testenv): bump `electrsd` to `0.28.0` deps(electrum): bump `electrum-client` to `0.20.0` fix(electrum): use `compute_txid()` instead of `txid` deps(esplora): bump `esplora-client` to `0.8.0` deps(bitcoind_rpc): bump `bitcoin` to `0.32.0`, `bitcoincore-rpc` to `0.19.0` fix(bitcoind_rpc): use `compute_txid()` instead of `txid` fix(nursery/tmp_plan): use proper `sighash` errors, and fix the expected `Signature` fields fix(sqlite): use `compute_txid()` instead of `txid` deps(hwi): bump `hwi` to `0.9.0` deps(wallet): bump `bitcoin` to `0.32.0`, miniscript to `12.0.0` fix(wallet): use `compute_txid()` and `minimal_non_dust()` - update to use `compute_txid()` instead of deprecated `txid()` - update to use `minimal_non_dust()` instead of `dust_value()` - remove unused `bitcoin::hex::FromHex`. fix(wallet): uses `.into` conversion on `Network` for `NetworkKind` - uses `.into()` when appropriate, otherwise use the explicit `NetworkKind`, and it's `.is_mainnet()` method. fix(wallet): add P2wpkh, Taproot, InputsIndex errors to `SignerError` fix(wallet): fields on taproot, and ecdsa `Signature` structure fix(wallet/wallet): convert `Weight` to `usize` for now - converts the `bitcoin-units::Weight` type to `usize` with help of `to_wu()` method. - it should be updated/refactored in the future to handle the `Weight` type throughout the code instead of current `usize`, only converting it for now. - allows the usage of deprecated `is_provably_unspendable()`, needs further discussion if suggested `is_op_return` is suitable. - update the expect field to `signature`, as it was renamed from `sig`. fix(wallet/wallet): use `is_op_return` instead of `is_provably_unspendable` fix(wallet/wallet): use `relative::Locktime` instead of `Sequence` fix(wallet/descriptor): use `ParsePublicKeyError` fix(wallet/descriptor): use `.into()` to convert from `AbsLockTime` and `RelLockTime` to `absolute::LockTime` and `relative::LockTime` fix(wallet/wallet): use `Message::from_digest()` instead of relying on deprecated `ThirtyTwoByteHash` trait. fix(wallet/descriptor+wallet): expect `Threshold` type, and handle it internally fix(wallet/wallet): remove `0x` prefix from expected `TxId` display fix(examples): use `compute_txid()` instead of `txid` fix(ci): remove usage of `bitcoin/no-std` feature - remove comment: `# The `no-std` feature it's implied when the `std` feature is disabled.`

ValuedMammal · 2024-06-12T14:04:45Z

ACK 2a45640

crates/wallet/src/descriptor/policy.rs

crates/wallet/src/wallet/tx_builder.rs

Methods `make_multi` and `make_multi_a` were essentially the same thing except for a different const generic param on `Threshold`. So we rm `make_multi_a` and put a const generic param on `make_multi`.

notmandatory

ACK 1120081

438cd46 refactor(wallet)!: change WeightedUtxo to use Weight type (Leonardo Lima) Pull request description: fixes #1466 depends on #1448  ### Description This PR is a follow-up on top of #1448, and should be rebased and merged after it, it uses the rust-bitcoin `Weight` type instead of the current `usize` usage. NOTE: ~~It also adds a new `MiniscriptError` variant, and remove the `.unwrap()` usage.~~ As suggested I'll address this on another issue #1485, trying to reproduce the error first.  ### Notes to the reviewers It should be ready to review after #1448 gets merged.  ### Changelog notice - Change `WeightedUtxo` `satisfaction_weight` has been changed to use `Weight` type. - Change `TxBuilder` methods `add_foreign_utxo` and `add_foreign_utxo_with_sequence` to expect `Weight` as `satisfaction_weight` type.   ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing ACKs for top commit: storopoli: Anyways, ACK 438cd46 notmandatory: ACK 438cd46 Tree-SHA512: 1998fe659833da890ce07aa746572ae24d035e636732c1a11b7828ffed48e753adb4108f42d00b7cd05e6f45831a7a9840faa26f94058fc13760497837af002f

oleonardolima mentioned this pull request May 23, 2024

Upgrade rust-bitcoin to 0.32 #1422

Closed

7 tasks

notmandatory assigned oleonardolima Jun 3, 2024

notmandatory added the api A breaking API change label Jun 3, 2024

notmandatory added this to the 1.0.0-alpha milestone Jun 3, 2024

oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch 11 times, most recently from 49e2290 to 9d6c6ee Compare June 6, 2024 19:46

oleonardolima changed the title ~~wip(bump(deps)): upgrade rust bitcoin to 0.32.0~~ bump(deps): upgrade rust bitcoin to 0.32.0, miniscript to 0.12.0 and others Jun 6, 2024

oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from 4cf2e53 to e6a93de Compare June 6, 2024 21:58

oleonardolima marked this pull request as ready for review June 6, 2024 22:27

This was referenced Jun 6, 2024

feat: use new NetworkKind throughout the BDK codebase #1465

Open

feat: use new Weight type throughout the BDK codebase, instead of usize for weight units #1466

Closed

ValuedMammal reviewed Jun 7, 2024

View reviewed changes

oleonardolima mentioned this pull request Jun 7, 2024

refactor(wallet)!: Use Weight type instead of usize #1468

Merged

3 tasks

notmandatory reviewed Jun 8, 2024

View reviewed changes

oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch 2 times, most recently from 950c166 to e048247 Compare June 11, 2024 21:03

tcharding reviewed Jun 11, 2024

View reviewed changes

oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from e048247 to 29e65e3 Compare June 12, 2024 00:54

oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch 5 times, most recently from 98f67ad to fe5cd18 Compare June 12, 2024 02:25

ValuedMammal requested changes Jun 12, 2024

View reviewed changes

crates/wallet/src/descriptor/policy.rs Show resolved Hide resolved

crates/wallet/src/wallet/signer.rs Outdated Show resolved Hide resolved

oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from fe5cd18 to cecb349 Compare June 12, 2024 13:20

oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from cecb349 to acee6b5 Compare June 12, 2024 13:31

oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from acee6b5 to 2a45640 Compare June 12, 2024 13:32

oleonardolima requested review from ValuedMammal, notmandatory and tcharding June 12, 2024 13:32

ValuedMammal approved these changes Jun 12, 2024

View reviewed changes

evanlinjin reviewed Jun 12, 2024

View reviewed changes

crates/wallet/src/descriptor/policy.rs Outdated Show resolved Hide resolved

evanlinjin reviewed Jun 12, 2024

View reviewed changes

crates/wallet/src/wallet/tx_builder.rs Show resolved Hide resolved

chore(wallet): rm dup code

1120081

Methods `make_multi` and `make_multi_a` were essentially the same thing except for a different const generic param on `Threshold`. So we rm `make_multi_a` and put a const generic param on `make_multi`.

notmandatory approved these changes Jun 13, 2024

View reviewed changes

notmandatory merged commit 3b040a7 into bitcoindevkit:master Jun 13, 2024
12 checks passed

notmandatory mentioned this pull request Jun 13, 2024

Test the bitcoin v0.32.0-rc1 release #1400

Closed

oleonardolima deleted the deps/upgrade-rust-bitcoin-to-0.32.0 branch June 13, 2024 13:44

oleonardolima mentioned this pull request Jun 13, 2024

What are strategies to handle the error return of max_weigh_to_satisfy ? rust-bitcoin/rust-miniscript#695

Open

notmandatory mentioned this pull request Jun 14, 2024

Release 1.0.0-alpha.13 #1471

Closed

31 tasks

This was referenced Jun 21, 2024

chore: should we remove unused SignerError variants ? #1484

Open

feat: try to reproduce an insatiable descriptor error & improve/remove .unwrap usage #1485

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bump(deps): upgrade rust `bitcoin` to `0.32.0`, `miniscript` to `0.12.0` and others #1448

bump(deps): upgrade rust `bitcoin` to `0.32.0`, `miniscript` to `0.12.0` and others #1448

oleonardolima commented May 22, 2024 •

edited

Loading

oleonardolima commented Jun 6, 2024 •

edited

Loading

ValuedMammal Jun 7, 2024

oleonardolima Jun 10, 2024

ValuedMammal Jun 10, 2024

ValuedMammal Jun 7, 2024

oleonardolima Jun 10, 2024

oleonardolima Jun 11, 2024 •

edited

Loading

oleonardolima Jun 12, 2024 •

edited

Loading

ValuedMammal Jun 12, 2024

ValuedMammal commented Jun 7, 2024

notmandatory left a comment

notmandatory Jun 8, 2024

oleonardolima Jun 10, 2024

oleonardolima Jun 11, 2024 •

edited

Loading

notmandatory Jun 8, 2024

oleonardolima Jun 10, 2024

tcharding Jun 11, 2024

oleonardolima Jun 12, 2024

tcharding commented Jun 11, 2024

ValuedMammal commented Jun 12, 2024

ValuedMammal commented Jun 12, 2024

oleonardolima commented Jun 12, 2024

ValuedMammal commented Jun 12, 2024

notmandatory left a comment

bump(deps): upgrade rust bitcoin to 0.32.0, miniscript to 0.12.0 and others #1448

bump(deps): upgrade rust bitcoin to 0.32.0, miniscript to 0.12.0 and others #1448

Conversation

oleonardolima commented May 22, 2024 • edited Loading

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

Bugfixes:

oleonardolima commented Jun 6, 2024 • 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

oleonardolima Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

oleonardolima Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValuedMammal commented Jun 7, 2024

notmandatory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleonardolima Jun 11, 2024 • 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

tcharding commented Jun 11, 2024

ValuedMammal commented Jun 12, 2024

ValuedMammal commented Jun 12, 2024

oleonardolima commented Jun 12, 2024

ValuedMammal commented Jun 12, 2024

notmandatory left a comment

Choose a reason for hiding this comment

bump(deps): upgrade rust `bitcoin` to `0.32.0`, `miniscript` to `0.12.0` and others #1448

bump(deps): upgrade rust `bitcoin` to `0.32.0`, `miniscript` to `0.12.0` and others #1448

oleonardolima commented May 22, 2024 •

edited

Loading

oleonardolima commented Jun 6, 2024 •

edited

Loading

oleonardolima Jun 11, 2024 •

edited

Loading

oleonardolima Jun 12, 2024 •

edited

Loading

oleonardolima Jun 11, 2024 •

edited

Loading