MiniTapscript: port Miniscript to Tapscript #27255

darosior · 2023-03-14T12:24:34Z

Miniscript was targeting P2WSH, and as such can currently only be used in wsh() descriptors. This pull request introduces support for Tapscript in Miniscript and makes Miniscript available inside tr() descriptors. It adds support for both watching and signing TapMiniscript descriptors.

The main changes to Miniscript for Tapscript are the following:

A new multi_a fragment is introduced with the same semantics as multi. Like in other descriptors multi and multi_a can exclusively be used in respectively P2WSH and Tapscript.
The d: fragment has the u property under Tapscript, since the MINIMALIF rule is now consensus. See also miniscript: the 'd:' wrapper must not be 'u' #24906.
Keys are now serialized as 32 bytes. (Note this affects the key hashes.)
The resource consumption checks and calculation changed. Some limits were lifted in Tapscript, and signatures are now 64 bytes long.

The largest amount of complexity probably lies in the last item. Scripts under Taproot can now run into the maximum stack size while executing a fragment. For instance if you've got a stack size of 999 due to the initial witness plus some execution that happened before and try to execute a hash256 it would DUP (increasing the stack size 1000), HASH160 and then push the hash on the stack making the script fail.
To make sure this does not happen on any of the spending paths of a sane Miniscript, we introduce a tracking of the maximum stack size during execution of a fragment. See the commits messages for details. Those commits were separated from the resource consumption change, and the fuzz target was tweaked to sometimes pad the witness so the script runs on the brink of the stack size limit to make sure the stack size was not underestimated.

Existing Miniscript unit, functional and fuzz tests are extended with Tapscript logic and test cases. Care was taken for seed stability in the fuzz targets where we cared more about them.

The design of Miniscript for Tapscript is the result of discussions between various people over the past year(s). To the extent of my knowledge at least Pieter Wuille, Sanket Kanjalkar, Andrew Poelstra and Andrew Chow contributed thoughts and ideas.

DrahtBot · 2023-03-14T12:24:37Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sipa, achow101
Concept ACK	michaelfolkson, josibake
Stale ACK	scgbckbone, bigspider

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28602 (descriptors: Disallow hybrid and uncompressed keys when inferring by achow101)
#28583 (refactor: [tidy] modernize-use-emplace by maflcko)
#28528 (test: Use test framework utils in functional tests by osagie98)
#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.

michaelfolkson · 2023-03-14T12:40:02Z

Concept ACK, an important piece in opening up Taproot scripting.

The design of Miniscript for Tapscript is the result of discussions between various people over the past year(s).

Your PR description is great but perhaps you can link to a few of those discussions for extra background reading for those that are interested? e.g. https://gist.github.com/sipa/06c5c844df155d4e5044c2c8cac9c05e

Also just checking if this PR should be in Draft or not? Are there ongoing open discussions on anything important or is this ready for extensive review?

darosior · 2023-03-14T14:31:10Z

Yeah, thanks for the suggestion. Reviewers may be interested in sipa/miniscript#134 as well.

Also just checking if this PR should be in Draft or not? Are there ongoing open discussions on anything important or is this ready for extensive review?

It is ready for (extensive or not) review. Otherwise i'd have opened it as a draft. But please tell me clearly if you disagree.

darosior · 2023-03-14T14:58:54Z

As it stands in this PR it would be possible to import a tr() descriptor unspendable by standardness: in addition to the script itself, we also need to check if the whole witness (including the control block, which is outside Miniscript..) isn't larger than the maximum standard transaction size.

EDIT: Actually since we are still bounded by the MAX_STACK_SIZE of 1000 and no element is larger than 65 bytes, i think we can do something more elegant than having to compute the maximum witness size of a script. Just have the following conservative bound: refuse any script whose size is larger than MAX_STANDARD_TX_WEIGHT - (1 + 65) * MAX_STACK_SIZE - TAPROOT_CONTROL_MAX_SIZE (i.e. 329871 bytes). This is still plenty, and should there ever be a usecase for a larger script we can always relax this check by having an accurate max witness size calculation.

src/script/miniscript.h

src/test/descriptor_tests.cpp

src/test/fuzz/miniscript.cpp

bigspider · 2023-03-15T15:25:05Z

src/script/miniscript.h

@@ -316,11 +316,22 @@ struct MaxInt {
        return a.value + b.value;
    }

+    friend MaxInt<I> operator-(const MaxInt<I>& a, const I& b) {
+        if (!a.valid) return {};
+        return a.value - b;


I suppose it never happens, but worth checking that a.value >= b?
Especially since it could be used with unsigned types.

Hmm that's a good point re unsigned type, but it needs to be able to return negative values. So i guess it's up to the caller to make sure they don't call this with b > a if a is unsigned, as with regular substractions?

darosior · 2023-03-16T17:04:31Z

In the last push i've:

Added a commit to avoid a stack overflow during the destruction of a too large Node, due to recursive calls in shared_ptr's destructor. (The reason the CI was failing under MSVC.)
Lowered the maximum size of a TapMiniscript to make sure a spending witness would never hit the maximum standard transaction size limit. (It's still pretty high.)
Addressed all comments from @bigspider.

This PR is now ready for another round of review.

darosior · 2023-03-17T10:31:02Z

Pushed to try to wake the CI up. Looks like it worked!

Sjors · 2023-03-17T12:40:32Z

Very cool. I also noticed bitcoin.sipa.be/miniscript has been updated.

Has there been any thought into how MuSig2 (and threshold friends) would fit into this in the future? I guess that's only a problem for the compiler, since for the purpose of script it doesn't matter if e.g. public key C is an aggregate of A + B. But it does change the meaning of something like tr(C,multi_a(2,A,B)) (i.e. regular tapscript multisig fallback if MuSig2 coordination fails - maybe not a good example, since this already works).

michaelfolkson · 2023-03-17T12:45:35Z

Has there been any thought into how MuSig2 (and threshold friends) would fit into this in the future?

Discussed in sipa's gist. Can comment on that gist @Sjors

josibake · 2023-03-27T15:30:34Z

Concept ACK

josibake

Left some style nits, nothing important. Might want to run clang-tidy, as it ended up reformatting a lot of the new code. Also got a few warnings, not quite sure about the FromPKBytes warning

Still trying to wrap my head around the logic; I'll follow up with a (hopefully) more helpful review

src/script/descriptor.cpp

src/script/miniscript.cpp

src/script/miniscript.h

src/script/miniscript.cpp

src/script/miniscript.h

src/script/miniscript.cpp

darosior · 2023-04-27T15:33:23Z

Reviewers, i've seen the comments i'm going to address them soon and rebase this.

I have discussed this with @sipa in person today. A couple notes from my recollection:

I introduce the diff (net difference in stack size before and after executing a fragment) and exec (maximum stack size reached during execution) here to account for the maximum stack size at all time. Pieter noticed we may be able to express the size in function of the diff. Or vice versa.
We tried to assert this property in the fuzz test.
We noticed or_i is the only fragment with two possible canonical dissatisfactions. If both dissatisfactions are available (only if both fragments are d) we choose the one with the largest stack size. But it seems unnecessary, the satisfier would always choose the smallest one. But the satisfier chooses in function of the actual witness size (not stack size) so it could be checking the dissatisfaction with the largest stack size.

We'll see if we can introduce some of this to the existing code and rebase this on top.

src/test/fuzz/miniscript.cpp

To avoid recursive calls in shared_ptr's destructor that could lead to a stack overflow.

Make sure we can spend a maximum-sized Miniscript under Tapscript context.

sipa · 2023-10-08T14:00:02Z

ACK ec0fc14

I have reviewed the code carefully, and contributed a new stack size limit checking implementation, while testing with fuzzing and the other included tests. I have not tested the feature manually. Also note that some of the code is mine, so my review does not cover that.

Unsure what @DrahtBot is smoking when claiming this needs rebase. It looks perfectly based to me.

achow101 · 2023-10-08T16:01:23Z

ACK ec0fc14

murchandamus · 2023-10-26T18:02:18Z

I looked over the release notes draft for 26.0 today. I think this was not mentioned. Should this have release notes?

sipa · 2023-10-26T18:07:12Z

@murchandamus Yes!

Currently, only the P2WSH context is supported. Miniscript for Taproot was not yet well specified at the time of writing. Currently, there is an open PR for Bitcoin Core at bitcoin/bitcoin#27255 which, when merged, can be taken as a reference. This implementation contains: - miniscript parser - type checker - malleability check - script length limit check (P2WSH standardness rule) - op count check (P2WSH consensus rule) - duplicate pubkey check - script generation - witness generation (satisfactions) - unit tests that create receive addresses based on some miniscripts and spend from them again (see `TestRedeem()`). Unit tests exist to check that this implementation's type checking passes some of the unit tests of rust-miniscript. See [./testdata](./testdata). Missing/todo: - timelock mixing check - stack size check (P2WSH standardness rule) - witness generation for thresh/multi currently has a naive implementation that is very inefficient (exponential runtime). This should be replaced with a fast algorithm. - maybe more Co-authored-by: Oliver Gugger <gugger@gmail.com>

darosior force-pushed the tapminiscript branch 2 times, most recently from 386c71c to 33f28a8 Compare March 14, 2023 14:09

DrahtBot mentioned this pull request Mar 14, 2023

descriptors: Be able to specify change and receiving in a single descriptor string #22838

Open