From 7b57d55c288d23b752e5d3540e12d80e896b5ab7 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 27 Mar 2026 13:35:27 -0600 Subject: [PATCH 1/7] zcash_client_sqlite: Add regression test for truncate_to_chain_state below wallet birthday Tests that truncate_to_chain_state succeeds when the target height is below the wallet birthday (i.e., there is no entry in the blocks table at that height). This exercises the case where a wallet needs to rewind to a height it has never scanned. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../src/data_api/testing/pool.rs | 103 ++++++++++++++++++ zcash_client_sqlite/src/chain.rs | 11 ++ zcash_client_sqlite/src/testing/pool.rs | 7 ++ 3 files changed, 121 insertions(+) diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index 5c604cf553..5fc840a769 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -4370,6 +4370,109 @@ where ); } +pub fn truncate_to_chain_state_below_birthday( + ds_factory: Dsf, + cache: impl TestCache, +) where + Dsf: DataStoreFactory, + ::AccountId: std::fmt::Debug, +{ + // Regression test: truncate_to_chain_state should succeed when truncating to a height + // below the wallet birthday (where no entry exists in the blocks table). Previously, + // this would fail with RequestedRewindInvalid because select_truncation_height requires + // the target height to have an entry in the blocks table. + + use crate::data_api::ll::wallet::PRUNING_DEPTH; + + let mut st = TestBuilder::new() + .with_data_store_factory(ds_factory) + .with_block_cache(cache) + .with_initial_chain_state(|rng, network| { + let birthday_height = network.activation_height(NetworkUpgrade::Sapling).unwrap() + 200; + + let (prior_sapling_roots, sapling_initial_tree) = + Frontier::random_with_prior_subtree_roots(rng, 1u64, NonZeroU8::new(16).unwrap()); + let prior_sapling_roots = prior_sapling_roots + .into_iter() + .map(|root| CommitmentTreeRoot::from_parts(birthday_height - 100, root)) + .collect::>(); + + #[cfg(feature = "orchard")] + let (prior_orchard_roots, orchard_initial_tree) = + Frontier::random_with_prior_subtree_roots(rng, 1u64, NonZeroU8::new(16).unwrap()); + #[cfg(feature = "orchard")] + let prior_orchard_roots = prior_orchard_roots + .into_iter() + .map(|root| CommitmentTreeRoot::from_parts(birthday_height - 100, root)) + .collect::>(); + + InitialChainState { + chain_state: ChainState::new( + birthday_height - 1, + BlockHash([5; 32]), + sapling_initial_tree, + #[cfg(feature = "orchard")] + orchard_initial_tree, + ), + prior_sapling_roots, + #[cfg(feature = "orchard")] + prior_orchard_roots, + } + }) + .with_account_having_current_birthday() + .build(); + + // Generate and scan a few initial blocks from the birthday height. + let other_fvk = T::random_fvk(st.rng_mut()); + let birthday_height = st.test_account().unwrap().birthday().height(); + + for _ in 0..5 { + st.generate_next_block( + &other_fvk, + AddressType::DefaultExternal, + Zatoshis::const_from_u64(10000), + ); + } + st.scan_cached_blocks(birthday_height, 5); + + // Generate and scan blocks well beyond PRUNING_DEPTH to ensure early checkpoints + // are pruned from the note commitment tree. + let extra_blocks = PRUNING_DEPTH + 10; + for _ in 0..extra_blocks { + st.generate_next_block( + &other_fvk, + AddressType::DefaultExternal, + Zatoshis::const_from_u64(5000), + ); + } + st.scan_cached_blocks(birthday_height + 5, extra_blocks as usize); + + // Get the prior chain state from the account birthday. This chain state is at + // birthday_height - 1, which has valid tree frontiers but NO entry in the blocks + // table (since the wallet never scanned a block at that height). + let prior_chain_state = st + .test_account() + .unwrap() + .birthday() + .prior_chain_state() + .clone(); + + // This should succeed. On the buggy code, this fails with RequestedRewindInvalid + // because select_truncation_height cannot find an entry in the blocks table at the + // target height. + let _target_height = prior_chain_state.block_height(); + st.wallet_mut() + .truncate_to_chain_state(prior_chain_state) + .expect("truncate_to_chain_state below birthday should succeed"); + + // All blocks were above the target height, so they should have been removed. + assert_eq!( + st.wallet().get_block_hash(birthday_height).unwrap(), + None, + "blocks at birthday height should be removed after truncating below birthday" + ); +} + pub fn reorg_to_checkpoint(ds_factory: Dsf, cache: C) where Dsf: DataStoreFactory, diff --git a/zcash_client_sqlite/src/chain.rs b/zcash_client_sqlite/src/chain.rs index 36911c4680..621672ce65 100644 --- a/zcash_client_sqlite/src/chain.rs +++ b/zcash_client_sqlite/src/chain.rs @@ -373,6 +373,17 @@ mod tests { testing::pool::truncate_to_chain_state::() } + #[test] + fn truncate_to_chain_state_below_birthday_sapling() { + testing::pool::truncate_to_chain_state_below_birthday::() + } + + #[test] + #[cfg(feature = "orchard")] + fn truncate_to_chain_state_below_birthday_orchard() { + testing::pool::truncate_to_chain_state_below_birthday::() + } + #[test] fn reorg_to_checkpoint_sapling() { testing::pool::reorg_to_checkpoint::() diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 0fcaa40a21..87fe0018ed 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -312,6 +312,13 @@ pub(crate) fn truncate_to_chain_state() { ) } +pub(crate) fn truncate_to_chain_state_below_birthday() { + zcash_client_backend::data_api::testing::pool::truncate_to_chain_state_below_birthday::( + TestDbFactory::default(), + BlockCache::new(), + ) +} + pub(crate) fn reorg_to_checkpoint() { zcash_client_backend::data_api::testing::pool::reorg_to_checkpoint::( TestDbFactory::default(), From 1931a496988cedbe09a99559234d71b89c5d1055 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 27 Mar 2026 09:48:16 -0600 Subject: [PATCH 2/7] zcash_client_sqlite: Fix min_shared_checkpoint_height to query checkpoint intersection The previous query computed MAX(MIN(sapling), MIN(orchard)), which returns a lower bound on where both trees have started checkpointing but does not guarantee that a checkpoint at that specific height exists in both tables. This caused incorrect safe rewind height reporting and incorrect truncation targets in truncate_to_chain_state. The fix queries for the actual minimum checkpoint height that exists in the intersection of both checkpoint tables, matching the semantics used by select_truncation_height. Co-Authored-By: Claude Opus 4.6 (1M context) --- zcash_client_sqlite/src/wallet.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 88f3ecce24..840124e163 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -3262,20 +3262,21 @@ pub(crate) fn set_transaction_status( Ok(()) } -/// Find the minimum checkpoint height for each tree and take the maximum of those minima. -/// -/// This returns the earliest height at which all trees have a checkpoint. The orchard table exists -/// unconditionally but is empty when the `orchard` feature is not active; MAX ignores NULLs, so -/// this correctly returns only the sapling minimum in that case. +/// Returns the minimum checkpoint height that exists in all note commitment trees that contain +/// data. When both trees have checkpoints, returns the minimum of their intersection. When only +/// one tree has checkpoints, returns that tree's minimum. Returns `None` when both are empty. fn min_shared_checkpoint_height( conn: &rusqlite::Connection, ) -> Result, SqliteClientError> { Ok(conn .query_row( - "SELECT MAX(m) FROM ( - SELECT MIN(checkpoint_id) AS m FROM sapling_tree_checkpoints + "SELECT MIN(checkpoint_id) FROM ( + SELECT checkpoint_id FROM sapling_tree_checkpoints + WHERE checkpoint_id IN (SELECT checkpoint_id FROM orchard_tree_checkpoints) + OR NOT EXISTS (SELECT 1 FROM orchard_tree_checkpoints) UNION ALL - SELECT MIN(checkpoint_id) AS m FROM orchard_tree_checkpoints + SELECT checkpoint_id FROM orchard_tree_checkpoints + WHERE NOT EXISTS (SELECT 1 FROM sapling_tree_checkpoints) )", [], |row| row.get::<_, Option>(0), @@ -3313,9 +3314,8 @@ fn select_truncation_height( .map_or_else( || { // If we don't have a checkpoint at a height less than or equal to the requested - // truncation height, query for the minimum height to which it's possible for us to - // truncate so that we can report it to the caller. We take the maximum of the per-tree - // minimums, since that is the earliest height at which all trees have a checkpoint. + // truncation height, query for the minimum shared checkpoint height so that we can + // report the safe rewind height to the caller. Err(SqliteClientError::RequestedRewindInvalid { safe_rewind_height: min_shared_checkpoint_height(conn)?, requested_height, From ace053db5bd9137656cb271f60bfcc1f6ed355e4 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 27 Mar 2026 11:20:30 -0600 Subject: [PATCH 3/7] zcash_client_sqlite: Allow truncate_to_chain_state to truncate to unscanned heights. Previously, truncate_to_chain_state could fail if no entry existed in the `blocks` table at the given height. This change repeals that restriction. --- zcash_client_sqlite/src/wallet.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 840124e163..0e41b3be5b 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -3501,18 +3501,22 @@ pub(crate) fn truncate_to_chain_state( Err(SqliteClientError::RequestedRewindInvalid { safe_rewind_height, .. }) => { - // The safe rewind height is at a position greater than the requested height, so we - // truncate to that height first to clear out checkpoints that would otherwise cause - // the checkpoint we're about to insert to be immediately pruned, then, we fall through - // to insertion of the frontier. if let Some(min_checkpoint_height) = safe_rewind_height { - truncate_to_height( + // The safe rewind height is at a position greater than the requested height, so we + // truncate wallet data and tree state to the earliest shared checkpoint. This removes + // blocks and transaction data above that height. Given that we always add + // checkpoints in pairs, if there are at least two checkpoints in any table then + // the minimum between them will result in checkpoints having been removed, and so + // there will be space for the checkpoint that is about to be inserted. + truncate_to_height_internal( wdb.conn.0, &wdb.params, #[cfg(feature = "transparent-inputs")] &wdb.gap_limits, min_checkpoint_height, )?; + } else { + // There are no checkpoints in either table; just continue. } } Err(e) => { @@ -3520,11 +3524,7 @@ pub(crate) fn truncate_to_chain_state( } }; - // If there are checkpoints, truncate to the earliest common one first. If there are no - // checkpoints at all, we can skip this step and proceed directly to inserting the frontier. - // Insert the frontier from the chain state, creating a checkpoint at the target height. - // Because we just truncated down to a single checkpoint, there is room for this new one. wdb.with_sapling_tree_mut(|tree| { tree.insert_frontier( chain_state.final_sapling_tree().clone(), @@ -3548,8 +3548,11 @@ pub(crate) fn truncate_to_chain_state( Ok::<_, SqliteClientError>(()) })?; - // Now truncate to the target height checkpoint we just created. - let truncated_height = truncate_to_height( + // Now truncate wallet data to the target height. We use truncate_to_height_internal + // directly (bypassing select_truncation_height) because the frontier insertion created + // tree checkpoints at target_height but did not add a blocks table entry, and + // select_truncation_height requires the height to be present in the blocks table. + let truncated_height = truncate_to_height_internal( wdb.conn.0, &wdb.params, #[cfg(feature = "transparent-inputs")] From 67070dc5c73e8ac4ab3c17a134f33aa4f6274267 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Sat, 28 Mar 2026 16:29:07 -0600 Subject: [PATCH 4/7] zcash_client_sqlite: Improve legibility of `min_shared_checkpoint_height` query. --- zcash_client_sqlite/src/wallet.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 0e41b3be5b..3ed342f853 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -3271,12 +3271,19 @@ fn min_shared_checkpoint_height( Ok(conn .query_row( "SELECT MIN(checkpoint_id) FROM ( - SELECT checkpoint_id FROM sapling_tree_checkpoints - WHERE checkpoint_id IN (SELECT checkpoint_id FROM orchard_tree_checkpoints) - OR NOT EXISTS (SELECT 1 FROM orchard_tree_checkpoints) - UNION ALL - SELECT checkpoint_id FROM orchard_tree_checkpoints - WHERE NOT EXISTS (SELECT 1 FROM sapling_tree_checkpoints) + -- When both trees have checkpoints, returns the minimum of their intersection. + SELECT MIN(sc.checkpoint_id) AS checkpoint_id + FROM sapling_tree_checkpoints sc + JOIN orchard_tree_checkpoints oc ON oc.checkpoint_id = sc.checkpoint_id + -- When only one tree has checkpoints, returns that tree's minimum. + UNION ALL + SELECT MIN(sc.checkpoint_id) AS checkpoint_id + FROM sapling_tree_checkpoints sc + WHERE NOT EXISTS (SELECT 1 FROM orchard_tree_checkpoints) + UNION ALL + SELECT MIN(oc.checkpoint_id) AS checkpoint_id + FROM orchard_tree_checkpoints oc + WHERE NOT EXISTS (SELECT 1 FROM sapling_tree_checkpoints) )", [], |row| row.get::<_, Option>(0), From 3d91f05f4c277032d96d8b942a79b9b94f3ab357 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Sun, 29 Mar 2026 17:56:34 -0600 Subject: [PATCH 5/7] zcash_client_sqlite: Test truncate_to_chain_state when target > max_scanned. This tests the scenario where the wallet has been truncated to an earlier chain state (for example, when adding an account with an earlier birthday) and then truncate_to_chain_state is called again with a *greater* height, such that a discontinuity would be introduced in the subtree roots by the chain state insertion. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../src/data_api/testing/pool.rs | 120 ++++++++++++++++++ zcash_client_sqlite/src/chain.rs | 11 ++ zcash_client_sqlite/src/testing/pool.rs | 7 + 3 files changed, 138 insertions(+) diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index 5fc840a769..71b0651672 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -4473,6 +4473,126 @@ pub fn truncate_to_chain_state_below_birthday( ); } +pub fn truncate_to_chain_state_above_scanned( + ds_factory: Dsf, + cache: impl TestCache, +) where + Dsf: DataStoreFactory, + ::AccountId: std::fmt::Debug, +{ + // Regression test: when truncate_to_chain_state is called with a target height above + // the max scanned height, the frontier insertion must be skipped (it would introduce + // a subtree root discontinuity) but the scan queue must still be trimmed. Without the + // fix, inserting a frontier in shard 2 when the wallet only has shard 0 fails because + // shard 1's subtree root is unknown. + + use crate::data_api::ll::wallet::PRUNING_DEPTH; + + let mut st = TestDsl::with_sapling_birthday_account(ds_factory, cache).build::(); + + let birthday_height = st.test_account().unwrap().birthday().height(); + + // Generate and scan initial blocks, then scan beyond PRUNING_DEPTH to ensure + // early checkpoints are pruned. + let other_fvk = T::random_fvk(st.rng_mut()); + let initial_blocks = 5u32; + for _ in 0..initial_blocks { + st.generate_next_block( + &other_fvk, + AddressType::DefaultExternal, + Zatoshis::const_from_u64(10000), + ); + } + st.scan_cached_blocks(birthday_height, initial_blocks as usize); + + let extra_blocks = PRUNING_DEPTH + 10; + for _ in 0..extra_blocks { + st.generate_next_block( + &other_fvk, + AddressType::DefaultExternal, + Zatoshis::const_from_u64(5000), + ); + } + st.scan_cached_blocks(birthday_height + initial_blocks, extra_blocks as usize); + let max_scanned = birthday_height + initial_blocks + extra_blocks - 1; + + // Simulate downloading subtree roots from the network: add a known subtree root + // for shard 0 only. This creates a state where shard 0 exists in the shard store + // but shard 1 does not. + T::put_subtree_roots( + &mut st, + 0, + &[CommitmentTreeRoot::from_parts( + birthday_height, + T::empty_tree_leaf(), + )], + ) + .unwrap(); + + // Extend the scan queue beyond max_scanned. + let chain_tip = max_scanned + 500; + st.wallet_mut().update_chain_tip(chain_tip).unwrap(); + + // Construct a ChainState above max_scanned with a frontier in shard 2. The wallet + // has shard 0 (from put_subtree_roots above) but does NOT have shard 1. Inserting a + // frontier in shard 2 introduces a discontinuity because shard 1's subtree root is + // unknown. + let target_height = max_scanned + 50; + let shard_2_tree_size: u64 = (0x2 << 16) + 2; + let (_, shard2_sapling_frontier) = Frontier::random_with_prior_subtree_roots( + st.rng_mut(), + shard_2_tree_size, + NonZeroU8::new(16).unwrap(), + ); + #[cfg(feature = "orchard")] + let (_, shard2_orchard_frontier) = Frontier::random_with_prior_subtree_roots( + st.rng_mut(), + shard_2_tree_size, + NonZeroU8::new(16).unwrap(), + ); + + let target_chain_state = ChainState::new( + target_height, + BlockHash([7; 32]), + shard2_sapling_frontier, + #[cfg(feature = "orchard")] + shard2_orchard_frontier, + ); + + // Verify the scan queue extends beyond the target. + let pre_truncation_tip = st + .wallet() + .chain_height() + .unwrap() + .expect("chain tip should be set"); + assert!(pre_truncation_tip > target_height); + + // Truncate to the target height, which is above max_scanned. With the fix, this + // skips the frontier insertion (avoiding the discontinuity) and trims the scan queue. + // Without the fix, this would fail because inserting a frontier in shard 2 requires + // shard 1's subtree root, which is unknown. + st.wallet_mut() + .truncate_to_chain_state(target_chain_state) + .expect("truncate_to_chain_state above max scanned should succeed"); + + // The scan queue should have been trimmed to the target height. + let post_truncation_tip = st + .wallet() + .chain_height() + .unwrap() + .expect("chain tip should still be set after truncation"); + assert_eq!( + post_truncation_tip, target_height, + "scan queue should be trimmed to target height, not extend to the old chain tip" + ); + + // Existing blocks below max_scanned should be preserved. + assert!( + st.wallet().get_block_hash(max_scanned).unwrap().is_some(), + "blocks at max_scanned should be preserved" + ); +} + pub fn reorg_to_checkpoint(ds_factory: Dsf, cache: C) where Dsf: DataStoreFactory, diff --git a/zcash_client_sqlite/src/chain.rs b/zcash_client_sqlite/src/chain.rs index 621672ce65..20af5f26b8 100644 --- a/zcash_client_sqlite/src/chain.rs +++ b/zcash_client_sqlite/src/chain.rs @@ -384,6 +384,17 @@ mod tests { testing::pool::truncate_to_chain_state_below_birthday::() } + #[test] + fn truncate_to_chain_state_above_scanned_sapling() { + testing::pool::truncate_to_chain_state_above_scanned::() + } + + #[test] + #[cfg(feature = "orchard")] + fn truncate_to_chain_state_above_scanned_orchard() { + testing::pool::truncate_to_chain_state_above_scanned::() + } + #[test] fn reorg_to_checkpoint_sapling() { testing::pool::reorg_to_checkpoint::() diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 87fe0018ed..4db2a7643c 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -319,6 +319,13 @@ pub(crate) fn truncate_to_chain_state_below_birthday() { ) } +pub(crate) fn truncate_to_chain_state_above_scanned() { + zcash_client_backend::data_api::testing::pool::truncate_to_chain_state_above_scanned::( + TestDbFactory::default(), + BlockCache::new(), + ) +} + pub(crate) fn reorg_to_checkpoint() { zcash_client_backend::data_api::testing::pool::reorg_to_checkpoint::( TestDbFactory::default(), From eb3ea655f03ffaea7537621d0e1b2ff14f25f3b8 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Sat, 28 Mar 2026 18:14:09 -0600 Subject: [PATCH 6/7] zcash_client_sqlite: Do not insert birthday chain states above max-scanned height. `Shardtree` will return an error if insertion results in a discontinuity in subtree roots. When adding an account after truncation, this can cause the insertion to fail. --- zcash_client_sqlite/src/wallet.rs | 265 ++++++++++++++++-------------- 1 file changed, 143 insertions(+), 122 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 3ed342f853..68f9bfb2d7 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -539,59 +539,65 @@ pub(crate) fn add_account( birthday: birthday.height(), }; - // If a birthday frontier is available, insert it into the note commitment tree. If the - // birthday frontier is the empty frontier, we don't need to do anything. - if let Some(frontier) = birthday.sapling_frontier().value() { - debug!("Inserting Sapling frontier into ShardTree: {:?}", frontier); - let shard_store = - SqliteShardStore::<_, ::sapling::Node, SAPLING_SHARD_HEIGHT>::from_connection( - conn, - crate::SAPLING_TABLES_PREFIX, + // If a birthday frontiers are available and the birthday height is less than or equal to the + // max-scanned height, insert them into the note commitment trees. Otherwise, we don't need to + // do anything. + if block_max_scanned(conn, params)? + .iter() + .any(|m| m.block_height() > birthday.height()) + { + if let Some(frontier) = birthday.sapling_frontier().value() { + debug!("Inserting Sapling frontier into ShardTree: {:?}", frontier); + let shard_store = + SqliteShardStore::<_, ::sapling::Node, SAPLING_SHARD_HEIGHT>::from_connection( + conn, + crate::SAPLING_TABLES_PREFIX, + )?; + let mut shard_tree: ShardTree< + _, + { ::sapling::NOTE_COMMITMENT_TREE_DEPTH }, + SAPLING_SHARD_HEIGHT, + > = ShardTree::new(shard_store, PRUNING_DEPTH.try_into().unwrap()); + shard_tree.insert_frontier_nodes( + frontier.clone(), + Retention::Checkpoint { + // This subtraction is safe, because all leaves in the tree appear in blocks, and + // the invariant that birthday.height() always corresponds to the block for which + // `frontier` is the tree state at the start of the block. Together, this means + // there exists a prior block for which frontier is the tree state at the end of + // the block. + id: birthday.height() - 1, + marking: Marking::Reference, + }, )?; - let mut shard_tree: ShardTree< - _, - { ::sapling::NOTE_COMMITMENT_TREE_DEPTH }, - SAPLING_SHARD_HEIGHT, - > = ShardTree::new(shard_store, PRUNING_DEPTH.try_into().unwrap()); - shard_tree.insert_frontier_nodes( - frontier.clone(), - Retention::Checkpoint { - // This subtraction is safe, because all leaves in the tree appear in blocks, and - // the invariant that birthday.height() always corresponds to the block for which - // `frontier` is the tree state at the start of the block. Together, this means - // there exists a prior block for which frontier is the tree state at the end of - // the block. - id: birthday.height() - 1, - marking: Marking::Reference, - }, - )?; - } + } - #[cfg(feature = "orchard")] - if let Some(frontier) = birthday.orchard_frontier().value() { - debug!("Inserting Orchard frontier into ShardTree: {:?}", frontier); - let shard_store = SqliteShardStore::< - _, - ::orchard::tree::MerkleHashOrchard, - ORCHARD_SHARD_HEIGHT, - >::from_connection(conn, crate::ORCHARD_TABLES_PREFIX)?; - let mut shard_tree: ShardTree< - _, - { ::orchard::NOTE_COMMITMENT_TREE_DEPTH as u8 }, - ORCHARD_SHARD_HEIGHT, - > = ShardTree::new(shard_store, PRUNING_DEPTH.try_into().unwrap()); - shard_tree.insert_frontier_nodes( - frontier.clone(), - Retention::Checkpoint { - // This subtraction is safe, because all leaves in the tree appear in blocks, and - // the invariant that birthday.height() always corresponds to the block for which - // `frontier` is the tree state at the start of the block. Together, this means - // there exists a prior block for which frontier is the tree state at the end of - // the block. - id: birthday.height() - 1, - marking: Marking::Reference, - }, - )?; + #[cfg(feature = "orchard")] + if let Some(frontier) = birthday.orchard_frontier().value() { + debug!("Inserting Orchard frontier into ShardTree: {:?}", frontier); + let shard_store = SqliteShardStore::< + _, + ::orchard::tree::MerkleHashOrchard, + ORCHARD_SHARD_HEIGHT, + >::from_connection(conn, crate::ORCHARD_TABLES_PREFIX)?; + let mut shard_tree: ShardTree< + _, + { ::orchard::NOTE_COMMITMENT_TREE_DEPTH as u8 }, + ORCHARD_SHARD_HEIGHT, + > = ShardTree::new(shard_store, PRUNING_DEPTH.try_into().unwrap()); + shard_tree.insert_frontier_nodes( + frontier.clone(), + Retention::Checkpoint { + // This subtraction is safe, because all leaves in the tree appear in blocks, and + // the invariant that birthday.height() always corresponds to the block for which + // `frontier` is the tree state at the start of the block. Together, this means + // there exists a prior block for which frontier is the tree state at the end of + // the block. + id: birthday.height() - 1, + marking: Marking::Reference, + }, + )?; + } } // The ignored range always starts at Sapling activation @@ -3483,82 +3489,97 @@ pub(crate) fn truncate_to_chain_state( ) -> Result<(), SqliteClientError> { let target_height = chain_state.block_height(); - // Try the simple case first: if a checkpoint exists at or below the target height, - // truncate_to_height will succeed directly. - match select_truncation_height(wdb.conn.0, target_height) { - Ok(h) => { - if h == target_height { - // There is a checkpoint for the requested height, we can just truncate to it and - // return. - return truncate_to_height_internal( - wdb.conn.0, - &wdb.params, - #[cfg(feature = "transparent-inputs")] - &wdb.gap_limits, - h, - ) - .map(|_| ()); - } else { - // The returned height corresponds to a checkpoint that is below the requested - // height. Inserting a checkpoint at a height *greater* than this returned height - // may cause an older checkpoint to be deleted, but that's fine, so we just fall - // through here. + // Only truncate trees when the maximum scanned height is greater than the target height. When + // the target height is at or above the max scanned height, we skip frontier insertion (it is + // unnecessary at the max scanned height, and could introduce a subtree root discontinuity + // above it,and the frontier will be added naturally during scanning). We will however still + // need to truncate the scan queue so that ranges above the target are removed. + let truncate_trees = block_max_scanned(wdb.conn.0, &wdb.params)? + .is_some_and(|meta| meta.block_height() > target_height); + + if truncate_trees { + // Try the simple case first: if a checkpoint exists at or below the target height, + // truncate_to_height will succeed directly. + match select_truncation_height(wdb.conn.0, target_height) { + Ok(h) => { + if h == target_height { + // There is a checkpoint for the requested height, we can just truncate to + // it and return. + return truncate_to_height_internal( + wdb.conn.0, + &wdb.params, + #[cfg(feature = "transparent-inputs")] + &wdb.gap_limits, + h, + ) + .map(|_| ()); + } else { + // The returned height corresponds to a checkpoint that is below the + // requested height. Inserting a checkpoint at a height *greater* than this + // returned height may cause an older checkpoint to be deleted, but that's + // fine, so we just fall through here. + } } - } - Err(SqliteClientError::RequestedRewindInvalid { - safe_rewind_height, .. - }) => { - if let Some(min_checkpoint_height) = safe_rewind_height { - // The safe rewind height is at a position greater than the requested height, so we - // truncate wallet data and tree state to the earliest shared checkpoint. This removes - // blocks and transaction data above that height. Given that we always add - // checkpoints in pairs, if there are at least two checkpoints in any table then - // the minimum between them will result in checkpoints having been removed, and so - // there will be space for the checkpoint that is about to be inserted. - truncate_to_height_internal( - wdb.conn.0, - &wdb.params, - #[cfg(feature = "transparent-inputs")] - &wdb.gap_limits, - min_checkpoint_height, - )?; - } else { - // There are no checkpoints in either table; just continue. + Err(SqliteClientError::RequestedRewindInvalid { + safe_rewind_height, .. + }) => { + if let Some(min_checkpoint_height) = safe_rewind_height { + // The safe rewind height is at a position greater than the requested + // height, so we truncate wallet data and tree state to the earliest shared + // checkpoint. This removes blocks and transaction data above that height. + // Given that we always add checkpoints in pairs, if there are at least two + // checkpoints in any table then the minimum between them will result in + // checkpoints having been removed, and so there will be space for the + // checkpoint that is about to be inserted. + truncate_to_height_internal( + wdb.conn.0, + &wdb.params, + #[cfg(feature = "transparent-inputs")] + &wdb.gap_limits, + min_checkpoint_height, + )?; + } else { + // There are no checkpoints in either table; just continue. + } } - } - Err(e) => { - return Err(e); - } - }; + Err(e) => { + return Err(e); + } + }; - // Insert the frontier from the chain state, creating a checkpoint at the target height. - wdb.with_sapling_tree_mut(|tree| { - tree.insert_frontier( - chain_state.final_sapling_tree().clone(), - Retention::Checkpoint { - id: target_height, - marking: Marking::None, - }, - )?; - Ok::<_, SqliteClientError>(()) - })?; + // Insert the frontier from the chain state, creating a checkpoint at the target + // height. + wdb.with_sapling_tree_mut(|tree| { + tree.insert_frontier( + chain_state.final_sapling_tree().clone(), + Retention::Checkpoint { + id: target_height, + marking: Marking::None, + }, + )?; + Ok::<_, SqliteClientError>(()) + })?; - #[cfg(feature = "orchard")] - wdb.with_orchard_tree_mut(|tree| { - tree.insert_frontier( - chain_state.final_orchard_tree().clone(), - Retention::Checkpoint { - id: target_height, - marking: Marking::None, - }, - )?; - Ok::<_, SqliteClientError>(()) - })?; + #[cfg(feature = "orchard")] + wdb.with_orchard_tree_mut(|tree| { + tree.insert_frontier( + chain_state.final_orchard_tree().clone(), + Retention::Checkpoint { + id: target_height, + marking: Marking::None, + }, + )?; + Ok::<_, SqliteClientError>(()) + })?; + } - // Now truncate wallet data to the target height. We use truncate_to_height_internal - // directly (bypassing select_truncation_height) because the frontier insertion created - // tree checkpoints at target_height but did not add a blocks table entry, and - // select_truncation_height requires the height to be present in the blocks table. + // Truncate wallet data to the target height. This always trims the scan queue so that + // ranges above target_height are removed. When truncate_trees is true, it also truncates + // blocks and note commitment trees (using the checkpoint created by the frontier insertion + // above). We use truncate_to_height_internal directly (bypassing select_truncation_height) + // because the frontier insertion created tree checkpoints at target_height but did not add + // a blocks table entry, and select_truncation_height requires the height to be present in + // the blocks table. let truncated_height = truncate_to_height_internal( wdb.conn.0, &wdb.params, From 5921afca7994cccfad3a541a3171b2778ec08f35 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 30 Mar 2026 13:22:58 -0600 Subject: [PATCH 7/7] Address comments from code review. --- zcash_client_sqlite/src/wallet.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 68f9bfb2d7..e3f0a27cbd 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -542,10 +542,7 @@ pub(crate) fn add_account( // If a birthday frontiers are available and the birthday height is less than or equal to the // max-scanned height, insert them into the note commitment trees. Otherwise, we don't need to // do anything. - if block_max_scanned(conn, params)? - .iter() - .any(|m| m.block_height() > birthday.height()) - { + if block_max_scanned(conn, params)?.is_some_and(|m| m.block_height() > birthday.height()) { if let Some(frontier) = birthday.sapling_frontier().value() { debug!("Inserting Sapling frontier into ShardTree: {:?}", frontier); let shard_store = @@ -2834,8 +2831,7 @@ fn parse_block_metadata( #[cfg(feature = "orchard")] if _params .activation_height(NetworkUpgrade::Nu5) - .iter() - .any(|nu5_activation| &block_height >= nu5_activation) + .is_some_and(|nu5_activation| block_height >= nu5_activation) { _orchard_tree_size_opt } else { @@ -3477,8 +3473,8 @@ pub(crate) fn truncate_to_height_internal( /// This function enables precise truncation even when the target height's checkpoint has been /// pruned from the note commitment tree. It works in two cases: /// -/// - If a checkpoint exists at or below the target height, it delegates to -/// [`truncate_to_height`] directly. +/// - If a checkpoint exists at the target height, this behaves identically to +/// [`truncate_to_height`]. /// - If the target height is below the oldest available checkpoint, it first truncates to the /// oldest checkpoint to ensure that the a checkpoint added at the provided frontier position does /// not get immediately pruned, then inserts the provided frontier as a new checkpoint at the @@ -3492,8 +3488,8 @@ pub(crate) fn truncate_to_chain_state( // Only truncate trees when the maximum scanned height is greater than the target height. When // the target height is at or above the max scanned height, we skip frontier insertion (it is // unnecessary at the max scanned height, and could introduce a subtree root discontinuity - // above it,and the frontier will be added naturally during scanning). We will however still - // need to truncate the scan queue so that ranges above the target are removed. + // above it; the frontier will be added naturally during scanning). We will however still need + // to truncate the scan queue so that ranges above the target are removed. let truncate_trees = block_max_scanned(wdb.conn.0, &wdb.params)? .is_some_and(|meta| meta.block_height() > target_height);