Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
evanlinjin wants to merge 2 commits into
base: master
Choose a base branch
from
+156 −11
Open

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
13a145c
test(selection): Refactor tests to reuse key strings
evanlinjin May 28, 2026
ca41645
feat(selection): always declare psbt input sighash type
evanlinjin May 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 156 additions & 11 deletions src/selection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use alloc::boxed::Box;
use alloc::vec::Vec;
use bitcoin::{EcdsaSighashType, TapSighashType};
use core::cmp::Ordering;
use core::fmt::{Debug, Display};

use miniscript::bitcoin;
use miniscript::bitcoin::{absolute, transaction, OutPoint, Psbt, Sequence};
use miniscript::psbt::PsbtExt;
Expand Down Expand Up @@ -306,6 +306,25 @@ impl Selection {
)));
}
}
// Safety auto-lock: any 64B Schnorr placeholder forces `Default`, independent
// of `declare_sighash`. A 64B-budgeted Plan signed with a 65B sig would
// silently under-fund the tx, and there is no caller scenario where that's
// intended — so this fires even when declaration is opted out.

@nymius nymius Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would keep all text content in ASCII: - instead of —, and -> instead of →.

use miniscript::miniscript::satisfy::Placeholder;
let any_64b_schnorr = plan

@nymius nymius Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understood what we wanted was to avoid undershooting the fees. If we want to do that, shouldn't we check some_65_schnorr and enforce All in that case? Default for taproot removes the trailing sighash byte, allowing a 65 byte signature within a tx that has allocated fees for a 64 signature.

@evanlinjin evanlinjin Jun 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

DEFAULT means 1 less byte per signature. If all signatures use DEFAULT even though the plan was expecting some ALL, means we will have less weight than expected. Less weight means we will overshoot the feerate which is safe.

.witness_template()
.iter()
.filter_map(|p| match p {
Placeholder::SchnorrSigPk(_, _, size)
| Placeholder::SchnorrSigPkHash(_, _, size) => Some(*size == 64),
_ => None,
})
.reduce(|a, b| a || b);
psbt_input.sighash_type = match any_64b_schnorr {
Some(true) => Some(TapSighashType::Default.into()),
Some(false) => Some(TapSighashType::All.into()),
None => Some(EcdsaSighashType::All.into()),
};

continue;
}
Expand Down Expand Up @@ -345,16 +364,18 @@ mod tests {
use miniscript::{plan::Assets, Descriptor, DescriptorPublicKey};
use rand_core::OsRng;

const TEST_DESCRIPTOR: &str = "tr([83737d5e/86h/1h/0h]tpubDDR5GgtoxS8fJyjjvdahN4VzV5DV6jtbcyvVXhEKq2XtpxjxBXmxH3r8QrNbQqHg4bJM1EGkxi7Pjfkgnui9jQWqS7kxHvX6rhUeriLDKxz/0/*)";
const TEST_DESCRIPTOR_PK: &str = "[83737d5e/86h/1h/0h]tpubDDR5GgtoxS8fJyjjvdahN4VzV5DV6jtbcyvVXhEKq2XtpxjxBXmxH3r8QrNbQqHg4bJM1EGkxi7Pjfkgnui9jQWqS7kxHvX6rhUeriLDKxz/0/*";
const TEST_HEX_PK: &str = "032b0558078bec38694a84933d659303e2575dae7e91685911454115bfd64487e3";
const TEST_KEY_HEX: &str = "032b0558078bec38694a84933d659303e2575dae7e91685911454115bfd64487e3";
const TEST_KEY_TR: &str = "[83737d5e/86h/1h/0h]tpubDDR5GgtoxS8fJyjjvdahN4VzV5DV6jtbcyvVXhEKq2XtpxjxBXmxH3r8QrNbQqHg4bJM1EGkxi7Pjfkgnui9jQWqS7kxHvX6rhUeriLDKxz/0/*";
const TEST_KEY_TR_2: &str = "[83737d5e/86h/1h/0h]tpubDDR5GgtoxS8fJyjjvdahN4VzV5DV6jtbcyvVXhEKq2XtpxjxBXmxH3r8QrNbQqHg4bJM1EGkxi7Pjfkgnui9jQWqS7kxHvX6rhUeriLDKxz/1/*";
const TEST_KEY_TR_3: &str = "[44444444/86h/1h/0h]tpubDDR5GgtoxS8fJyjjvdahN4VzV5DV6jtbcyvVXhEKq2XtpxjxBXmxH3r8QrNbQqHg4bJM1EGkxi7Pjfkgnui9jQWqS7kxHvX6rhUeriLDKxz/2/*";
const TEST_KEY_WPKH: &str = "[83737d5e/84h/1h/0h]tpubDDR5GgtoxS8fJyjjvdahN4VzV5DV6jtbcyvVXhEKq2XtpxjxBXmxH3r8QrNbQqHg4bJM1EGkxi7Pjfkgnui9jQWqS7kxHvX6rhUeriLDKxz/0/*";

fn setup_cltv_input(
cltv: absolute::LockTime,
) -> anyhow::Result<(Input, Descriptor<DescriptorPublicKey>)> {
let secp = Secp256k1::new();
let desc_str = format!("wsh(and_v(v:pk({TEST_HEX_PK}),after({cltv})))");
let desc_pk: DescriptorPublicKey = TEST_HEX_PK.parse()?;
let desc_str = format!("wsh(and_v(v:pk({TEST_KEY_HEX}),after({cltv})))");
let desc_pk: DescriptorPublicKey = TEST_KEY_HEX.parse()?;
let (desc, _) = Descriptor::parse_descriptor(&secp, &desc_str)?;
let plan = desc
.at_derivation_index(0)?
Expand Down Expand Up @@ -472,12 +493,12 @@ mod tests {

pub fn setup_test_input(confirmation_height: u32) -> anyhow::Result<Input> {
let secp = Secp256k1::new();
let desc = Descriptor::parse_descriptor(&secp, TEST_DESCRIPTOR)
let desc = Descriptor::parse_descriptor(&secp, &format!("tr({TEST_KEY_TR})"))
.unwrap()
.0;
let def_desc = desc.at_derivation_index(0).unwrap();
let script_pubkey = def_desc.script_pubkey();
let desc_pk: DescriptorPublicKey = TEST_DESCRIPTOR_PK.parse()?;
let desc_pk: DescriptorPublicKey = TEST_KEY_TR.parse()?;
let assets = Assets::new().add(desc_pk);
let plan = def_desc.plan(&assets).expect("failed to create plan");

Expand Down Expand Up @@ -663,8 +684,7 @@ mod tests {
// `assets`, forcing planning to use the script-path leaf (which sets
// `plan.relative_timelock`).
let secp = Secp256k1::new();
let desc_str =
format!("tr({TEST_HEX_PK},and_v(v:pk({TEST_DESCRIPTOR_PK}),older({csv_blocks})))");
let desc_str = format!("tr({TEST_KEY_HEX},and_v(v:pk({TEST_KEY_TR}),older({csv_blocks})))");
let desc = Descriptor::parse_descriptor(&secp, &desc_str)?
.0
.at_derivation_index(0)?;
Expand All @@ -678,7 +698,7 @@ mod tests {
}],
};
let assets = Assets::new()
.add(TEST_DESCRIPTOR_PK.parse::<DescriptorPublicKey>()?)
.add(TEST_KEY_TR.parse::<DescriptorPublicKey>()?)
.older(relative::LockTime::from_height(csv_blocks));
let plan = desc.plan(&assets).expect("script-path plan with CSV");
let status = crate::ConfirmationStatus {
Expand Down Expand Up @@ -798,4 +818,129 @@ mod tests {
shuffled.sort();
assert_eq!(shuffled, original);
}

fn input_with_assets(desc_str: &str, assets: Assets) -> anyhow::Result<Input> {
let secp = Secp256k1::new();
let (desc, _) = Descriptor::parse_descriptor(&secp, desc_str)?;
let def_desc = desc.at_derivation_index(0)?;
let script_pubkey = def_desc.script_pubkey();
let plan = def_desc.plan(&assets).expect("plan");
let prev_tx = Transaction {
version: transaction::Version::TWO,
lock_time: absolute::LockTime::ZERO,
input: vec![TxIn::default()],
output: vec![TxOut {
script_pubkey,
value: Amount::from_sat(100_000),
}],
};
Ok(Input::from_prev_tx(plan, prev_tx, 0, None)?)
}

fn non_default_taproot_assets(key: &DescriptorPublicKey) -> Assets {
use miniscript::plan::{CanSign, TaprootCanSign};
let mut assets = Assets::default();
for deriv_path in key.full_derivation_paths() {
let can_sign = CanSign {
ecdsa: true,

@nymius nymius Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't ecdsa irrelevant for this case?

@evanlinjin evanlinjin Jun 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure, but I don't think it's a problem leaving it as true. Some signers can sign both ECDSA and Schnorr

taproot: TaprootCanSign {
sighash_default: false,
..TaprootCanSign::default()
},
};
assets
.keys
.insert(((key.master_fingerprint(), deriv_path), can_sign));
}
assets
}

fn run_sighash_case(input: Input, params: PsbtParams) -> anyhow::Result<bitcoin::Psbt> {
let output = Output::with_script(ScriptBuf::new(), Amount::from_sat(9_000));
let selection = Selection::new(vec![input], vec![output]);
Ok(selection.create_psbt(params)?)
}

/// `create_psbt` writes the correct `sighash_type` on Plan-derived inputs across every
/// (witness-template, `declare_sighash`) combination:
///
/// - 64B Schnorr Plan → `Default`.
/// - 65B Schnorr Plan → `All`.
/// - Mixed 64B+65B Schnorr Plan → `Default`.

@nymius nymius Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Again, I think Default for 64B+65B is the unsafe sighash.

/// - ECDSA Plan → `EcdsaSighashType::All`.
#[test]
fn test_sighash_policy() -> anyhow::Result<()> {
use miniscript::plan::{CanSign, TaprootCanSign};

let tr_key: DescriptorPublicKey = TEST_KEY_TR.parse()?;
let wpkh_key: DescriptorPublicKey = TEST_KEY_WPKH.parse()?;

// Mixed-Assets Plan: one key budgeted 64B, one key budgeted 65B.
let mixed_assets = {
let key_default: DescriptorPublicKey = TEST_KEY_TR_2.parse()?;
let key_non_default: DescriptorPublicKey = TEST_KEY_TR_3.parse()?;
let mut assets = Assets::default();
for deriv_path in key_default.full_derivation_paths() {
assets.keys.insert((
(key_default.master_fingerprint(), deriv_path),
CanSign::default(),
));
}
for deriv_path in key_non_default.full_derivation_paths() {
assets.keys.insert((
(key_non_default.master_fingerprint(), deriv_path),
CanSign {
ecdsa: true,
taproot: TaprootCanSign {
sighash_default: false,
..TaprootCanSign::default()
},
},
));
}
assets
Comment on lines +889 to +901

@nymius nymius Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can't we use non_default_taproot_assets here?

};

type Expected = Option<bitcoin::psbt::PsbtSighashType>;
let cases: Vec<(&str, Input, Expected)> = vec![
(
"64B Tap",
input_with_assets(
&format!("tr({TEST_KEY_TR})"),
Assets::new().add(tr_key.clone()),
)?,
Some(TapSighashType::Default.into()),
),
(
"65B Tap",
input_with_assets(
&format!("tr({TEST_KEY_TR})"),
non_default_taproot_assets(&tr_key),
)?,
Some(TapSighashType::All.into()),
),
(
"ECDSA",
input_with_assets(
&format!("wpkh({TEST_KEY_WPKH})"),
Assets::new().add(wpkh_key.clone()),
)?,
Some(EcdsaSighashType::All.into()),
),
(
"Mixed Tap (64B + 65B)",
input_with_assets(
&format!("tr({TEST_KEY_TR},multi_a(2,{TEST_KEY_TR_2},{TEST_KEY_TR_3}))"),
mixed_assets,
)?,
Some(TapSighashType::Default.into()),
),
];

for (name, input, expected) in cases {
let psbt = run_sighash_case(input, PsbtParams::default())?;
assert_eq!(psbt.inputs[0].sighash_type, expected, "{name}");

@nymius nymius Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here, there is any way to check against an expected fee rate?

}
Ok(())
}
}