Skip to content

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

Open
evanlinjin wants to merge 3 commits into
bitcoindevkit:masterfrom
evanlinjin:fix/input-signer-preserve-unknown-fields
Open

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

Conversation

@evanlinjin

@evanlinjin evanlinjin commented Jun 13, 2026

Copy link
Copy Markdown
Member

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 FinalizeErrorfinalize_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

evanlinjin and others added 3 commits June 2, 2026 08:16
**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>
@evanlinjin evanlinjin self-assigned this Jun 13, 2026
@evanlinjin evanlinjin added bug Something isn't working api Changes the public API labels Jun 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Changes the public API bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant