Monero protocol messages added

[ph4r05]

Initial Monero Protocol messages:

• get watch only credentials
• key image sync
• transaction signature

[prusnak]

Let's drop the m_ prefix.

[ph4r05]

Yep why not. I took the field naming from the original Monero code but I don't like it either so I am happy to change it.

[prusnak]

Let's drop the m_ prefix.

[prusnak]

Do we need this in production code? If not, I would rename these messages to contain Debug prefix to make it obvious they don't belong to production.

[ph4r05]

I needed to discuss this. It is not needed for the production code.
So prefixing with Debug and leaving it in the messages-monero.proto is OK?

[prusnak]

The question is whether we still need these messages or if they were only useful for development and are no longer necessary once the code has been written. If they are still needed, let's use the Debug prefix and keep them in messages-monero proto file. If they are not needed, I would drop them completely.

[prusnak]

What's the difference between Tsx and Tx in message names?

[ph4r05]

Tsx vs Tx, I guess it is kind of mixed from multiple sources, but should mean the same, Transaction. Would you prefer unification to Tx?

[prusnak]

Let's unify to Transaction. There is no reason to use abbreviations in message/field names.

I would also expand Addr -> Address, Rct -> RingCt. The more descriptive names, the better.

Also I don't understand the MoneroTsxMlsagDone name.

[ph4r05]

The Mlsag stands for Multilayered Spontaneous Anonymous Group Signatures, the protocol step corresponds to the
https://github.com/monero-project/monero/blob/a844844cda9a0beb62d95015f92327366d73b3a1/src/ringct/rctSigs.cpp#L715

The long message name version would thus be MoneroTransactionMultilayeredSpontaneousAnonymousGroupSignaturesDone or better MoneroTransactionPreMultilayeredSpontaneousAnonymousGroupSignaturesHashDone.

[ph4r05]

I think I will need some benchmarking w.r.t. abbreviations expansions. I am not sure whether to expand MLSAG either. Rct -> RingCt makes sense, RingConfidentialTransaction would be a bit messy.

The question is whether aforementioned message should be named MoneroTransactionPreMultilayeredSpontaneousAnonymousGroupSignaturesHashDone and how long message identifiers can protoc handle. I would prefer mlsag abbreviation.

Some field names are taken directly from the Monero serialization schemes, e.g.,
https://github.com/ph4r05/monero-serialize/blob/1a582fbfdf0990c5f9155897207d71188373383e/monero_serialize/xmrtypes.py#L795

The thing is I would like to preserve message field names for these serialization schemes as data could be serialized also in JSON and having different field names could ruin the deserialization and cause pain later.

This is the case for RPC JSON and KV serialization used e.g., in the wallet files. These cases affect monero-glue code, not trezor-core.monero but the question is whether we want to have different field names for same messages here.

E.g. I used field name real_out_tx_key , used in https://github.com/monero-project/monero/blob/709658d166712aea0af870716899534306a836cf/src/cryptonote_core/cryptonote_tx_utils.h#L61

However, considering only protobuf protocol messages - they can be quite isolated from Monero scheme messages. So we can do big changes without much pain. But the code in trezor-core is more tightly coupled to classical Monero scheme messages.

[prusnak]

I agree that we don't want to expand everything and it makes sense to leave the field names as is - to reflect their counterparts in Monero code.

I propose the following changes:

a) change Tx and Txs to Transaction in message names
b) change MoneroSubAddrIndicesList to MoneroSubAddressIndicesList
c) change MoneroRctSig to MoneroRingCtSig

To discuss:

d) can we drop MoneroRespError completely? - we don't have specific Error messages for any other coins and we don't want to leak exceptions from Trezor anyway (as they might contain sensitive info) - so we usually return Failure(FirmwareError) message in most of the cases

e) most of other coins follow the FooRequest/FooAck scheme, while you use Foo/FooResp scheme - I agree that the original scheme is not ideal, that's why I am not pushing for it too hard, but maybe we can use that for consistency

What do you think?

[ph4r05]

Yep makes sense! I am on it.

d) definitely, in production there should not be stack trace. I can remove this message completely.

e) I can switch it to FooRequest/FooAck, no problem.

[ph4r05]

refactoring finished

[prusnak]

I merged the PR, thanks!

I did some refactoring after the merge too (in a5e6dff ):

1. I used nested messages, we haven't use this before, but it really unclutters the definition by hiding definitions used in exactly one place into the message itself.

2. I dropped the @prev directive from the comments, it's superfluous.

I don't think we need MoneroTransactionSignRequest and MoneroKeyImageSyncRequest. Why not use the messages they wrap directly? If you I agree, I'll just drop them from the definition.

[ph4r05]

Thanks for the merge! Regarding the wrapping messages - I find it more clear to have the whole protocol encapsulated in the particular wrapping message. There is only one message handler registered for the particular purpose (tx sign, ki sync) and the protocol sub-message multiplexing is hidden in the protocol handler. For tx sign I would need to register 7 different request handlers, route it to the single tx sign handler and do the message multiplexing again. I found this approach more messy. The protocols for tx sign and ki sync are stateful, so it also made sense to tie the messages together in the wrapping request message and hide undrelying multistep logic in ththe handler. Moreover it is easy to extend the protocol with the common request fields if needed in the future. Regarding the embedded messages - I will check how the imports work in the trezor-core code. If that works well after the changes. But I guess the embedded messages are still OK to import. ne 22. 7. 2018 v 20:07 odesílatel Pavol Rusnak <notifications@github.com> napsal:

…

I merged the PR, thanks! I did some refactoring after the merge too (in a5e6dff <a5e6dff> ): 1. I used nested messages, we haven't use this before, but it really unclutters the definition by hiding definitions used in exactly one place into the message itself. 2. I dropped the @Prev directive from the comments, it's superfluous. I don't think we need MoneroTransactionSignRequest and MoneroKeyImageSyncRequest. Why not use the messages they wrap directly? If you I agree, I'll just drop them from the definition. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#164 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABAQWaUBAKqiEBcl0WBw7PpgRxNFWaHpks5uJL92gaJpZM4VLDMl> .

[ph4r05]

Maybe the wrapping messages will make more sense after I make PR to trezor-core so the benefit is visible from the code.

[prusnak]

Regarding the embedded messages - I will check how the imports work in the trezor-core code.

We did the same trick for other messages and currently, there is no difference in trezor-core code whether the messages are nested or not.

Maybe the wrapping messages will make more sense after I make PR to trezor-core so the benefit is visible from the code.

OK.