From 7bc3d5e931195d2bde87a3c914e6624f9aabab0a Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Wed, 6 May 2026 16:39:46 +0700 Subject: [PATCH 1/4] feat(key-wallet): add `keep-finalized-transactions` feature MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit By default, records of chainlocked transactions are now dropped from each managed account's in-memory `transactions` map; only their txids are retained (in a new `finalized_txids` set on `ManagedCoreKeysAccount`) to keep dedup, `has_transaction`, and finality queries working. The opt-in `keep-finalized-transactions` Cargo feature reverts to the old behavior — every processed transaction stays in the map for the wallet's lifetime. The drop is driven off `TransactionContext::is_finalized_in_block` (chainlock only), not `is_finalized` (chainlock or IS-lock), so IS-locked records survive long enough to absorb the surrounding block-confirmation event (xdustinface review on #709). `confirm_transaction` now returns `Option` so callers always observe the record even when it's about to be dropped. The feature is forwarded through `key-wallet-manager` and `key-wallet-ffi`. Three FFI accessors that walk the full `transactions` map (`managed_core_account_get_transaction_count`, `managed_core_account_get_transactions`, `managed_core_account_free_transactions`) are gated to the feature because they would otherwise return a partial history. Co-Authored-By: Claude Opus 4.7 (1M context) --- key-wallet-ffi/Cargo.toml | 10 ++ key-wallet-ffi/FFI_API.md | 10 +- key-wallet-ffi/src/managed_account.rs | 48 +++++- key-wallet-manager/Cargo.toml | 7 + key-wallet/Cargo.toml | 13 ++ .../managed_account/managed_account_trait.rs | 29 ++++ .../managed_core_funds_account.rs | 88 ++++++++-- .../managed_core_keys_account.rs | 90 +++++++++- .../keep_finalized_transactions_tests.rs | 156 ++++++++++++++++++ key-wallet/src/tests/mod.rs | 2 + .../transaction_context.rs | 27 +++ .../transaction_checking/wallet_checker.rs | 45 +++-- 12 files changed, 483 insertions(+), 42 deletions(-) create mode 100644 key-wallet/src/tests/keep_finalized_transactions_tests.rs diff --git a/key-wallet-ffi/Cargo.toml b/key-wallet-ffi/Cargo.toml index 3237e0579..203ec8f65 100644 --- a/key-wallet-ffi/Cargo.toml +++ b/key-wallet-ffi/Cargo.toml @@ -18,6 +18,16 @@ bip38 = ["key-wallet/bip38"] bincode = ["key-wallet/bincode", "key-wallet-manager/bincode"] eddsa = ["dashcore/eddsa", "key-wallet/eddsa"] bls = ["dashcore/bls", "key-wallet/bls"] +# Forward to `key-wallet/keep-finalized-transactions` (via key-wallet-manager). +# With this on, every processed transaction (including chainlocked ones) +# stays in the in-memory `transactions` map for the wallet's lifetime. +# With it off (the default), records of chainlocked transactions are +# dropped and only their txids are kept (in `finalized_txids`) for dedup. +# See `key-wallet`'s feature documentation for details. +keep-finalized-transactions = [ + "key-wallet/keep-finalized-transactions", + "key-wallet-manager/keep-finalized-transactions", +] [dependencies] key-wallet = { path = "../key-wallet" } diff --git a/key-wallet-ffi/FFI_API.md b/key-wallet-ffi/FFI_API.md index 8a5163215..3b7a7be0a 100644 --- a/key-wallet-ffi/FFI_API.md +++ b/key-wallet-ffi/FFI_API.md @@ -230,7 +230,7 @@ Functions: 108 | `managed_account_collection_summary_data` | Get structured account collection summary data for managed collection ... | managed_account_collection | | `managed_account_collection_summary_free` | Free a managed account collection summary and all its allocated memory #... | managed_account_collection | | `managed_core_account_free` | Free a managed account handle # Safety - `account` must be a valid pointer... | managed_account | -| `managed_core_account_free_transactions` | Free transactions array returned by managed_core_account_get_transactions #... | managed_account | +| `managed_core_account_free_transactions` | Free transactions array returned by managed_core_account_get_transactions ... | managed_account | | `managed_core_account_get_account_type` | Get the account type of a managed account # Safety - `account` must be a... | managed_account | | `managed_core_account_get_address_pool` | Get an address pool from a managed account by type This function returns... | managed_account | | `managed_core_account_get_balance` | Get the balance of a managed account # Safety - `account` must be a valid... | managed_account | @@ -238,7 +238,7 @@ Functions: 108 | `managed_core_account_get_index` | Get the account index from a managed account Returns the primary account... | managed_account | | `managed_core_account_get_internal_address_pool` | Get the internal address pool from a managed account This function returns... | managed_account | | `managed_core_account_get_network` | Get the network of a managed account # Safety - `account` must be a valid... | managed_account | -| `managed_core_account_get_transaction_count` | Get the number of transactions in a managed account # Safety - `account`... | managed_account | +| `managed_core_account_get_transaction_count` | Get the number of transactions in a managed account Only available with the... | managed_account | | `managed_core_account_get_transactions` | Get all transactions from a managed account Returns an array of... | managed_account | | `managed_core_account_get_utxo_count` | Get the number of UTXOs in a managed account # Safety - `account` must be... | managed_account | | `managed_platform_account_free` | Free a managed platform account handle # Safety - `account` must be a... | managed_account | @@ -3047,7 +3047,7 @@ managed_core_account_free_transactions(transactions: *mut FFITransactionRecord, ``` **Description:** -Free transactions array returned by managed_core_account_get_transactions # Safety - `transactions` must be a pointer returned by `managed_core_account_get_transactions` - `count` must be the count returned by `managed_core_account_get_transactions` - This function must only be called once per allocation +Free transactions array returned by managed_core_account_get_transactions Only available with the `keep-finalized-transactions` Cargo feature, in which configuration `managed_core_account_get_transactions` is also available — the two functions are paired. # Safety - `transactions` must be a pointer returned by `managed_core_account_get_transactions` - `count` must be the count returned by `managed_core_account_get_transactions` - This function must only be called once per allocation **Safety:** - `transactions` must be a pointer returned by `managed_core_account_get_transactions` - `count` must be the count returned by `managed_core_account_get_transactions` - This function must only be called once per allocation @@ -3175,7 +3175,7 @@ managed_core_account_get_transaction_count(account: *const FFIManagedCoreAccount ``` **Description:** -Get the number of transactions in a managed account # Safety - `account` must be a valid pointer to an FFIManagedCoreAccount instance +Get the number of transactions in a managed account Only available with the `keep-finalized-transactions` Cargo feature. With the feature off (the default), records of chainlocked transactions are dropped from the in-memory map, so the count would not reflect the full history — the function is intentionally not exposed. # Safety - `account` must be a valid pointer to an FFIManagedCoreAccount instance **Safety:** - `account` must be a valid pointer to an FFIManagedCoreAccount instance @@ -3191,7 +3191,7 @@ managed_core_account_get_transactions(account: *const FFIManagedCoreAccount, tra ``` **Description:** -Get all transactions from a managed account Returns an array of FFITransactionRecord structures. # Safety - `account` must be a valid pointer to an FFIManagedCoreAccount instance - `transactions_out` must be a valid pointer to receive the transactions array pointer - `count_out` must be a valid pointer to receive the count - The caller must free the returned array using `managed_core_account_free_transactions` +Get all transactions from a managed account Returns an array of FFITransactionRecord structures. Only available with the `keep-finalized-transactions` Cargo feature. With the feature off (the default), records of chainlocked transactions are dropped from the in-memory map, so this would only return a partial history — the function is intentionally not exposed. # Safety - `account` must be a valid pointer to an FFIManagedCoreAccount instance - `transactions_out` must be a valid pointer to receive the transactions array pointer - `count_out` must be a valid pointer to receive the count - The caller must free the returned array using `managed_core_account_free_transactions` **Safety:** - `account` must be a valid pointer to an FFIManagedCoreAccount instance - `transactions_out` must be a valid pointer to receive the transactions array pointer - `count_out` must be a valid pointer to receive the count - The caller must free the returned array using `managed_core_account_free_transactions` diff --git a/key-wallet-ffi/src/managed_account.rs b/key-wallet-ffi/src/managed_account.rs index 34330136a..9595526a1 100644 --- a/key-wallet-ffi/src/managed_account.rs +++ b/key-wallet-ffi/src/managed_account.rs @@ -7,6 +7,7 @@ use dash_network::ffi::FFINetwork; use dashcore::hashes::Hash; use std::os::raw::{c_char, c_uint}; +#[cfg(feature = "keep-finalized-transactions")] use std::ptr::slice_from_raw_parts_mut; use std::sync::Arc; @@ -614,8 +615,14 @@ pub unsafe extern "C" fn managed_core_account_get_balance( true } +#[cfg(feature = "keep-finalized-transactions")] /// Get the number of transactions in a managed account /// +/// Only available with the `keep-finalized-transactions` Cargo feature. With +/// the feature off (the default), records of chainlocked transactions are +/// dropped from the in-memory map, so the count would not reflect the full +/// history — the function is intentionally not exposed. +/// /// # Safety /// /// - `account` must be a valid pointer to an FFIManagedCoreAccount instance @@ -914,10 +921,16 @@ impl Drop for FFITransactionRecord { } } +#[cfg(feature = "keep-finalized-transactions")] /// Get all transactions from a managed account /// /// Returns an array of FFITransactionRecord structures. /// +/// Only available with the `keep-finalized-transactions` Cargo feature. With +/// the feature off (the default), records of chainlocked transactions are +/// dropped from the in-memory map, so this would only return a partial +/// history — the function is intentionally not exposed. +/// /// # Safety /// /// - `account` must be a valid pointer to an FFIManagedCoreAccount instance @@ -951,8 +964,13 @@ pub unsafe extern "C" fn managed_core_account_get_transactions( true } +#[cfg(feature = "keep-finalized-transactions")] /// Free transactions array returned by managed_core_account_get_transactions /// +/// Only available with the `keep-finalized-transactions` Cargo feature, in +/// which configuration `managed_core_account_get_transactions` is also +/// available — the two functions are paired. +/// /// # Safety /// /// - `transactions` must be a pointer returned by `managed_core_account_get_transactions` @@ -1547,10 +1565,13 @@ pub unsafe extern "C" fn managed_platform_account_result_free_error( mod tests { use super::*; use crate::address_pool::address_pool_free; + use crate::types::{FFIAccountCreationOptionType, FFIWalletAccountCreationOptions}; + // These types are only used by the FFITransactionRecord tests, which run + // only when transactions stay in memory. + #[cfg(feature = "keep-finalized-transactions")] use crate::types::{ - FFIAccountCreationOptionType, FFIBlockInfo, FFIInputDetail, FFIOutputDetail, FFIOutputRole, - FFITransactionContext, FFITransactionContextType, FFITransactionDirection, - FFITransactionType, FFIWalletAccountCreationOptions, + FFIBlockInfo, FFIInputDetail, FFIOutputDetail, FFIOutputRole, FFITransactionContext, + FFITransactionContextType, FFITransactionDirection, FFITransactionType, }; use crate::wallet_manager::{ wallet_manager_add_wallet_from_mnemonic_with_options, wallet_manager_create, @@ -1828,9 +1849,15 @@ mod tests { assert_eq!(balance_out.locked, 0); assert_eq!(balance_out.total, 0); - // Test get_transaction_count - let tx_count = managed_core_account_get_transaction_count(account); - assert_eq!(tx_count, 0); // Initially no transactions + // Test get_transaction_count (only available with the + // `keep-finalized-transactions` feature; without it the function + // is not exposed because chainlocked records are pruned and the + // count would be incomplete) + #[cfg(feature = "keep-finalized-transactions")] + { + let tx_count = managed_core_account_get_transaction_count(account); + assert_eq!(tx_count, 0); // Initially no transactions + } // Test get_utxo_count let utxo_count = managed_core_account_get_utxo_count(account); @@ -1858,8 +1885,11 @@ mod tests { let account_type = managed_core_account_get_account_type(ptr::null(), &mut index_out); assert_eq!(account_type, FFIAccountKind::StandardBIP44); // Default type - let tx_count = managed_core_account_get_transaction_count(ptr::null()); - assert_eq!(tx_count, 0); + #[cfg(feature = "keep-finalized-transactions")] + { + let tx_count = managed_core_account_get_transaction_count(ptr::null()); + assert_eq!(tx_count, 0); + } let utxo_count = managed_core_account_get_utxo_count(ptr::null()); assert_eq!(utxo_count, 0); @@ -2080,6 +2110,7 @@ mod tests { } } + #[cfg(feature = "keep-finalized-transactions")] #[test] fn test_free_transactions_null_safety() { unsafe { @@ -2088,6 +2119,7 @@ mod tests { } } + #[cfg(feature = "keep-finalized-transactions")] #[test] fn test_ffi_transaction_record_roundtrip() { let mut records = Vec::new(); diff --git a/key-wallet-manager/Cargo.toml b/key-wallet-manager/Cargo.toml index 1b7347c05..dd0b68849 100644 --- a/key-wallet-manager/Cargo.toml +++ b/key-wallet-manager/Cargo.toml @@ -14,6 +14,13 @@ bincode = ["key-wallet/bincode", "dep:bincode"] test-utils = ["key-wallet/test-utils"] bls = ["key-wallet/bls"] eddsa = ["key-wallet/eddsa"] +# Forward to `key-wallet/keep-finalized-transactions`. With this on, +# every processed transaction (including chainlocked ones) stays in +# the in-memory `transactions` map for the wallet's lifetime. With +# it off (the default), records of chainlocked transactions are +# dropped and only their txids are kept (in `finalized_txids`) for +# dedup. See `key-wallet`'s feature documentation for details. +keep-finalized-transactions = ["key-wallet/keep-finalized-transactions"] [dependencies] key-wallet = { path = "../key-wallet", default-features = false } diff --git a/key-wallet/Cargo.toml b/key-wallet/Cargo.toml index a19209a38..47299a6ad 100644 --- a/key-wallet/Cargo.toml +++ b/key-wallet/Cargo.toml @@ -16,6 +16,19 @@ bip38 = ["scrypt", "aes", "bs58", "rand"] eddsa = ["dashcore/eddsa"] bls = ["dashcore/bls"] test-utils = ["dashcore/test-utils"] +# Keep the full `TransactionRecord` for transactions that have reached a +# "finalized in block" state — i.e. they have a ChainLock confirming the +# block they were mined in. With this feature ON, every processed +# transaction (including ones already chainlocked) stays in the +# `transactions` map for the wallet's lifetime; finalization is implicit +# in the stored `TransactionContext`. With it OFF (the default), records +# of chainlocked transactions are dropped from the map and a per-account +# `finalized_txids: HashSet` retains only their txids so +# `has_transaction` / `transaction_is_finalized_in_block` still answer +# correctly. An InstantSend lock alone does NOT trigger record dropping +# — we keep the record around so the surrounding block confirmation can +# still write its height / block hash before the chainlock arrives. +keep-finalized-transactions = [] [dependencies] internals = { path = "../internals", package = "dashcore-private" } diff --git a/key-wallet/src/managed_account/managed_account_trait.rs b/key-wallet/src/managed_account/managed_account_trait.rs index d9fc9c2ce..7be35c31a 100644 --- a/key-wallet/src/managed_account/managed_account_trait.rs +++ b/key-wallet/src/managed_account/managed_account_trait.rs @@ -44,6 +44,35 @@ pub trait ManagedAccountTrait { /// Get mutable transactions fn transactions_mut(&mut self) -> &mut BTreeMap; + /// 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`. + 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`]). + /// + /// `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; + /// Return the current monitor revision. /// /// Bumped whenever the monitored address set changes (e.g. new addresses diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index 492c6a0f2..2e53ee7ed 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -229,22 +229,46 @@ impl ManagedCoreFundsAccount { } } - /// Re-process an existing transaction with updated context (e.g., mempool→block confirmation) - /// and potentially new address matches from gap limit rescans. + /// Re-process an existing transaction with updated context (e.g., + /// mempool→block confirmation) and potentially new address matches + /// from gap limit rescans. + /// + /// Returns `Some(record)` when the call results in a state change the + /// caller should surface (record newly inserted or context updated). + /// The record is cloned BEFORE any chainlock-driven pruning, so the + /// caller can always include it in an event even when the + /// `keep-finalized-transactions` Cargo feature is off and the record + /// is dropped from `transactions` immediately after. + /// + /// Returns `None` when: + /// - the tx is already finalized in a chainlocked block (record is + /// immutable; further events are redundant), or + /// - the existing record's context already matches and confirmation + /// status didn't change. pub(crate) fn confirm_transaction( &mut self, tx: &Transaction, account_match: &AccountMatch, context: TransactionContext, transaction_type: TransactionType, - ) -> bool { - if !self.keys.transactions().contains_key(&tx.txid()) { - self.record_transaction(tx, account_match, context, transaction_type); - return true; + ) -> Option { + let txid = tx.txid(); + + // Already finalized in a chainlocked block: the tx is immutable — + // no record update, no UTXO refresh, no event needed. + if self.keys.transaction_is_finalized_in_block(&txid) { + return None; + } + + if !self.keys.has_transaction(&txid) { + // Genuinely new sighting — delegate to record_transaction + // (which handles finalize-on-record itself). + let record = self.record_transaction(tx, account_match, context, transaction_type); + return Some(record); } let mut changed = false; - if let Some(tx_record) = self.keys.transactions_mut().get_mut(&tx.txid()) { + if let Some(tx_record) = self.keys.transactions_mut().get_mut(&txid) { debug_assert_eq!( tx_record.transaction_type, transaction_type, @@ -261,8 +285,28 @@ impl ManagedCoreFundsAccount { changed = !was_confirmed; } } + + // Capture the (possibly updated) record before any pruning so the + // caller can still emit it in an event. + let record_after = if changed { + self.keys.transactions().get(&txid).cloned() + } else { + None + }; + + // The chainlock is the trigger for dropping the full record under + // the default feature configuration; an IS-lock alone is *not* + // 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(); self.update_utxos(tx, account_match, context); - changed + #[cfg(not(feature = "keep-finalized-transactions"))] + if drop_now { + self.keys.drop_finalized_transaction(&txid); + } + record_after } /// Record a new transaction and update UTXOs for spendable account types @@ -368,9 +412,21 @@ impl ManagedCoreFundsAccount { ); let record = tx_record.clone(); - self.keys.transactions_mut().insert(tx.txid(), tx_record); - + let txid = tx.txid(); + self.keys.transactions_mut().insert(txid, tx_record); + + // If the very first sighting is already chainlocked (e.g. + // 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(); self.update_utxos(tx, account_match, context); + #[cfg(not(feature = "keep-finalized-transactions"))] + if drop_now { + self.keys.drop_finalized_transaction(&txid); + } + record } @@ -608,6 +664,18 @@ impl ManagedAccountTrait for ManagedCoreFundsAccount { self.keys.transactions_mut() } + fn has_transaction(&self, txid: &Txid) -> bool { + self.keys.has_transaction(txid) + } + + fn transaction_is_finalized(&self, txid: &Txid) -> bool { + 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() } diff --git a/key-wallet/src/managed_account/managed_core_keys_account.rs b/key-wallet/src/managed_account/managed_core_keys_account.rs index e42b76631..c6cf485ca 100644 --- a/key-wallet/src/managed_account/managed_core_keys_account.rs +++ b/key-wallet/src/managed_account/managed_core_keys_account.rs @@ -19,6 +19,8 @@ use dashcore::Txid; #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; +#[cfg(not(feature = "keep-finalized-transactions"))] +use std::collections::HashSet; /// Managed core keys account with mutable state but no funds tracking. /// @@ -37,8 +39,30 @@ pub struct ManagedCoreKeysAccount { managed_account_type: ManagedAccountType, /// Network this account belongs to network: Network, - /// Transaction history for this account + /// Transaction history for this account. + /// + /// With the `keep-finalized-transactions` Cargo feature ON, every + /// processed transaction lives here for the wallet's lifetime — + /// including ones that have been chainlocked. With the feature OFF + /// (the default), records of chainlocked transactions are dropped + /// from this map and only their txids are retained in + /// `finalized_txids` to bound memory growth. transactions: BTreeMap, + /// Txids of transactions that have been finalized in a chainlocked + /// block and whose full records have been dropped from + /// `transactions` to save memory. + /// + /// Only present when the `keep-finalized-transactions` Cargo feature + /// is OFF — with the feature on, finalized records stay in + /// `transactions` and there's no need for a separate set. + /// + /// Note: an InstantSend lock alone does NOT add a txid here. We + /// wait for the surrounding block to be chainlocked so the record + /// can absorb the block-confirmation event (height / block hash) + /// before being dropped. + #[cfg(not(feature = "keep-finalized-transactions"))] + #[cfg_attr(feature = "serde", serde(default))] + finalized_txids: HashSet, /// Revision counter incremented when the monitored address set changes /// (e.g. new addresses generated). Used to detect bloom filter staleness. #[cfg_attr(feature = "serde", serde(skip))] @@ -52,10 +76,31 @@ impl ManagedCoreKeysAccount { managed_account_type, network, transactions: BTreeMap::new(), + #[cfg(not(feature = "keep-finalized-transactions"))] + finalized_txids: HashSet::new(), monitor_revision: 0, } } + /// Drop the full record for `txid` and remember only its txid. + /// + /// Only defined when the `keep-finalized-transactions` Cargo feature + /// 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 + /// returning `true`. + /// + /// With the feature on the full record stays in `transactions` + /// indefinitely, so there's nothing to do — the function does not + /// exist in that mode. + #[cfg(not(feature = "keep-finalized-transactions"))] + pub(crate) fn drop_finalized_transaction(&mut self, txid: &Txid) { + self.finalized_txids.insert(*txid); + self.transactions.remove(txid); + } + /// Create a `ManagedCoreKeysAccount` from an [`Account`](super::super::Account). pub fn from_account(account: &super::super::Account) -> Self { let key_source = address_pool::KeySource::Public(account.account_xpub); @@ -136,6 +181,49 @@ impl ManagedAccountTrait for ManagedCoreKeysAccount { &mut self.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 + } + + 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 + } + + 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 + } + fn monitor_revision(&self) -> u64 { self.monitor_revision } diff --git a/key-wallet/src/tests/keep_finalized_transactions_tests.rs b/key-wallet/src/tests/keep_finalized_transactions_tests.rs new file mode 100644 index 000000000..37a511399 --- /dev/null +++ b/key-wallet/src/tests/keep_finalized_transactions_tests.rs @@ -0,0 +1,156 @@ +//! Tests for the `keep-finalized-transactions` Cargo feature. +//! +//! These tests assert the dual semantics of the feature: +//! +//! - With the feature ON, every processed transaction stays in the +//! in-memory `transactions` map for the wallet's lifetime, including +//! chainlocked ones. +//! - With the feature OFF (the default), records of chainlocked +//! transactions are dropped from the map and only their txids are kept +//! 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. + +use crate::{ + managed_account::managed_account_trait::ManagedAccountTrait, + test_utils::TestWalletContext, + transaction_checking::{BlockInfo, TransactionContext}, +}; +#[cfg(not(feature = "keep-finalized-transactions"))] +use dashcore::ephemerealdata::instant_lock::InstantLock; +use dashcore::hashes::Hash; +use dashcore::{BlockHash, Transaction}; + +/// Walks a single transaction through Mempool → InBlock → InChainLockedBlock +/// and asserts that the record survives the chainlock when the feature is ON. +#[cfg(feature = "keep-finalized-transactions")] +#[tokio::test] +async fn test_chainlocked_record_kept_when_feature_on() { + let mut ctx = TestWalletContext::new_random(); + let tx = Transaction::dummy(&ctx.receive_address, 0..1, &[150_000]); + let txid = tx.txid(); + + // Mempool → record exists + let _ = ctx.check_transaction(&tx, TransactionContext::Mempool).await; + assert!(ctx.bip44_account().has_transaction(&txid)); + assert!(ctx.bip44_account().transactions().contains_key(&txid)); + + // InBlock → record still there, finalized-in-block stays false + let block_hash = BlockHash::from_slice(&[7u8; 32]).expect("hash"); + let _ = ctx + .check_transaction( + &tx, + TransactionContext::InBlock(BlockInfo::new(100, block_hash, 1_700_000_000)), + ) + .await; + assert!(ctx.bip44_account().has_transaction(&txid)); + assert!(!ctx.bip44_account().transaction_is_finalized_in_block(&txid)); + + // InChainLockedBlock → record MUST still live in the map. + let _ = ctx + .check_transaction( + &tx, + TransactionContext::InChainLockedBlock(BlockInfo::new(100, block_hash, 1_700_000_000)), + ) + .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" + ); +} + +/// With the feature OFF (default) a chainlocked transaction's record is +/// dropped from the map; only the txid is retained for dedup. The +/// `has_transaction` / `transaction_is_finalized` queries must keep +/// working off the txid set. +#[cfg(not(feature = "keep-finalized-transactions"))] +#[tokio::test] +async fn test_chainlocked_record_dropped_when_feature_off() { + let mut ctx = TestWalletContext::new_random(); + let tx = Transaction::dummy(&ctx.receive_address, 0..1, &[150_000]); + let txid = tx.txid(); + + // Mempool → record exists in the map. + let _ = ctx.check_transaction(&tx, TransactionContext::Mempool).await; + 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. + let block_hash = BlockHash::from_slice(&[7u8; 32]).expect("hash"); + let _ = ctx + .check_transaction( + &tx, + TransactionContext::InChainLockedBlock(BlockInfo::new(100, block_hash, 1_700_000_000)), + ) + .await; + assert!( + !ctx.bip44_account().transactions().contains_key(&txid), + "with the feature OFF the chainlocked record must be dropped" + ); + 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. +#[cfg(not(feature = "keep-finalized-transactions"))] +#[tokio::test] +async fn test_islocked_record_kept_when_feature_off() { + let mut ctx = TestWalletContext::new_random(); + let tx = Transaction::dummy(&ctx.receive_address, 0..1, &[150_000]); + let txid = tx.txid(); + + let _ = ctx.check_transaction(&tx, TransactionContext::Mempool).await; + let _ = + ctx.check_transaction(&tx, TransactionContext::InstantSend(InstantLock::default())).await; + + 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" + ); + assert!( + ctx.bip44_account().transactions().contains_key(&txid), + "IS-locked records must survive so a later InBlock event can populate \ + block-confirmation info" + ); +} + +/// 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. +#[cfg(not(feature = "keep-finalized-transactions"))] +#[tokio::test] +async fn test_islocked_then_chainlocked_drops_at_chainlock() { + let mut ctx = TestWalletContext::new_random(); + let tx = Transaction::dummy(&ctx.receive_address, 0..1, &[200_000]); + let txid = tx.txid(); + + let _ = ctx.check_transaction(&tx, TransactionContext::Mempool).await; + let _ = + ctx.check_transaction(&tx, TransactionContext::InstantSend(InstantLock::default())).await; + assert!(ctx.bip44_account().transactions().contains_key(&txid), "still present after IS-lock"); + + let block_hash = BlockHash::from_slice(&[3u8; 32]).expect("hash"); + let _ = ctx + .check_transaction( + &tx, + TransactionContext::InChainLockedBlock(BlockInfo::new(42, block_hash, 1_700_000_000)), + ) + .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)); +} diff --git a/key-wallet/src/tests/mod.rs b/key-wallet/src/tests/mod.rs index d92850c79..4e5a01b72 100644 --- a/key-wallet/src/tests/mod.rs +++ b/key-wallet/src/tests/mod.rs @@ -16,6 +16,8 @@ mod edge_case_tests; mod integration_tests; +mod keep_finalized_transactions_tests; + mod managed_account_collection_tests; mod performance_tests; diff --git a/key-wallet/src/transaction_checking/transaction_context.rs b/key-wallet/src/transaction_checking/transaction_context.rs index 910f69763..ece1d74e6 100644 --- a/key-wallet/src/transaction_checking/transaction_context.rs +++ b/key-wallet/src/transaction_checking/transaction_context.rs @@ -73,6 +73,33 @@ 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. + /// + /// `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. + pub fn is_finalized_in_block(&self) -> bool { + matches!(self, TransactionContext::InChainLockedBlock(_)) + } + /// Returns the block info if confirmed. pub fn block_info(&self) -> Option<&BlockInfo> { match self { diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index f2856d818..bd13ed57e 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -72,7 +72,7 @@ impl WalletTransactionChecker for ManagedWalletInfo { if let Some(account) = self.accounts.get_by_account_type_match(&account_match.account_type_match) { - if account.transactions().contains_key(&txid) { + if account.has_transaction(&txid) { is_new = false; break; } @@ -86,12 +86,21 @@ impl WalletTransactionChecker for ManagedWalletInfo { if !self.instant_send_locks.insert(txid) { return result; } - // Only accept IS transitions for unconfirmed transactions + // 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` + // catches that case via `finalized_txids` and the in-map + // record check covers `InBlock`. let already_confirmed = result.affected_accounts.iter().any(|am| { - self.accounts - .get_by_account_type_match(&am.account_type_match) - .and_then(|a| a.transactions().get(&txid)) - .map_or(false, |r| r.is_confirmed()) + let Some(account) = + self.accounts.get_by_account_type_match(&am.account_type_match) + else { + return false; + }; + if account.transaction_is_finalized_in_block(&txid) { + return true; + } + account.transactions().get(&txid).is_some_and(|r| r.is_confirmed()) }); if already_confirmed { return result; @@ -151,15 +160,15 @@ impl WalletTransactionChecker for ManagedWalletInfo { result.new_records.push(record); result.state_modified = true; } else { - let existed_before = account.transactions().contains_key(&tx.txid()); - if account.confirm_transaction(tx, &account_match, context.clone(), tx_type) { + let existed_before = account.has_transaction(&tx.txid()); + if let Some(record) = + account.confirm_transaction(tx, &account_match, context.clone(), tx_type) + { result.state_modified = true; - if let Some(record) = account.transactions().get(&tx.txid()) { - if existed_before { - result.updated_records.push(record.clone()); - } else { - result.new_records.push(record.clone()); - } + if existed_before { + result.updated_records.push(record); + } else { + result.new_records.push(record); } } } @@ -1182,8 +1191,8 @@ mod tests { let block_context = TransactionContext::InBlock(BlockInfo::new(600, block_hash, 1700000000)); let tx_type = TransactionRouter::classify_transaction(&tx); - let changed = account.confirm_transaction(&tx, &account_match, block_context, tx_type); - assert!(changed, "Should return true when backfilling a missing record"); + let backfilled = account.confirm_transaction(&tx, &account_match, block_context, tx_type); + assert!(backfilled.is_some(), "Should return Some when backfilling a missing record"); // Verify the transaction was recorded with block context let record = account.transactions().get(&txid).expect("Should have backfilled record"); @@ -1233,8 +1242,8 @@ mod tests { .first_bip44_managed_account_mut() .expect("Should have BIP44 account"); let tx_type = TransactionRouter::classify_transaction(&tx); - let changed = account.confirm_transaction(&tx, &account_match, block_context, tx_type); - assert!(changed, "Should return true when confirming unconfirmed tx"); + let confirmed = account.confirm_transaction(&tx, &account_match, block_context, tx_type); + assert!(confirmed.is_some(), "Should return Some when confirming unconfirmed tx"); let record = account.transactions().get(&txid).expect("Should have record"); assert!(record.is_confirmed()); From 34c47eb26c08c8c7cc8212655ab731c5184843b8 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Wed, 6 May 2026 16:47:03 +0700 Subject: [PATCH 2/4] fix(dash-spv-ffi): enable `keep-finalized-transactions` for tests Integration tests under `dash-spv-ffi/tests/dashd_sync/` walk the full per-account transaction history (`managed_core_account_get_transactions`, `managed_core_account_get_transaction_count`, `managed_core_account_free_transactions`) to verify end-to-end wallet sync against a regtest dashd. These accessors are gated behind the `keep-finalized-transactions` feature on `key-wallet-ffi`, so under the default feature set they aren't compiled in and the test build fails to resolve the imports. Add `key-wallet-ffi` as a dev-dependency with the feature enabled. Cargo unifies features across the build graph at test time, which makes the gated accessors available in the test binaries without expanding the lib crate's default feature surface. Co-Authored-By: Claude Opus 4.7 (1M context) --- dash-spv-ffi/Cargo.toml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/dash-spv-ffi/Cargo.toml b/dash-spv-ffi/Cargo.toml index 7521ba57a..68254c60e 100644 --- a/dash-spv-ffi/Cargo.toml +++ b/dash-spv-ffi/Cargo.toml @@ -28,6 +28,12 @@ clap = { version = "4.5", features = ["derive"] } [dev-dependencies] dash-spv = { path = "../dash-spv", features = ["test-utils"] } +# Tests inspect per-account transaction history end-to-end (including +# chainlocked transactions), which requires the `keep-finalized-transactions` +# feature on `key-wallet-ffi`. Cargo unifies features across the build graph at +# test time, so this enables the gated FFI accessors in test builds without +# changing the lib's default feature surface. +key-wallet-ffi = { path = "../key-wallet-ffi", features = ["keep-finalized-transactions"] } serial_test = "3.0" tempfile = "3.8" From eb5afa239367a703e43d39c266d1582c8e534f9e Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Thu, 7 May 2026 16:22:05 +0700 Subject: [PATCH 3/4] refactor(key-wallet): align finalization with chainlock-only semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../managed_account/managed_account_trait.rs | 25 +++---- .../managed_core_funds_account.rs | 16 ++--- .../managed_core_keys_account.rs | 65 ++++++++----------- .../keep_finalized_transactions_tests.rs | 36 +++++----- .../transaction_context.rs | 26 ++------ .../transaction_checking/wallet_checker.rs | 4 +- 6 files changed, 67 insertions(+), 105 deletions(-) diff --git a/key-wallet/src/managed_account/managed_account_trait.rs b/key-wallet/src/managed_account/managed_account_trait.rs index 7be35c31a..bab178060 100644 --- a/key-wallet/src/managed_account/managed_account_trait.rs +++ b/key-wallet/src/managed_account/managed_account_trait.rs @@ -45,25 +45,16 @@ pub trait ManagedAccountTrait { fn transactions_mut(&mut self) -> &mut BTreeMap; /// 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 @@ -71,7 +62,7 @@ pub trait ManagedAccountTrait { /// 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. /// diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index 2e53ee7ed..2402e36d1 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -254,9 +254,9 @@ impl ManagedCoreFundsAccount { ) -> Option { 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() } diff --git a/key-wallet/src/managed_account/managed_core_keys_account.rs b/key-wallet/src/managed_account/managed_core_keys_account.rs index c6cf485ca..8a520cc22 100644 --- a/key-wallet/src/managed_account/managed_core_keys_account.rs +++ b/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 { diff --git a/key-wallet/src/tests/keep_finalized_transactions_tests.rs b/key-wallet/src/tests/keep_finalized_transactions_tests.rs index 37a511399..602d9c937 100644 --- a/key-wallet/src/tests/keep_finalized_transactions_tests.rs +++ b/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)); } diff --git a/key-wallet/src/transaction_checking/transaction_context.rs b/key-wallet/src/transaction_checking/transaction_context.rs index ece1d74e6..2377ecf19 100644 --- a/key-wallet/src/transaction_checking/transaction_context.rs +++ b/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(_)) } diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index bd13ed57e..d29844a9b 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/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()) From 13ed52c1cba9562eee301195bed7d76d6b78f324 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Thu, 7 May 2026 16:28:24 +0700 Subject: [PATCH 4/4] docs(key-wallet): fix stale `transaction_is_finalized_in_block` mention The previous commit renamed the trait method to `transaction_is_finalized` but missed this Cargo.toml feature-doc reference. Caught by CodeRabbit on PR #733. Co-Authored-By: Claude Opus 4.7 (1M context) --- key-wallet/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/key-wallet/Cargo.toml b/key-wallet/Cargo.toml index 47299a6ad..2a0cbf779 100644 --- a/key-wallet/Cargo.toml +++ b/key-wallet/Cargo.toml @@ -24,7 +24,7 @@ test-utils = ["dashcore/test-utils"] # in the stored `TransactionContext`. With it OFF (the default), records # of chainlocked transactions are dropped from the map and a per-account # `finalized_txids: HashSet` retains only their txids so -# `has_transaction` / `transaction_is_finalized_in_block` still answer +# `has_transaction` / `transaction_is_finalized` still answer # correctly. An InstantSend lock alone does NOT trigger record dropping # — we keep the record around so the surrounding block confirmation can # still write its height / block hash before the chainlock arrives.