From 504efcfab77bf65583f2d88daf2447c2df5547b5 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 8 Sep 2023 04:04:10 +0000 Subject: [PATCH] zcash_client_sqlite: Fix scan range matching with incomplete shard The `v_sapling_shard_scan_ranges` view pairs every scan range with every shard range, such that each row shows an overlapping pair. For the complete shards, this is an overlap check between two ranges, which the previous query was performing correctly (if verbosely). For the last incomplete shard, we have a half-open range that needs to be handled separately. The previous query only handled the case where a scan range was contained within the last shard, and did not handle the case where the scan range contained the last shard. This led to a puzzling bug, where `WalletDb::get_wallet_summary` was sometimes treating any note received within the last shard as part of the wallet's pending balance. If the wallet's scan queue contained a range that encompassed the last incomplete shard, the bug in the `v_sapling_shard_scan_ranges` view meant that it omitted any mention of the last shard, which translated into these notes being considered unmined when joining `sapling_received_notes` against the sub-view `v_sapling_shards_scan_state`. The bug was made harder to diagnose due to the previous commit's bug that was causing scan ranges to not be correctly merged; this resulted in smaller scan ranges that were more likely to be contained within the last shard, making it visible in `v_sapling_shard_scan_ranges` and enabling notes to be detected as mined. The fixed view uses a simpler query that enables us to handle complete and incomplete shards together. Time spent investigating and fixing: 4.5 hours Co-authored-by: Kris Nuttycombe --- zcash_client_sqlite/src/wallet/init.rs | 14 +++++++------- .../migrations/v_sapling_shard_unscanned_ranges.rs | 14 +++++++------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index a8ba6a80d..18c27f303 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -383,14 +383,14 @@ mod tests { FROM sapling_tree_shards shard LEFT OUTER JOIN sapling_tree_shards prev_shard ON shard.shard_index = prev_shard.shard_index + 1 - INNER JOIN scan_queue ON - (scan_queue.block_range_start >= subtree_start_height AND shard.subtree_end_height IS NULL) OR - (scan_queue.block_range_start BETWEEN subtree_start_height AND shard.subtree_end_height) OR - ((scan_queue.block_range_end - 1) BETWEEN subtree_start_height AND shard.subtree_end_height) OR + -- Join with scan ranges that overlap with the subtree's involved blocks. + INNER JOIN scan_queue ON ( + subtree_start_height < scan_queue.block_range_end AND ( - scan_queue.block_range_start <= prev_shard.subtree_end_height - AND (scan_queue.block_range_end - 1) >= shard.subtree_end_height - )", + scan_queue.block_range_start <= shard.subtree_end_height OR + shard.subtree_end_height IS NULL + ) + )", u32::from(st.network().activation_height(NetworkUpgrade::Sapling).unwrap()), ), // v_sapling_shard_unscanned_ranges diff --git a/zcash_client_sqlite/src/wallet/init/migrations/v_sapling_shard_unscanned_ranges.rs b/zcash_client_sqlite/src/wallet/init/migrations/v_sapling_shard_unscanned_ranges.rs index 7a72942c0..dc19265d2 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/v_sapling_shard_unscanned_ranges.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/v_sapling_shard_unscanned_ranges.rs @@ -52,14 +52,14 @@ impl RusqliteMigration for Migration

{ FROM sapling_tree_shards shard LEFT OUTER JOIN sapling_tree_shards prev_shard ON shard.shard_index = prev_shard.shard_index + 1 - INNER JOIN scan_queue ON - (scan_queue.block_range_start >= subtree_start_height AND shard.subtree_end_height IS NULL) OR - (scan_queue.block_range_start BETWEEN subtree_start_height AND shard.subtree_end_height) OR - ((scan_queue.block_range_end - 1) BETWEEN subtree_start_height AND shard.subtree_end_height) OR + -- Join with scan ranges that overlap with the subtree's involved blocks. + INNER JOIN scan_queue ON ( + subtree_start_height < scan_queue.block_range_end AND ( - scan_queue.block_range_start <= prev_shard.subtree_end_height - AND (scan_queue.block_range_end - 1) >= shard.subtree_end_height - )", + scan_queue.block_range_start <= shard.subtree_end_height OR + shard.subtree_end_height IS NULL + ) + )", SAPLING_SHARD_HEIGHT, SAPLING_SHARD_HEIGHT, u32::from(self.params.activation_height(NetworkUpgrade::Sapling).unwrap()),