fix(key-wallet): keep dedup working when keep_txs_in_memory is off

CodeRabbit review caught that confirm_transaction's dedup guard
(self.transactions.contains_key(&txid)) is always false with the
feature off, so re-events (mempool→block, IS-lock arrivals after a
block) are treated as brand new every time — replaying record_transaction,
re-bumping monitor_revision, and spuriously flipping state_modified.

Add a `processed_txids: HashSet<Txid>` field that's always populated
regardless of the feature, and use it as the dedup signal. has_transaction
now reads it (so call sites still work in both configs); record_transaction
inserts into it; Deserialize rebuilds it from `transactions` keys.

Also adds an idempotency regression test (mempool→block on the same tx
must not change utxo count or balance and must bump monitor_revision at
most once) that runs in both feature configurations.

Existing wallet_checker tests that simulated a missing record by
removing from `transactions` directly are updated to clear
`processed_txids` too (since the dedup signal now lives there).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: QuantumExplorer

key-wallet/src/managed_account/mod.rs

@@ -69,6 +69,14 @@ pub struct ManagedCoreAccount {
     pub transactions: BTreeMap<Txid, TransactionRecord>,
     /// UTXO set for this account
     pub utxos: BTreeMap<OutPoint, Utxo>,
+    /// Txids of every transaction this account has already processed.
+    ///
+    /// Always populated regardless of `keep_txs_in_memory`, so the
+    /// dedup guard in `confirm_transaction` works in either feature
+    /// configuration. Rebuilt from `transactions` during deserialization
+    /// when the feature is enabled.
+    #[cfg_attr(feature = "serde", serde(skip_serializing))]
+    pub(crate) processed_txids: HashSet<Txid>,
     /// Outpoints spent by recorded transactions.
     /// Rebuilt from `transactions` during deserialization.
     #[cfg_attr(feature = "serde", serde(skip_serializing))]
@@ -95,25 +103,20 @@ impl ManagedCoreAccount {
             #[cfg(feature = "keep_txs_in_memory")]
             transactions: BTreeMap::new(),
             utxos: BTreeMap::new(),
+            processed_txids: HashSet::new(),
             spent_outpoints: HashSet::new(),
             monitor_revision: 0,
         }
     }
 
-    /// Returns `true` if a transaction record for `txid` is stored on this account.
+    /// Returns `true` if this account has already processed `txid`.
     ///
-    /// Always returns `false` when the `keep_txs_in_memory` Cargo feature is
-    /// disabled, since no records are kept.
+    /// Backed by an always-present `processed_txids` set, so this works
+    /// regardless of whether the `keep_txs_in_memory` Cargo feature is
+    /// enabled. Use [`Self::get_transaction`] to retrieve the full record
+    /// (only available when the feature is enabled).
     pub fn has_transaction(&self, txid: &Txid) -> bool {
-        #[cfg(feature = "keep_txs_in_memory")]
-        {
-            self.transactions.contains_key(txid)
-        }
-        #[cfg(not(feature = "keep_txs_in_memory"))]
-        {
-            let _ = txid;
-            false
-        }
+        self.processed_txids.contains(txid)
     }
 
     /// Returns the stored transaction record for `txid`, if any.
@@ -463,49 +466,51 @@ impl ManagedCoreAccount {
     /// Re-process an existing transaction with updated context (e.g., mempool→block confirmation)
     /// and potentially new address matches from gap limit rescans.
     ///
-    /// When the `keep_txs_in_memory` Cargo feature is disabled, no per-tx
-    /// records exist, so this always falls through to `record_transaction`
-    /// and reports the call as a state change.
+    /// Deduplication uses `processed_txids`, which is populated regardless
+    /// of the `keep_txs_in_memory` Cargo feature. When the feature is off
+    /// the per-tx record isn't stored, so we can't detect a confirmation
+    /// status transition; we still refresh the UTXO state and report no
+    /// change.
     pub(crate) fn confirm_transaction(
         &mut self,
         tx: &Transaction,
         account_match: &AccountMatch,
         context: TransactionContext,
         transaction_type: TransactionType,
     ) -> bool {
+        if !self.processed_txids.contains(&tx.txid()) {
+            self.record_transaction(tx, account_match, context, transaction_type);
+            return true;
+        }
+
+        #[cfg_attr(not(feature = "keep_txs_in_memory"), allow(unused_mut))]
+        let mut changed = false;
         #[cfg(feature = "keep_txs_in_memory")]
-        {
-            if !self.transactions.contains_key(&tx.txid()) {
-                self.record_transaction(tx, account_match, context, transaction_type);
-                return true;
-            }
-
-            let mut changed = false;
-            if let Some(tx_record) = self.transactions.get_mut(&tx.txid()) {
-                debug_assert_eq!(
-                    tx_record.transaction_type,
-                    transaction_type,
-                    "transaction_type changed between recordings for {}",
-                    tx.txid()
-                );
-                if tx_record.context != context {
-                    let was_confirmed = tx_record.context.confirmed();
-                    tx_record.update_context(context.clone());
-                    // Only signal a change when confirmation status actually changes,
-                    // not for upgrades within the confirmed state (e.g. InBlock → InChainLockedBlock).
-                    // TODO: emit a change event for InBlock → InChainLockedBlock once chainlock
-                    // wallet transaction events are properly handled
-                    changed = !was_confirmed;
-                }
+        if let Some(tx_record) = self.transactions.get_mut(&tx.txid()) {
+            debug_assert_eq!(
+                tx_record.transaction_type,
+                transaction_type,
+                "transaction_type changed between recordings for {}",
+                tx.txid()
+            );
+            if tx_record.context != context {
+                let was_confirmed = tx_record.context.confirmed();
+                tx_record.update_context(context.clone());
+                // Only signal a change when confirmation status actually changes,
+                // not for upgrades within the confirmed state (e.g. InBlock → InChainLockedBlock).
+                // TODO: emit a change event for InBlock → InChainLockedBlock once chainlock
+                // wallet transaction events are properly handled
+                changed = !was_confirmed;
             }
-            self.update_utxos(tx, account_match, context);
-            changed
         }
         #[cfg(not(feature = "keep_txs_in_memory"))]
         {
-            self.record_transaction(tx, account_match, context, transaction_type);
-            true
+            // No record stored — silence the unused-binding warning on
+            // `transaction_type` and let UTXO state convey the upgrade.
+            let _ = transaction_type;
         }
+        self.update_utxos(tx, account_match, context);
+        changed
     }
 
     /// Record a new transaction and update UTXOs for spendable account types
@@ -610,6 +615,7 @@ impl ManagedCoreAccount {
         );
 
         let record = tx_record.clone();
+        self.processed_txids.insert(tx.txid());
         #[cfg(feature = "keep_txs_in_memory")]
         self.transactions.insert(tx.txid(), tx_record);
 
@@ -1412,6 +1418,7 @@ impl<'de> Deserialize<'de> for ManagedCoreAccount {
 
         let helper = Helper::deserialize(deserializer)?;
 
+        let processed_txids: HashSet<Txid> = helper.transactions.keys().copied().collect();
         let spent_outpoints = helper
             .transactions
             .values()
@@ -1428,6 +1435,7 @@ impl<'de> Deserialize<'de> for ManagedCoreAccount {
             #[cfg(feature = "keep_txs_in_memory")]
             transactions: helper.transactions,
             utxos: helper.utxos,
+            processed_txids,
             spent_outpoints,
             monitor_revision: 0,
         })

key-wallet/src/tests/keep_txs_in_memory_tests.rs

@@ -3,11 +3,14 @@
 //! When the feature is enabled (the default), `ManagedCoreAccount` keeps a
 //! per-account `transactions` map populated as transactions are processed.
 //! When the feature is disabled, the field does not exist and processing a
-//! transaction only updates UTXOs and balance.
+//! transaction only updates UTXOs and balance — but `processed_txids`
+//! still tracks which txids have been seen so `confirm_transaction`
+//! continues to deduplicate re-events.
 
 use crate::test_utils::TestWalletContext;
-use crate::transaction_checking::TransactionContext;
-use dashcore::Transaction;
+use crate::transaction_checking::{BlockInfo, TransactionContext};
+use dashcore::{BlockHash, Transaction};
+use dashcore_hashes::Hash;
 
 #[tokio::test]
 async fn record_transaction_updates_utxos_and_balance() {
@@ -45,47 +48,54 @@ async fn transactions_are_not_stored_when_feature_disabled() {
     let _ = ctx.check_transaction(&tx, TransactionContext::Mempool).await;
 
     let account = ctx.bip44_account();
-    assert!(!account.has_transaction(&tx.txid()));
+    // No record is stored, so `get_transaction` returns None ...
     assert!(account.get_transaction(&tx.txid()).is_none());
+    // ... but `has_transaction` still returns true so re-events dedupe.
+    assert!(account.has_transaction(&tx.txid()));
 }
 
-#[cfg(feature = "keep_txs_in_memory")]
-#[test]
-fn helper_methods_proxy_transactions_field() {
-    use crate::account::{StandardAccountType, TransactionRecord};
-    use crate::managed_account::transaction_record::TransactionDirection;
-    use crate::managed_account::ManagedCoreAccount;
-    use crate::transaction_checking::TransactionType;
-    use crate::AccountType;
-    use dashcore::TxIn;
+/// Re-processing the same transaction (mempool → block) must not double-count
+/// UTXOs or change balance, regardless of whether the `keep_txs_in_memory`
+/// feature is enabled. Without the always-present `processed_txids` set this
+/// regresses when the feature is off because `confirm_transaction`'s only
+/// dedup signal disappears with the `transactions` map.
+#[tokio::test]
+async fn confirm_transaction_dedupes_repeat_events() {
+    let mut ctx = TestWalletContext::new_random();
 
-    let mut account = ManagedCoreAccount::dummy_bip44();
-    let tx = Transaction {
-        version: 1,
-        lock_time: 0,
-        input: vec![TxIn::default()],
-        output: Vec::new(),
-        special_transaction_payload: None,
-    };
-    let record = TransactionRecord::new(
-        tx.clone(),
-        AccountType::Standard {
-            index: 0,
-            standard_account_type: StandardAccountType::BIP44Account,
-        },
-        TransactionContext::Mempool,
-        TransactionType::Standard,
-        TransactionDirection::Incoming,
-        Vec::new(),
-        Vec::new(),
-        0,
-    );
-    let txid = tx.txid();
+    let tx = Transaction::dummy(&ctx.receive_address, 0..1, &[200_000]);
+
+    // First: mempool
+    let r1 = ctx.check_transaction(&tx, TransactionContext::Mempool).await;
+    assert!(r1.is_relevant);
+    assert!(r1.is_new_transaction);
+    let utxo_count_after_mempool = ctx.bip44_account().utxos.len();
+    let balance_after_mempool = ctx.bip44_account().balance.spendable();
+    let revision_after_mempool = ctx.bip44_account().monitor_revision();
 
-    assert!(!account.has_transaction(&txid));
-    assert!(account.get_transaction(&txid).is_none());
+    // Second: same tx confirmed in a block
+    let block = TransactionContext::InBlock(BlockInfo::new(
+        100,
+        BlockHash::from_slice(&[7u8; 32]).unwrap(),
+        1_700_000_000,
+    ));
+    let r2 = ctx.check_transaction(&tx, block).await;
+    assert!(r2.is_relevant);
+    assert!(!r2.is_new_transaction, "block confirmation must not be flagged as a new tx");
 
-    account.transactions.insert(txid, record);
-    assert!(account.has_transaction(&txid));
-    assert!(account.get_transaction(&txid).is_some());
+    let account = ctx.bip44_account();
+    assert_eq!(
+        account.utxos.len(),
+        utxo_count_after_mempool,
+        "UTXO count must not grow on confirmation"
+    );
+    assert_eq!(
+        account.balance.spendable(),
+        balance_after_mempool,
+        "spendable balance must not change between mempool and block confirmation"
+    );
+    // monitor_revision can advance once for the block-context UTXO refresh,
+    // but it must not balloon to N per replay.
+    let bumped_by = account.monitor_revision().saturating_sub(revision_after_mempool);
+    assert!(bumped_by <= 1, "monitor_revision bumped {bumped_by} times on a single confirmation");
 }

key-wallet/src/transaction_checking/wallet_checker.rs

@@ -1139,13 +1139,17 @@ mod tests {
         let (mut ctx, tx) = TestWalletContext::new_random().with_mempool_funding(300_000).await;
         let txid = tx.txid();
 
-        // Simulate the account missing the mempool record by removing it
+        // Simulate the account missing the mempool record by removing it.
+        // Clear `processed_txids` too — otherwise the always-present dedup
+        // set would keep `has_transaction` truthy and the wallet checker
+        // wouldn't treat this confirmation as new.
         let account = ctx
             .managed_wallet
             .first_bip44_managed_account_mut()
             .expect("Should have BIP44 account");
         assert!(account.transactions.contains_key(&txid));
         account.transactions.remove(&txid);
+        account.processed_txids.remove(&txid);
         assert!(!account.transactions.contains_key(&txid));
 
         // Now process the same tx as a block confirmation.
@@ -1195,6 +1199,7 @@ mod tests {
             .first_bip44_managed_account_mut()
             .expect("Should have BIP44 account");
         account.transactions.remove(&txid);
+        account.processed_txids.remove(&txid);
         account.utxos.clear();
         assert!(!account.transactions.contains_key(&txid));
         assert!(account.utxos.is_empty());