Merge pull request #621 from zcash/sqlite-remove-tx

zcash_client_backend: Add `WalletWrite::remove_unmined_tx` method

zcash_client_backend/CHANGELOG.md

@@ -26,10 +26,13 @@ and this library adheres to Rust's notion of
   of a `zcash_client_backend::zip321::TransactionRequest` value.
   This facilitates the implementation of ZIP 321 support in wallets and
   provides substantially greater flexibility in transaction creation.
+- An `unstable` feature flag; this is added to parts of the API that may change
+  in any release.
 - `zcash_client_backend::address`:
   - `RecipientAddress::Unified`
 - `zcash_client_backend::data_api`:
-    `WalletRead::get_unified_full_viewing_keys`
+  - `WalletRead::get_unified_full_viewing_keys`
+  - `WalletWrite::remove_unmined_tx` (behind the `unstable` feature flag).
 - `zcash_client_backend::proto`:
   - `actions` field on `compact_formats::CompactTx`
   - `compact_formats::CompactOrchardAction`

zcash_client_backend/Cargo.toml

@@ -57,6 +57,7 @@ test-dependencies = [
     "orchard/test-dependencies",
     "zcash_primitives/test-dependencies",
 ]
+unstable = []
 
 [lib]
 bench = false

zcash_client_backend/src/data_api.rs

@@ -272,6 +272,14 @@ pub trait WalletWrite: WalletRead {
 
     fn store_sent_tx(&mut self, sent_tx: &SentTransaction) -> Result<Self::TxRef, Self::Error>;
 
+    /// Removes the specified unmined transaction from the persistent wallet store, if it
+    /// exists.
+    ///
+    /// Returns an error if the specified transaction has been mined. To remove a mined
+    /// transaction, first use [`WalletWrite::rewind_to_height`] to unmine it.
+    #[cfg(feature = "unstable")]
+    fn remove_unmined_tx(&mut self, txid: &TxId) -> Result<(), Self::Error>;
+
     /// Rewinds the wallet database to the specified height.
     ///
     /// This method assumes that the state of the underlying data store is
@@ -494,6 +502,11 @@ pub mod testing {
             Ok(TxId::from_bytes([0u8; 32]))
         }
 
+        #[cfg(feature = "unstable")]
+        fn remove_unmined_tx(&mut self, _txid: &TxId) -> Result<(), Self::Error> {
+            Ok(())
+        }
+
         fn rewind_to_height(&mut self, _block_height: BlockHeight) -> Result<(), Self::Error> {
             Ok(())
         }

zcash_client_sqlite/CHANGELOG.md

@@ -16,6 +16,8 @@ and this library adheres to Rust's notion of
     transparent address decoding.
   - `SqliteClientError::RequestedRewindInvalid`, to report when requested
     rewinds exceed supported bounds.
+- An `unstable` feature flag; this is added to parts of the API that may change
+  in any release. It enables `zcash_client_backend`'s `unstable` feature flag.
 
 ### Changed
 - Various **BREAKING CHANGES** have been made to the database tables. These will

zcash_client_sqlite/Cargo.toml

@@ -38,6 +38,7 @@ zcash_proofs = { version = "0.7", path = "../zcash_proofs" }
 mainnet = []
 test-dependencies = ["zcash_client_backend/test-dependencies"]
 transparent-inputs = ["zcash_client_backend/transparent-inputs"]
+unstable = ["zcash_client_backend/unstable"]
 
 [lib]
 bench = false

zcash_client_sqlite/src/error.rs

@@ -11,6 +11,9 @@ use zcash_primitives::consensus::BlockHeight;
 
 use crate::{NoteId, PRUNING_HEIGHT};
 
+#[cfg(feature = "unstable")]
+use zcash_primitives::transaction::TxId;
+
 /// The primary error type for the SQLite wallet backend.
 #[derive(Debug)]
 pub enum SqliteClientError {
@@ -30,6 +33,10 @@ pub enum SqliteClientError {
     /// Illegal attempt to reinitialize an already-initialized wallet database.
     TableNotEmpty,
 
+    /// A transaction that has already been mined was requested for removal.
+    #[cfg(feature = "unstable")]
+    TransactionIsMined(TxId),
+
     /// A Bech32-encoded key or address decoding error
     Bech32DecodeError(Bech32DecodeError),
 
@@ -85,6 +92,8 @@ impl fmt::Display for SqliteClientError {
             SqliteClientError::Base58(e) => write!(f, "{}", e),
             SqliteClientError::TransparentAddress(e) => write!(f, "{}", e),
             SqliteClientError::TableNotEmpty => write!(f, "Table is not empty"),
+            #[cfg(feature = "unstable")]
+            SqliteClientError::TransactionIsMined(txid) => write!(f, "Cannot remove transaction {} which has already been mined", txid),
             SqliteClientError::DbError(e) => write!(f, "{}", e),
             SqliteClientError::Io(e) => write!(f, "{}", e),
             SqliteClientError::InvalidMemo(e) => write!(f, "{}", e),

zcash_client_sqlite/src/lib.rs

@@ -558,6 +558,65 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> {
         })
     }
 
+    #[cfg(feature = "unstable")]
+    fn remove_unmined_tx(&mut self, txid: &TxId) -> Result<(), Self::Error> {
+        self.transactionally(|up| {
+            // Find the transaction to be deleted.
+            let tx_ref = up.stmt_select_tx_ref(txid)?;
+
+            // Check that the transaction has not been mined (otherwise deleting it would
+            // require deleting child transactions that we haven't been authorized to
+            // remove).
+            if up
+                .wallet_db
+                .conn
+                .query_row_named(
+                    "SELECT block FROM transactions WHERE id_tx = :tx",
+                    &[(":tx", &tx_ref)],
+                    |row| row.get::<_, Option<u32>>(0),
+                )?
+                .is_some()
+            {
+                return Err(SqliteClientError::TransactionIsMined(*txid));
+            }
+
+            // Delete received notes that were outputs of the given transaction.
+            up.wallet_db.conn.execute_named(
+                "DELETE FROM received_notes WHERE tx = :tx",
+                &[(":tx", &tx_ref)],
+            )?;
+
+            // Unmine received notes that were inputs to the given transaction.
+            up.wallet_db.conn.execute_named(
+                "UPDATE received_notes SET spent = null WHERE spent = :tx",
+                &[(":tx", &tx_ref)],
+            )?;
+
+            // We don't normally want to delete sent notes, as this can contain data that
+            // is not recoverable from the chain. However, given that the caller wants to
+            // delete the transaction in which these notes were sent, we cannot keep these
+            // database entries around.
+            up.wallet_db
+                .conn
+                .execute_named("DELETE FROM sent_notes WHERE tx = :tx", &[(":tx", &tx_ref)])?;
+
+            // Delete the UTXOs because these are effectively cached and we don't have a
+            // good way of knowing whether they're spent.
+            up.wallet_db.conn.execute_named(
+                "DELETE FROM utxos WHERE spent_in_tx = :tx",
+                &[(":tx", &tx_ref)],
+            )?;
+
+            // Now that it isn't depended on, delete the transaction.
+            up.wallet_db.conn.execute_named(
+                "DELETE FROM transactions WHERE id_tx = :tx",
+                &[(":tx", &tx_ref)],
+            )?;
+
+            Ok(())
+        })
+    }
+
     fn rewind_to_height(&mut self, block_height: BlockHeight) -> Result<(), Self::Error> {
         wallet::rewind_to_height(self.wallet_db, block_height)
     }
@@ -602,6 +661,7 @@ impl BlockSource for BlockDb {
 }
 
 #[cfg(test)]
+#[allow(deprecated)]
 mod tests {
     use ff::PrimeField;
     use group::GroupEncoding;
@@ -840,4 +900,116 @@ mod tests {
             .execute(params![u32::from(cb.height()), cb_bytes,])
             .unwrap();
     }
+
+    #[cfg(feature = "unstable")]
+    #[test]
+    fn remove_unmined_tx_reverts_balance() {
+        use secrecy::Secret;
+        use tempfile::NamedTempFile;
+        use zcash_client_backend::data_api::{chain::scan_cached_blocks, WalletWrite};
+        use zcash_primitives::zip32::ExtendedSpendingKey;
+
+        use crate::{
+            chain::init::init_cache_database,
+            error::SqliteClientError,
+            wallet::{get_balance, init::init_wallet_db, rewind_to_height},
+        };
+
+        let cache_file = NamedTempFile::new().unwrap();
+        let db_cache = BlockDb::for_path(cache_file.path()).unwrap();
+        init_cache_database(&db_cache).unwrap();
+
+        let data_file = NamedTempFile::new().unwrap();
+        let mut db_data = WalletDb::for_path(data_file.path(), network()).unwrap();
+        init_wallet_db(&mut db_data, Some(Secret::new(vec![]))).unwrap();
+
+        // Add an account to the wallet.
+        let (dfvk, _taddr) = init_test_accounts_table(&db_data);
+
+        // Account balance should be zero.
+        assert_eq!(
+            get_balance(&db_data, AccountId::from(0)).unwrap(),
+            Amount::zero()
+        );
+
+        // Create a fake CompactBlock sending value to the address.
+        let value = Amount::from_u64(5).unwrap();
+        let (cb, nf) = fake_compact_block(
+            sapling_activation_height(),
+            BlockHash([0; 32]),
+            &dfvk,
+            value,
+        );
+        insert_into_cache(&db_cache, &cb);
+
+        // Create a second fake CompactBlock spending value from the address.
+        let extsk2 = ExtendedSpendingKey::master(&[0]);
+        let to2 = extsk2.default_address().1;
+        let value2 = Amount::from_u64(2).unwrap();
+        let cb2 = fake_compact_block_spending(
+            sapling_activation_height() + 1,
+            cb.hash(),
+            (nf, value),
+            &dfvk,
+            to2,
+            value2,
+        );
+        insert_into_cache(&db_cache, &cb2);
+
+        // Scan the cache.
+        let mut db_write = db_data.get_update_ops().unwrap();
+        scan_cached_blocks(&network(), &db_cache, &mut db_write, None).unwrap();
+
+        // Account balance should equal the change.
+        assert_eq!(
+            get_balance(&db_data, AccountId::from(0)).unwrap(),
+            (value - value2).unwrap()
+        );
+
+        // Attempting to remove the first transaction should fail because it is mined.
+        let mut db_ops = db_data.get_update_ops().unwrap();
+        let txid = cb.vtx.get(0).unwrap().txid();
+        assert!(matches!(
+            db_ops.remove_unmined_tx(&txid).unwrap_err(),
+            SqliteClientError::TransactionIsMined(x) if x == txid,
+        ));
+
+        // Attempting to remove the second transaction should fail because it is mined.
+        let txid2 = cb2.vtx.get(0).unwrap().txid();
+        assert!(matches!(
+            db_ops.remove_unmined_tx(&txid2).unwrap_err(),
+            SqliteClientError::TransactionIsMined(x) if x == txid2,
+        ));
+
+        // Rewind the wallet by one block, to unmine the second transaction.
+        rewind_to_height(&db_data, sapling_activation_height()).unwrap();
+
+        // Account balance should be zero: the first transaction is spent, but the second
+        // transaction is unmined.
+        assert_eq!(
+            get_balance(&db_data, AccountId::from(0)).unwrap(),
+            Amount::zero()
+        );
+
+        // Attempting to remove the first transaction should still fail.
+        assert!(matches!(
+            db_ops.remove_unmined_tx(&txid).unwrap_err(),
+            SqliteClientError::TransactionIsMined(x) if x == txid,
+        ));
+
+        // The second transaction can be removed.
+        db_ops.remove_unmined_tx(&txid2).unwrap();
+
+        // Account balance should now reflect the first received note.
+        assert_eq!(get_balance(&db_data, AccountId::from(0)).unwrap(), value);
+
+        // Scan the cache again.
+        scan_cached_blocks(&network(), &db_cache, &mut db_write, None).unwrap();
+
+        // Account balance should once again equal the change.
+        assert_eq!(
+            get_balance(&db_data, AccountId::from(0)).unwrap(),
+            (value - value2).unwrap()
+        );
+    }
 }