wallet: finalize_psbt should clear BIP32 derivations

[ValuedMammal]

According to BIP174 after building the final scriptWitness and scriptSig "all other data except the UTXO and unknown fields in the input key-value map should be cleared from the PSBT". This would include the BIP32 derivation paths for known pubkeys.

To Reproduce

let (mut wallet, _) = get_funded_wallet(get_test_wpkh());
let addr = wallet.next_unused_address(KeychainKind::External).unwrap();
let mut builder = wallet.build_tx();
builder.drain_to(addr.script_pubkey()).drain_wallet();
let mut psbt = builder.finish().unwrap();
assert!(wallet.sign(&mut psbt, SignOptions::default(),).unwrap());

assert!(psbt.inputs[0].bip32_derivation.is_empty());
assert!(psbt.outputs[0].bip32_derivation.is_empty());

Additional context

I seem to recall @notmandatory mentioned there is a reason for having more granularity in choosing which fields should remain as opposed to just clearing the necessary fields by default. Is that still the case?

[storopoli]

I've have recently worked into these nitty details into the rust-bitcoin codebase (e.g. rust-bitcoin/rust-bitcoin#2747 and rust-bitcoin/rust-bitcoin.github.io#32).

For production, the step 4 of BIP 174 (Finalizer) should be done with the
psbt::PsbtExt from the miniscript crate trait.
It provides a
.finalize_mut
to a Psbt object,
which takes in a mutable reference to Psbt and populates the final_witness and final_scriptsig for all inputs.

I am not familiarized with the Psbt API in BDK, but if we are using somehow rust-bitcoin we should definitely use the finalize_mut mentioned above.

[ValuedMammal]

Replaying this comment here from discord:

Clearing bip32_derivation was left out of #1310 at the last minute because we felt it wasn't specific to taproot. A psbt has bip32_derivation as well as tap_key_origins and both of these include a KeySource. So to fix this one we either need another sign option or we should simplify the sign options into one option to clear them all. Last time we discussed it I thought the verdict was that some external signers require the presence of fields like partial_sigs, hence the various options.

[ValuedMammal]

It provides a
.finalize_mut
to a Psbt object,
which takes in a mutable reference to Psbt and populates the final_witness and final_scriptsig for all inputs.

I'm also a fan of finalize_mut, it seems the major difference is that currently bdk tries to finalize individual inputs using a custom input Satisfier impl that depends on the state of the wallet.

[ValuedMammal]

The original issue for remove_partial_sigs #612 doesn't mention a strong use case for keeping the extra data around after finalizing that would warrant having its own sign option, so basically I think the sign options for removing extra fields can be consolidated into a single option or it should just happen automatically when try_finalize is set.

[notmandatory]

If the bip 174 spec says to clear all fields then I think that should be the default, and maybe only option. I don't recall a reason to not clear the fields after finalize. I also prefer using the rust-bitcoin .finalize_mut function instead of existing code, which was probably done before .finalize_mut was available.

[ValuedMammal]

A couple reasons in my estimation why we're not currently using finalize_mut

• Descriptor::satisfy already constructs the witness so it's redundant to call finalize_mut again
• The method only clears the inputs, whereas we want to clear fields from the outputs as well
• This section of the code is directly implicated in another issue timestamp timelocks are not satisfied #642 and I'd like to find a resolution on that before scrapping existing code (although to be frank this functionality seems mostly untested?)

It might be possible to totally replace the current Wallet::finalize_psbt with miniscript's finalize_mut, but it might need more of a refactor.

[notmandatory]

If using rust-bitcoin's finalize_mut is going to be a bigger refactor and not just a drop in replacement then at this point I'd rather defer it to post-1.0.0. Would it make sense to tackle the finalize_mut integration in the future when we're also replacing the policy stuff with the new miniscript plan module?