change(consensus) verify that mempool transaction UTXOs are in the best chain (#5616)

* Uses BestChainUtxo to find utxos for mempool

* adds missing input test

* Apply suggestions from code review

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

* update other instances of the renamed InputNotFound error

* adds read::unspent_utxo fn

* adds test for success case

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

zebra-chain/src/transaction/arbitrary.rs

@@ -930,16 +930,7 @@ pub fn test_transactions(
         Network::Testnet => zebra_test::vectors::TESTNET_BLOCKS.iter(),
     };
 
-    blocks.flat_map(|(&block_height, &block_bytes)| {
+    transactions_from_blocks(blocks)
-        let block = block_bytes
-            .zcash_deserialize_into::<block::Block>()
-            .expect("block is structurally valid");
-
-        block
-            .transactions
-            .into_iter()
-            .map(move |transaction| (block::Height(block_height), transaction))
-    })
 }
 
 /// Generate an iterator over fake V5 transactions.
@@ -950,18 +941,23 @@ pub fn fake_v5_transactions_for_network<'b>(
     network: Network,
     blocks: impl DoubleEndedIterator<Item = (&'b u32, &'b &'static [u8])> + 'b,
 ) -> impl DoubleEndedIterator<Item = Transaction> + 'b {
-    blocks.flat_map(move |(height, original_bytes)| {
+    transactions_from_blocks(blocks)
-        let original_block = original_bytes
+        .map(move |(height, transaction)| transaction_to_fake_v5(&transaction, network, height))
+}
+
+/// Generate an iterator over ([`block::Height`], [`Arc<Transaction>`]).
+pub fn transactions_from_blocks<'a>(
+    blocks: impl DoubleEndedIterator<Item = (&'a u32, &'a &'static [u8])> + 'a,
+) -> impl DoubleEndedIterator<Item = (block::Height, Arc<Transaction>)> + 'a {
+    blocks.flat_map(|(&block_height, &block_bytes)| {
+        let block = block_bytes
             .zcash_deserialize_into::<block::Block>()
             .expect("block is structurally valid");
 
-        original_block
+        block
             .transactions
             .into_iter()
-            .map(move |transaction| {
+            .map(move |transaction| (block::Height(block_height), transaction))
-                transaction_to_fake_v5(&transaction, network, block::Height(*height))
-            })
-            .map(Transaction::from)
     })
 }
 

zebra-consensus/src/error.rs

@@ -177,6 +177,9 @@ pub enum TransactionError {
 
     #[error("must have at least one active orchard flag")]
     NotEnoughFlags,
+
+    #[error("could not find a mempool transaction input UTXO in the best chain")]
+    TransparentInputNotFound,
 }
 
 impl From<BoxError> for TransactionError {

zebra-consensus/src/transaction.rs

@@ -356,8 +356,9 @@ where
             // https://zips.z.cash/zip-0213#specification
 
             // Load spent UTXOs from state.
+            // TODO: Make this a method of `Request` and replace `tx.clone()` with `self.transaction()`?
             let (spent_utxos, spent_outputs) =
-                Self::spent_utxos(tx.clone(), req.known_utxos(), state).await?;
+                Self::spent_utxos(tx.clone(), req.known_utxos(), req.is_mempool(), state).await?;
 
             let cached_ffi_transaction =
                 Arc::new(CachedFfiTransaction::new(tx.clone(), spent_outputs));
@@ -464,6 +465,7 @@ where
     async fn spent_utxos(
         tx: Arc<Transaction>,
         known_utxos: Arc<HashMap<transparent::OutPoint, OrderedUtxo>>,
+        is_mempool: bool,
         state: Timeout<ZS>,
     ) -> Result<
         (
@@ -478,9 +480,20 @@ where
         for input in inputs {
             if let transparent::Input::PrevOut { outpoint, .. } = input {
                 tracing::trace!("awaiting outpoint lookup");
+                // Currently, Zebra only supports known UTXOs in block transactions.
+                // But it might support them in the mempool in future.
                 let utxo = if let Some(output) = known_utxos.get(outpoint) {
                     tracing::trace!("UXTO in known_utxos, discarding query");
                     output.utxo.clone()
+                } else if is_mempool {
+                    let query = state
+                        .clone()
+                        .oneshot(zs::Request::UnspentBestChainUtxo(*outpoint));
+                    if let zebra_state::Response::UnspentBestChainUtxo(utxo) = query.await? {
+                        utxo.ok_or(TransactionError::TransparentInputNotFound)?
+                    } else {
+                        unreachable!("UnspentBestChainUtxo always responds with Option<Utxo>")
+                    }
                 } else {
                     let query = state
                         .clone()

zebra-consensus/src/transaction/tests.rs

@@ -19,12 +19,15 @@ use zebra_chain::{
     transaction::{
         arbitrary::{
             fake_v5_transactions_for_network, insert_fake_orchard_shielded_data, test_transactions,
+            transactions_from_blocks,
         },
         Hash, HashType, JoinSplitData, LockTime, Transaction,
     },
     transparent::{self, CoinbaseData},
 };
 
+use zebra_test::mock_service::MockService;
+
 use crate::error::TransactionError;
 
 use super::{check, Request, Verifier};
@@ -177,6 +180,92 @@ fn v5_transaction_with_no_inputs_fails_validation() {
     );
 }
 
+#[tokio::test]
+async fn mempool_request_with_missing_input_is_rejected() {
+    let mut state: MockService<_, _, _, _> = MockService::build().for_prop_tests();
+    let verifier = Verifier::new(Network::Mainnet, state.clone());
+
+    let (height, tx) = transactions_from_blocks(zebra_test::vectors::MAINNET_BLOCKS.iter())
+        .find(|(_, tx)| !(tx.is_coinbase() || tx.inputs().is_empty()))
+        .expect("At least one non-coinbase transaction with transparent inputs in test vectors");
+
+    let expected_state_request = zebra_state::Request::UnspentBestChainUtxo(match tx.inputs()[0] {
+        transparent::Input::PrevOut { outpoint, .. } => outpoint,
+        transparent::Input::Coinbase { .. } => panic!("requires a non-coinbase transaction"),
+    });
+
+    tokio::spawn(async move {
+        state
+            .expect_request(expected_state_request)
+            .await
+            .expect("verifier should call mock state service")
+            .respond(zebra_state::Response::UnspentBestChainUtxo(None));
+    });
+
+    let verifier_response = verifier
+        .oneshot(Request::Mempool {
+            transaction: tx.into(),
+            height,
+        })
+        .await;
+
+    assert_eq!(
+        verifier_response,
+        Err(TransactionError::TransparentInputNotFound)
+    );
+}
+
+#[tokio::test]
+async fn mempool_request_with_present_input_is_accepted() {
+    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::unlocked(),
+        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"),
+    };
+
+    tokio::spawn(async move {
+        state
+            .expect_request(zebra_state::Request::UnspentBestChainUtxo(input_outpoint))
+            .await
+            .expect("verifier should call mock state service")
+            .respond(zebra_state::Response::UnspentBestChainUtxo(
+                known_utxos
+                    .get(&input_outpoint)
+                    .map(|utxo| utxo.utxo.clone()),
+            ));
+    });
+
+    let verifier_response = verifier
+        .oneshot(Request::Mempool {
+            transaction: tx.into(),
+            height,
+        })
+        .await;
+
+    assert!(
+        verifier_response.is_ok(),
+        "expected successful verification, got: {verifier_response:?}"
+    );
+}
+
 #[test]
 fn v5_transaction_with_no_outputs_fails_validation() {
     let transaction = fake_v5_transactions_for_network(

zebra-state/src/request.rs

@@ -458,6 +458,12 @@ pub enum Request {
     /// * [`Response::Transaction(None)`](Response::Transaction) otherwise.
     Transaction(transaction::Hash),
 
+    /// Looks up a UTXO identified by the given [`OutPoint`](transparent::OutPoint),
+    /// returning `None` immediately if it is unknown.
+    ///
+    /// Checks verified blocks in the finalized chain and the _best_ non-finalized chain.
+    UnspentBestChainUtxo(transparent::OutPoint),
+
     /// Looks up a block by hash or height in the current best chain.
     ///
     /// Returns
@@ -545,6 +551,7 @@ impl Request {
             Request::Tip => "tip",
             Request::BlockLocator => "block_locator",
             Request::Transaction(_) => "transaction",
+            Request::UnspentBestChainUtxo { .. } => "unspent_best_chain_utxo",
             Request::Block(_) => "block",
             Request::FindBlockHashes { .. } => "find_block_hashes",
             Request::FindBlockHeaders { .. } => "find_block_headers",
@@ -613,10 +620,7 @@ pub enum ReadRequest {
     /// returning `None` immediately if it is unknown.
     ///
     /// Checks verified blocks in the finalized chain and the _best_ non-finalized chain.
-    ///
+    UnspentBestChainUtxo(transparent::OutPoint),
-    /// This request is purely informational, there is no guarantee that
-    /// the UTXO remains unspent in the best chain.
-    BestChainUtxo(transparent::OutPoint),
 
     /// Looks up a UTXO identified by the given [`OutPoint`](transparent::OutPoint),
     /// returning `None` immediately if it is unknown.
@@ -750,7 +754,7 @@ impl ReadRequest {
             ReadRequest::Block(_) => "block",
             ReadRequest::Transaction(_) => "transaction",
             ReadRequest::TransactionIdsForBlock(_) => "transaction_ids_for_block",
-            ReadRequest::BestChainUtxo { .. } => "best_chain_utxo",
+            ReadRequest::UnspentBestChainUtxo { .. } => "unspent_best_chain_utxo",
             ReadRequest::AnyChainUtxo { .. } => "any_chain_utxo",
             ReadRequest::BlockLocator => "block_locator",
             ReadRequest::FindBlockHashes { .. } => "find_block_hashes",
@@ -789,6 +793,9 @@ impl TryFrom<Request> for ReadRequest {
 
             Request::Block(hash_or_height) => Ok(ReadRequest::Block(hash_or_height)),
             Request::Transaction(tx_hash) => Ok(ReadRequest::Transaction(tx_hash)),
+            Request::UnspentBestChainUtxo(outpoint) => {
+                Ok(ReadRequest::UnspentBestChainUtxo(outpoint))
+            }
 
             Request::BlockLocator => Ok(ReadRequest::BlockLocator),
             Request::FindBlockHashes { known_blocks, stop } => {

zebra-state/src/response.rs

@@ -36,6 +36,9 @@ pub enum Response {
     /// Response to [`Request::Transaction`] with the specified transaction.
     Transaction(Option<Arc<Transaction>>),
 
+    /// Response to [`Request::UnspentBestChainUtxo`] with the UTXO
+    UnspentBestChainUtxo(Option<transparent::Utxo>),
+
     /// Response to [`Request::Block`] with the specified block.
     Block(Option<Arc<Block>>),
 
@@ -81,12 +84,9 @@ pub enum ReadResponse {
     /// The response to a `FindBlockHeaders` request.
     BlockHeaders(Vec<block::CountedHeader>),
 
-    /// The response to a `BestChainUtxo` request, from verified blocks in the
+    /// The response to a `UnspentBestChainUtxo` request, from verified blocks in the
     /// _best_ non-finalized chain, or the finalized chain.
-    ///
+    UnspentBestChainUtxo(Option<transparent::Utxo>),
-    /// This response is purely informational, there is no guarantee that
-    /// the UTXO remains unspent in the best chain.
-    BestChainUtxo(Option<transparent::Utxo>),
 
     /// The response to an `AnyChainUtxo` request, from verified blocks in
     /// _any_ non-finalized chain, or the finalized chain.
@@ -132,6 +132,8 @@ impl TryFrom<ReadResponse> for Response {
             ReadResponse::Transaction(tx_and_height) => {
                 Ok(Response::Transaction(tx_and_height.map(|(tx, _height)| tx)))
             }
+            ReadResponse::UnspentBestChainUtxo(utxo) => Ok(Response::UnspentBestChainUtxo(utxo)),
+
 
             ReadResponse::AnyChainUtxo(_) => Err("ReadService does not track pending UTXOs. \
                                                   Manually unwrap the response, and handle pending UTXOs."),
@@ -141,7 +143,6 @@ impl TryFrom<ReadResponse> for Response {
             ReadResponse::BlockHeaders(headers) => Ok(Response::BlockHeaders(headers)),
 
             ReadResponse::TransactionIdsForBlock(_)
-            | ReadResponse::BestChainUtxo(_)
             | ReadResponse::SaplingTree(_)
             | ReadResponse::OrchardTree(_)
             | ReadResponse::AddressBalance(_)

zebra-state/src/service.rs

@@ -1021,6 +1021,7 @@ impl Service<Request> for StateService {
             | Request::Tip
             | Request::BlockLocator
             | Request::Transaction(_)
+            | Request::UnspentBestChainUtxo(_)
             | Request::Block(_)
             | Request::FindBlockHashes { .. }
             | Request::FindBlockHeaders { .. } => {
@@ -1217,7 +1218,7 @@ impl Service<ReadRequest> for ReadStateService {
             }
 
             // Currently unused.
-            ReadRequest::BestChainUtxo(outpoint) => {
+            ReadRequest::UnspentBestChainUtxo(outpoint) => {
                 let timer = CodeTimer::start();
 
                 let state = self.clone();
@@ -1227,17 +1228,21 @@ impl Service<ReadRequest> for ReadStateService {
                     span.in_scope(move || {
                         let utxo = state.non_finalized_state_receiver.with_watch_data(
                             |non_finalized_state| {
-                                read::utxo(non_finalized_state.best_chain(), &state.db, outpoint)
+                                read::unspent_utxo(
+                                    non_finalized_state.best_chain(),
+                                    &state.db,
+                                    outpoint,
+                                )
                             },
                         );
 
                         // The work is done in the future.
-                        timer.finish(module_path!(), line!(), "ReadRequest::BestChainUtxo");
+                        timer.finish(module_path!(), line!(), "ReadRequest::UnspentBestChainUtxo");
 
-                        Ok(ReadResponse::BestChainUtxo(utxo))
+                        Ok(ReadResponse::UnspentBestChainUtxo(utxo))
                     })
                 })
-                .map(|join_result| join_result.expect("panic in ReadRequest::BestChainUtxo"))
+                .map(|join_result| join_result.expect("panic in ReadRequest::UnspentBestChainUtxo"))
                 .boxed()
             }
 

zebra-state/src/service/read.rs

@@ -28,7 +28,9 @@ pub use address::{
     utxo::{address_utxos, AddressUtxos, ADDRESS_HEIGHTS_FULL_RANGE},
 };
 
-pub use block::{any_utxo, block, block_header, transaction, transaction_hashes_for_block, utxo};
+pub use block::{
+    any_utxo, block, block_header, transaction, transaction_hashes_for_block, unspent_utxo, utxo,
+};
 
 #[cfg(feature = "getblocktemplate-rpcs")]
 pub use block::hash;

zebra-state/src/service/read/block.rs

@@ -141,6 +141,22 @@ where
         .or_else(|| db.utxo(&outpoint).map(|utxo| utxo.utxo))
 }
 
+/// Returns the [`Utxo`] for [`transparent::OutPoint`], if it exists and is unspent in the
+/// non-finalized `chain` or finalized `db`.
+pub fn unspent_utxo<C>(
+    chain: Option<C>,
+    db: &ZebraDb,
+    outpoint: transparent::OutPoint,
+) -> Option<Utxo>
+where
+    C: AsRef<Chain>,
+{
+    match chain {
+        Some(chain) if chain.as_ref().spent_utxos.contains(&outpoint) => None,
+        chain => utxo(chain, db, outpoint),
+    }
+}
+
 /// Returns the [`Utxo`] for [`transparent::OutPoint`], if it exists in any chain
 /// in the `non_finalized_state`, or in the finalized `db`.
 ///