From bcd8617721176083a4d336e5b5579487fb6dbd0e Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 23 Oct 2024 08:41:02 -0600 Subject: [PATCH 1/5] zcash_client_sqlite: Add a test demonstrating the bug described in #1571 The update to a shielding transaction causes the information that the output is considered change to be lost. This happens because the scanning process does not have access to any information about the inputs to the transaction, and so it does not recognize the output as change. --- zcash_client_backend/src/data_api/testing/pool.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index e818c3e00..a104e8c4c 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -1797,6 +1797,18 @@ where assert_eq!(tx.received_note_count(), 0); assert_eq!(tx.sent_note_count(), 0); assert!(tx.is_shielding()); + + // Generate and scan the block including the transaction + let (h, _) = st.generate_next_block_including(*txids.first()); + st.scan_cached_blocks(h, 1); + + // Ensure that the transaction metadata is still correct after the update produced by scanning. + let tx = st.get_tx_from_history(*txids.first()).unwrap().unwrap(); + assert_eq!(tx.spent_note_count(), 1); + assert!(tx.has_change()); + assert_eq!(tx.received_note_count(), 0); + assert_eq!(tx.sent_note_count(), 0); + assert!(tx.is_shielding()); } // FIXME: This requires fixes to the test framework. From 05ee294c45c64a83043592240c968aed10065563 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 23 Oct 2024 09:42:13 -0600 Subject: [PATCH 2/5] zcash_client_sqlite: Make it possible to initialize tests to a specific migration state. --- zcash_client_sqlite/src/lib.rs | 18 +++--- zcash_client_sqlite/src/testing/db.rs | 36 +++++++++-- zcash_client_sqlite/src/testing/pool.rs | 60 +++++++++---------- zcash_client_sqlite/src/wallet.rs | 10 ++-- zcash_client_sqlite/src/wallet/init.rs | 4 +- zcash_client_sqlite/src/wallet/scanning.rs | 16 ++--- zcash_client_sqlite/src/wallet/transparent.rs | 4 +- 7 files changed, 88 insertions(+), 60 deletions(-) diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index de70c5955..f11022927 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -1910,7 +1910,7 @@ mod tests { #[test] fn validate_seed() { let st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .with_account_from_sapling_activation(BlockHash([0; 32])) .build(); let account = st.test_account().unwrap(); @@ -1940,7 +1940,7 @@ mod tests { #[test] pub(crate) fn get_next_available_address() { let mut st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .with_account_from_sapling_activation(BlockHash([0; 32])) .build(); let account = st.test_account().cloned().unwrap(); @@ -1962,7 +1962,7 @@ mod tests { #[test] pub(crate) fn import_account_hd_0() { let st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .with_account_from_sapling_activation(BlockHash([0; 32])) .set_account_index(zip32::AccountId::ZERO) .build(); @@ -1974,7 +1974,7 @@ mod tests { #[test] pub(crate) fn import_account_hd_1_then_2() { let mut st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .build(); let birthday = AccountBirthday::from_parts( @@ -2065,7 +2065,7 @@ mod tests { #[test] pub(crate) fn import_account_hd_1_then_conflicts() { let mut st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .build(); let birthday = AccountBirthday::from_parts( @@ -2097,7 +2097,7 @@ mod tests { #[test] pub(crate) fn import_account_ufvk_then_conflicts() { let mut st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .build(); let birthday = AccountBirthday::from_parts( @@ -2142,7 +2142,7 @@ mod tests { #[test] pub(crate) fn create_account_then_conflicts() { let mut st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .build(); let birthday = AccountBirthday::from_parts( @@ -2175,7 +2175,7 @@ mod tests { use crate::testing::BlockCache; let st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .with_block_cache(BlockCache::new()) .with_account_from_sapling_activation(BlockHash([0; 32])) .build(); @@ -2208,7 +2208,7 @@ mod tests { use crate::testing::FsBlockCache; let mut st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .with_block_cache(FsBlockCache::new()) .build(); diff --git a/zcash_client_sqlite/src/testing/db.rs b/zcash_client_sqlite/src/testing/db.rs index f972fdfde..61831cc18 100644 --- a/zcash_client_sqlite/src/testing/db.rs +++ b/zcash_client_sqlite/src/testing/db.rs @@ -2,6 +2,7 @@ use ambassador::Delegate; use rusqlite::Connection; use std::collections::HashMap; use std::num::NonZeroU32; +use uuid::Uuid; use tempfile::NamedTempFile; @@ -31,7 +32,11 @@ use zcash_primitives::{ }; use zcash_protocol::{consensus::BlockHeight, local_consensus::LocalNetwork, memo::Memo}; -use crate::{error::SqliteClientError, wallet::init::init_wallet_db, AccountId, WalletDb}; +use crate::{ + error::SqliteClientError, + wallet::init::{init_wallet_db, init_wallet_db_internal}, + AccountId, WalletDb, +}; #[cfg(feature = "transparent-inputs")] use { @@ -142,7 +147,26 @@ unsafe fn run_sqlite3>(db_path: S, command: &str) { eprintln!("------"); } -pub(crate) struct TestDbFactory; +pub(crate) struct TestDbFactory { + target_migrations: Option>, +} + +impl TestDbFactory { + #[allow(dead_code)] + pub(crate) fn new(target_migrations: Vec) -> Self { + Self { + target_migrations: Some(target_migrations), + } + } +} + +impl Default for TestDbFactory { + fn default() -> Self { + Self { + target_migrations: Default::default(), + } + } +} impl DataStoreFactory for TestDbFactory { type Error = (); @@ -154,7 +178,11 @@ impl DataStoreFactory for TestDbFactory { fn new_data_store(&self, network: LocalNetwork) -> Result { let data_file = NamedTempFile::new().unwrap(); let mut db_data = WalletDb::for_path(data_file.path(), network).unwrap(); - init_wallet_db(&mut db_data, None).unwrap(); + if let Some(migrations) = &self.target_migrations { + init_wallet_db_internal(&mut db_data, None, &migrations, true).unwrap(); + } else { + init_wallet_db(&mut db_data, None).unwrap(); + } Ok(TestDb::from_parts(db_data, data_file)) } } @@ -166,7 +194,7 @@ impl Reset for TestDb { let network = *st.network(); let old_db = std::mem::replace( st.wallet_mut(), - TestDbFactory.new_data_store(network).unwrap(), + TestDbFactory::default().new_data_store(network).unwrap(), ); old_db.take_data_file() } diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 35f3d9a06..9d5d0a3c6 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -31,14 +31,14 @@ impl ShieldedPoolPersistence for OrchardPoolTester { pub(crate) fn send_single_step_proposed_transfer() { zcash_client_backend::data_api::testing::pool::send_single_step_proposed_transfer::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } pub(crate) fn send_with_multiple_change_outputs() { zcash_client_backend::data_api::testing::pool::send_with_multiple_change_outputs::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } @@ -46,7 +46,7 @@ pub(crate) fn send_with_multiple_change_outputs() { #[cfg(feature = "transparent-inputs")] pub(crate) fn send_multi_step_proposed_transfer() { zcash_client_backend::data_api::testing::pool::send_multi_step_proposed_transfer::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), |e, account_id, expected_bad_index| { matches!( @@ -60,7 +60,7 @@ pub(crate) fn send_multi_step_proposed_transfer() { #[cfg(feature = "transparent-inputs")] pub(crate) fn proposal_fails_if_not_all_ephemeral_outputs_consumed() { zcash_client_backend::data_api::testing::pool::proposal_fails_if_not_all_ephemeral_outputs_consumed::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } @@ -68,48 +68,48 @@ pub(crate) fn proposal_fails_if_not_all_ephemeral_outputs_consumed() { zcash_client_backend::data_api::testing::pool::create_to_address_fails_on_incorrect_usk::( - TestDbFactory, + TestDbFactory::default(), ) } #[allow(deprecated)] pub(crate) fn proposal_fails_with_no_blocks() { zcash_client_backend::data_api::testing::pool::proposal_fails_with_no_blocks::( - TestDbFactory, + TestDbFactory::default(), ) } pub(crate) fn spend_fails_on_unverified_notes() { zcash_client_backend::data_api::testing::pool::spend_fails_on_unverified_notes::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } pub(crate) fn spend_fails_on_locked_notes() { zcash_client_backend::data_api::testing::pool::spend_fails_on_locked_notes::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } pub(crate) fn ovk_policy_prevents_recovery_from_chain() { zcash_client_backend::data_api::testing::pool::ovk_policy_prevents_recovery_from_chain::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } pub(crate) fn spend_succeeds_to_t_addr_zero_change() { zcash_client_backend::data_api::testing::pool::spend_succeeds_to_t_addr_zero_change::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } pub(crate) fn change_note_spends_succeed() { zcash_client_backend::data_api::testing::pool::change_note_spends_succeed::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } @@ -118,15 +118,15 @@ pub(crate) fn external_address_change_spends_detected_in_restore_from_seed< T: ShieldedPoolTester, >() { zcash_client_backend::data_api::testing::pool::external_address_change_spends_detected_in_restore_from_seed::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } #[allow(dead_code)] pub(crate) fn zip317_spend() { - zcash_client_backend::data_api::testing::pool::zip317_spend::( - TestDbFactory, + zcash_client_backend::data_api::testing::pool::zip317_spend::( + TestDbFactory::default(), BlockCache::new(), ) } @@ -134,7 +134,7 @@ pub(crate) fn zip317_spend() { #[cfg(feature = "transparent-inputs")] pub(crate) fn shield_transparent() { zcash_client_backend::data_api::testing::pool::shield_transparent::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } @@ -143,14 +143,14 @@ pub(crate) fn shield_transparent() { #[allow(dead_code)] pub(crate) fn birthday_in_anchor_shard() { zcash_client_backend::data_api::testing::pool::birthday_in_anchor_shard::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } pub(crate) fn checkpoint_gaps() { zcash_client_backend::data_api::testing::pool::checkpoint_gaps::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } @@ -158,7 +158,7 @@ pub(crate) fn checkpoint_gaps() { #[cfg(feature = "orchard")] pub(crate) fn pool_crossing_required() { zcash_client_backend::data_api::testing::pool::pool_crossing_required::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } @@ -166,7 +166,7 @@ pub(crate) fn pool_crossing_required() { zcash_client_backend::data_api::testing::pool::fully_funded_fully_private::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } @@ -174,7 +174,7 @@ pub(crate) fn fully_funded_fully_private() { zcash_client_backend::data_api::testing::pool::fully_funded_send_to_t::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } @@ -182,7 +182,7 @@ pub(crate) fn fully_funded_send_to_t() { zcash_client_backend::data_api::testing::pool::multi_pool_checkpoint::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } @@ -193,14 +193,14 @@ pub(crate) fn multi_pool_checkpoints_with_pruning< P1: ShieldedPoolTester, >() { zcash_client_backend::data_api::testing::pool::multi_pool_checkpoints_with_pruning::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } pub(crate) fn valid_chain_states() { zcash_client_backend::data_api::testing::pool::valid_chain_states::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } @@ -209,35 +209,35 @@ pub(crate) fn valid_chain_states() { #[allow(dead_code)] pub(crate) fn invalid_chain_cache_disconnected() { zcash_client_backend::data_api::testing::pool::invalid_chain_cache_disconnected::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } pub(crate) fn data_db_truncation() { zcash_client_backend::data_api::testing::pool::data_db_truncation::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } pub(crate) fn reorg_to_checkpoint() { zcash_client_backend::data_api::testing::pool::reorg_to_checkpoint::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } pub(crate) fn scan_cached_blocks_allows_blocks_out_of_order() { zcash_client_backend::data_api::testing::pool::scan_cached_blocks_allows_blocks_out_of_order::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } pub(crate) fn scan_cached_blocks_finds_received_notes() { zcash_client_backend::data_api::testing::pool::scan_cached_blocks_finds_received_notes::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } @@ -245,7 +245,7 @@ pub(crate) fn scan_cached_blocks_finds_received_notes() { // TODO: This test can probably be entirely removed, as the following test duplicates it entirely. pub(crate) fn scan_cached_blocks_finds_change_notes() { zcash_client_backend::data_api::testing::pool::scan_cached_blocks_finds_change_notes::( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ) } @@ -254,5 +254,5 @@ pub(crate) fn scan_cached_blocks_detects_spends_out_of_order(TestDbFactory, BlockCache::new()) + >(TestDbFactory::default(), BlockCache::new()) } diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index edd3dae01..00618e9d1 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -3586,7 +3586,7 @@ mod tests { #[test] fn empty_database_has_no_balance() { let st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .with_account_from_sapling_activation(BlockHash([0; 32])) .build(); let account = st.test_account().unwrap(); @@ -3616,7 +3616,7 @@ mod tests { #[test] fn get_default_account_index() { let st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .with_account_from_sapling_activation(BlockHash([0; 32])) .build(); let account_id = st.test_account().unwrap().id(); @@ -3632,7 +3632,7 @@ mod tests { #[test] fn get_account_ids() { let mut st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .with_account_from_sapling_activation(BlockHash([0; 32])) .build(); @@ -3648,7 +3648,7 @@ mod tests { #[test] fn block_fully_scanned() { - check_block_fully_scanned(TestDbFactory) + check_block_fully_scanned(TestDbFactory::default()) } fn check_block_fully_scanned(dsf: DsF) { @@ -3713,7 +3713,7 @@ mod tests { #[test] fn test_account_birthday() { let st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .with_block_cache(BlockCache::new()) .with_account_from_sapling_activation(BlockHash([0; 32])) .build(); diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index b1246899b..f9d7ecbdc 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -312,7 +312,7 @@ pub fn init_wallet_db( init_wallet_db_internal(wdb, seed, &[], true) } -fn init_wallet_db_internal( +pub(crate) fn init_wallet_db_internal( wdb: &mut WalletDb, seed: Option>, target_migrations: &[Uuid], @@ -455,7 +455,7 @@ mod tests { #[test] fn verify_schema() { let st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .build(); use regex::Regex; diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index 9cb65e571..fd4fac0af 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -629,7 +629,7 @@ pub(crate) mod tests { let initial_height_offset = 310; let mut st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .with_block_cache(BlockCache::new()) .with_initial_chain_state(|rng, network| { let sapling_activation_height = @@ -796,7 +796,7 @@ pub(crate) mod tests { u32, ) { let st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .with_block_cache(BlockCache::new()) .with_initial_chain_state(|rng, network| { // We set the Sapling and Orchard frontiers at the birthday height to be @@ -891,7 +891,7 @@ pub(crate) mod tests { use ScanPriority::*; let mut st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .with_block_cache(BlockCache::new()) .build(); let sap_active = st.sapling_activation_height(); @@ -1044,7 +1044,7 @@ pub(crate) mod tests { // notes beyond the end of the first shard. let frontier_tree_size: u32 = (0x1 << 16) + 1234; let mut st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .with_block_cache(BlockCache::new()) .with_initial_chain_state(|rng, network| { let birthday_height = @@ -1237,7 +1237,7 @@ pub(crate) mod tests { // notes beyond the end of the first shard. let frontier_tree_size: u32 = (0x1 << 16) + 1234; let mut st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .with_block_cache(BlockCache::new()) .with_initial_chain_state(|rng, network| { let birthday_height = @@ -1462,7 +1462,7 @@ pub(crate) mod tests { use ScanPriority::*; let mut st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .build(); let ranges = vec![ @@ -1507,7 +1507,7 @@ pub(crate) mod tests { use ScanPriority::*; let mut st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .build(); let ranges = vec![ @@ -1579,7 +1579,7 @@ pub(crate) mod tests { // notes back from the end of the second shard. let birthday_tree_size: u32 = (0x1 << 17) - 50; let mut st = TestBuilder::new() - .with_data_store_factory(TestDbFactory) + .with_data_store_factory(TestDbFactory::default()) .with_block_cache(BlockCache::new()) .with_initial_chain_state(|rng, network| { let birthday_height = diff --git a/zcash_client_sqlite/src/wallet/transparent.rs b/zcash_client_sqlite/src/wallet/transparent.rs index 64208ff12..b63848040 100644 --- a/zcash_client_sqlite/src/wallet/transparent.rs +++ b/zcash_client_sqlite/src/wallet/transparent.rs @@ -827,14 +827,14 @@ mod tests { #[test] fn put_received_transparent_utxo() { zcash_client_backend::data_api::testing::transparent::put_received_transparent_utxo( - TestDbFactory, + TestDbFactory::default(), ); } #[test] fn transparent_balance_across_shielding() { zcash_client_backend::data_api::testing::transparent::transparent_balance_across_shielding( - TestDbFactory, + TestDbFactory::default(), BlockCache::new(), ); } From 6fb4f5c7a614c981891591a4700124293c62b8c6 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 23 Oct 2024 10:27:13 -0600 Subject: [PATCH 3/5] zcash_client_sqlite: Add a migration to fix broken is_change flagging --- zcash_client_sqlite/src/testing/db.rs | 11 +- .../src/wallet/init/migrations.rs | 4 + .../migrations/fix_bad_change_flagging.rs | 208 ++++++++++++++++++ 3 files changed, 214 insertions(+), 9 deletions(-) create mode 100644 zcash_client_sqlite/src/wallet/init/migrations/fix_bad_change_flagging.rs diff --git a/zcash_client_sqlite/src/testing/db.rs b/zcash_client_sqlite/src/testing/db.rs index 61831cc18..84ea3b4fd 100644 --- a/zcash_client_sqlite/src/testing/db.rs +++ b/zcash_client_sqlite/src/testing/db.rs @@ -147,6 +147,7 @@ unsafe fn run_sqlite3>(db_path: S, command: &str) { eprintln!("------"); } +#[derive(Default)] pub(crate) struct TestDbFactory { target_migrations: Option>, } @@ -160,14 +161,6 @@ impl TestDbFactory { } } -impl Default for TestDbFactory { - fn default() -> Self { - Self { - target_migrations: Default::default(), - } - } -} - impl DataStoreFactory for TestDbFactory { type Error = (); type AccountId = AccountId; @@ -179,7 +172,7 @@ impl DataStoreFactory for TestDbFactory { let data_file = NamedTempFile::new().unwrap(); let mut db_data = WalletDb::for_path(data_file.path(), network).unwrap(); if let Some(migrations) = &self.target_migrations { - init_wallet_db_internal(&mut db_data, None, &migrations, true).unwrap(); + init_wallet_db_internal(&mut db_data, None, migrations, true).unwrap(); } else { init_wallet_db(&mut db_data, None).unwrap(); } diff --git a/zcash_client_sqlite/src/wallet/init/migrations.rs b/zcash_client_sqlite/src/wallet/init/migrations.rs index 5d9ac6183..8ec0e9b49 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations.rs @@ -4,6 +4,7 @@ mod add_utxo_account; mod addresses_table; mod ensure_orchard_ua_receiver; mod ephemeral_addresses; +mod fix_bad_change_flagging; mod fix_broken_commitment_trees; mod full_account_ids; mod initial_setup; @@ -80,6 +81,8 @@ pub(super) fn all_migrations( // support_legacy_sqlite // | // fix_broken_commitment_trees + // | + // fix_bad_change_flagging vec![ Box::new(initial_setup::Migration {}), Box::new(utxos_table::Migration {}), @@ -141,6 +144,7 @@ pub(super) fn all_migrations( Box::new(fix_broken_commitment_trees::Migration { params: params.clone(), }), + Box::new(fix_bad_change_flagging::Migration), ] } diff --git a/zcash_client_sqlite/src/wallet/init/migrations/fix_bad_change_flagging.rs b/zcash_client_sqlite/src/wallet/init/migrations/fix_bad_change_flagging.rs new file mode 100644 index 000000000..9773ab19b --- /dev/null +++ b/zcash_client_sqlite/src/wallet/init/migrations/fix_bad_change_flagging.rs @@ -0,0 +1,208 @@ +//! Sets the `is_change` flag on output notes received by an internal key when input value was +//! provided from the account corresponding to that key. +use std::collections::HashSet; + +use rusqlite::named_params; +use schemerz_rusqlite::RusqliteMigration; +use uuid::Uuid; +use zip32::Scope; + +use crate::{ + wallet::{ + init::{migrations::fix_broken_commitment_trees, WalletMigrationError}, + scope_code, + }, + SAPLING_TABLES_PREFIX, +}; + +#[cfg(feature = "orchard")] +use crate::ORCHARD_TABLES_PREFIX; + +pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0x6d36656d_533b_4b65_ae91_dcb95c4ad289); + +const DEPENDENCIES: &[Uuid] = &[fix_broken_commitment_trees::MIGRATION_ID]; + +pub(super) struct Migration; + +impl schemerz::Migration for Migration { + fn id(&self) -> Uuid { + MIGRATION_ID + } + + fn dependencies(&self) -> HashSet { + DEPENDENCIES.iter().copied().collect() + } + + fn description(&self) -> &'static str { + "Sets the `is_change` flag on output notes received by an internal key when input value was provided from the account corresponding to that key." + } +} + +impl RusqliteMigration for Migration { + type Error = WalletMigrationError; + + fn up(&self, transaction: &rusqlite::Transaction) -> Result<(), WalletMigrationError> { + let fix_change_flag = |table_prefix| { + transaction.execute( + &format!( + "UPDATE {table_prefix}_received_notes + SET is_change = 1 + FROM sent_notes sn + WHERE sn.tx = {table_prefix}_received_notes.tx + AND sn.from_account_id = {table_prefix}_received_notes.account_id + AND {table_prefix}_received_notes.recipient_key_scope = :internal_scope" + ), + named_params! {":internal_scope": scope_code(Scope::Internal)}, + ) + }; + + fix_change_flag(SAPLING_TABLES_PREFIX)?; + #[cfg(feature = "orchard")] + fix_change_flag(ORCHARD_TABLES_PREFIX)?; + + Ok(()) + } + + fn down(&self, _: &rusqlite::Transaction) -> Result<(), WalletMigrationError> { + Err(WalletMigrationError::CannotRevert(MIGRATION_ID)) + } +} + +#[cfg(test)] +mod tests { + use crate::wallet::init::migrations::tests::test_migrate; + + #[cfg(feature = "transparent-inputs")] + use { + crate::{ + testing::{db::TestDbFactory, BlockCache}, + wallet::init::init_wallet_db, + }, + zcash_client_backend::{ + data_api::{ + testing::{ + pool::ShieldedPoolTester, sapling::SaplingPoolTester, AddressType, TestBuilder, + }, + wallet::input_selection::GreedyInputSelector, + Account as _, WalletRead as _, WalletWrite as _, + }, + fees::{standard, DustOutputPolicy}, + wallet::WalletTransparentOutput, + }, + zcash_primitives::{ + block::BlockHash, + transaction::{ + components::{OutPoint, TxOut}, + fees::StandardFeeRule, + }, + }, + zcash_protocol::value::Zatoshis, + }; + + #[test] + fn migrate() { + test_migrate(&[super::MIGRATION_ID]); + } + + #[cfg(feature = "transparent-inputs")] + fn shield_transparent() { + let ds_factory = TestDbFactory::new(super::DEPENDENCIES.to_vec()); + let cache = BlockCache::new(); + let mut st = TestBuilder::new() + .with_data_store_factory(ds_factory) + .with_block_cache(cache) + .with_account_from_sapling_activation(BlockHash([0; 32])) + .build(); + + let account = st.test_account().cloned().unwrap(); + let dfvk = T::test_account_fvk(&st); + + let uaddr = st + .wallet() + .get_current_address(account.id()) + .unwrap() + .unwrap(); + let taddr = uaddr.transparent().unwrap(); + + // Ensure that the wallet has at least one block + let (h, _, _) = st.generate_next_block( + &dfvk, + AddressType::Internal, + Zatoshis::const_from_u64(50000), + ); + st.scan_cached_blocks(h, 1); + + let utxo = WalletTransparentOutput::from_parts( + OutPoint::fake(), + TxOut { + value: Zatoshis::const_from_u64(100000), + script_pubkey: taddr.script(), + }, + Some(h), + ) + .unwrap(); + + let res0 = st.wallet_mut().put_received_transparent_utxo(&utxo); + assert_matches!(res0, Ok(_)); + + let fee_rule = StandardFeeRule::Zip317; + + let input_selector = GreedyInputSelector::new(); + let change_strategy = standard::SingleOutputChangeStrategy::new( + fee_rule, + None, + T::SHIELDED_PROTOCOL, + DustOutputPolicy::default(), + ); + + let txids = st + .shield_transparent_funds( + &input_selector, + &change_strategy, + Zatoshis::from_u64(10000).unwrap(), + account.usk(), + &[*taddr], + account.id(), + 1, + ) + .unwrap(); + assert_eq!(txids.len(), 1); + + let tx = st.get_tx_from_history(*txids.first()).unwrap().unwrap(); + assert_eq!(tx.spent_note_count(), 1); + assert!(tx.has_change()); + assert_eq!(tx.received_note_count(), 0); + assert_eq!(tx.sent_note_count(), 0); + assert!(tx.is_shielding()); + + // Prior to the fix that removes the source of the error this migration is addressing, + // this scanning will result in a state where `tx.is_shielding() == false`. However, + // we can't validate that here, because after that fix, this test would fail. + let (h, _) = st.generate_next_block_including(*txids.first()); + st.scan_cached_blocks(h, 1); + + // Complete the migration to resolve the incorrect change flag value. + init_wallet_db(st.wallet_mut().db_mut(), None).unwrap(); + + let tx = st.get_tx_from_history(*txids.first()).unwrap().unwrap(); + assert_eq!(tx.spent_note_count(), 1); + assert!(tx.has_change()); + assert_eq!(tx.received_note_count(), 0); + assert_eq!(tx.sent_note_count(), 0); + assert!(tx.is_shielding()); + } + + #[test] + #[cfg(feature = "transparent-inputs")] + fn sapling_shield_transparent() { + shield_transparent::(); + } + + #[test] + #[cfg(all(feature = "orchard", feature = "transparent-inputs"))] + fn orchard_shield_transparent() { + use zcash_client_backend::data_api::testing::orchard::OrchardPoolTester; + + shield_transparent::(); + } +} From 76ca8fcb811fa86c9af625ee40fb4600cd8855da Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 22 Oct 2024 17:25:03 -0600 Subject: [PATCH 4/5] zcash_client_sqlite: Once a note is determined to be change, don't rescind that determination. This is a minimal and correct but incomplete fix for zcash/librustzcash#1571. Additional work to distinguish change outputs is necessary to update existing wallets that have made an incorrect change determination in the past. --- zcash_client_sqlite/src/wallet/orchard.rs | 2 +- zcash_client_sqlite/src/wallet/sapling.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/zcash_client_sqlite/src/wallet/orchard.rs b/zcash_client_sqlite/src/wallet/orchard.rs index 890bfa7bb..321b1fcab 100644 --- a/zcash_client_sqlite/src/wallet/orchard.rs +++ b/zcash_client_sqlite/src/wallet/orchard.rs @@ -252,7 +252,7 @@ pub(crate) fn put_received_note( rseed = :rseed, nf = IFNULL(:nf, nf), memo = IFNULL(:memo, memo), - is_change = IFNULL(:is_change, is_change), + is_change = MAX(:is_change, is_change), commitment_tree_position = IFNULL(:commitment_tree_position, commitment_tree_position), recipient_key_scope = :recipient_key_scope RETURNING orchard_received_notes.id", diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index 27b4fde3c..b4df74fbe 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -355,7 +355,7 @@ pub(crate) fn put_received_note( rcm = :rcm, nf = IFNULL(:nf, nf), memo = IFNULL(:memo, memo), - is_change = IFNULL(:is_change, is_change), + is_change = MAX(:is_change, is_change), commitment_tree_position = IFNULL(:commitment_tree_position, commitment_tree_position), recipient_key_scope = :recipient_key_scope RETURNING sapling_received_notes.id", From d75309080106639920f8302e555f482dfc35e6c2 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 23 Oct 2024 10:47:10 -0600 Subject: [PATCH 5/5] zcash_client_sqlite: Ensure that previously-received change is correctly flagged when recording sent outputs. Fixes #1571 --- zcash_client_sqlite/src/wallet.rs | 35 +++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 00618e9d1..43d8d7777 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -3223,9 +3223,38 @@ fn recipient_params( } } +fn flag_previously_received_change( + conn: &rusqlite::Transaction, + tx_ref: TxRef, +) -> Result<(), SqliteClientError> { + let flag_received_change = |table_prefix| { + conn.execute( + &format!( + "UPDATE {table_prefix}_received_notes + SET is_change = 1 + FROM sent_notes sn + WHERE sn.tx = {table_prefix}_received_notes.tx + AND sn.tx = :tx + AND sn.from_account_id = {table_prefix}_received_notes.account_id + AND {table_prefix}_received_notes.recipient_key_scope = :internal_scope" + ), + named_params! { + ":tx": tx_ref.0, + ":internal_scope": scope_code(Scope::Internal) + }, + ) + }; + + flag_received_change(SAPLING_TABLES_PREFIX)?; + #[cfg(feature = "orchard")] + flag_received_change(ORCHARD_TABLES_PREFIX)?; + + Ok(()) +} + /// Records information about a transaction output that your wallet created. pub(crate) fn insert_sent_output( - conn: &rusqlite::Connection, + conn: &rusqlite::Transaction, params: &P, tx_ref: TxRef, from_account: AccountId, @@ -3253,6 +3282,7 @@ pub(crate) fn insert_sent_output( ]; stmt_insert_sent_output.execute(sql_args)?; + flag_previously_received_change(conn, tx_ref)?; Ok(()) } @@ -3270,7 +3300,7 @@ pub(crate) fn insert_sent_output( /// the transaction. #[allow(clippy::too_many_arguments)] pub(crate) fn put_sent_output( - conn: &rusqlite::Connection, + conn: &rusqlite::Transaction, params: &P, from_account: AccountId, tx_ref: TxRef, @@ -3307,6 +3337,7 @@ pub(crate) fn put_sent_output( ]; stmt_upsert_sent_output.execute(sql_args)?; + flag_previously_received_change(conn, tx_ref)?; Ok(()) }