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

Add busy screen #2445

Merged
merged 4 commits into from
Aug 22, 2022
Merged

Add busy screen #2445

merged 4 commits into from
Aug 22, 2022

Conversation

andrewkozlik
Copy link
Contributor

@andrewkozlik andrewkozlik commented Aug 11, 2022

This PR implements a new SetBusy message in core which causes Trezor T to show a "Do not disconnect" screen instead of the standard homescreen or lockscreen. Suite will need to display this screen at the moment before it commits to participate in a CoinJoin round. The message should appear for about 2 minutes, after which the CoinJoin transaction will be signed. During these two minutes Trezor is just waiting for the signing phase. It is possible for other operations to be called, but UI workflows should be discouraged, because if the UI workflow is running when Suite needs to sign the CoinJoin, then the workflow will be interrupted. Applications should check the busy parameter in Features and prevent the user from launching a UI workflow if busy == True, e.g. by disabling the "Receive" button in Suite.

The message has an optional parameter expiry_ms that sets the time after which the "Do not disconnect" screen will automatically disappear. This is to prevent the message from appearing on the Trezor forever in case the desktop app shuts down in the middle of the CoinJoin workflow. Applications should explicitly hide the busy screen when the workflow is complete by calling SetBusy with expiry_ms=0 or not set.

In the future we can add an enum to SetBusy which will indicate the reason for the busy state. Right now it only makes sense to show CoinJoin as the reason.

image

@andrewkozlik andrewkozlik added core Trezor Core firmware. Runs on Trezor Model T and T2B1. R&D Research and development team related labels Aug 11, 2022
@andrewkozlik andrewkozlik force-pushed the andrewkozlik/busyscreen branch from 2e3d11b to 15daebe Compare August 11, 2022 11:23
@andrewkozlik andrewkozlik marked this pull request as ready for review August 11, 2022 12:18
@andrewkozlik andrewkozlik removed the request for review from prusnak August 11, 2022 12:18
@andrewkozlik
Copy link
Contributor Author

One thought that came to my mind after creating this PR is that it might be good to replace Features.busy with Features.busy_session, which would either be None or would contain the busy session's ID. That way trezor-connect would know which session initiated the busy state and it would not allow any other sessions to initialize non-silent workflows, such as GetAddress with show_display=true. On the other hand for CoinJoin in the busy state even the busy session does not need to initiate any non-silent workflows. So this might be over-engineering for some future use-case which we don't have yet. Or do we? I think for now trezor-connect should just block all non-silent operations on Trezor if Features.busy==true, regardless of which session they come from.

In case we decide to use Features.busy_session, we could store only the busy session's index number in the sessionless cache, so that we don't store the entire session ID. In start_session() we could then prevent the algorithm from selecting the busy session as the least recently used session. Again maybe that's just overengineering, since it's unlikely to happen, but if the cached passphrase got lost, then it would break the CoinJoin. Come to think of it, maybe we should introduce a rule into start_session() which will prevent the session from being selected as the least recently used if APP_COMMON_AUTHORIZATION_DATA is set.

@matejcik
Copy link
Contributor

which would either be None or would contain the busy session's ID.

we currently consider the session id to be (weakly) private so directly exposing an existing session id is a bad idea

in general, I'm wondering, is there a strong reason to allow anything during the busy state? You could just send DoPreauthorized and then wait for 2 minutes before responding with the actual action -- during which time, Trezor would be inside a workflow and showing this screen.

@andrewkozlik
Copy link
Contributor Author

andrewkozlik commented Aug 15, 2022

You could just send DoPreauthorized and then wait for 2 minutes before responding with the actual action -- during which time, Trezor would be inside a workflow and showing this screen.

That was actually my initial implementation, see e1252ca. It is indeed quite simple in its minimal form.

When @szymonlesisz implemented this, he actually decided to cancel the "Do not disconnect" screen just before signing and then run a fresh DoPreauthorized message for the signing. As I recall the reason for this is that the approach is more error-proof in case the device gets taken over by another application while waiting for the signing phase.

The other thing is that if another application sends in a GetFeatures message or silent GetAddress, e.g. for a testnet address, as trezor-connect often does, then the "Do not disconnect" dialog will be brought down. So we'd have to whitelist certain messages, at least GetFeatures, Cancel and the ones that can be preauthorized and then block all other messages. Also, for GetFeatures keep the DoPreauthorized state. Similarly U2F/FIDO2 would bring the dialog down, so we'd need to handle that too somehow.

Then in discussion with @prusnak I think we agreed that it would be nicer if Trezor would normally respond to GetPublicKey and similar silent requests. Come to think of it, I'd say in some cases it is legitimate to process non-silent calls, like U2F/FIDO2. The worst that can happen is that the FIDO screen will be interrupted when CoinJoin signing begins and the FIDO login will fail.

My impression is that if we want to make the simple solution robust, it actually becomes way more complicated, but I haven't tried. Whereas the solution in this PR seems robust out of the box, I hope.

Finally there are some thoughts around scalability. If we allow simultaneous CoinJoins for two accounts on the same Trezor (different passphrase each), then one account can be in the critical phase, while the second account is registering inputs (GetOwnershipProof) and switching session IDs.

python/src/trezorlib/device.py Outdated Show resolved Hide resolved
python/src/trezorlib/cli/device.py Outdated Show resolved Hide resolved
tests/device_tests/bitcoin/test_authorize_coinjoin.py Outdated Show resolved Hide resolved
@andrewkozlik andrewkozlik force-pushed the andrewkozlik/busyscreen branch from fd02ad8 to 7d8ea6c Compare August 22, 2022 15:13
@andrewkozlik andrewkozlik merged commit f253d7e into master Aug 22, 2022
@andrewkozlik andrewkozlik deleted the andrewkozlik/busyscreen branch August 22, 2022 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Trezor Core firmware. Runs on Trezor Model T and T2B1. R&D Research and development team related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants