Merge rust-bitcoin/rust-miniscript#780: expression: replace recursive Tree datatype with a non-recursive one

6c8c34b347f9e3d531c9a0b771e6fa607a5c8e9b descriptor: add fuzz test comparing to published rust-miniscript 12 (Andrew Poelstra)
ebae0ef9b7ac6d36a35c1d3ed80cc561d96760dd miniscript: remove unused Result from extdata type_check (Andrew Poelstra)
d05a034376dfd081c766a5965e72c3ba8056d28e expression: rewrite Tree module to no longer use a recursive data type (Andrew Poelstra)
616b6b6cf2b55cf4fe9230ac07c57ab4c249b9cb expression: hide all Tree fields and encapsulate API (Andrew Poelstra)
4137a5953c2abedb1834ac6ab0103e022fd1e256 expression: encapsulate threshold parsing (Andrew Poelstra)

Pull request description:

  Replaces the recursive `expression::Tree` datatype with a nonrecursive one. Exposes a limited API consisting of operations which can be done efficiently (mainly, in-order iteration) and which are necessary for parsing trees and converting them to other types.

  As a side effect this simplifies/unifies some more code and provides better error messages, in particular for threshold parsing. But that isn't a focus of this PR and I haven't quantified the changes.

  This is the last of the "expression" PRs. I have followups which go in two directions: (1) eliminating more recursion and recursive datatypes, and (2) improving the TapTree API, which I found I needed this new expression API to do cleanly.

  Will post benchmarks once I have them.

ACKs for top commit:
  sanket1729:
    utACK 6c8c34b347f9e3d531c9a0b771e6fa607a5c8e9b

Tree-SHA512: ee64f1aa5fdc3917b6561713249fb04a39baa30c43090705cd899ec88bc828fc6227ae6ee91f58b22ba02dd4a2026d8240156382fb27cbfb3a904599d629974a

Author: heap-coder

.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 <[email protected]>, Sanket Kanjalkar <[email protected]>"]
 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())
     }
 }