finalizer: BIP174 compliance + sighash/signature-size validation#79
Open
evanlinjin wants to merge 3 commits into
Open
finalizer: BIP174 compliance + sighash/signature-size validation#79evanlinjin wants to merge 3 commits into
evanlinjin wants to merge 3 commits into
Conversation
**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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Hardens
Finalizerto 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 dedicatedFinalizeErrorinstead of leakingminiscript::Error.More context here: #74 (comment)
What changed:
unknownandproprietary.SighashMismatch— a signature's sighash type disagrees with the input's declaredPSBT_IN_SIGHASH_TYPE. Mandated by BIP174.SighashNotAllowed— noPSBT_IN_SIGHASH_TYPEis declared, yet a signature uses neitherDEFAULTnorALL. Stricter-than-spec safeguard against silently changing signing semantics.SignatureTooLarge— a satisfied schnorr witness is larger than the size the spendingPlancommitted to (e.g. a 65-byteSIGHASH_ALLsig where 64-byteSIGHASH_DEFAULTwas 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.FinalizeError—finalize_input/FinalizeMapnow returnFinalizeError, modelling the failures above plusSatisfaction(miniscript::Error).Notes to the reviewers
SignatureTooLargeis intentionally not unit-tested: it is only reachable in release builds. In debug, miniscript'ssatisfy_selfhits adebug_assert!on the signature size and panics before our check runs - an upstream bug.Out of scope, for follow-up:
TODOin code). An upstream change could give ECDSA placeholders a committed size to capture low-R grinding fee savings safely.debug_assert→ recoverable-error fix.Changelog notice
Before submitting