-
Notifications
You must be signed in to change notification settings - Fork 249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
zcash_client_sqlite: Fix migration DAG edges #1506
Changes from 3 commits
7c416d5
6903fa6
34402f7
e4b3fb2
b1cb8be
3dba2fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -31,6 +31,7 @@ | |||||||||||
|
||||||||||||
use schemer_rusqlite::RusqliteMigration; | ||||||||||||
use secrecy::SecretVec; | ||||||||||||
use uuid::Uuid; | ||||||||||||
use zcash_protocol::consensus; | ||||||||||||
|
||||||||||||
use super::WalletMigrationError; | ||||||||||||
|
@@ -69,8 +70,10 @@ | |||||||||||
// orchard_received_notes spend_key_available | ||||||||||||
// / \ | ||||||||||||
// ensure_orchard_ua_receiver utxos_to_txos | ||||||||||||
// / \ | ||||||||||||
// ephemeral_addresses tx_retrieval_queue | ||||||||||||
// | | ||||||||||||
// ephemeral_addresses | ||||||||||||
// | | ||||||||||||
// tx_retrieval_queue | ||||||||||||
vec![ | ||||||||||||
Box::new(initial_setup::Migration {}), | ||||||||||||
Box::new(utxos_table::Migration {}), | ||||||||||||
|
@@ -131,8 +134,75 @@ | |||||||||||
] | ||||||||||||
} | ||||||||||||
|
||||||||||||
/// All states of the migration DAG that have been exposed in a public crate release, in | ||||||||||||
/// the order that crate users would have encountered them. | ||||||||||||
/// | ||||||||||||
/// Omitted versions had the same migration state as the first prior version that is | ||||||||||||
/// included. | ||||||||||||
#[allow(dead_code)] | ||||||||||||
const PUBLIC_MIGRATION_STATES: &[&[Uuid]] = &[ | ||||||||||||
V_0_4_0, V_0_6_0, V_0_8_0, V_0_9_0, V_0_10_0, V_0_10_3, V_0_11_0, V_0_11_1, | ||||||||||||
]; | ||||||||||||
|
||||||||||||
/// Leaf migrations in the 0.4.0 release. | ||||||||||||
const V_0_4_0: &[Uuid] = &[add_transaction_views::MIGRATION_ID]; | ||||||||||||
|
||||||||||||
/// Leaf migrations in the 0.6.0 release. | ||||||||||||
const V_0_6_0: &[Uuid] = &[v_transactions_net::MIGRATION_ID]; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I verified that as of v0.6.0 the ASCII art was correct, and |
||||||||||||
|
||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However, at v0.7.0 a
Suggested change
Non-blocking. |
||||||||||||
/// Leaf migrations in the 0.8.0 release. | ||||||||||||
const V_0_8_0: &[Uuid] = &[ | ||||||||||||
nullifier_map::MIGRATION_ID, | ||||||||||||
v_transactions_note_uniqueness::MIGRATION_ID, | ||||||||||||
wallet_summaries::MIGRATION_ID, | ||||||||||||
]; | ||||||||||||
|
||||||||||||
/// Leaf migrations in the 0.9.0 release. | ||||||||||||
const V_0_9_0: &[Uuid] = &[ | ||||||||||||
nullifier_map::MIGRATION_ID, | ||||||||||||
receiving_key_scopes::MIGRATION_ID, | ||||||||||||
v_transactions_note_uniqueness::MIGRATION_ID, | ||||||||||||
wallet_summaries::MIGRATION_ID, | ||||||||||||
]; | ||||||||||||
|
||||||||||||
/// Leaf migrations in the 0.10.0 release. | ||||||||||||
const V_0_10_0: &[Uuid] = &[ | ||||||||||||
nullifier_map::MIGRATION_ID, | ||||||||||||
orchard_received_notes::MIGRATION_ID, | ||||||||||||
orchard_shardtree::MIGRATION_ID, | ||||||||||||
]; | ||||||||||||
|
||||||||||||
/// Leaf migrations in the 0.10.3 release. | ||||||||||||
const V_0_10_3: &[Uuid] = &[ | ||||||||||||
ensure_orchard_ua_receiver::MIGRATION_ID, | ||||||||||||
nullifier_map::MIGRATION_ID, | ||||||||||||
orchard_shardtree::MIGRATION_ID, | ||||||||||||
]; | ||||||||||||
|
||||||||||||
/// Leaf migrations in the 0.11.0 release. | ||||||||||||
const V_0_11_0: &[Uuid] = &[ | ||||||||||||
ensure_orchard_ua_receiver::MIGRATION_ID, | ||||||||||||
ephemeral_addresses::MIGRATION_ID, | ||||||||||||
nullifier_map::MIGRATION_ID, | ||||||||||||
orchard_shardtree::MIGRATION_ID, | ||||||||||||
spend_key_available::MIGRATION_ID, | ||||||||||||
tx_retrieval_queue::MIGRATION_ID, | ||||||||||||
]; | ||||||||||||
|
||||||||||||
/// Leaf migrations in the 0.11.1 release. | ||||||||||||
const V_0_11_1: &[Uuid] = &[ | ||||||||||||
ensure_orchard_ua_receiver::MIGRATION_ID, | ||||||||||||
nullifier_map::MIGRATION_ID, | ||||||||||||
orchard_shardtree::MIGRATION_ID, | ||||||||||||
spend_key_available::MIGRATION_ID, | ||||||||||||
tx_retrieval_queue::MIGRATION_ID, | ||||||||||||
]; | ||||||||||||
|
||||||||||||
#[cfg(test)] | ||||||||||||
mod tests { | ||||||||||||
use std::collections::HashSet; | ||||||||||||
|
||||||||||||
use rusqlite::Connection; | ||||||||||||
use secrecy::Secret; | ||||||||||||
use tempfile::NamedTempFile; | ||||||||||||
use uuid::Uuid; | ||||||||||||
|
@@ -157,4 +227,55 @@ | |||||||||||
Ok(_) | ||||||||||||
); | ||||||||||||
} | ||||||||||||
|
||||||||||||
#[test] | ||||||||||||
fn migrate_between_releases_without_data() { | ||||||||||||
let data_file = NamedTempFile::new().unwrap(); | ||||||||||||
let mut db_data = WalletDb::for_path(data_file.path(), Network::TestNetwork).unwrap(); | ||||||||||||
|
||||||||||||
let seed = [0xab; 32].to_vec(); | ||||||||||||
|
||||||||||||
let mut prev_state = HashSet::new(); | ||||||||||||
let mut ensure_migration_state_changed = |conn: &Connection| { | ||||||||||||
let new_state = conn | ||||||||||||
.prepare_cached("SELECT * FROM schemer_migrations") | ||||||||||||
.unwrap() | ||||||||||||
.query_map([], |row| row.get::<_, [u8; 16]>(0).map(Uuid::from_bytes)) | ||||||||||||
.unwrap() | ||||||||||||
.collect::<Result<HashSet<Uuid>, _>>() | ||||||||||||
.unwrap(); | ||||||||||||
assert!(prev_state != new_state); | ||||||||||||
prev_state = new_state; | ||||||||||||
}; | ||||||||||||
|
||||||||||||
let mut prev_leaves: &[Uuid] = &[]; | ||||||||||||
for migrations in super::PUBLIC_MIGRATION_STATES { | ||||||||||||
assert_matches!( | ||||||||||||
init_wallet_db_internal( | ||||||||||||
&mut db_data, | ||||||||||||
Some(Secret::new(seed.clone())), | ||||||||||||
migrations, | ||||||||||||
daira marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
false | ||||||||||||
), | ||||||||||||
Ok(_) | ||||||||||||
); | ||||||||||||
|
||||||||||||
// If we have any new leaves, ensure the migration state changed. This lets us | ||||||||||||
// represent releases that changed the graph edges without introducing any new | ||||||||||||
// migrations. | ||||||||||||
if migrations.iter().any(|m| !prev_leaves.contains(m)) { | ||||||||||||
ensure_migration_state_changed(&db_data.conn); | ||||||||||||
} | ||||||||||||
|
||||||||||||
prev_leaves = *migrations; | ||||||||||||
} | ||||||||||||
|
||||||||||||
// Now check that we can migrate from the last public release to the current | ||||||||||||
// migration state in this branch. | ||||||||||||
assert_matches!( | ||||||||||||
init_wallet_db_internal(&mut db_data, Some(Secret::new(seed.clone())), &[], false), | ||||||||||||
Check failure on line 276 in zcash_client_sqlite/src/wallet/init/migrations.rs GitHub Actions / Clippy (MSRV)redundant clone
|
||||||||||||
Ok(_) | ||||||||||||
); | ||||||||||||
// We don't ensure that the migration state changed, because it may not have. | ||||||||||||
} | ||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,10 +10,12 @@ use zcash_protocol::consensus::{self, BlockHeight, BranchId}; | |
|
||
use crate::wallet::{self, init::WalletMigrationError}; | ||
|
||
use super::utxos_to_txos; | ||
use super::ephemeral_addresses; | ||
|
||
pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0xfec02b61_3988_4b4f_9699_98977fac9e7f); | ||
|
||
const DEPENDENCIES: [Uuid; 1] = [ephemeral_addresses::MIGRATION_ID]; | ||
|
||
pub(super) struct Migration<P> { | ||
pub(super) params: P, | ||
} | ||
|
@@ -24,7 +26,7 @@ impl<P> schemer::Migration for Migration<P> { | |
} | ||
|
||
fn dependencies(&self) -> HashSet<Uuid> { | ||
[utxos_to_txos::MIGRATION_ID].into_iter().collect() | ||
DEPENDENCIES.into_iter().collect() | ||
} | ||
|
||
fn description(&self) -> &'static str { | ||
|
@@ -133,10 +135,89 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> { | |
|
||
#[cfg(test)] | ||
mod tests { | ||
use crate::wallet::init::migrations::tests::test_migrate; | ||
use rusqlite::named_params; | ||
use secrecy::Secret; | ||
use tempfile::NamedTempFile; | ||
use zcash_primitives::{ | ||
legacy::{Script, TransparentAddress}, | ||
transaction::{components::transparent, Authorized, TransactionData, TxVersion}, | ||
}; | ||
use zcash_protocol::{ | ||
consensus::{BranchId, Network}, | ||
value::Zatoshis, | ||
}; | ||
|
||
use crate::{ | ||
wallet::init::{init_wallet_db_internal, migrations::tests::test_migrate}, | ||
WalletDb, | ||
}; | ||
|
||
use super::{DEPENDENCIES, MIGRATION_ID}; | ||
|
||
#[test] | ||
fn migrate() { | ||
test_migrate(&[super::MIGRATION_ID]); | ||
test_migrate(&[MIGRATION_ID]); | ||
} | ||
|
||
#[test] | ||
fn migrate_with_data() { | ||
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]; | ||
|
||
// Migrate to database state just prior to this migration. | ||
init_wallet_db_internal( | ||
&mut db_data, | ||
Some(Secret::new(seed_bytes.clone())), | ||
&DEPENDENCIES, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good way to approach migration tests; it ensures that only the nodes in the graph leading up to this node are evaluated, so anything that's missing should get caught. It does require inspecting what data is required in order to test all the possible branches within the migration though. |
||
false, | ||
) | ||
.unwrap(); | ||
|
||
// Add transactions to the wallet that exercise the data migration. | ||
let add_tx_to_wallet = |tx: TransactionData<Authorized>| { | ||
let tx = tx.freeze().unwrap(); | ||
let txid = tx.txid(); | ||
let mut raw_tx = vec![]; | ||
tx.write(&mut raw_tx).unwrap(); | ||
db_data | ||
.conn | ||
.execute( | ||
r#"INSERT INTO transactions (txid, raw) VALUES (:txid, :raw);"#, | ||
named_params! {":txid": txid.as_ref(), ":raw": raw_tx}, | ||
) | ||
.unwrap(); | ||
}; | ||
add_tx_to_wallet(TransactionData::from_parts( | ||
TxVersion::Zip225, | ||
BranchId::Nu5, | ||
0, | ||
12345678.into(), | ||
Some(transparent::Bundle { | ||
vin: vec![transparent::TxIn { | ||
prevout: transparent::OutPoint::fake(), | ||
script_sig: Script(vec![]), | ||
sequence: 0, | ||
}], | ||
vout: vec![transparent::TxOut { | ||
value: Zatoshis::const_from_u64(10_000), | ||
script_pubkey: TransparentAddress::PublicKeyHash([7; 20]).script(), | ||
}], | ||
authorization: transparent::Authorized, | ||
}), | ||
None, | ||
None, | ||
None, | ||
)); | ||
|
||
// Check that we can apply this migration. | ||
init_wallet_db_internal( | ||
&mut db_data, | ||
Some(Secret::new(seed_bytes)), | ||
&[MIGRATION_ID], | ||
false, | ||
) | ||
.unwrap(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified that {v0.4.1, v0.4.2, v0.5.0} did not add migrations relative to v0.4.0, and at v0.4.2 the ASCII art was correct. In all of these versions,
add_transaction_views
was indeed the only leaf.