refactor: document coinbase rules, refactor to ease understanding (#4056)

* refactor: document coinbase rules, refactor to ease understanding

* Update zebra-consensus/src/block/check.rs

Co-authored-by: teor <teor@riseup.net>

* remove no longer used contains_prevout_input()

* remove unused CoinbaseHasPrevOutInput

* update coinbase description in documentation

Co-authored-by: teor <teor@riseup.net>

zebra-chain/src/transaction.rs

@@ -476,8 +476,9 @@ impl Transaction {
     }
 
     /// Returns `true` if this transaction has valid inputs for a coinbase
-    /// transaction, that is, has a single input and it is a coinbase input.
+    /// transaction, that is, has a single input and it is a coinbase input
-    pub fn has_valid_coinbase_transaction_inputs(&self) -> bool {
+    /// (null prevout).
+    pub fn is_coinbase(&self) -> bool {
         self.inputs().len() == 1
             && matches!(
                 self.inputs().get(0),
@@ -485,20 +486,16 @@ impl Transaction {
             )
     }
 
-    /// Returns `true` if transaction contains any coinbase inputs.
+    /// Returns `true` if this transaction has valid inputs for a non-coinbase
-    pub fn has_any_coinbase_inputs(&self) -> bool {
+    /// transaction, that is, does not have any coinbase input (non-null prevouts).
-        self.inputs()
-            .iter()
-            .any(|input| matches!(input, transparent::Input::Coinbase { .. }))
-    }
-
-    /// Returns `true` if transaction contains any `PrevOut` inputs.
     ///
-    /// `PrevOut` inputs are also known as `transparent` inputs in the spec.
+    /// Note that it's possible for a transaction return false in both
-    pub fn contains_prevout_input(&self) -> bool {
+    /// [`Transaction::is_coinbase`] and [`Transaction::is_valid_non_coinbase`],
+    /// though those transactions will be rejected.
+    pub fn is_valid_non_coinbase(&self) -> bool {
         self.inputs()
             .iter()
-            .any(|input| matches!(input, transparent::Input::PrevOut { .. }))
+            .all(|input| matches!(input, transparent::Input::PrevOut { .. }))
     }
 
     // sprout

zebra-chain/src/transaction/arbitrary.rs

@@ -430,7 +430,7 @@ impl Transaction {
         &mut self,
         outputs: &HashMap<transparent::OutPoint, transparent::Output>,
     ) -> Result<Amount<NonNegative>, ValueBalanceError> {
-        if self.has_valid_coinbase_transaction_inputs() {
+        if self.is_coinbase() {
             // TODO: if needed, fixup coinbase:
             // - miner subsidy
             // - founders reward or funding streams (hopefully not?)

zebra-consensus/src/block.rs

@@ -180,17 +180,13 @@ where
             let now = Utc::now();
             check::time_is_valid_at(&block.header, now, &height, &hash)
                 .map_err(VerifyBlockError::Time)?;
-            check::coinbase_is_first(&block)?;
+            let coinbase_tx = check::coinbase_is_first(&block)?;
-            let coinbase_tx = block
-                .transactions
-                .get(0)
-                .expect("must have coinbase transaction");
             check::subsidy_is_valid(&block, network)?;
 
             // Now do the slower checks
 
             // Check compatibility with ZIP-212 shielded Sapling and Orchard coinbase output decryption
-            tx::check::coinbase_outputs_are_decryptable(coinbase_tx, network, height)?;
+            tx::check::coinbase_outputs_are_decryptable(&coinbase_tx, network, height)?;
 
             // Send transactions to the transaction verifier to be checked
             let mut async_checks = FuturesUnordered::new();

zebra-consensus/src/block/check.rs

@@ -1,7 +1,7 @@
 //! Consensus check functions
 
 use chrono::{DateTime, Utc};
-use std::collections::HashSet;
+use std::{collections::HashSet, sync::Arc};
 
 use zebra_chain::{
     amount::{Amount, Error as AmountError, NonNegative},
@@ -15,28 +15,46 @@ use crate::{error::*, parameters::SLOW_START_INTERVAL};
 
 use super::subsidy;
 
-/// Returns `Ok(())` if there is exactly one coinbase transaction in `Block`,
+/// Checks if there is exactly one coinbase transaction in `Block`,
-/// and that coinbase transaction is the first transaction in the block.
+/// and if that coinbase transaction is the first transaction in the block.
+/// Returns the coinbase transaction is successful.
 ///
-/// "The first (and only the first) transaction in a block is a coinbase
+/// > A transaction that has a single transparent input with a null prevout field,
-/// transaction, which collects and spends any miner subsidy and transaction
+/// > is called a coinbase transaction. Every block has a single coinbase
-/// fees paid by transactions included in this block." [§3.10][3.10]
+/// > transaction as the first transaction in the block.
 ///
-/// [3.10]: https://zips.z.cash/protocol/protocol.pdf#coinbasetransactions
+/// <https://zips.z.cash/protocol/protocol.pdf#coinbasetransactions>
-pub fn coinbase_is_first(block: &Block) -> Result<(), BlockError> {
+pub fn coinbase_is_first(block: &Block) -> Result<Arc<transaction::Transaction>, BlockError> {
+    // # Consensus
+    //
+    // > A block MUST have at least one transaction
+    //
+    // <https://zips.z.cash/protocol/protocol.pdf#blockheader>
     let first = block
         .transactions
         .get(0)
         .ok_or(BlockError::NoTransactions)?;
+    // > The first transaction in a block MUST be a coinbase transaction,
+    // > and subsequent transactions MUST NOT be coinbase transactions.
+    //
+    // <https://zips.z.cash/protocol/protocol.pdf#blockheader>
+    //
+    // > A transaction that has a single transparent input with a null prevout
+    // > field, is called a coinbase transaction.
+    //
+    // <https://zips.z.cash/protocol/protocol.pdf#coinbasetransactions>
     let mut rest = block.transactions.iter().skip(1);
-    if !first.has_valid_coinbase_transaction_inputs() {
+    if !first.is_coinbase() {
         return Err(TransactionError::CoinbasePosition)?;
     }
-    if rest.any(|tx| tx.has_any_coinbase_inputs()) {
+    // > A transparent input in a non-coinbase transaction MUST NOT have a null prevout
+    //
+    // <https://zips.z.cash/protocol/protocol.pdf#txnconsensus>
+    if !rest.all(|tx| tx.is_valid_non_coinbase()) {
         return Err(TransactionError::CoinbaseAfterFirst)?;
     }
 
-    Ok(())
+    Ok(first.clone())
 }
 
 /// Returns `Ok(())` if `hash` passes:

zebra-consensus/src/error.rs

@@ -42,9 +42,6 @@ pub enum TransactionError {
     #[error("coinbase input found in non-coinbase transaction")]
     CoinbaseAfterFirst,
 
-    #[error("coinbase transaction MUST NOT have any transparent (PrevOut) inputs")]
-    CoinbaseHasPrevOutInput,
-
     #[error("coinbase transaction MUST NOT have any JoinSplit descriptions")]
     CoinbaseHasJoinSplit,
 
@@ -63,6 +60,9 @@ pub enum TransactionError {
     #[error("coinbase inputs MUST NOT exist in mempool")]
     CoinbaseInMempool,
 
+    #[error("non-coinbase transactions MUST NOT have coinbase inputs")]
+    NonCoinbaseHasCoinbaseInput,
+
     #[error("transaction is locked until after block height {}", _0.0)]
     LockedUntilAfterBlockHeight(block::Height),
 

zebra-consensus/src/transaction.rs

@@ -313,15 +313,17 @@ where
             check::has_inputs_and_outputs(&tx)?;
             check::has_enough_orchard_flags(&tx)?;
 
-            if req.is_mempool() && tx.has_any_coinbase_inputs() {
+            if req.is_mempool() && tx.is_coinbase() {
                 return Err(TransactionError::CoinbaseInMempool);
             }
-            if tx.has_valid_coinbase_transaction_inputs() {
+            if tx.is_coinbase() {
                 check::coinbase_tx_no_prevout_joinsplit_spend(&tx)?;
+            } else if !tx.is_valid_non_coinbase() {
+                return Err(TransactionError::NonCoinbaseHasCoinbaseInput);
             }
 
             // Validate `nExpiryHeight` consensus rules
-            if tx.has_any_coinbase_inputs() {
+            if tx.is_coinbase() {
                 check::coinbase_expiry_height(&req.height(), &tx, network)?;
             } else {
                 check::non_coinbase_expiry_height(&req.height(), &tx)?;
@@ -396,7 +398,7 @@ where
 
             // Calculate the fee only for non-coinbase transactions.
             let mut miner_fee = None;
-            if !tx.has_valid_coinbase_transaction_inputs() {
+            if !tx.is_coinbase() {
                 // TODO: deduplicate this code with remaining_transaction_value (#TODO: open ticket)
                 miner_fee = match value_balance {
                     Ok(vb) => match vb.remaining_transaction_value() {
@@ -678,7 +680,7 @@ where
     ) -> Result<AsyncChecks, TransactionError> {
         let transaction = request.transaction();
 
-        if transaction.has_valid_coinbase_transaction_inputs() {
+        if transaction.is_coinbase() {
             // The script verifier only verifies PrevOut inputs and their corresponding UTXOs.
             // Coinbase transactions don't have any PrevOut inputs.
             Ok(AsyncChecks::new())

zebra-consensus/src/transaction/check.rs

@@ -111,8 +111,9 @@ pub fn has_enough_orchard_flags(tx: &Transaction) -> Result<(), TransactionError
 ///
 /// # Consensus
 ///
-/// > A coinbase transaction MUST NOT have any transparent inputs with non-null prevout fields,
+/// > A coinbase transaction MUST NOT have any JoinSplit descriptions.
-/// > JoinSplit descriptions, or Spend descriptions.
+///
+/// > A coinbase transaction MUST NOT have any Spend descriptions.
 ///
 /// > [NU5 onward] In a version 5 coinbase transaction, the enableSpendsOrchard flag MUST be 0.
 ///
@@ -124,10 +125,8 @@ pub fn has_enough_orchard_flags(tx: &Transaction) -> Result<(), TransactionError
 ///
 /// <https://zips.z.cash/protocol/protocol.pdf#txnconsensus>
 pub fn coinbase_tx_no_prevout_joinsplit_spend(tx: &Transaction) -> Result<(), TransactionError> {
-    if tx.has_valid_coinbase_transaction_inputs() {
+    if tx.is_coinbase() {
-        if tx.contains_prevout_input() {
+        if tx.joinsplit_count() > 0 {
-            return Err(TransactionError::CoinbaseHasPrevOutInput);
-        } else if tx.joinsplit_count() > 0 {
             return Err(TransactionError::CoinbaseHasJoinSplit);
         } else if tx.sapling_spends_per_anchor().count() > 0 {
             return Err(TransactionError::CoinbaseHasSpend);

zebra-consensus/src/transaction/tests.rs

@@ -210,7 +210,7 @@ fn v5_coinbase_transaction_without_enable_spends_flag_passes_validation() {
         zebra_test::vectors::MAINNET_BLOCKS.iter(),
     )
     .rev()
-    .find(|transaction| transaction.has_valid_coinbase_transaction_inputs())
+    .find(|transaction| transaction.is_coinbase())
     .expect("At least one fake V5 coinbase transaction in the test vectors");
 
     insert_fake_orchard_shielded_data(&mut transaction);
@@ -225,7 +225,7 @@ fn v5_coinbase_transaction_with_enable_spends_flag_fails_validation() {
         zebra_test::vectors::MAINNET_BLOCKS.iter(),
     )
     .rev()
-    .find(|transaction| transaction.has_valid_coinbase_transaction_inputs())
+    .find(|transaction| transaction.is_coinbase())
     .expect("At least one fake V5 coinbase transaction in the test vectors");
 
     let shielded_data = insert_fake_orchard_shielded_data(&mut transaction);
@@ -1386,8 +1386,7 @@ fn v4_with_signed_sprout_transfer_is_accepted() {
         let (height, transaction) = test_transactions(network)
             .rev()
             .filter(|(_, transaction)| {
-                !transaction.has_valid_coinbase_transaction_inputs()
+                !transaction.is_coinbase() && transaction.inputs().is_empty()
-                    && transaction.inputs().is_empty()
             })
             .find(|(_, transaction)| transaction.sprout_groth16_joinsplits().next().is_some())
             .expect("No transaction found with Groth16 JoinSplits");
@@ -1456,9 +1455,7 @@ async fn v4_with_joinsplit_is_rejected_for_modification(
 
     let (height, mut transaction) = test_transactions(network)
         .rev()
-        .filter(|(_, transaction)| {
+        .filter(|(_, transaction)| !transaction.is_coinbase() && transaction.inputs().is_empty())
-            !transaction.has_valid_coinbase_transaction_inputs() && transaction.inputs().is_empty()
-        })
         .find(|(_, transaction)| transaction.sprout_groth16_joinsplits().next().is_some())
         .expect("No transaction found with Groth16 JoinSplits");
 
@@ -1496,8 +1493,7 @@ fn v4_with_sapling_spends() {
         let (height, transaction) = test_transactions(network)
             .rev()
             .filter(|(_, transaction)| {
-                !transaction.has_valid_coinbase_transaction_inputs()
+                !transaction.is_coinbase() && transaction.inputs().is_empty()
-                    && transaction.inputs().is_empty()
             })
             .find(|(_, transaction)| transaction.sapling_spends_per_anchor().next().is_some())
             .expect("No transaction found with Sapling spends");
@@ -1537,8 +1533,7 @@ fn v4_with_duplicate_sapling_spends() {
         let (height, mut transaction) = test_transactions(network)
             .rev()
             .filter(|(_, transaction)| {
-                !transaction.has_valid_coinbase_transaction_inputs()
+                !transaction.is_coinbase() && transaction.inputs().is_empty()
-                    && transaction.inputs().is_empty()
             })
             .find(|(_, transaction)| transaction.sapling_spends_per_anchor().next().is_some())
             .expect("No transaction found with Sapling spends");
@@ -1583,8 +1578,7 @@ fn v4_with_sapling_outputs_and_no_spends() {
         let (height, transaction) = test_transactions(network)
             .rev()
             .filter(|(_, transaction)| {
-                !transaction.has_valid_coinbase_transaction_inputs()
+                !transaction.is_coinbase() && transaction.inputs().is_empty()
-                    && transaction.inputs().is_empty()
             })
             .find(|(_, transaction)| {
                 transaction.sapling_spends_per_anchor().next().is_none()
@@ -1629,10 +1623,7 @@ fn v5_with_sapling_spends() {
         let transaction =
             fake_v5_transactions_for_network(network, zebra_test::vectors::MAINNET_BLOCKS.iter())
                 .rev()
-                .filter(|transaction| {
+                .filter(|transaction| !transaction.is_coinbase() && transaction.inputs().is_empty())
-                    !transaction.has_valid_coinbase_transaction_inputs()
-                        && transaction.inputs().is_empty()
-                })
                 .find(|transaction| transaction.sapling_spends_per_anchor().next().is_some())
                 .expect("No transaction found with Sapling spends");
 
@@ -1674,10 +1665,7 @@ fn v5_with_duplicate_sapling_spends() {
         let mut transaction =
             fake_v5_transactions_for_network(network, zebra_test::vectors::MAINNET_BLOCKS.iter())
                 .rev()
-                .filter(|transaction| {
+                .filter(|transaction| !transaction.is_coinbase() && transaction.inputs().is_empty())
-                    !transaction.has_valid_coinbase_transaction_inputs()
-                        && transaction.inputs().is_empty()
-                })
                 .find(|transaction| transaction.sapling_spends_per_anchor().next().is_some())
                 .expect("No transaction found with Sapling spends");
 

zebra-state/src/service/check/utxo.rs

@@ -111,6 +111,13 @@ pub fn transparent_spend(
 /// an attempt to spend the same satoshis twice."
 ///
 /// https://developer.bitcoin.org/devguide/block_chain.html#introduction
+///
+/// # Consensus
+///
+/// > Every non-null prevout MUST point to a unique UTXO in either a preceding block,
+/// > or a previous transaction in the same block.
+///
+/// <https://zips.z.cash/protocol/protocol.pdf#txnconsensus>
 fn transparent_spend_chain_order(
     spend: transparent::OutPoint,
     spend_tx_index_in_block: usize,
@@ -229,7 +236,7 @@ pub fn remaining_transaction_value(
 ) -> Result<(), ValidateContextError> {
     for (tx_index_in_block, transaction) in prepared.block.transactions.iter().enumerate() {
         // TODO: check coinbase transaction remaining value (#338, #1162)
-        if transaction.has_valid_coinbase_transaction_inputs() {
+        if transaction.is_coinbase() {
             continue;
         }
 

zebra-state/src/service/tests.rs

@@ -105,7 +105,7 @@ async fn test_populated_state_responds_correctly(
             for transaction in &block.transactions {
                 let transaction_hash = transaction.hash();
 
-                let from_coinbase = transaction.has_valid_coinbase_transaction_inputs();
+                let from_coinbase = transaction.is_coinbase();
                 for (index, output) in transaction.outputs().iter().cloned().enumerate() {
                     let outpoint = transparent::OutPoint {
                         hash: transaction_hash,

zebrad/src/components/inbound/tests/fake_peer_set.rs

@@ -217,7 +217,7 @@ async fn mempool_advertise_transaction_ids() -> Result<(), crate::BoxError> {
     let test_transaction = block
         .transactions
         .into_iter()
-        .find(|tx| !tx.has_any_coinbase_inputs())
+        .find(|tx| !tx.is_coinbase())
         .expect("at least one non-coinbase transaction");
     let test_transaction_id = test_transaction.unmined_id();
     let txs = HashSet::from_iter([test_transaction_id]);