ZSA integration (step 1): Integrate ZSA-compatible crates into Zebra while maintaining original Orchard (Vanilla) support for now [to upstream] #24

dmidem · 2024-10-29T13:50:03Z

This draft PR updates Zebra to use QED-it's ZSA-compatible versions of the following crates:

halo2
zcash_note_encryption
spling-crypto
orchard
Crates from the libruszcash repository:
- zcash_primitives
- zcash_protocol
- zcash_address
- zcash_encoding
- zcash_history
- zcash_client_backend

These version of the crates are backward-compatible and support both the current Orchard (Vanilla) protocol and the upcoming ZSA variation.

This PR maintains support for the existing Orchard protocol only, without yet enabling or integrating ZSA-specific features!

…l Orchard support only, without supporting and enabling ZSA features.

…uffix to their .yml files)

…loy related workflows (add .disabled suffix to their .yml files)

arya2 · 2024-10-31T01:18:17Z

zebra-consensus/src/primitives/halo2/tests.rs

+                            // FIXME: support OrchardZSA too, 580 works for OrchardVanilla only!
+                            // FIXME: consider more "type safe" way to do the following conversion
+                            // (now it goes through &[u8])
+                            enc_ciphertext: <[u8; 580]>::try_from(
+                                a.encrypted_note().enc_ciphertext.as_ref(),
+                            )
+                            .unwrap()
+                            .into(),


Suggested change

// FIXME: support OrchardZSA too, 580 works for OrchardVanilla only!

// FIXME: consider more "type safe" way to do the following conversion

// (now it goes through &[u8])

enc_ciphertext: <[u8; 580]>::try_from(

a.encrypted_note().enc_ciphertext.as_ref(),

)

.unwrap()

.into(),

enc_ciphertext: a.encrypted_note().enc_ciphertext.0.into(),

Link to this comment copied to #37

arya2 · 2024-10-31T01:31:46Z

zebra-chain/src/primitives/zcash_primitives.rs

+    // FIXME: is this correct?
+    #[cfg(zcash_unstable = "nu6")]
+    type OrchardZsaAuth = orchard::bundle::Authorized;
+


This seems unnecessary if it's going to be the same as the OrchardAuth type?

I tend to agree, why not use OrchardAuth everywhere?

I had to duplicate it because is OrchardZsaAuth required by Authorization trait:

https://github.com/QED-it/librustzcash/blob/zsa1/zcash_primitives/src/transaction/mod.rs#L303

arya2 · 2024-10-31T01:45:38Z

zebra-chain/src/primitives/zcash_primitives.rs

+    // FIXME: is this correct?
+    #[cfg(zcash_unstable = "nu6")]
+    type IssueAuth = orchard::issuance::Signed;


This one does seem necessary.

Link to this comment copied to #37

arya2 · 2024-10-31T02:29:27Z

zebra-chain/src/primitives/zcash_primitives.rs

+            // FIXME: do we need to pass another arg values or orchard_zsa and issue instead of IdentityMap?
+            .map_authorization(
+                f_transparent,
+                IdentityMap,
+                IdentityMap,
+                IdentityMap,
+                IdentityMap,
+            );


This seems fine, the transparent outputs are needed for the TransparentAuthorizingContext impl used via the zp_tx::sighash::signature_hash() call just below this impl block, where the shielded bundles aren't accessed at all for v5 transactions and only the SaplingAuth type is constrained for v4.

Link to this comment copied to #37

arya2 · 2024-10-31T02:51:29Z

zebra-consensus/src/primitives/halo2.rs

+        // FIXME: simplify the flags creation - make `Flags::from_parts` method pub?
+        // FIXME: support OrchardZSA?
+        let flags = match (enable_spend, enable_output) {
+            (false, false) => orchard::builder::BundleType::DISABLED.flags(),
+            (false, true) => orchard::bundle::Flags::SPENDS_DISABLED_WITHOUT_ZSA,
+            (true, false) => orchard::bundle::Flags::OUTPUTS_DISABLED,
+            (true, true) => orchard::bundle::Flags::ENABLED_WITHOUT_ZSA,
+        };


The Flags type in zebra_chain::orchard::shielded_data needs a new associated constant like const ENABLE_ZSA = 0b00000100 for its ZcashSerialize/ZcashDeserialize impls to parse the ZSA-enabled bit.

Suggested change

// FIXME: simplify the flags creation - make `Flags::from_parts` method pub?

// FIXME: support OrchardZSA?

let flags = match (enable_spend, enable_output) {

(false, false) => orchard::builder::BundleType::DISABLED.flags(),

(false, true) => orchard::bundle::Flags::SPENDS_DISABLED_WITHOUT_ZSA,

(true, false) => orchard::bundle::Flags::OUTPUTS_DISABLED,

(true, true) => orchard::bundle::Flags::ENABLED_WITHOUT_ZSA,

};

// FIXME: support OrchardZSA?

let flags = orchard::bundle::Flags::from_byte(shielded_data.flags.bits())

.expect("type should not have unexpected bits");

Link to this comment copied to #37

arya2 · 2024-10-31T03:09:03Z

zebra-chain/src/orchard/note/ciphertexts.rs

@@ -1,5 +1,7 @@
 //! Encrypted parts of Orchard notes.

+// FIXME: make it a generic and add support for OrchardZSA (encrypted tote size ofr it is not 580!)


Optional: There are some trait impls in this file that can be removed/derived once const generics have been added.

Nitpick:

Suggested change

// FIXME: make it a generic and add support for OrchardZSA (encrypted tote size ofr it is not 580!)

// FIXME: make it a generic and add support for OrchardZSA (where encrypted note size is not 580!)

Link to this comment copied to #37

PaulLaux

added some comments,
plus clean FIXME as much as possible.

PaulLaux · 2024-10-31T08:21:07Z

zebra-chain/Cargo.toml

@@ -68,7 +68,7 @@ bitflags = "2.5.0"
 bitflags-serde-legacy = "0.1.1"
 blake2b_simd = "1.0.2"
 blake2s_simd = "1.0.2"
-bridgetree = "0.6.0"
+bridgetree = "0.4.0"


It is worth to comment out this and every downgraded dependency.

Link to this comment copied to #37

PaulLaux · 2024-10-31T08:36:43Z

zebra-chain/src/parameters/network_upgrade.rs

+            // FIXME: remove cfg and process Nu7 properly (uses Self::Nu6 for now)
+            #[cfg(zcash_unstable = "nu6")]


All nu6 -> nu7 flags should be marked with:
#[cfg(zcash_unstable = "nu6" /* TODO nu7 */ )]
(similar to our librustzcash style)

(all nu6 -> nu7 locations)

Link to this comment copied to #37

PaulLaux · 2024-10-31T08:42:26Z

zebra-chain/src/primitives/zcash_primitives.rs

@@ -137,6 +137,16 @@ impl zp_tx::components::orchard::MapAuth<orchard::bundle::Authorized, orchard::b
    }
 }

+// FIXME: is this implemetation correct?
+#[cfg(zcash_unstable = "nu6")]


nu6 or nu7 ?

Link to this comment copied to #37

PaulLaux · 2024-10-31T08:49:27Z

zebra-chain/src/primitives/zcash_primitives.rs

+    fn map_issue_authorization(&self, s: orchard::issuance::Signed) -> orchard::issuance::Signed {
+        s
+    }
+}


maybe just

impl zp_tx::components::issuance::MapIssueAuth<orchard::issuance::Signed, orchard::issuance::Signed> for IdentityMap { fn map_issue_authorization(&self, signed: orchard::issuance::Signed) -> orchard::issuance::Signed { signed } }

I used the existing style of short names used in this file, see other map_authorization functions above in this file. Link for this comment copied to #37.

PaulLaux · 2024-10-31T08:55:45Z

zebra-chain/src/primitives/zcash_primitives.rs

+    // FIXME: is this correct?
+    #[cfg(zcash_unstable = "nu6")]
+    type OrchardZsaAuth = orchard::bundle::Authorized;
+


I tend to agree, why not use OrchardAuth everywhere?

PaulLaux · 2024-10-31T09:01:33Z

zebra-consensus/src/primitives/halo2.rs

@@ -75,7 +75,8 @@ pub type ItemVerifyingKey = VerifyingKey;

 lazy_static::lazy_static! {
    /// The halo2 proof verifying key.
-    pub static ref VERIFYING_KEY: ItemVerifyingKey = ItemVerifyingKey::build();
+    // FIXME: support OrchardZSA?
+    pub static ref VERIFYING_KEY: ItemVerifyingKey = ItemVerifyingKey::build::<OrchardVanilla>();


yes, but not in this PR

Done in #37.

PaulLaux · 2024-10-31T09:48:13Z

zebra-consensus/src/primitives/halo2.rs

@@ -143,6 +144,15 @@ impl From<&zebra_chain::orchard::ShieldedData> for Item {
            .flags
            .contains(zebra_chain::orchard::Flags::ENABLE_OUTPUTS);

+        // FIXME: simplify the flags creation - make `Flags::from_parts` method pub?
+        // FIXME: support OrchardZSA?


not in this PR

Link for this comment copied to #37.

dmidem added 14 commits July 29, 2024 10:13

Prepare to use Orchard ZSA

43b5e50

Switch Zebra to QED-it ZSA-compatible crates but maintain the origina…

95dce56

…l Orchard support only, without supporting and enabling ZSA features.

Merge branch 'main' into zsa-integration-zsadeps-old

51009ae

Prepare to use Orchard ZSA

2fabac7

Add basic CI checks workflow

836516d

Fix ci-basic.yml

e29a64c

Fix ci-basic.yml (2)

790072d

Add installing of build-essential to ci-basic.yml

7695882

Try to use librocksdb-dev in ci-basic.yml

9a8d37e

Run ci-basic.yml on ubuntu-24.04

925e81a

Enable nu6 cfg flag in ci-basic.yml

61f9f95

Adjust the code with librustzcash/zcash_protocol nu6 related changes

2e9c089

Add RUSTDOCFLAGS to ci-basic.yml

0dce1a4

Merge branch 'zsa1' into zsa-integration-zsadeps-old

77e9d4d

github-actions bot added the C-trivial label Oct 29, 2024

dmidem assigned PaulLaux and unassigned PaulLaux Oct 29, 2024

dmidem requested a review from PaulLaux October 29, 2024 13:50

dmidem added 2 commits October 29, 2024 14:52

Set Rust cfg zcash_unstable to nu6 in .cargo/config.toml

e294837

Disable GCP/Firebase/Docker/deploy related workflows (add .disabled s…

9f55f35

…uffix to their .yml files)

dmidem removed the C-trivial label Oct 29, 2024

Trigger CI

6b43dd1

github-actions bot added the C-trivial label Oct 29, 2024

Restore the names of ci-*.yml files )i.e. non GCP/Firebase/Docker/dep…

c6f8913

…loy related workflows (add .disabled suffix to their .yml files)

dmidem removed the C-trivial label Oct 29, 2024

dmidem added 2 commits October 29, 2024 16:07

Update Cargo.lock

1c061e9

Use zsa1 branch for librustzcash crates

8c76592

PaulLaux requested a review from arya2 October 30, 2024 13:41

arya2 reviewed Oct 31, 2024

View reviewed changes

PaulLaux reviewed Oct 31, 2024

View reviewed changes

dmidem mentioned this pull request Jan 11, 2025

ZSA integration (cumulative steps 1–7): up to Orchard ZSA consensus modifications #37

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZSA integration (step 1): Integrate ZSA-compatible crates into Zebra while maintaining original Orchard (Vanilla) support for now [to upstream] #24

ZSA integration (step 1): Integrate ZSA-compatible crates into Zebra while maintaining original Orchard (Vanilla) support for now [to upstream] #24

dmidem commented Oct 29, 2024

arya2 Oct 31, 2024

dmidem Jan 11, 2025

arya2 Oct 31, 2024

PaulLaux Oct 31, 2024

dmidem Nov 11, 2024

arya2 Oct 31, 2024

dmidem Jan 11, 2025

arya2 Oct 31, 2024

dmidem Jan 11, 2025

arya2 Oct 31, 2024 •

edited

Loading

dmidem Jan 11, 2025

arya2 Oct 31, 2024 •

edited

Loading

dmidem Jan 11, 2025

PaulLaux left a comment

PaulLaux Oct 31, 2024

dmidem Jan 11, 2025

PaulLaux Oct 31, 2024

PaulLaux Oct 31, 2024

dmidem Jan 11, 2025

PaulLaux Oct 31, 2024

dmidem Jan 11, 2025

PaulLaux Oct 31, 2024

dmidem Jan 11, 2025 •

edited

Loading

PaulLaux Oct 31, 2024

PaulLaux Oct 31, 2024

dmidem Jan 11, 2025

PaulLaux Oct 31, 2024

dmidem Jan 11, 2025

		@@ -1,5 +1,7 @@
		//! Encrypted parts of Orchard notes.

		// FIXME: make it a generic and add support for OrchardZSA (encrypted tote size ofr it is not 580!)

		// FIXME: remove cfg and process Nu7 properly (uses Self::Nu6 for now)
		#[cfg(zcash_unstable = "nu6")]

ZSA integration (step 1): Integrate ZSA-compatible crates into Zebra while maintaining original Orchard (Vanilla) support for now [to upstream] #24

Are you sure you want to change the base?

ZSA integration (step 1): Integrate ZSA-compatible crates into Zebra while maintaining original Orchard (Vanilla) support for now [to upstream] #24

Conversation

dmidem commented Oct 29, 2024

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

arya2 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arya2 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulLaux 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

dmidem Jan 11, 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

arya2 Oct 31, 2024 •

edited

Loading

arya2 Oct 31, 2024 •

edited

Loading

dmidem Jan 11, 2025 •

edited

Loading