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

[evanlinjin]

Closes #60.
Supersedes #69.

Description

Note: Currently iterating on the design — see this comment for the current proposal. Diff still reflects the original approach pending reviewer feedback.

The problem

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 and 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.

The solution

This PR adds an always-on safety auto-lock: any Plan whose Schnorr witness template includes a 64B signature gets TapSighashType::Default written, independent of declare_sighash.

We also replace PsbtParams::sighash_type with PsbtParams::declare_sighash: bool. When declare_sighash is set (default), SIGHASH_ALL is written to all PSBT inputs that are "weight-safe".

The new behavior is summarized below:

Plan's Schnorr placeholders	declare_sighash: false	declare_sighash: true (default)
Any 64B	TapSighashType::Default (forced)	TapSighashType::Default (forced)
All 65B	unset	TapSighashType::All
None (ECDSA)	unset	EcdsaSighashType::All

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

Callers needing non-ALL sighashes (SINGLE, NONE, *_ANYONECANPAY) set 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.

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 sets declare_sighash = true as default, so that ALL will be written.

• Rationale: BIP-174 requires finalizers to reject sigs that don't match a declared sighash_type - we use this as a defense. ALL/DEFAULT is what the super-majority of callers would want.

Alternative (rejected) solutions

• Placing a sighash_type field in Plan-based bdk_tx::Input - This is a PSBT-concern. In fact, input witness can have multiple signatures.

Scope creep

I leaned up the tests a little beforehand: dca56db

Changelog notice

Added:
- `PsbtParams::declare_sighash` which writes `sighash_type = ALL` to all "weight-safe" inputs. This is the default that the super majority of callers would want.

Changed:
- Plan-based `Input`s whose witness template contains at least one 64B Schnorr signature will have `sighash_type = DEFAULT` written for that input, regardless of whether `PsbtParams::desclare_sighash` is set.

Removed:
- `PsbtParams::sighash_type` as transaction-wide sighash type setting is not useful.

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.