Skip to content
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.

Sign up for GitHub

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

Jump to bottom

zcash_client_sqlite: Fix migration DAG edges #1506

Merged
merged 6 commits into from
Aug 21, 2024
Merged

zcash_client_sqlite: Fix migration DAG edges #1506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7c416d5
zcash_client_sqlite: Add test for linear migration between db states
str4d Aug 21, 2024
6903fa6
zcash_client_sqlite: Add test showing tx_retrieval_queue migration bug
str4d Aug 21, 2024
34402f7
zcash_client_sqlite: Adjust migration DAG edges to fix dependency bug
str4d Aug 21, 2024
e4b3fb2
zcash_client_sqlite: Fix clippy complaints.
nuttycom Aug 21, 2024
b1cb8be
zcash_client_sqlite: Make migration dependencies easier to track.
nuttycom Aug 21, 2024
3dba2fa
zcash_client_sqlite: Make `tx_retrieval_queue` depend upon all leaf m…
nuttycom Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions zcash_client_sqlite/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this library adheres to Rust's notion of

## [Unreleased]

### Fixed
- The dependencies of the `tx_retrieval_queue` migration have been fixed to
enable migrating wallets containing certain kinds of transactions.

## [0.11.0] - 2024-08-20

`zcash_client_sqlite` now provides capabilities for the management of ephemeral
Expand Down
155 changes: 135 additions & 20 deletions zcash_client_sqlite/src/wallet/init/migrations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ use std::rc::Rc;

use schemer_rusqlite::RusqliteMigration;
use secrecy::SecretVec;
use uuid::Uuid;
use zcash_protocol::consensus;

use super::WalletMigrationError;
Expand All @@ -51,26 +52,28 @@ pub(super) fn all_migrations<P: consensus::Parameters + 'static>(
// |
// v_transactions_net
// |
// received_notes_nullable_nf------
// / | \
// / | \
// --------------- shardtree_support sapling_memo_consistency nullifier_map
// / / \ \
// orchard_shardtree add_account_birthdays receiving_key_scopes v_transactions_transparent_history
// | \ | |
// v_sapling_shard_unscanned_ranges \ | v_tx_outputs_use_legacy_false
// | \ | |
// wallet_summaries \ | v_transactions_shielding_balance
// \ \ | |
// \ \ | v_transactions_note_uniqueness
// \ \ | /
// -------------------- full_account_ids
// | \
// orchard_received_notes spend_key_available
// / \
// ensure_orchard_ua_receiver utxos_to_txos
// / \
// ephemeral_addresses tx_retrieval_queue
// received_notes_nullable_nf----------------------
// / | \
// / | \
// --------------- shardtree_support sapling_memo_consistency nullifier_map
// / / \ \ |
// orchard_shardtree add_account_birthdays receiving_key_scopes v_transactions_transparent_history |
// | | \ | | |
// | v_sapling_shard_unscanned_ranges \ | v_tx_outputs_use_legacy_false |
// | | \ | | |
// | wallet_summaries \ | v_transactions_shielding_balance |
// | \ \ | | /
// \ \ \ | v_transactions_note_uniqueness /
// \ \ \ | / /
// \ -------------------- full_account_ids /
// \ / \ /
// \ orchard_received_notes spend_key_available /
// \ / \ / /
// \ ensure_orchard_ua_receiver utxos_to_txos / /
// \ \ | / /
// \ \ ephemeral_addresses / /
// \ \ | / /
// ------------------------------ tx_retrieval_queue ----------------------------
vec![
Box::new(initial_setup::Migration {}),
Box::new(utxos_table::Migration {}),
Expand Down Expand Up @@ -131,8 +134,69 @@ pub(super) fn all_migrations<P: consensus::Parameters + 'static>(
]
}

/// 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];
Copy link
Contributor

@daira daira Aug 21, 2024 •
edited
Loading

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.


/// Leaf migrations in the 0.6.0 release.
const V_0_6_0: &[Uuid] = &[v_transactions_net::MIGRATION_ID];
Copy link
Contributor

Choose a reason for hiding this comment

The 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 v_transactions_net was the only leaf.


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, at v0.7.0 a received_notes_nullable_nf migration was added that was not documented in the ASCII art.

Suggested change
/// Leaf migrations in the 0.7.0 release.
const V_0_7_0: &[Uuid] = &[received_notes_nullable_nf::MIGRATION_ID];

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] = &[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;
Expand All @@ -157,4 +221,55 @@ mod tests {
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)), &[], false),
Ok(_)
);
// We don't ensure that the migration state changed, because it may not have.
}
}
6 changes: 3 additions & 3 deletions zcash_client_sqlite/src/wallet/init/migrations/add_account_birthdays.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ 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];
const DEPENDENCIES: &[Uuid] = &[shardtree_support::MIGRATION_ID];

pub(super) struct Migration<P> {
pub(super) params: P,
Expand All @@ -24,7 +24,7 @@ impl<P> schemer::Migration for Migration<P> {
}

fn dependencies(&self) -> HashSet<Uuid> {
DEPENDENCIES.into_iter().collect()
DEPENDENCIES.iter().copied().collect()
}

fn description(&self) -> &'static str {
Expand Down Expand Up @@ -91,7 +91,7 @@ mod tests {
init_wallet_db_internal(
&mut db_data,
Some(Secret::new(seed_bytes.clone())),
&DEPENDENCIES,
DEPENDENCIES,
false,
)
.unwrap();
Expand Down
12 changes: 6 additions & 6 deletions zcash_client_sqlite/src/wallet/init/migrations/add_transaction_views.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ use crate::wallet::init::WalletMigrationError;

pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0x282fad2e_8372_4ca0_8bed_71821320909f);

const DEPENDENCIES: &[Uuid] = &[
add_utxo_account::MIGRATION_ID,
sent_notes_to_internal::MIGRATION_ID,
];

pub(crate) struct Migration;

impl schemer::Migration for Migration {
Expand All @@ -26,12 +31,7 @@ impl schemer::Migration for Migration {
}

fn dependencies(&self) -> HashSet<Uuid> {
[
add_utxo_account::MIGRATION_ID,
sent_notes_to_internal::MIGRATION_ID,
]
.into_iter()
.collect()
DEPENDENCIES.iter().copied().collect()
}

fn description(&self) -> &'static str {
Expand Down
6 changes: 3 additions & 3 deletions zcash_client_sqlite/src/wallet/init/migrations/add_utxo_account.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ use {
/// This migration adds an account identifier column to the UTXOs table.
pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0x761884d6_30d8_44ef_b204_0b82551c4ca1);

const DEPENDENCIES: &[Uuid] = &[utxos_table::MIGRATION_ID, addresses_table::MIGRATION_ID];

pub(super) struct Migration<P> {
pub(super) _params: P,
}
Expand All @@ -38,9 +40,7 @@ impl<P> schemer::Migration for Migration<P> {
}

fn dependencies(&self) -> HashSet<Uuid> {
[utxos_table::MIGRATION_ID, addresses_table::MIGRATION_ID]
.into_iter()
.collect()
DEPENDENCIES.iter().copied().collect()
}

fn description(&self) -> &'static str {
Expand Down
4 changes: 3 additions & 1 deletion zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ use super::ufvk_support;
/// the `accounts` table.
pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0xd956978c_9c87_4d6e_815d_fb8f088d094c);

const DEPENDENCIES: &[Uuid] = &[ufvk_support::MIGRATION_ID];

pub(crate) struct Migration<P: consensus::Parameters> {
pub(crate) params: P,
}
Expand All @@ -30,7 +32,7 @@ impl<P: consensus::Parameters> schemer::Migration for Migration<P> {
}

fn dependencies(&self) -> HashSet<Uuid> {
[ufvk_support::MIGRATION_ID].into_iter().collect()
DEPENDENCIES.iter().copied().collect()
}

fn description(&self) -> &'static str {
Expand Down
4 changes: 3 additions & 1 deletion zcash_client_sqlite/src/wallet/init/migrations/ensure_orchard_ua_receiver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ use crate::{wallet::init::WalletMigrationError, UA_ORCHARD, UA_TRANSPARENT};

pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0x604349c7_5ce5_4768_bea6_12d106ccda93);

const DEPENDENCIES: &[Uuid] = &[orchard_received_notes::MIGRATION_ID];

pub(super) struct Migration<P> {
pub(super) params: P,
}
Expand All @@ -25,7 +27,7 @@ impl<P> schemer::Migration for Migration<P> {
}

fn dependencies(&self) -> HashSet<Uuid> {
[orchard_received_notes::MIGRATION_ID].into_iter().collect()
DEPENDENCIES.iter().copied().collect()
}

fn description(&self) -> &'static str {
Expand Down
6 changes: 3 additions & 3 deletions zcash_client_sqlite/src/wallet/init/migrations/ephemeral_addresses.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use super::utxos_to_txos;

pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0x0e1d4274_1f8e_44e2_909d_689a4bc2967b);

const DEPENDENCIES: [Uuid; 1] = [utxos_to_txos::MIGRATION_ID];
const DEPENDENCIES: &[Uuid] = &[utxos_to_txos::MIGRATION_ID];

#[allow(dead_code)]
pub(super) struct Migration<P> {
Expand All @@ -29,7 +29,7 @@ impl<P> schemer::Migration for Migration<P> {
}

fn dependencies(&self) -> HashSet<Uuid> {
DEPENDENCIES.into_iter().collect()
DEPENDENCIES.iter().copied().collect()
}

fn description(&self) -> &'static str {
Expand Down Expand Up @@ -210,7 +210,7 @@ mod tests {
init_wallet_db_internal(
&mut db_data,
Some(Secret::new(seed0.clone())),
&super::DEPENDENCIES,
super::DEPENDENCIES,
false,
)
.unwrap();
Expand Down
16 changes: 8 additions & 8 deletions zcash_client_sqlite/src/wallet/init/migrations/full_account_ids.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,20 @@ pub(crate) struct Migration<P: consensus::Parameters> {
pub(super) params: P,
}

const DEPENDENCIES: &[Uuid] = &[
receiving_key_scopes::MIGRATION_ID,
add_account_birthdays::MIGRATION_ID,
v_transactions_note_uniqueness::MIGRATION_ID,
wallet_summaries::MIGRATION_ID,
];

impl<P: consensus::Parameters> schemer::Migration for Migration<P> {
fn id(&self) -> Uuid {
MIGRATION_ID
}

fn dependencies(&self) -> HashSet<Uuid> {
[
receiving_key_scopes::MIGRATION_ID,
add_account_birthdays::MIGRATION_ID,
v_transactions_note_uniqueness::MIGRATION_ID,
wallet_summaries::MIGRATION_ID,
]
.into_iter()
.collect()
DEPENDENCIES.iter().copied().collect()
}

fn description(&self) -> &'static str {
Expand Down
6 changes: 3 additions & 3 deletions zcash_client_sqlite/src/wallet/init/migrations/nullifier_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ use super::received_notes_nullable_nf;

pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0xe2d71ac5_6a44_4c6b_a9a0_6d0a79d355f1);

const DEPENDENCIES: &[Uuid] = &[received_notes_nullable_nf::MIGRATION_ID];

pub(super) struct Migration;

impl schemer::Migration for Migration {
Expand All @@ -21,9 +23,7 @@ impl schemer::Migration for Migration {
}

fn dependencies(&self) -> HashSet<Uuid> {
[received_notes_nullable_nf::MIGRATION_ID]
.into_iter()
.collect()
DEPENDENCIES.iter().copied().collect()
}

fn description(&self) -> &'static str {
Expand Down
4 changes: 3 additions & 1 deletion zcash_client_sqlite/src/wallet/init/migrations/orchard_received_notes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ use crate::wallet::{init::WalletMigrationError, pool_code};

pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0x51d7a273_aa19_4109_9325_80e4a5545048);

const DEPENDENCIES: &[Uuid] = &[full_account_ids::MIGRATION_ID];

pub(super) struct Migration;

impl schemer::Migration for Migration {
Expand All @@ -20,7 +22,7 @@ impl schemer::Migration for Migration {
}

fn dependencies(&self) -> HashSet<Uuid> {
[full_account_ids::MIGRATION_ID].into_iter().collect()
DEPENDENCIES.iter().copied().collect()
}

fn description(&self) -> &'static str {
Expand Down
Loading
Loading