Signing support for Miniscript Descriptors

[darosior]

This makes the Miniscript descriptors solvable.

Note this introduces signing support for much more complex scripts than the wallet was previously able to solve, and the whole tooling isn't provided for a complete Miniscript integration in the wallet. Particularly, the PSBT<->Miniscript integration isn't entirely covered in this PR.

[DrahtBot]

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sipa, achow101
Stale ACK	instagibbs

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:

• #26756 (wallet: Replace GetBalance() logic with AvailableCoins() by w0xlt)
• #26627 (wallet: Migrate non-HD keys with single combo containing a list of keys by achow101)
• #26626 (descriptors: Add a KEY expression representing a list of individual keys by achow101)
• #26573 (Wallet: don't underestimate the fees when spending a Taproot output by darosior)
• #26567 (Wallet: estimate the size of signed inputs using descriptors by darosior)
• #26101 (script: create V1SigVersion for functions which should only accept taproot/tapscript by theuni)
• #26076 (Switch hardened derivation marker to h (in normalized descriptors and new wallets) by Sjors)
• #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.

[achow101]

Particularly, the PSBT<->Miniscript integration isn't part of this PR.

I don't think it would be particularly hard considering that you're already using SignatureData for non-wallet data lookups. I think all that would really need to be done is to update FillSignatureData to include the preimages from the PSBT.

Edit: This diff does the trick

diff --git a/src/psbt.cpp b/src/psbt.cpp
index 8248609ba6..9e09ccf15d 100644
--- a/src/psbt.cpp
+++ b/src/psbt.cpp
@@ -113,6 +113,18 @@ void PSBTInput::FillSignatureData(SignatureData& sigdata) const
     for (const auto& key_pair : hd_keypaths) {
         sigdata.misc_pubkeys.emplace(key_pair.first.GetID(), key_pair);
     }
+    for (const auto& [hash, preimage] : ripemd160_preimages) {
+        sigdata.hash_preimages.emplace(std::vector<unsigned char>(hash.begin(), hash.end()), preimage);
+    }
+    for (const auto& [hash, preimage] : sha256_preimages) {
+        sigdata.hash_preimages.emplace(std::vector<unsigned char>(hash.begin(), hash.end()), preimage);
+    }
+    for (const auto& [hash, preimage] : hash160_preimages) {
+        sigdata.hash_preimages.emplace(std::vector<unsigned char>(hash.begin(), hash.end()), preimage);
+    }
+    for (const auto& [hash, preimage] : hash256_preimages) {
+        sigdata.hash_preimages.emplace(std::vector<unsigned char>(hash.begin(), hash.end()), preimage);
+    }
 }
 
 void PSBTInput::FromSignatureData(const SignatureData& sigdata)

[darosior]

Rebased, addressed Andrew's comments, added some cleanups from @sipa, and added a miniscript_random fuzz target which generates a random Miniscript node based on a binary encoding. We might eventually add another one which tries to be smart, either here or in a follow-up.

I don't think it would be particularly hard considering that you're already using SignatureData for non-wallet data lookups. I think all that would really need to be done is to update FillSignatureData to include the preimages from the PSBT.

Yeah, but i figured it would be cleaner to have this as part of a PR which'd also update the various PSBT RPCs and test it using those.

[darosior]

@instagibbs thanks for the suggestions and the examples. This functional test was clearly lacking imagination. :) However what you suggest is already thoroughly tested by the unit tests (and to a lesser extent by the fuzz tests). But it's inexpensive to test also in the functional test so i included your examples and added a couple more things to test for each descriptor: the number of signatures expected (always equal to the number of private keys we have in the descriptor), whether this "completes" the transaction or not, and the expected witness stack size of the satisfaction (to assert we used the expected satisfaction path).

@sipa thanks, i think it'll be helpful to reviewers now and to future readers. I squashed Simplify InputStack constructor calls into the first commit of this PR. I cherry-picked all your commits adding comments and explanations about the satisfaction logic, that i squashed in a single documentation commit (now the second commit of this PR). I was going to say the two commits aligning the properties with the website belong in another PR.. But it's a tiny diff and the e property is related to signing, so i just cherry-picked them as well (squashed in one commit).

[instagibbs]

ACK 36478ac

reviewed tests only

[sipa]

utACK 6c7a17a (to the extent that it's not my own code).

[sipa]

Given the need for all these dummy SignatureData objects that need to be created in calls, would it make sense to add another overload of SignSignature that has no SignaturaData& argument?

[darosior]

Will do if i need to retouch.

[achow101]

ACK 6c7a17a

Reviewed code and non-fuzz tests.

[Sjors]

@darosior do I understand correctly that you need #26567 in order for PSBT stuff to work? Can I just combine both PR's to continue the test I was doing, or are there more changes required?

[darosior]

@Sjors see #24149 (comment). A minimal PSBT integration is part of this PR now. We treat Miniscript-related data when we are given a PSBT. However to fill Miniscript-related data to a PSBT from our wallet will need more work than just #26567, and also some design decisions to be taken.

[darosior]

Hmm looks like this checks for duplicate keys on every single node created whereas we could just check that on the top-level node in GenNode()?

[darosior]

#27117