psbt: Taproot fields for PSBT

achow101 commented at 8:47 pm on July 26, 2021: member

Implements the Taproot fields for PSBT described in BIP 371.

DrahtBot added the label Descriptors on Jul 26, 2021

DrahtBot added the label RPC/REST/ZMQ on Jul 26, 2021

DrahtBot added the label Wallet on Jul 26, 2021

DrahtBot commented at 12:28 pm on July 27, 2021: 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:

#23578 (Add external signer taproot support by Sjors)
#23502 (wallet: Avoid underpaying transaction fees when signing taproot spends by achow101)
#21283 (Implement BIP 370 PSBTv2 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.

mjdietzx commented at 2:24 pm on September 1, 2021: contributor

Concept and Approach ACK. I was experimenting with a basic taproot n-of-m multisig using a single OP_CHECKSIGADD based script on top of this branch. Taproot fields in the PSBTs I was creating (and attempting to sign - although I haven’t got that working yet) looked good when I inspected them. I did some very light testing of this, mostly sanity checks

achow101 force-pushed on Sep 1, 2021

achow101 force-pushed on Sep 20, 2021

achow101 force-pushed on Sep 29, 2021

achow101 force-pushed on Nov 16, 2021

Sjors commented at 3:53 pm on November 22, 2021: member

Needs rebase due to silent merge conflict in descriptor.cpp.

achow101 force-pushed on Nov 22, 2021

achow101 force-pushed on Nov 25, 2021

in src/psbt.h:71 in 64dc0962d3 outdated

64@@ -56,6 +65,12 @@ struct PSBTInput
65     CScriptWitness final_script_witness;
66     std::map<CPubKey, KeyOriginInfo> hd_keypaths;
67     std::map<CKeyID, SigPair> partial_sigs;
68+    std::vector<unsigned char> m_tap_key_sig;
69+    std::map<std::pair<XOnlyPubKey, uint256>, std::vector<unsigned char>> m_tap_script_sigs;
70+    std::map<std::pair<CScript, int>, std::set<std::vector<unsigned char>, ShortestVectorFirstComparator>> m_tap_scripts;
71+    std::map<XOnlyPubKey, std::pair<std::set<uint256>, KeyOriginInfo>> m_tap_keypaths;

Sjors commented at 5:59 am on December 5, 2021:

keypath is ambiguous, maybe call this m_tap_bip32_paths?

Also, maybe add a comment above these new entries that refers to BIP 341.

achow101 commented at 8:40 pm on December 6, 2021:

Done

in src/script/sign.h:79 in 57622204c8 outdated

73@@ -74,6 +74,7 @@ struct SignatureData {
74     std::map<CKeyID, std::pair<CPubKey, KeyOriginInfo>> misc_pubkeys;
75     std::vector<unsigned char> taproot_key_path_sig; /// Schnorr signature for key path spending
76     std::map<std::pair<XOnlyPubKey, uint256>, std::vector<unsigned char>> taproot_script_sigs; ///< (Partial) schnorr signatures, indexed by XOnlyPubKey and leaf_hash.
77+    std::map<XOnlyPubKey, std::pair<std::set<uint256>, KeyOriginInfo>> taproot_misc_pubkeys;

Sjors commented at 6:17 am on December 5, 2021:

57622204c81628f790a5355d2f7197d8ed23ebc1 : worth documenting that “misc” may include the inner key (with an empty set of hashes).

achow101 commented at 8:40 pm on December 6, 2021:

Added some docs for this

in src/rpc/rawtransaction.cpp:1136 in 48d5949aa9 outdated

1080@@ -1081,6 +1081,43 @@ static RPCHelpMan decodepsbt()
1081                                 {
1082                                     {RPCResult::Type::STR_HEX, "", "hex-encoded witness data (if any)"},
1083                                 }},
1084+                                {RPCResult::Type::STR_HEX, "taproot_key_path_sig", /* optional */ true, "hex-encoded signature for the Taproot key path spend"},

Sjors commented at 6:35 am on December 5, 2021:

48d5949aa94410b12263df9b7a3cbc4d7cd50b6f: having a fully decoded example in a functional test case would be handy

achow101 commented at 7:42 pm on December 6, 2021:

Is there not? There are tests added which check for the presence of the taproot fields in psbts spending taproot inputs.

Sjors commented at 3:04 am on December 7, 2021:

I noticed the presence checking tests, though perhaps checking against the fully decoded result could reveal more subtle issues.

in src/psbt.cpp:371 in 833ecb8dec outdated

356@@ -357,10 +357,10 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction&
357     input.FromSignatureData(sigdata);
358 
359     // If we have a witness signature, put a witness UTXO.
360-    // TODO: For segwit v1, we should remove the non_witness_utxo
361     if (sigdata.witness) {
362         input.witness_utxo = utxo;
363-        // input.non_witness_utxo = nullptr;
364+        // We can remove the non_witness_utxo if and only if there are no non-segwit or segwit v0
365+        // inputs in this transaction, so we can deal with this later.

Sjors commented at 6:39 am on December 5, 2021:

833ecb8dec1a10e3bc1b8d5bcaf0b6839277f582 “later” -> “in FillPSBT”?

achow101 commented at 8:40 pm on December 6, 2021:

Done

in src/wallet/wallet.cpp:1911 in 833ecb8dec outdated

1893@@ -1894,6 +1894,30 @@ TransactionError CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bool& comp
1894         }
1895     }
1896 
1897+    // Figure out if any witness_utxos should be dropped

Sjors commented at 6:42 am on December 5, 2021:

833ecb8dec1a10e3bc1b8d5bcaf0b6839277f582: non_witness_utxos (also below)

Don’t do this for SIGHASH_SINGLE and SIGNHASH_ANYONECANPAY, since new non-taproot inputs might be added later? Or should whoever adds such an input just add them back in? Maybe add a functional test for this.

achow101 commented at 8:41 pm on December 6, 2021:

It only matters for SIGHASH_ANYONECANPAY. I’ve added a check for that.

in src/wallet/wallet.cpp:1927 in 833ecb8dec outdated

1908+        if (wit_ver == 0) {
1909+            // Segwit v0, so we cannot drop any witness_utxos
1910+            to_drop.clear();
1911+            break;
1912+        }
1913+        to_drop.push_back(i);

Sjors commented at 6:49 am on December 5, 2021:

833ecb8dec1a10e3bc1b8d5bcaf0b6839277f582: maybe only do this if there actually is a non_witness_utxo? Otherwise you might as well use a go_drop bool and loop over all inputs after this loop. Then you also wouldn’t need to track the index.

achow101 commented at 8:41 pm on December 6, 2021:

Done

in src/script/sign.cpp:153 in 9b86c58d95 outdated

148@@ -149,6 +149,7 @@ static bool CreateTaprootScriptSig(const BaseSignatureCreator& creator, Signatur
149     auto it = sigdata.taproot_script_sigs.find(lookup_key);
150     if (it != sigdata.taproot_script_sigs.end()) {
151         sig_out = it->second;
152+        return true;

Sjors commented at 6:52 am on December 5, 2021:

9b86c58d95539056981c2a04a7345b992b1ce70c: this seems like a bug fix, is there a test you could add that demonstrates this?

achow101 commented at 7:59 pm on December 6, 2021:

The existing tests already cover this case (try reverting this commit, the tests will fail).

Sjors commented at 3:41 am on December 7, 2021:

On your latest rebase f7b61d2, when I revert 9b86c58d95539056981c2a04a7345b992b1ce70c, then indeed wallet_taproot.py fails. Still, having the fix and test in the same commit would be much easier to folllow. Moving this bit from the test over to this commit does the trick:

0-            res = self.psbt_offline.walletprocesspsbt(psbt)
1-            assert(res['complete'])
2+            res = self.psbt_offline.walletprocesspsbt(psbt=psbt, finalize=False)
3+
4+            // Check that pre-existing signatures in CreateTaprootScriptSig are actually used,
5+            // if a signature is found for the given key and leaf hash.
6+            decoded = self.psbt_offline.decodepsbt(res["psbt"])

achow101 commented at 4:38 am on December 7, 2021:

I’ve moved the change that causes the test failure into the previous commit.

The test isn’t for this bug specifically, so I don’t think it needs any special mention. The test was written first, and it just so happens that it triggered this bug.

Sjors commented at 6:54 am on December 5, 2021: member

Mostly happy with 58af8917bd80b0a918df1a5d38c4b63beae5306f

achow101 force-pushed on Dec 5, 2021

sipa commented at 7:46 pm on December 6, 2021: member

In order to make this more testable, would it make sense to add support for OP_CHECKSIGADD-based multisig to descriptors first?

achow101 commented at 7:58 pm on December 6, 2021: member

In order to make this more testable, would it make sense to add support for OP_CHECKSIGADD-based multisig to descriptors first?

I think it is fine without those.

achow101 force-pushed on Dec 6, 2021

in src/script/sign.h:68 in 11c1e17fd6 outdated

64@@ -65,6 +65,7 @@ typedef std::pair<CPubKey, std::vector<unsigned char>> SigPair;
65 struct SignatureData {
66     bool complete = false; ///< Stores whether the scriptSig and scriptWitness are complete
67     bool witness = false; ///< Stores whether the input this SigData corresponds to is a witness input
68+    TxoutType spk_type = TxoutType::NONSTANDARD; ///< Stores the type of scriptPubKey the input spends

Sjors commented at 3:59 am on December 7, 2021:

11c1e17fd622e962d9d3987322a2b51b3181a1db : this is set, but otherwise unused.

achow101 commented at 4:38 am on December 7, 2021:

Removed

Sjors commented at 4:00 am on December 7, 2021: member

utACK bd75271a53054bb1bc7d880d5fbf667be4ab5e41

Having a (draft) PR on top of this that implements OP_CHECKSIGADD is certainly not harmful.

achow101 force-pushed on Dec 7, 2021

achow101 force-pushed on Dec 8, 2021

Sjors commented at 3:10 am on December 9, 2021: member

re-utACK 57d354a0d7ba99478eab1afc4304202a07ef4bf7

achow101 marked this as ready for review on Dec 10, 2021

DrahtBot added the label Needs rebase on Dec 10, 2021

achow101 force-pushed on Dec 11, 2021

DrahtBot removed the label Needs rebase on Dec 11, 2021

Sjors commented at 7:17 am on December 13, 2021: member

re-utACK 8a675a00053db4621f7f27cf22e8d65713dff03b

DrahtBot added the label Needs rebase on Dec 13, 2021

achow101 force-pushed on Dec 13, 2021

DrahtBot removed the label Needs rebase on Dec 13, 2021

laanwj added this to the "Blockers" column in a project

michaelfolkson commented at 11:12 am on February 1, 2022: contributor

Concept ACK, Approach ACK.

Will hopefully find the time to test with a hardware wallet, HWI (single sig PSBT as Taproot multisig descriptor not yet merged) and do a code review/comparison with the BIP.

Would certainly be nice to get this in and PR author has added high priority label. External projects are waiting on it.

edit: What hardware wallets can I test this with? Testnet/Signet coins and new PSBT fields? Just Coldcard?

achow101 referenced this in commit 985ec9705c on Feb 1, 2022

prusnak commented at 3:21 pm on February 19, 2022: contributor

edit: What hardware wallets can I test this with? @michaelfolkson Trezor supports Taproot with HWI (master).

prusnak commented at 3:22 pm on February 19, 2022: contributor

Shall we try to merge this in time for 23.0? @achow101 @laanwj @MarcoFalke

MarcoFalke commented at 3:47 pm on February 19, 2022: member

This can’t be merged because it needs rebase for several weeks due to a silent conflict

michaelfolkson commented at 4:14 pm on February 19, 2022: contributor

Don’t know anything about the silent conflict but we are past the feature freeze of February 15th too. Hopefully we can get a bunch of new Taproot stuff (e.g. #24043) in 24.0.

prusnak commented at 4:25 pm on February 19, 2022: contributor

we are past the feature freeze of February 15th too.

ACK

achow101 force-pushed on Feb 19, 2022

Sjors commented at 2:59 pm on February 21, 2022: member

This also needs review from more people (I’ll re-ACK the rebase once those arrive).

prusnak commented at 3:04 pm on February 21, 2022: contributor

Hopefully we can get a bunch of new Taproot stuff (e.g. #24043) in 24.0.

Let’s add this PR (#22558) and PR #24043 to the 24.0 Milestone, so they receive enough eyes?

gruve-p commented at 3:11 pm on February 21, 2022: contributor

Approach ACK

achow101 added this to the milestone 24.0 on Feb 22, 2022

in src/psbt.h:581 in b704884935 outdated

577+                        throw std::ios_base::failure("Input Taproot key signature key is more than one byte type");
578+                    }
579+                    s >> m_tap_key_sig;
580+                    if (m_tap_key_sig.size() < 64) {
581+                        throw std::ios_base::failure("Input Taproot key path signature is shorter than 64 bytes");
582+                    } else if (m_tap_key_sig.size() > 65) {

Empact commented at 3:37 pm on March 2, 2022:

nit: would these sizes be useful extracted as constants?

laanwj commented at 2:24 pm on June 22, 2022:

Agree, there’s quite some magic values here (also repeated), having constants makes it easier to review. (or maybe expanding it, like 1 + 32 + 32, as it’s mentioned in the BIP) (oh, nm, that’s for PSBT_IN_TAP_SCRIPT_SIG’s keydata, not here)

in src/psbt.h:850 in b704884935 outdated

844+                    } else if (key.size() != 1) {
845+                        throw std::ios_base::failure("Output Taproot internal key key is more than one byte type");
846+                    }
847+                    UnserializeFromVector(s, m_tap_internal_key);
848+                    break;
849+                }

Empact commented at 3:38 pm on March 2, 2022:

nit: some of these blocks are not strictly necessary, e.g. in this case.

laanwj commented at 2:53 pm on June 22, 2022:

Why this block isn’t necessary?

jb55 commented at 7:33 pm on March 3, 2022: contributor

Concept ACK. Testing this now because HWI isn’t signing one of my taproot inputs :(

sipa commented at 7:52 pm on March 3, 2022: member

@achow101 I wonder if you’re still open to modifications of BIP371.

Thinking about later integration with MuSig(2), I think it’s somewhat unfortunate that the currently suggested PSBT_IN_TAP_BIP32_DERIVATION field conveys both (a) which keys are participating in which leaves and (b) how those are derived. Post-MuSig2, I expect that often keys will be present that aren’t directly BIP32-derived, but are a MuSig* aggregate of BIP32-derived keys. It’s not impossible to integrate that on top here, but it’d involve some duplication.

My suggestion would be to have these fields:

PSBT_IN_TAP_KEY_SIG: unmodified
PSBT_IN_TAP_SCRIPT_SIG: unmodified
PSBT_IN_TAP_LEAF_SCRIPT: unmodified
PSBT_IN_TAP_KEY_TWEAK: concatenation of PSBT_IN_TAP_INTERNAL_KEY and PSBT_IN_TAP_MERKLE_ROOT; it conveys “the output key is a tweak of this key with this Merkle root”.
PSBT_IN_TAP_KEY_IN_SCRIPT: like PSBT_IN_TAP_BIP32_DERIVATION now, but without the BIP32 path; it just conveys “this key occurs in these leaves”.
PSBT_IN_TAP_BIP32_DERIVATION: like PSBT_IN_TAP_BIP32_DERIVATION, but without the list of leaf hashes. It can be used for (a) the output key directly (b) the internal key (provided in PSBT_IN_TAP_KEY_TWEAK) of (c) a key occurring in a leaf script (provided by PSBT_IN_TAP_KEY_IN_SCRIPT).

I’m also happy to take this to the ML if you think this deserves more discussion.

Ping @sanket1729 @darosior @jonasnick.

achow101 commented at 8:01 pm on March 3, 2022: member

I wonder if you’re still open to modifications of BIP371.

BIP 371 is already in use in production, notably by Ledger. So I think it might be too late for any modifications.

cc @bigspider

sipa commented at 8:26 pm on March 3, 2022: member

Ok, no reason to change things in that case.

jb55 commented at 9:39 pm on March 3, 2022: contributor

tACK b704884935766748cf533577c1babacfb6d4b5a5

Was able to get a mixed taproot keypath + wpkh spend working on HWI with this PR

Haven’t looked through the code that much, only tested behavior and I was happy with the result.

bigspider commented at 0:20 am on March 4, 2022: none

Thanks @achow101 for the ping. At this time, since we only support BIP-86 addresses, the only taproot field we rely on is PSBT_IN_TAP_BIP32_DERIVATION, where we assume that the list of hashes is indeed empty (so the expected value is a single byte 0x00, followed by 4 bytes for the fingerprint, followed by the derivation steps.

prusnak commented at 7:59 am on March 4, 2022: contributor

where we assume that the list of hashes is indeed empty

So it seems that @sipa’s suggestion can be implemented in a compatible way that does not break your usage of PSBT_IN_TAP_BIP32_DERIVATION

DrahtBot added the label Needs rebase on Mar 4, 2022

bigspider commented at 3:28 pm on March 4, 2022: none

Leaving the 0x00 would not break anything as I understand it. If desired in order to have a clean standard, I suppose we can have a reasonable upgrade path by letting the next version of the app to skip the 0x00 if present and keeping this loose check for some time.

achow101 force-pushed on Mar 4, 2022

DrahtBot removed the label Needs rebase on Mar 4, 2022

sanket1729 commented at 12:24 pm on March 7, 2022: contributor

… I expect that often keys will be present that aren’t directly BIP32-derived, but are a MuSig* aggregate of BIP32-derived keys

I was thinking most keys would be BIP32 derived keys of a Musig2 aggregate like musig2(A,B,C)/* or BIP32 derived keys of a Musig2 aggregate of BIP32-derived keys, like musig2(A/*, B/*, C/*)/*? The case you mention seems like musgi2(A/*, B/*, C/*)

michaelfolkson commented at 12:43 pm on March 7, 2022: contributor

@achow101 I wonder if you’re still open to modifications of BIP371.

Seems like we’re in multi threaded conversation territory here with discussion of possible BIP 371 changes and review of this PR as is. I’ve opened an issue (#24492) to discuss possible BIP 371 changes in case that helps.

I’m also happy to take this to the ML if you think this deserves more discussion.

Or alternatively I’ll close the issue and possible BIP 371 changes can be taken to the mailing list. I definitely think an email to the mailing list is a good idea once those proposed changes have been solidified.

michaelfolkson commented at 7:38 pm on March 11, 2022: contributor

Seems like we’re in multi threaded conversation territory here with discussion of possible BIP 371 changes and review of this PR as is. I’ve opened an issue (https://github.com/bitcoin/bitcoin/issues/24492) to discuss possible BIP 371 changes in case that helps. @sipa’s proposed changes won’t be made to BIP 371 due to uncertainty over whether other projects have already deployed. Closed the issue.

Sjors commented at 10:10 am on March 16, 2022: member

re-utACK 28868805bbf263d75cbd02eb7d510f1e343b979f

guggero commented at 1:57 pm on April 30, 2022: contributor

I’m currently testing this PR with/against our PSBT implementation in lnd. I was able to get a BIP86 key spend working :tada:

But I can’t seem to get bitcoind to recognize and sign for a key spend path with a merkle root provided. Perhaps that’s not the scope for this PR? If so then I apologize.

According to my question and the answer I got here, it should be enough to set the following fields:

 0    {
 1      "witness_utxo": {
 2        "amount": 4.00000000,
 3        "scriptPubKey": {
 4          "asm": "1 5a7730b9788468ac1b90baac80005da4965496e4d88bcbac95bd3c83cd906f3a",
 5          "desc": "addr(bcrt1ptfmnpwtcs352cxush2kgqqza5jt9f9hymz9uhty4h57g8nvsduaq6s9ypp)#q562pd9u",
 6          "hex": "51205a7730b9788468ac1b90baac80005da4965496e4d88bcbac95bd3c83cd906f3a",
 7          "address": "bcrt1ptfmnpwtcs352cxush2kgqqza5jt9f9hymz9uhty4h57g8nvsduaq6s9ypp",
 8          "type": "witness_v1_taproot"
 9        }
10      },
11      "taproot_bip32_derivs": [
12        {
13          "pubkey": "63cc52f06e0a91e3e6456631aa070221d58ccd1ed8c5b6639d4f88786dcd500f",
14          "master_fingerprint": "00000000",
15          "path": "m/86'/0'/0'/0/0",
16          "leaf_hashes": [
17          ]
18        }
19      ],
20      "taproot_internal_key": "63cc52f06e0a91e3e6456631aa070221d58ccd1ed8c5b6639d4f88786dcd500f",
21      "taproot_merkle_root": "6c2e4bb01e316abaaee288d69c06cc608cedefd6e1a06813786c4ec51b6e1d38"
22    }

Full PSBT:

0cHNidP8BAHECAAAAAT/GF0O3okL4iQV80uYewSbhqa7qWoD2nG489WsPxw9xAAAAAAD/////AoCT3BQAAAAAFgAUKpx5R067Qv191nSkawzjIGJAQ+ri6PoCAAAAABYAFIyIwvqKctH/eWgjlieETO9w55xoAAAAAAABASsAhNcXAAAAACJRIFp3MLl4hGisG5C6rIAAXaSWVJbk2IvLrJW9PIPNkG86IRZjzFLwbgqR4+ZFZjGqBwIh1YzNHtjFtmOdT4h4bc1QDxkAAAAAAFYAAIAAAACAAAAAgAAAAAAAAAAAARcgY8xS8G4KkePmRWYxqgcCIdWMzR7YxbZjnU+IeG3NUA8BGCBsLkuwHjFquq7iiNacBsxgjO3v1uGgaBN4bE7FG24dOAAAIgICZ5Cr1jOOzCBlO+GZ+Y3VSYA26Z/kFrcSYqzM00TKPjQY1G/d61QAAIABAACAAAAAgAEAAAADAAAAAA==

But when I use bitcoin-cli walletprocesspsbt with that PSBT (the private key is definitely in the wallet, it’s the same key I used for the previous BIP86 key spend), then I just get the same PSBT and "complete": false back.

Is this me doing something wrong or is this out of scope for this PR?

achow101 commented at 3:58 pm on April 30, 2022: member

@guggero Are you using a legacy or a descriptor wallet? Is the taproot output script being watched by the wallet (i.e. you imported the descriptor for it)?

guggero commented at 8:30 am on May 2, 2022: contributor

It’s a descriptor wallet but I didn’t add that specific output descriptor to it. It’s the same internal key as was used in the BIP86 test, so the key is there. I assumed the PSBT would be sufficient as the “assembly instruction” for the signature, since the private key is known to the wallet. But I guess I have to learn more about how descriptor wallets work in this context. I’ll try again with the correct descriptor imported and watched by the wallet, that will very likely resolve my issue. Thanks!

Sjors commented at 11:02 am on May 2, 2022: member

since the private key is known to the wallet.

Afaik we don’t sign anything unless we explicitly watch the scriptPubKey, which (for descriptor wallets) is only the case if a descriptor has been imported. In addition, that descriptor must have the private key: a descriptor with just the xpub won’t work even if we have the corresponding seed. See also #23417 and #23544.

achow101 commented at 2:19 pm on May 2, 2022: member

Afaik we don’t sign anything unless we explicitly watch the scriptPubKey, which (for descriptor wallets) is only the case if a descriptor has been imported.

No, descriptor wallets will sign if it sees any keys that it owns in the BIP32 derivation paths. I just forgot to update that to search the taproot BIP32 derivation paths too.

achow101 commented at 3:32 pm on May 2, 2022: member

@guggero I’ve pushed some commits that should fix that problem. Can you try again?

guggero commented at 12:44 pm on May 3, 2022: contributor

Thank you very much, this fixed the “key spend with script root” test case. Is script path signing expected to work as well?

I tried it and got a wrong result. But again, this could be due to me still using the internal and script leaf key from the BIP86 descriptor. But I guess the result is still kind of unexpected. Since I supplied a leaf hash in the BIP32 derivation info and a script with a control block, I don’t expect a signature to be put into the final script witness:

0$ bitcoin-cli walletprocesspsbt cHNidP8BAFICAAAAAV0virKr77xOIRw1FU3mHJUQ6oO7UmoEgIUUsED3ZRiuAQAAAAD/////AYCy5g4AAAAAFgAUy79OrU4hd2M3xvJMufO1pLdEWcUAAAAAAAEBKwCj4REAAAAAIlEgLWIhk/X61lqdbrKkZqR9AsFvT4JFt4lmaYWbtsDTy3ciFcBjzFLwbgqR4+ZFZjGqBwIh1YzNHtjFtmOdT4h4bc1QDyMgY8xS8G4KkePmRWYxqgcCIdWMzR7YxbZjnU+IeG3NUA+swCEWY8xS8G4KkePmRWYxqgcCIdWMzR7YxbZjnU+IeG3NUA85ARuXHYQK/Sy6fb774buzt5a3DU2Va/o8Pz27k/FkM3v/AAAAAFYAAIAAAACAAAAAgAAAAAAAAAAAARcgY8xS8G4KkePmRWYxqgcCIdWMzR7YxbZjnU+IeG3NUA8BGCAblx2ECv0sun2+++G7s7eWtw1NlWv6PD89u5PxZDN7/wAA
1{
2  "psbt": "cHNidP8BAFICAAAAAV0virKr77xOIRw1FU3mHJUQ6oO7UmoEgIUUsED3ZRiuAQAAAAD/////AYCy5g4AAAAAFgAUy79OrU4hd2M3xvJMufO1pLdEWcUAAAAAAAEBKwCj4REAAAAAIlEgLWIhk/X61lqdbrKkZqR9AsFvT4JFt4lmaYWbtsDTy3cBCEIBQGcgRxlVx6JBG4e3xDCw24Pm8QBFlHpvISfrlUAP1ajvKnh9aAAKCDqGBk5BysaADcNy+KFoBGtRk5GrmhgOwVoAAA==",
3  "complete": true
4}

sipa commented at 2:17 pm on May 3, 2022: member

@guggero I haven’t inspected things in detail, but I suspect that what’s going on is that you’ve simply provided sufficient information in the PSBT to construct a key-path spend, so that’s what it constructs (as it’s smaller in witness size).

achow101 commented at 2:25 pm on May 3, 2022: member

@sipa is correct. We will always try to create a key path spend if enough information is available to do so. You’ve provided the internal key and the derivation path for the internal key, even if it also has a leaf hash.

guggero commented at 2:43 pm on May 3, 2022: contributor

Ah, that makes a lot of sense, thank you for your answers, @sipa and @achow101 . Without the internal key and merkle root fields I get the expected witness stack :tada:

import descriptor (importdescriptors '[{"desc":"tr(tprv8ZgxMBicQKsPekctUrTRdca69GwGek7eoSafZ23pNYEsyHctxwWfUjgsq8UKwo3cLDpViLRv84bxtsZevZ6w4ZJqtMMqDZbnVreNZYbe6bu/86'"'"'/0'"'"'/0'"'"'/0/0)#53fkwtjk","timestamp":"now"}]')
BIP 86 key spend
- send coins to bcrt1pwr5wkkq5jfq8655dk929mar3588qncd0j24csyq22awcjwzh88mq5uhwph (BIP86 key spend, internal key 63cc52f06e0a91e3e6456631aa070221d58ccd1ed8c5b6639d4f88786dcd500f)
- create PSBT that references that output
- sign with walletprocesspsbt cHNidP8BAHECAAAAAeFJXY+feoKbYaGLcgspwL36Hwl9Ehr5sF4Zy7nj4TpQAAAAAAAAAAAAAo3j+gIAAAAAFgAUD1vsYoxpaHURXVbiu/mxgWarqwiAdNIaAAAAABYAFCqceUdOu0L9fdZ0pGsM4yBiQEPqAAAAAAABASsAZc0dAAAAACJRIHDo61gUkkB9Uo2xVF30caHOCeGvkquIEApXXYk4Vzn2IRZjzFLwbgqR4+ZFZjGqBwIh1YzNHtjFtmOdT4h4bc1QDxkAAAAAAFYAAIAAAACAAAAAgAAAAAAAAAAAAAAA
script root key spend
- send coins to bcrt1ptfmnpwtcs352cxush2kgqqza5jt9f9hymz9uhty4h57g8nvsduaq6s9ypp (script root key spend, internal key 63cc52f06e0a91e3e6456631aa070221d58ccd1ed8c5b6639d4f88786dcd500f, merkle root 6c2e4bb01e316abaaee288d69c06cc608cedefd6e1a06813786c4ec51b6e1d38)
- create PSBT that references that output
- sign with walletprocesspsbt cHNidP8BAHECAAAAAT/GF0O3okL4iQV80uYewSbhqa7qWoD2nG489WsPxw9xAAAAAAD/////AoCT3BQAAAAAFgAUKpx5R067Qv191nSkawzjIGJAQ+ri6PoCAAAAABYAFIyIwvqKctH/eWgjlieETO9w55xoAAAAAAABASsAhNcXAAAAACJRIFp3MLl4hGisG5C6rIAAXaSWVJbk2IvLrJW9PIPNkG86IRZjzFLwbgqR4+ZFZjGqBwIh1YzNHtjFtmOdT4h4bc1QDxkAAAAAAFYAAIAAAACAAAAAgAAAAAAAAAAAARcgY8xS8G4KkePmRWYxqgcCIdWMzR7YxbZjnU+IeG3NUA8BGCBsLkuwHjFquq7iiNacBsxgjO3v1uGgaBN4bE7FG24dOAAAIgICZ5Cr1jOOzCBlO+GZ+Y3VSYA26Z/kFrcSYqzM00TKPjQY1G/d61QAAIABAACAAAAAgAEAAAADAAAAAA==
script spend
- send coins to bcrt1p943zryl4ltt948twk2jxdfraqtqk7nuzgkmcjenfskdmdsxnedmsfzxf2p (script spend, internal key 63cc52f06e0a91e3e6456631aa070221d58ccd1ed8c5b6639d4f88786dcd500f, merkle root 1b971d840afd2cba7dbefbe1bbb3b796b70d4d956bfa3c3f3dbb93f164337bff)
- create PSBT that references that output
- sign with walletprocesspsbt cHNidP8BAFICAAAAAV0virKr77xOIRw1FU3mHJUQ6oO7UmoEgIUUsED3ZRiuAQAAAAD/////AcBg0hEAAAAAFgAUy79OrU4hd2M3xvJMufO1pLdEWcUAAAAAAAEBKwCj4REAAAAAIlEgLWIhk/X61lqdbrKkZqR9AsFvT4JFt4lmaYWbtsDTy3ciFcBjzFLwbgqR4+ZFZjGqBwIh1YzNHtjFtmOdT4h4bc1QDyMgY8xS8G4KkePmRWYxqgcCIdWMzR7YxbZjnU+IeG3NUA+swCEWY8xS8G4KkePmRWYxqgcCIdWMzR7YxbZjnU+IeG3NUA85ARuXHYQK/Sy6fb774buzt5a3DU2Va/o8Pz27k/FkM3v/AAAAAFYAAIAAAACAAAAAgAAAAAAAAAAAAAA=

tACK d5674f1

S3RK commented at 7:14 am on May 4, 2022: contributor

I was also testing this PR and found one minor issue. analyzepsbt command doesn’t correctly determine next role as “signer” for taproot inputs. This is due to the fact that SignTaproot doesn’t fill SignatureData::missing_sigs.

I’m not sure what is the best way to fix this as it’s not always possible to know which signatures are missing for taproot inputs. Based on my current understanding, I propose:

fill in SignatureData::missing_sigs as accurate as possible based on known information. Avoid providing wrong hints (e.g. when internal key is invalid)
remove the check requiring missing_sigs to be non empty in src/node/psbt.cpp:77. I think the idea was to guide users as to what they need to proceed with the input. But this check is too strict for taproot as we don’t always have this information, but still can determine that “signer” is the next role

DrahtBot added the label Needs rebase on May 18, 2022

achow101 force-pushed on May 18, 2022

DrahtBot removed the label Needs rebase on May 18, 2022

laanwj commented at 6:28 am on June 14, 2022: member

I was also testing this PR and found one minor issue. analyzepsbt command doesn’t correctly determine next role as “signer” for taproot inputs.

Is resolving this is a blocker for the PR or, is it something better done in a follow-up?

achow101 commented at 3:13 am on June 15, 2022: member

Is resolving this is a blocker for the PR or, is it something better done in a follow-up?

I think it can be done in a followup.

Sjors commented at 9:11 am on June 15, 2022: member

@guggero I’ve pushed some commits that should fix that problem. Can you try again?

Those two new commits could use a test case.

in src/wallet/scriptpubkeyman.cpp:2187 in 2b006944f4 outdated

2179@@ -2180,6 +2180,19 @@ TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction&
2180                     *keys = Merge(*keys, *pk_keys);
2181                 }
2182             }
2183+            for (const auto& pk_pair : input.m_tap_bip32_paths) {
2184+                const XOnlyPubKey& pubkey = pk_pair.first;
2185+                for (unsigned char prefix : {0x02, 0x03}) {
2186+                    unsigned char b[33] = {prefix};
2187+                    std::copy(pubkey.begin(), pubkey.end(), b + 1);

laanwj commented at 1:45 pm on June 22, 2022:

Should we assert the length of pubkey here to make sure this doesn’t ever overflow the buffer?

achow101 commented at 3:33 pm on June 23, 2022:

XOnlyPubKey can only be 32 bytes, so I don’t think it matters? It internally contains a uint256.

in src/script/standard.cpp:655 in 2b006944f4 outdated

648+    assert(IsComplete());
649+    std::vector<std::tuple<uint8_t, uint8_t, CScript>> tuples;
650+    if (m_branch.size()) {
651+        const auto& leaves = m_branch[0]->leaves;
652+        for (const auto& leaf : leaves) {
653+            uint8_t depth = (uint8_t)leaf.merkle_branch.size();

laanwj commented at 1:53 pm on June 22, 2022:

Will this size always fit in a byte? Might want to assert here.

achow101 commented at 7:02 pm on June 23, 2022:

Added an assert.

in src/psbt.h:620 in 2b006944f4 outdated

615+                    } else if ((key.size() - 2) % 32 != 0) {
616+                        throw std::ios_base::failure("Input Taproot leaf script key's control block size is not valid");
617+                    }
618+                    std::vector<unsigned char> script_v;
619+                    s >> script_v;
620+                    assert(!script_v.empty());

laanwj commented at 2:39 pm on June 22, 2022:

Is using an assert here dangerous? I mean, the application shouldn’t crash on an invalid PSBT.

Sjors commented at 3:18 pm on June 22, 2022:

From commit 088e8366765dc7002e9cbcb45654e0709e233cd5. It’s also not clear to me why this would be safe.

achow101 commented at 7:03 pm on June 23, 2022:

Changed the assert to just a check that script_v is not empty, and it will throw if it is.

achow101 force-pushed on Jun 23, 2022

achow101 commented at 7:02 pm on June 23, 2022: member

Those two new commits could use a test case.

Done.

achow101 force-pushed on Jun 23, 2022

Sjors commented at 1:11 pm on June 24, 2022: member

mwallet_taproot.py CI failure, probably not spurious?

in src/psbt.cpp:223 in 1db84e9aec outdated

217@@ -177,6 +218,16 @@ void PSBTOutput::FillSignatureData(SignatureData& sigdata) const
218     for (const auto& key_pair : hd_keypaths) {
219         sigdata.misc_pubkeys.emplace(key_pair.first.GetID(), key_pair);
220     }
221+    if (!m_tap_internal_key.IsNull()) {
222+        sigdata.tr_spenddata.internal_key = m_tap_internal_key;
223+    }
224+    if (!m_tap_tree.IsEmpty()) {
225+        TaprootSpendData spenddata = m_tap_tree.GetSpendData();

MarcoFalke commented at 1:17 pm on June 24, 2022:

0 node1 stderr script/standard.cpp:498:53: runtime error: load of value 21, which is not a valid value for type 'bool'
1    [#0](/bitcoin-bitcoin/0/) 0x55ba8cce31d5 in TaprootBuilder::GetSpendData() const src/./src/script/standard.cpp:498:53
2    [#1](/bitcoin-bitcoin/1/) 0x55ba8cc138e9 in PSBTOutput::FillSignatureData(SignatureData&) const src/./src/psbt.cpp:225:49
3
4SUMMARY: UndefinedBehaviorSanitizer: invalid-bool-load script/standard.cpp:498:53 in

https://cirrus-ci.com/task/5083471289778176?logs=ci#L3013

Sjors commented at 1:25 pm on June 24, 2022:

Indeed, somehow m_parity got set to 21. Digging to see how that was achieved.

Sjors commented at 1:34 pm on June 24, 2022:

My guess is that it’s unitialized because GetSpendData() was called before Finalize(). We should probably have an assert for that.

achow101 commented at 3:27 pm on June 24, 2022:

I’ve added a call to Finalize after parsing, so hopefully it works now.

Sjors commented at 4:09 pm on June 24, 2022:

0'Finalize' has type 'const TaprootBuilder', but function is not marked const
1        m_tap_tree.Finalize(m_tap_internal_key)

achow101 commented at 6:41 pm on June 24, 2022:

Fixed.

achow101 force-pushed on Jun 24, 2022

in src/psbt.h:620 in d94fffe683 outdated

617@@ -618,7 +618,7 @@ struct PSBTInput
618                     std::vector<unsigned char> script_v;
619                     s >> script_v;
620                     if (script_v.empty()) {
621-                        throw std::ios_base:failure("Input Taproot leaf script must be at least 1 byte");
622+                        throw std::ios_base::failure("Input Taproot leaf script must be at least 1 byte");

Sjors commented at 1:48 pm on June 27, 2022:

d94fffe683e2bffc0417e5bf72c2c78a9166e1e3: nit if you have to touch earlier commits, belongs in 324bba0d0b863f7bc3825d1efc3d405eb3894536

achow101 commented at 8:44 pm on June 27, 2022:

Done

in test/functional/rpc_psbt.py:726 in d94fffe683 outdated

722@@ -723,5 +723,46 @@ def test_psbt_input_keys(psbt_input, keys):
723         )
724         assert_equal(psbt2["fee"], psbt3["fee"])
725 
726+        self.log.info("Test signing inputs that the wallet has keys for but is not watching the scripts")

Sjors commented at 1:57 pm on June 27, 2022:

I assume this works for individual private keys, but not for master keys?

I also seems to work if you have an xpriv and the descriptor has been expanded far enough. Might be good to add a test case for that behaviour too.

achow101 commented at 8:45 pm on June 27, 2022:

I assume this works for individual private keys, but not for master keys?

Yes, we’re not doing the dumb thing from legacy wallets where the seed key is a legitimate key we can receive at.

I also seems to work if you have an xpriv and the descriptor has been expanded far enough. Might be good to add a test case for that behaviour too.

Yes, that’s the point, the test already covers that? Unless you mean something else.

Sjors commented at 10:13 am on June 28, 2022:

Yes, we’re not doing the dumb thing from legacy wallets where the seed key is a legitimate key we can receive at.

That’s not what I mean. I was referering to be able to sign with any key that derives from the seed, assuming a derivation path is given.

Yes, that’s the point, the test already covers that? Unless you mean something else.

The test imports a single private key into the descriptor-less wallet, not an xpriv.

achow101 commented at 2:46 pm on June 28, 2022:

We will only sign with child keys. Anything that requires deriving other child keys from master keys is not supported.

in test/functional/rpc_psbt.py:755 in d94fffe683 outdated

750+        if self.options.descriptors:
751+            eckey = ECKey()
752+            eckey.generate()
753+            privkey = bytes_to_wif(eckey.get_bytes())
754+
755+            desc = descsum_create("tr(50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0,pk({}))".format(eckey.get_pubkey().get_bytes().hex()))

Sjors commented at 1:59 pm on June 27, 2022:

d94fffe683e2bffc0417e5bf72c2c78a9166e1e3 where does the 5092…03ac0 keypath come from? I assume it’s intentionally unspendable, but that’s not super clear now.

achow101 commented at 8:46 pm on June 27, 2022:

From wallet_taproot.py, which got it from the BIP. I’ve refactored that into test_framework/key.py so that it can be shared.

in src/wallet/scriptpubkeyman.cpp:2190 in 58a1a0c7a3 outdated

2185+                for (unsigned char prefix : {0x02, 0x03}) {
2186+                    unsigned char b[33] = {prefix};
2187+                    std::copy(pubkey.begin(), pubkey.end(), b + 1);
2188+                    CPubKey fullpubkey;
2189+                    fullpubkey.Set(b, b + 33);
2190+                    std::unique_ptr<FlatSigningProvider> pk_keys = GetSigningProvider(fullpubkey);

Sjors commented at 2:14 pm on June 27, 2022:

58a1a0c7a3f813c887d349ea3670584aff3773ad : we could probably use a GetSigningProvider specifically for XOnlyPubKey, but that can wait.

in src/script/sign.cpp:233 in 37f6d29df5 outdated

229@@ -230,16 +230,16 @@ static bool SignTaproot(const SigningProvider& provider, const BaseSignatureCrea
230     // Try key path spending.
231     {
232         KeyOriginInfo info;
233-        if (provider.GetKeyOriginByXOnly(spenddata.internal_key, info)) {
234-            auto it = sigdata.taproot_misc_pubkeys.find(spenddata.internal_key);
235+        if (provider.GetKeyOriginByXOnly(sigdata.tr_spenddata.internal_key, info)) {

Sjors commented at 2:36 pm on June 27, 2022:

37f6d29df5405d0cbebbb4368a6dcbc95c8f00e5 : maybe just squash this entire commit into f5e58b6b3dfa1622d7c03f82318ef06eb6c6818c or f7c7be9794d868a91c25a743f00b3fd351589d5f and add a comment along the lines of the commit message?

achow101 commented at 8:47 pm on June 27, 2022:

IMO it’s a separate enough change that it should be its own commit.

in src/script/standard.cpp:654 in d94fffe683 outdated

649+    assert(IsComplete());
650+    std::vector<std::tuple<uint8_t, uint8_t, CScript>> tuples;
651+    if (m_branch.size()) {
652+        const auto& leaves = m_branch[0]->leaves;
653+        for (const auto& leaf : leaves) {
654+            assert(leaf.merkle_branch.size() < TAPROOT_CONTROL_MAX_NODE_COUNT);

Sjors commented at 2:45 pm on June 27, 2022:

4542ba12866c5f9e7bb0da186800f2f5e450bea6: <= TAPROOT_CONTROL_MAX_NODE_COUNT ? See also TaprootBuilder::Insert. In descriptor.cpp we check branches.size() > TAPROOT_CONTROL_MAX_NODE_COUNT.

achow101 commented at 8:47 pm on June 27, 2022:

Fixed.

Sjors approved

Sjors commented at 2:53 pm on June 27, 2022: member

utACK d94fffe683e2bffc0417e5bf72c2c78a9166e1e3 modulo <= question

achow101 force-pushed on Jun 27, 2022

Move individual KeyOriginInfo de/ser to separate function

To make it easier to de/serialize individual KeyOriginInfo for PSBTs,
separate the actual de/serialization of KeyOriginInfo to its own
function.

This is an additional separation where any length prefix is processed by
the caller.

ce911204e4

Add TaprootBuilder::GetTreeTuples

GetTreeTuples returns the leaves in DFS order as tuples of depth, leaf
version, and script. This is a representation of the tree that can be
serialized.

d43923c381

Add serialization methods to XOnlyPubKey

It is useful to have serialzation methods for XOnlyPubKey. These will
serialize the internal uint256, so it is not prefixed with the length as
CPubKey does.

d557eff2ad

Implement de/ser of PSBT's Taproot fields 05e2cc9a30

Fill PSBT Taproot input data to/from SignatureData 52e3f2f88e

Fetch key origins for Taproot keys 4d1223e512

Store TaprootBuilder in SigningProviders instead of TaprootSpendData

TaprootSpendData can be gotten from TaprootBuilder, however for PSBT, we
also need TaprootBuilders directly (for the outputs). So we store the
TaprootBuilder in the FlatSigningProvider and when the TaprootSpendData
is needed, we generate it on the fly using the stored builder.

3ae5b6af21

Assert that TaprootBuilder is Finalized during GetSpendData

GetSpendData needs to be finalized in order to be used. To avoid future
bugs, assert `!m_output_key.IsNull()` as m_output_key is only set during
Finalize.

25b6ae46e7

Fill PSBT Taproot output data to/from SignatureData ac7747585f

Implement decodepsbt for Taproot fields 7dccdd3157

psbt: Remove non_witness_utxo for segwit v1+

If all inputs are segwit v1+, the non_witness_utxos can be removed.

103c6fd279

tests: Test taproot fields for PSBT 0ad21e7c55

taproot: Use pre-existing signatures if available

Actually use pre-existing signatures in CreateTaprootScriptSig if a
signature is found for the given key and leaf hash.

496a1bbe5e

psbt, test: Check for taproot fields in taproot psbt test 1ece9a3715

psbt: Implement merge for Taproot fields 5f12fe3f36

sign: Use sigdata taproot spenddata when signing

The taproot spenddata stored in a sigdata is the combination of data
existing previously (e.g. in a PSBT) and the data stored in a
SigningProvider. In order to use the external data when signing, we need
to be using the sigdata's spenddata.

6cff82722f

wallet: also search taproot pubkeys in FillPSBT

When filling a PSBT, we search the listed pubkeys in order to determine
whether the current DescriptorScriptPubKeyMan could sign the transaction
even if it is not watching the scripts. With Taproot, the taproot
pubkeys need to be searched as well.

a73b56888a

test: Test signing psbts without explicitly having scripts b80de4c505

achow101 force-pushed on Jun 27, 2022

laanwj commented at 2:19 pm on June 28, 2022: member

Code review ACK b80de4c505bf6377f2e476133dce6f2a803f1fa1

laanwj merged this on Jun 28, 2022

laanwj closed this on Jun 28, 2022

sidhujag referenced this in commit 097323eeed on Jun 28, 2022

MarcoFalke commented at 8:39 am on June 30, 2022: member

I am pretty sure the memory issue still persists:

bitcoin deleted a comment on Jun 30, 2022

Sjors commented at 11:39 am on June 30, 2022: member

@MarcoFalke that’s a different one than we found: #22558#pullrequestreview-1018466595

(not that that’s great news)

in src/psbt.h:869 in b80de4c505

864+                        uint8_t leaf_ver;
865+                        CScript script;
866+                        s_tree >> depth;
867+                        s_tree >> leaf_ver;
868+                        s_tree >> script;
869+                        m_tap_tree->Add((int)depth, script, (int)leaf_ver, true /* track */);

Sjors commented at 1:28 pm on June 30, 2022:

This is the only place where we call TaprootBuilder::Add (in this PR). We’re not checking leaf_ver here, so with a corrupt PSBT the leaf_version & ~TAPROOT_LEAF_MASK assert gets hit.

azuchi referenced this in commit e505a537b3 on Jul 7, 2022

MarcoFalke commented at 10:08 am on July 19, 2022: member

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=49260

achow101 commented at 3:48 pm on July 19, 2022: member

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=49260

#25513 fixes that.

fanquake referenced this in commit 6900162aea on Jul 19, 2022

sidhujag referenced this in commit 0420a41b55 on Jul 20, 2022

fanquake referenced this in commit 28cf756971 on Oct 26, 2022

sidhujag referenced this in commit b2fd8cd7be on Oct 27, 2022

bitcoin locked this on Jul 19, 2023

psbt: Taproot fields for PSBT #22558

Conflicts