diff --git a/zcash_client_memory/src/testing/mod.rs b/zcash_client_memory/src/testing/mod.rs index 87f13ab545..cfba444df7 100644 --- a/zcash_client_memory/src/testing/mod.rs +++ b/zcash_client_memory/src/testing/mod.rs @@ -28,6 +28,7 @@ use zcash_primitives::transaction::TxId; use zcash_protocol::consensus::BlockHeight; use zcash_protocol::local_consensus::LocalNetwork; +use crate::account; use crate::{Account, AccountId, Error, MemBlockCache, MemoryWalletDb, SentNoteId}; pub mod pool; @@ -213,13 +214,16 @@ where &self, ) -> Result>, Error> { - Ok(self + let mut history = self .tx_table .iter() .map(|(txid, tx)| { // find all the notes associated with this transaction + // A transaction may send and/or receive one or more notes + + // notes spent (consumed) by the transaction + println!("received_note_spends: {:?}", self.received_note_spends); - // notes spent by the transaction let spent_notes = self .received_note_spends .iter() @@ -264,10 +268,33 @@ where .filter(|received_note| received_note.txid() == *txid) .collect::>(); - let account_id = sent_notes - .first() - .map(|(_, note)| note.from_account_id) - .unwrap_or_default(); + // A transaction can send and receive notes to/from multiple accounts + // For a transaction to be visible to this wallet it must have either scanned it from the chain + // or been created by this wallet. + let receiving_account_id = received_notes.first().map(|note| note.account_id()); + let sending_account_id = sent_notes.first().map(|(_, note)| note.from_account_id); + let account_id = match (receiving_account_id, sending_account_id) { + (Some(receiving), Some(sending)) => { + if receiving == sending { + receiving + } else { + // This transaction is associated with multiple accounts + // This should not happen + return Err(Error::Other( + "Transaction associated with multiple accounts".to_string(), + )); + } + } + (Some(account_id), _) => account_id, + (_, Some(account_id)) => account_id, + _ => { + // This transaction is not associated with any account + // This should not happen + return Err(Error::Other( + "Transaction not associated with any account".to_string(), + )); + } + }; let balance_gained: u64 = received_notes .iter() @@ -296,23 +323,31 @@ where let has_change = received_notes.iter().any(|note| note.is_change); - zcash_client_backend::data_api::testing::TransactionSummary::from_parts( - account_id, // account_id - *txid, // txid - tx.expiry_height(), // expiry_height - tx.mined_height(), // mined_height - ZatBalance::const_from_i64((balance_gained as i64) - (balance_lost as i64)), // account_value_delta - tx.fee(), // fee_paid - spent_notes.len() + spent_utxos.len(), // spent_note_count - has_change, // has_change - sent_notes.len(), // sent_note_count (excluding change) - received_notes.iter().filter(|note| !note.is_change).count(), // received_note_count (excluding change) - 0, // TODO: memo_count - false, // TODO: expired_unmined - is_shielding, // is_shielding + Ok( + zcash_client_backend::data_api::testing::TransactionSummary::from_parts( + account_id, // account_id + *txid, // txid + tx.expiry_height(), // expiry_height + tx.mined_height(), // mined_height + ZatBalance::const_from_i64((balance_gained as i64) - (balance_lost as i64)), // account_value_delta + tx.fee(), // fee_paid + spent_notes.len() + spent_utxos.len(), // spent_note_count + has_change, // has_change + sent_notes.len(), // sent_note_count (excluding change) + received_notes.iter().filter(|note| !note.is_change).count(), // received_note_count (excluding change) + 0, // TODO: memo_count + false, // TODO: expired_unmined + is_shielding, // is_shielding + ), ) }) - .collect()) + .collect::, Error>>()?; + history.sort_by(|a, b| { + b.mined_height() + .cmp(&a.mined_height()) + .then(b.txid().cmp(&a.txid())) + }); + Ok(history) } fn get_checkpoint_history( diff --git a/zcash_client_memory/src/testing/pool/sapling.rs b/zcash_client_memory/src/testing/pool/sapling.rs index 83c2ccfd75..dca7a04cb2 100644 --- a/zcash_client_memory/src/testing/pool/sapling.rs +++ b/zcash_client_memory/src/testing/pool/sapling.rs @@ -87,6 +87,11 @@ fn checkpoint_gaps() { testing::pool::checkpoint_gaps::() } +#[test] +fn scan_cached_blocks_detects_change_notes() { + testing::pool::scan_cached_blocks_finds_change_notes::() +} + #[test] fn scan_cached_blocks_detects_spends_out_of_order() { testing::pool::scan_cached_blocks_detects_spends_out_of_order::() diff --git a/zcash_client_memory/src/types/account.rs b/zcash_client_memory/src/types/account.rs index bc4707ef58..6e4f76a689 100644 --- a/zcash_client_memory/src/types/account.rs +++ b/zcash_client_memory/src/types/account.rs @@ -86,11 +86,11 @@ impl Accounts { birthday: AccountBirthday, purpose: AccountPurpose, ) -> Result<(AccountId, Account), Error> { + self.nonce += 1; let account_id = AccountId(self.nonce); let acc = Account::new(account_id, kind, viewing_key, birthday, purpose)?; - self.nonce += 1; self.accounts.insert(account_id, acc.clone()); Ok((account_id, acc)) diff --git a/zcash_client_memory/src/types/nullifier.rs b/zcash_client_memory/src/types/nullifier.rs index eb79f96290..8c5641b047 100644 --- a/zcash_client_memory/src/types/nullifier.rs +++ b/zcash_client_memory/src/types/nullifier.rs @@ -8,7 +8,7 @@ use serde_with::FromInto; use zcash_primitives::consensus::BlockHeight; use zcash_protocol::PoolType; -/// Maps a nullifier to the block height and transaction index where it was spent. +/// Maps a nullifier to the block height and transaction index (NOT txid!) where it was spent. #[serde_as] #[derive(Serialize, Deserialize)] pub(crate) struct NullifierMap( diff --git a/zcash_client_memory/src/types/transaction.rs b/zcash_client_memory/src/types/transaction.rs index f79dd4ef52..31901d608b 100644 --- a/zcash_client_memory/src/types/transaction.rs +++ b/zcash_client_memory/src/types/transaction.rs @@ -115,6 +115,16 @@ impl TransactionTable { self.0.get(&txid) } + pub(crate) fn get_by_height_and_index( + &self, + height: BlockHeight, + index: u32, + ) -> Option<&TransactionEntry> { + self.0 + .values() + .find(|entry| entry.block == Some(height) && entry.tx_index == Some(index)) + } + /// Inserts information about a MINED transaction that was observed to /// contain a note related to this wallet pub(crate) fn put_tx_meta(&mut self, tx_meta: WalletTx, height: BlockHeight) { diff --git a/zcash_client_memory/src/wallet_read.rs b/zcash_client_memory/src/wallet_read.rs index ac43b4112e..e87f4855e3 100644 --- a/zcash_client_memory/src/wallet_read.rs +++ b/zcash_client_memory/src/wallet_read.rs @@ -45,7 +45,7 @@ use { }; use super::{Account, AccountId, MemoryWalletDb}; -use crate::{error::Error, MemoryWalletBlock, SentNoteId}; +use crate::{error::Error, MemoryWalletBlock, Nullifier, SentNoteId}; impl WalletRead for MemoryWalletDb

{ type Error = Error; @@ -595,13 +595,22 @@ impl WalletRead for MemoryWalletDb

{ .map(|(account_id, _, nf)| (account_id, nf)) .collect(), NullifierQuery::Unspent => nullifiers - .filter_map(|(account_id, txid, nf)| { - let tx_status = self.tx_table.tx_status(&txid); - let expiry_height = self.tx_table.expiry_height(&txid); - if matches!(tx_status, Some(TransactionStatus::Mined(_))) - || expiry_height.is_none() - { - None + .filter_map(|(account_id, _, nf)| { + // find any tx we know of that spends this nullifier and if so require that it is unmined or expired + if let Some((height, tx_index)) = self.nullifiers.get(&Nullifier::Sapling(nf)) { + if let Some(spending_tx) = + self.tx_table.get_by_height_and_index(*height, *tx_index) + { + if matches!(spending_tx.status(), TransactionStatus::Mined(_)) + || spending_tx.expiry_height().is_none() + { + None + } else { + Some((account_id, nf)) + } + } else { + None + } } else { Some((account_id, nf)) } @@ -622,13 +631,22 @@ impl WalletRead for MemoryWalletDb

{ .map(|(account_id, _, nf)| (account_id, nf)) .collect(), NullifierQuery::Unspent => nullifiers - .filter_map(|(account_id, txid, nf)| { - let tx_status = self.tx_table.tx_status(&txid); - let expiry_height = self.tx_table.expiry_height(&txid); - if matches!(tx_status, Some(TransactionStatus::Mined(_))) - || expiry_height.is_none() - { - None + .filter_map(|(account_id, _, nf)| { + // find any tx we know of that spends this nullifier and if so require that it is unmined or expired + if let Some((height, tx_index)) = self.nullifiers.get(&Nullifier::Orchard(nf)) { + if let Some(spending_tx) = + self.tx_table.get_by_height_and_index(*height, *tx_index) + { + if matches!(spending_tx.status(), TransactionStatus::Mined(_)) + || spending_tx.expiry_height().is_none() + { + None + } else { + Some((account_id, nf)) + } + } else { + None + } } else { Some((account_id, nf)) } diff --git a/zcash_client_memory/src/wallet_write.rs b/zcash_client_memory/src/wallet_write.rs index d7e325a23e..c59d6f5cb1 100644 --- a/zcash_client_memory/src/wallet_write.rs +++ b/zcash_client_memory/src/wallet_write.rs @@ -76,8 +76,7 @@ impl WalletWrite for MemoryWalletDb

{ let seed_fingerprint = SeedFingerprint::from_seed(seed.expose_secret()) .ok_or_else(|| Self::Error::InvalidSeedLength)?; let account_index = self - .max_zip32_account_index(&seed_fingerprint) - .unwrap() + .max_zip32_account_index(&seed_fingerprint)? .map(|a| a.next().ok_or_else(|| Self::Error::AccountOutOfRange)) .transpose()? .unwrap_or(zip32::AccountId::ZERO); @@ -321,6 +320,11 @@ impl WalletWrite for MemoryWalletDb

{ // Mark the Sapling nullifiers of the spent notes as spent in the `sapling_spends` map. for spend in transaction.sapling_spends() { + println!( + "marking note {:?} as spent in transaction {:?}", + spend.nf(), + txid + ); self.mark_sapling_note_spent(*spend.nf(), txid)?; }