Documentation updates, fixes, and cleanups

Co-authored-by: Daira Hopwood <[email protected]>

zcash_client_backend/CHANGELOG.md

@@ -85,6 +85,8 @@ and this library adheres to Rust's notion of
     feature flag, has been modified by the addition of a `sapling_tree` property. 
   - `wallet::input_selection`:
     - `Proposal::target_height` (use `Proposal::min_target_height` instead).
+- `zcash_client_backend::data_api::chain::validate_chain` (logic merged into
+  `chain::scan_cached_blocks`.
 - `zcash_client_backend::data_api::chain::error::{ChainError, Cause}` have been 
   replaced by `zcash_client_backend::scanning::ScanError`
 - `zcash_client_backend::wallet::WalletSaplingOutput::{witness, witness_mut}`

zcash_client_backend/src/data_api.rs

@@ -89,6 +89,8 @@ pub trait WalletRead {
     /// tree size information for each block; or else the scan is likely to fail if notes belonging
     /// to the wallet are detected.
     ///
+    /// The returned range(s) may include block heights beyond the current chain tip.
+    ///
     /// [`CompactBlock`]: crate::proto::compact_formats::CompactBlock
     fn suggest_scan_ranges(&self) -> Result<Vec<ScanRange>, Self::Error>;
 
@@ -502,8 +504,8 @@ pub trait WalletWrite: WalletRead {
     /// Updates the wallet's view of the blockchain.
     ///
     /// This method is used to provide the wallet with information about the state of the
-    /// blockchain, and detect any previously scanned that needs to be re-validated before
-    /// proceeding with scanning. It should be called at wallet startup prior to calling
+    /// blockchain, and detect any previously scanned data that needs to be re-validated
+    /// before proceeding with scanning. It should be called at wallet startup prior to calling
     /// [`WalletRead::suggest_scan_ranges`] in order to provide the wallet with the information it
     /// needs to correctly prioritize scanning operations.
     fn update_chain_tip(&mut self, tip_height: BlockHeight) -> Result<(), Self::Error>;

zcash_client_backend/src/data_api/chain.rs

@@ -104,8 +104,7 @@
 //!                 }
 //!             }
 //!
-//!             // Truncation will have updated the suggested scan ranges, so we now
-//!             // re_request
+//!             // In case we updated the suggested scan ranges, now re-request.
 //!             scan_ranges = wallet_db.suggest_scan_ranges().map_err(Error::Wallet)?;
 //!         }
 //!         _ => {
@@ -123,10 +122,13 @@
 //!     // Download the blocks in `scan_range` into the block source. While in this example this
 //!     // step is performed in-line, it's fine for the download of scan ranges to be asynchronous
 //!     // and for the scanner to process the downloaded ranges as they become available in a
-//!     // separate thread.
+//!     // separate thread. The scan ranges should also be broken down into smaller chunks as
+//!     // appropriate, and for ranges with priority `Historic` it can be useful to download and
+//!     // scan the range in reverse order (to discover more recent unspent notes sooner), or from
+//!     // the start and end of the range inwards.
 //!     unimplemented!();
 //!
-//!     // Scan the downloaded blocks,
+//!     // Scan the downloaded blocks.
 //!     let scan_result = scan_cached_blocks(
 //!         &network,
 //!         &block_source,

zcash_client_backend/src/data_api/scanning.rs

@@ -10,13 +10,14 @@ pub enum ScanPriority {
     Scanned,
     /// Block ranges to be scanned to advance the fully-scanned height.
     Historic,
-    /// Block ranges adjacent to wallet open heights.
+    /// Block ranges adjacent to heights at which the user opened the wallet.
     OpenAdjacent,
     /// Blocks that must be scanned to complete note commitment tree shards adjacent to found notes.
     FoundNote,
     /// Blocks that must be scanned to complete the latest note commitment tree shard.
     ChainTip,
-    /// A previously-scanned range that must be verified has highest priority.
+    /// A previously scanned range that must be verified to check it is still in the
+    /// main chain, has highest priority.
     Verify,
 }
 

zcash_client_sqlite/src/chain.rs

@@ -26,10 +26,6 @@ pub mod migrations;
 /// Starting at `from_height`, the `with_row` callback is invoked with each block retrieved from
 /// the backing store. If the `limit` value provided is `None`, all blocks are traversed up to the
 /// maximum height.
-///
-/// # Panics
-///
-/// Panics if the provided `limit` value exceeds the range of a u32
 pub(crate) fn blockdb_with_blocks<F, DbErrT>(
     block_source: &BlockDb,
     from_height: Option<BlockHeight>,
@@ -200,10 +196,6 @@ pub(crate) fn blockmetadb_find_block(
 /// Starting at `from_height`, the `with_row` callback is invoked with each block retrieved from
 /// the backing store. If the `limit` value provided is `None`, all blocks are traversed up to the
 /// maximum height for which metadata is available.
-///
-/// # Panics
-///
-/// Panics if the provided `limit` value exceeds the range of a u32
 #[cfg(feature = "unstable")]
 pub(crate) fn fsblockdb_with_blocks<F, DbErrT>(
     cache: &FsBlockDb,

zcash_client_sqlite/src/wallet.rs

@@ -748,7 +748,7 @@ pub(crate) fn truncate_to_height<P: consensus::Parameters>(
 ) -> Result<(), SqliteClientError> {
     let sapling_activation_height = params
         .activation_height(NetworkUpgrade::Sapling)
-        .expect("Sapling activation height mutst be available.");
+        .expect("Sapling activation height must be available.");
 
     // Recall where we synced up to previously.
     let last_scanned_height = conn.query_row("SELECT MAX(height) FROM blocks", [], |row| {

zcash_client_sqlite/src/wallet/commitment_tree.rs

@@ -772,9 +772,9 @@ pub(crate) fn put_shard_roots<
         return Ok(());
     }
 
-    // We treat the cap as a DEPTH-SHARD_HEIGHT tree so that we can make a batch insertion of
-    // root data using `Position::from(start_index)` as the starting position and treating the
-    // roots as level-0 leaves.
+    // We treat the cap as a tree with `DEPTH - SHARD_HEIGHT` levels, so that we can make a
+    // batch insertion of root data using `Position::from(start_index)` as the starting position
+    // and treating the roots as level-0 leaves.
     #[derive(Clone, Debug, PartialEq, Eq)]
     struct LevelShifter<H, const SHARD_HEIGHT: u8>(H);
     impl<H: Hashable, const SHARD_HEIGHT: u8> Hashable for LevelShifter<H, SHARD_HEIGHT> {

zcash_client_sqlite/src/wallet/init/migrations/shardtree_support.rs

@@ -207,7 +207,8 @@ impl RusqliteMigration for Migration {
             }
         }
 
-        // Establish the scan queue & wallet history table
+        // Establish the scan queue & wallet history table.
+        // block_range_end is exclusive.
         debug!("Creating table for scan queue");
         transaction.execute_batch(
             "CREATE TABLE scan_queue (

zcash_client_sqlite/src/wallet/scanning.rs

@@ -107,7 +107,8 @@ pub(crate) fn suggest_scan_ranges(
 
 // This implements the dominance rule for range priority. If the inserted range's priority is
 // `Verify`, this replaces any existing priority. Otherwise, if the current priority is
-// `Scanned`, this overwrites any priority
+// `Scanned`, it remains as `Scanned`; and if the new priority is `Scanned`, it
+// overrides any existing priority.
 fn update_priority(current: ScanPriority, inserted: ScanPriority) -> ScanPriority {
     match (current, inserted) {
         (_, ScanPriority::Verify) => ScanPriority::Verify,
@@ -129,14 +130,25 @@ fn dominance(current: &ScanPriority, inserted: &ScanPriority, insert: Insert) ->
     }
 }
 
+/// In the comments for each alternative, `()` represents the left range and `[]` represents the right range.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 enum RangeOrdering {
+    /// `(   ) [   ]`
     LeftFirstDisjoint,
+    /// `( [   ) ]`
     LeftFirstOverlap,
+    /// `[ (   ) ]`
     LeftContained,
+    /// ```text
+    /// (   )
+    /// [   ]
+    /// ```
     Equal,
+    /// `( [   ] )`
     RightContained,
+    /// `[ (   ] )`
     RightFirstOverlap,
+    /// `[   ] (   )`
     RightFirstDisjoint,
 }
 
@@ -317,7 +329,7 @@ impl SpanningTree {
             SpanningTree::Parent { span, left, right } => {
                 // TODO: this algorithm always preserves the existing partition point, and does not
                 // do any rebalancing or unification of ranges within the tree; `into_vec`
-                // performes such unification and the tree being unbalanced should be fine given
+                // performs such unification, and the tree being unbalanced should be fine given
                 // the relatively small number of ranges we should ordinarily be concerned with.
                 use RangeOrdering::*;
                 match RangeOrdering::cmp(&span, to_insert.block_range()) {
@@ -425,7 +437,7 @@ pub(crate) fn insert_queue_entries<'a>(
         trace!("Inserting queue entry {}", entry);
         if !entry.is_empty() {
             stmt.execute(named_params![
-                ":block_range_start": u32::from(entry.block_range().start) ,
+                ":block_range_start": u32::from(entry.block_range().start),
                 ":block_range_end": u32::from(entry.block_range().end),
                 ":priority": priority_code(&entry.priority())
             ])?;
@@ -467,7 +479,7 @@ pub(crate) fn replace_queue_entries(
             ":end": u32::from(query_range.end),
         ])?;
 
-        // Iterate over the ranges in the scan queue that overlaps the range that we have
+        // Iterate over the ranges in the scan queue that overlap the range that we have
         // identified as needing to be fully scanned. For each such range add it to the
         // spanning tree (these should all be nonoverlapping ranges, but we might coalesce
         // some in the process).
@@ -552,9 +564,9 @@ pub(crate) fn scan_complete<P: consensus::Parameters>(
             WHERE shard_index = :shard_index",
         )?;
 
-        // if no notes belonging to the wallet were found, so don't need to extend the scanning
+        // if no notes belonging to the wallet were found, we don't need to extend the scanning
         // range suggestions to include the associated subtrees, and our bounds are just the
-        // scanned range
+        // scanned range.
         subtree_bounds
             .map(|(min_idx, max_idx)| {
                 let range_min = if *min_idx > 0 {
@@ -627,13 +639,12 @@ pub(crate) fn update_chain_tip<P: consensus::Parameters>(
     )?;
 
     // Create a scanning range for the fragment of the last shard leading up to new tip.
-    // However, only do so if the start of the shard is at a stable height.
     let shard_entry = shard_start_height
         .filter(|h| h < &chain_end)
         .map(|h| ScanRange::from_parts(h..chain_end, ScanPriority::ChainTip));
 
     // Create scanning ranges to either validate potentially invalid blocks at the wallet's view
-    // of the chain tip,
+    // of the chain tip, or connect the prior tip to the new tip.
     let tip_entry = block_height_extrema(conn)?.map(|(_, prior_tip)| {
         // If we don't have shard metadata, this means we're doing linear scanning, so create a
         // scan range from the prior tip to the current tip with `Historic` priority.
@@ -964,12 +975,12 @@ mod tests {
         let mut db_data = WalletDb::for_path(data_file.path(), tests::network()).unwrap();
         init_wallet_db(&mut db_data, Some(Secret::new(vec![]))).unwrap();
 
-        // Add an account to the wallet
+        // Add an account to the wallet.
         let (dfvk, _taddr) = init_test_accounts_table(&mut db_data);
 
         assert_matches!(
-            // in the following, we don't care what the root hashes are, they just need to be
-            // distinct
+            // In the following, we don't care what the root hashes are, they just need to be
+            // distinct.
             db_data.put_sapling_subtree_roots(
                 0,
                 &[
@@ -1031,7 +1042,7 @@ mod tests {
             Ok(())
         );
 
-        // Verify the that adjacent range needed to make the note spendable has been prioritized
+        // Verify the that adjacent range needed to make the note spendable has been prioritized.
         let sap_active = u32::from(sapling_activation_height());
         assert_matches!(
             db_data.suggest_scan_ranges(),
@@ -1040,7 +1051,7 @@ mod tests {
             ]
         );
 
-        // Check that the scanned range has been properly persisted
+        // Check that the scanned range has been properly persisted.
         assert_matches!(
             suggest_scan_ranges(&db_data.conn, Scanned),
             Ok(scan_ranges) if scan_ranges == vec![
@@ -1049,8 +1060,8 @@ mod tests {
             ]
         );
 
-        // simulate the wallet going offline for a bit, update the chain tip to 30 blocks in the
-        // future
+        // Simulate the wallet going offline for a bit, update the chain tip to 20 blocks in the
+        // future.
         assert_matches!(
             db_data.update_chain_tip(sapling_activation_height() + 340),
             Ok(())
@@ -1066,7 +1077,7 @@ mod tests {
             ]
         );
 
-        // Now simulate a jump ahead more than 100 blocks
+        // Now simulate a jump ahead more than 100 blocks.
         assert_matches!(
             db_data.update_chain_tip(sapling_activation_height() + 450),
             Ok(())