Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic Miniscript support in output descriptors #16800

Closed
wants to merge 7 commits into from

Conversation

sipa
Copy link
Member

@sipa sipa commented Sep 4, 2019

This pull request introduces support for miniscript in Bitcoin Core.

The bulk of the code is in the 3 commits that add the miniscript module, including conversion from/to CScript, converting to and parsing from its engineer-readable string notation, property analysis and ops limit/stack size limit that are necessary to assess the security of arbitrary scripts.

A number of tests are included, including tests against known scripts, and against randomly generated scripts.

The final commit integrates the miniscript module into descriptors. This is only rudimentary, as it is not yet integrated in the signing code. I'm including it here to give something accessible to play with, but if desirable I can move that to a later PR as well.

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Sep 4, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17056 (descriptors: Introduce sortedmulti descriptor by achow101)
  • #16889 (Add some general std::vector utility functions by sipa)
  • #16887 (Abstract out some of the descriptor Span-parsing helpers by sipa)
  • #16807 (Let validateaddress locate error in Bech32 address by meshcollider)
  • #16710 (build: Enable -Wsuggest-override if available by hebasto)
  • #16440 (BIP-322: Generic signed message format by kallewoof)
  • #15590 (Descriptor: add GetAddressType() and IsSegWit() by Sjors)
  • #13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by l2a5b1)

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.

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Sep 4, 2019

src/script/miniscript.h Outdated Show resolved Hide resolved
src/test/miniscript_tests.cpp Outdated Show resolved Hide resolved
src/script/miniscript.h Outdated Show resolved Hide resolved
@sipa sipa force-pushed the 201908_miniscript branch 2 times, most recently from d0804c7 to 7912f91 Compare Sep 4, 2019
@sipa
Copy link
Member Author

@sipa sipa commented Sep 4, 2019

@practicalswift Thanks for reminding me, I had forgotten about those (and wasn't really looking at the miniscript repo while working on this PR). Specifically:

  • The overflow/underflow issue and the OOM issue don't apply here, as they're in the compiler code which I'm not PR'ing here.
  • The unhandled exception I've fixed independently because stoul and friends are locale dependent (both here and upstream).
  • The stack depth we need to fix still, I've commented on the issue.

Also, this PR should be perfectly in sync with the upstream repo, so if you want to PR fixes, they can go there preferably, and I'll incorporate them here.

@sipa sipa force-pushed the 201908_miniscript branch 5 times, most recently from 0e511de to 046a0b9 Compare Sep 4, 2019
src/script/miniscript.h Outdated Show resolved Hide resolved
sipa added 2 commits Sep 5, 2019
Added are:

* Vector(arg1,arg2,arg3,...) constructs a vector with the specified
  arguments as elements. The vector's type is derived from the
  arguments. If some of the arguments are rvalue references, they
  will be moved into place rather than copied (which can't be achieved
  using list initialization).

* Cat(vector1,vector2) returns a concatenation of the two vectors,
  efficiently moving elements when relevant.

Vector generalizes (and replaces) the Singleton function in
src/descriptor.cpp, and Cat replaces the Cat function in bech32.cpp
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Oct 11, 2019
… helpers

bb36372 test: add unit tests for Span-parsing helpers (Sebastian Falbesoner)
5e69aee Add documenting comments to spanparsing.h (Pieter Wuille)
230d43f Abstract out some of the descriptor Span-parsing helpers (Pieter Wuille)

Pull request description:

  As suggested here: bitcoin#16800 (comment).

  This moves the Span parsing functions out of the descriptor module, making them more easily usable for other parsers (in particular, in preparation for miniscript parsing).

ACKs for top commit:
  MarcoFalke:
    ACK bb36372

Tree-SHA512: b5c5c11a9bc3f0a1c2c4cfa22755654ecfb8d4b69da0dc1fb9f04e1556dc0f6ffd87ad153600963279ac465d587d7971b53d240ced802d12693682411ac73deb
MarcoFalke added a commit that referenced this issue Oct 18, 2019
7d8d3e6 Add tests for util/vector.h's Cat and Vector (Pieter Wuille)
e65e61c Add some general std::vector utility functions (Pieter Wuille)

Pull request description:

  This is another general improvement extracted from #16800 .

  Two functions are added are:

  * Vector(arg1,arg2,arg3,...) constructs a vector with the specified arguments as elements. The vector's type is derived from the arguments. If some of the arguments are rvalue references, they will be moved into place rather than copied (which can't be achieved using list initialization).
  * Cat(vector1,vector2) returns a concatenation of the two vectors, efficiently moving elements when relevant.

  Vector generalizes (and replaces) the `Singleton` function in src/descriptor.cpp, and `Cat` replaces the function in bech32.cpp

ACKs for top commit:
  laanwj:
    ACK 7d8d3e6
  MarcoFalke:
    ACK 7d8d3e6 (enjoyed reading the tests, but did not compile)

Tree-SHA512: 92325f14e90d7e7d9d920421979aec22bb0d730e0291362b4326cccc76f9c2d865bec33a797c5c0201773468c3773cb50ce52c8eee4c1ec1a4d10db5cf2b9d2a
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Oct 18, 2019
7d8d3e6 Add tests for util/vector.h's Cat and Vector (Pieter Wuille)
e65e61c Add some general std::vector utility functions (Pieter Wuille)

Pull request description:

  This is another general improvement extracted from bitcoin#16800 .

  Two functions are added are:

  * Vector(arg1,arg2,arg3,...) constructs a vector with the specified arguments as elements. The vector's type is derived from the arguments. If some of the arguments are rvalue references, they will be moved into place rather than copied (which can't be achieved using list initialization).
  * Cat(vector1,vector2) returns a concatenation of the two vectors, efficiently moving elements when relevant.

  Vector generalizes (and replaces) the `Singleton` function in src/descriptor.cpp, and `Cat` replaces the function in bech32.cpp

ACKs for top commit:
  laanwj:
    ACK 7d8d3e6
  MarcoFalke:
    ACK 7d8d3e6 (enjoyed reading the tests, but did not compile)

Tree-SHA512: 92325f14e90d7e7d9d920421979aec22bb0d730e0291362b4326cccc76f9c2d865bec33a797c5c0201773468c3773cb50ce52c8eee4c1ec1a4d10db5cf2b9d2a
@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Oct 27, 2019

@sipa Would you mind cherry-picking in the miniscript::FromScript(...) fuzzer from #17129 in to this PR? That would allow for closing #17129 which is entirely dependent on the merge of this one anyway :)

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Nov 12, 2019

@fanquake Could you add "Waiting for author"? :)

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Nov 12, 2019

"Needs rebase" implies "Waiting for author"

@achow101
Copy link
Member

@achow101 achow101 commented Feb 7, 2020

I've noticed that there are some Miniscript functions that may conflict in naming or functionality with existing output descriptors that may cause issues.

  • multi() and thresh_m() are functionally the same. So the easy thing to do would be to just make multi() an alias for thresh_m()
  • sortedmulti() currently has no Miniscript counterpart. But I think that is easily solved by having a sorted_thresh_m() which is semantically the same.
  • pkh() and pk_h() are very similar, both in naming and function. pkh() is the same as c:pk_h() so I think we can just call pkh() an alias for c:pk_h(). Since pkh() and pk_h() are named differently, the only other issue is that they may confuse people.
  • pk() in descriptors directly conflicts with pk() in Miniscript. pk() in descriptors is c:pk() in Miniscript. Because they are named exactly the same but have (slightly) different meanings, this can cause some issues.
    • One solution is to change the meaning of pk() depending on its location. If found at the top level or as the direct child of a sh() or wsh(), it would mean the current meaning today (to preserve compatibility). If found elsewhere, it would have the Miniscript meaning. But this solution is confusing as pk() has multiple meanings and it could be easy to mess up.
    • Alternatively we could deprecate and remove the usage of pk() and replace it with c:pk(). We could add c:pk() now (for 0.20) and have it mean the same thing as pk() now. Then once Miniscript is merged, remove pk()'s descriptor definition and just use the Miniscript definition. We could add warnings in some places as well to let people know that their usage of pk() is deprecated. The good thing about this is that pk() probably isn't currently being used so this probably won't effect a lot of people.

We will still need to preserve the current naming of functions as people may already be using them and changing these would break compatibility. At least most of these are straightforward aliases.

@bitcoin bitcoin deleted a comment from bee22193 Feb 7, 2020
@Sjors
Copy link
Member

@Sjors Sjors commented Feb 10, 2020

Concept ACK, after many hours of @apoelstra explaining Miniscript and its relation to Output Descriptors :-)

I like @achow101's suggestion:

  • Alternatively we could deprecate and remove the usage of pk() and replace it with c:pk(). We could add c:pk() now (for 0.20) and have it mean the same thing as pk() now.

Would be great to get a rebase of this before 0.20 splits off, so we can deprecate other Output Descriptor stuff early on, if needed.

@sipa
Copy link
Member Author

@sipa sipa commented Feb 12, 2020

After a number of discussions, I think this highlights the possibility to decrease the gap between Miniscript and non-Miniscript descriptors.

My suggestion is making the following changes to Miniscript:

  • Rename thresh_m to multi.
  • Rename pk to pk_k.
  • Add an alias pk(A)=c:pk_k(A) (just like and_n(A,B)=andor(A,B,0) for example).
  • Add an alias pkh(A)=c:pk_h(A).

That means that for users, there doesn't need to be a distinction between Miniscript or not, as pk(A) and wsh(pk(A)) would look uniform. Meanwhile, implementations could choose what to deal with at what layer (e.g. top-level pkh could be dealt with as a special case, or through a sufficiently generic Miniscript library).

I prefer this approach as at this stage I'm more comfortable with making some small changes to Miniscript, than to change descriptors in general.

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Feb 13, 2020

@sipa I would like to continue my robustness testing of this PR. Could you please rebase on master and perhaps also pull in my fix for the heap out-of-bounds read present in the current version of this PR: "Avoid heap out-of-bounds read in Node::CalcOps (test case: OP_0 OP_2 OP_EQUAL) and assertion failure in ComputeType (test case: OP_0 OP_0 OP_EQUAL)"? :)

@darosior
Copy link
Member

@darosior darosior commented Jul 24, 2020

Concept ACK

@michaelfolkson
Copy link
Contributor

@michaelfolkson michaelfolkson commented Jul 30, 2021

Scratching the surface on what would be needed to revitalize this PR. Obviously there is a mega rebase to do as it has been sitting here for a while but there have also been some changes since to the C++ implementation of Miniscript. Presumably it would make sense to work on the Core rebase first and then the Miniscript updates?

edit:

This version seems to deviate somewhat to the version in the upstream repo which makes it unclear to me which of the issues I found during my review that have been addressed:

Plus this. Not just Miniscript updates since this PR was opened but deviations before the updates.

@meshcollider
Copy link
Contributor

@meshcollider meshcollider commented Aug 22, 2021

(Just for the information of those wondering what's happening here, there are currently a few things being finished up in the miniscript repository before this PR is rebased, but it is a current WIP so expect something soon-ish!)

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Dec 22, 2021

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@Sjors
Copy link
Member

@Sjors Sjors commented Dec 28, 2021

It would be nice to have an up to date version here to compare with ElementsProject/libwally-core#310

@darosior
Copy link
Member

@darosior darosior commented Dec 28, 2021

@fanquake
Copy link
Member

@fanquake fanquake commented Jan 25, 2022

@sipa did you want to close this now that #24147 is open?

@sipa
Copy link
Member Author

@sipa sipa commented Jan 25, 2022

Superseded by #24147.

@sipa sipa closed this Jan 25, 2022
laanwj added a commit that referenced this issue Apr 5, 2022
2da94a4 fuzz: add a fuzz target for Miniscript decoding from Script (Antoine Poinsot)
f836999 Miniscript: ops limit and stack size computation (Pieter Wuille)
2e55e88 Miniscript: conversion from script (Pieter Wuille)
1ddaa66 Miniscript: type system, script creation, text notation, tests (Pieter Wuille)
4fe2936 script: expose getter for CScriptNum, add a BuildScript helper (Antoine Poinsot)
f4e289f script: move CheckMinimalPush from interpreter to script.h (Antoine Poinsot)
31ec6ae script: make IsPushdataOp non-static (Antoine Poinsot)

Pull request description:

  Miniscript is a language for writing (a subset of) Bitcoin Scripts in a structured way.

  Miniscript permits:
  - To safely extend the Output Descriptor language to many more scripting features thanks to the typing system (composition).
  - Statical analysis of spending conditions, maximum spending cost of each branch, security properties, third-party malleability.
  - General satisfaction of any correctly typed ("valid" [0]) Miniscript. The satisfaction itself is also analyzable.
  - To extend the possibilities of external signers, because of all of the above and since it carries enough metadata.

  Miniscript guarantees:
  - That for any statically-analyzed as "safe" [0] Script, a witness can be constructed in the bounds of the consensus and standardness rules (standardness complete).
  - That unless the conditions of the Miniscript are met, no witness can be created for the Script (consensus sound).
  - Third-party malleability protection for the satisfaction of a sane Miniscript, which is too complex to summarize here.

  For more details around Miniscript (including the specifications), please refer to the [website](https://bitcoin.sipa.be/miniscript/).

  Miniscript was designed by Pieter Wuille, Andrew Poelstra and Sanket Kanjalkar.
  This PR is an updated and rebased version of #16800. See [the commit history of the Miniscript repository](https://github.com/sipa/miniscript/commits/master) for details about the changes made since September 2019 (TL;DR: bugfixes, introduction of timelock conflicts in the type system, `pk()` and `pkh()` aliases, `thresh_m` renamed to `multi`, all recursive algorithms were made non-recursive).

  This PR is also the first in a series of 3:
  - The first one (here) integrates the backbone of Miniscript.
  - The second one (#24148) introduces support for Miniscript in Output Descriptors, allowing for watch-only support of Miniscript Descriptors in the wallet.
  - The third one (#24149) implements signing for these Miniscript Descriptors, using Miniscript's satisfaction algorithm.

  Note to reviewers:
  - Miniscript is currently defined only for P2WSH. No Taproot yet.
  - Miniscript is different from the policy language (a high-level logical representation of a spending policy). A policy->Miniscript compiler is not included here.
  - The fuzz target included here is more interestingly extended in the 3rd PR to check a script's satisfaction against `VerifyScript`. I think it could be further improved by having custom mutators as we now have for multisig (see #23105). A minified corpus of Miniscript Scripts is available at bitcoin-core/qa-assets#85.

  [0] We call "valid" any correctly-typed Miniscript. And "safe" any sane Miniscript, ie one whose satisfaction isn't malleable, which requires a key for any spending path, etc..

ACKs for top commit:
  jb55:
    ACK 2da94a4
  laanwj:
    Light code review ACK 2da94a4 (mostly reviewed the changes to the existing code and build system)

Tree-SHA512: d3ef558436cfcc699a50ad13caf1e776f7d0addddb433ee28ef38f66ea5c3e581382d8c748ccac9b51768e4b95712ed7a6112b0e3281a6551e0f325331de9167
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet