zcash_client_backend: Make `WalletRead::get_transaction` return `Result<Option<Transaction>, _>`

This should never have had the behavior of returning an error on a
missing txid in the first place; doing so conflates database corruption
or connectivity errors with the ordinary case where data may not be
available.

zcash_client_backend/CHANGELOG.md

@@ -64,6 +64,9 @@ and this library adheres to Rust's notion of
     - `get_account_for_ufvk` now returns `Self::Account` instead of a bare
       `AccountId`.
     - Added `get_orchard_nullifiers` method.
+    - `get_transaction` now returns `Result<Option<Transaction>, _>` rather
+      than returning an `Err` if the `txid` parameter does not correspond to
+      a transaction in the database.
   - Changes to the `InputSource` trait:
     - `select_spendable_notes` now takes its `target_value` argument as a
       `NonNegativeAmount`. Also, it now returns a `SpendableNotes` data 

zcash_client_backend/src/data_api.rs

@@ -837,7 +837,7 @@ pub trait WalletRead {
     fn get_memo(&self, note_id: NoteId) -> Result<Option<Memo>, Self::Error>;
 
     /// Returns a transaction.
-    fn get_transaction(&self, txid: TxId) -> Result<Transaction, Self::Error>;
+    fn get_transaction(&self, txid: TxId) -> Result<Option<Transaction>, Self::Error>;
 
     /// Returns the nullifiers for Sapling notes that the wallet is tracking, along with their
     /// associated account IDs, that are either unspent or have not yet been confirmed as spent (in
@@ -1791,8 +1791,8 @@ pub mod testing {
             Ok(None)
         }
 
-        fn get_transaction(&self, _txid: TxId) -> Result<Transaction, Self::Error> {
-            Err(())
+        fn get_transaction(&self, _txid: TxId) -> Result<Option<Transaction>, Self::Error> {
+            Ok(None)
         }
 
         fn get_sapling_nullifiers(

zcash_client_sqlite/src/lib.rs

@@ -461,8 +461,9 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> WalletRead for W
         }
     }
 
-    fn get_transaction(&self, txid: TxId) -> Result<Transaction, Self::Error> {
-        wallet::get_transaction(self.conn.borrow(), &self.params, txid).map(|(_, tx)| tx)
+    fn get_transaction(&self, txid: TxId) -> Result<Option<Transaction>, Self::Error> {
+        wallet::get_transaction(self.conn.borrow(), &self.params, txid)
+            .map(|res| res.map(|(_, tx)| tx))
     }
 
     fn get_sapling_nullifiers(

zcash_client_sqlite/src/testing.rs

@@ -480,6 +480,7 @@ where
         let tx = self
             .wallet()
             .get_transaction(txid)
+            .unwrap()
             .expect("TxId should exist in the wallet");
 
         // Index 0 is by definition a coinbase transaction, and the wallet doesn't

zcash_client_sqlite/src/testing/pool.rs

@@ -199,6 +199,7 @@ pub(crate) fn send_single_step_proposed_transfer<T: ShieldedPoolTester>() {
     let tx = st
         .wallet()
         .get_transaction(sent_tx_id)
+        .unwrap()
         .expect("Created transaction was stored.");
     let ufvks = [(account, usk.to_unified_full_viewing_key())]
         .into_iter()

zcash_client_sqlite/src/wallet.rs

@@ -1353,12 +1353,8 @@ pub(crate) fn get_transaction<P: Parameters>(
     conn: &rusqlite::Connection,
     params: &P,
     txid: TxId,
-) -> Result<(BlockHeight, Transaction), SqliteClientError> {
-    let (tx_bytes, block_height, expiry_height): (
-        Vec<_>,
-        Option<BlockHeight>,
-        Option<BlockHeight>,
-    ) = conn.query_row(
+) -> Result<Option<(BlockHeight, Transaction)>, SqliteClientError> {
+    conn.query_row(
         "SELECT raw, block, expiry_height FROM transactions
         WHERE txid = ?",
         [txid.as_ref()],
@@ -1366,55 +1362,58 @@ pub(crate) fn get_transaction<P: Parameters>(
             let h: Option<u32> = row.get(1)?;
             let expiry: Option<u32> = row.get(2)?;
             Ok((
-                row.get(0)?,
+                row.get::<_, Vec<u8>>(0)?,
                 h.map(BlockHeight::from),
                 expiry.map(BlockHeight::from),
             ))
         },
-    )?;
-
-    // We need to provide a consensus branch ID so that pre-v5 `Transaction` structs
-    // (which don't commit directly to one) can store it internally.
-    // - If the transaction is mined, we use the block height to get the correct one.
-    // - If the transaction is unmined and has a cached non-zero expiry height, we use
-    //   that (relying on the invariant that a transaction can't be mined across a network
-    //   upgrade boundary, so the expiry height must be in the same epoch).
-    // - Otherwise, we use a placeholder for the initial transaction parse (as the
-    //   consensus branch ID is not used there), and then either use its non-zero expiry
-    //   height or return an error.
-    if let Some(height) =
-        block_height.or_else(|| expiry_height.filter(|h| h > &BlockHeight::from(0)))
-    {
-        Transaction::read(&tx_bytes[..], BranchId::for_height(params, height))
-            .map(|t| (height, t))
-            .map_err(SqliteClientError::from)
-    } else {
-        let tx_data = Transaction::read(&tx_bytes[..], BranchId::Sprout)
-            .map_err(SqliteClientError::from)?
-            .into_data();
-
-        let expiry_height = tx_data.expiry_height();
-        if expiry_height > BlockHeight::from(0) {
-            TransactionData::from_parts(
-                tx_data.version(),
-                BranchId::for_height(params, expiry_height),
-                tx_data.lock_time(),
-                expiry_height,
-                tx_data.transparent_bundle().cloned(),
-                tx_data.sprout_bundle().cloned(),
-                tx_data.sapling_bundle().cloned(),
-                tx_data.orchard_bundle().cloned(),
-            )
-            .freeze()
-            .map(|t| (expiry_height, t))
-            .map_err(SqliteClientError::from)
+    )
+    .optional()?
+    .map(|(tx_bytes, block_height, expiry_height)| {
+        // We need to provide a consensus branch ID so that pre-v5 `Transaction` structs
+        // (which don't commit directly to one) can store it internally.
+        // - If the transaction is mined, we use the block height to get the correct one.
+        // - If the transaction is unmined and has a cached non-zero expiry height, we use
+        //   that (relying on the invariant that a transaction can't be mined across a network
+        //   upgrade boundary, so the expiry height must be in the same epoch).
+        // - Otherwise, we use a placeholder for the initial transaction parse (as the
+        //   consensus branch ID is not used there), and then either use its non-zero expiry
+        //   height or return an error.
+        if let Some(height) =
+            block_height.or_else(|| expiry_height.filter(|h| h > &BlockHeight::from(0)))
+        {
+            Transaction::read(&tx_bytes[..], BranchId::for_height(params, height))
+                .map(|t| (height, t))
+                .map_err(SqliteClientError::from)
         } else {
-            Err(SqliteClientError::CorruptedData(
-                "Consensus branch ID not known, cannot parse this transaction until it is mined"
-                    .to_string(),
-            ))
+            let tx_data = Transaction::read(&tx_bytes[..], BranchId::Sprout)
+                .map_err(SqliteClientError::from)?
+                .into_data();
+
+            let expiry_height = tx_data.expiry_height();
+            if expiry_height > BlockHeight::from(0) {
+                TransactionData::from_parts(
+                    tx_data.version(),
+                    BranchId::for_height(params, expiry_height),
+                    tx_data.lock_time(),
+                    expiry_height,
+                    tx_data.transparent_bundle().cloned(),
+                    tx_data.sprout_bundle().cloned(),
+                    tx_data.sapling_bundle().cloned(),
+                    tx_data.orchard_bundle().cloned(),
+                )
+                .freeze()
+                .map(|t| (expiry_height, t))
+                .map_err(SqliteClientError::from)
+            } else {
+                Err(SqliteClientError::CorruptedData(
+                    "Consensus branch ID not known, cannot parse this transaction until it is mined"
+                        .to_string(),
+                ))
+            }
         }
-    }
+    })
+    .transpose()
 }
 
 /// Returns the memo for a sent note, if the sent note is known to the wallet.
@@ -2986,7 +2985,12 @@ mod tests {
         check_balance(&st, 2, NonNegativeAmount::ZERO);
 
         // Expire the shielding transaction.
-        let expiry_height = st.wallet().get_transaction(txid).unwrap().expiry_height();
+        let expiry_height = st
+            .wallet()
+            .get_transaction(txid)
+            .unwrap()
+            .expect("Transaction exists in the wallet.")
+            .expiry_height();
         st.wallet_mut().update_chain_tip(expiry_height).unwrap();
 
         // TODO: Making the transparent output spendable in this situation requires

zcash_client_sqlite/src/wallet/init/migrations/sapling_memo_consistency.rs

@@ -87,8 +87,8 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
         )?;
 
         for ((id_tx, txid), ufvks) in tx_sent_notes {
-            let (block_height, tx) =
-                get_transaction(transaction, &self.params, txid).map_err(|err| match err {
+            let (block_height, tx) = get_transaction(transaction, &self.params, txid)
+                .map_err(|err| match err {
                     SqliteClientError::CorruptedData(msg) => {
                         WalletMigrationError::CorruptedData(msg)
                     }
@@ -97,6 +97,12 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
                         "An error was encountered decoding transaction data: {:?}",
                         other
                     )),
+                })?
+                .ok_or_else(|| {
+                    WalletMigrationError::CorruptedData(format!(
+                        "Transaction not found for id {:?}",
+                        txid
+                    ))
                 })?;
 
             let decrypted_outputs = decrypt_transaction(&self.params, block_height, &tx, &ufvks);

zcash_keys/src/keys.rs

@@ -1,6 +1,5 @@
 //! Helper functions for managing light client key material.
 
-
 use std::{error, fmt};
 
 use zcash_address::unified::{self, Container, Encoding, Typecode};