Skip to content

Commit 950b793

Browse files
apoelstraapoelstra
committed
AbsLockTime: move to module, consolidate error checking
We have the type `AbsLockTime` whose purpose is to provide an ordering for locktimes. This type is converted from strings and script values, but we don't check validity during construction. Instead we have multiple places where validity is manually checked. Fix this. Note that this is an API-breaking change for people who are manually constructing policies and Miniscripts since it removes a few ways to unconditionally convert u32s into locktimes.
1 parent 60d7c98 commit 950b793

File tree

12 files changed

+148
-141
lines changed

12 files changed

+148
-141
lines changed

‎src/lib.rs

Lines changed: 8 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@
8787
#![deny(missing_docs)]
8888
// Clippy lints that we have disabled
8989
#![allow(clippy::iter_kv_map)] // https://github.com/rust-lang/rust-clippy/issues/11752
90+
#![allow(clippy::manual_range_contains)] // I hate this lint -asp
9091

9192
#[cfg(target_pointer_width = "16")]
9293
compile_error!(
@@ -125,19 +126,19 @@ pub mod iter;
125126
pub mod miniscript;
126127
pub mod plan;
127128
pub mod policy;
129+
mod primitives;
128130
pub mod psbt;
129131

130132
#[cfg(test)]
131133
mod test_utils;
132134
mod util;
133135

134-
use core::{cmp, fmt, hash, str};
136+
use core::{fmt, hash, str};
135137
#[cfg(feature = "std")]
136138
use std::error;
137139

138140
use bitcoin::hashes::{hash160, ripemd160, sha256, Hash};
139141
use bitcoin::hex::DisplayHex;
140-
use bitcoin::locktime::absolute;
141142
use bitcoin::{script, Opcode};
142143

143144
pub use crate::blanket_traits::FromStrKey;
@@ -149,6 +150,7 @@ pub use crate::miniscript::decode::Terminal;
149150
pub use crate::miniscript::satisfy::{Preimage32, Satisfier};
150151
pub use crate::miniscript::{hash256, Miniscript};
151152
use crate::prelude::*;
153+
pub use crate::primitives::absolute_locktime::{AbsLockTime, AbsLockTimeError};
152154

153155
/// Public key trait which can be converted to Hash type
154156
pub trait MiniscriptKey: Clone + Eq + Ord + fmt::Debug + fmt::Display + hash::Hash {
@@ -501,6 +503,8 @@ pub enum Error {
501503
/// At least two BIP389 key expressions in the descriptor contain tuples of
502504
/// derivation indexes of different lengths.
503505
MultipathDescLenMismatch,
506+
/// Invalid absolute locktime
507+
AbsoluteLockTime(AbsLockTimeError),
504508
}
505509

506510
// https://github.com/sipa/miniscript/pull/5 for discussion on this number
@@ -576,6 +580,7 @@ impl fmt::Display for Error {
576580
Error::TrNoScriptCode => write!(f, "No script code for Tr descriptors"),
577581
Error::TrNoExplicitScript => write!(f, "No script code for Tr descriptors"),
578582
Error::MultipathDescLenMismatch => write!(f, "At least two BIP389 key expressions in the descriptor contain tuples of derivation indexes of different lengths"),
583+
Error::AbsoluteLockTime(ref e) => e.fmt(f),
579584
}
580585
}
581586
}
@@ -628,6 +633,7 @@ impl error::Error for Error {
628633
ContextError(e) => Some(e),
629634
AnalysisError(e) => Some(e),
630635
PubKeyCtxError(e, _) => Some(e),
636+
AbsoluteLockTime(e) => Some(e),
631637
}
632638
}
633639
}
@@ -711,51 +717,6 @@ fn hex_script(s: &str) -> bitcoin::ScriptBuf {
711717
bitcoin::ScriptBuf::from(v)
712718
}
713719

714-
/// An absolute locktime that implements `Ord`.
715-
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
716-
pub struct AbsLockTime(absolute::LockTime);
717-
718-
impl AbsLockTime {
719-
/// Constructs an `AbsLockTime` from an nLockTime value or the argument to OP_CHEKCLOCKTIMEVERIFY.
720-
pub fn from_consensus(n: u32) -> Self { Self(absolute::LockTime::from_consensus(n)) }
721-
722-
/// Returns the inner `u32` value. This is the value used when creating this `LockTime`
723-
/// i.e., `n OP_CHECKLOCKTIMEVERIFY` or nLockTime.
724-
///
725-
/// This calls through to `absolute::LockTime::to_consensus_u32()` and the same usage warnings
726-
/// apply.
727-
pub fn to_consensus_u32(self) -> u32 { self.0.to_consensus_u32() }
728-
729-
/// Returns the inner `u32` value.
730-
///
731-
/// Equivalent to `AbsLockTime::to_consensus_u32()`.
732-
pub fn to_u32(self) -> u32 { self.to_consensus_u32() }
733-
}
734-
735-
impl From<absolute::LockTime> for AbsLockTime {
736-
fn from(lock_time: absolute::LockTime) -> Self { Self(lock_time) }
737-
}
738-
739-
impl From<AbsLockTime> for absolute::LockTime {
740-
fn from(lock_time: AbsLockTime) -> absolute::LockTime { lock_time.0 }
741-
}
742-
743-
impl cmp::PartialOrd for AbsLockTime {
744-
fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> { Some(self.cmp(other)) }
745-
}
746-
747-
impl cmp::Ord for AbsLockTime {
748-
fn cmp(&self, other: &Self) -> cmp::Ordering {
749-
let this = self.0.to_consensus_u32();
750-
let that = other.0.to_consensus_u32();
751-
this.cmp(&that)
752-
}
753-
}
754-
755-
impl fmt::Display for AbsLockTime {
756-
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(&self.0, f) }
757-
}
758-
759720
#[cfg(test)]
760721
mod tests {
761722
use core::str::FromStr;

‎src/miniscript/astelem.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -256,9 +256,9 @@ impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Termina
256256
expression::terminal(&top.args[0], |x| Pk::from_str(x).map(Terminal::PkH))
257257
}
258258
("after", 1) => expression::terminal(&top.args[0], |x| {
259-
expression::parse_num(x).map(|x| {
260-
Terminal::After(AbsLockTime::from(absolute::LockTime::from_consensus(x)))
261-
})
259+
expression::parse_num(x)
260+
.and_then(|x| AbsLockTime::from_consensus(x).map_err(Error::AbsoluteLockTime))
261+
.map(Terminal::After)
262262
}),
263263
("older", 1) => expression::terminal(&top.args[0], |x| {
264264
expression::parse_num(x).map(|x| Terminal::Older(Sequence::from_consensus(x)))

‎src/miniscript/decode.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ pub fn parse<Ctx: ScriptContext>(
359359
Tk::CheckSequenceVerify, Tk::Num(n)
360360
=> term.reduce0(Terminal::Older(Sequence::from_consensus(n)))?,
361361
Tk::CheckLockTimeVerify, Tk::Num(n)
362-
=> term.reduce0(Terminal::After(AbsLockTime::from_consensus(n)))?,
362+
=> term.reduce0(Terminal::After(AbsLockTime::from_consensus(n).map_err(Error::AbsoluteLockTime)?))?,
363363
// hashlocks
364364
Tk::Equal => match_token!(
365365
tokens,

‎src/miniscript/mod.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1370,7 +1370,7 @@ mod tests {
13701370
(format!("t:or_c(pk({}),v:pkh({}))", key_present, key_missing), None, None),
13711371
(
13721372
format!("thresh(2,pk({}),s:pk({}),snl:after(1))", key_present, key_missing),
1373-
Some(AbsLockTime::from_consensus(1)),
1373+
Some(AbsLockTime::from_consensus(1).unwrap()),
13741374
None,
13751375
),
13761376
(
@@ -1388,7 +1388,7 @@ mod tests {
13881388
"thresh(3,pk({}),s:pk({}),snl:older(10),snl:after(11))",
13891389
key_present, key_missing
13901390
),
1391-
Some(AbsLockTime::from_consensus(11)),
1391+
Some(AbsLockTime::from_consensus(11).unwrap()),
13921392
Some(bitcoin::Sequence(10)),
13931393
),
13941394
(

‎src/miniscript/types/extra_props.rs

Lines changed: 4 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,12 @@
66
use core::cmp;
77
use core::iter::once;
88

9-
use bitcoin::{absolute, Sequence};
9+
use bitcoin::Sequence;
1010

1111
use super::{Error, ErrorKind, ScriptContext};
1212
use crate::miniscript::context::SigType;
1313
use crate::prelude::*;
14-
use crate::{script_num_size, MiniscriptKey, Terminal};
14+
use crate::{script_num_size, AbsLockTime, MiniscriptKey, Terminal};
1515

1616
/// Timelock information for satisfaction of a fragment.
1717
#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Default, Hash)]
@@ -343,7 +343,7 @@ impl ExtData {
343343
}
344344

345345
/// Extra properties for the `after` fragment.
346-
pub fn after(t: absolute::LockTime) -> Self {
346+
pub fn after(t: AbsLockTime) -> Self {
347347
ExtData {
348348
pk_cost: script_num_size(t.to_consensus_u32() as usize) + 1,
349349
has_free_verify: false,
@@ -923,18 +923,7 @@ impl ExtData {
923923
_ => unreachable!(),
924924
}
925925
}
926-
Terminal::After(t) => {
927-
// Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The
928-
// number on the stack would be a 5 bytes signed integer but Miniscript's B type
929-
// only consumes 4 bytes from the stack.
930-
if t == absolute::LockTime::ZERO.into() {
931-
return Err(Error {
932-
fragment_string: fragment.to_string(),
933-
error: ErrorKind::InvalidTime,
934-
});
935-
}
936-
Self::after(t.into())
937-
}
926+
Terminal::After(t) => Self::after(t),
938927
Terminal::Older(t) => {
939928
if t == Sequence::ZERO || !t.is_relative_lock_time() {
940929
return Err(Error {

‎src/miniscript/types/mod.rs

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use core::fmt;
1414
#[cfg(feature = "std")]
1515
use std::error;
1616

17-
use bitcoin::{absolute, Sequence};
17+
use bitcoin::Sequence;
1818

1919
pub use self::correctness::{Base, Correctness, Input};
2020
pub use self::extra_props::ExtData;
@@ -478,18 +478,7 @@ impl Type {
478478
_ => unreachable!(),
479479
}
480480
}
481-
Terminal::After(t) => {
482-
// Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The
483-
// number on the stack would be a 5 bytes signed integer but Miniscript's B type
484-
// only consumes 4 bytes from the stack.
485-
if t == absolute::LockTime::ZERO.into() {
486-
return Err(Error {
487-
fragment_string: fragment.to_string(),
488-
error: ErrorKind::InvalidTime,
489-
});
490-
}
491-
Ok(Self::time())
492-
}
481+
Terminal::After(_) => Ok(Self::time()),
493482
Terminal::Older(t) => {
494483
if t == Sequence::ZERO || !t.is_relative_lock_time() {
495484
return Err(Error {

‎src/policy/compiler.rs

Lines changed: 5 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use core::{cmp, f64, fmt, hash, mem};
99
#[cfg(feature = "std")]
1010
use std::error;
1111

12-
use bitcoin::{absolute, Sequence};
12+
use bitcoin::Sequence;
1313
use sync::Arc;
1414

1515
use crate::miniscript::context::SigType;
@@ -446,18 +446,7 @@ impl CompilerExtData {
446446
_ => unreachable!(),
447447
}
448448
}
449-
Terminal::After(t) => {
450-
// Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The
451-
// number on the stack would be a 5 bytes signed integer but Miniscript's B type
452-
// only consumes 4 bytes from the stack.
453-
if t == absolute::LockTime::ZERO.into() {
454-
return Err(types::Error {
455-
fragment_string: fragment.to_string(),
456-
error: types::ErrorKind::InvalidTime,
457-
});
458-
}
459-
Ok(Self::time())
460-
}
449+
Terminal::After(_) => Ok(Self::time()),
461450
Terminal::Older(t) => {
462451
if t == Sequence::ZERO || !t.is_relative_lock_time() {
463452
return Err(types::Error {
@@ -1265,7 +1254,7 @@ mod tests {
12651254
use super::*;
12661255
use crate::miniscript::{Legacy, Segwitv0, Tap};
12671256
use crate::policy::Liftable;
1268-
use crate::{script_num_size, ToPublicKey};
1257+
use crate::{script_num_size, AbsLockTime, ToPublicKey};
12691258

12701259
type SPolicy = Concrete<String>;
12711260
type BPolicy = Concrete<bitcoin::PublicKey>;
@@ -1311,8 +1300,8 @@ mod tests {
13111300
let pol: SPolicy = Concrete::And(vec![
13121301
Arc::new(Concrete::Key("A".to_string())),
13131302
Arc::new(Concrete::And(vec![
1314-
Arc::new(Concrete::after(9)),
1315-
Arc::new(Concrete::after(1000_000_000)),
1303+
Arc::new(Concrete::After(AbsLockTime::from_consensus(9).unwrap())),
1304+
Arc::new(Concrete::After(AbsLockTime::from_consensus(1_000_000_000).unwrap())),
13161305
])),
13171306
]);
13181307
assert!(pol.compile::<Segwitv0>().is_err());

‎src/policy/concrete.rs

Lines changed: 6 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -74,12 +74,6 @@ impl<Pk> Policy<Pk>
7474
where
7575
Pk: MiniscriptKey,
7676
{
77-
/// Construct a `Policy::After` from `n`. Helper function equivalent to
78-
/// `Policy::After(absolute::LockTime::from_consensus(n))`.
79-
pub fn after(n: u32) -> Policy<Pk> {
80-
Policy::After(AbsLockTime::from(absolute::LockTime::from_consensus(n)))
81-
}
82-
8377
/// Construct a `Policy::Older` from `n`. Helper function equivalent to
8478
/// `Policy::Older(Sequence::from_consensus(n))`.
8579
pub fn older(n: u32) -> Policy<Pk> { Policy::Older(Sequence::from_consensus(n)) }
@@ -756,13 +750,7 @@ impl<Pk: MiniscriptKey> Policy<Pk> {
756750

757751
for policy in self.pre_order_iter() {
758752
match *policy {
759-
After(n) => {
760-
if n == absolute::LockTime::ZERO.into() {
761-
return Err(PolicyError::ZeroTime);
762-
} else if n.to_u32() > 2u32.pow(31) {
763-
return Err(PolicyError::TimeTooFar);
764-
}
765-
}
753+
After(_) => {}
766754
Older(n) => {
767755
if n == Sequence::ZERO {
768756
return Err(PolicyError::ZeroTime);
@@ -978,15 +966,11 @@ impl<Pk: FromStrKey> Policy<Pk> {
978966
("UNSATISFIABLE", 0) => Ok(Policy::Unsatisfiable),
979967
("TRIVIAL", 0) => Ok(Policy::Trivial),
980968
("pk", 1) => expression::terminal(&top.args[0], |pk| Pk::from_str(pk).map(Policy::Key)),
981-
("after", 1) => {
982-
let num = expression::terminal(&top.args[0], expression::parse_num)?;
983-
if num > 2u32.pow(31) {
984-
return Err(Error::PolicyError(PolicyError::TimeTooFar));
985-
} else if num == 0 {
986-
return Err(Error::PolicyError(PolicyError::ZeroTime));
987-
}
988-
Ok(Policy::after(num))
989-
}
969+
("after", 1) => expression::terminal(&top.args[0], |x| {
970+
expression::parse_num(x)
971+
.and_then(|x| AbsLockTime::from_consensus(x).map_err(Error::AbsoluteLockTime))
972+
.map(Policy::After)
973+
}),
990974
("older", 1) => {
991975
let num = expression::terminal(&top.args[0], expression::parse_num)?;
992976
if num > 2u32.pow(31) {

‎src/policy/mod.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -323,11 +323,13 @@ mod tests {
323323
ConcretePol::from_str("or(pk())").unwrap_err().to_string(),
324324
"Or policy fragment must take 2 arguments"
325325
);
326+
// this weird "unexpected" wrapping of the error will go away in a later PR
327+
// which rewrites the expression parser
326328
assert_eq!(
327329
ConcretePol::from_str("thresh(3,after(0),pk(),pk())")
328330
.unwrap_err()
329331
.to_string(),
330-
"Time must be greater than 0; n > 0"
332+
"unexpected «absolute locktimes in Miniscript have a minimum value of 1»",
331333
);
332334

333335
assert_eq!(

0 commit comments

Comments
 (0)