Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update rust bitcoin #1023

Merged
merged 12 commits into from
Aug 4, 2023
Merged

Update rust bitcoin #1023

merged 12 commits into from
Aug 4, 2023

Conversation

danielabrozzoni
Copy link
Member

@danielabrozzoni danielabrozzoni commented Jun 28, 2023 •
edited
Loading

Description

Updates to rust-bitcoin 0.30.0 and miniscript 0.10.0

Not covered in this PR:

Heads up, I'm explicitly denying multipath descriptors until we have better tests for them. See the commit message of 23fba7a

Changelog notice

  • Update to rust-bitcoin 0.30.0 and miniscript 10.0.0

Checklists

All Submissions:

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

@danielabrozzoni danielabrozzoni marked this pull request as draft June 28, 2023 13:34
@notmandatory notmandatory added this to the 1.0.0-alpha.2 milestone Jul 4, 2023
@apoelstra
Copy link

Done reviewing 81ef698.

Overall looks great. I apologize for what an effort this was.

@notmandatory notmandatory mentioned this pull request Jul 13, 2023
Closed
24 tasks
@notmandatory
Copy link
Member

This should fix #916

@notmandatory notmandatory mentioned this pull request Jul 13, 2023
Closed
tcharding
tcharding reviewed Jul 13, 2023
View reviewed changes
Copy link
Contributor

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

Wow you guys really use rust-bitcoin a lot. I hope we can stabalize the API soon so you don't have to go through all this work too many times. Thanks for being patient with our wild API changes.

crates/bdk/Cargo.toml Outdated Show resolved Hide resolved
crates/bdk/src/descriptor/mod.rs Outdated Show resolved Hide resolved
crates/bdk/src/descriptor/policy.rs Outdated Show resolved Hide resolved
crates/bdk/src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
crates/bdk/src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
crates/bdk/tests/wallet.rs Outdated Show resolved Hide resolved
crates/bdk/tests/wallet.rs Outdated Show resolved Hide resolved
crates/chain/src/spk_iter.rs Outdated Show resolved Hide resolved
nursery/tmp_plan/src/lib.rs Outdated Show resolved Hide resolved
nursery/tmp_plan/src/lib.rs Outdated Show resolved Hide resolved
@notmandatory notmandatory mentioned this pull request Jul 15, 2023
Closed
19 tasks
@danielabrozzoni danielabrozzoni force-pushed the update_rust_bitcoin branch 13 times, most recently from f940de6 to 6259639 Compare July 19, 2023 17:05
@tcharding
Copy link
Contributor

Clean! Nice one.

@danielabrozzoni danielabrozzoni force-pushed the update_rust_bitcoin branch 2 times, most recently from de47a98 to 4b8a1f8 Compare July 25, 2023 15:11
@nondiremanuel nondiremanuel mentioned this pull request Jul 29, 2023
Closed
19 tasks
evanlinjin
evanlinjin reviewed Aug 2, 2023
View reviewed changes
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Thank you for this work!

Can I suggest to use .expect() instead of .unwrap() in some places?

I understand that it becomes tedious to do it everywhere, especially if there are multiple calls to .at_derivation_index() (in which case I would suggest to leave a comment explaining why we don't expect a hardened wildcard).

An explanation as to why we do not expect an error goes a long way.

crates/bdk/src/descriptor/error.rs Outdated Show resolved Hide resolved
crates/bdk/src/descriptor/template.rs Show resolved Hide resolved
nursery/coin_select/src/coin_selector.rs Outdated Show resolved Hide resolved
crates/electrum/Cargo.toml Outdated Show resolved Hide resolved
crates/chain/src/spk_txout_index.rs Outdated Show resolved Hide resolved
crates/bdk/src/wallet/mod.rs Show resolved Hide resolved
evanlinjin
evanlinjin requested changes Aug 3, 2023
View reviewed changes
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

I have looked through everything and tested the examples and made sure they all still work 😄

In my opinion, these are mandatory changes:

After which, I will give an ACK.

@danielabrozzoni
Update bdk_chain to bitcoin 0.30.0
853d361
@danielabrozzoni
Update bdk to bitcoin 0.30.0
f4e3ba3
@danielabrozzoni
Update crates/electrum to bitcoin 0.30.0
91b0f0b
@danielabrozzoni
Update crates/esplora to rust-bitcoin 0.30.0
6eb0795
@danielabrozzoni
Update wallet_esplora and wallet_esplora_async to...
6c29e53 
...rust-bitcoin 0.30.0
@danielabrozzoni
Update coin_select to rust-bitcoin 0.30.0
b8ac16d
@danielabrozzoni
Update tmp_plan to rust-bitcoin 0.30.0
468d2a0
@danielabrozzoni
Update example_cli to rust-bitcoin 0.30.0
2949bdc
@danielabrozzoni
Update example_electrum to rust-bitcoin 0.30.0
3dac3f9
@danielabrozzoni
Update wallet_electrum to rust-bitcoin 0.30.0
b73385d
@danielabrozzoni
Explicitly deny multipath keys
792b39f 
Although there is *some* code to handle multipath keys inside bdk,
it's all untested, and from a few quick tests it
seems that it's pretty easy to find buggy edge cases.
Better to deny multipath descs for now, and revisit the
decision once we work on supporting multidescriptor wallets.
@danielabrozzoni
ci: Pin rustls to keep the MSRV
1da3b30
@danielabrozzoni
Copy link
Member Author

Rebased and fixed review comments.

evanlinjin
evanlinjin approved these changes Aug 4, 2023
View reviewed changes
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

ACK 1da3b30

Looked through everything and made sure the examples still worked as expected (which they did).

@evanlinjin evanlinjin merged commit feafaac into bitcoindevkit:master Aug 4, 2023
This was referenced Aug 7, 2023
Merged
Closed
@nondiremanuel nondiremanuel mentioned this pull request Aug 8, 2023
Closed
26 tasks
@danielabrozzoni danielabrozzoni deleted the update_rust_bitcoin branch October 11, 2023 12:14
@notmandatory notmandatory mentioned this pull request Oct 11, 2023
Closed
12 tasks
@ValuedMammal ValuedMammal mentioned this pull request May 3, 2024
Merged
5 tasks
notmandatory added a commit that referenced this pull request Jun 25, 2024
@notmandatory
Merge #1424: Remove trait ComputeSighash
275e069 
55a1729 ref(signer): Use `Psbt::sighash_ecdsa` for computing sighashes (valued mammal)
f2a2dae refactor(signer): Remove trait ComputeSighash (valued mammal)

Pull request description:

  This PR does some cleanup of the `bdk_wallet` signer module most notably by removing the internal trait `ComputeSighash` and replacing old code for computing the sighash (for legacy and segwit context) with a single method [`Psbt::sighash_ecdsa`](https://docs.rs/bitcoin/0.31.2/bitcoin/psbt/struct.Psbt.html#method.sighash_ecdsa). The logic for computing the taproot sighash is unchanged and extracted to a new helper function `compute_tap_sighash`.

  - [x] Unimplement `ComputeSighash`
  - [x] Try de-duplicating code by using `Psbt::sighash_ecdsa`. see #1023 (comment)
  - Not done in this PR: Consider removing unused `SignerError` variants

  fixes #1038

  ### Notes to the reviewers

  ### Changelog notice

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

Top commit has no ACKs.

Tree-SHA512: 56af3c9c463513ca3bae5480aa5b90d78de119c3c09c824a7220eb6832d5f403b172afc8168228918ea1adabb4bf8fca858790adfebf84fc334b4fc1cc99d3cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evanlinjin evanlinjin evanlinjin approved these changes

@tcharding tcharding tcharding approved these changes

@apoelstra apoelstra apoelstra approved these changes

Assignees

@danielabrozzoni danielabrozzoni

Labels
None yet
Projects
BDK
Archived in project
Milestone
1.0.0-alpha.2
Development

Successfully merging this pull request may close these issues.
5 participants
@danielabrozzoni @apoelstra @notmandatory @tcharding @evanlinjin