ZSA integration (step 3): Add initial Transaction V6 support to Zebra (currently copies V5 behavior) #16

dmidem · 2024-10-17T08:03:53Z

This PR introduces initial support for Transaction version 6 (Tx V6) in the Zebra codebase as part of the broader effort to add ZSA (Zcash Shielded Assets) support.

Key changes

Added a new V6 variant to the Transaction enum in the zebra-chain crate, currently replicating the behavior of V5.
Updated relevant code paths across the project to handle the new V6 variant, ensuring basic compatibility.
Tx V6 currently functions as a copy of Tx V5 (including tests); its behavior will be updated in future PRs to reflect the actual V6 specification, incorporating ZSA-specific features and changes.
Placeholder/dummy values are used for activation height, group ID, etc., which will be replaced with proper values in future updates.

Notes

This implementation provides a foundational structure for Tx V6 but does not yet include ZSA-specific features, new transaction structures, or detailed serialization/deserialization changes.
Future work will focus on completing Tx V6 support with ZSA-specific elements, proper configuration values, and full integration into the consensus layer.

Next steps

Address any FIXME comments in the code.
Converted zebra-chain::orchard::ShieldedData struct to be a generic that supports both V5 and V6.
Implement serialization/deserialization for V6 (previous work from the /tx_v6_with_ser_1.8.0 branch can be utilized).
Update Tx V6 to perform its intended behaviors distinct from V5 in subsequent PRs.
Implement verification processes for Tx V6 transactions.
Add tests for Tx V6 transactions.

…main

This commit introduces basic support for Transaction version 6 (Tx V6). This initial implementation treats Tx V6 as a simple copy of Tx V5, without yet integrating ZSA-specific features or the new transaction structure. - Added a new V6 variant to the Transaction enum in the zebra-chain crate. - Updated relevant code to handle the new V6 variant. Note: Tests and additional adjustments are still pending, and will be addressed in subsequent commits.

…py V5 behaviour for now)

…-txv6

PaulLaux

added some comments

PaulLaux · 2024-10-31T15:12:09Z

zebra-chain/src/parameters/transaction.rs

+/// Orchard transactions must use transaction version 5 and this version
+/// group ID.
+// FIXME: use a proper value!
+pub const TX_V6_VERSION_GROUP_ID: u32 = 0x26A7_270B;


Let's use a random number in the meanwhile.

Optional: It's 0x124A69F8 in zcash_primitives, if that was random, let's use that so it won't need an update.

Changed to 0x124A69F8 in #37. Link to this comment copied to #37.

Temporarily changed to 0x7777_7777 to align with the a new value used in librustzcash (zcash_primitives) and test vectors. Kept a FIXME comment to change it in the future.

Temporarily changed to 0x124A_69F8 to align with the a value used in OLD version of librustzcash (zcash_primitives) and test vectors. Kept a FIXME comment to change it in the future.

PaulLaux · 2024-10-31T15:12:39Z

zebra-chain/src/transaction.rs

@@ -142,6 +142,28 @@ pub enum Transaction {
        /// The orchard data for this transaction, if any.
        orchard_shielded_data: Option<orchard::ShieldedData>,
    },
+    // FIXME: implement V6 properly (now it's just a coipy of V5)


Adding the changed-files and codespell lint jobs from here may be trivial and help catch typos.

Link to this comment copied to #37.

PaulLaux · 2024-10-31T15:13:32Z

zebra-chain/src/transaction.rs

@@ -142,6 +142,28 @@ pub enum Transaction {
        /// The orchard data for this transaction, if any.
        orchard_shielded_data: Option<orchard::ShieldedData>,
    },
+    // FIXME: implement V6 properly (now it's just a coipy of V5)
+    /// A `version = 6` transaction , which supports Orchard ZSA, Orchard Vanille, Sapling and


which supports OrchardZSA, Orchard, Sapling ...

Link to this comment copied to #37.

PaulLaux · 2024-10-31T15:16:02Z

zebra-chain/src/transaction.rs

+    // FIXME: implement V6 properly (now it's just a coipy of V5)
+    /// A `version = 6` transaction , which supports Orchard ZSA, Orchard Vanille, Sapling and
+    /// transparent, but not Sprout.
+    V6 {


can we put this behind a feature flag?
(this and all subsequent V6 handling)

Done in #37.

PaulLaux · 2024-10-31T16:14:47Z

zebra-chain/src/transaction.rs

+            }
+            | Transaction::V6 {
+                sapling_shielded_data: None,
+                ..


Let's simplify the entire function:

/// Access the deduplicated [`sapling::tree::Root`]s in this transaction, /// regardless of version. pub fn sapling_anchors(&self) -> Box<dyn Iterator<Item = sapling::tree::Root> + '_> { // This function returns a boxed iterator because the different // transaction variants end up having different iterator types match self { Transaction::V4 { sapling_shielded_data: Some(sapling_shielded_data), .. } => Box::new(sapling_shielded_data.anchors()), Transaction::V5 { sapling_shielded_data: Some(sapling_shielded_data), .. } => Box::new(sapling_shielded_data.anchors()), Transaction::V6 { sapling_shielded_data: Some(sapling_shielded_data), .. } => Box::new(sapling_shielded_data.anchors()), // No Spends (V1, V2, V3 and V4, V5, V6 without sapling shielded data) _ => Box::new(iter::empty()), } }

(x4 for all sapling functions)

@arya2 what do you think about this simplification across the entire file?
This will significantly reduce boilerplate without affecting semantics.

The simplification looks nice, I'd lean towards whatever's easiest since it all needs to be refactored to a new-type around the zcash_primitives Transaction type soon, and if there's no difference in difficulty, I'd go with:

use Transaction::*; match self { V4 { sapling_shielded_data: Some(sapling_shielded_data), .. } => Box::new(sapling_shielded_data.anchors()), V5 { sapling_shielded_data: Some(sapling_shielded_data), .. } | V6 { sapling_shielded_data: Some(sapling_shielded_data), .. } => Box::new(sapling_shielded_data.anchors()), // No Spends V1 { .. } | V2 { .. } | V3 { .. } | V4 { .. } | V5 { .. } | V6 { .. } => { Box::new(std::iter::empty()) } }

because it currently seems excessively explicit/verbose, but avoiding a catch-all arm makes it easier to see that it needs an update when a new variant is added. If only Rust supported enums in range patterns we could have the best of both here and elsewhere in Zebra (I think there's a crate for it but adding a crate seems worse than being verbose). Stable support for if let guards would've been nice here and elsewhere in this file too 🤷.

Such simplification would work here and in similar places. However, if we add a feature flag for the V6 feature as we discussed:

V5 { ... } | #[cfg(feature = "tx-v6")] V6 { ... } => { ... }

It wouldn't compile. Here's a playground example to illustrate the issue.

I introduced the "tx-v6" feature flag in the next PR (#17, "Step 4"). However, it would definitely be better to include it in this PR ("Step 3") since I introduced V6 here (the absence of the "tx-v6" feature flag in this PR makes the review process confusing).

Anyway, we can't afford to use the | operator for such simplification during the transition period when the "tx-v6" feature flag is needed. Although I'm not completely sure if Rust doesn't have workarounds to achieve that.

And it was one of the reasons why I added the tx_v5_and_v6 macro here – to avoid such code duplication. However, the macro itself can be confusing because its usage doesn't follow the existing style for processing transaction version matches. I might need to remove it, and we can discuss this further.

PaulLaux · 2024-10-31T16:39:26Z

zebra-chain/src/transaction/txid.rs

+    // auto-detects the transaction version by the first byte, so the same function
+    // can be used here for both V5 and V6.
+    // FIXME: fix spec refs below for V6
+    /// Compute the Transaction ID for a V5/V6 transaction in the given network upgrade.


// Compute the Transaction ID for transactions V5 to V6.

Link to this comment copied to #37.

PaulLaux · 2024-10-31T16:39:59Z

zebra-chain/src/transaction/txid.rs

+    // FIXME: it looks like the updated zcash_primitives in librustzcash
+    // auto-detects the transaction version by the first byte, so the same function
+    // can be used here for both V5 and V6.


remove these 3 lines

txid_v5_v6()'s approach would work for all transactions, but it's an unnecessary memory/cache allocation when txid_v1_to_v4() is so simple and already implemented.

Link to this comment copied to #37.

PaulLaux · 2024-10-31T16:41:38Z

zebra-state/src/service/check/utxo.rs

@@ -63,6 +63,7 @@ pub fn transparent_spend(
                finalized_state,
            )?;

+            // FIXME: what about v6?


Remove comment and fix line 69 to be
// This is a particular issue for v5 and v6 transactions,

Link to this comment copied to #37.

PaulLaux · 2024-10-31T16:42:59Z

zebra-state/src/service/non_finalized_state/chain.rs

+                    &None,
+                    &None,
+                    sapling_shielded_data,
+                    orchard_shielded_data,


do we need to deal with issue bundle here?
(x2)

Yeah, this is probably the best place to update the issued_assets map for the non-finalized part of the chain, same for revert_chain_with() when rolling back the chain for a fork.

Link to this comment copied to #37.

PaulLaux · 2024-10-31T16:48:05Z

zebrad/src/components/mempool/storage/tests/prop.rs

@@ -446,6 +446,7 @@ enum SpendConflictTestInput {

        conflict: SpendConflictForTransactionV5,
    },
+    // FIXME: add and use V6?


// FIXME: add V6 test

Link to this comment copied to #37.

arya2 · 2024-10-31T23:40:14Z

zebra-chain/src/parameters/transaction.rs

+
+/// The version group ID for version 6 transactions.
+///
+/// Orchard transactions must use transaction version 5 and this version


Suggested change

/// Orchard transactions must use transaction version 5 and this version

/// Orchard ZSA transactions must use transaction version 6 and this version

Link to this comment copied to #37.

arya2 · 2024-10-31T23:44:31Z

zebra-chain/src/transaction.rs

@@ -142,6 +142,28 @@ pub enum Transaction {
        /// The orchard data for this transaction, if any.
        orchard_shielded_data: Option<orchard::ShieldedData>,
    },
+    // FIXME: implement V6 properly (now it's just a coipy of V5)


Adding the changed-files and codespell lint jobs from here may be trivial and help catch typos.

arya2 · 2024-11-01T00:34:26Z

zebra-chain/src/transaction.rs

+            }
+            | Transaction::V6 {
+                sapling_shielded_data: None,
+                ..


The simplification looks nice, I'd lean towards whatever's easiest since it all needs to be refactored to a new-type around the zcash_primitives Transaction type soon, and if there's no difference in difficulty, I'd go with:

use Transaction::*; match self { V4 { sapling_shielded_data: Some(sapling_shielded_data), .. } => Box::new(sapling_shielded_data.anchors()), V5 { sapling_shielded_data: Some(sapling_shielded_data), .. } | V6 { sapling_shielded_data: Some(sapling_shielded_data), .. } => Box::new(sapling_shielded_data.anchors()), // No Spends V1 { .. } | V2 { .. } | V3 { .. } | V4 { .. } | V5 { .. } | V6 { .. } => { Box::new(std::iter::empty()) } }

because it currently seems excessively explicit/verbose, but avoiding a catch-all arm makes it easier to see that it needs an update when a new variant is added. If only Rust supported enums in range patterns we could have the best of both here and elsewhere in Zebra (I think there's a crate for it but adding a crate seems worse than being verbose). Stable support for if let guards would've been nice here and elsewhere in this file too 🤷.

arya2 · 2024-11-01T00:42:52Z

zebra-chain/src/transaction.rs

+            Transaction::V6 {
+                sapling_shielded_data: Some(sapling_shielded_data),
+                ..
+            } => Box::new(sapling_shielded_data.spends_per_anchor()),


Nitpick/Optional: Transaction::V5 | Transaction::V6 would work here and for all of the sapling methods below too.

It would be a good simplification, but it's probably not compatible with the "tx-v6" feature flag. I’ve provided more details in this response.

arya2 · 2024-11-01T01:01:12Z

zebra-chain/src/transaction/arbitrary.rs

@@ -918,6 +918,22 @@ pub fn transaction_to_fake_v5(
            orchard_shielded_data: None,
        },
        v5 @ V5 { .. } => v5.clone(),
+        V6 {


I would rename the function to something like transaction_to_fake_min_v5() instead of converting v6 transactions to v5 here, since there aren't any v6 transactions in the test blocks (zebra_test::vectors::{MAINNET_BLOCKS, TESTNET_BLOCKS}, accessed via network.block_iter() in some places), and if they're added later, it would improve test coverage to keep them as v6 transactions in the tests where this is used (a total of ~20 places counting indirect calls, and only v5_transaction_is_accepted_after_nu5_activation_for_network looks like it would need an update other than a rename).

Link to this comment copied to #37.

arya2 · 2024-11-01T01:18:39Z

zebra-chain/src/transaction/serialize.rs

+            } => {
+                // Transaction V6 spec:
+                // FIXME: specify a proper ref
+                // https://zips.z.cash/protocol/protocol.pdf#txnencoding


Optional: Leave both so it can be referenced while it's only in the ZIP and won't need an update if the encoding is meant to eventually be added to the protocol specification, though I'm not sure if that's the intention, I asked here.

Update: I've only found the version group IDs for V5 in the protocol spec so far, I think we'll probably want to keep this reference here (or add it back later).

arya2 · 2024-11-01T01:44:18Z

zebra-chain/src/parameters/transaction.rs

+/// Orchard transactions must use transaction version 5 and this version
+/// group ID.
+// FIXME: use a proper value!
+pub const TX_V6_VERSION_GROUP_ID: u32 = 0x26A7_270B;


Optional: It's 0x124A69F8 in zcash_primitives, if that was random, let's use that so it won't need an update.

arya2 · 2024-11-01T02:09:52Z

zebra-chain/src/transaction/txid.rs

+    // FIXME: it looks like the updated zcash_primitives in librustzcash
+    // auto-detects the transaction version by the first byte, so the same function
+    // can be used here for both V5 and V6.


txid_v5_v6()'s approach would work for all transactions, but it's an unnecessary memory/cache allocation when txid_v1_to_v4() is so simple and already implemented.

arya2 · 2024-11-01T02:23:34Z

zebra-state/src/service/non_finalized_state/chain.rs

+                    &None,
+                    &None,
+                    sapling_shielded_data,
+                    orchard_shielded_data,


Yeah, this is probably the best place to update the issued_assets map for the non-finalized part of the chain, same for revert_chain_with() when rolling back the chain for a fork.

…to aling with the one used in librustzcash

…sh_primitives) interface (OrchardBundle enum etc.)" This reverts commit 998a2c7.

…h the one used in OLD librustzcash

dmidem added 25 commits August 18, 2024 21:47

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

054e655

…main

Add initial Transaction V6 support in other crates and tests (just co…

83d981b

…py V5 behaviour for now)

Fix compilation errors in tests

c8fa779

Merge branch 'zsa1' into switch-to-zsa-crates-nu6

2d23d6b

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

9d6c8e9

…-txv6

Trigger CI

53c65b6

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

c67e2ae

…-txv6

Add basic CI checks workflow

c238847

Fix ci-basic.yml

2c13ae9

Fix ci-basic.yml (2)

5a839c6

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

367a14b

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

f425747

Run ci-basic.yml on ubuntu-24.04

831c847

Enable nu6 cfg flag in ci-basic.yml

ecaf98d

Adjust the code with librustzcash/zcash_protocol nu6 related changes

7d11159

Add RUSTDOCFLAGS to ci-basic.yml

6a0196e

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

8e53d24

…-txv6

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

1526276

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

654b8d4

…-txv6

Merge remote-tracking branch 'zcash/main' into switch-to-zsa-crates-nu6

9e94dc5

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

a6405d7

…-txv6

Merge branch 'main' into switch-to-zsa-crates-nu6

12ade98

Merge branch 'main' into switch-to-zsa-crates-nu6-txv6

a09774e

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

a026dd2

…-txv6

dmidem mentioned this pull request Oct 17, 2024

Step3: Add initial Transaction V6 support to Zebra (currently copies V5 behavior) #9

Closed

dmidem changed the title ~~Step3: Add initial Transaction V6 support to Zebra (currently copies V5 behavior)~~ Stepv3: Add initial Transaction V6 support to Zebra (currently copies V5 behavior) Oct 17, 2024

dmidem changed the title ~~Stepv3: Add initial Transaction V6 support to Zebra (currently copies V5 behavior)~~ Step 3: Add initial Transaction V6 support to Zebra (currently copies V5 behavior) Oct 17, 2024

dmidem added 2 commits October 17, 2024 10:38

Merge branch 'main' into switch-to-zsa-crates-nu6

bb1395c

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

8d18425

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

dmidem changed the title ~~Step 3: Add initial Transaction V6 support to Zebra (currently copies V5 behavior)~~ ZSA integration, step 3: Add initial Transaction V6 support to Zebra (currently copies V5 behavior) Oct 18, 2024

dmidem changed the title ~~ZSA integration, step 3: Add initial Transaction V6 support to Zebra (currently copies V5 behavior)~~ ZSA integration (step 3): Add initial Transaction V6 support to Zebra (currently copies V5 behavior) Oct 18, 2024

dmidem deleted the branch zsa-integration-zsadeps October 29, 2024 12:28

dmidem closed this Oct 29, 2024

dmidem reopened this Oct 29, 2024

dmidem added 2 commits October 29, 2024 15:19

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

f90a10f

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

9e5a01c

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

PaulLaux reviewed Oct 31, 2024

View reviewed changes

arya2 reviewed Nov 1, 2024

View reviewed changes

This was referenced Jan 11, 2025

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

Open

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

Open

dmidem added 10 commits February 10, 2025 15:35

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

9e51b24

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

a158e9f

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

bed9fb5

Change TX_V6_VERSION_GROUP_ID value back to a temporary 0x7777_7777, …

164e5de

…to aling with the one used in librustzcash

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

8195426

Revert "Update zebra-chain to sync with the updated librustzcash (zca…

4ad68ae

…sh_primitives) interface (OrchardBundle enum etc.)" This reverts commit 998a2c7.

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

b219b66

Change TX_V6_VERSION_GROUP_ID value back to 0x124A_69F8, to aling wit…

08de3de

…h the one used in OLD librustzcash

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

f16bd20

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

51eaeda

	/// Orchard transactions must use transaction version 5 and this version
	/// Orchard ZSA transactions must use transaction version 6 and this version

ZSA integration (step 3): Add initial Transaction V6 support to Zebra (currently copies V5 behavior) #16

Are you sure you want to change the base?

ZSA integration (step 3): Add initial Transaction V6 support to Zebra (currently copies V5 behavior) #16

Conversation

dmidem commented Oct 17, 2024 • edited by PaulLaux Loading

Key changes

Notes

Next steps

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

dmidem Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

dmidem Feb 13, 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

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

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

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 commented Oct 17, 2024 •

edited by PaulLaux

Loading

dmidem Jan 11, 2025 •

edited

Loading

dmidem Feb 13, 2025 •

edited

Loading

arya2 Nov 1, 2024 •

edited

Loading