Skip to content
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

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

Open
jlopp opened this issue Aug 28, 2020 · 5 comments
Labels
hw-generic related to hardware wallets, irrespective of manufacturer topic-wallet 👛 related to wallet.py, or maybe address_synchronizer.py/coinchooser.py

Comments

@jlopp
Copy link

jlopp commented Aug 28, 2020

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
Copy link
Member

SomberNight commented Aug 28, 2020

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:

electrum/electrum/wallet.py

Lines 1565 to 1569 in c313c70

# sign. start with ready keystores.
for k in sorted(self.get_keystores(), key=lambda ks: ks.ready_to_sign(), reverse=True):
try:
if k.can_sign(tmp_tx):
k.sign_transaction(tmp_tx, password)

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

@SomberNight SomberNight added hw-generic related to hardware wallets, irrespective of manufacturer topic-wallet 👛 related to wallet.py, or maybe address_synchronizer.py/coinchooser.py labels Aug 28, 2020
@SomberNight
Copy link
Member

SomberNight commented Aug 28, 2020

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
Copy link
Author

jlopp commented Aug 28, 2020

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
Copy link
Member

SomberNight commented Aug 28, 2020

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
Copy link
Author

jlopp commented Aug 28, 2020

That sounds correct. A couple other scenarios to note:

  • If this coldcard is plugged in when I start Electrum, it finds the coldcard and puts it in a ready state, then it correctly gets prompted to sign the transaction
  • If I don't have the coldcard plugged in when I start Electrum, then plug it in, then click "pay" on this unsigned payment, the device still isn't detected and put into the ready state and it steps through the above described flow that ends in the pairing error.

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

@SomberNight SomberNight changed the title Coldcard pairing error in wallet with many hardware cosigners, pairing order should be smarter (pairing error, keystore, sign_transaction, multisig) Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hw-generic related to hardware wallets, irrespective of manufacturer topic-wallet 👛 related to wallet.py, or maybe address_synchronizer.py/coinchooser.py
Projects
None yet
Development

No branches or pull requests

2 participants