Validate non coinbase expiration height (#3103)

* validate non coinbase expiration height

* change var name

* move checks to transaction verifier

* Add variants and debug fields to transaction expiry errors

* Fix a failing existing test

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

zebra-consensus/src/block.rs

@@ -187,11 +187,6 @@ where
                 .expect("must have coinbase transaction");
             check::subsidy_is_valid(&block, network)?;
 
-            // Validate `nExpiryHeight` consensus rules
-            // TODO: check non-coinbase transaction expiry against the block height (#2387)
-            //       check the maximum expiry height for non-coinbase transactions (#2387)
-            check::coinbase_expiry_height(&height, coinbase_tx, network)?;
-
             // Now do the slower checks
 
             // Check compatibility with ZIP-212 shielded Sapling and Orchard coinbase output decryption

zebra-consensus/src/block/check.rs

@@ -286,48 +286,3 @@ pub fn merkle_root_validity(
 
     Ok(())
 }
-
-/// Returns `Ok(())` if the expiry height for the coinbase transaction is valid
-/// according to specifications [7.1] and [ZIP-203].
-///
-/// [7.1]: https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus
-/// [ZIP-203]: https://zips.z.cash/zip-0203
-pub fn coinbase_expiry_height(
-    height: &Height,
-    coinbase: &transaction::Transaction,
-    network: Network,
-) -> Result<(), BlockError> {
-    match NetworkUpgrade::Nu5.activation_height(network) {
-        // If Nu5 does not have a height, apply the pre-Nu5 rule.
-        None => validate_expiry_height_max(coinbase.expiry_height()),
-        Some(activation_height) => {
-            // Conesnsus rule: from NU5 activation, the nExpiryHeight field of a
-            // coinbase transaction MUST be set equal to the block height.
-            if *height >= activation_height {
-                match coinbase.expiry_height() {
-                    None => Err(TransactionError::CoinbaseExpiration)?,
-                    Some(expiry) => {
-                        if expiry != *height {
-                            return Err(TransactionError::CoinbaseExpiration)?;
-                        }
-                    }
-                }
-                return Ok(());
-            }
-            // Consensus rule: [Overwinter to Canopy inclusive, pre-NU5] nExpiryHeight
-            // MUST be less than or equal to 499999999.
-            validate_expiry_height_max(coinbase.expiry_height())
-        }
-    }
-}
-
-/// Validate the consensus rule: nExpiryHeight MUST be less than or equal to 499999999.
-fn validate_expiry_height_max(expiry_height: Option<Height>) -> Result<(), BlockError> {
-    if let Some(expiry) = expiry_height {
-        if expiry > Height::MAX_EXPIRY_HEIGHT {
-            return Err(TransactionError::CoinbaseExpiration)?;
-        }
-    }
-
-    Ok(())
-}

zebra-consensus/src/block/tests.rs

@@ -719,16 +719,16 @@ fn legacy_sigops_count_for_historic_blocks() {
 }
 
 #[test]
-fn coinbase_height_validation() -> Result<(), Report> {
+fn transaction_expiration_height_validation() -> Result<(), Report> {
     zebra_test::init();
 
-    coinbase_height_for_network(Network::Mainnet)?;
+    transaction_expiration_height_for_network(Network::Mainnet)?;
-    coinbase_height_for_network(Network::Testnet)?;
+    transaction_expiration_height_for_network(Network::Testnet)?;
 
     Ok(())
 }
 
-fn coinbase_height_for_network(network: Network) -> Result<(), Report> {
+fn transaction_expiration_height_for_network(network: Network) -> Result<(), Report> {
     let block_iter = match network {
         Network::Mainnet => zebra_test::vectors::MAINNET_BLOCKS.iter(),
         Network::Testnet => zebra_test::vectors::TESTNET_BLOCKS.iter(),
@@ -737,13 +737,22 @@ fn coinbase_height_for_network(network: Network) -> Result<(), Report> {
     for (&height, block) in block_iter {
         let block = Block::zcash_deserialize(&block[..]).expect("block should deserialize");
 
-        // Validate
+        for (n, transaction) in block.transactions.iter().enumerate() {
-        let result = check::coinbase_expiry_height(
+            if n == 0 {
+                // coinbase
+                let result = transaction::check::coinbase_expiry_height(
                     &Height(height),
-            block.transactions.get(0).unwrap(),
+                    transaction,
                     network,
                 );
                 assert!(result.is_ok());
+            } else {
+                // non coinbase
+                let result =
+                    transaction::check::non_coinbase_expiry_height(&Height(height), transaction);
+                assert!(result.is_ok());
+            }
+        }
     }
 
     Ok(())

zebra-consensus/src/error.rs

@@ -15,6 +15,9 @@ use crate::{block::MAX_BLOCK_SIGOPS, BoxError};
 #[cfg(any(test, feature = "proptest-impl"))]
 use proptest_derive::Arbitrary;
 
+/// Workaround for format string identifier rules.
+const MAX_EXPIRY_HEIGHT: block::Height = block::Height::MAX_EXPIRY_HEIGHT;
+
 #[derive(Error, Copy, Clone, Debug, PartialEq, Eq)]
 pub enum SubsidyError {
     #[error("no coinbase transaction in block")]
@@ -70,8 +73,36 @@ pub enum TransactionError {
     #[cfg_attr(any(test, feature = "proptest-impl"), proptest(skip))]
     LockedUntilAfterBlockTime(DateTime<Utc>),
 
-    #[error("coinbase expiration height is invalid")]
+    #[error(
-    CoinbaseExpiration,
+        "coinbase expiry {expiry_height:?} must be the same as the block {block_height:?} \
+         after NU5 activation, failing transaction: {transaction_hash:?}"
+    )]
+    CoinbaseExpiryBlockHeight {
+        expiry_height: Option<zebra_chain::block::Height>,
+        block_height: zebra_chain::block::Height,
+        transaction_hash: zebra_chain::transaction::Hash,
+    },
+
+    #[error(
+        "expiry {expiry_height:?} must be less than the maximum {MAX_EXPIRY_HEIGHT:?} \
+         coinbase: {is_coinbase}, block: {block_height:?}, failing transaction: {transaction_hash:?}"
+    )]
+    MaximumExpiryHeight {
+        expiry_height: zebra_chain::block::Height,
+        is_coinbase: bool,
+        block_height: zebra_chain::block::Height,
+        transaction_hash: zebra_chain::transaction::Hash,
+    },
+
+    #[error(
+        "transaction must not be mined at a block {block_height:?} \
+         greater than its expiry {expiry_height:?}, failing transaction {transaction_hash:?}"
+    )]
+    ExpiredTransaction {
+        expiry_height: zebra_chain::block::Height,
+        block_height: zebra_chain::block::Height,
+        transaction_hash: zebra_chain::transaction::Hash,
+    },
 
     #[error("coinbase transaction failed subsidy validation")]
     #[cfg_attr(any(test, feature = "proptest-impl"), proptest(skip))]

zebra-consensus/src/transaction.rs

@@ -307,6 +307,13 @@ where
                 check::coinbase_tx_no_prevout_joinsplit_spend(&tx)?;
             }
 
+            // Validate `nExpiryHeight` consensus rules
+            if tx.has_any_coinbase_inputs() {
+                check::coinbase_expiry_height(&req.height(), &tx, network)?;
+            } else {
+                check::non_coinbase_expiry_height(&req.height(), &tx)?;
+            }
+
             // [Canopy onward]: `vpub_old` MUST be zero.
             // https://zips.z.cash/protocol/protocol.pdf#joinsplitdesc
             check::disabled_add_to_sprout_pool(&tx, req.height(), network)?;

zebra-consensus/src/transaction/check.rs

@@ -297,3 +297,112 @@ pub fn coinbase_outputs_are_decryptable(
 
     Ok(())
 }
+
+/// Returns `Ok(())` if the expiry height for the coinbase transaction is valid
+/// according to specifications [7.1] and [ZIP-203].
+///
+/// [7.1]: https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus
+/// [ZIP-203]: https://zips.z.cash/zip-0203
+pub fn coinbase_expiry_height(
+    block_height: &Height,
+    coinbase: &Transaction,
+    network: Network,
+) -> Result<(), TransactionError> {
+    let expiry_height = coinbase.expiry_height();
+
+    match NetworkUpgrade::Nu5.activation_height(network) {
+        // If Nu5 does not have a height, apply the pre-Nu5 rule.
+        None => validate_expiry_height_max(expiry_height, true, block_height, coinbase),
+        Some(activation_height) => {
+            // Consensus rule: from NU5 activation, the nExpiryHeight field of a
+            // coinbase transaction MUST be set equal to the block height.
+            if *block_height >= activation_height {
+                match expiry_height {
+                    None => Err(TransactionError::CoinbaseExpiryBlockHeight {
+                        expiry_height,
+                        block_height: *block_height,
+                        transaction_hash: coinbase.hash(),
+                    })?,
+                    Some(expiry) => {
+                        if expiry != *block_height {
+                            return Err(TransactionError::CoinbaseExpiryBlockHeight {
+                                expiry_height,
+                                block_height: *block_height,
+                                transaction_hash: coinbase.hash(),
+                            })?;
+                        }
+                    }
+                }
+                return Ok(());
+            }
+            // Consensus rule: [Overwinter to Canopy inclusive, pre-NU5] nExpiryHeight
+            // MUST be less than or equal to 499999999.
+            validate_expiry_height_max(expiry_height, true, block_height, coinbase)
+        }
+    }
+}
+
+/// Returns `Ok(())` if the expiry height for a non coinbase transaction is valid
+/// according to specifications [7.1] and [ZIP-203].
+///
+/// [7.1]: https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus
+/// [ZIP-203]: https://zips.z.cash/zip-0203
+pub fn non_coinbase_expiry_height(
+    block_height: &Height,
+    transaction: &Transaction,
+) -> Result<(), TransactionError> {
+    if transaction.is_overwintered() {
+        let expiry_height = transaction.expiry_height();
+
+        validate_expiry_height_max(expiry_height, false, block_height, transaction)?;
+        validate_expiry_height_mined(expiry_height, block_height, transaction)?;
+    }
+    Ok(())
+}
+
+/// Validate the consensus rule: nExpiryHeight MUST be less than or equal to 499999999.
+///
+/// The remaining arguments are not used for validation,
+/// they are only used to create errors.
+fn validate_expiry_height_max(
+    expiry_height: Option<Height>,
+    is_coinbase: bool,
+    block_height: &Height,
+    transaction: &Transaction,
+) -> Result<(), TransactionError> {
+    if let Some(expiry_height) = expiry_height {
+        if expiry_height > Height::MAX_EXPIRY_HEIGHT {
+            return Err(TransactionError::MaximumExpiryHeight {
+                expiry_height,
+                is_coinbase,
+                block_height: *block_height,
+                transaction_hash: transaction.hash(),
+            })?;
+        }
+    }
+
+    Ok(())
+}
+
+/// Validate the consensus rule: If a transaction is not a coinbase transaction
+/// and its nExpiryHeight field is nonzero, then it MUST NOT be mined at a block
+/// height greater than its nExpiryHeight.
+///
+/// The `transaction` is only used to create errors.
+fn validate_expiry_height_mined(
+    expiry_height: Option<Height>,
+    block_height: &Height,
+    transaction: &Transaction,
+) -> Result<(), TransactionError> {
+    if let Some(expiry_height) = expiry_height {
+        if *block_height > expiry_height {
+            return Err(TransactionError::ExpiredTransaction {
+                expiry_height,
+                block_height: *block_height,
+                transaction_hash: transaction.hash(),
+            })?;
+        }
+    }
+
+    Ok(())
+}

zebra-consensus/src/transaction/tests.rs

@@ -1,4 +1,5 @@
 use std::{
+    cmp::max,
     collections::HashMap,
     convert::{TryFrom, TryInto},
     sync::Arc,
@@ -301,6 +302,10 @@ async fn v5_transaction_is_accepted_after_nu5_activation_testnet() {
 
 async fn v5_transaction_is_accepted_after_nu5_activation_for_network(network: Network) {
     let nu5 = NetworkUpgrade::Nu5;
+    let nu5_activation_height = nu5
+        .activation_height(network)
+        .expect("NU5 activation height is specified");
+
     let blocks = match network {
         Network::Mainnet => zebra_test::vectors::MAINNET_BLOCKS.iter(),
         Network::Testnet => zebra_test::vectors::TESTNET_BLOCKS.iter(),
@@ -317,13 +322,16 @@ async fn v5_transaction_is_accepted_after_nu5_activation_for_network(network: Ne
 
     let expected_hash = transaction.unmined_id();
 
+    let fake_block_height = max(
+        nu5_activation_height,
+        transaction.expiry_height().unwrap_or(nu5_activation_height),
+    );
+
     let result = verifier
         .oneshot(Request::Block {
             transaction: Arc::new(transaction),
             known_utxos: Arc::new(HashMap::new()),
-            height: nu5
+            height: fake_block_height,
-                .activation_height(network)
-                .expect("NU5 activation height is specified"),
             time: chrono::MAX_DATETIME,
         })
         .await;