psbt: clear Taproot and MuSig2 signatures when joining #36352

joinpsbts builds a new transaction from the inputs and outputs of the given PSBTs, so any existing signatures are no longer valid for it. PartiallySignedTransaction::AddInput handles that by clearing partial_sigs and the final scriptSig/witness (psbt.cpp:180), but that code predates Taproot and was never extended to the Taproot and MuSig2 fields.

So a Taproot input that was signed before the join keeps its old taproot_key_path_sig. SignTaproot (sign.cpp:608) doesn't produce a new key path signature when one is already present, so the input can't be completed afterwards:

• a bech32 (P2WPKH) input: after joinpsbts, walletprocesspsbt re-signs and returns complete: true
• a bech32m (P2TR) input: taproot_key_path_sig is still there after the join, and walletprocesspsbt / finalizepsbt return complete: false. utxoupdatepsbt, descriptorprocesspsbt and signing again don't help, so the only way out is editing the PSBT by hand.

Script path signatures (taproot_script_path_sigs) behave the same. For MuSig2 the old public nonce is kept, but the secret nonce is looked up by a session id that includes the old sighash, so no new partial signature is made either.

This also clears m_tap_key_sig, m_tap_script_sigs, m_musig2_pubnonces and m_musig2_partial_sigs in the v0 branch of AddInput. Participant pubkeys are key data, not signatures, so they're kept. The v2 branch is unchanged: there, adding inputs is governed by the BIP370 modifiable flags.

The new check in rpc_psbt.py signs a v0 PSBT with a P2TR input, joins it with another one, and checks that the key path signature is gone and the joined PSBT can be signed. It fails on master and passes with the fix. wallet_musig.py still passes.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

External sites

• Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36352
• Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36352.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36352#author-ralph

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #36382 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36382.svg">} (rpc: flag invalid taproot leaf control blocks in decodepsbt by fametrano)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

tACK e1aa0baa146a3edc3dd3fc143e870419d81e2e4a

Thanks for the fix. I built it and ran rpc_psbt.py: it passes here and fails on master. When I call joinpsbts directly, all four fields are dropped on this branch and kept on master.

The test only checks taproot_key_path_sig, though: it still passes if any one of the other three new clear() lines is deleted. Adding the other three fields to tr_psbt1 before the join (random values, as in test_decodepsbt_musig2_input_output_types) and checking all four afterwards catches each deletion:

         tr_psbt1 = self.nodes[1].walletprocesspsbt(psbt=tr_psbt1, finalize=False)['psbt']
-        assert "taproot_key_path_sig" in self.nodes[0].decodepsbt(tr_psbt1)['inputs'][0]
+        tr_psbt1_obj = PSBT.from_base64(tr_psbt1)
+        _, part_pubkey = generate_keypair()
+        _, agg_pubkey = generate_keypair()
+        leaf_hash = randbytes(32)
+        tr_psbt1_obj.i[0].map[bytes([PSBT_IN_TAP_SCRIPT_SIG]) + part_pubkey[1:] + leaf_hash] = randbytes(64)
+        tr_psbt1_obj.i[0].map[bytes([PSBT_IN_MUSIG2_PUB_NONCE]) + part_pubkey + agg_pubkey + leaf_hash] = randbytes(66)
+        tr_psbt1_obj.i[0].map[bytes([PSBT_IN_MUSIG2_PARTIAL_SIG]) + part_pubkey + agg_pubkey + leaf_hash] = randbytes(32)
+        tr_psbt1 = tr_psbt1_obj.to_base64()
+        tr_sig_fields = ["taproot_key_path_sig", "taproot_script_path_sigs", "musig2_pubnonces", "musig2_partial_sigs"]
+        for field in tr_sig_fields:
+            assert field in self.nodes[0].decodepsbt(tr_psbt1)['inputs'][0]
         ...
         for tr_input in self.nodes[0].decodepsbt(tr_joined)['inputs']:
-            assert "taproot_key_path_sig" not in tr_input
+            for field in tr_sig_fields:
+                assert field not in tr_input

(plus PSBT_IN_TAP_SCRIPT_SIG in the imports). This still passes on the PR and fails on master and on each single deletion.

Thanks for testing, and good catch on the test. Took your suggestion in 4efa6b7d70: the input now also gets a script path sig, a MuSig2 pubnonce and a partial sig before the join, and all four fields are checked afterwards. Removing any one of the four clear() lines now makes rpc_psbt.py fail.

reACK 4efa6b7d70e36afac66d5eb6a2fa05f683becdba

Thanks for taking the test. rpc_psbt.py passes here. It fails without the fix, and when any one of the four clear() lines is removed.