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

trezor: support v2.5.2+, more tests, fix chaingen, static libusub fix #8752

Merged
merged 2 commits into from
Oct 26, 2023

Conversation

ph4r05
Copy link
Contributor

@ph4r05 ph4r05 commented Feb 28, 2023

  • passphrase logic: remove backward compatibility for 2.4.3, code cleanup
  • fix LibUSB cmake for static builds on OSX, FreeBSD
  • fix Trezor support for FreeBSD, Android. Failt build if Trezor support is enabled but dependencies are missing
  • tests: all Trezor tests now work with passphrase logic enabled by default. Passphrase test added with different passphrase. no_passphrase test added, Trezor pin test added. Testing wallet opening with correct and incorrect passphrase. Trezor test chain revamp, cleanup. Smaller chain, chain file versioning added (most of the changes in the PR).
  • tests: Trezor tests support TEST_MINING_ENABLED, TEST_MINING_TIMEOUT env vars to change mining-related tests behaviour.
  • monero tests: fix tx create from sources, input locking. Originally, creating a synthetic transactions with chaingen could create a transaction with outputs that are still locked in the current block, thus failing chain validation by the daemon. Simple unlock check was added. Some buggy tests were fixed as well as new unlock-checking version of tx creation rejected those, fixes are simple - mostly using correct block after a rewind to construct a transaction

As there are occasionally reported Trezor integration bugs (monero-project/monero-gui#4105), this PR removes some old code, forces users to use the newest Trezor firmware and adds more tests.

Tests are refactored so the passphrase flow is used for all transactions, besides one special test so we cover all code paths.


Protobuf

Protobuf v21 is the latest compatible protobuf version, as Protobuf v23 requires C++17 (through its dependency Abseil library).

To disable Protobuf version check and later compilation test, define -DUSE_DEVICE_TREZOR_PROTOBUF_TEST=OFF. But the compilation may fail later with less descriptive error.

To build with unlinked protobuf@21 on OSX:

CMAKE_PREFIX_PATH=$(find /opt/homebrew/Cellar/protobuf@21 -maxdepth 1 -type d -name "21.*" -print -quit) \
make debug-test-trezor -j8

Similarly on other systems, point CMAKE_PREFIX_PATH to Protobuf v21 installation.

OSX install:

brew install protobuf@21 
brew link protobuf@21

MSYS32:

curl -O https://repo.msys2.org/mingw/mingw64/mingw-w64-x86_64-protobuf-c-1.4.1-1-any.pkg.tar.zst
curl -O https://repo.msys2.org/mingw/mingw64/mingw-w64-x86_64-protobuf-21.9-1-any.pkg.tar.zst
pacman --noconfirm -U mingw-w64-x86_64-protobuf-c-1.4.1-1-any.pkg.tar.zst mingw-w64-x86_64-protobuf-21.9-1-any.pkg.tar.zst

@ph4r05 ph4r05 marked this pull request as draft March 1, 2023 09:59
@ph4r05 ph4r05 force-pushed the pr/trezor-tests branch 8 times, most recently from 705f2bf to 6f9a485 Compare March 2, 2023 12:27
@ph4r05 ph4r05 marked this pull request as ready for review March 2, 2023 14:09
@ph4r05 ph4r05 force-pushed the pr/trezor-tests branch 3 times, most recently from 59e885e to 953aa6e Compare March 3, 2023 10:21
@ph4r05 ph4r05 changed the title trezor: support v2.5.2+, add more trezor tests trezor: support v2.5.2+, more trezor tests, fix chaingen Mar 3, 2023
@ph4r05 ph4r05 changed the title trezor: support v2.5.2+, more trezor tests, fix chaingen trezor: support v2.5.2+, more tests, fix chaingen Mar 3, 2023
@ph4r05 ph4r05 force-pushed the pr/trezor-tests branch 2 times, most recently from 205577a to b7634ce Compare March 3, 2023 14:28
@ph4r05 ph4r05 force-pushed the pr/trezor-tests branch 2 times, most recently from 04a088f to cf552cc Compare March 9, 2023 11:26
@ph4r05 ph4r05 marked this pull request as draft March 9, 2023 11:43
@ph4r05 ph4r05 force-pushed the pr/trezor-tests branch 7 times, most recently from 42e83f3 to c2ad934 Compare March 9, 2023 18:04
@ph4r05
Copy link
Contributor Author

ph4r05 commented Sep 29, 2023

ok @selsta, I've removed concurrency, will add it as separate PR.
btw do you need me to squash the commits by force push? Can it be merged with "Squash and merge"?

Screenshot 2023-09-29 at 15 26 36

@selsta
Copy link
Collaborator

selsta commented Sep 29, 2023

Yes, please squash and then force-push.

Regarding the "Squash and Merge" feature, as far as I know it means the commit signatures of the author get removed, and the maintainer would sign it with their own key. This isn't something we want.

@ph4r05
Copy link
Contributor Author

ph4r05 commented Sep 29, 2023

@selsta done

@selsta
Copy link
Collaborator

selsta commented Sep 29, 2023

Sorry for having another change request, the Trezor related changes seem good to me, but one suggestion from moneromooo is that the Trezor unrelated chaingen / core test bug fixes (if possible) should be a in a separate commit so that it can be bisected and reverted if issues are found. This is really critical code so we have to be careful here with not mixing up too many changes in a large commit.

16:30 <moneromoooo> I try to make one commit per fix when I can. It's *usually* not hard to commit a fix separately when you code a larger thing and you uncover some bug while doing so. Not *always* easy, but usually. And since this patch isn't really needed since it's just for trezor, it applies even more here.
16:31 <moneromoooo> Quite often I'll have a string of random fixes or small improvements, then the main patch that adds a feature. It makes it easier to review, bisect, revert piecemeal if needed.

@ph4r05
Copy link
Contributor Author

ph4r05 commented Sep 29, 2023

So the same PR but different commit? 2 commits then? I doubt it makes sense to separate chaingen to more commits.

Also, do we all understand that by sepparating trezor changes and chaingen causes trezor commit alone to fail tests? Bisect would have to be done on the prticular test.

@selsta
Copy link
Collaborator

selsta commented Sep 29, 2023

If the chaingen bug is unrelated to the Trezor changes then it would ideally be first commit fixes chaingen bug, second commit the Trezor changes. If the bug is intertwined with the Trezor changes then I guess it isn't possible.

@ph4r05
Copy link
Contributor Author

ph4r05 commented Sep 29, 2023

I get the point @moneromooo-monero. You are right, it makes sense to separate it to more commits.

Just to reiterate, it is not just for Trezor, the bug is present there in general and Trezor tests are testing scenarios not covered by unit tests, thus hiting edge cases when it has to be matching precisely on block boundaries.

I will get to this in following days. Trezor commit (failing tests on its own), then chaingen fixes.

@ph4r05
Copy link
Contributor Author

ph4r05 commented Sep 29, 2023

Will see what can I do @selsta. Should be fine, but it can take few days

@selsta
Copy link
Collaborator

selsta commented Sep 29, 2023

Thank you and sorry this is dragging out, just with core test changes we are usually extra careful.

@ph4r05
Copy link
Contributor Author

ph4r05 commented Sep 29, 2023

I get it, no worries. Glad we will merge it soon

- fix tx create from sources, input locking. Originally, creating a synthetic transactions with chaingen could create a transaction with outputs that are still locked in the current block, thus failing chain validation by the daemon. Simple unlock check was added. Some buggy tests were fixed as well as new unlock-checking version of tx creation rejected those, fixes are simple - mostly using correct block after a rewind to construct a transaction
@ph4r05
Copy link
Contributor Author

ph4r05 commented Sep 29, 2023

I am splitting the commits, I will commit the first one, wait for tests, then if it is all green another one (Trezor).
btw regarding approval - we dont need to code review it again, right? 😅

@jeffro256
Copy link
Contributor

Sorry to pester you more, but for reference, do you have a minimal reproducible example code that triggers the core_tests to break without the changes in this commit?

@ph4r05
Copy link
Contributor Author

ph4r05 commented Sep 29, 2023

I am sorry but I am not sure I have time and/or energy for this.

I may find something, but not sure. It is like 3 months I worked on this.

When you go through the reviews in this PR, you may find what you are looking for.

- passphrase logic: remove backward compatibility for 2.4.3, code cleanup.
- fix LibUSB cmake for static builds on OSX
- tests: all tests now work with passphrase logic enabled. Passphrase test added with different passphrase. no_passphrase test added, Trezor pin test added. Testing wallet opening with correct and incorrect passphrase. Trezor test chain revamp, cleanup. Smaller chain, chain file versioning added.
- tests: Trezor tests support TEST_MINING_ENABLED, TEST_MINING_TIMEOUT env vars to change mining-related tests behaviour.
- requires protobuf@21 on osx for now (c++14), building with unlinked protobuf: `CMAKE_PREFIX_PATH=$(find /opt/homebrew/Cellar/protobuf@21 -maxdepth 1 -type d -name "21.*" -print -quit) \
make debug-test-trezor -j8`
@ph4r05
Copy link
Contributor Author

ph4r05 commented Sep 30, 2023

@jeffro256 take for example this change, 056c996#r1238127696

Original code rewound the chain after some operations, i.e., added CRYPTONOTE_MINED_MONEY_UNLOCK_WINDOW blocks and moved it from blk_5 to blk_5r.

Then the buggy code tried to build a transaction on top of blk_5 (old block before rewinding), this does not make any sense, fund selection was broken, rewinding is precisely the reason so the transaction construction can use the funds according to the chain rules (unlock window). Creating transaction on top of blk_5 uses funds that are not yet ready (i.e. locked).

When I then pass this code to the full fledged daemon validator (thats what Trezor tests do), it fails, as it should on the real network. Also, code already rewound the chain so I believe this is a systematic error. Author IMO intended to construct a transaction after unlock has passed. IMO It makes sense to have tests that correspond to the real chain rules. Otherwise it creates confusion, passing test chain would fail real daemon validation. And the rewinding was already there. Its not like I am rewriting the whole tests so they pass the real daemon validation. I am just using correct block to build TX on. Which was I believe original intention of the author.

As commit message says

Originally, creating a synthetic transactions with chaingen could create a transaction with outputs that are still locked in the current block, thus failing chain validation by the daemon. Simple unlock check was added. Some buggy tests were fixed as the new unlock-checking version of tx creation rejected those, fixes are simple - mostly using correct block after a rewind to construct a transaction

@selsta I split the commits, each passes the tests. Do you think we can proceed with the merging? :)

@ph4r05
Copy link
Contributor Author

ph4r05 commented Sep 30, 2023

@SChernykh if you can, reapprove? 😅 If you want to diff it, this is branch you approved lately https://github.com/ph4r05/monero/tree/pr/trezor-tests-orig

this one is rebased on master, split to 2 commits. Thanks! :)

@SChernykh
Copy link
Contributor

Diff is too big now (700+ files) because of the rebase. I'll try to figure out how to get the minimal diff for review later today.

@ph4r05
Copy link
Contributor Author

ph4r05 commented Sep 30, 2023

if you rebase original branch on your own on the master, then do the diff?

@SChernykh
Copy link
Contributor

I did back-to-back comparison of master...ph4r05:monero:pr/trezor-tests-orig and master...ph4r05:monero:pr/trezor-tests and they're identical.

@selsta
Copy link
Collaborator

selsta commented Sep 30, 2023

@ph4r05 thank you, I think from your side everything is good now, I'm still trying to understand the chaingen changes and afterwards it can be approved

@ph4r05
Copy link
Contributor Author

ph4r05 commented Sep 30, 2023

Thanks @selsta!
I think tldr for chaingen is: create tx using funds that will be unlocked when tx is added to the chain so full validation passes on the chain.

@selsta
Copy link
Collaborator

selsta commented Oct 4, 2023

@ph4r05 jeffro256 is still reviewing the chain-gen changes, but it seems so far everything is good and he will approve it in the next days. Then we can add it to the merge queue.

Copy link
Contributor

@jeffro256 jeffro256 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't go in-depth for the trezor-specific tests, but I skimmed them and can confirm that at least they look reasonable and don't affect other parts of the code. As for the core tests, the changes made are correct and fix existing bugs, or add functionality for the trezor tests in a backwards-compatible way.

@ph4r05
Copy link
Contributor Author

ph4r05 commented Oct 4, 2023

Thank you all for your time!

@luigi1111 luigi1111 merged commit 8f0343d into monero-project:master Oct 26, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants