Changes required for Keystone firmware support #1666

nuttycom · 2024-12-17T05:15:42Z

This is a feature branch depended on by the downstream Keystone firmware; do not rebase it.

This depends upon an updated fork or `bip32` maintained by `KeystoneHQ`

Co-authored-by: Kris Nuttycombe <[email protected]>

codecov · 2024-12-17T06:35:28Z

Codecov Report

Attention: Patch coverage is 8.95522% with 61 lines in your changes missing coverage. Please review.

Project coverage is 51.89%. Comparing base (2ec38ba) to head (6dab605).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
pczt/src/common.rs	0.00%	27 Missing ⚠️
pczt/src/roles/low_level_signer/mod.rs	0.00%	24 Missing ⚠️
devtools/src/bin/inspect/transaction.rs	0.00%	2 Missing ⚠️
zcash_client_sqlite/src/wallet/sapling.rs	33.33%	2 Missing ⚠️
zcash_transparent/src/builder.rs	0.00%	2 Missing ⚠️
components/zcash_protocol/src/consensus.rs	0.00%	1 Missing ⚠️
pczt/src/roles/tx_extractor/mod.rs	0.00%	1 Missing ⚠️
zcash_client_backend/src/data_api/testing.rs	50.00%	1 Missing ⚠️
zcash_transparent/src/pczt/signer.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1666      +/-   ##
==========================================
- Coverage   52.23%   51.89%   -0.34%     
==========================================
  Files         179      180       +1     
  Lines       21351    21169     -182     
==========================================
- Hits        11152    10986     -166     
+ Misses      10199    10183      -16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Brings #1667 into the Keystone firmware branch.

pczt/CHANGELOG.md

Instead the PCZT is parsed into its protocol-specific types, and getters on those are used. The remaining getters are used for sighash derivation, the one part of the stack that is not currently modular.

daira · 2025-01-28T15:58:13Z

Cargo.lock

 "unicode-normalization",
 "zeroize",
 ]

 [[package]]
 name = "bip32"
-version = "0.5.1"
+version = "0.6.0-pre.1"


Why the preview version? I was unable to find any documentation of what changed in the preview here: https://github.com/iqlusioninc/crates/blob/bip32/v0.6.0-pre.1/bip32/CHANGELOG.md (or in corresponding files for the RustCrypto dependencies).

Oh I see, it's to get iqlusioninc/crates#1254

Depending on all of these previews and rcs does not seem ideal.

hmac 0.13.0-pre.4

ripemd 0.2.0-pre.4

sha2 0.11.0-pre.4

digest 0.11.0-pre.9

block-buffer 0.11.0-rc.3

crypto-common 0.2.0-rc.1

I realize it's only temporary, and that you asked @tony-iqlusion for a backport here. (I guess a bip32 0.6.0 release would do as well given that we've bitten the bullet of MSRV 1.81.0. But that would also require releases of the crates above iiuc.)

Here's a bip32 v0.5.3 backport release, however be aware it uses an older version of the secp256k1 crate (v0.27) to retain compatibility with other v0.5.x releases: https://github.com/iqlusioninc/crates/pull/1261/files

I'd hoped that a backport might suffice, but it turned out to be necessary to use secp256k1 0.29 because the firmware already did, and duplicate C bindings would be a nightmare.

Note v0.30 is out and we'll bump to that (at least) before a final release

So maybe we should ask Keystone to bump the firmware to secp256k1 0.30?

@tony-iqlusion when you say "at least", is that just because there might be another release after 0.30 that you could opportunistically upgrade to, or might it be necessary? (At a glance, 0.30 was tagged only last week but there are some significant changes since that tag, basically updating from rustsecp256k1_v0_10_0_* to rustsecp256k1_v0_11_*.)

Ideally we'd bump to whatever the latest version is prior to a final release

The problem we were having with this has apparently been resolved by zcash/zcash-devtool#78, so all good for now.

daira · 2025-01-28T16:21:39Z

pczt/src/common.rs

@@ -36,6 +39,7 @@ pub struct Global {
    /// - If omitted, the fallback locktime is assumed to be 0.
    pub(crate) fallback_lock_time: Option<u32>,


Why doesn't this have #[getset(get = "pub")]? Similarly for coin_type, and for some other fields in orchard.rs and sapling.rs.

I believe that this is intended to be set only by the Constructor role.

daira · 2025-01-28T16:26:41Z

pczt/src/orchard.rs

    pub(crate) value_sum: (u64, bool),

    /// The Orchard anchor for this transaction.
    ///
    /// Set by the Creator.
+    #[getset(get = "pub")]
    pub(crate) anchor: [u8; 32],

    /// The Orchard bundle proof.


Should zkproof and bsk have #[getset(get = "pub")]?

Not until we find a need for those getters in protocol-unaware code (maybe for deriving the Authorizing Data Commitment, but I don't yet see a need to do that for an in-progress transaction).

daira · 2025-01-28T16:27:11Z

pczt/src/orchard.rs

@@ -67,6 +70,7 @@ pub struct Action {
    // These are required fields that are part of the final transaction, and are filled in
    // by the Constructor when adding an output.
    //
+    #[getset(get = "pub")]
    pub(crate) cv_net: [u8; 32],
    #[getset(get = "pub")]
    pub(crate) spend: Spend,


Should rcv have #[getset(get = "pub")]?

It was not necessary on the protocol-agnostic type for the sighash calculation (as it's private data, and useless for checking commitments without protocol awareness).

daira · 2025-01-28T16:29:03Z

pczt/src/orchard.rs

@@ -97,6 +101,7 @@ pub struct Spend {
    //
    #[getset(get = "pub")]
    pub(crate) nullifier: [u8; 32],
+    #[getset(get = "pub")]
    pub(crate) rk: [u8; 32],

    /// The spend authorization signature.


Should some of the fields below have #[getset(get = "pub")]?

daira · 2025-01-28T16:29:52Z

pczt/src/orchard.rs

    pub(crate) enc_ciphertext: Vec<u8>,
    /// The encrypted note plaintext for the output.
    ///
    /// Encoded as a `Vec<u8>` because its length depends on the transaction version.
+    #[getset(get = "pub")]
    pub(crate) out_ciphertext: Vec<u8>,

    /// The [raw encoding] of the Orchard payment address that will receive the output.


Should ock and zip32_derivation have #[getset(get = "pub")]?

daira · 2025-01-28T16:32:20Z

pczt/src/sapling.rs

    pub(crate) value_sum: i128,

    /// The Sapling anchor for this transaction.
    ///
    /// Set by the Creator.
+    #[getset(get = "pub")]
    pub(crate) anchor: [u8; 32],

    /// The Sapling binding signature signing key.


Should bsk have #[getset(get = "pub")]?

daira · 2025-01-28T16:32:43Z

pczt/src/sapling.rs

    pub(crate) nullifier: [u8; 32],
+    #[getset(get = "pub")]
    pub(crate) rk: [u8; 32],

    /// The Spend proof.


Should some of the fields below have #[getset(get = "pub")]?

daira · 2025-01-28T16:33:15Z

pczt/src/transparent.rs

@@ -66,6 +67,7 @@ pub struct Input {
    // needed for computing the binding signatures.
    #[getset(get = "pub")]
    pub(crate) value: u64,
+    #[getset(get = "pub")]
    pub(crate) script_pubkey: Vec<u8>,

    /// The script required to spend this output, if it is P2SH.


Should some of the fields below have #[getset(get = "pub")]?

daira · 2025-01-28T16:33:58Z

pczt/src/transparent.rs

    pub(crate) value: u64,
+    #[getset(get = "pub")]
    pub(crate) script_pubkey: Vec<u8>,

    /// The script required to spend this output, if it is P2SH.


Should redeem_script and bip32_derivation have #[getset(get = "pub")]?

daira

utACK with comments.

nuttycom

utACK 6dab605 (I opened this PR and so can't give it an Approve review)

nuttycom · 2025-01-28T16:37:33Z

devtools/src/bin/inspect/transaction.rs

I assume that after these changes, we'll do the rest of the devtools removal (in favor of having this functionality in zcash-devtool?)

Correct (it should have been done already, but not going to disturb this PR further).

nuttycom · 2025-01-28T16:38:18Z

pczt/src/common.rs

@@ -36,6 +39,7 @@ pub struct Global {
    /// - If omitted, the fallback locktime is assumed to be 0.
    pub(crate) fallback_lock_time: Option<u32>,


I believe that this is intended to be set only by the Constructor role.

str4d · 2025-01-29T09:40:37Z

Cargo.toml

 ripemd = { version = "0.1", default-features = false }
-secp256k1 = { version = "0.27", default-features = false, features = ["alloc"] }
+secp256k1 = { version = "0.29", default-features = false, features = ["alloc"] }


Note to self: secp256k1 and bip32 need to be mentioned in the changelogs. I'll do that in a follow-up PR.

nuttycom and others added 2 commits December 17, 2024 05:11

keystone: use secp256k1 version 0.29.

26412a3

This depends upon an updated fork or `bip32` maintained by `KeystoneHQ`

pczt: Changes to be usable by Keystone firmware

48a569b

Co-authored-by: Kris Nuttycombe <[email protected]>

str4d mentioned this pull request Dec 17, 2024

Zcash vendored crate removal KeystoneHQ/keystone3-firmware#1505

Merged

2 tasks

str4d added 3 commits December 17, 2024 13:13

cargo fmt

7dfb121

Merge branch 'transparent-key-deriv-bugfix' into keystone_no_std

9407f09

Brings #1667 into the Keystone firmware branch.

Merge branch 'zcash_keys_no_std' into keystone_no_std

ab80376

str4d mentioned this pull request Dec 18, 2024

[WIP]Feat/zcash KeystoneHQ/keystone3-firmware#1366

Merged

str4d mentioned this pull request Jan 18, 2025

Enable Keystone to replace their vendored versions of the Zcash crates with the published versions #1561

Open

str4d added 4 commits January 28, 2025 02:58

Merge branch 'main' into keystone_no_std

cbb5006

Bump MSRV to 1.81

6db1eb5

Fix clippy lints after MSRV bump

29a4758

Replace pinned bip32 revision with published pre-release

65f5790

str4d changed the title ~~Changes required for keystone support that require dependency updates beyond what we can currently do.~~ Changes required for Keystone firmware support Jan 28, 2025

str4d added 2 commits January 28, 2025 04:08

Silence false-positive clippy lint

2de815f

pczt: Update changelog

28ad975

str4d marked this pull request as ready for review January 28, 2025 04:24

str4d reviewed Jan 28, 2025

View reviewed changes

pczt/CHANGELOG.md Outdated Show resolved Hide resolved

pczt: Remove added getters that ended up unused by Keystone

6dab605

Instead the PCZT is parsed into its protocol-specific types, and getters on those are used. The remaining getters are used for sighash derivation, the one part of the stack that is not currently modular.

daira reviewed Jan 28, 2025

View reviewed changes

daira approved these changes Jan 28, 2025

View reviewed changes

nuttycom commented Jan 28, 2025

View reviewed changes

str4d reviewed Jan 29, 2025

View reviewed changes

str4d merged commit 9f5355c into main Jan 29, 2025
31 of 33 checks passed

str4d deleted the keystone_no_std branch January 29, 2025 09:40

daira mentioned this pull request Feb 14, 2025

Rust updates Electric-Coin-Company/zcash-android-wallet-sdk#1683

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes required for Keystone firmware support #1666

Changes required for Keystone firmware support #1666

nuttycom commented Dec 17, 2024 •

edited by str4d

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

daira Jan 28, 2025

daira Jan 28, 2025

daira Jan 28, 2025 •

edited

Loading

tony-iqlusion Jan 28, 2025

str4d Jan 28, 2025

tony-iqlusion Jan 28, 2025

daira Feb 3, 2025

tony-iqlusion Feb 3, 2025

daira Feb 4, 2025 •

edited

Loading

daira Jan 28, 2025 •

edited

Loading

nuttycom Jan 28, 2025

daira Jan 28, 2025

str4d Jan 29, 2025

daira Jan 28, 2025

str4d Jan 28, 2025

daira Jan 28, 2025

daira Jan 28, 2025

daira Jan 28, 2025

daira Jan 28, 2025

daira Jan 28, 2025

daira Jan 28, 2025

daira left a comment

nuttycom left a comment

nuttycom Jan 28, 2025

str4d Jan 29, 2025

nuttycom Jan 28, 2025

str4d Jan 29, 2025

str4d Jan 29, 2025

		@@ -36,6 +39,7 @@ pub struct Global {
		/// - If omitted, the fallback locktime is assumed to be 0.
		pub(crate) fallback_lock_time: Option<u32>,

Changes required for Keystone firmware support #1666

Changes required for Keystone firmware support #1666

Conversation

nuttycom commented Dec 17, 2024 • edited by str4d Loading

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira Jan 28, 2025 • edited Loading

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

daira Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

daira Jan 28, 2025 • edited Loading

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

daira left a comment

Choose a reason for hiding this comment

nuttycom 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

nuttycom commented Dec 17, 2024 •

edited by str4d

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

daira Jan 28, 2025 •

edited

Loading

daira Feb 4, 2025 •

edited

Loading

daira Jan 28, 2025 •

edited

Loading