Redesign Sapling data model for V5 shared anchor and spends

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

@@ -38,9 +38,9 @@ To highlight changes most of the document comments from the code snippets in the
 
 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.
+In V4, anchors are per-spend, but in V5, they are per-transaction. In V5, the shared anchor is only present if there is at least one spend.
 
-For consistency, also we move some fields into the `ShieldedData` type, and rename some fields and types.
+For consistency, we also move some fields into the `ShieldedData` type, and rename some fields and types.
 
 ## Orchard Additions Overview
 [orchard-additions-overview]: #orchard-additions-overview
@@ -128,11 +128,11 @@ struct FieldNotPresent;
 
 impl AnchorVariant for PerSpendAnchor {
     type Shared = FieldNotPresent;
-    type PerSpend = tree::Root;
+    type PerSpend = sapling::tree::Root;
 }
 
 impl AnchorVariant for SharedAnchor {
-    type Shared = tree::Root;
+    type Shared = sapling::tree::Root;
     type PerSpend = FieldNotPresent;
 }
 
@@ -146,20 +146,51 @@ trait AnchorVariant {
 [changes-to-sapling-shieldeddata]: #changes-to-sapling-shieldeddata
 
 We use `AnchorVariant` in `ShieldedData` to model the anchor differences between V4 and V5:
+* in V4, there is a per-spend anchor
+* in V5, there is a shared anchor, which is only present when there are spends
+
+If there are no spends and no outputs:
+* in v4, the value_balance is fixed to zero
+* in v5, the value balance field is not present
+* in both versions, the binding_sig field is not present
 
 ```rust
+/// ShieldedData ensures that value_balance and binding_sig are only present when
+/// there is at least one spend or output.
 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>,
+    transfers: sapling::TransferData<AnchorV>,
     binding_sig: redjubjub::Signature<Binding>,
 }
+
+/// TransferData ensures that:
+/// * there is at least one spend or output, and
+/// * the shared anchor is only present when there are spends
+enum sapling::TransferData<AnchorV: AnchorVariant> {
+    /// In Transaction::V5, if there are any spends,
+    /// there must also be a shared spend anchor.
+    SpendsAndMaybeOutputs {
+        shared_anchor: AnchorV::Shared,
+        spends: AtLeastOne<Spend<AnchorV>>,
+        maybe_outputs: Vec<Output>,
+    }
+
+    /// If there are no spends, there must not be a shared
+    /// anchor.
+    JustOutputs {
+        outputs: AtLeastOne<Output>,
+    }
+}
 ```
 
+The `AtLeastOne` type is a vector wrapper which always contains at least one
+element. For more details, see [its documentation](https://github.com/ZcashFoundation/zebra/blob/673b95dea5f0b057c11f2f450943b012fec75c00/zebra-chain/src/serialization/constraint.rs).
+<!-- TODO: update link to main branch when PR #2021 merges -->
+
+Some of these fields are in a different order to the serialized data, see
+[the V4 and V5 transaction specs](https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus)
+for details.
+
 The following types have `ZcashSerialize` and `ZcashDeserialize` implementations,
 because they can be serialized into a single byte vector:
 * `Amount`
@@ -314,12 +345,7 @@ struct orchard::ShieldedData {
     value_balance: Amount,
     shared_anchor: tree::Root,
     proof: Halo2Proof,
-    /// An authorized action description.
-    ///
-    /// Storing this separately ensures that it is impossible to construct
-    /// an invalid `ShieldedData` with no actions.
-    first: AuthorizedAction,
-    rest: Vec<AuthorizedAction>,
+    actions: AtLeastOne<AuthorizedAction>,
     binding_sig: redpallas::Signature<Binding>,
 }
 ```

zebra-chain/src/sapling.rs

@@ -22,6 +22,6 @@ pub use keys::Diversifier;
 pub use note::{EncryptedNote, Note, Nullifier, WrappedNoteKey};
 pub use output::{Output, OutputInTransactionV4, OutputPrefixInTransactionV5};
 pub use shielded_data::{
-    AnchorVariant, FieldNotPresent, PerSpendAnchor, SharedAnchor, ShieldedData,
+    AnchorVariant, FieldNotPresent, PerSpendAnchor, SharedAnchor, ShieldedData, TransferData,
 };
 pub use spend::{Spend, SpendPrefixInTransactionV5};

zebra-chain/src/sapling/shielded_data.rs

@@ -4,7 +4,6 @@
 //! The `value_balance` change is handled using the default zero value.
 //! The anchor change is handled using the `AnchorVariant` type trait.
 
-use futures::future::Either;
 use serde::{de::DeserializeOwned, Serialize};
 
 use crate::{
@@ -17,7 +16,7 @@ use crate::{
         output::OutputPrefixInTransactionV5, spend::SpendPrefixInTransactionV5, tree, Nullifier,
         Output, Spend, ValueCommitment,
     },
-    serialization::{serde_helpers, TrustedPreallocate},
+    serialization::{AtLeastOne, TrustedPreallocate},
 };
 
 use std::{
@@ -74,54 +73,93 @@ pub trait AnchorVariant {
 ///
 /// The Sapling `value_balance` field is optional in `Transaction::V5`, but
 /// required in `Transaction::V4`. In both cases, if there is no `ShieldedData`,
-/// then the field value must be zero. Therefore, only need to store
+/// then the field value must be zero. Therefore, we only need to store
 /// `value_balance` when there is some Sapling `ShieldedData`.
 ///
 /// In `Transaction::V4`, each `Spend` has its own anchor. In `Transaction::V5`,
-/// there is a single `shared_anchor` for the entire transaction. This
-/// structural difference is modeled using the `AnchorVariant` type trait.
-#[derive(Clone, Debug, Serialize, Deserialize)]
+/// there is a single `shared_anchor` for the entire transaction, which is only
+/// present when there is at least one spend. These structural differences are
+/// modeled using the `AnchorVariant` type trait and `TransferData` enum.
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
 pub struct ShieldedData<AnchorV>
 where
     AnchorV: AnchorVariant + Clone,
 {
     /// The net value of Sapling spend transfers minus output transfers.
     pub value_balance: Amount,
-    /// The shared anchor for all `Spend`s in this transaction.
-    ///
-    /// 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.
-    ///
-    /// Storing this separately ensures that it is impossible to construct
-    /// an invalid `ShieldedData` with no spends or outputs.
-    ///
-    /// However, it's not necessary to access or process `first` and `rest`
-    /// separately, as the [`ShieldedData::spends`] and [`ShieldedData::outputs`]
-    /// methods provide iterators over all of the [`Spend`]s and
-    /// [`Output`]s.
-    #[serde(with = "serde_helpers::Either")]
-    pub first: Either<Spend<AnchorV>, Output>,
-    /// The rest of the [`Spend`]s for this transaction.
-    ///
-    /// Note that the [`ShieldedData::spends`] method provides an iterator
-    /// over all spend descriptions.
-    pub rest_spends: Vec<Spend<AnchorV>>,
-    /// The rest of the [`Output`]s for this transaction.
+
+    /// A bundle of spends and outputs, containing at least one spend or
+    /// output.
     ///
-    /// Note that the [`ShieldedData::outputs`] method provides an iterator
-    /// over all output descriptions.
-    pub rest_outputs: Vec<Output>,
+    /// In V5 transactions, also contains a shared anchor, if there are any
+    /// spends.
+    pub transfers: TransferData<AnchorV>,
+
     /// A signature on the transaction hash.
     pub binding_sig: Signature<Binding>,
 }
 
+/// A bundle of [`Spend`] and [`Output`] descriptions, and a shared anchor.
+///
+/// This wrapper type bundles at least one Spend or Output description with
+/// the required anchor data, so that an `Option<ShieldedData>` (which contains
+/// this type) correctly models the presence or absence of any spends and
+/// shielded data, across both V4 and V5 transactions.
+///
+/// Specifically, TransferData ensures that:
+/// * there is at least one spend or output, and
+/// * the shared anchor is only present when there are spends.
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
+pub enum TransferData<AnchorV>
+where
+    AnchorV: AnchorVariant + Clone,
+{
+    /// A bundle containing at least one spend, and the shared spend anchor.
+    /// There can also be zero or more outputs.
+    ///
+    /// In Transaction::V5, if there are any spends, there must also be a shared
+    /// spend anchor.
+    SpendsAndMaybeOutputs {
+        /// The shared anchor for all `Spend`s in this transaction.
+        ///
+        /// 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.
+        ///
+        /// Use the `shared_anchor` method to access this field.
+        shared_anchor: AnchorV::Shared,
+
+        /// At least one spend.
+        ///
+        /// Use the [`ShieldedData::spends`] method to get an iterator over the
+        /// [`Spend`]s in this `TransferData`.
+        spends: AtLeastOne<Spend<AnchorV>>,
+
+        /// Maybe some outputs (can be empty).
+        ///
+        /// Use the [`ShieldedData::outputs`] method to get an iterator over the
+        /// [`Outputs`]s in this `TransferData`.
+        maybe_outputs: Vec<Output>,
+    },
+
+    /// A bundle containing at least one output, with no spends and no shared
+    /// spend anchor.
+    ///
+    /// In Transaction::V5, if there are no spends, there must not be a shared
+    /// anchor.
+    JustOutputs {
+        /// At least one output.
+        ///
+        /// Use the [`ShieldedData::outputs`] method to get an iterator over the
+        /// [`Outputs`]s in this `TransferData`.
+        outputs: AtLeastOne<Output>,
+    },
+}
+
 impl<AnchorV> ShieldedData<AnchorV>
 where
     AnchorV: AnchorVariant + Clone,
@@ -136,9 +174,13 @@ where
     ///
     /// 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.clone())))
+        self.spends().cloned().map(move |spend| {
+            Spend::<PerSpendAnchor>::from((
+                spend,
+                self.shared_anchor()
+                    .expect("shared anchor must be Some if there are any spends"),
+            ))
+        })
     }
 }
 
@@ -153,22 +195,21 @@ where
     ///
     /// Use this function for serialization.
     pub fn spends(&self) -> impl Iterator<Item = &Spend<AnchorV>> {
-        match self.first {
-            Either::Left(ref spend) => Some(spend),
-            Either::Right(_) => None,
-        }
-        .into_iter()
-        .chain(self.rest_spends.iter())
+        self.transfers.spends()
     }
 
     /// Iterate over the [`Output`]s for this transaction.
     pub fn outputs(&self) -> impl Iterator<Item = &Output> {
-        match self.first {
-            Either::Left(_) => None,
-            Either::Right(ref output) => Some(output),
-        }
-        .into_iter()
-        .chain(self.rest_outputs.iter())
+        self.transfers.outputs()
+    }
+
+    /// Provide the shared anchor for this transaction, if present.
+    ///
+    /// The shared anchor is only present if:
+    /// * there is at least one spend, and
+    /// * this is a `V5` transaction.
+    pub fn shared_anchor(&self) -> Option<AnchorV::Shared> {
+        self.transfers.shared_anchor()
     }
 
     /// Collect the [`Nullifier`]s for this transaction, if it contains
@@ -216,40 +257,51 @@ where
     }
 }
 
-// Technically, it's possible to construct two equivalent representations
-// of a ShieldedData with at least one spend and at least one output, depending
-// on which goes in the `first` slot.  This is annoying but a smallish price to
-// pay for structural validity.
-//
-// A `ShieldedData<PerSpendAnchor>` can never be equal to a
-// `ShieldedData<SharedAnchor>`, even if they have the same effects.
-
-impl<AnchorV> std::cmp::PartialEq for ShieldedData<AnchorV>
+impl<AnchorV> TransferData<AnchorV>
 where
-    AnchorV: AnchorVariant + Clone + PartialEq,
+    AnchorV: AnchorVariant + Clone,
 {
-    fn eq(&self, other: &Self) -> bool {
-        // First check that the lengths match, so we know it is safe to use zip,
-        // which truncates to the shorter of the two iterators.
-        if self.spends().count() != other.spends().count() {
-            return false;
-        }
-        if self.outputs().count() != other.outputs().count() {
-            return false;
+    /// Iterate over the [`Spend`]s for this transaction, returning them as
+    /// their generic type.
+    pub fn spends(&self) -> impl Iterator<Item = &Spend<AnchorV>> {
+        use TransferData::*;
+
+        let spends = match self {
+            SpendsAndMaybeOutputs { spends, .. } => Some(spends.iter()),
+            JustOutputs { .. } => None,
+        };
+
+        // this awkward construction avoids returning a newtype struct or
+        // type-erased boxed iterator
+        spends.into_iter().flatten()
+    }
+
+    /// Iterate over the [`Output`]s for this transaction.
+    pub fn outputs(&self) -> impl Iterator<Item = &Output> {
+        use TransferData::*;
+
+        match self {
+            SpendsAndMaybeOutputs { maybe_outputs, .. } => maybe_outputs,
+            JustOutputs { outputs, .. } => outputs.as_vec(),
         }
+        .iter()
+    }
+
+    /// Provide the shared anchor for this transaction, if present.
+    ///
+    /// The shared anchor is only present if:
+    /// * there is at least one spend, and
+    /// * this is a `V5` transaction.
+    pub fn shared_anchor(&self) -> Option<AnchorV::Shared> {
+        use TransferData::*;
 
-        // Now check that all the fields match
-        self.value_balance == other.value_balance
-            && self.shared_anchor == other.shared_anchor
-            && self.binding_sig == other.binding_sig
-            && self.spends().zip(other.spends()).all(|(a, b)| a == b)
-            && self.outputs().zip(other.outputs()).all(|(a, b)| a == b)
+        match self {
+            SpendsAndMaybeOutputs { shared_anchor, .. } => Some(shared_anchor.clone()),
+            JustOutputs { .. } => None,
+        }
     }
 }
 
-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