From 9f9a1fb92f009102c342b895b0cb6d27d2825415 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 2 Sep 2023 03:00:33 +0000 Subject: [PATCH 1/4] zcash_client_sqlite: Rework `update_chain_tip` tests to pin behaviour --- zcash_client_sqlite/src/wallet/scanning.rs | 360 +++++++++++++++++++-- 1 file changed, 330 insertions(+), 30 deletions(-) diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index 7d1d9ed3c..3ea883cd0 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -834,6 +834,7 @@ mod tests { use incrementalmerkletree::{frontier::Frontier, Hashable, Level, Position}; + use secrecy::SecretVec; use zcash_client_backend::data_api::{ chain::CommitmentTreeRoot, scanning::{ScanPriority, ScanRange}, @@ -844,12 +845,14 @@ mod tests { consensus::{BlockHeight, NetworkUpgrade, Parameters}, sapling::Node, transaction::components::Amount, + zip32::DiversifiableFullViewingKey, }; use crate::{ error::SqliteClientError, - testing::{AddressType, TestBuilder}, + testing::{AddressType, BlockCache, TestBuilder, TestState}, wallet::scanning::{insert_queue_entries, replace_queue_entries, suggest_scan_ranges}, + VERIFY_LOOKAHEAD, }; use super::{join_nonoverlapping, Joined, RangeOrdering, SpanningTree}; @@ -1047,6 +1050,30 @@ mod tests { ); } + #[test] + fn spanning_tree_insert_gaps() { + use ScanPriority::*; + + let t = spanning_tree(&[(0..3, Historic), (6..8, ChainTip)]).unwrap(); + + assert_eq!( + t.into_vec(), + vec![scan_range(0..6, Historic), scan_range(6..8, ChainTip),] + ); + + let t = spanning_tree(&[(0..3, Historic), (3..4, Verify), (6..8, ChainTip)]).unwrap(); + + assert_eq!( + t.into_vec(), + vec![ + scan_range(0..3, Historic), + scan_range(3..4, Verify), + scan_range(4..6, Historic), + scan_range(6..8, ChainTip), + ] + ); + } + #[test] fn spanning_tree_insert_rfd_span() { use ScanPriority::*; @@ -1321,11 +1348,13 @@ mod tests { ); } - #[test] - fn create_account_creates_ignored_range() { - use ScanPriority::*; - - let mut st = TestBuilder::new() + fn test_with_canopy_birthday() -> ( + TestState, + DiversifiableFullViewingKey, + AccountBirthday, + u32, + ) { + let st = TestBuilder::new() .with_block_cache() .with_test_account(|network| { // We use Canopy activation as an arbitrary birthday height that's greater than Sapling @@ -1346,27 +1375,190 @@ mod tests { let dfvk = st.test_account_sapling().unwrap(); let sap_active = st.sapling_activation_height(); + (st, dfvk, birthday, sap_active.into()) + } + + #[test] + fn create_account_creates_ignored_range() { + use ScanPriority::*; + + let (st, _, birthday, sap_active) = test_with_canopy_birthday(); + let birthday_height = birthday.height().into(); + let expected = vec![ // The range up to the wallet's birthday height is ignored. - scan_range(u32::from(sap_active)..u32::from(birthday.height()), Ignored), + scan_range(sap_active..birthday_height, Ignored), + ]; + let actual = suggest_scan_ranges(&st.wallet().conn, Ignored).unwrap(); + assert_eq!(actual, expected); + } + + #[test] + fn update_chain_tip_before_create_account() { + use ScanPriority::*; + + let mut st = TestBuilder::new().with_block_cache().build(); + let sap_active = st.sapling_activation_height(); + + // Update the chain tip. + let new_tip = sap_active + 1000; + st.wallet_mut().update_chain_tip(new_tip).unwrap(); + let chain_end = u32::from(new_tip + 1); + + let expected = vec![ + // The range up to the chain end is ignored. + scan_range(sap_active.into()..chain_end, Ignored), ]; let actual = suggest_scan_ranges(&st.wallet().conn, Ignored).unwrap(); assert_eq!(actual, expected); - // Set up some shard root history before the wallet birthday + // Now add an account. + let wallet_birthday = sap_active + 500; + st.wallet_mut() + .create_account( + &SecretVec::new(vec![0; 32]), + AccountBirthday::from_parts(wallet_birthday, Frontier::empty(), None), + ) + .unwrap(); + + let expected = vec![ + // The account's birthday onward is marked for recovery. + scan_range(wallet_birthday.into()..chain_end, Historic), + // The range up to the wallet's birthday height is ignored. + scan_range(sap_active.into()..wallet_birthday.into(), Ignored), + ]; + let actual = suggest_scan_ranges(&st.wallet().conn, Ignored).unwrap(); + assert_eq!(actual, expected); + } + + #[test] + fn update_chain_tip_with_no_subtree_roots() { + use ScanPriority::*; + + let (mut st, _, birthday, sap_active) = test_with_canopy_birthday(); + + // Set up the following situation: + // + // prior_tip new_tip + // |<--- 500 --->| + // wallet_birthday + let prior_tip = birthday.height(); + let wallet_birthday = birthday.height().into(); + + // Update the chain tip. + let new_tip = prior_tip + 500; + st.wallet_mut().update_chain_tip(new_tip).unwrap(); + let chain_end = u32::from(new_tip + 1); + + // Verify that the suggested scan ranges match what is expected. + let expected = vec![ + // The wallet's birthday onward is marked for recovery. + scan_range(wallet_birthday..chain_end, Historic), + // The range below the wallet's birthday height is ignored. + scan_range(sap_active..wallet_birthday, Ignored), + ]; + + let actual = suggest_scan_ranges(&st.wallet().conn, Ignored).unwrap(); + assert_eq!(actual, expected); + } + + #[test] + fn update_chain_tip_when_never_scanned() { + use ScanPriority::*; + + let (mut st, _, birthday, sap_active) = test_with_canopy_birthday(); + + // Set up the following situation: + // + // last_shard_start prior_tip new_tip + // |<----- 1000 ----->|<--- 500 --->| + // wallet_birthday + let prior_tip_height = birthday.height(); + + // Set up some shard root history before the wallet birthday. + let last_shard_start = birthday.height() - 1000; st.wallet_mut() .put_sapling_subtree_roots( 0, &[CommitmentTreeRoot::from_parts( - birthday.height() - 1000, + last_shard_start, // fake a hash, the value doesn't matter Node::empty_leaf(), )], ) .unwrap(); + // Update the chain tip. + let tip_height = prior_tip_height + 500; + st.wallet_mut().update_chain_tip(tip_height).unwrap(); + let chain_end = u32::from(tip_height + 1); + + // Verify that the suggested scan ranges match what is expected. + let expected = vec![ + // The entire last (incomplete) shard's range is marked for catching up to the + // chain tip, to ensure that if any notes are discovered after the wallet's + // birthday, they will be spendable. + scan_range(last_shard_start.into()..chain_end, ChainTip), + // The range below the last shard is ignored. + scan_range(sap_active..last_shard_start.into(), Ignored), + ]; + + let actual = suggest_scan_ranges(&st.wallet().conn, Ignored).unwrap(); + assert_eq!(actual, expected); + } + + #[test] + fn update_chain_tip_unstable_max_scanned() { + use ScanPriority::*; + + let (mut st, dfvk, birthday, sap_active) = test_with_canopy_birthday(); + + // Set up the following situation: + // + // prior_tip new_tip + // |<--- 500 --->|<- 40 ->|<-- 70 -->|<- 20 ->| + // wallet_birthday max_scanned last_shard_start + // + let max_scanned = birthday.height() + 500; + let prior_tip = max_scanned + 40; + + // Set up some shard root history before the wallet birthday. + let second_to_last_shard_start = birthday.height() - 1000; + st.wallet_mut() + .put_sapling_subtree_roots( + 0, + &[CommitmentTreeRoot::from_parts( + second_to_last_shard_start, + // fake a hash, the value doesn't matter + Node::empty_leaf(), + )], + ) + .unwrap(); + + // Set up prior chain state. This simulates us having imported a wallet + // with a birthday 520 blocks below the chain tip. + st.wallet_mut().update_chain_tip(prior_tip).unwrap(); + + // Verify that the suggested scan ranges match what is expected. + let expected = vec![ + // The second-to-last shard is currently the last shard, so it is marked for + // scanning to catch up to the prior chain tip. This includes heights prior to + // the wallet's birthday, because the wallet doesn't know that it already has + // enough data from the initial frontier to avoid having to scan this range. + scan_range( + second_to_last_shard_start.into()..(prior_tip + 1).into(), + ChainTip, + ), + // The range below the second-to-last shard is ignored. + scan_range(sap_active..second_to_last_shard_start.into(), Ignored), + ]; + + let actual = suggest_scan_ranges(&st.wallet().conn, Ignored).unwrap(); + assert_eq!(actual, expected); + + // Now, scan the max scanned block. st.generate_block_at( - birthday.height(), + max_scanned, BlockHash([0u8; 32]), &dfvk, AddressType::DefaultExternal, @@ -1375,42 +1567,150 @@ mod tests { .try_into() .unwrap(), ); - st.scan_cached_blocks(birthday.height(), 1); + st.scan_cached_blocks(max_scanned, 1); - // Update the chain tip - let tip_height = st - .wallet() - .params - .activation_height(NetworkUpgrade::Nu5) + // Now simulate shutting down, and then restarting 90 blocks later, after a shard + // has been completed. + let last_shard_start = prior_tip + 70; + st.wallet_mut() + .put_sapling_subtree_roots( + 0, + &[CommitmentTreeRoot::from_parts( + last_shard_start, + // fake a hash, the value doesn't matter + Node::empty_leaf(), + )], + ) .unwrap(); - st.wallet_mut().update_chain_tip(tip_height).unwrap(); + + let new_tip = last_shard_start + 20; + st.wallet_mut().update_chain_tip(new_tip).unwrap(); + let chain_end = u32::from(new_tip + 1); // Verify that the suggested scan ranges match what is expected let expected = vec![ - // The birthday height is the "first to be scanned" (as the wallet birthday), - // so we verify 10 blocks starting at that height. + // The max scanned block's connectivity is verified by scanning the next 10 blocks. scan_range( - u32::from(birthday.height())..u32::from(birthday.height() + 10), + (max_scanned + 1).into()..(max_scanned + 1 + VERIFY_LOOKAHEAD).into(), Verify, ), - // The remainder of the shard after the verify segment is required in order to make - // notes spendable, so it has priority `ChainTip` + // The last shard needs to catch up to the chain tip in order to make notes spendable. + scan_range(last_shard_start.into()..chain_end, ChainTip), + // The range between the verification blocks and the prior tip is still in the queue. scan_range( - u32::from(birthday.height() + 10)..u32::from(tip_height + 1), + (max_scanned + 1 + VERIFY_LOOKAHEAD).into()..(prior_tip + 1).into(), ChainTip, ), - // The remainder of the shard prior to the birthday height must be scanned because the - // wallet doesn't know that it already has enough data from the initial frontier to - // avoid having to scan this range. + // The remainder of the second-to-last shard's range is still in the queue. scan_range( - u32::from(birthday.height() - 1000)..u32::from(birthday.height()), + second_to_last_shard_start.into()..max_scanned.into(), ChainTip, ), - // The range below the wallet's birthday height is ignored + // The gap between the prior tip and the last shard is deferred as low priority. + scan_range((prior_tip + 1).into()..last_shard_start.into(), Historic), + // The max scanned block itself is left as-is. + scan_range(max_scanned.into()..(max_scanned + 1).into(), Scanned), + // The range below the second-to-last shard is ignored. + scan_range(sap_active..second_to_last_shard_start.into(), Ignored), + ]; + + let actual = suggest_scan_ranges(&st.wallet().conn, Ignored).unwrap(); + assert_eq!(actual, expected); + } + + #[test] + fn update_chain_tip_stable_max_scanned() { + use ScanPriority::*; + + let (mut st, dfvk, birthday, sap_active) = test_with_canopy_birthday(); + + // Set up the following situation: + // + // prior_tip new_tip + // |<--- 500 --->|<- 20 ->|<-- 50 -->|<- 20 ->| + // wallet_birthday max_scanned last_shard_start + // + let max_scanned = birthday.height() + 500; + let prior_tip = max_scanned + 20; + + // Set up some shard root history before the wallet birthday. + let second_to_last_shard_start = birthday.height() - 1000; + st.wallet_mut() + .put_sapling_subtree_roots( + 0, + &[CommitmentTreeRoot::from_parts( + second_to_last_shard_start, + // fake a hash, the value doesn't matter + Node::empty_leaf(), + )], + ) + .unwrap(); + + // Set up prior chain state. This simulates us having imported a wallet + // with a birthday 520 blocks below the chain tip. + st.wallet_mut().update_chain_tip(prior_tip).unwrap(); + + // Verify that the suggested scan ranges match what is expected. + let expected = vec![ + // The second-to-last shard is currently the last shard, so it is marked for + // scanning to catch up to the prior chain tip. This includes heights prior to + // the wallet's birthday, because the wallet doesn't know that it already has + // enough data from the initial frontier to avoid having to scan this range. scan_range( - u32::from(sap_active)..u32::from(birthday.height() - 1000), - Ignored, + second_to_last_shard_start.into()..(prior_tip + 1).into(), + ChainTip, ), + // The range below the second-to-last shard is ignored. + scan_range(sap_active..second_to_last_shard_start.into(), Ignored), + ]; + + let actual = suggest_scan_ranges(&st.wallet().conn, Ignored).unwrap(); + assert_eq!(actual, expected); + + // Now, scan the max scanned block. + st.generate_block_at( + max_scanned, + BlockHash([0u8; 32]), + &dfvk, + AddressType::DefaultExternal, + Amount::const_from_i64(10000), + u64::from(birthday.sapling_frontier().value().unwrap().position() + 1) + .try_into() + .unwrap(), + ); + st.scan_cached_blocks(max_scanned, 1); + + // Now simulate shutting down, and then restarting 70 blocks later, after a shard + // has been completed. + let last_shard_start = prior_tip + 50; + st.wallet_mut() + .put_sapling_subtree_roots( + 0, + &[CommitmentTreeRoot::from_parts( + last_shard_start, + // fake a hash, the value doesn't matter + Node::empty_leaf(), + )], + ) + .unwrap(); + + let new_tip = last_shard_start + 20; + st.wallet_mut().update_chain_tip(new_tip).unwrap(); + let chain_end = u32::from(new_tip + 1); + + // Verify that the suggested scan ranges match what is expected. + let expected = vec![ + // The blocks after the max scanned block up to the chain tip are prioritised. + scan_range((max_scanned + 1).into()..chain_end, ChainTip), + // The remainder of the second-to-last shard's range is still in the queue. + scan_range( + second_to_last_shard_start.into()..max_scanned.into(), + ChainTip, + ), + // The max scanned block itself is left as-is. + scan_range(max_scanned.into()..(max_scanned + 1).into(), Scanned), + // The range below the second-to-last shard is ignored. + scan_range(sap_active..second_to_last_shard_start.into(), Ignored), ]; let actual = suggest_scan_ranges(&st.wallet().conn, Ignored).unwrap(); From 81d7d9349cbfdbc1ca3ff54e5228f0e788dcbc00 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 2 Sep 2023 03:01:23 +0000 Subject: [PATCH 2/4] zcash_client_sqlite: Fix bugs in `update_chain_tip` Co-authored-by: Kris Nuttycombe --- zcash_client_sqlite/src/wallet/scanning.rs | 189 +++++++++++++-------- 1 file changed, 116 insertions(+), 73 deletions(-) diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index 3ea883cd0..7d8d1c79a 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -18,6 +18,8 @@ use crate::{ PRUNING_DEPTH, VERIFY_LOOKAHEAD, }; +use super::wallet_birthday; + #[derive(Debug, Clone, Copy)] enum InsertOn { Left, @@ -715,8 +717,17 @@ pub(crate) fn update_chain_tip( params: &P, new_tip: BlockHeight, ) -> Result<(), SqliteClientError> { + // If the caller provided a chain tip that is before Sapling activation, do nothing. + let sapling_activation = match params.activation_height(NetworkUpgrade::Sapling) { + Some(h) => h, + None => return Ok(()), + }; + // Read the previous max scanned height from the blocks table - let prior_tip = block_height_extrema(conn)?.map(|(_, prior_tip)| prior_tip); + let max_scanned = block_height_extrema(conn)?.map(|(_, max_scanned)| max_scanned); + + // Read the wallet birthday (if known). + let wallet_birthday = wallet_birthday(conn)?; // If the chain tip is below the prior max scanned height, then the caller has caught // the chain in the middle of a reorg. Do nothing; the caller will continue using the @@ -730,11 +741,10 @@ pub(crate) fn update_chain_tip( // We don't check the shard height, as normal usage would have the caller update the // shard state prior to this call, so it is possible and expected to be in a situation // where we should update the tip-related scan ranges but not the shard-related ones. - if let Some(h) = prior_tip { - if new_tip < h { - return Ok(()); - } - } + match max_scanned { + Some(h) if new_tip < h => return Ok(()), + _ => (), + }; // `ScanRange` uses an exclusive upper bound. let chain_end = new_tip + 1; @@ -748,82 +758,115 @@ pub(crate) fn update_chain_tip( )?; // Create a scanning range for the fragment of the last shard leading up to new tip. - let shard_entry = shard_start_height + let tip_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, or connect the prior tip to the new tip. - let tip_entry = prior_tip.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. - if shard_entry.is_none() { - ScanRange::from_parts(prior_tip..chain_end, ScanPriority::Historic) - } else { - // Determine the height to which we expect blocks retrieved from the block source to be stable - // and not subject to being reorg'ed. - let stable_height = new_tip.saturating_sub(PRUNING_DEPTH); + // Create scan ranges to either validate potentially invalid blocks at the wallet's + // view of the chain tip, or connect the prior tip to the new tip. + let tip_entry = max_scanned.map_or_else( + || { + // No blocks have been scanned, so we need to anchor the start of the new scan + // range to something else. + wallet_birthday.map_or_else( + // We don't have a wallet birthday, which means we have no accounts yet. + // We can therefore ignore all blocks up to the chain tip. + || ScanRange::from_parts(sapling_activation..chain_end, ScanPriority::Ignored), + // We have a wallet birthday, so mark all blocks between that and the + // chain tip as `Historic` (performing wallet recovery). + |wallet_birthday| { + ScanRange::from_parts(wallet_birthday..chain_end, ScanPriority::Historic) + }, + ) + }, + |max_scanned| { + // The scan range starts at the block after the max scanned height. Since + // `scan_cached_blocks` retrieves the metadata for the block being connected to + // (if it exists), the connectivity of the scan range to the max scanned block + // will always be checked if relevant. + let min_unscanned = max_scanned + 1; - // 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 `VERIFY_LOOKAHEAD` - // 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 - // prior tip will always be checked. Since `Verify` ranges have maximum priority, even - // if the block source begins downloading blocks from the shard scan range (which ends - // at the stable height) the scanner should not attempt to scan those blocks until the - // tip range has been completely checked and any required rewinds have been performed. - if prior_tip >= stable_height { - // This may overlap the `shard_entry` range and if so will be coalesced with it. - ScanRange::from_parts(prior_tip..chain_end, ScanPriority::ChainTip) + // 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. + if tip_shard_entry.is_none() { + ScanRange::from_parts(min_unscanned..chain_end, ScanPriority::Historic) } else { - // The prior tip is in the range that we now expect to be stable, so we need to verify - // and advance it up to at most the stable height. The shard entry will then cover - // the range to the new tip at the lower `ChainTip` priority. - ScanRange::from_parts( - prior_tip..min(stable_height, prior_tip + VERIFY_LOOKAHEAD), - ScanPriority::Verify, - ) + // Determine the height to which we expect new blocks retrieved from the + // block source to be stable and not subject to being reorg'ed. + let stable_height = new_tip.saturating_sub(PRUNING_DEPTH); + + // If the wallet's max scanned height is above the stable height, + // prioritize the range between it and the new tip as `ChainTip`. + if max_scanned > stable_height { + // We are in the steady-state case, where a wallet is close to the + // chain tip and just needs to catch up. + // + // This overlaps the `tip_shard_entry` range and so will be coalesced + // with it. + ScanRange::from_parts(min_unscanned..chain_end, ScanPriority::ChainTip) + } else { + // In this case, the max scanned height is considered stable relative + // to the chain tip. However, it may be stable or unstable relative to + // the prior chain tip, which we could determine by looking up the + // prior chain tip height from the scan queue. For simplicity we merge + // these two cases together, and pretend that the max scanned block is + // always unstable relative to the prior chain tip. + // + // To confirm its stability, prioritize the `VERIFY_LOOKAHEAD` blocks + // above the max scanned height as `Verify`: + // + // - We use `Verify` to ensure this connectivity check is performed, + // along with any required rewinds, before any `ChainTip` ranges + // (from this or any prior `update_chain_tip` call) are scanned. + // + // - We prioritize `VERIFY_LOOKAHEAD` blocks because this is expected + // to be 12.5 minutes, within which it is reasonable for a user to + // have potentially received a transaction (if they opened their + // wallet to provide an address to someone else, or spent their own + // funds creating a change output), without necessarily having left + // their wallet open long enough for the transaction to be mined and + // the corresponding block to be scanned. + // + // - We limit the range to at most the stable region, to prevent any + // `Verify` ranges from being susceptible to reorgs, and potentially + // interfering with subsequent `Verify` ranges defined by future + // calls to `update_chain_tip`. Any gap between `stable_height` and + // `shard_start_height` will be filled by the scan range merging + // logic with a `Historic` range. + // + // If `max_scanned == stable_height` then this is a zero-length range. + // In this case, any non-empty `(stable_height+1)..shard_start_height` + // will be marked `Historic`, minimising the prioritised blocks at the + // chain tip and allowing for other ranges (for example, `FoundNote`) + // to take priority. + ScanRange::from_parts( + min_unscanned..min(stable_height + 1, min_unscanned + VERIFY_LOOKAHEAD), + ScanPriority::Verify, + ) + } } - } - }); - if let Some(entry) = &shard_entry { + }, + ); + if let Some(entry) = &tip_shard_entry { debug!("{} will update latest shard", entry); } - if let Some(entry) = &tip_entry { - debug!("{} will connect prior tip to new tip", entry); - } + debug!("{} will connect prior scanned state to new tip", tip_entry); - let query_range = match (shard_entry.as_ref(), tip_entry.as_ref()) { - (Some(se), Some(te)) => Some(Range { - start: min(se.block_range().start, te.block_range().start), - end: max(se.block_range().end, te.block_range().end), - }), - (Some(se), None) => Some(se.block_range().clone()), - (None, Some(te)) => Some(te.block_range().clone()), - (None, None) => None, + let query_range = match tip_shard_entry.as_ref() { + Some(se) => Range { + start: min(se.block_range().start, tip_entry.block_range().start), + end: max(se.block_range().end, tip_entry.block_range().end), + }, + None => tip_entry.block_range().clone(), }; - if let Some(query_range) = query_range { - replace_queue_entries::( - conn, - &query_range, - shard_entry.into_iter().chain(tip_entry.into_iter()), - false, - )?; - } else { - // If we have neither shard data nor any existing block data in the database, we should also - // have no existing scan queue entries and can fall back to linear scanning from Sapling - // activation. - if let Some(sapling_activation) = params.activation_height(NetworkUpgrade::Sapling) { - // If the caller provided a chain tip that is before Sapling activation, do - // nothing. - if sapling_activation < chain_end { - let scan_range = - ScanRange::from_parts(sapling_activation..chain_end, ScanPriority::Historic); - insert_queue_entries(conn, Some(scan_range).iter())?; - } - } - } + replace_queue_entries::( + conn, + &query_range, + tip_shard_entry.into_iter().chain(Some(tip_entry)), + false, + )?; Ok(()) } @@ -1341,8 +1384,8 @@ mod tests { assert_matches!( st.wallet().suggest_scan_ranges(), Ok(scan_ranges) if scan_ranges == vec![ - scan_range((sap_active + 319)..(sap_active + 329), Verify), - scan_range((sap_active + 329)..(sap_active + 451), ChainTip), + scan_range((sap_active + 320)..(sap_active + 330), Verify), + scan_range((sap_active + 330)..(sap_active + 451), ChainTip), scan_range((sap_active + 300)..(sap_active + 310), ChainTip) ] ); From afec2ee218fe2dceded3fa2d18f6e67f9d8a1f4f Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 5 Sep 2023 14:51:09 +0000 Subject: [PATCH 3/4] `zcash_client_sqlite`: Fix refactor bug: ignore pre-Sapling chain tips The pre-refactor code did this correctly, but the comment was moved without fully moving the logic. --- zcash_client_sqlite/src/wallet/scanning.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index 7d8d1c79a..00632cfa8 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -719,8 +719,8 @@ pub(crate) fn update_chain_tip( ) -> Result<(), SqliteClientError> { // If the caller provided a chain tip that is before Sapling activation, do nothing. let sapling_activation = match params.activation_height(NetworkUpgrade::Sapling) { - Some(h) => h, - None => return Ok(()), + Some(h) if h <= new_tip => h, + _ => return Ok(()), }; // Read the previous max scanned height from the blocks table From f132af1f3350813f185a2c5c1eaf31a659785fe5 Mon Sep 17 00:00:00 2001 From: str4d Date: Tue, 5 Sep 2023 15:54:15 +0100 Subject: [PATCH 4/4] Documentation fixes Co-authored-by: Daira Emma Hopwood --- zcash_client_sqlite/src/wallet/scanning.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index 00632cfa8..640f6bf9b 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -810,13 +810,13 @@ pub(crate) fn update_chain_tip( // to the chain tip. However, it may be stable or unstable relative to // the prior chain tip, which we could determine by looking up the // prior chain tip height from the scan queue. For simplicity we merge - // these two cases together, and pretend that the max scanned block is - // always unstable relative to the prior chain tip. + // these two cases together, and proceed as though the max scanned + // block is unstable relative to the prior chain tip. // // To confirm its stability, prioritize the `VERIFY_LOOKAHEAD` blocks // above the max scanned height as `Verify`: // - // - We use `Verify` to ensure this connectivity check is performed, + // - We use `Verify` to ensure that a connectivity check is performed, // along with any required rewinds, before any `ChainTip` ranges // (from this or any prior `update_chain_tip` call) are scanned. //