ZSA integration (step 3): Add initial Transaction V6 support to Zebra (currently copies V5 behavior)#16
ZSA integration (step 3): Add initial Transaction V6 support to Zebra (currently copies V5 behavior)#16dmidem wants to merge 45 commits into
Conversation
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)
dmidem
changed the title
dmidem
changed the title
| /// 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; |
There was a problem hiding this comment.
Let's use a random number in the meanwhile.
There was a problem hiding this comment.
Optional: It's 0x124A69F8 in zcash_primitives, if that was random, let's use that so it won't need an update.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| /// 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) |
| 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 |
There was a problem hiding this comment.
which supports OrchardZSA, Orchard, Sapling ...
| // 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 { |
There was a problem hiding this comment.
can we put this behind a feature flag?
(this and all subsequent V6 handling)
| } | ||
| | Transaction::V6 { | ||
| sapling_shielded_data: None, | ||
| .. |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
@arya2 what do you think about this simplification across the entire file?
This will significantly reduce boilerplate without affecting semantics.
There was a problem hiding this comment.
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 🤷.
There was a problem hiding this comment.
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.
| // 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. |
There was a problem hiding this comment.
// Compute the Transaction ID for transactions V5 to V6.
| // 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Removed the FIXME comment.
| finalized_state, | ||
| )?; | ||
|
|
||
| // FIXME: what about v6? |
There was a problem hiding this comment.
Remove comment and fix line 69 to be
// This is a particular issue for v5 and v6 transactions,
| &None, | ||
| &None, | ||
| sapling_shielded_data, | ||
| orchard_shielded_data, |
There was a problem hiding this comment.
do we need to deal with issue bundle here?
(x2)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Handled in future PRs (see zsa-integration-state branch).
|
|
||
| conflict: SpendConflictForTransactionV5, | ||
| }, | ||
| // FIXME: add and use V6? |
|
|
||
| /// The version group ID for version 6 transactions. | ||
| /// | ||
| /// Orchard transactions must use transaction version 5 and this version |
There was a problem hiding this comment.
| /// Orchard transactions must use transaction version 5 and this version | |
| /// Orchard ZSA transactions must use transaction version 6 and this version |
| /// 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) |
There was a problem hiding this comment.
Adding the changed-files and codespell lint jobs from here may be trivial and help catch typos.
| } | ||
| | Transaction::V6 { | ||
| sapling_shielded_data: None, | ||
| .. |
There was a problem hiding this comment.
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 🤷.
| Transaction::V6 { | ||
| sapling_shielded_data: Some(sapling_shielded_data), | ||
| .. | ||
| } => Box::new(sapling_shielded_data.spends_per_anchor()), |
There was a problem hiding this comment.
Nitpick/Optional: Transaction::V5 | Transaction::V6 would work here and for all of the sapling methods below too.
There was a problem hiding this comment.
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.
| orchard_shielded_data: None, | ||
| }, | ||
| v5 @ V5 { .. } => v5.clone(), | ||
| V6 { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Fixed: renamed transaction_to_fake_v5 function to transaction_to_fake_min_v5 and changed V6 arm of the match to:
V6 => panic!("V6 transactions are not supported in this test!"),| } => { | ||
| // Transaction V6 spec: | ||
| // FIXME: specify a proper ref | ||
| // https://zips.z.cash/protocol/protocol.pdf#txnencoding |
There was a problem hiding this comment.
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).
| /// 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; |
There was a problem hiding this comment.
Optional: It's 0x124A69F8 in zcash_primitives, if that was random, let's use that so it won't need an update.
| // 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. |
There was a problem hiding this comment.
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.
| &None, | ||
| &None, | ||
| sapling_shielded_data, | ||
| orchard_shielded_data, |
There was a problem hiding this comment.
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
…and panic if V6 passed into it
…action/arbitrary.rs
|
Closed in favour of #37 |
1 similar comment
|
Closed in favour of #37 |
This pull request merges the changes from several incremental PRs into one cumulative set of updates on top of the `zsa1` branch. It introduces ZSA-compatible crates, Network Upgrade 7 (Nu7), initial Transaction V6 support, Orchard generics for ZSA, property-based testing enhancements, note commitment handling for ZSA issuance, and initial consensus modifications for Orchard ZSA. It does not include the final state management changes or additional tests — those come in subsequent PRs. Below is a high-level overview of the merged changes: 1. **ZSA-Compatible Crates Integration (Step 1)** #24: - Replaces core libraries (`halo2`, `zcash_note_encryption`, etc.) with QED-it’s ZSA-compatible forks. - Maintains full support for Orchard “Vanilla” without activating any new ZSA features yet. 2. **Network Upgrade 7 (Nu7) Support (Step 2)** #15 - Introduces code paths and placeholders for Nu7, the upcoming network upgrade required for ZSA. - Adds `FIXME` comments where final activation heights and other specifics must be filled in. 3. **Transaction V6 Foundations (Step 3)** #16 - Adds a `V6` variant to Zebra’s `Transaction` enum, initially mirroring `V5` logic. - Sets a baseline for future ZSA-related modifications, including placeholders for relevant fields and logic. 4. **Refactor Orchard Structures to Generics (Step 4)** #17 - Converts key Orchard data structures (e.g., `ShieldedData`) to generics, enabling a single code path for both Orchard Vanilla and Orchard ZSA. - Implements serialization/deserialization for `V6` transactions, including a `burn` field in the ZSA flavor. 5. **Orchard Proptests with ZSA Enhancements (Step 5)** #18 - Extends the property-based testing framework to handle ZSA-specific fields and behaviors in `Transaction V6`. - Refactors code organization (e.g., extracting `Burn` types) for better clarity and future expansion. 6. **Integration of ZSA Issuance Commitments (Step 6)** #25 - Merges issuance action note commitments with existing shielded data commitments for `V6` transactions. - Ensures `Transaction::orchard_note_commitments` includes issuance note commitments when present, preserving `V5` behavior. 7. **Initial ZSA Consensus Support (Step 7)** #28 - Modifies `zebra-consensus` to support Orchard ZSA. **Next Steps** - **State Management & Additional Testing**: Future PRs will introduce state-layer modifications, refine consensus checks, and add more comprehensive tests. By consolidating these first several steps into a single PR, we aim to simplify the review process .
3 participants
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
Transactionenum in thezebra-chaincrate, currently replicating the behavior of V5.Notes
Next steps
FIXMEcomments in the code.zebra-chain::orchard::ShieldedDatastruct to be a generic that supports both V5 and V6./tx_v6_with_ser_1.8.0branch can be utilized).