Redesign Transaction V5 serialization, impl trusted vector security, nullifier utility functions (#1996)

* add sapling shielded data to transaction V5

* implement nullifiers

* test v5 in shielded_data_roundtrip

* Explicitly design serialization for Transaction V5

Implement serialization for V4 and V5 spends and outputs, to make sure
that the design works.

* Test serialization for v5 spends and outputs

Also add a few missing v4 tests.

* Delete a disabled proptest

* Make v5 transactions a top-level heading

And add a missing serialized type.

* Fix a comment typo

* v5 transaction RFC: split array serialization

Based on #2017

* RFC: explicitly describe serialized field order

And link to the spec

* RFC: add the shared anchor serialization rule test

Co-authored-by: teor <teor@riseup.net>

book/src/dev/rfcs/0010-v5-transaction.md

@@ -36,7 +36,7 @@ To highlight changes most of the document comments from the code snippets in the
 ## Sapling Changes Overview
 [sapling-changes-overview]: #sapling-changes-overview
 
-V4 and V5 transactions both support sapling, but the underlying data structures are different. So need to make the sapling data types generic over the V4 and V5 structures.
+V4 and V5 transactions both support sapling, but the underlying data structures are different. So we need to make the sapling data types generic over the V4 and V5 structures.
 
 In V4, anchors are per-spend, but in V5, they are per-transaction.
 
@@ -52,9 +52,25 @@ Orchard uses `Halo2Proof`s with corresponding signature type changes. Each Orcha
 ## Other Transaction V5 Changes
 [other-transaction-v5-changes]: #other-transaction-v5-changes
 
-The order of some of the fields changed from V4 to V5. For example the `lock_time` and `expiry_height` were moved above the transparent inputs and outputs.
+V5 transactions split `Spend`s, `Output`s, and `AuthorizedAction`s into multiple arrays,
+with a single `compactsize` count before the first array. We add new
+`zcash_deserialize_external_count` and `zcash_serialize_external_count` utility functions,
+which make it easier to serialize and deserialize these arrays correctly.
 
-Zebra enums and structs put fields in serialized order. Composite fields are ordered based on **last** data deserialized for each field.
+The order of some of the fields changed from V4 to V5. For example the `lock_time` and
+`expiry_height` were moved above the transparent inputs and outputs.
+
+The serialized field order and field splits are in [the V5 transaction section in the NU5 spec](https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus).
+(Currently, the V5 spec is on a separate page after the V1-V4 specs.)
+
+Zebra's structs sometimes use a different order from the spec.
+We combine fields that occur together, to make it impossible to represent structurally
+invalid Zcash data.
+
+In general:
+* Zebra enums and structs put fields in serialized order.
+* Composite structs and emnum variants are ordered based on **last** data
+  deserialized for the composite.
 
 # Reference-level explanation
 [reference-level-explanation]: #reference-level-explanation
@@ -86,6 +102,18 @@ enum Transaction::V4 {
 }
 ```
 
+The following types have `ZcashSerialize` and `ZcashDeserialize` implementations,
+because they can be serialized into a single byte vector:
+* `transparent::Input`
+* `transparent::Output`
+* `LockTime`
+* `block::Height`
+* `Option<JoinSplitData<Groth16Proof>>`
+
+Note: `Option<sapling::ShieldedData<PerSpendAnchor>>` does not have serialize or deserialize implementations,
+because the binding signature is after the joinsplits. Its serialization and deserialization is handled as
+part of `Transaction::V4`.
+
 ### Anchor Variants
 [anchor-variants]: #anchor-variants
 
@@ -123,17 +151,27 @@ We use `AnchorVariant` in `ShieldedData` to model the anchor differences between
 struct sapling::ShieldedData<AnchorV: AnchorVariant> {
     value_balance: Amount,
     shared_anchor: AnchorV::Shared,
+    // The following fields are in a different order to the serialized data, see:
+    // https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus
     first: Either<Spend<AnchorV>, Output>,
     rest_spends: Vec<Spend<AnchorV>>,
     rest_outputs: Vec<Output>,
-    binding_sig: Signature<Binding>,
+    binding_sig: redjubjub::Signature<Binding>,
 }
 ```
 
+The following types have `ZcashSerialize` and `ZcashDeserialize` implementations,
+because they can be serialized into a single byte vector:
+* `Amount`
+* `sapling::tree::Root`
+* `redjubjub::Signature<Binding>`
+
 ### Adding V5 Sapling Spend
 [adding-v5-sapling-spend]: #adding-v5-sapling-spend
 
-Sapling spend code is located at `zebra-chain/src/sapling/spend.rs`. We use `AnchorVariant` to model the anchor differences between V4 and V5:
+Sapling spend code is located at `zebra-chain/src/sapling/spend.rs`.
+We use `AnchorVariant` to model the anchor differences between V4 and V5.
+And we create a struct for serializing V5 transaction spends:
 
 ```rust
 struct Spend<AnchorV: AnchorVariant> {
@@ -141,16 +179,50 @@ struct Spend<AnchorV: AnchorVariant> {
     per_spend_anchor: AnchorV::PerSpend,
     nullifier: note::Nullifier,
     rk: redjubjub::VerificationKeyBytes<SpendAuth>,
+    // This field is stored in a separate array in v5 transactions, see:
+    // https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus
+    // parse using `zcash_deserialize_external_count` and `zcash_serialize_external_count`
     zkproof: Groth16Proof,
+    // This fields is stored in another separate array in v5 transactions
     spend_auth_sig: redjubjub::Signature<SpendAuth>,
 }
+
+/// The serialization prefix fields of a `Spend` in Transaction V5.
+///
+/// In `V5` transactions, spends are split into multiple arrays, so the prefix,
+/// proof, and signature must be serialised and deserialized separately.
+///
+/// Serialized as `SpendDescriptionV5` in [protocol specification §7.3].
+struct SpendPrefixInTransactionV5 {
+    cv: commitment::ValueCommitment,
+    nullifier: note::Nullifier,
+    rk: redjubjub::VerificationKeyBytes<SpendAuth>,
+}
 ```
 
-### No Changes to Sapling Output
+The following types have `ZcashSerialize` and `ZcashDeserialize` implementations,
-[no-changes-to-sapling-output]: #no-changes-to-sapling-output
+because they can be serialized into a single byte vector:
+* `Spend<PerSpendAnchor>` (moved from the pre-RFC `Spend`)
+* `SpendPrefixInTransactionV5` (new)
+* `Groth16Proof`
+* `redjubjub::Signature<redjubjub::SpendAuth>` (new - for v5 spend auth sig arrays)
 
-In Zcash the Sapling output representations are the same for V4 and V5 transactions, so no variants are needed. The output code is located at `zebra-chain/src/sapling/output.rs`:
+Note: `Spend<SharedAnchor>` does not have serialize and deserialize implementations.
+It must be split using `into_v5_parts` before serialization, and
+recombined using `from_v5_parts` after deserialization.
 
+These convenience methods convert between `Spend<SharedAnchor>` and its v5 parts:
+`SpendPrefixInTransactionV5`, the spend proof, and the spend auth signature.
+
+### Changes to Sapling Output
+[changes-to-sapling-output]: #changes-to-sapling-output
+
+In Zcash the Sapling output fields are the same for V4 and V5 transactions,
+so the `Output` struct is unchanged. However, V4 and V5 transactions serialize
+outputs differently, so we create additional structs for serializing outputs in
+each transaction version.
+
+The output code is located at `zebra-chain/src/sapling/output.rs`:
 ```rust
 struct Output {
     cv: commitment::ValueCommitment,
@@ -158,14 +230,48 @@ struct Output {
     ephemeral_key: keys::EphemeralPublicKey,
     enc_ciphertext: note::EncryptedNote,
     out_ciphertext: note::WrappedNoteKey,
+    // This field is stored in a separate array in v5 transactions, see:
+    // https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus
+    // parse using `zcash_deserialize_external_count` and `zcash_serialize_external_count`
     zkproof: Groth16Proof,
 }
+
+/// Wrapper for `Output` serialization in a `V4` transaction.
+struct OutputInTransactionV4(pub Output);
+
+/// The serialization prefix fields of an `Output` in Transaction V5.
+///
+/// In `V5` transactions, spends are split into multiple arrays, so the prefix
+/// and proof must be serialised and deserialized separately.
+///
+/// Serialized as `OutputDescriptionV5` in [protocol specification §7.3].
+struct OutputPrefixInTransactionV5 {
+    cv: commitment::ValueCommitment,
+    cm_u: jubjub::Fq,
+    ephemeral_key: keys::EphemeralPublicKey,
+    enc_ciphertext: note::EncryptedNote,
+    out_ciphertext: note::WrappedNoteKey,
+}
 ```
 
-## Orchard Additions
+The following fields have `ZcashSerialize` and `ZcashDeserialize` implementations,
-[orchard-additions]: #orchard-additions
+because they can be serialized into a single byte vector:
+* `OutputInTransactionV4` (moved from `Output`)
+* `OutputPrefixInTransactionV5` (new)
+* `Groth16Proof`
 
-### Adding V5 Transactions
+Note: The serialize and deserialize implementations on `Output` are moved to
+`OutputInTransactionV4`. In v4 transactions, outputs must be wrapped using
+`into_v4` before serialization, and unwrapped using
+`from_v4` after deserialization. In transaction v5, outputs
+must be split using `into_v5_parts` before serialization, and
+recombined using `from_v5_parts` after deserialization.
+
+These convenience methods convert `Output` to:
+* its v4 serialization wrapper `OutputInTransactionV4`, and
+* its v5 parts: `OutputPrefixInTransactionV5` and the output proof.
+
+## Adding V5 Transactions
 [adding-v5-transactions]: #adding-v5-transactions
 
 Now lets see how the V5 transaction is specified in the protocol, this is the second table of [Transaction Encoding and Consensus](https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus) and how are we going to represent it based in the above changes for Sapling fields and the new Orchard fields.
@@ -185,6 +291,18 @@ enum Transaction::V5 {
 
 To model the V5 anchor type, `sapling_shielded_data` uses the `SharedAnchor` variant located at `zebra-chain/src/transaction/sapling/shielded_data.rs`.
 
+The following fields have `ZcashSerialize` and `ZcashDeserialize` implementations,
+because they can be serialized into a single byte vector:
+* `LockTime`
+* `block::Height`
+* `transparent::Input`
+* `transparent::Output`
+* `Option<sapling::ShieldedData<SharedAnchor>>` (new)
+* `Option<orchard::ShieldedData>` (new)
+
+## Orchard Additions
+[orchard-additions]: #orchard-additions
+
 ### Adding Orchard ShieldedData
 [adding-orchard-shieldeddata]: #adding-orchard-shieldeddata
 
@@ -202,12 +320,19 @@ struct orchard::ShieldedData {
     /// an invalid `ShieldedData` with no actions.
     first: AuthorizedAction,
     rest: Vec<AuthorizedAction>,
-    binding_sig: redpallas::Signature<redpallas::Binding>,
+    binding_sig: redpallas::Signature<Binding>,
 }
 ```
 
 The fields are ordered based on the **last** data deserialized for each field.
 
+The following types have `ZcashSerialize` and `ZcashDeserialize` implementations,
+because they can be serialized into a single byte vector:
+* `orchard::Flags` (new)
+* `Amount`
+* `Halo2Proof` (new)
+* `redpallas::Signature<Binding>` (new)
+
 ### Adding Orchard AuthorizedAction
 [adding-orchard-authorizedaction]: #adding-orchard-authorizedaction
 
@@ -219,12 +344,27 @@ In `V5` transactions, there is one `SpendAuth` signature for every `Action`. To
 /// Every authorized Orchard `Action` must have a corresponding `SpendAuth` signature.
 struct orchard::AuthorizedAction {
     action: Action,
-    spend_auth_sig: redpallas::Signature<redpallas::SpendAuth>,
+    // This field is stored in a separate array in v5 transactions, see:
+    // https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus
+    // parse using `zcash_deserialize_external_count` and `zcash_serialize_external_count`
+    spend_auth_sig: redpallas::Signature<SpendAuth>,
 }
 ```
 
 Where `Action` is defined as [Action definition](https://github.com/ZcashFoundation/zebra/blob/68c12d045b63ed49dd1963dd2dc22eb991f3998c/zebra-chain/src/orchard/action.rs#L18-L41).
 
+The following types have `ZcashSerialize` and `ZcashDeserialize` implementations,
+because they can be serialized into a single byte vector:
+* `Action` (new)
+* `redpallas::Signature<SpendAuth>` (new)
+
+Note: `AuthorizedAction` does not have serialize and deserialize implementations.
+It must be split using `into_parts` before serialization, and
+recombined using `from_parts` after deserialization.
+
+These convenience methods convert between `AuthorizedAction` and its parts:
+`Action` and the spend auth signature.
+
 ### Adding Orchard Flags
 [adding-orchard-flags]: #adding-orchard-flags
 
@@ -261,6 +401,7 @@ This type is also defined in `orchard/shielded_data.rs`.
   - "Fake" Sapling-only and Sapling/Transparent transactions based on the existing test vectors, converted from V4 to V5 format
     - We can write a test utility function to automatically do these conversions
   - An empty transaction, with no Orchard, Sapling, or Transparent data
+    - A v5 transaction with no spends, but some outputs, to test the shared anchor serialization rule
   - Any available `zcashd` test vectors
 - After NU5 activation on testnet:
   - Add test vectors using the testnet activation block and 2 more post-activation blocks

zebra-chain/src/sapling.rs

@@ -20,7 +20,7 @@ pub use address::Address;
 pub use commitment::{CommitmentRandomness, NoteCommitment, ValueCommitment};
 pub use keys::Diversifier;
 pub use note::{EncryptedNote, Note, Nullifier, WrappedNoteKey};
-pub use output::Output;
+pub use output::{Output, OutputInTransactionV4};
 pub use shielded_data::{
     AnchorVariant, FieldNotPresent, PerSpendAnchor, SharedAnchor, ShieldedData,
 };

zebra-chain/src/sapling/arbitrary.rs

@@ -4,8 +4,8 @@ use proptest::{arbitrary::any, array, collection::vec, prelude::*};
 use crate::primitives::Groth16Proof;
 
 use super::{
-    keys, note, tree, FieldNotPresent, NoteCommitment, Output, PerSpendAnchor, SharedAnchor, Spend,
+    keys, note, tree, FieldNotPresent, NoteCommitment, Output, OutputInTransactionV4,
-    ValueCommitment,
+    PerSpendAnchor, SharedAnchor, Spend, ValueCommitment,
 };
 
 impl Arbitrary for Spend<PerSpendAnchor> {
@@ -89,3 +89,13 @@ impl Arbitrary for Output {
 
     type Strategy = BoxedStrategy<Self>;
 }
+
+impl Arbitrary for OutputInTransactionV4 {
+    type Parameters = ();
+
+    fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy {
+        any::<Output>().prop_map(OutputInTransactionV4).boxed()
+    }
+
+    type Strategy = BoxedStrategy<Self>;
+}

zebra-chain/src/sapling/output.rs

@@ -12,6 +12,11 @@ use super::{commitment, keys, note};
 
 /// A _Output Description_, as described in [protocol specification §7.4][ps].
 ///
+/// # Differences between Transaction Versions
+///
+/// `V4` transactions serialize the fields of spends and outputs together.
+/// `V5` transactions split them into multiple arrays.
+///
 /// [ps]: https://zips.z.cash/protocol/protocol.pdf#outputencoding
 #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
 pub struct Output {
@@ -30,7 +35,73 @@ pub struct Output {
     pub zkproof: Groth16Proof,
 }
 
+/// Wrapper for `Output` serialization in a `V4` transaction.
+///
+/// https://zips.z.cash/protocol/protocol.pdf#outputencoding
+#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
+pub struct OutputInTransactionV4(pub Output);
+
+/// The serialization prefix fields of an `Output` in Transaction V5.
+///
+/// In `V5` transactions, spends are split into multiple arrays, so the prefix
+/// and proof must be serialised and deserialized separately.
+///
+/// Serialized as `OutputDescriptionV5` in [protocol specification §7.3][ps].
+///
+/// [ps]: https://zips.z.cash/protocol/protocol.pdf#outputencoding
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
+pub struct OutputPrefixInTransactionV5 {
+    /// A value commitment to the value of the input note.
+    pub cv: commitment::ValueCommitment,
+    /// The u-coordinate of the note commitment for the output note.
+    #[serde(with = "serde_helpers::Fq")]
+    pub cm_u: jubjub::Fq,
+    /// An encoding of an ephemeral Jubjub public key.
+    pub ephemeral_key: keys::EphemeralPublicKey,
+    /// A ciphertext component for the encrypted output note.
+    pub enc_ciphertext: note::EncryptedNote,
+    /// A ciphertext component for the encrypted output note.
+    pub out_ciphertext: note::WrappedNoteKey,
+}
+
 impl Output {
+    /// Remove the V4 transaction wrapper from this output.
+    pub fn from_v4(output: OutputInTransactionV4) -> Output {
+        output.0
+    }
+
+    /// Add a V4 transaction wrapper to this output.
+    pub fn into_v4(self) -> OutputInTransactionV4 {
+        OutputInTransactionV4(self)
+    }
+
+    /// Combine the prefix and non-prefix fields from V5 transaction
+    /// deserialization.
+    pub fn from_v5_parts(prefix: OutputPrefixInTransactionV5, zkproof: Groth16Proof) -> Output {
+        Output {
+            cv: prefix.cv,
+            cm_u: prefix.cm_u,
+            ephemeral_key: prefix.ephemeral_key,
+            enc_ciphertext: prefix.enc_ciphertext,
+            out_ciphertext: prefix.out_ciphertext,
+            zkproof,
+        }
+    }
+
+    /// Split out the prefix and non-prefix fields for V5 transaction
+    /// serialization.
+    pub fn into_v5_parts(self) -> (OutputPrefixInTransactionV5, Groth16Proof) {
+        let prefix = OutputPrefixInTransactionV5 {
+            cv: self.cv,
+            cm_u: self.cm_u,
+            ephemeral_key: self.ephemeral_key,
+            enc_ciphertext: self.enc_ciphertext,
+            out_ciphertext: self.out_ciphertext,
+        };
+
+        (prefix, self.zkproof)
+    }
+
     /// Encodes the primary inputs for the proof statement as 5 Bls12_381 base
     /// field elements, to match bellman::groth16::verify_proof.
     ///
@@ -54,45 +125,103 @@ impl Output {
     }
 }
 
-impl ZcashSerialize for Output {
+impl OutputInTransactionV4 {
+    /// Add V4 transaction wrapper to this output.
+    pub fn from_output(output: Output) -> OutputInTransactionV4 {
+        OutputInTransactionV4(output)
+    }
+
+    /// Remove the V4 transaction wrapper from this output.
+    pub fn into_output(self) -> Output {
+        self.0
+    }
+}
+
+impl ZcashSerialize for OutputInTransactionV4 {
     fn zcash_serialize<W: io::Write>(&self, mut writer: W) -> Result<(), io::Error> {
-        self.cv.zcash_serialize(&mut writer)?;
+        let output = self.0.clone();
-        writer.write_all(&self.cm_u.to_bytes())?;
+        output.cv.zcash_serialize(&mut writer)?;
-        self.ephemeral_key.zcash_serialize(&mut writer)?;
+        writer.write_all(&output.cm_u.to_bytes())?;
-        self.enc_ciphertext.zcash_serialize(&mut writer)?;
+        output.ephemeral_key.zcash_serialize(&mut writer)?;
-        self.out_ciphertext.zcash_serialize(&mut writer)?;
+        output.enc_ciphertext.zcash_serialize(&mut writer)?;
-        self.zkproof.zcash_serialize(&mut writer)?;
+        output.out_ciphertext.zcash_serialize(&mut writer)?;
+        output.zkproof.zcash_serialize(&mut writer)?;
         Ok(())
     }
 }
 
-impl ZcashDeserialize for Output {
+impl ZcashDeserialize for OutputInTransactionV4 {
     fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
-        Ok(Output {
+        Ok(OutputInTransactionV4(Output {
             cv: commitment::ValueCommitment::zcash_deserialize(&mut reader)?,
             cm_u: jubjub::Fq::zcash_deserialize(&mut reader)?,
             ephemeral_key: keys::EphemeralPublicKey::zcash_deserialize(&mut reader)?,
             enc_ciphertext: note::EncryptedNote::zcash_deserialize(&mut reader)?,
             out_ciphertext: note::WrappedNoteKey::zcash_deserialize(&mut reader)?,
             zkproof: Groth16Proof::zcash_deserialize(&mut reader)?,
+        }))
+    }
+}
+
+// In a V5 transaction, zkproof is deserialized separately, so we can only
+// deserialize V5 outputs in the context of a V5 transaction.
+//
+// Instead, implement serialization and deserialization for the
+// Output prefix fields, which are stored in the same array.
+
+impl ZcashSerialize for OutputPrefixInTransactionV5 {
+    fn zcash_serialize<W: io::Write>(&self, mut writer: W) -> Result<(), io::Error> {
+        self.cv.zcash_serialize(&mut writer)?;
+        writer.write_all(&self.cm_u.to_bytes())?;
+        self.ephemeral_key.zcash_serialize(&mut writer)?;
+        self.enc_ciphertext.zcash_serialize(&mut writer)?;
+        self.out_ciphertext.zcash_serialize(&mut writer)?;
+        Ok(())
+    }
+}
+
+impl ZcashDeserialize for OutputPrefixInTransactionV5 {
+    fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
+        Ok(OutputPrefixInTransactionV5 {
+            cv: commitment::ValueCommitment::zcash_deserialize(&mut reader)?,
+            cm_u: jubjub::Fq::zcash_deserialize(&mut reader)?,
+            ephemeral_key: keys::EphemeralPublicKey::zcash_deserialize(&mut reader)?,
+            enc_ciphertext: note::EncryptedNote::zcash_deserialize(&mut reader)?,
+            out_ciphertext: note::WrappedNoteKey::zcash_deserialize(&mut reader)?,
         })
     }
 }
 
+/// The size of a v5 output, without associated fields.
+///
+/// This is the size of outputs in the initial array, there is another
+/// array of zkproofs required in the transaction format.
+pub(crate) const OUTPUT_PREFIX_SIZE: u64 = 32 + 32 + 32 + 580 + 80;
 /// An output contains: a 32 byte cv, a 32 byte cmu, a 32 byte ephemeral key
 /// a 580 byte encCiphertext, an 80 byte outCiphertext, and a 192 byte zkproof
 /// [ps]: https://zips.z.cash/protocol/protocol.pdf#outputencoding
-pub(crate) const OUTPUT_SIZE: u64 = 32 + 32 + 32 + 580 + 80 + 192;
+pub(crate) const OUTPUT_SIZE: u64 = OUTPUT_PREFIX_SIZE + 192;
 
-/// The maximum number of outputs in a valid Zcash on-chain transaction.
+/// The maximum number of sapling outputs in a valid Zcash on-chain transaction.
+/// This maximum is the same for transaction V4 and V5, even though the fields are
+/// serialized in a different order.
 ///
 /// If a transaction contains more outputs than can fit in maximally large block, it might be
 /// valid on the network and in the mempool, but it can never be mined into a block. So
 /// rejecting these large edge-case transactions can never break consensus
-impl TrustedPreallocate for Output {
+impl TrustedPreallocate for OutputInTransactionV4 {
     fn max_allocation() -> u64 {
         // Since a serialized Vec<Output> uses at least one byte for its length,
         // the max allocation can never exceed (MAX_BLOCK_BYTES - 1) / OUTPUT_SIZE
         (MAX_BLOCK_BYTES - 1) / OUTPUT_SIZE
     }
 }
+
+impl TrustedPreallocate for OutputPrefixInTransactionV5 {
+    fn max_allocation() -> u64 {
+        // Since V4 and V5 have the same fields,
+        // and the V5 associated fields are required,
+        // a valid max allocation can never exceed this size
+        OutputInTransactionV4::max_allocation()
+    }
+}

zebra-chain/src/sapling/shielded_data.rs

@@ -5,17 +5,23 @@
 //! The anchor change is handled using the `AnchorVariant` type trait.
 
 use futures::future::Either;
+use serde::{de::DeserializeOwned, Serialize};
 
 use crate::{
     amount::Amount,
-    primitives::redjubjub::{Binding, Signature},
+    primitives::{
-    sapling::{tree, Nullifier, Output, Spend, ValueCommitment},
+        redjubjub::{Binding, Signature},
-    serialization::serde_helpers,
+        Groth16Proof,
+    },
+    sapling::{
+        output::OutputPrefixInTransactionV5, spend::SpendPrefixInTransactionV5, tree, Nullifier,
+        Output, Spend, ValueCommitment,
+    },
+    serialization::{serde_helpers, TrustedPreallocate},
 };
 
-use serde::{de::DeserializeOwned, Serialize};
 use std::{
-    cmp::{Eq, PartialEq},
+    cmp::{max, Eq, PartialEq},
     fmt::Debug,
 };
 
@@ -83,7 +89,13 @@ where
     pub value_balance: Amount,
     /// The shared anchor for all `Spend`s in this transaction.
     ///
-    /// Some transaction versions do not have this field.
+    /// The anchor is the root of the Sapling note commitment tree in a previous
+    /// block. This root should be in the best chain for a transaction to be
+    /// mined, and it must be in the relevant chain for a transaction to be
+    /// valid.
+    ///
+    /// Some transaction versions have a per-spend anchor, rather than a shared
+    /// anchor.
     pub shared_anchor: AnchorV::Shared,
     /// Either a spend or output description.
     ///
@@ -237,3 +249,17 @@ where
 
 impl<AnchorV> std::cmp::Eq for ShieldedData<AnchorV> where AnchorV: AnchorVariant + Clone + PartialEq
 {}
+
+impl TrustedPreallocate for Groth16Proof {
+    fn max_allocation() -> u64 {
+        // Each V5 transaction proof array entry must have a corresponding
+        // spend or output prefix. We use the larger limit, so we don't reject
+        // any valid large blocks.
+        //
+        // TODO: put a separate limit on proofs in spends and outputs
+        max(
+            SpendPrefixInTransactionV5::max_allocation(),
+            OutputPrefixInTransactionV5::max_allocation(),
+        )
+    }
+}

zebra-chain/src/sapling/spend.rs

@@ -27,14 +27,23 @@ use super::{commitment, note, tree, AnchorVariant, FieldNotPresent, PerSpendAnch
 /// there is a single `shared_anchor` for the entire transaction. This
 /// structural difference is modeled using the `AnchorVariant` type trait.
 ///
+/// `V4` transactions serialize the fields of spends and outputs together.
+/// `V5` transactions split them into multiple arrays.
+///
 /// [ps]: https://zips.z.cash/protocol/protocol.pdf#spendencoding
 #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
 pub struct Spend<AnchorV: AnchorVariant> {
     /// A value commitment to the value of the input note.
     pub cv: commitment::ValueCommitment,
-    /// A root of the Sapling note commitment tree at some block height in the past.
+    /// An anchor for this spend.
     ///
-    /// Some transaction versions do not have this field.
+    /// The anchor is the root of the Sapling note commitment tree in a previous
+    /// block. This root should be in the best chain for a transaction to be
+    /// mined, and it must be in the relevant chain for a transaction to be
+    /// valid.
+    ///
+    /// Some transaction versions have a shared anchor, rather than a per-spend
+    /// anchor.
     pub per_spend_anchor: AnchorV::PerSpend,
     /// The nullifier of the input note.
     pub nullifier: note::Nullifier,
@@ -46,6 +55,24 @@ pub struct Spend<AnchorV: AnchorVariant> {
     pub spend_auth_sig: redjubjub::Signature<SpendAuth>,
 }
 
+/// The serialization prefix fields of a `Spend` in Transaction V5.
+///
+/// In `V5` transactions, spends are split into multiple arrays, so the prefix,
+/// proof, and signature must be serialised and deserialized separately.
+///
+/// Serialized as `SpendDescriptionV5` in [protocol specification §7.3][ps].
+///
+/// [ps]: https://zips.z.cash/protocol/protocol.pdf#spendencoding
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
+pub struct SpendPrefixInTransactionV5 {
+    /// A value commitment to the value of the input note.
+    pub cv: commitment::ValueCommitment,
+    /// The nullifier of the input note.
+    pub nullifier: note::Nullifier,
+    /// The randomized public key for `spend_auth_sig`.
+    pub rk: redjubjub::VerificationKeyBytes<SpendAuth>,
+}
+
 impl From<(Spend<SharedAnchor>, tree::Root)> for Spend<PerSpendAnchor> {
     /// Convert a `Spend<SharedAnchor>` and its shared anchor, into a
     /// `Spend<PerSpendAnchor>`.
@@ -98,6 +125,43 @@ impl Spend<PerSpendAnchor> {
     }
 }
 
+impl Spend<SharedAnchor> {
+    /// Combine the prefix and non-prefix fields from V5 transaction
+    /// deserialization.
+    pub fn from_v5_parts(
+        prefix: SpendPrefixInTransactionV5,
+        zkproof: Groth16Proof,
+        spend_auth_sig: redjubjub::Signature<SpendAuth>,
+    ) -> Spend<SharedAnchor> {
+        Spend::<SharedAnchor> {
+            cv: prefix.cv,
+            per_spend_anchor: FieldNotPresent,
+            nullifier: prefix.nullifier,
+            rk: prefix.rk,
+            zkproof,
+            spend_auth_sig,
+        }
+    }
+
+    /// Split out the prefix and non-prefix fields for V5 transaction
+    /// serialization.
+    pub fn into_v5_parts(
+        self,
+    ) -> (
+        SpendPrefixInTransactionV5,
+        Groth16Proof,
+        redjubjub::Signature<SpendAuth>,
+    ) {
+        let prefix = SpendPrefixInTransactionV5 {
+            cv: self.cv,
+            nullifier: self.nullifier,
+            rk: self.rk,
+        };
+
+        (prefix, self.zkproof, self.spend_auth_sig)
+    }
+}
+
 impl ZcashSerialize for Spend<PerSpendAnchor> {
     fn zcash_serialize<W: io::Write>(&self, mut writer: W) -> Result<(), io::Error> {
         self.cv.zcash_serialize(&mut writer)?;
@@ -112,11 +176,10 @@ impl ZcashSerialize for Spend<PerSpendAnchor> {
 
 impl ZcashDeserialize for Spend<PerSpendAnchor> {
     fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
-        use crate::sapling::{commitment::ValueCommitment, note::Nullifier};
         Ok(Spend {
-            cv: ValueCommitment::zcash_deserialize(&mut reader)?,
+            cv: commitment::ValueCommitment::zcash_deserialize(&mut reader)?,
             per_spend_anchor: tree::Root(reader.read_32_bytes()?),
-            nullifier: Nullifier::from(reader.read_32_bytes()?),
+            nullifier: note::Nullifier::from(reader.read_32_bytes()?),
             rk: reader.read_32_bytes()?.into(),
             zkproof: Groth16Proof::zcash_deserialize(&mut reader)?,
             spend_auth_sig: reader.read_64_bytes()?.into(),
@@ -124,19 +187,54 @@ impl ZcashDeserialize for Spend<PerSpendAnchor> {
     }
 }
 
-impl ZcashSerialize for Spend<SharedAnchor> {
+// zkproof and spend_auth_sig are deserialized separately, so we can only
+// deserialize Spend<SharedAnchor> in the context of a V5 transaction.
+//
+// Instead, implement serialization and deserialization for the
+// Spend<SharedAnchor> prefix fields, which are stored in the same array.
+
+impl ZcashSerialize for SpendPrefixInTransactionV5 {
     fn zcash_serialize<W: io::Write>(&self, mut writer: W) -> Result<(), io::Error> {
         self.cv.zcash_serialize(&mut writer)?;
         writer.write_32_bytes(&self.nullifier.into())?;
         writer.write_all(&<[u8; 32]>::from(self.rk)[..])?;
-        // zkproof and spend_auth_sig are serialized separately
         Ok(())
     }
 }
 
-// zkproof and spend_auth_sig are deserialized separately, so we can only
+impl ZcashDeserialize for SpendPrefixInTransactionV5 {
-// deserialize Spend<SharedAnchor> in the context of a transaction
+    fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
+        Ok(SpendPrefixInTransactionV5 {
+            cv: commitment::ValueCommitment::zcash_deserialize(&mut reader)?,
+            nullifier: note::Nullifier::from(reader.read_32_bytes()?),
+            rk: reader.read_32_bytes()?.into(),
+        })
+    }
+}
 
+/// In Transaction V5, SpendAuth signatures are serialized and deserialized in a
+/// separate array.
+impl ZcashSerialize for redjubjub::Signature<SpendAuth> {
+    fn zcash_serialize<W: io::Write>(&self, mut writer: W) -> Result<(), io::Error> {
+        writer.write_all(&<[u8; 64]>::from(*self)[..])?;
+        Ok(())
+    }
+}
+
+impl ZcashDeserialize for redjubjub::Signature<SpendAuth> {
+    fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
+        Ok(reader.read_64_bytes()?.into())
+    }
+}
+
+/// The size of a spend with a per-spend anchor.
+pub(crate) const ANCHOR_PER_SPEND_SIZE: u64 = SHARED_ANCHOR_SPEND_SIZE + 32;
+
+/// The size of a spend with a shared anchor, without associated fields.
+///
+/// This is the size of spends in the initial array, there are another
+/// 2 arrays of zkproofs and spend_auth_sigs required in the transaction format.
+pub(crate) const SHARED_ANCHOR_SPEND_PREFIX_SIZE: u64 = 32 + 32 + 32;
 /// The size of a spend with a shared anchor, including associated fields.
 ///
 /// A Spend contains: a 32 byte cv, a 32 byte anchor (transaction V4 only),
@@ -144,33 +242,32 @@ impl ZcashSerialize for Spend<SharedAnchor> {
 /// in V5), and a 64 byte spendAuthSig (serialized separately in V5).
 ///
 /// [ps]: https://zips.z.cash/protocol/protocol.pdf#spendencoding
-pub(crate) const SHARED_ANCHOR_SPEND_FULL_SIZE: u64 = SHARED_ANCHOR_SPEND_INITIAL_SIZE + 192 + 64;
+pub(crate) const SHARED_ANCHOR_SPEND_SIZE: u64 = SHARED_ANCHOR_SPEND_PREFIX_SIZE + 192 + 64;
-/// The size of a spend with a shared anchor, without associated fields.
-///
-/// This is the size of spends in the initial array, there are another
-/// 2 arrays of zkproofs and spend_auth_sigs required in the transaction format.
-pub(crate) const SHARED_ANCHOR_SPEND_INITIAL_SIZE: u64 = 32 + 32 + 32;
 
-/// The size of a spend with a per-spend anchor.
+/// The maximum number of sapling spends in a valid Zcash on-chain transaction V4.
-pub(crate) const ANCHOR_PER_SPEND_SIZE: u64 = SHARED_ANCHOR_SPEND_FULL_SIZE + 32;
-
-/// The maximum number of spends in a valid Zcash on-chain transaction V5.
-///
-/// If a transaction contains more spends than can fit in maximally large block, it might be
-/// valid on the network and in the mempool, but it can never be mined into a block. So
-/// rejecting these large edge-case transactions can never break consensus.
-impl TrustedPreallocate for Spend<SharedAnchor> {
-    fn max_allocation() -> u64 {
-        // Since a serialized Vec<Spend> uses at least one byte for its length,
-        // and the associated fields are required,
-        // a valid max allocation can never exceed this size
-        (MAX_BLOCK_BYTES - 1) / SHARED_ANCHOR_SPEND_FULL_SIZE
-    }
-}
-
-/// The maximum number of spends in a valid Zcash on-chain transaction V4.
 impl TrustedPreallocate for Spend<PerSpendAnchor> {
     fn max_allocation() -> u64 {
         (MAX_BLOCK_BYTES - 1) / ANCHOR_PER_SPEND_SIZE
     }
 }
+
+/// The maximum number of sapling spends in a valid Zcash on-chain transaction V5.
+///
+/// If a transaction contains more spends than can fit in maximally large block, it might be
+/// valid on the network and in the mempool, but it can never be mined into a block. So
+/// rejecting these large edge-case transactions can never break consensus.
+impl TrustedPreallocate for SpendPrefixInTransactionV5 {
+    fn max_allocation() -> u64 {
+        // Since a serialized Vec<Spend> uses at least one byte for its length,
+        // and the associated fields are required,
+        // a valid max allocation can never exceed this size
+        (MAX_BLOCK_BYTES - 1) / SHARED_ANCHOR_SPEND_SIZE
+    }
+}
+
+impl TrustedPreallocate for redjubjub::Signature<SpendAuth> {
+    fn max_allocation() -> u64 {
+        // Each associated field must have a corresponding spend prefix.
+        SpendPrefixInTransactionV5::max_allocation()
+    }
+}

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

@@ -1,38 +1,46 @@
 //! Tests for trusted preallocation during deserialization.
 
-use super::super::{
-    output::{Output, OUTPUT_SIZE},
-    spend::{
-        Spend, ANCHOR_PER_SPEND_SIZE, SHARED_ANCHOR_SPEND_FULL_SIZE,
-        SHARED_ANCHOR_SPEND_INITIAL_SIZE,
-    },
-};
-
 use crate::{
     block::MAX_BLOCK_BYTES,
-    sapling::{AnchorVariant, PerSpendAnchor, SharedAnchor},
+    primitives::Groth16Proof,
+    sapling::{
+        output::{
+            Output, OutputInTransactionV4, OutputPrefixInTransactionV5, OUTPUT_PREFIX_SIZE,
+            OUTPUT_SIZE,
+        },
+        spend::{
+            Spend, SpendPrefixInTransactionV5, ANCHOR_PER_SPEND_SIZE,
+            SHARED_ANCHOR_SPEND_PREFIX_SIZE, SHARED_ANCHOR_SPEND_SIZE,
+        },
+        PerSpendAnchor, SharedAnchor,
+    },
     serialization::{TrustedPreallocate, ZcashSerialize},
 };
 
 use proptest::prelude::*;
-use std::convert::TryInto;
+use std::{cmp::max, convert::TryInto};
 
 proptest! {
-    /// Confirm that each spend takes exactly ANCHOR_PER_SPEND_SIZE bytes when serialized.
+    /// Confirm that each `Spend<PerSpendAnchor>` takes exactly
-    /// This verifies that our calculated `TrustedPreallocate::max_allocation()` is indeed an upper bound.
+    /// ANCHOR_PER_SPEND_SIZE bytes when serialized.
+    ///
+    /// This verifies that our calculated `TrustedPreallocate::max_allocation()`
+    /// is indeed an upper bound.
     #[test]
     fn anchor_per_spend_size_is_small_enough(spend in Spend::<PerSpendAnchor>::arbitrary_with(())) {
         let serialized = spend.zcash_serialize_to_vec().expect("Serialization to vec must succeed");
         prop_assert!(serialized.len() as u64 == ANCHOR_PER_SPEND_SIZE)
     }
 
-    /// Confirm that each spend takes exactly SHARED_SPEND_SIZE bytes when serialized.
+    /// Confirm that each `Spend<SharedAnchor>` takes exactly SHARED_SPEND_SIZE
+    /// bytes when serialized.
     #[test]
     fn shared_anchor_spend_size_is_small_enough(spend in Spend::<SharedAnchor>::arbitrary_with(())) {
-        let mut serialized_len = spend.zcash_serialize_to_vec().expect("Serialization to vec must succeed").len();
+        let (prefix, zkproof, spend_auth_sig) = spend.into_v5_parts();
-        serialized_len += spend.zkproof.zcash_serialize_to_vec().expect("Serialization to vec must succeed").len();
+        let mut serialized_len = prefix.zcash_serialize_to_vec().expect("Serialization to vec must succeed").len();
-        serialized_len += &<[u8; 64]>::from(spend.spend_auth_sig).len();
+        serialized_len += zkproof.zcash_serialize_to_vec().expect("Serialization to vec must succeed").len();
-        prop_assert!(serialized_len as u64 == SHARED_ANCHOR_SPEND_FULL_SIZE)
+        serialized_len += spend_auth_sig.zcash_serialize_to_vec().expect("Serialization to vec must succeed").len();
+        prop_assert!(serialized_len as u64 == SHARED_ANCHOR_SPEND_SIZE)
     }
 }
 
@@ -49,7 +57,7 @@ proptest! {
             smallest_disallowed_serialized_len,
             largest_allowed_vec_len,
             largest_allowed_serialized_len,
-        ) = spend_max_allocation_is_big_enough(spend);
+        ) = max_allocation_is_big_enough(spend);
 
         // Check that our smallest_disallowed_vec is only one item larger than the limit
         prop_assert!(((smallest_disallowed_vec_len - 1) as u64) == Spend::<PerSpendAnchor>::max_allocation());
@@ -64,44 +72,156 @@ proptest! {
         prop_assert!((largest_allowed_serialized_len as u64) <= MAX_BLOCK_BYTES);
     }
 
-    /// Verify trusted preallocation for `Spend<SharedAnchor>`
+    /// Verify trusted preallocation for `Spend<SharedAnchor>` and its split fields
     #[test]
     fn shared_spend_max_allocation_is_big_enough(spend in Spend::<SharedAnchor>::arbitrary_with(())) {
+        let (prefix, zkproof, spend_auth_sig) = spend.into_v5_parts();
         let (
             smallest_disallowed_vec_len,
             smallest_disallowed_serialized_len,
             largest_allowed_vec_len,
             largest_allowed_serialized_len,
-        ) = spend_max_allocation_is_big_enough(spend);
+        ) = max_allocation_is_big_enough(prefix);
 
-        prop_assert!(((smallest_disallowed_vec_len - 1) as u64) == Spend::<SharedAnchor>::max_allocation());
         // Calculate the actual size of all required Spend fields
-        //
+        prop_assert!((smallest_disallowed_serialized_len as u64)/SHARED_ANCHOR_SPEND_PREFIX_SIZE*SHARED_ANCHOR_SPEND_SIZE >= MAX_BLOCK_BYTES);
-        // TODO: modify the test to serialize the associated zkproof and
+        prop_assert!((largest_allowed_serialized_len as u64)/SHARED_ANCHOR_SPEND_PREFIX_SIZE*SHARED_ANCHOR_SPEND_SIZE <= MAX_BLOCK_BYTES);
-        // spend_auth_sig fields
-        prop_assert!((smallest_disallowed_serialized_len as u64)/SHARED_ANCHOR_SPEND_INITIAL_SIZE*SHARED_ANCHOR_SPEND_FULL_SIZE >= MAX_BLOCK_BYTES);
 
-        prop_assert!((largest_allowed_vec_len as u64) == Spend::<SharedAnchor>::max_allocation());
+        // Now check the serialization limits
+        prop_assert!(((smallest_disallowed_vec_len - 1) as u64) == SpendPrefixInTransactionV5::max_allocation());
+        prop_assert!((largest_allowed_vec_len as u64) == SpendPrefixInTransactionV5::max_allocation());
+        prop_assert!((largest_allowed_serialized_len as u64) <= MAX_BLOCK_BYTES);
+
+        // And check the other fields
+        let (
+            smallest_disallowed_vec_len,
+            _smallest_disallowed_serialized_len,
+            largest_allowed_vec_len,
+            largest_allowed_serialized_len,
+        ) = max_allocation_is_big_enough(zkproof);
+
+        // Proofs are special-cased, because a proof array is deserialized as
+        // part of both spends and outputs.
+        prop_assert!(((smallest_disallowed_vec_len - 1) as u64) == Groth16Proof::max_allocation());
+        prop_assert!((largest_allowed_vec_len as u64) == Groth16Proof::max_allocation());
+        prop_assert!((largest_allowed_serialized_len as u64) <= MAX_BLOCK_BYTES);
+
+        // Regardless of where they are deserialized, proofs must not exceed the
+        // greatest upper bound across spends and outputs.
+        prop_assert!((largest_allowed_vec_len as u64) <= max(SpendPrefixInTransactionV5::max_allocation(), OutputPrefixInTransactionV5::max_allocation()));
+
+
+        let (
+            smallest_disallowed_vec_len,
+            _smallest_disallowed_serialized_len,
+            largest_allowed_vec_len,
+            largest_allowed_serialized_len,
+        ) = max_allocation_is_big_enough(spend_auth_sig);
+
+        prop_assert!(((smallest_disallowed_vec_len - 1) as u64) == SpendPrefixInTransactionV5::max_allocation());
+        prop_assert!((largest_allowed_vec_len as u64) == SpendPrefixInTransactionV5::max_allocation());
         prop_assert!((largest_allowed_serialized_len as u64) <= MAX_BLOCK_BYTES);
     }
 }
 
-/// Return the following calculations on `spend`:
+proptest! {
+    /// Confirm that each output takes exactly OUTPUT_SIZE bytes when serialized
+    /// in a V4 or V5 transaction.
+    ///
+    /// This verifies that our calculated `TrustedPreallocate::max_allocation()`
+    /// is indeed an upper bound.
+    #[test]
+    fn output_size_is_small_enough(output in Output::arbitrary_with(())) {
+        let v4_serialized = output.clone().into_v4().zcash_serialize_to_vec().expect("Serialization to vec must succeed");
+        prop_assert!(v4_serialized.len() as u64 == OUTPUT_SIZE);
+
+        let (prefix, zkproof) = output.into_v5_parts();
+        let mut v5_serialized_len = prefix.zcash_serialize_to_vec().expect("Serialization to vec must succeed").len();
+        v5_serialized_len += zkproof.zcash_serialize_to_vec().expect("Serialization to vec must succeed").len();
+        prop_assert!(v5_serialized_len as u64 == OUTPUT_SIZE)
+    }
+}
+
+proptest! {
+    #![proptest_config(ProptestConfig::with_cases(128))]
+
+    /// Verify that...
+    /// 1. The smallest disallowed vector of `Outputs`s is too large to fit in a Zcash block
+    /// 2. The largest allowed vector is small enough to fit in a legal Zcash block
+    ///
+    /// when serialized in a V4 or V5 transaction.
+    #[test]
+    fn output_max_allocation_is_big_enough(output in Output::arbitrary_with(())) {
+
+        let (
+            smallest_disallowed_vec_len,
+            smallest_disallowed_serialized_len,
+            largest_allowed_vec_len,
+            largest_allowed_serialized_len,
+        ) = max_allocation_is_big_enough(output.clone().into_v4());
+
+        // Check that our smallest_disallowed_vec is only one item larger than the limit
+        prop_assert!(((smallest_disallowed_vec_len - 1) as u64) == OutputInTransactionV4::max_allocation());
+        // Check that our smallest_disallowed_vec is too big to send as a protocol message
+        // Note that a serialized block always includes at least one byte for the number of transactions,
+        // so any serialized Vec<Spend> at least MAX_BLOCK_BYTES long is too large to fit in a block.
+        prop_assert!((smallest_disallowed_serialized_len as u64) >= MAX_BLOCK_BYTES);
+
+        // Check that our largest_allowed_vec contains the maximum number of spends
+        prop_assert!((largest_allowed_vec_len as u64) == OutputInTransactionV4::max_allocation());
+        // Check that our largest_allowed_vec is small enough to send as a protocol message
+        prop_assert!((largest_allowed_serialized_len as u64) <= MAX_BLOCK_BYTES);
+
+        let (prefix, zkproof) = output.into_v5_parts();
+        let (
+            smallest_disallowed_vec_len,
+            smallest_disallowed_serialized_len,
+            largest_allowed_vec_len,
+            largest_allowed_serialized_len,
+        ) = max_allocation_is_big_enough(prefix);
+
+        // Calculate the actual size of all required Output fields
+        prop_assert!((smallest_disallowed_serialized_len as u64)/OUTPUT_PREFIX_SIZE*OUTPUT_SIZE >= MAX_BLOCK_BYTES);
+        prop_assert!((largest_allowed_serialized_len as u64)/OUTPUT_PREFIX_SIZE*OUTPUT_SIZE <= MAX_BLOCK_BYTES);
+
+        // Now check the serialization limits
+        prop_assert!(((smallest_disallowed_vec_len - 1) as u64) == OutputPrefixInTransactionV5::max_allocation());
+        prop_assert!((largest_allowed_vec_len as u64) == OutputPrefixInTransactionV5::max_allocation());
+        prop_assert!((largest_allowed_serialized_len as u64) <= MAX_BLOCK_BYTES);
+
+        // And check the other fields
+        let (
+            smallest_disallowed_vec_len,
+            _smallest_disallowed_serialized_len,
+            largest_allowed_vec_len,
+            largest_allowed_serialized_len,
+        ) = max_allocation_is_big_enough(zkproof);
+
+        // Proofs are special-cased, because a proof array is deserialized as
+        // part of both spends and outputs.
+        prop_assert!(((smallest_disallowed_vec_len - 1) as u64) == Groth16Proof::max_allocation());
+        prop_assert!((largest_allowed_vec_len as u64) == Groth16Proof::max_allocation());
+        prop_assert!((largest_allowed_serialized_len as u64) <= MAX_BLOCK_BYTES);
+
+        // Regardless of where they are deserialized, proofs must not exceed the
+        // greatest upper bound across spends and outputs.
+        prop_assert!((largest_allowed_vec_len as u64) <= max(SpendPrefixInTransactionV5::max_allocation(), OutputPrefixInTransactionV5::max_allocation()));
+    }
+}
+
+/// Return the following calculations on `item`:
 ///   smallest_disallowed_vec_len
 ///   smallest_disallowed_serialized_len
 ///   largest_allowed_vec_len
 ///   largest_allowed_serialized_len
-fn spend_max_allocation_is_big_enough<AnchorV>(
+fn max_allocation_is_big_enough<T>(item: T) -> (usize, usize, usize, usize)
-    spend: Spend<AnchorV>,
-) -> (usize, usize, usize, usize)
 where
-    AnchorV: AnchorVariant,
+    T: TrustedPreallocate + ZcashSerialize + Clone,
-    Spend<AnchorV>: TrustedPreallocate + ZcashSerialize + Clone,
 {
-    let max_allocation: usize = Spend::max_allocation().try_into().unwrap();
+    let max_allocation: usize = T::max_allocation().try_into().unwrap();
     let mut smallest_disallowed_vec = Vec::with_capacity(max_allocation + 1);
-    for _ in 0..(Spend::max_allocation() + 1) {
+    for _ in 0..(max_allocation + 1) {
-        smallest_disallowed_vec.push(spend.clone());
+        smallest_disallowed_vec.push(item.clone());
     }
     let smallest_disallowed_serialized = smallest_disallowed_vec
         .zcash_serialize_to_vec()
@@ -122,48 +242,3 @@ where
         largest_allowed_serialized.len(),
     )
 }
-
-proptest! {
-    /// Confirm that each output takes exactly OUTPUT_SIZE bytes when serialized.
-    /// This verifies that our calculated `TrustedPreallocate::max_allocation()` is indeed an upper bound.
-    #[test]
-    fn output_size_is_small_enough(output in Output::arbitrary_with(())) {
-        let serialized = output.zcash_serialize_to_vec().expect("Serialization to vec must succeed");
-        prop_assert!(serialized.len() as u64 == OUTPUT_SIZE)
-    }
-
-}
-
-proptest! {
-    #![proptest_config(ProptestConfig::with_cases(128))]
-
-    /// Verify that...
-    /// 1. The smallest disallowed vector of `Outputs`s is too large to fit in a Zcash block
-    /// 2. The largest allowed vector is small enough to fit in a legal Zcash block
-    #[test]
-    fn output_max_allocation_is_big_enough(output in Output::arbitrary_with(())) {
-
-        let max_allocation: usize = Output::max_allocation().try_into().unwrap();
-        let mut smallest_disallowed_vec = Vec::with_capacity(max_allocation + 1);
-        for _ in 0..(Output::max_allocation()+1) {
-            smallest_disallowed_vec.push(output.clone());
-        }
-        let smallest_disallowed_serialized = smallest_disallowed_vec.zcash_serialize_to_vec().expect("Serialization to vec must succeed");
-        // Check that our smallest_disallowed_vec is only one item larger than the limit
-        prop_assert!(((smallest_disallowed_vec.len() - 1) as u64) == Output::max_allocation());
-        // Check that our smallest_disallowed_vec is too big to be included in a valid block
-        // Note that a serialized block always includes at least one byte for the number of transactions,
-        // so any serialized Vec<Output> at least MAX_BLOCK_BYTES long is too large to fit in a block.
-        prop_assert!((smallest_disallowed_serialized.len() as u64) >= MAX_BLOCK_BYTES);
-
-        // Create largest_allowed_vec by removing one element from smallest_disallowed_vec without copying (for efficiency)
-        smallest_disallowed_vec.pop();
-        let largest_allowed_vec = smallest_disallowed_vec;
-        let largest_allowed_serialized = largest_allowed_vec.zcash_serialize_to_vec().expect("Serialization to vec must succeed");
-
-        // Check that our largest_allowed_vec contains the maximum number of Outputs
-        prop_assert!((largest_allowed_vec.len() as u64) == Output::max_allocation());
-        // Check that our largest_allowed_vec is small enough to fit in a Zcash block.
-        prop_assert!((largest_allowed_serialized.len() as u64) < MAX_BLOCK_BYTES);
-    }
-}

zebra-chain/src/sapling/tests/prop.rs

@@ -2,40 +2,107 @@ use proptest::prelude::*;
 
 use crate::{
     block,
-    sapling::{self, PerSpendAnchor},
+    sapling::{self, PerSpendAnchor, SharedAnchor},
     serialization::{ZcashDeserializeInto, ZcashSerialize},
     transaction::{LockTime, Transaction},
 };
 
 use futures::future::Either;
+use sapling::OutputInTransactionV4;
 
 proptest! {
-    // TODO: generalise this test for `ShieldedData<SharedAnchor>` (#1829)
+    /// Serialize and deserialize `Spend<PerSpendAnchor>`
     #[test]
-    fn shielded_data_roundtrip(shielded in any::<sapling::ShieldedData<PerSpendAnchor>>()) {
+    fn spend_v4_roundtrip(
+        spend in any::<sapling::Spend<PerSpendAnchor>>(),
+    ) {
+        zebra_test::init();
+
+        let data = spend.zcash_serialize_to_vec().expect("spend should serialize");
+        let spend_parsed = data.zcash_deserialize_into().expect("randomized spend should deserialize");
+        prop_assert_eq![spend, spend_parsed];
+    }
+
+    /// Serialize and deserialize `Spend<SharedAnchor>`
+    #[test]
+    fn spend_v5_roundtrip(
+        spend in any::<sapling::Spend<SharedAnchor>>(),
+    ) {
+        zebra_test::init();
+
+        let (prefix, zkproof, spend_auth_sig) = spend.into_v5_parts();
+
+        let data = prefix.zcash_serialize_to_vec().expect("spend prefix should serialize");
+        let parsed = data.zcash_deserialize_into().expect("randomized spend prefix should deserialize");
+        prop_assert_eq![prefix, parsed];
+
+        let data = zkproof.zcash_serialize_to_vec().expect("spend zkproof should serialize");
+        let parsed = data.zcash_deserialize_into().expect("randomized spend zkproof should deserialize");
+        prop_assert_eq![zkproof, parsed];
+
+        let data = spend_auth_sig.zcash_serialize_to_vec().expect("spend auth sig should serialize");
+        let parsed = data.zcash_deserialize_into().expect("randomized spend auth sig should deserialize");
+        prop_assert_eq![spend_auth_sig, parsed];
+    }
+
+    /// Serialize and deserialize `Output`
+    #[test]
+    fn output_roundtrip(
+        output in any::<sapling::Output>(),
+    ) {
+        zebra_test::init();
+
+        // v4 format
+        let data = output.clone().into_v4().zcash_serialize_to_vec().expect("output should serialize");
+        let output_parsed = data.zcash_deserialize_into::<OutputInTransactionV4>().expect("randomized output should deserialize").into_output();
+        prop_assert_eq![&output, &output_parsed];
+
+        // v5 format
+        let (prefix, zkproof) = output.into_v5_parts();
+
+        let data = prefix.zcash_serialize_to_vec().expect("output prefix should serialize");
+        let parsed = data.zcash_deserialize_into().expect("randomized output prefix should deserialize");
+        prop_assert_eq![prefix, parsed];
+
+        let data = zkproof.zcash_serialize_to_vec().expect("output zkproof should serialize");
+        let parsed = data.zcash_deserialize_into().expect("randomized output zkproof should deserialize");
+        prop_assert_eq![zkproof, parsed];
+
+    }
+}
+
+proptest! {
+    /// Serialize and deserialize `PerSpendAnchor` shielded data by including it
+    /// in a V4 transaction
+    //
+    // TODO: write a similar test for `ShieldedData<SharedAnchor>` (#1829)
+    #[test]
+    fn shielded_data_v4_roundtrip(
+        shielded_v4 in any::<sapling::ShieldedData<PerSpendAnchor>>(),
+    ) {
         zebra_test::init();
 
         // shielded data doesn't serialize by itself, so we have to stick it in
         // a transaction
+
+        // stick `PerSpendAnchor` shielded data into a v4 transaction
         let tx = Transaction::V4 {
             inputs: Vec::new(),
             outputs: Vec::new(),
             lock_time: LockTime::min_lock_time(),
             expiry_height: block::Height(0),
             joinsplit_data: None,
-            sapling_shielded_data: Some(shielded),
+            sapling_shielded_data: Some(shielded_v4),
         };
-
         let data = tx.zcash_serialize_to_vec().expect("tx should serialize");
         let tx_parsed = data.zcash_deserialize_into().expect("randomized tx should deserialize");
-
         prop_assert_eq![tx, tx_parsed];
     }
 
     /// Check that ShieldedData<PerSpendAnchor> is equal when `first` is swapped
     /// between a spend and an output
     //
-    // TODO: generalise this test for `ShieldedData<SharedAnchor>` (#1829)
+    // TODO: write a similar test for `ShieldedData<SharedAnchor>` (#1829)
     #[test]
     fn shielded_data_per_spend_swap_first_eq(shielded1 in any::<sapling::ShieldedData<PerSpendAnchor>>()) {
         use Either::*;
@@ -93,7 +160,7 @@ proptest! {
     /// Check that ShieldedData<PerSpendAnchor> serialization is equal if
     /// `shielded1 == shielded2`
     //
-    // TODO: generalise this test for `ShieldedData<SharedAnchor>` (#1829)
+    // TODO: write a similar test for `ShieldedData<SharedAnchor>` (#1829)
     #[test]
     fn shielded_data_per_spend_serialize_eq(shielded1 in any::<sapling::ShieldedData<PerSpendAnchor>>(), shielded2 in any::<sapling::ShieldedData<PerSpendAnchor>>()) {
         zebra_test::init();
@@ -140,7 +207,7 @@ proptest! {
     ///
     /// This test checks for extra fields that are not in `ShieldedData::eq`.
     //
-    // TODO: generalise this test for `ShieldedData<SharedAnchor>` (#1829)
+    // TODO: write a similar test for `ShieldedData<SharedAnchor>` (#1829)
     #[test]
     fn shielded_data_per_spend_field_assign_eq(shielded1 in any::<sapling::ShieldedData<PerSpendAnchor>>(), shielded2 in any::<sapling::ShieldedData<PerSpendAnchor>>()) {
         zebra_test::init();

zebra-chain/src/transaction.rs

@@ -106,6 +106,8 @@ pub enum Transaction {
         inputs: Vec<transparent::Input>,
         /// The transparent outputs from the transaction.
         outputs: Vec<transparent::Output>,
+        /// The sapling shielded data for this transaction, if any.
+        sapling_shielded_data: Option<sapling::ShieldedData<sapling::SharedAnchor>>,
         /// The rest of the transaction as bytes
         rest: Vec<u8>,
     },
@@ -188,12 +190,6 @@ impl Transaction {
                     .joinsplits()
                     .flat_map(|joinsplit| joinsplit.nullifiers.iter()),
             ),
-            // Maybe JoinSplits, maybe not, we're still deciding
-            Transaction::V5 { .. } => {
-                unimplemented!(
-                    "v5 transaction format as specified in ZIP-225 after decision on 2021-03-12"
-                )
-            }
             // No JoinSplits
             Transaction::V1 { .. }
             | Transaction::V2 {
@@ -207,7 +203,8 @@ impl Transaction {
             | Transaction::V4 {
                 joinsplit_data: None,
                 ..
-            } => Box::new(std::iter::empty()),
+            }
+            | Transaction::V5 { .. } => Box::new(std::iter::empty()),
         }
     }
 
@@ -216,21 +213,27 @@ impl Transaction {
         // This function returns a boxed iterator because the different
         // transaction variants end up having different iterator types
         match self {
-            // JoinSplits with Groth Proofs
+            // Spends with Groth Proofs
             Transaction::V4 {
                 sapling_shielded_data: Some(sapling_shielded_data),
                 ..
             } => Box::new(sapling_shielded_data.nullifiers()),
-            Transaction::V5 { .. } => {
+            Transaction::V5 {
-                unimplemented!("v5 transaction format as specified in ZIP-225")
+                sapling_shielded_data: Some(sapling_shielded_data),
-            }
+                ..
-            // No JoinSplits
+            } => Box::new(sapling_shielded_data.nullifiers()),
+
+            // No Spends
             Transaction::V1 { .. }
             | Transaction::V2 { .. }
             | Transaction::V3 { .. }
             | Transaction::V4 {
                 sapling_shielded_data: None,
                 ..
+            }
+            | Transaction::V5 {
+                sapling_shielded_data: None,
+                ..
             } => Box::new(std::iter::empty()),
         }
     }

zebra-chain/src/transaction/arbitrary.rs

@@ -108,15 +108,19 @@ impl Transaction {
             any::<block::Height>(),
             transparent::Input::vec_strategy(ledger_state, 10),
             vec(any::<transparent::Output>(), 0..10),
+            option::of(any::<sapling::ShieldedData<sapling::SharedAnchor>>()),
             any::<Vec<u8>>(),
         )
             .prop_map(
-                |(lock_time, expiry_height, inputs, outputs, rest)| Transaction::V5 {
+                |(lock_time, expiry_height, inputs, outputs, sapling_shielded_data, rest)| {
-                    lock_time,
+                    Transaction::V5 {
-                    expiry_height,
+                        lock_time,
-                    inputs,
+                        expiry_height,
-                    outputs,
+                        inputs,
-                    rest,
+                        outputs,
+                        sapling_shielded_data,
+                        rest,
+                    }
                 },
             )
             .boxed()
@@ -236,6 +240,43 @@ impl Arbitrary for sapling::ShieldedData<sapling::PerSpendAnchor> {
     type Strategy = BoxedStrategy<Self>;
 }
 
+impl Arbitrary for sapling::ShieldedData<sapling::SharedAnchor> {
+    type Parameters = ();
+
+    fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy {
+        (
+            any::<Amount>(),
+            any::<sapling::tree::Root>(),
+            prop_oneof![
+                any::<sapling::Spend<sapling::SharedAnchor>>().prop_map(Either::Left),
+                any::<sapling::Output>().prop_map(Either::Right)
+            ],
+            vec(any::<sapling::Spend<sapling::SharedAnchor>>(), 0..10),
+            vec(any::<sapling::Output>(), 0..10),
+            vec(any::<u8>(), 64),
+        )
+            .prop_map(
+                |(value_balance, shared_anchor, first, rest_spends, rest_outputs, sig_bytes)| {
+                    Self {
+                        value_balance,
+                        shared_anchor,
+                        first,
+                        rest_spends,
+                        rest_outputs,
+                        binding_sig: redjubjub::Signature::from({
+                            let mut b = [0u8; 64];
+                            b.copy_from_slice(sig_bytes.as_slice());
+                            b
+                        }),
+                    }
+                },
+            )
+            .boxed()
+    }
+
+    type Strategy = BoxedStrategy<Self>;
+}
+
 impl Arbitrary for Transaction {
     type Parameters = LedgerState;
 

zebra-chain/src/transaction/serialize.rs

@@ -17,6 +17,7 @@ use crate::{
 };
 
 use super::*;
+use sapling::Output;
 
 impl ZcashDeserialize for jubjub::Fq {
     fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
@@ -166,7 +167,11 @@ impl ZcashSerialize for Transaction {
                             spend.zcash_serialize(&mut writer)?;
                         }
                         writer.write_compactsize(shielded_data.outputs().count() as u64)?;
-                        for output in shielded_data.outputs() {
+                        for output in shielded_data
+                            .outputs()
+                            .cloned()
+                            .map(sapling::OutputInTransactionV4)
+                        {
                             output.zcash_serialize(&mut writer)?;
                         }
                     }
@@ -182,11 +187,14 @@ impl ZcashSerialize for Transaction {
                     None => {}
                 }
             }
+            // TODO: serialize sapling shielded data according to the V5 transaction spec
+            #[allow(unused_variables)]
             Transaction::V5 {
                 lock_time,
                 expiry_height,
                 inputs,
                 outputs,
+                sapling_shielded_data,
                 rest,
             } => {
                 // Write version 5 and set the fOverwintered bit.
@@ -197,6 +205,8 @@ impl ZcashSerialize for Transaction {
                 inputs.zcash_serialize(&mut writer)?;
                 outputs.zcash_serialize(&mut writer)?;
 
+                // TODO: serialize sapling shielded data according to the V5 transaction spec
+
                 // write the rest
                 writer.write_all(rest)?;
             }
@@ -272,7 +282,11 @@ impl ZcashDeserialize for Transaction {
 
                 let value_balance = (&mut reader).zcash_deserialize_into()?;
                 let mut shielded_spends = Vec::zcash_deserialize(&mut reader)?;
-                let mut shielded_outputs = Vec::zcash_deserialize(&mut reader)?;
+                let mut shielded_outputs =
+                    Vec::<sapling::OutputInTransactionV4>::zcash_deserialize(&mut reader)?
+                        .into_iter()
+                        .map(Output::from_v4)
+                        .collect();
 
                 let joinsplit_data = OptV4Jsd::zcash_deserialize(&mut reader)?;
 
@@ -311,7 +325,7 @@ impl ZcashDeserialize for Transaction {
                     joinsplit_data,
                 })
             }
-            (5, false) => {
+            (5, true) => {
                 let id = reader.read_u32::<LittleEndian>()?;
                 if id != TX_V5_VERSION_GROUP_ID {
                     return Err(SerializationError::Parse("expected TX_V5_VERSION_GROUP_ID"));
@@ -321,6 +335,8 @@ impl ZcashDeserialize for Transaction {
                 let inputs = Vec::zcash_deserialize(&mut reader)?;
                 let outputs = Vec::zcash_deserialize(&mut reader)?;
 
+                // TODO: deserialize sapling shielded data according to the V5 transaction spec
+
                 let mut rest = Vec::new();
                 reader.read_to_end(&mut rest)?;
 
@@ -329,6 +345,8 @@ impl ZcashDeserialize for Transaction {
                     expiry_height,
                     inputs,
                     outputs,
+                    // TODO: use deserialized sapling shielded data
+                    sapling_shielded_data: None,
                     rest,
                 })
             }

zebra-chain/src/transaction/sighash.rs

@@ -1,16 +1,19 @@
 use super::Transaction;
+
 use crate::{
     parameters::{
         ConsensusBranchId, NetworkUpgrade, OVERWINTER_VERSION_GROUP_ID, SAPLING_VERSION_GROUP_ID,
         TX_V5_VERSION_GROUP_ID,
     },
+    sapling,
     serialization::{WriteZcashExt, ZcashSerialize},
     transparent,
 };
+
 use blake2b_simd::Hash;
 use byteorder::{LittleEndian, WriteBytesExt};
-use io::Write;
+
-use std::io;
+use std::io::{self, Write};
 
 static ZIP143_EXPLANATION: &str = "Invalid transaction version: after Overwinter activation transaction versions 1 and 2 are rejected";
 static ZIP243_EXPLANATION: &str = "Invalid transaction version: after Sapling activation transaction versions 1, 2, and 3 are rejected";
@@ -478,7 +481,12 @@ impl<'a> SigHasher<'a> {
             .personal(ZCASH_SHIELDED_OUTPUTS_HASH_PERSONALIZATION)
             .to_state();
 
-        for output in shielded_data.outputs() {
+        // Correctness: checked for V4 transaction above
+        for output in shielded_data
+            .outputs()
+            .cloned()
+            .map(sapling::OutputInTransactionV4)
+        {
             output.zcash_serialize(&mut hash)?;
         }