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

Add support for Zcash unified addresses - memory optimized #2398

Merged
merged 1 commit into from
Aug 30, 2022
Merged

Add support for Zcash unified addresses - memory optimized #2398

merged 1 commit into from
Aug 30, 2022

Conversation

krnak
Copy link
Contributor

@krnak krnak commented Jul 23, 2022

specification: https://zips.z.cash/zip-0316
optimization: f4jumble now uses C blake2b module

@krnak krnak requested a review from matejcik as a code owner July 23, 2022 06:16
@krnak krnak force-pushed the zcash-unified-addresses-2 branch from cc876ce to 61dd5d2 Compare July 23, 2022 06:19
@krnak krnak mentioned this pull request Jul 23, 2022
Closed
@krnak
Copy link
Contributor Author

krnak commented Jul 25, 2022 •
edited
Loading

This PR is ready for review @andrewkozlik . I don't understand why CI core fw btconly build fails. There is no error message.

@andrewkozlik andrewkozlik self-requested a review July 25, 2022 15:29
prusnak
prusnak requested changes Jul 25, 2022
View reviewed changes
core/src/apps/zcash/f4jumble.py Outdated Show resolved Hide resolved
core/src/apps/zcash/unified_addresses.py Outdated Show resolved Hide resolved
core/src/apps/zcash/unified_addresses.py Outdated Show resolved Hide resolved
@andrewkozlik
Copy link
Contributor

I don't understand why CI core fw btconly build fails. There is no error message.

At the end of the log you can see

o%trezor/enums/ZcashReceiverTypecode.py
trezor/enums/ZcashReceiverTypecode.py

This is the output of tools/check-bitcoin-only. We should clarify the error message.

If you run the following commands you will get the same result.

BITCOIN_ONLY=1 make build_firmware
strings core/build/firmware/firmware.bin | grep Zcash

I think what you need to do is look at SConscript.firmware and SConscript.unix and exclude trezor/enums/Zcash*.py from the Bitcoin-only build.

@krnak
Copy link
Contributor Author

krnak commented Jul 25, 2022 •
edited
Loading

Thank you for clarification Andrew. I'm removing trezor/enums/Zcash*.py from btconly build in the fixup 2a18731 .

@krnak krnak requested a review from prusnak July 26, 2022 17:31
@krnak
Copy link
Contributor Author

krnak commented Aug 2, 2022

may I squash and merge , or waiting for another review ? @andrewkozlik

matejcik
matejcik approved these changes Aug 2, 2022
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

did a brief review, two small nitpicks.

@andrewkozlik do we want to add this to the upcoming release?

core/src/apps/zcash/unified_addresses.py Outdated
return hrp.encode() + bytes(16 - len(hrp))


def encode(receivers: Dict[Typecode, bytes], coin: CoinInfo) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

please use py3.10-style annotations, i.e., in this case dict[Typecode, bytes] without importing Dict

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fcc5fd1

core/src/apps/zcash/unified_addresses.py Outdated

def receiver_length(typecode: int):
"""Byte length of a receiver."""
if typecode in [Typecode.P2PKH, Typecode.P2SH]:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: these should be tuples (Typecode.P2PKH, Typecode.P2SH) not lists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c536e71

@andrewkozlik
Copy link
Contributor

may I squash and merge , or waiting for another review ?

I will try to review today.

do we want to add this to the upcoming release?

Is there any reason not to?

@matejcik
Copy link
Contributor

matejcik commented Aug 2, 2022

Is there any reason not to?

the freeze is today; my question is, are you as the main reviewer confident enough in this code to include it?

@andrewkozlik
Copy link
Contributor

the freeze is today; my question is, are you as the main reviewer confident enough in this code to include it?

I don't think we need to force this into the release, since Suite integration of unified addresses is not on the roadmap yet. So I am not sure this could be used in production by anyone in the nearest future.

On the other hand if we happen to merge this today somehow, then it's fine to include this in the freeze. (My apologies for not getting this reviewed earlier. I had a lot on my plate.)

@prusnak
Copy link
Member

prusnak commented Aug 2, 2022

Let's merge after the freeze if there is no strong reason to merge it before.

andrewkozlik
andrewkozlik approved these changes Aug 4, 2022
View reviewed changes
Copy link
Contributor

@andrewkozlik andrewkozlik left a comment

Choose a reason for hiding this comment

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

utACK

@matejcik
Copy link
Contributor

matejcik commented Aug 8, 2022

looks like this has all the approval it needs
please squash the fixups, rebase on current master, and fix the UI fixtures conflict

@krnak krnak force-pushed the zcash-unified-addresses-2 branch 2 times, most recently from d872ac0 to f17d25a Compare August 9, 2022 06:34
@krnak
Copy link
Contributor Author

krnak commented Aug 9, 2022

core fw regular debug build:

region `FLASH2' overflowed by 1024 bytes

😢

@andrewkozlik
Copy link
Contributor

Also, the address in test_unified_address has Invalid receivers order.

region `FLASH2' overflowed by 1024 bytes

Strange, I still have 4 kB left when I build with PYOPT=0 poetry run make -C core build_firmware on my machine.

code_length: 1696256
-rw-rw-r-- 1 andrew andrew  1699840 Aug  9 10:14 firmware.bin
-rwxrwxr-x 1 andrew andrew   786432 Aug  9 10:14 firmware.bin.p1
-rwxrwxr-x 1 andrew andrew   913408 Aug  9 10:14 firmware.bin.p2

@krnak krnak force-pushed the zcash-unified-addresses-2 branch from e738f24 to d1373e2 Compare August 12, 2022 06:47
@andrewkozlik andrewkozlik force-pushed the zcash-unified-addresses-2 branch from d1373e2 to a84ceab Compare August 30, 2022 14:04
@andrewkozlik
Copy link
Contributor

I rebased over the current master branch, which has ECMULT_GEN_PREC_BITS reduced from 4 to 2 to decrease the size of the precomputed tables for secp256k1-zkp, leaving more space in the flash memory.

@andrewkozlik andrewkozlik merged commit 031bac4 into master Aug 30, 2022
@andrewkozlik andrewkozlik deleted the zcash-unified-addresses-2 branch August 30, 2022 19:15
@sime sime mentioned this pull request Nov 18, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prusnak prusnak prusnak approved these changes

@matejcik matejcik matejcik approved these changes

@andrewkozlik andrewkozlik andrewkozlik approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@krnak @andrewkozlik @matejcik @prusnak