feat(selection)!: declare PSBT_IN_SIGHASH_TYPE on Plan-derived inputs

[evanlinjin]

Closes #60.
Supersedes #69.

Description

Two issues with how PsbtParams::sighash_type worked.

1. The field itself wasn't very useful. It was a tx-wide uniform override. However, the realistic use cases for non-ALL sighashes are inherently per-input.

2. It didn't guard against the catastrophic Taproot case. Taproot signature size depends on sighash (SIGHASH_DEFAULT is 64 bytes, other Taproot sighashes are 65 bytes). A miniscript Plan has the sighash type pre-baked and Plan::satisfaction_weight() is only accurate if the signer uses the assumed sighash. Leaving sighash_type = None lets a spec-compliant signer pick anything (BIP-174: "should sign using SIGHASH_ALL, but may use any sighash type they wish"), which can underfund the tx or weaken its commitment scheme (e.g. a signer choosing NONE would let a third party replace outputs).

This PR removes PsbtParams::sighash_type and instead writes PSBT_IN_SIGHASH_TYPE on every Plan-derived input with a safe default:

Plan's Schnorr placeholders	sighash_type
Any 64B (incl. mixed-size)	TapSighashType::Default
All 65B	TapSighashType::All
None (ECDSA)	EcdsaSighashType::All

PSBT-derived inputs (Input::from_psbt_input) are never touched — they come in with their own sighash_type already set.

Callers needing a non-ALL sighash (SINGLE, NONE, *_ANYONECANPAY) mutate psbt.inputs[i].sighash_type directly on the returned PSBT. Plans needing non-ALL semantics on every key should be built with uniform TaprootCanSign::sighash_default = false so the safety lock doesn't fire.

Notes to the reviewers

This PR differs from #60's proposal in two places.

The original issue suggested auto-writing DEFAULT only on uniformly-64B Plans (a plan can require multiple signatures with different sighash types). In this PR, a Plan with any 64B placeholder triggers sighash_type = DEFAULT.

• Rationale: Mixed sighash types in one Plan are uncommon in practice. We want to avoid under-funding at all costs whereas over-funding is fine.

The original issue proposed writing None as default for "weight-safe" inputs. This PR writes ALL for them.

• Rationale: BIP-174 lets signers pick any sighash when the field is unset ("may use any sighash type they wish"). Declaring ALL closes that loophole for spec-compliant signers — ALL/DEFAULT is what the super-majority of callers would want, and any deviation should be an explicit caller action (post-construction mutation), not a silent signer choice.

Alternative (rejected) solutions

• Placing a sighash_type field in Plan-based bdk_tx::Input — this is a PSBT-layer concern. An input's witness can also have multiple signatures.
• Adding a declare_sighash: bool flag to PsbtParams — earlier iterations of this PR explored this, but the simpler "always declare, mutate post-construction if you need otherwise" design landed instead (thanks @nymius for the suggestion). One less knob, same expressiveness.

Changelog notice

Changed:
- `create_psbt` writes `PSBT_IN_SIGHASH_TYPE` on every Plan-derived input. Plans whose witness template contains at least one 64B Schnorr signature get `sighash_type = DEFAULT`; all other Plan-derived inputs get `sighash_type = ALL` (`TapSighashType::All` for Taproot, `EcdsaSighashType::All` for ECDSA).

Removed:
- `PsbtParams::sighash_type` — the tx-wide uniform override was not useful. Callers needing a non-`ALL` sighash should mutate `psbt.inputs[i].sighash_type` on the returned PSBT.

Before submitting

• I followed the contribution guidelines
• This PR breaks the existing API

[ValuedMammal]

Thanks for the detailed description. I was in favor of the original proposal in #60 - remove the sighash_type field and auto-write DEFAULT if the plan's weight assumption depends on it. The behavior should be clearly documented. I think sighash type shouldn't be part of PsbtParams at all but handled by the signer/coordinator.

[evanlinjin]

Thanks for the detailed description. I was in favor of the original proposal in #60 - remove the sighash_type field and auto-write DEFAULT if the plan's weight assumption depends on it. The behavior should be clearly documented. I think sighash type shouldn't be part of PsbtParams at all but handled by the signer/coordinator.

create_psbt plays the Creator + Updater roles, so PSBT_IN_SIGHASH_TYPE is its decision to make. The Signer role must honor what's declared, and Finalizers must reject signatures that don't match (they don't set such fields).

What's the motivation for PsbtParams::declare_sighash?

• declare_sighash == true is what most callers want. They want their signatures to cover the whole transaction and we use PSBT_IN_SIGHASH_TYPE to enforce it.
• declare_sighash == false - the caller saying "I want something custom, leave it to me". We still write DEFAULT for 64B Schnorr Plans because any other sighash will underfund the tx.

[notmandatory]

Concept ACK

I like the approach to defaulting to "safe" sighash ALL and leaving it up to the user to manually op-out of the default and set their non-typical sighash types.

The coding is well documented and easy enough to follow. But can you add a test for the mixed sighash type concern described in problem 2 ? I don't doubt it will pass but would be good to have a unit test to make sure it doesn't break in the future.

[evanlinjin]

@notmandatory I added the mixed sighash test!

[ValuedMammal]

NACK

[evanlinjin]

@ValuedMammal Let me present my reasoning clearly so it's easier for you to address. Here are my claims:

1. PSBT_IN_SIGHASH_TYPE is meant to be set by the Updater (as per BIP-174). Signers consume it, they don't decide it. create_psbt plays both the Creator and Updater roles.

2. Leaving PSBT_IN_SIGHASH_TYPE unset means the signer is free to use anything. BIP-174: "should sign using SIGHASH_ALL, but may use any sighash type they wish". A signer picking anything other than ALL/DEFAULT means inputs/outputs may be added/replaced.

3. Declaring sighash binds compliant signers. BIP-174: "Signatures for this input must use the sighash type".

4. The super-majority of callers want ALL/DEFAULT sighash types only. Therefore, we should have this as our default.

5. 64B Schnorr Plans must be locked to DEFAULT otherwise we risk underfunding the transaction which is dangerous.

6. declare_sighash: false preserves the "leave unset" behavior. Callers wishing for this behavior still have it.

Could you indicate which one you reject? Need to know what we are arguing about.

[notmandatory]

@evanlinjin did you check how the core wallet handles this?

[evanlinjin]

@evanlinjin did you check how the core wallet handles this?

Thanks for asking. I just checked and I noticed a few things.

1. PSBTs created by the Bitcoin Core Wallet never have PSBT_IN_SIGHASH_TYPE set.

2. The Bitcoin Core Signer signs with DEFAULT/ALL when PSBT_IN_SIGHASH_TYPE is not provided.

3. The Bitcoin Core Finalizer does not rely on PSBT_IN_SIGHASH_TYPE being set to enforce sighash correctness. At finalize time, the expected sighash is taken from the PSBT field if present, and otherwise falls back to ALL/DEFAULT — the finalizer then rejects any signature whose actual sighash byte doesn't match that expectation. 1 (Note: the separate "expected sighash supplied as an explicit option, independent of the PSBT field" behavior is the signing path via walletprocesspsbt's -sighashtype arg, not the finalize path.)

In other words, sighash correctness is enforced by logic that is not mandated by BIP-174: BIP-174 only requires a finalizer to reject signatures that don't match a specified sighash. Core goes further — it defaults the expectation to ALL/DEFAULT when the field is absent, and it also assumes its signer produces ALL/DEFAULT by default. A spec-compliant foreign finalizer is permitted to do neither.

My Takeaway

After doing this research, I've reconsidered my stance on how we should enforce sighash correctness.

1. We should harden our Finalizer so its expected sighash is (PSBT field if present, else ALL/DEFAULT), and it rejects signatures that deviate. For the Taproot weight assumption specifically, the finalizer should derive the expected type from bdk_tx::Input/the Plan (e.g. expect DEFAULT/64B for a 64B-Schnorr plan) and reject a 65B sig. An alternative expectation can be supplied explicitly to the finalizer, not via the PSBT.
2. We can assume Signers appropriately determine the correct sighash type, so we don't need to set PSBT_IN_SIGHASH_TYPE in create_psbt for the common case.
3. Like Bitcoin Core, we expect the caller to use our Finalizer to finalize the transaction.

One caveat

Takeaway 3 is load-bearing. Core can leave the field unset because it is its own strict finalizer. When create_psbt is only the Creator in a multiparty/interchange flow and the PSBT is finalized by a foreign finalizer, the field was the only cross-boundary signal — and a spec-compliant foreign finalizer may accept a non-ALL signature on a field-unset input.

To cover that case without re-introducing the field by default, I propose we keep declare_sighash as an opt-in (off by default) for callers who know their PSBT will be finalized elsewhere, while the default path relies on our hardened finalizer like Core. In which case, declare_sighash = false will leave all PSBT_IN_SIGHASH_TYPE fields as None and declare_sighash = true will set all PSBT_IN_SIGHASH_TYPE fields as DEFAULT/ALL. WDYT?

@notmandatory @ValuedMammal would you be happy with this approach?

[nymius]

Is the final concern for this field related to fee estimation?
IMHO, the exceptional case is the taproot default, if it wasn't because of it, this shouldn't be an issue, as all sighash flags would occupy the same single byte.
If the Finalizer spec already handles mismatches between the expected sighash and the given sighash, and Signer only uses the specified sighash if set and acceptable, should we handle this logic here at all?
Looking at the planning module, the reason why satisfaction weight doesn't overshoot the estimation is because it assumes the signer will use the cheapest signature in case of mixed assets. Also, it only cares about the signature itself.
Why we aren't aiming to fix this under-estimation at the selection level rather than setting some parameters that don't have much influence in the sat amount once selection is done?
Can we have a e2e test like synopsis.rs where the target feerate is undershoot?

[evanlinjin]

Thanks @nymius, a few of the questions are already covered in the thread but I'll address them all for further clarity.

Is the final concern for this field related to fee estimation?

Not only. The root concern is that the spec allows a Signer to create a signature with a sighash of any type if PSBT_IN_SIGHASH_TYPE is unset.

This is dangerous in the following ways:

1. If coin selection assumed sighash will be DEFAULT and feerate target is 0.1sats/B, but then the signer signs with ALL, the resultant tx will be unbroadcastable.
2. If PSBT_IN_SIGHASH_TYPE is unset, it is spec-legal for the signer to sign with any sighash. This may create a transaction where inputs and outputs are mutable.
3. The Finalizer spec only checks against PSBT_IN_SIGHASH_TYPE so if that field is unset, Finalizer may say "okay" to any signature of any sighash type.

If the Finalizer spec already handles mismatches between the expected sighash and the given sighash, and Signer only uses the specified sighash if set and acceptable, should we handle this logic here at all?

If purely following the spec, the Finalizer will only handle mismatches if PSBT_IN_SIGHASH_TYPE is set.

Bitcoin Core's finalizer does not follow the spec. As mentioned in my comment here, Bitcoin Core's Finalizer goes beyond the spec to ensure the resultant tx is what the user expected.

Why we aren't aiming to fix this under-estimation at the selection level rather than setting some parameters that don't have much influence in the sat amount once selection is done?

Are you suggesting that we budget pessimistically? Over-funding the majority of taproot transactions to defend against a potential signer deviation seems like the wrong trade. We can solve the same problem with enforcement (either via the PSBT field or our hardened finalizer). This gets the same correctness without sacrificing on fees.

Can we have a e2e test like synopsis.rs where the target feerate is undershoot?

Good idea. Will add as a follow up once there is some sort of rough consensus on the right solution.

My proposed solution is mentioned in this comment here. Let me know what you think.

[notmandatory]

@evanlinjin thanks for the Core research and detailed analysis of what it is doing. I'm in favor of your update to follow Core's approach by hardening our finalizer and making the declare_sighash flag optional and default false.

Also if I understand this right we'll need to carefully explain in future documentation/tutorials that if the user knows they'll be using a non-default taproot sighash then they MUST set the declare_sighash flag to true to avoid potentially underpaying fees. There's no other way to communicate the extra byte is needed to coin selection right?

[nymius]

1. If coin selection assumed sighash will be DEFAULT and feerate target is 0.1sats/B, but then the signer signs with ALL, the resultant tx will be unbroadcastable.

I was considering this the fee estimation case.

2. If `PSBT_IN_SIGHASH_TYPE` is unset, it is spec-legal for the signer to sign with any sighash. This may create a transaction where inputs and outputs are mutable.

For this I would move the responsability to the psbt package and implement a sane signer like the one in core.

3. The Finalizer spec only checks against `PSBT_IN_SIGHASH_TYPE` so if that field is unset, Finalizer may say "okay" to any signature of any sighash type.

Ok, I think this is reason enough to always set the field when we are about to introduce ambiguity or possible errors, like this case.

Bitcoin Core's finalizer does not follow the spec. As mentioned in my comment here, Bitcoin Core's Finalizer goes beyond the spec to ensure the resultant tx is what the user expected.

Why we aren't aiming to fix this under-estimation at the selection level rather than setting some parameters that don't have much influence in the sat amount once selection is done?

Are you suggesting that we budget pessimistically? Over-funding the majority of taproot transactions to defend against a potential signer deviation seems like the wrong trade. We can solve the same problem with enforcement (either via the PSBT field or our hardened finalizer). This gets the same correctness without sacrificing on fees.

No, that's simple but brittle. I was wondering if there were other alternatives that allowed coin selection to get some introspection in the sighash. It seems not.

My proposed solution is mentioned in this comment here. Let me know what you think.

I would prefer to leave the Finalizer details for the package we are using now, or will be used in the future (rust-psbt). Same with the signer.
What I would do to control de issue is set a default PSBT_IN_SIGHASH_FLAG for every input, with SIGHASH_ALL for any non taproot input. Only skip this if the SIGHASH_FLAG is already set.

For taproot inputs I would use the logic in this PR to extract the safest flag. If the user pretends to change the flag, he can do it later. I wouldn't prevent the user with more than a warning if he reintroduces the chances of the misestimation of fees after setting the flag.

[evanlinjin]

@evanlinjin thanks for the Core research and detailed analysis of what it is doing. I'm in favor of your update to follow Core's approach by hardening our finalizer and making the declare_sighash flag optional and default false.

Also if I understand this right we'll need to carefully explain in future documentation/tutorials that if the user knows they'll be using a non-default taproot sighash then they MUST set the declare_sighash flag to true to avoid potentially underpaying fees. There's no other way to communicate the extra byte is needed to coin selection right?

Yes. So purely based on the BIP-174 spec, there is no other way to communicate the correct sighash to the signer other than PSBT_IN_SIGHASH_TYPE. That being said, we can also argue that only a badly implemented signer would do anything other than DEFAULT for Schnorr and ALL for everything else when PSBT_IN_SIGHASH_TYPE is not specified (this is the Bitcoin Core approach).

Either case, we should expect our Finalizer to ensure DEFAULT for Schnorr and ALL for everything else unless if stated otherwise by PSBT_IN_SIGHASH_TYPE.

My new idea of the declare_sighash boolean as stated here is whether we want to explicitly communicate the common-sense sighash type to the signer (via PSBT_IN_SIGHASH_TYPE). This is for potentially poorly implemented signers or multi-party protocols where an external finalizer (that doesn't do Bitcoin-core level checks) will be used. Alternatively, I will also be happy with @nymius' suggestion of getting rid of the declare_sighash completely and always set DEFAULT/ALL for PSBT_IN_SIGHASH_TYPE.

[nymius]

I prefer to not introduce declare_sighash, and just provide the defaults. In the case the intention of the signer is a different one, it will have to opt in for each of the inputs he wants to modify the sighash flag.

[notmandatory]

I prefer to not introduce declare_sighash, and just provide the defaults. In the case the intention of the signer is a different one, it will have to opt in for each of the inputs he wants to modify the sighash flag.

Just to clarify, do you mean the user should set the exposed TaprootCanSign::sighash_default field to false in miniscript's Assets.keys for this case?

If so then that works for me as long as docs are added to make it clear this must be done when a taproot key spend input is going to be get a non-standard sighash post-selection.

[nymius]

Just to clarify, do you mean the user should set the exposed TaprootCanSign::sighash_default field to false in miniscript's Assets.keys for this case?

I meant to always set DEFAULT/ALL for PSBT_IN_SIGHASH_TYPE. I didn't think in the Assets, but now that you mention it, I would say it shouldn't change. The non-default behavior should be opt in, and we can make clear in the docs what other changes need to be done, but we shouldn't have to add other guardrails if the user is explicitly moving out from the default behavior.

[nymius]

As a side note, psbt.sign calls these two methods at the moment of signing: https://github.com/rust-bitcoin/rust-bitcoin/blob/0.32.x/bitcoin/src/psbt/map/input.rs#L226-L243, which provide the safe defaults. I can set these defaults in the field at the moment of creation.

[evanlinjin]

@ValuedMammal @notmandatory @nymius The PR has been changed to always set PSBT_IN_SIGHASH_TYPE to DEFAULT/ALL as per @nymius' suggestion.