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,