Merge pull request #1496 from nuttycom/fix/transparent_spend_recording

zcash_client_sqlite: Track all transparent spends.

zcash_client_backend/src/data_api.rs

@@ -566,7 +566,7 @@ pub struct SpendableNotes<NoteRef> {
 
 /// A request for transaction data enhancement, spentness check, or discovery
 /// of spends from a given transparent address within a specific block range.
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub enum TransactionDataRequest {
     /// Information about the chain's view of a transaction is requested.
     ///

zcash_client_sqlite/src/wallet/db.rs

@@ -365,6 +365,28 @@ CREATE TABLE "transparent_received_output_spends" (
     UNIQUE (transparent_received_output_id, transaction_id)
 )"#;
 
+/// A cache of the relationship between a transaction and the prevout data of its
+/// transparent inputs.
+///
+/// This table is used in out-of-order wallet recovery to cache the information about
+/// what transaction(s) spend each transparent outpoint, so that if an output belonging
+/// to the wallet is detected after the transaction that spends it has been processed,
+/// the spend can also be recorded as part of the process of adding the output to
+/// [`TABLE_TRANSPARENT_RECEIVED_OUTPUTS`].
+pub(super) const TABLE_TRANSPARENT_SPEND_MAP: &str = r#"
+CREATE TABLE transparent_spend_map (
+    spending_transaction_id INTEGER NOT NULL,
+    prevout_txid BLOB NOT NULL,
+    prevout_output_index INTEGER NOT NULL,
+    FOREIGN KEY (spending_transaction_id) REFERENCES transactions(id_tx)
+    -- NOTE: We can't create a unique constraint on just (prevout_txid, prevout_output_index) 
+    -- because the same output may be attempted to be spent in multiple transactions, even 
+    -- though only one will ever be mined.
+    CONSTRAINT transparent_spend_map_unique UNIQUE (
+        spending_transaction_id, prevout_txid, prevout_output_index
+    )
+)"#;
+
 /// Stores the outputs of transactions created by the wallet.
 ///
 /// Unlike with outputs received by the wallet, we store sent outputs for all pools in

zcash_client_sqlite/src/wallet/init.rs

@@ -53,6 +53,9 @@ pub enum WalletMigrationError {
 
     /// Reverting the specified migration is not supported.
     CannotRevert(Uuid),
+
+    /// Some other unexpected violation of database business rules occurred
+    Other(SqliteClientError),
 }
 
 impl From<rusqlite::Error> for WalletMigrationError {
@@ -79,6 +82,21 @@ impl From<AddressGenerationError> for WalletMigrationError {
     }
 }
 
+impl From<SqliteClientError> for WalletMigrationError {
+    fn from(value: SqliteClientError) -> Self {
+        match value {
+            SqliteClientError::CorruptedData(err) => WalletMigrationError::CorruptedData(err),
+            SqliteClientError::DbError(err) => WalletMigrationError::DbError(err),
+            SqliteClientError::CommitmentTree(err) => WalletMigrationError::CommitmentTree(err),
+            SqliteClientError::BalanceError(err) => WalletMigrationError::BalanceError(err),
+            SqliteClientError::AddressGeneration(err) => {
+                WalletMigrationError::AddressGeneration(err)
+            }
+            other => WalletMigrationError::Other(other),
+        }
+    }
+}
+
 impl fmt::Display for WalletMigrationError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match &self {
@@ -106,6 +124,13 @@ impl fmt::Display for WalletMigrationError {
             WalletMigrationError::CannotRevert(uuid) => {
                 write!(f, "Reverting migration {} is not supported", uuid)
             }
+            WalletMigrationError::Other(err) => {
+                write!(
+                    f,
+                    "Unexpected violation of database business rules: {}",
+                    err
+                )
+            }
         }
     }
 }
@@ -117,6 +142,7 @@ impl std::error::Error for WalletMigrationError {
             WalletMigrationError::BalanceError(e) => Some(e),
             WalletMigrationError::CommitmentTree(e) => Some(e),
             WalletMigrationError::AddressGeneration(e) => Some(e),
+            WalletMigrationError::Other(e) => Some(e),
             _ => None,
         }
     }
@@ -406,6 +432,7 @@ mod tests {
             db::TABLE_TRANSACTIONS,
             db::TABLE_TRANSPARENT_RECEIVED_OUTPUT_SPENDS,
             db::TABLE_TRANSPARENT_RECEIVED_OUTPUTS,
+            db::TABLE_TRANSPARENT_SPEND_MAP,
             db::TABLE_TRANSPARENT_SPEND_SEARCH_QUEUE,
             db::TABLE_TX_LOCATOR_MAP,
             db::TABLE_TX_RETRIEVAL_QUEUE,

zcash_client_sqlite/src/wallet/init/migrations.rs

@@ -125,7 +125,9 @@ pub(super) fn all_migrations<P: consensus::Parameters + 'static>(
             params: params.clone(),
         }),
         Box::new(spend_key_available::Migration),
-        Box::new(tx_retrieval_queue::Migration),
+        Box::new(tx_retrieval_queue::Migration {
+            params: params.clone(),
+        }),
     ]
 }
 

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

@@ -4,17 +4,21 @@ use rusqlite::{named_params, Transaction};
 use schemer_rusqlite::RusqliteMigration;
 use std::collections::HashSet;
 use uuid::Uuid;
+use zcash_client_backend::data_api::DecryptedTransaction;
 use zcash_primitives::transaction::builder::DEFAULT_TX_EXPIRY_DELTA;
+use zcash_protocol::consensus::{self, BlockHeight, BranchId};
 
-use crate::wallet::init::WalletMigrationError;
+use crate::wallet::{self, init::WalletMigrationError};
 
 use super::utxos_to_txos;
 
 pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0xfec02b61_3988_4b4f_9699_98977fac9e7f);
 
-pub(crate) struct Migration;
+pub(super) struct Migration<P> {
+    pub(super) params: P,
+}
 
-impl schemer::Migration for Migration {
+impl<P> schemer::Migration for Migration<P> {
     fn id(&self) -> Uuid {
         MIGRATION_ID
     }
@@ -28,7 +32,7 @@ impl schemer::Migration for Migration {
     }
 }
 
-impl RusqliteMigration for Migration {
+impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
     type Error = WalletMigrationError;
 
     fn up(&self, transaction: &Transaction) -> Result<(), WalletMigrationError> {
@@ -48,6 +52,19 @@ impl RusqliteMigration for Migration {
                 output_index INTEGER NOT NULL,
                 FOREIGN KEY (transaction_id) REFERENCES transactions(id_tx),
                 CONSTRAINT value_received_height UNIQUE (transaction_id, output_index)
+            );
+
+            CREATE TABLE transparent_spend_map (
+                spending_transaction_id INTEGER NOT NULL,
+                prevout_txid BLOB NOT NULL,
+                prevout_output_index INTEGER NOT NULL,
+                FOREIGN KEY (spending_transaction_id) REFERENCES transactions(id_tx)
+                -- NOTE: We can't create a unique constraint on just (prevout_txid, prevout_output_index) 
+                -- because the same output may be attempted to be spent in multiple transactions, even 
+                -- though only one will ever be mined.
+                CONSTRAINT transparent_spend_map_unique UNIQUE (
+                    spending_transaction_id, prevout_txid, prevout_output_index
+                )
             );",
         )?;
 
@@ -62,12 +79,50 @@ impl RusqliteMigration for Migration {
             named_params![":default_expiry_delta": DEFAULT_TX_EXPIRY_DELTA],
         )?;
 
+        // Call `decrypt_and_store_transaction` for each transaction known to the wallet to
+        // populate the enhancement queues with any transparent history information that we don't
+        // already have.
+        let mut stmt_transactions =
+            transaction.prepare("SELECT raw, mined_height FROM transactions")?;
+        let mut rows = stmt_transactions.query([])?;
+        while let Some(row) = rows.next()? {
+            let tx_data = row.get::<_, Option<Vec<u8>>>(0)?;
+            let mined_height = row.get::<_, Option<u32>>(1)?.map(BlockHeight::from);
+
+            if let Some(tx_data) = tx_data {
+                let tx = zcash_primitives::transaction::Transaction::read(
+                    &tx_data[..],
+                    // We assume unmined transactions are created with the current consensus branch ID.
+                    mined_height
+                        .map_or(BranchId::Sapling, |h| BranchId::for_height(&self.params, h)),
+                )
+                .map_err(|_| {
+                    WalletMigrationError::CorruptedData(
+                        "Could not read serialized transaction data.".to_owned(),
+                    )
+                })?;
+
+                wallet::store_decrypted_tx(
+                    transaction,
+                    &self.params,
+                    DecryptedTransaction::new(
+                        mined_height,
+                        &tx,
+                        vec![],
+                        #[cfg(feature = "orchard")]
+                        vec![],
+                    ),
+                )?;
+            }
+        }
+
         Ok(())
     }
 
     fn down(&self, transaction: &Transaction) -> Result<(), WalletMigrationError> {
         transaction.execute_batch(
-            "DROP TABLE transparent_spend_search_queue;
+            "DROP TABLE transparent_spend_map;
+             DROP TABLE transparent_spend_search_queue;
              ALTER TABLE transactions DROP COLUMN target_height;
              DROP TABLE tx_retrieval_queue;",
         )?;

zcash_client_sqlite/src/wallet/transparent.rs

@@ -427,25 +427,31 @@ pub(crate) fn add_transparent_account_balances(
 }
 
 /// Marks the given UTXO as having been spent.
+///
+/// Returns `true` if the UTXO was known to the wallet.
 pub(crate) fn mark_transparent_utxo_spent(
     conn: &rusqlite::Connection,
-    tx_ref: TxRef,
+    spent_in_tx: TxRef,
     outpoint: &OutPoint,
-) -> Result<(), SqliteClientError> {
+) -> Result<bool, SqliteClientError> {
+    let spend_params = named_params![
+        ":spent_in_tx": spent_in_tx.0,
+        ":prevout_txid": outpoint.hash().as_ref(),
+        ":prevout_idx": outpoint.n(),
+    ];
     let mut stmt_mark_transparent_utxo_spent = conn.prepare_cached(
         "INSERT INTO transparent_received_output_spends (transparent_received_output_id, transaction_id)
          SELECT txo.id, :spent_in_tx
          FROM transparent_received_outputs txo
          JOIN transactions t ON t.id_tx = txo.transaction_id
          WHERE t.txid = :prevout_txid
          AND txo.output_index = :prevout_idx
-         ON CONFLICT (transparent_received_output_id, transaction_id) DO NOTHING",
+         ON CONFLICT (transparent_received_output_id, transaction_id)
+         -- The following UPDATE is effectively a no-op, but we perform it anyway so that the
+         -- number of affected rows can be used to determine whether a record existed.
+         DO UPDATE SET transaction_id = :spent_in_tx",
     )?;
-    stmt_mark_transparent_utxo_spent.execute(named_params![
-        ":spent_in_tx": tx_ref.0,
-        ":prevout_txid": outpoint.hash().as_ref(),
-        ":prevout_idx": outpoint.n(),
-    ])?;
+    let affected_rows = stmt_mark_transparent_utxo_spent.execute(spend_params)?;
 
     // Since we know that the output is spent, we no longer need to search for
     // it to find out if it has been spent.
@@ -461,7 +467,24 @@ pub(crate) fn mark_transparent_utxo_spent(
         ":prevout_idx": outpoint.n(),
     ])?;
 
-    Ok(())
+    // If no rows were affected, we know that we don't actually have the output in
+    // `transparent_received_outputs` yet, so we have to record the output as spent
+    // so that when we eventually detect the output, we can create the spend record.
+    if affected_rows == 0 {
+        conn.execute(
+            "INSERT INTO transparent_spend_map (
+                spending_transaction_id,
+                prevout_txid,
+                prevout_output_index
+            )
+            VALUES (:spent_in_tx, :prevout_txid, :prevout_idx)
+            ON CONFLICT (spending_transaction_id, prevout_txid, prevout_output_index)
+            DO NOTHING",
+            spend_params,
+        )?;
+    }
+
+    Ok(affected_rows > 0)
 }
 
 /// Adds the given received UTXO to the datastore.
@@ -494,6 +517,11 @@ pub(crate) fn transaction_data_requests<P: consensus::Parameters>(
     conn: &rusqlite::Connection,
     params: &P,
 ) -> Result<Vec<TransactionDataRequest>, SqliteClientError> {
+    // `lightwalletd` will return an error for `GetTaddressTxids` requests having an end height
+    // greater than the current chain tip height, so we take the chain tip height into account
+    // here in order to make this pothole easier for clients of the API to avoid.
+    let chain_tip_height = super::chain_tip_height(conn)?;
+
     // We cannot construct address-based transaction data requests for the case where we cannot
     // determine the height at which to begin, so we require that either the target height or mined
     // height be set.
@@ -509,11 +537,16 @@ pub(crate) fn transaction_data_requests<P: consensus::Parameters>(
         .query_and_then([], |row| {
             let address = TransparentAddress::decode(params, &row.get::<_, String>(0)?)?;
             let block_range_start = BlockHeight::from(row.get::<_, u32>(1)?);
+            let max_end_height = block_range_start + DEFAULT_TX_EXPIRY_DELTA + 1;
+
             Ok::<TransactionDataRequest, SqliteClientError>(
                 TransactionDataRequest::SpendsFromAddress {
                     address,
                     block_range_start,
-                    block_range_end: Some(block_range_start + DEFAULT_TX_EXPIRY_DELTA + 1),
+                    block_range_end: Some(
+                        chain_tip_height
+                            .map_or(max_end_height, |h| std::cmp::min(h + 1, max_end_height)),
+                    ),
                 },
             )
         })?
@@ -728,6 +761,29 @@ pub(crate) fn put_transparent_output<P: consensus::Parameters>(
 
     let utxo_id = stmt_upsert_transparent_output
         .query_row(sql_args, |row| row.get::<_, i64>(0).map(UtxoId))?;
+
+    // If we have a record of the output already having been spent, then mark it as spent using the
+    // stored reference to the spending transaction.
+    let spending_tx_ref = conn
+        .query_row(
+            "SELECT ts.spending_transaction_id
+             FROM transparent_spend_map ts
+             JOIN transactions t ON t.id_tx = ts.spending_transaction_id
+             WHERE ts.prevout_txid = :prevout_txid
+             AND ts.prevout_output_index = :prevout_idx
+             ORDER BY t.block NULLS LAST LIMIT 1",
+            named_params![
+                ":prevout_txid": outpoint.txid().as_ref(),
+                ":prevout_idx": outpoint.n()
+            ],
+            |row| row.get::<_, i64>(0).map(TxRef),
+        )
+        .optional()?;
+
+    if let Some(spending_transaction_id) = spending_tx_ref {
+        mark_transparent_utxo_spent(conn, spending_transaction_id, outpoint)?;
+    }
+
     Ok(utxo_id)
 }