fix nullifier spend detection logic

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<Vec<zcash_client_backend::data_api::testing::TransactionSummary<AccountId>>, 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::<Vec<_>>();
 
-                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::<Result<Vec<_>, 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(

zcash_client_memory/src/testing/pool/sapling.rs

@@ -87,6 +87,11 @@ fn checkpoint_gaps() {
     testing::pool::checkpoint_gaps::<SaplingPoolTester>()
 }
 
+#[test]
+fn scan_cached_blocks_detects_change_notes() {
+    testing::pool::scan_cached_blocks_finds_change_notes::<SaplingPoolTester>()
+}
+
 #[test]
 fn scan_cached_blocks_detects_spends_out_of_order() {
     testing::pool::scan_cached_blocks_detects_spends_out_of_order::<SaplingPoolTester>()

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))

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(

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<AccountId>, height: BlockHeight) {

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<P: consensus::Parameters> WalletRead for MemoryWalletDb<P> {
     type Error = Error;
@@ -595,13 +595,22 @@ impl<P: consensus::Parameters> WalletRead for MemoryWalletDb<P> {
                 .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<P: consensus::Parameters> WalletRead for MemoryWalletDb<P> {
                 .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))
                     }

zcash_client_memory/src/wallet_write.rs

@@ -76,8 +76,7 @@ impl<P: consensus::Parameters> WalletWrite for MemoryWalletDb<P> {
             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<P: consensus::Parameters> WalletWrite for MemoryWalletDb<P> {
 
                 // 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)?;
                 }