bitcoindevkit / bitcoindevkit/bdk-tx
Finalizer doesn't enforce BIP-174 requirements (sighash type, unknown fields)
- Dominant language
- Rust
- Stars
- 13
- Forks
- 17
- Avg merge
- 17h 22m
- Merged PRs (30d)
- 1
Description
The local [`bdk_tx::Finalizer::finalize_input`](https://github.com/bitcoindevkit/bdk-tx/blob/master/src/finalizer.rs) misses two BIP-174 finalizer requirements.
## 1. `PSBT_IN_SIGHASH_TYPE` enforcement (MUST)
BIP-174 states (under `PSBT_IN_SIGHASH_TYPE`):
> Signatures for this input must use the sighash type, finalizers **must** fail to finalize inputs which have signatures that do not match the specified sighash type.
The current implementation calls `plan.satisfy(&PsbtInputSatisfier)` to build the witness from whatever signatures are present in the PSBT input, with no cross-check against `psbt_input.sighash_type`. If a buggy or non-conforming signer produces signatures whose sighash byte doesn't match the declared `PSBT_IN_SIGHASH_TYPE`, finalization still succeeds and the resulting transaction broadcasts.
This is the load-bearing channel for `PSBT_IN_SIGHASH_TYPE` being a useful defense — without finalizer enforcement, the only protection comes from the signer voluntarily honoring the declared value. The spec requires the check; we don't do it.
**Proposed fix:** before `plan.satisfy()`, if `psbt_input.sighash_type` is set, iterate the input's signature fields and compare each signature's effective sighash type against the declared value:
- **ECDSA** (`partial_sigs`): each `bitcoin::ecdsa::Signature` carries an explicit `sighash_type` field — compare directly to the declared `PsbtSighashType` (as `EcdsaSighashType`).
- **Taproot key-path** (`tap_key_sig`): a 64-byte signature implies `SIGHASH_DEFAULT`; a 65-byte signature has the sighash byte as its trailing byte. Compare to the declared `PsbtSighashType` (as `TapSighashType`).
- **Taproot script-path** (`tap_script_sigs`): same encoding rule per signature; same comparison.
Return an error on any mismatch.
## 2. Unknown fields not preserved (SHOULD/MUST)
BIP-174 states (under the Input Finalizer section):
> All other data except the UTXO and **unknown fields** in the input key-value map should be cleared from the PSBT.
The current implementation uses `core::mem::take` to clear the entire `psbt::Input`, then writes back only `non_witness_utxo` and `witness_utxo`. The `Input::unknown` field (and likely `Input::proprietary` by extension) is dropped, contrary to the BIP. Callers who attach metadata expecting it to survive finalization currently lose it.
**Proposed fix:** preserve `input.unknown` (and likely `input.proprietary`) when reconstructing the finalized input.
## Impact
(1) is the more critical of the two — it's the only channel that makes a declared `PSBT_IN_SIGHASH_TYPE` actually binding. Without it, callers relying on declared sighash for defense-in-depth (e.g. against signers that may pick non-`ALL` defaults when given license to do so via an unset field) get no protection from this Finalizer.
(2) is a hygiene/conformance issue but matters for callers using `Input::unknown` or `Input::proprietary` to thread metadata through their PSBT pipeline.
## Related
- Discussed in #74 review (PsbtParams::declare_sighash).
- Upstream rust-miniscript's [`psbt::finalizer`](https://docs.rs/miniscript/latest/miniscript/psbt/index.html) has the same `sighash_type` enforcement gap. A separate upstream issue should be filed against rust-bitcoin/rust-miniscript so callers using that finalizer (instead of bdk-tx's) get the same defense.
Contributor guide
Assessment
This issue has not been assessed yet.