Merge #419: Fix bug in absolute timelock usage

42e1216 Unit test mixed up absolute timelocks (Tobin C. Harding)
e4eb285 Add a minimal timelock module (Tobin C. Harding)
0b62f5e Fix incorrect timelock docs (Tobin C. Harding)

Pull request description:

  Currently if we mix up height/time absolute timelocks when filtering policies the result is incorrect (to the best of my understanding).

  Add a `timelock` module that includes a single public function for checking if absolute timelocks are the same unit. Use the new function to fix a bug in policy filtering.

  - Patch 1 is a docs bug fix
  - Patch 2 is the bug fix
  - Patch 3 is a unit test patch that fails if its moved before patch 2. Please review the unit test carefully to make sure I'm not confused.

  ## Note

  There is [ongoing discussion](rust-bitcoin/rust-bitcoin#994) around trying to design a suitable timelock API. This PR is an attempt to make some forward progress by taking baby steps _and_ making objective improvements. I decided to do it here in miniscript because it will be easier to iterate on and more obvious when there are concrete usage examples along with each change.

  cc dpc

ACKs for top commit:
  apoelstra:
    ACK 42e1216

Tree-SHA512: 93a55c550a903477cf20f50873d7b778bc36c1eaaf9a31c247d46d931f46100296d3fc3c89cf34ad6b029a7a14f6ae20d8208f22f4536fffe23cd5dab5fa674b

Author: sanket1729

src/lib.rs

@@ -104,6 +104,7 @@ pub mod interpreter;
 pub mod miniscript;
 pub mod policy;
 pub mod psbt;
+pub mod timelock;
 
 mod util;
 

src/miniscript/types/mod.rs

@@ -298,13 +298,13 @@ pub trait Property: Sized {
     /// Type property of a timelock
     fn from_time(t: u32) -> Self;
 
-    /// Type property of a relative timelock. Default implementation simply
+    /// Type property of an absolute timelock. Default implementation simply
     /// passes through to `from_time`
     fn from_after(t: u32) -> Self {
         Self::from_time(t)
     }
 
-    /// Type property of an absolute timelock. Default implementation simply
+    /// Type property of a relative timelock. Default implementation simply
     /// passes through to `from_time`
     fn from_older(t: u32) -> Self {
         Self::from_time(t)

src/policy/semantic.rs

@@ -22,7 +22,7 @@ use bitcoin::hashes::{hash160, ripemd160, sha256, sha256d};
 
 use super::concrete::PolicyError;
 use super::ENTAILMENT_MAX_TERMINALS;
-use crate::{errstr, expression, Error, ForEach, ForEachKey, MiniscriptKey};
+use crate::{errstr, expression, timelock, Error, ForEach, ForEachKey, MiniscriptKey};
 
 /// Abstract policy which corresponds to the semantics of a Miniscript
 /// and which allows complex forms of analysis, e.g. filtering and
@@ -543,7 +543,9 @@ impl<Pk: MiniscriptKey> Policy<Pk> {
     pub fn at_height(mut self, time: u32) -> Policy<Pk> {
         self = match self {
             Policy::After(t) => {
-                if t > time {
+                if !timelock::absolute_timelocks_are_same_unit(t, time) {
+                    Policy::Unsatisfiable
+                } else if t > time {
                     Policy::Unsatisfiable
                 } else {
                     Policy::After(t)
@@ -762,6 +764,7 @@ mod tests {
         assert_eq!(policy.n_keys(), 0);
         assert_eq!(policy.minimum_n_keys(), Some(0));
 
+        // Block height 1000.
         let policy = StringPolicy::from_str("after(1000)").unwrap();
         assert_eq!(policy, Policy::After(1000));
         assert_eq!(policy.absolute_timelocks(), vec![1000]);
@@ -770,6 +773,26 @@ mod tests {
         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.n_keys(), 0);
+        assert_eq!(policy.minimum_n_keys(), Some(0));
+
+        // UNIX timestamp of 10 seconds after the epoch.
+        let policy = StringPolicy::from_str("after(500000010)").unwrap();
+        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());
         assert_eq!(policy.n_keys(), 0);
         assert_eq!(policy.minimum_n_keys(), Some(0));
     }

src/timelock.rs

@@ -0,0 +1,21 @@
+//! Various functions for manipulating Bitcoin timelocks.
+
+use crate::miniscript::limits::LOCKTIME_THRESHOLD;
+
+/// Returns true if `a` and `b` are the same unit i.e., both are block heights or both are UNIX
+/// timestamps. `a` and `b` are nLockTime values.
+pub fn absolute_timelocks_are_same_unit(a: u32, b: u32) -> bool {
+    n_lock_time_is_block_height(a) == n_lock_time_is_block_height(b)
+}
+
+// https://github.com/bitcoin/bitcoin/blob/9ccaee1d5e2e4b79b0a7c29aadb41b97e4741332/src/script/script.h#L39
+
+/// Returns true if nLockTime `n` is to be interpreted as a block height.
+pub fn n_lock_time_is_block_height(n: u32) -> bool {
+    n < LOCKTIME_THRESHOLD
+}
+
+/// Returns true if nLockTime `n` is to be interpreted as a UNIX timestamp.
+pub fn n_lock_time_is_timestamp(n: u32) -> bool {
+    n >= LOCKTIME_THRESHOLD
+}