zcash_client_sqlite: Only select notes for which witnesses can be constructed.

This change modifies the implementation of `get_spendable_sapling_notes`
and `select_spendable_sapling_notes` to only return notes at positions
where the associated note commitment tree shard has been fully scanned.
This is slightly more conservative than it needs to be, because
there could be cases where witnesses imported into the tree in the
`shardtree_support` migration cover the complete range of a subtree (and
hence that subtree doesn't need to be re-scanned). However, we can't
detect or depend upon that condition in general without attempting to
create a witness for each note retrieved.

A possible alternative to this approach would be to not bound our query
results on the requested total, and instead attempt to construct a
witness for each note we retrieve, skipping the note if we cannot
construct a witness. However, given that accessing the note commitment
tree can be a costly operation requiring nontrivial deserialization
costs, the more conservative database-oriented approach is perhaps
better.

zcash_client_sqlite/src/wallet/init.rs

@@ -353,14 +353,14 @@ mod tests {
 
     use zcash_client_backend::{
         address::RecipientAddress,
-        data_api::WalletRead,
+        data_api::{scanning::ScanPriority, WalletRead},
         encoding::{encode_extended_full_viewing_key, encode_payment_address},
         keys::{sapling, UnifiedFullViewingKey, UnifiedSpendingKey},
     };
 
     use zcash_primitives::{
         block::BlockHash,
-        consensus::{BlockHeight, BranchId, Parameters},
+        consensus::{BlockHeight, BranchId, NetworkUpgrade, Parameters},
         transaction::{TransactionData, TxVersion},
         zip32::sapling::ExtendedFullViewingKey,
     };
@@ -368,6 +368,7 @@ mod tests {
     use crate::{
         error::SqliteClientError,
         tests::{self, network},
+        wallet::scanning::priority_code,
         AccountId, WalletDb,
     };
 
@@ -558,6 +559,33 @@ mod tests {
         }
 
         let expected_views = vec![
+            // v_sapling_shard_unscanned_ranges
+            format!(
+                "CREATE VIEW v_sapling_shard_unscanned_ranges AS
+                SELECT
+                    shard.shard_index,
+                    shard.shard_index << 16 AS start_position,
+                    (shard.shard_index + 1) << 16 AS end_position_exclusive,
+                    IFNULL(prev_shard.subtree_end_height, {}) AS subtree_start_height,
+                    shard.subtree_end_height AS subtree_end_height,
+                    shard.contains_marked,
+                    scan_queue.block_range_start,
+                    scan_queue.block_range_end,
+                    scan_queue.priority
+                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 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
+                    (
+                        scan_queue.block_range_start <= prev_shard.subtree_end_height
+                        AND (scan_queue.block_range_end - 1) >= shard.subtree_end_height
+                    )
+                WHERE scan_queue.priority != {}",
+                u32::from(tests::network().activation_height(NetworkUpgrade::Sapling).unwrap()),
+                priority_code(&ScanPriority::Scanned),
+            ),
             // v_transactions
             "CREATE VIEW v_transactions AS
             WITH
@@ -649,7 +677,7 @@ mod tests {
             LEFT JOIN sent_note_counts
                       ON sent_note_counts.account_id = notes.account_id
                       AND sent_note_counts.id_tx = notes.id_tx
-            GROUP BY notes.account_id, transactions.id_tx",
+            GROUP BY notes.account_id, transactions.id_tx".to_owned(),
             // v_tx_outputs
             "CREATE VIEW v_tx_outputs AS
             SELECT sapling_received_notes.tx           AS id_tx,
@@ -693,7 +721,7 @@ mod tests {
                       ON (sent_notes.tx, sent_notes.output_pool, sent_notes.output_index) =
                          (sapling_received_notes.tx, 2, sapling_received_notes.output_index)
             WHERE  sapling_received_notes.is_change IS NULL
-               OR  sapling_received_notes.is_change = 0"
+               OR  sapling_received_notes.is_change = 0".to_owned(),
         ];
 
         let mut views_query = db_data
@@ -706,7 +734,7 @@ mod tests {
             let sql: String = row.get(0).unwrap();
             assert_eq!(
                 re.replace_all(&sql, " "),
-                re.replace_all(expected_views[expected_idx], " ")
+                re.replace_all(&expected_views[expected_idx], " ")
             );
             expected_idx += 1;
         }

zcash_client_sqlite/src/wallet/init/migrations.rs

@@ -9,6 +9,7 @@ mod sent_notes_to_internal;
 mod shardtree_support;
 mod ufvk_support;
 mod utxos_table;
+mod v_sapling_shard_unscanned_ranges;
 mod v_transactions_net;
 
 use schemer_rusqlite::RusqliteMigration;
@@ -36,6 +37,8 @@ pub(super) fn all_migrations<P: consensus::Parameters + 'static>(
     //                  received_notes_nullable_nf
     //                 /          |           \
     // shardtree_support    nullifier_map    sapling_memo_consistency
+    //         |
+    // v_sapling_shard_unscanned_ranges
     vec![
         Box::new(initial_setup::Migration {}),
         Box::new(utxos_table::Migration {}),
@@ -58,5 +61,8 @@ pub(super) fn all_migrations<P: consensus::Parameters + 'static>(
         Box::new(sapling_memo_consistency::Migration {
             params: params.clone(),
         }),
+        Box::new(v_sapling_shard_unscanned_ranges::Migration {
+            params: params.clone(),
+        }),
     ]
 }

zcash_client_sqlite/src/wallet/init/migrations/v_sapling_shard_unscanned_ranges.rs

@@ -0,0 +1,75 @@
+//! This migration adds a view that returns the un-scanned ranges associated with each sapling note
+//! commitment tree shard.
+
+use std::collections::HashSet;
+
+use schemer_rusqlite::RusqliteMigration;
+use uuid::Uuid;
+use zcash_client_backend::data_api::scanning::ScanPriority;
+use zcash_primitives::consensus;
+
+use crate::wallet::{init::WalletMigrationError, scanning::priority_code};
+
+use super::shardtree_support;
+
+pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0xfa934bdc_97b6_4980_8a83_b2cb1ac465fd);
+
+pub(super) struct Migration<P> {
+    pub(super) params: P,
+}
+
+impl<P> schemer::Migration for Migration<P> {
+    fn id(&self) -> Uuid {
+        MIGRATION_ID
+    }
+
+    fn dependencies(&self) -> HashSet<Uuid> {
+        [shardtree_support::MIGRATION_ID].into_iter().collect()
+    }
+
+    fn description(&self) -> &'static str {
+        "Adds a view that returns the un-scanned ranges associated with each sapling note commitment tree shard."
+    }
+}
+
+impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
+    type Error = WalletMigrationError;
+
+    fn up(&self, transaction: &rusqlite::Transaction) -> Result<(), Self::Error> {
+        transaction.execute_batch(
+            &format!(
+                "CREATE VIEW v_sapling_shard_unscanned_ranges AS
+                SELECT
+                    shard.shard_index,
+                    shard.shard_index << 16 AS start_position,
+                    (shard.shard_index + 1) << 16 AS end_position_exclusive,
+                    IFNULL(prev_shard.subtree_end_height, {}) AS subtree_start_height,
+                    shard.subtree_end_height AS subtree_end_height,
+                    shard.contains_marked,
+                    scan_queue.block_range_start,
+                    scan_queue.block_range_end,
+                    scan_queue.priority
+                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 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
+                    (
+                        scan_queue.block_range_start <= prev_shard.subtree_end_height
+                        AND (scan_queue.block_range_end - 1) >= shard.subtree_end_height
+                    )
+                WHERE scan_queue.priority != {}",
+                u32::from(self.params.activation_height(consensus::NetworkUpgrade::Sapling).unwrap()),
+                priority_code(&ScanPriority::Scanned),
+            )
+        )?;
+
+        Ok(())
+    }
+
+    fn down(&self, transaction: &rusqlite::Transaction) -> Result<(), Self::Error> {
+        transaction.execute_batch("DROP VIEW v_sapling_shard_unscanned_ranges;")?;
+        Ok(())
+    }
+}

zcash_client_sqlite/src/wallet/sapling.rs

@@ -139,7 +139,15 @@ pub(crate) fn get_spendable_sapling_notes(
          WHERE account = :account
          AND spent IS NULL
          AND transactions.block <= :anchor_height
-         AND id_note NOT IN rarray(:exclude)",
+         AND id_note NOT IN rarray(:exclude)
+         AND NOT EXISTS (
+            SELECT 1 FROM v_sapling_shard_unscanned_ranges unscanned
+            -- select all the unscanned ranges involving the shard containing this note
+            WHERE sapling_received_notes.commitment_tree_position >= unscanned.start_position
+            AND sapling_received_notes.commitment_tree_position < unscanned.end_position_exclusive
+            -- exclude unscanned ranges above the anchor height which don't affect spendability
+            AND unscanned.block_range_start <= :anchor_height
+         )",
     )?;
 
     let excluded: Vec<Value> = exclude.iter().map(|n| Value::from(n.0)).collect();
@@ -165,9 +173,7 @@ pub(crate) fn select_spendable_sapling_notes(
     exclude: &[ReceivedNoteId],
 ) -> Result<Vec<ReceivedSaplingNote<ReceivedNoteId>>, SqliteClientError> {
     // The goal of this SQL statement is to select the oldest notes until the required
-    // value has been reached, and then fetch the witnesses at the desired height for the
+    // value has been reached.
-    // selected notes. This is achieved in several steps:
-    //
     // 1) Use a window function to create a view of all notes, ordered from oldest to
     //    newest, with an additional column containing a running sum:
     //    - Unspent notes accumulate the values of all unspent notes in that note's
@@ -188,11 +194,20 @@ pub(crate) fn select_spendable_sapling_notes(
                  SUM(value)
                     OVER (PARTITION BY account, spent ORDER BY id_note) AS so_far
              FROM sapling_received_notes
-             INNER JOIN transactions ON transactions.id_tx = sapling_received_notes.tx
+             INNER JOIN transactions
+                ON transactions.id_tx = sapling_received_notes.tx
              WHERE account = :account
              AND spent IS NULL
              AND transactions.block <= :anchor_height
              AND id_note NOT IN rarray(:exclude)
+             AND NOT EXISTS (
+                SELECT 1 FROM v_sapling_shard_unscanned_ranges unscanned
+                -- select all the unscanned ranges involving the shard containing this note
+                WHERE sapling_received_notes.commitment_tree_position >= unscanned.start_position
+                AND sapling_received_notes.commitment_tree_position < unscanned.end_position_exclusive
+                -- exclude unscanned ranges above the anchor height which don't affect spendability
+                AND unscanned.block_range_start <= :anchor_height
+             )
          )
          SELECT id_note, diversifier, value, rcm, commitment_tree_position
          FROM eligible WHERE so_far < :target_value
@@ -370,7 +385,7 @@ pub(crate) mod tests {
         block::BlockHash,
         consensus::BranchId,
         legacy::TransparentAddress,
-        memo::Memo,
+        memo::{Memo, MemoBytes},
         sapling::{
             note_encryption::try_sapling_output_recovery, prover::TxProver, Note, PaymentAddress,
         },
@@ -418,10 +433,7 @@ pub(crate) mod tests {
         zcash_client_backend::{
             data_api::wallet::shield_transparent_funds, wallet::WalletTransparentOutput,
         },
-        zcash_primitives::{
+        zcash_primitives::transaction::components::{amount::NonNegativeAmount, OutPoint, TxOut},
-            memo::MemoBytes,
-            transaction::components::{amount::NonNegativeAmount, OutPoint, TxOut},
-        },
     };
 
     pub(crate) fn test_prover() -> impl TxProver {