diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index f8dedbe63..801760156 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_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/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..824fdacd5 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/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; + /// 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(()) } 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/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 e1b2550a8..8bacff0de 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/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>(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() + ); + } }