Skip to content

fix(wallet): make sign()'s sighash guard context-aware - #528

Open
muhahahmad68 wants to merge 1 commit into
bitcoindevkit:masterfrom
muhahahmad68:fix/sign-context-aware-sighash-guard
Open

fix(wallet): make sign()'s sighash guard context-aware#528
muhahahmad68 wants to merge 1 commit into
bitcoindevkit:masterfrom
muhahahmad68:fix/sign-context-aware-sighash-guard

Conversation

@muhahahmad68

@muhahahmad68 muhahahmad68 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Fixes #519.

Description

The pre-flight sighash guard in Wallet::sign() checked every PSBT input against a flat set of allowed sighash values without regard to whether that input was actually Taproot. As a result, TapSighashType::Default (0x00) was accepted unconditionally for every input, including Legacy/Segwitv0 ones, where it has no valid ECDSA meaning.

This is currently masked for the built-in software signer, since sign_input()'s guard (introduced in #476) independently rejects it downstream. However, Wallet::sign() dispatches to any TransactionSigner, and the whole point of this pre-flight guard is that individual signers can trust it has already filtered non-standard sighashes.

Notes to the reviewers

Infer Taproot-ness per input from witness_utxo.script_pubkey, consistent with how complete_transaction() determines Taproot-ness for foreign UTXOs, and apply the correct sighash check for that input's context.

Tests added:

  • sign_rejects_ecdsa_input_with_taproot_default_sighash — confirms a non-Taproot input carrying TapSighashType::Default is now rejected
  • sign_allows_taproot_input_with_default_sighash — confirms a genuine Taproot spend still signs and finalizes normally

Before submitting

@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.96%. Comparing base (6fc6846) to head (c92965e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #528      +/-   ##
==========================================
+ Coverage   81.91%   81.96%   +0.04%     
==========================================
  Files          25       25              
  Lines        6535     6542       +7     
  Branches      302      300       -2     
==========================================
+ Hits         5353     5362       +9     
+ Misses       1075     1074       -1     
+ Partials      107      106       -1     
Flag Coverage Δ
rust 81.96% <100.00%> (+0.04%) ⬆️

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

☔ View full report in Codecov by Harness.
📢 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.

@muhahahmad68
muhahahmad68 force-pushed the fix/sign-context-aware-sighash-guard branch from c4ab82f to 950420a Compare August 10, 2026 08:02
@ValuedMammal

Copy link
Copy Markdown
Contributor

One suggestion I have for determining whether an input is a p2tr spend is it may be safer and more reliabe to test witness_utxo.script_pubkey.is_p2tr() rather than the tap_* metadata fields (at least that's consistent with how the check is handled elsewhere e.g. complete_transaction).

@muhahahmad68

Copy link
Copy Markdown
Contributor Author

One suggestion I have for determining whether an input is a p2tr spend is it may be safer and more reliabe to test witness_utxo.script_pubkey.is_p2tr() rather than the tap_* metadata fields (at least that's consistent with how the check is handled elsewhere e.g. complete_transaction).

Good catch

@muhahahmad68
muhahahmad68 force-pushed the fix/sign-context-aware-sighash-guard branch from 4ddc409 to 3956e04 Compare August 11, 2026 07:13
@muhahahmad68
muhahahmad68 force-pushed the fix/sign-context-aware-sighash-guard branch from 3956e04 to 61d49ee Compare September 1, 2026 11:44
The pre-flight sighash guard in Wallet::sign() checked every PSBT
input against a flat set of allowed sighash values without regard
to whether that input was actually Taproot. As a result,
TapSighashType::Default (0x00) was accepted unconditionally for
every input, including Legacy/Segwitv0 ones, where it has no valid
ECDSA meaning.

This is currently masked for the built-in software signer, since
sign_input()'s guard (introduced in bitcoindevkit#476) independently rejects it
downstream. However, Wallet::sign() dispatches to any
TransactionSigner, and the whole point of this pre-flight guard is
that individual signers can trust it has already filtered
non-standard sighashes.

Infer Taproot-ness per input from witness_utxo.script_pubkey,
consistent with how complete_transaction() determines Taproot-ness
for foreign UTXOs, and apply the correct sighash check for that
input's context.

Fixes bitcoindevkit#519.
@muhahahmad68
muhahahmad68 force-pushed the fix/sign-context-aware-sighash-guard branch from 61d49ee to c92965e Compare September 1, 2026 12:06
@ValuedMammal

Copy link
Copy Markdown
Contributor

I opened a draft PR to this branch muhahahmad68#1, still open for discussion.
The motivation is to handle the shared logic when it comes to prevout resolution
and input validation. For instance in the allow_all_sighashes guard, we were
determining P2TR-ness by looking at the witness_utxo alone with no additional
validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Wallet::sign()'s sighash pre-flight guard doesn't reject non-standard sighashes on non-Taproot inputs

2 participants