Skip to content

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

Merged
notmandatory merged 2 commits intobitcoindevkit:masterfrom
ValuedMammal:refactor/signer-sighash
Jun 25, 2024
Merged

refactor(wallet): Use Psbt::sighash_ecdsa for computing sighashes#1424
notmandatory merged 2 commits intobitcoindevkit:masterfrom
ValuedMammal:refactor/signer-sighash

Conversation

@ValuedMammal
Copy link
Copy Markdown
Collaborator

@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
Copy Markdown
Member

evanlinjin commented Jun 15, 2024

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
Copy Markdown
Collaborator

@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
Comment on lines +519 to +522
let (msg, hash_ty) = psbt
.sighash_ecdsa(input_index, &mut sighasher)
.map_err(SignerError::Psbt)?;
Copy link
Copy Markdown
Collaborator

@oleonardolima oleonardolima Jun 19, 2024

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
Copy Markdown
Collaborator Author

@ValuedMammal ValuedMammal Jun 20, 2024

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
Copy Markdown
Collaborator Author

ValuedMammal commented Jun 20, 2024

  • 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
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 0a7ae6c

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

@notmandatory
Copy link
Copy Markdown
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 bitcoindevkit/bdk_wallet#4.

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

@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 Apr 7, 2025
Closed
@notmandatory
Copy link
Copy Markdown
Member

notmandatory commented Jun 23, 2024

@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
Copy Markdown
Member

evanlinjin commented Jun 25, 2024

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

@notmandatory notmandatory notmandatory approved these changes

@oleonardolima oleonardolima oleonardolima approved these changes

Assignees

@ValuedMammal ValuedMammal

Labels

api A breaking API change module-wallet

Projects

BDK Wallet
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