Skip to content
This repository was archived by the owner on Jan 22, 2026. It is now read-only.

chore(deps): upgrade rust-bitcoin to 0.32.0#99

Merged
notmandatory merged 5 commits intobitcoindevkit:masterfrom
oleonardolima:deps/upgrade-rust-bitcoin-to-0.32.0
Jun 4, 2024
Merged

chore(deps): upgrade rust-bitcoin to 0.32.0#99
notmandatory merged 5 commits intobitcoindevkit:masterfrom
oleonardolima:deps/upgrade-rust-bitcoin-to-0.32.0

Conversation

@oleonardolima
Copy link
Copy Markdown
Contributor

@oleonardolima oleonardolima commented May 4, 2024

partially fixes #1422

Description

It updates the rust-bitcoin dependency to 0.32.0, and the miniscript to 0.12.0.

Notes to the reviewers

It's open for any comments.

Changelog notice

  • Update the bitcoin crate dependency to 0.32.0
  • Update the miniscript crate dependency to 0.12.0
  • Expliciltly format the DerivationPath with m/ prefix before passing it as argument to HWI interface, the fmt::Display trait removed the m/ prefix on rust-bitcoin 0.32.0.

Checklists

All Submissions:

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

@oleonardolima oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from 3ff002f to f0f9bfd Compare May 4, 2024 21:02
@oleonardolima oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from da1b5e7 to 4fe5143 Compare May 15, 2024 01:19
@oleonardolima oleonardolima changed the title wip(deps): upgrade rust-bitcoin to 0.32.0 chore(deps): upgrade rust-bitcoin to 0.32.0 May 15, 2024
@oleonardolima oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from 4fe5143 to 8178fa7 Compare May 15, 2024 01:19
@oleonardolima oleonardolima marked this pull request as ready for review May 15, 2024 01:19
@oleonardolima
Copy link
Copy Markdown
Contributor Author

@notmandatory Is the CI known for being flaky ? It seems to have a problem importing the external hwi library 🤔

@oleonardolima oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from d675ae2 to c062eda Compare May 28, 2024 17:17
@notmandatory
Copy link
Copy Markdown
Member

notmandatory commented May 29, 2024

The good news is I confirmed this project CI was not working even without your changes, the bad news is I don't know how to fix it. I'm going to try getting all the emulators working on my local system and then figure out from there what needs to be updated.

@oleonardolima
Copy link
Copy Markdown
Contributor Author

oleonardolima commented May 29, 2024

The good news is I confirmed this project CI was not working even without your changes, the bad news is I don't know how to fix it. I'm going to try getting all the emulators working on my local system and then figure out from there what needs to be updates.

I've been trying that, I found that:

  • the ledger emulator is failing on latest release, but it works on https://github.com/LedgerHQ/speculos/pkgs/container/speculos/183595929?tag=sha-2c6cdf8, that's the last tag that succeded run on the CI.
  • the trezor emulator works just fine
  • but with both emulators there's some failure on the getkeypool command, it always returns a BadArgument error from HWI, I'll dig a little deeper and try to find the bug/change/version that introduced it.

@oleonardolima
Copy link
Copy Markdown
Contributor Author

oleonardolima commented May 29, 2024

The good news is I confirmed this project CI was not working even without your changes, the bad news is I don't know how to fix it. I'm going to try getting all the emulators working on my local system and then figure out from there what needs to be updates.

@notmandatory Also, if you manage to get the emulators running on macos easily let me know, I surrendered and I'm using containers/linux machines instead 😂

@oleonardolima oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch 7 times, most recently from 23f68f2 to e3f622e Compare June 4, 2024 00:21
@notmandatory
Copy link
Copy Markdown
Member

BTW, I was not able to get the emulators working locally on my mac, had to use the CI for testing.

@oleonardolima oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch 3 times, most recently from 3b246dd to 7b0b04e Compare June 4, 2024 01:05
@oleonardolima
Copy link
Copy Markdown
Contributor Author

oleonardolima commented Jun 4, 2024

BTW, I was not able to get the emulators working locally on my mac, had to use the CI for testing.

Yep, I did the same, not sure if it's because is macOS, or it's also hard to run on linux

@notmandatory
Copy link
Copy Markdown
Member

I merged #101, please rebase and then this one should be ready to go. Thanks!

@oleonardolima oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from 7b0b04e to 9eba3fd Compare June 4, 2024 01:44
@oleonardolima oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from 9eba3fd to be7933a Compare June 4, 2024 01:46
Copy link
Copy Markdown
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK be7933a

@oleonardolima
Copy link
Copy Markdown
Contributor Author

oleonardolima commented Jun 4, 2024

I merged #101, please rebase and then this one should be ready to go. Thanks!

Done, should be ready for another review and merge. ACK'd faster than my comment lol

@notmandatory notmandatory merged commit 5e88904 into bitcoindevkit:master Jun 4, 2024
This was referenced Jun 4, 2024
@oleonardolima oleonardolima deleted the deps/upgrade-rust-bitcoin-to-0.32.0 branch June 4, 2024 14:21
path: &DerivationPath,
expert: bool,
) -> Result<HWIExtendedPubKey, Error> {
let prefixed_path = format!("m/{}", path);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drats, that turned out ugly didn't it. Maybe we should have a function that explicitly does this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a specific function for that would help.

Copy link
Copy Markdown

@Kixunil Kixunil Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including m/ is actually incorrect/misunderstanding of the BIP. Why did you keep the incorrect behavior?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Kixunil , which BIP in particular do you mean?

Anyway this PR isn't meant to change how this lib works, only to bump the version of rust-bitcoin. If there's something we need to fix we can do it in another PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

32, see this issue: rust-bitcoin/rust-bitcoin#2451

However, I've just noticed this is still debated: rust-bitcoin/rust-bitcoin#2671

Copy link
Copy Markdown
Contributor Author

@oleonardolima oleonardolima Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including m/ is actually incorrect/misunderstanding of the BIP. Why did you keep the incorrect behavior?

Hey @Kixunil , which BIP in particular do you mean?

Anyway this PR isn't meant to change how this lib works, only to bump the version of rust-bitcoin. If there's something we need to fix we can do it in another PR.

Yes, I kept it because it would require changes on the upstream hwilib, ACK's, and so on, as Steve mentioned not the scope of this PR.

Thanks, I'll keep an eye on the issue discussion, and update it in another PR if appropriate.

notmandatory added a commit to bitcoindevkit/bdk that referenced this pull request Jun 13, 2024
…ipt` to `0.12.0` and others

1120081 chore(wallet): rm dup code (志宇)
2a45640 deps(bdk): bump `bitcoin` to `0.32.0`, miniscript to `12.0.0` (Leonardo Lima)

Pull request description:

  fixes #1422
  <!-- You can erase any parts of this template not applicable to your Pull Request. -->

  ### 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 bitcoindevkit/rust-electrum-client#133
  - esplora-client bitcoindevkit/rust-esplora-client#85
  - hwi bitcoindevkit/rust-hwi#99

  <ins>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.</ins>

  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` #1465
  - conversion from the new `Weight` type to current expected `usize` #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.

  <!-- Describe the purpose of this PR, what's being adding and/or fixed -->

  ### Notes to the reviewers
  <ins>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.</ins>

  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 😁

  <!-- In this section you can include notes directed to the reviewers, like explaining why some parts
  of the PR were done in a specific way -->

  ### Changelog notice
  > // TODO(@oleonardolima): Do another pass and double check the changes
  - 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.

  <!-- Notice the release manager should include in the release tag message changelog -->
  <!-- See https://keepachangelog.com/en/1.0.0/ for examples -->

  ### 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

  #### 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

ACKs for top commit:
  notmandatory:
    ACK 1120081

Tree-SHA512: ba1ab64603b41014d3f0866d846167f77d31959ca6f1d9c3181d5e543964f5d772e05651d63935ba7bbffeba41a66868d27de4c32129739b9ca50f3bbaf9f2a1
binary-hunter347iu added a commit to binary-hunter347iu/rust-hwi that referenced this pull request Sep 28, 2025
…to `0.32.0`

be7933a9f79f9a08cd0c2f0e44727c709efbe263 chore: bump version to `0.9.0` (Leonardo Lima)
ede9ea751519703221bab4ea485ef5e205de3da1 fix: format `DerivationPath` w/ prefix `m/` when using as hwilib function argument (Leonardo Lima)
7afc15a1098b4b61feb2be4c0247cd758263c3b3 fix: use `compute_txid()` instead of deprecated `txid()` (Leonardo Lima)
c15e1a2ece853a3154a9c73bf1a83de54f8ca43d chore: upgrade `miniscript` dependency to `0.12.0` (Leonardo Lima)
be5b382ab3543b72bebc925a54e24811f4c94314 chore: upgrade `bitcoin` dependency to `0.32.0` (Leonardo Lima)

Pull request description:

  <!-- You can erase any parts of this template not applicable to your Pull Request. -->

  `partially fixes` [#1422](bitcoindevkit/bdk#1422)

  ### Description

  It updates the `rust-bitcoin` dependency to `0.32.0`, and the `miniscript` to `0.12.0`.

  <!-- Describe the purpose of this PR, what's being adding and/or fixed -->

  ### Notes to the reviewers

  It's open for any comments.
  <!-- In this section you can include notes directed to the reviewers, like explaining why some parts
  of the PR were done in a specific way -->

  ### Changelog notice

  - Update the `bitcoin` crate dependency to `0.32.0`
  - Update the `miniscript` crate dependency to `0.12.0`
  - Expliciltly format the `DerivationPath` with `m/` prefix before passing it as argument to HWI interface, the `fmt::Display` trait removed the `m/` prefix on `rust-bitcoin` `0.32.0`.

  <!-- Notice the release manager should include in the release tag message changelog -->
  <!-- See https://keepachangelog.com/en/1.0.0/ for examples -->

  ### 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:
  notmandatory:
    ACK be7933a9f79f9a08cd0c2f0e44727c709efbe263

Tree-SHA512: 52bca723ae6e7dd9642c016d7881323be21d03bef05e9f9e21803e7fd5d8924c25d7a9731b4b3c61018648b4666e92ee1ca695ea8df8016f90a9446e8d64cefa
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants