diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 606c09635..fc09fee04 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -451,12 +451,14 @@ impl SentTransactionOutput { pub fn value(&self) -> Amount { self.value } - /// Returns the memo that was attached to the output, if any. + /// Returns the memo that was attached to the output, if any. This will only be `None` + /// for transparent outputs. pub fn memo(&self) -> Option<&MemoBytes> { self.memo.as_ref() } - /// Returns t decrypted note, if the sent output belongs to this wallet + /// Returns the account to which change (or wallet-internal value in the case of a shielding + /// transaction) was sent, along with the change note. pub fn sapling_change_to(&self) -> Option<&(AccountId, sapling::Note)> { self.sapling_change_to.as_ref() } diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 630ff80da..2d09231e8 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -545,30 +545,29 @@ where for payment in proposal.transaction_request().payments() { match &payment.recipient_address { RecipientAddress::Unified(ua) => { + let memo = payment + .memo + .as_ref() + .map_or_else(MemoBytes::empty, |m| m.clone()); builder.add_sapling_output( external_ovk, *ua.sapling().expect("TODO: Add Orchard support to builder"), payment.amount, - payment.memo.clone().unwrap_or_else(MemoBytes::empty), + memo.clone(), )?; sapling_output_meta.push(( Recipient::Unified(ua.clone(), PoolType::Shielded(ShieldedProtocol::Sapling)), payment.amount, - payment.memo.clone(), + Some(memo), )); } RecipientAddress::Shielded(addr) => { - builder.add_sapling_output( - external_ovk, - *addr, - payment.amount, - payment.memo.clone().unwrap_or_else(MemoBytes::empty), - )?; - sapling_output_meta.push(( - Recipient::Sapling(*addr), - payment.amount, - payment.memo.clone(), - )); + let memo = payment + .memo + .as_ref() + .map_or_else(MemoBytes::empty, |m| m.clone()); + builder.add_sapling_output(external_ovk, *addr, payment.amount, memo.clone())?; + sapling_output_meta.push((Recipient::Sapling(*addr), payment.amount, Some(memo))); } RecipientAddress::Transparent(to) => { if payment.memo.is_some() { @@ -584,11 +583,14 @@ where for change_value in proposal.balance().proposed_change() { match change_value { ChangeValue::Sapling(amount) => { + let memo = change_memo + .as_ref() + .map_or_else(MemoBytes::empty, |m| m.clone()); builder.add_sapling_output( internal_ovk(), dfvk.change_address().1, *amount, - MemoBytes::empty(), + memo.clone(), )?; sapling_output_meta.push(( Recipient::InternalAccount( @@ -596,7 +598,7 @@ where PoolType::Shielded(ShieldedProtocol::Sapling), ), *amount, - change_memo.clone(), + Some(memo), )) } } diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index aede6ff26..3c97d60a9 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -240,7 +240,7 @@ impl, P: consensus::Parameters> WalletRead for W } fn get_transaction(&self, id_tx: i64) -> Result { - wallet::get_transaction(self.conn.borrow(), &self.params, id_tx) + wallet::get_transaction(self.conn.borrow(), &self.params, id_tx).map(|(_, tx)| tx) } fn get_sapling_nullifiers( diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index c7fb373c7..e49901b78 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -129,8 +129,14 @@ pub(crate) fn pool_code(pool_type: PoolType) -> i64 { } pub(crate) fn memo_repr(memo: Option<&MemoBytes>) -> Option<&[u8]> { - memo.filter(|m| *m != &MemoBytes::empty()) - .map(|m| m.as_slice()) + memo.map(|m| { + if m == &MemoBytes::empty() { + // we store the empty memo as a single 0xf6 byte + &[0xf6] + } else { + m.as_slice() + } + }) } pub(crate) fn get_max_account_id( @@ -493,11 +499,16 @@ pub(crate) fn get_received_memo( } /// Looks up a transaction by its internal database identifier. +/// +/// Returns the decoded transaction, along with the block height that was used in its decoding. +/// This is either the block height at which the transaction was mined, or the expiry height if the +/// wallet created the transaction but the transaction has not yet been mined from the perspective +/// of the wallet. pub(crate) fn get_transaction( conn: &rusqlite::Connection, params: &P, id_tx: i64, -) -> Result { +) -> Result<(BlockHeight, Transaction), SqliteClientError> { let (tx_bytes, block_height, expiry_height): ( Vec<_>, Option, @@ -530,6 +541,7 @@ pub(crate) fn get_transaction( block_height.or_else(|| expiry_height.filter(|h| h > &BlockHeight::from(0))) { Transaction::read(&tx_bytes[..], BranchId::for_height(params, height)) + .map(|t| (height, t)) .map_err(SqliteClientError::from) } else { let tx_data = Transaction::read(&tx_bytes[..], BranchId::Sprout) @@ -549,6 +561,7 @@ pub(crate) fn get_transaction( tx_data.orchard_bundle().cloned(), ) .freeze() + .map(|t| (expiry_height, t)) .map_err(SqliteClientError::from) } else { Err(SqliteClientError::CorruptedData( @@ -1262,7 +1275,7 @@ pub(crate) fn insert_sent_output( ":to_address": &to_address, ":to_account": &to_account, ":value": &i64::from(output.value()), - ":memo": output.memo().filter(|m| *m != &MemoBytes::empty()).map(|m| m.as_slice()), + ":memo": memo_repr(output.memo()) ]; stmt_insert_sent_output.execute(sql_args)?; diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index a589e9788..8ba478c88 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -574,8 +574,9 @@ mod tests { ELSE 1 END AS received_count, CASE - WHEN sapling_received_notes.memo IS NULL THEN 0 - ELSE 1 + 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 UNION @@ -606,8 +607,9 @@ mod tests { COUNT(DISTINCT sent_notes.id_note) as sent_notes, SUM( CASE - WHEN sent_notes.memo IS NULL THEN 0 - ELSE 1 + WHEN (sent_notes.memo IS NULL OR sent_notes.memo = X'F6') + THEN 0 + ELSE 1 END ) AS memo_count FROM sent_notes diff --git a/zcash_client_sqlite/src/wallet/init/migrations.rs b/zcash_client_sqlite/src/wallet/init/migrations.rs index 7052dea9d..25d29c68c 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations.rs @@ -4,6 +4,7 @@ mod addresses_table; mod initial_setup; mod nullifier_map; mod received_notes_nullable_nf; +mod sapling_memo_consistency; mod sent_notes_to_internal; mod shardtree_support; mod ufvk_support; @@ -22,19 +23,19 @@ pub(super) fn all_migrations( ) -> Vec>> { // initial_setup // / \ - // utxos_table ufvk_support ---------- - // \ \ \ - // \ addresses_table sent_notes_to_internal - // \ / / - // add_utxo_account / - // \ / - // add_transaction_views - // / - // v_transactions_net - // / - // received_notes_nullable_nf - // / \ - // shardtree_support nullifier_map + // utxos_table ufvk_support --------- + // \ | \ + // \ addresses_table sent_notes_to_internal + // \ / / + // add_utxo_account / + // \ / + // add_transaction_views + // | + // v_transactions_net + // | + // received_notes_nullable_nf + // / | \ + // shardtree_support nullifier_map sapling_memo_consistency vec![ Box::new(initial_setup::Migration {}), Box::new(utxos_table::Migration {}), @@ -54,5 +55,8 @@ pub(super) fn all_migrations( Box::new(received_notes_nullable_nf::Migration), Box::new(shardtree_support::Migration), Box::new(nullifier_map::Migration), + Box::new(sapling_memo_consistency::Migration { + params: params.clone(), + }), ] } diff --git a/zcash_client_sqlite/src/wallet/init/migrations/sapling_memo_consistency.rs b/zcash_client_sqlite/src/wallet/init/migrations/sapling_memo_consistency.rs new file mode 100644 index 000000000..4cfc98382 --- /dev/null +++ b/zcash_client_sqlite/src/wallet/init/migrations/sapling_memo_consistency.rs @@ -0,0 +1,209 @@ +//! This migration reads the wallet's raw transaction data and updates the `sent_notes` table to +//! ensure that memo entries are consistent with the decrypted transaction's outputs. The empty +//! memo is now consistently represented as a single `0xf6` byte. + +use std::collections::{BTreeMap, HashMap, HashSet}; + +use rusqlite::named_params; +use schemer_rusqlite::RusqliteMigration; +use uuid::Uuid; +use zcash_client_backend::{decrypt_transaction, keys::UnifiedFullViewingKey}; +use zcash_primitives::{consensus, zip32::AccountId}; + +use crate::{ + error::SqliteClientError, + wallet::{get_transaction, init::WalletMigrationError, memo_repr}, +}; + +use super::received_notes_nullable_nf; + +pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0x7029b904_6557_4aa1_9da5_6904b65d2ba5); + +pub(super) struct Migration

{ + pub(super) params: P, +} + +impl

schemer::Migration for Migration

{ + fn id(&self) -> Uuid { + MIGRATION_ID + } + + fn dependencies(&self) -> HashSet { + [received_notes_nullable_nf::MIGRATION_ID] + .into_iter() + .collect() + } + + fn description(&self) -> &'static str { + "This migration reads the wallet's raw transaction data and updates the `sent_notes` table to + ensure that memo entries are consistent with the decrypted transaction's outputs. The empty + memo is now consistently represented as a single `0xf6` byte." + } +} + +impl RusqliteMigration for Migration

{ + type Error = WalletMigrationError; + + fn up(&self, transaction: &rusqlite::Transaction) -> Result<(), Self::Error> { + let mut stmt_raw_tx = transaction.prepare( + "SELECT DISTINCT sent_notes.tx, accounts.account, accounts.ufvk + FROM sent_notes + JOIN accounts ON sent_notes.from_account = accounts.account + JOIN transactions ON transactions.id_tx = sent_notes.tx + WHERE transactions.raw IS NOT NULL", + )?; + + let mut rows = stmt_raw_tx.query([])?; + + let mut tx_sent_notes: BTreeMap> = + BTreeMap::new(); + while let Some(row) = rows.next()? { + let id_tx: i64 = row.get(0)?; + let account: u32 = row.get(1)?; + let ufvk_str: String = row.get(2)?; + let ufvk = UnifiedFullViewingKey::decode(&self.params, &ufvk_str).map_err(|e| { + WalletMigrationError::CorruptedData(format!( + "Could not decode unified full viewing key for account {}: {:?}", + account, e + )) + })?; + + tx_sent_notes + .entry(id_tx) + .or_default() + .insert(AccountId::from(account), ufvk); + } + + let mut stmt_update_sent_memo = transaction.prepare( + "UPDATE sent_notes + SET memo = :memo + WHERE tx = :id_tx + AND output_index = :output_index", + )?; + + for (id_tx, ufvks) in tx_sent_notes { + let (block_height, tx) = + get_transaction(transaction, &self.params, id_tx).map_err(|err| match err { + SqliteClientError::CorruptedData(msg) => { + WalletMigrationError::CorruptedData(msg) + } + SqliteClientError::DbError(err) => WalletMigrationError::DbError(err), + other => WalletMigrationError::CorruptedData(format!( + "An error was encountered decoding transaction data: {:?}", + other + )), + })?; + + let decrypted_outputs = decrypt_transaction(&self.params, block_height, &tx, &ufvks); + for d_out in decrypted_outputs { + stmt_update_sent_memo.execute(named_params![ + ":id_tx": id_tx, + ":output_index": d_out.index, + ":memo": memo_repr(Some(&d_out.memo)) + ])?; + } + } + + // Update the `v_transactions` view to avoid counting the empty memo as a memo + transaction.execute_batch( + "DROP VIEW v_transactions; + CREATE VIEW v_transactions AS + WITH + notes AS ( + SELECT sapling_received_notes.account AS account_id, + sapling_received_notes.tx AS id_tx, + 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 + 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 sapling_received_notes.account AS account_id, + sapling_received_notes.spent AS id_tx, + 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 + WHERE sapling_received_notes.spent IS NOT NULL + ), + sent_note_counts AS ( + 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 OR sent_notes.memo = X'F6') + THEN 0 + ELSE 1 + END + ) AS memo_count + FROM sent_notes + 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 sapling_received_notes.is_change IS NULL + OR sapling_received_notes.is_change = 0 + 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 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 <= 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, _: &rusqlite::Transaction) -> Result<(), Self::Error> { + panic!("Reversing this migration is not supported."); + } +} diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index 335782307..88cf3b209 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -371,7 +371,7 @@ pub(crate) fn put_received_note( #[cfg(test)] #[allow(deprecated)] pub(crate) mod tests { - use std::num::NonZeroU32; + use std::{convert::Infallible, num::NonZeroU32}; use rusqlite::Connection; use secrecy::Secret; @@ -383,8 +383,13 @@ pub(crate) mod tests { block::BlockHash, consensus::{BlockHeight, BranchId}, legacy::TransparentAddress, + memo::Memo, sapling::{note_encryption::try_sapling_output_recovery, prover::TxProver}, - transaction::{components::Amount, fees::zip317::FeeRule as Zip317FeeRule, Transaction}, + transaction::{ + components::Amount, + fees::{fixed::FeeRule as FixedFeeRule, zip317::FeeRule as Zip317FeeRule}, + Transaction, + }, zip32::{sapling::ExtendedSpendingKey, Scope}, }; @@ -394,13 +399,17 @@ pub(crate) mod tests { self, chain::scan_cached_blocks, error::Error, - wallet::{create_spend_to_address, input_selection::GreedyInputSelector, spend}, + wallet::{ + create_proposed_transaction, create_spend_to_address, + input_selection::GreedyInputSelector, propose_transfer, spend, + }, WalletRead, WalletWrite, }, - fees::{zip317, DustOutputPolicy}, + decrypt_transaction, + fees::{fixed, zip317, DustOutputPolicy}, keys::UnifiedSpendingKey, wallet::OvkPolicy, - zip321::{Payment, TransactionRequest}, + zip321::{self, Payment, TransactionRequest}, }; use crate::{ @@ -413,21 +422,17 @@ pub(crate) mod tests { get_balance, get_balance_at, init::{init_blocks_table, init_wallet_db}, }, - AccountId, BlockDb, WalletDb, + AccountId, BlockDb, NoteId, WalletDb, }; #[cfg(feature = "transparent-inputs")] use { zcash_client_backend::{ - data_api::wallet::shield_transparent_funds, fees::fixed, - wallet::WalletTransparentOutput, + data_api::wallet::shield_transparent_funds, wallet::WalletTransparentOutput, }, zcash_primitives::{ memo::MemoBytes, - transaction::{ - components::{amount::NonNegativeAmount, OutPoint, TxOut}, - fees::fixed::FeeRule as FixedFeeRule, - }, + transaction::components::{amount::NonNegativeAmount, OutPoint, TxOut}, }, }; @@ -440,6 +445,166 @@ pub(crate) mod tests { } } + #[test] + fn send_proposed_transfer() { + let cache_file = NamedTempFile::new().unwrap(); + let db_cache = BlockDb(Connection::open(cache_file.path()).unwrap()); + init_cache_database(&db_cache).unwrap(); + + let data_file = NamedTempFile::new().unwrap(); + let mut db_data = WalletDb::for_path(data_file.path(), tests::network()).unwrap(); + init_wallet_db(&mut db_data, None).unwrap(); + + // Add an account to the wallet + let seed = Secret::new([0u8; 32].to_vec()); + let (account, usk) = db_data.create_account(&seed).unwrap(); + let dfvk = usk.sapling().to_diversifiable_full_viewing_key(); + + // Add funds to the wallet in a single note + let value = Amount::from_u64(60000).unwrap(); + let (cb, _) = fake_compact_block( + sapling_activation_height(), + BlockHash([0; 32]), + &dfvk, + AddressType::DefaultExternal, + value, + 0, + ); + insert_into_cache(&db_cache, &cb); + scan_cached_blocks( + &tests::network(), + &db_cache, + &mut db_data, + sapling_activation_height(), + 1, + ) + .unwrap(); + + // Verified balance matches total balance + let (_, anchor_height) = db_data + .get_target_and_anchor_heights(NonZeroU32::new(1).unwrap()) + .unwrap() + .unwrap(); + assert_eq!( + get_balance(&db_data.conn, AccountId::from(0)).unwrap(), + value + ); + assert_eq!( + get_balance_at(&db_data.conn, AccountId::from(0), anchor_height).unwrap(), + value + ); + + let to_extsk = ExtendedSpendingKey::master(&[]); + let to: RecipientAddress = to_extsk.default_address().1.into(); + let request = zip321::TransactionRequest::new(vec![Payment { + recipient_address: to, + amount: Amount::from_u64(10000).unwrap(), + memo: None, // this should result in the creation of an empty memo + label: None, + message: None, + other_params: vec![], + }]) + .unwrap(); + + let fee_rule = FixedFeeRule::standard(); + let change_strategy = fixed::SingleOutputChangeStrategy::new(fee_rule); + let input_selector = + &GreedyInputSelector::new(change_strategy, DustOutputPolicy::default()); + let proposal_result = propose_transfer::<_, _, _, Infallible>( + &mut db_data, + &tests::network(), + account, + input_selector, + request, + NonZeroU32::new(1).unwrap(), + ); + assert_matches!(proposal_result, Ok(_)); + + let change_memo = "Test change memo".parse::().unwrap(); + let create_proposed_result = create_proposed_transaction::<_, _, Infallible, _>( + &mut db_data, + &tests::network(), + test_prover(), + &usk, + OvkPolicy::Sender, + proposal_result.unwrap(), + NonZeroU32::new(1).unwrap(), + Some(change_memo.clone().into()), + ); + assert_matches!(create_proposed_result, Ok(_)); + + let sent_tx_id = create_proposed_result.unwrap(); + + // Verify that the sent transaction was stored and that we can decrypt the memos + let tx = db_data + .get_transaction(sent_tx_id) + .expect("Created transaction was stored."); + let ufvks = [(account, usk.to_unified_full_viewing_key())] + .into_iter() + .collect(); + let decrypted_outputs = decrypt_transaction( + &tests::network(), + sapling_activation_height() + 1, + &tx, + &ufvks, + ); + assert_eq!(decrypted_outputs.len(), 2); + + let mut found_tx_change_memo = false; + let mut found_tx_empty_memo = false; + for output in decrypted_outputs { + if output.memo == change_memo.clone().into() { + found_tx_change_memo = true + } + if output.memo == Memo::Empty.into() { + found_tx_empty_memo = true + } + } + assert!(found_tx_change_memo); + assert!(found_tx_empty_memo); + + // Verify that the stored sent notes match what we're expecting + let mut stmt_sent_notes = db_data + .conn + .prepare("SELECT id_note FROM sent_notes WHERE tx = ?") + .unwrap(); + + let sent_note_ids = stmt_sent_notes + .query(rusqlite::params![sent_tx_id]) + .unwrap() + .mapped(|row| row.get::<_, i64>(0).map(NoteId::SentNoteId)) + .collect::, _>>() + .unwrap(); + + assert_eq!(sent_note_ids.len(), 2); + + // The sent memo should be the empty memo for the sent output, and the + // change output's memo should be as specified. + let mut found_sent_change_memo = false; + let mut found_sent_empty_memo = false; + for sent_note_id in sent_note_ids { + match db_data + .get_memo(sent_note_id) + .expect("Note id is valid") + .as_ref() + { + Some(m) if m == &change_memo => { + found_sent_change_memo = true; + } + Some(m) if m == &Memo::Empty => { + found_sent_empty_memo = true; + } + Some(other) => panic!("Unexpected memo value: {:?}", other), + None => panic!("Memo should not be stored as NULL"), + } + } + assert!(found_sent_change_memo); + assert!(found_sent_empty_memo); + + // Check that querying for a nonexistent sent note returns an error + assert_matches!(db_data.get_memo(NoteId::SentNoteId(12345)), Err(_)); + } + #[test] fn create_to_address_fails_on_incorrect_usk() { let data_file = NamedTempFile::new().unwrap();