Merge rust-bitcoin/rust-miniscript#424: Fix up mixup of age/height

heap-coder · heap-coder · commit 8cfec362e616 · 2022-06-10T11:53:19.000-07:00
fec801414f542c5d378bc5a749d1cfac1c5b5276 Use lock_time instead of height (Tobin C. Harding)
3d76d02ba95c1372d7126c08e7a1f47fc0743fb9 Pass correct fields to evaluate_[after|older] (Tobin C. Harding)
b9c9d5bdaf71e9ee2b60c123734425e865a90451 Re-name function parameter to 'age' (Tobin C. Harding)
bfd6991945efbcc2d67b5f3a9333ba5244a52b68 Re-name function at_height -&gt; at_lock_time (Tobin C. Harding)
bb091eb7bc2e7ffa3b14d643c15bf22846fc851e Use correct parameter call order (Tobin C. Harding)

Pull request description:

  We currently use age/height as identifiers for values related to CLTV and CSV. However we get them mixed up in the function call to `from_txdata`. Do a few improvements to fix the mixup and improve the overall situation.

  - Patch 2 is a breaking change, changes a public method name.
  - Patch 1 an 4 are bug fixes.

  (This description and PR are totally re-written, the comment directly below is now stale.)

ACKs for top commit:
  apoelstra:
    ACK fec801414f542c5d378bc5a749d1cfac1c5b5276
  sanket1729:
    utACK fec801414f542c5d378bc5a749d1cfac1c5b5276

Tree-SHA512: fd116b3595bad9a5523e9e38ef4ca8d0df370629115893ee04bb0e42e061ea60d4dbc2f77b128bef4e016300f44b8e20d15013f24faa25b93a205556fc6df873
diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs
@@ -49,7 +49,7 @@ pub struct Interpreter<'txin> {
     /// is the leaf script; for key-spends it is `None`.
     script_code: Option<bitcoin::Script>,
     age: u32,
-    height: u32,
+    lock_time: u32,
 }
 
 // A type representing functions for checking signatures that accept both
@@ -170,16 +170,16 @@ impl<'txin> Interpreter<'txin> {
         spk: &bitcoin::Script,
         script_sig: &'txin bitcoin::Script,
         witness: &'txin Witness,
-        age: u32,
-        height: u32,
+        age: u32,       // CSV, relative lock time.
+        lock_time: u32, // CLTV, absolute lock time.
     ) -> Result<Self, Error> {
         let (inner, stack, script_code) = inner::from_txdata(spk, script_sig, witness)?;
         Ok(Interpreter {
             inner,
             stack,
             script_code,
             age,
-            height,
+            lock_time,
         })
     }
 
@@ -210,7 +210,7 @@ impl<'txin> Interpreter<'txin> {
             // call interpreter.iter() without mutating interpreter
             stack: self.stack.clone(),
             age: self.age,
-            height: self.height,
+            lock_time: self.lock_time,
             has_errored: false,
         }
     }
@@ -529,7 +529,7 @@ pub struct Iter<'intp, 'txin: 'intp> {
     state: Vec<NodeEvaluationState<'intp>>,
     stack: Stack<'txin>,
     age: u32,
-    height: u32,
+    lock_time: u32,
     has_errored: bool,
 }
 
@@ -606,15 +606,15 @@ where
                 Terminal::After(ref n) => {
                     debug_assert_eq!(node_state.n_evaluated, 0);
                     debug_assert_eq!(node_state.n_satisfied, 0);
-                    let res = self.stack.evaluate_after(n, self.age);
+                    let res = self.stack.evaluate_after(n, self.lock_time);
                     if res.is_some() {
                         return res;
                     }
                 }
                 Terminal::Older(ref n) => {
                     debug_assert_eq!(node_state.n_evaluated, 0);
                     debug_assert_eq!(node_state.n_satisfied, 0);
-                    let res = self.stack.evaluate_older(n, self.height);
+                    let res = self.stack.evaluate_older(n, self.age);
                     if res.is_some() {
                         return res;
                     }
@@ -1132,7 +1132,7 @@ mod tests {
                     n_satisfied: 0,
                 }],
                 age: 1002,
-                height: 1002,
+                lock_time: 1002,
                 has_errored: false,
             }
         }
diff --git a/src/interpreter/stack.rs b/src/interpreter/stack.rs
@@ -232,9 +232,9 @@ impl<'txin> Stack<'txin> {
     pub(super) fn evaluate_after(
         &mut self,
         n: &u32,
-        age: u32,
+        lock_time: u32,
     ) -> Option<Result<SatisfiedConstraint, Error>> {
-        if age >= *n {
+        if lock_time >= *n {
             self.push(Element::Satisfied);
             Some(Ok(SatisfiedConstraint::AbsoluteTimelock { time: *n }))
         } else {
@@ -251,9 +251,9 @@ impl<'txin> Stack<'txin> {
     pub(super) fn evaluate_older(
         &mut self,
         n: &u32,
-        height: u32,
+        age: u32,
     ) -> Option<Result<SatisfiedConstraint, Error>> {
-        if height >= *n {
+        if age >= *n {
             self.push(Element::Satisfied);
             Some(Ok(SatisfiedConstraint::RelativeTimelock { time: *n }))
         } else {
diff --git a/src/policy/semantic.rs b/src/policy/semantic.rs
@@ -538,39 +538,39 @@ impl<Pk: MiniscriptKey> Policy<Pk> {
     }
 
     /// Filter a policy by eliminating relative timelock constraints
-    /// that are not satisfied at the given age.
-    pub fn at_age(mut self, time: u32) -> Policy<Pk> {
+    /// that are not satisfied at the given `age`.
+    pub fn at_age(mut self, age: u32) -> Policy<Pk> {
         self = match self {
             Policy::Older(t) => {
-                if t > time {
+                if t > age {
                     Policy::Unsatisfiable
                 } else {
                     Policy::Older(t)
                 }
             }
             Policy::Threshold(k, subs) => {
-                Policy::Threshold(k, subs.into_iter().map(|sub| sub.at_age(time)).collect())
+                Policy::Threshold(k, subs.into_iter().map(|sub| sub.at_age(age)).collect())
             }
             x => x,
         };
         self.normalized()
     }
 
     /// Filter a policy by eliminating absolute timelock constraints
-    /// that are not satisfied at the given age.
-    pub fn at_height(mut self, time: u32) -> Policy<Pk> {
+    /// that are not satisfied at the given `n` (`n OP_CHECKLOCKTIMEVERIFY`).
+    pub fn at_lock_time(mut self, n: u32) -> Policy<Pk> {
         self = match self {
             Policy::After(t) => {
-                if !timelock::absolute_timelocks_are_same_unit(t, time) {
+                if !timelock::absolute_timelocks_are_same_unit(t, n) {
                     Policy::Unsatisfiable
-                } else if t > time {
+                } else if t > n {
                     Policy::Unsatisfiable
                 } else {
                     Policy::After(t)
                 }
             }
             Policy::Threshold(k, subs) => {
-                Policy::Threshold(k, subs.into_iter().map(|sub| sub.at_height(time)).collect())
+                Policy::Threshold(k, subs.into_iter().map(|sub| sub.at_lock_time(n)).collect())
             }
             x => x,
         };
@@ -787,12 +787,15 @@ mod tests {
         assert_eq!(policy, Policy::After(1000));
         assert_eq!(policy.absolute_timelocks(), vec![1000]);
         assert_eq!(policy.relative_timelocks(), vec![]);
-        assert_eq!(policy.clone().at_height(0), Policy::Unsatisfiable);
-        assert_eq!(policy.clone().at_height(999), Policy::Unsatisfiable);
-        assert_eq!(policy.clone().at_height(1000), policy.clone());
-        assert_eq!(policy.clone().at_height(10000), policy.clone());
-        // Pass a UNIX timestamp to at_height while policy uses a block height.
-        assert_eq!(policy.clone().at_height(500_000_001), Policy::Unsatisfiable);
+        assert_eq!(policy.clone().at_lock_time(0), Policy::Unsatisfiable);
+        assert_eq!(policy.clone().at_lock_time(999), Policy::Unsatisfiable);
+        assert_eq!(policy.clone().at_lock_time(1000), policy.clone());
+        assert_eq!(policy.clone().at_lock_time(10000), policy.clone());
+        // Pass a UNIX timestamp to at_lock_time while policy uses a block height.
+        assert_eq!(
+            policy.clone().at_lock_time(500_000_001),
+            Policy::Unsatisfiable
+        );
         assert_eq!(policy.n_keys(), 0);
         assert_eq!(policy.minimum_n_keys(), Some(0));
 
@@ -801,16 +804,22 @@ mod tests {
         assert_eq!(policy, Policy::After(500_000_010));
         assert_eq!(policy.absolute_timelocks(), vec![500_000_010]);
         assert_eq!(policy.relative_timelocks(), vec![]);
-        // Pass a block height to at_height while policy uses a UNIX timestapm.
-        assert_eq!(policy.clone().at_height(0), Policy::Unsatisfiable);
-        assert_eq!(policy.clone().at_height(999), Policy::Unsatisfiable);
-        assert_eq!(policy.clone().at_height(1000), Policy::Unsatisfiable);
-        assert_eq!(policy.clone().at_height(10000), Policy::Unsatisfiable);
-        // And now pass a UNIX timestamp to at_height while policy also uses a timestamp.
-        assert_eq!(policy.clone().at_height(500_000_000), Policy::Unsatisfiable);
-        assert_eq!(policy.clone().at_height(500_000_001), Policy::Unsatisfiable);
-        assert_eq!(policy.clone().at_height(500_000_010), policy.clone());
-        assert_eq!(policy.clone().at_height(500_000_012), policy.clone());
+        // Pass a block height to at_lock_time while policy uses a UNIX timestapm.
+        assert_eq!(policy.clone().at_lock_time(0), Policy::Unsatisfiable);
+        assert_eq!(policy.clone().at_lock_time(999), Policy::Unsatisfiable);
+        assert_eq!(policy.clone().at_lock_time(1000), Policy::Unsatisfiable);
+        assert_eq!(policy.clone().at_lock_time(10000), Policy::Unsatisfiable);
+        // And now pass a UNIX timestamp to at_lock_time while policy also uses a timestamp.
+        assert_eq!(
+            policy.clone().at_lock_time(500_000_000),
+            Policy::Unsatisfiable
+        );
+        assert_eq!(
+            policy.clone().at_lock_time(500_000_001),
+            Policy::Unsatisfiable
+        );
+        assert_eq!(policy.clone().at_lock_time(500_000_010), policy.clone());
+        assert_eq!(policy.clone().at_lock_time(500_000_012), policy.clone());
         assert_eq!(policy.n_keys(), 0);
         assert_eq!(policy.minimum_n_keys(), Some(0));
     }
diff --git a/src/psbt/finalizer.rs b/src/psbt/finalizer.rs
@@ -295,7 +295,7 @@ fn interpreter_inp_check<C: secp256k1::Verification, T: Borrow<TxOut>>(
         let cltv = psbt.unsigned_tx.lock_time;
         let csv = psbt.unsigned_tx.input[index].sequence;
         let interpreter =
-            interpreter::Interpreter::from_txdata(spk, script_sig, witness, cltv, csv)
+            interpreter::Interpreter::from_txdata(spk, script_sig, witness, csv, cltv)
                 .map_err(|e| Error::InputError(InputError::Interpreter(e), index))?;
         let iter = interpreter.iter(secp, &psbt.unsigned_tx, index, utxos);
         if let Some(error) = iter.filter_map(Result::err).next() {
