Miniscript support in Output Descriptors

darosior commented at 10:29 am on January 25, 2022: member

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 https://github.com/bitcoin-core/qa-assets/pull/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.

darosior force-pushed on Jan 25, 2022

DrahtBot added the label Build system on Jan 25, 2022

DrahtBot added the label Consensus on Jan 25, 2022

DrahtBot added the label Descriptors on Jan 25, 2022

darosior force-pushed on Jan 25, 2022

DrahtBot commented at 0:22 am on January 26, 2022: contributor

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.

DrahtBot added the label Needs rebase on Jan 26, 2022

laanwj removed the label Consensus on Jan 27, 2022

darosior force-pushed on Feb 11, 2022

darosior commented at 12:06 pm on February 11, 2022: member

Rebased.

DrahtBot removed the label Needs rebase on Feb 11, 2022

darosior force-pushed on Feb 21, 2022

DrahtBot added the label Needs rebase on Mar 4, 2022

darosior force-pushed on Mar 17, 2022

darosior commented at 5:09 pm on March 17, 2022: member

Rebased.

DrahtBot removed the label Needs rebase on Mar 17, 2022

darosior force-pushed on Mar 18, 2022

laanwj referenced this in commit d492dc1cda on Apr 5, 2022

darosior commented at 1:50 pm on April 5, 2022: member

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 commented at 12:37 pm on April 20, 2022: member

Rebased on top of #24860. I’ve made some cleanups and added more comments. I’ve also merged the unit tests of the Miniscript descriptors with the commit introducing the feature. I’ve changed the author of the commit to myself from Pieter as it was modified quite a lot from his original one and i don’t want to wrongly attribute him my own mistakes :p (of course, the co-author credits were switched accordingly).

darosior force-pushed on Apr 20, 2022

darosior commented at 1:05 pm on May 16, 2022: member

Hmm, so the use of uint32_t as key types makes the check for duplicate keys void. I need to look into it.

sipa commented at 1:19 pm on May 16, 2022: member

@darosior Perhaps add an KeyCompare(Key, Key) function to contexts, and use that as set ordering in the duplicate check, rather than Key::operator< ?

in src/script/miniscript.h:305 in 22b2171c48 outdated

297@@ -298,6 +298,8 @@ struct Node {
298     const Type typ;
299     //! Cached script length (computed by CalcScriptLen).
300     const size_t scriptlen;
301+    //! Whether a public key appears more than once in this node.
302+    const bool duplicate_key;

OliverOffing commented at 4:53 pm on May 25, 2022:

Naming could be improved. The name duplicate_key might suggest that the variable holds the value or reference of the key that is duplicated, when in fact it can only hold either true or false—and there could be multiple duplicated keys. It’d be best to name this variable has_duplicate_keys instead, which would go nicely along the naming scheme of IsNonMalleable and IsSane.

darosior commented at 10:56 am on May 26, 2022:

The name duplicate_key might suggest that the variable holds the value or reference of the key that is duplicated, when in fact it can only hold either true or false

I don’t think it’s worth making the variable name even longer. The bool type should be enough for one to get that this value holds, well, a boolean and not a reference to the key. Also note this commit is part of #24860 which features a ContainsDuplicateKey() public API in the same spirit as IsNonMalleable and IsSane.

and there could be multiple duplicated keys

The detection algorithm cuts through and wouldn’t know that, it just knows that “there exists one duplicate key”.

darosior commented at 11:10 am on May 26, 2022: member

Rebased on top of #24860 to integrate the IsSane renaming and key duplicate check using a KeyCompare method. I added a unit test for duplicate key check when parsing a Miniscript descriptor and took the opportunity to rework a bit the error messages to make them consistent (Now always “is not sane: [reason]”).

darosior force-pushed on May 26, 2022

fanquake referenced this in commit 695ca641a4 on Jun 4, 2022

DrahtBot added the label Needs rebase on Jun 4, 2022

sidhujag referenced this in commit da822e0083 on Jun 5, 2022

sipa commented at 2:49 pm on June 6, 2022: member

@darosior Needs rebase! (wheeee)

darosior force-pushed on Jun 6, 2022

darosior commented at 4:20 pm on June 6, 2022: member

Rebased. :)

DrahtBot removed the label Needs rebase on Jun 6, 2022

in src/script/descriptor.cpp:934 in 32eb838c00 outdated

929+class ScriptMaker {
930+    //! Keys contained in the Miniscript (the evaluation of DescriptorImpl::m_pubkey_args).
931+    const std::vector<CPubKey>& m_keys;
932+
933+public:
934+    ScriptMaker(const std::vector<CPubKey>& keys) : m_keys(keys) {}

sipa commented at 4:07 pm on June 8, 2022:

Add LIFETIMEBOUND attribute to keys?

in src/script/descriptor.cpp:958 in 32eb838c00 outdated

953+    const std::vector<std::unique_ptr<PubkeyProvider>>& m_pubkeys;
954+    //! Whether to serialize keys as private or public.
955+    bool m_private;
956+
957+public:
958+    StringMaker(const SigningProvider* arg, const std::vector<std::unique_ptr<PubkeyProvider>>& pubkeys, bool priv)

sipa commented at 4:08 pm on June 8, 2022:

Add LIFETIMEBOUND attribute to arg and to pubkeys?

in src/script/descriptor.cpp:1183 in 32eb838c00 outdated

1178+    //! List of keys contained in the Miniscript.
1179+    mutable std::vector<std::unique_ptr<PubkeyProvider>> m_keys;
1180+    //! Used to detect key parsing errors within a Miniscript.
1181+    mutable std::string m_key_parsing_error;
1182+
1183+    KeyParser(FlatSigningProvider* out, const SigningProvider* in) : m_out(out), m_in(in) {}

sipa commented at 4:15 pm on June 8, 2022:

Add LIFETIMEBOUND here to out and in as well?

in src/script/descriptor.cpp:1186 in 32eb838c00 outdated

1181+    mutable std::string m_key_parsing_error;
1182+
1183+    KeyParser(FlatSigningProvider* out, const SigningProvider* in) : m_out(out), m_in(in) {}
1184+
1185+    bool KeyCompare(const Key& a, const Key& b) const {
1186+        return m_keys[a]->ToString() < m_keys[b]->ToString();

sipa commented at 4:18 pm on June 8, 2022:

That sounds pretty slow. Perhaps it’s worth adding generic comparison operators to the PubkeyProvider classes? Could be left as a TODO.

darosior commented at 6:41 pm on June 9, 2022:

Yeah, it’s also slightly incorrect. For instance if a user -for whatever reason- has a descriptor with 2 xpubs with the same key but different chaincodes, this would not detect them as duplicates (same with same key but diff origins). I wanted to revisit that but it slipped my mind.

I added an operator< to PubkeyProvider:

 0diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
 1index 94c9071999..66c4e1cba9 100644
 2--- a/src/script/descriptor.cpp
 3+++ b/src/script/descriptor.cpp
 4@@ -162,6 +162,20 @@ public:
 5 
 6     virtual ~PubkeyProvider() = default;
 7 
 8+    /** Compare two public keys represented by this provider.
 9+     * Used by the Miniscript descriptors to check for duplicate keys in the script.
10+     */
11+    bool operator<(PubkeyProvider& other) const {
12+        CPubKey a, b;
13+        SigningProvider dummy;
14+        KeyOriginInfo dummy_info;
15+
16+        GetPubKey(0, dummy, a, dummy_info);
17+        other.GetPubKey(0, dummy, b, dummy_info);
18+
19+        return a < b;
20+    }
21+
22     /** Derive a public key.
23      *  read_cache is the cache to read keys from (if not nullptr)
24      *  write_cache is the cache to write keys to (if not nullptr)
25@@ -1183,7 +1197,7 @@ struct KeyParser {
26     KeyParser(FlatSigningProvider* out LIFETIMEBOUND, const SigningProvider* in LIFETIMEBOUND) : m_out(out), m_in(in) {}
27 
28     bool KeyCompare(const Key& a, const Key& b) const {
29-        return m_keys[a]->ToString() < m_keys[b]->ToString();
30+        return *m_keys[a] < *m_keys[b];
31     }
32 
33     template<typename I> std::optional<Key> FromString(I begin, I end) const

in src/script/descriptor.cpp:1193 in 32eb838c00 outdated

1188+
1189+    template<typename I> std::optional<Key> FromString(I begin, I end) const
1190+    {
1191+        assert(m_out);
1192+        Key key = m_keys.size();
1193+        auto pk = ParsePubkey(key, Span<const char>(&*begin, &*end), ParseScriptContext::P2WSH, *m_out, m_key_parsing_error);

sipa commented at 4:20 pm on June 8, 2022:

I think {&*begin, &*end} can be used instead of Span<const char>(&*begin, &*end).

in src/script/descriptor.cpp:1220 in 32eb838c00 outdated

1215+
1216+    template<typename I> std::optional<Key> FromPKHBytes(I begin, I end) const
1217+    {
1218+        assert(end - begin == 20);
1219+        assert(m_in);
1220+        uint160 hash(std::vector<unsigned char>(begin, end));

sipa commented at 4:21 pm on June 8, 2022:

Constructing a vector just to turn it into a uint160 sounds a bit inefficient. I think this works too:

0uint160 hash;
1std::copy(begin, end, hash.begin());

in src/script/descriptor.cpp:1471 in 32eb838c00 outdated

1449@@ -1279,6 +1450,38 @@ std::unique_ptr<DescriptorImpl> ParseScript(uint32_t& key_exp_index, Span<const
1450         error = "Can only have raw() at top level";
1451         return nullptr;
1452     }
1453+    // Process miniscript expressions.
1454+    {
1455+        KeyParser parser(&out, nullptr);
1456+        auto node = miniscript::FromString(std::string(expr.begin(), expr.end()), parser);

sipa commented at 4:23 pm on June 8, 2022:

Perhaps for a follow-up PR, but maybe we want to change the FromString methods (in miniscript code, but also descriptors in general) to take std::string_views? That would avoid intermediary string objects like here.

darosior commented at 6:45 pm on June 9, 2022:

Since it’s going to touch the Miniscript code again, i’d prefer to keep this as a follow-up if you don’t mind.

in src/script/descriptor.cpp:1475 in 32eb838c00 outdated

1470+                if (const auto str = insane_node->ToString(parser)) error = *str;
1471+                if (!insane_node->IsValid()) { error += " is invalid";}
1472+                else {
1473+                    error += " is not sane";
1474+                    if (!insane_node->IsNonMalleable()) error += ": malleable witnesses exist";
1475+                    else if (insane_node == node.get() && !insane_node->NeedsSignature()) error += ": witnesses without signature exist";

sipa commented at 4:24 pm on June 8, 2022:

Use braces and separate lines, unless for single-line “then” statements without “else"s.

darosior force-pushed on Jun 9, 2022

in src/script/descriptor.cpp:515 in e9cac52614 outdated

507@@ -493,12 +508,12 @@ class BIP32PubkeyProvider final : public PubkeyProvider
508 /** Base class for all Descriptor implementations. */
509 class DescriptorImpl : public Descriptor
510 {
511-    //! Public key arguments for this descriptor (size 1 for PK, PKH, WPKH; any size for Multisig).
512+protected:
513+    //! Public key arguments for this descriptor (size 1 for PK, PKH, WPKH; any size for WSH and Multisig).
514     const std::vector<std::unique_ptr<PubkeyProvider>> m_pubkey_args;
515     //! The string name of the descriptor function.
516     const std::string m_name;

achow101 commented at 8:14 pm on June 20, 2022:

In e9cac52614d11e30d1f6fec33efce3d00953bd70 “Miniscript support in output descriptors”

nit: m_name does not need to be protected.

darosior commented at 6:08 pm on June 21, 2022:

Yeah, but

 0diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
 1index 66c4e1cba9..b40e330f67 100644
 2--- a/src/script/descriptor.cpp
 3+++ b/src/script/descriptor.cpp
 4@@ -508,11 +508,12 @@ public:
 5 /** Base class for all Descriptor implementations. */
 6 class DescriptorImpl : public Descriptor
 7 {
 8+    //! The string name of the descriptor function.
 9+    const std::string m_name;
10+
11 protected:
12     //! Public key arguments for this descriptor (size 1 for PK, PKH, WPKH; any size for WSH and Multisig).
13     const std::vector<std::unique_ptr<PubkeyProvider>> m_pubkey_args;
14-    //! The string name of the descriptor function.
15-    const std::string m_name;
16 
17     //! The sub-descriptor arguments (empty for everything but SH and WSH).
18     //! In doc/descriptors.m this is referred to as SCRIPT expressions sh(SCRIPT)

Would make me reorder the initializer list of all classes heriting from DescriptorImpl, so i guess it’s not so bad if it’s just protected instead of private? Happy to change if you prefer, still. (This or doing protectred: then private: then protected: again.)

achow101 commented at 8:28 pm on June 20, 2022: member

4665124eca32c654df55fccf314f74d8fb9731be “miniscript: add a helper to find the deepest insane sub” does not seem to find the deepest, just an insane sub that has no children?

Why not have Key be PubkeyProvider so that each miniscript node can handle its own keys?

darosior commented at 5:53 pm on June 21, 2022: member

Thanks for the review @achow101.

Regarding FindInsaneSub, you are right. I should rename the commit. The intention here is to find the first insane sub that is closest to be a leaf.

Key can’t be a PubkeyProvider because we instanciate Keys. Neither can it be a unique_ptr since we may copy it. Following Pieter’s comment last month (https://github.com/bitcoin/bitcoin/pull/24148#issuecomment-1127664730) i tried making it a shared_ptr in order to avoid adding a KeyCompare requirement on all contexts. Unfortunately it quickly becomes unmanageable.

achow101 commented at 9:59 pm on June 23, 2022: member

ACK faa6f2176e487f7992557a1620923ebfaf60f432

darosior commented at 1:25 pm on July 4, 2022: member

@sipa: could you give this another look when you have a minute?

darosior commented at 8:21 am on July 5, 2022: member

I’ve got a decent fuzzing coverage of this PR with the corpus at https://github.com/bitcoin-core/qa-assets/pull/92 (using this branch along with #25540 to avoid memory blowups). A coverage report is available here.

Sjors commented at 1:45 pm on July 7, 2022: member

Can you add a test for FindInsaneSub? Right now its only function is to produce better error messages, but who knows what people do with it later… Though I guess such tests would much overlap with the test vectors in descriptor_tests.cpp.

Any particular reason why you’re moving InferPubkey and InferXOnlyPubkey (no problem with --color-moved=dimmed-zebra) in e9cac52614d11e30d1f6fec33efce3d00953bd70?

Nit: bool operator<(PubkeyProvider& other) and the new StringMaker class could go in their own commits (with tests, which also make them easier to review).

in src/script/descriptor.cpp:979 in e9cac52614 outdated

974+
975+    std::optional<std::string> ToString(uint32_t key) const
976+    {
977+        std::string ret;
978+        if (m_private) {
979+            if (!m_pubkeys[key]->ToPrivateString(*m_arg, ret)) return {};

Sjors commented at 3:30 pm on July 7, 2022:

What is supposed to happen here if we have a descriptor with 1 private key and otherwise only public keys?

darosior commented at 11:22 am on July 11, 2022:

The same as for existing descriptors, a failure:

 0diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
 1index a8c666079d..668cddfbba 100644
 2--- a/src/test/descriptor_tests.cpp
 3+++ b/src/test/descriptor_tests.cpp
 4@@ -488,6 +488,13 @@ Check("sh(wsh(multi(20,KzoAz5CanayRKex3fSLQ2BwJpN7U52gZvxMyk78nDMHuqrUxuSJy,KwGN
 5 
 6     // Miniscript tests
 7 
 8+    FlatSigningProvider prov;
 9+    std::string err, out;
10+    const auto multi_desc = Parse("wsh(multi(1,L4gM1FBdyHNpkzsFh9ipnofLhpZRp2mwobpeULy1a6dBTvw8Ywtd,032707170c71d8f75e4ca4e3fce870b9409dcaf12b051d3bcadff74747fa7619c0))", prov, err, false);
11+    assert(multi_desc && !multi_desc->ToPrivateString(prov, out));
12+    const auto ms_desc = Parse("wsh(or_i(pk(L4gM1FBdyHNpkzsFh9ipnofLhpZRp2mwobpeULy1a6dBTvw8Ywtd),pk(032707170c71d8f75e4ca4e3fce870b9409dcaf12b051d3bcadff74747fa7619c0)))", prov, err, false);
13+    assert(ms_desc && !ms_desc->ToPrivateString(prov, out));
14+
15     // Invalid checksum
16     CheckUnparsable("wsh(and_v(vc:andor(pk(L4gM1FBdyHNpkzsFh9ipnofLhpZRp2mwobpeULy1a6dBTvw8Ywtd),pk_k(Kx9HCDjGiwFcgVNhTrS5z5NeZdD6veeam61eDxLDCkGWujvL4Gnn),and_v(v:older(1),pk_k(L4o2kDvXXDRH2VS9uBnouScLduWt4dZnM25se7kvEjJeQ285en2A))),after(10)))#abcdef12", "wsh(and_v(vc:andor(pk(03cdabb7f2dce7bfbd8a0b9570c6fd1e712e5d64045e9d6b517b3d5072251dc204),pk_k(032707170c71d8f75e4ca4e3fce870b9409dcaf12b051d3bcadff74747fa7619c0),and_v(v:older(1),pk_k(02aa27e5eb2c185e87cd1dbc3e0efc9cb1175235e0259df1713424941c3cb40402))),after(10)))#abcdef12", "Provided checksum 'abcdef12' does not match computed checksum 'tyzp6a7p'");
17     // Only p2wsh context is valid

It is certainly suboptimal for certain usecases (like importing a partially private desc and calling listdescriptors true), but it’s the existing behaviour and i don’t think it should be fixed in this PR.

Sjors commented at 1:30 pm on July 13, 2022:

Indeed, something we can address later.

sipa commented at 1:39 pm on July 13, 2022:

See #24361

darosior force-pushed on Jul 11, 2022

darosior commented at 4:39 pm on July 11, 2022: member

Indeed a test for FindInsaneSub is redundant with the descriptor parsing failure test vectors. But it’s trivial so i added one. I also took this opportunity to rename the commit to not mention it’s looking for the “deepest” insane sub, as noted by @achow101.

InferPubkey is used in the KeyParser so it needs to be declared. I made InferXOnlyPubkey stick with the former for pure style/organisation reasons: it appeared to be “righter” and not harder to review (this just extends the moved patch from ~8 to ~16 lines which is trivially reviewable with --color-moved as you mentioned).

As for splitting the commit, i’d prefer to leave it like that if you don’t feel too strongly about it (and since you marked it as nit). I think it would just make the commits less atomic, and the StringMaker/ScriptMaker are tested through the MiniscriptDescriptor.

in src/script/descriptor.cpp:1200 in abd1c0444f outdated

1195+    mutable std::string m_key_parsing_error;
1196+
1197+    KeyParser(FlatSigningProvider* out LIFETIMEBOUND, const SigningProvider* in LIFETIMEBOUND) : m_out(out), m_in(in) {}
1198+
1199+    bool KeyCompare(const Key& a, const Key& b) const {
1200+        return *m_keys[a] < *m_keys[b];

Sjors commented at 3:20 pm on July 13, 2022:

Maybe use .at(a) here, as well as in ToString()

in src/script/descriptor.cpp:1486 in abd1c0444f outdated

1481+            if (!node->IsSane()) {
1482+                // Try to find the first insane sub for better error reporting.
1483+                auto insane_node = node.get();
1484+                if (const auto sub = node->FindInsaneSub()) insane_node = sub;
1485+                if (const auto str = insane_node->ToString(parser)) error = *str;
1486+                if (!insane_node->IsValid()) { error += " is invalid";}

Sjors commented at 3:30 pm on July 13, 2022:

0if (!insane_node->IsValid()) {
1  error += " is invalid";
2} else {

Sjors approved

Sjors commented at 3:35 pm on July 13, 2022: member

tACK 49ac002733157f5629303d1cf59fa3151fdf1e73

I was able to import a miniscript descriptor into a watch-only wallet and send some signet coins to it. I’ll continue testing after #24148 is rebased.

Nit: when you use an invalid miniscript fragment, like or_v(…) it says “A function is needed within P2WSH”. It might be more clear to use or_v(…) is invalid here.

bitcoin deleted a comment on Jul 13, 2022

achow101 commented at 8:00 pm on July 13, 2022: member

re-ACK 49ac002733157f5629303d1cf59fa3151fdf1e73

fanquake requested review from sipa on Jul 13, 2022

achow101 requested review from instagibbs on Jul 13, 2022

in test/functional/wallet_miniscript.py:72 in 49ac002733 outdated

67+        self.nodes[0].createwallet(
68+            wallet_name="ms_wo", descriptors=True, disable_private_keys=True
69+        )
70+        self.ms_wo_wallet = self.nodes[0].get_wallet_rpc("ms_wo")
71+
72+        # Sanity check we wouldn't let an insane Miniscript descriptor in

instagibbs commented at 8:30 pm on July 13, 2022:

need to define insane(I don’t know what that means)

darosior commented at 9:46 am on July 14, 2022:

It was discussed in the OP of the Miniscript PR, #24147, and is defined in src/script/miniscript.h: https://github.com/bitcoin/bitcoin/blob/062b9db0ccb6af8bfbaa2b29132408cda9991b40/src/script/miniscript.h#L842-L846

instagibbs commented at 1:22 pm on July 14, 2022:

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

in test/functional/wallet_miniscript.py:56 in 49ac002733 outdated

51+        assert_equal(
52+            self.ms_wo_wallet.getnewaddress(), self.funder.deriveaddresses(desc, 1)[1]
53+        )
54+
55+        self.log.info("Testing we detect funds sent to one of them")
56+        addr = self.ms_wo_wallet.getnewaddress()

instagibbs commented at 8:35 pm on July 13, 2022:

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)

in src/script/descriptor.cpp:1218 in 49ac002733 outdated

1213+    std::optional<std::string> ToString(const Key& key) const
1214+    {
1215+        return m_keys[key]->ToString();
1216+    }
1217+
1218+    template<typename I> std::optional<Key> FromPKBytes(I begin, I end) const

w0xlt commented at 1:14 am on July 14, 2022:

nit: Maybe is FromPublicKeyBytes() clearer ?

darosior commented at 10:10 am on July 14, 2022:

This interface was introduced in #24147, i’m not going to change it and all the callsites now. :)

in src/script/descriptor.cpp:1230 in 49ac002733 outdated

1225+            return key;
1226+        }
1227+        return {};
1228+    }
1229+
1230+    template<typename I> std::optional<Key> FromPKHBytes(I begin, I end) const

w0xlt commented at 1:14 am on July 14, 2022:

nit: Maybe is FromPublicKeyHashBytes() clearer ?

darosior commented at 10:11 am on July 14, 2022:

Same here

w0xlt approved

w0xlt commented at 1:24 am on July 14, 2022: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/24148/commits/49ac002733157f5629303d1cf59fa3151fdf1e73

The first commit (https://github.com/bitcoin/bitcoin/pull/24148/commits/d4b6f6842a5273afd1e7fe68d18d272566eb90c8) fails to run the miniscript unit test.

0$ git reset --hard d4b6f6842a5273afd1e7fe68d18d272566eb90c8
1$ make
2$ src/test/test_bitcoin --run_test=miniscript_tests 
3Running 1 test case...
4test/miniscript_tests.cpp(303): error: in "miniscript_tests/fixed_tests": check ms_ins && !ms_ins->IsSane() has failed
5unknown location(0): fatal error: in "miniscript_tests/fixed_tests": memory access violation at address: 0x00000038: no mapping at fault address
6test/miniscript_tests.cpp(303): last checkpoint

The second commit (https://github.com/bitcoin/bitcoin/pull/24148/commits/10b905fe98ccef3840e81664ea36fe11860f1d6d) fixes this error.

Maybe they might be squashed or the test file changed in the second.

miniscript: don't check for top level validity at parsing time

Letting the caller perform the checks allows for finer-grained error
reporting.

c38c7c5817

miniscript: add a helper to find the first insane sub with no child

This is helpful for finer grained descriptor parsing error: when there
are multiple errors to report in a Miniscript descriptor start with the
"smallest" fragments: the ones closer to be a leaf.

Co-Authored-By: Pieter Wuille <pieter@wuille.net>

d25d58bf5f

qa: better error reporting on descriptor parsing error

A nit, but was helpful when writing unit tests for Miniscript parsing

4a082887be

darosior force-pushed on Jul 14, 2022

Miniscript support in output descriptors

Miniscript descriptors are defined under P2WSH context (either `wsh()`
or `sh(wsh())`).
Only sane Miniscripts are accepted, as insane ones (although valid by
type) can have surprising behaviour with regard to malleability
guarantees and resources limitations.
As Miniscript descriptors are longer and more complex than "legacy"
descriptors, care was taken in error reporting to help a user determine
for what reason a provided Miniscript is insane.

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>

bfb036756a

qa: functional test Miniscript watchonly support ffc79b8e49

darosior force-pushed on Jul 14, 2022

darosior commented at 10:17 am on July 14, 2022: member

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):

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

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

Sjors commented at 12:10 pm on July 14, 2022: member

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 commented at 12:16 pm on July 14, 2022: member

re-utACK ffc79b8e492c6dd1352e528fd82e45d8d25eaa04

w0xlt approved

w0xlt commented at 12:16 pm on July 14, 2022: contributor

reACK https://github.com/bitcoin/bitcoin/pull/24148/commits/ffc79b8e492c6dd1352e528fd82e45d8d25eaa04

instagibbs approved

instagibbs commented at 1:29 pm on July 14, 2022: member

only reviewed the tests and interface, non blocking nits only

achow101 commented at 6:33 pm on July 14, 2022: member

ACK ffc79b8e492c6dd1352e528fd82e45d8d25eaa04

achow101 merged this on Jul 14, 2022

achow101 closed this on Jul 14, 2022

achow101 referenced this in commit 826fae6a0f on Jul 15, 2022

sanket1729 commented at 5:06 pm on July 18, 2022: contributor

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

sidhujag referenced this in commit 2a8811a52a on Jul 18, 2022

sidhujag referenced this in commit 2d6dd421d0 on Jul 18, 2022

Sjors commented at 6:23 pm on July 19, 2022: member

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?

in src/script/descriptor.cpp:1208 in bfb036756a outdated

1203+    template<typename I> std::optional<Key> FromString(I begin, I end) const
1204+    {
1205+        assert(m_out);
1206+        Key key = m_keys.size();
1207+        auto pk = ParsePubkey(key, {&*begin, &*end}, ParseScriptContext::P2WSH, *m_out, m_key_parsing_error);
1208+        if (!pk) return {};

mzumsande commented at 7:19 pm on July 19, 2022:

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

0script/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> > > >]’:
1script/descriptor.cpp:1243:17: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized]
2 1243 |         return {};
3      |                 ^
4script/descriptor.cpp: In function ‘miniscript::NodeRef<Key> miniscript::internal::Parse(Span<const char>, const Ctx&) [with Key = unsigned int; Ctx = {anonymous}::KeyParser]’:
5script/descriptor.cpp:1208:26: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized]
6 1208 |         if (!pk) return {};

sipa commented at 9:11 pm on July 19, 2022:

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

bitcoin deleted a comment on Jul 20, 2022

S3RK commented at 7:39 am on July 21, 2022: contributor

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.

sipa referenced this in commit 2256a8855b on Feb 16, 2023

bitcoin locked this on Sep 8, 2023

Miniscript support in Output Descriptors #24148

Conflicts