spesmilo / electrum

Electrum Bitcoin Wallet
https://electrum.org
MIT License
7.45k stars 3.09k forks source link

in wallet with many hardware cosigners, pairing order should be smarter (pairing error, keystore, sign_transaction, multisig) #6541

Open jlopp opened 4 years ago

jlopp commented 4 years ago

I'm doing a stress test 8 of 8 native multisig wallet with 8 different hardware devices.

Electrum 4.0.2 appimage on debian

One odd issue I've noticed when starting to sign a transaction is that it seems to roll through prompting to connect every device, but if I connect a device out of order (which is really hard to determine) then I get this error: Screenshot from 2020-08-26 18-20-11

To be precise, in this setup there are 2 different coldcards and in order to sign with the "second" coldcard I have to click "no" on the first connection attempt and THEN plug in the "second" coldcard.

Screenshot from 2020-08-28 12-40-12

Maybe this is a big ask since I'm not sure how the logic works, but it seems like electrum should be smart enough to detect that the device is actually part of the key set.

I'll also note that I have not experienced this issue with Trezor or Bitbox but have with Ledger. It might just be that I've always plugged them in in the expected order.

SomberNight commented 4 years ago

The signing logic used to simply try to sign with each keystore in order. This was annoying for e.g. a 2of3 where you had the first and the third hw cosigner connected, and you had to click through to cancel scanning for the second device. It was then changed to do a one-off scan to see what is connected, try those first, and then try the rest in order: https://github.com/spesmilo/electrum/blob/c313c702fda3d6859d3b1a94a7e2cbccf5289df7/electrum/wallet.py#L1565-L1569

Based on your report I assume you are plugging in devices ~one-by-one, hence the one-off scan does not help.

SomberNight commented 4 years ago

Hmm that does not explain your issue.

Just before the pairing error (first picture), do you a get a dialog with "Please select which Coldcard device to use"?

jlopp commented 4 years ago

Yes, I get the dialog and I select the only coldcard that is currently connected. Then it says "signing" for a few seconds before switching to the "cannot pair" error.

SomberNight commented 4 years ago

All right, in that case I understand. Let me explain the current logic, and what I think is happening:

When you click sign, a loop starts that will try to sign with each keystore. The order in which keystores are attempted: at the time "sign" was clicked, we put the "ready" keystores at the beginning (so e.g. device connected), otherwise the secondary sort order is the keystores-added-in-wizard-order.

In your case, neither coldcards were "ready" when you clicked "sign". At one point in the flow however, you connected the second coldcard. The loop arrives at the first coldcard keystore first, at which point there are several heuristics it runs to figure out which connected device might match the keystore. None of the heuristics match, so the pairing will not be done automatically, at which point a dialog is displayed to prompt the user to choose a device for the keystore. Here you select the second coldcard, which is not the correct device, and the pairing fails. Note that you could cancel this manual-select-device dialog in which case the keystore would be skipped and the flow would continue.

To explain why the user is prompted to select the device manually instead of skipping the keystore right away, I need to give an example with a Trezor device, and I need to explain the auto-pairing heuristics. The heuristics, in short, are the following: (1) we have a "device id" stored in the keystore: if there is a device connected with a matching ID, we auto-select it (note that the device id for a Trezor device is a random bytestring that is generated each time the Trezor is wiped) (2) we have a "label" stored in the keystore: if there is a device connected with a matching label (Trezors can be labelled by the user), we auto-select it Now imagine that a user lost their Trezor, bought a new one, restored it with the same seed, and set a different label on it. As the device id would also have changed, we would not auto-select this Trezor for the keystore, but prompt the user instead to select a matching device for the keystore. Then, if pairing succeeds, we overwrite the persisted device id and label. (this example does not work for e.g. coldcard as there both the label and the "device id" is actually based on the bip32 root fingerprint)

Note also that the manual-select-device dialog only lists "unpaired" connected devices.

Therefore, I suppose, ideally we should change the code somehow to get the device-connected-in-the-meantime paired first.

jlopp commented 4 years ago

That sounds correct. A couple other scenarios to note:

So it seems to me like it isn't scanning for connected devices when you click to sign an unsigned payment