Documentation updates, fixes, and cleanups

Co-authored-by: Daira Hopwood <daira@jacaranda.org>

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
+    /// blockchain, and detect any previously scanned data that needs to be re-validated
-    /// proceeding with scanning. It should be called at wallet startup prior to calling
+    /// 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
+//!             // In case we updated the suggested scan ranges, now re-request.
-//!             // 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_backend/src/scanning.rs

@@ -175,7 +175,7 @@ impl fmt::Display for ScanError {
                 at_height
             ),
             BlockHeightDiscontinuity { prev_height, new_height } => {
-                write!(f, "Block height discontinuity at height {}; next height is : {}", prev_height, new_height)
+                write!(f, "Block height discontinuity at height {}; previous height was: {}", new_height, prev_height)
             }
             TreeSizeMismatch { protocol, at_height, given, computed } => {
                 write!(f, "The {:?} note commitment tree size provided by a compact block did not match the expected size at height {}; given {}, expected {}", protocol, at_height, given, computed)

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
+    // We treat the cap as a tree with `DEPTH - SHARD_HEIGHT` levels, so that we can make a
-    // root data using `Position::from(start_index)` as the starting position and treating the
+    // batch insertion of root data using `Position::from(start_index)` as the starting position
-    // roots as level-0 leaves.
+    // 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 dominance(current: &ScanPriority, inserted: &ScanPriority, insert: Insert) -> Dominance {
     match (current.cmp(inserted), (current, inserted)) {
         (Ordering::Equal, _) => Dominance::Equal,
@@ -118,14 +119,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,
 }
 
@@ -307,10 +319,11 @@ impl SpanningTree {
         match self {
             SpanningTree::Leaf(cur) => Self::from_joined(insert(cur, to_insert)),
             SpanningTree::Parent { span, left, right } => {
-                // TODO: this algorithm always preserves the existing partition point, and does not
+                // This algorithm always preserves the existing partition point, and does not do
-                // do any rebalancing or unification of ranges within the tree; `into_vec`
+                // any rebalancing or unification of ranges within the tree. This should be okay
-                // performes such unification and the tree being unbalanced should be fine given
+                // because `into_vec` performs such unification, and the tree being unbalanced
-                // the relatively small number of ranges we should ordinarily be concerned with.
+                // 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()) {
                     LeftFirstDisjoint => {
@@ -459,7 +472,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).
@@ -554,9 +567,9 @@ pub(crate) fn scan_complete<P: consensus::Parameters>(
                 .flatten())
         };
 
-        // 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 {
@@ -617,13 +630,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.
@@ -634,7 +646,7 @@ pub(crate) fn update_chain_tip<P: consensus::Parameters>(
             // and not subject to being reorg'ed.
             let stable_height = new_tip.saturating_sub(PRUNING_DEPTH);
 
-            // if the wallet's prior tip is above the stable height, prioritize the range between
+            // If the wallet's prior tip is above the stable height, prioritize the range between
             // it and the new tip as `ChainTip`. Otherwise, prioritize the `VALIDATION_DEPTH`
             // blocks above the wallet's prior tip as `Verify`. Since `scan_cached_blocks`
             // retrieves the metadata for the block being connected to, the connectivity to the
@@ -954,12 +966,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
+            // In the following, we don't care what the root hashes are, they just need to be
-            // distinct
+            // distinct.
             db_data.put_sapling_subtree_roots(
                 0,
                 &[
@@ -1021,7 +1033,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(),
@@ -1030,7 +1042,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![
@@ -1039,8 +1051,8 @@ mod tests {
             ]
         );
 
-        // simulate the wallet going offline for a bit, update the chain tip to 30 blocks in the
+        // Simulate the wallet going offline for a bit, update the chain tip to 20 blocks in the
-        // future
+        // future.
         assert_matches!(
             db_data.update_chain_tip(sapling_activation_height() + 340),
             Ok(())
@@ -1056,7 +1068,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(())