Skip to content

fix(signer): respect allow_all_sighashes in SignerWrapper::sign_input#476

Open
muhahahmad68 wants to merge 1 commit intobitcoindevkit:masterfrom
muhahahmad68:fix/fix/sign-input-allow-all-sighashes
Open

fix(signer): respect allow_all_sighashes in SignerWrapper::sign_input#476
muhahahmad68 wants to merge 1 commit intobitcoindevkit:masterfrom
muhahahmad68:fix/fix/sign-input-allow-all-sighashes

Conversation

@muhahahmad68
Copy link
Copy Markdown

@muhahahmad68 muhahahmad68 commented May 3, 2026

Description

SignOptions::allow_all_sighashes is documented as controlling whether the signer will accept non-SIGHASH_ALL sighash types. However, the check only existed inside Wallet::sign as a PSBT-wide pre-flight guard. The underlying SignerWrapper<PrivateKey>::sign_input never consulted it, meaning callers using InputSigner or TransactionSigner directly could bypass the check entirely.

Added a sighash guard in SignerWrapper<PrivateKey>::sign_input before the match self.ctx dispatch. This ensures the check covers all signing contexts (Legacy, Segwitv0, Taproot) and all delegating implementations in a single place.

Tests Added

  • Taproot input with SIGHASH_NONE and allow_all_sighashes: false returns NonStandardSighash
  • Same setup with allow_all_sighashes: true passes the guard successfully

Fixes #469

Checklists

All Submissions:

New Features:

  • N/A

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@codecov
Copy link
Copy Markdown

codecov Bot commented May 4, 2026

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.07%. Comparing base (a9ad3b9) to head (8c6d411).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/wallet/signer.rs 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #476      +/-   ##
==========================================
+ Coverage   80.05%   80.07%   +0.01%     
==========================================
  Files          24       24              
  Lines        5360     5369       +9     
  Branches      244      249       +5     
==========================================
+ Hits         4291     4299       +8     
  Misses        990      990              
- Partials       79       80       +1     
Flag Coverage Δ
rust 80.07% <88.88%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ValuedMammal ValuedMammal moved this to Needs Review in BDK Wallet May 4, 2026
@ValuedMammal ValuedMammal added bug Something isn't working audit Suggested as result of external code audit labels May 4, 2026
@ValuedMammal ValuedMammal added this to the Wallet 3.1.0 milestone May 4, 2026
@muhahahmad68 muhahahmad68 force-pushed the fix/fix/sign-input-allow-all-sighashes branch from a187c70 to 3f07adc Compare May 4, 2026 21:22
Comment thread src/wallet/signer.rs Outdated
);

psbt.inputs[0].sighash_type = Some(TapSighashType::All.into());
assert_ne!(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

instead of these assert_ne!s it'd be more readable if you're asserting if it is Ok(())

here and below

Comment thread src/wallet/signer.rs Outdated

psbt.inputs[0].sighash_type = Some(TapSighashType::Default.into());
assert_ne!(
signer.sign_input(&mut psbt, 0, &opts_reject, &secp),
Copy link
Copy Markdown
Contributor

@benthecarman benthecarman May 5, 2026

Choose a reason for hiding this comment

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

after you've had one successful sig, it'll always succeed because it is already signed. you also need to clear out the signatures before each. Imo would be cleanest to start with base_psbt and for each assertion clone it and make modifications as you need, that way you don't have any risk of secondary effects

@muhahahmad68
Copy link
Copy Markdown
Author

Makes more sense, I'll effect the changes as suggested. Much thanks @benthecarman

@muhahahmad68 muhahahmad68 force-pushed the fix/fix/sign-input-allow-all-sighashes branch from 3f07adc to 8c6d411 Compare May 5, 2026 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

audit Suggested as result of external code audit bug Something isn't working

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

sign_input should respect allow_all_sighashes sign option

3 participants