Add PSBT creator, signer and finalizer roles for BIP-322 - #75
Conversation
169b567 to
10744cf
Compare
4a82844 to
99e7843
Compare
e2123af to
ed645d3
Compare
|
gmgm @aagbotemi I just merged #73, could you fix the conflicts in this branch and then I'll have a look at this. Thanks! |
ed645d3 to
4d83393
Compare
4d83393 to
4256d84
Compare
|
@raphjaph I have fixed the conflict. Thank you. |
4256d84 to
16cf758
Compare
raphjaph
left a comment
raphjaph
left a comment
There was a problem hiding this comment.
There's a lot going on in this PR (PSBTs, different address types, multisigs), which is unavoidable but makes it quite complex to review all edge cases. So this is just my first pass review. We definitely need more tests for all this. Here a list of what I can think of
p2tr_psbt_roundtrip
p2wpkh_psbt_roundtrip
p2sh_p2wpkh_psbt_roundtrip
p2pkh_psbt_roundtrip
p2sh_p2wsh_2of2_psbt_roundtrip
p2sh_multisig_2of2_psbt_roundtrip
ordinary_psbt_rejected
missing_global_message_field_rejected
tampered_prevout_txid_rejected
wrong_prevout_vout_rejected
nonzero_output_value_rejected
non_op_return_output_rejected
extra_output_rejected
missing_utxo_fields_rejected
sign_rejects_wrong_key
sign_rejects_key_not_in_multisig_script
sign_rejects_tampered_witness_script
sign_rejects_multi_input_psbt
finalize_rejects_insufficient_multisig_sigs
finalize_rejects_unsigned_single_sig
finalize_rejects_mismatched_partial_sig_key
finalize_rejects_extra_partial_sigs
finalize_rejects_tap_key_sig_on_non_p2tr_challenge
create_rejects_witness_script_for_single_sig
create_rejects_unknown_p2sh_script
create_bare_p2sh_sets_correct_utxo_field
These are a lot of tests but this PR introduces PSBTs, which are passed around between different parties so we need to be very strict about the structure and the edge cases.
| let (pub_key, signature) = psbt.inputs[0] | ||
| .partial_sigs | ||
| .iter() | ||
| .next() | ||
| .map(|(pub_key, signature)| (*pub_key, *signature)) | ||
| .ok_or(Error::SignatureCount { | ||
| required: 1, | ||
| provided: 0, | ||
| })?; |
There was a problem hiding this comment.
We should bind the pubkey to the challenge here. Also you always just take the first entry and then hardcode the Error to provided = 0. This applies to all 3 single sig checks in this module. It should make sure there's only 1 signature. For the multisig path you're already doing the correct thing.
There was a problem hiding this comment.
These probably need a proper helper function or smth
| if let Some(signature) = psbt.inputs[0].tap_key_sig { | ||
| let mut witness = Witness::new(); | ||
| witness.push(signature.to_vec()); | ||
| psbt.inputs[0].final_script_witness = Some(witness); | ||
| psbt.inputs[0].tap_key_sig = None; | ||
| psbt.inputs[0].tap_internal_key = None; | ||
| return encode_finalized(psbt); | ||
| } |
There was a problem hiding this comment.
We should check that challenge really is P2TR
| let first = psbt.inputs.first()?; | ||
| let first_txin = psbt.unsigned_tx.input.first()?; | ||
|
|
||
| let message_challenge = if let Some(txout) = &first.witness_utxo { |
There was a problem hiding this comment.
We should validate that witness_utxo.value is 0
| psbt.inputs[0].tap_internal_key = Some(x_only_public_key); | ||
|
|
||
| let prevouts = [TxOut { | ||
| value: Amount::from_sat(0), |
There was a problem hiding this comment.
| value: Amount::from_sat(0), | |
| value: Amount::ZERO, |
| } | ||
|
|
||
| let sighash = SighashCache::new(psbt.unsigned_tx.clone()) | ||
| .p2wpkh_signature_hash(0, challenge, Amount::from_sat(0), sighash_type) |
There was a problem hiding this comment.
| .p2wpkh_signature_hash(0, challenge, Amount::from_sat(0), sighash_type) | |
| .p2wpkh_signature_hash(0, challenge, Amount::ZERO, sighash_type) |
You use Amount::from_sat(0) and also Amount::ZERO inconsistently, let's use Amount::ZERO everywhere
| if spk.is_p2wpkh() || spk.is_p2tr() { | ||
| return Ok(psbt); | ||
| } | ||
| if spk.is_p2pkh() { | ||
| psbt.inputs[0].witness_utxo = None; | ||
| psbt.inputs[0].non_witness_utxo = Some(to_spend); | ||
| return Ok(psbt); | ||
| } | ||
|
|
||
| let Some(witness_script) = witness_script else { | ||
| if spk.is_p2sh() { | ||
| return Ok(psbt); | ||
| } | ||
| return Err(Error::InvalidWitness); | ||
| }; | ||
|
|
||
| let p2wsh_program = ScriptBuf::new_p2wsh(&witness_script.wscript_hash()); | ||
|
|
There was a problem hiding this comment.
create_bip322_psbt: witness_script silently ignored for P2WPKH/P2TR; unsignable PSBT returned for bare P2SH, keeping witness_utxo where BIP-174 requires non_witness_utxo
|
@raphjaph, all requested changes has been addressed for re-review. Thank you. |
2 participants
Summary
Implement BIP-322's multi-party PSBT workflow, a creator builds an unsigned signing request, independent signers add partial signatures after detecting the request, and a finalizer assembles and encodes the result.
Rebased on #73
Changes
detect_bip322_psbtto check the detection properties.Bip322Psbtto carry the detected message and challenge for signers.create_bip322_psbtto build an unsigned PSBT with the message, UTXO, and script fields set.sign_bip322_psbt_inputto detect the request, adds a partial signature (or tap_key_sig for taproot) to the first input.finalize_bip322_psbtto assemble signatures per BIP174 and encodes as afulsignature.create_to_spendinto a script-basedcreate_to_spend_from_scriptso detection can reconstructto_spendfrom a scriptPubKey.Closes #74