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
#976, which will be
fixed in the next commit.

Author: apoelstra

src/miniscript/satisfy/mod.rs

@@ -563,59 +563,103 @@ 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; _] = [
+            "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.
+        // 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::<crate::DefiniteDescriptorKey>::from_str(&descriptor_str).unwrap();