From 054bcc7384e5371cb57c2725fec3d2bf3f68a579 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 | 2 ++ zcash_client_backend/src/data_api/chain.rs | 10 +++++---- zcash_client_backend/src/data_api/scanning.rs | 5 +++-- zcash_client_sqlite/src/chain.rs | 8 ------- zcash_client_sqlite/src/wallet.rs | 2 +- .../init/migrations/shardtree_support.rs | 3 ++- zcash_client_sqlite/src/wallet/scanning.rs | 22 ++++++++++++++----- 8 files changed, 33 insertions(+), 21 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..7f38e8ff61 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>; 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 8d7e1bca5c..d677e45b2c 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_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/init/migrations/shardtree_support.rs b/zcash_client_sqlite/src/wallet/init/migrations/shardtree_support.rs index bb478d7cb5..70fb894620 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/shardtree_support.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/shardtree_support.rs @@ -203,7 +203,8 @@ impl RusqliteMigration for Migration { } } - // Establish the scan queue & wallet history table + // Establish the scan queue & wallet history table. + // block_range_end is exclusive. transaction.execute_batch( "CREATE TABLE scan_queue ( block_range_start INTEGER NOT NULL, diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index b3d7acf755..0c0d7af53c 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 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()) ])?; @@ -633,7 +645,7 @@ pub(crate) fn update_chain_tip( .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. @@ -1049,7 +1061,7 @@ 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 assert_matches!( db_data.update_chain_tip(sapling_activation_height() + 340),