Rethink ButtonRequests

[mroz22]

Hi guys,

I might have found some inconsistencies worth refactoring, see:

ResetDevice call

• for model T responds with ButtonRequest_ResetDevice
• for model one responds with ButtonRequest_ProtectCall

BackupDevice call

• for model T responds with ButtonRequest_ResetDevice
• for model one responds with ButtonRequest_ConfirmWord

I would suggest to change it this way:

ResetDevice call

• for model T responds with ButtonRequest_ResetDevice
• for model one responds with ButtonRequest_ResetDevice

BackupDevice call

• for model T responds with ButtonRequest_ProtectCall
• for model one responds with ButtonRequest_ConfirmWord

[tsusanka]

We have discussed this with @mroz22 and we've concluded this needs more love. Also, this is a breaking change, so it makes sense to fix all inaccuracies at the same time. In particular, we need to:

1. discuss what backup_device and reset_device should return
2. investigate all other places if ButtonRequestType makes sense

Because this needs cooperation from the wallet team, I'm afraid this will need some meeting to discuss this. Therefore I'm moving this to backlog for now

[prusnak]

Let's also fix the case mentioned in https://github.com/trezor/trezor-firmware/issues/18

[tsusanka]

@TomasZorvan also encountered this. I'm setting higher priority because of that.

[tsusanka]

We will:

• Remove the data field, because it is most likely not being used
• code stays
• We might add some some optional structures based on the usage (Shamirdata etc.), to be defined
• Replace Other with Unknown and add a check that we do not send such ButtonRequestType (does it make sense to have it then?)
• Investigate and add new types to ButtonRequestType
• Considering adding an index to denote that we are on X-th such screen

And we might make it a bit smarter by sending it directly in ui.Layout - tracked in #643

[prusnak]

Removed the ButtonRequest.data field in https://github.com/trezor/trezor-firmware/pull/644

[matejcik]

on TT it would be useful to have a distinct (set of?) ButtonRequest codes that precede PIN entry and possibly passphrase entry. This could be used to automate PIN entry in unit tests.

[matejcik]

I'm currently thinking about "semantic ButtonRequests" that we talked about at some points.

From the layouting thing, ISTM the solution is to extend the ButtonRequest so:

message ButtonRequest {
    optional ButtonRequestType code = 1;  // technically deprecated but we probably want to keep this?
    required string type = 2;
    repeated KeyValue content = 3;

    message KeyValue {
        required string key = 1;
        optional string value = 2;
    }
}

IOW, reduce every layout to a set of string keys and string (only!) values. in addition, a string (not enum) for "type", so that this is easily extensible and can be more descriptive.

no further structuring. let's review the other possible types:

• int => Worth considering, but the usefulness is limited. We want to transmit amounts as strings, so that appropriate decimalization (and units) are retained. Weird values like locktimes, number of seconds for autolock, etc., don't make it worth the exceptional type.
• bool => Again, usefulness is limited. The usual case, an information being present, is resolved by including (or not) the key with no value. Truly boolean values can be encoded as "yes" and "no", "true" and "false", etc., depending on the context, or set by convention.
• bytes => all buttonrequest-relevant data is human readable. The key should either contain hex (or base58 or whatever is appropriate) encoding of the field, same as it reads on screen, or there should be a separate flow message to transmit the data.
• nested dicts => Let's not make things complicated. A single buttonrequest should cover a relatively small flat structure.
• arrays => In case some information repeats, the above structure allows repeating the same key. For nested arrays the above argument applies.

example:

{
    type: "bitcoin_confirm_tx",
    content: [
        { key: "output_address", value: "DTjR1yfjefmWvcdA8M8T5YcyzToWdiUP29" },
        { key: "output_amount", value: "3.1415 DOGE" },
        { key: "output_address", value: "D6eudUhUhXpG2uH8UtQzEogug1gQtXYHzr" },
        { key: "output_amount", value: "40000 DOGE" },
        { key: "change_path", value: "m/44'/3'/0'/1/0" },
        { key: "change_amount", value: "420.69 DOGE" },
        { key: "total", value: "40523.8315 DOGE" },
        { key: "fee", value: "100 DOGE" },
    ]
}

[tsusanka]

That looks great! Some quick remarks from @szymonlesisz or @mroz22?

[mmilata]

Got a few questions:

What would be the compatibility guarantees of type and content? Would wallets need to consider their values only as a hint that they should not rely on?

Since this will be used internally by the firmware apps is there a way to make it hard to include secret information in content, such as mnemonic words or PIN matrix?

Would it make sense for type to be just another key in content? I'm thinking of requests like { key: "type", value: "confirm_tx" }, { key: "coin", value: "bitcoin" }, ....

[matejcik]

What would be the compatibility guarantees of type and content? Would wallets need to consider their values only as a hint that they should not rely on?

I'd consider them as parallel to the UI itself; for minor layout changes these would stay the same, for restructuring these would change.

As for type, ISTM the hardest question is coming up with a naming scheme that's as predictable as possible, so that an UI reflow doesn't break simply on rewords. OTOH perhaps type should consistently describe content?

Would it make sense for type to be just another key in content?

That doesn't seem useful? type is the one thing you always want to send, even if the content is empty. Having a separate field enforces this, and it saves space for the string "type" which would otherwise always need to be sent.

Since this will be used internally by the firmware apps is there a way to make it hard to include secret information in content, such as mnemonic words or PIN matrix?

This is a big one for discussion. We could use a naming convention (sensitive_foo), or some sort of naming trick ($foobar ?), maybe use static typechecking so that a value can't be sent until it is explicitly cleared?

[tsusanka]

As discussed on today's meeting we would like to add progress_step and progress_total. Those value inform the Host how many pages (not only related to Pagination but in general, how many screens will occur) in this workflow.

---

So for example during a Doge transaction signing you will encounter this screen:

Confirm transaction

Send 3.1415 DOGE to
DTjR1yfjefmWvcdA8M8T5YcyzToWdiUP29

and you will receive this ButtonRequest:

{
    type: "doge_confirm_output",
        progress_step: 1,
        progress_total: 2,
    content: [
        { key: "output_address", value: "DTjR1yfjefmWvcdA8M8T5YcyzToWdiUP29" },
        { key: "output_amount", value: "3.1415 DOGE" }
    ]
}

---

On a next screen you will see:

Confirm transaction

Spend 3.1515 DOGE
including 0.01 DOGE fee.

and will receive this ButtonRequest:

{
    type: "doge_confirm_tx",
        progress_step: 2,
        progress_total: 2,
    content: [
        { key: "fee", value: "0.01" },
        { key: "output_amount", value: "3.1415 DOGE" }
    ]
}

[matejcik]

during a Doge transaction signing

I think the type should be defined not by the selected coin but by the implementing code. This is implemented by the Bitcoin app so the type will be bitcoin_confirm_output and bitcoin_confirm_tx

[matejcik]

@mmilata says:

I'm not really sure how to incorporate the requirement that ButtonRequests provide progress_step and progress_total. Tracking the steps manually in every handler would be very messy.

ISTM that "progress steps" and "number of ButtonRequests" is not necessarily the same thing. Cross-posting into this issue for @mroz22 and @szymonlesisz to comment.

There's several ways we could look at this. The most simplistic one is to say that progress_total is the number of screens that will be shown in the current flow, and progress_step is the current step number. This does not seem correct. GetAddress(show_display=True) is the obvious case: the user can flip between address and QR code as long as they want. This should be the same "progress step" regardless of how many times they do it.

Another option is to say that progress_step is a fixed property of a particular dialog. For example, when in ResetDevice flow the user is asked for PIN, that's progress step 2; if the user does not want to set PIN, step 2 is skipped and the device moves on to step 3. This makes sense for flows that do not have variable number of steps. It's problematic for e.g. Bitcoin signing where each output is a step; or for mnemonic backup/recovery, where each word is a step.

What currently seems best to me is to consider progress_total and progress_step as local properties of a particular layout type. IOW: signing flow consists of a number of phases: "verify inputs (optional)", "verify outputs", "confirm total". Each of those can have its own progress_total: number of inputs, number of outputs, always a single step for "confirm total".

Suite would need to base its progress dialogs primarily off types, with progress providing additional data. Does that make sense from your POV?

[mmilata]

Also, IIRC we talked about pageable layouts having progress_total equal to the number of pages (edit: and send ButtonRequest on every scroll). This would make progress_total depend on the content and thus being quite variable.

ISTM that "progress steps" and "number of ButtonRequests" is not necessarily the same thing.

Yeah, I suppose that can't work. Could we say that within a particular handler invocation _progress_total never changes and progress_step never decreases_?

[matejcik]

_progress_step never decreases_?

if you're paging back and forth, i'd expect progress_step to go back too

[andrewkozlik]

As discussed on today's meeting we would like to add progress_step and progress_total. Those value inform the Host how many pages (not only related to Pagination but in general, how many screens will occur) in this workflow.

Let's make sure that the solution can be used to fix https://github.com/trezor/trezor-firmware/issues/1631#issuecomment-848911956. If Suite loads during Shamir recovery, the RecoveryDevice message it sends to Trezor resets the workflow, meaning that if the user is in the middle of entering a share, they have to start entering the share again. This seems to happen even if Suite is running before Trezor is plugged in, because it takes a while for Suite and Trezor to connect and in the meantime the user may have entered a few words already. See video in https://github.com/trezor/trezor-firmware/issues/1631#issuecomment-848911956. As I understand it, resetting the flow in Trezor is required so that Suite will be able to count the button requests and show recovery progress. The necessary information should be communicated in the ButtonRequest so that Trezor can hook the RecoveryDevice message into the ongoing recovery workflow without restarting it.

[matejcik]

In #1671 I am adding page and pages fields to the ButtonRequest, indicating (a) whether the screen is paginated or not, and (b) which page the user is currently on.

IIUC this isn't currently a requirement for Suite, but it's extremely useful for device tests. Also worth noting, Monero's naive_pagination is already implementing this behavior, presumably Monero GUI is using it.

---

per this proposal, we won't be implementing any sort of global progress counters, instead these will be superseded by ButtonRequest.name string identifiers of individual screens.

It is also not a requirement to have a generic key-value map, as proposed above.

Instead, I'm thinking that we should keep an index of the current screen -- e.g., if there are multiple outputs, the following could be sent:

{ name: "confirm_amount", index: 0 }
{ name: "confirm_destination", index: 0, page: 1, pages: 2 }
{ name: "confirm_destination", index: 0, page: 2, pages: 2 }
{ name: "confirm_amount", index: 1 }
{ name: "confirm_destination", index: 1, page: 1, pages: 2 }
{ name: "confirm_destination", index: 1, page: 2, pages: 2 }

This might get complicated, e.g., in EOS, where you can send multiple actions per transaction, each of which could have multiple outputs.

Or in SuperShamir backup, where you have multiple shares from multiple groups.

Two possible solutions:

(a) the index is global: confirm_output(7) identifies output 3 of group 2, and we presume that both host and device know the size of the groups, so this can be counted correctly.

(b) we add group and the output above becomes confirm_output(index=3, group=2). The above cases are well covered by one level of nesting, and I don't think we currently need more. If we ever did, however, this becomes unwieldy.

(In SuperShamir recovery, you have (1) current word of (2) n-th share of (3) n-th group. However, the process is agnostic about which share of which group you are entering, so index would only be the number of the word of the current share.)

[matejcik]

I created a prototype in https://github.com/trezor/trezor-firmware/compare/matejcik/buttonrequests?expand=1.

Suite folks, please take a look. The testcase diff is probably most interesting for you: matejcik/buttonrequests?expand=1#diff-990799ff40

I'm not happy about the naming convention, and I'd love some actual convention. Something like type:what? bitcoin:destination, bitcoin:total, bitcoin:warn:fee_over_threshold ?

(but then the "bitcoin:" part could be a separate field in protobuf, and for that matter "warn" could also be one of an enum, say: Warning, Error, Success, Show, Confirm? This of course depends on whether anyone actually cares about this information.)

[matejkriz]

Notes from testing the prototype in Suite:

• The name param looks super useful 👍

• name + index combination is supposed to be unique.

• Would be really nice if we could get the max index. It would help a lot with automated testing (to be able to read how many times we should click)

• I'm a bit confused, that I see the same ButtonRequest code ButtonRequest_RecoveryHomepage and name recovery for two screens on TT: "Select number of words" and "Enter recovery seed" from seed check flow.

• ad Warning/Error/Success/Show/Confirm levels: I believe we don't care much, but for a reasonable decision I could imagine some user testing...

• ad bitcoin:, eth: and so on: In current Suite design we don't care about the current network so we don't have a use case for that now, but I could see in proposals for new Suite UI design that we might need it (to be able to select coin directly from send transaction flow).

[matejkriz]

meeting notes:

• The current goal is to be able to distinguish the specific step in flow (not to know the final number of steps beforehand).
  • So in Suite we should not just count the number of ButtonRequests for progress bar, but also verify the names
• Next step for @matejcik is to fill in the names in other flows (especially managing device, but also eth).
• Next step for Suite is to prepare testable branch.
• Current network (bitcoin:, eth:) won't be in the name at least for now.
• We would like to create a documentation of the new ButtonRequests
  • from FW tests screenshot of Trezor screen with associated ButtonRequest OR from Connect tests screenshot of Trezor screen together with popup context and Button request
  • short description for every ButtonRequest probably in Notion or in repo

[grdddj]

One finding, possibly worth discussing/fixing - at the end of backup process, when randomly asked to confirm three words, confirm_word() is called, which does not send any ButtonRequests.

We may want to send button requests together with the index of word (0, 1, 2)

[grdddj]

Same situation as above (not sending ButtonRequests even if we maybe wanted to) happens in the recovery process - request_word() is called, which just sends a Keyboard object.

Therefore, no indexes are being sent while doing recovery

[hynek-jina]

Waiting for feedback from Suite

[Hannsek]

We discussed this today from the Send flow perspective. We need to know what exactly Suite (@Hermez-cz, @matejkriz) and Mobile (@mnuky, @Nodonisko) need. Can you tell?

[Hermez-cz]

In Suite we'd ideally like to know about every on-device user action separately. Has the user confirmed? Unconfirmed? Scrolled/paged forward? Scrolled/paged back? Opened additional information (on TT)? clicked anything else? Let Suite ideally know, so we can respond synchronously and provide user with exact state updates and contextual information.