Fix: Wallet signing may finalize invalid pre-seeded signatures - #546
Naomi-Gift wants to merge 3 commits into
Conversation
|
@oleonardolima Please can you review? |
busayo-OD
left a comment
busayo-OD
left a comment
There was a problem hiding this comment.
The fix looks good. It might be worth adding regression tests covering invalid pre-seeded signatures in the affected paths.
busayo-OD
left a comment
busayo-OD
left a comment
There was a problem hiding this comment.
Thanks for the test coverage. I have a few comments on the test changes.
| /// An input that already has `final_script_sig` set must be skipped entirely — | ||
| /// the signer must not touch it, even if `partial_sigs` contains a dummy entry. | ||
| #[test] | ||
| fn sign_input_skips_already_finalized_legacy_input() { |
There was a problem hiding this comment.
This test and the sign_input_skips_already_finalized_segwit_input test cover existing behavior rather than the invalid pre-seeded signature case, so I think they and their helpers can be left out of this PR. The second commit can also be squashed into the first since it only fixes changes introduced there.
| /// A pre-seeded invalid ECDSA signature in `partial_sigs` must be replaced | ||
| /// by a valid signature after `sign_input` runs on a Legacy (P2PKH) input. | ||
| #[test] | ||
| fn sign_input_legacy_overwrites_invalid_preseeded_sig() { |
There was a problem hiding this comment.
This unit test, the sign_input_segwitv0_overwrites_invalid_preseeded_sig test, and the sign_input_tap_key_spend_overwrites_invalid_preseeded_sig test duplicate the regression coverage provided by the sign_with_signers tests, so I think they can be left out of this PR.
| } | ||
|
|
||
| #[test] | ||
| fn test_sign_single_xprv_with_master_fingerprint_and_path() { |
There was a problem hiding this comment.
Could we keep the existing test and add the regression test separately? Same for test_sign_single_xprv_bip44_path, since both are existing coverage unrelated to this fix.
2 participants
Resloves #542
Problem
sign_inputskipped signing when the PSBT already had a signature, without checking if it was valid. This could makefinalize_psbtreturnOk(true)with an invalid signature.This affected ECDSA, Taproot key path, and Taproot script path signing.
Fix
Removed the checks that skipped existing wallet-controlled signature slots. The wallet now always creates and writes a fresh, verified signature.
The checks for already finalized inputs remain unchanged.
Testing
Existing test suite passes with the guards removed. No new tests added in this PR.