Merge pull request #1297 from nuttycom/sqlite_wallet/branching_chain_…

…test_fixes

Fix update_chain_tip_unstable_max_scanned tests.

zcash_client_backend/CHANGELOG.md

@@ -71,6 +71,8 @@ and this library adheres to Rust's notion of
     - `get_transaction` now returns `Result<Option<Transaction>, _>` rather
       than returning an `Err` if the `txid` parameter does not correspond to
       a transaction in the database.
+  - `WalletWrite::create_account` now takes its `AccountBirthday` argument by 
+    reference.
   - Changes to the `InputSource` trait:
     - `select_spendable_notes` now takes its `target_value` argument as a
       `NonNegativeAmount`. Also, it now returns a `SpendableNotes` data 

zcash_client_backend/src/data_api.rs

@@ -1297,11 +1297,7 @@ impl<AccountId> SentTransactionOutput<AccountId> {
 /// note commitment tree state is recorded at that height.
 #[derive(Clone, Debug)]
 pub struct AccountBirthday {
-    height: BlockHeight,
-    sapling_frontier: Frontier<sapling::Node, { sapling::NOTE_COMMITMENT_TREE_DEPTH }>,
-    #[cfg(feature = "orchard")]
-    orchard_frontier:
-        Frontier<orchard::tree::MerkleHashOrchard, { orchard::NOTE_COMMITMENT_TREE_DEPTH as u8 }>,
+    prior_chain_state: ChainState,
     recover_until: Option<BlockHeight>,
 }
 
@@ -1326,10 +1322,9 @@ impl From<io::Error> for BirthdayError {
 impl AccountBirthday {
     /// Constructs a new [`AccountBirthday`] from its constituent parts.
     ///
-    /// * `height`: The birthday height of the account. This is defined as the height of the first
-    ///    block to be scanned in wallet recovery.
-    /// * `sapling_frontier`: The Sapling note commitment tree frontier as of the end of the block
-    ///    prior to the birthday height.
+    /// * `prior_chain_state`: The chain state prior to the birthday height of the account. The
+    ///    birthday height  is defined as the height of the first block to be scanned in wallet
+    ///    recovery.
     /// * `recover_until`: An optional height at which the wallet should exit "recovery mode". In
     ///    order to avoid confusing shifts in wallet balance and spendability that may temporarily be
     ///    visible to a user during the process of recovering from seed, wallets may optionally set a
@@ -1340,20 +1335,9 @@ impl AccountBirthday {
     /// This API is intended primarily to be used in testing contexts; under normal circumstances,
     /// [`AccountBirthday::from_treestate`] should be used instead.
     #[cfg(feature = "test-dependencies")]
-    pub fn from_parts(
-        height: BlockHeight,
-        sapling_frontier: Frontier<sapling::Node, { sapling::NOTE_COMMITMENT_TREE_DEPTH }>,
-        #[cfg(feature = "orchard")] orchard_frontier: Frontier<
-            orchard::tree::MerkleHashOrchard,
-            { orchard::NOTE_COMMITMENT_TREE_DEPTH as u8 },
-        >,
-        recover_until: Option<BlockHeight>,
-    ) -> Self {
+    pub fn from_parts(prior_chain_state: ChainState, recover_until: Option<BlockHeight>) -> Self {
         Self {
-            height,
-            sapling_frontier,
-            #[cfg(feature = "orchard")]
-            orchard_frontier,
+            prior_chain_state,
             recover_until,
         }
     }
@@ -1373,10 +1357,7 @@ impl AccountBirthday {
         recover_until: Option<BlockHeight>,
     ) -> Result<Self, BirthdayError> {
         Ok(Self {
-            height: BlockHeight::try_from(treestate.height + 1)?,
-            sapling_frontier: treestate.sapling_tree()?.to_frontier(),
-            #[cfg(feature = "orchard")]
-            orchard_frontier: treestate.orchard_tree()?.to_frontier(),
+            prior_chain_state: treestate.to_chain_state()?,
             recover_until,
         })
     }
@@ -1386,7 +1367,7 @@ impl AccountBirthday {
     pub fn sapling_frontier(
         &self,
     ) -> &Frontier<sapling::Node, { sapling::NOTE_COMMITMENT_TREE_DEPTH }> {
-        &self.sapling_frontier
+        self.prior_chain_state.final_sapling_tree()
     }
 
     /// Returns the Orchard note commitment tree frontier as of the end of the block at
@@ -1396,20 +1377,20 @@ impl AccountBirthday {
         &self,
     ) -> &Frontier<orchard::tree::MerkleHashOrchard, { orchard::NOTE_COMMITMENT_TREE_DEPTH as u8 }>
     {
-        &self.orchard_frontier
+        self.prior_chain_state.final_orchard_tree()
     }
 
     /// Returns the birthday height of the account.
     pub fn height(&self) -> BlockHeight {
-        self.height
+        self.prior_chain_state.block_height() + 1
     }
 
     /// Returns the height at which the wallet should exit "recovery mode".
     pub fn recover_until(&self) -> Option<BlockHeight> {
         self.recover_until
     }
 
-    #[cfg(feature = "test-dependencies")]
+    #[cfg(any(test, feature = "test-dependencies"))]
     /// Constructs a new [`AccountBirthday`] at the given network upgrade's activation,
     /// with no "recover until" height.
     ///
@@ -1419,17 +1400,18 @@ impl AccountBirthday {
     pub fn from_activation<P: zcash_primitives::consensus::Parameters>(
         params: &P,
         network_upgrade: NetworkUpgrade,
+        prior_block_hash: BlockHash,
     ) -> AccountBirthday {
         AccountBirthday::from_parts(
-            params.activation_height(network_upgrade).unwrap(),
-            Frontier::empty(),
-            #[cfg(feature = "orchard")]
-            Frontier::empty(),
+            ChainState::empty(
+                params.activation_height(network_upgrade).unwrap() - 1,
+                prior_block_hash,
+            ),
             None,
         )
     }
 
-    #[cfg(feature = "test-dependencies")]
+    #[cfg(any(test, feature = "test-dependencies"))]
     /// Constructs a new [`AccountBirthday`] at Sapling activation, with no
     /// "recover until" height.
     ///
@@ -1438,8 +1420,9 @@ impl AccountBirthday {
     /// Panics if the Sapling activation height is not set.
     pub fn from_sapling_activation<P: zcash_primitives::consensus::Parameters>(
         params: &P,
+        prior_block_hash: BlockHash,
     ) -> AccountBirthday {
-        Self::from_activation(params, NetworkUpgrade::Sapling)
+        Self::from_activation(params, NetworkUpgrade::Sapling, prior_block_hash)
     }
 }
 
@@ -1482,7 +1465,7 @@ pub trait WalletWrite: WalletRead {
     fn create_account(
         &mut self,
         seed: &SecretVec<u8>,
-        birthday: AccountBirthday,
+        birthday: &AccountBirthday,
     ) -> Result<(Self::AccountId, UnifiedSpendingKey), Self::Error>;
 
     /// Generates and persists the next available diversified address, given the current
@@ -1887,7 +1870,7 @@ pub mod testing {
         fn create_account(
             &mut self,
             seed: &SecretVec<u8>,
-            _birthday: AccountBirthday,
+            _birthday: &AccountBirthday,
         ) -> Result<(Self::AccountId, UnifiedSpendingKey), Self::Error> {
             let account = zip32::AccountId::ZERO;
             UnifiedSpendingKey::from_seed(&self.network, seed.expose_secret(), account)

zcash_client_sqlite/src/lib.rs

@@ -535,7 +535,7 @@ impl<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P>
     fn create_account(
         &mut self,
         seed: &SecretVec<u8>,
-        birthday: AccountBirthday,
+        birthday: &AccountBirthday,
     ) -> Result<(AccountId, UnifiedSpendingKey), Self::Error> {
         self.transactionally(|wdb| {
             let seed_fingerprint =
@@ -1695,7 +1695,8 @@ extern crate assert_matches;
 #[cfg(test)]
 mod tests {
     use secrecy::SecretVec;
-    use zcash_client_backend::data_api::{AccountBirthday, WalletRead, WalletWrite};
+    use zcash_client_backend::data_api::{WalletRead, WalletWrite};
+    use zcash_primitives::block::BlockHash;
 
     use crate::{testing::TestBuilder, AccountId, DEFAULT_UA_REQUEST};
 
@@ -1708,52 +1709,57 @@ mod tests {
     #[test]
     fn validate_seed() {
         let st = TestBuilder::new()
-            .with_test_account(AccountBirthday::from_sapling_activation)
+            .with_account_from_sapling_activation(BlockHash([0; 32]))
             .build();
+        let account = st.test_account().unwrap();
 
         assert!({
-            let account = st.test_account().unwrap().0;
             st.wallet()
-                .validate_seed(account, st.test_seed().unwrap())
+                .validate_seed(account.account_id(), st.test_seed().unwrap())
                 .unwrap()
         });
 
         // check that passing an invalid account results in a failure
         assert!({
-            let account = AccountId(3);
+            let wrong_account_index = AccountId(3);
             !st.wallet()
-                .validate_seed(account, st.test_seed().unwrap())
+                .validate_seed(wrong_account_index, st.test_seed().unwrap())
                 .unwrap()
         });
 
         // check that passing an invalid seed results in a failure
         assert!({
-            let account = st.test_account().unwrap().0;
             !st.wallet()
-                .validate_seed(account, &SecretVec::new(vec![1u8; 32]))
+                .validate_seed(account.account_id(), &SecretVec::new(vec![1u8; 32]))
                 .unwrap()
         });
     }
 
     #[test]
     pub(crate) fn get_next_available_address() {
         let mut st = TestBuilder::new()
-            .with_test_account(AccountBirthday::from_sapling_activation)
+            .with_account_from_sapling_activation(BlockHash([0; 32]))
             .build();
-        let account = st.test_account().unwrap();
+        let account = st.test_account().cloned().unwrap();
 
-        let current_addr = st.wallet().get_current_address(account.0).unwrap();
+        let current_addr = st
+            .wallet()
+            .get_current_address(account.account_id())
+            .unwrap();
         assert!(current_addr.is_some());
 
         // TODO: Add Orchard
         let addr2 = st
             .wallet_mut()
-            .get_next_available_address(account.0, DEFAULT_UA_REQUEST)
+            .get_next_available_address(account.account_id(), DEFAULT_UA_REQUEST)
             .unwrap();
         assert!(addr2.is_some());
         assert_ne!(current_addr, addr2);
 
-        let addr2_cur = st.wallet().get_current_address(account.0).unwrap();
+        let addr2_cur = st
+            .wallet()
+            .get_current_address(account.account_id())
+            .unwrap();
         assert_eq!(addr2, addr2_cur);
     }
 
@@ -1763,15 +1769,16 @@ mod tests {
         // Add an account to the wallet.
         let st = TestBuilder::new()
             .with_block_cache()
-            .with_test_account(AccountBirthday::from_sapling_activation)
+            .with_account_from_sapling_activation(BlockHash([0; 32]))
             .build();
         let account = st.test_account().unwrap();
+        let ufvk = account.usk().to_unified_full_viewing_key();
+        let (taddr, _) = account.usk().default_transparent_address();
 
-        let (_, usk, _) = st.test_account().unwrap();
-        let ufvk = usk.to_unified_full_viewing_key();
-        let (taddr, _) = usk.default_transparent_address();
-
-        let receivers = st.wallet().get_transparent_receivers(account.0).unwrap();
+        let receivers = st
+            .wallet()
+            .get_transparent_receivers(account.account_id())
+            .unwrap();
 
         // The receiver for the default UA should be in the set.
         assert!(receivers.contains_key(
@@ -1799,8 +1806,8 @@ mod tests {
 
         // Generate some fake CompactBlocks.
         let seed = [0u8; 32];
-        let account = zip32::AccountId::ZERO;
-        let extsk = sapling::spending_key(&seed, st.wallet().params.coin_type(), account);
+        let hd_account_index = zip32::AccountId::ZERO;
+        let extsk = sapling::spending_key(&seed, st.wallet().params.coin_type(), hd_account_index);
         let dfvk = extsk.to_diversifiable_full_viewing_key();
         let (h1, meta1, _) = st.generate_next_block(
             &dfvk,