From 50ea2a5b0f33a049e7c12f39b6b60c4e11232dff Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 9 Aug 2023 13:54:07 -0600 Subject: [PATCH] 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 | 40 ++++++++-- .../src/wallet/init/migrations.rs | 6 ++ .../v_sapling_shard_unscanned_ranges.rs | 75 +++++++++++++++++++ zcash_client_sqlite/src/wallet/sapling.rs | 36 ++++++--- 4 files changed, 139 insertions(+), 18 deletions(-) create mode 100644 zcash_client_sqlite/src/wallet/init/migrations/v_sapling_shard_unscanned_ranges.rs diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index a14642516..da9657ca4 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/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; } @@ -971,7 +999,7 @@ mod tests { wdb.conn.execute( "INSERT INTO transactions (block, id_tx, txid, raw) VALUES (0, 0, :txid, :tx_bytes)", named_params![ - ":txid": tx.txid().as_ref(), + ":txid": tx.txid().as_ref(), ":tx_bytes": &tx_bytes[..] ], )?; diff --git a/zcash_client_sqlite/src/wallet/init/migrations.rs b/zcash_client_sqlite/src/wallet/init/migrations.rs index 25d29c68c..834960ec4 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations.rs +++ b/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( // 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( Box::new(sapling_memo_consistency::Migration { params: params.clone(), }), + Box::new(v_sapling_shard_unscanned_ranges::Migration { + params: params.clone(), + }), ] } 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 new file mode 100644 index 000000000..06dab783e --- /dev/null +++ b/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

{ + pub(super) params: P, +} + +impl

schemer::Migration for Migration

{ + fn id(&self) -> Uuid { + MIGRATION_ID + } + + fn dependencies(&self) -> HashSet { + [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 RusqliteMigration for Migration

{ + 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(()) + } +} diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index 2d0499b3e..b8891efe4 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/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 = exclude.iter().map(|n| Value::from(n.0)).collect(); @@ -165,9 +173,7 @@ pub(crate) fn select_spendable_sapling_notes( exclude: &[ReceivedNoteId], ) -> Result>, 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 - // selected notes. This is achieved in several steps: - // + // value has been reached. // 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::{ - memo::MemoBytes, - transaction::components::{amount::NonNegativeAmount, OutPoint, TxOut}, - }, + zcash_primitives::transaction::components::{amount::NonNegativeAmount, OutPoint, TxOut}, }; pub(crate) fn test_prover() -> impl TxProver { @@ -555,8 +567,8 @@ pub(crate) mod tests { let mut stmt_sent_notes = db_data .conn .prepare( - "SELECT output_index - FROM sent_notes + "SELECT output_index + FROM sent_notes JOIN transactions ON transactions.id_tx = sent_notes.tx WHERE transactions.txid = ?", )