Output descriptors/miniscript support

[jgriffiths]

Based on the excellent work of @k-matsuzawa in #202 (which this PR replaces).

The major changes from the original PR are:

• Move to a parse-first API to support multiple operations/queries without re-parsing.
• Use wally_map for variable substitutions.
• Use the new _n api call variants to work directly from the input string
• Remove all sub-allocations while parsing
• Remove script generation allocations completely in most cases, otherwise from 1MB to typically less than 1k.
• Expose descriptor canonicalization, network and features
• API support for future ranged descriptors
• Expose to SWIG targets
• Extend the test cases
• Various bug fixes, speedups, simplifications

@k-matsuzawa I would be very grateful if you could review when you have time.

Note: This is rebased on #365, and will be updated until that branch is merged to master. Only the last commit is specific to this PR.

[jgriffiths]

Note that the parsing code still allocates a large buffer for each child when generating scripts; I intend to fix this by computing the exact required length up-front in the future. Since this is an internal change, it won't affect the exposed API/ABI.

[darosior]

I still plan to review this. (Sorry for taking so long with #202..)

[k-matsuzawa]

Thanks! I reviewed it.
I commented on one thing. Other than that it looks good!

Note that the parsing code still allocates a large buffer for each child when generating scripts; I intend to fix this by computing the exact required length up-front in the future. Since this is an internal change, it won't affect the exposed API/ABI.

I'm sorry. 🙏

[jgriffiths]

I'm sorry.

The implementation is totally suitable for typical use, please don't apologise! My desire to reduce these allocations is only for the special case of using wally on constrained devices like Jade and the bitbox02, but I don't expect external contributors to have to optimise for that when submitting!

[Sjors]

Would it make sense to split out Miniscript parsing and do that later / separate? I thought there were still some pending changes to the Miniscript syntax to make it compatible with descriptors? bitcoin/bitcoin#16800 (comment)

[darosior]

The Miniscript syntax for P2WSH is now final. The pkh() and pk() aliases, as well as the rename of thresh_m() to multi() made the syntax of Miniscript descriptors backward compatible with "legacy" descriptors.

[jgriffiths]

Note: Moving back to WIP, I think we need a couple more features.

[LeoComandini]

utACK 99b25e0 (just the first commit "bip32: Allow public path derivation with hardened child elements"), with comment

[Sjors]

There's a couple of functions that I would find quite useful, for sanity checking a multisig wallet:

• infer the (sub)descriptor type(s): e.g. it's a witness script hash (KIND_DESCRIPTOR_SH) with a sorted multisig (KIND_DESCRIPTOR_MULTI_S )
• for multisig: get the threshold value
• extract xpub(s)
• know if it's ranged, and if not: which index
• infer the network (though you can't distinguish testnet, regtest and signet, and you can't tell from a hex public key, so maybe this is not realistic)

All of that currently happens internally in descriptor.c. Could wait for a followup.

[Sjors]

I was able to wrap two of the new functions and run some rudimentary sanity checks on them in Sjors/libwally-swift#57

[Sjors]

Similar to what I'm suggesting in bitcoin/bitcoin#24147 (comment), instead of introducing all of Miniscript you could start with the subset that's needed for current descriptor functionality: KIND_MINISCRIPT_PK(_K), KIND_MINISCRIPT_PK(H/_H), and KIND_MINISCRIPT_MULTI. You only need TYPE_B and TYPE_K and none of the PROP_ logic.

That allows you to introduce a bunch of plumbing, without the need for reviewers to understand all of Miniscript. And in combination with my suggestions above, it's already practically useful for building single- and simple multisig wallets in a way that's compatible with what's out there (Specter, ColdCard, etc).

[darosior]

For the Miniscript part, how about we look into making https://github.com/sipa/miniscript/ a library and exposing a C API?

[jgriffiths]

For the Miniscript part, how about we look into making https://github.com/sipa/miniscript/ a library and exposing a C API?

C++ code is not acceptable for wally, and our implementation requirements are stricter than some other implementations; because we need to work on embedded devices and hardware wallets we need to constrain both code size and stack/memory usage for exposed functionality.

[darosior]

Is it still WIP? What is left to be done there? Any way i can help this work to move forward?

[jgriffiths]

Is it still WIP? What is left to be done there? Any way i can help this work to move forward?

Hi @darosior sorry for the late reply. This PR is still WIP for 2 reasons:

1 - The interface currently only allows generating one script type; I'd like to extend this to be able to generate scriptsigs, scriptpubkeys and witness scripts (as well as tapscript in due course)
2 - The creation of scripts internally currently requires a lot of memory which needs fixing in order to work well on smaller devices like the Jade or Bitbox02

Item 1 is a pre-requisite for merging as it will change the API interface slightly. 2 is not blocker for merging as the internals can be refactored once the interface is stable.

I am currently working on PSBT v2/PSET support for wally, and hope to be able to re-visit this following that. Unfortunately I have rather extreme demands on my time at present.

[jgriffiths]

Hi @k-matsuzawa my apologies for taking so long to get back to this PR. I have updated it further, including changing the API, and hope to get this into the upcoming 0.8.8 release.

I hope you have have time to review this new version, and will switch to it in due course. In the event that you will remain with your own implementation for a time, please see the individual commits squashed for this PR at https://github.com/ElementsProject/libwally-core/tree/descriptor_backup_7_feb_23. There are some bug fixes there which you may wish to pick up.

Compared to the initial version, this now uses a parsed representation as the descriptor object rather than passing around the string. This is needed to prevent re-parsing overhead for multiple operations/queries on the descriptor. The large upfront memory allocation for script generation has been removed. In most cases no allocations are now needed to generate. When allocation is required, the length allocated is reduced to the maximum length needed to contain all sub-scripts, which is typically in the order of 1kb or so.

I have also merged in your branches extra fixes as you can see from the link above.

[k-matsuzawa]

@jgriffiths Thanks!
I was able to quickly check out the latest implementation and confirm that it works the same way. I will replace it when it is released 0.8.8!

[jgriffiths]

@k-matsuzawa thanks, looking forward to merging this shortly :)

[jgriffiths]

@Sjors FYI

[Sjors]

Nice! It looks like the first 5 commits could be split off, so review can focus on the actual descriptors / miniscript stuff?

[jgriffiths]

Nice! It looks like the first 5 commits could be split off, so review can focus on the actual descriptors / miniscript stuff?

Yes, sorry if it isn't clear, the first commits are a separate and will be merged to master shortly, I'm keeping this PR rebased over master for easier testing by reviewers.

[Sjors]

Can you update the PR description to point to #365? That seems to be where those commits are living.

[jgriffiths]

Can you update the PR description to point to #365?
Done, sorry for the confusion.

[Sjors]

I was able to rebase my LibWallySwift implementation on the latest changes. It uses only a small fraction of the functionality though.

Some features that are still on my wish list, but can wait for a followup:

1. Determine the descriptor type (e.g. sorted_multi()) and output type (legacy, p2sh segwit, native segwit, taproot)
2. Obtain the (extended) keys
3. Obtain the threshold for (sorted_multi()) and other such variables

[jgriffiths]

infer the network (though you can't distinguish testnet, regtest and signet, and you can't tell from a hex public key, so maybe this is not realistic)

@Sjors its turns out this is required in order to allow address generation from untyped expressions. Please see the latest commits around get_network/set_network for details.

[jgriffiths]

OK, I've squashed the recent commits in this branch and backed up the un-squashed one in the descriptor_backup tree for review/posterity. This includes a new change to make use of a parsed descriptor thread-safe.

That's all the time I have for this release. AFAICS the API is now sufficient to support future work like multi-path and @Sjors wish list items.

@Sjors Please raise a bug to track your wish list so they aren't lost when this is merged, thanks.