From 44abd3450bb19e50c852ff38ef9585b2a2b96e8b Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 9 Aug 2023 17:31:23 +0000 Subject: [PATCH] Require `BlockSource::with_blocks` fail on non-existent `from_height` Previously this was not clearly specified, and the implementations in `zcash_client_sqlite` behaved similarly to when `from_height = None`. Closes zcash/librustzcash#892. --- zcash_client_backend/CHANGELOG.md | 3 +- zcash_client_backend/src/data_api/chain.rs | 2 ++ zcash_client_sqlite/CHANGELOG.md | 8 ++++- zcash_client_sqlite/src/chain.rs | 34 ++++++++++++++++++++++ zcash_client_sqlite/src/error.rs | 3 ++ zcash_client_sqlite/src/lib.rs | 8 +++++ 6 files changed, 56 insertions(+), 2 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index c76b580ea..bcd1b6b5d 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -50,7 +50,8 @@ and this library adheres to Rust's notion of permits the caller to control the starting position of the scan range. In addition, the `limit` parameter is now required and has type `usize`. - `chain::BlockSource::with_blocks` now takes its limit as an `Option` - instead of `Option`. + instead of `Option`. It is also now required to return an error if + `from_height` is set to a block that does not exist in `self`. - A new `CommitmentTree` variant has been added to `data_api::error::Error` - `wallet::{create_spend_to_address, create_proposed_transaction, shield_transparent_funds}` all now require that `WalletCommitmentTrees` be diff --git a/zcash_client_backend/src/data_api/chain.rs b/zcash_client_backend/src/data_api/chain.rs index e6c512517..60d0cff1d 100644 --- a/zcash_client_backend/src/data_api/chain.rs +++ b/zcash_client_backend/src/data_api/chain.rs @@ -199,6 +199,8 @@ pub trait BlockSource { /// `from_height`, applying the provided callback to each block. If `from_height` /// is `None` then scanning will begin at the first available block. /// + /// Returns an error if `from_height` is set to a block that does not exist in `self`. + /// /// * `WalletErrT`: the types of errors produced by the wallet operations performed /// as part of processing each row. /// * `NoteRefT`: the type of note identifiers in the wallet data store, for use in diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index 8b2da642a..02a9f5742 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -25,7 +25,11 @@ and this library adheres to Rust's notion of a note could be spent with fewer than `min_confirmations` confirmations if the wallet did not contain enough observed blocks to satisfy the `min_confirmations` value specified; this situation is now treated as an error. -- A `BlockConflict` variant has been added to `zcash_client_sqlite::error::SqliteClientError` +- `zcash_client_sqlite::error::SqliteClientError` has new error variants: + - `SqliteClientError::BlockConflict` + - `SqliteClientError::CacheMiss` +- `zcash_client_backend::FsBlockDbError` has a new error variant: + - `FsBlockDbError::CacheMiss` - `zcash_client_sqlite::FsBlockDb::write_block_metadata` now overwrites any existing metadata entries that have the same height as a new entry. @@ -39,6 +43,8 @@ and this library adheres to Rust's notion of - Fixed an off-by-one error in the `BlockSource` implementation for the SQLite-backed `BlockDb` block database which could result in blocks being skipped at the start of scan ranges. +- `zcash_client_sqlite::{BlockDb, FsBlockDb}::with_blocks` now return an error + if `from_height` is set to a block height that does not exist in the cache. - `WalletDb::get_transaction` no longer returns an error when called on a transaction that has not yet been mined, unless the transaction's consensus branch ID cannot be determined by other means. diff --git a/zcash_client_sqlite/src/chain.rs b/zcash_client_sqlite/src/chain.rs index 7a2396dfe..c66cde05d 100644 --- a/zcash_client_sqlite/src/chain.rs +++ b/zcash_client_sqlite/src/chain.rs @@ -58,8 +58,20 @@ where ]) .map_err(to_chain_error)?; + // Only look for the `from_height` in the scanned blocks if it is set. + let mut from_height_found = from_height.is_none(); while let Some(row) = rows.next().map_err(to_chain_error)? { let height = BlockHeight::from_u32(row.get(0).map_err(to_chain_error)?); + if !from_height_found { + // We will only perform this check on the first row. + let from_height = from_height.expect("can only reach here if set"); + if from_height != height { + return Err(to_chain_error(SqliteClientError::CacheMiss(from_height))); + } else { + from_height_found = true; + } + } + let data: Vec = row.get(1).map_err(to_chain_error)?; let block = CompactBlock::decode(&data[..]).map_err(to_chain_error)?; if block.height() != height { @@ -73,6 +85,11 @@ where with_row(block)?; } + if !from_height_found { + let from_height = from_height.expect("can only reach here if set"); + return Err(to_chain_error(SqliteClientError::CacheMiss(from_height))); + } + Ok(()) } @@ -260,8 +277,20 @@ where ) .map_err(to_chain_error)?; + // Only look for the `from_height` in the scanned blocks if it is set. + let mut from_height_found = from_height.is_none(); for row_result in rows { let cbr = row_result.map_err(to_chain_error)?; + if !from_height_found { + // We will only perform this check on the first row. + let from_height = from_height.expect("can only reach here if set"); + if from_height != cbr.height { + return Err(to_chain_error(FsBlockDbError::CacheMiss(from_height))); + } else { + from_height_found = true; + } + } + let mut block_file = File::open(cbr.block_file_path(&cache.blocks_dir)).map_err(to_chain_error)?; let mut block_data = vec![]; @@ -282,6 +311,11 @@ where with_block(block)?; } + if !from_height_found { + let from_height = from_height.expect("can only reach here if set"); + return Err(to_chain_error(FsBlockDbError::CacheMiss(from_height))); + } + Ok(()) } diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index dec1aa709..618b1d1c0 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/zcash_client_sqlite/src/error.rs @@ -85,6 +85,8 @@ pub enum SqliteClientError { /// An error occurred in inserting data into or accessing data from one of the wallet's note /// commitment trees. CommitmentTree(ShardTreeError), + + CacheMiss(BlockHeight), } impl error::Error for SqliteClientError { @@ -128,6 +130,7 @@ impl fmt::Display for SqliteClientError { #[cfg(feature = "transparent-inputs")] SqliteClientError::AddressNotRecognized(_) => write!(f, "The address associated with a received txo is not identifiable as belonging to the wallet."), SqliteClientError::CommitmentTree(err) => write!(f, "An error occurred accessing or updating note commitment tree data: {}.", err), + SqliteClientError::CacheMiss(height) => write!(f, "Requested height {} does not exist in the block cache.", height), } } } diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 4a9cc7b44..5cc7cd49b 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -883,6 +883,7 @@ pub enum FsBlockDbError { InvalidBlockstoreRoot(PathBuf), InvalidBlockPath(PathBuf), CorruptedData(String), + CacheMiss(BlockHeight), } #[cfg(feature = "unstable")] @@ -1047,6 +1048,13 @@ impl std::fmt::Display for FsBlockDbError { e, ) } + FsBlockDbError::CacheMiss(height) => { + write!( + f, + "Requested height {} does not exist in the block cache", + height + ) + } } } }