Skip to content

miniscript: don't use malleable satisfier, even for or_i dissatisfactions #980

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

Merged
apoelstra merged 2 commits into from
Jun 14, 2026
+96 −55
Merged

miniscript: don't use malleable satisfier, even for or_i dissatisfactions #980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cfb93eb
descriptor: fix up regression_895 test
apoelstra May 19, 2025
99501a5
miniscript: don't use malleable satisfier, even for or_i dissatisfact…
apoelstra Jun 5, 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
139 changes: 95 additions & 44 deletions src/miniscript/satisfy/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,86 +563,137 @@ mod tests {

#[test]
fn regression_895() {
// Tests a pathological descriptor whose cheapest satisfaction would require mixing
// timelocks, although a more expensive satisfaction is available that avoids the
// timelock mixing. This descriptor is accepted by rust-miniscript but its satisfier
// cannot produce a satisfaction for it.
// Tests a pathological descriptor whose cheapest satisfaction involves computing a
// dissatisfaction containing a timelock. Such dissatisfactions exist because the
// `and_v` fragment, uniquely, has a dissatisfaction that includes the satisfaction
// of its left child. (Check sat_dissat.rs and search 'dissat: ' to see how the
// dissatisfactions of every fragment are computed. You will see that exactly one,
// and_v, uses the satisfaction of a child.)
//
// Prior to PR #895, the satisfaction logic would yield the invalid timelock-mixing
// satisfaction. After PR #895, it yields no satisfaction at all.
//
// The correct behavior is arguably to find the more expensive satisfaction. Doing
// this would would require some sort of backtracking in the satisfier and is highly
// nontrivial. Since triggering this bug requires generating a satisfaction in a
// context where both a height-based and time-based timelock are available, an
// impossible situation, it is probably not worth fixing.
// Prior to PR #895, the satisfier did not keep track of timelocks that were used
// for dissatisfactions. This can lead to incorrect plans, as demonstrated in the
// below test vectors.

// Setup: an unavailable key, used to make some branches unsatisfiable from the POV
// Setup: unavailable keys, used to make some branches unsatisfiable from the POV
// of the satisfier (but not the typechecker).
let unavailable_key = PublicKey::from_str(
"02eb64639a17f7334bb5a1a3aad857d6fec65faef439db3de72f85c88bc2906ad3",
)
.unwrap();
let available_keys: [PublicKey; 2] = [
"02eb64639a17f7334bb5a1a3aad857d6fec65faef439db3de72f85c88bc2906ad1"
.parse()
.unwrap(),
"02eb64639a17f7334bb5a1a3aad857d6fec65faef439db3de72f85c88bc2906ad3"
.parse()
.unwrap(),
];
let available_key_map = {
let dummy_sig = bitcoin::ecdsa::Signature::sighash_all(
secp256k1::ecdsa::Signature::from_compact(&[1; 64]).unwrap(),
);
let mut map = std::collections::BTreeMap::new();
map.insert(
crate::DefiniteDescriptorKey::new(available_keys[0].into()).unwrap(),
dummy_sig,
);
map.insert(
crate::DefiniteDescriptorKey::new(available_keys[1].into()).unwrap(),
dummy_sig,
);
map
};

// Generate a large pile of distinct unavailable keys.
let secp = secp256k1::Secp256k1::new();
let unavailable_keys: Vec<PublicKey> =
core::iter::successors(Some(available_keys[0]), |prev| {
prev.inner
.add_exp_tweak(&secp, &secp256k1::Scalar::ONE)
.ok()
.map(PublicKey::new)
})
.skip(1)
.take(20)
.collect();
assert_eq!(unavailable_keys.len(), 20); // sanity-check the above loop

// Create a fragment which is very expensive to dissatisfy.
let expensive_threshold = format!(
"thresh(10,pk({}),{})",
unavailable_keys[1], // we need key[0] below
unavailable_keys
.iter()
.skip(2)
.map(|k| format!("a:pkh({k})"))
.collect::<Vec<_>>()
.join(","),
);

// Setup: a satisfier that thinks that both a height-based and time-based timelock
// are available. This is needed for the second test.
let satisfier = (
available_key_map,
absolute::LockTime::from_height(1000).unwrap(),
absolute::LockTime::from_time(2000000000).unwrap(),
);

// Construct a script that would mix timelocks:
// or_b(
// n:or_i(
// and_v(v:after(144),pk()), // dissatisfied by a height-based timelock
// thresh(3,pk(),s:pk(),s:pk()) // dissatisfied by 3empty sigs (more expensive)
// and_v(v:after(144),and_v(v:pk(),pk())), // dissatisfied by a height-based timelock, a sig, and 0
// expensive_threshold // dissatisfied by a giant pile of pubkeyhash preimages
// ),
// sdv:after(50) // satisfied by a lower height-based timelock
// ajt:and_v(v:after(50),v:pk({})))) // satisfied by a lower height-based timelock and a sig
// )
//
// Here the or_i cannot be satisfied because none of the keys are available, so it
// Here the or_i cannot be satisfied due to missing keys on both branches, so it
// must be dissatisfied (and the after(50) branch must be satisfied). However, there
// are two dissatisfactions for the or_i: one which dissatisfies the first branch,
// by using the height-based timelock, and one which dissatisfies the second branch,
// which is ignored since it's the more expensive of the two possibilities.
//
// We therefore take both the after(144) and after(50) branches, and the resulting
// plan should show after(144) since it's the higher one. However, prior to #895,
// we "did not notice" the after(144) since it appears as part of a dissatisfaction.
// Since the first branch's dissatisfaction is HASSIG but the second branch's is not,
// the satisfier is required to dissatisfy the second branch to avoid malleability.
// But if we use `into_plan_mall` we can see the bug.
//
// Unrelatedly: the fact that the LHS of the `or_i` has a malleable dissatisfaction
// means that the whole script is malleable, and the fact that this parses at all is
// an instance of https://github.com/rust-bitcoin/rust-miniscript/issues/734

// Itstead, it takes both the after(144) and after(50) branches, and the resulting
// plan should show after(144) since it's the higher one. However, prior to #895,
// we "did not notice" the after(144) since it appears as part of a dissatisfaction,
// leading to a plan that did not match the actual timelock requirement.
let descriptor_str = format!(
"wsh(or_b(n:or_i(and_v(v:after(144),pk({})),thresh(3,pk({}),s:pk({}),s:pk({}))),sdv:after(50)))",
unavailable_key, unavailable_key, unavailable_key, unavailable_key,
"wsh(or_b(n:or_i(and_v(v:after(144),and_v(v:pk({}),pk({}))),{expensive_threshold}),ajt:and_v(v:after(50),v:pk({}))))",
available_keys[0], unavailable_keys[0], available_keys[1],
);
// Need DefiniteDescriptorKey https://github.com/rust-bitcoin/rust-miniscript/issues/927
let descriptor =
Descriptor::<crate::DefiniteDescriptorKey>::from_str(&descriptor_str).unwrap();
// Compute plan and confirm the timelock is correct.
let plan = descriptor.into_plan(&satisfier).unwrap();
// Compute plan and confirm the timelock is correct -- 144 for a malleable transaction
let plan = descriptor.clone().into_plan_mall(&satisfier).unwrap();
assert_eq!(plan.absolute_timelock, Some(absolute::LockTime::from_height(144).unwrap()),);
// ...and 50 for a non-malleable one (since take the expensive_threshold alternate)
let plan = descriptor.into_plan(&satisfier).unwrap();
assert_eq!(plan.absolute_timelock, Some(absolute::LockTime::from_height(50).unwrap()),);

// Same descriptor as above, except that now we use a time-based timelock rather than a
// lower height-based one. This time the "ideal" behavior would be that once we get to
// the final time-based timelock, we somehow backtrack and then use the more-expensive
// dissatisfaction choice for the `or_i`. (The type system guarantees that such a choice
// exists; otherwise the whole script would be flagged as mixing timelocks.)
// lower height-based one.
//
// However, our code architecture doesn't let us backtrack like this, and it's only possible
// to get into this situation if (a) you have a pathological script like this, and (b) you
// call .plan() or .satisfy() with both a height-based and time-based timelock set (which
// is impossible for any actual transaction). So for now we just use this unit test to
// document the behavior.
// Again, we wind up taking both timelock branches. Prior to #895, we would do this because
// we did not track the after(144) at all. After #895, we do it because our timelock logic
// handles mixed timelocks by silently clobbering one of them. (See the implementation of
// `concatenate_rev`, which calls `cmp::max` on the timelocks. This clobbers a locktime.
// See #979 for an explanation of this.)
//
// This time the "ideal" behavior would be that we track best satisfactions for both time-
// and height-based locktimes, and whenever we are forced into a conflict we throw one away.
// (The type system guarantees that we will always have at least one satisfaction left;
// otherwise the whole script would be flagged as mixing timelocks.)
//
// For now we just use this unit test to document the behavior.
let descriptor_str = format!(
"wsh(or_b(n:or_i(and_v(v:after(144),pk({})),thresh(2,pk({}),s:pk({}))),sdv:after(1000000000)))",
unavailable_key, unavailable_key, unavailable_key,
"wsh(or_b(n:or_i(and_v(v:after(144),and_v(v:pk({}),pk({}))),{expensive_threshold}),ajt:and_v(v:after(1000000000),v:pk({}))))",
available_keys[0], unavailable_keys[0], available_keys[1],
);
let descriptor =
Descriptor::<crate::DefiniteDescriptorKey>::from_str(&descriptor_str).unwrap();

let plan = descriptor.into_plan(&satisfier).unwrap();
let plan = descriptor.into_plan_mall(&satisfier).unwrap();
assert_eq!(
plan.absolute_timelock,
Some(absolute::LockTime::from_time(1000000000).unwrap()),
Expand Down
12 changes: 1 addition & 11 deletions src/miniscript/satisfy/sat_dissat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,18 +442,8 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
Terminal::OrI(_, _) => {
let SatDissat { dissat: r_dis, sat: r_sat } = stack.pop().unwrap();
let SatDissat { dissat: l_dis, sat: l_sat } = stack.pop().unwrap();
// Regarding use of `minimum_mall` for dissatisfactions: this is correct, because
// dissatisfactions for individual fragments are not required to be unique. (We
// track this property using the `ty.malleability.dissat` field.)
//
// This is because usually dissatisfactions are eventually dropped, e.g. by the
// `j:` wrapper or because at the top-level a dissatisfaction is simply invalid.
// In cases where non-unique dissatisfactions would cause malleability, e.g. in
// the `or_c` or `or_b` fragments which use dissatisfactions as part of their
// satisfactions, we set `ty.malleability.non_malleable` appropriately (and
// potentially reject the whole script at creation time).
SatDissat {
dissat: Self::minimum_mall(
dissat: min_fn(
Self {
stack: Witness::combine(l_dis.stack, Witness::push_1()),
..l_dis
Expand Down
Loading