From f53ea2d7787c59477596b52eac1020d612033c34 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 15 Aug 2023 08:35:07 -0600 Subject: [PATCH 1/4] Add `get_wallet_summary` to `WalletRead` The intent of this API is to provide a single API which returns in a single call: * per-account balances, including pending values * wallet sync progress Fixes #865 Fixes #900 --- zcash_client_backend/CHANGELOG.md | 8 +- zcash_client_backend/src/data_api.rs | 188 ++++++++- zcash_client_sqlite/src/lib.rs | 17 +- zcash_client_sqlite/src/testing.rs | 76 +++- zcash_client_sqlite/src/wallet.rs | 373 ++++++++++++++---- zcash_client_sqlite/src/wallet/init.rs | 54 ++- .../src/wallet/init/migrations.rs | 4 + .../v_sapling_shard_unscanned_ranges.rs | 32 +- .../init/migrations/wallet_summaries.rs | 89 +++++ zcash_client_sqlite/src/wallet/sapling.rs | 140 +++---- zcash_client_sqlite/src/wallet/scanning.rs | 23 +- zcash_primitives/CHANGELOG.md | 5 + .../src/transaction/components/amount.rs | 31 ++ 13 files changed, 839 insertions(+), 201 deletions(-) create mode 100644 zcash_client_sqlite/src/wallet/init/migrations/wallet_summaries.rs diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 80042ceade..5740dbaea5 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -22,15 +22,19 @@ and this library adheres to Rust's notion of - `impl Eq for zcash_client_backend::zip321::{Payment, TransactionRequest}` - `impl Debug` for `zcash_client_backend::{data_api::wallet::input_selection::Proposal, wallet::ReceivedSaplingNote}` - `zcash_client_backend::data_api`: + - `AccountBalance` - `AccountBirthday` + - `Balance` - `BlockMetadata` - `NoteId` - `NullifierQuery` for use with `WalletRead::get_sapling_nullifiers` + - `Ratio` - `ScannedBlock` - `ShieldedProtocol` - `WalletCommitmentTrees` + - `WalletSummary` - `WalletRead::{chain_height, block_metadata, block_fully_scanned, suggest_scan_ranges, - get_wallet_birthday, get_account_birthday}` + get_wallet_birthday, get_account_birthday, get_wallet_summary}` - `WalletWrite::{put_blocks, update_chain_tip}` - `chain::CommitmentTreeRoot` - `scanning` A new module containing types required for `suggest_scan_ranges` @@ -113,6 +117,8 @@ and this library adheres to Rust's notion of instead to obtain the wallet's view of the chain tip instead, or `suggest_scan_ranges` to obtain information about blocks that need to be scanned. + - `WalletRead::get_balance_at` has been removed. Use `WalletRead::get_wallet_summary` + instead. - `WalletRead::{get_all_nullifiers, get_commitment_tree, get_witnesses}` have been removed without replacement. The utility of these methods is now subsumed by those available from the `WalletCommitmentTrees` trait. diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 2211318e47..df923d756b 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -1,9 +1,11 @@ //! Interfaces for wallet data persistence & low-level wallet utilities. -use std::fmt::Debug; -use std::io; -use std::num::NonZeroU32; -use std::{collections::HashMap, num::TryFromIntError}; +use std::{ + collections::{BTreeMap, HashMap}, + fmt::Debug, + io, + num::{NonZeroU32, TryFromIntError}, +}; use incrementalmerkletree::{frontier::Frontier, Retention}; use secrecy::SecretVec; @@ -15,7 +17,10 @@ use zcash_primitives::{ memo::{Memo, MemoBytes}, sapling::{self, Node, NOTE_COMMITMENT_TREE_DEPTH}, transaction::{ - components::{amount::Amount, OutPoint}, + components::{ + amount::{Amount, NonNegativeAmount}, + OutPoint, + }, Transaction, TxId, }, zip32::{AccountId, ExtendedFullViewingKey}, @@ -46,6 +51,155 @@ pub enum NullifierQuery { All, } +/// Balance information for a value within a single shielded pool in an account. +#[derive(Debug, Clone, Copy)] +pub struct Balance { + /// The value in the account that may currently be spent; it is possible to compute witnesses + /// for all the notes that comprise this value, and all of this value is confirmed to the + /// required confirmation depth. + pub spendable_value: NonNegativeAmount, + + /// The value in the account of shielded change notes that do not yet have sufficient + /// confirmations to be spendable. + pub change_pending_confirmation: NonNegativeAmount, + + /// The value in the account of all remaining received notes that either do not have sufficient + /// confirmations to be spendable, or for which witnesses cannot yet be constructed without + /// additional scanning. + pub value_pending_spendability: NonNegativeAmount, +} + +impl Balance { + /// The [`Balance`] value having zero values for all its fields. + pub const ZERO: Self = Self { + spendable_value: NonNegativeAmount::ZERO, + change_pending_confirmation: NonNegativeAmount::ZERO, + value_pending_spendability: NonNegativeAmount::ZERO, + }; + + /// Returns the total value of funds represented by this [`Balance`]. + pub fn total(&self) -> NonNegativeAmount { + (self.spendable_value + self.change_pending_confirmation + self.value_pending_spendability) + .expect("Balance cannot overflow MAX_MONEY") + } +} + +/// Balance information for a single account. The sum of this struct's fields is the total balance +/// of the wallet. +#[derive(Debug, Clone, Copy)] +pub struct AccountBalance { + /// The value of unspent Sapling outputs belonging to the account. + pub sapling_balance: Balance, + + /// The value of all unspent transparent outputs belonging to the account, irrespective of + /// confirmation depth. + /// + /// Unshielded balances are not subject to confirmation-depth constraints, because the only + /// possible operation on a transparent balance is to shield it, it is possible to create a + /// zero-conf transaction to perform that shielding, and the resulting shielded notes will be + /// subject to normal confirmation rules. + pub unshielded: NonNegativeAmount, +} + +impl AccountBalance { + /// Returns the total value of funds belonging to the account. + pub fn total(&self) -> NonNegativeAmount { + (self.sapling_balance.total() + self.unshielded) + .expect("Account balance cannot overflow MAX_MONEY") + } +} + +/// A polymorphic ratio type, usually used for rational numbers. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct Ratio { + numerator: T, + denominator: T, +} + +impl Ratio { + /// Constructs a new Ratio from a numerator and a denominator. + pub fn new(numerator: T, denominator: T) -> Self { + Self { + numerator, + denominator, + } + } + + /// Returns the numerator of the ratio. + pub fn numerator(&self) -> &T { + &self.numerator + } + + /// Returns the denominator of the ratio. + pub fn denominator(&self) -> &T { + &self.denominator + } +} + +/// A type representing the potentially-spendable value of unspent outputs in the wallet. +/// +/// The balances reported using this data structure may overestimate the total spendable value of +/// the wallet, in the case that the spend of a previously received shielded note has not yet been +/// detected by the process of scanning the chain. The balances reported using this data structure +/// can only be certain to be unspent in the case that [`Self::is_synced`] is true, and even in +/// this circumstance it is possible that a newly created transaction could conflict with a +/// not-yet-mined transaction in the mempool. +#[derive(Debug, Clone)] +pub struct WalletSummary { + account_balances: BTreeMap, + chain_tip_height: BlockHeight, + fully_scanned_height: BlockHeight, + sapling_scan_progress: Option>, +} + +impl WalletSummary { + /// Constructs a new [`WalletSummary`] from its constituent parts. + pub fn new( + account_balances: BTreeMap, + chain_tip_height: BlockHeight, + fully_scanned_height: BlockHeight, + sapling_scan_progress: Option>, + ) -> Self { + Self { + account_balances, + chain_tip_height, + fully_scanned_height, + sapling_scan_progress, + } + } + + /// Returns the balances of accounts in the wallet, keyed by account ID. + pub fn account_balances(&self) -> &BTreeMap { + &self.account_balances + } + + /// Returns the height of the current chain tip. + pub fn chain_tip_height(&self) -> BlockHeight { + self.chain_tip_height + } + + /// Returns the height below which all blocks wallet have been scanned, ignoring blocks below + /// the wallet birthday. + pub fn fully_scanned_height(&self) -> BlockHeight { + self.fully_scanned_height + } + + /// Returns the progress of scanning Sapling outputs, in terms of the ratio between notes + /// scanned and the total number of notes added to the chain since the wallet birthday. + /// + /// This ratio should only be used to compute progress percentages, and the numerator and + /// denominator should not be treated as authoritative note counts. Returns `None` if the + /// wallet is unable to determine the size of the note commitment tree. + pub fn sapling_scan_progress(&self) -> Option> { + self.sapling_scan_progress + } + + /// Returns whether or not wallet scanning is complete. + pub fn is_synced(&self) -> bool { + self.chain_tip_height == self.fully_scanned_height + } +} + /// Read-only operations required for light wallet functions. /// /// This trait defines the read-only portion of the storage interface atop which @@ -157,15 +311,12 @@ pub trait WalletRead { extfvk: &ExtendedFullViewingKey, ) -> Result; - /// Returns the wallet balance for an account as of the specified block height. - /// - /// This may be used to obtain a balance that ignores notes that have been received so recently - /// that they are not yet deemed spendable. - fn get_balance_at( + /// Returns the wallet balances and sync status for an account given the specified minimum + /// number of confirmations, or `Ok(None)` if the wallet has no balance data available. + fn get_wallet_summary( &self, - account: AccountId, - anchor_height: BlockHeight, - ) -> Result; + min_confirmations: u32, + ) -> Result, Self::Error>; /// Returns the memo for a note. /// @@ -747,7 +898,7 @@ pub mod testing { use super::{ chain::CommitmentTreeRoot, scanning::ScanRange, AccountBirthday, BlockMetadata, DecryptedTransaction, NoteId, NullifierQuery, ScannedBlock, SentTransaction, - WalletCommitmentTrees, WalletRead, WalletWrite, SAPLING_SHARD_HEIGHT, + WalletCommitmentTrees, WalletRead, WalletSummary, WalletWrite, SAPLING_SHARD_HEIGHT, }; pub struct MockWalletDb { @@ -853,12 +1004,11 @@ pub mod testing { Ok(false) } - fn get_balance_at( + fn get_wallet_summary( &self, - _account: AccountId, - _anchor_height: BlockHeight, - ) -> Result { - Ok(Amount::zero()) + _min_confirmations: u32, + ) -> Result, Self::Error> { + Ok(None) } fn get_memo(&self, _id_note: NoteId) -> Result, Self::Error> { diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 318fc16abc..670f5f97bd 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -66,7 +66,7 @@ use zcash_client_backend::{ scanning::{ScanPriority, ScanRange}, AccountBirthday, BlockMetadata, DecryptedTransaction, NoteId, NullifierQuery, PoolType, Recipient, ScannedBlock, SentTransaction, ShieldedProtocol, WalletCommitmentTrees, - WalletRead, WalletWrite, SAPLING_SHARD_HEIGHT, + WalletRead, WalletSummary, WalletWrite, SAPLING_SHARD_HEIGHT, }, keys::{UnifiedFullViewingKey, UnifiedSpendingKey}, proto::compact_formats::CompactBlock, @@ -88,7 +88,10 @@ pub mod error; pub mod serialization; pub mod wallet; -use wallet::commitment_tree::{self, put_shard_roots}; +use wallet::{ + commitment_tree::{self, put_shard_roots}, + SubtreeScanProgress, +}; #[cfg(test)] mod testing; @@ -243,12 +246,11 @@ impl, P: consensus::Parameters> WalletRead for W wallet::is_valid_account_extfvk(self.conn.borrow(), &self.params, account, extfvk) } - fn get_balance_at( + fn get_wallet_summary( &self, - account: AccountId, - anchor_height: BlockHeight, - ) -> Result { - wallet::get_balance_at(self.conn.borrow(), account, anchor_height) + min_confirmations: u32, + ) -> Result, Self::Error> { + wallet::get_wallet_summary(self.conn.borrow(), min_confirmations, &SubtreeScanProgress) } fn get_memo(&self, note_id: NoteId) -> Result, Self::Error> { @@ -456,6 +458,7 @@ impl WalletWrite for WalletDb block.block_hash(), block.block_time(), block.metadata().sapling_tree_size(), + block.sapling_commitments().len().try_into().unwrap(), )?; for tx in block.transactions() { diff --git a/zcash_client_sqlite/src/testing.rs b/zcash_client_sqlite/src/testing.rs index d16af5a39a..7fbb46e192 100644 --- a/zcash_client_sqlite/src/testing.rs +++ b/zcash_client_sqlite/src/testing.rs @@ -25,7 +25,7 @@ use zcash_client_backend::{ input_selection::{GreedyInputSelectorError, InputSelector, Proposal}, propose_transfer, spend, }, - AccountBirthday, WalletWrite, + AccountBirthday, WalletSummary, WalletWrite, }, keys::UnifiedSpendingKey, proto::compact_formats::{ @@ -46,7 +46,10 @@ use zcash_primitives::{ Note, Nullifier, PaymentAddress, }, transaction::{ - components::{amount::BalanceError, Amount}, + components::{ + amount::{BalanceError, NonNegativeAmount}, + Amount, + }, fees::FeeRule, TxId, }, @@ -56,7 +59,10 @@ use zcash_primitives::{ use crate::{ chain::init::init_cache_database, error::SqliteClientError, - wallet::{commitment_tree, init::init_wallet_db, sapling::tests::test_prover}, + wallet::{ + commitment_tree, get_wallet_summary, init::init_wallet_db, sapling::tests::test_prover, + SubtreeScanProgress, + }, AccountId, ReceivedNoteId, WalletDb, }; @@ -65,9 +71,7 @@ use super::BlockDb; #[cfg(feature = "transparent-inputs")] use { zcash_client_backend::data_api::wallet::{propose_shielding, shield_transparent_funds}, - zcash_primitives::{ - legacy::TransparentAddress, transaction::components::amount::NonNegativeAmount, - }, + zcash_primitives::legacy::TransparentAddress, }; #[cfg(feature = "unstable")] @@ -286,6 +290,66 @@ where limit, ) } + + pub(crate) fn get_total_balance(&self, account: AccountId) -> NonNegativeAmount { + get_wallet_summary(&self.wallet().conn, 0, &SubtreeScanProgress) + .unwrap() + .unwrap() + .account_balances() + .get(&account) + .unwrap() + .total() + } + + pub(crate) fn get_spendable_balance( + &self, + account: AccountId, + min_confirmations: u32, + ) -> NonNegativeAmount { + let binding = + get_wallet_summary(&self.wallet().conn, min_confirmations, &SubtreeScanProgress) + .unwrap() + .unwrap(); + let balance = binding.account_balances().get(&account).unwrap(); + + balance.sapling_balance.spendable_value + } + + pub(crate) fn get_pending_shielded_balance( + &self, + account: AccountId, + min_confirmations: u32, + ) -> NonNegativeAmount { + let binding = + get_wallet_summary(&self.wallet().conn, min_confirmations, &SubtreeScanProgress) + .unwrap() + .unwrap(); + let balance = binding.account_balances().get(&account).unwrap(); + + (balance.sapling_balance.value_pending_spendability + + balance.sapling_balance.change_pending_confirmation) + .unwrap() + } + + pub(crate) fn get_pending_change( + &self, + account: AccountId, + min_confirmations: u32, + ) -> NonNegativeAmount { + let binding = + get_wallet_summary(&self.wallet().conn, min_confirmations, &SubtreeScanProgress) + .unwrap() + .unwrap(); + let balance = binding.account_balances().get(&account).unwrap(); + + balance.sapling_balance.change_pending_confirmation + } + + pub(crate) fn get_wallet_summary(&self, min_confirmations: u32) -> WalletSummary { + get_wallet_summary(&self.wallet().conn, min_confirmations, &SubtreeScanProgress) + .unwrap() + .unwrap() + } } impl TestState { diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index e8b1c0e293..41b775102c 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -68,11 +68,13 @@ use incrementalmerkletree::Retention; use rusqlite::{self, named_params, OptionalExtension, ToSql}; use shardtree::ShardTree; use std::cmp; -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; use std::convert::TryFrom; use std::io::{self, Cursor}; use std::num::NonZeroU32; use tracing::debug; +use zcash_client_backend::data_api::{AccountBalance, Balance, Ratio, WalletSummary}; +use zcash_primitives::transaction::components::amount::NonNegativeAmount; use zcash_client_backend::data_api::{ scanning::{ScanPriority, ScanRange}, @@ -106,7 +108,7 @@ use crate::{ }; use crate::{SAPLING_TABLES_PREFIX, VERIFY_LOOKAHEAD}; -use self::scanning::replace_queue_entries; +use self::scanning::{parse_priority_code, replace_queue_entries}; #[cfg(feature = "transparent-inputs")] use { @@ -519,28 +521,264 @@ pub(crate) fn get_balance( } } -/// Returns the verified balance for the account at the specified height, -/// This may be used to obtain a balance that ignores notes that have been -/// received so recently that they are not yet deemed spendable. -pub(crate) fn get_balance_at( +pub(crate) trait ScanProgress { + fn sapling_scan_progress( + &self, + conn: &rusqlite::Connection, + birthday_height: BlockHeight, + fully_scanned_height: BlockHeight, + chain_tip_height: BlockHeight, + ) -> Result>, SqliteClientError>; +} + +pub(crate) struct SubtreeScanProgress; + +impl ScanProgress for SubtreeScanProgress { + fn sapling_scan_progress( + &self, + conn: &rusqlite::Connection, + birthday_height: BlockHeight, + fully_scanned_height: BlockHeight, + chain_tip_height: BlockHeight, + ) -> Result>, SqliteClientError> { + if fully_scanned_height == chain_tip_height { + // Compute the total blocks scanned since the wallet birthday + conn.query_row( + "SELECT SUM(sapling_output_count) + FROM blocks + WHERE height >= :birthday_height", + named_params![":birthday_height": u32::from(birthday_height)], + |row| { + let scanned = row.get::<_, Option>(0)?; + Ok(scanned.map(|n| Ratio::new(n, n))) + }, + ) + .map_err(SqliteClientError::from) + } else { + // Compute the number of fully scanned notes directly from the blocks table + let fully_scanned_size = conn.query_row( + "SELECT MAX(sapling_commitment_tree_size) + FROM blocks + WHERE height <= :fully_scanned_height", + named_params![":fully_scanned_height": u32::from(fully_scanned_height)], + |row| row.get::<_, Option>(0), + )?; + + // Compute the total blocks scanned so far above the fully scanned height + let scanned_count = conn.query_row( + "SELECT SUM(sapling_output_count) + FROM blocks + WHERE height > :fully_scanned_height", + named_params![":fully_scanned_height": u32::from(fully_scanned_height)], + |row| row.get::<_, Option>(0), + )?; + + // We don't have complete information on how many outputs will exist in the shard at + // the chain tip without having scanned the chain tip block, so we overestimate by + // computing the maximum possible number of notes directly from the shard indices. + // + // TODO: it would be nice to be able to reliably have the size of the commitment tree + // at the chain tip without having to have scanned that block. + Ok(conn + .query_row( + "SELECT MIN(shard_index), MAX(shard_index) + FROM sapling_tree_shards + WHERE subtree_end_height > :fully_scanned_height + OR subtree_end_height IS NULL", + named_params![":fully_scanned_height": u32::from(fully_scanned_height)], + |row| { + let min_tree_size = row + .get::<_, Option>(0)? + .map(|min| min << SAPLING_SHARD_HEIGHT); + let max_idx = row.get::<_, Option>(1)?; + Ok(fully_scanned_size.or(min_tree_size).zip(max_idx).map( + |(min_tree_size, max)| { + let max_tree_size = (max + 1) << SAPLING_SHARD_HEIGHT; + Ratio::new( + scanned_count.unwrap_or(0), + max_tree_size - min_tree_size, + ) + }, + )) + }, + ) + .optional()? + .flatten()) + } + } +} + +/// Returns the spendable balance for the account at the specified height. +/// +/// This may be used to obtain a balance that ignores notes that have been detected so recently +/// that they are not yet spendable, or for which it is not yet possible to construct witnesses. +pub(crate) fn get_wallet_summary( conn: &rusqlite::Connection, - account: AccountId, - anchor_height: BlockHeight, -) -> Result { - let balance = conn.query_row( - "SELECT SUM(value) FROM sapling_received_notes - INNER JOIN transactions ON transactions.id_tx = sapling_received_notes.tx - WHERE account = ? AND spent IS NULL AND transactions.block <= ?", - [u32::from(account), u32::from(anchor_height)], - |row| row.get(0).or(Ok(0)), + min_confirmations: u32, + progress: &impl ScanProgress, +) -> Result, SqliteClientError> { + let chain_tip_height = match scan_queue_extrema(conn)? { + Some((_, max)) => max, + None => { + return Ok(None); + } + }; + + let birthday_height = + wallet_birthday(conn)?.expect("If a scan range exists, we know the wallet birthday."); + + let fully_scanned_height = + block_fully_scanned(conn)?.map_or(birthday_height - 1, |m| m.block_height()); + let summary_height = chain_tip_height + 1 - min_confirmations; + + let sapling_scan_progress = progress.sapling_scan_progress( + conn, + birthday_height, + fully_scanned_height, + chain_tip_height, )?; - match Amount::from_i64(balance) { - Ok(amount) if !amount.is_negative() => Ok(amount), - _ => Err(SqliteClientError::CorruptedData( - "Sum of values in sapling_received_notes is out of range".to_string(), - )), + // If the shard containing the anchor is contains any unscanned ranges below the summary + // height, none of our balance is currently spendable. + let any_spendable = conn.query_row( + "SELECT EXISTS( + SELECT 1 FROM v_sapling_shard_unscanned_ranges + WHERE :summary_height + BETWEEN subtree_start_height + AND IFNULL(subtree_end_height, :summary_height) + AND block_range_start <= :summary_height + )", + named_params![":summary_height": u32::from(summary_height)], + |row| row.get::<_, bool>(0).map(|b| !b), + )?; + + let mut stmt_select_notes = conn.prepare_cached( + "SELECT n.account, n.value, n.is_change, scan_state.max_priority, t.block, t.expiry_height + FROM sapling_received_notes n + JOIN transactions t ON t.id_tx = n.tx + LEFT OUTER JOIN v_sapling_shards_scan_state scan_state + ON n.commitment_tree_position >= scan_state.start_position + AND n.commitment_tree_position < scan_state.end_position_exclusive + WHERE n.spent IS NULL + AND ( + t.expiry_height IS NULL + OR t.block IS NOT NULL + OR t.expiry_height >= :summary_height + )", + )?; + + let mut account_balances: BTreeMap = BTreeMap::new(); + let mut rows = + stmt_select_notes.query(named_params![":summary_height": u32::from(summary_height)])?; + while let Some(row) = rows.next()? { + let account = row.get::<_, u32>(0).map(AccountId::from)?; + + let value_raw = row.get::<_, i64>(1)?; + let value = NonNegativeAmount::from_nonnegative_i64(value_raw).map_err(|_| { + SqliteClientError::CorruptedData(format!("Negative received note value: {}", value_raw)) + })?; + + let is_change = row.get::<_, bool>(2)?; + + // If `max_priority` is null, this means that the note is not positioned; the note + // will not be spendable, so we assign the scan priority to `ChainTip` as a priority + // that is greater than `Scanned` + let max_priority_raw = row.get::<_, Option>(3)?; + let max_priority = max_priority_raw.map_or_else( + || Ok(ScanPriority::ChainTip), + |raw| { + parse_priority_code(raw).ok_or_else(|| { + SqliteClientError::CorruptedData(format!( + "Priority code {} not recognized.", + raw + )) + }) + }, + )?; + + let received_height = row + .get::<_, Option>(4) + .map(|opt| opt.map(BlockHeight::from))?; + + let is_spendable = any_spendable + && received_height.iter().any(|h| h <= &summary_height) + && max_priority <= ScanPriority::Scanned; + + let is_pending_change = is_change && received_height.iter().all(|h| h > &summary_height); + + let (spendable_value, change_pending_confirmation, value_pending_spendability) = { + let zero = NonNegativeAmount::ZERO; + if is_spendable { + (value, zero, zero) + } else if is_pending_change { + (zero, value, zero) + } else { + (zero, zero, value) + } + }; + + account_balances + .entry(account) + .and_modify(|bal| { + bal.sapling_balance.spendable_value = (bal.sapling_balance.spendable_value + + spendable_value) + .expect("Spendable value cannot overflow"); + bal.sapling_balance.change_pending_confirmation = + (bal.sapling_balance.change_pending_confirmation + change_pending_confirmation) + .expect("Pending change value cannot overflow"); + bal.sapling_balance.value_pending_spendability = + (bal.sapling_balance.value_pending_spendability + value_pending_spendability) + .expect("Value pending spendability cannot overflow"); + }) + .or_insert(AccountBalance { + sapling_balance: Balance { + spendable_value, + change_pending_confirmation, + value_pending_spendability, + }, + unshielded: NonNegativeAmount::ZERO, + }); + } + + #[cfg(feature = "transparent-inputs")] + { + let mut stmt_transparent_balances = conn.prepare( + "SELECT u.received_by_account, SUM(u.value_zat) + FROM utxos u + LEFT OUTER JOIN transactions tx + ON tx.id_tx = u.spent_in_tx + WHERE u.height <= :max_height + AND tx.block IS NULL + GROUP BY u.received_by_account", + )?; + let mut rows = stmt_transparent_balances + .query(named_params![":max_height": u32::from(summary_height)])?; + + while let Some(row) = rows.next()? { + let account = AccountId::from(row.get::<_, u32>(0)?); + let raw_value = row.get(1)?; + let value = NonNegativeAmount::from_nonnegative_i64(raw_value).map_err(|_| { + SqliteClientError::CorruptedData(format!("Negative UTXO value {:?}", raw_value)) + })?; + + account_balances + .entry(account) + .and_modify(|bal| bal.unshielded = value) + .or_insert(AccountBalance { + sapling_balance: Balance::ZERO, + unshielded: value, + }); + } } + + let summary = WalletSummary::new( + account_balances, + chain_tip_height, + fully_scanned_height, + sapling_scan_progress, + ); + + Ok(Some(summary)) } /// Returns the memo for a received note, if the note is known to the wallet. @@ -828,52 +1066,50 @@ pub(crate) fn block_metadata( pub(crate) fn block_fully_scanned( conn: &rusqlite::Connection, ) -> Result, SqliteClientError> { - // We assume here that the wallet was either initialized via `init_blocks_table`, or - // its birthday is Sapling activation, so the earliest block in the `blocks` table is - // the first fully-scanned block (because it occurs before any wallet activity). - // - // We further assume that the only way we get a contiguous range of block heights in - // the `blocks` table starting with this earliest block, is if all scanning operations - // have been performed on those blocks. This holds because the `blocks` table is only - // altered by `WalletDb::put_blocks` via `put_block`, and the effective combination of - // intra-range linear scanning and the nullifier map ensures that we discover all - // wallet-related information within the contiguous range. - // - // The fully-scanned height is therefore the greatest height in the first contiguous - // range of block rows, which is a combined case of the "gaps and islands" and - // "greatest N per group" SQL query problems. - conn.query_row( - "SELECT height, hash, sapling_commitment_tree_size, sapling_tree - FROM blocks - INNER JOIN ( - WITH contiguous AS ( - SELECT height, ROW_NUMBER() OVER (ORDER BY height) - height AS grp - FROM blocks + if let Some(birthday_height) = wallet_birthday(conn)? { + // We assume that the only way we get a contiguous range of block heights in the `blocks` table + // starting with the birthday block, is if all scanning operations have been performed on those + // blocks. This holds because the `blocks` table is only altered by `WalletDb::put_blocks` via + // `put_block`, and the effective combination of intra-range linear scanning and the nullifier + // map ensures that we discover all wallet-related information within the contiguous range. + // + // The fully-scanned height is therefore the greatest height in the first contiguous range of + // block rows, which is a combined case of the "gaps and islands" and "greatest N per group" + // SQL query problems. + conn.query_row( + "SELECT height, hash, sapling_commitment_tree_size, sapling_tree + FROM blocks + INNER JOIN ( + WITH contiguous AS ( + SELECT height, ROW_NUMBER() OVER (ORDER BY height) - height AS grp + FROM blocks + ) + SELECT MIN(height) AS group_min_height, MAX(height) AS group_max_height + FROM contiguous + GROUP BY grp + HAVING :birthday_height BETWEEN group_min_height AND group_max_height ) - SELECT MAX(height) AS [fully_scanned_height] - FROM contiguous - GROUP BY grp - ORDER BY height - LIMIT 1 + ON height = group_max_height", + named_params![":birthday_height": u32::from(birthday_height)], + |row| { + let height: u32 = row.get(0)?; + let block_hash: Vec = row.get(1)?; + let sapling_tree_size: Option = row.get(2)?; + let sapling_tree: Vec = row.get(3)?; + Ok(( + BlockHeight::from(height), + block_hash, + sapling_tree_size, + sapling_tree, + )) + }, ) - ON height = fully_scanned_height", - [], - |row| { - let height: u32 = row.get(0)?; - let block_hash: Vec = row.get(1)?; - let sapling_tree_size: Option = row.get(2)?; - let sapling_tree: Vec = row.get(3)?; - Ok(( - BlockHeight::from(height), - block_hash, - sapling_tree_size, - sapling_tree, - )) - }, - ) - .optional() - .map_err(SqliteClientError::from) - .and_then(|meta_row| meta_row.map(parse_block_metadata).transpose()) + .optional() + .map_err(SqliteClientError::from) + .and_then(|meta_row| meta_row.map(parse_block_metadata).transpose()) + } else { + Ok(None) + } } /// Returns the block height at which the specified transaction was mined, @@ -1171,6 +1407,7 @@ pub(crate) fn put_block( block_hash: BlockHash, block_time: u32, sapling_commitment_tree_size: u32, + sapling_output_count: u32, ) -> Result<(), SqliteClientError> { let block_hash_data = conn .query_row( @@ -1200,6 +1437,7 @@ pub(crate) fn put_block( hash, time, sapling_commitment_tree_size, + sapling_output_count, sapling_tree ) VALUES ( @@ -1207,19 +1445,22 @@ pub(crate) fn put_block( :hash, :block_time, :sapling_commitment_tree_size, + :sapling_output_count, x'00' ) ON CONFLICT (height) DO UPDATE SET hash = :hash, time = :block_time, - sapling_commitment_tree_size = :sapling_commitment_tree_size", + sapling_commitment_tree_size = :sapling_commitment_tree_size, + sapling_output_count = :sapling_output_count", )?; stmt_upsert_block.execute(named_params![ ":height": u32::from(block_height), ":hash": &block_hash.0[..], ":block_time": block_time, - ":sapling_commitment_tree_size": sapling_commitment_tree_size + ":sapling_commitment_tree_size": sapling_commitment_tree_size, + ":sapling_output_count": sapling_output_count, ])?; Ok(()) diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 98162eebd6..a8ba6a80d2 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -219,7 +219,9 @@ mod tests { time INTEGER NOT NULL, sapling_tree BLOB NOT NULL , sapling_commitment_tree_size INTEGER, - orchard_commitment_tree_size INTEGER)", + orchard_commitment_tree_size INTEGER, + sapling_output_count INTEGER, + orchard_action_count INTEGER)", "CREATE TABLE nullifier_map ( spend_pool INTEGER NOT NULL, nf BLOB NOT NULL, @@ -365,16 +367,15 @@ mod tests { } let expected_views = vec![ - // v_sapling_shard_unscanned_ranges + // v_sapling_shard_scan_ranges format!( - "CREATE VIEW v_sapling_shard_unscanned_ranges AS - WITH wallet_birthday AS (SELECT MIN(birthday_height) AS height FROM accounts) + "CREATE VIEW v_sapling_shard_scan_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.subtree_end_height, shard.contains_marked, scan_queue.block_range_start, scan_queue.block_range_end, @@ -383,18 +384,53 @@ mod tests { 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 ( scan_queue.block_range_start <= prev_shard.subtree_end_height AND (scan_queue.block_range_end - 1) >= shard.subtree_end_height - ) - INNER JOIN wallet_birthday - WHERE scan_queue.priority > {} - AND scan_queue.block_range_end > wallet_birthday.height", + )", u32::from(st.network().activation_height(NetworkUpgrade::Sapling).unwrap()), + ), + // v_sapling_shard_unscanned_ranges + format!( + "CREATE VIEW v_sapling_shard_unscanned_ranges AS + WITH wallet_birthday AS (SELECT MIN(birthday_height) AS height FROM accounts) + SELECT + shard_index, + start_position, + end_position_exclusive, + subtree_start_height, + subtree_end_height, + contains_marked, + block_range_start, + block_range_end, + priority + FROM v_sapling_shard_scan_ranges + INNER JOIN wallet_birthday + WHERE priority > {} + AND block_range_end > wallet_birthday.height", priority_code(&ScanPriority::Scanned) ), + // v_sapling_shards_scan_state + "CREATE VIEW v_sapling_shards_scan_state AS + SELECT + shard_index, + start_position, + end_position_exclusive, + subtree_start_height, + subtree_end_height, + contains_marked, + MAX(priority) AS max_priority + FROM v_sapling_shard_scan_ranges + GROUP BY + shard_index, + start_position, + end_position_exclusive, + subtree_start_height, + subtree_end_height, + contains_marked".to_owned(), // v_transactions "CREATE VIEW v_transactions AS WITH diff --git a/zcash_client_sqlite/src/wallet/init/migrations.rs b/zcash_client_sqlite/src/wallet/init/migrations.rs index f5cd2e2ac2..03381ba2cf 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations.rs @@ -12,6 +12,7 @@ mod ufvk_support; mod utxos_table; mod v_sapling_shard_unscanned_ranges; mod v_transactions_net; +mod wallet_summaries; use schemer_rusqlite::RusqliteMigration; use secrecy::SecretVec; @@ -42,6 +43,8 @@ pub(super) fn all_migrations( // add_account_birthdays // | // v_sapling_shard_unscanned_ranges + // | + // wallet_summaries vec![ Box::new(initial_setup::Migration {}), Box::new(utxos_table::Migration {}), @@ -72,5 +75,6 @@ pub(super) fn all_migrations( Box::new(v_sapling_shard_unscanned_ranges::Migration { params: params.clone(), }), + Box::new(wallet_summaries::Migration), ] } 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 ea1aa68ce2..7a72942c09 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 @@ -38,14 +38,13 @@ impl RusqliteMigration for Migration

{ fn up(&self, transaction: &rusqlite::Transaction) -> Result<(), Self::Error> { transaction.execute_batch( &format!( - "CREATE VIEW v_sapling_shard_unscanned_ranges AS - WITH wallet_birthday AS (SELECT MIN(birthday_height) AS height FROM accounts) + "CREATE VIEW v_sapling_shard_scan_ranges AS SELECT shard.shard_index, shard.shard_index << {} AS start_position, (shard.shard_index + 1) << {} AS end_position_exclusive, IFNULL(prev_shard.subtree_end_height, {}) AS subtree_start_height, - shard.subtree_end_height AS subtree_end_height, + shard.subtree_end_height, shard.contains_marked, scan_queue.block_range_start, scan_queue.block_range_end, @@ -54,22 +53,39 @@ impl RusqliteMigration for Migration

{ 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 ( scan_queue.block_range_start <= prev_shard.subtree_end_height AND (scan_queue.block_range_end - 1) >= shard.subtree_end_height - ) - INNER JOIN wallet_birthday - WHERE scan_queue.priority > {} - AND scan_queue.block_range_end > wallet_birthday.height;", + )", SAPLING_SHARD_HEIGHT, SAPLING_SHARD_HEIGHT, u32::from(self.params.activation_height(NetworkUpgrade::Sapling).unwrap()), - priority_code(&ScanPriority::Scanned), ) )?; + transaction.execute_batch(&format!( + "CREATE VIEW v_sapling_shard_unscanned_ranges AS + WITH wallet_birthday AS (SELECT MIN(birthday_height) AS height FROM accounts) + SELECT + shard_index, + start_position, + end_position_exclusive, + subtree_start_height, + subtree_end_height, + contains_marked, + block_range_start, + block_range_end, + priority + FROM v_sapling_shard_scan_ranges + INNER JOIN wallet_birthday + WHERE priority > {} + AND block_range_end > wallet_birthday.height;", + priority_code(&ScanPriority::Scanned), + ))?; + Ok(()) } diff --git a/zcash_client_sqlite/src/wallet/init/migrations/wallet_summaries.rs b/zcash_client_sqlite/src/wallet/init/migrations/wallet_summaries.rs new file mode 100644 index 0000000000..5807bae65c --- /dev/null +++ b/zcash_client_sqlite/src/wallet/init/migrations/wallet_summaries.rs @@ -0,0 +1,89 @@ +//! This migration adds views and database changes required to provide accurate wallet summaries. + +use std::collections::HashSet; + +use schemer_rusqlite::RusqliteMigration; +use uuid::Uuid; + +use crate::wallet::init::WalletMigrationError; + +use super::v_sapling_shard_unscanned_ranges; + +pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0xc5bf7f71_2297_41ff_89e1_75e07c4e8838); + +pub(super) struct Migration; + +impl schemer::Migration for Migration { + fn id(&self) -> Uuid { + MIGRATION_ID + } + + fn dependencies(&self) -> HashSet { + [v_sapling_shard_unscanned_ranges::MIGRATION_ID] + .into_iter() + .collect() + } + + fn description(&self) -> &'static str { + "Adds views and data required to produce accurate wallet summaries." + } +} + +impl RusqliteMigration for Migration { + type Error = WalletMigrationError; + + fn up(&self, transaction: &rusqlite::Transaction) -> Result<(), Self::Error> { + // Add columns to the `blocks` table to track the number of scanned outputs in each block. + // We use the note commitment tree size information that we have in contiguous regions to + // populate this data, but we don't make any attempt to handle the boundary cases because + // we're just using this information for the progress metric, which can be a bit sloppy. + transaction.execute_batch( + "ALTER TABLE blocks ADD COLUMN sapling_output_count INTEGER; + ALTER TABLE blocks ADD COLUMN orchard_action_count INTEGER;", + )?; + + transaction.execute_batch( + // set the number of outputs everywhere that we have sequential Sapling blocks + "CREATE TEMPORARY TABLE block_deltas AS + SELECT + cur.height AS height, + (cur.sapling_commitment_tree_size - prev.sapling_commitment_tree_size) AS sapling_delta, + (cur.orchard_commitment_tree_size - prev.orchard_commitment_tree_size) AS orchard_delta + FROM blocks cur + INNER JOIN blocks prev + ON cur.height = prev.height + 1; + + UPDATE blocks + SET sapling_output_count = block_deltas.sapling_delta, + orchard_action_count = block_deltas.orchard_delta + FROM block_deltas + WHERE block_deltas.height = blocks.height;" + )?; + + transaction.execute_batch( + "CREATE VIEW v_sapling_shards_scan_state AS + SELECT + shard_index, + start_position, + end_position_exclusive, + subtree_start_height, + subtree_end_height, + contains_marked, + MAX(priority) AS max_priority + FROM v_sapling_shard_scan_ranges + GROUP BY + shard_index, + start_position, + end_position_exclusive, + subtree_start_height, + subtree_end_height, + contains_marked;", + )?; + + Ok(()) + } + + fn down(&self, _transaction: &rusqlite::Transaction) -> Result<(), Self::Error> { + panic!("This migration cannot be reverted."); + } +} diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index 099e6d6847..4e1f3702ab 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -437,7 +437,10 @@ pub(crate) mod tests { note_encryption::try_sapling_output_recovery, prover::TxProver, Note, PaymentAddress, }, transaction::{ - components::{amount::BalanceError, Amount}, + components::{ + amount::{BalanceError, NonNegativeAmount}, + Amount, + }, fees::{fixed::FeeRule as FixedFeeRule, zip317::FeeRule as Zip317FeeRule}, Transaction, }, @@ -450,7 +453,7 @@ pub(crate) mod tests { self, error::Error, wallet::input_selection::{GreedyInputSelector, GreedyInputSelectorError}, - AccountBirthday, ShieldedProtocol, WalletRead, + AccountBirthday, Ratio, ShieldedProtocol, WalletRead, }, decrypt_transaction, fees::{fixed, zip317, DustOutputPolicy}, @@ -462,14 +465,14 @@ pub(crate) mod tests { use crate::{ error::SqliteClientError, testing::{AddressType, BlockCache, TestBuilder, TestState}, - wallet::{commitment_tree, get_balance, get_balance_at}, + wallet::{commitment_tree, get_balance}, AccountId, NoteId, ReceivedNoteId, }; #[cfg(feature = "transparent-inputs")] use { zcash_client_backend::{data_api::WalletWrite, wallet::WalletTransparentOutput}, - zcash_primitives::transaction::components::{amount::NonNegativeAmount, OutPoint, TxOut}, + zcash_primitives::transaction::components::{OutPoint, TxOut}, }; pub(crate) fn test_prover() -> impl TxProver { @@ -497,18 +500,13 @@ pub(crate) mod tests { st.scan_cached_blocks(h, 1); // Verified balance matches total balance - let (_, anchor_height) = st - .wallet() - .get_target_and_anchor_heights(NonZeroU32::new(1).unwrap()) - .unwrap() - .unwrap(); assert_eq!( get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), value ); assert_eq!( - get_balance_at(&st.wallet().conn, AccountId::from(0), anchor_height).unwrap(), - value + st.get_total_balance(account), + NonNegativeAmount::try_from(value).unwrap() ); let to_extsk = ExtendedSpendingKey::master(&[]); @@ -693,47 +691,45 @@ pub(crate) mod tests { .with_test_account(AccountBirthday::from_sapling_activation) .build(); - let (_, usk, _) = st.test_account().unwrap(); + let (account, usk, _) = st.test_account().unwrap(); let dfvk = st.test_account_sapling().unwrap(); // Add funds to the wallet in a single note - let value = Amount::from_u64(50000).unwrap(); - let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + let value = NonNegativeAmount::from_u64(50000).unwrap(); + let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into()); st.scan_cached_blocks(h1, 1); // Verified balance matches total balance - let (_, anchor_height) = st - .wallet() - .get_target_and_anchor_heights(NonZeroU32::new(10).unwrap()) - .unwrap() - .unwrap(); - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - value - ); assert_eq!( - get_balance_at(&st.wallet().conn, AccountId::from(0), anchor_height).unwrap(), - value + get_balance(&st.wallet().conn, account).unwrap(), + value.into() ); + assert_eq!(st.get_total_balance(account), value); + + // Value is considered pending + assert_eq!(st.get_pending_shielded_balance(account, 10), value); + + // Wallet is fully scanned + let summary = st.get_wallet_summary(1); + assert_eq!(summary.sapling_scan_progress(), Some(Ratio::new(1, 1))); // Add more funds to the wallet in a second note - let (h2, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + let (h2, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into()); st.scan_cached_blocks(h2, 1); // Verified balance does not include the second note - let (_, anchor_height2) = st - .wallet() - .get_target_and_anchor_heights(NonZeroU32::new(10).unwrap()) - .unwrap() - .unwrap(); + let total = (value + value).unwrap(); assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - (value + value).unwrap() - ); - assert_eq!( - get_balance_at(&st.wallet().conn, AccountId::from(0), anchor_height2).unwrap(), - value + get_balance(&st.wallet().conn, account).unwrap(), + total.into() ); + assert_eq!(st.get_spendable_balance(account, 2), value); + assert_eq!(st.get_pending_shielded_balance(account, 2), value); + assert_eq!(st.get_total_balance(account), total); + + // Wallet is still fully scanned + let summary = st.get_wallet_summary(1); + assert_eq!(summary.sapling_scan_progress(), Some(Ratio::new(2, 2))); // Spend fails because there are insufficient verified notes let extsk2 = ExtendedSpendingKey::master(&[]); @@ -758,7 +754,7 @@ pub(crate) mod tests { // Mine blocks SAPLING_ACTIVATION_HEIGHT + 2 to 9 until just before the second // note is verified for _ in 2..10 { - st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into()); } st.scan_cached_blocks(h2 + 1, 8); @@ -781,7 +777,7 @@ pub(crate) mod tests { ); // Mine block 11 so that the second note becomes verified - let (h11, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + let (h11, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into()); st.scan_cached_blocks(h11, 1); // Second spend should now succeed @@ -805,17 +801,14 @@ pub(crate) mod tests { .with_test_account(AccountBirthday::from_sapling_activation) .build(); - let (_, usk, _) = st.test_account().unwrap(); + let (account, usk, _) = st.test_account().unwrap(); let dfvk = st.test_account_sapling().unwrap(); // Add funds to the wallet in a single note let value = Amount::from_u64(50000).unwrap(); let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); st.scan_cached_blocks(h1, 1); - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - value - ); + assert_eq!(get_balance(&st.wallet().conn, account).unwrap(), value); // Send some of the funds to another address let extsk2 = ExtendedSpendingKey::master(&[]); @@ -904,17 +897,14 @@ pub(crate) mod tests { .with_test_account(AccountBirthday::from_sapling_activation) .build(); - let (_, usk, _) = st.test_account().unwrap(); + let (account, usk, _) = st.test_account().unwrap(); let dfvk = st.test_account_sapling().unwrap(); // Add funds to the wallet in a single note let value = Amount::from_u64(50000).unwrap(); let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); st.scan_cached_blocks(h1, 1); - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - value - ); + assert_eq!(get_balance(&st.wallet().conn, account).unwrap(), value); let extsk2 = ExtendedSpendingKey::master(&[]); let addr2 = extsk2.default_address().1; @@ -1005,7 +995,7 @@ pub(crate) mod tests { .with_test_account(AccountBirthday::from_sapling_activation) .build(); - let (_, usk, _) = st.test_account().unwrap(); + let (account, usk, _) = st.test_account().unwrap(); let dfvk = st.test_account_sapling().unwrap(); // Add funds to the wallet in a single note @@ -1014,18 +1004,10 @@ pub(crate) mod tests { st.scan_cached_blocks(h, 1); // Verified balance matches total balance - let (_, anchor_height) = st - .wallet() - .get_target_and_anchor_heights(NonZeroU32::new(1).unwrap()) - .unwrap() - .unwrap(); - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - value - ); + assert_eq!(get_balance(&st.wallet().conn, account).unwrap(), value); assert_eq!( - get_balance_at(&st.wallet().conn, AccountId::from(0), anchor_height).unwrap(), - value + st.get_total_balance(account), + NonNegativeAmount::try_from(value).unwrap() ); let to = TransparentAddress::PublicKey([7; 20]).into(); @@ -1049,27 +1031,25 @@ pub(crate) mod tests { .with_test_account(AccountBirthday::from_sapling_activation) .build(); - let (_, usk, _) = st.test_account().unwrap(); + let (account, usk, _) = st.test_account().unwrap(); let dfvk = st.test_account_sapling().unwrap(); - // Add funds to the wallet in a single note + // Add funds to the wallet in a single note owned by the internal spending key let value = Amount::from_u64(60000).unwrap(); let (h, _, _) = st.generate_next_block(&dfvk, AddressType::Internal, value); st.scan_cached_blocks(h, 1); // Verified balance matches total balance - let (_, anchor_height) = st - .wallet() - .get_target_and_anchor_heights(NonZeroU32::new(10).unwrap()) - .unwrap() - .unwrap(); + assert_eq!(get_balance(&st.wallet().conn, account).unwrap(), value); assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - value + st.get_total_balance(account), + NonNegativeAmount::try_from(value).unwrap() ); + + // the balance is considered pending assert_eq!( - get_balance_at(&st.wallet().conn, AccountId::from(0), anchor_height).unwrap(), - value + st.get_pending_shielded_balance(account, 10), + NonNegativeAmount::try_from(value).unwrap() ); let to = TransparentAddress::PublicKey([7; 20]).into(); @@ -1093,7 +1073,7 @@ pub(crate) mod tests { .with_test_account(AccountBirthday::from_sapling_activation) .build(); - let (_, usk, _) = st.test_account().unwrap(); + let (account, usk, _) = st.test_account().unwrap(); let dfvk = st.test_account_sapling().unwrap(); // Add funds to the wallet @@ -1116,18 +1096,10 @@ pub(crate) mod tests { // Verified balance matches total balance let total = Amount::from_u64(60000).unwrap(); - let (_, anchor_height) = st - .wallet() - .get_target_and_anchor_heights(NonZeroU32::new(1).unwrap()) - .unwrap() - .unwrap(); - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - total - ); + assert_eq!(get_balance(&st.wallet().conn, account).unwrap(), total); assert_eq!( - get_balance_at(&st.wallet().conn, AccountId::from(0), anchor_height).unwrap(), - total + st.get_total_balance(account), + NonNegativeAmount::try_from(total).unwrap() ); let input_selector = GreedyInputSelector::new( diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index 640f6bf9b6..fba6cd4d9c 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -881,7 +881,8 @@ mod tests { use zcash_client_backend::data_api::{ chain::CommitmentTreeRoot, scanning::{ScanPriority, ScanRange}, - AccountBirthday, WalletCommitmentTrees, WalletRead, WalletWrite, + AccountBirthday, Ratio, WalletCommitmentTrees, WalletRead, WalletWrite, + SAPLING_SHARD_HEIGHT, }; use zcash_primitives::{ block::BlockHash, @@ -1689,6 +1690,10 @@ mod tests { ) .unwrap(); + // We have scan ranges and a subtree, but have scanned no blocks. + let summary = st.get_wallet_summary(1); + assert_eq!(summary.sapling_scan_progress(), None); + // 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(); @@ -1723,6 +1728,14 @@ mod tests { ); st.scan_cached_blocks(max_scanned, 1); + // We have scanned a block, so we now have a starting tree position, 500 blocks above the + // wallet birthday but before the end of the shard. + let summary = st.get_wallet_summary(1); + assert_eq!( + summary.sapling_scan_progress(), + Some(Ratio::new(1, 0x1 << SAPLING_SHARD_HEIGHT)) + ); + // Now simulate shutting down, and then restarting 70 blocks later, after a shard // has been completed. let last_shard_start = prior_tip + 50; @@ -1758,6 +1771,14 @@ mod tests { let actual = suggest_scan_ranges(&st.wallet().conn, Ignored).unwrap(); assert_eq!(actual, expected); + + // We've crossed a subtree boundary, and so still only have one scanned note but have two + // shards worth of notes to scan. + let summary = st.get_wallet_summary(1); + assert_eq!( + summary.sapling_scan_progress(), + Some(Ratio::new(1, 0x1 << (SAPLING_SHARD_HEIGHT + 1))) + ); } #[test] diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index d5855b66a9..ee6d3861bc 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -16,6 +16,11 @@ and this library adheres to Rust's notion of - `impl HashSer for String` is provided under the `test-dependencies` feature flag. This is a test-only impl; the identity leaf value is `_` and the combining operation is concatenation. +- `zcash_primitives::transaction::components::amount::NonNegativeAmount::ZERO` +- Additional trait implementations for `NonNegativeAmount`: + - `TryFrom for NonNegativeAmount` + - `Add for NonNegativeAmount` + - `Add for Option` ### Changed - `zcash_primitives::transaction`: diff --git a/zcash_primitives/src/transaction/components/amount.rs b/zcash_primitives/src/transaction/components/amount.rs index b4e8c4f1e5..fc7e53a6e4 100644 --- a/zcash_primitives/src/transaction/components/amount.rs +++ b/zcash_primitives/src/transaction/components/amount.rs @@ -238,6 +238,9 @@ impl TryFrom for Amount { pub struct NonNegativeAmount(Amount); impl NonNegativeAmount { + /// Returns the identity `NonNegativeAmount` + pub const ZERO: Self = NonNegativeAmount(Amount(0)); + /// Creates a NonNegativeAmount from a u64. /// /// Returns an error if the amount is outside the range `{0..MAX_MONEY}`. @@ -259,6 +262,34 @@ impl From for Amount { } } +impl TryFrom for NonNegativeAmount { + type Error = (); + + fn try_from(value: Amount) -> Result { + if value.is_negative() { + Err(()) + } else { + Ok(NonNegativeAmount(value)) + } + } +} + +impl Add for NonNegativeAmount { + type Output = Option; + + fn add(self, rhs: NonNegativeAmount) -> Option { + (self.0 + rhs.0).map(NonNegativeAmount) + } +} + +impl Add for Option { + type Output = Self; + + fn add(self, rhs: NonNegativeAmount) -> Option { + self.and_then(|lhs| lhs + rhs) + } +} + /// A type for balance violations in amount addition and subtraction /// (overflow and underflow of allowed ranges) #[derive(Copy, Clone, Debug, PartialEq, Eq)] From 7abd1324dedca54f69873d3d1886f768b776e55b Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 5 Sep 2023 11:58:24 -0600 Subject: [PATCH 2/4] Apply suggestions from code review Co-authored-by: Daira Emma Hopwood Co-authored-by: Jack Grigg --- zcash_client_backend/src/data_api.rs | 24 ++-- zcash_client_sqlite/src/chain.rs | 115 ++++++++---------- zcash_client_sqlite/src/testing.rs | 115 +++++++++--------- zcash_client_sqlite/src/wallet.rs | 70 +++-------- .../init/migrations/wallet_summaries.rs | 2 +- zcash_client_sqlite/src/wallet/sapling.rs | 61 ++++------ zcash_client_sqlite/src/wallet/scanning.rs | 6 +- .../src/transaction/components/amount.rs | 16 +++ 8 files changed, 178 insertions(+), 231 deletions(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index df923d756b..d42b0da95f 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -51,8 +51,8 @@ pub enum NullifierQuery { All, } -/// Balance information for a value within a single shielded pool in an account. -#[derive(Debug, Clone, Copy)] +/// Balance information for a value within a single pool in an account. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct Balance { /// The value in the account that may currently be spent; it is possible to compute witnesses /// for all the notes that comprise this value, and all of this value is confirmed to the @@ -86,7 +86,7 @@ impl Balance { /// Balance information for a single account. The sum of this struct's fields is the total balance /// of the wallet. -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct AccountBalance { /// The value of unspent Sapling outputs belonging to the account. pub sapling_balance: Balance, @@ -144,12 +144,12 @@ impl Ratio { /// can only be certain to be unspent in the case that [`Self::is_synced`] is true, and even in /// this circumstance it is possible that a newly created transaction could conflict with a /// not-yet-mined transaction in the mempool. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct WalletSummary { account_balances: BTreeMap, chain_tip_height: BlockHeight, fully_scanned_height: BlockHeight, - sapling_scan_progress: Option>, + scan_progress: Option>, } impl WalletSummary { @@ -158,13 +158,13 @@ impl WalletSummary { account_balances: BTreeMap, chain_tip_height: BlockHeight, fully_scanned_height: BlockHeight, - sapling_scan_progress: Option>, + scan_progress: Option>, ) -> Self { Self { account_balances, chain_tip_height, fully_scanned_height, - sapling_scan_progress, + scan_progress, } } @@ -178,20 +178,20 @@ impl WalletSummary { self.chain_tip_height } - /// Returns the height below which all blocks wallet have been scanned, ignoring blocks below - /// the wallet birthday. + /// Returns the height below which all blocks have been scanned by the wallet, ignoring blocks + /// below the wallet birthday. pub fn fully_scanned_height(&self) -> BlockHeight { self.fully_scanned_height } - /// Returns the progress of scanning Sapling outputs, in terms of the ratio between notes + /// Returns the progress of scanning shielded outputs, in terms of the ratio between notes /// scanned and the total number of notes added to the chain since the wallet birthday. /// /// This ratio should only be used to compute progress percentages, and the numerator and /// denominator should not be treated as authoritative note counts. Returns `None` if the /// wallet is unable to determine the size of the note commitment tree. - pub fn sapling_scan_progress(&self) -> Option> { - self.sapling_scan_progress + pub fn scan_progress(&self) -> Option> { + self.scan_progress } /// Returns whether or not wallet scanning is complete. diff --git a/zcash_client_sqlite/src/chain.rs b/zcash_client_sqlite/src/chain.rs index a69bcce221..950d2e2b30 100644 --- a/zcash_client_sqlite/src/chain.rs +++ b/zcash_client_sqlite/src/chain.rs @@ -326,7 +326,10 @@ mod tests { use zcash_primitives::{ block::BlockHash, - transaction::{components::Amount, fees::zip317::FeeRule}, + transaction::{ + components::{amount::NonNegativeAmount, Amount}, + fees::zip317::FeeRule, + }, zip32::ExtendedSpendingKey, }; @@ -344,7 +347,7 @@ mod tests { use crate::{ testing::{AddressType, TestBuilder}, - wallet::{get_balance, truncate_to_height}, + wallet::truncate_to_height, AccountId, }; @@ -441,24 +444,21 @@ mod tests { let dfvk = st.test_account_sapling().unwrap(); - // Account balance should be zero - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - Amount::zero() - ); + // Wallet summary is not yet available + assert_eq!(st.get_wallet_summary(0), None); // Create fake CompactBlocks sending value to the address - let value = Amount::from_u64(5).unwrap(); - let value2 = Amount::from_u64(7).unwrap(); - let (h, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); - st.generate_next_block(&dfvk, AddressType::DefaultExternal, value2); + let value = NonNegativeAmount::from_u64(5).unwrap(); + let value2 = NonNegativeAmount::from_u64(7).unwrap(); + let (h, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into()); + st.generate_next_block(&dfvk, AddressType::DefaultExternal, value2.into()); // Scan the cache st.scan_cached_blocks(h, 2); // Account balance should reflect both received notes assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), + st.get_total_balance(AccountId::from(0)), (value + value2).unwrap() ); @@ -469,7 +469,7 @@ mod tests { // Account balance should be unaltered assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), + st.get_total_balance(AccountId::from(0)), (value + value2).unwrap() ); @@ -479,17 +479,14 @@ mod tests { .unwrap(); // Account balance should only contain the first received note - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - value - ); + assert_eq!(st.get_total_balance(AccountId::from(0)), value); // Scan the cache again st.scan_cached_blocks(h, 2); // Account balance should again reflect both received notes assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), + st.get_total_balance(AccountId::from(0)), (value + value2).unwrap() ); } @@ -505,17 +502,14 @@ mod tests { let dfvk = st.test_account_sapling().unwrap(); // Create a block with height SAPLING_ACTIVATION_HEIGHT - let value = Amount::from_u64(50000).unwrap(); - let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + let value = NonNegativeAmount::from_u64(50000).unwrap(); + let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into()); st.scan_cached_blocks(h1, 1); - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - value - ); + assert_eq!(st.get_total_balance(AccountId::from(0)), value); // Create blocks to reach SAPLING_ACTIVATION_HEIGHT + 2 - let (h2, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); - let (h3, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + let (h2, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into()); + let (h3, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into()); // Scan the later block first st.scan_cached_blocks(h3, 1); @@ -523,8 +517,8 @@ mod tests { // Now scan the block of height SAPLING_ACTIVATION_HEIGHT + 1 st.scan_cached_blocks(h2, 1); assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - Amount::from_u64(150_000).unwrap() + st.get_total_balance(AccountId::from(0)), + NonNegativeAmount::from_u64(150_000).unwrap() ); // We can spend the received notes @@ -562,35 +556,29 @@ mod tests { let dfvk = st.test_account_sapling().unwrap(); - // Account balance should be zero - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - Amount::zero() - ); + // Wallet summary is not yet available + assert_eq!(st.get_wallet_summary(0), None); // Create a fake CompactBlock sending value to the address - let value = Amount::from_u64(5).unwrap(); - let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + let value = NonNegativeAmount::from_u64(5).unwrap(); + let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into()); // Scan the cache st.scan_cached_blocks(h1, 1); // Account balance should reflect the received note - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - value - ); + assert_eq!(st.get_total_balance(AccountId::from(0)), value); // Create a second fake CompactBlock sending more value to the address - let value2 = Amount::from_u64(7).unwrap(); - let (h2, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value2); + let value2 = NonNegativeAmount::from_u64(7).unwrap(); + let (h2, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value2.into()); // Scan the cache again st.scan_cached_blocks(h2, 1); // Account balance should reflect both received notes assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), + st.get_total_balance(AccountId::from(0)), (value + value2).unwrap() ); } @@ -603,38 +591,33 @@ mod tests { .build(); let dfvk = st.test_account_sapling().unwrap(); - // Account balance should be zero - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - Amount::zero() - ); + // Wallet summary is not yet available + assert_eq!(st.get_wallet_summary(0), None); // Create a fake CompactBlock sending value to the address - let value = Amount::from_u64(5).unwrap(); + let value = NonNegativeAmount::from_u64(5).unwrap(); let (received_height, _, nf) = - st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into()); // Scan the cache st.scan_cached_blocks(received_height, 1); // Account balance should reflect the received note - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - value - ); + assert_eq!(st.get_total_balance(AccountId::from(0)), value); // Create a second fake CompactBlock spending value from the address let extsk2 = ExtendedSpendingKey::master(&[0]); let to2 = extsk2.default_address().1; - let value2 = Amount::from_u64(2).unwrap(); - let (spent_height, _) = st.generate_next_block_spending(&dfvk, (nf, value), to2, value2); + let value2 = NonNegativeAmount::from_u64(2).unwrap(); + let (spent_height, _) = + st.generate_next_block_spending(&dfvk, (nf, value.into()), to2, value2.into()); // Scan the cache again st.scan_cached_blocks(spent_height, 1); // Account balance should equal the change assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), + st.get_total_balance(AccountId::from(0)), (value - value2).unwrap() ); } @@ -648,29 +631,27 @@ mod tests { let dfvk = st.test_account_sapling().unwrap(); - // Account balance should be zero - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - Amount::zero() - ); + // Wallet summary is not yet available + assert_eq!(st.get_wallet_summary(0), None); // Create a fake CompactBlock sending value to the address - let value = Amount::from_u64(5).unwrap(); + let value = NonNegativeAmount::from_u64(5).unwrap(); let (received_height, _, nf) = - st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into()); // Create a second fake CompactBlock spending value from the address let extsk2 = ExtendedSpendingKey::master(&[0]); let to2 = extsk2.default_address().1; - let value2 = Amount::from_u64(2).unwrap(); - let (spent_height, _) = st.generate_next_block_spending(&dfvk, (nf, value), to2, value2); + let value2 = NonNegativeAmount::from_u64(2).unwrap(); + let (spent_height, _) = + st.generate_next_block_spending(&dfvk, (nf, value.into()), to2, value2.into()); // Scan the spending block first. st.scan_cached_blocks(spent_height, 1); // Account balance should equal the change assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), + st.get_total_balance(AccountId::from(0)), (value - value2).unwrap() ); @@ -679,7 +660,7 @@ mod tests { // Account balance should be the same. assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), + st.get_total_balance(AccountId::from(0)), (value - value2).unwrap() ); } diff --git a/zcash_client_sqlite/src/testing.rs b/zcash_client_sqlite/src/testing.rs index 7fbb46e192..0034ff4bbb 100644 --- a/zcash_client_sqlite/src/testing.rs +++ b/zcash_client_sqlite/src/testing.rs @@ -14,6 +14,7 @@ use tempfile::NamedTempFile; #[cfg(feature = "unstable")] use tempfile::TempDir; +use zcash_client_backend::data_api::AccountBalance; #[allow(deprecated)] use zcash_client_backend::{ address::RecipientAddress, @@ -290,66 +291,6 @@ where limit, ) } - - pub(crate) fn get_total_balance(&self, account: AccountId) -> NonNegativeAmount { - get_wallet_summary(&self.wallet().conn, 0, &SubtreeScanProgress) - .unwrap() - .unwrap() - .account_balances() - .get(&account) - .unwrap() - .total() - } - - pub(crate) fn get_spendable_balance( - &self, - account: AccountId, - min_confirmations: u32, - ) -> NonNegativeAmount { - let binding = - get_wallet_summary(&self.wallet().conn, min_confirmations, &SubtreeScanProgress) - .unwrap() - .unwrap(); - let balance = binding.account_balances().get(&account).unwrap(); - - balance.sapling_balance.spendable_value - } - - pub(crate) fn get_pending_shielded_balance( - &self, - account: AccountId, - min_confirmations: u32, - ) -> NonNegativeAmount { - let binding = - get_wallet_summary(&self.wallet().conn, min_confirmations, &SubtreeScanProgress) - .unwrap() - .unwrap(); - let balance = binding.account_balances().get(&account).unwrap(); - - (balance.sapling_balance.value_pending_spendability - + balance.sapling_balance.change_pending_confirmation) - .unwrap() - } - - pub(crate) fn get_pending_change( - &self, - account: AccountId, - min_confirmations: u32, - ) -> NonNegativeAmount { - let binding = - get_wallet_summary(&self.wallet().conn, min_confirmations, &SubtreeScanProgress) - .unwrap() - .unwrap(); - let balance = binding.account_balances().get(&account).unwrap(); - - balance.sapling_balance.change_pending_confirmation - } - - pub(crate) fn get_wallet_summary(&self, min_confirmations: u32) -> WalletSummary { - get_wallet_summary(&self.wallet().conn, min_confirmations, &SubtreeScanProgress) - .unwrap() - .unwrap() - } } impl TestState { @@ -594,6 +535,60 @@ impl TestState { min_confirmations, ) } + + fn with_account_balance T>( + &self, + account: AccountId, + min_confirmations: u32, + f: F, + ) -> T { + let binding = + get_wallet_summary(&self.wallet().conn, min_confirmations, &SubtreeScanProgress) + .unwrap() + .unwrap(); + + f(binding.account_balances().get(&account).unwrap()) + } + + pub(crate) fn get_total_balance(&self, account: AccountId) -> NonNegativeAmount { + self.with_account_balance(account, 0, |balance| balance.total()) + } + + pub(crate) fn get_spendable_balance( + &self, + account: AccountId, + min_confirmations: u32, + ) -> NonNegativeAmount { + self.with_account_balance(account, min_confirmations, |balance| { + balance.sapling_balance.spendable_value + }) + } + + pub(crate) fn get_pending_shielded_balance( + &self, + account: AccountId, + min_confirmations: u32, + ) -> NonNegativeAmount { + self.with_account_balance(account, min_confirmations, |balance| { + balance.sapling_balance.value_pending_spendability + + balance.sapling_balance.change_pending_confirmation + }) + .unwrap() + } + + pub(crate) fn get_pending_change( + &self, + account: AccountId, + min_confirmations: u32, + ) -> NonNegativeAmount { + self.with_account_balance(account, min_confirmations, |balance| { + balance.sapling_balance.change_pending_confirmation + }) + } + + pub(crate) fn get_wallet_summary(&self, min_confirmations: u32) -> Option { + get_wallet_summary(&self.wallet().conn, min_confirmations, &SubtreeScanProgress).unwrap() + } } #[allow(dead_code)] diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 41b775102c..d929bbb462 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -493,34 +493,6 @@ pub(crate) fn is_valid_account_extfvk( }) } -/// Returns the balance for the account, including all mined unspent notes that we know -/// about. -/// -/// WARNING: This balance is potentially unreliable, as mined notes may become unmined due -/// to chain reorgs. You should generally not show this balance to users without some -/// caveat. Use [`get_balance_at`] where you need a more reliable indication of the -/// wallet balance. -#[cfg(test)] -pub(crate) fn get_balance( - conn: &rusqlite::Connection, - account: AccountId, -) -> Result { - let balance = conn.query_row( - "SELECT SUM(value) FROM sapling_received_notes - INNER JOIN transactions ON transactions.id_tx = sapling_received_notes.tx - WHERE account = ? AND spent IS NULL AND transactions.block IS NOT NULL", - [u32::from(account)], - |row| row.get(0).or(Ok(0)), - )?; - - match Amount::from_i64(balance) { - Ok(amount) if !amount.is_negative() => Ok(amount), - _ => Err(SqliteClientError::CorruptedData( - "Sum of values in sapling_received_notes is out of range".to_string(), - )), - } -} - pub(crate) trait ScanProgress { fn sapling_scan_progress( &self, @@ -629,7 +601,7 @@ pub(crate) fn get_wallet_summary( let fully_scanned_height = block_fully_scanned(conn)?.map_or(birthday_height - 1, |m| m.block_height()); - let summary_height = chain_tip_height + 1 - min_confirmations; + let summary_height = (chain_tip_height + 1).saturating_sub(std::cmp::max(min_confirmations, 1)); let sapling_scan_progress = progress.sapling_scan_progress( conn, @@ -638,10 +610,10 @@ pub(crate) fn get_wallet_summary( chain_tip_height, )?; - // If the shard containing the anchor is contains any unscanned ranges below the summary - // height, none of our balance is currently spendable. + // If the shard containing the summary height contains any unscanned ranges that start below or + // including that height, none of our balance is currently spendable. let any_spendable = conn.query_row( - "SELECT EXISTS( + "SELECT NOT EXISTS( SELECT 1 FROM v_sapling_shard_unscanned_ranges WHERE :summary_height BETWEEN subtree_start_height @@ -649,11 +621,11 @@ pub(crate) fn get_wallet_summary( AND block_range_start <= :summary_height )", named_params![":summary_height": u32::from(summary_height)], - |row| row.get::<_, bool>(0).map(|b| !b), + |row| row.get::<_, bool>(0), )?; let mut stmt_select_notes = conn.prepare_cached( - "SELECT n.account, n.value, n.is_change, scan_state.max_priority, t.block, t.expiry_height + "SELECT n.account, n.value, n.is_change, scan_state.max_priority, t.block FROM sapling_received_notes n JOIN transactions t ON t.id_tx = n.tx LEFT OUTER JOIN v_sapling_shards_scan_state scan_state @@ -696,9 +668,7 @@ pub(crate) fn get_wallet_summary( }, )?; - let received_height = row - .get::<_, Option>(4) - .map(|opt| opt.map(BlockHeight::from))?; + let received_height = row.get::<_, Option>(4)?.map(BlockHeight::from); let is_spendable = any_spendable && received_height.iter().any(|h| h <= &summary_height) @@ -763,7 +733,10 @@ pub(crate) fn get_wallet_summary( account_balances .entry(account) - .and_modify(|bal| bal.unshielded = value) + .and_modify(|bal| { + bal.unshielded = + (bal.unshielded + value).expect("Unshielded value cannot overflow") + }) .or_insert(AccountBalance { sapling_balance: Balance::ZERO, unshielded: value, @@ -1924,8 +1897,6 @@ mod tests { use crate::{testing::TestBuilder, AccountId}; - use super::get_balance; - #[cfg(feature = "transparent-inputs")] use { incrementalmerkletree::frontier::Frontier, @@ -1945,11 +1916,8 @@ mod tests { .with_test_account(AccountBirthday::from_sapling_activation) .build(); - // The account should be empty - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - Amount::zero() - ); + // The account should have no summary information + assert_eq!(st.get_wallet_summary(0), None); // We can't get an anchor height, as we have not scanned any blocks. assert_eq!( @@ -1959,15 +1927,17 @@ mod tests { None ); - // An invalid account has zero balance + // The default address is set for the test account + assert_matches!( + st.wallet().get_current_address(AccountId::from(0)), + Ok(Some(_)) + ); + + // No default address is set for an un-initialized account assert_matches!( st.wallet().get_current_address(AccountId::from(1)), Ok(None) ); - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - Amount::zero() - ); } #[test] diff --git a/zcash_client_sqlite/src/wallet/init/migrations/wallet_summaries.rs b/zcash_client_sqlite/src/wallet/init/migrations/wallet_summaries.rs index 5807bae65c..2be9ca15ab 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/wallet_summaries.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/wallet_summaries.rs @@ -43,7 +43,7 @@ impl RusqliteMigration for Migration { )?; transaction.execute_batch( - // set the number of outputs everywhere that we have sequential Sapling blocks + // set the number of outputs everywhere that we have sequential blocks "CREATE TEMPORARY TABLE block_deltas AS SELECT cur.height AS height, diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index 4e1f3702ab..d2a5271c11 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -465,7 +465,7 @@ pub(crate) mod tests { use crate::{ error::SqliteClientError, testing::{AddressType, BlockCache, TestBuilder, TestState}, - wallet::{commitment_tree, get_balance}, + wallet::commitment_tree, AccountId, NoteId, ReceivedNoteId, }; @@ -495,19 +495,12 @@ pub(crate) mod tests { let dfvk = st.test_account_sapling().unwrap(); // Add funds to the wallet in a single note - let value = Amount::from_u64(60000).unwrap(); - let (h, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + let value = NonNegativeAmount::from_u64(60000).unwrap(); + let (h, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into()); st.scan_cached_blocks(h, 1); // Verified balance matches total balance - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - value - ); - assert_eq!( - st.get_total_balance(account), - NonNegativeAmount::try_from(value).unwrap() - ); + assert_eq!(st.get_total_balance(account), value); let to_extsk = ExtendedSpendingKey::master(&[]); let to: RecipientAddress = to_extsk.default_address().1.into(); @@ -664,11 +657,8 @@ pub(crate) mod tests { let dfvk = st.test_account_sapling().unwrap(); let to = dfvk.default_address().1.into(); - // Account balance should be zero - assert_eq!( - get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(), - Amount::zero() - ); + // Wallet summary is not yet available + assert_eq!(st.get_wallet_summary(0), None); // We cannot do anything if we aren't synchronised assert_matches!( @@ -700,10 +690,6 @@ pub(crate) mod tests { st.scan_cached_blocks(h1, 1); // Verified balance matches total balance - assert_eq!( - get_balance(&st.wallet().conn, account).unwrap(), - value.into() - ); assert_eq!(st.get_total_balance(account), value); // Value is considered pending @@ -711,7 +697,10 @@ pub(crate) mod tests { // Wallet is fully scanned let summary = st.get_wallet_summary(1); - assert_eq!(summary.sapling_scan_progress(), Some(Ratio::new(1, 1))); + assert_eq!( + summary.and_then(|s| s.scan_progress()), + Some(Ratio::new(1, 1)) + ); // Add more funds to the wallet in a second note let (h2, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into()); @@ -719,17 +708,16 @@ pub(crate) mod tests { // Verified balance does not include the second note let total = (value + value).unwrap(); - assert_eq!( - get_balance(&st.wallet().conn, account).unwrap(), - total.into() - ); assert_eq!(st.get_spendable_balance(account, 2), value); assert_eq!(st.get_pending_shielded_balance(account, 2), value); assert_eq!(st.get_total_balance(account), total); // Wallet is still fully scanned let summary = st.get_wallet_summary(1); - assert_eq!(summary.sapling_scan_progress(), Some(Ratio::new(2, 2))); + assert_eq!( + summary.and_then(|s| s.scan_progress()), + Some(Ratio::new(2, 2)) + ); // Spend fails because there are insufficient verified notes let extsk2 = ExtendedSpendingKey::master(&[]); @@ -805,10 +793,10 @@ pub(crate) mod tests { let dfvk = st.test_account_sapling().unwrap(); // Add funds to the wallet in a single note - let value = Amount::from_u64(50000).unwrap(); - let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + let value = NonNegativeAmount::from_u64(50000).unwrap(); + let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into()); st.scan_cached_blocks(h1, 1); - assert_eq!(get_balance(&st.wallet().conn, account).unwrap(), value); + assert_eq!(st.get_total_balance(account), value); // Send some of the funds to another address let extsk2 = ExtendedSpendingKey::master(&[]); @@ -848,7 +836,7 @@ pub(crate) mod tests { st.generate_next_block( &ExtendedSpendingKey::master(&[i as u8]).to_diversifiable_full_viewing_key(), AddressType::DefaultExternal, - value, + value.into(), ); } st.scan_cached_blocks(h1 + 1, 41); @@ -874,7 +862,7 @@ pub(crate) mod tests { let (h43, _, _) = st.generate_next_block( &ExtendedSpendingKey::master(&[42]).to_diversifiable_full_viewing_key(), AddressType::DefaultExternal, - value, + value.into(), ); st.scan_cached_blocks(h43, 1); @@ -901,10 +889,10 @@ pub(crate) mod tests { let dfvk = st.test_account_sapling().unwrap(); // Add funds to the wallet in a single note - let value = Amount::from_u64(50000).unwrap(); - let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + let value = NonNegativeAmount::from_u64(50000).unwrap(); + let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into()); st.scan_cached_blocks(h1, 1); - assert_eq!(get_balance(&st.wallet().conn, account).unwrap(), value); + assert_eq!(st.get_total_balance(account), value); let extsk2 = ExtendedSpendingKey::master(&[]); let addr2 = extsk2.default_address().1; @@ -975,7 +963,7 @@ pub(crate) mod tests { st.generate_next_block( &ExtendedSpendingKey::master(&[i as u8]).to_diversifiable_full_viewing_key(), AddressType::DefaultExternal, - value, + value.into(), ); } st.scan_cached_blocks(h1 + 1, 42); @@ -1004,7 +992,6 @@ pub(crate) mod tests { st.scan_cached_blocks(h, 1); // Verified balance matches total balance - assert_eq!(get_balance(&st.wallet().conn, account).unwrap(), value); assert_eq!( st.get_total_balance(account), NonNegativeAmount::try_from(value).unwrap() @@ -1040,7 +1027,6 @@ pub(crate) mod tests { st.scan_cached_blocks(h, 1); // Verified balance matches total balance - assert_eq!(get_balance(&st.wallet().conn, account).unwrap(), value); assert_eq!( st.get_total_balance(account), NonNegativeAmount::try_from(value).unwrap() @@ -1096,7 +1082,6 @@ pub(crate) mod tests { // Verified balance matches total balance let total = Amount::from_u64(60000).unwrap(); - assert_eq!(get_balance(&st.wallet().conn, account).unwrap(), total); assert_eq!( st.get_total_balance(account), NonNegativeAmount::try_from(total).unwrap() diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index fba6cd4d9c..47066e1627 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -1692,7 +1692,7 @@ mod tests { // We have scan ranges and a subtree, but have scanned no blocks. let summary = st.get_wallet_summary(1); - assert_eq!(summary.sapling_scan_progress(), None); + assert_eq!(summary.and_then(|s| s.scan_progress()), None); // Set up prior chain state. This simulates us having imported a wallet // with a birthday 520 blocks below the chain tip. @@ -1732,7 +1732,7 @@ mod tests { // wallet birthday but before the end of the shard. let summary = st.get_wallet_summary(1); assert_eq!( - summary.sapling_scan_progress(), + summary.and_then(|s| s.scan_progress()), Some(Ratio::new(1, 0x1 << SAPLING_SHARD_HEIGHT)) ); @@ -1776,7 +1776,7 @@ mod tests { // shards worth of notes to scan. let summary = st.get_wallet_summary(1); assert_eq!( - summary.sapling_scan_progress(), + summary.and_then(|s| s.scan_progress()), Some(Ratio::new(1, 0x1 << (SAPLING_SHARD_HEIGHT + 1))) ); } diff --git a/zcash_primitives/src/transaction/components/amount.rs b/zcash_primitives/src/transaction/components/amount.rs index fc7e53a6e4..dbb3e85d7c 100644 --- a/zcash_primitives/src/transaction/components/amount.rs +++ b/zcash_primitives/src/transaction/components/amount.rs @@ -290,6 +290,22 @@ impl Add for Option { } } +impl Sub for NonNegativeAmount { + type Output = Option; + + fn sub(self, rhs: NonNegativeAmount) -> Option { + (self.0 - rhs.0).and_then(|amt| NonNegativeAmount::try_from(amt).ok()) + } +} + +impl Sub for Option { + type Output = Self; + + fn sub(self, rhs: NonNegativeAmount) -> Option { + self.and_then(|lhs| lhs - rhs) + } +} + /// A type for balance violations in amount addition and subtraction /// (overflow and underflow of allowed ranges) #[derive(Copy, Clone, Debug, PartialEq, Eq)] From 6cbb107c71124b78cde1a19dc52173fb447045b1 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 5 Sep 2023 16:32:28 -0600 Subject: [PATCH 3/4] zcash_client_sqlite: allow zero-conf transactions in unshielded balance. --- zcash_client_sqlite/src/wallet.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index d929bbb462..cc52f40ab9 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -712,6 +712,7 @@ pub(crate) fn get_wallet_summary( #[cfg(feature = "transparent-inputs")] { + let zero_conf_height = (chain_tip_height + 1).saturating_sub(min_confirmations); let mut stmt_transparent_balances = conn.prepare( "SELECT u.received_by_account, SUM(u.value_zat) FROM utxos u @@ -722,7 +723,7 @@ pub(crate) fn get_wallet_summary( GROUP BY u.received_by_account", )?; let mut rows = stmt_transparent_balances - .query(named_params![":max_height": u32::from(summary_height)])?; + .query(named_params![":max_height": u32::from(zero_conf_height)])?; while let Some(row) = rows.next()? { let account = AccountId::from(row.get::<_, u32>(0)?); From 1e12e9d0e6ccb4583a173e4e8e295b9cab38663a Mon Sep 17 00:00:00 2001 From: str4d Date: Wed, 6 Sep 2023 00:18:11 +0100 Subject: [PATCH 4/4] Clarify zero-conf shielded note behaviour for `get_wallet_summary` Co-authored-by: Daira Emma Hopwood --- zcash_client_sqlite/src/wallet.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index cc52f40ab9..a4bb4846bc 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -584,6 +584,9 @@ impl ScanProgress for SubtreeScanProgress { /// /// This may be used to obtain a balance that ignores notes that have been detected so recently /// that they are not yet spendable, or for which it is not yet possible to construct witnesses. +/// +/// `min_confirmations` can be 0, but that case is currently treated identically to +/// `min_confirmations == 1` for shielded notes. This behaviour may change in the future. pub(crate) fn get_wallet_summary( conn: &rusqlite::Connection, min_confirmations: u32,