feat(core): add Zcash Rust primitives #2510

krnak · 2022-09-12T13:43:32Z

This PR adds

pasta curves arithmetic (from pasta_curves crate)
redpallas signature scheme (from reddsa crate)
poseidon hash function (my own tested implementation)

Implementation details

Three pasta curves struct (Fp, Scalar and Point) are exposed to micropython using generic container Wrapped<T> defined in micropython/wrap.rs.

Newly imported crates depend on blake2b_simd crate. Since there is already a C implementation of blake2b, I just added a Rust interface to it (in rust/blake2b_hal) and then I overwrote blake2b_simd by blake2b_hal via [crates-io.patch].

Poseidon function requires some precomputed round constants. Since the generating algorithm is pretty lightweight (it is based on LSFR), I decided to replace the constants table (6144 bytes) by my own highly optimized implementation of the generator itself (circa 512 kb). Generator is tested on all expected constants. Poseidon implementation is tested on official zcash test vectors.

~~Crate reddsa is patched to include this minor fix~~

~~Fix alloc feature ZcashFoundation/reddsa#28~~

Crate pasta_curves is patched, because these issue and pr are pending:

Related links:

pasta curves crate
~~reddsa crate~~
Poseidon specification
Poseidon Orchard instantiation (§5.4.1.10)
Poseidon reference implementation - python
Poseidon reference implementation - Rust (quite more general)

prusnak · 2022-09-12T14:45:44Z

It would be great if you introduced FEATURE_FLAGS["ZCASH_SHIELDED"] so we could easily turn on/off the feature

krnak · 2022-09-19T10:08:56Z

How to deal with this ? @prusnak

ZCASH_SHIELDED
ERROR: Altcoin strings found in Bitcoin-only firmware.

I will need this flag to be accessible from python to disable some functionalities in module zcash/signer.py.

prusnak · 2022-09-21T11:05:40Z

I will need this flag to be accessible from python to disable some functionalities in module zcash/signer.py.

We need to replace utils.ZCASH_SHIELDED with a True/False literal, similarly to what we do here:

trezor-firmware/core/site_scons/site_tools/micropython/__init__.py

Line 31 in 4bed278

rf"-e 's/utils\.BITCOIN_ONLY/{btc_only}/g'",

Maybe this does the trick?

diff --git a/core/SConscript.firmware b/core/SConscript.firmware
index 96ffec17d..3d0d30cb4 100644
--- a/core/SConscript.firmware
+++ b/core/SConscript.firmware
@@ -678,7 +678,7 @@ if FROZEN:
         SOURCE_PY.extend(Glob(SOURCE_PY_DIR + 'apps/bitcoin/sign_tx/zcash_v4.py'))
         SOURCE_PY.extend(Glob(SOURCE_PY_DIR + 'trezor/enums/Zcash*.py'))
 
-    source_mpy = env.FrozenModule(source=SOURCE_PY, source_dir=SOURCE_PY_DIR, bitcoin_only=BITCOIN_ONLY)
+    source_mpy = env.FrozenModule(source=SOURCE_PY, source_dir=SOURCE_PY_DIR, bitcoin_only=BITCOIN_ONLY, zcash_shielded=FEATURE_FLAGS['ZCASH_SHIELDED'])
 
     source_mpyc = env.FrozenCFile(
         target='frozen_mpy.c', source=source_mpy, qstr_header=qstr_preprocessed)
diff --git a/core/site_scons/site_tools/micropython/__init__.py b/core/site_scons/site_tools/micropython/__init__.py
index 38c40880f..8f897a3ca 100644
--- a/core/site_scons/site_tools/micropython/__init__.py
+++ b/core/site_scons/site_tools/micropython/__init__.py
@@ -26,9 +26,11 @@ def generate(env):
         # replace "utils.BITCOIN_ONLY" with literal constant (True/False)
         # so the compiler can optimize out the things we don't want
         btc_only = env['bitcoin_only'] == '1'
+        zcash_shielded = env['zcash_shielded'] == '1'
         interim = f"{target[:-4]}.i"  # replace .mpy with .i
         sed_scripts = " ".join([
             rf"-e 's/utils\.BITCOIN_ONLY/{btc_only}/g'",
+            rf"-e 's/utils\.ZCASH_SHIELDED/{zcash_shielded}/g'",
             r"-e 's/if TYPE_CHECKING/if False/'",
             r"-e 's/import typing/# \0/'",
             r"-e '/from typing import (/,/^\s*)/ {s/^/# /}'",

If we do the change above I guess we can drop this change completely? Not sure if that's what we want or we want to add exception for this to the check. Maybe @matejcik can help?

core/embed/rust/Cargo.toml

krnak · 2022-09-29T09:08:08Z

I made changes suggested by @prusnak , but I'm still getting ERROR: Altcoin strings found in Bitcoin-only firmware..

I isolated all new code by ZCASH_SHIELDED flag and tried to define this flag. Since I'm not a creator of TT flag system, neither bitcoin-only edition, resolving this issue could take me hours. I would appreciate if I could let this issue up to you.

andrewkozlik · 2022-10-07T08:37:52Z

I made changes suggested by @prusnak , but I'm still getting ERROR: Altcoin strings found in Bitcoin-only firmware..

178d06b should fix it.

If we do the change above I guess we can drop this change completely? Not sure if that's what we want or we want to add exception for this to the check. Maybe @matejcik can help?

I dropped the QSTR, which means that from trezorutils import ZCASH_SHIELDED is importing something that doesn't exist, but it's only for TYPE_CHECKING, so that seems OK. It seems cleaner than adding an exception to tools/check-bitcoin-only.

krnak · 2022-10-07T08:42:16Z

thank you andrew

andrewkozlik · 2022-10-07T09:03:01Z

178d06b should fix it.

Darn, I hadn't realized this would break the unit tests:

File "../src/trezor/crypto/hashlib.py", line 16, in <module>
AttributeError: 'module' object has no attribute 'ZCASH_SHIELDED'

I think there is no way around that, is there? Adding an exception to tools/check-bitcoin-only: 017bb4f.

prusnak · 2022-10-07T09:23:11Z

I think there is no way around that, is there? Adding an exception to tools/check-bitcoin-only: 017bb4f.

Yeah, I guess 017bb4f is the best way how we can fix this.

matejcik · 2022-10-20T11:37:21Z

istm we could keep ZCASH_SHIELDED qstr only for emulator build?
device build is always frozen so the fixed bool replacement is always applied

matejcik

Review is here.

Before merging, we will also need to do at least a brief review of the pulled in dependencies.

core/embed/extmod/modtrezorutils/modtrezorutils.c

core/embed/rust/blake2b_hal/src/lib.rs

matejcik · 2022-11-11T12:11:39Z

core/embed/rust/src/micropython/obj.rs

+impl<const N: usize> TryFrom<Obj> for [u8; N] {
+    type Error = Error;
+
+    fn try_from(obj: Obj) -> Result<[u8; N], Error> {


note: we have removed the Buffer type so this will need to be redone in terms of get_buffer().

istm it's gonna be trivially Ok(unsafe { get_buffer(obj) }.try_into()?) with the appropriate SAFETY comment

ok . I suggest resolving this after squashing and rebasing

core/embed/rust/src/micropython/wrap.rs

core/site_scons/site_tools/micropython/__init__.py

core/embed/rust/src/zcash_primitives/pallas/common.rs

core/embed/rust/src/zcash_primitives/pallas/generators.rs

core/embed/rust/src/zcash_primitives/pallas/scalar.rs

core/embed/rust/src/zcash_primitives/pallas/point.rs

core/embed/rust/src/zcash_primitives/pallas/scalar.rs

matejcik · 2022-11-18T12:11:54Z

Tentative approve, pending review of the Rust dependencies.

str4d · 2022-12-06T02:20:25Z

@Jarys just to clarify, the only thing you need patched into pasta_curves for this PR is the alloc-free hash-to-curve implementation? If so, I'll try and find some time to work on it.

krnak · 2022-12-06T03:28:18Z

Yes, I confirm. The only thing I need to be patched in pasta_curves is alloc-free hash-to-curve implementation.

krnak requested review from matejcik and prusnak as code owners September 12, 2022 13:43

krnak force-pushed the zcash-rust-primitives branch from 30ab596 to 8715425 Compare September 12, 2022 14:00

krnak mentioned this pull request Sep 15, 2022

Zcash shielded transactions #2472

Draft

7 tasks

krnak force-pushed the zcash-rust-primitives branch from 109f81a to 385157e Compare September 20, 2022 12:23

PRabahy reviewed Sep 28, 2022

View reviewed changes

core/embed/rust/Cargo.toml Show resolved Hide resolved

matejcik reviewed Nov 14, 2022

View reviewed changes

krnak force-pushed the zcash-rust-primitives branch from 23beeec to e73a02a Compare November 16, 2022 12:48

feat(core): add Zcash Rust primitives

6311675

krnak force-pushed the zcash-rust-primitives branch from e73a02a to 6311675 Compare November 16, 2022 12:50

Hannsek added the blocked Blocked by external force. Third party inputs required. label Apr 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): add Zcash Rust primitives #2510

feat(core): add Zcash Rust primitives #2510

krnak commented Sep 12, 2022 •

edited

Loading

prusnak commented Sep 12, 2022 •

edited

Loading

krnak commented Sep 19, 2022 •

edited

Loading

prusnak commented Sep 21, 2022 •

edited

Loading

krnak commented Sep 29, 2022

andrewkozlik commented Oct 7, 2022

krnak commented Oct 7, 2022

andrewkozlik commented Oct 7, 2022

prusnak commented Oct 7, 2022

matejcik commented Oct 20, 2022

matejcik left a comment

matejcik Nov 11, 2022

krnak Nov 16, 2022

matejcik commented Nov 18, 2022

str4d commented Dec 6, 2022

krnak commented Dec 6, 2022

feat(core): add Zcash Rust primitives #2510

Are you sure you want to change the base?

feat(core): add Zcash Rust primitives #2510

Conversation

krnak commented Sep 12, 2022 • edited Loading

Implementation details

Related links:

prusnak commented Sep 12, 2022 • edited Loading

krnak commented Sep 19, 2022 • edited Loading

prusnak commented Sep 21, 2022 • edited Loading

krnak commented Sep 29, 2022

andrewkozlik commented Oct 7, 2022

krnak commented Oct 7, 2022

andrewkozlik commented Oct 7, 2022

prusnak commented Oct 7, 2022

matejcik commented Oct 20, 2022

matejcik left a comment

Choose a reason for hiding this comment

matejcik Nov 11, 2022

Choose a reason for hiding this comment

krnak Nov 16, 2022

Choose a reason for hiding this comment

matejcik commented Nov 18, 2022

str4d commented Dec 6, 2022

krnak commented Dec 6, 2022

krnak commented Sep 12, 2022 •

edited

Loading

prusnak commented Sep 12, 2022 •

edited

Loading

krnak commented Sep 19, 2022 •

edited

Loading

prusnak commented Sep 21, 2022 •

edited

Loading