From 8a6ea105bcaa0032b6d444da48df8fb2be48d186 Mon Sep 17 00:00:00 2001 From: sanket1729 Date: Sat, 6 Jun 2020 04:04:04 -0500 Subject: [PATCH 1/9] removed duplicate fuzz tests --- fuzz/fuzz_targets/roundtrip_policy.rs | 40 ++++----------------------- 1 file changed, 6 insertions(+), 34 deletions(-) diff --git a/fuzz/fuzz_targets/roundtrip_policy.rs b/fuzz/fuzz_targets/roundtrip_policy.rs index ab93e29cf..ec12c6411 100644 --- a/fuzz/fuzz_targets/roundtrip_policy.rs +++ b/fuzz/fuzz_targets/roundtrip_policy.rs @@ -1,8 +1,10 @@ extern crate miniscript; +extern crate regex; use std::str::FromStr; use miniscript::{policy, DummyKey}; +use regex::Regex; type DummyPolicy = policy::Concrete; type DummyPolicy2 = policy::Semantic; @@ -11,6 +13,10 @@ fn do_test(data: &[u8]) { let data_str = String::from_utf8_lossy(data); if let Ok(pol) = DummyPolicy::from_str(&data_str) { let output = pol.to_string(); + //remove all instances of 1@ + let re = Regex::new("(\\D)1@").unwrap(); + let output = re.replace_al l(&output, "$1"); + let data_str = re.replace_all(&data_str, "$1"); assert_eq!(data_str, output); } if let Ok(pol) = DummyPolicy2::from_str(&data_str) { @@ -38,37 +44,3 @@ fn main() { }); } } - -#[cfg(test)] -mod tests { - fn extend_vec_from_hex(hex: &str, out: &mut Vec) { - 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 duplicate_crash() { - let mut a = Vec::new(); - extend_vec_from_hex("00", &mut a); - super::do_test(&a); - } - - #[test] - fn duplicate_crash_2() { - let mut a = Vec::new(); - extend_vec_from_hex("746872657368", &mut a); // thresh - super::do_test(&a); - } -} From 351576ae2a30bd2ef62a9c6f63c05d133d300805 Mon Sep 17 00:00:00 2001 From: sanket1729 Date: Sat, 6 Jun 2020 04:05:11 -0500 Subject: [PATCH 2/9] Updated fuzz tests --- fuzz/Cargo.toml | 1 + fuzz/README | 39 +++++++++++++++++++ fuzz/fuzz_targets/compile_descriptor.rs | 28 ------------- fuzz/fuzz_targets/roundtrip_concrete.rs | 7 +++- fuzz/fuzz_targets/roundtrip_descriptor.rs | 36 +---------------- .../roundtrip_miniscript_script.rs | 27 ------------- fuzz/fuzz_targets/roundtrip_miniscript_str.rs | 27 ------------- fuzz/fuzz_targets/roundtrip_semantic.rs | 29 +------------- 8 files changed, 48 insertions(+), 146 deletions(-) create mode 100644 fuzz/README diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index ea1164c23..efe3a72d4 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -14,6 +14,7 @@ honggfuzz_fuzz = ["honggfuzz"] [dependencies] honggfuzz = { version = "0.5", optional = true } afl = { version = "0.3", optional = true } +regex = { version = "1.3.9"} miniscript = { path = "..", features = ["fuzztarget", "compiler"] } # Prevent this from interfering with workspaces diff --git a/fuzz/README b/fuzz/README new file mode 100644 index 000000000..29bfbb8f8 --- /dev/null +++ b/fuzz/README @@ -0,0 +1,39 @@ +# Fuzz Tests + +Repository for fuzz testing Miniscript. + +## How to reproduce crashes? + +Travis should output a offending hex("048531e80700ae6400670000af5168" in the example) +which you can use as shown. Copy and paste the following code lines into file reporting crashes and +replace the hex with the offending hex. +Refer to file [roundtrip_concrete.rs](./fuzz_targets/roundtrip_concrete.rs) for an example. + +``` +#[cfg(test)] +mod tests { + fn extend_vec_from_hex(hex: &str, out: &mut Vec) { + 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 duplicate_crash() { + let mut a = Vec::new(); + extend_vec_from_hex("048531e80700ae6400670000af5168", &mut a); + super::do_test(&a); + } +} +``` \ No newline at end of file diff --git a/fuzz/fuzz_targets/compile_descriptor.rs b/fuzz/fuzz_targets/compile_descriptor.rs index 5f4ddb2d3..140e230ec 100644 --- a/fuzz/fuzz_targets/compile_descriptor.rs +++ b/fuzz/fuzz_targets/compile_descriptor.rs @@ -46,31 +46,3 @@ fn main() { }); } } - -#[cfg(test)] -mod tests { - fn extend_vec_from_hex(hex: &str, out: &mut Vec) { - 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 duplicate_crash() { - super::do_test(b"pkh()"); - let mut a = Vec::new(); - extend_vec_from_hex("00", &mut a); - super::do_test(&a); - } -} diff --git a/fuzz/fuzz_targets/roundtrip_concrete.rs b/fuzz/fuzz_targets/roundtrip_concrete.rs index fdaf62f21..d5805b236 100644 --- a/fuzz/fuzz_targets/roundtrip_concrete.rs +++ b/fuzz/fuzz_targets/roundtrip_concrete.rs @@ -1,8 +1,9 @@ extern crate miniscript; - +extern crate regex; use std::str::FromStr; use miniscript::{policy, DummyKey}; +use regex::Regex; type DummyPolicy = policy::Concrete; @@ -10,6 +11,10 @@ fn do_test(data: &[u8]) { let data_str = String::from_utf8_lossy(data); if let Ok(pol) = DummyPolicy::from_str(&data_str) { let output = pol.to_string(); + //remove all instances of 1@ + let re = Regex::new("(\\D)1@").unwrap(); + let output = re.replace_all(&output, "$1"); + let data_str = re.replace_all(&data_str, "$1"); assert_eq!(data_str, output); } } diff --git a/fuzz/fuzz_targets/roundtrip_descriptor.rs b/fuzz/fuzz_targets/roundtrip_descriptor.rs index ac1b2bcdb..e80583643 100644 --- a/fuzz/fuzz_targets/roundtrip_descriptor.rs +++ b/fuzz/fuzz_targets/roundtrip_descriptor.rs @@ -32,38 +32,4 @@ fn main() { do_test(data); }); } -} - -#[cfg(test)] -mod tests { - fn extend_vec_from_hex(hex: &str, out: &mut Vec) { - 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 duplicate_crash() { - let mut a = Vec::new(); - extend_vec_from_hex("00", &mut a); - super::do_test(&a); - } - - #[test] - fn test_cpkk_alias() { - let mut a = Vec::new(); - extend_vec_from_hex("633a706b5f6b2829", &mut a); // c:pk_k() - super::do_test(&a); - } -} +} \ No newline at end of file diff --git a/fuzz/fuzz_targets/roundtrip_miniscript_script.rs b/fuzz/fuzz_targets/roundtrip_miniscript_script.rs index 3ce8ad939..8f2be3151 100644 --- a/fuzz/fuzz_targets/roundtrip_miniscript_script.rs +++ b/fuzz/fuzz_targets/roundtrip_miniscript_script.rs @@ -34,30 +34,3 @@ fn main() { }); } } - -#[cfg(test)] -mod tests { - fn extend_vec_from_hex(hex: &str, out: &mut Vec) { - 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 duplicate_crash() { - let mut a = Vec::new(); - extend_vec_from_hex("007c920092935187", &mut a); - super::do_test(&a); - } -} diff --git a/fuzz/fuzz_targets/roundtrip_miniscript_str.rs b/fuzz/fuzz_targets/roundtrip_miniscript_str.rs index 3cf46712b..f4d7558a5 100644 --- a/fuzz/fuzz_targets/roundtrip_miniscript_str.rs +++ b/fuzz/fuzz_targets/roundtrip_miniscript_str.rs @@ -33,30 +33,3 @@ fn main() { }); } } - -#[cfg(test)] -mod tests { - fn extend_vec_from_hex(hex: &str, out: &mut Vec) { - 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 duplicate_crash() { - let mut a = Vec::new(); - extend_vec_from_hex("00", &mut a); - super::do_test(&a); - } -} diff --git a/fuzz/fuzz_targets/roundtrip_semantic.rs b/fuzz/fuzz_targets/roundtrip_semantic.rs index e65e95a48..75b1dac2f 100644 --- a/fuzz/fuzz_targets/roundtrip_semantic.rs +++ b/fuzz/fuzz_targets/roundtrip_semantic.rs @@ -32,31 +32,4 @@ fn main() { do_test(data); }); } -} - -#[cfg(test)] -mod tests { - fn extend_vec_from_hex(hex: &str, out: &mut Vec) { - 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 duplicate_crash() { - let mut a = Vec::new(); - extend_vec_from_hex("048531e80700ae6400670000af5168", &mut a); - super::do_test(&a); - } -} +} \ No newline at end of file From 7fe3466b87f6ddfdbb4a9e1673093d9da8c5a2d4 Mon Sep 17 00:00:00 2001 From: sanket1729 Date: Sat, 6 Jun 2020 04:06:56 -0500 Subject: [PATCH 3/9] Added unit tests from fuzz repo --- src/descriptor/mod.rs | 19 +++++++++++++++---- src/policy/mod.rs | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index ab30b4dcf..906710d30 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -536,14 +536,25 @@ mod tests { use bitcoin::{self, secp256k1, PublicKey}; use miniscript::satisfy::BitcoinSig; use std::str::FromStr; - use Descriptor; - use Miniscript; - use Satisfier; + use {Descriptor, DummyKey, Miniscript, Satisfier}; type StdDescriptor = Descriptor; const TEST_PK: &'static str = "pk(020000000000000000000000000000000000000000000000000000000000000002)"; + fn roundtrip_descriptor(s: &str) { + let desc = Descriptor::::from_str(&s).unwrap(); + let output = desc.to_string(); + let normalize_aliases = s.replace("c:pk_k(", "pk("); + assert_eq!(normalize_aliases, output); + } + + #[test] + fn desc_rtt_tests() { + roundtrip_descriptor("c:pk_k()"); + roundtrip_descriptor("wsh(pk())"); + roundtrip_descriptor("wsh(c:pk_k())"); + } #[test] fn parse_descriptor() { StdDescriptor::from_str("(").unwrap_err(); @@ -898,7 +909,7 @@ mod tests { } #[test] - fn empty_multi() { + fn roundtrip_tests() { let descriptor = Descriptor::::from_str("multi"); assert_eq!( descriptor.unwrap_err().to_string(), diff --git a/src/policy/mod.rs b/src/policy/mod.rs index 99c85bc42..62d4aa8e3 100644 --- a/src/policy/mod.rs +++ b/src/policy/mod.rs @@ -142,3 +142,40 @@ impl Liftable for Concrete { .normalized() } } + +#[cfg(test)] +mod tests { + use super::{Concrete, Semantic}; + use std::str::FromStr; + use DummyKey; + + type ConcretePol = Concrete; + type SemanticPol = Semantic; + + fn concrete_policy_rtt(s: &str) { + let conc = ConcretePol::from_str(s).unwrap(); + let output = conc.to_string(); + assert_eq!(s, output); + } + + fn semantic_policy_rtt(s: &str) { + let sem = SemanticPol::from_str(s).unwrap(); + let output = sem.to_string(); + assert_eq!(s, output); + } + + #[test] + fn policy_rtt_tests() { + concrete_policy_rtt("pk()"); + concrete_policy_rtt("or(1@pk(),1@pk())"); + concrete_policy_rtt("or(99@pk(),1@pk())"); + concrete_policy_rtt("and(pk(),or(99@pk(),1@older(12960)))"); + + semantic_policy_rtt("pkh()"); + semantic_policy_rtt("or(pkh(),pkh())"); + + //fuzzer crashes + assert!(ConcretePol::from_str("thresh()").is_err()); + assert!(SemanticPol::from_str("thresh()").is_err()); + } +} From 835a7a7779bd687fc270da1a20af0402860b0c5e Mon Sep 17 00:00:00 2001 From: sanket1729 Date: Sat, 6 Jun 2020 05:30:41 -0500 Subject: [PATCH 4/9] fixed benches --- .travis.yml | 1 + src/policy/compiler.rs | 34 +++++++++++++++++----------------- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/.travis.yml b/.travis.yml index 912da9128..8b9d22c91 100644 --- a/.travis.yml +++ b/.travis.yml @@ -9,6 +9,7 @@ matrix: env: DO_FUZZ=true DO_LINT=true - rust: beta - rust: nightly + env: DO_BENCH=true - rust: 1.22.0 script: diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs index 1d664c566..35c9606c5 100644 --- a/src/policy/compiler.rs +++ b/src/policy/compiler.rs @@ -1391,44 +1391,44 @@ mod tests { #[cfg(all(test, feature = "unstable"))] mod benches { - use secp256k1; use std::str::FromStr; use test::{black_box, Bencher}; - use Concrete; - use ParseTree; - + use super::Concrete; + use DummyKey; #[bench] pub fn compile(bh: &mut Bencher) { - let desc = Concrete::::from_str( - "and(thresh(2,and(sha256(),or(sha256(),pk())),pk(),pk(),pk(),sha256()),pkh())", + let h = (0..64).map(|_| "a").collect::(); + let desc = Concrete::::from_str( + &format!("and(thresh(2,and(sha256({}),or(sha256({}),pk())),pk(),pk(),pk(),sha256({})),pk())", h, h, h) ) .expect("parsing"); bh.iter(|| { - let pt = ParseTree::compile(&desc); - black_box(pt); + let pt = desc.compile(); + black_box(pt).unwrap(); }); } #[bench] pub fn compile_large(bh: &mut Bencher) { - let desc = Concrete::::from_str( - "or(pkh(),thresh(9,sha256(),pkh(),pk(),and(or(pkh(),pk()),pk()),time_e(),pk(),pk(),pk(),pk(),and(pk(),pk())))" + let h = (0..64).map(|_| "a").collect::(); + let desc = Concrete::::from_str( + &format!("or(pk(),thresh(9,sha256({}),pk(),pk(),and(or(pk(),pk()),pk()),after(100),pk(),pk(),pk(),pk(),and(pk(),pk())))", h) ).expect("parsing"); bh.iter(|| { - let pt = ParseTree::compile(&desc); - black_box(pt); + let pt = desc.compile(); + black_box(pt).unwrap(); }); } #[bench] pub fn compile_xlarge(bh: &mut Bencher) { - let desc = Concrete::::from_str( - "or(pk(),thresh(4,pkh(),time_e(),multi(),and(after(),or(pkh(),or(pkh(),and(pkh(),thresh(2,multi(),or(pkh(),and(thresh(5,sha256(),or(pkh(),pkh()),pkh(),pkh(),pkh(),multi(),pkh(),multi(),pk(),pkh(),pk()),pkh())),pkh(),or(and(pkh(),pk()),pk()),after()))))),pkh()))" + let desc = Concrete::::from_str( + "or(pk(),thresh(4,pk(),older(100),pk(),and(after(100),or(pk(),or(pk(),and(pk(),thresh(2,pk(),or(pk(),and(thresh(5,pk(),or(pk(),pk()),pk(),pk(),pk(),pk(),pk(),pk(),pk(),pk(),pk()),pk())),pk(),or(and(pk(),pk()),pk()),after(100)))))),pk()))" ).expect("parsing"); bh.iter(|| { - let pt = ParseTree::compile(&desc); - black_box(pt); + let pt = desc.compile(); + black_box(pt).unwrap(); }); } -} +} \ No newline at end of file From 63c895c3f27eb7b4f576ecee1af9199fac5aa018 Mon Sep 17 00:00:00 2001 From: sanket1729 Date: Sat, 6 Jun 2020 05:31:16 -0500 Subject: [PATCH 5/9] suppressed and fixed warnings --- src/lib.rs | 2 ++ src/miniscript/mod.rs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index bd8d10465..7447d77d2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -80,6 +80,8 @@ //! ``` //! //! +#![allow(bare_trait_objects)] + #![cfg_attr(all(test, feature = "unstable"), feature(test))] pub extern crate bitcoin; #[cfg(feature = "serde")] diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 35034b367..36e17efad 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -300,7 +300,7 @@ where use std::marker::PhantomData; use std::str::FromStr; - struct StrVisitor(PhantomData<(Qk)>); + struct StrVisitor(PhantomData); impl<'de, Qk> de::Visitor<'de> for StrVisitor where From e40e01222f79767f3b8cdf47d1fd68841ea3ed9e Mon Sep 17 00:00:00 2001 From: sanket1729 Date: Sat, 6 Jun 2020 05:33:19 -0500 Subject: [PATCH 6/9] removed redundant roundtrip_policy --- fuzz/fuzz_targets/roundtrip_policy.rs | 46 --------------------------- 1 file changed, 46 deletions(-) delete mode 100644 fuzz/fuzz_targets/roundtrip_policy.rs diff --git a/fuzz/fuzz_targets/roundtrip_policy.rs b/fuzz/fuzz_targets/roundtrip_policy.rs deleted file mode 100644 index ec12c6411..000000000 --- a/fuzz/fuzz_targets/roundtrip_policy.rs +++ /dev/null @@ -1,46 +0,0 @@ - -extern crate miniscript; -extern crate regex; - -use std::str::FromStr; -use miniscript::{policy, DummyKey}; -use regex::Regex; - -type DummyPolicy = policy::Concrete; -type DummyPolicy2 = policy::Semantic; - -fn do_test(data: &[u8]) { - let data_str = String::from_utf8_lossy(data); - if let Ok(pol) = DummyPolicy::from_str(&data_str) { - let output = pol.to_string(); - //remove all instances of 1@ - let re = Regex::new("(\\D)1@").unwrap(); - let output = re.replace_al l(&output, "$1"); - let data_str = re.replace_all(&data_str, "$1"); - assert_eq!(data_str, output); - } - if let Ok(pol) = DummyPolicy2::from_str(&data_str) { - let output = pol.to_string(); - assert_eq!(data_str, output); - } -} - -#[cfg(feature = "afl")] -extern crate afl; -#[cfg(feature = "afl")] -fn main() { - afl::read_stdio_bytes(|data| { - do_test(&data); - }); -} - -#[cfg(feature = "honggfuzz")] -#[macro_use] extern crate honggfuzz; -#[cfg(feature = "honggfuzz")] -fn main() { - loop { - fuzz!(|data| { - do_test(data); - }); - } -} From 71a52f08ed4179ae8b26211dba52986f06be20da Mon Sep 17 00:00:00 2001 From: sanket1729 Date: Sat, 6 Jun 2020 05:45:54 -0500 Subject: [PATCH 7/9] fmt fixes --- contrib/test.sh | 2 +- fuzz/Cargo.toml | 6 +----- src/descriptor/mod.rs | 2 +- src/lib.rs | 1 - src/policy/compiler.rs | 9 +++++---- 5 files changed, 8 insertions(+), 12 deletions(-) diff --git a/contrib/test.sh b/contrib/test.sh index 8381eebd1..112f149c7 100755 --- a/contrib/test.sh +++ b/contrib/test.sh @@ -46,5 +46,5 @@ fi # Bench if told to if [ "$DO_BENCH" = true ] then - cargo bench --features unstable + cargo bench --features="unstable compiler" fi diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index efe3a72d4..311afd911 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -43,8 +43,4 @@ path = "fuzz_targets/roundtrip_semantic.rs" [[bin]] name = "compile_descriptor" -path = "fuzz_targets/compile_descriptor.rs" - -[[bin]] -name = "roundtrip_policy" -path = "fuzz_targets/roundtrip_policy.rs" +path = "fuzz_targets/compile_descriptor.rs" \ No newline at end of file diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index 906710d30..e6b23a993 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -489,7 +489,7 @@ where fn deserialize>(d: D) -> Result, D::Error> { use std::marker::PhantomData; - struct StrVisitor(PhantomData<(Qk)>); + struct StrVisitor(PhantomData); impl<'de, Qk> de::Visitor<'de> for StrVisitor where diff --git a/src/lib.rs b/src/lib.rs index 7447d77d2..7e660120c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -81,7 +81,6 @@ //! //! #![allow(bare_trait_objects)] - #![cfg_attr(all(test, feature = "unstable"), feature(test))] pub extern crate bitcoin; #[cfg(feature = "serde")] diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs index 35c9606c5..4b1f4fd0a 100644 --- a/src/policy/compiler.rs +++ b/src/policy/compiler.rs @@ -1399,9 +1399,10 @@ mod benches { #[bench] pub fn compile(bh: &mut Bencher) { let h = (0..64).map(|_| "a").collect::(); - let desc = Concrete::::from_str( - &format!("and(thresh(2,and(sha256({}),or(sha256({}),pk())),pk(),pk(),pk(),sha256({})),pk())", h, h, h) - ) + let desc = Concrete::::from_str(&format!( + "and(thresh(2,and(sha256({}),or(sha256({}),pk())),pk(),pk(),pk(),sha256({})),pk())", + h, h, h + )) .expect("parsing"); bh.iter(|| { let pt = desc.compile(); @@ -1431,4 +1432,4 @@ mod benches { black_box(pt).unwrap(); }); } -} \ No newline at end of file +} From ff9373a232df2717e42ad3b9f4035b5619d0b3f5 Mon Sep 17 00:00:00 2001 From: sanket1729 Date: Tue, 16 Jun 2020 13:34:14 -0500 Subject: [PATCH 8/9] And or fragments must have exactly two arguments --- src/policy/concrete.rs | 8 ++++---- src/policy/semantic.rs | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs index a7501b67e..c003b2920 100644 --- a/src/policy/concrete.rs +++ b/src/policy/concrete.rs @@ -401,8 +401,8 @@ where hash160::Hash::from_hex(x).map(Policy::Hash160) }), ("and", _) => { - if top.args.is_empty() { - return Err(errstr("and without args")); + if top.args.len() != 2 { + return Err(errstr("and fragment must have exactly two children")); } let mut subs = Vec::with_capacity(top.args.len()); for arg in &top.args { @@ -411,8 +411,8 @@ where Ok(Policy::And(subs)) } ("or", _) => { - if top.args.is_empty() { - return Err(errstr("or without args")); + if top.args.len() != 2 { + return Err(errstr("or fragment must have exactly two children")); } let mut subs = Vec::with_capacity(top.args.len()); for arg in &top.args { diff --git a/src/policy/semantic.rs b/src/policy/semantic.rs index e1705bc6f..f94f91128 100644 --- a/src/policy/semantic.rs +++ b/src/policy/semantic.rs @@ -234,8 +234,8 @@ where hash160::Hash::from_hex(x).map(Policy::Hash160) }), ("and", _) => { - if top.args.is_empty() { - return Err(errstr("and without args")); + if top.args.len() != 2 { + return Err(errstr("and fragment must have exactly two children")); } let mut subs = Vec::with_capacity(top.args.len()); for arg in &top.args { @@ -244,8 +244,8 @@ where Ok(Policy::And(subs)) } ("or", _) => { - if top.args.is_empty() { - return Err(errstr("or without args")); + if top.args.len() != 2 { + return Err(errstr("or fragment must have exactly two children")); } let mut subs = Vec::with_capacity(top.args.len()); for arg in &top.args { From b0c64520db6dec136f3e61d8cdb079f5fcfe541b Mon Sep 17 00:00:00 2001 From: sanket1729 Date: Tue, 16 Jun 2020 21:10:06 -0500 Subject: [PATCH 9/9] Policy fixes --- src/lib.rs | 10 ++++++++++ src/policy/compiler.rs | 33 ++------------------------------- src/policy/concrete.rs | 41 +++++++++++++++++++++++++---------------- src/policy/mod.rs | 39 +++++++++++++++++++++++++++++++++++++++ src/policy/semantic.rs | 6 +++--- 5 files changed, 79 insertions(+), 50 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 7e660120c..befff7218 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -310,6 +310,8 @@ pub enum Error { #[cfg(feature = "compiler")] ///Compiler related errors CompilerError(policy::compiler::CompilerError), + ///Errors related to policy + PolicyError(policy::concrete::PolicyError), ///Interpreter related errors InterpreterError(descriptor::InterpreterError), /// Bad Script Sig. As per standardness rules, only pushes are allowed in @@ -401,6 +403,7 @@ impl fmt::Display for Error { Error::InterpreterError(ref e) => fmt::Display::fmt(e, f), #[cfg(feature = "compiler")] Error::CompilerError(ref e) => fmt::Display::fmt(e, f), + Error::PolicyError(ref e) => fmt::Display::fmt(e, f), Error::BadScriptSig => f.write_str("Script sig must only consist of pushes"), Error::NonEmptyWitness => f.write_str("Non empty witness for Pk/Pkh"), Error::NonEmptyScriptSig => f.write_str("Non empty script sig for segwit spend"), @@ -429,6 +432,13 @@ impl From for Error { } } +#[doc(hidden)] +impl From for Error { + fn from(e: policy::concrete::PolicyError) -> Error { + Error::PolicyError(e) + } +} + /// The size of an encoding of a number in Script pub fn script_num_size(n: usize) -> usize { match n { diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs index 4b1f4fd0a..16f694b13 100644 --- a/src/policy/compiler.rs +++ b/src/policy/compiler.rs @@ -18,6 +18,7 @@ //! use std::collections::HashMap; +use std::convert::From; use std::{cmp, error, f64, fmt}; use miniscript::types::extra_props::MAX_OPS_PER_SCRIPT; @@ -1136,9 +1137,6 @@ mod tests { use std::str::FromStr; use miniscript::satisfy; - use policy::concrete::PolicyError::{ - IncorrectThresh, NonBinaryArgAnd, NonBinaryArgOr, TimeTooFar, ZeroTime, - }; use policy::Liftable; use BitcoinSig; use DummyKey; @@ -1173,40 +1171,13 @@ mod tests { } fn policy_compile_lift_check(s: &str) -> Result<(), CompilerError> { - let policy = DummyPolicy::from_str(s).expect("parse"); + let policy = DummyPolicy::from_str(s).unwrap(); let miniscript = policy.compile()?; assert_eq!(policy.lift().sorted(), miniscript.lift().sorted()); Ok(()) } - #[test] - fn compile_invalid() { - assert_eq!( - policy_compile_lift_check("thresh(2,pk(),thresh(0))"), - Err(CompilerError::PolicyError(IncorrectThresh)) - ); - assert_eq!( - policy_compile_lift_check("thresh(2,pk(),thresh(0,pk()))"), - Err(CompilerError::PolicyError(IncorrectThresh)) - ); - assert_eq!( - policy_compile_lift_check("and(pk())"), - Err(CompilerError::PolicyError(NonBinaryArgAnd)) - ); - assert_eq!( - policy_compile_lift_check("or(pk())"), - Err(CompilerError::PolicyError(NonBinaryArgOr)) - ); - assert_eq!( - policy_compile_lift_check("thresh(3,after(0),pk(),pk())"), - Err(CompilerError::PolicyError(ZeroTime)) - ); - assert_eq!( - policy_compile_lift_check("thresh(2,older(2147483650),pk(),pk())"), - Err(CompilerError::PolicyError(TimeTooFar)) - ); - } #[test] fn compile_basic() { assert!(policy_compile_lift_check("pk()").is_ok()); diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs index c003b2920..2e23d5d09 100644 --- a/src/policy/concrete.rs +++ b/src/policy/concrete.rs @@ -89,7 +89,7 @@ impl fmt::Display for PolicyError { } PolicyError::NonBinaryArgOr => f.write_str("Or policy fragment must take 2 arguments"), PolicyError::IncorrectThresh => { - f.write_str("Threshold k must be greater than 0 and less than n") + f.write_str("Threshold k must be greater than 0 and less than or equal to n 0 { f.write_str("Relative/Absolute time must be less than 2^31; n < 2^31") @@ -382,12 +382,24 @@ where } match (frag_name, top.args.len() as u32) { ("pk", 1) => expression::terminal(&top.args[0], |pk| Pk::from_str(pk).map(Policy::Key)), - ("after", 1) => expression::terminal(&top.args[0], |x| { - expression::parse_num(x).map(Policy::After) - }), - ("older", 1) => expression::terminal(&top.args[0], |x| { - expression::parse_num(x).map(Policy::Older) - }), + ("after", 1) => { + let num = expression::terminal(&top.args[0], |x| expression::parse_num(x))?; + if num > 2u32.pow(31) { + return Err(Error::PolicyError(PolicyError::TimeTooFar)); + } else if num == 0 { + return Err(Error::PolicyError(PolicyError::ZeroTime)); + } + Ok(Policy::After(num)) + } + ("older", 1) => { + let num = expression::terminal(&top.args[0], |x| expression::parse_num(x))?; + if num > 2u32.pow(31) { + return Err(Error::PolicyError(PolicyError::TimeTooFar)); + } else if num == 0 { + return Err(Error::PolicyError(PolicyError::ZeroTime)); + } + Ok(Policy::Older(num)) + } ("sha256", 1) => expression::terminal(&top.args[0], |x| { sha256::Hash::from_hex(x).map(Policy::Sha256) }), @@ -402,7 +414,7 @@ where }), ("and", _) => { if top.args.len() != 2 { - return Err(errstr("and fragment must have exactly two children")); + return Err(Error::PolicyError(PolicyError::NonBinaryArgAnd)); } let mut subs = Vec::with_capacity(top.args.len()); for arg in &top.args { @@ -412,7 +424,7 @@ where } ("or", _) => { if top.args.len() != 2 { - return Err(errstr("or fragment must have exactly two children")); + return Err(Error::PolicyError(PolicyError::NonBinaryArgOr)); } let mut subs = Vec::with_capacity(top.args.len()); for arg in &top.args { @@ -421,16 +433,13 @@ where Ok(Policy::Or(subs)) } ("thresh", nsubs) => { - if top.args.is_empty() { - return Err(errstr("thresh without args")); - } - if !top.args[0].args.is_empty() { - return Err(errstr(top.args[0].args[0].name)); + if top.args.is_empty() || !top.args[0].args.is_empty() { + return Err(Error::PolicyError(PolicyError::IncorrectThresh)); } let thresh = expression::parse_num(top.args[0].name)?; - if thresh >= nsubs { - return Err(errstr(top.args[0].name)); + if thresh >= nsubs || thresh <= 0 { + return Err(Error::PolicyError(PolicyError::IncorrectThresh)); } let mut subs = Vec::with_capacity(top.args.len() - 1); diff --git a/src/policy/mod.rs b/src/policy/mod.rs index 62d4aa8e3..e9e93160e 100644 --- a/src/policy/mod.rs +++ b/src/policy/mod.rs @@ -178,4 +178,43 @@ mod tests { assert!(ConcretePol::from_str("thresh()").is_err()); assert!(SemanticPol::from_str("thresh()").is_err()); } + + #[test] + fn compile_invalid() { + // Since the root Error does not support Eq type, we hvae to + // compare the string representations of the error + assert_eq!( + ConcretePol::from_str("thresh(2,pk(),thresh(0))") + .unwrap_err() + .to_string(), + "Threshold k must be greater than 0 and less than or equal to n 0 0" + ); + + assert_eq!( + ConcretePol::from_str("thresh(2,older(2147483650),pk(),pk())") + .unwrap_err() + .to_string(), + "Relative/Absolute time must be less than 2^31; n < 2^31" + ); + } } diff --git a/src/policy/semantic.rs b/src/policy/semantic.rs index f94f91128..ee108dfcd 100644 --- a/src/policy/semantic.rs +++ b/src/policy/semantic.rs @@ -18,11 +18,11 @@ use bitcoin::hashes::hex::FromHex; use bitcoin::hashes::{hash160, ripemd160, sha256, sha256d}; use std::{fmt, str}; +use super::concrete::PolicyError; use errstr; use std::str::FromStr; use Error; use {expression, MiniscriptKey}; - /// Abstract policy which corresponds to the semantics of a Miniscript /// and which allows complex forms of analysis, e.g. filtering and /// normalization. @@ -235,7 +235,7 @@ where }), ("and", _) => { if top.args.len() != 2 { - return Err(errstr("and fragment must have exactly two children")); + return Err(Error::PolicyError(PolicyError::NonBinaryArgAnd)); } let mut subs = Vec::with_capacity(top.args.len()); for arg in &top.args { @@ -245,7 +245,7 @@ where } ("or", _) => { if top.args.len() != 2 { - return Err(errstr("or fragment must have exactly two children")); + return Err(Error::PolicyError(PolicyError::NonBinaryArgOr)); } let mut subs = Vec::with_capacity(top.args.len()); for arg in &top.args {