Monero hackathon results #2219

matejcik · 2022-04-13T13:51:12Z

This is a significant cleanup of Monero code and a good step on the way to #642.

remove support for HF12 and below
remove MLSAG support
clean up monero cryptography naming
get rid of "optional first argument" pattern, in favor of mandatory argument that is allowed to be None
(and fix several bugs related to this feature)

One protobuf-level change is worth noting: the integer field network_type was replaced by an enum. This does not change the on-the-wire format.

@andrewkozlik or @onvej-sl please leave us a note whether it's ok to remove range_proof from the crypto library like this.

@onvej-sl you reviewed the Monero code previously so you might want to take a look; however, there are basically no functional changes here except for the removal of some steps, and the automated tests against Monero blob pass.

matejcik · 2022-04-13T13:51:36Z

@ph4r05 @mmilata @grdddj please have a look over this, whether this is what you expect to see

mmilata

Nice job with the typing and with the extmod. I scanned the changes and haven't spotted any issue.

mmilata · 2022-04-16T19:18:47Z

ci/shell.nix

@@ -20,7 +20,7 @@ let
  }) { };
  moneroTests = nixpkgs.fetchurl {
    url = "https://github.com/ph4r05/monero/releases/download/v0.17.3.0-dev-tests/trezor_tests";
-    sha256 = "sha256-tTQTe/Yk6oURq7GDOEotJ7Y2UpCgyuU5odjEHNTMrmE=";
+    sha256 = "b534137bf624ea8511abb183384a2d27b6365290a0cae539a1d8c41cd4ccae61";


This is fine

$ nix hash to-sri --type sha256 b534137bf624ea8511abb183384a2d27b6365290a0cae539a1d8c41cd4ccae61 sha256-tTQTe/Yk6oURq7GDOEotJ7Y2UpCgyuU5odjEHNTMrmE=

But we can still keep the switch to hex notation as part of this PR. Honestly, I hate the base64-notation for files and I guess hex is easier for ph4r05 to provide.

mmilata · 2022-04-16T19:42:06Z

core/src/apps/monero/xmr/serialize/message_types.py

+
+#     @classmethod
+#     def f_specs(cls) -> XmrFspec:
+#         return ()


Wanna delete this?

done in rebase

grdddj · 2022-04-19T11:19:33Z

Currently, the unused_monero.py snippet is not working as expected, because of file-system issues and the removal of the alias core/src/apps/monero/xmr/crypto/__init__.py file. I am looking at how to make it work again.

grdddj · 2022-04-19T12:57:14Z

Currently, the unused_monero.py snippet is not working as expected, because of file-system issues and the removal of the alias core/src/apps/monero/xmr/crypto/__init__.py file. I am looking at how to make it work again.

Fixed in a0ac673 (on grdddj/monero_hackathon_snippet_fix) branch)

Current results say:

3 unused functions:
ge25519_mul8 {'mapping': ['ge25519_mul8'], 'is_used': False}
xmr_add_keys2 {'mapping': ['xmr_add_keys2'], 'is_used': False}
xmr_add_keys3 {'mapping': ['xmr_add_keys3'], 'is_used': False}

It really looks like these three functions from core/mocks/generated/trezorcrypto/monero.pyi are unused and could be deleted

ph4r05 · 2022-04-19T13:16:21Z

Did you pls check also usage in tests? I recall mul8 might be used there

grdddj · 2022-04-19T13:20:34Z

Did you pls check also usage in tests? I recall mul8 might be used there

Right, that is really used in tests - I will need to modify the script to also look there. Those two others look safe to delete, I will do it in my own branch and run CI on it

andrewkozlik · 2022-04-19T16:12:15Z

@andrewkozlik or @onvej-sl please leave us a note whether it's ok to remove range_proof from the crypto library like this.

If I understand correctly, this is dead code in Trezor. In that case I am for removing it, because we won't be maintaining it. So if somebody happens to be using this function, they should fork the code and take over the maintenance.

grdddj · 2022-04-19T16:47:02Z

Those two others look safe to delete, I will do it in my own branch and run CI on it

CI is green on grdddj/monero_hackathon_snippet_fix where I removed xmr_add_keys2 and xmr_add_keys3, so we could hopefully safely merge it into this PR branch

ph4r05 · 2022-04-21T14:05:32Z

For completeness, I got notified that a new hardfork is being prepared, part of which is new Bulletproof+ protocol. I am currently studying changes required to support this protocol on Trezor.

Fallback would be to generate it on the client-side completely, but we will need new to release a new Trezor firmware anyway to support transaction signatures on a new HF (I need to check whether it will be mandatory to use BP+ on new HF or whether we can still use old versions).

Other related info:

sethforprivacy · 2022-04-21T14:23:01Z

I need to check whether it will be mandatory to use BP+ on new HF or whether we can still use old versions

BP+ will be mandatory after the hard-fork for all transactions, just to clarify and save you some searching :)

ph4r05 · 2022-04-26T13:30:35Z

Note: I've implemented BP+ verifier and prover in my new branch. I also cleaned up some existing BP code. Should we add changes to this PR @matejcik ?

matejcik · 2022-04-26T13:53:38Z

@ph4r05 please make a separate PR going to the monero-hackathon-results branch. I'll change base once this is merged.

@grdddj please push your changes into this branch.

grdddj · 2022-04-26T14:29:35Z

@grdddj please push your changes into this branch.

Done, there are four new fixups - I did not want to force-push, so they will need to be squashed

* remove support for HF12 and below * remove MLSAG support * clean up monero cryptography naming * get rid of "optional first argument" pattern, in favor of mandatory argument that is allowed to be None (and fix several bugs related to this feature) Co-authored-by: grdddj <jiri.musil06@seznam.cz> Co-authored-by: Martin Milata <martin@martinmilata.cz> Co-authored-by: matejcik <ja@matejcik.cz>

__slots__ are unsupported in micropython [no changelog]

[no changelog] Co-authored-by: Martin Milata <martin@martinmilata.cz>

[no changelog]

- implement BulletProof plus verifier and prover - use bulletproof exception to signalize proof generation failed and should be tried again. More robust, fixes bug that was not triggered yet (return tuple did not work properly in all situations) - precomputed 2**i vector is removed as it can be easily computed - BP code cleanup, minor optimizations, comments

- old BP GI, HI constants are shortened to reduce firmware size

ph4r05 · 2022-05-31T18:59:50Z

hi @matejcik , there was a change in the Monero (monero-project/monero#8277) we might need to address by a new PR. Is there pls any codefreeze date for a new firmware release so all XMR-related PRs are merged by the next release? Thanks!

matejcik · 2022-06-01T10:35:01Z

no timeframe currently.

matejcik requested review from onvej-sl, andrewkozlik and prusnak as code owners April 13, 2022 13:51

trezor-ci added this to To be reviewed in Pull Requests via automation Apr 13, 2022

matejcik requested review from mmilata and grdddj and removed request for prusnak and onvej-sl April 13, 2022 13:51

matejcik force-pushed the monero-hackathon-results branch 3 times, most recently from a91830f to ac0e1a2 Compare April 13, 2022 13:54

Pull Requests automation moved this from To be reviewed to Finalizing Apr 16, 2022

mmilata approved these changes Apr 16, 2022

View reviewed changes

ph4r05 mentioned this pull request Apr 26, 2022

XMR: HP15 support, implement BulletproofPlus, refactor and cleanup #2232

Merged

matejcik moved this from Finalizing to In progress in Pull Requests Apr 28, 2022

matejcik force-pushed the monero-hackathon-results branch 2 times, most recently from 13f60c6 to 437bef8 Compare May 11, 2022 12:54

matejcik and others added 9 commits May 16, 2022 10:46

fix(core): drop support for __slots__ from obj_eq / obj_repr

48147c8

__slots__ are unsupported in micropython [no changelog]

chore(crypto): drop support for Monero range proof

f23fa0d

chore(core/monero): update test binary to newest version

fbbd33d

[no changelog] Co-authored-by: Martin Milata <martin@martinmilata.cz>

snippet: unused Monero mapping

e197199

feat(core/tools): support @Overloads in generated mocks

a0d7536

[no changelog]

style(python/monero): improve type signature on Monero functions

9b6b260

fix(xmr): remove 8192B from firmware image so it can fit to the memory

6174243

- old BP GI, HI constants are shortened to reduce firmware size

feat(xmr): add support for HF15, BP+

f8df5ea

matejcik force-pushed the monero-hackathon-results branch from 708fe78 to f8df5ea Compare May 16, 2022 08:46

matejcik merged commit 5e6582a into master May 16, 2022

Pull Requests automation moved this from In progress to Merged May 16, 2022

matejcik deleted the monero-hackathon-results branch May 16, 2022 10:37

grdddj mentioned this pull request May 16, 2022

Mypy tests for Monero app #1883

Closed

ph4r05 mentioned this pull request May 26, 2022

feat(trezor): add HF15 support, BP+ monero-project/monero#8299

Merged

ph4r05 mentioned this pull request May 31, 2022

fix(xmr): Remove inv8 optimization as in xmr master #2314

Merged

ph4r05 mentioned this pull request Jun 20, 2022

fix(xmr): add missing view_tags to hf15 #2345

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Monero hackathon results #2219

Monero hackathon results #2219

matejcik commented Apr 13, 2022

matejcik commented Apr 13, 2022

mmilata left a comment

mmilata Apr 16, 2022

prusnak Apr 16, 2022 •

edited

mmilata Apr 16, 2022

matejcik May 11, 2022

grdddj commented Apr 19, 2022

grdddj commented Apr 19, 2022

ph4r05 commented Apr 19, 2022

grdddj commented Apr 19, 2022

andrewkozlik commented Apr 19, 2022

grdddj commented Apr 19, 2022

ph4r05 commented Apr 21, 2022 •

edited

sethforprivacy commented Apr 21, 2022

ph4r05 commented Apr 26, 2022

matejcik commented Apr 26, 2022

grdddj commented Apr 26, 2022

ph4r05 commented May 31, 2022

matejcik commented Jun 1, 2022

Monero hackathon results #2219

Monero hackathon results #2219

Conversation

matejcik commented Apr 13, 2022

matejcik commented Apr 13, 2022

mmilata left a comment

Choose a reason for hiding this comment

mmilata Apr 16, 2022

Choose a reason for hiding this comment

prusnak Apr 16, 2022 • edited

Choose a reason for hiding this comment

mmilata Apr 16, 2022

Choose a reason for hiding this comment

matejcik May 11, 2022

Choose a reason for hiding this comment

grdddj commented Apr 19, 2022

grdddj commented Apr 19, 2022

ph4r05 commented Apr 19, 2022

grdddj commented Apr 19, 2022

andrewkozlik commented Apr 19, 2022

grdddj commented Apr 19, 2022

ph4r05 commented Apr 21, 2022 • edited

sethforprivacy commented Apr 21, 2022

ph4r05 commented Apr 26, 2022

matejcik commented Apr 26, 2022

grdddj commented Apr 26, 2022

ph4r05 commented May 31, 2022

matejcik commented Jun 1, 2022

prusnak Apr 16, 2022 •

edited

ph4r05 commented Apr 21, 2022 •

edited