From 843930a15afc6e9eeb34c78d9568dedf0c992a8f Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 13 Jul 2023 18:25:56 +0100 Subject: [PATCH] Documentation updates, fixes, and cleanups Co-authored-by: Daira Hopwood --- zcash_client_backend/CHANGELOG.md | 2 + zcash_client_backend/src/data_api.rs | 6 ++- zcash_client_backend/src/data_api/chain.rs | 10 +++-- zcash_client_backend/src/data_api/scanning.rs | 5 ++- zcash_client_backend/src/scanning.rs | 2 +- zcash_client_sqlite/src/chain.rs | 8 ---- zcash_client_sqlite/src/wallet.rs | 2 +- .../src/wallet/commitment_tree.rs | 6 +-- .../init/migrations/shardtree_support.rs | 3 +- zcash_client_sqlite/src/wallet/scanning.rs | 43 ++++++++++++------- 10 files changed, 49 insertions(+), 38 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index e5c1ae7d77..1d77875dd6 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/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}` diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 4f4c80c869..ac6118e7c8 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/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, 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>; diff --git a/zcash_client_backend/src/data_api/chain.rs b/zcash_client_backend/src/data_api/chain.rs index 54a285eb44..0f6f94545d 100644 --- a/zcash_client_backend/src/data_api/chain.rs +++ b/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, diff --git a/zcash_client_backend/src/data_api/scanning.rs b/zcash_client_backend/src/data_api/scanning.rs index a2be3c3532..34849de8b7 100644 --- a/zcash_client_backend/src/data_api/scanning.rs +++ b/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, } diff --git a/zcash_client_backend/src/scanning.rs b/zcash_client_backend/src/scanning.rs index e49f71331c..302cf18432 100644 --- a/zcash_client_backend/src/scanning.rs +++ b/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) diff --git a/zcash_client_sqlite/src/chain.rs b/zcash_client_sqlite/src/chain.rs index 9091899754..8c8a1a2723 100644 --- a/zcash_client_sqlite/src/chain.rs +++ b/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( block_source: &BlockDb, from_height: Option, @@ -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( cache: &FsBlockDb, diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index b90a061072..df71c47742 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -748,7 +748,7 @@ pub(crate) fn truncate_to_height( ) -> 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| { diff --git a/zcash_client_sqlite/src/wallet/commitment_tree.rs b/zcash_client_sqlite/src/wallet/commitment_tree.rs index 892d8c12ac..4e551829bf 100644 --- a/zcash_client_sqlite/src/wallet/commitment_tree.rs +++ b/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); impl Hashable for LevelShifter { diff --git a/zcash_client_sqlite/src/wallet/init/migrations/shardtree_support.rs b/zcash_client_sqlite/src/wallet/init/migrations/shardtree_support.rs index 14a0c20a65..3abb29a67f 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/shardtree_support.rs +++ b/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 ( diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index ca8756422b..ace9cfca1c 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/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, } @@ -309,7 +321,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()) { @@ -417,7 +429,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()) ])?; @@ -459,7 +471,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). @@ -544,9 +556,9 @@ pub(crate) fn scan_complete( 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 { @@ -619,13 +631,12 @@ pub(crate) fn update_chain_tip( )?; // 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. @@ -956,12 +967,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, &[ @@ -1023,7 +1034,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(), @@ -1032,7 +1043,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![ @@ -1041,8 +1052,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(()) @@ -1058,7 +1069,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(())