Skip to content

[signer] Add an option to explicitly allow using non-ALL sighashes #353

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
Merged
afilini merged 1 commit into from
May 27, 2021
Merged

[signer] Add an option to explicitly allow using non-ALL sighashes #353

afilini merged 1 commit into from
May 27, 2021

Conversation

@afilini
Copy link
Member

@afilini afilini commented May 26, 2021

Description

Instead of blindly using the sighash_type set in a psbt input, we now only sign SIGHASH_ALL inputs by default, and require the user to explicitly opt-in to using other sighashes if they desire to do so.

Fixes #350

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • 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
  • I've updated CHANGELOG.md

@afilini
[signer] Add an option to explicitly allow using non-ALL sighashes
881ca8d 
Instead of blindly using the `sighash_type` set in a psbt input, we
now only sign `SIGHASH_ALL` inputs by default, and require the user to
explicitly opt-in to using other sighashes if they desire to do so.

Fixes bitcoindevkit#350
@LLFourn
Copy link
Collaborator

LLFourn commented May 27, 2021

ACK 881ca8d. I guess what you really want is to make sure that you are not signing any of your inputs with an unusual sighash (regardless of what others do). I don't think we need to account for this though since you are likely going to have to do a lot of manual verification work anyway in more exotic scenarios.

notmandatory
notmandatory approved these changes May 27, 2021
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.

utACK 881ca8d. Test is easy to follow and covers expected cases.

@afilini
Copy link
Member Author

afilini commented May 27, 2021

I guess what you really want is to make sure that you are not signing any of your inputs with an unusual sighash (regardless of what others do). I don't think we need to account for this though since you are likely going to have to do a lot of manual verification work anyway in more exotic scenarios.

Yeah I tried to do that initially but it was a mess. If somebody needs that kind of "precision" they can just always opt-in on the BDK side and implement the check themselves before calling sign()

@afilini afilini merged commit 881ca8d into bitcoindevkit:master May 27, 2021
@afilini afilini deleted the feat/opt-in-non-standard-sighashes branch May 27, 2021 15:05
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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add an explicit TxBuilder method to enable non-ALL sighashes

3 participants

@afilini @LLFourn @notmandatory