Make block height argument to `BlockSource::with_blocks` optional.

Previously, if a caller wanted to use a block source to perform
scanning from the first available block, they would have to guess
at the block height to start from. Changing this to an optional
argument makes this explicit.

zcash_client_backend/src/data_api/chain.rs

@@ -88,7 +88,7 @@ use std::convert::Infallible;
 
 use zcash_primitives::{
     block::BlockHash,
-    consensus::{self, BlockHeight, NetworkUpgrade},
+    consensus::{self, BlockHeight},
     merkle_tree::CommitmentTree,
     sapling::{note_encryption::PreparedIncomingViewingKey, Nullifier},
     zip32::Scope,
@@ -111,7 +111,8 @@ pub trait BlockSource {
     type Error;
 
     /// Scan the specified `limit` number of blocks from the blockchain, starting at
-    /// `from_height`, applying the provided callback to each block.
+    /// `from_height`, applying the provided callback to each block. If `from_height`
+    /// is `None` then scanning will begin at the first available block.
     ///
     /// * `WalletErrT`: the types of errors produced by the wallet operations performed
     ///   as part of processing each row.
@@ -119,7 +120,7 @@ pub trait BlockSource {
     ///   reporting errors related to specific notes.
     fn with_blocks<F, WalletErrT, NoteRefT>(
         &self,
-        from_height: BlockHeight,
+        from_height: Option<BlockHeight>,
         limit: Option<u32>,
         with_row: F,
     ) -> Result<(), error::Error<WalletErrT, Self::Error, NoteRefT>>
@@ -147,7 +148,7 @@ pub trait BlockSource {
 /// - `limit` Maximum number of blocks that should be validated in this call.
 ///
 /// Returns:
-/// - `Ok((BlockHeight, BlockHash))` if the combined chain is valid up to the given height
+/// - `Ok(())` if the combined chain is valid up to the given height
 /// and block hash.
 /// - `Err(Error::Chain(cause))` if the combined chain is invalid.
 /// - `Err(e)` if there was an error during validation unrelated to chain validity.
@@ -155,9 +156,9 @@ pub trait BlockSource {
 /// This function does not mutate either of the databases.
 pub fn validate_chain<BlockSourceT>(
     block_source: &BlockSourceT,
-    validate_from: (BlockHeight, BlockHash),
+    mut validate_from: Option<(BlockHeight, BlockHash)>,
     limit: Option<u32>,
-) -> Result<(BlockHeight, BlockHash), Error<Infallible, BlockSourceT::Error, Infallible>>
+) -> Result<(), Error<Infallible, BlockSourceT::Error, Infallible>>
 where
     BlockSourceT: BlockSource,
 {
@@ -166,20 +167,26 @@ where
     // source at the `validate_from` height, which can then be used to verify chain integrity by
     // comparing against the `validate_from` hash.
 
-    let (mut valid_height, mut valid_hash) = validate_from;
+    block_source.with_blocks::<_, Infallible, Infallible>(
-    block_source
+        validate_from.map(|(h, _)| h),
-        .with_blocks::<_, Infallible, Infallible>(valid_height, limit, move |block| {
+        limit,
-            if block.height() != valid_height + 1 {
+        move |block| {
-                Err(ChainError::block_height_discontinuity(valid_height + 1, block.height()).into())
+            if let Some((valid_height, valid_hash)) = validate_from {
-            } else if block.prev_hash() != valid_hash {
+                if block.height() != valid_height + 1 {
-                Err(ChainError::prev_hash_mismatch(block.height()).into())
+                    return Err(ChainError::block_height_discontinuity(
-            } else {
+                        valid_height + 1,
-                valid_height = block.height();
+                        block.height(),
-                valid_hash = block.hash();
+                    )
-                Ok(())
+                    .into());
+                } else if block.prev_hash() != valid_hash {
+                    return Err(ChainError::prev_hash_mismatch(block.height()).into());
+                }
             }
-        })
+
-        .map(|_| (valid_height, valid_hash))
+            validate_from = Some((block.height(), block.hash()));
+            Ok(())
+        },
+    )
 }
 
 /// Scans at most `limit` new blocks added to the block source for any transactions received by the
@@ -215,19 +222,11 @@ where
     BlockSourceT: BlockSource,
     DbT: WalletWrite,
 {
-    let sapling_activation_height = params
-        .activation_height(NetworkUpgrade::Sapling)
-        .expect("Sapling activation height is known.");
-
     // Recall where we synced up to previously.
-    // If we have never synced, use sapling activation height to select all cached CompactBlocks.
     let mut last_height = data_db
         .block_height_extrema()
-        .map(|opt| {
+        .map_err(Error::Wallet)?
-            opt.map(|(_, max)| max)
+        .map(|(_, max)| max);
-                .unwrap_or(sapling_activation_height - 1)
-        })
-        .map_err(Error::Wallet)?;
 
     // Fetch the UnifiedFullViewingKeys we are tracking
     let ufvks = data_db
@@ -241,13 +240,21 @@ where
         .collect();
 
     // Get the most recent CommitmentTree
-    let mut tree = data_db
+    let mut tree = last_height.map_or_else(
-        .get_commitment_tree(last_height)
+        || Ok(CommitmentTree::empty()),
-        .map(|t| t.unwrap_or_else(CommitmentTree::empty))
+        |h| {
-        .map_err(Error::Wallet)?;
+            data_db
+                .get_commitment_tree(h)
+                .map(|t| t.unwrap_or_else(CommitmentTree::empty))
+                .map_err(Error::Wallet)
+        },
+    )?;
 
     // Get most recent incremental witnesses for the notes we are tracking
-    let mut witnesses = data_db.get_witnesses(last_height).map_err(Error::Wallet)?;
+    let mut witnesses = last_height.map_or_else(
+        || Ok(vec![]),
+        |h| data_db.get_witnesses(h).map_err(Error::Wallet),
+    )?;
 
     // Get the nullifiers for the notes we are tracking
     let mut nullifiers = data_db.get_nullifiers().map_err(Error::Wallet)?;
@@ -283,12 +290,12 @@ where
             let current_height = block.height();
 
             // Scanned blocks MUST be height-sequential.
-            if current_height != (last_height + 1) {
+            if let Some(h) = last_height {
-                return Err(ChainError::block_height_discontinuity(
+                if current_height != (h + 1) {
-                    last_height + 1,
+                    return Err(
-                    current_height,
+                        ChainError::block_height_discontinuity(h + 1, current_height).into(),
-                )
+                    );
-                .into());
+                }
             }
 
             let block_hash = BlockHash::from_slice(&block.hash);
@@ -359,7 +366,7 @@ where
 
             witnesses.extend(new_witnesses);
 
-            last_height = current_height;
+            last_height = Some(current_height);
 
             Ok(())
         },
@@ -384,7 +391,7 @@ pub mod testing {
 
         fn with_blocks<F, DbErrT, NoteRef>(
             &self,
-            _from_height: BlockHeight,
+            _from_height: Option<BlockHeight>,
             _limit: Option<u32>,
             _with_row: F,
         ) -> Result<(), Error<DbErrT, Infallible, NoteRef>>

zcash_client_sqlite/src/chain.rs

@@ -28,7 +28,7 @@ pub mod migrations;
 /// blocks are traversed up to the maximum height.
 pub(crate) fn blockdb_with_blocks<F, DbErrT, NoteRef>(
     block_source: &BlockDb,
-    last_scanned_height: BlockHeight,
+    last_scanned_height: Option<BlockHeight>,
     limit: Option<u32>,
     mut with_row: F,
 ) -> Result<(), Error<DbErrT, SqliteClientError, NoteRef>>
@@ -51,7 +51,7 @@ where
 
     let mut rows = stmt_blocks
         .query(params![
-            u32::from(last_scanned_height),
+            last_scanned_height.map_or(0u32, u32::from),
             limit.unwrap_or(u32::max_value()),
         ])
         .map_err(to_chain_error)?;
@@ -197,7 +197,7 @@ pub(crate) fn blockmetadb_find_block(
 #[cfg(feature = "unstable")]
 pub(crate) fn fsblockdb_with_blocks<F, DbErrT, NoteRef>(
     cache: &FsBlockDb,
-    last_scanned_height: BlockHeight,
+    last_scanned_height: Option<BlockHeight>,
     limit: Option<u32>,
     mut with_block: F,
 ) -> Result<(), Error<DbErrT, FsBlockDbError, NoteRef>>
@@ -213,16 +213,16 @@ where
         .conn
         .prepare(
             "SELECT height, blockhash, time, sapling_outputs_count, orchard_actions_count
-         FROM compactblocks_meta
+             FROM compactblocks_meta
-         WHERE height > ?
+             WHERE height > ?
-         ORDER BY height ASC LIMIT ?",
+             ORDER BY height ASC LIMIT ?",
         )
         .map_err(to_chain_error)?;
 
     let rows = stmt_blocks
         .query_map(
             params![
-                u32::from(last_scanned_height),
+                last_scanned_height.map_or(0u32, u32::from),
                 limit.unwrap_or(u32::max_value()),
             ],
             |row| {
@@ -319,25 +319,20 @@ mod tests {
         insert_into_cache(&db_cache, &cb);
 
         // Cache-only chain should be valid
-        let validate_chain_result =
+        let validate_chain_result = validate_chain(
-            validate_chain(&db_cache, (fake_block_height, fake_block_hash), Some(1));
+            &db_cache,
-
+            Some((fake_block_height, fake_block_hash)),
-        assert_matches!(
+            Some(1),
-            validate_chain_result,
-            Ok((_fake_block_height, _fake_block_hash))
         );
 
+        assert_matches!(validate_chain_result, Ok(()));
+
         // Scan the cache
         let mut db_write = db_data.get_update_ops().unwrap();
         scan_cached_blocks(&tests::network(), &db_cache, &mut db_write, None).unwrap();
 
         // Data-only chain should be valid
-        validate_chain(
+        validate_chain(&db_cache, db_data.get_max_height_hash().unwrap(), None).unwrap();
-            &db_cache,
-            db_data.get_max_height_hash().unwrap().unwrap(),
-            None,
-        )
-        .unwrap();
 
         // Create a second fake CompactBlock sending more value to the address
         let (cb2, _) = fake_compact_block(
@@ -350,23 +345,13 @@ mod tests {
         insert_into_cache(&db_cache, &cb2);
 
         // Data+cache chain should be valid
-        validate_chain(
+        validate_chain(&db_cache, db_data.get_max_height_hash().unwrap(), None).unwrap();
-            &db_cache,
-            db_data.get_max_height_hash().unwrap().unwrap(),
-            None,
-        )
-        .unwrap();
 
         // Scan the cache again
         scan_cached_blocks(&tests::network(), &db_cache, &mut db_write, None).unwrap();
 
         // Data-only chain should be valid
-        validate_chain(
+        validate_chain(&db_cache, db_data.get_max_height_hash().unwrap(), None).unwrap();
-            &db_cache,
-            db_data.get_max_height_hash().unwrap().unwrap(),
-            None,
-        )
-        .unwrap();
     }
 
     #[test]
@@ -405,12 +390,7 @@ mod tests {
         scan_cached_blocks(&tests::network(), &db_cache, &mut db_write, None).unwrap();
 
         // Data-only chain should be valid
-        validate_chain(
+        validate_chain(&db_cache, db_data.get_max_height_hash().unwrap(), None).unwrap();
-            &db_cache,
-            db_data.get_max_height_hash().unwrap().unwrap(),
-            None,
-        )
-        .unwrap();
 
         // Create more fake CompactBlocks that don't connect to the scanned ones
         let (cb3, _) = fake_compact_block(
@@ -431,11 +411,7 @@ mod tests {
         insert_into_cache(&db_cache, &cb4);
 
         // Data+cache chain should be invalid at the data/cache boundary
-        let val_result = validate_chain(
+        let val_result = validate_chain(&db_cache, db_data.get_max_height_hash().unwrap(), None);
-            &db_cache,
-            db_data.get_max_height_hash().unwrap().unwrap(),
-            None,
-        );
 
         assert_matches!(val_result, Err(Error::Chain(e)) if e.at_height() == sapling_activation_height() + 2);
     }
@@ -476,12 +452,7 @@ mod tests {
         scan_cached_blocks(&tests::network(), &db_cache, &mut db_write, None).unwrap();
 
         // Data-only chain should be valid
-        validate_chain(
+        validate_chain(&db_cache, db_data.get_max_height_hash().unwrap(), None).unwrap();
-            &db_cache,
-            db_data.get_max_height_hash().unwrap().unwrap(),
-            None,
-        )
-        .unwrap();
 
         // Create more fake CompactBlocks that contain a reorg
         let (cb3, _) = fake_compact_block(
@@ -502,11 +473,7 @@ mod tests {
         insert_into_cache(&db_cache, &cb4);
 
         // Data+cache chain should be invalid inside the cache
-        let val_result = validate_chain(
+        let val_result = validate_chain(&db_cache, db_data.get_max_height_hash().unwrap(), None);
-            &db_cache,
-            db_data.get_max_height_hash().unwrap().unwrap(),
-            None,
-        );
 
         assert_matches!(val_result, Err(Error::Chain(e)) if e.at_height() == sapling_activation_height() + 3);
     }

zcash_client_sqlite/src/lib.rs

@@ -723,7 +723,7 @@ impl BlockSource for BlockDb {
 
     fn with_blocks<F, DbErrT, NoteRef>(
         &self,
-        from_height: BlockHeight,
+        from_height: Option<BlockHeight>,
         limit: Option<u32>,
         with_row: F,
     ) -> Result<(), data_api::chain::error::Error<DbErrT, Self::Error, NoteRef>>
@@ -904,7 +904,7 @@ impl BlockSource for FsBlockDb {
 
     fn with_blocks<F, DbErrT, NoteRef>(
         &self,
-        from_height: BlockHeight,
+        from_height: Option<BlockHeight>,
         limit: Option<u32>,
         with_row: F,
     ) -> Result<(), data_api::chain::error::Error<DbErrT, Self::Error, NoteRef>>