Merge #569: Update MSRV to 1.48. Fixes CI

6b76997 Use a static map to lookup which characters are allowed (sanket1729)
60fde9e Check that we only accept INPUT_CHARSET in from_str (sanket1729)
a396605 Fix taproot internal key parsing (sanket1729)
68c7c49 clippy warnings (sanket1729)
b238366 Update MSRV to 1.48 (sanket1729)

Pull request description:

ACKs for top commit:
  apoelstra:
    ACK 6b76997

Tree-SHA512: 5b42b3a1d8156c5a7aa5924bfbc5d79f8196173b4f9a2b502480edbfa4d2f9e2cd74dd34839de76e12a74ad0ccfc335f816890db1b4b6705bdbb2e7bc81fcebc

Author: apoelstra

.github/workflows/fuzz.yml

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

.github/workflows/rust.yml

@@ -54,8 +54,7 @@ jobs:
           - rust: stable
           - rust: beta
           - rust: nightly
-          - rust: 1.41.1
-          - rust: 1.47
+          - rust: 1.48
     steps:
       - name: Checkout Crate
         uses: actions/checkout@v2

README.md

@@ -1,6 +1,6 @@
 ![Build](https://github.com/rust-bitcoin/rust-miniscript/workflows/Continuous%20integration/badge.svg)
 
-**Minimum Supported Rust Version:** 1.41.1
+**Minimum Supported Rust Version:** 1.48.0
 
 # Miniscript
 
@@ -40,8 +40,7 @@ The cargo feature `std` is enabled by default. At least one of the features `std
 Enabling the `no-std` feature does not disable `std`. To disable the `std` feature you must disable default features. The `no-std` feature only enables additional features required for this crate to be usable without `std`. Both can be enabled without conflict.
 
 ## Minimum Supported Rust Version (MSRV)
-This library should always compile with any combination of features (minus
-`no-std`) on **Rust 1.41.1** or **Rust 1.47** with `no-std`.
+This library should always compile with any combination of features on **Rust 1.48.0**.
 
 Some dependencies do not play nicely with our MSRV, if you are running the tests
 you may need to pin some dependencies. See `./contrib/test.sh` for current pinning.

bitcoind-tests/Cargo.toml

@@ -9,7 +9,7 @@ publish = false
 
 [dependencies]
 miniscript = {path = "../"}
-bitcoind = { version = "0.30.0" }
+bitcoind = { version = "0.32.0" }
 actual-rand = { package = "rand", version = "0.8.4"}
 secp256k1 = {version = "0.27.0", features = ["rand-std"]}
 internals = { package = "bitcoin-private", version = "0.1.0", default_features = false }

clippy.toml

@@ -1 +1 @@
-msrv = "1.41.1"
+msrv = "1.48.0"

contrib/test.sh

@@ -14,18 +14,14 @@ then
     cargo fmt -- --check
 fi
 
-# Pin dependencies required to build with Rust 1.41.1
-if cargo --version | grep "1\.41\.0"; then
+# Pin dependencies required to build with Rust 1.48.0
+if cargo --version | grep "1\.48\.0"; then
     cargo update -p once_cell --precise 1.13.1
+    cargo update -p quote --precise 1.0.28
+    cargo update -p proc-macro2 --precise 1.0.63
     cargo update -p serde_json --precise 1.0.99
-    cargo update -p serde --precise 1.0.156
-fi
-
-# Pin dependencies required to build with Rust 1.47.0
-if cargo --version | grep "1\.47\.0"; then
-    cargo update -p once_cell --precise 1.13.1
-    cargo update -p serde_json --precise 1.0.99
-    cargo update -p serde --precise 1.0.156
+    cargo update -p serde --precise 1.0.152
+    cargo update -p log --precise 0.4.18
 fi
 
 # Test bitcoind integration tests if told to (this only works with the stable toolchain)

fuzz/Cargo.toml

@@ -12,7 +12,7 @@ cargo-fuzz = true
 honggfuzz = { version = "0.5.55", default-features = false }
 miniscript = { path = "..", features = [ "compiler" ] }
 
-regex = "1.4"
+regex = "1.0"
 
 [[bin]]
 name = "roundtrip_miniscript_str"

fuzz/fuzz_targets/roundtrip_descriptor.rs

@@ -23,9 +23,27 @@ fn main() {
 
 #[cfg(test)]
 mod tests {
-    use super::*;
+    fn extend_vec_from_hex(hex: &str, out: &mut Vec<u8>) {
+        let mut b = 0;
+        for (idx, c) in hex.as_bytes().iter().enumerate() {
+            b <<= 4;
+            match *c {
+                b'A'..=b'F' => b |= c - b'A' + 10,
+                b'a'..=b'f' => b |= c - b'a' + 10,
+                b'0'..=b'9' => b |= c - b'0',
+                _ => panic!("Bad hex"),
+            }
+            if (idx & 1) == 1 {
+                out.push(b);
+                b = 0;
+            }
+        }
+    }
+
     #[test]
-    fn test() {
-        do_test(b"pkh()");
+    fn duplicate_crash3() {
+        let mut a = Vec::new();
+        extend_vec_from_hex("747228726970656d616e645f6e5b5c79647228726970656d616e645f6e5b5c7964646464646464646464646464646464646464646464646464646b5f6872702c29", &mut a);
+        super::do_test(&a);
     }
 }

src/descriptor/bare.rs

@@ -192,7 +192,7 @@ where
     where
         T: Translator<P, Q, E>,
     {
-        Ok(Bare::new(self.ms.translate_pk(t)?).map_err(TranslateErr::OuterError)?)
+        Bare::new(self.ms.translate_pk(t)?).map_err(TranslateErr::OuterError)
     }
 }
 

src/descriptor/checksum.rs

@@ -8,10 +8,10 @@
 use core::fmt;
 use core::iter::FromIterator;
 
+pub use crate::expression::VALID_CHARS;
 use crate::prelude::*;
 use crate::Error;
 
-const INPUT_CHARSET: &str =  "0123456789()[],'/*abcdefgh@:$%{}IJKLMNOPQRSTUVWXYZ&+-.;<=>?!^_|~ijklmnopqrstuvwxyzABCDEFGH`#\"\\ ";
 const CHECKSUM_CHARSET: &[u8] = b"qpzry9x8gf2tvdw0s3jn54khce6mua7l";
 
 fn poly_mod(mut c: u64, val: u64) -> u64 {
@@ -101,9 +101,14 @@ impl Engine {
     /// state! It is safe to continue feeding it data but the result will not be meaningful.
     pub fn input(&mut self, s: &str) -> Result<(), Error> {
         for ch in s.chars() {
-            let pos = INPUT_CHARSET.find(ch).ok_or_else(|| {
-                Error::BadDescriptor(format!("Invalid character in checksum: '{}'", ch))
-            })? as u64;
+            let pos = VALID_CHARS
+                .get(ch as usize)
+                .ok_or_else(|| {
+                    Error::BadDescriptor(format!("Invalid character in checksum: '{}'", ch))
+                })?
+                .ok_or_else(|| {
+                    Error::BadDescriptor(format!("Invalid character in checksum: '{}'", ch))
+                })? as u64;
             self.c = poly_mod(self.c, pos & 31);
             self.cls = self.cls * 3 + (pos >> 5);
             self.clscount += 1;

src/descriptor/key.rs

@@ -999,13 +999,13 @@ impl MiniscriptKey for DescriptorPublicKey {
     }
 
     fn is_x_only_key(&self) -> bool {
-        match self {
+        matches!(
+            self,
             DescriptorPublicKey::Single(SinglePub {
                 key: SinglePubKey::XOnly(ref _key),
                 ..
-            }) => true,
-            _ => false,
-        }
+            })
+        )
     }
 
     fn num_der_paths(&self) -> usize {

src/descriptor/sh.rs

@@ -348,13 +348,13 @@ impl<Pk: MiniscriptKey + ToPublicKey> Sh<Pk> {
                 let witness_script = wsh.inner_script().to_v0_p2wsh();
                 let push_bytes = <&PushBytes>::try_from(witness_script.as_bytes())
                     .expect("Witness script is not too large");
-                script::Builder::new().push_slice(&push_bytes).into_script()
+                script::Builder::new().push_slice(push_bytes).into_script()
             }
             ShInner::Wpkh(ref wpkh) => {
                 let redeem_script = wpkh.script_pubkey();
                 let push_bytes: &PushBytes =
                     <&PushBytes>::try_from(redeem_script.as_bytes()).expect("Script not too large");
-                script::Builder::new().push_slice(&push_bytes).into_script()
+                script::Builder::new().push_slice(push_bytes).into_script()
             }
             ShInner::SortedMulti(..) | ShInner::Ms(..) => ScriptBuf::new(),
         }

src/descriptor/tr.rs

@@ -350,7 +350,7 @@ impl<Pk: MiniscriptKey + ToPublicKey> Tr<Pk> {
         let builder = bitcoin::blockdata::script::Builder::new();
         builder
             .push_opcode(opcodes::all::OP_PUSHNUM_1)
-            .push_slice(&output_key.serialize())
+            .push_slice(output_key.serialize())
             .into_script()
     }
 
@@ -405,8 +405,7 @@ where
     type Item = (u8, &'a Miniscript<Pk, Tap>);
 
     fn next(&mut self) -> Option<Self::Item> {
-        while !self.stack.is_empty() {
-            let (depth, last) = self.stack.pop().expect("Size checked above");
+        while let Some((depth, last)) = self.stack.pop() {
             match *last {
                 TapTree::Tree(ref l, ref r) => {
                     self.stack.push((depth + 1, r));
@@ -519,17 +518,19 @@ impl<Pk: MiniscriptKey> fmt::Display for Tr<Pk> {
 
 // Helper function to parse string into miniscript tree form
 fn parse_tr_tree(s: &str) -> Result<expression::Tree, Error> {
-    for ch in s.bytes() {
-        if !ch.is_ascii() {
-            return Err(Error::Unprintable(ch));
-        }
-    }
+    expression::check_valid_chars(s)?;
 
     if s.len() > 3 && &s[..3] == "tr(" && s.as_bytes()[s.len() - 1] == b')' {
         let rest = &s[3..s.len() - 1];
         if !rest.contains(',') {
+            let key = expression::Tree::from_str(rest)?;
+            if !key.args.is_empty() {
+                return Err(Error::Unexpected(
+                    "invalid taproot internal key".to_string(),
+                ));
+            }
             let internal_key = expression::Tree {
-                name: rest,
+                name: key.name,
                 args: vec![],
             };
             return Ok(expression::Tree {
@@ -541,8 +542,14 @@ fn parse_tr_tree(s: &str) -> Result<expression::Tree, Error> {
         let (key, script) = split_once(rest, ',')
             .ok_or_else(|| Error::BadDescriptor("invalid taproot descriptor".to_string()))?;
 
+        let key = expression::Tree::from_str(key)?;
+        if !key.args.is_empty() {
+            return Err(Error::Unexpected(
+                "invalid taproot internal key".to_string(),
+            ));
+        }
         let internal_key = expression::Tree {
-            name: key,
+            name: key.name,
             args: vec![],
         };
         if script.is_empty() {
@@ -569,19 +576,13 @@ fn split_once(inp: &str, delim: char) -> Option<(&str, &str)> {
     if inp.is_empty() {
         None
     } else {
-        let mut found = inp.len();
-        for (idx, ch) in inp.chars().enumerate() {
-            if ch == delim {
-                found = idx;
-                break;
-            }
-        }
-        // No comma or trailing comma found
-        if found >= inp.len() - 1 {
-            Some((inp, ""))
-        } else {
-            Some((&inp[..found], &inp[found + 1..]))
-        }
+        // find the first character that matches delim
+        let res = inp
+            .chars()
+            .position(|ch| ch == delim)
+            .map(|idx| (&inp[..idx], &inp[idx + 1..]))
+            .unwrap_or((inp, ""));
+        Some(res)
     }
 }
 

src/expression.rs

@@ -8,6 +8,29 @@ use core::str::FromStr;
 use crate::prelude::*;
 use crate::{errstr, Error, MAX_RECURSION_DEPTH};
 
+/// Allowed characters are descriptor strings.
+pub const INPUT_CHARSET: &str =  "0123456789()[],'/*abcdefgh@:$%{}IJKLMNOPQRSTUVWXYZ&+-.;<=>?!^_|~ijklmnopqrstuvwxyzABCDEFGH`#\"\\ ";
+
+/// Map of valid characters in descriptor strings.
+#[rustfmt::skip]
+pub const VALID_CHARS: [Option<u8>; 128] = [
+    None, None, None, None, None, None, None, None, None, None, None, None, None,
+    None, None, None, None, None, None, None, None, None, None, None, None, None,
+    None, None, None, None, None, None, Some(94), Some(59), Some(92), Some(91),
+    Some(28), Some(29), Some(50), Some(15), Some(10), Some(11), Some(17), Some(51),
+    Some(14), Some(52), Some(53), Some(16), Some(0), Some(1), Some(2), Some(3),
+    Some(4), Some(5), Some(6), Some(7), Some(8), Some(9), Some(27), Some(54),
+    Some(55), Some(56), Some(57), Some(58), Some(26), Some(82), Some(83),
+    Some(84), Some(85), Some(86), Some(87), Some(88), Some(89), Some(32), Some(33),
+    Some(34), Some(35), Some(36), Some(37), Some(38), Some(39), Some(40), Some(41),
+    Some(42), Some(43), Some(44), Some(45), Some(46), Some(47), Some(48), Some(49),
+    Some(12), Some(93), Some(13), Some(60), Some(61), Some(90), Some(18), Some(19),
+    Some(20), Some(21), Some(22), Some(23), Some(24), Some(25), Some(64), Some(65),
+    Some(66), Some(67), Some(68), Some(69), Some(70), Some(71), Some(72), Some(73),
+    Some(74), Some(75), Some(76), Some(77), Some(78), Some(79), Some(80), Some(81),
+    Some(30), Some(62), Some(31), Some(63), None,
+];
+
 #[derive(Debug)]
 /// A token of the form `x(...)` or `x`
 pub struct Tree<'a> {
@@ -166,13 +189,7 @@ impl<'a> Tree<'a> {
     /// Parses a tree from a string
     #[allow(clippy::should_implement_trait)] // Cannot use std::str::FromStr because of lifetimes.
     pub fn from_str(s: &'a str) -> Result<Tree<'a>, Error> {
-        // Filter out non-ASCII because we byte-index strings all over the
-        // place and Rust gets very upset when you splinch a string.
-        for ch in s.bytes() {
-            if !ch.is_ascii() {
-                return Err(Error::Unprintable(ch));
-            }
-        }
+        check_valid_chars(s)?;
 
         let (top, rem) = Tree::from_slice(s)?;
         if rem.is_empty() {
@@ -183,6 +200,23 @@ impl<'a> Tree<'a> {
     }
 }
 
+/// Filter out non-ASCII because we byte-index strings all over the
+/// place and Rust gets very upset when you splinch a string.
+pub fn check_valid_chars(s: &str) -> Result<(), Error> {
+    for ch in s.bytes() {
+        if !ch.is_ascii() {
+            return Err(Error::Unprintable(ch));
+        }
+        // Index bounds: We know that ch is ASCII, so it is <= 127.
+        if VALID_CHARS[ch as usize].is_none() {
+            return Err(Error::Unexpected(
+                "Only characters in INPUT_CHARSET are allowed".to_string(),
+            ));
+        }
+    }
+    Ok(())
+}
+
 /// Parse a string as a u32, for timelocks or thresholds
 pub fn parse_num(s: &str) -> Result<u32, Error> {
     if s.len() > 1 {
@@ -253,4 +287,13 @@ mod tests {
         assert!(parse_num("+6").is_err());
         assert!(parse_num("-6").is_err());
     }
+
+    #[test]
+    fn test_valid_char_map() {
+        let mut valid_chars = [None; 128];
+        for (i, ch) in super::INPUT_CHARSET.chars().enumerate() {
+            valid_chars[ch as usize] = Some(i as u8);
+        }
+        assert_eq!(valid_chars, super::VALID_CHARS);
+    }
 }

src/interpreter/inner.rs

@@ -43,7 +43,7 @@ fn script_from_stack_elem<Ctx: ScriptContext>(
 ) -> Result<Miniscript<Ctx::Key, Ctx>, Error> {
     match *elem {
         stack::Element::Push(sl) => {
-            Miniscript::parse_with_ext(&bitcoin::Script::from_bytes(sl), &ExtParams::allow_all())
+            Miniscript::parse_with_ext(bitcoin::Script::from_bytes(sl), &ExtParams::allow_all())
                 .map_err(Error::from)
         }
         stack::Element::Satisfied => {

src/iter/tree.rs

@@ -220,7 +220,7 @@ impl<T: TreeLike> Iterator for PreOrderIter<T> {
                 self.stack.push(left);
             }
             Tree::Nary(children) => {
-                self.stack.extend(children.into_iter().rev().cloned());
+                self.stack.extend(children.iter().rev().cloned());
             }
         }
         Some(top)

src/miniscript/analyzable.rs

@@ -221,10 +221,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
 
     /// Whether the given miniscript contains a raw pkh fragment
     pub fn contains_raw_pkh(&self) -> bool {
-        self.iter().any(|ms| match ms.node {
-            Terminal::RawPkH(_) => true,
-            _ => false,
-        })
+        self.iter().any(|ms| matches!(ms.node, Terminal::RawPkH(_)))
     }
 
     /// Check whether the underlying Miniscript is safe under the current context

src/miniscript/astelem.rs

@@ -465,7 +465,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
             Terminal::RawPkH(ref hash) => builder
                 .push_opcode(opcodes::all::OP_DUP)
                 .push_opcode(opcodes::all::OP_HASH160)
-                .push_slice(&hash.to_byte_array())
+                .push_slice(hash.to_byte_array())
                 .push_opcode(opcodes::all::OP_EQUALVERIFY),
             Terminal::After(t) => builder
                 .push_int(absolute::LockTime::from(t).to_consensus_u32() as i64)