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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
May 11, 2022
Merged

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

merged 3 commits into from
May 11, 2022

Conversation

ph4r05
Copy link
Contributor

@ph4r05 ph4r05 commented Apr 26, 2022 •
edited
Loading

Changes:

  • Add HP15 support
  • implement BulletProof plus verifier and prover. BP+ will be mandatory in upcoming hard-fork.
  • 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 on the fly
  • BP code cleanup, minor optimizations, comments

Based on PR #2219

A new hardfork is being prepared, part of which is a new Bulletproof+ protocol.

Other related info:

Technical note

I had to add new BP+ GI, HI constants. This increases firmware memory footprint by 16kB, TWO vector was removed, so total 14 kB increase. Until the new hard-fork is activated, we have to keep both old and new constants to make computation possible. If there is a problem with memory, we can shorten older BP constants by half (which causes longer generation times as the rest has to be costly computed on-the-fly). In that case the total extra size would be 6144 B.

@ph4r05 ph4r05 force-pushed the monero-bpplus-sq branch 4 times, most recently from 795c06a to 206e0b7 Compare April 26, 2022 19:02
@ph4r05
feat(xmr): implement bp+, refactor, code cleanup
b3b3d31 
- 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
@ph4r05 ph4r05 force-pushed the monero-bpplus-sq branch from 206e0b7 to b3b3d31 Compare April 26, 2022 19:26
@ph4r05
Copy link
Contributor Author

ph4r05 commented Apr 26, 2022

When I tried to install firmware with this new BP+ version the image was too big to fit to the flash memory. For now I removed upper 4096B from old GI, HI vectors. Everything works the same, but verification time for larger transactions will take longer

@ph4r05
Copy link
Contributor Author

ph4r05 commented Apr 26, 2022 •
edited
Loading

Interestingly, after device starts, I get HP internal error with "please, unplug the device" error message. Ideas how to diagnose it?

The same bug is also on the branch 5b2ab09, so I presume it is a problem of #2219. Can you pls try building it and running on a real device to check if we can replicate it? Thanks! :)

What is worrying is that after the device prints that error, it starts sending random keypresses to the connected PC, mostly moving cursor around and scrolling. Does it behave like USB keyboard? This looks quite funny / suspicious.

UPDATE: Once I rebased this branch on master, problem was fixed.

@ph4r05 ph4r05 force-pushed the monero-bpplus-sq branch from 7793246 to c78ee86 Compare April 26, 2022 22:43
@ph4r05
Copy link
Contributor Author

ph4r05 commented Apr 27, 2022

Another thing - I need to update protobuf defs in order to support new HF. But it needs to be merged so I can open Monero Wallet PR. Any ideas on a timeframe for the PR being merged? :)

@matejcik
Copy link
Contributor

Any ideas on a timeframe for the PR being merged? :)

Sometime next week. Probably after the freeze on Tuesday.

@ph4r05 ph4r05 force-pushed the monero-bpplus-sq branch from c78ee86 to ff13988 Compare April 27, 2022 14:33
@ph4r05 ph4r05 changed the title XMR: implement BulletproofPlus, refactor and cleanup old Bulletproof code XMR: HP15 support, implement BulletproofPlus, refactor and cleanup Apr 27, 2022
@ph4r05
Copy link
Contributor Author

ph4r05 commented Apr 27, 2022 •
edited
Loading

OK update - I've implemented HF15 support now. All required changes are pushed in this PR, no protobuf change is required for Monero Wallet to work.

Update: I have to test BP verification on 16 inputs on a real device, there is a small memory problem here.

@ph4r05 ph4r05 marked this pull request as draft April 27, 2022 14:45
@ph4r05 ph4r05 force-pushed the monero-bpplus-sq branch 2 times, most recently from 1c0dd59 to bc94e3c Compare April 27, 2022 16:53
@ph4r05 ph4r05 marked this pull request as ready for review April 27, 2022 16:53
@ph4r05
Copy link
Contributor Author

ph4r05 commented Apr 27, 2022 •
edited
Loading

OK I solved it. Monero wallet related changes: monero-project/monero#8299

Device tests pass on a real device in max setting, HF13 (now) and HF15 (july, BP+).
Once HF15 is active, we can remove a lot of unused code related to old BulletProofs (half of the bulletproofs.py file mostly and 8kB of constants).

I will build trezor_tests blob soon.

@ph4r05 ph4r05 mentioned this pull request Apr 27, 2022
Merged
@ph4r05
fix(xmr): remove 8192B from firmware image so it can fit to the memory
b9cce8e 
- old BP GI, HI constants are shortened to reduce firmware size
@ph4r05 ph4r05 force-pushed the monero-bpplus-sq branch 2 times, most recently from f98a2c7 to 67f52b3 Compare April 27, 2022 17:15
@sethforprivacy sethforprivacy mentioned this pull request Apr 28, 2022
Closed
79 tasks
@sethforprivacy
Copy link

Thank you so much for all of the hard work on this, @ph4r05! Great to see such top-notch support for Monero on Trezor.

@ph4r05
Copy link
Contributor Author

ph4r05 commented Apr 28, 2022

Thanks @sethforprivacy! It was really fun implementing Bulletproof+ for Trezor :) Also thanks for pinging us ahead of time

@ph4r05
Copy link
Contributor Author

ph4r05 commented Apr 28, 2022 •
edited
Loading

@matejcik new testing binary blob that is able to test both HF13 and HF15 https://github.com/ph4r05/monero/releases/tag/v0.17.3.2-dev-tests-u18.04-01

It would be awesome if we could also fix this for a next release #2217. It also slows down tx sign procedure if there are more UTXOs.

@ph4r05 ph4r05 force-pushed the monero-bpplus-sq branch from 67f52b3 to d8876f8 Compare April 28, 2022 16:01
@ph4r05
Copy link
Contributor Author

ph4r05 commented Apr 28, 2022

Another thing, I hard-coded to the monero wallet that firmware version 2.5.1 supports HF15. Should a new firmware with the PR included have higher version, pls let me know so I change it in the monero PR. thanks!

@prusnak prusnak removed their request for review April 28, 2022 21:46
@matejcik
Copy link
Contributor

Another thing, I hard-coded to the monero wallet that firmware version 2.5.1 supports HF15. Should a new firmware with the PR included have higher version, pls let me know so I change it in the monero PR. thanks!

💩 didn't notice this one.
We are releasing 2.5.1 which does not include this PR. Please bump to 2.5.2 in the monero PR.

@matejcik matejcik merged commit d3092f5 into trezor:monero-hackathon-results May 11, 2022
@ph4r05 ph4r05 mentioned this pull request May 31, 2022
Merged
@ph4r05 ph4r05 mentioned this pull request Jun 20, 2022
Merged
@bosomt
Copy link

bosomt commented Aug 4, 2022

@ph4r05 Is there anything that can/need to be tested on our side for 2.5.2 or discovery/send/receive of TX is enough ?

https://www.exploremonero.com/transaction/d605e8a1417035aceb60514c4cea54feb3e9af8dc740e93ac220379a79f33531

Device: model T 2.5.2 Universal (revision 0d87b55ba4fed7eecc72bf2a94ee473830b095e9)

@matejcik
Copy link
Contributor

matejcik commented Aug 5, 2022

it will be enough, but afer HF15 activates on Aug 13 https://www.getmonero.org/2022/04/20/network-upgrade-july-2022.html

@bosomt
Copy link

bosomt commented Aug 17, 2022

QA OK

  • moneroGUI 0.18.1.0-release (Qt 5.12.8)
  • Device: model T 2.5.2 regular (revision 0d87b55)
  • txid 1c45ce86d6ade9d7574a93e4514d8dccee909195c7b1ea0bc5a6ad54919e8a98

image

@bosomt
Copy link

bosomt commented Aug 17, 2022

One question, im not sure ho MoneroGUI works but i rather ask if its OK.
When i press show address on device i see completely different address :)
But it belongs to same account /i just sent some monero to that address and i still have same amount ;)

Screenshot 2022-08-17 at 11 04 29

image

@prusnak
Copy link
Member

prusnak commented Aug 17, 2022

When i press show address on device i see completely different address :)

Please open another issue for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matejcik matejcik matejcik left review comments

@onvej-sl onvej-sl Awaiting requested review from onvej-sl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ph4r05 @matejcik @sethforprivacy @bosomt @prusnak