expression: replace recursive Tree datatype with a non-recursive one

.github/workflows/cron-daily-fuzz.yml

@@ -22,6 +22,7 @@ compile_descriptor,
 compile_taproot,
 parse_descriptor,
 parse_descriptor_secret,
+regression_descriptor_parse,
 roundtrip_concrete,
 roundtrip_descriptor,
 roundtrip_miniscript_script,

Cargo-minimal.lock

@@ -111,7 +111,8 @@ name = "descriptor-fuzz"
 version = "0.0.1"
 dependencies = [
  "honggfuzz",
- "miniscript",
+ "miniscript 12.3.0",
+ "miniscript 13.0.0",
  "regex",
 ]
 
@@ -181,7 +182,17 @@ dependencies = [
 
 [[package]]
 name = "miniscript"
-version = "12.2.0"
+version = "12.3.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "5bd3c9608217b0d6fa9c9c8ddd875b85ab72bd4311cfc8db35e1b5a08fc11f4d"
+dependencies = [
+ "bech32",
+ "bitcoin",
+]
+
+[[package]]
+name = "miniscript"
+version = "13.0.0"
 dependencies = [
  "bech32",
  "bitcoin",

Cargo-recent.lock

@@ -111,7 +111,8 @@ name = "descriptor-fuzz"
 version = "0.0.1"
 dependencies = [
  "honggfuzz",
- "miniscript",
+ "miniscript 12.3.0",
+ "miniscript 13.0.0",
  "regex",
 ]
 
@@ -181,7 +182,17 @@ dependencies = [
 
 [[package]]
 name = "miniscript"
-version = "12.2.0"
+version = "12.3.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "5bd3c9608217b0d6fa9c9c8ddd875b85ab72bd4311cfc8db35e1b5a08fc11f4d"
+dependencies = [
+ "bech32",
+ "bitcoin",
+]
+
+[[package]]
+name = "miniscript"
+version = "13.0.0"
 dependencies = [
  "bech32",
  "bitcoin",

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "miniscript"
-version = "12.2.0"
+version = "13.0.0"
 authors = ["Andrew Poelstra <apoelstra@wpsoftware.net>, Sanket Kanjalkar <sanket1729@gmail.com>"]
 license = "CC0-1.0"
 homepage = "https://github.com/rust-bitcoin/rust-miniscript/"

fuzz/Cargo.toml

@@ -11,7 +11,10 @@ cargo-fuzz = true
 
 [dependencies]
 honggfuzz = { version = "0.5.56", default-features = false }
-miniscript = { path = "..", features = [ "compiler" ] }
+# We shouldn't need an explicit version on the next line, but Andrew's tools
+# choke on it otherwise. See https://github.com/nix-community/crate2nix/issues/373
+miniscript = { path = "..", features = [ "compiler" ], version = "13.0" }
+old_miniscript = { package = "miniscript", version = "12.3" }
 
 regex = "1.0"
 
@@ -31,6 +34,10 @@ path = "fuzz_targets/parse_descriptor.rs"
 name = "parse_descriptor_secret"
 path = "fuzz_targets/parse_descriptor_secret.rs"
 
+[[bin]]
+name = "regression_descriptor_parse"
+path = "fuzz_targets/regression_descriptor_parse.rs"
+
 [[bin]]
 name = "roundtrip_concrete"
 path = "fuzz_targets/roundtrip_concrete.rs"

fuzz/fuzz_targets/regression_descriptor_parse.rs

@@ -0,0 +1,43 @@
+use core::str::FromStr;
+
+use honggfuzz::fuzz;
+use miniscript::{Descriptor, DescriptorPublicKey};
+use old_miniscript::{Descriptor as OldDescriptor, DescriptorPublicKey as OldDescriptorPublicKey};
+
+type Desc = Descriptor<DescriptorPublicKey>;
+type OldDesc = OldDescriptor<OldDescriptorPublicKey>;
+
+fn do_test(data: &[u8]) {
+    let data_str = String::from_utf8_lossy(data);
+    match (Desc::from_str(&data_str), OldDesc::from_str(&data_str)) {
+        (Err(_), Err(_)) => {}
+        (Ok(x), Err(e)) => panic!("new logic parses {} as {:?}, old fails with {}", data_str, x, e),
+        (Err(e), Ok(x)) => panic!("old logic parses {} as {:?}, new fails with {}", data_str, x, e),
+        (Ok(new), Ok(old)) => {
+            assert_eq!(
+                old.to_string(),
+                new.to_string(),
+                "input {} (left is old, right is new)",
+                data_str
+            )
+        }
+    }
+}
+
+fn main() {
+    loop {
+        fuzz!(|data| {
+            do_test(data);
+        });
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    #[test]
+    fn duplicate_crash() {
+        crate::do_test(
+            b"tr(02dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd,{1,unun:0})",
+        )
+    }
+}

fuzz/generate-files.sh

@@ -24,6 +24,7 @@ cargo-fuzz = true
 [dependencies]
 honggfuzz = { version = "0.5.56", default-features = false }
 miniscript = { path = "..", features = [ "compiler" ] }
+old_miniscript = { package = "miniscript", git = "https://github.com/apoelstra/rust-miniscript/", rev = "1259375d7b7c91053e09d1cbe3db983612fe301c" }
 
 regex = "1.0"
 EOF

src/descriptor/bare.rs

@@ -175,8 +175,8 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Bare<Pk> {
 }
 
 impl<Pk: FromStrKey> FromTree for Bare<Pk> {
-    fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
-        let sub = Miniscript::<Pk, BareCtx>::from_tree(top)?;
+    fn from_tree(root: expression::TreeIterItem) -> Result<Self, Error> {
+        let sub = Miniscript::<Pk, BareCtx>::from_tree(root)?;
         BareCtx::top_level_checks(&sub)?;
         Bare::new(sub)
     }
@@ -186,7 +186,7 @@ impl<Pk: FromStrKey> core::str::FromStr for Bare<Pk> {
     type Err = Error;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let top = expression::Tree::from_str(s)?;
-        Self::from_tree(&top)
+        Self::from_tree(top.root())
     }
 }
 
@@ -369,8 +369,8 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Pkh<Pk> {
 }
 
 impl<Pk: FromStrKey> FromTree for Pkh<Pk> {
-    fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
-        let pk = top
+    fn from_tree(root: expression::TreeIterItem) -> Result<Self, Error> {
+        let pk = root
             .verify_terminal_parent("pkh", "public key")
             .map_err(Error::Parse)?;
         Pkh::new(pk).map_err(Error::ContextError)
@@ -381,7 +381,7 @@ impl<Pk: FromStrKey> core::str::FromStr for Pkh<Pk> {
     type Err = Error;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let top = expression::Tree::from_str(s)?;
-        Self::from_tree(&top)
+        Self::from_tree(top.root())
     }
 }
 

src/descriptor/mod.rs

@@ -965,8 +965,8 @@ impl Descriptor<DefiniteDescriptorKey> {
 
 impl<Pk: FromStrKey> crate::expression::FromTree for Descriptor<Pk> {
     /// Parse an expression tree into a descriptor.
-    fn from_tree(top: &expression::Tree) -> Result<Descriptor<Pk>, Error> {
-        Ok(match (top.name, top.args.len() as u32) {
+    fn from_tree(top: expression::TreeIterItem) -> Result<Descriptor<Pk>, Error> {
+        Ok(match (top.name(), top.n_children()) {
             ("pkh", 1) => Descriptor::Pkh(Pkh::from_tree(top)?),
             ("wpkh", 1) => Descriptor::Wpkh(Wpkh::from_tree(top)?),
             ("sh", 1) => Descriptor::Sh(Sh::from_tree(top)?),
@@ -981,7 +981,7 @@ impl<Pk: FromStrKey> FromStr for Descriptor<Pk> {
     type Err = Error;
     fn from_str(s: &str) -> Result<Descriptor<Pk>, Error> {
         let top = expression::Tree::from_str(s)?;
-        let ret = Self::from_tree(&top)?;
+        let ret = Self::from_tree(top.root())?;
         if let Descriptor::Tr(ref inner) = ret {
             // FIXME preserve weird/broken behavior from 12.x.
             // See https://github.com/rust-bitcoin/rust-miniscript/issues/734

src/descriptor/segwitv0.rs

@@ -247,13 +247,13 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Wsh<Pk> {
 }
 
 impl<Pk: FromStrKey> crate::expression::FromTree for Wsh<Pk> {
-    fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
+    fn from_tree(top: expression::TreeIterItem) -> Result<Self, Error> {
         let top = top
             .verify_toplevel("wsh", 1..=1)
             .map_err(From::from)
             .map_err(Error::Parse)?;
 
-        if top.name == "sortedmulti" {
+        if top.name() == "sortedmulti" {
             return Ok(Wsh { inner: WshInner::SortedMulti(SortedMultiVec::from_tree(top)?) });
         }
         let sub = Miniscript::from_tree(top)?;
@@ -284,7 +284,7 @@ impl<Pk: FromStrKey> core::str::FromStr for Wsh<Pk> {
     type Err = Error;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let top = expression::Tree::from_str(s)?;
-        Wsh::<Pk>::from_tree(&top)
+        Wsh::<Pk>::from_tree(top.root())
     }
 }
 
@@ -483,7 +483,7 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Wpkh<Pk> {
 }
 
 impl<Pk: FromStrKey> crate::expression::FromTree for Wpkh<Pk> {
-    fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
+    fn from_tree(top: expression::TreeIterItem) -> Result<Self, Error> {
         let pk = top
             .verify_terminal_parent("wpkh", "public key")
             .map_err(Error::Parse)?;
@@ -495,7 +495,7 @@ impl<Pk: FromStrKey> core::str::FromStr for Wpkh<Pk> {
     type Err = Error;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let top = expression::Tree::from_str(s)?;
-        Self::from_tree(&top)
+        Self::from_tree(top.root())
     }
 }
 

src/descriptor/sh.rs

@@ -81,13 +81,13 @@ impl<Pk: MiniscriptKey> fmt::Display for Sh<Pk> {
 }
 
 impl<Pk: FromStrKey> crate::expression::FromTree for Sh<Pk> {
-    fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
+    fn from_tree(top: expression::TreeIterItem) -> Result<Self, Error> {
         let top = top
             .verify_toplevel("sh", 1..=1)
             .map_err(From::from)
             .map_err(Error::Parse)?;
 
-        let inner = match top.name {
+        let inner = match top.name() {
             "wsh" => ShInner::Wsh(Wsh::from_tree(top)?),
             "wpkh" => ShInner::Wpkh(Wpkh::from_tree(top)?),
             "sortedmulti" => ShInner::SortedMulti(SortedMultiVec::from_tree(top)?),
@@ -105,7 +105,7 @@ impl<Pk: FromStrKey> core::str::FromStr for Sh<Pk> {
     type Err = Error;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let top = expression::Tree::from_str(s)?;
-        Self::from_tree(&top)
+        Self::from_tree(top.root())
     }
 }
 

src/descriptor/sortedmulti.rs

@@ -59,7 +59,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
     }
 
     /// Parse an expression tree into a SortedMultiVec
-    pub fn from_tree(tree: &expression::Tree) -> Result<Self, Error>
+    pub fn from_tree(tree: expression::TreeIterItem) -> Result<Self, Error>
     where
         Pk: FromStrKey,
     {
@@ -69,10 +69,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
 
         let ret = Self {
             inner: tree
-                .to_null_threshold()
-                .map_err(Error::ParseThreshold)?
-                .translate_by_index(|i| tree.args[i + 1].verify_terminal("public key"))
-                .map_err(Error::Parse)?,
+                .verify_threshold(|sub| sub.verify_terminal("public_key").map_err(Error::Parse))?,
             phantom: PhantomData,
         };
         ret.constructor_check()