From dd60f51d3cc4c03430fe1d7c96462df0251b3856 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 5 Sep 2023 09:22:38 -0600 Subject: [PATCH] Address unresovled code review comments from #907 --- zcash_client_backend/CHANGELOG.md | 5 ++-- zcash_client_sqlite/src/error.rs | 4 +-- zcash_client_sqlite/src/wallet.rs | 13 +++------ zcash_client_sqlite/src/wallet/scanning.rs | 34 +++++++++++----------- 4 files changed, 26 insertions(+), 30 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 80042cead..683f23815 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -14,8 +14,9 @@ and this library adheres to Rust's notion of birthday. The account birthday is defined as the minimum height among blocks to be scanned when recovering an account. - Account creation now requires the caller to provide account birthday information, - including the state of the note commitment tree at the end of the block prior to - the birthday height. + including the state of the note commitment tree at the end of the block prior + to the birthday height. A wallet's birthday is the earliest birthday height + among accounts maintained by the wallet. ### Added - `impl Eq for zcash_client_backend::address::RecipientAddress` diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index 54d00639b..b513fc3d1 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/zcash_client_sqlite/src/error.rs @@ -135,9 +135,9 @@ impl fmt::Display for SqliteClientError { SqliteClientError::BlockConflict(h) => write!(f, "A block hash conflict occurred at height {}; rewind required.", u32::from(*h)), SqliteClientError::NonSequentialBlocks => write!(f, "`put_blocks` requires that the provided block range be sequential"), SqliteClientError::DiversifierIndexOutOfRange => write!(f, "The space of available diversifier indices is exhausted"), - SqliteClientError::AccountUnknown(id) => write!(f, "Account {} does not belong to this wallet.", u32::from(*id)), + SqliteClientError::AccountUnknown(acct_id) => write!(f, "Account {} does not belong to this wallet.", u32::from(*acct_id)), - SqliteClientError::KeyDerivationError(acct_id) => write!(f, "Key derivation failed for account {:?}", acct_id), + SqliteClientError::KeyDerivationError(acct_id) => write!(f, "Key derivation failed for account {}", u32::from(*acct_id)), SqliteClientError::AccountIdDiscontinuity => write!(f, "Wallet account identifiers must be sequential."), SqliteClientError::AccountIdOutOfRange => write!(f, "Wallet account identifiers must be less than 0x7FFFFFFF."), #[cfg(feature = "transparent-inputs")] diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index e8b1c0e29..678ad6f47 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -165,9 +165,6 @@ pub(crate) fn add_account( key: &UnifiedFullViewingKey, birthday: AccountBirthday, ) -> Result<(), SqliteClientError> { - // Set the wallet birthday, falling back to the chain tip if not specified - let chain_tip = scan_queue_extrema(conn)?.map(|(_, max)| max); - conn.execute( "INSERT INTO accounts (account, ufvk, birthday_height, recover_until_height) VALUES (:account, :ufvk, :birthday_height, :recover_until_height)", @@ -227,9 +224,9 @@ pub(crate) fn add_account( )?; }; - // Rewrite the scan ranges starting from the birthday height so that we'll ensure we + // Rewrite the scan ranges from the birthday height up to the chain tip so that we'll ensure we // re-scan to find any notes that might belong to the newly added account. - if let Some(t) = chain_tip { + if let Some(t) = scan_queue_extrema(conn)?.map(|(_, max)| max) { let rescan_range = birthday.height()..(t + 1); replace_queue_entries::( @@ -240,7 +237,7 @@ pub(crate) fn add_account( ScanPriority::Historic, )) .into_iter(), - true, + true, // force rescan )?; } @@ -1687,7 +1684,6 @@ mod tests { #[cfg(feature = "transparent-inputs")] use { - incrementalmerkletree::frontier::Frontier, secrecy::Secret, zcash_client_backend::{ data_api::WalletWrite, encoding::AddressCodec, wallet::WalletTransparentOutput, @@ -1738,8 +1734,7 @@ mod tests { // Add an account to the wallet let seed = Secret::new([0u8; 32].to_vec()); - let birthday = - AccountBirthday::from_parts(st.sapling_activation_height(), Frontier::empty(), None); + let birthday = AccountBirthday::from_sapling_activation(&st.network()); let (account_id, _usk) = st.wallet_mut().create_account(&seed, birthday).unwrap(); let uaddr = st .wallet() diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index 7d1d9ed3c..18efa866a 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -543,23 +543,23 @@ pub(crate) fn replace_queue_entries( let mut suggested_stmt = conn .prepare_cached( "SELECT block_range_start, block_range_end, priority - FROM scan_queue - WHERE ( - -- the start is contained within or adjacent to the range - :start >= block_range_start - AND :start <= block_range_end - ) - OR ( - -- the end is contained within or adjacent to the range - :end >= block_range_start - AND :end <= block_range_end - ) - OR ( - -- start..end contains the entire range - block_range_start >= :start - AND block_range_end <= :end - ) - ORDER BY block_range_end", + FROM scan_queue + WHERE ( + -- the start is contained within or adjacent to the range + :start >= block_range_start + AND :start <= block_range_end + ) + OR ( + -- the end is contained within or adjacent to the range + :end >= block_range_start + AND :end <= block_range_end + ) + OR ( + -- start..end contains the entire range + block_range_start >= :start + AND block_range_end <= :end + ) + ORDER BY block_range_end", ) .map_err(E::db_error)?;