From dee43854cb69fe428d4779eda7202d7744de0fb5 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 11 Aug 2023 16:41:38 -0600 Subject: [PATCH] zcash_client_sqlite: Ensure that target and anchor heights are relative to the chain tip. Prior to the scan-before-sync changes, the wallet was able to assume that the maximum scanned block height at the time of the spend was within a few blocks of the chain tip. However, under linear scanning after the spend-before-sync changes this invariant no longer holds, resulting in a situation where in linear sync conditions the wallet could attempt to create transactions with already-past expiry heights. This change separates the notion of "chain tip" from "max scanned height", relying upon the `scan_queue` table to maintain the wallet's view of the consensus chain height and using information from the `blocks` table only in situations where the latest and/or earliest scanned height is required. As part of this change, the `WalletRead` interface is also modified to disambiguate these concepts. --- zcash_client_backend/CHANGELOG.md | 16 ++- zcash_client_backend/src/data_api.rs | 58 ++++------- zcash_client_backend/src/data_api/wallet.rs | 4 +- zcash_client_sqlite/src/chain.rs | 2 +- zcash_client_sqlite/src/lib.rs | 23 ++++- zcash_client_sqlite/src/wallet.rs | 62 +++++++++++- zcash_client_sqlite/src/wallet/sapling.rs | 105 ++++++++------------ zcash_client_sqlite/src/wallet/scanning.rs | 8 +- 8 files changed, 157 insertions(+), 121 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index bcd1b6b5d..6980d35ef 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -17,7 +17,7 @@ and this library adheres to Rust's notion of - `ScannedBlock` - `ShieldedProtocol` - `WalletCommitmentTrees` - - `WalletRead::{block_metadata, block_fully_scanned, suggest_scan_ranges}` + - `WalletRead::{chain_height, block_metadata, block_fully_scanned, suggest_scan_ranges}` - `WalletWrite::{put_blocks, update_chain_tip}` - `chain::CommitmentTreeRoot` - `scanning` A new module containing types required for `suggest_scan_ranges` @@ -89,10 +89,13 @@ and this library adheres to Rust's notion of ### Removed - `zcash_client_backend::data_api`: - - `WalletRead::get_all_nullifiers` - - `WalletRead::{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. + - `WalletRead::block_height_extrema` has been removed. Use `chain_height` + 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_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. - `WalletWrite::advance_by_block` (use `WalletWrite::put_blocks` instead). - `PrunedBlock` has been replaced by `ScannedBlock` - `testing::MockWalletDb`, which is available under the `test-dependencies` @@ -107,6 +110,9 @@ and this library adheres to Rust's notion of have been removed as individual incremental witnesses are no longer tracked on a per-note basis. The global note commitment tree for the wallet should be used to obtain witnesses for spend operations instead. +- Default implementations of `zcash_client_backend::data_api::WalletRead::{ + get_target_and_anchor_heights, get_max_height_hash + }` have been removed. These should be implemented in a backend-specific fashion. ## [0.9.0] - 2023-04-28 diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index af1116031..d1711375f 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -1,6 +1,5 @@ //! Interfaces for wallet data persistence & low-level wallet utilities. -use std::cmp; use std::collections::HashMap; use std::fmt::Debug; use std::num::NonZeroU32; @@ -60,10 +59,11 @@ pub trait WalletRead { /// or a UUID. type NoteRef: Copy + Debug + Eq + Ord; - /// Returns the minimum and maximum block heights for stored blocks. + /// Returns the height of the chain as known to the wallet as of the most recent call to + /// [`WalletWrite::update_chain_tip`]. /// - /// This will return `Ok(None)` if no block data is present in the database. - fn block_height_extrema(&self) -> Result, Self::Error>; + /// This will return `Ok(None)` if the height of the current consensus chain tip is unknown. + fn chain_height(&self) -> Result, Self::Error>; /// Returns the available block metadata for the block at the specified height, if any. fn block_metadata(&self, height: BlockHeight) -> Result, Self::Error>; @@ -98,22 +98,7 @@ pub trait WalletRead { fn get_target_and_anchor_heights( &self, min_confirmations: NonZeroU32, - ) -> Result, Self::Error> { - self.block_height_extrema().map(|heights| { - heights.map(|(min_height, max_height)| { - let target_height = max_height + 1; - - // Select an anchor min_confirmations back from the target block, - // unless that would be before the earliest block we have. - let anchor_height = BlockHeight::from(cmp::max( - u32::from(target_height).saturating_sub(min_confirmations.into()), - u32::from(min_height), - )); - - (target_height, anchor_height) - }) - }) - } + ) -> Result, Self::Error>; /// Returns the minimum block height corresponding to an unspent note in the wallet. fn get_min_unspent_height(&self) -> Result, Self::Error>; @@ -123,22 +108,10 @@ pub trait WalletRead { /// is not found in the database. fn get_block_hash(&self, block_height: BlockHeight) -> Result, Self::Error>; - /// Returns the block hash for the block at the maximum height known - /// in stored data. + /// Returns the block height and hash for the block at the maximum scanned block height. /// - /// This will return `Ok(None)` if no block data is present in the database. - fn get_max_height_hash(&self) -> Result, Self::Error> { - self.block_height_extrema() - .and_then(|extrema_opt| { - extrema_opt - .map(|(_, max_height)| { - self.get_block_hash(max_height) - .map(|hash_opt| hash_opt.map(move |hash| (max_height, hash))) - }) - .transpose() - }) - .map(|oo| oo.flatten()) - } + /// This will return `Ok(None)` if no blocks have been scanned. + fn get_max_height_hash(&self) -> Result, Self::Error>; /// Returns the block height in which the specified transaction was mined, or `Ok(None)` if the /// transaction is not in the main chain. @@ -613,7 +586,7 @@ pub mod testing { use incrementalmerkletree::Address; use secrecy::{ExposeSecret, SecretVec}; use shardtree::{error::ShardTreeError, store::memory::MemoryShardStore, ShardTree}; - use std::{collections::HashMap, convert::Infallible}; + use std::{collections::HashMap, convert::Infallible, num::NonZeroU32}; use zcash_primitives::{ block::BlockHash, @@ -662,7 +635,14 @@ pub mod testing { type Error = (); type NoteRef = u32; - fn block_height_extrema(&self) -> Result, Self::Error> { + fn chain_height(&self) -> Result, Self::Error> { + Ok(None) + } + + fn get_target_and_anchor_heights( + &self, + _min_confirmations: NonZeroU32, + ) -> Result, Self::Error> { Ok(None) } @@ -692,6 +672,10 @@ pub mod testing { Ok(None) } + fn get_max_height_hash(&self) -> Result, Self::Error> { + Ok(None) + } + fn get_tx_height(&self, _txid: TxId) -> Result, Self::Error> { Ok(None) } diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index c202a597b..da820e729 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -66,9 +66,7 @@ where // for mempool transactions. let height = data .get_tx_height(tx.txid())? - .or(data - .block_height_extrema()? - .map(|(_, max_height)| max_height + 1)) + .or(data.chain_height()?.map(|max_height| max_height + 1)) .or_else(|| params.activation_height(NetworkUpgrade::Sapling)) .expect("Sapling activation height must be known."); diff --git a/zcash_client_sqlite/src/chain.rs b/zcash_client_sqlite/src/chain.rs index c66cde05d..f95f6b63a 100644 --- a/zcash_client_sqlite/src/chain.rs +++ b/zcash_client_sqlite/src/chain.rs @@ -370,7 +370,7 @@ mod tests { let (dfvk, _taddr) = init_test_accounts_table(&mut db_data); // Empty chain should return None - assert_matches!(db_data.get_max_height_hash(), Ok(None)); + assert_matches!(db_data.chain_height(), Ok(None)); // Create a fake CompactBlock sending value to the address let (cb, _) = fake_compact_block( diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 5cc7cd49b..4935e2c5c 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -38,7 +38,10 @@ use maybe_rayon::{ }; use rusqlite::{self, Connection}; use secrecy::{ExposeSecret, SecretVec}; -use std::{borrow::Borrow, collections::HashMap, convert::AsRef, fmt, ops::Range, path::Path}; +use std::{ + borrow::Borrow, collections::HashMap, convert::AsRef, fmt, num::NonZeroU32, ops::Range, + path::Path, +}; use incrementalmerkletree::Position; use shardtree::{error::ShardTreeError, ShardTree}; @@ -157,8 +160,10 @@ impl, P: consensus::Parameters> WalletRead for W type Error = SqliteClientError; type NoteRef = ReceivedNoteId; - fn block_height_extrema(&self) -> Result, Self::Error> { - wallet::block_height_extrema(self.conn.borrow()).map_err(SqliteClientError::from) + fn chain_height(&self) -> Result, Self::Error> { + wallet::scan_queue_extrema(self.conn.borrow()) + .map(|h| h.map(|(_, max)| max)) + .map_err(SqliteClientError::from) } fn block_metadata(&self, height: BlockHeight) -> Result, Self::Error> { @@ -174,6 +179,14 @@ impl, P: consensus::Parameters> WalletRead for W .map_err(SqliteClientError::from) } + fn get_target_and_anchor_heights( + &self, + min_confirmations: NonZeroU32, + ) -> Result, Self::Error> { + wallet::get_target_and_anchor_heights(self.conn.borrow(), min_confirmations) + .map_err(SqliteClientError::from) + } + fn get_min_unspent_height(&self) -> Result, Self::Error> { wallet::get_min_unspent_height(self.conn.borrow()).map_err(SqliteClientError::from) } @@ -182,6 +195,10 @@ impl, P: consensus::Parameters> WalletRead for W wallet::get_block_hash(self.conn.borrow(), block_height).map_err(SqliteClientError::from) } + fn get_max_height_hash(&self) -> Result, Self::Error> { + wallet::get_max_height_hash(self.conn.borrow()).map_err(SqliteClientError::from) + } + fn get_tx_height(&self, txid: TxId) -> Result, Self::Error> { wallet::get_tx_height(self.conn.borrow(), txid).map_err(SqliteClientError::from) } diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index ac60f2ced..9de950997 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -65,9 +65,11 @@ //! - `memo` the shielded memo associated with the output, if any. use rusqlite::{self, named_params, OptionalExtension, ToSql}; +use std::cmp; use std::collections::HashMap; use std::convert::TryFrom; use std::io::{self, Cursor}; +use std::num::NonZeroU32; use zcash_client_backend::data_api::scanning::{ScanPriority, ScanRange}; use zcash_client_backend::data_api::{NoteId, ShieldedProtocol}; @@ -623,6 +625,45 @@ pub(crate) fn block_height_extrema( }) } +/// Returns the minimum and maximum heights of blocks in the chain which may be scanned. +pub(crate) fn scan_queue_extrema( + conn: &rusqlite::Connection, +) -> Result, rusqlite::Error> { + conn.query_row( + "SELECT MIN(block_range_start), MAX(block_range_end) FROM scan_queue", + [], + |row| { + let min_height: Option = row.get(0)?; + let max_height: Option = row.get(1)?; + + // Scan ranges are end-exclusive, so we subtract 1 from `max_height` to obtain the + // height of the last known chain tip; + Ok(min_height + .map(BlockHeight::from) + .zip(max_height.map(|h| BlockHeight::from(h.saturating_sub(1))))) + }, + ) +} + +pub(crate) fn get_target_and_anchor_heights( + conn: &rusqlite::Connection, + min_confirmations: NonZeroU32, +) -> Result, rusqlite::Error> { + scan_queue_extrema(conn).map(|heights| { + heights.map(|(min_height, max_height)| { + let target_height = max_height + 1; + // Select an anchor min_confirmations back from the target block, + // unless that would be before the earliest block we have. + let anchor_height = BlockHeight::from(cmp::max( + u32::from(target_height).saturating_sub(min_confirmations.into()), + u32::from(min_height), + )); + + (target_height, anchor_height) + }) + }) +} + fn parse_block_metadata( row: (BlockHeight, Vec, Option, Vec), ) -> Result { @@ -765,6 +806,21 @@ pub(crate) fn get_block_hash( .optional() } +pub(crate) fn get_max_height_hash( + conn: &rusqlite::Connection, +) -> Result, rusqlite::Error> { + conn.query_row( + "SELECT height, hash FROM blocks ORDER BY height DESC LIMIT 1", + [], + |row| { + let height = row.get::<_, u32>(0).map(BlockHeight::from)?; + let row_data = row.get::<_, Vec<_>>(1)?; + Ok((height, BlockHash::from_slice(&row_data))) + }, + ) + .optional() +} + /// Gets the height to which the database must be truncated if any truncation that would remove a /// number of blocks greater than the pruning height is attempted. pub(crate) fn get_min_unspent_height( @@ -772,9 +828,9 @@ pub(crate) fn get_min_unspent_height( ) -> Result, SqliteClientError> { conn.query_row( "SELECT MIN(tx.block) - FROM sapling_received_notes n - JOIN transactions tx ON tx.id_tx = n.tx - WHERE n.spent IS NULL", + FROM sapling_received_notes n + JOIN transactions tx ON tx.id_tx = n.tx + WHERE n.spent IS NULL", [], |row| { row.get(0) diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index b98732201..2d0499b3e 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -368,12 +368,14 @@ pub(crate) mod tests { use zcash_primitives::{ block::BlockHash, - consensus::{BlockHeight, BranchId}, + consensus::BranchId, legacy::TransparentAddress, memo::Memo, - sapling::{note_encryption::try_sapling_output_recovery, prover::TxProver}, + sapling::{ + note_encryption::try_sapling_output_recovery, prover::TxProver, Note, PaymentAddress, + }, transaction::{ - components::Amount, + components::{amount::BalanceError, Amount}, fees::{fixed::FeeRule as FixedFeeRule, zip317::FeeRule as Zip317FeeRule}, Transaction, }, @@ -388,7 +390,8 @@ pub(crate) mod tests { error::Error, wallet::{ create_proposed_transaction, create_spend_to_address, - input_selection::GreedyInputSelector, propose_transfer, spend, + input_selection::{GreedyInputSelector, GreedyInputSelectorError}, + propose_transfer, spend, }, ShieldedProtocol, WalletRead, WalletWrite, }, @@ -401,15 +404,13 @@ pub(crate) mod tests { use crate::{ chain::init::init_cache_database, + error::SqliteClientError, tests::{ self, fake_compact_block, insert_into_cache, network, sapling_activation_height, AddressType, }, - wallet::{ - get_balance, get_balance_at, - init::{init_blocks_table, init_wallet_db}, - }, - AccountId, BlockDb, NoteId, WalletDb, + wallet::{commitment_tree, get_balance, get_balance_at, init::init_wallet_db}, + AccountId, BlockDb, NoteId, ReceivedNoteId, WalletDb, }; #[cfg(feature = "transparent-inputs")] @@ -651,6 +652,12 @@ pub(crate) mod tests { let dfvk = usk.sapling().to_diversifiable_full_viewing_key(); let to = dfvk.default_address().1.into(); + // Account balance should be zero + assert_eq!( + get_balance(&db_data.conn, AccountId::from(0)).unwrap(), + Amount::zero() + ); + // We cannot do anything if we aren't synchronised assert_matches!( create_spend_to_address( @@ -668,53 +675,6 @@ pub(crate) mod tests { ); } - #[test] - fn create_to_address_fails_on_insufficient_balance() { - let data_file = NamedTempFile::new().unwrap(); - let mut db_data = WalletDb::for_path(data_file.path(), tests::network()).unwrap(); - init_wallet_db(&mut db_data, None).unwrap(); - init_blocks_table( - &mut db_data, - BlockHeight::from(1u32), - BlockHash([1; 32]), - 1, - &[0x0, 0x0, 0x0], - ) - .unwrap(); - - // Add an account to the wallet - let seed = Secret::new([0u8; 32].to_vec()); - let (_, usk) = db_data.create_account(&seed).unwrap(); - let dfvk = usk.sapling().to_diversifiable_full_viewing_key(); - let to = dfvk.default_address().1.into(); - - // Account balance should be zero - assert_eq!( - get_balance(&db_data.conn, AccountId::from(0)).unwrap(), - Amount::zero() - ); - - // We cannot spend anything - assert_matches!( - create_spend_to_address( - &mut db_data, - &tests::network(), - test_prover(), - &usk, - &to, - Amount::from_u64(1).unwrap(), - None, - OvkPolicy::Sender, - NonZeroU32::new(1).unwrap(), - ), - Err(data_api::error::Error::InsufficientFunds { - available, - required - }) - if available == Amount::zero() && required == Amount::from_u64(10001).unwrap() - ); - } - #[test] fn create_to_address_fails_on_unverified_notes() { let cache_file = NamedTempFile::new().unwrap(); @@ -1101,7 +1061,19 @@ pub(crate) mod tests { let addr2 = extsk2.default_address().1; let to = addr2.into(); - let send_and_recover_with_policy = |db_data: &mut WalletDb, ovk_policy| { + #[allow(clippy::type_complexity)] + let send_and_recover_with_policy = |db_data: &mut WalletDb, + ovk_policy| + -> Result< + Option<(Note, PaymentAddress, MemoBytes)>, + Error< + SqliteClientError, + commitment_tree::Error, + GreedyInputSelectorError, + Infallible, + ReceivedNoteId, + >, + > { let txid = create_spend_to_address( db_data, &tests::network(), @@ -1112,8 +1084,7 @@ pub(crate) mod tests { None, ovk_policy, NonZeroU32::new(1).unwrap(), - ) - .unwrap(); + )?; // Fetch the transaction from the database let raw_tx: Vec<_> = db_data @@ -1137,18 +1108,19 @@ pub(crate) mod tests { ); if result.is_some() { - return result; + return Ok(result); } } - None + Ok(None) }; // Send some of the funds to another address, keeping history. // The recipient output is decryptable by the sender. - let (_, recovered_to, _) = - send_and_recover_with_policy(&mut db_data, OvkPolicy::Sender).unwrap(); - assert_eq!(&recovered_to, &addr2); + assert_matches!( + send_and_recover_with_policy(&mut db_data, OvkPolicy::Sender), + Ok(Some((_, recovered_to, _))) if recovered_to == addr2 + ); // Mine blocks SAPLING_ACTIVATION_HEIGHT + 1 to 42 (that don't send us funds) // so that the first transaction expires @@ -1175,7 +1147,10 @@ pub(crate) mod tests { // Send the funds again, discarding history. // Neither transaction output is decryptable by the sender. - assert!(send_and_recover_with_policy(&mut db_data, OvkPolicy::Discard).is_none()); + assert_matches!( + send_and_recover_with_policy(&mut db_data, OvkPolicy::Discard), + Ok(None) + ); } #[test] diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index d3f456e1a..83781837a 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -619,12 +619,12 @@ pub(crate) fn update_chain_tip( params: &P, new_tip: BlockHeight, ) -> Result<(), SqliteClientError> { - // Read the previous tip height from the blocks table. + // Read the previous max scanned height from the blocks table let prior_tip = block_height_extrema(conn)?.map(|(_, prior_tip)| prior_tip); - // If the chain tip is below the prior tip height, then the caller has caught the - // chain in the middle of a reorg. Do nothing; the caller will continue using the old - // scan ranges and either: + // If the chain tip is below the prior max scanned height, then the caller has caught + // the chain in the middle of a reorg. Do nothing; the caller will continue using the + // old scan ranges and either: // - encounter an error trying to fetch the blocks (and thus trigger the same handling // logic as if this happened with the old linear scanning code); or // - encounter a discontinuity error in `scan_cached_blocks`, at which point they will