From d7380889c21142b44041ff8ba7efdb6ff9cfddb6 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 | 60 ++++++++- zcash_client_sqlite/src/wallet/sapling.rs | 139 ++++++++------------ zcash_client_sqlite/src/wallet/scanning.rs | 8 +- 8 files changed, 168 insertions(+), 142 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 8941d8e31d..6523a1c4c2 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -12,7 +12,7 @@ and this library adheres to Rust's notion of wallet::ReceivedSaplingNote, }` - `impl Debug` for `zcash_client_backend::{ - data_api::wallet::input_selection::Proposal, + data_api::wallet::input_selection::Proposal, wallet::ReceivedSaplingNote }` - `impl Eq for zcash_client_backend::{ @@ -27,7 +27,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, get_spendable_sapling_note, get_unspent_transparent_output}` - `WalletWrite::{put_blocks, update_chain_tip}` - `chain::CommitmentTreeRoot` @@ -128,10 +128,14 @@ 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; the `blocks` + table contains information about scanned blocks, but the wallet may be aware + that the chain tip is at a greater height than is represented in the `blocks` + table without having downloaded or scanned the blocks at the chain tip. + - `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` diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index c612927a6f..2371444679 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. @@ -628,7 +601,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, @@ -677,7 +650,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) } @@ -707,6 +687,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 6466ee9550..3b9c68948e 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -65,9 +65,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 5809db613e..ae69d91828 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 bdf5cd8d59..be077ebc35 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 39d64cb4b0..c5df79daf2 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}; @@ -624,6 +626,43 @@ pub(crate) fn block_height_extrema( }) } +/// Returns the minimum and maximum heights for blocks known to the wallet. +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)?; + Ok(min_height + .map(BlockHeight::from) + .zip(max_height.map(BlockHeight::from))) + }, + ) +} + +pub(crate) fn get_target_and_anchor_heights( + conn: &rusqlite::Connection, + min_confirmations: NonZeroU32, +) -> Result, rusqlite::Error> { + scan_queue_extrema(conn).map(|heights| { + // Scan ranges are end-exclusive, so we can use the max block_range_end value directly as + // the target height as it is one greater than the last known height of the chain tip. + heights.map(|(min_height, target_height)| { + // 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 { @@ -766,6 +805,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( @@ -773,9 +827,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 5db18e6e43..83ecf8b3e7 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -403,13 +403,18 @@ pub(crate) mod tests { use zcash_primitives::{ block::BlockHash, - consensus::{BlockHeight, BranchId, Network}, + consensus::{BranchId, Network}, 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, - fees::{zip317::FeeRule as Zip317FeeRule, StandardFeeRule}, + fees::{ + zip317::{FeeError as Zip317FeeError, FeeRule as Zip317FeeRule}, + StandardFeeRule, + }, Transaction, }, zip32::{sapling::ExtendedSpendingKey, Scope}, @@ -423,7 +428,7 @@ pub(crate) mod tests { error::Error, wallet::{ create_proposed_transaction, - input_selection::{GreedyInputSelector, Proposal}, + input_selection::{GreedyInputSelector, GreedyInputSelectorError, Proposal}, propose_standard_transfer_to_address, propose_transfer, spend, }, ShieldedProtocol, WalletRead, WalletWrite, @@ -438,14 +443,12 @@ 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}, - }, + wallet::{commitment_tree, get_balance, get_balance_at, init::init_wallet_db}, AccountId, BlockDb, NoteId, ReceivedNoteId, WalletDb, }; @@ -715,50 +718,13 @@ pub(crate) mod tests { let dfvk = usk.sapling().to_diversifiable_full_viewing_key(); let to = dfvk.default_address().1.into(); - // We cannot do anything if we aren't synchronised - assert_matches!( - propose_standard_transfer_to_address::<_, _, Infallible>( - &mut db_data, - &tests::network(), - StandardFeeRule::Zip317, - account, - NonZeroU32::new(1).unwrap(), - &to, - Amount::from_u64(1).unwrap(), - None, - None - ), - Err(data_api::error::Error::ScanRequired) - ); - } - - #[test] - fn proposal_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 (account, 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 + // We cannot do anything if we aren't synchronised assert_matches!( propose_standard_transfer_to_address::<_, _, Infallible>( &mut db_data, @@ -771,11 +737,7 @@ pub(crate) mod tests { None, None ), - Err(data_api::error::Error::InsufficientFunds { - available, - required - }) - if available == Amount::zero() && required == Amount::from_u64(10001).unwrap() + Err(data_api::error::Error::ScanRequired) ); } @@ -1225,38 +1187,41 @@ pub(crate) mod tests { #[allow(deprecated)] let fee_rule = StandardFeeRule::PreZip313; - let send_and_recover_with_policy = |db_data: &mut WalletDb, ovk_policy| { - let proposal = { - let res = propose_standard_transfer_to_address::<_, _, Infallible>( - db_data, - &tests::network(), - fee_rule, - account, - NonZeroU32::new(1).unwrap(), - &to, - Amount::from_u64(15000).unwrap(), - None, - None, - ); - assert_matches!(res, Ok(_)); - res.unwrap() - }; + #[allow(clippy::type_complexity)] + let send_and_recover_with_policy = |db_data: &mut WalletDb, + ovk_policy| + -> Result< + Option<(Note, PaymentAddress, zcash_primitives::memo::MemoBytes)>, + Error< + SqliteClientError, + commitment_tree::Error, + GreedyInputSelectorError, + Zip317FeeError, + >, + > { + let proposal = propose_standard_transfer_to_address( + db_data, + &tests::network(), + fee_rule, + account, + NonZeroU32::new(1).unwrap(), + &to, + Amount::from_u64(15000).unwrap(), + None, + None, + )?; check_proposal_serialization_roundtrip(db_data, &proposal); // Executing the proposal should succeed - let txid = { - let res = create_proposed_transaction::<_, _, Infallible, _>( - db_data, - &tests::network(), - test_prover(), - &usk, - ovk_policy, - &proposal, - ); - assert_matches!(res, Ok(_)); - res.unwrap() - }; + let txid = create_proposed_transaction( + db_data, + &tests::network(), + test_prover(), + &usk, + ovk_policy, + &proposal, + )?; // Fetch the transaction from the database let raw_tx: Vec<_> = db_data @@ -1280,18 +1245,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 @@ -1318,7 +1284,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 d3f456e1ab..83781837ac 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