HomePhabricator

[electrum] fix pubkey sorting when signing multisig

Description

[electrum] fix pubkey sorting when signing multisig

Summary:
Transaction.get_sorted_pubkeys used to sort keys in place in the coin dict, so when Keystore.can_sign checked whether the signature in the same position as its xpub was not None, it could assume that the pubkeys were already in the same order as the sigs.

When we switched from using Transaction.get_sorted_pubkeys(coin_dict) to using TxInput.get_sorted_pubkeys() in most places, while letting some code still work with coin dicts, we broke the highly undocumented assumption that pubkeys would be sorted "in-place" at the time of signing the transaction. Keystore.can_sign could return False if some signature was placed in the position in which its pubkey was mistakenly located (the signatures were always put in the correct order by Transaction.sign)

Also Transaction._sign_txin updated the pubkey in the TxInput based on its sorted index, when it was actually not sorted in-place.

As a result, multisig signing was sometimes broken depending on the order in which the process of creating and signing the transaction happened. The keystore would sometimes mistakenly assume that it already had done its signing job, and the pubkeys were foobared (one pubkey duplicated, one missing).

Fix this by:

  • using TxInput in keystore and accessing the xpubkeys via TxInput.get_sorted_pubkeys, so we don't need to assume that previous code did the sorting
  • only updating the pubkey in the very specific case where it is required, which happens to be when there is only a single pubkey
  • sorting the pubkeys in TxInput.to_coin_dict, just in case there is some other piece of code still using coin dicts (Transaction.inputs()) and making the same assumption.

Test Plan:
Send multiple transactions with a multisig setup, making sure that the order of the involved pubkeys varies compared to the order of the keystores (I patched the transaction dialog with a print(tx.txinputs()[0].pubkeys to verify).

python test_runner.py

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D14606

Details

Provenance
PiRKAuthored on Oct 5 2023, 14:33
PiRKPushed on Oct 6 2023, 14:39
Reviewer
Restricted Project
Differential Revision
D14606: [electrum] fix pubkey sorting when signing multisig
Parents
rABC755e0fa69c34: [electrum] fix x_pubkey format in multisig wallet
Branches
Unknown
Tags
Unknown