From 0de652c68720a5361490ff0cdab74a36fb8ab3d3 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 13 Oct 2023 15:28:50 -0600 Subject: [PATCH 1/2] zcash_client_sqlite: Fix incorrect note deduplication in `v_transactions` The `v_transactions` view is built upon the set of received notes, received note values being added to the balance for the transaction and spent notes being deducted from this balance. This fixes an error wherein if multiple identically-valued notes were spent in a transaction, only one of those notes' values was being counted as having been spent. --- zcash_client_sqlite/src/wallet/init.rs | 12 +- .../src/wallet/init/migrations.rs | 4 + .../v_transactions_note_uniqueness.rs | 160 ++++++++++++++++++ 3 files changed, 172 insertions(+), 4 deletions(-) create mode 100644 zcash_client_sqlite/src/wallet/init/migrations/v_transactions_note_uniqueness.rs diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 38ab33404..49e2b79ee 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -435,7 +435,8 @@ mod tests { "CREATE VIEW v_transactions AS WITH notes AS ( - SELECT sapling_received_notes.account AS account_id, + SELECT sapling_received_notes.id_note AS id, + sapling_received_notes.account AS account_id, transactions.block AS block, transactions.txid AS txid, 2 AS pool, @@ -457,7 +458,8 @@ mod tests { JOIN transactions ON transactions.id_tx = sapling_received_notes.tx UNION - SELECT utxos.received_by_account AS account_id, + SELECT utxos.id_utxo AS id, + utxos.received_by_account AS account_id, utxos.height AS block, utxos.prevout_txid AS txid, 0 AS pool, @@ -467,7 +469,8 @@ mod tests { 0 AS memo_present FROM utxos UNION - SELECT sapling_received_notes.account AS account_id, + SELECT sapling_received_notes.id_note AS id, + sapling_received_notes.account AS account_id, transactions.block AS block, transactions.txid AS txid, 2 AS pool, @@ -479,7 +482,8 @@ mod tests { JOIN transactions ON transactions.id_tx = sapling_received_notes.spent UNION - SELECT utxos.received_by_account AS account_id, + SELECT utxos.id_utxo AS id, + utxos.received_by_account AS account_id, transactions.block AS block, transactions.txid AS txid, 0 AS pool, diff --git a/zcash_client_sqlite/src/wallet/init/migrations.rs b/zcash_client_sqlite/src/wallet/init/migrations.rs index db8ee2194..4c1486be5 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations.rs @@ -12,6 +12,7 @@ mod ufvk_support; mod utxos_table; mod v_sapling_shard_unscanned_ranges; mod v_transactions_net; +mod v_transactions_note_uniqueness; mod v_transactions_shielding_balance; mod v_transactions_transparent_history; mod v_tx_outputs_use_legacy_false; @@ -48,6 +49,8 @@ pub(super) fn all_migrations( // v_sapling_shard_unscanned_ranges v_tx_outputs_use_legacy_false // | | // wallet_summaries v_transactions_shielding_balance + // | + // v_transactions_note_uniqueness vec![ Box::new(initial_setup::Migration {}), Box::new(utxos_table::Migration {}), @@ -82,5 +85,6 @@ pub(super) fn all_migrations( Box::new(v_transactions_transparent_history::Migration), Box::new(v_tx_outputs_use_legacy_false::Migration), Box::new(v_transactions_shielding_balance::Migration), + Box::new(v_transactions_note_uniqueness::Migration), ] } diff --git a/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_note_uniqueness.rs b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_note_uniqueness.rs new file mode 100644 index 000000000..81bf82de2 --- /dev/null +++ b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_note_uniqueness.rs @@ -0,0 +1,160 @@ +//! This migration fixes a bug in `v_transactions` where distinct but otherwise identical notes +//! were being incorrectly deduplicated. + +use std::collections::HashSet; + +use schemer_rusqlite::RusqliteMigration; +use uuid::Uuid; + +use crate::wallet::init::WalletMigrationError; + +use super::v_transactions_shielding_balance; + +pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0xdba47c86_13b5_4601_94b2_0cde0abe1e45); + +pub(super) struct Migration; + +impl schemer::Migration for Migration { + fn id(&self) -> Uuid { + MIGRATION_ID + } + + fn dependencies(&self) -> HashSet { + [v_transactions_shielding_balance::MIGRATION_ID] + .into_iter() + .collect() + } + + fn description(&self) -> &'static str { + "Fixes a bug in v_transactions that was omitting value from identically-valued notes." + } +} + +impl RusqliteMigration for Migration { + type Error = WalletMigrationError; + + fn up(&self, transaction: &rusqlite::Transaction) -> Result<(), Self::Error> { + transaction.execute_batch( + "DROP VIEW v_transactions; + CREATE VIEW v_transactions AS + WITH + notes AS ( + SELECT sapling_received_notes.id_note AS id, + sapling_received_notes.account AS account_id, + transactions.block AS block, + transactions.txid AS txid, + 2 AS pool, + sapling_received_notes.value AS value, + CASE + WHEN sapling_received_notes.is_change THEN 1 + ELSE 0 + END AS is_change, + CASE + WHEN sapling_received_notes.is_change THEN 0 + ELSE 1 + END AS received_count, + CASE + WHEN (sapling_received_notes.memo IS NULL OR sapling_received_notes.memo = X'F6') + THEN 0 + ELSE 1 + END AS memo_present + FROM sapling_received_notes + JOIN transactions + ON transactions.id_tx = sapling_received_notes.tx + UNION + SELECT utxos.id_utxo AS id, + utxos.received_by_account AS account_id, + utxos.height AS block, + utxos.prevout_txid AS txid, + 0 AS pool, + utxos.value_zat AS value, + 0 AS is_change, + 1 AS received_count, + 0 AS memo_present + FROM utxos + UNION + SELECT sapling_received_notes.id_note AS id, + sapling_received_notes.account AS account_id, + transactions.block AS block, + transactions.txid AS txid, + 2 AS pool, + -sapling_received_notes.value AS value, + 0 AS is_change, + 0 AS received_count, + 0 AS memo_present + FROM sapling_received_notes + JOIN transactions + ON transactions.id_tx = sapling_received_notes.spent + UNION + SELECT utxos.id_utxo AS id, + utxos.received_by_account AS account_id, + transactions.block AS block, + transactions.txid AS txid, + 0 AS pool, + -utxos.value_zat AS value, + 0 AS is_change, + 0 AS received_count, + 0 AS memo_present + FROM utxos + JOIN transactions + ON transactions.id_tx = utxos.spent_in_tx + ), + sent_note_counts AS ( + SELECT sent_notes.from_account AS account_id, + transactions.txid AS txid, + COUNT(DISTINCT sent_notes.id_note) as sent_notes, + SUM( + CASE + WHEN (sent_notes.memo IS NULL OR sent_notes.memo = X'F6' OR sapling_received_notes.tx IS NOT NULL) + THEN 0 + ELSE 1 + END + ) AS memo_count + FROM sent_notes + JOIN transactions + ON transactions.id_tx = sent_notes.tx + LEFT JOIN sapling_received_notes + ON (sent_notes.tx, sent_notes.output_pool, sent_notes.output_index) = + (sapling_received_notes.tx, 2, sapling_received_notes.output_index) + WHERE COALESCE(sapling_received_notes.is_change, 0) = 0 + GROUP BY account_id, txid + ), + blocks_max_height AS ( + SELECT MAX(blocks.height) as max_height FROM blocks + ) + SELECT notes.account_id AS account_id, + notes.block AS mined_height, + notes.txid AS txid, + transactions.tx_index AS tx_index, + transactions.expiry_height AS expiry_height, + transactions.raw AS raw, + SUM(notes.value) AS account_balance_delta, + transactions.fee AS fee_paid, + SUM(notes.is_change) > 0 AS has_change, + MAX(COALESCE(sent_note_counts.sent_notes, 0)) AS sent_note_count, + SUM(notes.received_count) AS received_note_count, + SUM(notes.memo_present) + MAX(COALESCE(sent_note_counts.memo_count, 0)) AS memo_count, + blocks.time AS block_time, + ( + blocks.height IS NULL + AND transactions.expiry_height BETWEEN 1 AND blocks_max_height.max_height + ) AS expired_unmined + FROM notes + LEFT JOIN transactions + ON notes.txid = transactions.txid + JOIN blocks_max_height + LEFT JOIN blocks ON blocks.height = notes.block + LEFT JOIN sent_note_counts + ON sent_note_counts.account_id = notes.account_id + AND sent_note_counts.txid = notes.txid + GROUP BY notes.account_id, notes.txid;" + )?; + + Ok(()) + } + + fn down(&self, _transaction: &rusqlite::Transaction) -> Result<(), Self::Error> { + panic!("This migration cannot be reverted."); + } +} + From 11e909a2d1f8b95d225e40cbf9ce9a42a4564301 Mon Sep 17 00:00:00 2001 From: Daira Emma Hopwood Date: Wed, 18 Oct 2023 00:41:34 +0100 Subject: [PATCH 2/2] Add regression test for incorrect note deduplication in `v_transactions`. Signed-off-by: Daira Emma Hopwood --- .../v_transactions_note_uniqueness.rs | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_note_uniqueness.rs b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_note_uniqueness.rs index 81bf82de2..3636ac93f 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_note_uniqueness.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_note_uniqueness.rs @@ -158,3 +158,93 @@ impl RusqliteMigration for Migration { } } +#[cfg(test)] +mod tests { + use rusqlite::{self, params}; + use tempfile::NamedTempFile; + + use zcash_client_backend::keys::UnifiedSpendingKey; + use zcash_primitives::{consensus::Network, zip32::AccountId}; + + use crate::{ + wallet::init::{init_wallet_db_internal, migrations::v_transactions_net}, + WalletDb, + }; + + #[test] + 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(); + + // Create an account in the wallet + let usk0 = + UnifiedSpendingKey::from_seed(&db_data.params, &[0u8; 32][..], AccountId::from(0)) + .unwrap(); + let ufvk0 = usk0.to_unified_full_viewing_key(); + db_data + .conn + .execute( + "INSERT INTO accounts (account, ufvk) VALUES (0, ?)", + params![ufvk0.encode(&db_data.params)], + ) + .unwrap(); + + // Tx 0 contains two received notes, both of 2 zatoshis, that are controlled by account 0. + db_data.conn.execute_batch( + "INSERT INTO blocks (height, hash, time, sapling_tree) VALUES (0, 0, 0, x'00'); + INSERT INTO transactions (block, id_tx, txid) VALUES (0, 0, 'tx0'); + + INSERT INTO received_notes (tx, output_index, account, diversifier, value, rcm, nf, is_change) + VALUES (0, 0, 0, '', 2, '', 'nf_a', false); + INSERT INTO received_notes (tx, output_index, account, diversifier, value, rcm, nf, is_change) + VALUES (0, 3, 0, '', 2, '', 'nf_b', false);").unwrap(); + + let check_balance_delta = |db_data: &mut WalletDb, + expected_notes: i64| { + let mut q = db_data + .conn + .prepare( + "SELECT account_id, account_balance_delta, has_change, memo_count, sent_note_count, received_note_count + FROM v_transactions", + ) + .unwrap(); + let mut rows = q.query([]).unwrap(); + let mut row_count = 0; + while let Some(row) = rows.next().unwrap() { + row_count += 1; + let account: i64 = row.get(0).unwrap(); + let account_balance_delta: i64 = row.get(1).unwrap(); + let has_change: bool = row.get(2).unwrap(); + let memo_count: i64 = row.get(3).unwrap(); + let sent_note_count: i64 = row.get(4).unwrap(); + let received_note_count: i64 = row.get(5).unwrap(); + match account { + 0 => { + assert_eq!(account_balance_delta, 2 * expected_notes); + assert!(!has_change); + assert_eq!(memo_count, 0); + assert_eq!(sent_note_count, 0); + assert_eq!(received_note_count, expected_notes); + } + other => { + panic!( + "Account {:?} is not expected to exist in the wallet.", + other + ); + } + } + } + assert_eq!(row_count, 1); + }; + + // Check for the bug (#1020). + check_balance_delta(&mut db_data, 1); + + // Apply the current migration. + init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID]).unwrap(); + + // Now it should be correct. + check_balance_delta(&mut db_data, 2); + } +}