bdk icon indicating copy to clipboard operation
bdk copied to clipboard

Remove extra taproot fields when finalizing PSBT

Open ValuedMammal opened this issue 1 year ago • 3 comments

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 fmt and cargo clippy before 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

ValuedMammal avatar Jan 31 '24 21:01 ValuedMammal

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.

ValuedMammal avatar Feb 10 '24 22:02 ValuedMammal

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

danielabrozzoni avatar Feb 23 '24 12:02 danielabrozzoni

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

ValuedMammal avatar Feb 23 '24 14:02 ValuedMammal