Move the check in `transaction::check::sapling_balances_match` to `V4` deserialization (#2234)

* Implement `PartialEq<i64>` for `Amount`

Allows to compare an `Amount` instance directly to an integer.

* Add `SerializationError::BadTransactionBalance`

Error variant representing deserialization of a transaction that doesn't
conform to the Sapling consensus rule where the balance MUST be zero if
there aren't any shielded spends and outputs.

* Validate consensus rule when deserializing

Return an error if the deserialized V4 transaction has a non-zero value
balance but doesn't have any Sapling shielded spends nor outputs.

* Add consensus rule link to field documentation

Describe how the consensus rule is validated structurally by
`ShieldedData`.

* Clarify that `value_balance` is zero

Make the description more concise and objective.

Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>

* Update field documentation

Include information about how the consensus rule is guaranteed during
serialization.

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

* Remove `check::sapling_balances_match` function

The check is redundant because the respective consensus rule is
validated structurally by `ShieldedData`.

* Test deserialization of invalid V4 transaction

A transaction with no Sapling shielded spends and no outputs but with a
non-zero balance value should fail to deserialize.

* Change least-significant byte of the value balance

State how the byte index is calculated, and change the least
significant-byte to be non-zero.

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

zebra-chain/src/amount.rs

@@ -227,6 +227,18 @@ impl<C1, C2> PartialEq<Amount<C2>> for Amount<C1> {
     }
 }
 
+impl<C> PartialEq<i64> for Amount<C> {
+    fn eq(&self, other: &i64) -> bool {
+        self.0.eq(other)
+    }
+}
+
+impl<C> PartialEq<Amount<C>> for i64 {
+    fn eq(&self, other: &Amount<C>) -> bool {
+        self.eq(&other.0)
+    }
+}
+
 impl Eq for Amount<NegativeAllowed> {}
 impl Eq for Amount<NonNegative> {}
 

zebra-chain/src/sapling/shielded_data.rs

@@ -86,6 +86,21 @@ where
     AnchorV: AnchorVariant + Clone,
 {
     /// The net value of Sapling spend transfers minus output transfers.
+    ///
+    /// [`ShieldedData`] validates this [value balance consensus
+    /// rule](https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus):
+    ///
+    /// "If effectiveVersion = 4 and there are no Spend descriptions or Output
+    /// descriptions, then valueBalanceSapling MUST be 0."
+    ///
+    /// During deserialization, this rule is checked when there are no spends and
+    /// no outputs.
+    ///
+    /// During serialization, this rule is structurally validated by [`ShieldedData`].
+    /// `value_balance` is a field in [`ShieldedData`], which must have at least
+    /// one spend or output in its `transfers` field. If [`ShieldedData`] is `None`
+    /// then there can not possibly be any spends or outputs, and the
+    /// `value_balance` is always serialized as zero.
     pub value_balance: Amount,
 
     /// A bundle of spends and outputs, containing at least one spend or

zebra-chain/src/serialization/error.rs

@@ -23,4 +23,10 @@ pub enum SerializationError {
         #[from]
         source: crate::amount::Error,
     },
+    /// Invalid transaction with a non-zero balance and no Sapling shielded spends or outputs.
+    ///
+    /// Transaction does not conform to the Sapling [consensus
+    /// rule](https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus).
+    #[error("transaction balance is non-zero but doesn't have Sapling shielded spends or outputs")]
+    BadTransactionBalance,
 }

zebra-chain/src/transaction/serialize.rs

@@ -608,6 +608,12 @@ impl ZcashDeserialize for Transaction {
                         outputs: shielded_outputs.try_into().expect("checked for outputs"),
                     })
                 } else {
+                    // There are no shielded outputs and no shielded spends, so the value balance
+                    // MUST be zero:
+                    // https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus
+                    if value_balance != 0 {
+                        return Err(SerializationError::BadTransactionBalance);
+                    }
                     None
                 };
 

zebra-chain/src/transaction/tests/vectors.rs

@@ -3,7 +3,7 @@ use super::super::*;
 use crate::{
     block::{Block, Height, MAX_BLOCK_BYTES},
     parameters::{Network, NetworkUpgrade},
-    serialization::{ZcashDeserialize, ZcashDeserializeInto, ZcashSerialize},
+    serialization::{SerializationError, ZcashDeserialize, ZcashDeserializeInto, ZcashSerialize},
 };
 
 use std::convert::TryInto;
@@ -37,6 +37,37 @@ fn librustzcash_tx_hash() {
     assert_eq!(hash, expected);
 }
 
+#[test]
+fn doesnt_deserialize_transaction_with_invalid_value_balance() {
+    zebra_test::init();
+
+    let dummy_transaction = Transaction::V4 {
+        inputs: vec![],
+        outputs: vec![],
+        lock_time: LockTime::Height(Height(1)),
+        expiry_height: Height(10),
+        joinsplit_data: None,
+        sapling_shielded_data: None,
+    };
+
+    let mut input_bytes = Vec::new();
+    dummy_transaction
+        .zcash_serialize(&mut input_bytes)
+        .expect("dummy transaction should serialize");
+    // Set value balance to non-zero
+    // There are 4 * 4 byte fields and 2 * 1 byte compact sizes = 18 bytes before the 8 byte amount
+    // (Zcash is little-endian unless otherwise specified:
+    // https://zips.z.cash/protocol/nu5.pdf#endian)
+    input_bytes[18] = 1;
+
+    let result = Transaction::zcash_deserialize(&input_bytes[..]);
+
+    assert!(matches!(
+        result,
+        Err(SerializationError::BadTransactionBalance)
+    ));
+}
+
 #[test]
 fn zip143_deserialize_and_round_trip() {
     zebra_test::init();

zebra-consensus/src/transaction.rs

@@ -215,8 +215,6 @@ where
                     }
 
                     if let Some(sapling_shielded_data) = sapling_shielded_data {
-                        check::sapling_balances_match(&sapling_shielded_data)?;
-
                         for spend in sapling_shielded_data.spends_per_anchor() {
                             // Consensus rule: cv and rk MUST NOT be of small
                             // order, i.e. [h_J]cv MUST NOT be 𝒪_J and [h_J]rk

zebra-consensus/src/transaction/check.rs

@@ -4,7 +4,7 @@
 
 use zebra_chain::{
     orchard::Flags,
-    sapling::{AnchorVariant, Output, PerSpendAnchor, ShieldedData, Spend},
+    sapling::{Output, PerSpendAnchor, Spend},
     transaction::Transaction,
 };
 
@@ -40,31 +40,6 @@ pub fn has_inputs_and_outputs(tx: &Transaction) -> Result<(), TransactionError>
     }
 }
 
-/// Check that if there are no Spends or Outputs, the Sapling valueBalance is also 0.
-///
-/// If effectiveVersion = 4 and there are no Spend descriptions or Output descriptions,
-/// then valueBalanceSapling MUST be 0.
-///
-/// This check is redundant for `Transaction::V5`, because the transaction format
-/// omits `valueBalanceSapling` when there are no spends and no outputs. But it's
-/// simpler to just do the redundant check anyway.
-///
-/// https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus
-pub fn sapling_balances_match<AnchorV>(
-    sapling_shielded_data: &ShieldedData<AnchorV>,
-) -> Result<(), TransactionError>
-where
-    AnchorV: AnchorVariant + Clone,
-{
-    if (sapling_shielded_data.spends().count() + sapling_shielded_data.outputs().count() != 0)
-        || i64::from(sapling_shielded_data.value_balance) == 0
-    {
-        Ok(())
-    } else {
-        Err(TransactionError::BadBalance)
-    }
-}
-
 /// Check that a coinbase transaction has no PrevOut inputs, JoinSplits, or spends.
 ///
 /// A coinbase transaction MUST NOT have any transparent inputs, JoinSplit descriptions,

zebra-consensus/src/transaction/tests.rs

@@ -35,8 +35,6 @@ fn v5_fake_transactions() -> Result<(), Report> {
                     ..
                 } => {
                     if let Some(s) = sapling_shielded_data {
-                        check::sapling_balances_match(&s)?;
-
                         for spend in s.spends_per_anchor() {
                             check::spend_cv_rk_not_small_order(&spend)?
                         }