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: integration tests #4977

Merged
merged 1 commit into from Mar 14, 2019

Conversation

@ph4r05
Copy link
Contributor

commented Dec 13, 2018

This PR adds integration tests with the Trezor emulator.

  • Tests Key image sync and transaction signing.
  • Several transaction signing scenarios with varying #utxo, types of utxo (received to sub-address, received with additional tx keys), # of outputs, types of outputs (normal, to sub-address, to multiple addresses), payment id.

How it works

  • Tests generate valid blockchain (init step) and then spends mined funds with the Trezor (test step).
    • Init step: In order to save CPU time, the common blockchain is generated before all tests are run. Before each test, the blockchain is rewound to the original state. Core verifies only new blocks on the blockchain added by the test being run.
    • Test step: There are several testing scenarios added for testing transaction signature on Trezor. Scenarios differ in number of UTXO, outputs and other transaction parameters to verify multiple TX signing codepaths. Testing Trezor transactions are generated according to the test parameters by a TrensactionBuilder and sent to Trezor for signing.
    • Each transaction test tries to spend some funds by signing TX with the Trezor. Transaction is later verified.
  • Generated transactions are validated by the Monero core (daemon logic), like tests that are already in place.
  • Generated transactions are checked whether they are well-formed and whether the received funds are spendable. Tests directly use tools::wallet2 for this.

I needed to get RCT and Bulletproof transaction working with chaingen.cpp functions so I had to do some overhaul of the chaingen. The changes can either stay as they are or I can move this modified version of chaingen to the trezor tests directory. Depends on the reviews.

The common blockchain created at the beginning of the Trezor tests can be saved to a file so the it is not generated with each test run. The argument --chain_path provides path where to save/load the common chain. The #5122 is required for chain save/load work correctly.

Testing

You first need to get Trezor emulator running. Ideally follow official building documentation.

Emulator can run in the Docker. I will add instructions later.

Summary for Trezor emulator build:

git clone --recursive https://github.com/trezor/trezor-core.git
cd trezor-core
make vendor

# ubuntu / debian:
sudo apt-get install scons libsdl2-dev libsdl2-image-dev

# fedora:
sudo yum install scons SDL2-devel SDL2_image-devel

# osx:
brew install scons sdl2 sdl2_image pkg-config

# build and start emulator
make build_unix

# for build without UI run
make build_unix_noui

# run emulator
PYOPT=0 make emu

To build and run Monero integration tests:

cd -
make debug-test-trezor
./build/$os/$branch/debug/tests/trezor/trezor_tests
@ph4r05 ph4r05 referenced this pull request Dec 13, 2018
26 of 31 tasks complete

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/tests branch from 61c45c5 to a22997d Dec 13, 2018

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2018

After going through the PR again I am seriously thinking about small refactoring, namely:

  • Separate changes to separate chaingen files, just for trezor tests. So it is independent, added as a separate file. This will be much easier to review and less prone to errors and regressions as now - large amount of edits in existing chaingen.

  • Wallet2 friend class thining. I need to use Wallet in the tests and to call small number of private methods. Now I use testing friend class for that. I am thinking about reducing this to minimum with clean API - private methods will be re-exported in the Wallet_Accessor_Test, while the test wallet logic will be moved to a separate class so accessor does really only testing adapter on private methods.

What do you think @moneromooo-monero ?

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2018

It will take me a bit to have an opinion, given 3k lines.

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/tests branch 4 times, most recently from 36fd6fa to 3fd9c5e Dec 14, 2018

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/tests branch from 3fd9c5e to c657612 Jan 2, 2019

@moneromooo-monero
Copy link
Contributor

left a comment

Sorry for the delay. I've not looked at the test stuff yet.

src/device_trezor/device_trezor_base.hpp Show resolved Hide resolved
src/device_trezor/trezor/transport.hpp Show resolved Hide resolved
src/device_trezor/trezor/transport.cpp Show resolved Hide resolved
src/device_trezor/device_trezor.cpp Outdated Show resolved Hide resolved
src/device_trezor/device_trezor_base.cpp Outdated Show resolved Hide resolved
src/device_trezor/device_trezor_base.cpp Outdated Show resolved Hide resolved
m_features = client_exchange<messages::management::Features>(msg);
}

void device_trezor_base::load_device(const std::string & mnemonic, const std::string & pin,

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Jan 15, 2019

Contributor

wipeable_string ?

This comment has been minimized.

Copy link
@ph4r05

ph4r05 Feb 13, 2019

Author Contributor

This function is applicable only with debugging Trezor firmware, basically with an emulator for integration tests. It cannot be used on production so I thought normal strings would do the job here.

src/device_trezor/trezor/transport.cpp Show resolved Hide resolved
src/wallet/wallet2.h Outdated Show resolved Hide resolved
@ph4r05

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

Thanks for the review. I am a bit slammed at the moment but I will get to that on 28th Jan.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2019

Tests: this is pretty chunky, but it seems OK overall, though I read through quickly. Feel free to do whatever cleanup you mentioned, but overall this seems fine. Some explanations of the main changes to the core test machinery would be nice, the core test code is hard to understand (for me anyway).

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/tests branch 3 times, most recently from 6bb5728 to 575eea2 Feb 13, 2019

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

I am working on getting tests back running after the rebase. I will ping once it's done.

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/tests branch from 575eea2 to c5abb96 Feb 14, 2019

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

OK tests are passing again

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/tests branch from c5abb96 to 271272f Feb 20, 2019

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

I've added chain serialization/deserialization so the initial blockchain that is required for subsequent payment tests is not generated with each test run. The argument --chain_path provides path where to save/load the common chain.

The #5122 is required for chain save/load work correctly.

@moneromooo-monero I've updated PR description to better explain what is going on in the tests.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

I needed to get RCT and Bulletproof transaction working with chaingen.cpp functions so I had to do some overhaul of the chaingen.

That seems odd. There are rct and BP core tests, and they pass.

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

I needed to get RCT and Bulletproof transaction working with chaingen.cpp functions so I had to do some overhaul of the chaingen.

That seems odd. There are rct and BP core tests, and they pass.

I mean, the transactions in the tests are constructed manually (the transaction is constructed in quite a detailed way, setting inputs and so on). My motivation for changes was to add an easy way to create various transaction types on the chain for the further testing.

chaingen.{h,cpp} did not provide API to generate RCTv2 transactions on the blockchain (as far as I know, I didn't fint it). Like there is a macro MAKE_TX_MIX_LIST which creates a transaction that is later added to the block. I made MAKE_TX_MIX_LIST_RCT which enables to generate v2 transactions on the blockchain, with bulletproofs. The idea of this is to spend RCTv2 transactions in Trezor, which I could not generate with changen before in an easy way. (alternative was to do it manually).

With the modifications I made to the chaingen it is easy to generate various transactions on the chain (like using subaddresses, RCTv2), example of the chain generation: https://github.com/ph4r05/monero/blob/271272f91bd13a8e85ff2fd066cc7fbf53e2f65a/tests/trezor/trezor_tests.cpp#L696

Then tests make use of various transactions present on the blockchain so the TX signature test covers more code paths.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

Ah, OK, that might be true.
I'll review that when I have some time for it, hopefully soon.

@moneromooo-monero
Copy link
Contributor

left a comment

I think I'm fine with it generally. I don't like wallet2.h being included in the chaingen.h though, I see it wants transfer_details now, anything else ? If not, given transfer_details is not wallet specific, maybe it could be moved in cryptonote_core somewhere. It'd also please endogenic I think :)

tests/core_tests/chaingen.cpp Outdated Show resolved Hide resolved
tests/core_tests/chaingen.cpp Outdated Show resolved Hide resolved
tests/core_tests/chaingen.cpp Outdated Show resolved Hide resolved
tests/core_tests/chaingen.cpp Outdated Show resolved Hide resolved
tests/core_tests/chaingen.cpp Outdated Show resolved Hide resolved
tests/core_tests/chaingen.cpp Outdated Show resolved Hide resolved
tests/core_tests/chaingen.cpp Outdated Show resolved Hide resolved

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/tests branch 3 times, most recently from f8a806b to 2f0ebfb Feb 28, 2019

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

Wallet2 is quite used in the chaingen now, e.g., creating transactions by passing wallet as a source of the transaction, processing new blocks with the wallet code. Would you rather prefer to separate wallet-related code to a new file?

I've added another commit to this PR which separates wallet code from the chaingen. Wallet dependent code was moved to a new files wallet_tools.{cpp,h} which is then included by trezor_tests.h.

If you think this new approach is more clean I can squash it then.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2019

I think it makes more sense, yes. Thanks.

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/tests branch from f854e0e to 78cbfea Mar 2, 2019

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

I like it more like this also. Squashed.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2019

Anyone else planning to review this ?

@ph4r05 ph4r05 force-pushed the ph4r05:trezor/tests branch from 78cbfea to 5ea1790 Mar 5, 2019

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@moneromooo-monero I noticed a discussion on the IRC and I think these tests could be easily modified to support also Ledger. The core could be the same, just the transaction signature would go to the Ledger instead of the Trezor.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

Are you saying you're willing to do that ? If so that's great :) This is probably going to get merged soon, since I kinda doubt anyone is really going to review as well, so another PR on top would be best, unless you think a lot of shuffling around is needed first.

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Well... I can certainly help with that but I am busy to do it on my own 😅
It was more like suggestion for Ledger guys.

Regarding the shuffling - I think this is OK for now. The heavy changes has been made in chaingen and wallet_tools.

@moneromooo-monero
Copy link
Contributor

left a comment

Alright, let it be approved for now, and it'll get merge soon unless someone else wants to review asap.

@fluffypony
Copy link
Collaborator

left a comment

Reviewed

@fluffypony fluffypony merged commit 5ea1790 into monero-project:master Mar 14, 2019

6 of 10 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details
buildbot/monero-static-win32 Build done.
Details
buildbot/monero-static-win64 Build started.
Details
buildbot/monero-linux-armv7 Build done.
Details
buildbot/monero-linux-armv8 Build done.
Details
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-amd64 Build done.
Details
fluffypony added a commit that referenced this pull request Mar 14, 2019
Merge pull request #4977
5ea1790 device/trezor: debugging features, trezor tests (Dusan Klinec)

@ph4r05 ph4r05 deleted the ph4r05:trezor/tests branch Apr 15, 2019

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