From 9416b5d5cd1d4ce85b2d9250d0a5f71e8b3ad270 Mon Sep 17 00:00:00 2001
From: Janito Vaqueiro Ferreira Filho <janito.vff@gmail.com>
Date: Wed, 2 Jun 2021 22:54:08 -0300
Subject: [PATCH] Update
 `transaction::check::coinbase_tx_no_joinsplit_or_spend` to validate V5
 coinbase transactions with Orchard shielded data (#2236)

* Add a `Transaction::orchard_shielded_data` getter

Allows accessing the Orchard shielded data if it is present in the
transaction, regardless of the transaction version.

* Refactor `orchard_nullifiers` to use new getter

Allows making the method more concise.

* Add `CoinbaseHasEnableSpendsOrchard` error variant

Used when the validation rule is not met.

* Implement `enableSpendsOrchard` in coinbase check

The flag must not be set for the coinbase transaction.

* Refactor `Transaction::orchard_*` getters

Use the fact that `Option<T>` implements `Iterator<T>` to simplify the
code and remove the need for boxing the iterators.

Co-authored-by: teor <teor@riseup.net>
---
 zebra-chain/src/transaction.rs           | 54 ++++++++++--------------
 zebra-consensus/src/error.rs             |  3 ++
 zebra-consensus/src/transaction/check.rs |  8 +++-
 3 files changed, 32 insertions(+), 33 deletions(-)

diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs
index 91bccf7b8..56603c5e9 100644
--- a/zebra-chain/src/transaction.rs
+++ b/zebra-chain/src/transaction.rs
@@ -395,47 +395,39 @@ impl Transaction {
 
     // orchard
 
-    /// Iterate over the [`orchard::Action`]s in this transaction, if there are any.
-    pub fn orchard_actions(&self) -> Box<dyn Iterator<Item = &orchard::Action> + '_> {
+    /// Access the [`orchard::ShieldedData`] in this transaction, if there are any,
+    /// regardless of version.
+    pub fn orchard_shielded_data(&self) -> Option<&orchard::ShieldedData> {
         match self {
-            // Actions
+            // Maybe Orchard shielded data
             Transaction::V5 {
-                orchard_shielded_data: Some(orchard_shielded_data),
+                orchard_shielded_data,
                 ..
-            } => Box::new(orchard_shielded_data.actions()),
+            } => orchard_shielded_data.as_ref(),
 
-            // No Actions
+            // No Orchard shielded data
             Transaction::V1 { .. }
             | Transaction::V2 { .. }
             | Transaction::V3 { .. }
-            | Transaction::V4 { .. }
-            | Transaction::V5 {
-                orchard_shielded_data: None,
-                ..
-            } => Box::new(std::iter::empty()),
+            | Transaction::V4 { .. } => None,
         }
     }
 
-    /// Access the [`orchard::Nullifier`]s in this transaction, regardless of version.
-    pub fn orchard_nullifiers(&self) -> Box<dyn Iterator<Item = &orchard::Nullifier> + '_> {
-        // This function returns a boxed iterator because the different
-        // transaction variants can have different iterator types
-        match self {
-            // Actions
-            Transaction::V5 {
-                orchard_shielded_data: Some(orchard_shielded_data),
-                ..
-            } => Box::new(orchard_shielded_data.nullifiers()),
+    /// Iterate over the [`orchard::Action`]s in this transaction, if there are any,
+    /// regardless of version.
+    pub fn orchard_actions(&self) -> impl Iterator<Item = &orchard::Action> {
+        self.orchard_shielded_data()
+            .into_iter()
+            .map(orchard::ShieldedData::actions)
+            .flatten()
+    }
 
-            // No Actions
-            Transaction::V1 { .. }
-            | Transaction::V2 { .. }
-            | Transaction::V3 { .. }
-            | Transaction::V4 { .. }
-            | Transaction::V5 {
-                orchard_shielded_data: None,
-                ..
-            } => Box::new(std::iter::empty()),
-        }
+    /// Access the [`orchard::Nullifier`]s in this transaction, if there are any,
+    /// regardless of version.
+    pub fn orchard_nullifiers(&self) -> impl Iterator<Item = &orchard::Nullifier> {
+        self.orchard_shielded_data()
+            .into_iter()
+            .map(orchard::ShieldedData::nullifiers)
+            .flatten()
     }
 }
diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs
index 63f2ac998..de1ab7434 100644
--- a/zebra-consensus/src/error.rs
+++ b/zebra-consensus/src/error.rs
@@ -38,6 +38,9 @@ pub enum TransactionError {
     #[error("coinbase transaction MUST NOT have any Output descriptions pre-Heartwood")]
     CoinbaseHasOutputPreHeartwood,
 
+    #[error("coinbase transaction MUST NOT have the EnableSpendsOrchard flag set")]
+    CoinbaseHasEnableSpendsOrchard,
+
     #[error("coinbase transaction failed subsidy validation")]
     Subsidy(#[from] SubsidyError),
 
diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs
index f728c0e64..aa5727b93 100644
--- a/zebra-consensus/src/transaction/check.rs
+++ b/zebra-consensus/src/transaction/check.rs
@@ -3,6 +3,7 @@
 //! Code in this file can freely assume that no pre-V4 transactions are present.
 
 use zebra_chain::{
+    orchard::Flags,
     sapling::{AnchorVariant, Output, PerSpendAnchor, ShieldedData, Spend},
     transaction::Transaction,
 };
@@ -84,8 +85,11 @@ pub fn coinbase_tx_no_prevout_joinsplit_spend(tx: &Transaction) -> Result<(), Tr
             return Err(TransactionError::CoinbaseHasSpend);
         }
 
-        // TODO: Orchard validation (#1980)
-        // In a version 5 coinbase transaction, the enableSpendsOrchard flag MUST be 0.
+        if let Some(orchard_shielded_data) = tx.orchard_shielded_data() {
+            if orchard_shielded_data.flags.contains(Flags::ENABLE_SPENDS) {
+                return Err(TransactionError::CoinbaseHasEnableSpendsOrchard);
+            }
+        }
     }
 
     Ok(())