From 2584d958ee34341c4bfbf43934234ae9df6e79b0 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 8 Oct 2024 12:17:43 -0600 Subject: [PATCH 01/11] zcash_client_sqlite: `recover_until_height` is an exclusive end. In scan progress computation, `recover_until_height` was incorrectly being treated as an inclusive end, meaning that it was possible for a block's notes to be counted both within the recovery range and within the scanning range. --- zcash_client_sqlite/src/wallet.rs | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index df9d7b3ff3..3e46a900b3 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -849,10 +849,10 @@ fn subtree_scan_progress( fully_scanned_height: BlockHeight, chain_tip_height: BlockHeight, ) -> Result { - let mut stmt_scanned_count_between = conn.prepare_cached(&format!( + let mut stmt_scanned_count_until = conn.prepare_cached(&format!( "SELECT SUM({output_count_col}) FROM blocks - WHERE :start_height <= height AND height <= :end_height", + WHERE :start_height <= height AND height < :end_height", ))?; let mut stmt_scanned_count_from = conn.prepare_cached(&format!( "SELECT SUM({output_count_col}) @@ -875,7 +875,7 @@ fn subtree_scan_progress( // the recover-until height. let recover = recover_until_height .map(|end_height| { - stmt_scanned_count_between.query_row( + stmt_scanned_count_until.query_row( named_params! { ":start_height": u32::from(birthday_height), ":end_height": u32::from(end_height), @@ -894,8 +894,7 @@ fn subtree_scan_progress( let scan = stmt_scanned_count_from.query_row( named_params! { ":start_height": u32::from( - recover_until_height.map(|h| h + 1) - .unwrap_or(birthday_height) + recover_until_height.unwrap_or(birthday_height) ), }, |row| { @@ -932,12 +931,12 @@ fn subtree_scan_progress( }) .transpose()?; - // Get the note commitment tree size as of the end of the recover-until height. + // Get the note commitment tree size as of the start of the recover-until height. let recover_until_size = recover_until_height .map(|end_height| { stmt_start_tree_size .query_row( - named_params![":start_height": u32::from(end_height + 1)], + named_params![":start_height": u32::from(end_height)], |row| row.get::<_, Option>(0), ) .optional() @@ -945,11 +944,10 @@ fn subtree_scan_progress( }) .transpose()?; - // Count the total outputs scanned so far on the birthday side of the - // recover-until height. + // Count the total outputs scanned so far on the birthday side of the recover-until height. let recovered_count = recover_until_height .map(|end_height| { - stmt_scanned_count_between.query_row( + stmt_scanned_count_until.query_row( named_params! { ":start_height": u32::from(birthday_height), ":end_height": u32::from(end_height), @@ -967,11 +965,11 @@ fn subtree_scan_progress( &format!( "SELECT MIN(shard_index) FROM {table_prefix}_tree_shards - WHERE subtree_end_height > :start_height + WHERE subtree_end_height >= :start_height OR subtree_end_height IS NULL", ), named_params! { - ":start_height": u32::from(recover_until_height.unwrap_or(birthday_height) + 1), + ":start_height": u32::from(recover_until_height.unwrap_or(birthday_height)), }, |row| { let min_tree_size = row @@ -1178,7 +1176,7 @@ fn subtree_scan_progress( // Count the total outputs scanned so far on the chain tip side of the // recover-until height. let scanned_count = stmt_scanned_count_from.query_row( - named_params![":start_height": u32::from(recover_until_height.unwrap_or(birthday_height) + 1)], + named_params![":start_height": u32::from(recover_until_height.unwrap_or(birthday_height))], |row| row.get::<_, Option>(0), )?; From acb09112219309001bb2b5ab61bfb9abb9decc0c Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 8 Oct 2024 17:43:13 -0600 Subject: [PATCH 02/11] zcash_client_sqlite: Minor refactoring of `subtree_scan_progress` arguments. --- zcash_client_sqlite/src/wallet.rs | 32 ++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 3e46a900b3..b09c5f47b8 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -840,15 +840,26 @@ pub(crate) struct SubtreeScanProgress; fn subtree_scan_progress( conn: &rusqlite::Connection, params: &P, - table_prefix: &'static str, - output_count_col: &'static str, - shard_height: u8, + shielded_protocol: ShieldedProtocol, pool_activation_height: BlockHeight, birthday_height: BlockHeight, recover_until_height: Option, fully_scanned_height: BlockHeight, chain_tip_height: BlockHeight, ) -> Result { + let (table_prefix, output_count_col, shard_height) = match shielded_protocol { + ShieldedProtocol::Sapling => ( + SAPLING_TABLES_PREFIX, + "sapling_output_count", + SAPLING_SHARD_HEIGHT, + ), + ShieldedProtocol::Orchard => ( + ORCHARD_TABLES_PREFIX, + "orchard_action_count", + ORCHARD_SHARD_HEIGHT, + ), + }; + let mut stmt_scanned_count_until = conn.prepare_cached(&format!( "SELECT SUM({output_count_col}) FROM blocks @@ -1033,11 +1044,10 @@ fn subtree_scan_progress( // Get the tree size at the last scanned height, if known. let last_scanned = block_max_scanned(conn, params)?.and_then(|last_scanned| { - match table_prefix { - SAPLING_TABLES_PREFIX => last_scanned.sapling_tree_size(), + match shielded_protocol { + ShieldedProtocol::Sapling => last_scanned.sapling_tree_size(), #[cfg(feature = "orchard")] - ORCHARD_TABLES_PREFIX => last_scanned.orchard_tree_size(), - _ => unreachable!(), + ShieldedProtocol::Orchard => last_scanned.orchard_tree_size(), } .map(|tree_size| (last_scanned.block_height(), u64::from(tree_size))) }); @@ -1207,9 +1217,7 @@ impl ScanProgress for SubtreeScanProgress { subtree_scan_progress( conn, params, - SAPLING_TABLES_PREFIX, - "sapling_output_count", - SAPLING_SHARD_HEIGHT, + ShieldedProtocol::Sapling, params .activation_height(NetworkUpgrade::Sapling) .expect("Sapling activation height must be available."), @@ -1234,9 +1242,7 @@ impl ScanProgress for SubtreeScanProgress { subtree_scan_progress( conn, params, - ORCHARD_TABLES_PREFIX, - "orchard_action_count", - ORCHARD_SHARD_HEIGHT, + ShieldedProtocol::Orchard, params .activation_height(NetworkUpgrade::Nu5) .expect("NU5 activation height must be available."), From c35c565a8c4b8c38245c0a997baf0b5e3328fab1 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 8 Oct 2024 16:51:27 -0600 Subject: [PATCH 03/11] zcash_client_sqlite: Fix fallback tree size usage in scan progress. We now compute the fallback tree size at the birthday height and the recover-until height separately in order to avoid the mixing of concerns. --- .../src/data_api/testing/pool.rs | 17 +-- zcash_client_sqlite/src/wallet.rs | 126 ++++++++---------- zcash_client_sqlite/src/wallet/scanning.rs | 16 +-- 3 files changed, 70 insertions(+), 89 deletions(-) diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index be0476cd56..bcda45dcca 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -881,20 +881,17 @@ pub fn spend_fails_on_unverified_notes( NonNegativeAmount::ZERO ); - // The account is configured without a recover-until height, so is by definition - // fully recovered, and we count 1 per pool for both numerator and denominator. - let fully_recovered = { - let n = 1; - #[cfg(feature = "orchard")] - let n = n * 2; - Some(Ratio::new(n, n)) - }; + // If none of the wallet's accounts have a recover-until height, then there + // is no recovery phase for the wallet, and therefore the denominator in the + // resulting ratio (the number of notes in the recovery range) is zero. + let no_recovery = Some(Ratio::new(0, 0)); + // Wallet is fully scanned let summary = st.get_wallet_summary(1); assert_eq!( summary.as_ref().and_then(|s| s.recovery_progress()), - fully_recovered, + no_recovery, ); assert_eq!( summary.and_then(|s| s.scan_progress()), @@ -915,7 +912,7 @@ pub fn spend_fails_on_unverified_notes( let summary = st.get_wallet_summary(1); assert_eq!( summary.as_ref().and_then(|s| s.recovery_progress()), - fully_recovered + no_recovery ); assert_eq!( summary.and_then(|s| s.scan_progress()), diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index b09c5f47b8..1c153824f4 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -898,10 +898,11 @@ fn subtree_scan_progress( ) }) .transpose()? - // If none of the wallet's accounts have a recover-until height, then we can't - // (yet) distinguish general scanning from recovery, so treat the wallet as - // fully recovered. - .unwrap_or_else(|| Some(Ratio::new(1, 1))); + // If none of the wallet's accounts have a recover-until height, then there + // is no recovery phase for the wallet, and therefore the denominator in the + // resulting ratio (the number of notes in the recovery range) is zero. + .unwrap_or_else(|| Some(Ratio::new(0, 0))); + let scan = stmt_scanned_count_from.query_row( named_params! { ":start_height": u32::from( @@ -915,9 +916,46 @@ fn subtree_scan_progress( )?; Ok(Progress { scan, recover }) } else { + // In case we didn't have information about the tree size at the recover-until + // height, get the tree size from a nearby subtree. It's fine for this to be + // approximate; it just shifts the boundary between scan and recover progress. + let mut get_tree_size_near = |as_of: BlockHeight| { + stmt_start_tree_size + .query_row( + named_params![":start_height": u32::from(as_of)], + |row| row.get::<_, Option>(0), + ) + .optional() + .map(|opt| opt.flatten()) + .transpose() + .or_else(|| { + conn.query_row( + &format!( + "SELECT MIN(shard_index) + FROM {table_prefix}_tree_shards + WHERE subtree_end_height >= :start_height + OR subtree_end_height IS NULL", + ), + named_params! { + ":start_height": u32::from(as_of), + }, + |row| { + let min_tree_size = row + .get::<_, Option>(0)? + .map(|min_idx| min_idx << shard_height); + Ok(min_tree_size) + }, + ) + .optional() + .map(|opt| opt.flatten()) + .transpose() + }) + .transpose() + }; + // Get the starting note commitment tree size from the wallet birthday, or failing that // from the blocks table. - let start_size = conn + let birthday_size = conn .query_row( &format!( "SELECT birthday_{table_prefix}_tree_size @@ -930,29 +968,14 @@ fn subtree_scan_progress( .optional()? .flatten() .map(Ok) - .or_else(|| { - stmt_start_tree_size - .query_row( - named_params![":start_height": u32::from(birthday_height)], - |row| row.get::<_, Option>(0), - ) - .optional() - .map(|opt| opt.flatten()) - .transpose() - }) + // If we don't have an explicit birthday tree size, find something nearby. + .or_else(|| get_tree_size_near(birthday_height).transpose()) .transpose()?; // Get the note commitment tree size as of the start of the recover-until height. let recover_until_size = recover_until_height - .map(|end_height| { - stmt_start_tree_size - .query_row( - named_params![":start_height": u32::from(end_height)], - |row| row.get::<_, Option>(0), - ) - .optional() - .map(|opt| opt.flatten()) - }) + // Find a tree size near to the recover-until height + .map(get_tree_size_near) .transpose()?; // Count the total outputs scanned so far on the birthday side of the recover-until height. @@ -967,31 +990,6 @@ fn subtree_scan_progress( ) }) .transpose()?; - - // In case we didn't have information about the tree size at the recover-until - // height, get the tree size from a nearby subtree. It's fine for this to be - // approximate; it just shifts the boundary between scan and recover progress. - let min_tree_size = conn - .query_row( - &format!( - "SELECT MIN(shard_index) - FROM {table_prefix}_tree_shards - WHERE subtree_end_height >= :start_height - OR subtree_end_height IS NULL", - ), - named_params! { - ":start_height": u32::from(recover_until_height.unwrap_or(birthday_height)), - }, - |row| { - let min_tree_size = row - .get::<_, Option>(0)? - .map(|min_idx| min_idx << shard_height); - Ok(min_tree_size) - }, - ) - .optional()? - .flatten(); - // If we've scanned the block at the chain tip, we know how many notes are // currently in the tree. let tip_tree_size = match stmt_end_tree_size_at @@ -1164,25 +1162,16 @@ fn subtree_scan_progress( let recover = recovered_count .zip(recover_until_size) .map(|(recovered, end_size)| { - start_size - .or(min_tree_size) - .zip(end_size) - .map(|(start_size, end_size)| { - Ratio::new(recovered.unwrap_or(0), end_size - start_size) - }) + birthday_size.zip(end_size).map(|(start_size, end_size)| { + Ratio::new(recovered.unwrap_or(0), end_size - start_size) + }) }) - // If none of the wallet's accounts have a recover-until height, then we can't - // (yet) distinguish general scanning from recovery, so treat the wallet as - // fully recovered. - .unwrap_or_else(|| Some(Ratio::new(1, 1))); - - let scan = if recover_until_height.map_or(false, |h| h == chain_tip_height) { - // The wallet was likely just created for a recovery from seed, or with an - // imported viewing key. In this state, it is fully synced as there is nothing - // else for us to scan beyond `recover_until_height`; ensure we show 100% - // instead of 0%. - Some(Ratio::new(1, 1)) - } else { + // If none of the wallet's accounts have a recover-until height, then there + // is no recovery phase for the wallet, and therefore the denominator in the + // resulting ratio (the number of notes in the recovery range) is zero. + .unwrap_or_else(|| Some(Ratio::new(0, 0))); + + let scan = { // Count the total outputs scanned so far on the chain tip side of the // recover-until height. let scanned_count = stmt_scanned_count_from.query_row( @@ -1191,8 +1180,7 @@ fn subtree_scan_progress( )?; recover_until_size - .unwrap_or(start_size) - .or(min_tree_size) + .unwrap_or(birthday_size) .zip(tip_tree_size) .map(|(start_size, tip_tree_size)| { Ratio::new(scanned_count.unwrap_or(0), tip_tree_size - start_size) diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index af182003bc..a77700f697 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -1292,20 +1292,16 @@ pub(crate) mod tests { let birthday = account.birthday(); let sap_active = st.sapling_activation_height(); - // The account is configured without a recover-until height, so is by definition - // fully recovered, and we count 1 per pool for both numerator and denominator. - let fully_recovered = { - let n = 1; - #[cfg(feature = "orchard")] - let n = n * 2; - Some(Ratio::new(n, n)) - }; + // If none of the wallet's accounts have a recover-until height, then there + // is no recovery phase for the wallet, and therefore the denominator in the + // resulting ratio (the number of notes in the recovery range) is zero. + let no_recovery = Some(Ratio::new(0, 0)); // We have scan ranges and a subtree, but have scanned no blocks. let summary = st.get_wallet_summary(1); assert_eq!( summary.as_ref().and_then(|s| s.recovery_progress()), - fully_recovered, + no_recovery, ); assert_eq!(summary.and_then(|s| s.scan_progress()), None); @@ -1347,7 +1343,7 @@ pub(crate) mod tests { assert_eq!( summary.as_ref().and_then(|s| s.recovery_progress()), - fully_recovered, + no_recovery ); // Progress denominator depends on which pools are enabled (which changes the From d89b04c48313635cbfa482de36e6f61d72a59d37 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 8 Oct 2024 18:00:32 -0600 Subject: [PATCH 04/11] zcash_client_sqlite: Factor out tree size estimation from scan progress computation. --- zcash_client_sqlite/src/wallet.rs | 324 +++++++++++++++--------------- 1 file changed, 165 insertions(+), 159 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 1c153824f4..2da02284a4 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -836,6 +836,161 @@ pub(crate) trait ScanProgress { #[derive(Debug)] pub(crate) struct SubtreeScanProgress; +fn estimate_tree_size( + conn: &rusqlite::Connection, + params: &P, + shielded_protocol: ShieldedProtocol, + pool_activation_height: BlockHeight, + chain_tip_height: BlockHeight, +) -> Result, SqliteClientError> { + let (table_prefix, shard_height) = match shielded_protocol { + ShieldedProtocol::Sapling => (SAPLING_TABLES_PREFIX, SAPLING_SHARD_HEIGHT), + ShieldedProtocol::Orchard => (ORCHARD_TABLES_PREFIX, ORCHARD_SHARD_HEIGHT), + }; + + // Estimate the size of the tree by linear extrapolation from available + // data closest to the chain tip. + // + // - If we have scanned blocks within the incomplete subtree, and we know + // the tree size for the end of the most recent scanned range, then we + // extrapolate from the start of the incomplete subtree: + // + // subtree + // / \ + // / \ + // / \ + // / \ + // |<--------->| | + // | scanned | tip + // last_scanned + // + // + // subtree + // / \ + // / \ + // / \ + // / \ + // |<------->| | + // | scanned | tip + // last_scanned + // + // - If we don't have scanned blocks within the incomplete subtree, or we + // don't know the tree size, then we extrapolate from the block-width of + // the last complete subtree. + // + // This avoids having a sharp discontinuity in the progress percentages + // shown to users, and gets more accurate the closer to the chain tip we + // have scanned. + // + // TODO: it would be nice to be able to reliably have the size of the + // commitment tree at the chain tip without having to have scanned that + // block. + + // Get the tree size at the last scanned height, if known. + let last_scanned = block_max_scanned(conn, params)?.and_then(|last_scanned| { + match shielded_protocol { + ShieldedProtocol::Sapling => last_scanned.sapling_tree_size(), + #[cfg(feature = "orchard")] + ShieldedProtocol::Orchard => last_scanned.orchard_tree_size(), + } + .map(|tree_size| (last_scanned.block_height(), u64::from(tree_size))) + }); + + // Get the last completed subtree. + let last_completed_subtree = conn + .query_row( + &format!( + "SELECT shard_index, subtree_end_height + FROM {table_prefix}_tree_shards + WHERE subtree_end_height IS NOT NULL + ORDER BY shard_index DESC + LIMIT 1" + ), + [], + |row| { + Ok(( + incrementalmerkletree::Address::from_parts( + incrementalmerkletree::Level::new(shard_height), + row.get(0)?, + ), + BlockHeight::from_u32(row.get(1)?), + )) + }, + ) + // `None` if we have no subtree roots yet. + .optional()?; + + if let Some((last_completed_subtree, last_completed_subtree_end)) = last_completed_subtree { + // If we know the tree size at the last scanned height, and that + // height is within the incomplete subtree, extrapolate. + let tip_tree_size = last_scanned.and_then(|(last_scanned, last_scanned_tree_size)| { + (last_scanned > last_completed_subtree_end) + .then(|| { + let scanned_notes = last_scanned_tree_size + - u64::from(last_completed_subtree.position_range_end()); + let scanned_range = u64::from(last_scanned - last_completed_subtree_end); + let unscanned_range = u64::from(chain_tip_height - last_scanned); + + (scanned_notes * unscanned_range) + .checked_div(scanned_range) + .map(|extrapolated_unscanned_notes| { + last_scanned_tree_size + extrapolated_unscanned_notes + }) + }) + .flatten() + }); + + if let Some(tree_size) = tip_tree_size { + Ok(Some(tree_size)) + } else if let Some(second_to_last_completed_subtree_end) = last_completed_subtree + .index() + .checked_sub(1) + .and_then(|subtree_index| { + conn.query_row( + &format!( + "SELECT subtree_end_height + FROM {table_prefix}_tree_shards + WHERE shard_index = :shard_index" + ), + named_params! {":shard_index": subtree_index}, + |row| Ok(row.get::<_, Option<_>>(0)?.map(BlockHeight::from_u32)), + ) + .transpose() + }) + .transpose()? + { + let notes_in_complete_subtrees = u64::from(last_completed_subtree.position_range_end()); + + let subtree_notes = 1 << shard_height; + let subtree_range = + u64::from(last_completed_subtree_end - second_to_last_completed_subtree_end); + let unscanned_range = u64::from(chain_tip_height - last_completed_subtree_end); + + Ok((subtree_notes * unscanned_range) + .checked_div(subtree_range) + .map(|extrapolated_incomplete_subtree_notes| { + notes_in_complete_subtrees + extrapolated_incomplete_subtree_notes + })) + } else { + // There's only one completed subtree; its start height must + // be the activation height for this shielded protocol. + let subtree_notes = 1 << shard_height; + + let subtree_range = u64::from(last_completed_subtree_end - pool_activation_height); + let unscanned_range = u64::from(chain_tip_height - last_completed_subtree_end); + + Ok((subtree_notes * unscanned_range) + .checked_div(subtree_range) + .map(|extrapolated_incomplete_subtree_notes| { + subtree_notes + extrapolated_incomplete_subtree_notes + })) + } + } else { + // We don't have subtree information, so give up. We'll get it soon. + Ok(None) + } +} + #[allow(clippy::too_many_arguments)] fn subtree_scan_progress( conn: &rusqlite::Connection, @@ -932,9 +1087,9 @@ fn subtree_scan_progress( conn.query_row( &format!( "SELECT MIN(shard_index) - FROM {table_prefix}_tree_shards - WHERE subtree_end_height >= :start_height - OR subtree_end_height IS NULL", + FROM {table_prefix}_tree_shards + WHERE subtree_end_height >= :start_height + OR subtree_end_height IS NULL", ), named_params! { ":start_height": u32::from(as_of), @@ -1001,162 +1156,13 @@ fn subtree_scan_progress( .flatten() { Some(tree_size) => Some(tree_size), - None => { - // Estimate the size of the tree by linear extrapolation from available - // data closest to the chain tip. - // - // - If we have scanned blocks within the incomplete subtree, and we know - // the tree size for the end of the most recent scanned range, then we - // extrapolate from the start of the incomplete subtree: - // - // subtree - // / \ - // / \ - // / \ - // / \ - // |<--------->| | - // | scanned | tip - // last_scanned - // - // - // subtree - // / \ - // / \ - // / \ - // / \ - // |<------->| | - // | scanned | tip - // last_scanned - // - // - If we don't have scanned blocks within the incomplete subtree, or we - // don't know the tree size, then we extrapolate from the block-width of - // the last complete subtree. - // - // This avoids having a sharp discontinuity in the progress percentages - // shown to users, and gets more accurate the closer to the chain tip we - // have scanned. - // - // TODO: it would be nice to be able to reliably have the size of the - // commitment tree at the chain tip without having to have scanned that - // block. - - // Get the tree size at the last scanned height, if known. - let last_scanned = block_max_scanned(conn, params)?.and_then(|last_scanned| { - match shielded_protocol { - ShieldedProtocol::Sapling => last_scanned.sapling_tree_size(), - #[cfg(feature = "orchard")] - ShieldedProtocol::Orchard => last_scanned.orchard_tree_size(), - } - .map(|tree_size| (last_scanned.block_height(), u64::from(tree_size))) - }); - - // Get the last completed subtree. - let last_completed_subtree = conn - .query_row( - &format!( - "SELECT shard_index, subtree_end_height - FROM {table_prefix}_tree_shards - WHERE subtree_end_height IS NOT NULL - ORDER BY shard_index DESC - LIMIT 1" - ), - [], - |row| { - Ok(( - incrementalmerkletree::Address::from_parts( - incrementalmerkletree::Level::new(shard_height), - row.get(0)?, - ), - BlockHeight::from_u32(row.get(1)?), - )) - }, - ) - // `None` if we have no subtree roots yet. - .optional()?; - - if let Some((last_completed_subtree, last_completed_subtree_end)) = - last_completed_subtree - { - // If we know the tree size at the last scanned height, and that - // height is within the incomplete subtree, extrapolate. - let tip_tree_size = - last_scanned.and_then(|(last_scanned, last_scanned_tree_size)| { - (last_scanned > last_completed_subtree_end) - .then(|| { - let scanned_notes = last_scanned_tree_size - - u64::from(last_completed_subtree.position_range_end()); - let scanned_range = - u64::from(last_scanned - last_completed_subtree_end); - let unscanned_range = - u64::from(chain_tip_height - last_scanned); - - (scanned_notes * unscanned_range) - .checked_div(scanned_range) - .map(|extrapolated_unscanned_notes| { - last_scanned_tree_size + extrapolated_unscanned_notes - }) - }) - .flatten() - }); - - if let Some(tree_size) = tip_tree_size { - Some(tree_size) - } else if let Some(second_to_last_completed_subtree_end) = - last_completed_subtree - .index() - .checked_sub(1) - .and_then(|subtree_index| { - conn.query_row( - &format!( - "SELECT subtree_end_height - FROM {table_prefix}_tree_shards - WHERE shard_index = :shard_index" - ), - named_params! {":shard_index": subtree_index}, - |row| { - Ok(row.get::<_, Option<_>>(0)?.map(BlockHeight::from_u32)) - }, - ) - .transpose() - }) - .transpose()? - { - let notes_in_complete_subtrees = - u64::from(last_completed_subtree.position_range_end()); - - let subtree_notes = 1 << shard_height; - let subtree_range = u64::from( - last_completed_subtree_end - second_to_last_completed_subtree_end, - ); - let unscanned_range = - u64::from(chain_tip_height - last_completed_subtree_end); - - (subtree_notes * unscanned_range) - .checked_div(subtree_range) - .map(|extrapolated_incomplete_subtree_notes| { - notes_in_complete_subtrees + extrapolated_incomplete_subtree_notes - }) - } else { - // There's only one completed subtree; its start height must - // be the activation height for this shielded protocol. - let subtree_notes = 1 << shard_height; - - let subtree_range = - u64::from(last_completed_subtree_end - pool_activation_height); - let unscanned_range = - u64::from(chain_tip_height - last_completed_subtree_end); - - (subtree_notes * unscanned_range) - .checked_div(subtree_range) - .map(|extrapolated_incomplete_subtree_notes| { - subtree_notes + extrapolated_incomplete_subtree_notes - }) - } - } else { - // We don't have subtree information, so give up. We'll get it soon. - None - } - } + None => estimate_tree_size( + conn, + params, + shielded_protocol, + pool_activation_height, + chain_tip_height, + )?, }; let recover = recovered_count From 930f893fb4fec1b87525f9144afa580f1840205f Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 8 Oct 2024 18:33:50 -0600 Subject: [PATCH 05/11] Factor out minor internal code duplication in progress computation. --- .../src/data_api/testing/pool.rs | 1 - zcash_client_sqlite/src/wallet.rs | 42 ++++++++++++------- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index bcda45dcca..3bf8715640 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -886,7 +886,6 @@ pub fn spend_fails_on_unverified_notes( // resulting ratio (the number of notes in the recovery range) is zero. let no_recovery = Some(Ratio::new(0, 0)); - // Wallet is fully scanned let summary = st.get_wallet_summary(1); assert_eq!( diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 2da02284a4..4ba7bb510a 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -836,6 +836,28 @@ pub(crate) trait ScanProgress { #[derive(Debug)] pub(crate) struct SubtreeScanProgress; +fn table_constants( + shielded_protocol: ShieldedProtocol, +) -> Result<(&'static str, &'static str, u8), SqliteClientError> { + match shielded_protocol { + ShieldedProtocol::Sapling => Ok(( + SAPLING_TABLES_PREFIX, + "sapling_output_count", + SAPLING_SHARD_HEIGHT, + )), + #[cfg(feature = "orchard")] + ShieldedProtocol::Orchard => Ok(( + ORCHARD_TABLES_PREFIX, + "orchard_action_count", + ORCHARD_SHARD_HEIGHT, + )), + #[cfg(not(feature = "orchard"))] + ShieldedProtocol::Orchard => { + Err(SqliteClientError::UnsupportedPoolType(PoolType::ORCHARD)) + } + } +} + fn estimate_tree_size( conn: &rusqlite::Connection, params: &P, @@ -843,10 +865,7 @@ fn estimate_tree_size( pool_activation_height: BlockHeight, chain_tip_height: BlockHeight, ) -> Result, SqliteClientError> { - let (table_prefix, shard_height) = match shielded_protocol { - ShieldedProtocol::Sapling => (SAPLING_TABLES_PREFIX, SAPLING_SHARD_HEIGHT), - ShieldedProtocol::Orchard => (ORCHARD_TABLES_PREFIX, ORCHARD_SHARD_HEIGHT), - }; + let (table_prefix, _, shard_height) = table_constants(shielded_protocol)?; // Estimate the size of the tree by linear extrapolation from available // data closest to the chain tip. @@ -892,6 +911,8 @@ fn estimate_tree_size( ShieldedProtocol::Sapling => last_scanned.sapling_tree_size(), #[cfg(feature = "orchard")] ShieldedProtocol::Orchard => last_scanned.orchard_tree_size(), + #[cfg(not(feature = "orchard"))] + ShieldedProtocol::Orchard => None } .map(|tree_size| (last_scanned.block_height(), u64::from(tree_size))) }); @@ -1002,18 +1023,7 @@ fn subtree_scan_progress( fully_scanned_height: BlockHeight, chain_tip_height: BlockHeight, ) -> Result { - let (table_prefix, output_count_col, shard_height) = match shielded_protocol { - ShieldedProtocol::Sapling => ( - SAPLING_TABLES_PREFIX, - "sapling_output_count", - SAPLING_SHARD_HEIGHT, - ), - ShieldedProtocol::Orchard => ( - ORCHARD_TABLES_PREFIX, - "orchard_action_count", - ORCHARD_SHARD_HEIGHT, - ), - }; + let (table_prefix, output_count_col, shard_height) = table_constants(shielded_protocol)?; let mut stmt_scanned_count_until = conn.prepare_cached(&format!( "SELECT SUM({output_count_col}) From fa42b2e838c3e4da7489e41d06e7991bd96d15e9 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 9 Oct 2024 08:46:49 -0600 Subject: [PATCH 06/11] zcash_client_backend: Suppress dead code warning. --- zcash_client_backend/src/data_api.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 875f2ee747..adbb6a9418 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -1270,6 +1270,7 @@ pub trait WalletTest: InputSource + WalletRead { /// /// This type is opaque, and exists for use by tests defined in this crate. #[cfg(any(test, feature = "test-dependencies"))] +#[allow(dead_code)] #[derive(Clone, Debug)] pub struct OutputOfSentTx { value: NonNegativeAmount, From f89b3ce7d230ecd92e953db5216899c2c0a4308e Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 10 Oct 2024 12:59:51 -0600 Subject: [PATCH 07/11] Fix rustfmt and cargo vet errors. --- supply-chain/imports.lock | 7 +++++++ zcash_client_sqlite/src/wallet.rs | 6 ++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/supply-chain/imports.lock b/supply-chain/imports.lock index f30e75731f..ada5729de1 100644 --- a/supply-chain/imports.lock +++ b/supply-chain/imports.lock @@ -346,6 +346,13 @@ user-id = 169181 user-login = "nuttycom" user-name = "Kris Nuttycombe" +[[publisher.zip321]] +version = "0.2.0" +when = "2024-10-04" +user-id = 169181 +user-login = "nuttycom" +user-name = "Kris Nuttycombe" + [[audits.bytecode-alliance.wildcard-audits.bumpalo]] who = "Nick Fitzgerald " criteria = "safe-to-deploy" diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 4ba7bb510a..b4a29f294c 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -852,9 +852,7 @@ fn table_constants( ORCHARD_SHARD_HEIGHT, )), #[cfg(not(feature = "orchard"))] - ShieldedProtocol::Orchard => { - Err(SqliteClientError::UnsupportedPoolType(PoolType::ORCHARD)) - } + ShieldedProtocol::Orchard => Err(SqliteClientError::UnsupportedPoolType(PoolType::ORCHARD)), } } @@ -912,7 +910,7 @@ fn estimate_tree_size( #[cfg(feature = "orchard")] ShieldedProtocol::Orchard => last_scanned.orchard_tree_size(), #[cfg(not(feature = "orchard"))] - ShieldedProtocol::Orchard => None + ShieldedProtocol::Orchard => None, } .map(|tree_size| (last_scanned.block_height(), u64::from(tree_size))) }); From e4c6a3d0bd5d7633d70542e9275e1a5b498c8389 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 9 Oct 2024 14:27:59 -0600 Subject: [PATCH 08/11] zcash_client_sqlite: Modify `Progress::scan` to be non-optional & fix tests. --- zcash_client_sqlite/src/lib.rs | 4 +- zcash_client_sqlite/src/wallet.rs | 143 +++++++++++++-------- zcash_client_sqlite/src/wallet/scanning.rs | 5 +- 3 files changed, 96 insertions(+), 56 deletions(-) diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 4e297d3342..293ac34396 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -128,7 +128,7 @@ pub mod error; pub mod wallet; use wallet::{ commitment_tree::{self, put_shard_roots}, - SubtreeScanProgress, + SubtreeProgressEstimator, }; #[cfg(test)] @@ -461,7 +461,7 @@ impl, P: consensus::Parameters> WalletRead for W &self.conn.borrow().unchecked_transaction()?, &self.params, min_confirmations, - &SubtreeScanProgress, + &SubtreeProgressEstimator, ) } diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index b4a29f294c..516459ae4a 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -806,20 +806,34 @@ pub(crate) fn get_derived_account( #[derive(Debug)] pub(crate) struct Progress { - scan: Option>, - recover: Option>, + scan: Ratio, + recovery: Option>, } -pub(crate) trait ScanProgress { +impl Progress { + pub(crate) fn new(scan: Ratio, recovery: Option>) -> Self { + Self { scan, recovery } + } + + pub(crate) fn scan(&self) -> Ratio { + self.scan + } + + pub(crate) fn recovery(&self) -> Option> { + self.recovery + } +} + +pub(crate) trait ProgressEstimator { fn sapling_scan_progress( &self, conn: &rusqlite::Connection, params: &P, birthday_height: BlockHeight, recover_until_height: Option, - fully_scanned_height: BlockHeight, + fully_scanned_height: Option, chain_tip_height: BlockHeight, - ) -> Result; + ) -> Result, SqliteClientError>; #[cfg(feature = "orchard")] fn orchard_scan_progress( @@ -828,13 +842,13 @@ pub(crate) trait ScanProgress { params: &P, birthday_height: BlockHeight, recover_until_height: Option, - fully_scanned_height: BlockHeight, + fully_scanned_height: Option, chain_tip_height: BlockHeight, - ) -> Result; + ) -> Result, SqliteClientError>; } #[derive(Debug)] -pub(crate) struct SubtreeScanProgress; +pub(crate) struct SubtreeProgressEstimator; fn table_constants( shielded_protocol: ShieldedProtocol, @@ -1005,8 +1019,22 @@ fn estimate_tree_size( })) } } else { - // We don't have subtree information, so give up. We'll get it soon. - Ok(None) + // If there are no completed subtrees, but we have scanned some blocks, we can still + // interpolate based upon the tree size as of the last scanned block. Here, since we + // don't have any subtree data to draw on, we will interpolate based on the number of + // blocks since the pool activation height + Ok( + last_scanned.and_then(|(last_scanned_height, last_scanned_tree_size)| { + let subtree_range = u64::from(last_scanned_height - pool_activation_height); + let unscanned_range = u64::from(chain_tip_height - last_scanned_height); + + (last_scanned_tree_size * unscanned_range) + .checked_div(subtree_range) + .map(|extrapolated_incomplete_subtree_notes| { + last_scanned_tree_size + extrapolated_incomplete_subtree_notes + }) + }), + ) } } @@ -1018,9 +1046,9 @@ fn subtree_scan_progress( pool_activation_height: BlockHeight, birthday_height: BlockHeight, recover_until_height: Option, - fully_scanned_height: BlockHeight, + fully_scanned_height: Option, chain_tip_height: BlockHeight, -) -> Result { +) -> Result, SqliteClientError> { let (table_prefix, output_count_col, shard_height) = table_constants(shielded_protocol)?; let mut stmt_scanned_count_until = conn.prepare_cached(&format!( @@ -1044,7 +1072,7 @@ fn subtree_scan_progress( WHERE height = :height", ))?; - if fully_scanned_height == chain_tip_height { + if fully_scanned_height == Some(chain_tip_height) { // Compute the total blocks scanned since the wallet birthday on either side of // the recover-until height. let recover = recover_until_height @@ -1077,7 +1105,8 @@ fn subtree_scan_progress( Ok(scanned.map(|n| Ratio::new(n, n))) }, )?; - Ok(Progress { scan, recover }) + + Ok(scan.map(|scan| Progress::new(scan, recover))) } else { // In case we didn't have information about the tree size at the recover-until // height, get the tree size from a nearby subtree. It's fine for this to be @@ -1153,8 +1182,9 @@ fn subtree_scan_progress( ) }) .transpose()?; - // If we've scanned the block at the chain tip, we know how many notes are - // currently in the tree. + + // If we've scanned the block at the chain tip, we know how many notes are currently in the + // tree. let tip_tree_size = match stmt_end_tree_size_at .query_row( named_params! {":height": u32::from(chain_tip_height)}, @@ -1201,11 +1231,11 @@ fn subtree_scan_progress( }) }; - Ok(Progress { scan, recover }) + Ok(scan.map(|scan| Progress::new(scan, recover))) } } -impl ScanProgress for SubtreeScanProgress { +impl ProgressEstimator for SubtreeProgressEstimator { #[tracing::instrument(skip(conn, params))] fn sapling_scan_progress( &self, @@ -1213,9 +1243,9 @@ impl ScanProgress for SubtreeScanProgress { params: &P, birthday_height: BlockHeight, recover_until_height: Option, - fully_scanned_height: BlockHeight, + fully_scanned_height: Option, chain_tip_height: BlockHeight, - ) -> Result { + ) -> Result, SqliteClientError> { subtree_scan_progress( conn, params, @@ -1238,9 +1268,9 @@ impl ScanProgress for SubtreeScanProgress { params: &P, birthday_height: BlockHeight, recover_until_height: Option, - fully_scanned_height: BlockHeight, + fully_scanned_height: Option, chain_tip_height: BlockHeight, - ) -> Result { + ) -> Result, SqliteClientError> { subtree_scan_progress( conn, params, @@ -1268,7 +1298,7 @@ pub(crate) fn get_wallet_summary( tx: &rusqlite::Transaction, params: &P, min_confirmations: u32, - progress: &impl ScanProgress, + progress: &impl ProgressEstimator, ) -> Result>, SqliteClientError> { let chain_tip_height = match chain_tip_height(tx)? { Some(h) => h, @@ -1277,12 +1307,16 @@ pub(crate) fn get_wallet_summary( } }; - let birthday_height = - wallet_birthday(tx)?.expect("If a scan range exists, we know the wallet birthday."); + let birthday_height = match wallet_birthday(tx)? { + Some(h) => h, + None => { + return Ok(None); + } + }; + let recover_until_height = recover_until_height(tx)?; - let fully_scanned_height = - block_fully_scanned(tx, params)?.map_or(birthday_height - 1, |m| m.block_height()); + let fully_scanned_height = block_fully_scanned(tx, params)?.map(|m| m.block_height()); let summary_height = (chain_tip_height + 1).saturating_sub(std::cmp::max(min_confirmations, 1)); let sapling_progress = progress.sapling_scan_progress( @@ -1304,34 +1338,37 @@ pub(crate) fn get_wallet_summary( chain_tip_height, )?; #[cfg(not(feature = "orchard"))] - let orchard_progress: Progress = Progress { - scan: None, - recover: None, - }; + let orchard_progress: Option = None; // Treat Sapling and Orchard outputs as having the same cost to scan. - let scan_progress = sapling_progress - .scan - .zip(orchard_progress.scan) - .map(|(s, o)| { - Ratio::new( - s.numerator() + o.numerator(), - s.denominator() + o.denominator(), - ) - }) - .or(sapling_progress.scan) - .or(orchard_progress.scan); - let recover_progress = sapling_progress - .recover - .zip(orchard_progress.recover) + let progress = sapling_progress + .as_ref() + .zip(orchard_progress.as_ref()) .map(|(s, o)| { - Ratio::new( - s.numerator() + o.numerator(), - s.denominator() + o.denominator(), + Progress::new( + Ratio::new( + s.scan().numerator() + o.scan().numerator(), + s.scan().denominator() + o.scan().denominator(), + ), + s.recovery() + .zip(o.recovery()) + .map(|(s, o)| { + Ratio::new( + s.numerator() + o.numerator(), + s.denominator() + o.denominator(), + ) + }) + .or_else(|| s.recovery()) + .or_else(|| o.recovery()), ) }) - .or(sapling_progress.recover) - .or(orchard_progress.recover); + .or(sapling_progress) + .or(orchard_progress); + + let progress = match progress { + Some(p) => p, + None => return Ok(None), + }; let mut stmt_accounts = tx.prepare_cached("SELECT id FROM accounts")?; let mut account_balances = stmt_accounts @@ -1552,9 +1589,9 @@ pub(crate) fn get_wallet_summary( let summary = WalletSummary::new( account_balances, chain_tip_height, - fully_scanned_height, - scan_progress, - recover_progress, + fully_scanned_height.unwrap_or(birthday_height - 1), + Some(progress.scan), + progress.recovery, next_sapling_subtree_index, #[cfg(feature = "orchard")] next_orchard_subtree_index, diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index a77700f697..594c9c32ca 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -1303,7 +1303,10 @@ pub(crate) mod tests { summary.as_ref().and_then(|s| s.recovery_progress()), no_recovery, ); - assert_eq!(summary.and_then(|s| s.scan_progress()), None); + assert_matches!( + summary.and_then(|s| s.scan_progress()), + Some(progress) if progress.numerator() == &0 + ); // Set up prior chain state. This simulates us having imported a wallet // with a birthday 520 blocks below the chain tip. From c10828110c62b2c42b23529e5706634e87dea9db Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 10 Oct 2024 17:02:34 -0600 Subject: [PATCH 09/11] Apply code style suggestions from code review. --- zcash_client_sqlite/src/wallet.rs | 152 ++++++++++++++++-------------- 1 file changed, 80 insertions(+), 72 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 516459ae4a..388433b4f5 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -953,7 +953,9 @@ fn estimate_tree_size( // `None` if we have no subtree roots yet. .optional()?; - if let Some((last_completed_subtree, last_completed_subtree_end)) = last_completed_subtree { + let result = if let Some((last_completed_subtree, last_completed_subtree_end)) = + last_completed_subtree + { // If we know the tree size at the last scanned height, and that // height is within the incomplete subtree, extrapolate. let tip_tree_size = last_scanned.and_then(|(last_scanned, last_scanned_tree_size)| { @@ -974,7 +976,7 @@ fn estimate_tree_size( }); if let Some(tree_size) = tip_tree_size { - Ok(Some(tree_size)) + Some(tree_size) } else if let Some(second_to_last_completed_subtree_end) = last_completed_subtree .index() .checked_sub(1) @@ -999,11 +1001,11 @@ fn estimate_tree_size( u64::from(last_completed_subtree_end - second_to_last_completed_subtree_end); let unscanned_range = u64::from(chain_tip_height - last_completed_subtree_end); - Ok((subtree_notes * unscanned_range) + (subtree_notes * unscanned_range) .checked_div(subtree_range) .map(|extrapolated_incomplete_subtree_notes| { notes_in_complete_subtrees + extrapolated_incomplete_subtree_notes - })) + }) } else { // There's only one completed subtree; its start height must // be the activation height for this shielded protocol. @@ -1012,30 +1014,30 @@ fn estimate_tree_size( let subtree_range = u64::from(last_completed_subtree_end - pool_activation_height); let unscanned_range = u64::from(chain_tip_height - last_completed_subtree_end); - Ok((subtree_notes * unscanned_range) + (subtree_notes * unscanned_range) .checked_div(subtree_range) .map(|extrapolated_incomplete_subtree_notes| { subtree_notes + extrapolated_incomplete_subtree_notes - })) + }) } } else { // If there are no completed subtrees, but we have scanned some blocks, we can still // interpolate based upon the tree size as of the last scanned block. Here, since we // don't have any subtree data to draw on, we will interpolate based on the number of // blocks since the pool activation height - Ok( - last_scanned.and_then(|(last_scanned_height, last_scanned_tree_size)| { - let subtree_range = u64::from(last_scanned_height - pool_activation_height); - let unscanned_range = u64::from(chain_tip_height - last_scanned_height); - - (last_scanned_tree_size * unscanned_range) - .checked_div(subtree_range) - .map(|extrapolated_incomplete_subtree_notes| { - last_scanned_tree_size + extrapolated_incomplete_subtree_notes - }) - }), - ) - } + last_scanned.and_then(|(last_scanned_height, last_scanned_tree_size)| { + let subtree_range = u64::from(last_scanned_height - pool_activation_height); + let unscanned_range = u64::from(chain_tip_height - last_scanned_height); + + (last_scanned_tree_size * unscanned_range) + .checked_div(subtree_range) + .map(|extrapolated_incomplete_subtree_notes| { + last_scanned_tree_size + extrapolated_incomplete_subtree_notes + }) + }) + }; + + Ok(result) } #[allow(clippy::too_many_arguments)] @@ -1075,24 +1077,24 @@ fn subtree_scan_progress( if fully_scanned_height == Some(chain_tip_height) { // Compute the total blocks scanned since the wallet birthday on either side of // the recover-until height. - let recover = recover_until_height - .map(|end_height| { - stmt_scanned_count_until.query_row( - named_params! { - ":start_height": u32::from(birthday_height), - ":end_height": u32::from(end_height), - }, - |row| { - let recovered = row.get::<_, Option>(0)?; - Ok(recovered.map(|n| Ratio::new(n, n))) - }, - ) - }) - .transpose()? - // If none of the wallet's accounts have a recover-until height, then there - // is no recovery phase for the wallet, and therefore the denominator in the - // resulting ratio (the number of notes in the recovery range) is zero. - .unwrap_or_else(|| Some(Ratio::new(0, 0))); + let recover = match recover_until_height { + Some(end_height) => stmt_scanned_count_until.query_row( + named_params! { + ":start_height": u32::from(birthday_height), + ":end_height": u32::from(end_height), + }, + |row| { + let recovered = row.get::<_, Option>(0)?; + Ok(recovered.map(|n| Ratio::new(n, n))) + }, + )?, + None => { + // If none of the wallet's accounts have a recover-until height, then there + // is no recovery phase for the wallet, and therefore the denominator in the + // resulting ratio (the number of notes in the recovery range) is zero. + Some(Ratio::new(0, 0)) + } + }; let scan = stmt_scanned_count_from.query_row( named_params! { @@ -1112,60 +1114,66 @@ fn subtree_scan_progress( // height, get the tree size from a nearby subtree. It's fine for this to be // approximate; it just shifts the boundary between scan and recover progress. let mut get_tree_size_near = |as_of: BlockHeight| { - stmt_start_tree_size - .query_row( - named_params![":start_height": u32::from(as_of)], - |row| row.get::<_, Option>(0), - ) - .optional() - .map(|opt| opt.flatten()) - .transpose() - .or_else(|| { - conn.query_row( - &format!( - "SELECT MIN(shard_index) + let size_from_blocks = stmt_start_tree_size + .query_row(named_params![":start_height": u32::from(as_of)], |row| { + row.get::<_, Option>(0) + }) + .optional()? + .flatten(); + + let size_from_subtree_roots = || { + conn.query_row( + &format!( + "SELECT MIN(shard_index) FROM {table_prefix}_tree_shards WHERE subtree_end_height >= :start_height OR subtree_end_height IS NULL", - ), - named_params! { - ":start_height": u32::from(as_of), - }, - |row| { - let min_tree_size = row - .get::<_, Option>(0)? - .map(|min_idx| min_idx << shard_height); - Ok(min_tree_size) - }, - ) - .optional() - .map(|opt| opt.flatten()) - .transpose() - }) - .transpose() + ), + named_params! { + ":start_height": u32::from(as_of), + }, + |row| { + let min_tree_size = row + .get::<_, Option>(0)? + .map(|min_idx| min_idx << shard_height); + Ok(min_tree_size) + }, + ) + .optional() + .map(|opt| opt.flatten()) + }; + + match size_from_blocks { + Some(size) => Ok(Some(size)), + None => size_from_subtree_roots(), + } }; // Get the starting note commitment tree size from the wallet birthday, or failing that // from the blocks table. - let birthday_size = conn + let birthday_size = match conn .query_row( &format!( "SELECT birthday_{table_prefix}_tree_size - FROM accounts - WHERE birthday_height = :birthday_height", + FROM accounts + WHERE birthday_height = :birthday_height", ), named_params![":birthday_height": u32::from(birthday_height)], |row| row.get::<_, Option>(0), ) .optional()? .flatten() - .map(Ok) + { + Some(tree_size) => Some(tree_size), // If we don't have an explicit birthday tree size, find something nearby. - .or_else(|| get_tree_size_near(birthday_height).transpose()) - .transpose()?; + None => get_tree_size_near(birthday_height)?, + }; // Get the note commitment tree size as of the start of the recover-until height. - let recover_until_size = recover_until_height + // The outer option indicates whether or not we have recover-until height information; + // the inner option indicates whether or not we were able to obtain a tree size given + // the recover-until height. + let recover_until_size: Option> = recover_until_height // Find a tree size near to the recover-until height .map(get_tree_size_near) .transpose()?; From 370c83fc486b8803ac988cb5bd889da6f439e997 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 10 Oct 2024 17:02:34 -0600 Subject: [PATCH 10/11] zcash_client_backend: Clarify documentation of `scan_progress` and `recovery_progress` methods. --- zcash_client_backend/src/data_api.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index adbb6a9418..ce743fc53c 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -534,10 +534,11 @@ impl WalletSummary { /// Progress is represented in terms of the ratio between notes scanned and the total /// number of notes added to the chain in the relevant window. This ratio should only /// be used to compute progress percentages, and the numerator and denominator should - /// not be treated as authoritative note counts. + /// not be treated as authoritative note counts. The denominator of this ratio is + /// guaranteed to be nonzero. /// - /// Returns `None` if the wallet is unable to determine the size of the note - /// commitment tree. + /// Returns `None` if the wallet has insufficient information to be able to determine + /// scan progress. pub fn scan_progress(&self) -> Option> { self.scan_progress } @@ -554,10 +555,12 @@ impl WalletSummary { /// Progress is represented in terms of the ratio between notes scanned and the total /// number of notes added to the chain in the relevant window. This ratio should only /// be used to compute progress percentages, and the numerator and denominator should - /// not be treated as authoritative note counts. + /// not be treated as authoritative note counts. Note that both the numerator and the + /// denominator of this ratio may be zero in the case that there is no recovery range + /// that need be scanned. /// - /// Returns `None` if the wallet is unable to determine the size of the note - /// commitment tree. + /// Returns `None` if the wallet has insufficient information to be able to determine + /// progress in scanning between the wallet birthday and the wallet recovery height. pub fn recovery_progress(&self) -> Option> { self.recovery_progress } From 279479c19e8db8e97a0d618aee9be3dd4988181f Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 10 Oct 2024 17:02:34 -0600 Subject: [PATCH 11/11] Release zcash_client_sqlite version 0.12.1 --- Cargo.lock | 2 +- supply-chain/imports.lock | 11 +++++++++++ zcash_client_sqlite/CHANGELOG.md | 10 ++++++++++ zcash_client_sqlite/Cargo.toml | 2 +- 4 files changed, 23 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d6f0a049f1..c9c624b56d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5897,7 +5897,7 @@ dependencies = [ [[package]] name = "zcash_client_sqlite" -version = "0.12.0" +version = "0.12.1" dependencies = [ "ambassador", "assert_matches", diff --git a/supply-chain/imports.lock b/supply-chain/imports.lock index ada5729de1..0d834618da 100644 --- a/supply-chain/imports.lock +++ b/supply-chain/imports.lock @@ -9,6 +9,10 @@ audited_as = "0.13.0" version = "0.12.0" audited_as = "0.11.2" +[[unpublished.zcash_client_sqlite]] +version = "0.12.1" +audited_as = "0.12.0" + [[unpublished.zcash_keys]] version = "0.4.0" audited_as = "0.3.0" @@ -270,6 +274,13 @@ user-id = 169181 user-login = "nuttycom" user-name = "Kris Nuttycombe" +[[publisher.zcash_client_sqlite]] +version = "0.12.0" +when = "2024-10-04" +user-id = 169181 +user-login = "nuttycom" +user-name = "Kris Nuttycombe" + [[publisher.zcash_encoding]] version = "0.2.0" when = "2022-10-19" diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index 6a646adfc3..771aa1b1fe 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -7,6 +7,16 @@ and this library adheres to Rust's notion of ## [Unreleased] +## [0.12.1] - 2024-10-10 + +### Fixed +- An error in scan progress computation was fixed. As part of this fix, wallet + summary information is now only returned in the case that some note + commitment tree size information can be determined, either from subtree root + download or from downloaded block data. NOTE: The recovery progress ratio may + be present as `0:0` in the case that the recovery range contains no notes; + this was not adequately documented in the previous release. + ## [0.12.0] - 2024-10-04 ### Added diff --git a/zcash_client_sqlite/Cargo.toml b/zcash_client_sqlite/Cargo.toml index 38a251775e..e1db2d2130 100644 --- a/zcash_client_sqlite/Cargo.toml +++ b/zcash_client_sqlite/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "zcash_client_sqlite" description = "An SQLite-based Zcash light client" -version = "0.12.0" +version = "0.12.1" authors = [ "Jack Grigg ", "Kris Nuttycombe "