consensus: fix bug in tx input/output presence check

Making this check's match statement exhaustive revealed a bug similar to
the previous commit.  The logic in the spec is written in terms of
numbers, but our data is internally represented in terms of enums
(ADTs).  This kind of cross-representation rule translation is a bug
surface, which we can avoid by converting to counts and summing up.  (We
should use one style at a time).

zebra-consensus/src/error.rs

@@ -43,8 +43,11 @@ pub enum TransactionError {
     #[error("transaction version number MUST be >= 4")]
     WrongVersion,
 
-    #[error("at least one of tx_in_count, nShieldedSpend, and nJoinSplit MUST be nonzero")]
+    #[error("must have at least one input: transparent, shielded spend, or joinsplit")]
-    NoTransfer,
+    NoInputs,
+
+    #[error("must have at least one output: transparent, shielded output, or joinsplit")]
+    NoOutputs,
 
     #[error("if there are no Spends or Outputs, the value balance MUST be 0.")]
     BadBalance,

zebra-consensus/src/transaction.rs

@@ -130,7 +130,7 @@ where
                         }
                     }
 
-                    check::some_money_is_spent(&tx)?;
+                    check::has_inputs_and_outputs(&tx)?;
                     check::any_coinbase_inputs_no_transparent_outputs(&tx)?;
 
                     let sighash = tx.sighash(

zebra-consensus/src/transaction/check.rs

@@ -25,28 +25,52 @@ pub fn validate_joinsplit_sig(
         .map_err(TransactionError::Ed25519)
 }
 
-/// Check that at least one of tx_in_count, nShieldedSpend, and nJoinSplit MUST
+/// Checks that the transaction has inputs and outputs.
-/// be non-zero.
+///
+/// More specifically:
+///
+/// * at least one of tx_in_count, nShieldedSpend, and nJoinSplit MUST be non-zero.
+/// * at least one of tx_out_count, nShieldedOutput, and nJoinSplit MUST be non-zero.
 ///
 /// https://zips.z.cash/protocol/canopy.pdf#txnencodingandconsensus
-pub fn some_money_is_spent(tx: &Transaction) -> Result<(), TransactionError> {
+pub fn has_inputs_and_outputs(tx: &Transaction) -> Result<(), TransactionError> {
+    // The consensus rule is written in terms of numbers, but our transactions
+    // hold enum'd data. Mixing pattern matching and numerical checks is risky,
+    // so convert everything to counts and sum up.
     match tx {
         Transaction::V4 {
             inputs,
-            joinsplit_data: Some(joinsplit_data),
+            outputs,
-            shielded_data: Some(shielded_data),
+            joinsplit_data,
+            shielded_data,
             ..
         } => {
-            if !inputs.is_empty()
+            let tx_in_count = inputs.len();
-                || joinsplit_data.joinsplits().count() > 0
+            let tx_out_count = outputs.len();
-                || shielded_data.spends().count() > 0
+            let n_joinsplit = joinsplit_data
-            {
+                .as_ref()
-                Ok(())
+                .map(|d| d.joinsplits().count())
+                .unwrap_or(0);
+            let n_shielded_spend = shielded_data
+                .as_ref()
+                .map(|d| d.spends().count())
+                .unwrap_or(0);
+            let n_shielded_output = shielded_data
+                .as_ref()
+                .map(|d| d.outputs().count())
+                .unwrap_or(0);
+
+            if tx_in_count + n_shielded_spend + n_joinsplit == 0 {
+                Err(TransactionError::NoInputs)
+            } else if tx_out_count + n_shielded_output + n_joinsplit == 0 {
+                Err(TransactionError::NoOutputs)
             } else {
-                Err(TransactionError::NoTransfer)
+                Ok(())
             }
         }
-        _ => Err(TransactionError::WrongVersion),
+        Transaction::V1 { .. } | Transaction::V2 { .. } | Transaction::V3 { .. } => {
+            unreachable!("tx version is checked first")
+        }
     }
 }