Conversation
|
Done reviewing 81ef698. Overall looks great. I apologize for what an effort this was. |
|
This should fix #916 |
tcharding
left a comment
There was a problem hiding this comment.
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.
f940de6 to
6259639
Compare
|
Clean! Nice one. |
de47a98 to
4b8a1f8
Compare
evanlinjin
left a comment
There was a problem hiding this comment.
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.
evanlinjin
left a comment
There was a problem hiding this comment.
I have looked through everything and tested the examples and made sure they all still work 😄
In my opinion, these are mandatory changes:
- Do not pin dependencies: #1023 (comment)
- Better description for multipath error: #1023 (comment)
- Incorrect weight calculation: #1023 (comment)
After which, I will give an ACK.
...rust-bitcoin 0.30.0
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.
4b8a1f8 to
1da3b30
Compare
|
Rebased and fixed review comments. |
evanlinjin
left a comment
There was a problem hiding this comment.
ACK 1da3b30
Looked through everything and made sure the examples still worked as expected (which they did).
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
Description
Updates to rust-bitcoin 0.30.0 and miniscript 0.10.0
Not covered in this PR:
max_weight_to_satisfyinstead ofmax_satisfaction_weight#1036. Although the latter is deprecated, I think it's better if I update it in a separate PR, as this one is pretty big already.bitcoin::FeeRateinstead ofbdk::FeeRate#1037ComputeSighash#1038Heads up, I'm explicitly denying multipath descriptors until we have better tests for them. See the commit message of 23fba7a
Changelog notice
rust-bitcoin0.30.0 andminiscript10.0.0Checklists
All Submissions:
cargo fmtandcargo clippybefore committing