Merge pull request #870 from zcash/fix-get_transaction

zcash_client_sqlite: Fix `WalletDb::get_transaction` for unmined txs

zcash_client_sqlite/CHANGELOG.md

@@ -28,6 +28,9 @@ and this library adheres to Rust's notion of
 - Fixed an off-by-one error in the `BlockSource` implementation for the SQLite-backed
  `BlockDb` block database which could result in blocks being skipped at the start of
  scan ranges.
+- `WalletDb::get_transaction` no longer returns an error when called on a transaction
+  that has not yet been mined, unless the transaction's consensus branch ID cannot be
+  determined by other means.
 
 ## [0.7.1] - 2023-05-17
 

zcash_client_sqlite/src/wallet.rs

@@ -69,6 +69,7 @@ use std::collections::HashMap;
 use std::convert::TryFrom;
 use std::io::{self, Cursor};
 use zcash_client_backend::data_api::ShieldedProtocol;
+use zcash_primitives::transaction::TransactionData;
 
 use zcash_primitives::{
     block::BlockHash,
@@ -491,18 +492,65 @@ pub(crate) fn get_transaction<P: Parameters>(
     params: &P,
     id_tx: i64,
 ) -> Result<Transaction, SqliteClientError> {
-    let (tx_bytes, block_height): (Vec<_>, BlockHeight) = conn.query_row(
-        "SELECT raw, block FROM transactions
+    let (tx_bytes, block_height, expiry_height): (
+        Vec<_>,
+        Option<BlockHeight>,
+        Option<BlockHeight>,
+    ) = conn.query_row(
+        "SELECT raw, block, expiry_height FROM transactions
         WHERE id_tx = ?",
         [id_tx],
         |row| {
-            let h: u32 = row.get(1)?;
-            Ok((row.get(0)?, BlockHeight::from(h)))
+            let h: Option<u32> = row.get(1)?;
+            let expiry: Option<u32> = row.get(2)?;
+            Ok((
+                row.get(0)?,
+                h.map(BlockHeight::from),
+                expiry.map(BlockHeight::from),
+            ))
         },
     )?;
 
-    Transaction::read(&tx_bytes[..], BranchId::for_height(params, block_height))
-        .map_err(SqliteClientError::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_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_err(SqliteClientError::from)
+        } else {
+            Err(SqliteClientError::CorruptedData(
+                "Consensus branch ID not known, cannot parse this transaction until it is mined"
+                    .to_string(),
+            ))
+        }
+    }
 }
 
 /// Returns the memo for a sent note.