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

refactor(wallet): Use Psbt::sighash_ecdsa for computing sighashes #1424

Merged
merged 2 commits into from
Jun 25, 2024
Merged

refactor(wallet): Use Psbt::sighash_ecdsa for computing sighashes #1424

merged 2 commits into from
Jun 25, 2024

Conversation

ValuedMammal
Copy link
Contributor

@ValuedMammal ValuedMammal commented May 3, 2024 •
edited
Loading

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. The logic for computing the taproot sighash is unchanged and extracted to a new helper function compute_tap_sighash.

  • Unimplement ComputeSighash
  • Try de-duplicating code by using Psbt::sighash_ecdsa. see Update rust bitcoin #1023 (comment)
  • Not done in this PR: Consider removing unused SignerError variants

fixes #1038

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

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

@ValuedMammal ValuedMammal self-assigned this May 3, 2024
@nondiremanuel nondiremanuel added this to the 1.0.0-alpha milestone May 7, 2024
@ValuedMammal ValuedMammal force-pushed the refactor/signer-sighash branch from 0b7e13b to 75e1f23 Compare May 13, 2024 22:01
@ValuedMammal ValuedMammal force-pushed the refactor/signer-sighash branch from 75e1f23 to a4ed0e9 Compare May 25, 2024 00:53
@ValuedMammal ValuedMammal marked this pull request as ready for review May 25, 2024 01:02
@ValuedMammal ValuedMammal force-pushed the refactor/signer-sighash branch from a4ed0e9 to c71d30c Compare May 25, 2024 12:56
@notmandatory notmandatory added module-wallet api A breaking API change labels Jun 1, 2024
@ValuedMammal ValuedMammal mentioned this pull request Jun 10, 2024
Merged
8 tasks
@evanlinjin
Copy link
Member

I'll make it a priority to review this next week.

@ValuedMammal ValuedMammal force-pushed the refactor/signer-sighash branch from c71d30c to 1f33c0b Compare June 16, 2024 23:53
oleonardolima
oleonardolima reviewed Jun 19, 2024
View reviewed changes
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

It's looking good! Thanks for addressing it.
Using the ctx in a better way, and improving the errors definitely helps.

crates/wallet/src/wallet/signer.rs Outdated Show resolved Hide resolved
crates/wallet/src/wallet/signer.rs Outdated
Comment on lines 519 to 522
let (msg, hash_ty) = psbt
.sighash_ecdsa(input_index, &mut sighasher)
.map_err(SignerError::Psbt)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'm wondering if calling it msg instead of sighash or hash can lead to wrong interpretations of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dare say that msg is the correct interpretation for the return type of sighash_ecdsa

@ValuedMammal ValuedMammal force-pushed the refactor/signer-sighash branch from 1f33c0b to 0a7ae6c Compare June 20, 2024 15:28
@ValuedMammal
Copy link
Contributor Author

  • Addressed comments from @oleonardolima
  • Also updated the description to include a reminder about unused SignerError variants

notmandatory
notmandatory approved these changes Jun 21, 2024
View reviewed changes
Copy link
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 0a7ae6c

I like the cleanup, using functions instead of traits is a nice simplification.

@notmandatory
Copy link
Member

notmandatory commented Jun 21, 2024 •
edited
Loading

We need a follow-up research issue to figure out how to remove signing from bdk_wallet and also how to make more use the rust-bitcoin's Psbt::sign().

I suspect this will lead into the related refactoring using the new rust-miniscript plan module (instead of current bdk_wallet policy module), see #1382.

oleonardolima
oleonardolima approved these changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

ACK 0a7ae6c

@oleonardolima oleonardolima mentioned this pull request Jun 21, 2024
Open
@notmandatory
Copy link
Member

@evanlinjin do you want to take a look at this one? Otherwise I'm inclined to merge it.

@ValuedMammal
refactor(signer): Remove trait ComputeSighash
f2a2dae
@ValuedMammal
ref(signer): Use Psbt::sighash_ecdsa for computing sighashes
55a1729 
- Change param `hash` to `&Message` in `sign_psbt_ecdsa`
- Remove unused methods `compute_legacy_sighash`,
and `compute_segwitv0_sighash`.
- Match on `self.ctx` when signing for `SignerWrapper<PrivateKey>`
@ValuedMammal ValuedMammal force-pushed the refactor/signer-sighash branch from 0a7ae6c to 55a1729 Compare June 24, 2024 13:09
@evanlinjin
Copy link
Member

I need to be honest, I don't have time to review this thoroughly. However, everything looks correct from a scan.

@notmandatory notmandatory merged commit 275e069 into bitcoindevkit:master Jun 25, 2024
12 checks passed
@ValuedMammal ValuedMammal deleted the refactor/signer-sighash branch June 26, 2024 02:06
@notmandatory notmandatory changed the title Remove trait ComputeSighash refactor(wallet): Remove trait ComputeSighash, use Psbt::sighash_ecdsa for computing sighashes. Jul 20, 2024
@notmandatory notmandatory changed the title refactor(wallet): Remove trait ComputeSighash, use Psbt::sighash_ecdsa for computing sighashes. refactor(wallet): Remove trait ComputeSighash, use Psbt::sighash_ecdsa for sighashes. Jul 20, 2024
@notmandatory notmandatory changed the title refactor(wallet): Remove trait ComputeSighash, use Psbt::sighash_ecdsa for sighashes. refactor(wallet): Use Psbt::sighash_ecdsa for computing sighashes, remove ComputeSighash. Jul 20, 2024
@notmandatory notmandatory changed the title refactor(wallet): Use Psbt::sighash_ecdsa for computing sighashes, remove ComputeSighash. refactor(wallet): Use Psbt::sighash_ecdsa for computing sighashes. Jul 20, 2024
@notmandatory notmandatory changed the title refactor(wallet): Use Psbt::sighash_ecdsa for computing sighashes. refactor(wallet): Use Psbt::sighash_ecdsa for computing sighashes Jul 20, 2024
@notmandatory notmandatory mentioned this pull request Jul 20, 2024
Closed
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleonardolima oleonardolima oleonardolima approved these changes

@notmandatory notmandatory notmandatory approved these changes

Assignees

@ValuedMammal ValuedMammal

Labels
api A breaking API change module-wallet
Projects
BDK
Archived in project
Milestone
1.0.0-alpha
Development

Successfully merging this pull request may close these issues.

Remove ComputeSighash
5 participants
@ValuedMammal @evanlinjin @notmandatory @oleonardolima @nondiremanuel