From bb091eb7bc2e7ffa3b14d643c15bf22846fc851e Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 6 Jun 2022 10:24:51 +1000 Subject: [PATCH 1/5] Use correct parameter call order We are passing `csv` and `cltv` variables to `from_txdata` in the wrong order. While we are at it, add code comments linking age/csv and height/cltv. --- src/interpreter/mod.rs | 4 ++-- src/psbt/finalizer.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index 0c96da5fe..fa31358d9 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -169,8 +169,8 @@ 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. + height: u32, // CLTV, absolute lock time. ) -> Result { let (inner, stack, script_code) = inner::from_txdata(spk, script_sig, witness)?; Ok(Interpreter { diff --git a/src/psbt/finalizer.rs b/src/psbt/finalizer.rs index 4804bc581..cd52cadfb 100644 --- a/src/psbt/finalizer.rs +++ b/src/psbt/finalizer.rs @@ -295,7 +295,7 @@ fn interpreter_inp_check>( 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() { From bfd6991945efbcc2d67b5f3a9333ba5244a52b68 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 6 Jun 2022 10:28:05 +1000 Subject: [PATCH 2/5] Re-name function at_height -> at_lock_time The function takes as a parameter that represents the argument from `OP_CLTV`, this is a height _or_ time. Improve the function name to reflect this. --- src/policy/semantic.rs | 51 +++++++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 21 deletions(-) diff --git a/src/policy/semantic.rs b/src/policy/semantic.rs index 83d60b172..fa7b66d34 100644 --- a/src/policy/semantic.rs +++ b/src/policy/semantic.rs @@ -540,20 +540,20 @@ impl Policy { } /// 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 { + /// that are not satisfied at the given `n` (`n OP_CHECKLOCKTIMEVERIFY`). + pub fn at_lock_time(mut self, n: u32) -> Policy { 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, }; @@ -770,12 +770,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)); @@ -784,16 +787,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)); } From b9c9d5bdaf71e9ee2b60c123734425e865a90451 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 6 Jun 2022 10:30:35 +1000 Subject: [PATCH 3/5] Re-name function parameter to 'age' The parameter to `to_age` represents either a number of blocks _or_ a time (medium time passed). The term 'age' is associated with relative time locks in general. Use 'age' as the parameter name instead of 'time'. --- src/policy/semantic.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/policy/semantic.rs b/src/policy/semantic.rs index fa7b66d34..3160df0c5 100644 --- a/src/policy/semantic.rs +++ b/src/policy/semantic.rs @@ -521,18 +521,18 @@ impl Policy { } /// 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 { + /// that are not satisfied at the given `age`. + pub fn at_age(mut self, age: u32) -> Policy { 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, }; From 3d76d02ba95c1372d7126c08e7a1f47fc0743fb9 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 6 Jun 2022 11:00:03 +1000 Subject: [PATCH 4/5] Pass correct fields to evaluate_[after|older] We are passing the wrong fields into `evaluate_after` and `evaluate_older`. 'after' is associated with _absolute_ timelocks, this means 'height'. 'older' is associated with _relative_ timelocks, this means 'height'. --- src/interpreter/mod.rs | 4 ++-- src/interpreter/stack.rs | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index fa31358d9..5d5b229cd 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -605,7 +605,7 @@ 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.height); if res.is_some() { return res; } @@ -613,7 +613,7 @@ where 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; } diff --git a/src/interpreter/stack.rs b/src/interpreter/stack.rs index c85cb21d0..75adbe9a1 100644 --- 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> { - 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> { - if height >= *n { + if age >= *n { self.push(Element::Satisfied); Some(Ok(SatisfiedConstraint::RelativeTimelock { time: *n })) } else { From fec801414f542c5d378bc5a749d1cfac1c5b5276 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 8 Jun 2022 09:31:26 +1000 Subject: [PATCH 5/5] Use lock_time instead of height Currently we are using the identifier `height` for the CLTV vaule. This is incorrect because the value can be either a height or time. There is also a risk of mixing up the value with the CSV `age` value. Use `lock_time` to clearly disambiguate the CLTV value. --- src/interpreter/mod.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index 5d5b229cd..de5039a8b 100644 --- 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, age: u32, - height: u32, + lock_time: u32, } // A type representing functions for checking signatures that accept both @@ -169,8 +169,8 @@ impl<'txin> Interpreter<'txin> { spk: &bitcoin::Script, script_sig: &'txin bitcoin::Script, witness: &'txin Witness, - age: u32, // CSV, relative lock time. - height: u32, // CLTV, absolute lock time. + age: u32, // CSV, relative lock time. + lock_time: u32, // CLTV, absolute lock time. ) -> Result { let (inner, stack, script_code) = inner::from_txdata(spk, script_sig, witness)?; Ok(Interpreter { @@ -178,7 +178,7 @@ impl<'txin> Interpreter<'txin> { stack, script_code, age, - height, + lock_time, }) } @@ -209,7 +209,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, } } @@ -528,7 +528,7 @@ pub struct Iter<'intp, 'txin: 'intp> { state: Vec>, stack: Stack<'txin>, age: u32, - height: u32, + lock_time: u32, has_errored: bool, } @@ -605,7 +605,7 @@ 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.height); + let res = self.stack.evaluate_after(n, self.lock_time); if res.is_some() { return res; } @@ -1131,7 +1131,7 @@ mod tests { n_satisfied: 0, }], age: 1002, - height: 1002, + lock_time: 1002, has_errored: false, } }