bitcoindevkit / bitcoindevkit/bdk_wallet

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

Open
#519 0 comments 1 reaction 0 assignees View on GitHub
bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.