Merge pull request #1334 from zcash/fix/ensure_orchard_in_default_ua

zcash_client_sqlite: Add a migration to ensure that default addresses…

zcash_client_sqlite/src/wallet/init.rs

@@ -258,13 +258,14 @@ pub fn init_wallet_db<P: consensus::Parameters + 'static>(
     wdb: &mut WalletDb<rusqlite::Connection, P>,
     seed: Option<SecretVec<u8>>,
 ) -> Result<(), MigratorError<WalletMigrationError>> {
-    init_wallet_db_internal(wdb, seed, &[])
+    init_wallet_db_internal(wdb, seed, &[], true)
 }
 
 fn init_wallet_db_internal<P: consensus::Parameters + 'static>(
     wdb: &mut WalletDb<rusqlite::Connection, P>,
     seed: Option<SecretVec<u8>>,
     target_migrations: &[Uuid],
+    verify_seed_relevance: bool,
 ) -> Result<(), MigratorError<WalletMigrationError>> {
     let seed = seed.map(Rc::new);
 
@@ -295,18 +296,24 @@ fn init_wallet_db_internal<P: consensus::Parameters + 'static>(
         .map_err(|e| MigratorError::Adapter(WalletMigrationError::from(e)))?;
 
     // Now that the migration succeeded, check whether the seed is relevant to the wallet.
-    if let Some(seed) = seed {
-        match wdb
-            .seed_relevance_to_derived_accounts(&seed)
-            .map_err(sqlite_client_error_to_wallet_migration_error)?
-        {
-            SeedRelevance::Relevant { .. } => (),
-            // Every seed is relevant to a wallet with no accounts; this is most likely a
-            // new wallet database being initialized for the first time.
-            SeedRelevance::NoAccounts => (),
-            // No seed is relevant to a wallet that only has imported accounts.
-            SeedRelevance::NotRelevant | SeedRelevance::NoDerivedAccounts => {
-                return Err(WalletMigrationError::SeedNotRelevant.into())
+    // We can only check this if we have migrated as far as `full_account_ids::MIGRATION_ID`,
+    // but unfortunately `schemer` does not currently expose its DAG of migrations. As a
+    // consequence, the caller has to choose whether or not this check should be performed
+    // based upon which migrations they're asking to apply.
+    if verify_seed_relevance {
+        if let Some(seed) = seed {
+            match wdb
+                .seed_relevance_to_derived_accounts(&seed)
+                .map_err(sqlite_client_error_to_wallet_migration_error)?
+            {
+                SeedRelevance::Relevant { .. } => (),
+                // Every seed is relevant to a wallet with no accounts; this is most likely a
+                // new wallet database being initialized for the first time.
+                SeedRelevance::NoAccounts => (),
+                // No seed is relevant to a wallet that only has imported accounts.
+                SeedRelevance::NotRelevant | SeedRelevance::NoDerivedAccounts => {
+                    return Err(WalletMigrationError::SeedNotRelevant.into())
+                }
             }
         }
     }

zcash_client_sqlite/src/wallet/init/migrations.rs

@@ -2,6 +2,7 @@ mod add_account_birthdays;
 mod add_transaction_views;
 mod add_utxo_account;
 mod addresses_table;
+mod ensure_orchard_ua_receiver;
 mod full_account_ids;
 mod initial_setup;
 mod nullifier_map;
@@ -62,6 +63,8 @@ pub(super) fn all_migrations<P: consensus::Parameters + 'static>(
     //                           -------------------- full_account_ids
     //                                                       |
     //                                             orchard_received_notes
+    //                                                       |
+    //                                           ensure_orchard_ua_receiver
     vec![
         Box::new(initial_setup::Migration {}),
         Box::new(utxos_table::Migration {}),
@@ -108,5 +111,8 @@ pub(super) fn all_migrations<P: consensus::Parameters + 'static>(
             params: params.clone(),
         }),
         Box::new(orchard_received_notes::Migration),
+        Box::new(ensure_orchard_ua_receiver::Migration {
+            params: params.clone(),
+        }),
     ]
 }

zcash_client_sqlite/src/wallet/init/migrations/add_transaction_views.rs

@@ -305,7 +305,8 @@ mod tests {
         let network = Network::TestNetwork;
         let data_file = NamedTempFile::new().unwrap();
         let mut db_data = WalletDb::for_path(data_file.path(), network).unwrap();
-        init_wallet_db_internal(&mut db_data, None, &[addresses_table::MIGRATION_ID]).unwrap();
+        init_wallet_db_internal(&mut db_data, None, &[addresses_table::MIGRATION_ID], false)
+            .unwrap();
         let usk = UnifiedSpendingKey::from_seed(&network, &[0u8; 32][..], AccountId::ZERO).unwrap();
         let ufvk = usk.to_unified_full_viewing_key();
 
@@ -336,7 +337,7 @@ mod tests {
             VALUES (0, 4, 0, '', 7, '', 'c', true, X'63');",
         ).unwrap();
 
-        init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID]).unwrap();
+        init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID], false).unwrap();
 
         let mut q = db_data
             .conn
@@ -407,6 +408,7 @@ mod tests {
             &mut db_data,
             None,
             &[utxos_table::MIGRATION_ID, ufvk_support::MIGRATION_ID],
+            false,
         )
         .unwrap();
 
@@ -479,7 +481,7 @@ mod tests {
             )
             .unwrap();
 
-        init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID]).unwrap();
+        init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID], false).unwrap();
 
         let fee = db_data
             .conn

zcash_client_sqlite/src/wallet/init/migrations/ensure_orchard_ua_receiver.rs

@@ -0,0 +1,200 @@
+//! This migration ensures that an Orchard receiver exists in the wallet's default Unified address.
+use std::collections::HashSet;
+
+use rusqlite::named_params;
+use schemer_rusqlite::RusqliteMigration;
+use uuid::Uuid;
+
+use zcash_client_backend::keys::{
+    UnifiedAddressRequest, UnifiedFullViewingKey, UnifiedIncomingViewingKey,
+};
+use zcash_primitives::consensus;
+
+use super::orchard_received_notes;
+use crate::{wallet::init::WalletMigrationError, UA_ORCHARD, UA_TRANSPARENT};
+
+pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0x604349c7_5ce5_4768_bea6_12d106ccda93);
+
+pub(super) struct Migration<P> {
+    pub(super) params: P,
+}
+
+impl<P> schemer::Migration for Migration<P> {
+    fn id(&self) -> Uuid {
+        MIGRATION_ID
+    }
+
+    fn dependencies(&self) -> HashSet<Uuid> {
+        [orchard_received_notes::MIGRATION_ID].into_iter().collect()
+    }
+
+    fn description(&self) -> &'static str {
+        "Ensures that the wallet's default address contains an Orchard receiver."
+    }
+}
+
+impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
+    type Error = WalletMigrationError;
+
+    fn up(&self, transaction: &rusqlite::Transaction<'_>) -> Result<(), Self::Error> {
+        let mut get_accounts = transaction.prepare(
+            r#"
+            SELECT id, ufvk, uivk
+            FROM accounts
+            "#,
+        )?;
+
+        let mut update_address = transaction.prepare(
+            r#"UPDATE "addresses"
+               SET address = :address
+               WHERE account_id = :account_id
+               AND diversifier_index_be = :j
+            "#,
+        )?;
+
+        let mut accounts = get_accounts.query([])?;
+        while let Some(row) = accounts.next()? {
+            let account_id = row.get::<_, u32>("id")?;
+            let ufvk_str: Option<String> = row.get("ufvk")?;
+            let uivk = if let Some(ufvk_str) = ufvk_str {
+                UnifiedFullViewingKey::decode(&self.params, &ufvk_str[..])
+                    .map_err(|_| {
+                        WalletMigrationError::CorruptedData("Unable to decode UFVK".to_string())
+                    })?
+                    .to_unified_incoming_viewing_key()
+            } else {
+                let uivk_str: String = row.get("uivk")?;
+                UnifiedIncomingViewingKey::decode(&self.params, &uivk_str[..]).map_err(|_| {
+                    WalletMigrationError::CorruptedData("Unable to decode UIVK".to_string())
+                })?
+            };
+
+            let (default_addr, diversifier_index) = uivk.default_address(
+                UnifiedAddressRequest::unsafe_new(UA_ORCHARD, true, UA_TRANSPARENT),
+            )?;
+
+            let mut di_be = *diversifier_index.as_bytes();
+            di_be.reverse();
+            update_address.execute(named_params![
+                ":address": default_addr.encode(&self.params),
+                ":account_id": account_id,
+                ":j": &di_be[..],
+            ])?;
+        }
+
+        Ok(())
+    }
+
+    fn down(&self, _transaction: &rusqlite::Transaction<'_>) -> Result<(), Self::Error> {
+        Err(WalletMigrationError::CannotRevert(MIGRATION_ID))
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use rusqlite::named_params;
+    use secrecy::SecretVec;
+    use tempfile::NamedTempFile;
+
+    use zcash_client_backend::keys::{UnifiedAddressRequest, UnifiedSpendingKey};
+    use zcash_keys::address::Address;
+    use zcash_primitives::consensus::Network;
+
+    use crate::{
+        wallet::init::{init_wallet_db, init_wallet_db_internal, migrations::addresses_table},
+        WalletDb, UA_ORCHARD, UA_TRANSPARENT,
+    };
+
+    #[test]
+    fn init_migrate_add_orchard_receiver() {
+        let data_file = NamedTempFile::new().unwrap();
+        let mut db_data = WalletDb::for_path(data_file.path(), Network::TestNetwork).unwrap();
+
+        let seed = vec![0x10; 32];
+        let account_id = 0u32;
+        let ufvk = UnifiedSpendingKey::from_seed(
+            &db_data.params,
+            &seed,
+            zip32::AccountId::try_from(account_id).unwrap(),
+        )
+        .unwrap()
+        .to_unified_full_viewing_key();
+
+        assert_matches!(
+            init_wallet_db_internal(
+                &mut db_data,
+                Some(SecretVec::new(seed.clone())),
+                &[addresses_table::MIGRATION_ID],
+                false
+            ),
+            Ok(_)
+        );
+
+        // Manually create an entry in the addresses table for an address that lacks an Orchard
+        // receiver.
+        db_data
+            .conn
+            .execute(
+                "INSERT INTO accounts (account, ufvk) VALUES (:account_id, :ufvk)",
+                named_params![
+                    ":account_id": account_id,
+                    ":ufvk": ufvk.encode(&db_data.params)
+                ],
+            )
+            .unwrap();
+
+        let (addr, diversifier_index) = ufvk
+            .default_address(UnifiedAddressRequest::unsafe_new(
+                false,
+                true,
+                UA_TRANSPARENT,
+            ))
+            .unwrap();
+        let mut di_be = *diversifier_index.as_bytes();
+        di_be.reverse();
+
+        db_data
+            .conn
+            .execute(
+                "INSERT INTO addresses (account, diversifier_index_be, address) 
+                 VALUES (:account_id, :j, :address) ",
+                named_params![
+                    ":account_id": account_id,
+                    ":j": &di_be[..],
+                    ":address": addr.encode(&db_data.params)
+                ],
+            )
+            .unwrap();
+
+        match db_data
+            .conn
+            .query_row("SELECT address FROM addresses", [], |row| {
+                Ok(Address::decode(&db_data.params, &row.get::<_, String>(0)?).unwrap())
+            }) {
+            Ok(Address::Unified(ua)) => {
+                assert!(ua.orchard().is_none());
+                assert!(ua.sapling().is_some());
+                assert_eq!(ua.transparent().is_some(), UA_TRANSPARENT);
+            }
+            other => panic!("Unexpected result from address decoding: {:?}", other),
+        }
+
+        assert_matches!(
+            init_wallet_db(&mut db_data, Some(SecretVec::new(seed))),
+            Ok(_)
+        );
+
+        match db_data
+            .conn
+            .query_row("SELECT address FROM addresses", [], |row| {
+                Ok(Address::decode(&db_data.params, &row.get::<_, String>(0)?).unwrap())
+            }) {
+            Ok(Address::Unified(ua)) => {
+                assert_eq!(ua.orchard().is_some(), UA_ORCHARD);
+                assert!(ua.sapling().is_some());
+                assert_eq!(ua.transparent().is_some(), UA_TRANSPARENT);
+            }
+            other => panic!("Unexpected result from address decoding: {:?}", other),
+        }
+    }
+}

zcash_client_sqlite/src/wallet/init/migrations/received_notes_nullable_nf.rs

@@ -238,7 +238,13 @@ mod tests {
     fn received_notes_nullable_migration() {
         let data_file = NamedTempFile::new().unwrap();
         let mut db_data = WalletDb::for_path(data_file.path(), Network::TestNetwork).unwrap();
-        init_wallet_db_internal(&mut db_data, None, &[v_transactions_net::MIGRATION_ID]).unwrap();
+        init_wallet_db_internal(
+            &mut db_data,
+            None,
+            &[v_transactions_net::MIGRATION_ID],
+            false,
+        )
+        .unwrap();
 
         // Create an account in the wallet
         let usk0 = UnifiedSpendingKey::from_seed(&db_data.params, &[0u8; 32][..], AccountId::ZERO)
@@ -263,7 +269,7 @@ mod tests {
             VALUES (0, 3, 0, '', 5, '', 'nf_b', false);").unwrap();
 
         // Apply the current migration
-        init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID]).unwrap();
+        init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID], false).unwrap();
 
         {
             let mut q = db_data

zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs

@@ -483,6 +483,7 @@ mod tests {
                 add_account_birthdays::MIGRATION_ID,
                 shardtree_support::MIGRATION_ID,
             ],
+            false,
         )
         .unwrap();
 
@@ -537,7 +538,7 @@ mod tests {
             .unwrap();
 
         // Apply the current migration
-        init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID]).unwrap();
+        init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID], false).unwrap();
 
         // There should be two rows in the `sapling_received_notes` table with correct scopes.
         let mut q = db_data
@@ -581,6 +582,7 @@ mod tests {
                 wallet_summaries::MIGRATION_ID,
                 shardtree_support::MIGRATION_ID,
             ],
+            false,
         )
         .unwrap();
 
@@ -714,7 +716,7 @@ mod tests {
             .unwrap();
 
         // Apply the current migration
-        init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID]).unwrap();
+        init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID], false).unwrap();
 
         // There should be two rows in the `sapling_received_notes` table with correct scopes.
         let mut q = db_data

zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs

@@ -217,8 +217,13 @@ mod tests {
     fn v_transactions_net() {
         let data_file = NamedTempFile::new().unwrap();
         let mut db_data = WalletDb::for_path(data_file.path(), Network::TestNetwork).unwrap();
-        init_wallet_db_internal(&mut db_data, None, &[add_transaction_views::MIGRATION_ID])
-            .unwrap();
+        init_wallet_db_internal(
+            &mut db_data,
+            None,
+            &[add_transaction_views::MIGRATION_ID],
+            false,
+        )
+        .unwrap();
 
         // Create two accounts in the wallet.
         let usk0 = UnifiedSpendingKey::from_seed(&db_data.params, &[0u8; 32][..], AccountId::ZERO)
@@ -383,7 +388,7 @@ mod tests {
         }
 
         // Run this migration
-        init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID]).unwrap();
+        init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID], false).unwrap();
 
         // Corrected behavior after v_transactions has been updated
         {

zcash_client_sqlite/src/wallet/init/migrations/v_transactions_note_uniqueness.rs

@@ -175,7 +175,13 @@ mod tests {
     fn v_transactions_note_uniqueness_migration() {
         let data_file = NamedTempFile::new().unwrap();
         let mut db_data = WalletDb::for_path(data_file.path(), Network::TestNetwork).unwrap();
-        init_wallet_db_internal(&mut db_data, None, &[v_transactions_net::MIGRATION_ID]).unwrap();
+        init_wallet_db_internal(
+            &mut db_data,
+            None,
+            &[v_transactions_net::MIGRATION_ID],
+            false,
+        )
+        .unwrap();
 
         // Create an account in the wallet
         let usk0 = UnifiedSpendingKey::from_seed(&db_data.params, &[0u8; 32][..], AccountId::ZERO)
@@ -241,7 +247,7 @@ mod tests {
         check_balance_delta(&mut db_data, 1);
 
         // Apply the current migration.
-        init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID]).unwrap();
+        init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID], false).unwrap();
 
         // Now it should be correct.
         check_balance_delta(&mut db_data, 2);