From 00cac40ef68a0c74e40e9a252000f21dfb82c58a Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 31 Aug 2024 20:36:29 +0000 Subject: [PATCH 1/3] descriptor: add unit test demonstrating sanity-checking behavior in <= 12.x See https://github.com/rust-bitcoin/rust-miniscript/issues/734 for discussion of this. Meanwhile, add a unit test so we can determine when the behavior changes. --- src/descriptor/mod.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index 1a2a7da16..d9b3a9f51 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -2011,6 +2011,22 @@ pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))"; Descriptor::::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/<0;1;2;3>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/<0;1;2>/*)))").unwrap_err(); } + #[test] + fn regression_734() { + Descriptor::::from_str( + "wsh(or_i(pk(0202baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0a66a),1))", + ) + .unwrap(); + Descriptor::::from_str( + "sh(or_i(pk(0202baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0a66a),1))", + ) + .unwrap(); + Descriptor::::from_str( + "tr(02baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0a66a,1)", + ) + .unwrap_err(); + } + #[test] fn test_context_pks() { let comp_key = bitcoin::PublicKey::from_str( From 67fdc506dfdfd7c3c35108fbd30010fda9a4a2eb Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 1 Sep 2024 13:48:13 +0000 Subject: [PATCH 2/3] descriptor: reject strings of the form "tr(,)" Fixes #736 --- src/descriptor/mod.rs | 8 ++++++++ src/descriptor/tr.rs | 3 --- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index d9b3a9f51..d6e706bb8 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -2011,6 +2011,14 @@ pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))"; Descriptor::::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/<0;1;2;3>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/<0;1;2>/*)))").unwrap_err(); } + #[test] + fn regression_736() { + Descriptor::::from_str( + "tr(0000000000000000000000000000000000000000000000000000000000000002,)", + ) + .unwrap_err(); + } + #[test] fn regression_734() { Descriptor::::from_str( diff --git a/src/descriptor/tr.rs b/src/descriptor/tr.rs index 2f505780c..41845f174 100644 --- a/src/descriptor/tr.rs +++ b/src/descriptor/tr.rs @@ -603,9 +603,6 @@ fn parse_tr_tree(s: &str) -> Result { return Err(Error::Unexpected("invalid taproot internal key".to_string())); } let internal_key = expression::Tree { name: key.name, args: vec![] }; - if script.is_empty() { - return Ok(expression::Tree { name: "tr", args: vec![internal_key] }); - } let (tree, rest) = expression::Tree::from_slice_delim(script, 1, '{')?; if rest.is_empty() { Ok(expression::Tree { name: "tr", args: vec![internal_key, tree] }) From 1259375d7b7c91053e09d1cbe3db983612fe301c Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 31 Aug 2024 19:29:01 +0000 Subject: [PATCH 3/3] miniscript: make display prefer 'u' over 'l' in the fragment l:0 When fuzzing my new non-recursive tree parsing logic, I noticed that we were deviating from the released 12.0 in the way that we display l:0. This is an ambiguous fragment which can be displayed either as l:0 or u:0. In our released code we use u:0 so stick with that. This was unintentially changed as part of #722. Change it back. While we are at it add a regression test for https://github.com/rust-bitcoin/rust-miniscript/pull/735 --- src/descriptor/mod.rs | 15 +++++++++++++++ src/miniscript/display.rs | 4 ++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index d6e706bb8..487bbcc45 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -1036,6 +1036,21 @@ mod tests { ); } + #[test] + fn display_prefers_u() { + // The fragments u:0 and l:0 are identical in terms of Script and + // in terms of the in-memory representation -- OrI(False, False). + // Test that the way we display the ambiguous fragment doesn't + // change, in case somebody somehow is depending on it. + let desc = StdDescriptor::from_str("sh(u:0)").unwrap(); + assert_eq!("sh(u:0)#ncq3yf9h", desc.to_string()); + + // This is a regression test for https://github.com/rust-bitcoin/rust-miniscript/pull/735 + // which was found at the same time. It's just a bug plain and simple. + let desc = StdDescriptor::from_str("sh(and_n(u:0,1))").unwrap(); + assert_eq!("sh(and_n(u:0,1))#5j5tw8nm", desc.to_string()); + } + #[test] fn desc_rtt_tests() { roundtrip_descriptor("c:pk_k()"); diff --git a/src/miniscript/display.rs b/src/miniscript/display.rs index 2a4e9f757..c6510b7d0 100644 --- a/src/miniscript/display.rs +++ b/src/miniscript/display.rs @@ -260,14 +260,14 @@ impl Terminal { Terminal::ZeroNotEqual(..) => "n", Terminal::AndV(_, ref r) if matches!(r.as_inner(), Terminal::True) => "t", Terminal::AndV(..) => "and_v", - Terminal::AndB(..) => "and_b", Terminal::AndOr(_, _, ref c) if matches!(c.as_inner(), Terminal::False) => "and_n", + Terminal::AndB(..) => "and_b", Terminal::AndOr(..) => "andor", Terminal::OrB(..) => "or_b", Terminal::OrD(..) => "or_d", Terminal::OrC(..) => "or_c", - Terminal::OrI(ref l, _) if matches!(l.as_inner(), Terminal::False) => "l", Terminal::OrI(_, ref r) if matches!(r.as_inner(), Terminal::False) => "u", + Terminal::OrI(ref l, _) if matches!(l.as_inner(), Terminal::False) => "l", Terminal::OrI(..) => "or_i", Terminal::Thresh(..) => "thresh", Terminal::Multi(..) => "multi",