consensus: fix bug in coinbase joinsplit/spend check

This function caused spurious "WrongVersion" errors, because the match
pattern in the first arm was non-exhaustive, but the fallthrough match
arm was present and assumed it would only be reached if the version was
incorrect.

This commit cleans up the implemenation, splits out the error variants,
and renames the check to be more precise.

To avoid this kind of bug in the future, two guidelines are useful:

1. Avoid fallthrough cases that circumvent non-exhaustive match checks;
2. Avoid nested conditionals, preferring a "straight-line" sequence of
   match arm => result pairs rather than nested matches or matches with
   conditionals inside.

zebra-consensus/src/error.rs

@@ -28,8 +28,14 @@ pub enum TransactionError {
     #[error("coinbase input found in non-coinbase transaction")]
     CoinbaseInputFound,
 
-    #[error("coinbase transaction MUST NOT have any JoinSplit descriptions or Spend descriptions")]
-    CoinbaseHasJoinSplitOrSpend,
+    #[error("coinbase transaction MUST NOT have any JoinSplit descriptions")]
+    CoinbaseHasJoinSplit,
+
+    #[error("coinbase transaction MUST NOT have any Spend descriptions")]
+    CoinbaseHasSpend,
+
+    #[error("coinbase transaction MUST NOT have any Output descriptions pre-Heartwood")]
+    CoinbaseHasOutputPreHeartwood,
 
     #[error("coinbase transaction failed subsidy validation")]
     Subsidy(#[from] SubsidyError),

zebra-consensus/src/transaction.rs

@@ -116,7 +116,7 @@ where
                     #[allow(clippy::if_same_then_else)] // delete when filled in
                     if tx.is_coinbase() {
                         // do something special for coinbase transactions
-                        check::coinbase_tx_does_not_spend_shielded(&tx)?;
+                        check::coinbase_tx_no_joinsplit_or_spend(&tx)?;
                     } else {
                         // otherwise, check no coinbase inputs
                         // feed all of the inputs to the script verifier

zebra-consensus/src/transaction/check.rs

@@ -1,3 +1,7 @@
+//! Transaction checks.
+//!
+//! Code in this file can freely assume that no pre-V4 transactions are present.
+
 use std::convert::TryFrom;
 
 use zebra_chain::{
@@ -86,22 +90,30 @@ pub fn shielded_balances_match(
 
 /// Check that a coinbase tx does not have any JoinSplit or Spend descriptions.
 ///
-/// https://zips.z.cash/protocol/canopy.pdf#consensusfrombitcoin
-pub fn coinbase_tx_does_not_spend_shielded(tx: &Transaction) -> Result<(), TransactionError> {
-    match tx {
-        Transaction::V4 {
-            joinsplit_data: Some(joinsplit_data),
-            shielded_data: Some(shielded_data),
-            ..
-        } => {
-            if !tx.is_coinbase()
-                || (joinsplit_data.joinsplits().count() == 0 && shielded_data.spends().count() == 0)
-            {
-                Ok(())
-            } else {
-                Err(TransactionError::CoinbaseHasJoinSplitOrSpend)
+/// https://zips.z.cash/protocol/canopy.pdf#txnencodingandconsensus
+pub fn coinbase_tx_no_joinsplit_or_spend(tx: &Transaction) -> Result<(), TransactionError> {
+    if tx.is_coinbase() {
+        match tx {
+            // Check if there is any JoinSplitData.
+            Transaction::V4 {
+                joinsplit_data: Some(_),
+                ..
+            } => Err(TransactionError::CoinbaseHasJoinSplit),
+
+            // The ShieldedData contains both Spends and Outputs, and Outputs
+            // are allowed post-Heartwood, so we have to count Spends.
+            Transaction::V4 {
+                shielded_data: Some(shielded_data),
+                ..
+            } if shielded_data.spends().count() > 0 => Err(TransactionError::CoinbaseHasSpend),
+
+            Transaction::V4 { .. } => Ok(()),
+
+            Transaction::V1 { .. } | Transaction::V2 { .. } | Transaction::V3 { .. } => {
+                unreachable!("tx version is checked first")
             }
         }
-        _ => Err(TransactionError::WrongVersion),
+    } else {
+        Ok(())
     }
 }