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

device/trezor: HF10 protocol upgrade #5211

Merged
merged 3 commits into from Mar 21, 2019

Conversation

Projects
None yet
4 participants
@ph4r05
Copy link
Contributor

ph4r05 commented Feb 28, 2019

HF10 protocol upgrade for Trezor.
Related to trezor/trezor-core#478

I am sorry for a larger PR, I thought it is easier for you to review all at once as it is easier to see dependencies and why particular choices have been made.

HF 10

  • Most of the changes relate to the deterministic output masks in HF10. Masks are generated when a destination is sent to the Trezor, this affects how BP offloading is done.
  • Transaction Inputs Commitments and outputs commitments balancing is now performed in the signing step of the protocol, thus pseudo_outs are updated in the sign step as Trezor adjusts masks so their sums equal.
  • We discontinued support for Borromean range signatures so I removed the related logic from the client protocol code (offloading, verification, serialization).
  • in_memory variant of the protocol which enabled to do signature a bit faster for standard transactions was removed. Trezor and Monero client code is now cleaner, safer (fewer branches). Speedup was not significant.
  • When all users upgrade to new Monero and Trezor firmwares, support for old protocol (client_version=0) can be dropped.
  • Key image import after the cold signing imports only key images computed by the Trezor, i.e., key images of spent inputs.
  • Minor refactoring of the Trezor client code (consts), passphrase entry refactored.
  • key image import only for device-computed inputs

Get TX key

  • Trezor supports get_tx_key for decrypting stored tx private keys.
  • wallet2::get_tx_key_device() calls get_tx_key on the Trezor if applicable
  • Simplewallet supports get_tx_key and get_tx_proof on hw device using the get_tx_key feature

Live refresh

  • Live refresh feature enables to do the refresh with Trezor, i.e., computing key images on the fly. More convenient and efficient for users (hw_key_image_sync was required before).
  • The first live refresh requires user confirmation on the device.
  • A thread is watching whether live refresh is being computed. If it is not for 30 seconds, it terminates the live refresh process - switches the Trezor state (display changes).

Wallet API

  • Initial Trezor support added so Monero GUI supports Trezor.
  • Wallet API Works with Monero-GUI (monero-project/monero-gui#2019)
  • Functional tests of the Wallet::API cold signing and refresh via mocked core RPC daemon (could be also used in different tests later). Mocker RPC daemon is created with the test cryptonote::core and runs on a local port. Wallet::API uses RPC daemon.

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/hf10 branch 2 times, most recently from 5b3dced to 816d5e4 Mar 5, 2019

@ph4r05 ph4r05 referenced this pull request Mar 5, 2019

Open

[WIP] Trezor integration roadmap #4810

26 of 31 tasks complete

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/hf10 branch 9 times, most recently from 3c9b61a to e42b7e1 Mar 5, 2019

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/hf10 branch from e42b7e1 to 4f0e0ee Mar 14, 2019

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Mar 14, 2019

OK this PR is ready for review :)

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Mar 14, 2019

@moneromooo-monero I've split code to more commits to make the review easier. I hope that helps.

Most of the code relates to the HF10 upgrade (and protocol simplification) + tests for HF9 and HF10.

I am wondering... as we have HF10/11 now, should I drop tests for HF9?

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/hf10 branch 6 times, most recently from 2d7fa51 to abf3fb5 Mar 14, 2019

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Mar 15, 2019

ToDo for me: Check if compatible with:

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/hf10 branch from abf3fb5 to 256014a Mar 15, 2019

@binaryFate

This comment has been minimized.

Copy link
Contributor

binaryFate commented Mar 15, 2019

I find it a bit messy if some crypto functions like hmac_keccak_init are introduced with the sole purpose of Trezor integration, but are not identified as such by name or namespace, or at least by being in their own file.

Over time we may lose track of what crypto stuff has been introduced by who and for what purposes, and rely on it in a slightly different context without giving it the appropriate level of scrutiny.

@binaryFate

This comment has been minimized.

Copy link
Contributor

binaryFate commented Mar 15, 2019

Also would it be possible to have files currently in src/device_trezor/ to be part of a submodule? It is messy if we end up with 20 directories part of the main repo, each one for a different hardware provider.

Surely we can't move everything, but right now it seems like a lot of things ported to the Monero code base itself in a very intertwined manner.

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Mar 15, 2019

I find it a bit messy if some crypto functions like hmac_keccak_init are introduced with the sole purpose of Trezor integration, but are not identified as such by name or namespace, or at least by being in their own file.

I thought more like the HMAC is a universal and useful crypto function which is easy to review if the underlying Keccak is correct. And it’s covered by tests.

Anyway, if you think that moving it out from the crypto to a separate folde then OK.
Any suggestions on naming and names space ? @moneromooo-monero ?

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Mar 15, 2019

Also would it be possible to have files currently in src/device_trezor/ to be part of a submodule? It is messy if we end up with 20 directories part of the main repo, each one for a different hardware provider.

Surely we can't move everything, but right now it seems like a lot of things ported to the Monero code base itself in a very intertwined manner.

Moving to a submodule would bring some benefits, however it could make the review a bit pain as the dependencies would be harder to spot.

I usually do submodules from pieces of code that can live on its own but Trezor integration is highly dependent on Monero core code. I don’t know.

I would prefer to leave it in the main repository but if the other guys (e.g. @moneromooo-monero ) agree on the submodule variant and create the repo for it I will move it there.
The question then is how to review this PR.

Show resolved Hide resolved src/crypto/keccak.c Outdated
Show resolved Hide resolved src/crypto/keccak.c Outdated
Show resolved Hide resolved src/crypto/keccak.c Outdated
Show resolved Hide resolved src/crypto/keccak.c Outdated
Show resolved Hide resolved src/crypto/keccak.c Outdated
Show resolved Hide resolved src/crypto/keccak.c Outdated
@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

moneromooo-monero commented Mar 15, 2019

MRL people are pro HMAC in general I believe, so if this is a real HMAC, it should be exposed in general. src/crypto is the right place for it, I'd put it in a separate file though.

About submodule or not for the trezor code, I think it's best to have it in tree since it's really closely intertwined. It's just a lot of code changes to review every time this changes for some reason ^_^

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/hf10 branch from 456b03b to f4bde67 Mar 17, 2019

Show resolved Hide resolved tests/trezor/daemon.cpp Outdated
Show resolved Hide resolved tests/trezor/daemon.cpp
Show resolved Hide resolved tests/trezor/daemon.h Outdated
Show resolved Hide resolved tests/trezor/trezor_tests.cpp Outdated
Show resolved Hide resolved tests/unit_tests/hmac_keccak.cpp Outdated
Show resolved Hide resolved tests/unit_tests/hmac_keccak.cpp Outdated

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/hf10 branch 8 times, most recently from b87374a to 2ad7b3b Mar 17, 2019

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Mar 20, 2019

So I've analyzed it with Coverity and it seems fine. (If you cannot see the analysis result I can invite you to the project).

Is there anything left for me to do on this PR? :)

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Mar 20, 2019

Hmm Iam trying clang sanitizers, the race in live_refresh monitor thread is still there according to the thread sanitizer. I am fixing it now...

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/hf10 branch from 2ad7b3b to 4bc27a5 Mar 20, 2019

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

moneromooo-monero commented Mar 20, 2019

I had finished the review. It can go in next.

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Mar 20, 2019

I've did small fix on the thread and the race on m_transport

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Mar 20, 2019

I had finished the review. It can go in next.

Great, thanks a lot! So now just a formal PR approve?

ph4r05 added some commits Mar 3, 2019

device/trezor: HF10 support added, wallet::API
- import only key images generated by cold signing process
- wallet_api: trezor methods added
- wallet: button request code added
- const added to methods
- wallet2::get_tx_key_device() tries to decrypt stored tx private keys using the device.
- simplewallet supports get_tx_key and get_tx_proof on hw device using the get_tx_key feature
- live refresh enables refresh with trezor i.e. computing key images on the fly. More convenient and efficient for users.
- device: has_ki_live_refresh added
- a thread is watching whether live refresh is being computed, if not for 30 seconds, it terminates the live refresh process - switches Trezor state
tests/trezor: HF9 and HF10 tests
- tests fixes for HF10, builder change, rct_config; fix_chain
- get_tx_key test
- proper testing after live refresh added
- live refresh synthetic test
- log available funds for easier test construction
- wallet::API tests with mocked daemon

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/hf10 branch from 4bc27a5 to c9b13fb Mar 20, 2019

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Mar 20, 2019

@moneromooo-monero, 😅
During the testing, I've noticed a small race in live refresh monitor. I've simplified the locking logic and fixed mutex locking. Now tests are OK for me. That should be all now. Sry.

Thanks for the check!

@fluffypony
Copy link
Collaborator

fluffypony left a comment

Reviewed

@fluffypony fluffypony merged commit c9b13fb into monero-project:master Mar 21, 2019

3 of 10 checks passed

buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-static-osx-10.13 Build done.
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details
buildbot/monero-static-win32 Build done.
Details
buildbot/monero-static-win64 Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
buildbot/monero-linux-armv7 Build done.
Details
buildbot/monero-linux-armv8 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details

fluffypony added a commit that referenced this pull request Mar 21, 2019

Merge pull request #5211
c9b13fb tests/trezor: HF9 and HF10 tests (Dusan Klinec)
a1fd1d4 device/trezor: HF10 support added, wallet::API (Dusan Klinec)
d74d26f crypto: hmac_keccak added (Dusan Klinec)
@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Mar 21, 2019

@fluffypony thanks! 🎉 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.