Monero support added [for review]

[ph4r05]

The first review attempt for Monero code.

Code can be tested with monero-agent as with the previous PoC:
https://github.com/ph4r05/monero-agent

Documentation for PoC (section "Testing with Trezor"):
https://github.com/ph4r05/monero-agent/blob/master/PoC.md

For testing you may actually need my forks as I use my dependencies with PRs merged (update submodules after clone).
https://github.com/ph4r05/trezor-core/tree/xmr-total-full

Integration docs:
https://github.com/ph4r05/monero-trezor-doc

This PR depends on PRs:

• xmr: monero crypto functions, tests trezor-crypto#169

---

Code review TODO

• import tests into python-trezor/travis
• split tx builder steps into seperate files (@tsusanka)
• rethink how to tie the steps together (@tsusanka, @jpochyla)
• modify iface to layout based on other coins (@tsusanka)
• move hmac/enc keys to seperate file (@tsusanka)
• remove multisig (@ph4r05)
• simplify (de)serialization code (@jpochyla)
• remove most of xmr/crypto.py (@ph4r05)
• review the package structure (@jpochyla)
• remove the lite protocol (@ph4r05)
• investigate hashers (keccak_hasher, mlsag_hasher) (@jpochyla)
• thoroughly review signing steps (@tsusanka)
  • 1
  • 2
  • 3
  • 4
  • 5
  • 6
  • 7
  • 8
  • 9
  • 10
• state.in_memory?
• common.ct_equal? (@jpochyla)
• ring_ct, mlsag2 (@tsusanka)
• bulletproofs (Ondra + Andrew)
• batch_size < output_count test (@ph4r05)
• key image sync (@jpochyla)
• monero.py (@tsusanka)

[ph4r05]

Will be removed

[tsusanka]

@ph4r05 I'll be doing some sort of preliminary review with @jpochyla doing review of my review later :). Consider it more as questions/suggestions. First, I would like to clarify few things:

• The https://github.com/ph4r05/trezor-core/tree/xmr-total-full branch includes this PR alongside with all dependencies merged, is that correct?
• What is the difference between https://github.com/ph4r05/trezor-core/tree/xmr-total-full and https://github.com/ph4r05/trezor-core/tree/xmr-total-2 ? Is the xmr-total-2 the same just with the commits squashed and no dependencies?
• If I would like to test it all, I can use the xmr-total-full branch and follow the instructions in monero-agent:PoC.md@master, correct?
• Is it okay if I merge this into a 'monero' branch in the very same manner as @prusnak has done with trezor-crypto? It feels bit hostile to replace your PR with mine, but it does make modifications easier.

[ph4r05]

OK thanks!

• xmr-total-full branch is basically my development branch with lots of commits and with my custom git submodule forks of trezor-common and trezor-crypto with all required stuff merged in.

• xmr-total-2 is all monero work done in one squashed commit, rebased on top of the master. It has original git submodules so this branch tests are failing, e.g. there is no monero functionality in trezor-crypto.

• Regarding the testing - yes. If you fork my xmr-total-full with all my submodules and follow PoC.md@master you should be able to both run trezor-specific tests (16 testing transactions) and sign transactions with agent as you did in the PoC phase of the project.

• PR merge - if the proposed way is easier for you then I think its OK. I am not sure how github / @prusnak is doing it in xmr: monero crypto functions, tests trezor-crypto#169 but there is also my avatar alongside the modified commits so its fine :)

[ph4r05]

ad PoC: if you had setup the environment for PoC previously just skip to Testing with Trezor section in PoC.md

[ph4r05]

BTW

• is there a way to set a timer after reception of a message, e.g. for a minute so if no next protocol message is received within the timeout the whole state is wiped out?

• what is the safest way to wipe out secrets from the memory allocated in the python code? E.g., spending keys for UTXO would be nice to zero out before garbage collecting. Is the currently only way to do it manually?

[tsusanka]

@ph4r05 there is no such file trezor-crypto/monero/crypto.c as required in SConscript.firmware. This is probably due to trezor/trezor-crypto#169 (comment). I've modified the line to trezor-crypto/ed25519-donna/ge25519.c, however the build still fails due to "ROTL32" redefined error. Would you like to look into it?

PS: you can use the monero branch to see the error or see travis

[ph4r05]

Hmm I think this PR is not directly usable with trezor/trezor-crypto until all changes are merged. As there are still ongoing changes and review process I decided not to keep it up-to-date with the trezor/trezor-crypto until the merging and review is finished.

For the testing purposes, you may use the branch ph4r05/xmr-total-full with working trezor-crypto changes. It uses my fork of the library with merged monero changes.

That should also fix the "ROTL32" redefined error as it is defined in ed25510-donna.h which did not have include-once ifndef

[tsusanka]

Ok fair enough. Let's wait then. @prusnak could you answer @ph4r05's questions above?

[tsusanka]

note: the monero branch includes this PR and is rebased on top of the current master

[prusnak]

is there a way to set a timer after reception of a message, e.g. for a minute so if no next protocol message is received within the timeout the whole state is wiped out?

No, this is not yet possible.

what is the safest way to wipe out secrets from the memory allocated in the python code? E.g., spending keys for UTXO would be nice to zero out before garbage collecting. Is the currently only way to do it manually?

Your request sparked an idea, which I implemented here: ebf912c

Since our modtrezorcrypto objects implement memzero descructors in __del__, we can use the following syntax to acheive what you want:

from trezor.crypto import SecureContext

with SecureContext() as sc:
    sc.var1 = ...
    sc.var2 = ...

Later we might move this variables to different parts of the CPU memory (CCRAM instead of main RAM) or something even more sofisticated.

[ph4r05]

I've refactored transaction signing so it uses protob serialization where suitable - to avoid cryptonote serialization for complex messages which are not forward compatible.

[ph4r05]

From now I will stop squashing commits after each change. I will squash it to one eventually after the review.

[ph4r05]

@tsusanka Change address is not confirmed by user now, it is verified to match primary account address (i.e., subaddress with index (account, 0)).

[ph4r05]

@tsusanka I've added tests for Monero Cryptonote serializer. For that I needed to add support for async test cases, which is also in a separate PR:

#319

EDIT: I've added some basic Monero crypto tests as well.

[ph4r05]

Trezor-crypto related: range_proof.{c,h} and the corresponding unit test can be now removed as I deprecated C-implementation of the Borromean range proof due to heap fragmentation problems. Python implementation is almost as fast as the C implementation (using in-place operations).

[tsusanka]

I suggest to get rid of this function and write it directly, because:

• it is used only twice and the second instance actually uses the use_r argument
• the use_r argument is confusing anyway
• it is called gen_r() but it actually sets the r_pub as well

[tsusanka]

This is the only place where common.defval_empty is used, let's remove it.

[tsusanka]

I think it is better to write it directly, it is unclear what is being hashed.

[ph4r05]

What do you mean? You propose to unroll / remove _tprefix_update method?

[tsusanka]

Indeed.

[ph4r05]

Btw just a minor thing, after using the latest trezor-common the src Travis job started failing. Ideas? I thought it may be good to fix it before merging protobuf PR with get_fields, maybe?

1.89s$ test "$GOAL" != "src" || pipenv run make templates_check
./tools/build_templates --check
--- ./tools/../src/apps/common/coininfo.py	2018-09-17 15:11:06.743812764 +0000
+++ /tmp/tmp.CucSLkIfYY	2018-09-17 15:12:11.042755779 +0000
@@ -520,7 +520,7 @@
         address_type_p2sh=5
         maxfee_kb=10000000
         signed_message_header="Namecoin Signed Message:\n"
-        xpub_magic=0x019da462
+        xpub_magic=0x0488b21e
         xpub_magic_segwit_p2sh=None
         xpub_magic_segwit_native=None
         bech32_prefix=None
make: *** [templates_check] Error 1

[tsusanka]

@ph4r05 run ./tools/build_templates

[ph4r05]

After pip install mako munch I've got:

Traceback (most recent call last):
  File "./../vendor/trezor-common/tools/cointool.py", line 706, in <module>
    cli()
  File "/Users/dusanklinec/.pyenv/versions/3.6.5/lib/python3.6/site-packages/click/core.py", line 722, in __call__
    return self.main(*args, **kwargs)
  File "/Users/dusanklinec/.pyenv/versions/3.6.5/lib/python3.6/site-packages/click/core.py", line 697, in main
    rv = self.invoke(ctx)
  File "/Users/dusanklinec/.pyenv/versions/3.6.5/lib/python3.6/site-packages/click/core.py", line 1066, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/dusanklinec/.pyenv/versions/3.6.5/lib/python3.6/site-packages/click/core.py", line 895, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/dusanklinec/.pyenv/versions/3.6.5/lib/python3.6/site-packages/click/core.py", line 535, in invoke
    return callback(*args, **kwargs)
  File "./../vendor/trezor-common/tools/cointool.py", line 702, in render
    do_render(file, dst)
  File "./../vendor/trezor-common/tools/cointool.py", line 675, in do_render
    render_file(src, dst, defs, support_info)
  File "./../vendor/trezor-common/tools/cointool.py", line 131, in render_file
    template = mako.template.Template(filename=src)
  File "/Users/dusanklinec/.pyenv/versions/3.6.5/lib/python3.6/site-packages/mako/template.py", line 283, in __init__
    "of the root path." % self.uri)
mako.exceptions.TemplateLookupException: Template uri "../src/apps/common/coininfo.py.mako" is invalid - it cannot be relative outside of the root path.

[tsusanka]

Mmmm, maybe cd tools; ./build_templates?

[ph4r05]

Aah I was originally in the tools/ dir, now I tried ./tools/build_templates and that worked.

[ph4r05]

Hmm I think there is a small issue with generated mocks at the moment.
The trezorcrypto mock is one file but maybe it should actually be a package which contains submodules like monero.

Why: A function add256_modm defined in modtrezorcrypto-monero.h is in the generated mock in trezorcrypto.add256_modm, not in trezorcrypto.monero.add256_modm as it should be.

A Quick & dirty hack is to class-scope all packages in modtrezorcrypto:

class monero:

    @staticmethod
    add256_modm(a, b): 
        """Scalar addition"""

    class Ge25519:
        """EC point""""

Now the mock import / typing should work.

[ph4r05]

Tests failing due to:

../../../../.local/share/virtualenvs/trezor-core-h18d73Mo/src/trezor/trezorlib/tests/device_tests/test_msg_signtx_decred.py:268: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../../../.local/share/virtualenvs/trezor-core-h18d73Mo/src/trezor/trezorlib/btc.py:200: in sign_tx
    res = client.call(proto.TxAck(tx=msg))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
self = <trezorlib.debuglink.TrezorClientDebugLink object at 0x2ac4d3d0d2b0>
msg = <TxAck: {'tx': <TransactionType: {'bin_outputs': [<TxOutputBinType: {'amount': 190000000, 'script_pubkey': b'v\xa9\x14\x81\x9d)\x1a/\x7f\xbfw\x0exK\xfdx\xb5\xce\x92\xc5\x8e\x95\xea\x88\xac', 'decred_script_version': 0}>]}>}>
    @tools.session
    def call(self, msg):
>       resp = self.call_raw(msg)
E       AssertionError: Expected responses:

Maybe rebasing on master?

@jpochyla @tsusanka let me pls know once you are done with the current work in progress so I can squash and rebase on the master.

Thanks!

[tsusanka]

@ph4r05 just pushed smth. So it's a go from my side

[ph4r05]

Just to note: the builds are currently failing because of
pypa/pipenv#3026