From 586b7e5bb08e850849520f9fa524dda2816efd82 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 29 Aug 2022 21:32:18 +0000 Subject: [PATCH 1/2] zcash_client_backend: Add `WalletWrite::remove_tx` method This is to replace the database mutations in the Android SDK. It is placed behind an `unstable` feature flag until we are satisfied that it is suitable as a general-purpose API (or replace it). --- zcash_client_backend/CHANGELOG.md | 5 +- zcash_client_backend/Cargo.toml | 1 + zcash_client_backend/src/data_api.rs | 11 +++ zcash_client_sqlite/CHANGELOG.md | 2 + zcash_client_sqlite/Cargo.toml | 1 + zcash_client_sqlite/src/lib.rs | 142 +++++++++++++++++++++++++++ 6 files changed, 161 insertions(+), 1 deletion(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index f8dedbe63..fd91c0411 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/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_tx` (behind the `unstable` feature flag). - `zcash_client_backend::proto`: - `actions` field on `compact_formats::CompactTx` - `compact_formats::CompactOrchardAction` diff --git a/zcash_client_backend/Cargo.toml b/zcash_client_backend/Cargo.toml index dd66b62fa..d2a887437 100644 --- a/zcash_client_backend/Cargo.toml +++ b/zcash_client_backend/Cargo.toml @@ -57,6 +57,7 @@ test-dependencies = [ "orchard/test-dependencies", "zcash_primitives/test-dependencies", ] +unstable = [] [lib] bench = false diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index f79a3d08f..01a4e7c0b 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -272,6 +272,12 @@ pub trait WalletWrite: WalletRead { fn store_sent_tx(&mut self, sent_tx: &SentTransaction) -> Result; + /// Removes the specified transaction from the persistent wallet store, if it exists. + /// + /// Any effects of the transaction (such as spending received notes) are unwound. + #[cfg(feature = "unstable")] + fn remove_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 +500,11 @@ pub mod testing { Ok(TxId::from_bytes([0u8; 32])) } + #[cfg(feature = "unstable")] + fn remove_tx(&mut self, _txid: &TxId) -> Result<(), Self::Error> { + Ok(()) + } + fn rewind_to_height(&mut self, _block_height: BlockHeight) -> Result<(), Self::Error> { Ok(()) } diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index b7f3acf95..2884f21c0 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/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 diff --git a/zcash_client_sqlite/Cargo.toml b/zcash_client_sqlite/Cargo.toml index 5f4e68196..8c7555b35 100644 --- a/zcash_client_sqlite/Cargo.toml +++ b/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 diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index e1b2550a8..e30edca4e 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -558,6 +558,59 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> { }) } + #[cfg(feature = "unstable")] + fn remove_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)?; + + // Delete witnesses for received notes that will be deleted. + up.wallet_db.conn.execute_named( + "DELETE FROM sapling_witnesses WHERE note IN ( + SELECT rn.id_note + FROM received_notes rn + WHERE rn.tx = :tx + )", + &[(":tx", &tx_ref)], + )?; + + // 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 +655,7 @@ impl BlockSource for BlockDb { } #[cfg(test)] +#[allow(deprecated)] mod tests { use ff::PrimeField; use group::GroupEncoding; @@ -840,4 +894,92 @@ mod tests { .execute(params![u32::from(cb.height()), cb_bytes,]) .unwrap(); } + + #[cfg(feature = "unstable")] + #[test] + fn remove_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, + 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() + ); + + // Remove the second transaction. + let mut db_ops = db_data.get_update_ops().unwrap(); + let txid2 = cb2.vtx.get(0).unwrap().txid(); + db_ops.remove_tx(&txid2).unwrap(); + + // Account balance should only reflect the first received note. + assert_eq!(get_balance(&db_data, AccountId::from(0)).unwrap(), value); + + // Scanning the cache again won't change the balance, as the wallet believes it + // has already scanned all the blocks in the cache. + scan_cached_blocks(&network(), &db_cache, &mut db_write, None).unwrap(); + assert_eq!(get_balance(&db_data, AccountId::from(0)).unwrap(), value); + + // Rewind the wallet by one block, and scan the cache again. + rewind_to_height(&db_data, sapling_activation_height()).unwrap(); + 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() + ); + } } From 4f5d75788307296fa367d656d810054dd34d9caf Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 30 Aug 2022 22:54:58 +0000 Subject: [PATCH 2/2] zcash_client_backend: Change to `WalletWrite::remove_unmined_tx` This fixes a bug in the logic ported from the Android SDK: it was possible to remove a transaction in the middle of a chain, which would cause a long-spent note to become unspent and cause the wallet balance to be over-counted. We now restrict transaction removal to unmined transactions, which is sufficient for the Android SDK use cases. --- zcash_client_backend/CHANGELOG.md | 2 +- zcash_client_backend/src/data_api.rs | 10 ++-- zcash_client_sqlite/src/error.rs | 9 ++++ zcash_client_sqlite/src/lib.rs | 74 +++++++++++++++++++--------- 4 files changed, 68 insertions(+), 27 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index fd91c0411..801760156 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -32,7 +32,7 @@ and this library adheres to Rust's notion of - `RecipientAddress::Unified` - `zcash_client_backend::data_api`: - `WalletRead::get_unified_full_viewing_keys` - - `WalletWrite::remove_tx` (behind the `unstable` feature flag). + - `WalletWrite::remove_unmined_tx` (behind the `unstable` feature flag). - `zcash_client_backend::proto`: - `actions` field on `compact_formats::CompactTx` - `compact_formats::CompactOrchardAction` diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 01a4e7c0b..824fdacd5 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -272,11 +272,13 @@ pub trait WalletWrite: WalletRead { fn store_sent_tx(&mut self, sent_tx: &SentTransaction) -> Result; - /// Removes the specified transaction from the persistent wallet store, if it exists. + /// Removes the specified unmined transaction from the persistent wallet store, if it + /// exists. /// - /// Any effects of the transaction (such as spending received notes) are unwound. + /// 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_tx(&mut self, txid: &TxId) -> Result<(), Self::Error>; + fn remove_unmined_tx(&mut self, txid: &TxId) -> Result<(), Self::Error>; /// Rewinds the wallet database to the specified height. /// @@ -501,7 +503,7 @@ pub mod testing { } #[cfg(feature = "unstable")] - fn remove_tx(&mut self, _txid: &TxId) -> Result<(), Self::Error> { + fn remove_unmined_tx(&mut self, _txid: &TxId) -> Result<(), Self::Error> { Ok(()) } diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index a45a66a08..5d27d4cc0 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/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), diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index e30edca4e..8bacff0de 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -559,20 +559,26 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> { } #[cfg(feature = "unstable")] - fn remove_tx(&mut self, txid: &TxId) -> Result<(), Self::Error> { + 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)?; - // Delete witnesses for received notes that will be deleted. - up.wallet_db.conn.execute_named( - "DELETE FROM sapling_witnesses WHERE note IN ( - SELECT rn.id_note - FROM received_notes rn - WHERE rn.tx = :tx - )", - &[(":tx", &tx_ref)], - )?; + // 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>(0), + )? + .is_some() + { + return Err(SqliteClientError::TransactionIsMined(*txid)); + } // Delete received notes that were outputs of the given transaction. up.wallet_db.conn.execute_named( @@ -897,7 +903,7 @@ mod tests { #[cfg(feature = "unstable")] #[test] - fn remove_tx_reverts_balance() { + fn remove_unmined_tx_reverts_balance() { use secrecy::Secret; use tempfile::NamedTempFile; use zcash_client_backend::data_api::{chain::scan_cached_blocks, WalletWrite}; @@ -905,6 +911,7 @@ mod tests { use crate::{ chain::init::init_cache_database, + error::SqliteClientError, wallet::{get_balance, init::init_wallet_db, rewind_to_height}, }; @@ -959,21 +966,44 @@ mod tests { (value - value2).unwrap() ); - // Remove the second transaction. + // 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(); - db_ops.remove_tx(&txid2).unwrap(); + assert!(matches!( + db_ops.remove_unmined_tx(&txid2).unwrap_err(), + SqliteClientError::TransactionIsMined(x) if x == txid2, + )); - // Account balance should only reflect the first received note. - assert_eq!(get_balance(&db_data, AccountId::from(0)).unwrap(), value); - - // Scanning the cache again won't change the balance, as the wallet believes it - // has already scanned all the blocks in the cache. - scan_cached_blocks(&network(), &db_cache, &mut db_write, None).unwrap(); - assert_eq!(get_balance(&db_data, AccountId::from(0)).unwrap(), value); - - // Rewind the wallet by one block, and scan the cache again. + // 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.