From 96ee32e5d22920a02b54cb37949ddc66bd4820fd Mon Sep 17 00:00:00 2001
From: Henry de Valence <hdevalence@hdevalence.ca>
Date: Thu, 19 Nov 2020 19:06:10 -0800
Subject: [PATCH] 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             | 10 ++++--
 zebra-consensus/src/transaction.rs       |  2 +-
 zebra-consensus/src/transaction/check.rs | 42 +++++++++++++++---------
 3 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs
index 70c002c32..2f5458d58 100644
--- a/zebra-consensus/src/error.rs
+++ b/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),
diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs
index 3f0986b67..b3fe435bb 100644
--- a/zebra-consensus/src/transaction.rs
+++ b/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
diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs
index aa3b13bc1..9761d4454 100644
--- a/zebra-consensus/src/transaction/check.rs
+++ b/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(())
     }
 }