Miniscript support in Output Descriptors

[darosior]

This adds Miniscript support for Output Descriptors without any signing logic (yet). See the OP of #24147 for a description of Miniscript and a rationale of having it in Bitcoin Core.
On its own, this PR adds "watchonly" support for Miniscript descriptors in the descriptor wallet. A follow-up adds signing support.

A minified corpus of Miniscript Descriptors for the descriptor_parse fuzz target is available at bitcoin-core/qa-assets#92.
The Miniscript descriptors used in the unit tests here and in #24149 were cross-tested against the Rust implementation at https://github.com/rust-bitcoin/rust-miniscript.

This PR contains code and insights from Pieter Wuille.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #25540 (miniscript: avoid wasteful computation, prevent memory blowup when fuzzing by darosior)
• #24361 (Descriptor unit tests and simplifications by sipa)
• #23480 (Add rawtr() descriptor for P2TR with specified (tweaked) output key by sipa)
• #22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[darosior]

Rebased.

[darosior]

Rebased.

[darosior]

I (slightly) messed up the last rebase here, but there was some updates in the meantime so i'll push a fix along with those soon.

[darosior]

Thanks @w0xlt, good catch: it's the unit test for FindInsaneSub i just added that i had only tested on the tip of the branch. I've swapped the two first commits now and they all individually compile and pass the tests.

@Sjors

interestingly even BOOST_CHECK(ms_ins); fails.

i was missing an a: wrapper for the second after, i've added a check that the Miniscript is valid (which does not change the test since an invalid sub is necessarily insane):

diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
index f5bea00e1e..bc5c49ef63 100644
--- a/src/test/miniscript_tests.cpp
+++ b/src/test/miniscript_tests.cpp
@@ -299,10 +299,10 @@ BOOST_AUTO_TEST_CASE(fixed_tests)
     // 2. It contains timelock mixes
     // We'll report the timelock mix error, as it's "deeper" (closer to be a leaf node) than the "no 's' property"
     // error is.
-    const auto ms_ins = miniscript::FromString("or_i(and_b(after(1),after(10000000)),pk(03cdabb7f2dce7bfbd8a0b9570c6fd1e712e5d64045e9d6b517b3d5072251dc204))", CONVERTER);
-    BOOST_CHECK(ms_ins && !ms_ins->IsSane());
+    const auto ms_ins = miniscript::FromString("or_i(and_b(after(1),a:after(1000000000)),pk(03cdabb7f2dce7bfbd8a0b9570c6fd1e712e5d64045e9d6b517b3d5072251dc204))", CONVERTER);
+    BOOST_CHECK(ms_ins && ms_ins->IsValid() && !ms_ins->IsSane());
     const auto insane_sub = ms_ins->FindInsaneSub();
-    BOOST_CHECK(insane_sub && *insane_sub->ToString(CONVERTER) == "and_b(after(1),after(10000000))");
+    BOOST_CHECK(insane_sub && *insane_sub->ToString(CONVERTER) == "and_b(after(1),a:after(1000000000))");
 
     // Timelock tests
     Test("after(100)", "?", TESTMODE_VALID | TESTMODE_NONMAL); // only heightlock

I also took the opportunity to fix the two nits @Sjors had previously.

[Sjors]

It's still a bit weird that a BOOST_CHECK doesn't just fail but hits a "no mapping at fault address" when the miniscript can't be parsed. You're following the same pattern as other tests though.

[Sjors]

re-utACK ffc79b8

[w0xlt]

reACK ffc79b8

[instagibbs]

only reviewed the tests and interface, non blocking nits only

[instagibbs]

imo makes more sense to do the rest of the test against an address we explicitly checked was derived correctly(instead of the 3rd one)

[instagibbs]

sure, what I mean is "assuming this is merged, as a reader of the test, what is this?" i.e. should include link or self contained definition

[achow101]

ACK ffc79b8

[sanket1729]

post-merge ACK. Did not review the tests. I like the error reporting by looking for inner insane sub.

[Sjors]

Opening a miniscript-descriptor wallet in an older version of Bitcoin Core causes a crash. (How) did we handle backward compatibility for tr() descriptors? Do we want to set a flag each time? Of have some generic mechanism to abort loading a wallet that has unrecognised descriptor / miniscript fragment?

[mzumsande]

I get two compiler warnings on master from this, gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0:

script/descriptor.cpp: In function ‘miniscript::NodeRef<Key> miniscript::internal::DecodeScript(I&, I, const Ctx&) [with Key = unsigned int; Ctx = {anonymous}::KeyParser; I = __gnu_cxx::__normal_iterator<std::pair<opcodetype, std::vector<unsigned char> >*, std::vector<std::pair<opcodetype, std::vector<unsigned char> > > >]’:
script/descriptor.cpp:1243:17: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 1243 |         return {};
      |                 ^
script/descriptor.cpp: In function ‘miniscript::NodeRef<Key> miniscript::internal::Parse(Span<const char>, const Ctx&) [with Key = unsigned int; Ctx = {anonymous}::KeyParser]’:
script/descriptor.cpp:1208:26: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 1208 |         if (!pk) return {};

[sipa]

I believe that's a known false positive in certain GCC versions.

[S3RK]

Opening a miniscript-descriptor wallet in an older version of Bitcoin Core causes a crash. (How) did we handle backward compatibility for tr() descriptors? Do we want to set a flag each time? Of have some generic mechanism to abort loading a wallet that has unrecognised descriptor / miniscript fragment?

Not sure how it was done with tr() descriptors. But before CWallet::SetMinVersion was used to handle wallet compatibility with various software versions.