Feature/trezor reconnect by OBorce · Pull Request #1874 · mintlayer/mintlayer-core

OBorce · 2025-01-30T17:25:38Z

If the trezor device returns an USB IO error it might mean that the trezor device has been disconnected, in that case try to reconnect to the device and try the failed operation again.

azarovh

Needs a rebase

ImplOfAnImpl · 2025-02-12T16:15:27Z

+            Err(trezor_client::Error::TransportSendMessage(
+                trezor_client::transport::error::Error::Usb(rusb::Error::Io),
+            )) => {
+                let (mut new_client, data) = find_trezor_device()?;


Calling find_trezor_device on each operation is not a great idea. What if another device is returned?

we check the public keys again, and if it is a different device we error out.

we check the public keys again, and if it is a different device we error out.

I understand this, the point was that it looks conceptually wrong.

Currently, the question is why do we need it if you already call init_device every time. Can't it recover the session after a usb error?

And even if it can't, calling find_trezor_device still seems wrong. Perhaps then you should split find_trezor_device into 2 functions - the "find" part and the "connect" part, so that the former is still called only once.

Perhaps then you should split find_trezor_device into 2 functions - the "find" part and the "connect" part, so that the former is still called only once.

I guess having a separate "connect" part won't be possible, because AvailableDevice is non-copyable. So we'll have to stick to calling find_trezor_device again.

Currently, the question is why do we need it if you already call init_device every time. Can't it recover the session after a usb error?

Mainly for the case if the USB device disconnects for whatever reason.

ImplOfAnImpl · 2025-02-12T16:20:02Z

+            Err(trezor_client::Error::TransportSendMessage(
+                trezor_client::transport::error::Error::Usb(rusb::Error::Io),
+            )) => {


IMO instead of calling the operation and then checking for error we should just unconditionally call Trezor::initialize, passing to it the previosly obtained session_id (it's inside features). At least this is recommended in the docs.

added an init_device(session_id) call which internally calls initialize before each operation.

ImplOfAnImpl

This PR is incomplete without changes to trezor_signer from #1895
I suggest moving those changes here (I mean the parts that are relevant for the "automatic" device selection), so that they can be reviewed together.

ImplOfAnImpl · 2025-03-21T15:10:18Z

+fn find_trezor_device() -> Result<(Trezor, TrezorData, Vec<u8>), TrezorError> {
    let mut devices = find_devices(false)
        .into_iter()
        .filter(|device| device.model == Model::Trezor || device.model == Model::TrezorEmulator)


So, what about TrezorLegacy aka Model One?

So, why don't we allow TrezorLegacy here?

Added but I don't think it is supported, when I run the emulator it doesn't list the Mintlayer compatibility so it will alwasy be filtered out in the next step.

ImplOfAnImpl · 2025-03-21T15:33:42Z

+            Ok(_) => Ok(()),
+            // In case of a USB IO error try to reconnect, and try again
+            Err(trezor_client::Error::TransportSendMessage(
+                trezor_client::transport::error::Error::Usb(rusb::Error::Io),


I tried disconnecting the device physically and re-connecting it and the error I got here after that was rusb::Error::NoDevice.
So, it should be handled here as well.

I wonder if the error is device-dependent though (I use Safe 3).
Perhaps we should just catch all trezor_client::transport::error::Error::Usb errors here?

P.S. plz also update comments - you currently mention "I/O error" everywhere.

Added matching on all USB errors

ImplOfAnImpl · 2025-03-21T17:54:07Z

+            Err(trezor_client::Error::TransportSendMessage(
+                trezor_client::transport::error::Error::Usb(rusb::Error::Io),
+            )) => {
+                let (mut new_client, data) = find_trezor_device()?;


Perhaps then you should split find_trezor_device into 2 functions - the "find" part and the "connect" part, so that the former is still called only once.

I guess having a separate "connect" part won't be possible, because AvailableDevice is non-copyable. So we'll have to stick to calling find_trezor_device again.

ImplOfAnImpl · 2025-03-27T16:38:55Z

+                if let Some(devices) = response.multiple_devices_available {
+                    match devices {
+                        wallet_rpc_lib::types::MultipleDevicesAvailable::Trezor { devices } => {
+                            let choices = CreateWalletDeviceSelectMenu::new(
+                                devices,
+                                wallet_path,
+                                CliHardwareWalletType::Trezor,
+                                true,
+                            );
+                            return Ok(ConsoleCommand::ChoiceMenu(Box::new(choices)));
+                        }
+                    }
+                }


I wonder whether we should do the same on OpenWallet. The scenario is that the user resets their device, changing its device id. And then connects this device and another one and tries opening a previously created wallet. Currently this will fail with "multiple connected Trezor devices found".
The scenario is not very probable, but it makes sense to support it at least for completeness.

P.S. but if you do it, plz write a comment explaining why we do such things on OpenWallet, because it'll look non-obvious.

azarovh · 2025-04-09T12:33:48Z

+        /// Optionally specify the ID for the trezor device to connect to in case there
+        /// are multiple trezor devices connected at the same time.
+        /// If not specified and there are multiple devices connected a choice will be presented
+        #[arg(long, conflicts_with_all(["mnemonic", "passphrase", "whether_to_store_seed_phrase"]))]


it should require hardware_wallet

fixed in separate PR #1907

azarovh · 2025-04-09T12:36:50Z

+        /// are multiple trezor devices connected at the same time.
+        /// If not specified and there are multiple devices connected a choice will be presented
+        #[arg(long, conflicts_with_all(["mnemonic", "passphrase", "whether_to_store_seed_phrase"]))]
+        trezor_device_id: Option<String>,


it seems like this string should be either device_id or included into HardwareWalletType::Trezor. Otherwise what happens when we add new wallet type?

fixed in separate PR #1907

OBorce requested review from ImplOfAnImpl, azarovh and erubboli as code owners January 30, 2025 17:25

OBorce force-pushed the feature/trezor-recconect branch 2 times, most recently from 4f6708d to e0f00b5 Compare January 31, 2025 07:32

OBorce force-pushed the refactor/wallet-generic-signer branch from 69592f8 to f16c3e3 Compare February 3, 2025 10:47

azarovh approved these changes Feb 3, 2025

View reviewed changes

Comment thread wallet/src/signer/trezor_signer/mod.rs Outdated

OBorce force-pushed the feature/trezor-recconect branch 2 times, most recently from 8a33777 to f8578ef Compare February 3, 2025 16:46

ImplOfAnImpl requested changes Feb 12, 2025

View reviewed changes

OBorce force-pushed the refactor/wallet-generic-signer branch 2 times, most recently from 82b71a8 to c9c1c91 Compare February 25, 2025 07:54

OBorce closed this Feb 25, 2025

OBorce force-pushed the feature/trezor-recconect branch from f8578ef to c9c1c91 Compare February 25, 2025 11:20

OBorce reopened this Feb 25, 2025

OBorce force-pushed the feature/trezor-recconect branch 2 times, most recently from 2df0787 to cf6ce8c Compare February 26, 2025 12:35

OBorce force-pushed the refactor/wallet-generic-signer branch from 48716c3 to 67609d5 Compare February 28, 2025 08:15

OBorce force-pushed the feature/trezor-recconect branch from cf6ce8c to 66a4f17 Compare February 28, 2025 08:55

OBorce force-pushed the refactor/wallet-generic-signer branch from 67609d5 to 220ace1 Compare February 28, 2025 09:58

Base automatically changed from refactor/wallet-generic-signer to master March 3, 2025 16:17

OBorce force-pushed the feature/trezor-recconect branch 2 times, most recently from 0401329 to 001afc6 Compare March 6, 2025 06:38

OBorce requested a review from ImplOfAnImpl March 14, 2025 03:32

ImplOfAnImpl reviewed Mar 21, 2025

View reviewed changes

ImplOfAnImpl mentioned this pull request Mar 21, 2025

Add selection of trezor device #1895

Closed

OBorce marked this pull request as draft March 25, 2025 10:00

OBorce force-pushed the feature/trezor-recconect branch 3 times, most recently from 09952ef to bc2d7f8 Compare March 26, 2025 10:00

OBorce marked this pull request as ready for review March 26, 2025 10:00

OBorce mentioned this pull request Mar 26, 2025

Add support for signing with standalone keys to Trezor #1890

Closed

ImplOfAnImpl approved these changes Mar 27, 2025

View reviewed changes

OBorce force-pushed the feature/trezor-recconect branch 2 times, most recently from 0ad3d75 to e8d0ed6 Compare April 1, 2025 15:06

azarovh approved these changes Apr 9, 2025

View reviewed changes

OBorce force-pushed the feature/trezor-recconect branch from e8d0ed6 to 844ee5b Compare April 11, 2025 12:34

OBorce added 9 commits April 14, 2025 16:39

Try to reconnect to the trezor device on IO error

d0e8687

fix comments and catch all USB errors for Trezor reconnect

a70c988

fix comments and catch all USB errors for Trezor reconnect

fc5631e

Add support for signing with standalone keys to Trezor

02ec704

fix comments

07dfbbf

Add selection of trezor device

517bb36

fix comments

4fd1af7

add trezor device selection on open wallet

8ad2b20

fix comments

37def6a

OBorce force-pushed the feature/trezor-recconect branch from 844ee5b to 37def6a Compare April 14, 2025 14:53

OBorce merged commit 440dd12 into master Apr 21, 2025

OBorce deleted the feature/trezor-recconect branch April 21, 2025 08:47

Conversation

OBorce commented Jan 30, 2025

Uh oh!

azarovh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ImplOfAnImpl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants