diff --git a/src/miniscript/satisfy/mod.rs b/src/miniscript/satisfy/mod.rs index 849e959cd..31259d182 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,56 +637,63 @@ 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. + // 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::::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::::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