Address unresovled code review comments from zcash#907

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`

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")]

zcash_client_sqlite/src/wallet.rs

@@ -165,9 +165,6 @@ pub(crate) fn add_account<P: consensus::Parameters>(
     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<P: consensus::Parameters>(
         )?;
     };
 
-    // 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::<SqliteClientError>(
@@ -240,7 +237,7 @@ pub(crate) fn add_account<P: consensus::Parameters>(
                 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()

zcash_client_sqlite/src/wallet/scanning.rs

@@ -543,23 +543,23 @@ pub(crate) fn replace_queue_entries<E: WalletError>(
         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)?;