From 17827539b37d8e7dff30869b7dbb8cdd0f073462 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 7 Apr 2023 11:33:46 -0600 Subject: [PATCH 1/5] Fix double-entry accounting errors in transaction views. --- zcash_client_sqlite/src/wallet/init.rs | 186 ++++--- .../src/wallet/init/migrations.rs | 4 + .../init/migrations/add_transaction_views.rs | 6 +- .../init/migrations/v_transactions_net.rs | 488 ++++++++++++++++++ 4 files changed, 583 insertions(+), 101 deletions(-) create mode 100644 zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index c6a8e529e..27e9fba71 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -453,33 +453,11 @@ mod tests { let expected_views = vec![ // v_transactions "CREATE VIEW v_transactions AS - SELECT notes.id_tx, - notes.mined_height, - notes.tx_index, - notes.txid, - notes.expiry_height, - notes.raw, - SUM(notes.value) + MAX(notes.fee) AS net_value, - MAX(notes.fee) AS fee_paid, - SUM(notes.sent_count) == 0 AS is_wallet_internal, - SUM(notes.is_change) > 0 AS has_change, - SUM(notes.sent_count) AS sent_note_count, - SUM(notes.received_count) AS received_note_count, - SUM(notes.memo_present) AS memo_count, - blocks.time AS block_time - FROM ( - SELECT transactions.id_tx AS id_tx, - transactions.block AS mined_height, - transactions.tx_index AS tx_index, - transactions.txid AS txid, - transactions.expiry_height AS expiry_height, - transactions.raw AS raw, - 0 AS fee, - CASE - WHEN received_notes.is_change THEN 0 - ELSE value - END AS value, - 0 AS sent_count, + WITH + notes AS ( + SELECT received_notes.account AS account_id, + received_notes.tx AS id_tx, + received_notes.value AS value, CASE WHEN received_notes.is_change THEN 1 ELSE 0 @@ -492,80 +470,92 @@ mod tests { WHEN received_notes.memo IS NULL THEN 0 ELSE 1 END AS memo_present - FROM transactions - JOIN received_notes ON transactions.id_tx = received_notes.tx + FROM received_notes UNION - SELECT transactions.id_tx AS id_tx, - transactions.block AS mined_height, - transactions.tx_index AS tx_index, - transactions.txid AS txid, - transactions.expiry_height AS expiry_height, - transactions.raw AS raw, - transactions.fee AS fee, - -sent_notes.value AS value, - CASE - WHEN sent_notes.from_account = sent_notes.to_account THEN 0 - ELSE 1 - END AS sent_count, + SELECT received_notes.account AS account_id, + received_notes.spent AS id_tx, + -received_notes.value AS value, 0 AS is_change, 0 AS received_count, - CASE - WHEN sent_notes.memo IS NULL THEN 0 - ELSE 1 - END AS memo_present - FROM transactions - JOIN sent_notes ON transactions.id_tx = sent_notes.tx - ) AS notes - LEFT JOIN blocks ON notes.mined_height = blocks.height - GROUP BY notes.id_tx", - // v_tx_received - "CREATE VIEW v_tx_received AS - SELECT transactions.id_tx AS id_tx, - transactions.block AS mined_height, - transactions.tx_index AS tx_index, - transactions.txid AS txid, - transactions.expiry_height AS expiry_height, - transactions.raw AS raw, - MAX(received_notes.account) AS received_by_account, - SUM(received_notes.value) AS received_total, - COUNT(received_notes.id_note) AS received_note_count, - SUM( - CASE - WHEN received_notes.memo IS NULL THEN 0 - ELSE 1 - END - ) AS memo_count, - blocks.time AS block_time - FROM transactions - JOIN received_notes - ON transactions.id_tx = received_notes.tx - LEFT JOIN blocks - ON transactions.block = blocks.height - GROUP BY received_notes.tx, received_notes.account", - // v_tx_received - "CREATE VIEW v_tx_sent AS - SELECT transactions.id_tx AS id_tx, - transactions.block AS mined_height, - transactions.tx_index AS tx_index, - transactions.txid AS txid, - transactions.expiry_height AS expiry_height, - transactions.raw AS raw, - MAX(sent_notes.from_account) AS sent_from_account, - SUM(sent_notes.value) AS sent_total, - COUNT(sent_notes.id_note) AS sent_note_count, - SUM( - CASE - WHEN sent_notes.memo IS NULL THEN 0 - ELSE 1 - END - ) AS memo_count, - blocks.time AS block_time - FROM transactions - JOIN sent_notes - ON transactions.id_tx = sent_notes.tx - LEFT JOIN blocks - ON transactions.block = blocks.height - GROUP BY sent_notes.tx, sent_notes.from_account", + 0 AS memo_present + FROM received_notes + WHERE received_notes.spent IS NOT NULL + ), + sent_note_counts AS ( + SELECT from_account AS account_id, + tx AS id_tx, + COUNT(DISTINCT id_note) as sent_notes, + SUM( + CASE + WHEN sent_notes.memo IS NULL THEN 0 + ELSE 1 + END + ) AS memo_count + FROM sent_notes + WHERE (sent_notes.tx, sent_notes.output_index) NOT IN ( + SELECT received_notes.tx, received_notes.output_index FROM received_notes + WHERE received_notes.is_change = 1 + ) + GROUP BY account_id, id_tx + ), + blocks_max_height AS ( + SELECT MAX(blocks.height) as max_height FROM blocks + ) + SELECT notes.account_id AS account_id, + transactions.id_tx AS id_tx, + transactions.block AS mined_height, + transactions.tx_index AS tx_index, + transactions.txid AS txid, + transactions.expiry_height AS expiry_height, + transactions.raw AS raw, + SUM(notes.value) AS net_transfer, + 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 <= blocks_max_height.max_height + ) AS expired_unmined + FROM transactions + JOIN notes ON notes.id_tx = transactions.id_tx + JOIN blocks_max_height + LEFT JOIN blocks ON blocks.height = transactions.block + LEFT JOIN sent_note_counts + ON sent_note_counts.account_id = notes.account_id + AND sent_note_counts.id_tx = notes.id_tx + GROUP BY notes.account_id, transactions.id_tx", + // v_tx_events + "CREATE VIEW v_tx_events AS + SELECT received_notes.tx AS id_tx, + received_notes.output_index AS output_index, + sent_notes.from_account AS from_account, + received_notes.account AS to_account, + NULL AS to_address, + received_notes.value AS value, + received_notes.is_change AS is_change, + received_notes.memo AS memo + FROM received_notes + LEFT JOIN sent_notes + ON sent_notes.tx = received_notes.tx + AND sent_notes.output_index = received_notes.output_index + UNION + SELECT sent_notes.tx AS id_tx, + sent_notes.output_index AS output_index, + sent_notes.from_account AS from_account, + received_notes.account AS to_account, + sent_notes.to_address AS to_address, + sent_notes.value AS value, + false AS is_change, + sent_notes.memo AS memo + FROM sent_notes + LEFT JOIN received_notes + ON received_notes.tx = sent_notes.tx + AND received_notes.output_index = sent_notes.output_index + WHERE received_notes.is_change IS NULL + OR received_notes.is_change = 0", ]; let mut views_query = db_data diff --git a/zcash_client_sqlite/src/wallet/init/migrations.rs b/zcash_client_sqlite/src/wallet/init/migrations.rs index 4880d7468..d566ae1bc 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations.rs @@ -5,6 +5,7 @@ mod initial_setup; mod sent_notes_to_internal; mod ufvk_support; mod utxos_table; +mod v_transactions_net; use schemer_rusqlite::RusqliteMigration; use secrecy::SecretVec; @@ -25,6 +26,8 @@ pub(super) fn all_migrations( // add_utxo_account / // \ / // add_transaction_views + // / + // v_transactions_net vec![ Box::new(initial_setup::Migration {}), Box::new(utxos_table::Migration {}), @@ -40,5 +43,6 @@ pub(super) fn all_migrations( }), Box::new(sent_notes_to_internal::Migration {}), Box::new(add_transaction_views::Migration), + Box::new(v_transactions_net::Migration), ] } diff --git a/zcash_client_sqlite/src/wallet/init/migrations/add_transaction_views.rs b/zcash_client_sqlite/src/wallet/init/migrations/add_transaction_views.rs index 63f8df440..70694f842 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/add_transaction_views.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/add_transaction_views.rs @@ -287,7 +287,7 @@ mod tests { use crate::{ tests, - wallet::init::{init_wallet_db, init_wallet_db_internal, migrations::addresses_table}, + wallet::init::{init_wallet_db_internal, migrations::addresses_table}, WalletDb, }; @@ -345,7 +345,7 @@ mod tests { VALUES (0, 4, 0, '', 7, '', 'c', true, X'63');", ).unwrap(); - init_wallet_db(&mut db_data, None).unwrap(); + init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID]).unwrap(); let mut q = db_data .conn @@ -476,7 +476,7 @@ mod tests { ) .unwrap(); - init_wallet_db(&mut db_data, None).unwrap(); + init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID]).unwrap(); let fee = db_data .conn diff --git a/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs new file mode 100644 index 000000000..5d9db779d --- /dev/null +++ b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs @@ -0,0 +1,488 @@ +//! Migration that fixes a bug in v_transactions that caused the change to be incorrectly ignored +//! as received value. +use std::collections::HashSet; + +use rusqlite::{self, named_params}; +use schemer; +use schemer_rusqlite::RusqliteMigration; +use uuid::Uuid; + +use super::add_transaction_views; +use crate::wallet::{init::WalletMigrationError, pool_code, PoolType}; + +pub(super) const MIGRATION_ID: Uuid = Uuid::from_fields( + 0x2aa4d24f, + 0x51aa, + 0x4a4c, + b"\x8d\x9b\xe5\xb8\xa7\x62\x86\x5f", +); + +pub(crate) struct Migration; + +impl schemer::Migration for Migration { + fn id(&self) -> Uuid { + MIGRATION_ID + } + + fn dependencies(&self) -> HashSet { + [add_transaction_views::MIGRATION_ID].into_iter().collect() + } + + fn description(&self) -> &'static str { + "Fix transaction views to correctly handle double-entry accounting for change." + } +} + +impl RusqliteMigration for Migration { + type Error = WalletMigrationError; + + fn up(&self, transaction: &rusqlite::Transaction) -> Result<(), WalletMigrationError> { + // As of this migration, the `received_notes` table only stores Sapling notes, so + // we can hard-code the `output_pool` value. + transaction.execute( + "INSERT INTO sent_notes (tx, output_pool, output_index, from_account, to_account, value) + SELECT tx, :output_pool, output_index, account, account, value + FROM received_notes + WHERE received_notes.is_change + EXCEPT + SELECT tx, :output_pool, output_index, from_account, from_account, value + FROM sent_notes", + named_params![ + ":output_pool": &pool_code(PoolType::Sapling) + ] + )?; + + transaction.execute_batch( + "DROP VIEW v_tx_received; + DROP VIEW v_tx_sent;", + )?; + + transaction.execute_batch( + "CREATE VIEW v_tx_events AS + SELECT received_notes.tx AS id_tx, + received_notes.output_index AS output_index, + sent_notes.from_account AS from_account, + received_notes.account AS to_account, + NULL AS to_address, + received_notes.value AS value, + received_notes.is_change AS is_change, + received_notes.memo AS memo + FROM received_notes + LEFT JOIN sent_notes + ON sent_notes.tx = received_notes.tx + AND sent_notes.output_index = received_notes.output_index + UNION + SELECT sent_notes.tx AS id_tx, + sent_notes.output_index AS output_index, + sent_notes.from_account AS from_account, + received_notes.account AS to_account, + sent_notes.to_address AS to_address, + sent_notes.value AS value, + false AS is_change, + sent_notes.memo AS memo + FROM sent_notes + LEFT JOIN received_notes + ON received_notes.tx = sent_notes.tx + AND received_notes.output_index = sent_notes.output_index + WHERE received_notes.is_change IS NULL + OR received_notes.is_change = 0", + )?; + + transaction.execute_batch( + "DROP VIEW v_transactions; + CREATE VIEW v_transactions AS + WITH + notes AS ( + SELECT received_notes.account AS account_id, + received_notes.tx AS id_tx, + received_notes.value AS value, + CASE + WHEN received_notes.is_change THEN 1 + ELSE 0 + END AS is_change, + CASE + WHEN received_notes.is_change THEN 0 + ELSE 1 + END AS received_count, + CASE + WHEN received_notes.memo IS NULL THEN 0 + ELSE 1 + END AS memo_present + FROM received_notes + UNION + SELECT received_notes.account AS account_id, + received_notes.spent AS id_tx, + -received_notes.value AS value, + 0 AS is_change, + 0 AS received_count, + 0 AS memo_present + FROM received_notes + WHERE received_notes.spent IS NOT NULL + ), + sent_note_counts AS ( + SELECT from_account AS account_id, + tx AS id_tx, + COUNT(DISTINCT id_note) as sent_notes, + SUM( + CASE + WHEN sent_notes.memo IS NULL THEN 0 + ELSE 1 + END + ) AS memo_count + FROM sent_notes + WHERE (sent_notes.tx, sent_notes.output_index) NOT IN ( + SELECT received_notes.tx, received_notes.output_index FROM received_notes + WHERE received_notes.is_change = 1 + ) + GROUP BY account_id, id_tx + ), + blocks_max_height AS ( + SELECT MAX(blocks.height) as max_height FROM blocks + ) + SELECT notes.account_id AS account_id, + transactions.id_tx AS id_tx, + transactions.block AS mined_height, + transactions.tx_index AS tx_index, + transactions.txid AS txid, + transactions.expiry_height AS expiry_height, + transactions.raw AS raw, + SUM(notes.value) AS net_transfer, + 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 <= blocks_max_height.max_height + ) AS expired_unmined + FROM transactions + JOIN notes ON notes.id_tx = transactions.id_tx + JOIN blocks_max_height + LEFT JOIN blocks ON blocks.height = transactions.block + LEFT JOIN sent_note_counts + ON sent_note_counts.account_id = notes.account_id + AND sent_note_counts.id_tx = notes.id_tx + GROUP BY notes.account_id, transactions.id_tx", + )?; + + Ok(()) + } + + fn down(&self, _transaction: &rusqlite::Transaction) -> Result<(), WalletMigrationError> { + // TODO: something better than just panic? + panic!("Cannot revert this migration."); + } +} + +#[cfg(test)] +mod tests { + use rusqlite::{self, params}; + use tempfile::NamedTempFile; + + use zcash_client_backend::keys::UnifiedSpendingKey; + use zcash_primitives::zip32::AccountId; + + use crate::{ + tests, + wallet::init::{init_wallet_db_internal, migrations::add_transaction_views}, + WalletDb, + }; + + #[test] + fn v_transactions_net() { + let data_file = NamedTempFile::new().unwrap(); + let mut db_data = WalletDb::for_path(data_file.path(), tests::network()).unwrap(); + init_wallet_db_internal(&mut db_data, None, &[add_transaction_views::MIGRATION_ID]) + .unwrap(); + + // Create two accounts in the wallet. + let usk0 = + UnifiedSpendingKey::from_seed(&tests::network(), &[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(&tests::network())], + ) + .unwrap(); + + let usk1 = + UnifiedSpendingKey::from_seed(&tests::network(), &[1u8; 32][..], AccountId::from(1)) + .unwrap(); + let ufvk1 = usk1.to_unified_full_viewing_key(); + db_data + .conn + .execute( + "INSERT INTO accounts (account, ufvk) VALUES (1, ?)", + params![ufvk1.encode(&tests::network())], + ) + .unwrap(); + + // - Tx 0 contains two received notes of 2 and 5 zatoshis that are controlled by account 0. + db_data.conn.execute_batch( + "INSERT INTO blocks (height, hash, time, sapling_tree) VALUES (0, 0, 0, ''); + 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, '', 5, '', 'nf_b', false);").unwrap(); + + // - Tx 1 creates two notes of 2 and 3 zatoshis for an external address, and a change note + // of 2 zatoshis. This is representative of a historic transaction where no `sent_notes` + // entry was created for the change value. + db_data.conn.execute_batch( + "INSERT INTO blocks (height, hash, time, sapling_tree) VALUES (1, 1, 1, ''); + INSERT INTO transactions (block, id_tx, txid) VALUES (1, 1, 'tx1'); + UPDATE received_notes SET spent = 1 WHERE tx = 0; + INSERT INTO sent_notes (tx, output_pool, output_index, from_account, to_account, to_address, value) + VALUES (1, 2, 0, 0, NULL, 'addra', 2); + INSERT INTO sent_notes (tx, output_pool, output_index, from_account, to_account, to_address, value, memo) + VALUES (1, 2, 1, 0, NULL, 'addrb', 3, X'61'); + INSERT INTO received_notes (tx, output_index, account, diversifier, value, rcm, nf, is_change) + VALUES (1, 2, 0, '', 2, '', 'nf_c', true);").unwrap(); + + // - Tx 2 sends the half of the wallet value from account 0 to account 1 and returns the + // other half to the sending account as change. + db_data.conn.execute_batch( + "INSERT INTO blocks (height, hash, time, sapling_tree) VALUES (2, 2, 2, ''); + INSERT INTO transactions (block, id_tx, txid) VALUES (2, 2, 'tx2'); + UPDATE received_notes SET spent = 2 WHERE tx = 1; + INSERT INTO sent_notes (tx, output_pool, output_index, from_account, to_account, to_address, value) + VALUES (2, 2, 0, 0, 0, NULL, 1); + INSERT INTO sent_notes (tx, output_pool, output_index, from_account, to_account, to_address, value) + VALUES (2, 2, 1, 0, 1, NULL, 1); + INSERT INTO received_notes (tx, output_index, account, diversifier, value, rcm, nf, is_change) + VALUES (2, 0, 0, '', 1, '', 'nf_d', true); + INSERT INTO received_notes (tx, output_index, account, diversifier, value, rcm, nf, is_change) + VALUES (2, 1, 1, '', 1, '', 'nf_e', false);", + ).unwrap(); + + // Behavior prior to change: + { + let mut q = db_data + .conn + .prepare( + "SELECT id_tx, received_by_account, received_total, received_note_count, memo_count + FROM v_tx_received", + ) + .unwrap(); + let mut rows = q.query([]).unwrap(); + let mut row_count = 0; + while let Some(row) = rows.next().unwrap() { + row_count += 1; + let tx: i64 = row.get(0).unwrap(); + let account: i64 = row.get(1).unwrap(); + let total: i64 = row.get(2).unwrap(); + let count: i64 = row.get(3).unwrap(); + let memo_count: i64 = row.get(4).unwrap(); + match (account, tx) { + (0, 0) => { + assert_eq!(total, 7); + assert_eq!(count, 2); + assert_eq!(memo_count, 0); + } + (0, 1) => { + // ERROR: transaction 1 only has change, should not be counted as received + assert_eq!(total, 2); + assert_eq!(count, 1); + assert_eq!(memo_count, 0); + } + (0, 2) => { + // ERROR: transaction 2 was counted twice: as a received transfer, and as change + assert_eq!(total, 1); + assert_eq!(count, 1); + assert_eq!(memo_count, 0); + } + (1, 2) => { + // Transaction 2 should be counted as received, as this is a cross-account + // transfer, not change. + assert_eq!(total, 1); + assert_eq!(count, 1); + assert_eq!(memo_count, 0); + } + _ => { + panic!("No such transaction."); + } + } + } + assert_eq!(row_count, 4); + + let mut q = db_data + .conn + .prepare("SELECT id_tx, sent_total, sent_note_count, memo_count FROM v_tx_sent") + .unwrap(); + let mut rows = q.query([]).unwrap(); + let mut row_count = 0; + while let Some(row) = rows.next().unwrap() { + row_count += 1; + let tx: i64 = row.get(0).unwrap(); + let total: i64 = row.get(1).unwrap(); + let count: i64 = row.get(2).unwrap(); + let memo_count: i64 = row.get(3).unwrap(); + match tx { + 1 => { + assert_eq!(total, 5); + assert_eq!(count, 2); + assert_eq!(memo_count, 1); + } + 2 => { + // ERROR: the total "sent" includes the change + assert_eq!(total, 2); + assert_eq!(count, 2); + assert_eq!(memo_count, 0); + } + other => { + panic!("Transaction {} is not a sent tx.", other); + } + } + } + assert_eq!(row_count, 2); + } + + // Run this migration + init_wallet_db_internal(&mut db_data, None, &[super::MIGRATION_ID]).unwrap(); + + // Corrected behavior after v_transactions has been updated + { + let mut q = db_data + .conn + .prepare( + "SELECT account_id, id_tx, net_transfer, 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 tx: i64 = row.get(1).unwrap(); + let net_transfer: i64 = row.get(2).unwrap(); + let has_change: bool = row.get(3).unwrap(); + let memo_count: i64 = row.get(4).unwrap(); + let sent_note_count: i64 = row.get(5).unwrap(); + let received_note_count: i64 = row.get(6).unwrap(); + match (account, tx) { + (0, 0) => { + assert_eq!(net_transfer, 7); + assert!(!has_change); + assert_eq!(memo_count, 0); + } + (0, 1) => { + assert_eq!(net_transfer, -5); + assert!(has_change); + assert_eq!(memo_count, 1); + } + (0, 2) => { + assert_eq!(net_transfer, -1); + assert!(has_change); + assert_eq!(memo_count, 0); + assert_eq!(sent_note_count, 1); + assert_eq!(received_note_count, 0); + } + (1, 2) => { + assert_eq!(net_transfer, 1); + assert!(!has_change); + assert_eq!(memo_count, 0); + assert_eq!(sent_note_count, 0); + assert_eq!(received_note_count, 1); + } + other => { + panic!("(Account, Transaction) pair {:?} is not expected to exist in the wallet.", other); + } + } + } + assert_eq!(row_count, 4); + } + + // tests for v_tx_events + { + let mut q = db_data + .conn + .prepare("SELECT * FROM v_tx_events WHERE id_tx = 1") + .unwrap(); + let mut rows = q.query([]).unwrap(); + let mut row_count = 0; + while let Some(row) = rows.next().unwrap() { + row_count += 1; + let tx: i64 = row.get(0).unwrap(); + let output_index: i64 = row.get(1).unwrap(); + let from_account: Option = row.get(2).unwrap(); + let to_account: Option = row.get(3).unwrap(); + let to_address: Option = row.get(4).unwrap(); + let value: i64 = row.get(5).unwrap(); + let is_change: bool = row.get(6).unwrap(); + //let memo: Option = row.get(7).unwrap(); + match output_index { + 0 => { + assert_eq!(from_account, Some(0)); + assert_eq!(to_account, None); + assert_eq!(to_address, Some("addra".to_string())); + assert_eq!(value, 2); + assert!(!is_change); + } + 1 => { + assert_eq!(from_account, Some(0)); + assert_eq!(to_account, None); + assert_eq!(to_address, Some("addrb".to_string())); + assert_eq!(value, 3); + assert!(!is_change); + } + 2 => { + assert_eq!(from_account, Some(0)); + assert_eq!(to_account, Some(0)); + assert_eq!(to_address, None); + assert_eq!(value, 2); + assert!(is_change); + } + other => { + panic!("Unexpected output index for tx {}: {}.", tx, other); + } + } + } + assert_eq!(row_count, 3); + + let mut q = db_data + .conn + .prepare("SELECT * FROM v_tx_events WHERE id_tx = 2") + .unwrap(); + let mut rows = q.query([]).unwrap(); + let mut row_count = 0; + while let Some(row) = rows.next().unwrap() { + row_count += 1; + let tx: i64 = row.get(0).unwrap(); + let output_index: i64 = row.get(1).unwrap(); + let from_account: Option = row.get(2).unwrap(); + let to_account: Option = row.get(3).unwrap(); + let to_address: Option = row.get(4).unwrap(); + let value: i64 = row.get(5).unwrap(); + let is_change: bool = row.get(6).unwrap(); + match output_index { + 0 => { + assert_eq!(from_account, Some(0)); + assert_eq!(to_account, Some(0)); + assert_eq!(to_address, None); + assert_eq!(value, 1); + assert!(is_change); + } + 1 => { + assert_eq!(from_account, Some(0)); + assert_eq!(to_account, Some(1)); + assert_eq!(to_address, None); + assert_eq!(value, 1); + assert!(!is_change); + } + other => { + panic!("Unexpected output index for tx {}: {}.", tx, other); + } + } + } + assert_eq!(row_count, 2); + } + } +} From 024c858c8a97574a72b33a5136c394dfa1645b58 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 11 Apr 2023 15:51:58 -0600 Subject: [PATCH 2/5] Ensure that we don't filter out sent UTXOs that collide with Sapling change output indices. Co-authored-by: Jack Grigg --- zcash_client_sqlite/src/wallet/init.rs | 31 +++++----- .../init/migrations/v_transactions_net.rs | 62 +++++++++++-------- 2 files changed, 53 insertions(+), 40 deletions(-) diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 27e9fba71..630e3c167 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -482,9 +482,9 @@ mod tests { WHERE received_notes.spent IS NOT NULL ), sent_note_counts AS ( - SELECT from_account AS account_id, - tx AS id_tx, - COUNT(DISTINCT id_note) as sent_notes, + SELECT sent_notes.from_account AS account_id, + sent_notes.tx AS id_tx, + COUNT(DISTINCT sent_notes.id_note) as sent_notes, SUM( CASE WHEN sent_notes.memo IS NULL THEN 0 @@ -492,10 +492,11 @@ mod tests { END ) AS memo_count FROM sent_notes - WHERE (sent_notes.tx, sent_notes.output_index) NOT IN ( - SELECT received_notes.tx, received_notes.output_index FROM received_notes - WHERE received_notes.is_change = 1 - ) + LEFT JOIN received_notes + ON (sent_notes.tx, sent_notes.output_pool, sent_notes.output_index) = + (received_notes.tx, 2, received_notes.output_index) + WHERE received_notes.is_change IS NULL + OR received_notes.is_change = 0 GROUP BY account_id, id_tx ), blocks_max_height AS ( @@ -516,9 +517,9 @@ mod tests { SUM(notes.memo_present) + MAX(COALESCE(sent_note_counts.memo_count, 0)) AS memo_count, blocks.time AS block_time, ( - blocks.height IS NULL + blocks.height IS NULL AND transactions.expiry_height <= blocks_max_height.max_height - ) AS expired_unmined + ) AS expired_unmined FROM transactions JOIN notes ON notes.id_tx = transactions.id_tx JOIN blocks_max_height @@ -530,6 +531,7 @@ mod tests { // v_tx_events "CREATE VIEW v_tx_events AS SELECT received_notes.tx AS id_tx, + 2 AS output_pool, received_notes.output_index AS output_index, sent_notes.from_account AS from_account, received_notes.account AS to_account, @@ -539,10 +541,11 @@ mod tests { received_notes.memo AS memo FROM received_notes LEFT JOIN sent_notes - ON sent_notes.tx = received_notes.tx - AND sent_notes.output_index = received_notes.output_index - UNION + ON (sent_notes.tx, sent_notes.output_pool, sent_notes.output_index) = + (received_notes.tx, 2, sent_notes.output_index) + UNION SELECT sent_notes.tx AS id_tx, + sent_notes.output_pool AS output_pool, sent_notes.output_index AS output_index, sent_notes.from_account AS from_account, received_notes.account AS to_account, @@ -552,8 +555,8 @@ mod tests { sent_notes.memo AS memo FROM sent_notes LEFT JOIN received_notes - ON received_notes.tx = sent_notes.tx - AND received_notes.output_index = sent_notes.output_index + ON (sent_notes.tx, sent_notes.output_pool, sent_notes.output_index) = + (received_notes.tx, 2, received_notes.output_index) WHERE received_notes.is_change IS NULL OR received_notes.is_change = 0", ]; diff --git a/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs index 5d9db779d..900d5dd42 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs @@ -60,6 +60,7 @@ impl RusqliteMigration for Migration { transaction.execute_batch( "CREATE VIEW v_tx_events AS SELECT received_notes.tx AS id_tx, + 2 AS output_pool, received_notes.output_index AS output_index, sent_notes.from_account AS from_account, received_notes.account AS to_account, @@ -69,10 +70,11 @@ impl RusqliteMigration for Migration { received_notes.memo AS memo FROM received_notes LEFT JOIN sent_notes - ON sent_notes.tx = received_notes.tx - AND sent_notes.output_index = received_notes.output_index - UNION + ON (sent_notes.tx, sent_notes.output_pool, sent_notes.output_index) = + (received_notes.tx, 2, sent_notes.output_index) + UNION SELECT sent_notes.tx AS id_tx, + sent_notes.output_pool AS output_pool, sent_notes.output_index AS output_index, sent_notes.from_account AS from_account, received_notes.account AS to_account, @@ -82,8 +84,8 @@ impl RusqliteMigration for Migration { sent_notes.memo AS memo FROM sent_notes LEFT JOIN received_notes - ON received_notes.tx = sent_notes.tx - AND received_notes.output_index = sent_notes.output_index + ON (sent_notes.tx, sent_notes.output_pool, sent_notes.output_index) = + (received_notes.tx, 2, received_notes.output_index) WHERE received_notes.is_change IS NULL OR received_notes.is_change = 0", )?; @@ -120,9 +122,9 @@ impl RusqliteMigration for Migration { WHERE received_notes.spent IS NOT NULL ), sent_note_counts AS ( - SELECT from_account AS account_id, - tx AS id_tx, - COUNT(DISTINCT id_note) as sent_notes, + SELECT sent_notes.from_account AS account_id, + sent_notes.tx AS id_tx, + COUNT(DISTINCT sent_notes.id_note) as sent_notes, SUM( CASE WHEN sent_notes.memo IS NULL THEN 0 @@ -130,10 +132,11 @@ impl RusqliteMigration for Migration { END ) AS memo_count FROM sent_notes - WHERE (sent_notes.tx, sent_notes.output_index) NOT IN ( - SELECT received_notes.tx, received_notes.output_index FROM received_notes - WHERE received_notes.is_change = 1 - ) + LEFT JOIN received_notes + ON (sent_notes.tx, sent_notes.output_pool, sent_notes.output_index) = + (received_notes.tx, 2, received_notes.output_index) + WHERE received_notes.is_change IS NULL + OR received_notes.is_change = 0 GROUP BY account_id, id_tx ), blocks_max_height AS ( @@ -154,9 +157,9 @@ impl RusqliteMigration for Migration { SUM(notes.memo_present) + MAX(COALESCE(sent_note_counts.memo_count, 0)) AS memo_count, blocks.time AS block_time, ( - blocks.height IS NULL + blocks.height IS NULL AND transactions.expiry_height <= blocks_max_height.max_height - ) AS expired_unmined + ) AS expired_unmined FROM transactions JOIN notes ON notes.id_tx = transactions.id_tx JOIN blocks_max_height @@ -411,15 +414,17 @@ mod tests { while let Some(row) = rows.next().unwrap() { row_count += 1; let tx: i64 = row.get(0).unwrap(); - let output_index: i64 = row.get(1).unwrap(); - let from_account: Option = row.get(2).unwrap(); - let to_account: Option = row.get(3).unwrap(); - let to_address: Option = row.get(4).unwrap(); - let value: i64 = row.get(5).unwrap(); - let is_change: bool = row.get(6).unwrap(); + let output_pool: i64 = row.get(1).unwrap(); + let output_index: i64 = row.get(2).unwrap(); + let from_account: Option = row.get(3).unwrap(); + let to_account: Option = row.get(4).unwrap(); + let to_address: Option = row.get(5).unwrap(); + let value: i64 = row.get(6).unwrap(); + let is_change: bool = row.get(7).unwrap(); //let memo: Option = row.get(7).unwrap(); match output_index { 0 => { + assert_eq!(output_pool, 2); assert_eq!(from_account, Some(0)); assert_eq!(to_account, None); assert_eq!(to_address, Some("addra".to_string())); @@ -427,6 +432,7 @@ mod tests { assert!(!is_change); } 1 => { + assert_eq!(output_pool, 2); assert_eq!(from_account, Some(0)); assert_eq!(to_account, None); assert_eq!(to_address, Some("addrb".to_string())); @@ -434,6 +440,7 @@ mod tests { assert!(!is_change); } 2 => { + assert_eq!(output_pool, 2); assert_eq!(from_account, Some(0)); assert_eq!(to_account, Some(0)); assert_eq!(to_address, None); @@ -456,14 +463,16 @@ mod tests { while let Some(row) = rows.next().unwrap() { row_count += 1; let tx: i64 = row.get(0).unwrap(); - let output_index: i64 = row.get(1).unwrap(); - let from_account: Option = row.get(2).unwrap(); - let to_account: Option = row.get(3).unwrap(); - let to_address: Option = row.get(4).unwrap(); - let value: i64 = row.get(5).unwrap(); - let is_change: bool = row.get(6).unwrap(); + let output_pool: i64 = row.get(1).unwrap(); + let output_index: i64 = row.get(2).unwrap(); + let from_account: Option = row.get(3).unwrap(); + let to_account: Option = row.get(4).unwrap(); + let to_address: Option = row.get(5).unwrap(); + let value: i64 = row.get(6).unwrap(); + let is_change: bool = row.get(7).unwrap(); match output_index { 0 => { + assert_eq!(output_pool, 2); assert_eq!(from_account, Some(0)); assert_eq!(to_account, Some(0)); assert_eq!(to_address, None); @@ -471,6 +480,7 @@ mod tests { assert!(is_change); } 1 => { + assert_eq!(output_pool, 2); assert_eq!(from_account, Some(0)); assert_eq!(to_account, Some(1)); assert_eq!(to_address, None); From 25b2148604f17d860a892884ccb374c427c0803e Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 12 Apr 2023 15:19:52 -0600 Subject: [PATCH 3/5] Account for received utxos in `v_transactions`. Also, rename `v_tx_events` to `v_tx_outputs`. --- zcash_client_sqlite/src/wallet/init.rs | 30 ++++- .../init/migrations/v_transactions_net.rs | 114 +++++++++++++++--- 2 files changed, 126 insertions(+), 18 deletions(-) diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 630e3c167..6ef658c11 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -457,6 +457,7 @@ mod tests { notes AS ( SELECT received_notes.account AS account_id, received_notes.tx AS id_tx, + 2 AS pool, received_notes.value AS value, CASE WHEN received_notes.is_change THEN 1 @@ -472,8 +473,20 @@ mod tests { END AS memo_present FROM received_notes UNION + SELECT utxos.received_by_account AS account_id, + transactions.id_tx AS id_tx, + 0 AS pool, + utxos.value_zat AS value, + 0 AS is_change, + 1 AS received_count, + 0 AS memo_present + FROM utxos + JOIN transactions + ON transactions.txid = utxos.prevout_txid + UNION SELECT received_notes.account AS account_id, received_notes.spent AS id_tx, + 2 AS pool, -received_notes.value AS value, 0 AS is_change, 0 AS received_count, @@ -528,8 +541,8 @@ mod tests { ON sent_note_counts.account_id = notes.account_id AND sent_note_counts.id_tx = notes.id_tx GROUP BY notes.account_id, transactions.id_tx", - // v_tx_events - "CREATE VIEW v_tx_events AS + // v_tx_outputs + "CREATE VIEW v_tx_outputs AS SELECT received_notes.tx AS id_tx, 2 AS output_pool, received_notes.output_index AS output_index, @@ -544,6 +557,19 @@ mod tests { ON (sent_notes.tx, sent_notes.output_pool, sent_notes.output_index) = (received_notes.tx, 2, sent_notes.output_index) UNION + SELECT transactions.id_tx AS id_tx, + 0 AS output_pool, + utxos.prevout_idx AS output_index, + NULL AS from_account, + utxos.received_by_account AS to_account, + utxos.address AS to_address, + utxos.value_zat AS value, + false AS is_change, + NULL AS memo + FROM utxos + JOIN transactions + ON transactions.txid = utxos.prevout_txid + UNION SELECT sent_notes.tx AS id_tx, sent_notes.output_pool AS output_pool, sent_notes.output_index AS output_index, diff --git a/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs index 900d5dd42..dcfbe9c8b 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs @@ -58,7 +58,7 @@ impl RusqliteMigration for Migration { )?; transaction.execute_batch( - "CREATE VIEW v_tx_events AS + "CREATE VIEW v_tx_outputs AS SELECT received_notes.tx AS id_tx, 2 AS output_pool, received_notes.output_index AS output_index, @@ -73,6 +73,19 @@ impl RusqliteMigration for Migration { ON (sent_notes.tx, sent_notes.output_pool, sent_notes.output_index) = (received_notes.tx, 2, sent_notes.output_index) UNION + SELECT transactions.id_tx AS id_tx, + 0 AS output_pool, + utxos.prevout_idx AS output_index, + NULL AS from_account, + utxos.received_by_account AS to_account, + utxos.address AS to_address, + utxos.value_zat AS value, + false AS is_change, + NULL AS memo + FROM utxos + JOIN transactions + ON transactions.txid = utxos.prevout_txid + UNION SELECT sent_notes.tx AS id_tx, sent_notes.output_pool AS output_pool, sent_notes.output_index AS output_index, @@ -97,6 +110,7 @@ impl RusqliteMigration for Migration { notes AS ( SELECT received_notes.account AS account_id, received_notes.tx AS id_tx, + 2 AS pool, received_notes.value AS value, CASE WHEN received_notes.is_change THEN 1 @@ -112,8 +126,20 @@ impl RusqliteMigration for Migration { END AS memo_present FROM received_notes UNION + SELECT utxos.received_by_account AS account_id, + transactions.id_tx AS id_tx, + 0 AS pool, + utxos.value_zat AS value, + 0 AS is_change, + 1 AS received_count, + 0 AS memo_present + FROM utxos + JOIN transactions + ON transactions.txid = utxos.prevout_txid + UNION SELECT received_notes.account AS account_id, received_notes.spent AS id_tx, + 2 AS pool, -received_notes.value AS value, 0 AS is_change, 0 AS received_count, @@ -250,11 +276,14 @@ mod tests { VALUES (1, 2, 0, '', 2, '', 'nf_c', true);").unwrap(); // - Tx 2 sends the half of the wallet value from account 0 to account 1 and returns the - // other half to the sending account as change. + // other half to the sending account as change. Also there's a random transparent utxo, + // received, who knows where it came from but it's for account 0. db_data.conn.execute_batch( "INSERT INTO blocks (height, hash, time, sapling_tree) VALUES (2, 2, 2, ''); INSERT INTO transactions (block, id_tx, txid) VALUES (2, 2, 'tx2'); UPDATE received_notes SET spent = 2 WHERE tx = 1; + INSERT INTO utxos (received_by_account, address, prevout_txid, prevout_idx, script, value_zat, height) + VALUES (0, 'taddr_tx2', 'tx2', 0, '', 1, 2); INSERT INTO sent_notes (tx, output_pool, output_index, from_account, to_account, to_address, value) VALUES (2, 2, 0, 0, 0, NULL, 1); INSERT INTO sent_notes (tx, output_pool, output_index, from_account, to_account, to_address, value) @@ -265,6 +294,15 @@ mod tests { VALUES (2, 1, 1, '', 1, '', 'nf_e', false);", ).unwrap(); + // - Tx 3 just receives transparent funds and does nothing else. For this to work, the + // transaction must be retrieved by the wallet. + db_data.conn.execute_batch( + "INSERT INTO blocks (height, hash, time, sapling_tree) VALUES (3, 3, 3, ''); + INSERT INTO transactions (block, id_tx, txid) VALUES (3, 3, 'tx3'); + + INSERT INTO utxos (received_by_account, address, prevout_txid, prevout_idx, script, value_zat, height) + VALUES (0, 'taddr_tx3', 'tx3', 0, '', 1, 3);").unwrap(); + // Behavior prior to change: { let mut q = db_data @@ -296,7 +334,8 @@ mod tests { assert_eq!(memo_count, 0); } (0, 2) => { - // ERROR: transaction 2 was counted twice: as a received transfer, and as change + // ERROR: transaction 2 was counted twice: as a received transfer, and as change. + // Also, received transparent funds didn't appear in `v_transactions`. assert_eq!(total, 1); assert_eq!(count, 1); assert_eq!(memo_count, 0); @@ -382,11 +421,11 @@ mod tests { assert_eq!(memo_count, 1); } (0, 2) => { - assert_eq!(net_transfer, -1); + assert_eq!(net_transfer, 0); assert!(has_change); assert_eq!(memo_count, 0); assert_eq!(sent_note_count, 1); - assert_eq!(received_note_count, 0); + assert_eq!(received_note_count, 1); } (1, 2) => { assert_eq!(net_transfer, 1); @@ -395,19 +434,26 @@ mod tests { assert_eq!(sent_note_count, 0); assert_eq!(received_note_count, 1); } + (0, 3) => { + assert_eq!(net_transfer, 1); + assert!(!has_change); + assert_eq!(memo_count, 0); + assert_eq!(sent_note_count, 0); + assert_eq!(received_note_count, 1); + } other => { panic!("(Account, Transaction) pair {:?} is not expected to exist in the wallet.", other); } } } - assert_eq!(row_count, 4); + assert_eq!(row_count, 5); } - // tests for v_tx_events + // tests for v_tx_outputs { let mut q = db_data .conn - .prepare("SELECT * FROM v_tx_events WHERE id_tx = 1") + .prepare("SELECT * FROM v_tx_outputs WHERE id_tx = 1") .unwrap(); let mut rows = q.query([]).unwrap(); let mut row_count = 0; @@ -456,7 +502,7 @@ mod tests { let mut q = db_data .conn - .prepare("SELECT * FROM v_tx_events WHERE id_tx = 2") + .prepare("SELECT * FROM v_tx_outputs WHERE id_tx = 2") .unwrap(); let mut rows = q.query([]).unwrap(); let mut row_count = 0; @@ -470,17 +516,22 @@ mod tests { let to_address: Option = row.get(5).unwrap(); let value: i64 = row.get(6).unwrap(); let is_change: bool = row.get(7).unwrap(); - match output_index { - 0 => { - assert_eq!(output_pool, 2); + match (output_pool, output_index) { + (0, 0) => { + assert_eq!(from_account, None); + assert_eq!(to_account, Some(0)); + assert_eq!(to_address, Some("taddr_tx2".to_string())); + assert_eq!(value, 1); + assert!(!is_change); + } + (2, 0) => { assert_eq!(from_account, Some(0)); assert_eq!(to_account, Some(0)); assert_eq!(to_address, None); assert_eq!(value, 1); assert!(is_change); } - 1 => { - assert_eq!(output_pool, 2); + (2, 1) => { assert_eq!(from_account, Some(0)); assert_eq!(to_account, Some(1)); assert_eq!(to_address, None); @@ -488,11 +539,42 @@ mod tests { assert!(!is_change); } other => { - panic!("Unexpected output index for tx {}: {}.", tx, other); + panic!("Unexpected output pool and index for tx {}: {:?}.", tx, other); } } } - assert_eq!(row_count, 2); + assert_eq!(row_count, 3); + + let mut q = db_data + .conn + .prepare("SELECT * FROM v_tx_outputs WHERE id_tx = 3") + .unwrap(); + let mut rows = q.query([]).unwrap(); + let mut row_count = 0; + while let Some(row) = rows.next().unwrap() { + row_count += 1; + let tx: i64 = row.get(0).unwrap(); + let output_pool: i64 = row.get(1).unwrap(); + let output_index: i64 = row.get(2).unwrap(); + let from_account: Option = row.get(3).unwrap(); + let to_account: Option = row.get(4).unwrap(); + let to_address: Option = row.get(5).unwrap(); + let value: i64 = row.get(6).unwrap(); + let is_change: bool = row.get(7).unwrap(); + match (output_pool, output_index) { + (0, 0) => { + assert_eq!(from_account, None); + assert_eq!(to_account, Some(0)); + assert_eq!(to_address, Some("taddr_tx3".to_string())); + assert_eq!(value, 1); + assert!(!is_change); + } + other => { + panic!("Unexpected output pool and index for tx {}: {:?}.", tx, other); + } + } + } + assert_eq!(row_count, 1); } } } From b6fd844505312467d4e578251c70ad812852b717 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 14 Apr 2023 11:08:06 -0600 Subject: [PATCH 4/5] Fix Rust formatting errors. --- .../src/wallet/init/migrations/v_transactions_net.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs index dcfbe9c8b..eafecd35b 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs @@ -539,7 +539,10 @@ mod tests { assert!(!is_change); } other => { - panic!("Unexpected output pool and index for tx {}: {:?}.", tx, other); + panic!( + "Unexpected output pool and index for tx {}: {:?}.", + tx, other + ); } } } @@ -570,7 +573,10 @@ mod tests { assert!(!is_change); } other => { - panic!("Unexpected output pool and index for tx {}: {:?}.", tx, other); + panic!( + "Unexpected output pool and index for tx {}: {:?}.", + tx, other + ); } } } From 7d18d1d02a583b9cef2ba246a520ad1bf0bbfa9f Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 14 Apr 2023 11:59:45 -0600 Subject: [PATCH 5/5] Adds documentation for the `v_transactions` and `v_tx_outputs` views. This change also settles on `account_value_delta` as the name of the column in `v_transactions` that describes the transaction's effect on the value of the associated account. --- zcash_client_sqlite/src/wallet.rs | 57 +++++++++++++++++++ zcash_client_sqlite/src/wallet/init.rs | 2 +- .../init/migrations/v_transactions_net.rs | 16 +++--- 3 files changed, 66 insertions(+), 9 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 071d0acef..ba8178404 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -6,6 +6,63 @@ //! //! [`WalletRead`]: zcash_client_backend::data_api::WalletRead //! [`WalletWrite`]: zcash_client_backend::data_api::WalletWrite +//! +//! # Views +//! +//! The wallet database exposes the following views as part of its public API: +//! +//! ## `v_transactions` +//! +//! This view exposes the history of transactions that affect the balance of each account in the +//! wallet. A transaction may be represented by multiple rows in this view, one for each account in +//! the wallet that contributes funds to or receives funds from the transaction in question. Each +//! row of the view contains: +//! - `account_balance_delta`: the net effect of the transaction on the associated account's +//! balance. This value is positive when funds are received by the account, and negative when the +//! balance of the account decreases due to a spend. +//! - `fee_paid`: the total fee paid to send the transaction, as a positive value. This fee is +//! associated with the transaction (similar to e.g. `txid` or `mined_height`), and not with any +//! specific account involved with that transaction. ` If multiple rows exist for a single +//! transaction, this fee amount will be repeated for each such row. Therefore, if more than one +//! of the wallet's accounts is involved with the transaction, this fee should be considered only +//! once in determining the total value sent from the wallet as a whole. +//! +//! ### Seed Phrase with Single Account +//! +//! In the case that the seed phrase for in this wallet has only been used to create a single +//! account, this view will contain one row per transaction, in the case that +//! `account_balance_delta` is negative, it is usually safe to add `fee_paid` back to the +//! `account_balance_delta` value to determine the amount sent to addresses outside the wallet. +//! +//! ### Seed Phrase with Multiple Accounts +//! +//! In the case that the seed phrase for in this wallet has been used to create multiple accounts, +//! this view may contain multiple rows per transaction, one for each account involved. In this +//! case, the total amount sent to addresses outside the wallet can usually be calculated by +//! grouping rows by `id_tx` and then using `SUM(account_balance_delta) + MAX(fee_paid)`. +//! +//! ### Imported Seed Phrases +//! +//! If a seed phrase is imported, and not every account associated with it is loaded into the +//! wallet, this view may show partial information about some transactions. In particular, any +//! computation that involves both `account_balance_delta` and `fee_paid` is likely to be +//! inaccurate. +//! +//! ## `v_tx_outputs` +//! +//! This view exposes the history of transaction outputs received by and sent from the wallet, +//! keyed by transaction ID, pool type, and output index. The contents of this view are useful for +//! producing a detailed report of the effects of a transaction. Each row of this view contains: +//! - `from_account` for sent outputs, the account from which the value was sent. +//! - `to_account` in the case that the output was received by an account in the wallet, the +//! identifier for the account receiving the funds. +//! - `to_address` the address to which an output was sent, or the address at which value was +//! received in the case of received transparent funds. +//! - `value` the value of the output. This is always a positive number, for both sent and received +//! outputs. +//! - `is_change` a boolean flag indicating whether this is a change output belonging to the +//! wallet. +//! - `memo` the shielded memo associated with the output, if any. use group::ff::PrimeField; use rusqlite::{named_params, OptionalExtension, ToSql}; diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 6ef658c11..d6820af7f 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -522,7 +522,7 @@ mod tests { transactions.txid AS txid, transactions.expiry_height AS expiry_height, transactions.raw AS raw, - SUM(notes.value) AS net_transfer, + 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, diff --git a/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs index eafecd35b..10c3a26a9 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs @@ -175,7 +175,7 @@ impl RusqliteMigration for Migration { transactions.txid AS txid, transactions.expiry_height AS expiry_height, transactions.raw AS raw, - SUM(notes.value) AS net_transfer, + 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, @@ -394,7 +394,7 @@ mod tests { let mut q = db_data .conn .prepare( - "SELECT account_id, id_tx, net_transfer, has_change, memo_count, sent_note_count, received_note_count + "SELECT account_id, id_tx, account_balance_delta, has_change, memo_count, sent_note_count, received_note_count FROM v_transactions", ) .unwrap(); @@ -404,38 +404,38 @@ mod tests { row_count += 1; let account: i64 = row.get(0).unwrap(); let tx: i64 = row.get(1).unwrap(); - let net_transfer: i64 = row.get(2).unwrap(); + let account_balance_delta: i64 = row.get(2).unwrap(); let has_change: bool = row.get(3).unwrap(); let memo_count: i64 = row.get(4).unwrap(); let sent_note_count: i64 = row.get(5).unwrap(); let received_note_count: i64 = row.get(6).unwrap(); match (account, tx) { (0, 0) => { - assert_eq!(net_transfer, 7); + assert_eq!(account_balance_delta, 7); assert!(!has_change); assert_eq!(memo_count, 0); } (0, 1) => { - assert_eq!(net_transfer, -5); + assert_eq!(account_balance_delta, -5); assert!(has_change); assert_eq!(memo_count, 1); } (0, 2) => { - assert_eq!(net_transfer, 0); + assert_eq!(account_balance_delta, 0); assert!(has_change); assert_eq!(memo_count, 0); assert_eq!(sent_note_count, 1); assert_eq!(received_note_count, 1); } (1, 2) => { - assert_eq!(net_transfer, 1); + assert_eq!(account_balance_delta, 1); assert!(!has_change); assert_eq!(memo_count, 0); assert_eq!(sent_note_count, 0); assert_eq!(received_note_count, 1); } (0, 3) => { - assert_eq!(net_transfer, 1); + assert_eq!(account_balance_delta, 1); assert!(!has_change); assert_eq!(memo_count, 0); assert_eq!(sent_note_count, 0);