feat(selection)!: declare sighash_type on Plan-derived inputs#74
feat(selection)!: declare sighash_type on Plan-derived inputs#74evanlinjin wants to merge 2 commits into
Conversation
35f1b48 to
3ff0b79
Compare
Previously, we had a string constants for test descriptors and test keys where the test keys were also hardcoded in the test descriptors. We remove the hard coded test descriptor and rename variables for clarity.
3ff0b79 to
f48fad7
Compare
|
Thanks for the detailed description. I was in favor of the original proposal in #60 - remove the |
What's the motivation for
|
There was a problem hiding this comment.
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.
f48fad7 to
8a758f1
Compare
Replaces `PsbtParams::sighash_type: Option<PsbtSighashType>` (a uniform tx-wide override) with `PsbtParams::declare_sighash: bool` (default `true`). A safety auto-lock always fires independent of `declare_sighash`: any Plan whose Schnorr witness template includes a 64B signature gets `TapSighashType::Default` written, preventing silent 64B-budget / 65B-sig under-funding. Mixed-size Plans land here too — the 65B-declared keys are over-funded by 1B each rather than risk under-funding the 64B-declared ones. For the remaining weight-safe inputs, `declare_sighash` controls whether to declare the implicit BIP-174 default explicitly: - `true`: write `TapSighashType::All` / `EcdsaSighashType::All` so finalizers enforce. - `false`: leave unset, caller manages. Breaking: callers using `PsbtParams::sighash_type` must move to post-construction `psbt.inputs[i].sighash_type` mutation. Exhaustive struct-literal construction of `PsbtParams` needs the new `declare_sighash` field (or `..Default::default()`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
8a758f1 to
afff4c6
Compare
|
@notmandatory I added the mixed sighash test! |
|
@ValuedMammal welcome to NACK, might need to formulate proper arguments 😅 |
|
@ValuedMammal Let me present my reasoning clearly so it's easier for you to address. Here are my claims:
Could you indicate which one you reject? Need to know what we are arguing about. |
Closes #60.
Supersedes #69.
Description
The problem
Two issues with how
PsbtParams::sighash_typeworked.1. The field itself wasn't very useful. It was a tx-wide uniform override. However, the realistic use cases for non-
ALLsighashes are inherently per-input.2. It didn't guard against the catastrophic Taproot case. Taproot signature size depends on sighash (
SIGHASH_DEFAULTis 64 bytes and other Taproot sighashes are 65 bytes). A miniscriptPlanhas the sighash type pre-baked andPlan::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::Defaultwritten, independent ofdeclare_sighash.We also replace
PsbtParams::sighash_typewithPsbtParams::declare_sighash: bool. Whendeclare_sighashis set (default),SIGHASH_ALLis written to all PSBT inputs that are "weight-safe".The new behavior is summarized below:
declare_sighash: falsedeclare_sighash: true(default)TapSighashType::Default(forced)TapSighashType::Default(forced)TapSighashType::AllEcdsaSighashType::AllPSBT-derived inputs (
Input::from_psbt_input) are never touched. They come in with their ownsighash_typealready set.Callers needing non-
ALLsighashes (SINGLE,NONE,*_ANYONECANPAY) setpsbt.inputs[i].sighash_typedirectly on the returned PSBT. Plans needing non-ALLsemantics on every key should be built with uniformTaprootCanSign::sighash_default = falseso the safety lock doesn't fire.Notes to the reviewers
This PR differs from #60's proposal.
The original issue suggested auto-writing
DEFAULTonly on uniformly-64B Plans (a plan can require multiple signatures with different sighash types). In this PR, aPlanwith any 64B placeholder triggerssighash_type = DEFAULT.The original issue proposed writing
Noneas default for "weight-safe" inputs. This PR setsdeclare_sighash = trueas default, so thatALLwill be written.sighash_type- we use this as a defense.ALL/DEFAULTis what the super-majority of callers would want.Alternative (rejected) solutions
sighash_typefield in Plan-basedbdk_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
Before submitting