From cfb93eb49f865f33907111c08deaa258fbe76404 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 19 May 2025 23:12:21 +0000 Subject: [PATCH 1/2] descriptor: fix up regression_895 test This is supposed to be testing mixed timelocks, but it uses descriptors that are invalid for several unrelated reasons. These will be enforced in a later PR, so fix the issues now. Specifically: * It is not allowed to use the same `unavailable_key` multiple times * The rightmost branch must not be sigless; the timelock needs to have a pubkey and'ed with it. Otherwise the whole script is malleable. * Similarly the "cheap dissatisfaction" of v:after(144),pk({})) must not be sigless. We turn it into v:after(144),and_v(v:pk({})),pk({})) which requires a signature even to be dissatisfied. * Also, the giant comments don't accurately reflect what's going on in this test. They claim that post-#895, we will refuse to create a satisfaction if we'd be forced into mixing timelocks. But we don't. We actually do the same wrong thing as before, just for different reasons. The resulting test now passes even when all `ValidationParams` are turned on, though in this PR we don't confirm this. HOWEVER, despite the test working with a non-malleable descriptor now, the satisfaction that this test produces is malleable. For this I have filed https://github.com/rust-bitcoin/rust-miniscript/issues/976, which will be fixed in the next commit. --- src/miniscript/satisfy/mod.rs | 130 +++++++++++++++++++++++----------- 1 file changed, 89 insertions(+), 41 deletions(-) diff --git a/src/miniscript/satisfy/mod.rs b/src/miniscript/satisfy/mod.rs index 849e959cd..fba411c38 100644 --- a/src/miniscript/satisfy/mod.rs +++ b/src/miniscript/satisfy/mod.rs @@ -563,29 +563,73 @@ 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 = + 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::>() + .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(), ); @@ -593,29 +637,29 @@ mod tests { // 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. + // However, because of #976, it does not. // - // 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 = @@ -625,19 +669,23 @@ mod tests { assert_eq!(plan.absolute_timelock, Some(absolute::LockTime::from_height(144).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. + // + // 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.) // - // 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. + // 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::::from_str(&descriptor_str).unwrap(); From 99501a5e52b039f40971c73ffea2ab2cb5bd5014 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Fri, 5 Jun 2026 17:47:12 +0000 Subject: [PATCH 2/2] miniscript: don't use malleable satisfier, even for or_i dissatisfactions ...unless the user has explicitly requested a malleable satisfaction. Fixes #976 --- src/miniscript/satisfy/mod.rs | 11 +++++++---- src/miniscript/satisfy/sat_dissat.rs | 12 +----------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/src/miniscript/satisfy/mod.rs b/src/miniscript/satisfy/mod.rs index fba411c38..31259d182 100644 --- a/src/miniscript/satisfy/mod.rs +++ b/src/miniscript/satisfy/mod.rs @@ -651,7 +651,7 @@ mod tests { // // 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. - // However, because of #976, it does not. + // But if we use `into_plan_mall` we can see the bug. // // 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, @@ -664,9 +664,12 @@ mod tests { // Need DefiniteDescriptorKey https://github.com/rust-bitcoin/rust-miniscript/issues/927 let descriptor = Descriptor::::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. @@ -690,7 +693,7 @@ mod tests { let descriptor = Descriptor::::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()), diff --git a/src/miniscript/satisfy/sat_dissat.rs b/src/miniscript/satisfy/sat_dissat.rs index 53cb17fd8..2c5775af2 100644 --- a/src/miniscript/satisfy/sat_dissat.rs +++ b/src/miniscript/satisfy/sat_dissat.rs @@ -442,18 +442,8 @@ impl Satisfaction> { 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