Skip to content

Commit db7b25c

Browse files
apoelstraapoelstra
committed
Merge #780: expression: replace recursive Tree datatype with a non-recursive one
6c8c34b descriptor: add fuzz test comparing to published rust-miniscript 12 (Andrew Poelstra) ebae0ef miniscript: remove unused Result from extdata type_check (Andrew Poelstra) d05a034 expression: rewrite Tree module to no longer use a recursive data type (Andrew Poelstra) 616b6b6 expression: hide all Tree fields and encapsulate API (Andrew Poelstra) 4137a59 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 6c8c34b Tree-SHA512: ee64f1aa5fdc3917b6561713249fb04a39baa30c43090705cd899ec88bc828fc6227ae6ee91f58b22ba02dd4a2026d8240156382fb27cbfb3a904599d629974a
2 parents ce42c66 + 6c8c34b commit db7b25c

22 files changed

+708
-353
lines changed

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

+1
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ compile_descriptor,
2222
compile_taproot,
2323
parse_descriptor,
2424
parse_descriptor_secret,
25+
regression_descriptor_parse,
2526
roundtrip_concrete,
2627
roundtrip_descriptor,
2728
roundtrip_miniscript_script,

‎Cargo-minimal.lock

+13-2
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,8 @@ name = "descriptor-fuzz"
111111
version = "0.0.1"
112112
dependencies = [
113113
"honggfuzz",
114-
"miniscript",
114+
"miniscript 12.3.0",
115+
"miniscript 13.0.0",
115116
"regex",
116117
]
117118

@@ -181,7 +182,17 @@ dependencies = [
181182

182183
[[package]]
183184
name = "miniscript"
184-
version = "12.2.0"
185+
version = "12.3.0"
186+
source = "registry+https://github.com/rust-lang/crates.io-index"
187+
checksum = "5bd3c9608217b0d6fa9c9c8ddd875b85ab72bd4311cfc8db35e1b5a08fc11f4d"
188+
dependencies = [
189+
"bech32",
190+
"bitcoin",
191+
]
192+
193+
[[package]]
194+
name = "miniscript"
195+
version = "13.0.0"
185196
dependencies = [
186197
"bech32",
187198
"bitcoin",

‎Cargo-recent.lock

+13-2
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,8 @@ name = "descriptor-fuzz"
111111
version = "0.0.1"
112112
dependencies = [
113113
"honggfuzz",
114-
"miniscript",
114+
"miniscript 12.3.0",
115+
"miniscript 13.0.0",
115116
"regex",
116117
]
117118

@@ -181,7 +182,17 @@ dependencies = [
181182

182183
[[package]]
183184
name = "miniscript"
184-
version = "12.2.0"
185+
version = "12.3.0"
186+
source = "registry+https://github.com/rust-lang/crates.io-index"
187+
checksum = "5bd3c9608217b0d6fa9c9c8ddd875b85ab72bd4311cfc8db35e1b5a08fc11f4d"
188+
dependencies = [
189+
"bech32",
190+
"bitcoin",
191+
]
192+
193+
[[package]]
194+
name = "miniscript"
195+
version = "13.0.0"
185196
dependencies = [
186197
"bech32",
187198
"bitcoin",

‎Cargo.toml

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[package]
22
name = "miniscript"
3-
version = "12.2.0"
3+
version = "13.0.0"
44
authors = ["Andrew Poelstra <[email protected]>, Sanket Kanjalkar <[email protected]>"]
55
license = "CC0-1.0"
66
homepage = "https://github.com/rust-bitcoin/rust-miniscript/"

‎fuzz/Cargo.toml

+8-1
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@ cargo-fuzz = true
1111

1212
[dependencies]
1313
honggfuzz = { version = "0.5.56", default-features = false }
14-
miniscript = { path = "..", features = [ "compiler" ] }
14+
# We shouldn't need an explicit version on the next line, but Andrew's tools
15+
# choke on it otherwise. See https://github.com/nix-community/crate2nix/issues/373
16+
miniscript = { path = "..", features = [ "compiler" ], version = "13.0" }
17+
old_miniscript = { package = "miniscript", version = "12.3" }
1518

1619
regex = "1.0"
1720

@@ -31,6 +34,10 @@ path = "fuzz_targets/parse_descriptor.rs"
3134
name = "parse_descriptor_secret"
3235
path = "fuzz_targets/parse_descriptor_secret.rs"
3336

37+
[[bin]]
38+
name = "regression_descriptor_parse"
39+
path = "fuzz_targets/regression_descriptor_parse.rs"
40+
3441
[[bin]]
3542
name = "roundtrip_concrete"
3643
path = "fuzz_targets/roundtrip_concrete.rs"

‎fuzz/fuzz_targets/regression_descriptor_parse.rs

+43
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
use core::str::FromStr;
2+
3+
use honggfuzz::fuzz;
4+
use miniscript::{Descriptor, DescriptorPublicKey};
5+
use old_miniscript::{Descriptor as OldDescriptor, DescriptorPublicKey as OldDescriptorPublicKey};
6+
7+
type Desc = Descriptor<DescriptorPublicKey>;
8+
type OldDesc = OldDescriptor<OldDescriptorPublicKey>;
9+
10+
fn do_test(data: &[u8]) {
11+
let data_str = String::from_utf8_lossy(data);
12+
match (Desc::from_str(&data_str), OldDesc::from_str(&data_str)) {
13+
(Err(_), Err(_)) => {}
14+
(Ok(x), Err(e)) => panic!("new logic parses {} as {:?}, old fails with {}", data_str, x, e),
15+
(Err(e), Ok(x)) => panic!("old logic parses {} as {:?}, new fails with {}", data_str, x, e),
16+
(Ok(new), Ok(old)) => {
17+
assert_eq!(
18+
old.to_string(),
19+
new.to_string(),
20+
"input {} (left is old, right is new)",
21+
data_str
22+
)
23+
}
24+
}
25+
}
26+
27+
fn main() {
28+
loop {
29+
fuzz!(|data| {
30+
do_test(data);
31+
});
32+
}
33+
}
34+
35+
#[cfg(test)]
36+
mod tests {
37+
#[test]
38+
fn duplicate_crash() {
39+
crate::do_test(
40+
b"tr(02dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd,{1,unun:0})",
41+
)
42+
}
43+
}

‎fuzz/generate-files.sh

+1
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ cargo-fuzz = true
2424
[dependencies]
2525
honggfuzz = { version = "0.5.56", default-features = false }
2626
miniscript = { path = "..", features = [ "compiler" ] }
27+
old_miniscript = { package = "miniscript", git = "https://github.com/apoelstra/rust-miniscript/", rev = "1259375d7b7c91053e09d1cbe3db983612fe301c" }
2728
2829
regex = "1.0"
2930
EOF

‎src/descriptor/bare.rs

+6-6
Original file line numberDiff line numberDiff line change
@@ -175,8 +175,8 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Bare<Pk> {
175175
}
176176

177177
impl<Pk: FromStrKey> FromTree for Bare<Pk> {
178-
fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
179-
let sub = Miniscript::<Pk, BareCtx>::from_tree(top)?;
178+
fn from_tree(root: expression::TreeIterItem) -> Result<Self, Error> {
179+
let sub = Miniscript::<Pk, BareCtx>::from_tree(root)?;
180180
BareCtx::top_level_checks(&sub)?;
181181
Bare::new(sub)
182182
}
@@ -186,7 +186,7 @@ impl<Pk: FromStrKey> core::str::FromStr for Bare<Pk> {
186186
type Err = Error;
187187
fn from_str(s: &str) -> Result<Self, Self::Err> {
188188
let top = expression::Tree::from_str(s)?;
189-
Self::from_tree(&top)
189+
Self::from_tree(top.root())
190190
}
191191
}
192192

@@ -369,8 +369,8 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Pkh<Pk> {
369369
}
370370

371371
impl<Pk: FromStrKey> FromTree for Pkh<Pk> {
372-
fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
373-
let pk = top
372+
fn from_tree(root: expression::TreeIterItem) -> Result<Self, Error> {
373+
let pk = root
374374
.verify_terminal_parent("pkh", "public key")
375375
.map_err(Error::Parse)?;
376376
Pkh::new(pk).map_err(Error::ContextError)
@@ -381,7 +381,7 @@ impl<Pk: FromStrKey> core::str::FromStr for Pkh<Pk> {
381381
type Err = Error;
382382
fn from_str(s: &str) -> Result<Self, Self::Err> {
383383
let top = expression::Tree::from_str(s)?;
384-
Self::from_tree(&top)
384+
Self::from_tree(top.root())
385385
}
386386
}
387387

‎src/descriptor/mod.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -965,8 +965,8 @@ impl Descriptor<DefiniteDescriptorKey> {
965965

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

‎src/descriptor/segwitv0.rs

+5-5
Original file line numberDiff line numberDiff line change
@@ -247,13 +247,13 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Wsh<Pk> {
247247
}
248248

249249
impl<Pk: FromStrKey> crate::expression::FromTree for Wsh<Pk> {
250-
fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
250+
fn from_tree(top: expression::TreeIterItem) -> Result<Self, Error> {
251251
let top = top
252252
.verify_toplevel("wsh", 1..=1)
253253
.map_err(From::from)
254254
.map_err(Error::Parse)?;
255255

256-
if top.name == "sortedmulti" {
256+
if top.name() == "sortedmulti" {
257257
return Ok(Wsh { inner: WshInner::SortedMulti(SortedMultiVec::from_tree(top)?) });
258258
}
259259
let sub = Miniscript::from_tree(top)?;
@@ -284,7 +284,7 @@ impl<Pk: FromStrKey> core::str::FromStr for Wsh<Pk> {
284284
type Err = Error;
285285
fn from_str(s: &str) -> Result<Self, Self::Err> {
286286
let top = expression::Tree::from_str(s)?;
287-
Wsh::<Pk>::from_tree(&top)
287+
Wsh::<Pk>::from_tree(top.root())
288288
}
289289
}
290290

@@ -483,7 +483,7 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Wpkh<Pk> {
483483
}
484484

485485
impl<Pk: FromStrKey> crate::expression::FromTree for Wpkh<Pk> {
486-
fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
486+
fn from_tree(top: expression::TreeIterItem) -> Result<Self, Error> {
487487
let pk = top
488488
.verify_terminal_parent("wpkh", "public key")
489489
.map_err(Error::Parse)?;
@@ -495,7 +495,7 @@ impl<Pk: FromStrKey> core::str::FromStr for Wpkh<Pk> {
495495
type Err = Error;
496496
fn from_str(s: &str) -> Result<Self, Self::Err> {
497497
let top = expression::Tree::from_str(s)?;
498-
Self::from_tree(&top)
498+
Self::from_tree(top.root())
499499
}
500500
}
501501

‎src/descriptor/sh.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -81,13 +81,13 @@ impl<Pk: MiniscriptKey> fmt::Display for Sh<Pk> {
8181
}
8282

8383
impl<Pk: FromStrKey> crate::expression::FromTree for Sh<Pk> {
84-
fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
84+
fn from_tree(top: expression::TreeIterItem) -> Result<Self, Error> {
8585
let top = top
8686
.verify_toplevel("sh", 1..=1)
8787
.map_err(From::from)
8888
.map_err(Error::Parse)?;
8989

90-
let inner = match top.name {
90+
let inner = match top.name() {
9191
"wsh" => ShInner::Wsh(Wsh::from_tree(top)?),
9292
"wpkh" => ShInner::Wpkh(Wpkh::from_tree(top)?),
9393
"sortedmulti" => ShInner::SortedMulti(SortedMultiVec::from_tree(top)?),
@@ -105,7 +105,7 @@ impl<Pk: FromStrKey> core::str::FromStr for Sh<Pk> {
105105
type Err = Error;
106106
fn from_str(s: &str) -> Result<Self, Self::Err> {
107107
let top = expression::Tree::from_str(s)?;
108-
Self::from_tree(&top)
108+
Self::from_tree(top.root())
109109
}
110110
}
111111

‎src/descriptor/sortedmulti.rs

+2-5
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
5959
}
6060

6161
/// Parse an expression tree into a SortedMultiVec
62-
pub fn from_tree(tree: &expression::Tree) -> Result<Self, Error>
62+
pub fn from_tree(tree: expression::TreeIterItem) -> Result<Self, Error>
6363
where
6464
Pk: FromStrKey,
6565
{
@@ -69,10 +69,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
6969

7070
let ret = Self {
7171
inner: tree
72-
.to_null_threshold()
73-
.map_err(Error::ParseThreshold)?
74-
.translate_by_index(|i| tree.args[i + 1].verify_terminal("public key"))
75-
.map_err(Error::Parse)?,
72+
.verify_threshold(|sub| sub.verify_terminal("public_key").map_err(Error::Parse))?,
7673
phantom: PhantomData,
7774
};
7875
ret.constructor_check()

0 commit comments

Comments
 (0)