Skip to content

Introduce per-input sighash type via Input::set_sighash_type#69

Closed
evanlinjin wants to merge 1 commit into
bitcoindevkit:masterfrom
evanlinjin:feature/per-input-sighash-type
Closed

Introduce per-input sighash type via Input::set_sighash_type#69
evanlinjin wants to merge 1 commit into
bitcoindevkit:masterfrom
evanlinjin:feature/per-input-sighash-type

Conversation

@evanlinjin
Copy link
Copy Markdown
Member

Description

Closes #60

Replace PsbtParams::sighash_type with a per-input setter/getter on Input. Selection::create_psbt propagates each input's sighash type into the resulting PSBT input.

Changelog notice

Added:
- `Input::set_sighash_type` (setter) and `Input::sighash_type` (getter) methods which specifies per-input sighash type.

Removed:
- `PsbtParams::sighash_type` which sets sighash type for all inputs. Replaced by `Input::{set_}sighash_type` for per-input control.

Before submitting

@evanlinjin evanlinjin self-assigned this May 14, 2026
@evanlinjin evanlinjin force-pushed the feature/per-input-sighash-type branch 3 times, most recently from 556f0e1 to 9ffb42c Compare May 15, 2026 20:25
Copy link
Copy Markdown
Contributor

@noahjoeris noahjoeris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one!
ACK 9ffb42c

Comment thread src/input.rs
Replace `PsbtParams::sighash_type` with a per-input setter/getter on
`Input`. `Selection::create_psbt` propagates each input's sighash type
into the resulting PSBT input.

`Input::set_sighash_type` accepts anything convertible into
`PsbtSighashType`, including the standard `EcdsaSighashType` and
`TapSighashType` from rust-bitcoin.

BREAKING CHANGE: `PsbtParams::sighash_type` is removed. Callers should
call `Input::set_sighash_type` on each input before selection.
@evanlinjin evanlinjin force-pushed the feature/per-input-sighash-type branch from 9ffb42c to 95d4b35 Compare May 16, 2026 12:55
@evanlinjin evanlinjin requested a review from noahjoeris May 16, 2026 12:56
Copy link
Copy Markdown
Contributor

@noahjoeris noahjoeris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 95d4b35

@evanlinjin

This comment was marked as outdated.

@evanlinjin evanlinjin marked this pull request as draft May 17, 2026 06:30
@evanlinjin
Copy link
Copy Markdown
Member Author

evanlinjin commented May 17, 2026

Updated design for this PR.

After working through it, the per-Input sighash API turns out to be the source of the problem rather than a feature. Removing it and letting psbt::Input.sighash_type be the single source of truth for per-input sighash policy.

The problem

For Taproot, signature size depends on sighash: 64 bytes for SIGHASH_DEFAULT, 65 bytes for anything else. Miniscript's Plan bakes this assumption in at construction time (via TaprootCanSign::sighash_default), and Plan::satisfaction_weight() is only accurate if the signer actually uses the assumed sighash.

Input::set_sighash_type introduces sighash state on Input independent of the Plan it was built from, so the two can disagree. The disagreement is silent — weight estimates become wrong, and coin selection drifts off.

Why sighash belongs at the PSBT layer, not on Input

BIP-174 treats sighash as a per-PSBT-input policy:

"The 32-bit unsigned integer specifying the sighash type to be used for this input. 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."

And when unset:

"If a sighash type is not provided, the signer should sign using SIGHASH_ALL, but may use any sighash type they wish."

One input, one sighash policy, enforced at finalization. Input in bdk-tx describes what is being spent and how it can be satisfied. The sighash policy is a property of the constructed PSBT, not of the spend description — so it belongs on the PSBT input itself (psbt::Input.sighash_type), which rust-bitcoin already exposes as a public, mutable field.

Proposed design

1. Revert the per-Input sighash API. Drop Input::set_sighash_type and Input::sighash_type. Input carries no sighash state.

2. In create_psbt, auto-write the PSBT input's sighash_type only where the Plan's weight assumption forces a specific value — for Default-Taproot Plans. Everything else is left unset; the caller mutates psbt.inputs[i].sighash_type directly if they need a specific non-ALL sighash.

The witness-size assumption is read from Plan::witness_template() via a local helper tap_sighash_default(plan) -> Option<bool>: the trailing usize on the first SchnorrSigPk/SchnorrSigPkHash placeholder is 64 (default-Taproot) or 65 (non-default), returning None for ECDSA Plans. Mixed sizes within a single Plan are pathological — debug-assert uniformity.

tap_sighash_default(plan) Behavior in create_psbt
Some(true) (Default-Taproot Plan) Auto-write TapSighashType::Default. This is the one case where the Plan's 64-byte weight assumption locks the sighash to exactly one value; any other choice silently under-funds the transaction.
Some(false) (non-Default-Taproot Plan) Leave the field unset. Signer defaults to SIGHASH_ALL (65 bytes — matches the Plan's weight assumption). Callers needing non-ALL semantics (SINGLE, NONE, *_ANYONECANPAY) set psbt.inputs[i].sighash_type post-construction.
None (ECDSA — sighash-invariant for weight) Leave the field unset.

PSBT-based Inputs (built via Input::from_psbt_input) are unaffected — their embedded psbt::Input already owns its sighash_type field. The auto-write applies only to Plan-based inputs.

3. Doc the one remaining footgun on create_psbt: mutating psbt.inputs[i].sighash_type after construction on a Taproot input is allowed but the caller is responsible for keeping the byte length consistent with the Plan's witness-size assumption (64B for Default, 65B for any other Taproot sighash). Mismatch silently mis-funds the transaction. The auto-write of Default for Default-Taproot Plans guards the catastrophic direction (64B-budget, 65B-signature) by default; overriding it is explicit caller intent.

Net effect

  • Input is purely a spend description, cannot carry inconsistent sighash state.
  • Single source of truth for per-input sighash on the constructed PSBT: psbt.inputs[i].sighash_type. No parallel override API.
  • Default-Taproot Plans get safe auto-lock with zero caller ceremony — prevents the catastrophic 64B→65B underpayment.
  • Non-Default-Taproot Plans match BIP-174's implicit default (SIGHASH_ALL, 65 bytes) and stay weight-accurate; callers needing non-ALL semantics make a one-line post-construction edit.
  • ECDSA Plans are unconstrained, matching reality.

Copy link
Copy Markdown
Contributor

@nymius nymius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this PR complete? I agree with the code here, but I wouldn't relay on a witness guessing scheme for extracting the SIGHASH type in the Plan variant.

@evanlinjin
Copy link
Copy Markdown
Member Author

evanlinjin commented May 18, 2026

Is this PR complete? I agree with the code here, but I wouldn't relay on a witness guessing scheme for extracting the SIGHASH type in the Plan variant.

It is not complete, waiting on upstream changes. Where did you think we were guessing the sighash type?

Sometimes plan is based on Sighash DEFAULT - if the resultant tx uses anything else, the feerate/fee will be different to what CS assumed. The idea is to error on inconsistency.

@nymius
Copy link
Copy Markdown
Contributor

nymius commented May 19, 2026

I was thinking in the rust-miniscript change. In the light of your last response in that PR, I guess miniscript doesn't expose the sighash values because is presuming the descriptor holder knows which sighashes it's going to use. Can't we avoid bubbling up the CanSign bool by getting the value earlier?

@evanlinjin
Copy link
Copy Markdown
Member Author

I was thinking in the rust-miniscript change. In the light of your last response in that PR, I guess miniscript doesn't expose the sighash values because is presuming the descriptor holder knows which sighashes it's going to use. Can't we avoid bubbling up the CanSign bool by getting the value earlier?

I realized that we can actually use Plan::witness_template method to check sighash constraints directly. So no need for upstream changes to miniscript.

@evanlinjin
Copy link
Copy Markdown
Member Author

Closing. Rationale: #69 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PsbtParams::sighash_type does not make sense

3 participants