diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 3c2ed1687..c22037021 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -2038,20 +2038,27 @@ pub trait WalletWrite: WalletRead { transactions: &[SentTransaction], ) -> Result<(), Self::Error>; - /// Truncates the wallet database to the specified height. + /// Truncates the wallet database to at most the specified height. /// - /// This method assumes that the state of the underlying data store is - /// consistent up to a particular block height. Since it is possible that - /// a chain reorg might invalidate some stored state, this method must be - /// implemented in order to allow users of this API to "reset" the data store - /// to correctly represent chainstate as of a specified block height. + /// Implementations of this method may choose a lower block height to which the data store will + /// be truncated if it is not possible to truncate exactly to the specified height. Upon + /// successful truncation, this method returns the height to which the data store was actually + /// truncated. /// - /// After calling this method, the block at the given height will be the - /// most recent block and all other operations will treat this block - /// as the chain tip for balance determination purposes. + /// This method assumes that the state of the underlying data store is consistent up to a + /// particular block height. Since it is possible that a chain reorg might invalidate some + /// stored state, this method must be implemented in order to allow users of this API to + /// "reset" the data store to correctly represent chainstate as of at most the requested block + /// height. /// - /// There may be restrictions on heights to which it is possible to truncate. - fn truncate_to_height(&mut self, block_height: BlockHeight) -> Result<(), Self::Error>; + /// After calling this method, the block at the returned height will be the most recent block + /// and all other operations will treat this block as the chain tip for balance determination + /// purposes. + /// + /// There may be restrictions on heights to which it is possible to truncate. Specifically, it + /// will only be possible to truncate to heights at which is is possible to create a witness + /// given the current state of the wallet's note commitment tree. + fn truncate_to_height(&mut self, max_height: BlockHeight) -> Result; /// Reserves the next `n` available ephemeral addresses for the given account. /// This cannot be undone, so as far as possible, errors associated with transaction diff --git a/zcash_client_backend/src/data_api/testing.rs b/zcash_client_backend/src/data_api/testing.rs index 3289a69d6..dd895e578 100644 --- a/zcash_client_backend/src/data_api/testing.rs +++ b/zcash_client_backend/src/data_api/testing.rs @@ -2633,8 +2633,11 @@ impl WalletWrite for MockWalletDb { Ok(()) } - fn truncate_to_height(&mut self, _block_height: BlockHeight) -> Result<(), Self::Error> { - Ok(()) + fn truncate_to_height( + &mut self, + _block_height: BlockHeight, + ) -> Result { + Err(()) } /// Adds a transparent UTXO received by the wallet to the data store. diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index c2461b085..6e13ef765 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -7,12 +7,15 @@ and this library adheres to Rust's notion of ## [Unreleased] +- `zcash_client_sqlite::error::SqliteClientError::RequestedRewindInvalid` + is now a structured variant. + ## [0.11.2] - 2024-08-21 ### Changed - The `v_tx_outputs` view was modified slightly to support older versions of `sqlite`. Queries to the exposed `v_tx_outputs` and `v_transactions` views - are supported for SQLite versions back to `3.19.x`. + are supported for SQLite versions back to `3.19.x`. - `zcash_client_sqlite::wallet::init::WalletMigrationError` has an additional variant, `DatabaseNotSupported`. The `init_wallet_db` function now checks that the sqlite version in use is compatible with the features required by diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index b66d33932..741003b6e 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/zcash_client_sqlite/src/error.rs @@ -12,7 +12,6 @@ use zcash_primitives::{consensus::BlockHeight, transaction::components::amount:: use crate::wallet::commitment_tree; use crate::AccountId; -use crate::PRUNING_DEPTH; #[cfg(feature = "transparent-inputs")] use { @@ -64,8 +63,12 @@ pub enum SqliteClientError { NonSequentialBlocks, /// A requested rewind would violate invariants of the storage layer. The payload returned with - /// this error is (safe rewind height, requested height). - RequestedRewindInvalid(BlockHeight, BlockHeight), + /// this error is (safe rewind height, requested height). If no safe rewind height can be + /// determined, the safe rewind height member will be `None`. + RequestedRewindInvalid { + safe_rewind_height: Option, + requested_height: BlockHeight, + }, /// An error occurred in generating a Zcash address. AddressGeneration(AddressGenerationError), @@ -152,8 +155,12 @@ impl fmt::Display for SqliteClientError { } SqliteClientError::Protobuf(e) => write!(f, "Failed to parse protobuf-encoded record: {}", e), SqliteClientError::InvalidNote => write!(f, "Invalid note"), - SqliteClientError::RequestedRewindInvalid(h, r) => - write!(f, "A rewind must be either of less than {} blocks, or at least back to block {} for your wallet; the requested height was {}.", PRUNING_DEPTH, h, r), + SqliteClientError::RequestedRewindInvalid { safe_rewind_height, requested_height } => write!( + f, + "A rewind for your wallet may only target height {} or greater; the requested height was {}.", + safe_rewind_height.map_or("".to_owned(), |h0| format!("{}", h0)), + requested_height + ), SqliteClientError::DecodingError(e) => write!(f, "{}", e), #[cfg(feature = "transparent-inputs")] SqliteClientError::TransparentDerivation(e) => write!(f, "{:?}", e), diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 124099b06..31e896d4c 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -1335,10 +1335,8 @@ impl WalletWrite for WalletDb }) } - fn truncate_to_height(&mut self, block_height: BlockHeight) -> Result<(), Self::Error> { - self.transactionally(|wdb| { - wallet::truncate_to_height(wdb.conn.0, &wdb.params, block_height) - }) + fn truncate_to_height(&mut self, max_height: BlockHeight) -> Result { + self.transactionally(|wdb| wallet::truncate_to_height(wdb.conn.0, &wdb.params, max_height)) } #[cfg(feature = "transparent-inputs")] diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 651c9a683..0fb1f4c35 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -2105,7 +2105,7 @@ pub(crate) fn get_max_height_hash( } /// Gets the height to which the database must be truncated if any truncation that would remove a -/// number of blocks greater than the pruning height is attempted. +/// number of blocks greater than the note commitment tree pruning depth is attempted. pub(crate) fn get_min_unspent_height( conn: &rusqlite::Connection, ) -> Result, SqliteClientError> { @@ -2357,34 +2357,86 @@ pub(crate) fn set_transaction_status( Ok(()) } -/// Truncates the database to the given height. +/// Truncates the database to at most the given height. /// /// If the requested height is greater than or equal to the height of the last scanned /// block, this function does nothing. /// /// This should only be executed inside a transactional context. +/// +/// Returns the block height to which the database was truncated. pub(crate) fn truncate_to_height( conn: &rusqlite::Transaction, params: &P, - block_height: BlockHeight, -) -> Result<(), SqliteClientError> { - let sapling_activation_height = params - .activation_height(NetworkUpgrade::Sapling) - .expect("Sapling activation height must be available."); + max_height: BlockHeight, +) -> Result { + // Determine a checkpoint to which we can rewind, if any. + #[cfg(not(feature = "orchard"))] + let truncation_height_query = r#" + SELECT MAX(height) FROM blocks + JOIN sapling_tree_checkpoints ON checkpoint_id = blocks.height + WHERE blocks.height <= :block_height + "#; + + #[cfg(feature = "orchard")] + let truncation_height_query = r#" + SELECT MAX(height) FROM blocks + JOIN sapling_tree_checkpoints sc ON sc.checkpoint_id = blocks.height + JOIN orchard_tree_checkpoints oc ON oc.checkpoint_id = blocks.height + WHERE blocks.height <= :block_height + "#; + + let truncation_height = conn + .query_row( + truncation_height_query, + named_params! {":block_height": u32::from(max_height)}, + |row| row.get::<_, Option>(0), + ) + .optional()? + .flatten() + .map_or_else( + || { + // If we don't have a checkpoint at a height less than or equal to the requested + // truncation height, query for the minimum height to which it's possible for us to + // truncate so that we can report it to the caller. + #[cfg(not(feature = "orchard"))] + let min_checkpoint_height_query = + "SELECT MIN(checkpoint_id) FROM sapling_tree_checkpoints"; + #[cfg(feature = "orchard")] + let min_checkpoint_height_query = "SELECT MIN(checkpoint_id) + FROM sapling_tree_checkpoints sc + JOIN orchard_tree_checkpoints oc + ON oc.checkpoint_id = sc.checkpoint_id"; + + let min_truncation_height = conn + .query_row(min_checkpoint_height_query, [], |row| { + row.get::<_, Option>(0) + }) + .optional()? + .flatten() + .map(BlockHeight::from); + + Err(SqliteClientError::RequestedRewindInvalid { + safe_rewind_height: min_truncation_height, + requested_height: max_height, + }) + }, + |h| Ok(BlockHeight::from(h)), + )?; - // Recall where we synced up to previously. let last_scanned_height = conn.query_row("SELECT MAX(height) FROM blocks", [], |row| { - row.get::<_, Option>(0) - .map(|opt| opt.map_or_else(|| sapling_activation_height - 1, BlockHeight::from)) - })?; + let h = row.get::<_, Option>(0)?; - if block_height < last_scanned_height - PRUNING_DEPTH { - if let Some(h) = get_min_unspent_height(conn)? { - if block_height > h { - return Err(SqliteClientError::RequestedRewindInvalid(h, block_height)); - } - } - } + Ok(h.map_or_else( + || { + params + .activation_height(NetworkUpgrade::Sapling) + .expect("Sapling activation height must be available.") + - 1 + }, + BlockHeight::from, + )) + })?; // Delete from the scanning queue any range with a start height greater than the // truncation height, and then truncate any remaining range by setting the end @@ -2393,13 +2445,13 @@ pub(crate) fn truncate_to_height( conn.execute( "DELETE FROM scan_queue WHERE block_range_start >= :new_end_height", - named_params![":new_end_height": u32::from(block_height + 1)], + named_params![":new_end_height": u32::from(truncation_height + 1)], )?; conn.execute( "UPDATE scan_queue SET block_range_end = :new_end_height WHERE block_range_end > :new_end_height", - named_params![":new_end_height": u32::from(block_height + 1)], + named_params![":new_end_height": u32::from(truncation_height + 1)], )?; // Mark transparent utxos as un-mined. Since the TXO is now not mined, it would ideally be @@ -2413,7 +2465,7 @@ pub(crate) fn truncate_to_height( FROM transactions tx WHERE tx.id_tx = transaction_id AND max_observed_unspent_height > :height", - named_params![":height": u32::from(block_height)], + named_params![":height": u32::from(truncation_height)], )?; // Un-mine transactions. This must be done outside of the last_scanned_height check because @@ -2422,32 +2474,32 @@ pub(crate) fn truncate_to_height( "UPDATE transactions SET block = NULL, mined_height = NULL, tx_index = NULL WHERE mined_height > :height", - named_params![":height": u32::from(block_height)], + named_params![":height": u32::from(truncation_height)], )?; // If we're removing scanned blocks, we need to truncate the note commitment tree and remove // affected block records from the database. - if block_height < last_scanned_height { + if truncation_height < last_scanned_height { // Truncate the note commitment trees let mut wdb = WalletDb { conn: SqlTransaction(conn), params: params.clone(), }; wdb.with_sapling_tree_mut(|tree| { - tree.truncate_removing_checkpoint(&block_height)?; + tree.truncate_removing_checkpoint(&truncation_height)?; // We do want a checkpoint preserved at the end of the block, but it should have no // other data associated with it. TODO: `truncate_removing_checkpoint` is an awkward // API to work with, and should be replaced with `truncate_to_checkpoint`. - tree.checkpoint(block_height)?; + tree.checkpoint(truncation_height)?; Ok::<_, SqliteClientError>(()) })?; #[cfg(feature = "orchard")] wdb.with_orchard_tree_mut(|tree| { - tree.truncate_removing_checkpoint(&block_height)?; + tree.truncate_removing_checkpoint(&truncation_height)?; // We do want a checkpoint preserved at the end of the block, but it should have no // other data associated with it. TODO: `truncate_removing_checkpoint` is an awkward // API to work with, and should be replaced with `truncate_to_checkpoint`. - tree.checkpoint(block_height)?; + tree.checkpoint(truncation_height)?; Ok::<_, SqliteClientError>(()) })?; @@ -2461,7 +2513,7 @@ pub(crate) fn truncate_to_height( // Now that they aren't depended on, delete un-mined blocks. conn.execute( "DELETE FROM blocks WHERE height > ?", - [u32::from(block_height)], + [u32::from(truncation_height)], )?; // Delete from the nullifier map any entries with a locator referencing a block @@ -2469,11 +2521,11 @@ pub(crate) fn truncate_to_height( conn.execute( "DELETE FROM tx_locator_map WHERE block_height > :block_height", - named_params![":block_height": u32::from(block_height)], + named_params![":block_height": u32::from(truncation_height)], )?; } - Ok(()) + Ok(truncation_height) } /// Returns a vector with the IDs of all accounts known to this wallet. diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 6d95ae239..1e50c7e8e 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -197,7 +197,7 @@ fn sqlite_client_error_to_wallet_migration_error(e: SqliteClientError) -> Wallet SqliteClientError::TableNotEmpty => unreachable!("wallet already initialized"), SqliteClientError::BlockConflict(_) | SqliteClientError::NonSequentialBlocks - | SqliteClientError::RequestedRewindInvalid(_, _) + | SqliteClientError::RequestedRewindInvalid { .. } | SqliteClientError::KeyDerivationError(_) | SqliteClientError::AccountIdDiscontinuity | SqliteClientError::AccountIdOutOfRange