From 07917421cb1dfea97590a157164955fac4190240 Mon Sep 17 00:00:00 2001
From: Henry de Valence <hdevalence@hdevalence.ca>
Date: Thu, 13 Aug 2020 14:04:43 -0700
Subject: [PATCH] Correct coinbase check (#898)

* chain: add Transaction::is_coinbase()

This matches the check in zcashd/src/primitives/transaction.h:682
(CTransaction::IsCoinBase).

* chain: correct Block::is_coinbase_first

This matches zcashd/src/main.cpp:3968-3974 in CheckBlock.

Previously, the check allowed the first transaction to have multiple coinbase inputs.

* chain: return slices from Transaction::inputs()/outputs()

They're slices internally so we might as well just expose them that way.
---
 zebra-chain/src/block.rs       | 28 +++++++++++++---------------
 zebra-chain/src/transaction.rs | 34 ++++++++++++++++++++++------------
 2 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/zebra-chain/src/block.rs b/zebra-chain/src/block.rs
index be541468..dcef0d15 100644
--- a/zebra-chain/src/block.rs
+++ b/zebra-chain/src/block.rs
@@ -57,7 +57,7 @@ impl Block {
         use crate::transaction::TransparentInput;
         self.transactions
             .get(0)
-            .and_then(|tx| tx.inputs().next())
+            .and_then(|tx| tx.inputs().get(0))
             .and_then(|input| match input {
                 TransparentInput::Coinbase { ref height, .. } => Some(*height),
                 _ => None,
@@ -69,24 +69,22 @@ impl Block {
     ///
     /// "The first (and only the first) transaction in a block is a coinbase
     /// transaction, which collects and spends any miner subsidy and transaction
-    /// fees paid by transactions included in this block."[S 3.10][3.10]
+    /// fees paid by transactions included in this block." [§3.10][3.10]
     ///
     /// [3.10]: https://zips.z.cash/protocol/protocol.pdf#coinbasetransactions
     pub fn is_coinbase_first(&self) -> Result<(), Error> {
-        if self.coinbase_height().is_some() {
-            // No coinbase inputs in additional transactions allowed
-            if self
-                .transactions
-                .iter()
-                .skip(1)
-                .any(|tx| tx.contains_coinbase_input())
-            {
-                Err("coinbase input found in additional transaction")?
-            }
-            Ok(())
-        } else {
-            Err("no coinbase transaction in block")?
+        let first = self
+            .transactions
+            .get(0)
+            .ok_or_else(|| "block has no transactions")?;
+        let mut rest = self.transactions.iter().skip(1);
+        if !first.is_coinbase() {
+            Err("first transaction must be coinbase")?;
         }
+        if rest.any(|tx| tx.contains_coinbase_input()) {
+            Err("coinbase input found in non-coinbase transaction")?;
+        }
+        Ok(())
     }
 
     /// Get the hash for the current block
diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs
index 70415c95..d5e9fadf 100644
--- a/zebra-chain/src/transaction.rs
+++ b/zebra-chain/src/transaction.rs
@@ -91,23 +91,23 @@ pub enum Transaction {
 }
 
 impl Transaction {
-    /// Iterate over the transparent inputs of this transaction, if any.
-    pub fn inputs(&self) -> impl Iterator<Item = &TransparentInput> {
+    /// Access the transparent inputs of this transaction, regardless of version.
+    pub fn inputs(&self) -> &[TransparentInput] {
         match self {
-            Transaction::V1 { ref inputs, .. } => inputs.iter(),
-            Transaction::V2 { ref inputs, .. } => inputs.iter(),
-            Transaction::V3 { ref inputs, .. } => inputs.iter(),
-            Transaction::V4 { ref inputs, .. } => inputs.iter(),
+            Transaction::V1 { ref inputs, .. } => inputs,
+            Transaction::V2 { ref inputs, .. } => inputs,
+            Transaction::V3 { ref inputs, .. } => inputs,
+            Transaction::V4 { ref inputs, .. } => inputs,
         }
     }
 
-    /// Iterate over the transparent outputs of this transaction, if any.
-    pub fn outputs(&self) -> impl Iterator<Item = &TransparentOutput> {
+    /// Access the transparent outputs of this transaction, regardless of version.
+    pub fn outputs(&self) -> &[TransparentOutput] {
         match self {
-            Transaction::V1 { ref outputs, .. } => outputs.iter(),
-            Transaction::V2 { ref outputs, .. } => outputs.iter(),
-            Transaction::V3 { ref outputs, .. } => outputs.iter(),
-            Transaction::V4 { ref outputs, .. } => outputs.iter(),
+            Transaction::V1 { ref outputs, .. } => outputs,
+            Transaction::V2 { ref outputs, .. } => outputs,
+            Transaction::V3 { ref outputs, .. } => outputs,
+            Transaction::V4 { ref outputs, .. } => outputs,
         }
     }
 
@@ -134,6 +134,16 @@ impl Transaction {
     /// Returns `true` if transaction contains any coinbase inputs.
     pub fn contains_coinbase_input(&self) -> bool {
         self.inputs()
+            .iter()
             .any(|input| matches!(input, TransparentInput::Coinbase { .. }))
     }
+
+    /// Returns `true` if this transaction is a coinbase transaction.
+    pub fn is_coinbase(&self) -> bool {
+        self.inputs().len() == 1
+            && matches!(
+                self.inputs().get(0),
+                Some(TransparentInput::Coinbase { .. })
+            )
+    }
 }