Merge pull request #893 from zcash/scanning-bugfixes

Scanning bugfixes

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<usize>`
-    instead of `Option<u32>`.
+    instead of `Option<u32>`. 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

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
@@ -300,7 +302,7 @@ where
                 return Err(Error::Scan(scan_error));
             }
 
-            if from_height == BlockHeight::from(0) {
+            if block.height() == BlockHeight::from(0) {
                 // We can always derive a valid `continuity_check_metadata` for the
                 // genesis block, even if the block source doesn't have
                 // `sapling_commitment_tree_size`. So briefly set it to a dummy value that

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.

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<u8> = 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(())
 }
 

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<commitment_tree::Error>),
+
+    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),
         }
     }
 }

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
+                )
+            }
         }
     }
 }