wip: Use review suggestions

Author: tcharding

src/descriptor/bare.rs

@@ -37,7 +37,7 @@ use crate::{
 
 /// Create a Bare Descriptor. That is descriptor that is
 /// not wrapped in sh or wsh. This covers the Pk descriptor
-#[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
+#[derive(Clone, PartialOrd, Eq, PartialEq, Hash)]
 pub struct Bare<Pk: MiniscriptKey> {
     /// underlying miniscript
     ms: Miniscript<Pk, BareCtx>,

src/descriptor/mod.rs

@@ -70,7 +70,7 @@ pub use self::key::{
 pub type KeyMap = HashMap<DescriptorPublicKey, DescriptorSecretKey>;
 
 /// Script descriptor
-#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[derive(Clone, PartialEq, Eq, PartialOrd, Hash)]
 pub enum Descriptor<Pk: MiniscriptKey> {
     /// A raw scriptpubkey (including pay-to-pubkey) under Legacy context
     Bare(Bare<Pk>),

src/descriptor/segwitv0.rs

@@ -33,7 +33,7 @@ use crate::{
     TranslatePk,
 };
 /// A Segwitv0 wsh descriptor
-#[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
+#[derive(Clone, PartialOrd, Eq, PartialEq, Hash)]
 pub struct Wsh<Pk: MiniscriptKey> {
     /// underlying miniscript
     inner: WshInner<Pk>,
@@ -177,7 +177,7 @@ impl<Pk: MiniscriptKey + ToPublicKey> Wsh<Pk> {
 }
 
 /// Wsh Inner
-#[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
+#[derive(Clone, PartialOrd, Eq, PartialEq, Hash)]
 pub enum WshInner<Pk: MiniscriptKey> {
     /// Sorted Multi
     SortedMulti(SortedMultiVec<Pk, Segwitv0>),

src/descriptor/sh.rs

@@ -37,14 +37,14 @@ use crate::{
 };
 
 /// A Legacy p2sh Descriptor
-#[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
+#[derive(Clone, PartialOrd, Eq, PartialEq, Hash)]
 pub struct Sh<Pk: MiniscriptKey> {
     /// underlying miniscript
     inner: ShInner<Pk>,
 }
 
 /// Sh Inner
-#[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
+#[derive(Clone, PartialOrd, Eq, PartialEq, Hash)]
 pub enum ShInner<Pk: MiniscriptKey> {
     /// Nested Wsh
     Wsh(Wsh<Pk>),

src/descriptor/tr.rs

@@ -25,7 +25,7 @@ use crate::{
 /// A Taproot Tree representation.
 // Hidden leaves are not yet supported in descriptor spec. Conceptually, it should
 // be simple to integrate those here, but it is best to wait on core for the exact syntax.
-#[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
+#[derive(Clone, PartialOrd, Eq, PartialEq, Hash)]
 pub enum TapTree<Pk: MiniscriptKey> {
     /// A taproot tree structure
     Tree(Arc<TapTree<Pk>>, Arc<TapTree<Pk>>),
@@ -89,16 +89,6 @@ impl<Pk: MiniscriptKey> PartialOrd for Tr<Pk> {
     }
 }
 
-impl<Pk: MiniscriptKey> Ord for Tr<Pk> {
-    fn cmp(&self, other: &Self) -> cmp::Ordering {
-        match self.internal_key.cmp(&other.internal_key) {
-            cmp::Ordering::Equal => {}
-            ord => return ord,
-        }
-        self.tree.cmp(&other.tree)
-    }
-}
-
 impl<Pk: MiniscriptKey> hash::Hash for Tr<Pk> {
     fn hash<H: hash::Hasher>(&self, state: &mut H) {
         self.internal_key.hash(state);

src/interpreter/stack.rs

@@ -17,7 +17,7 @@
 use bitcoin;
 use bitcoin::blockdata::{opcodes, script};
 use bitcoin::hashes::{hash160, ripemd160, sha256, sha256d, Hash};
-use bitcoin::{locktime, LockTime};
+use bitcoin::LockTime;
 
 use super::error::PkEvalErrInner;
 use super::{
@@ -235,13 +235,13 @@ impl<'txin> Stack<'txin> {
         n: &LockTime,
         lock_time: LockTime,
     ) -> Option<Result<SatisfiedConstraint, Error>> {
-        match locktime::is_satisfied(*n, lock_time) {
+        match lock_time.is_satisfied(*n) {
             Ok(true) => {
                 self.push(Element::Satisfied);
                 Some(Ok(SatisfiedConstraint::AbsoluteTimelock { n: *n }))
             },
-            Ok(false) => Some(Err(Error::AbsoluteLocktimeNotMet(n.to_u32()))),
-            Err(_) => Some(Err(Error::AbsoluteLocktimeComparisonInvalid(n.to_u32(), lock_time.to_u32()))),
+            Ok(false) => Some(Err(Error::AbsoluteLocktimeNotMet(n.to_consensus_u32()))),
+            Err(_) => Some(Err(Error::AbsoluteLocktimeComparisonInvalid(n.to_consensus_u32(), lock_time.to_consensus_u32()))),
         }
     }
 

src/miniscript/astelem.rs

@@ -655,7 +655,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
                 .push_slice(&Pk::hash_to_hash160(hash)[..])
                 .push_opcode(opcodes::all::OP_EQUALVERIFY),
             Terminal::After(t) => builder
-                .push_int(t.to_u32() as i64)
+                .push_int(t.to_consensus_u32() as i64)
                 .push_opcode(opcodes::all::OP_CLTV),
             Terminal::Older(t) => builder.push_int(t as i64).push_opcode(opcodes::all::OP_CSV),
             Terminal::Sha256(h) => builder
@@ -790,7 +790,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
         match *self {
             Terminal::PkK(ref pk) => Ctx::pk_len(pk),
             Terminal::PkH(..) => 24,
-            Terminal::After(n) => script_num_size(n.to_u32() as usize) + 1,
+            Terminal::After(n) => script_num_size(n.to_consensus_u32() as usize) + 1,
             Terminal::Older(n) => script_num_size(n as usize) + 1,
             Terminal::Sha256(..) => 33 + 6,
             Terminal::Hash256(..) => 33 + 6,

src/miniscript/decode.rs

@@ -125,7 +125,7 @@ enum NonTerm {
 }
 /// All AST elements
 #[allow(broken_intra_doc_links)]
-#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[derive(Clone, PartialEq, Eq, PartialOrd, Hash)]
 pub enum Terminal<Pk: MiniscriptKey, Ctx: ScriptContext> {
     /// `1`
     True,

src/miniscript/mod.rs

@@ -76,16 +76,7 @@ pub struct Miniscript<Pk: MiniscriptKey, Ctx: ScriptContext> {
 /// by the ast.
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> PartialOrd for Miniscript<Pk, Ctx> {
     fn partial_cmp(&self, other: &Miniscript<Pk, Ctx>) -> Option<cmp::Ordering> {
-        Some(self.node.cmp(&other.node))
-    }
-}
-
-/// `Ord` of `Miniscript` must depend only on node and not the type information.
-/// The type information and extra_properties can be deterministically determined
-/// by the ast.
-impl<Pk: MiniscriptKey, Ctx: ScriptContext> Ord for Miniscript<Pk, Ctx> {
-    fn cmp(&self, other: &Miniscript<Pk, Ctx>) -> cmp::Ordering {
-        self.node.cmp(&other.node)
+        self.node.partial_cmp(&other.node)
     }
 }
 

src/miniscript/types/extra_props.rs

@@ -342,7 +342,7 @@ impl Property for ExtData {
 
     fn from_after(t: LockTime) -> Self {
         ExtData {
-            pk_cost: script_num_size(t.to_u32() as usize) + 1,
+            pk_cost: script_num_size(t.to_consensus_u32() as usize) + 1,
             has_free_verify: false,
             ops: OpLimits::new(1, Some(0), None),
             stack_elem_count_sat: Some(0),
@@ -936,7 +936,7 @@ impl Property for ExtData {
                 // Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The
                 // number on the stack would be a 5 bytes signed integer but Miniscript's B type
                 // only consumes 4 bytes from the stack.
-                if t.to_u32() == 0 || (t.to_u32() & SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0 {
+                if t.to_consensus_u32() == 0 || (t.to_consensus_u32() & SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0 {
                     return Err(Error {
                         fragment: fragment.clone(),
                         error: ErrorKind::InvalidTime,

src/miniscript/types/mod.rs

@@ -100,7 +100,7 @@ pub enum ErrorKind {
 }
 
 /// Error type for typechecking
-#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
+#[derive(Clone, PartialEq, Eq, PartialOrd, Hash, Debug)]
 pub struct Error<Pk: MiniscriptKey, Ctx: ScriptContext> {
     /// The fragment that failed typecheck
     pub fragment: Terminal<Pk, Ctx>,
@@ -306,7 +306,7 @@ pub trait Property: Sized {
     /// Type property of an absolute timelock. Default implementation simply
     /// passes through to `from_time`
     fn from_after(t: LockTime) -> Self {
-        Self::from_time(t.to_u32())
+        Self::from_time(t.to_consensus_u32())
     }
 
     /// Type property of a relative timelock. Default implementation simply
@@ -439,7 +439,7 @@ pub trait Property: Sized {
                 // Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The
                 // number on the stack would be a 5 bytes signed integer but Miniscript's B type
                 // only consumes 4 bytes from the stack.
-                if t.to_u32() == 0 || (t.to_u32() & SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0 {
+                if t.to_consensus_u32() == 0 || (t.to_consensus_u32() & SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0 {
                     return Err(Error {
                         fragment: fragment.clone(),
                         error: ErrorKind::InvalidTime,
@@ -822,7 +822,7 @@ impl Property for Type {
                 // Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The
                 // number on the stack would be a 5 bytes signed integer but Miniscript's B type
                 // only consumes 4 bytes from the stack.
-                if t.to_u32() == 0 || (t.to_u32() & SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0 {
+                if t.to_consensus_u32() == 0 || (t.to_consensus_u32() & SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0 {
                     return Err(Error {
                         fragment: fragment.clone(),
                         error: ErrorKind::InvalidTime,

src/policy/concrete.rs

@@ -46,7 +46,7 @@ use crate::{errstr, Error, ForEach, ForEachKey, MiniscriptKey};
 /// Concrete policy which corresponds directly to a Miniscript structure,
 /// and whose disjunctions are annotated with satisfaction probabilities
 /// to assist the compiler
-#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[derive(Clone, PartialEq, Eq, PartialOrd, Hash)]
 pub enum Policy<Pk: MiniscriptKey> {
     /// Unsatisfiable
     Unsatisfiable,
@@ -532,9 +532,9 @@ impl<Pk: MiniscriptKey> Policy<Pk> {
                 }
             }
             Policy::After(n)  => {
-                if n.to_u32() == 0 {
+                if n.to_consensus_u32() == 0 {
                     Err(PolicyError::ZeroTime)
-                } else if n.to_u32() > 2u32.pow(31) {
+                } else if n.to_consensus_u32() > 2u32.pow(31) {
                     Err(PolicyError::TimeTooFar)
                 } else {
                     Ok(())

src/policy/semantic.rs

@@ -32,7 +32,7 @@ use crate::{errstr, expression, Error, ForEach, ForEachKey, MiniscriptKey};
 /// Semantic policies store only hashes of keys to ensure that objects
 /// representing the same policy are lifted to the same `Semantic`,
 /// regardless of their choice of `pk` or `pk_h` nodes.
-#[derive(Clone, PartialEq, Eq, PartialOrd, Ord)]
+#[derive(Clone, PartialEq, Eq, PartialOrd)]
 pub enum Policy<Pk: MiniscriptKey> {
     /// Unsatisfiable
     Unsatisfiable,
@@ -504,7 +504,7 @@ impl<Pk: MiniscriptKey> Policy<Pk> {
             | Policy::Ripemd160(..)
             | Policy::Hash160(..) => vec![],
             Policy::Older(..) => vec![],
-            Policy::After(t) => vec![t.to_u32()],
+            Policy::After(t) => vec![t.to_consensus_u32()],
             Policy::Threshold(_, ref subs) => subs.iter().fold(vec![], |mut acc, x| {
                 acc.extend(x.real_absolute_timelocks());
                 acc

src/psbt/mod.rs

@@ -29,7 +29,7 @@ use bitcoin::secp256k1::{self, Secp256k1};
 use bitcoin::util::psbt::{self, PartiallySignedTransaction as Psbt};
 use bitcoin::util::sighash::SighashCache;
 use bitcoin::util::taproot::{self, ControlBlock, LeafVersion, TapLeafHash};
-use bitcoin::{self, locktime, EcdsaSighashType, LockTime, SchnorrSighashType, Script};
+use bitcoin::{self, EcdsaSighashType, LockTime, SchnorrSighashType, Script};
 
 use crate::miniscript::iter::PkPkh;
 use crate::miniscript::limits::SEQUENCE_LOCKTIME_DISABLE_FLAG;
@@ -343,7 +343,7 @@ impl<'psbt, Pk: MiniscriptKey + ToPublicKey> Satisfier<Pk> for PsbtInputSatisfie
             return false;
         }
 
-        match locktime::is_satisfied(n, lock_time) {
+        match lock_time.is_satisfied(n) {
             Err(_) => false,
             Ok(res) => res,
         }