fix(rpc): Omit transactions with transparent coinbase spends that are immature at the next block height from block templates (#6510)

* Adds `maturity_height` to VerifiedUnminedTx

Filters out transactions that are invalid at next_block_height in getblocktemplate method

* Adds unit testing

* rustfmt

* rejects txs with immature coinbase spends from mempool

* Condenses fns for transparent coinbase spend check

* Updates calls to VerifiedUnminedTx::new()

* Update zebra-chain/src/transparent/utxo.rs

* Applies suggestions from code review

zebra-chain/src/block/arbitrary.rs

@@ -373,7 +373,7 @@ impl Arbitrary for Block {
 pub fn allow_all_transparent_coinbase_spends(
     _: transparent::OutPoint,
     _: transparent::CoinbaseSpendRestriction,
-    _: transparent::OrderedUtxo,
+    _: &transparent::Utxo,
 ) -> Result<(), ()> {
     Ok(())
 }
@@ -390,7 +390,7 @@ impl Block {
     /// `generate_valid_commitments` specifies if the generated blocks
     /// should have valid commitments. This makes it much slower so it's better
     /// to enable only when needed.
-    pub fn partial_chain_strategy<F, T, E>(
+    pub fn partial_chain_strategy<F, E>(
         mut current: LedgerState,
         count: usize,
         check_transparent_coinbase_spend: F,
@@ -400,8 +400,8 @@ impl Block {
         F: Fn(
                 transparent::OutPoint,
                 transparent::CoinbaseSpendRestriction,
-                transparent::OrderedUtxo,
+                &transparent::Utxo,
-            ) -> Result<T, E>
+            ) -> Result<(), E>
             + Copy
             + 'static,
     {
@@ -558,7 +558,7 @@ impl Block {
 /// Spends [`transparent::OutPoint`]s from `utxos`, and adds newly created outputs.
 ///
 /// If the transaction can't be fixed, returns `None`.
-pub fn fix_generated_transaction<F, T, E>(
+pub fn fix_generated_transaction<F, E>(
     mut transaction: Transaction,
     tx_index_in_block: usize,
     height: Height,
@@ -570,8 +570,8 @@ where
     F: Fn(
             transparent::OutPoint,
             transparent::CoinbaseSpendRestriction,
-            transparent::OrderedUtxo,
+            &transparent::Utxo,
-        ) -> Result<T, E>
+        ) -> Result<(), E>
         + Copy
         + 'static,
 {
@@ -640,7 +640,7 @@ where
 /// Modifies `transaction` and updates `spend_restriction` if needed.
 ///
 /// If there is no valid output, or many search attempts have failed, returns `None`.
-pub fn find_valid_utxo_for_spend<F, T, E>(
+pub fn find_valid_utxo_for_spend<F, E>(
     transaction: &mut Transaction,
     spend_restriction: &mut CoinbaseSpendRestriction,
     spend_height: Height,
@@ -651,8 +651,8 @@ where
     F: Fn(
             transparent::OutPoint,
             transparent::CoinbaseSpendRestriction,
-            transparent::OrderedUtxo,
+            &transparent::Utxo,
-        ) -> Result<T, E>
+        ) -> Result<(), E>
         + Copy
         + 'static,
 {
@@ -674,7 +674,7 @@ where
         if check_transparent_coinbase_spend(
             *candidate_outpoint,
             *spend_restriction,
-            candidate_utxo.clone(),
+            candidate_utxo.as_ref(),
         )
         .is_ok()
         {
@@ -683,7 +683,7 @@ where
             && check_transparent_coinbase_spend(
                 *candidate_outpoint,
                 delete_transparent_outputs,
-                candidate_utxo.clone(),
+                candidate_utxo.as_ref(),
             )
             .is_ok()
         {

zebra-chain/src/transparent/utxo.rs

@@ -54,6 +54,12 @@ pub struct OrderedUtxo {
     pub tx_index_in_block: usize,
 }
 
+impl AsRef<Utxo> for OrderedUtxo {
+    fn as_ref(&self) -> &Utxo {
+        &self.utxo
+    }
+}
+
 impl Utxo {
     /// Create a new UTXO from its fields.
     pub fn new(output: transparent::Output, height: block::Height, from_coinbase: bool) -> Utxo {

zebra-consensus/src/error.rs

@@ -8,7 +8,10 @@
 use chrono::{DateTime, Utc};
 use thiserror::Error;
 
-use zebra_chain::{amount, block, orchard, sapling, sprout, transparent};
+use zebra_chain::{
+    amount, block, orchard, sapling, sprout,
+    transparent::{self, MIN_TRANSPARENT_COINBASE_MATURITY},
+};
 use zebra_state::ValidateContextError;
 
 use crate::{block::MAX_BLOCK_SIGOPS, BoxError};
@@ -187,17 +190,42 @@ pub enum TransactionError {
     #[error("could not validate nullifiers and anchors on best chain")]
     #[cfg_attr(any(test, feature = "proptest-impl"), proptest(skip))]
     // This error variant is at least 128 bytes
-    ValidateNullifiersAndAnchorsError(Box<ValidateContextError>),
+    ValidateContextError(Box<ValidateContextError>),
 
     #[error("could not validate mempool transaction lock time on best chain")]
     #[cfg_attr(any(test, feature = "proptest-impl"), proptest(skip))]
     // TODO: turn this into a typed error
     ValidateMempoolLockTimeError(String),
+
+    #[error(
+        "immature transparent coinbase spend: \
+        attempt to spend {outpoint:?} at {spend_height:?}, \
+        but spends are invalid before {min_spend_height:?}, \
+        which is {MIN_TRANSPARENT_COINBASE_MATURITY:?} blocks \
+        after it was created at {created_height:?}"
+    )]
+    #[non_exhaustive]
+    ImmatureTransparentCoinbaseSpend {
+        outpoint: transparent::OutPoint,
+        spend_height: block::Height,
+        min_spend_height: block::Height,
+        created_height: block::Height,
+    },
+
+    #[error(
+        "unshielded transparent coinbase spend: {outpoint:?} \
+         must be spent in a transaction which only has shielded outputs"
+    )]
+    #[non_exhaustive]
+    UnshieldedTransparentCoinbaseSpend {
+        outpoint: transparent::OutPoint,
+        min_spend_height: block::Height,
+    },
 }
 
 impl From<ValidateContextError> for TransactionError {
     fn from(err: ValidateContextError) -> Self {
-        TransactionError::ValidateNullifiersAndAnchorsError(Box::new(err))
+        TransactionError::ValidateContextError(Box::new(err))
     }
 }
 

zebra-consensus/src/transaction.rs

@@ -224,10 +224,7 @@ impl Request {
 
     /// Returns true if the request is a mempool request.
     pub fn is_mempool(&self) -> bool {
-        match self {
+        matches!(self, Request::Mempool { .. })
-            Request::Block { .. } => false,
-            Request::Mempool { .. } => true,
-        }
     }
 }
 
@@ -377,6 +374,12 @@ where
                 Self::spent_utxos(tx.clone(), req.known_utxos(), req.is_mempool(), state.clone());
             let (spent_utxos, spent_outputs) = load_spent_utxos_fut.await?;
 
+            // WONTFIX: Return an error for Request::Block as well to replace this check in
+            //       the state once #2336 has been implemented?
+            if req.is_mempool() {
+                Self::check_maturity_height(&req, &spent_utxos)?;
+            }
+
             let cached_ffi_transaction =
                 Arc::new(CachedFfiTransaction::new(tx.clone(), spent_outputs));
 
@@ -420,7 +423,10 @@ where
                         unmined_tx,
                     ))
                     .map(|res| {
-                        assert!(res? == zs::Response::ValidBestChainTipNullifiersAndAnchors, "unexpected response to CheckBestChainTipNullifiersAndAnchors request");
+                        assert!(
+                            res? == zs::Response::ValidBestChainTipNullifiersAndAnchors,
+                            "unexpected response to CheckBestChainTipNullifiersAndAnchors request"
+                        );
                         Ok(())
                     }
                 );
@@ -567,6 +573,28 @@ where
         Ok((spent_utxos, spent_outputs))
     }
 
+    /// Accepts `request`, a transaction verifier [`&Request`](Request),
+    /// and `spent_utxos`, a HashMap of UTXOs in the chain that are spent by this transaction.
+    ///
+    /// Gets the `transaction`, `height`, and `known_utxos` for the request and checks calls
+    /// [`check::tx_transparent_coinbase_spends_maturity`] to verify that every transparent
+    /// coinbase output spent by the transaction will have matured by `height`.
+    ///
+    /// Returns `Ok(())` if every transparent coinbase output spent by the transaction is
+    /// mature and valid for the request height, or a [`TransactionError`] if the transaction
+    /// spends transparent coinbase outputs that are immature and invalid for the request height.
+    pub fn check_maturity_height(
+        request: &Request,
+        spent_utxos: &HashMap<transparent::OutPoint, transparent::Utxo>,
+    ) -> Result<(), TransactionError> {
+        check::tx_transparent_coinbase_spends_maturity(
+            request.transaction(),
+            request.height(),
+            request.known_utxos(),
+            spent_utxos,
+        )
+    }
+
     /// Verify a V4 transaction.
     ///
     /// Returns a set of asynchronous checks that must all succeed for the transaction to be

zebra-consensus/src/transaction/check.rs

@@ -2,7 +2,12 @@
 //!
 //! Code in this file can freely assume that no pre-V4 transactions are present.
 
-use std::{borrow::Cow, collections::HashSet, convert::TryFrom, hash::Hash};
+use std::{
+    borrow::Cow,
+    collections::{HashMap, HashSet},
+    hash::Hash,
+    sync::Arc,
+};
 
 use chrono::{DateTime, Utc};
 
@@ -13,6 +18,7 @@ use zebra_chain::{
     parameters::{Network, NetworkUpgrade},
     primitives::zcash_note_encryption,
     transaction::{LockTime, Transaction},
+    transparent,
 };
 
 use crate::error::TransactionError;
@@ -462,3 +468,29 @@ fn validate_expiry_height_mined(
 
     Ok(())
 }
+
+/// Accepts a transaction, block height, block UTXOs, and
+/// the transaction's spent UTXOs from the chain.
+///
+/// Returns `Ok(())` if spent transparent coinbase outputs are
+/// valid for the block height, or a [`Err(TransactionError)`](TransactionError)
+pub fn tx_transparent_coinbase_spends_maturity(
+    tx: Arc<Transaction>,
+    height: Height,
+    block_new_outputs: Arc<HashMap<transparent::OutPoint, transparent::OrderedUtxo>>,
+    spent_utxos: &HashMap<transparent::OutPoint, transparent::Utxo>,
+) -> Result<(), TransactionError> {
+    for spend in tx.spent_outpoints() {
+        let utxo = block_new_outputs
+            .get(&spend)
+            .map(|ordered_utxo| ordered_utxo.utxo.clone())
+            .or_else(|| spent_utxos.get(&spend).cloned())
+            .expect("load_spent_utxos_fut.await should return an error if a utxo is missing");
+
+        let spend_restriction = tx.coinbase_spend_restriction(height);
+
+        zebra_state::check::transparent_coinbase_spend(spend, spend_restriction, &utxo)?;
+    }
+
+    Ok(())
+}

zebra-consensus/src/transaction/tests.rs

@@ -564,6 +564,104 @@ async fn mempool_request_with_past_lock_time_is_accepted() {
     );
 }
 
+/// Tests that calls to the transaction verifier with a mempool request that spends
+/// immature coinbase outputs will return an error.
+#[tokio::test]
+async fn mempool_request_with_immature_spent_is_rejected() {
+    let _init_guard = zebra_test::init();
+
+    let mut state: MockService<_, _, _, _> = MockService::build().for_prop_tests();
+    let verifier = Verifier::new(Network::Mainnet, state.clone());
+
+    let height = NetworkUpgrade::Canopy
+        .activation_height(Network::Mainnet)
+        .expect("Canopy activation height is specified");
+    let fund_height = (height - 1).expect("fake source fund block height is too small");
+    let (input, output, known_utxos) = mock_transparent_transfer(fund_height, true, 0);
+
+    // Create a non-coinbase V4 tx with the last valid expiry height.
+    let tx = Transaction::V4 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::min_lock_time_timestamp(),
+        expiry_height: height,
+        joinsplit_data: None,
+        sapling_shielded_data: None,
+    };
+
+    let input_outpoint = match tx.inputs()[0] {
+        transparent::Input::PrevOut { outpoint, .. } => outpoint,
+        transparent::Input::Coinbase { .. } => panic!("requires a non-coinbase transaction"),
+    };
+
+    let spend_restriction = tx.coinbase_spend_restriction(height);
+
+    let coinbase_spend_height = Height(5);
+
+    let utxo = known_utxos
+        .get(&input_outpoint)
+        .map(|utxo| {
+            let mut utxo = utxo.utxo.clone();
+            utxo.height = coinbase_spend_height;
+            utxo.from_coinbase = true;
+            utxo
+        })
+        .expect("known_utxos should contain the outpoint");
+
+    let expected_error =
+        zebra_state::check::transparent_coinbase_spend(input_outpoint, spend_restriction, &utxo)
+            .map_err(Box::new)
+            .map_err(TransactionError::ValidateContextError)
+            .expect_err("check should fail");
+
+    tokio::spawn(async move {
+        state
+            .expect_request(zebra_state::Request::BestChainNextMedianTimePast)
+            .await
+            .expect("verifier should call mock state service with correct request")
+            .respond(zebra_state::Response::BestChainNextMedianTimePast(
+                DateTime32::MAX,
+            ));
+
+        state
+            .expect_request(zebra_state::Request::UnspentBestChainUtxo(input_outpoint))
+            .await
+            .expect("verifier should call mock state service with correct request")
+            .respond(zebra_state::Response::UnspentBestChainUtxo(
+                known_utxos.get(&input_outpoint).map(|utxo| {
+                    let mut utxo = utxo.utxo.clone();
+                    utxo.height = coinbase_spend_height;
+                    utxo.from_coinbase = true;
+                    utxo
+                }),
+            ));
+
+        state
+            .expect_request_that(|req| {
+                matches!(
+                    req,
+                    zebra_state::Request::CheckBestChainTipNullifiersAndAnchors(_)
+                )
+            })
+            .await
+            .expect("verifier should call mock state service with correct request")
+            .respond(zebra_state::Response::ValidBestChainTipNullifiersAndAnchors);
+    });
+
+    let verifier_response = verifier
+        .oneshot(Request::Mempool {
+            transaction: tx.into(),
+            height,
+        })
+        .await
+        .expect_err("verification of transaction with immature spend should fail");
+
+    assert_eq!(
+        verifier_response, expected_error,
+        "expected to fail verification, got: {verifier_response:?}"
+    );
+}
+
 #[test]
 fn v5_transaction_with_no_outputs_fails_validation() {
     let transaction = fake_v5_transactions_for_network(

zebra-rpc/src/methods/get_block_template_rpcs.rs

@@ -498,8 +498,13 @@ where
                 //
                 // We always return after 90 minutes on mainnet, even if we have the same response,
                 // because the max time has been reached.
-                let chain_tip_and_local_time =
+                let chain_tip_and_local_time @ zebra_state::GetBlockTemplateChainInfo {
-                    fetch_state_tip_and_local_time(state.clone()).await?;
+                    tip_hash,
+                    tip_height,
+                    max_time,
+                    cur_time,
+                    ..
+                } = fetch_state_tip_and_local_time(state.clone()).await?;
 
                 // Fetch the mempool data for the block template:
                 // - if the mempool transactions change, we might return from long polling.
@@ -512,7 +517,7 @@ where
                 // Optional TODO:
                 // - add a `MempoolChange` type with an `async changed()` method (like `ChainTip`)
                 let Some(mempool_txs) =
-                    fetch_mempool_transactions(mempool.clone(), chain_tip_and_local_time.tip_hash)
+                    fetch_mempool_transactions(mempool.clone(), tip_hash)
                         .await?
                         // If the mempool and state responses are out of sync:
                         // - if we are not long polling, omit mempool transactions from the template,
@@ -523,9 +528,9 @@ where
 
                 // - Long poll ID calculation
                 let server_long_poll_id = LongPollInput::new(
-                    chain_tip_and_local_time.tip_height,
+                    tip_height,
-                    chain_tip_and_local_time.tip_hash,
+                    tip_hash,
-                    chain_tip_and_local_time.max_time,
+                    max_time,
                     mempool_txs.iter().map(|tx| tx.transaction.id),
                 )
                 .generate_id();
@@ -582,9 +587,7 @@ where
                 // It can also be zero if cur_time was clamped to max_time. In that case,
                 // we want to wait for another change, and ignore this timeout. So we use an
                 // `OptionFuture::None`.
-                let duration_until_max_time = chain_tip_and_local_time
+                let duration_until_max_time = max_time.saturating_duration_since(cur_time);
-                    .max_time
-                    .saturating_duration_since(chain_tip_and_local_time.cur_time);
                 let wait_for_max_time: OptionFuture<_> = if duration_until_max_time.seconds() > 0 {
                     Some(tokio::time::sleep(duration_until_max_time.to_std()))
                 } else {
@@ -607,8 +610,8 @@ where
                     // This timer elapses every few seconds
                     _elapsed = wait_for_mempool_request => {
                         tracing::info!(
-                            max_time = ?chain_tip_and_local_time.max_time,
+                            ?max_time,
-                            cur_time = ?chain_tip_and_local_time.cur_time,
+                            ?cur_time,
                             ?server_long_poll_id,
                             ?client_long_poll_id,
                             GET_BLOCK_TEMPLATE_MEMPOOL_LONG_POLL_INTERVAL,
@@ -621,8 +624,8 @@ where
                         match tip_changed_result {
                             Ok(()) => {
                                 tracing::info!(
-                                    max_time = ?chain_tip_and_local_time.max_time,
+                                    ?max_time,
-                                    cur_time = ?chain_tip_and_local_time.cur_time,
+                                    ?cur_time,
                                     ?server_long_poll_id,
                                     ?client_long_poll_id,
                                     "returning from long poll because state has changed"
@@ -634,8 +637,8 @@ where
                                 // it will help with debugging.
                                 tracing::info!(
                                     ?recv_error,
-                                    max_time = ?chain_tip_and_local_time.max_time,
+                                    ?max_time,
-                                    cur_time = ?chain_tip_and_local_time.cur_time,
+                                    ?cur_time,
                                     ?server_long_poll_id,
                                     ?client_long_poll_id,
                                     "returning from long poll due to a state error.\
@@ -657,8 +660,8 @@ where
                         // This log should stay at info when the others go to debug,
                         // it's very rare.
                         tracing::info!(
-                            max_time = ?chain_tip_and_local_time.max_time,
+                            ?max_time,
-                            cur_time = ?chain_tip_and_local_time.cur_time,
+                            ?cur_time,
                             ?server_long_poll_id,
                             ?client_long_poll_id,
                             "returning from long poll because max time was reached"

zebra-state/src/lib.rs

@@ -36,7 +36,7 @@ pub use request::{FinalizedBlock, HashOrHeight, PreparedBlock, ReadRequest, Requ
 pub use response::{KnownBlock, MinedTx, ReadResponse, Response};
 pub use service::{
     chain_tip::{ChainTipChange, LatestChainTip, TipAction},
-    init, spawn_init,
+    check, init, spawn_init,
     watch_receiver::WatchReceiver,
     OutputIndex, OutputLocation, TransactionLocation,
 };

zebra-state/src/service.rs

@@ -69,7 +69,7 @@ pub mod block_iter;
 pub mod chain_tip;
 pub mod watch_receiver;
 
-pub(crate) mod check;
+pub mod check;
 
 pub(crate) mod finalized_state;
 pub(crate) mod non_finalized_state;

zebra-state/src/service/check.rs

@@ -31,6 +31,8 @@ pub(crate) mod difficulty;
 pub(crate) mod nullifier;
 pub(crate) mod utxo;
 
+pub use utxo::transparent_coinbase_spend;
+
 #[cfg(test)]
 mod tests;
 

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

@@ -53,8 +53,13 @@ fn accept_shielded_mature_coinbase_utxo_spend() {
     };
 
     let result =
-        check::utxo::transparent_coinbase_spend(outpoint, spend_restriction, ordered_utxo.clone());
+        check::utxo::transparent_coinbase_spend(outpoint, spend_restriction, ordered_utxo.as_ref());
-    assert_eq!(result, Ok(ordered_utxo));
+
+    assert_eq!(
+        result,
+        Ok(()),
+        "mature transparent coinbase spend check should return Ok(())"
+    );
 }
 
 /// Check that non-shielded spends of coinbase transparent outputs fail.
@@ -75,7 +80,8 @@ fn reject_unshielded_coinbase_utxo_spend() {
 
     let spend_restriction = transparent::CoinbaseSpendRestriction::SomeTransparentOutputs;
 
-    let result = check::utxo::transparent_coinbase_spend(outpoint, spend_restriction, ordered_utxo);
+    let result =
+        check::utxo::transparent_coinbase_spend(outpoint, spend_restriction, ordered_utxo.as_ref());
     assert_eq!(result, Err(UnshieldedTransparentCoinbaseSpend { outpoint }));
 }
 
@@ -100,7 +106,8 @@ fn reject_immature_coinbase_utxo_spend() {
     let spend_restriction =
         transparent::CoinbaseSpendRestriction::OnlyShieldedOutputs { spend_height };
 
-    let result = check::utxo::transparent_coinbase_spend(outpoint, spend_restriction, ordered_utxo);
+    let result =
+        check::utxo::transparent_coinbase_spend(outpoint, spend_restriction, ordered_utxo.as_ref());
     assert_eq!(
         result,
         Err(ImmatureTransparentCoinbaseSpend {

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

@@ -71,7 +71,7 @@ pub fn transparent_spend(
             // so we check transparent coinbase maturity and shielding
             // using known valid UTXOs during non-finalized chain validation.
             let spend_restriction = transaction.coinbase_spend_restriction(prepared.height);
-            let utxo = transparent_coinbase_spend(spend, spend_restriction, utxo)?;
+            transparent_coinbase_spend(spend, spend_restriction, utxo.as_ref())?;
 
             // We don't delete the UTXOs until the block is committed,
             // so we  need to check for duplicate spends within the same block.
@@ -185,26 +185,26 @@ fn transparent_spend_chain_order(
 pub fn transparent_coinbase_spend(
     outpoint: transparent::OutPoint,
     spend_restriction: transparent::CoinbaseSpendRestriction,
-    utxo: transparent::OrderedUtxo,
+    utxo: &transparent::Utxo,
-) -> Result<transparent::OrderedUtxo, ValidateContextError> {
+) -> Result<(), ValidateContextError> {
-    if !utxo.utxo.from_coinbase {
+    if !utxo.from_coinbase {
-        return Ok(utxo);
+        return Ok(());
     }
 
     match spend_restriction {
         OnlyShieldedOutputs { spend_height } => {
             let min_spend_height =
-                utxo.utxo.height + MIN_TRANSPARENT_COINBASE_MATURITY.try_into().unwrap();
+                utxo.height + MIN_TRANSPARENT_COINBASE_MATURITY.try_into().unwrap();
             let min_spend_height =
                 min_spend_height.expect("valid UTXOs have coinbase heights far below Height::MAX");
             if spend_height >= min_spend_height {
-                Ok(utxo)
+                Ok(())
             } else {
                 Err(ImmatureTransparentCoinbaseSpend {
                     outpoint,
                     spend_height,
                     min_spend_height,
-                    created_height: utxo.utxo.height,
+                    created_height: utxo.height,
                 })
             }
         }