common/protob: THP: required fields & separate ThpMessageType

[mmilata]

Followup of #5191. Related to #4976.

• some fields of THP messages are changed from optional to required, please check
• remove THP message IDs from MessageType enum and put them into their own ThpMessageType
• add wire_enum message attribute so that pb2py can correctly find wire type

[github-actions[bot]]

core UI changes	device test	click test	persistence test
T2T1 Model T	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
T3B1 Safe 3	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
T3T1 Safe 5	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
T3W1	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
All	main(screens)

[mmilata]

With regards to THP-TWP separation, would it help if we extended type_for_wire(wire_id) with second argument (namespace/context/app/...) that would currently be either MessageType or ThpMessageType? With disjoint wire IDs we'd only check that the ID is in the enum. Lookup in separate tables would be possible with some pb2py changes.

[matejcik]

With regards to THP-TWP separation, would it help if we extended type_for_wire(wire_id) with second argument (namespace/context/app/...) that would currently be either MessageType or ThpMessageType? With disjoint wire IDs we'd only check that the ID is in the enum. Lookup in separate tables would be possible with some pb2py changes.

yes, that's kind of what I had in mind for future extensions. for now i would just do the stupid thing with manual lookup in the enum, there's quite enough left to be done here without messing with (presumably) the binary format of the definitions

[mmilata]

~Implemented the manual lookup in 5aee681. Had to extend the protobuf blobs because it wasn't possible to look up enums by name - 8223ab7, b928433. Please take a look.~

[mmilata]

Force-pushed different version that allows you to tag any protobuf enum with option (wire_enum) = true. Wire id lookup table was extended to include 2-byte enum name Qstr which must also be passed to type_for_wire lookup function. I think the logic is less convoluted and now only one lookup is needed per message. PTAL

[M1nd3r]

@mmilata We could also duplicate some messages from MessageType to ThpMessageType: Cancel and ButtonRequest.

Notes: In http://github.com/trezor/trezor-firmware/pull/4976 in pairing.py https://github.com/trezor/trezor-firmware/pull/4976/files#diff-33acef33180f4e50401e821880709995f611242f99b211dfa10b32324130f528, there are messages that will be (in the future) checked against the ThpMessageType enum. -> I suspect that changes might be needed in the context.py functions like

async def maybe_call(
    msg: protobuf.MessageType, expected_type: type[LoadedMessageType]
) -> None:

as they currently accept only the MessageType. The function maybe_call is used in interact to send ButtonRequest. There are also other options, eg. passing the actual context to the interact.

Details can and will be discussed later. Nevertheless, the messages Cancel and ButtonRequest can be part of the ThpMessageType.

[mmilata]

@M1nd3r added Cancel and ButtonRequest (no ButtonAck?) in 71d0ffe1d8686654ea886ef6a84001483fcefa86

I think protobuf.MessageType is unfortunately named but different thing than trezor.enums.MessageType so no changes should be needed as long as same messages have the same wire id in all enums?

[M1nd3r]

@M1nd3r added Cancel and ButtonRequest (no ButtonAck?) in 71d0ffe

I think protobuf.MessageType is unfortunately named but different thing than trezor.enums.MessageType so no changes should be needed as long as same messages have the same wire id in all enums?

1. You are right about ButtonAck, we should probably add that too. But it does not need to happen in this PR. (We will learn about all missing messages when the separation of enums is implemented in the thp branch.)
2. I think you are right - as long as ThpMessages are part of protobuf.MessageType, and we will pass the correct enum into the type_for_wire in the right context, it should work just fine.

[mmilata]

cc @szymonlesisz - we modified messages.proto and messages-thp.proto a bit, not sure if it affects you. On binary level this should stay compatible with the previous definitions.