Initial Tx V6 support #7978

PaulLaux · 2023-11-22T08:11:21Z

Motivation

Not to be merged - this is a draft PR to facilitate discussion and collaboration.

This pull request aims to implement the Transaction V6 format as specified in ZIP 230 (https://qed-it.github.io/zips/zip-0230), enabling support for Zcash Shielded Assets (ZSA) features, such as burn and issuance.

This is an early PR to have a discussion around the proposed structure and the way forward.

The main objective is to have an agreement about extending the ShieldedData struct:

pub struct ShieldedData<V: OrchardFlavour> {
    /// The orchard flags for this transaction.
    /// Denoted as `flagsOrchard` in the spec.
    pub flags: Flags,
    /// The net value of Orchard spends minus outputs.
    /// Denoted as `valueBalanceOrchard` in the spec.
    pub value_balance: Amount,
    /// The shared anchor for all `Spend`s in this transaction.
    /// Denoted as `anchorOrchard` in the spec.
    pub shared_anchor: tree::Root,
    /// The aggregated zk-SNARK proof for all the actions in this transaction.
    /// Denoted as `proofsOrchard` in the spec.
    pub proof: Halo2Proof,
    /// The Orchard Actions, in the order they appear in the transaction.
    /// Denoted as `vActionsOrchard` and `vSpendAuthSigsOrchard` in the spec.
    pub actions: AtLeastOne<AuthorizedAction<V>>,
    /// A signature on the transaction `sighash`.
    /// Denoted as `bindingSigOrchard` in the spec.
    pub binding_sig: Signature<Binding>,

    #[cfg(feature = "tx-v6")]
    /// Assets intended for burning
    pub burn: V::BurnType,
}

PR Author Checklist

Check before marking the PR as ready for review:

Will the PR name make sense to users?
Does the PR have a priority label?
Have you added or updated tests?
Is the documentation up to date?

For significant changes:

Is there a summary in the CHANGELOG?
Can these changes be split into multiple PRs?

Specifications

This PR focuses on structural changes in zebra-chain to accommodate the future implementation of the Zcash Transaction Version 6 format. Serialization/deserialization for V6 has not been implemented in this iteration.

Complex Code or Requirements

No significant changes to concurrency, unsafe code, or complex consensus rules have been introduced.

Solution

Added a new V6 item to the Transaction enum.
Added a tx-v6 Rust feature flag to enable conditional compilation
Generalized ShieldedData data struct to support both Orchard and OrchardZSA flavours.

Testing

Unit tests have been updated to support the generic ShieldedData and validate the behavior of both V5 and V6 transactions. While new tests specific to V6 have not been added at this stage, existing tests ensure compatibility with V5 transactions.

Review

This PR is not blocking any other work. Reviewers are invited to verify that the scope aligns with the ticket, tests cover the PR's motivation, and any potential blockers have been addressed.

Reviewer Checklist

Check before approving the PR:

Does the PR scope match the ticket?
Are there enough tests to make sure it works? Do the tests cover the PR motivation?
Are all the PR blockers dealt with?
PR blockers can be dealt with in new tickets or PRs.

Follow Up Work

The next steps may include implementing V6 serialization/deserialization and adding specific tests for V6 transactions.

…nsaction methods, fix compilation errors

This commit introduces the initial support for V6 transactions, which include support for Zcash Shielded Assets (ZSA) features, such as burn and issuance. The changes encompass the necessary code adjustments and implementations to enable V6 transactions with these features. Additional testing and further refinements will follow.

…Note directly in arbitrary.rs + other minor changes

…ion errors

… it instead of HAS_BURN etc.

…the last version of the main branch)

…y.rs

…n to OrchardVariant

…g to OrchardZSA

… Cargo.toml for now to resolve cargo conflict with orchard_zsa

PaulLaux · 2023-11-22T14:49:13Z

zebra-chain/src/orchard/orchard_flavour.rs

+/// A trait representing compile-time settings of Orchard Shielded Protocol used in
+/// the transactions `V5` and `V6`.
+pub trait OrchardFlavour: Clone + Debug {
+    /// The size of the encrypted note for this protocol version.
+    const ENCRYPTED_NOTE_SIZE: usize;
+
+    /// A type representing an encrypted note for this protocol version.
+    type EncryptedNote: Clone
+        + Debug
+        + PartialEq
+        + Eq
+        + DeserializeOwned
+        + Serialize
+        + ZcashDeserialize
+        + ZcashSerialize;
+
+    /// A type representing a burn field for this protocol version.
+    type BurnType: Clone + Debug + Default + ZcashDeserialize + ZcashSerialize;
+}


This trait encapsulate the difference between orchard and orchardZSA

Looks good, thank you for the explicit NoBurn. (It's much easier to review than ().)

PaulLaux · 2023-11-22T14:51:09Z

zebra-chain/src/orchard/shielded_data.rs

+    #[cfg(feature = "tx-v6")]
+    /// Assets intended for burning
+    /// TODO: FIXME: Add ref to spec
+    pub burn: V::BurnType,
 }


We will appreciate the feedback on this extension:

pub struct ShieldedData<V: OrchardFlavour> { ... }

In general this seems like a useful design. We know it works because we used something similar for v4 and v5 transaction formats for sapling.

teor2345

This seems like a useful design, thank you for the clear code!

I'm just checking with the team how many reviewers we want to have a look at it.

teor2345 · 2023-11-23T23:44:26Z

zebra-chain/src/orchard/shielded_data.rs

+// TODO: FIXME: Consider moving it to transaction.rs as it's not used here. Or move its usage here from transaction.rs.
+/// A struct that contains references to several fields of an `Action` struct.
+/// Those fields are used in other parts of the code that call the `orchard_actions()` method of the `Transaction`.
+/// The goal of using `ActionRef` is that it's not a generic, unlike `Action`, so it can be returned from Transaction methods
+/// (the fields of `ActionRef` do not depend on the generic parameter `Version` of `Action`).
+pub struct ActionRef<'a> {
+    /// A reference to the value commitment to the net value of the input note minus the output note.
+    pub cv: &'a super::commitment::ValueCommitment,
+    /// A reference to the nullifier of the input note being spent.
+    pub nullifier: &'a super::note::Nullifier,
+    /// A reference to the randomized validating key for `spendAuthSig`.
+    pub rk: &'a reddsa::VerificationKeyBytes<SpendAuth>,
+    /// A reference to the x-coordinate of the note commitment for the output note.
+    pub cm_x: &'a pallas::Base,
+}


Since this data is small, it would be ok to clone it. But if it works with lifetimes that's also ok.

For example, we clone anchors to abstract over v4 and v5 sapling transaction structures:

zebra/zebra-chain/src/sapling/shielded_data.rs

Lines 298 to 322 in ca062d0

impl<AnchorV> TransferData<AnchorV>

where

AnchorV: AnchorVariant + Clone,

Spend<PerSpendAnchor>: From<(Spend<AnchorV>, AnchorV::Shared)>,

{

/// Iterate over the [`Spend`]s for this transaction, returning

/// `Spend<PerSpendAnchor>` regardless of the underlying transaction version.

///

/// Allows generic operations over V4 and V5 transactions, including:

/// * spend verification, and

/// * non-malleable transaction ID generation.

///

/// # Correctness

///

/// Do not use this function for serialization.

pub fn spends_per_anchor(&self) -> impl Iterator<Item = Spend<PerSpendAnchor>> + '_ {

self.spends().cloned().map(move |spend| {

Spend::<PerSpendAnchor>::from((

spend,

self.shared_anchor()

.expect("shared anchor must be Some if there are any spends"),

))

})

}

}

teor2345 · 2023-11-23T23:46:09Z

zebra-chain/src/orchard/shielded_data.rs

+        // 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::<super::OrchardZSA>::max_allocation();


This is acceptable as long as the max allocation is higher than any protocol limits. TrustedPreallocate just prevents denial of service attacks that preallocate a lot of memory by providing a very large list size.

teor2345 · 2023-11-23T23:49:09Z

zebra-chain/src/orchard/shielded_data.rs

+    #[cfg(feature = "tx-v6")]
+    /// Assets intended for burning
+    /// TODO: FIXME: Add ref to spec
+    pub burn: V::BurnType,
 }


In general this seems like a useful design. We know it works because we used something similar for v4 and v5 transaction formats for sapling.

teor2345 · 2023-11-23T23:50:08Z

zebra-chain/src/orchard/orchard_flavour.rs

+/// A trait representing compile-time settings of Orchard Shielded Protocol used in
+/// the transactions `V5` and `V6`.
+pub trait OrchardFlavour: Clone + Debug {
+    /// The size of the encrypted note for this protocol version.
+    const ENCRYPTED_NOTE_SIZE: usize;
+
+    /// A type representing an encrypted note for this protocol version.
+    type EncryptedNote: Clone
+        + Debug
+        + PartialEq
+        + Eq
+        + DeserializeOwned
+        + Serialize
+        + ZcashDeserialize
+        + ZcashSerialize;
+
+    /// A type representing a burn field for this protocol version.
+    type BurnType: Clone + Debug + Default + ZcashDeserialize + ZcashSerialize;
+}


Looks good, thank you for the explicit NoBurn. (It's much easier to review than ().)

arya2

This is excellent, thank you.

zebra-chain/src/orchard/orchard_flavour.rs

…r NoBurn

…ts field as copies instead of refs, for clarity and simplicity

Dmitry Demin added 30 commits September 29, 2023 18:47

Add draft transaction V6 support

a63c27b

Return Box dyn Iterator instead of impls Iterator for orchard_... Tra…

ec01dd1

…nsaction methods, fix compilation errors

Remove PropTest trait from tx_version.rs and constrain in V:Encrypted…

b90a93d

…Note directly in arbitrary.rs + other minor changes

Fix unit tests to support orchard ShieldedData generic

8a2de1d

Align tx-v6 feature flag usage across crates and fix related compilat…

f077fe9

…ion errors

Rename V5/V6 to TxV5/V6 in orcahrd, add BurnType to TxVersion and use…

dbc1d6c

… it instead of HAS_BURN etc.

Fix test compilation error

7ca0f72

Add Clone to TxVersion::BurnType

528b95f

Remove patch.crates-io from the root Cargo.toml

dc28259

Fix datetime_full to limit nsecs with 1_000_000_000_u32 (as it is in …

34d3ac0

…the last version of the main branch)

Remove 'default' for tx-v6 feature from zcash-chain's Cargo.toml

e0d77ba

Use a special new empty struct type NoBurn isntead of () union type

5425793

Add missed derives for NoBurn

b551585

Add ci-test to .github/workflows

6ad9fbe

Disable (for now) new clippy warning (unnecessary_unwrap) in arbitrar…

2dfe984

…y.rs

Use 1.72.0 Rust toolchain in ci-test woirkflow action

4571355

Move burn and issuance modules to orchard_zsa folder, rename TxVersio…

7f02f7f

…n to OrchardVariant

Rename orchard_variant.rs to variant.rs, TxV5 tag to Orchard, TxV6 ta…

d95dc5f

…g to OrchardZSA

Fix comment

2c8e2e2

Fix comment (2)

42469f1

Make fixes to resolve merge conflict with the latest version of zsa1

47a7d5a

Merge branch 'zsa1' into tx_v6

1187576

Add Cargo.lock

1678e69

Update Cargo.lock

cced989

Add missed orchard_zsa

c9e3210

Remove temp file

8eb2e76

Add missed orchard_zsa

dfc127a

Pin blake2b_simd/blake2s_simd to 1.0.1 in zebra-chain/zebra-consensus…

e43d151

… Cargo.toml for now to resolve cargo conflict with orchard_zsa

Trigger Build

24cbbc1

Dmitry Demin and others added 6 commits November 21, 2023 14:35

Trigger Build (2)

e1c0b1c

Trigger Build (3)

3c1f225

Trigger Build (4)

616d1ce

Trigger Build (5)

368cb54

Trigger Build (6)

09a3d53

renamed orchard_varient -> orchard_flavour

469d5b1

github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Nov 22, 2023

PaulLaux commented Nov 22, 2023

View reviewed changes

teor2345 added do-not-merge Tells Mergify not to merge this PR C-suggestion and removed C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Nov 22, 2023

teor2345 mentioned this pull request Nov 23, 2023

fix(ci): Skip GCP CI jobs on PRs from external contributors, let mergify test them after approval #7956

Merged

9 tasks

teor2345 reviewed Nov 23, 2023

View reviewed changes

arya2 reviewed Nov 24, 2023

View reviewed changes

zebra-chain/src/orchard/orchard_flavour.rs Outdated Show resolved Hide resolved

Merge branch 'main' into tx_v6_update

79b2df4

github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Dec 6, 2023

Dmitry Demin added 3 commits December 6, 2023 11:26

Remove unnecessary curly braces in ZcashDeserialize implementation fo…

1027bb3

…r NoBurn

Add ref to spec in comment for burn field of ShieldedData struct

04495ab

Rename ActionRef to ActionCopy in orchard::ShieldedData, and define i…

7ad4f34

…ts field as copies instead of refs, for clarity and simplicity

teor2345 removed the do-not-merge Tells Mergify not to merge this PR label Dec 12, 2023

oxarbitrage added the do-not-merge Tells Mergify not to merge this PR label Jan 17, 2024

arya2 removed the do-not-merge Tells Mergify not to merge this PR label Feb 26, 2024

arya2 added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Apr 15, 2024

oxarbitrage added the do-not-merge Tells Mergify not to merge this PR label May 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Tx V6 support #7978

Initial Tx V6 support #7978

PaulLaux commented Nov 22, 2023

PaulLaux Nov 22, 2023

teor2345 Nov 23, 2023

PaulLaux Nov 22, 2023 •

edited

teor2345 Nov 23, 2023

teor2345 left a comment

teor2345 Nov 23, 2023

teor2345 Nov 23, 2023

teor2345 Nov 23, 2023

teor2345 Nov 23, 2023

arya2 left a comment

	impl<AnchorV> TransferData<AnchorV>
	where
	AnchorV: AnchorVariant + Clone,
	Spend<PerSpendAnchor>: From<(Spend<AnchorV>, AnchorV::Shared)>,
	{
	/// Iterate over the [`Spend`]s for this transaction, returning
	/// `Spend<PerSpendAnchor>` regardless of the underlying transaction version.
	///
	/// Allows generic operations over V4 and V5 transactions, including:
	/// * spend verification, and
	/// * non-malleable transaction ID generation.
	///
	/// # Correctness
	///
	/// Do not use this function for serialization.
	pub fn spends_per_anchor(&self) -> impl Iterator<Item = Spend<PerSpendAnchor>> + '_ {
	self.spends().cloned().map(move \|spend\| {
	Spend::<PerSpendAnchor>::from((
	spend,
	self.shared_anchor()
	.expect("shared anchor must be Some if there are any spends"),
	))
	})
	}
	}

Initial Tx V6 support #7978

Are you sure you want to change the base?

Initial Tx V6 support #7978

Conversation

PaulLaux commented Nov 22, 2023

Motivation

Not to be merged - this is a draft PR to facilitate discussion and collaboration.

PR Author Checklist

Check before marking the PR as ready for review:

For significant changes:

Specifications

Complex Code or Requirements

Solution

Testing

Review

Reviewer Checklist

Follow Up Work

PaulLaux Nov 22, 2023

Choose a reason for hiding this comment

teor2345 Nov 23, 2023

Choose a reason for hiding this comment

PaulLaux Nov 22, 2023 • edited

Choose a reason for hiding this comment

teor2345 Nov 23, 2023

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 Nov 23, 2023

Choose a reason for hiding this comment

teor2345 Nov 23, 2023

Choose a reason for hiding this comment

teor2345 Nov 23, 2023

Choose a reason for hiding this comment

teor2345 Nov 23, 2023

Choose a reason for hiding this comment

arya2 left a comment

Choose a reason for hiding this comment

PaulLaux Nov 22, 2023 •

edited