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

oleonardolima · 2024-05-04T20:58:18Z

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 · 2024-05-15T01:20:30Z

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

src/types.rs

notmandatory · 2024-05-29T01:55:27Z

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 · 2024-05-29T14:31:59Z

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 · 2024-05-29T14:37:13Z

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 😂

notmandatory · 2024-06-04T00:42:54Z

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

oleonardolima · 2024-06-04T01:09:42Z

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 · 2024-06-04T01:39:59Z

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

function argument

notmandatory

ACK be7933a

oleonardolima · 2024-06-04T01:50:19Z

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

tcharding · 2024-06-04T21:49:47Z

src/interface.rs

@@ -224,8 +224,9 @@ impl HWIClient {
        path: &DerivationPath,
        expert: bool,
    ) -> Result<HWIExtendedPubKey, Error> {
+        let prefixed_path = format!("m/{}", path);


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

Yes, a specific function for that would help.

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.

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

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

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.

@tcharding

…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  ### 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.  ### 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 😁  ### 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.   ### 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

oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from 3ff002f to f0f9bfd Compare May 4, 2024 21:02

oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from da1b5e7 to 4fe5143 Compare May 15, 2024 01:19

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 force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from 4fe5143 to 8178fa7 Compare May 15, 2024 01:19

oleonardolima marked this pull request as ready for review May 15, 2024 01:19

oleonardolima mentioned this pull request May 28, 2024

Upgrade rust-bitcoin to 0.32 bitcoindevkit/bdk#1422

Closed

7 tasks

notmandatory reviewed May 28, 2024

View reviewed changes

src/types.rs Outdated Show resolved Hide resolved

notmandatory reviewed May 28, 2024

View reviewed changes

src/types.rs Outdated Show resolved Hide resolved

oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from d675ae2 to c062eda Compare May 28, 2024 17:17

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

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 requested a review from notmandatory June 4, 2024 01:17

oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from 7b0b04e to 9eba3fd Compare June 4, 2024 01:44

oleonardolima added 2 commits June 3, 2024 22:45

chore: upgrade bitcoin dependency to 0.32.0

be5b382

chore: upgrade miniscript dependency to 0.12.0

c15e1a2

oleonardolima added 3 commits June 3, 2024 22:45

fix: use compute_txid() instead of deprecated txid()

7afc15a

fix: format DerivationPath w/ prefix m/ when using as hwilib

ede9ea7

function argument

chore: bump version to 0.9.0

be7933a

oleonardolima force-pushed the deps/upgrade-rust-bitcoin-to-0.32.0 branch from 9eba3fd to be7933a Compare June 4, 2024 01:46

notmandatory approved these changes Jun 4, 2024

View reviewed changes

notmandatory merged commit 5e88904 into bitcoindevkit:master Jun 4, 2024
7 checks passed

This was referenced Jun 4, 2024

ci: Test CI #102

Closed

Depend on bitcoin v0.32.0-rc1 #98

Closed

oleonardolima deleted the deps/upgrade-rust-bitcoin-to-0.32.0 branch June 4, 2024 14:21

tcharding reviewed Jun 4, 2024

View reviewed changes

tcharding mentioned this pull request Jun 4, 2024

Consider Enabling explicitly formatting a derivation path with the m/ prefix rust-bitcoin/rust-bitcoin#2836

Open

oleonardolima mentioned this pull request Jun 6, 2024

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

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

oleonardolima commented May 4, 2024 •

edited

Loading

oleonardolima commented May 15, 2024

notmandatory commented May 29, 2024 •

edited

Loading

oleonardolima commented May 29, 2024 •

edited

Loading

oleonardolima commented May 29, 2024 •

edited

Loading

notmandatory commented Jun 4, 2024

oleonardolima commented Jun 4, 2024 •

edited

Loading

notmandatory commented Jun 4, 2024

notmandatory left a comment

oleonardolima commented Jun 4, 2024 •

edited

Loading

tcharding Jun 4, 2024

oleonardolima Jun 4, 2024

Kixunil Jun 8, 2024 •

edited

Loading

notmandatory Jun 8, 2024

Kixunil Jun 9, 2024

oleonardolima Jun 10, 2024 •

edited

Loading

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

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

Conversation

oleonardolima commented May 4, 2024 • edited Loading

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

oleonardolima commented May 15, 2024

notmandatory commented May 29, 2024 • edited Loading

oleonardolima commented May 29, 2024 • edited Loading

oleonardolima commented May 29, 2024 • edited Loading

notmandatory commented Jun 4, 2024

oleonardolima commented Jun 4, 2024 • edited Loading

notmandatory commented Jun 4, 2024

notmandatory left a comment

Choose a reason for hiding this comment

oleonardolima commented Jun 4, 2024 • edited Loading

tcharding Jun 4, 2024

Choose a reason for hiding this comment

oleonardolima Jun 4, 2024

Choose a reason for hiding this comment

Kixunil Jun 8, 2024 • edited Loading

Choose a reason for hiding this comment

notmandatory Jun 8, 2024

Choose a reason for hiding this comment

Kixunil Jun 9, 2024

Choose a reason for hiding this comment

oleonardolima Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

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

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

oleonardolima commented May 4, 2024 •

edited

Loading

notmandatory commented May 29, 2024 •

edited

Loading

oleonardolima commented May 29, 2024 •

edited

Loading

oleonardolima commented May 29, 2024 •

edited

Loading

oleonardolima commented Jun 4, 2024 •

edited

Loading

oleonardolima commented Jun 4, 2024 •

edited

Loading

Kixunil Jun 8, 2024 •

edited

Loading

oleonardolima Jun 10, 2024 •

edited

Loading