ZSA integration (step 4): Refactor Orchard structures to generics and add Orchard ZSA support for Transaction V6 #17

dmidem · 2024-10-17T08:06:09Z

This PR refactors the Orchard-related structures to be generics and adds support for both Orchard Vanilla and Orchard ZSA in Zebra's Transaction V6. This change allows the codebase to handle both versions of the Orchard protocol and implements necessary serialization/deserialization logic.

Key changes

Generic Orchard Structures:
- Refactored ShieldedData and Action structures to be generic, parameterized by Orchard flavor (OrchardVanilla or OrchardZSA), enabling support for both protocols in Tx V6.
burn Field in ShieldedData:
- Added a burn field to ShieldedData to support ZSA: a unit type for Tx V5 and a vector of burn items for Tx V6.
Updates to Transaction Enum Methods:
- Modified methods in the Transaction enum to handle the new generic structures properly and support both Orchard flavors.
Serialization and Deserialization for Tx V6:
- Implemented serialization and deserialization for Transaction V6, ensuring the new structures are correctly processed while avoiding code redundancy with Tx V5.

…(without unit tests fixing for now). - Refactored `ShieldedData` and `Action` structures to be generics parameterized by Orchard flavor (`OrchardVanilla` or `OrchardZSA`), enabling support for both Orchard protocols in Tx V6. - Introduced a `burn` field in `ShieldedData` to support ZSA, with unit type for Tx V5 and a vector of burn items for Tx V6. - Modified `Transaction` enum methods (orchard_...) to handle generics properly, ensuring compatibility with both Orchard flavors. - Implemented serialization and deserialization for Tx V6 while avoiding code redundancy with Tx V5 wherever possible.

…m/QED-it/zebra into switch-to-zsa-crates-nu6-txv6-gen

…s-nu6-txv6-gen

… with the upstream halo2/librustcash/orchard/sapling versions

…nu6-txv6-gen

…-zsa-crates-nu6-txv6-gen

…witch-to-zsa-crates-nu6-txv6-gen

…s-nu6-txv6-gen

… the crate tree

…t_desc to convert slice to vec, as slices require implementation of the serialization from scratch

PaulLaux

Added comments.
In addition,

for Serialize / De- Serialize let's use the functions from librustzcash instead of duplicating them. Especially for Issuance (a completely new component in Zebra.)
Need to properly handle feature flags. Currently, it is an unclear why 2 different flags are used.

PaulLaux · 2024-10-31T16:53:29Z

.github/workflows/ci-basic.yml

-      RUSTFLAGS: '--cfg zcash_unstable="nu6"'
-      RUSTDOCFLAGS: '--cfg zcash_unstable="nu6"'
+      #RUSTFLAGS: '--cfg zcash_unstable="nu6"'
+      #RUSTDOCFLAGS: '--cfg zcash_unstable="nu6"'


why comment out?

PaulLaux · 2024-10-31T16:57:20Z

zebra-chain/src/orchard.rs

+
+pub(crate) use shielded_data::ActionCommon;
+
+#[cfg(feature = "tx-v6")]


why not #[cfg(zcash_unstable = "nu6")]
we don't need them both, need to decide on one.

I like tx-v6 better because I think we just need the feature to ensure that v6 transactions can't be (de)serialized by versions of Zebra that aren't ready to deploy NU7, but either seems fine.

PaulLaux · 2024-10-31T17:00:25Z

zebra-chain/src/orchard/orchard_flavor_ext.rs

+/// the transactions `V5` and `V6`.
+pub trait OrchardFlavorExt: Clone + Debug {
+    /// A type representing an encrypted note for this protocol version.
+    /// A type representing an encrypted note for this protocol version.


duplicated line

PaulLaux · 2024-10-31T17:00:58Z

zebra-chain/src/orchard/orchard_flavor_ext.rs

@@ -0,0 +1,84 @@
+//! This module defines traits and structures for supporting the Orchard Shielded Protocol


Why do we need this file? why not use orchard:: orchard_flavor directly and impl the missing parts?

PaulLaux · 2024-10-31T17:13:05Z

zebra-chain/src/orchard/action.rs

@@ -93,7 +94,8 @@ impl ZcashDeserialize for Action {
            // https://zips.z.cash/protocol/protocol.pdf#concretesym but fixed to
            // 580 bytes in https://zips.z.cash/protocol/protocol.pdf#outputencodingandconsensus
            // See [`note::EncryptedNote::zcash_deserialize`].
-            enc_ciphertext: note::EncryptedNote::zcash_deserialize(&mut reader)?,
+            // FIXME: don't mention about 580 here as this should work for OrchardZSA too?


right, can remove comment.

PaulLaux · 2024-10-31T23:27:37Z

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

@@ -41,7 +40,7 @@ fn generate_test_vectors() {
    let anchor_bytes = [0; 32];
    let note_value = 10;

-    let shielded_data: Vec<zebra_chain::orchard::ShieldedData> = (1..=4)
+    let shielded_data: Vec<zebra_chain::orchard::ShieldedData<OrchardVanilla>> = (1..=4)


let shielded_data: Vec<ShieldedData<OrchardVanilla>> = (1..=4)

Subjective: I think orchard::ShieldedData would be slightly more readable, though every option seems fine.

PaulLaux · 2024-10-31T23:28:34Z

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


            let bundle = bundle
                .create_proof(&proving_key, rng)
                .unwrap()
                .apply_signatures(rng, [0; 32], &[])
                .unwrap();

-            zebra_chain::orchard::ShieldedData {
+            zebra_chain::orchard::ShieldedData::<OrchardVanilla> {


ShieldedData::<OrchardVanilla> {

PaulLaux · 2024-10-31T23:47:49Z

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

@@ -91,7 +91,7 @@ fn generate_test_vectors() {
                            // 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(
+                            enc_ciphertext: <[u8; OrchardVanilla::ENCRYPTED_NOTE_SIZE]>::try_from(


The type is already known
enc_ciphertext: a.encrypted_note().enc_ciphertext.as_ref().try_into().unwrap().into(),

PaulLaux · 2024-10-31T23:52:48Z

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

+            let maybe_shielded_data: Option<zebra_chain::orchard::ShieldedData<OrchardVanilla>> =
+                bytes
+                    .zcash_deserialize_into()
+                    .expect("a valid orchard::ShieldedData instance");


let shielded_data = zebra_test::vectors::ORCHARD_SHIELDED_DATA .clone() .iter() .map(|bytes| { bytes .zcash_deserialize_into::<ShieldedData<OrchardVanilla>>() .expect("a valid orchard::ShieldedData instance") }) .collect();

It should deserialize to None if there are no actions for OrchardVanilla (see comment on ZcashDeserialize impl about burns).

PaulLaux · 2024-10-31T23:53:04Z

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

+            let maybe_shielded_data: Option<zebra_chain::orchard::ShieldedData<OrchardVanilla>> =
+                bytes
+                    .zcash_deserialize_into()
+                    .expect("a valid orchard::ShieldedData instance");


arya2

Partial initial review, looking great so far!

arya2 · 2024-11-01T16:24:45Z

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


 // These impls all only exist because of array length restrictions.
 // TODO: use const generics https://github.com/ZcashFoundation/zebra/issues/2042

-impl Copy for EncryptedNote {}
+impl<const N: usize> Copy for EncryptedNote<N> {}


Optional: The Copy, Clone, and Debug traits here can be derived now.

arya2 · 2024-11-01T16:34:32Z

zebra-chain/src/orchard/orchard_flavor_ext.rs

+/// A structure representing a tag for Orchard protocol variant used for the transaction version `V6`
+/// (which ZSA features support).


Nitpick/Optional:

Suggested change

/// A structure representing a tag for Orchard protocol variant used for the transaction version `V6`

/// (which ZSA features support).

/// A structure representing a tag for Orchard protocol variant used for the transaction version 6

/// (with support for ZSAs).

arya2 · 2024-11-01T16:41:19Z

zebra-chain/src/orchard/orchard_flavor_ext.rs

+
+impl ZcashSerialize for NoBurn {
+    fn zcash_serialize<W: io::Write>(&self, mut _writer: W) -> Result<(), io::Error> {
+        Ok(())


~~Yeah, anytime there's a Vec being serialized, there's a CompactSize first, so it does need to insert a 0. Serializing an empty Vec should work too. Or zcash_serialize_empty_list().~~

Update:

There should be a 0 there for version 6 transactions and nothing for version 5 transactions, the code is correct as-is.

arya2 · 2024-11-01T16:50:24Z

zebra-chain/src/orchard/shielded_data.rs

@@ -20,9 +20,17 @@ use crate::{
    },
 };

+use super::OrchardFlavorExt;
+
+#[cfg(not(feature = "tx-v6"))]


We still need OrchardVanilla for v5 transactions

Suggested change

#[cfg(not(feature = "tx-v6"))]

arya2 · 2024-11-01T17:32:58Z

zebra-chain/src/orchard/shielded_data.rs

+        // The following expression doesn't work for generics, so a workaround with _ACTION_MAX_ALLOCATION_OK in
+        // AuthorizedAction impl is used instead:
+        // static_assertions::const_assert!(AuthorizedAction::<V>::ACTION_MAX_ALLOCATION < (1 << 16));
+        AuthorizedAction::<V>::ACTION_MAX_ALLOCATION


size_of() might work here.

arya2 · 2024-11-01T17:45:53Z

zebra-chain/src/orchard/shielded_data.rs

+        let result = Action::<OrchardVanilla>::max_allocation();
+
+        // TODO: FIXME: Check this: V6 is used as it provides the max size of the action.
+        // So it's used even for V5 - is this correct?


It would be better to use the smaller one. It's okay if the max allocation is a little too large.

arya2 · 2024-11-05T19:07:27Z

zebra-chain/src/orchard/tests/preallocate.rs

Could we add copies of the updated tests for OrchardZSA as well?

arya2 · 2024-11-05T19:21:52Z

zebra-chain/src/orchard_zsa/burn.rs

+
+/// Represents an Orchard ZSA burn item.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct BurnItem(AssetBase, Amount);


valueBurn is specified as a uint64, but Amount is an i64.

Suggested change

pub struct BurnItem(AssetBase, Amount);

pub struct BurnItem(AssetBase, u64);

arya2 · 2024-11-05T19:28:21Z

zebra-chain/src/orchard/shielded_data.rs

+        #[cfg(not(feature = "tx-v6"))]
+        let result = Action::<OrchardVanilla>::max_allocation();
+
+        // TODO: FIXME: Check this: V6 is used as it provides the max size of the action.
+        // So it's used even for V5 - is this correct?
+        #[cfg(feature = "tx-v6")]
+        let result = Action::<OrchardZSA>::max_allocation();
+
+        result


This suggestion relies on moving OrchardVanilla out from behind the feature flag.

Suggested change

#[cfg(not(feature = "tx-v6"))]

let result = Action::<OrchardVanilla>::max_allocation();

// TODO: FIXME: Check this: V6 is used as it provides the max size of the action.

// So it's used even for V5 - is this correct?

#[cfg(feature = "tx-v6")]

let result = Action::<OrchardZSA>::max_allocation();

result

Action::<OrchardVanilla>::max_allocation()

arya2 · 2024-11-05T19:30:16Z

zebra-chain/src/orchard_zsa/burn.rs

+impl TrustedPreallocate for BurnItem {
+    fn max_allocation() -> u64 {
+        // FIXME: is this a correct calculation way?
+        // The longest Vec<BurnItem> we receive from an honest peer must fit inside a valid block.


This code is correct as-is, it should be (MAX_BLOCK_BYTES - 1) / BURN_ITEM_SIZE and it should be implemented for BurnItem, not Vec<BurnItem>.

arya2 · 2024-11-05T19:33:07Z

zebra-chain/src/orchard_zsa/burn.rs

+}
+
+#[cfg(any(test, feature = "proptest-impl"))]
+impl serde::Serialize for BurnItem {


I like that it's qualified as serde::Serialize since ZcashSerialize is also used in this file, but we probably don't need this implementation at all. If we do need it, it could probably be conditionally derived with #[cfg_attr(condition, attribute)].

arya2 · 2024-11-05T21:03:45Z

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

@@ -41,7 +40,7 @@ fn generate_test_vectors() {
    let anchor_bytes = [0; 32];
    let note_value = 10;

-    let shielded_data: Vec<zebra_chain::orchard::ShieldedData> = (1..=4)
+    let shielded_data: Vec<zebra_chain::orchard::ShieldedData<OrchardVanilla>> = (1..=4)


Subjective: I think orchard::ShieldedData would be slightly more readable, though every option seems fine.

arya2 · 2024-11-05T21:11:46Z

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

@@ -91,7 +91,7 @@ fn generate_test_vectors() {
                            // 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(
+                            enc_ciphertext: <[u8; OrchardVanilla::ENCRYPTED_NOTE_SIZE]>::try_from(


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

arya2 · 2024-11-05T21:34:16Z

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

+            let maybe_shielded_data: Option<zebra_chain::orchard::ShieldedData<OrchardVanilla>> =
+                bytes
+                    .zcash_deserialize_into()
+                    .expect("a valid orchard::ShieldedData instance");


It should deserialize to None if there are no actions for OrchardVanilla (see comment on ZcashDeserialize impl about burns).

arya2 · 2024-11-05T21:35:58Z

zebra-chain/src/transaction/serialize.rs

    fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
        // Denoted as `nActionsOrchard` and `vActionsOrchard` in the spec.
-        let actions: Vec<orchard::Action> = (&mut reader).zcash_deserialize_into()?;
+        let actions: Vec<orchard::Action<V>> = (&mut reader).zcash_deserialize_into()?;


This method returns None if actions.is_empty(), but it likely needs to check for asset burns as well.

arya2 · 2024-11-05T21:49:24Z

zebra-state/src/service/check/tests/nullifier.rs

Could we add copies of the updated tests for OrchardZSA as well?

dmidem added 18 commits September 16, 2024 10:00

Fix compilation errors for unit tests

e98bb67

Add missed files

dd741e9

Fix ciphertexts.rs tests compilation error

c218fc5

Simplify the previous fix for ciphertexts.rs test

4807dd6

Merge branch 'switch-to-zsa-crates-nu6-txv6-gen' of https://github.co…

cccadcd

…m/QED-it/zebra into switch-to-zsa-crates-nu6-txv6-gen

Fix tests compilation errors for nullifiers

0f9c164

Merge branch 'switch-to-zsa-crates-nu6-txv6' into switch-to-zsa-crate…

20810ee

…s-nu6-txv6-gen

Merge branch 'switch-to-zsa-crates-nu6-txv6' into switch-to-zsa-crate…

986339e

…s-nu6-txv6-gen

Add several FIXME comments

4d8cdef

Remove (comment out) zcash_unstable=nu6 Rust flag and sync Cargo.lock…

1adadb1

… with the upstream halo2/librustcash/orchard/sapling versions

Merge remote-tracking branch 'origin/main' into switch-to-zsa-crates-…

e9b0d46

…nu6-txv6-gen

Attempt to make the code compilable when tx-v6 feature is disabled

e2e160c

Merge branch 'main' of https://github.com/QED-it/zebra into switch-to…

5eade11

…-zsa-crates-nu6-txv6-gen

Use tx-v6 feature across all crates

763cded

Merge branch 'switch-to-zsa-crates-nu6-txv6-gen-txv6-disabled' into s…

083171f

…witch-to-zsa-crates-nu6-txv6-gen

Merge branch 'switch-to-zsa-crates-nu6-txv6' into switch-to-zsa-crate…

d2917fb

…s-nu6-txv6-gen

Patch path for zcash_keys in Cargo.toml to avoid crate duplication in…

c979861

… the crate tree

dmidem mentioned this pull request Oct 17, 2024

Step 4: Refactor Orchard structures to generics and add Orchard ZSA support for Transaction V6 #11

Closed

Merge branch 'zsa-integration-txv6' into zsa-integration-generics

018203e

dmidem requested a review from PaulLaux October 17, 2024 09:11

Fix Orchard ZSA issuance serialization (use to_vec for notes and asse…

d2313fb

…t_desc to convert slice to vec, as slices require implementation of the serialization from scratch

dmidem changed the title ~~Step 4: Refactor Orchard structures to generics and add Orchard ZSA support for Transaction V6~~ ZSA integration, step 4: Refactor Orchard structures to generics and add Orchard ZSA support for Transaction V6 Oct 18, 2024

dmidem changed the title ~~ZSA integration, step 4: Refactor Orchard structures to generics and add Orchard ZSA support for Transaction V6~~ ZSA integration (step 4): Refactor Orchard structures to generics and add Orchard ZSA support for Transaction V6 Oct 18, 2024

dmidem added 3 commits October 29, 2024 15:21

Merge branch 'zsa-integration-txv6' into zsa-integration-generics

6b22fba

Add support for processing ZSA orchard_shielded_data in zebra_state

ab9a2b5

Merge branch 'zsa-integration-txv6' into zsa-integration-generics

c7cd215

PaulLaux requested a review from arya2 October 31, 2024 15:08

PaulLaux reviewed Oct 31, 2024

View reviewed changes

arya2 reviewed Nov 1, 2024

View reviewed changes

arya2 reviewed Nov 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZSA integration (step 4): Refactor Orchard structures to generics and add Orchard ZSA support for Transaction V6 #17

ZSA integration (step 4): Refactor Orchard structures to generics and add Orchard ZSA support for Transaction V6 #17

dmidem commented Oct 17, 2024

PaulLaux left a comment

PaulLaux Oct 31, 2024

PaulLaux Oct 31, 2024

arya2 Nov 1, 2024 •

edited

Loading

PaulLaux Oct 31, 2024

PaulLaux Oct 31, 2024

PaulLaux Oct 31, 2024

PaulLaux Oct 31, 2024

arya2 Nov 5, 2024

PaulLaux Oct 31, 2024

PaulLaux Oct 31, 2024

PaulLaux Oct 31, 2024

arya2 Nov 5, 2024

PaulLaux Oct 31, 2024

arya2 left a comment

arya2 Nov 1, 2024

arya2 Nov 1, 2024

arya2 Nov 1, 2024 •

edited

Loading

arya2 Nov 1, 2024

arya2 Nov 1, 2024

arya2 Nov 1, 2024

arya2 Nov 5, 2024

arya2 Nov 5, 2024

arya2 Nov 5, 2024

arya2 Nov 5, 2024

arya2 Nov 5, 2024

arya2 Nov 5, 2024

arya2 Nov 5, 2024

arya2 Nov 5, 2024

arya2 Nov 5, 2024

arya2 Nov 5, 2024


		pub(crate) use shielded_data::ActionCommon;

		#[cfg(feature = "tx-v6")]

		@@ -0,0 +1,84 @@
		//! This module defines traits and structures for supporting the Orchard Shielded Protocol

		/// A structure representing a tag for Orchard protocol variant used for the transaction version `V6`
		/// (which ZSA features support).

	pub struct BurnItem(AssetBase, Amount);
	pub struct BurnItem(AssetBase, u64);

ZSA integration (step 4): Refactor Orchard structures to generics and add Orchard ZSA support for Transaction V6 #17

Are you sure you want to change the base?

ZSA integration (step 4): Refactor Orchard structures to generics and add Orchard ZSA support for Transaction V6 #17

Conversation

dmidem commented Oct 17, 2024

Key changes

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

arya2 Nov 1, 2024 • 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

arya2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arya2 Nov 1, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arya2 Nov 1, 2024 •

edited

Loading

arya2 Nov 1, 2024 •

edited

Loading