bitcoindevkit / bitcoindevkit/bdk_wallet
`Wallet::sign()`'s sighash pre-flight guard doesn't reject non-standard sighashes on non-Taproot inputs
- Dominant language
- Rust
- Stars
- 59
- Forks
- 105
- Avg merge
- 10d 9h
- Merged PRs (30d)
- 1
Description
While working on #476 (context-aware `allow_all_sighashes` check in `SignerWrapper::sign_input`), it came up that `Wallet::sign()` has a pre-flight guard with the same underlying issue: a dead comparison and no awareness of which inputs are actually Taproot.
The guard in question (src/wallet/mod.rs):
```
if !sign_options.allow_all_sighashes
&& !psbt.inputs.iter().all(|i| {
i.sighash_type.is_none()
|| i.sighash_type == Some(EcdsaSighashType::All.into())
|| i.sighash_type == Some(TapSighashType::All.into())
|| i.sighash_type == Some(TapSighashType::Default.into())
})
{
return Err(SignerError::NonStandardSighash);
}
```
### Problems:
`EcdsaSighashType::All.into()` and `TapSighashType::All.into()` resolve to the same `PsbtSighashType` value, so one of these branches is redundant, same dead comparison as before #476.
More importantly, this check runs as a flat loop over every PSBT input with no idea which input is Taproot and which isn't. `TapSighashType::Default` (byte 0x00) is accepted unconditionally for all inputs, including Legacy/Segwitv0 ones, where `Default` isn't a meaningful ECDSA sighash at all.
### Impact:
I wrote a test that builds a normal Segwitv0, manually sets input 0's sighash_type to `TapSighashType::Default`'s byte value, then calls wallet.sign(...) with `allow_all_sighashes: false`. The guard lets it through as expected from the code above.
The `sign_input()` fix from #476 catches it downstream and returns NonStandardSighash anyway. But Wallet::sign() dispatches to any TransactionSigner, and the whole point of this is that individual signers can reasonably trust it's already filtered non-standard sighashes and skip re-validating themselves. For a custom signer that does trust the guard, this is a real bypass.
### Suggested fix:
Since psbt::Input carries tap_internal_key / tap_merkle_root, Taproot can be inferred per-input without needing a SignerContext lookup
Happy to open a PR for this, flagging as an issue first per usual since I want to confirm the framing is right before writing the fix.
Contributor guide
Research direction
Start in src/wallet/mod.rs at Wallet::sign()'s pre-flight sighash guard, then compare its behavior with SignerWrapper::sign_input from #476. Reproduce the reported normal Segwitv0 case using a TapSighashType::Default byte value and allow_all_sighashes: false. Done means the guard distinguishes Taproot inputs and rejects that non-standard sighash on non-Taproot inputs.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- cryptography
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100