[WIP]Feat/zcash #1366

soralit · 2024-09-26T02:15:46Z

Keystone Zcash UR Registries

This protocol is based on the Uniform Resources. It describes the data schemas (UR Registries) used in Zcash integrations.

Introduction

Keystone's QR workflow involves two main steps: linking the wallet and signing data, broken down into three sub-steps:

Wallet Linking: Keystone generates a QR code with public key info for the Watch-Only wallet to scan and import.
Transaction Creation: The Watch-Only wallet creates a transaction and generates a QR code for Keystone to scan, parse, and display.
Signing Authorization: Keystone signs the transaction, displays the result as a QR code for the Watch-Only wallet to scan and broadcast.

Two UR Registries are needed for these steps, utilizing the Partially Created Zcash Transaction structure.

Zcash Accounts

Unified Full Viewing Key (UFVK)

UFVK is a standard account expression format in Zcash as per ZIP-316. It consists of:

Transparent
Sprout
Sapling
Orchard

This protocol focuses on the Transparent and Orchard components.

CDDL for Zcash Accounts

The specification uses CDDL and includes crypto-hdkey and crypto-key-path specs defined in https://github.com/BlockchainCommons/Research/blob/master/papers/bcr-2020-007-hdkey.md.

zcash-accounts = {
    seed-fingerprint: bytes.32, ; the seed fingerprint specified by ZIP-32 to identify the wallet
    accounts: [+ zcash-ufvk],
    ? origin: text, ; source of data, e.g., Keystone
}

zcash-ufvk = {
    ? transparent: crypto-hdkey,
    orchard: zcash-fvk,
    ? name: text,
}

zcash-fvk = {
    key-path: crypto-key-path,
    key-data: bytes,
}

zcash-ufvk describes the UFVK of a Zcash account. Each seed has multiple accounts with different indexes. For index 0, zcash-ufvk should contain a BIP32 extended public key with path M/44'/133'/0' (transparent) and an Orchard FVK with path M_orchard/32'/133'/0' (Orchard).

CDDL for Zcash PCZT

zcash-pczt {
    data: bytes, ; Zcash PCZT, signatures inserted after signing.
}

docs/protocols/ur_registrys/draft/zcash.md

soralit · 2024-10-08T10:38:55Z

Latest protocol is updated in the code base.

daira · 2024-10-08T14:42:55Z

//is versionGroupId still needed?

The purpose of versionGroupId is to unambiguously identify the transaction format even if version numbers are reused across Zcash forks, and that's still relevant for PCZTs.

I think we'd want to include Sapling components even if it isn't supported for Keystone, because it is needed for other applications of PCZTs.

rust/pczt/src/protos/pczt.proto

images/coin/coinZec.png

daira · 2024-10-09T14:40:33Z

images/walletList/walletListZashi.png

This is probably not going to work in light mode, because the Zashi logo and name are in white.

rust/apps/zcash/src/pczt/check.rs

rust/Cargo.toml

str4d · 2024-12-18T10:26:25Z

rust/rust_c/src/trng.rs

+        // TODO: Determine how to correctly map error codes from the underlying hardware
+        // into the `getrandom` custom error code space `Error::CUSTOM_START..=u32::MAX`.
+        if ret != 0 {
+            let error = NonZeroU32::new(Error::CUSTOM_START.saturating_add_signed(ret)).unwrap();
+            return Err(Error::from(error));
+        }


This TODO still needs addressing by someone with access to the TRNG documentation, in particular to confirm that ret will never be negative. (Also to confirm that ret = 0 means success, but I think that's the case as otherwise this function would have been consistently returning an error.)

Just checked the ret is asserted in the SE driver level so the error code should always be SUCCESS_CODE(0) here.

str4d

Reviewed as of d41a8f2. This is not a full review or audit of the PR, just what I had time to look over.

str4d · 2024-12-18T10:35:41Z

src/managers/account_manager.h

@@ -45,6 +46,12 @@ typedef struct {
    char walletName[WALLET_NAME_MAX_LEN + 1];
 } AccountInfo_t;

+typedef struct {
+    uint8_t accountIndex;


This currently puts an upper limit of 256 accounts (2^8, vs the total possible 2^31), which is fine for now (given that only account 0 is supported at present) assuming that this cache struct can be altered later.

str4d · 2024-12-18T10:50:33Z

src/managers/account_manager.h

@@ -7,6 +7,7 @@
 #include "stdio.h"

 #define WALLET_NAME_MAX_LEN                 16
+#define ZCASH_UFVK_MAX_LEN                  384


The Keystone device is generating the UFVKs, so the limit here only needs to encompass the data those UFVKs will contain:

Orchard: 96 bytes (+ 2 for TLV overhead)

Transparent: 65 bytes (+ 2 for TLV overhead)

HRP padding: 16 bytes

Total: 181 bytes

Bech32 encoding: 6 + (181*8/5) + 6 = 302 characters

A limit of 384 bytes leaves plenty of overhead for e.g. metadata.

str4d · 2024-12-18T10:55:28Z

rust/zcash_vendor/src/lib.rs

+#![feature(error_in_core)]
+extern crate alloc;
+
+pub mod pczt_ext;


This file is only used by rust/apps/zcash/src/pczt/sign.rs, so if desired we could move this file there, and remove rust/zcash_vendor entirely (moving its relevant imports into the other crates). Non-blocking, I might look at this refactor separately.

str4d · 2024-12-18T11:56:14Z

rust/keystore/src/algorithms/zcash/mod.rs

+    if path.len() == 3
+        && path[0] == zip32::ChildIndex::hardened(32)
+        && path[1] == zip32::ChildIndex::hardened(coin_type)
+    {
+        let account_id = zip32::AccountId::try_from(path[2].index() - (1 << 31)).expect("valid");


I'm adding an orchard::pczt::Zip32Derivation::extract_account_index API in zcash/orchard#449 that can replace this in future.

str4d · 2024-12-18T12:06:39Z

rust/apps/zcash/src/pczt/check.rs

+            if zip32_derivation.seed_fingerprint() == seed_fingerprint
+                && zip32_derivation.derivation_path()
+                    == &[
+                        zip32::ChildIndex::hardened(32),
+                        zip32::ChildIndex::hardened(params.network_type().coin_type()),
+                        account_index.into(),
+                    ] =>


This can in future be replaced by checking the output of Zip32Derivation::extract_account_index once the firmware is upgraded to an orchard release containing zcash/orchard#449.

str4d · 2024-12-18T12:33:01Z

rust/apps/zcash/src/pczt/check.rs

+        .map_err(|e| {
+            ZcashError::InvalidPczt(alloc::format!("invalid Orchard action cmx: {:?}", e))
+        })?;
+


The user_address field of the Orchard action output is currently checked in pczt::parse::parse_orchard_output. We could instead move that check to here, given that action.output().recipient() is confirmed to be present if verify_note_commitment passes. Then all that parse_orchard_output would be checking is that the decrypted note's recipient matches action.output().recipient() (unless that is also moved here per the TODO below).

I meant to note that I did later see the comment about CPU management being why this was done later. I'd be interested to know what specifically is the CPU bottleneck.

str4d · 2024-12-18T12:48:51Z

rust/apps/zcash/src/pczt/check.rs

+        _ => Err(ZcashError::InvalidPczt(
+            "transparent output script pubkey is not a public key hash".to_string(),
+        )),


This is going to prevent users from sending Keystone-controlled funds to P2SH addresses, which should be fixed at some point (the PCZT format does allow it). The same checking should be applied to user_address in that case as above, but instead that it matches Receiver::P2sh(hash).

str4d · 2024-12-18T12:51:01Z

rust/apps/zcash/src/pczt/check.rs

+        _ => Err(ZcashError::InvalidPczt(
+            "transparent input script pubkey is not a public key hash".to_string(),
+        )),


This will prevent creation of transactions where one of the inputs is a P2PKH coin controlled by a Keystone device, and another input is a P2SH coin. It will also prevent users from manually constructing e.g. P2SH multi-sig addresses where one of the pubkeys is controlled by a Keystone device.

Unlike on the output side, I think this limitation is fine for now; Zashi doesn't support creating these kinds of transactions. But it might be something to consider for future firmware updates.

str4d · 2024-12-18T12:55:31Z

rust/apps/zcash/src/pczt/parse.rs

+    let total_transfer_value = format_zec_value((total_output_value - total_change_value) as f64);
+    let fee_value = format_zec_value((total_input_value - total_output_value) as f64);
+
+    let has_sapling = pczt.sapling().spends().len() > 0 || pczt.sapling().outputs().len() > 0;


Currently total_transfer_value will be completely incorrect if has_sapling = true. It should at a minimum include the Sapling bundle's value_sum in the calculation; that value is included in the sighash, and thus even if it hasn't been checked to match the individual Sapling spends and outputs, it can still be "trusted" (because any change to it by the online wallet would invalidate the signature).

str4d · 2024-12-18T17:18:45Z

rust/apps/zcash/src/pczt/parse.rs

+    //decode as utf8.
+    if first <= 0xF4 {
+        let mut temp_memo = memo_bytes.to_vec();
+        temp_memo.reverse();
+        let mut result = vec![];
+        temp_memo.iter().for_each(|v| {
+            if *v != 0 {
+                result.push(*v);
+            }
+        });
+        result.reverse();
+
+        return Some(String::from_utf8(result).unwrap());
+    }


This is subtly incorrect: rather than only stripping trailing nul bytes from the memo bytes, it is stripping every nul byte from the memo bytes. nul is a valid byte in UTF-8 (as part of the NUL character).

If a NUL character cannot be included in rendered memo strings (if Keystone requires C-strings), then it should be replaced with an appropriate Unicode replacement character.

I aim to remove until the first non-zero value but implement a wrong one.

soralit marked this pull request as draft September 26, 2024 02:15

daira reviewed Sep 26, 2024

View reviewed changes

docs/protocols/ur_registrys/draft/zcash.md Outdated Show resolved Hide resolved

daira reviewed Sep 26, 2024

View reviewed changes

docs/protocols/ur_registrys/draft/zcash.md Show resolved Hide resolved

soralit force-pushed the feat/zcash branch from 4d593a4 to a38baee Compare September 29, 2024 04:00

soralit force-pushed the feat/zcash branch from a38baee to 6f857af Compare October 8, 2024 10:43

soralit force-pushed the feat/zcash branch from 6f857af to 580014a Compare October 9, 2024 07:47

soralit commented Oct 9, 2024

View reviewed changes

rust/pczt/src/protos/pczt.proto Outdated Show resolved Hide resolved

soralit force-pushed the feat/zcash branch 3 times, most recently from 9cda39e to 90596a9 Compare October 9, 2024 09:38

daira reviewed Oct 9, 2024

View reviewed changes

images/coin/coinZec.png Outdated Show resolved Hide resolved

daira reviewed Oct 9, 2024

View reviewed changes

images/walletList/walletListZashi.png Outdated

Copy link

daira Oct 9, 2024

Choose a reason for hiding this comment

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

This is probably not going to work in light mode, because the Zashi logo and name are in white.

soralit force-pushed the feat/zcash branch 3 times, most recently from 13f57a2 to a809fdc Compare October 14, 2024 05:31

soralit force-pushed the feat/zcash branch 2 times, most recently from 515890a to ea142af Compare November 5, 2024 10:13

soralit force-pushed the feat/zcash branch from ea142af to 1b8acca Compare November 13, 2024 07:49

soralit force-pushed the feat/zcash branch from bfe5f9a to 24ab478 Compare November 26, 2024 06:28

soralit force-pushed the feat/zcash branch 5 times, most recently from 6453e73 to 011282a Compare December 11, 2024 11:59

str4d suggested changes Dec 12, 2024

View reviewed changes

rust/apps/zcash/src/pczt/check.rs Outdated Show resolved Hide resolved

rust/apps/zcash/src/pczt/check.rs Outdated Show resolved Hide resolved

soralit force-pushed the feat/zcash branch 2 times, most recently from 7e801ad to 387adfe Compare December 16, 2024 05:32

str4d reviewed Dec 16, 2024

View reviewed changes

rust/apps/zcash/src/pczt/check.rs Outdated Show resolved Hide resolved

soralit added 8 commits December 18, 2024 13:12

style(zcash): optimize import

7a179c9

style(zcash): run cargo fix

b379e52

fix(bitcoin): issues happened after upgrade bitcoin

dd637e4

fix: disable checking ATM

f2fb1c3

fix: transaction signing failed issue

aa0220a

feat: ui refinement for sapling part

2e2e5b9

feat: ui refinement for sapling part

b0b01da

fix: zcash receive page issue

0b79d5c

soralit force-pushed the feat/zcash branch from b8a3ab6 to 0b79d5c Compare December 18, 2024 07:44

soralit added 5 commits December 18, 2024 16:17

fix: unit test

4641f77

feat: upgrade zcash dep

1a8bc53

feat: support passphrase and slip39 to zcash

bed660d

fix: ui issue

fa3f10c

fix: open checking logic

6a04d69

soralit force-pushed the feat/zcash branch from ec5b2b5 to 6a04d69 Compare December 18, 2024 10:06

soralit added 2 commits December 18, 2024 18:26

fix: disable zcash for passphrase and slip39 wallet for now

3882c14

chore: bump beta version

2a7c871

str4d reviewed Dec 18, 2024

View reviewed changes

soralit added 2 commits December 18, 2024 18:31

fix: build

6cc43e1

fix: passphrase judge error

d41a8f2

str4d reviewed Dec 18, 2024

View reviewed changes

fix: issues

337981f

soralit force-pushed the feat/zcash branch from 4ccacbe to 337981f Compare December 19, 2024 02:14

soralit added 5 commits December 19, 2024 13:39

feat: allow p2sh transparent output and input. refactor ut

9a81ada

fix: amount calculation

05b1372

fix: app crash

71bdfb8

fix: connect wallet issue

79cc493

fix: ui issues

82a380c

soralit merged commit 14d54b1 into master Dec 20, 2024
3 of 9 checks passed

soralit deleted the feat/zcash branch December 20, 2024 01:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]Feat/zcash #1366

[WIP]Feat/zcash #1366

soralit commented Sep 26, 2024 •

edited

Loading

soralit commented Oct 8, 2024 •

edited

Loading

daira commented Oct 8, 2024 •

edited

Loading

daira Oct 9, 2024

str4d Dec 18, 2024

soralit Dec 19, 2024

str4d left a comment

str4d Dec 18, 2024

str4d Dec 18, 2024

str4d Dec 18, 2024

str4d Dec 18, 2024

str4d Dec 18, 2024

str4d Dec 18, 2024

str4d Dec 19, 2024

str4d Dec 18, 2024

str4d Dec 18, 2024

str4d Dec 18, 2024

str4d Dec 18, 2024

soralit Dec 19, 2024

[WIP]Feat/zcash #1366

[WIP]Feat/zcash #1366

Conversation

soralit commented Sep 26, 2024 • edited Loading

Keystone Zcash UR Registries

Introduction

Zcash Accounts

Unified Full Viewing Key (UFVK)

CDDL for Zcash Accounts

CDDL for Zcash PCZT

soralit commented Oct 8, 2024 • edited Loading

daira commented Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soralit commented Sep 26, 2024 •

edited

Loading

soralit commented Oct 8, 2024 •

edited

Loading

daira commented Oct 8, 2024 •

edited

Loading