finalizer: BIP174 compliance + sighash/signature-size validation by evanlinjin · Pull Request #79 · bitcoindevkit/bdk-tx

evanlinjin · 2026-06-13T20:05:37Z

Description

Hardens Finalizer to follow BIP174 more closely and refuse to emit a transaction that would be malformed or unsafe to broadcast. The finalizer now validates signatures before assembling the final witness/scriptSig, and reports failures through a dedicated FinalizeError instead of leaking miniscript::Error.

More context here: #74 (comment)

What changed:

Retain unknown/proprietary input fields. Per BIP174 a finalizer must clear all input data except the UTXO and unknown/proprietary fields; finalization now preserves unknown and proprietary.
SighashMismatch — a signature's sighash type disagrees with the input's declared PSBT_IN_SIGHASH_TYPE. Mandated by BIP174.
SighashNotAllowed — no PSBT_IN_SIGHASH_TYPE is declared, yet a signature uses neither DEFAULT nor ALL. Stricter-than-spec safeguard against silently changing signing semantics.
SignatureTooLarge — a satisfied schnorr witness is larger than the size the spending Plan committed to (e.g. a 65-byte SIGHASH_ALL sig where 64-byte SIGHASH_DEFAULT was planned). Because the plan's weight/feerate estimate derives from the committed size, a heavier witness makes the tx undershoot its target feerate and risk being unbroadcastable. Only the larger direction is rejected; a smaller witness merely overpays and is allowed.
Dedicated FinalizeError — finalize_input / FinalizeMap now return FinalizeError, modelling the failures above plus Satisfaction(miniscript::Error).

Notes to the reviewers

SignatureTooLarge is intentionally not unit-tested: it is only reachable in release builds. In debug, miniscript's satisfy_self hits a debug_assert! on the signature size and panics before our check runs - an upstream bug.

Out of scope, for follow-up:

ECDSA signatures are estimated at their 73-byte max, so they can't exceed the plan and need no check today (TODO in code). An upstream change could give ECDSA placeholders a committed size to capture low-R grinding fee savings safely.
The upstream rust-miniscript debug_assert → recoverable-error fix.

Changelog notice

Changed:
- **(breaking)** `Finalizer::finalize_input`, `FinalizeMap`, and `FinalizeMap::results` now return the new `FinalizeError` type instead of `miniscript::Error`.

Added:
- `FinalizeError` enum with `SighashMismatch`, `SighashNotAllowed`, `SignatureTooLarge`, and `Satisfaction` variants.
- Finalization now validates that each signature's sighash type respects `PSBT_IN_SIGHASH_TYPE` (or is `DEFAULT`/`ALL` when unset) and that satisfied witness sizes do not exceed the spending plan.

Fixed:
- Finalization now retains `unknown` and `PSBT_IN_PROPRIETARY` input fields, as required by BIP174.

Before submitting

I followed the contribution guidelines
This PR breaks the existing API

**BIP174:** All other data except the UTXO and unknown fields (including PSBT_IN_PROPRIETARY fields the Input Finalizer does not understand) in the input key-value map should be cleared from the PSBT.

Finalization can now fail for reasons miniscript does not model, so `finalize_input` and `FinalizeMap` return a dedicated `FinalizeError` instead of `miniscript::Error`. The finalizer now rejects an input when: - a signature's sighash type disagrees with the declared `PSBT_IN_SIGHASH_TYPE` (mandated by BIP174); - no type is declared yet a signature is neither DEFAULT nor ALL; - a satisfied schnorr witness is larger than the plan committed to (e.g. a 65-byte SIGHASH_ALL sig where 64-byte DEFAULT was planned), which would make the transaction undershoot its target feerate and risk being unbroadcastable. BREAKING CHANGE: `Finalizer::finalize_input`, `FinalizeMap`, and `FinalizeMap::results` now use `FinalizeError` in place of `miniscript::Error`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Add tests for the two sighash checks `finalize_input` now performs: `SighashMismatch` (declared PSBT_IN_SIGHASH_TYPE disagrees with the signature) and `SighashNotAllowed` (no type declared, signature is neither DEFAULT nor ALL). `SignatureTooLarge` is left uncovered: it is only reachable in release builds, since in debug miniscript's `satisfy_self` panics on a `debug_assert!` of the signature size before the check is reached. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

nymius

cACK 0291c17

nymius · 2026-06-17T19:08:07Z

+        let mut psbt_in_sighashes = {
+            let partial_sigs = psbt_in.partial_sigs.values().map(|s| s.sighash_type as u32);
+            let tap_key_sig = psbt_in.tap_key_sig.iter().map(|s| s.sighash_type as u32);
+            let tap_script_sigs = psbt_in
+                .tap_script_sigs
+                .values()
+                .map(|s| s.sighash_type as u32);
+            partial_sigs.chain(tap_key_sig).chain(tap_script_sigs)
+        };


Why would an input have partial_sigs and tap_script_sigs? Shouldn't that be an error? I think we should check the sighashes in an "ad hoc" fasihon, for each kind of signature, and avoid reducing them to the same thing.

I don't think the finalizer should reject every invalid state a PSBT can represent. It's job is to produce a correct final witness.

I think we should check the sighashes in an "ad hoc" fasihon, for each kind of signature, and avoid reducing them to the same thing.

A per-kind check would literally do the exact same thing though.

nymius · 2026-06-17T19:08:53Z

            }
-            if !stack.is_empty() {
-                psbt_input.final_script_witness = Some(Witness::from_slice(&stack));
+        } else if let Some(sighash_mismatch) = psbt_in_sighashes.find(|&t| t > 0x01 /*ALL*/) {


There is a PsbtSighashFlag::ALL for 0x01.

That type doesn't exist in the version of bitcoin/miniscript we are using here.

nymius · 2026-06-17T19:19:02Z

+            if let Placeholder::SchnorrSigPk(_, _, size)
+            | Placeholder::SchnorrSigPkHash(_, _, size) = temp
+            {
+                // Only a witness *larger* than the plan is dangerous.


How should a recovery mechanism look like in the case we want to save the extra sats? Should we return an enum here with "Finalized, NotFinalized, Overshoot" or something similar? Should we compare the target fee rate and inner fee rate at the end of the tx creation process and rewind if they don't match?

Interesting question. I think we should reserve this PR purely for safety. Everything else can be tackled in separate PRs. Could you create a ticket? Thanks!

evanlinjin and others added 3 commits June 2, 2026 08:16

fix(finalizer): Retain unknown input fields when finalizing

2d3b848

**BIP174:** All other data except the UTXO and unknown fields (including PSBT_IN_PROPRIETARY fields the Input Finalizer does not understand) in the input key-value map should be cleared from the PSBT.

evanlinjin requested review from ValuedMammal, notmandatory and nymius June 13, 2026 20:06

evanlinjin self-assigned this Jun 13, 2026

evanlinjin added bug Something isn't working api Changes the public API labels Jun 13, 2026

nymius reviewed Jun 17, 2026

View reviewed changes

fix(finalizer): clarify SighashNotAllowed error message

7129486

evanlinjin force-pushed the fix/input-signer-preserve-unknown-fields branch from d7f6d63 to 7129486 Compare June 24, 2026 05:03

evanlinjin requested a review from nymius June 24, 2026 05:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

finalizer: BIP174 compliance + sighash/signature-size validation#79

finalizer: BIP174 compliance + sighash/signature-size validation#79
evanlinjin wants to merge 4 commits into
bitcoindevkit:masterfrom
evanlinjin:fix/input-signer-preserve-unknown-fields

evanlinjin commented Jun 13, 2026 •

edited

Loading

Uh oh!

nymius left a comment

Uh oh!

nymius Jun 17, 2026

Uh oh!

evanlinjin Jun 24, 2026

Uh oh!

nymius Jun 17, 2026

Uh oh!

evanlinjin Jun 24, 2026

Uh oh!

nymius Jun 17, 2026

Uh oh!

evanlinjin Jun 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

evanlinjin commented Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Changelog notice

Before submitting

Uh oh!

nymius left a comment

Choose a reason for hiding this comment

Uh oh!

nymius Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

evanlinjin Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

nymius Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

evanlinjin Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

nymius Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

evanlinjin Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

evanlinjin commented Jun 13, 2026 •

edited

Loading