Merge #89: Fix a bunch of clippy lints and get CI working again

e151a85 ci: bump fuzz MSRV to 1.65 (Andrew Poelstra)
b10c7a2 ci: add one more pin (Andrew Poelstra)
7c977b5 ForEachKey: clean up Concrete impl, remove Semantic impl (Andrew Poelstra)
8bc9311 doc: improve list of fields covered by sighash (Andrew Poelstra)
97b2229 clippy: unnecessary vecs (Andrew Poelstra)
8894215 clippy: miscellaneous simple fixes (Andrew Poelstra)
fcb2c3d clippy: canonical partial_cmp implementations (Andrew Poelstra)
bb88c18 clippy: replace fold calls with try_fold (Andrew Poelstra)
9815ca7 rustc: silence some "dead code" warnings (Andrew Poelstra)
4bab769 rustfmt: change to new notation (Andrew Poelstra)
de4e2ec cargo: lint unused cfg parameters (Andrew Poelstra)

Pull request description:

  Probably needs #88 to go in in parallel.

ACKs for top commit:
  RCasatta:
    utACK e151a85

Tree-SHA512: 80260fa8b38c543b437230b1f6e2464ef97a1755ff3565ae8204f7fbfe55fe0b5a197632de48dce8bc2e0170d0e30f5a3676314f62ce61cee8b5b0c6b7c83487

.github/workflows/fuzz.yml

@@ -40,7 +40,7 @@ roundtrip_confidential,
           key: cache-${{ matrix.target }}-${{ hashFiles('**/Cargo.toml','**/Cargo.lock') }}
       - uses: actions-rs/toolchain@v1
         with:
-          toolchain: 1.58
+          toolchain: 1.65
           override: true
           profile: minimal
       - name: fuzz

Cargo.toml

@@ -35,6 +35,8 @@ bitcoin = { version = "0.31.0", features = ["base64"] }
 secp256k1 = {version = "0.28.0", features = ["rand-std"]}
 actual-base64 = { package = "base64", version = "0.13.0" }
 
+[lints.rust]
+unexpected_cfgs = { level = "deny", check-cfg = ['cfg(miniscript_bench)'] }
 
 [[example]]
 name = "htlc"

contrib/test.sh

@@ -11,6 +11,7 @@ rustc --version
 if cargo --version | grep "1\.58"; then
     cargo update -p byteorder --precise 1.4.3
     cargo update -p cc --precise 1.0.94
+    cargo update -p ppv-lite86 --precise 0.2.17
 fi
 
 # Format if told to

examples/sign_multisig.rs

@@ -46,8 +46,8 @@ fn main() {
         }],
     };
 
-    #[cfg_attr(feature="cargo-fmt", rustfmt_skip)]
-    let public_keys = vec![
+    #[rustfmt::skip]
+    let public_keys = [
         bitcoin::PublicKey::from_slice(&[2; 33]).expect("key 1"),
         bitcoin::PublicKey::from_slice(&[
             0x02,

src/confidential/mod.rs

@@ -408,7 +408,7 @@ mod tests {
             ConfidentialTest {
                 key: Key::Bare(single_ct_key.clone()),
                 descriptor: crate::Descriptor::new_wpkh(single_spk_key).unwrap(),
-                descriptor_str: format!("ct(02dce16018bbbb8e36de7b394df5b5166e9adb7498be7d881a85a09aeecf76b623,elwpkh(03774eec7a3d550d18e9f89414152025b3b0ad6a342b19481f702d843cff06dfc4))#h5e0p6m9"),
+                descriptor_str: "ct(02dce16018bbbb8e36de7b394df5b5166e9adb7498be7d881a85a09aeecf76b623,elwpkh(03774eec7a3d550d18e9f89414152025b3b0ad6a342b19481f702d843cff06dfc4))#h5e0p6m9".to_string(),
                 conf_addr: "lq1qq0r6pegudzm0tzpszelc34qjln4fdxawgwmgnza63wwpzdy6jrm0grmqvvk2ce5ksnxcs9ecgtnryt7xg3406y5ccl0k2glns",
                 unconf_addr: "ex1qpasxxt9vv6tgfnvgzuuy9e3j9lryg6hawrval4",
             },

src/descriptor/csfs_cov/cov.rs

@@ -25,7 +25,7 @@
 //! 1. nVersion of the transaction (4-byte little endian)
 //! 2. hashPrevouts (32-byte hash)
 //! 3. hashSequence (32-byte hash)
-//! 3b. ELEMENTS EXTRA hashIssuances (32-byte hash)
+//! 3. ELEMENTS EXTRA hashIssuances (32-byte hash)
 //! 4. outpoint (32-byte hash + 4-byte little endian)
 //! 5. scriptCode of the input (serialized as scripts inside CTxOuts)
 //! 6. value of the output spent by this input (8-byte little endian)

src/descriptor/csfs_cov/mod.rs

@@ -26,7 +26,7 @@
 //! 1. nVersion of the transaction (4-byte little endian)
 //! 2. hashPrevouts (32-byte hash)
 //! 3. hashSequence (32-byte hash)
-//! 3b. ELEMENTS EXTRA hashIssuances (32-byte hash)
+//! 3. ELEMENTS EXTRA hashIssuances (32-byte hash)
 //! 4. outpoint (32-byte hash + 4-byte little endian)
 //! 5. scriptCode of the input (serialized as scripts inside CTxOuts)
 //! 6. value of the output spent by this input (8-byte little endian)
@@ -373,13 +373,6 @@ mod tests {
                 .to_string(),
             "ert1qamjdykcfzkcsvc9z32a6qcz3mwr85a3k7z7qf2uaufem2q3lsjxqj4y4fy"
         );
-
-        println!(
-            "{}",
-            desc.address(&elements::AddressParams::ELEMENTS)
-                .unwrap()
-                .to_string()
-        );
     }
     #[test]
     fn spend_tx() {

src/descriptor/csfs_cov/script_internals.rs

@@ -67,7 +67,10 @@ impl CovOperations for script::Builder {
             .push_opcode(all::OP_ENDIF)
     }
 
+    #[rustfmt::skip]
     fn verify_cov(self, key: &bitcoin::PublicKey) -> Self {
+        use elements::opcodes::all::{OP_CAT, OP_SWAP};
+
         let mut builder = self;
         // The miniscript is of type B, which should have pushed 1
         // onto the stack if it satisfied correctly.(which it should)
@@ -100,25 +103,22 @@ impl CovOperations for script::Builder {
         builder = builder.push_opcode(all::OP_TOALTSTACK);
         // alt_stk = [bitcoinsig]
         // stk = [ecsig i10 i9 i8 i7 i6 i5 i4 i3b i3 i2 i1]
-        // Ignore fmt skip because it butchers these lines
-        #[cfg_attr(feature="cargo-fmt", rustfmt_skip)]
-        {
-            // Do the size checks on all respective items in sighash calculation
-            use elements::opcodes::all::{OP_CAT, OP_SWAP};
-            builder = builder.chk_size(4).push_opcode(OP_SWAP); // item 1: ver
-            builder = builder.chk_size(32).push_opcode(OP_CAT).push_opcode(OP_SWAP);//item 2: hashprevouts
-            builder = builder.chk_size(32).push_opcode(OP_CAT).push_opcode(OP_SWAP);//item 3: hashsequence
-            builder = builder.chk_size(32).push_opcode(OP_CAT).push_opcode(OP_SWAP);//item 3b: hashissuances
-            builder = builder.chk_size(36).push_opcode(OP_CAT).push_opcode(OP_SWAP);//item 4: outpoint
-            // Item 5: Script code is of constant size because we only consider everything after
-            // codeseparator. This will be replaced with a push slice in a later commit
-            builder = builder.chk_size(3).push_opcode(OP_CAT).push_opcode(OP_SWAP);//item 5: script code
-            builder = builder.chk_amt().push_opcode(OP_CAT).push_opcode(OP_SWAP);       //item 6: check confAmt
-            builder = builder.chk_size(4).push_opcode(OP_CAT).push_opcode(OP_SWAP); //item 7: sequence
-            builder = builder.chk_size(32).push_opcode(OP_CAT).push_opcode(OP_SWAP);//item 8: hashoutputs
-            builder = builder.chk_size(4).push_opcode(OP_CAT).push_opcode(OP_SWAP); //item 9: nlocktime
-            builder = builder.chk_size(4).push_opcode(OP_CAT);                           //item 10: sighash type
-        }
+
+        // Do the size checks on all respective items in sighash calculation
+        builder = builder.chk_size(4).push_opcode(OP_SWAP); // item 1: ver
+        builder = builder.chk_size(32).push_opcode(OP_CAT).push_opcode(OP_SWAP);//item 2: hashprevouts
+        builder = builder.chk_size(32).push_opcode(OP_CAT).push_opcode(OP_SWAP);//item 3: hashsequence
+        builder = builder.chk_size(32).push_opcode(OP_CAT).push_opcode(OP_SWAP);//item 3b: hashissuances
+        builder = builder.chk_size(36).push_opcode(OP_CAT).push_opcode(OP_SWAP);//item 4: outpoint
+        // Item 5: Script code is of constant size because we only consider everything after
+        // codeseparator. This will be replaced with a push slice in a later commit
+        builder = builder.chk_size(3).push_opcode(OP_CAT).push_opcode(OP_SWAP); //item 5: script code
+        builder = builder.chk_amt().push_opcode(OP_CAT).push_opcode(OP_SWAP);   //item 6: check confAmt
+        builder = builder.chk_size(4).push_opcode(OP_CAT).push_opcode(OP_SWAP); //item 7: sequence
+        builder = builder.chk_size(32).push_opcode(OP_CAT).push_opcode(OP_SWAP);//item 8: hashoutputs
+        builder = builder.chk_size(4).push_opcode(OP_CAT).push_opcode(OP_SWAP); //item 9: nlocktime
+        builder = builder.chk_size(4).push_opcode(OP_CAT);                      //item 10: sighash type
+
         // Now sighash is on the top of the stack
         // alt_stk = [bitcoinsig]
         // stk = [ecsig (i1||i2||i3||i3b||i4||i5||i6||i7||i8||i9||i10)]

src/descriptor/key.rs

@@ -874,8 +874,7 @@ fn parse_xkey_deriv<K: InnerXKey>(
         // step all the vectors of indexes contain a single element. If it did though, one of the
         // vectors contains more than one element.
         // Now transform this list of vectors of steps into distinct derivation paths.
-        .fold(Ok(Vec::new()), |paths, index_list| {
-            let mut paths = paths?;
+        .try_fold(Vec::new(), |mut paths, index_list| {
             let mut index_list = index_list?.into_iter();
             let first_index = index_list
                 .next()
@@ -948,7 +947,7 @@ impl<K: InnerXKey> DescriptorXKey<K> {
             Some((fingerprint, ref path)) => (
                 fingerprint,
                 path.into_iter()
-                    .chain(self.derivation_path.into_iter())
+                    .chain(&self.derivation_path)
                     .collect(),
             ),
             None => (

src/descriptor/mod.rs

@@ -1016,7 +1016,6 @@ impl<Ext: Extension + ParseableExt> Descriptor<DescriptorPublicKey, Ext> {
     ///
     /// For multipath descriptors it will return as many descriptors as there is
     /// "parallel" paths. For regular descriptors it will just return itself.
-    #[allow(clippy::blocks_in_if_conditions)]
     pub fn into_single_descriptors(self) -> Result<Vec<Self>, Error> {
         // All single-path descriptors contained in this descriptor.
         let mut descriptors = Vec::new();

src/descriptor/pegin/dynafed_pegin.rs

@@ -297,7 +297,7 @@ where
     C: secp256k1_zkp::Verification,
 {
     fn pk(&mut self, pk: &Pk) -> Result<bitcoin::PublicKey, ()> {
-        Ok(tweak_key(&pk.to_public_key(), self.1, &self.0[..]))
+        Ok(tweak_key(&pk.to_public_key(), self.1, self.0))
     }
 
     // We don't need to implement these methods as we are not using them in the policy.

src/descriptor/tr.rs

@@ -87,11 +87,7 @@ impl<Pk: MiniscriptKey, Ext: Extension> Eq for Tr<Pk, Ext> {}
 
 impl<Pk: MiniscriptKey, Ext: Extension> PartialOrd for Tr<Pk, Ext> {
     fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> {
-        match self.internal_key.partial_cmp(&other.internal_key) {
-            Some(cmp::Ordering::Equal) => {}
-            ord => return ord,
-        }
-        self.tree.partial_cmp(&other.tree)
+        Some(self.cmp(other))
     }
 }
 
@@ -977,7 +973,7 @@ mod tests {
         assert!(!tr.for_each_key(|k| k.starts_with("acc")));
     }
 
-    fn verify_from_str<'a>(desc_str: &str, internal_key: &str, scripts: &[TapLeafScript<'a, String, NoExt>]) {
+    fn verify_from_str(desc_str: &str, internal_key: &str, scripts: &[TapLeafScript<String, NoExt>]) {
         let desc = Tr::<String, NoExt>::from_str(desc_str).unwrap();
         assert_eq!(desc_str, &desc.to_string());
         assert_eq!(internal_key, &desc.internal_key);

src/extensions/param.rs

@@ -132,9 +132,7 @@ impl CovExtArgs {
 
 impl PartialOrd for CovExtArgs {
     fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
-        // HACKY implementation, need Ord/PartialOrd to make it work with other components
-        // in the library
-        self.to_string().partial_cmp(&other.to_string())
+        Some(self.cmp(other))
     }
 }
 

src/lib.rs

@@ -125,8 +125,6 @@ mod macros;
 #[macro_use]
 mod pub_macros;
 
-pub use pub_macros::*;
-
 pub mod confidential;
 pub mod descriptor;
 pub mod expression;

src/miniscript/context.rs

@@ -757,7 +757,7 @@ impl ScriptContext for BareCtx {
         }
         match ms.node {
             Terminal::PkK(ref key) if key.is_x_only_key() => {
-                return Err(ScriptContextError::XOnlyKeysNotAllowed(
+                Err(ScriptContextError::XOnlyKeysNotAllowed(
                     key.to_string(),
                     Self::name_str(),
                 ))

src/miniscript/ms_tests.rs

@@ -68,7 +68,7 @@ mod tests {
     }
 
     #[test]
-    #[cfg_attr(feature="cargo-fmt", rustfmt_skip)]
+    #[rustfmt::skip]
     fn invalid_tests_from_alloy() {
         invalid_ms("or_b(or_i(0,sha256(926a54995ca48600920a19bf7bc502ca5f2f7d07e6f804c4f00ebf0325084dbc)),after(1))");
         invalid_ms("or_b(s:pk_h(A),after(500000001))");
@@ -5646,7 +5646,7 @@ mod tests {
         invalid_ms("c:or_b(sha256(926a54995ca48600920a19bf7bc502ca5f2f7d07e6f804c4f00ebf0325084dbc),pk_k(A))");
     }
     #[test]
-    #[cfg_attr(feature="cargo-fmt", rustfmt_skip)]
+    #[rustfmt::skip]
     fn mall_8f1e8_tests_from_alloy() {
         ms_test("or_d(or_d(sha256(926a54995ca48600920a19bf7bc502ca5f2f7d07e6f804c4f00ebf0325084dbc),sha256(926a54995ca48600920a19bf7bc502ca5f2f7d07e6f804c4f00ebf0325084dbc)),after(500000001))", "Bf");
         ms_test("andor(sha256(926a54995ca48600920a19bf7bc502ca5f2f7d07e6f804c4f00ebf0325084dbc),or_d(multi(2,A,B,C),sha256(926a54995ca48600920a19bf7bc502ca5f2f7d07e6f804c4f00ebf0325084dbc)),after(1))", "B");
@@ -9143,7 +9143,7 @@ mod tests {
 
     }
     #[test]
-    #[cfg_attr(feature="cargo-fmt", rustfmt_skip)]
+    #[rustfmt::skip]
     fn main_tests_from_alloy() {
         ms_test("or_d(or_d(multi(2,A,B,C),or_d(multi(2,D,E,F),multi(2,G,I,J))),multi(2,K,L,M))", "Bdusem");
         ms_test("andor(multi(2,A,B,C),or_d(multi(2,D,E,F),sha256(926a54995ca48600920a19bf7bc502ca5f2f7d07e6f804c4f00ebf0325084dbc)),c:pk_h(G))", "Bdusem");
@@ -15044,7 +15044,7 @@ mod tests {
 }
 
     #[test]
-    #[cfg_attr(feature="cargo-fmt", rustfmt_skip)]
+    #[rustfmt::skip]
     fn malleable_tests_from_alloy() {
         ms_test("and_v(v:after(500000001),or_d(j:multi(2,A,B,C),multi(2,D,E,F)))", "usB");
         ms_test("or_b(j:multi(2,A,B,C),a:andor(multi(2,D,E,F),multi(2,G,I,J),multi(2,K,L,M)))", "dBesu");
@@ -22076,8 +22076,8 @@ mod tests {
     // This does not actually test timelock mixing. See: https://github.com/rust-bitcoin/rust-miniscript/issues/514
     // for details
     #[test]
+    #[rustfmt::skip]
     fn conflict_tests_from_alloy() {
-        #[cfg_attr(feature="cargo-fmt", rustfmt_skip)]
         {
             ms_test("andor(multi(2,A,B,C),andor(multi(2,D,E,F),after(500000001),n:after(1)),0)","Bedsm");
             ms_test("and_v(v:after(500000001),or_d(multi(2,A,B,C),and_b(multi(2,D,E,F),a:after(1))))","Busm");

src/miniscript/satisfy.rs

@@ -9,7 +9,7 @@
 
 use std::collections::{BTreeMap, HashMap};
 use std::sync::Arc;
-use std::{cmp, i64, mem};
+use std::{cmp, mem};
 
 use bitcoin::hashes::hash160;
 use bitcoin::secp256k1::XOnlyPublicKey;

src/miniscript/types/extra_props.rs

@@ -51,7 +51,7 @@ impl OpLimits {
 
     /// Worst case opcode count when this element is satisfied
     pub fn op_count(&self) -> Option<usize> {
-        opt_add(Some(self.count), self.sat)
+        self.sat.map(|sat| self.count + sat)
     }
 }
 
@@ -821,11 +821,11 @@ impl Property for ExtData {
                 .iter()
                 .rev()
                 .enumerate()
-                .fold(Some(0), |acc, (i, &(x, y))| {
+                .try_fold(0, |acc, (i, &(x, y))| {
                     if i <= k {
-                        opt_add(acc, x)
+                        x.map(|x| x + acc)
                     } else {
-                        opt_add(acc, y)
+                        y.map(|y| y + acc)
                     }
                 });
 
@@ -834,11 +834,11 @@ impl Property for ExtData {
             .iter()
             .rev()
             .enumerate()
-            .fold(Some(0), |acc, (i, &(x, y))| {
+            .try_fold(0, |acc, (i, &(x, y))| {
                 if i <= k {
-                    opt_max(acc, x)
+                    x.map(|x| cmp::max(x, acc))
                 } else {
-                    opt_max(acc, y)
+                    y.map(|y| cmp::max(y, acc))
                 }
             });
 
@@ -848,11 +848,11 @@ impl Property for ExtData {
             max_sat_size_vec
                 .iter()
                 .enumerate()
-                .fold(Some((0, 0)), |acc, (i, &(x, y))| {
+                .try_fold((0, 0), |acc, (i, &(x, y))| {
                     if i <= k {
-                        opt_tuple_add(acc, x)
+                        x.map(|x| (acc.0 + x.0, acc.1 + x.1))
                     } else {
-                        opt_tuple_add(acc, y)
+                        y.map(|y| (acc.0 + y.0, acc.1 + y.1))
                     }
                 });
 
@@ -861,11 +861,11 @@ impl Property for ExtData {
             ops_count_sat_vec
                 .iter()
                 .enumerate()
-                .fold(Some(0), |acc, (i, &(x, y))| {
+                .try_fold(0, |acc, (i, &(x, y))| {
                     if i <= k {
-                        opt_add(acc, x)
+                        x.map(|x| x + acc)
                     } else {
-                        opt_add(acc, Some(y))
+                        Some(y + acc)
                     }
                 });
 
@@ -1083,11 +1083,6 @@ fn opt_add(a: Option<usize>, b: Option<usize>) -> Option<usize> {
     a.and_then(|x| b.map(|y| x + y))
 }
 
-/// Returns Some((x0+y0, x1+y1)) is both x and y are Some. Otherwise, returns `None`.
-fn opt_tuple_add(a: Option<(usize, usize)>, b: Option<(usize, usize)>) -> Option<(usize, usize)> {
-    a.and_then(|x| b.map(|(w, s)| (w + x.0, s + x.1)))
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

src/policy/compiler.rs

@@ -34,7 +34,7 @@ impl Eq for OrdF64 {}
 // to derive both or neither. Better to be explicit.
 impl PartialOrd for OrdF64 {
     fn partial_cmp(&self, other: &OrdF64) -> Option<cmp::Ordering> {
-        self.0.partial_cmp(&other.0)
+        Some(self.cmp(other))
     }
 }
 impl Ord for OrdF64 {