-
Notifications
You must be signed in to change notification settings - Fork 307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove extra taproot fields when finalizing PSBT #1310
Remove extra taproot fields when finalizing PSBT #1310
Conversation
crates/bdk/tests/psbt.rs
Outdated
..Default::default() | ||
} | ||
) | ||
.unwrap()); | ||
// Checks that we signed using the right key | ||
assert!( | ||
psbt.finalize_mut(&secp).is_ok(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danielabrozzoni Curiously, removing tap_key_sig
from a psbt input made this fail somewhere during finalize_mut
, hence this change. But this shouldn't affect the meaning of the test, other than to assert finalization succeeded in Wallet::sign
. Just wanted to run this by you because I think it was one of your tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this is that finalize_mut
doesn't really make sense here: wallet.sign
is already finalizing the transaction (and you are asserting that, thanks for adding it!), and the only reason why finalize_mut
wouldn't complain before is because we left the taproot_extras in the psbt, and so miniscript was able to re-finalize it. Can you please add a commit removing this assert with finalize_mut? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I don't believe asserting that sign
returns true is sufficient to check which key did the signing, whereas finalize_mut
will take steps to validate sigs, if I understand. If we just have wallet not try to finalize, then this will make more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your approach works, but the previous one worked too:
tap_signatures
is an Option, it is not a BTreeMap likepartial_sigs
- The bug was that we would sign with whatever key we had in hand, thus replacing the previous signature that was in the PSBT
- In the wallet at the time of signing the key with higher precedence is the right one, so we are signing with the right one first, and the wrong one second. Without the bug fix, we would replace the right signature with the wrong one and we wouldn't be able to finalize. After the fix, we check that we're signing with the right key, and so adding a wrong key that signs last doesn't really matter (it is not used for signing anyways)
- If the tx is finalized it means that we had the right signature in place
Perhaps instead of rewriting the test, we could add comments better explaining what's going on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. How about we take your original suggestion and get rid of finalize_mut
.
The only problem I see is that this test (below) passes on the pre #1200 buggy code, when it shouldn't?
expand
fn test_psbt_multiple_internalkey_signers() {
use bdk::signer::{SignerContext, SignerOrdering, SignerWrapper};
use bdk::KeychainKind;
use bitcoin::{secp256k1::Secp256k1, PrivateKey};
use miniscript::psbt::PsbtExt;
use std::sync::Arc;
let secp = Secp256k1::new();
let (mut wallet, _) = get_funded_wallet(get_test_tr_single_sig());
let send_to = wallet.get_address(AddressIndex::New);
let mut builder = wallet.build_tx();
builder.add_recipient(send_to.script_pubkey(), 10_000);
let mut psbt = builder.finish().unwrap();
// Adds a signer for the wrong internal key, bdk should not use this key to sign
wallet.add_signer(
KeychainKind::External,
// A signerordering lower than 100, bdk will use this signer first
SignerOrdering(0),
Arc::new(SignerWrapper::new(
PrivateKey::from_wif("5J5PZqvCe1uThJ3FZeUUFLCh2FuK9pZhtEK4MzhNmugqTmxCdwE").unwrap(),
SignerContext::Tap {
is_internal_key: true,
},
)),
);
let finalized = wallet.sign(&mut psbt, SignOptions::default()).unwrap();
assert!(finalized);
// Checks that we signed using the right key
let err = psbt.finalize_mut(&secp);
dbg!(err);
}
so finalize_mut
was doing its job, even though the wallet considers it finalized. So what made the tx unbroadcastable, I wonder?
This is probably a bit out of scope for the current PR, but in the meantime I'll think of some other ways we can test the 'correct internal key' logic if you think that would be valuable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you're right, thanks! I think the problem lays in the fact that finalized
is calculated based on "do we have the descriptor to satisfy this input? If that's so, and we can satisfy every input, then the transaction must be finalized".
I am a bit torn here. The only solution that comes to my mind is to get rid of the test - we realized it wasn't even testing what I thought it was, and atm we don't have any function to check "did we sign with the right key?".
@evanlinjin inputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: we can also comment the test for now instead of removing it, with a comment pointing to this discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Referring to this
bdk/crates/bdk/src/wallet/signer.rs
Lines 459 to 467 in 7aca884
if let SignerContext::Tap { is_internal_key } = self.ctx { | |
if let Some(psbt_internal_key) = psbt.inputs[input_index].tap_internal_key { | |
if is_internal_key | |
&& psbt.inputs[input_index].tap_key_sig.is_none() | |
&& sign_options.sign_with_tap_internal_key | |
&& x_only_pubkey == psbt_internal_key | |
{ | |
let (hash, hash_ty) = Tap::sighash(psbt, input_index, None)?; | |
sign_psbt_schnorr( |
The bug was that we would sign with whatever key we had in hand, thus replacing the previous signature that was in the PSBT
In the wallet at the time of signing the key with higher precedence is the right one, so we are signing with the right one first, and the wrong one second.
My interpretation is that we sign with the wrong key first, since it has the lower SignerOrdering, and then skip signing for subsequent signers, as at that point tap_key_sig
is no longer None.
After the fix, we check that we're signing with the right key, and so adding a wrong key that signs last doesn't really matter (it is not used for signing anyways)
Agreed
Edit: we can also comment the test for now instead of removing it, with a comment pointing to this discussion
I'm ok with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My interpretation is that we sign with the wrong key first, since it has the lower SignerOrdering, and then skip signing for subsequent signers, as at that point tap_key_sig is no longer None.
Yes, that's exactly what we would do
fa46980
to
188ba36
Compare
188ba36
to
0d395c5
Compare
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 edit: I found a way to do it by fixing up the existing test. |
This is fantastic, thanks! I'm thinking, instead of having 4 commits, just having two, one updating the test |
For sure, will squash |
to verify the signature of the input and ensure the right internal key is used to sign. This fixes a shortcoming of a previous version of the test that relied on the result of `finalize_psbt` but would have erroneously allowed signing with the wrong key.
1ea6ae9
to
e536372
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for doing this! I prefer if we remove taproot extra fields in all cases when witness/scriptsig is finalized (instead of just when the tap_internal_key
is present). Otherwise, it looks good to me!
crates/bdk/src/wallet/mod.rs
Outdated
@@ -1972,6 +1972,18 @@ impl<D> Wallet<D> { | |||
if sign_options.remove_partial_sigs { | |||
psbt_input.partial_sigs.clear(); | |||
} | |||
if sign_options.remove_taproot_extras | |||
&& psbt_input.tap_internal_key.is_some() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I guess this is not strictly necessary. Also, it might be the case that the signer decides to clear some fields and not all.
&& psbt_input.tap_internal_key.is_some() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rationale was that without this condition, we would clear bip32 derivations even for non taproot spends. Maybe bip32_derivation
doesn't belong in the list of "taproot extras"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. What is the rationale of clearing the bip_derivation
field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I'm thinking the taproot derivation info is already represented by tap_key_origins
, meaning bip32_derivation
isn't relevant here.
crates/bdk/src/wallet/mod.rs
Outdated
if output.tap_internal_key.is_some() { | ||
output.bip32_derivation.clear(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if output.tap_internal_key.is_some() { | |
output.bip32_derivation.clear(); | |
} | |
output.bip32_derivation.clear(); |
I think we should clear this for all finalized outputs.
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. test(wallet): Add test `test_taproot_remove_tapfields_after_finalize_sign_option` that checks various fields have been cleared for taproot Psbt `Input`s and `Output`s according to BIP371.
e536372
to
5840ce4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 5840ce4
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 thefinal_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:
cargo fmt
andcargo clippy
before committingBugfixes: