refactor(key-wallet): align finalization with chainlock-only semantics

Address xdustinface review feedback on PR #733:

- Drop `TransactionContext::is_finalized()` (the soft IS-or-chainlock
  check). The wallet now treats only a chainlock as finality.
- Rename `TransactionContext::is_finalized_in_block()` to
  `is_chain_locked()` so the predicate name describes the variant
  rather than overloading "finalized" — same naming style as the
  existing `is_instant_send()` helper.
- Drop `ManagedAccountTrait::transaction_is_finalized()` (the
  unused soft-finality variant) and rename
  `transaction_is_finalized_in_block()` to `transaction_is_finalized()`.
  Now that there's a single finalization concept, the trait method
  name aligns with the feature name.
- Replace runtime `if cfg!(...) { ... } else { ... }` branches in
  `ManagedCoreKeysAccount::has_transaction` and
  `transaction_is_finalized` with two `#[cfg]`-gated function bodies
  — one per feature configuration. Same for the chainlock-driven
  drop call in `confirm_transaction` / `record_transaction`: the
  `let drop_now = …` binding now only exists when the feature is off,
  removing the `#[allow(unused_variables)]` workaround.
- Rewrite the doc on `has_transaction` so it's clear what it actually
  reports (live record OR finalized-txid marker) and how callers use
  it (distinguish brand-new sightings from re-processings) instead of
  the misleading "dedup signal in `confirm_transaction`" wording.

Drop logic still triggers on the strict `is_chain_locked()` check —
IS-locked-but-not-yet-chainlocked records still survive so the
surrounding block-confirmation event can populate height / block
hash before the chainlock catches up.

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

Author: QuantumExplorer

key-wallet/src/managed_account/managed_account_trait.rs

@@ -45,33 +45,24 @@ pub trait ManagedAccountTrait {
     fn transactions_mut(&mut self) -> &mut BTreeMap<Txid, TransactionRecord>;
 
     /// Returns `true` if this account has already processed `txid`,
-    /// whether it's still mutable in `transactions` or has been
-    /// finalized-and-pruned (under the default feature configuration).
-    /// Used as the dedup signal in `confirm_transaction`.
+    /// whether it is still represented as a full record in `transactions`
+    /// or has been pruned (under the default feature configuration) and
+    /// is now only retained as a finalized-txid marker. Used by callers
+    /// that need to distinguish a brand-new sighting from a re-processing
+    /// (mempool → block, IS-lock arrival, chainlock, …).
     fn has_transaction(&self, txid: &Txid) -> bool;
 
-    /// Returns `true` if `txid` has reached a finalized state — i.e. it
-    /// has either an InstantSend lock or a ChainLock and is no longer
-    /// expected to change.
-    ///
-    /// This is the *soft* finality check (mirrors
-    /// [`crate::transaction_checking::TransactionContext::is_finalized`]).
-    /// Use [`Self::transaction_is_finalized_in_block`] for the stricter
-    /// "fully confirmed in a chainlocked block" answer that drives
-    /// memory-pruning decisions.
-    fn transaction_is_finalized(&self, txid: &Txid) -> bool;
-
     /// Returns `true` if `txid` has been mined in a block that is itself
-    /// chainlocked — the strongest finality signal (mirrors
-    /// [`crate::transaction_checking::TransactionContext::is_finalized_in_block`]).
+    /// chainlocked — the only finality signal we treat as terminal
+    /// (mirrors [`crate::transaction_checking::TransactionContext::is_chain_locked`]).
     ///
     /// `InBlock` alone is not enough (the block can still be reorganized
     /// out), and `InstantSend` alone is not enough either (the
     /// surrounding block confirmation may still arrive and write the
     /// height / block hash before the chainlock catches up). Only
     /// `InChainLockedBlock` qualifies. This is the trigger for dropping
     /// the full record under the default feature configuration.
-    fn transaction_is_finalized_in_block(&self, txid: &Txid) -> bool;
+    fn transaction_is_finalized(&self, txid: &Txid) -> bool;
 
     /// Return the current monitor revision.
     ///

key-wallet/src/managed_account/managed_core_funds_account.rs

@@ -254,9 +254,9 @@ impl ManagedCoreFundsAccount {
     ) -> Option<TransactionRecord> {
         let txid = tx.txid();
 
-        // Already finalized in a chainlocked block: the tx is immutable —
+        // Already finalized via a chainlock: the tx is immutable —
         // no record update, no UTXO refresh, no event needed.
-        if self.keys.transaction_is_finalized_in_block(&txid) {
+        if self.keys.transaction_is_finalized(&txid) {
             return None;
         }
 
@@ -299,8 +299,8 @@ impl ManagedCoreFundsAccount {
         // enough — we keep the record so the surrounding block
         // confirmation can still write its height / block hash before the
         // chainlock catches up.
-        #[allow(unused_variables)]
-        let drop_now = context.is_finalized_in_block();
+        #[cfg(not(feature = "keep-finalized-transactions"))]
+        let drop_now = context.is_chain_locked();
         self.update_utxos(tx, account_match, context);
         #[cfg(not(feature = "keep-finalized-transactions"))]
         if drop_now {
@@ -419,8 +419,8 @@ impl ManagedCoreFundsAccount {
         // a wallet rescan from storage), drop the full record now and
         // keep only the txid in `finalized_txids`. No-op when the
         // feature is on (we want to keep the full record).
-        #[allow(unused_variables)]
-        let drop_now = context.is_finalized_in_block();
+        #[cfg(not(feature = "keep-finalized-transactions"))]
+        let drop_now = context.is_chain_locked();
         self.update_utxos(tx, account_match, context);
         #[cfg(not(feature = "keep-finalized-transactions"))]
         if drop_now {
@@ -672,10 +672,6 @@ impl ManagedAccountTrait for ManagedCoreFundsAccount {
         self.keys.transaction_is_finalized(txid)
     }
 
-    fn transaction_is_finalized_in_block(&self, txid: &Txid) -> bool {
-        self.keys.transaction_is_finalized_in_block(txid)
-    }
-
     fn monitor_revision(&self) -> u64 {
         self.keys.monitor_revision()
     }

key-wallet/src/managed_account/managed_core_keys_account.rs

@@ -88,8 +88,9 @@ impl ManagedCoreKeysAccount {
     /// is OFF (the default). Called when a transaction transitions into
     /// `InChainLockedBlock` — the record's information is no longer
     /// expected to change, so we save memory by replacing it with a
-    /// txid-only entry. [`Self::has_transaction`] keeps reporting it as
-    /// known, and [`Self::transaction_is_finalized_in_block`] keeps
+    /// txid-only entry. [`ManagedAccountTrait::has_transaction`] keeps
+    /// reporting it as known, and
+    /// [`ManagedAccountTrait::transaction_is_finalized`] keeps
     /// returning `true`.
     ///
     /// With the feature on the full record stays in `transactions`
@@ -181,47 +182,37 @@ impl ManagedAccountTrait for ManagedCoreKeysAccount {
         &mut self.transactions
     }
 
+    /// With the `keep-finalized-transactions` feature ON, every record
+    /// we have ever processed stays in `transactions` — that map is the
+    /// authoritative dedup set.
+    #[cfg(feature = "keep-finalized-transactions")]
     fn has_transaction(&self, txid: &Txid) -> bool {
-        if self.transactions.contains_key(txid) {
-            return true;
-        }
-        // Under the default feature configuration, the record may have
-        // been pruned; the txid stays in `finalized_txids`. With the
-        // feature on, every record stays in `transactions` so the first
-        // check above is exhaustive.
-        #[cfg(not(feature = "keep-finalized-transactions"))]
-        {
-            return self.finalized_txids.contains(txid);
-        }
-        #[allow(unreachable_code)]
-        false
+        self.transactions.contains_key(txid)
+    }
+
+    /// With the feature OFF (the default), chainlocked records are
+    /// pruned from `transactions` and only their txids are retained in
+    /// `finalized_txids`. Both sets need to be consulted.
+    #[cfg(not(feature = "keep-finalized-transactions"))]
+    fn has_transaction(&self, txid: &Txid) -> bool {
+        self.transactions.contains_key(txid) || self.finalized_txids.contains(txid)
     }
 
+    /// With the feature ON, finalized records live in `transactions`,
+    /// so we resolve the answer purely off the live record's context.
+    #[cfg(feature = "keep-finalized-transactions")]
     fn transaction_is_finalized(&self, txid: &Txid) -> bool {
-        if let Some(r) = self.transactions.get(txid) {
-            return r.context.is_finalized();
-        }
-        // Record was pruned; only chainlocked txids ever land in
-        // `finalized_txids`, and chainlocked counts as finalized.
-        #[cfg(not(feature = "keep-finalized-transactions"))]
-        {
-            return self.finalized_txids.contains(txid);
-        }
-        #[allow(unreachable_code)]
-        false
+        self.transactions.get(txid).is_some_and(|r| r.context.is_chain_locked())
     }
 
-    fn transaction_is_finalized_in_block(&self, txid: &Txid) -> bool {
-        if let Some(r) = self.transactions.get(txid) {
-            return r.context.is_finalized_in_block();
-        }
-        // Same logic — `finalized_txids` only contains chainlocked txs.
-        #[cfg(not(feature = "keep-finalized-transactions"))]
-        {
-            return self.finalized_txids.contains(txid);
-        }
-        #[allow(unreachable_code)]
-        false
+    /// With the feature OFF, chainlocked records are dropped from
+    /// `transactions` and only their txids are retained in
+    /// `finalized_txids`. A live record can never satisfy this check
+    /// (it would have been pruned at the chainlock event), so the only
+    /// `true` answer comes from the txid set.
+    #[cfg(not(feature = "keep-finalized-transactions"))]
+    fn transaction_is_finalized(&self, txid: &Txid) -> bool {
+        self.finalized_txids.contains(txid)
     }
 
     fn monitor_revision(&self) -> u64 {

key-wallet/src/tests/keep_finalized_transactions_tests.rs

@@ -10,6 +10,10 @@
 //!   for dedup. IS-locked-but-not-yet-chainlocked records still live in
 //!   the map so we don't lose the block-confirmation event when it
 //!   arrives.
+//!
+//! "Finalized" in this crate means *chainlocked* — see
+//! [`crate::transaction_checking::TransactionContext::is_chain_locked`].
+//! IS-lock alone is **not** finality.
 
 use crate::{
     managed_account::managed_account_trait::ManagedAccountTrait,
@@ -35,7 +39,7 @@ async fn test_chainlocked_record_kept_when_feature_on() {
     assert!(ctx.bip44_account().has_transaction(&txid));
     assert!(ctx.bip44_account().transactions().contains_key(&txid));
 
-    // InBlock → record still there, finalized-in-block stays false
+    // InBlock → record still there, finalization stays false
     let block_hash = BlockHash::from_slice(&[7u8; 32]).expect("hash");
     let _ = ctx
         .check_transaction(
@@ -44,7 +48,7 @@ async fn test_chainlocked_record_kept_when_feature_on() {
         )
         .await;
     assert!(ctx.bip44_account().has_transaction(&txid));
-    assert!(!ctx.bip44_account().transaction_is_finalized_in_block(&txid));
+    assert!(!ctx.bip44_account().transaction_is_finalized(&txid));
 
     // InChainLockedBlock → record MUST still live in the map.
     let _ = ctx
@@ -55,7 +59,6 @@ async fn test_chainlocked_record_kept_when_feature_on() {
         .await;
     assert!(ctx.bip44_account().has_transaction(&txid));
     assert!(ctx.bip44_account().transaction_is_finalized(&txid));
-    assert!(ctx.bip44_account().transaction_is_finalized_in_block(&txid));
     assert!(
         ctx.bip44_account().transactions().contains_key(&txid),
         "with the feature ON the record must stay in the map after chainlock"
@@ -78,7 +81,7 @@ async fn test_chainlocked_record_dropped_when_feature_off() {
     assert!(ctx.bip44_account().transactions().contains_key(&txid));
 
     // InChainLockedBlock → record dropped, but `has_transaction` and
-    // `transaction_is_finalized*` still report the tx via the txid set.
+    // `transaction_is_finalized` still report the tx via the txid set.
     let block_hash = BlockHash::from_slice(&[7u8; 32]).expect("hash");
     let _ = ctx
         .check_transaction(
@@ -92,14 +95,13 @@ async fn test_chainlocked_record_dropped_when_feature_off() {
     );
     assert!(ctx.bip44_account().has_transaction(&txid));
     assert!(ctx.bip44_account().transaction_is_finalized(&txid));
-    assert!(ctx.bip44_account().transaction_is_finalized_in_block(&txid));
 }
 
-/// IS-lock alone is "soft" finalized but not "finalized in block". The
-/// record must NOT be dropped when feature is OFF because we still need
-/// the in-memory record to absorb the eventual block-confirmation
-/// event (height / block hash). This guards against the pre-review bug
-/// where dropping on IS-lock lost block-confirmation tracking.
+/// IS-lock is **not** finalization. The record must NOT be dropped when
+/// the feature is OFF because we still need the in-memory record to
+/// absorb the eventual block-confirmation event (height / block hash).
+/// This guards against the pre-review bug where dropping on IS-lock
+/// lost block-confirmation tracking.
 #[cfg(not(feature = "keep-finalized-transactions"))]
 #[tokio::test]
 async fn test_islocked_record_kept_when_feature_off() {
@@ -113,12 +115,8 @@ async fn test_islocked_record_kept_when_feature_off() {
 
     assert!(ctx.bip44_account().has_transaction(&txid));
     assert!(
-        ctx.bip44_account().transaction_is_finalized(&txid),
-        "IS-lock counts as soft-finalized"
-    );
-    assert!(
-        !ctx.bip44_account().transaction_is_finalized_in_block(&txid),
-        "IS-lock is not the strict block-finalization the drop check uses"
+        !ctx.bip44_account().transaction_is_finalized(&txid),
+        "IS-lock alone is not finalization — only a chainlock counts"
     );
     assert!(
         ctx.bip44_account().transactions().contains_key(&txid),
@@ -129,8 +127,8 @@ async fn test_islocked_record_kept_when_feature_off() {
 
 /// IS-lock first, then a chainlocked block: the record must drop only at
 /// the chainlock step. We also assert that the chainlock event still
-/// "lands" — `transaction_is_finalized_in_block` must report `true` when
-/// asked via the txid set.
+/// "lands" — `transaction_is_finalized` must report `true` when asked
+/// via the txid set.
 #[cfg(not(feature = "keep-finalized-transactions"))]
 #[tokio::test]
 async fn test_islocked_then_chainlocked_drops_at_chainlock() {
@@ -152,5 +150,5 @@ async fn test_islocked_then_chainlocked_drops_at_chainlock() {
         .await;
     assert!(!ctx.bip44_account().transactions().contains_key(&txid), "dropped at chainlock");
     assert!(ctx.bip44_account().has_transaction(&txid));
-    assert!(ctx.bip44_account().transaction_is_finalized_in_block(&txid));
+    assert!(ctx.bip44_account().transaction_is_finalized(&txid));
 }

key-wallet/src/transaction_checking/transaction_context.rs

@@ -73,30 +73,16 @@ impl TransactionContext {
         matches!(self, TransactionContext::InstantSend(_))
     }
 
-    /// Returns whether the transaction is in a "finalized" state — i.e. it
-    /// has either an InstantSend lock or a ChainLock and is no longer
-    /// expected to change state.
-    ///
-    /// This is the *soft* finality check. It treats both IS-lock and
-    /// ChainLock as final. Use [`Self::is_finalized_in_block`] for the
-    /// stricter "fully confirmed in a chainlocked block" answer that
-    /// drives memory-pruning decisions.
-    pub fn is_finalized(&self) -> bool {
-        matches!(
-            self,
-            TransactionContext::InstantSend(_) | TransactionContext::InChainLockedBlock(_)
-        )
-    }
-
     /// Returns whether the transaction has been mined in a block that is
-    /// itself chainlocked — the strongest finality signal we have.
+    /// itself chainlocked — the strongest finality signal we have, and
+    /// the only one we treat as truly "finalized".
     ///
     /// `InBlock` alone is not enough (the block can still be reorganized
-    /// out), and `InstantSend` alone is not enough either (the surrounding
-    /// block confirmation may still arrive and write the height /
-    /// block hash before the chainlock catches up). Only
+    /// out), and `InstantSend` alone is not enough either (the
+    /// surrounding block confirmation may still arrive and write the
+    /// height / block hash before the chainlock catches up). Only
     /// `InChainLockedBlock` qualifies.
-    pub fn is_finalized_in_block(&self) -> bool {
+    pub fn is_chain_locked(&self) -> bool {
         matches!(self, TransactionContext::InChainLockedBlock(_))
     }
 

key-wallet/src/transaction_checking/wallet_checker.rs

@@ -88,7 +88,7 @@ impl WalletTransactionChecker for ManagedWalletInfo {
                 }
                 // Only accept IS transitions for unconfirmed transactions.
                 // A chainlocked tx may have had its full record dropped
-                // under the default feature config — `transaction_is_finalized_in_block`
+                // under the default feature config — `transaction_is_finalized`
                 // catches that case via `finalized_txids` and the in-map
                 // record check covers `InBlock`.
                 let already_confirmed = result.affected_accounts.iter().any(|am| {
@@ -97,7 +97,7 @@ impl WalletTransactionChecker for ManagedWalletInfo {
                     else {
                         return false;
                     };
-                    if account.transaction_is_finalized_in_block(&txid) {
+                    if account.transaction_is_finalized(&txid) {
                         return true;
                     }
                     account.transactions().get(&txid).is_some_and(|r| r.is_confirmed())