Remove extra taproot fields when finalizing PSBT
We currently allow removing partial_sigs from a finalized PSBT, which is relevant to non-taproot inputs, however taproot related PSBT fields were left in place despite the recommendation of BIP371 to remove them once the final_script_witness is constructed. This can cause confusion for parsers that encounter extra taproot metadata in an already satisfied input.
Fix this by introducing a new member to SignOptions remove_taproot_extras, which when true will remove extra taproot related data from a PSBT upon successful finalization. This change makes removal of all taproot extras the default but configurable.
fixes #1243
Notes to the reviewers
If there's a better or more descriptive name for remove_taproot_extras, I'm open to changing it.
Changelog notice
Fixed an issue finalizing taproot inputs following BIP371
Checklists
All Submissions:
- [x] I've signed all my commits
- [x] I followed the contribution guidelines
- [x] I ran
cargo fmtandcargo clippybefore committing
Bugfixes:
- [x] I've added tests to reproduce the issue which are now passing
- [x] I'm linking the issue being fixed by this PR
I have a bit more clarity on things. The wallet considers a psbt finalized if it can produce a signature and fill in the witness or scriptsig, and I think that's enough for our purposes. However the resulting transaction can still be rejected by the network for other reasons (the coins were already spent, a timelock wasn't met, etc).
We still need to ensure the correct key is used for signing, and the most straightforward way is to verify the signature against the intended pubkey with verify_schnorr. This requires access to more of the internals, like computing the sighash, so the test for this can ~~possibly just go in signer.rs, where we simulate the behavior of the wallet by looping over different signers, calling sign_input,~~ and verifying the signature we end up with against the expected xonly key.
edit: I found a way to do it by fixing up the existing test.
This is fantastic, thanks! Just a little nit, can you reorder the commits so that they're atomical and hygienic? (More info here: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches)
I'm thinking, instead of having 4 commits, just having two, one updating the test test_psbt_multiple_internalkey_signers, one removing the extra taproot fields when finalizing
I'm thinking, instead of having 4 commits, just having two, one updating the test
test_psbt_multiple_internalkey_signers, one removing the extra taproot fields when finalizing
For sure, will squash