Merge pull request #1433 from nuttycom/fix_migration_pragmas

zcash_client_sqlite: Fix handling of PRAGMA directives.

zcash_client_sqlite/src/wallet/init.rs

@@ -270,13 +270,13 @@ fn init_wallet_db_internal<P: consensus::Parameters + 'static>(
 ) -> Result<(), MigratorError<WalletMigrationError>> {
     let seed = seed.map(Rc::new);
 
-    // Turn off foreign keys, and ensure that table replacement/modification
-    // does not break views
+    // Turn off foreign key enforcement, to ensure that table replacement does not break foreign
+    // key references in table definitions.
+    //
+    // It is necessary to perform this operation globally using the outer connection because this
+    // pragma has no effect when set or unset within a transaction.
     wdb.conn
-        .execute_batch(
-            "PRAGMA foreign_keys = OFF;
-             PRAGMA legacy_alter_table = TRUE;",
-        )
+        .execute_batch("PRAGMA foreign_keys = OFF;")
         .map_err(|e| MigratorError::Adapter(WalletMigrationError::from(e)))?;
     let adapter = RusqliteAdapter::new(&mut wdb.conn, Some("schemer_migrations".to_string()));
     adapter.init().expect("Migrations table setup succeeds.");
@@ -325,7 +325,7 @@ fn init_wallet_db_internal<P: consensus::Parameters + 'static>(
 #[cfg(test)]
 #[allow(deprecated)]
 mod tests {
-    use rusqlite::{self, named_params, ToSql};
+    use rusqlite::{self, named_params, Connection, ToSql};
     use secrecy::Secret;
 
     use tempfile::NamedTempFile;
@@ -356,6 +356,15 @@ mod tests {
         zcash_primitives::zip32::DiversifierIndex,
     };
 
+    pub(crate) fn describe_tables(conn: &Connection) -> Result<Vec<String>, rusqlite::Error> {
+        let result = conn
+            .prepare("SELECT sql FROM sqlite_schema WHERE type = 'table' ORDER BY tbl_name")?
+            .query_and_then([], |row| row.get::<_, String>(0))?
+            .collect::<Result<Vec<_>, _>>()?;
+
+        Ok(result)
+    }
+
     #[test]
     fn verify_schema() {
         let st = TestBuilder::new().build();
@@ -390,20 +399,13 @@ mod tests {
             db::TABLE_UTXOS,
         ];
 
-        let mut tables_query = st
-            .wallet()
-            .conn
-            .prepare("SELECT sql FROM sqlite_schema WHERE type = 'table' ORDER BY tbl_name")
-            .unwrap();
-        let mut rows = tables_query.query([]).unwrap();
-        let mut expected_idx = 0;
-        while let Some(row) = rows.next().unwrap() {
-            let sql: String = row.get(0).unwrap();
+        let rows = describe_tables(&st.wallet().conn).unwrap();
+        assert_eq!(rows.len(), expected_tables.len());
+        for (actual, expected) in rows.iter().zip(expected_tables.iter()) {
             assert_eq!(
-                re.replace_all(&sql, " "),
-                re.replace_all(expected_tables[expected_idx], " ").trim(),
+                re.replace_all(actual, " "),
+                re.replace_all(expected, " ").trim(),
             );
-            expected_idx += 1;
         }
 
         let expected_indices = vec![

zcash_client_sqlite/src/wallet/init/migrations.rs

@@ -116,3 +116,31 @@ pub(super) fn all_migrations<P: consensus::Parameters + 'static>(
         }),
     ]
 }
+
+#[cfg(test)]
+mod tests {
+    use secrecy::Secret;
+    use tempfile::NamedTempFile;
+    use uuid::Uuid;
+    use zcash_protocol::consensus::Network;
+
+    use crate::{wallet::init::init_wallet_db_internal, WalletDb};
+
+    /// Tests that we can migrate from a completely empty wallet database to the target
+    /// migrations.
+    pub(crate) fn test_migrate(migrations: &[Uuid]) {
+        let data_file = NamedTempFile::new().unwrap();
+        let mut db_data = WalletDb::for_path(data_file.path(), Network::TestNetwork).unwrap();
+
+        let seed = [0xab; 32];
+        assert_matches!(
+            init_wallet_db_internal(
+                &mut db_data,
+                Some(Secret::new(seed.to_vec())),
+                migrations,
+                false
+            ),
+            Ok(_)
+        );
+    }
+}

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

@@ -12,6 +12,8 @@ use super::shardtree_support;
 
 pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0xeeec0d0d_fee0_4231_8c68_5f3a7c7c2245);
 
+const DEPENDENCIES: [Uuid; 1] = [shardtree_support::MIGRATION_ID];
+
 pub(super) struct Migration<P> {
     pub(super) params: P,
 }
@@ -22,7 +24,7 @@ impl<P> schemer::Migration for Migration<P> {
     }
 
     fn dependencies(&self) -> HashSet<Uuid> {
-        [shardtree_support::MIGRATION_ID].into_iter().collect()
+        DEPENDENCIES.into_iter().collect()
     }
 
     fn description(&self) -> &'static str {
@@ -52,12 +54,10 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
             INSERT INTO accounts_new (account, ufvk, birthday_height)
             SELECT account, ufvk, birthday_height FROM accounts;
 
-            PRAGMA foreign_keys=OFF;
             PRAGMA legacy_alter_table = ON;
             DROP TABLE accounts;
             ALTER TABLE accounts_new RENAME TO accounts;
-            PRAGMA legacy_alter_table = OFF;
-            PRAGMA foreign_keys=ON;",
+            PRAGMA legacy_alter_table = OFF;",
             u32::from(
                 self.params
                     .activation_height(NetworkUpgrade::Sapling)
@@ -72,3 +72,48 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
         Err(WalletMigrationError::CannotRevert(MIGRATION_ID))
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use secrecy::Secret;
+    use tempfile::NamedTempFile;
+    use zcash_protocol::consensus::Network;
+
+    use super::{DEPENDENCIES, MIGRATION_ID};
+    use crate::{wallet::init::init_wallet_db_internal, WalletDb};
+
+    #[test]
+    fn migrate() {
+        let data_file = NamedTempFile::new().unwrap();
+        let mut db_data = WalletDb::for_path(data_file.path(), Network::TestNetwork).unwrap();
+
+        let seed_bytes = vec![0xab; 32];
+        init_wallet_db_internal(
+            &mut db_data,
+            Some(Secret::new(seed_bytes.clone())),
+            &DEPENDENCIES,
+            false,
+        )
+        .unwrap();
+
+        db_data
+            .conn
+            .execute_batch(r#"INSERT INTO accounts (account, ufvk) VALUES (0, 'not_a_real_ufvk');"#)
+            .unwrap();
+        db_data
+            .conn
+            .execute_batch(
+                "INSERT INTO addresses (account, diversifier_index_be, address) 
+                VALUES (0, X'', 'not_a_real_address');",
+            )
+            .unwrap();
+
+        init_wallet_db_internal(
+            &mut db_data,
+            Some(Secret::new(seed_bytes)),
+            &[MIGRATION_ID],
+            false,
+        )
+        .unwrap();
+    }
+}

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

@@ -230,3 +230,13 @@ fn get_legacy_transparent_address<P: consensus::Parameters>(
         Ok(None)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::wallet::init::migrations::tests::test_migrate;
+
+    #[test]
+    fn migrate() {
+        test_migrate(&[super::MIGRATION_ID]);
+    }
+}

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

@@ -213,3 +213,13 @@ fn insert_address<P: consensus::Parameters>(
 
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::wallet::init::migrations::tests::test_migrate;
+
+    #[test]
+    fn migrate() {
+        test_migrate(&[super::MIGRATION_ID]);
+    }
+}

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

@@ -37,19 +37,13 @@ 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 get_accounts = transaction.prepare("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
-            "#,
+               AND diversifier_index_be = :j"#,
         )?;
 
         let mut accounts = get_accounts.query([])?;