fix: distinguish between new and existing transactions in wallet

[xdustinface]

BlockProcessingResult now distinguishes between new and existing transactions. When blocks are rescanned (e.g., after gap limit maintenance discovers new addresses), transactions that already exist in the wallet are categorized as existing rather than new.

Summary by CodeRabbit

• Refactor

  • Block processing now separates newly discovered vs previously known transactions and exposes totals and an iterator for all relevant txids; logging reports totals plus per-transaction details.
  • Transaction and check results now propagate an explicit new-vs-existing flag through processing.

• Bug Fixes

  • Rescans no longer duplicate transactions or inflate wallet totals; existing transactions are recognized.

• Tests

  • Added/updated tests validating new vs existing classification, rescan behavior, and stable history/counts.

• New Features

  • Added lookup to resolve accounts by type/match for improved account handling.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Splits relevant transactions into newly-discovered vs existing: replaces relevant_txids with new_txids and existing_txids, adds relevant_txids() and relevant_tx_count() accessors, propagates is_new_transaction through transaction checks, updates processing, logging, tests, and adds a managed-account lookup helper (duplicate insertion present).

Changes

Cohort / File(s)	Summary
BlockProcessingResult refactor key-wallet-manager/src/wallet_interface.rs, key-wallet-manager/src/test_utils/wallet.rs	Remove relevant_txids; add pub new_txids: Vec<Txid> and pub existing_txids: Vec<Txid>; add relevant_txids() iterator and relevant_tx_count() accessor; test util updated to populate new fields.
Block processing & routing key-wallet-manager/src/wallet_manager/process_block.rs, key-wallet-manager/src/wallet_manager/mod.rs	process_block now routes txids into new_txids or existing_txids based on is_new_transaction; CheckTransactionsResult gains is_new_transaction: bool and is propagated.
Transaction checking logic key-wallet/src/transaction_checking/account_checker.rs, key-wallet/src/transaction_checking/wallet_checker.rs	Add pub is_new_transaction: bool to TransactionCheckResult; wallet checker inspects existing account history during update to mark rescanned transactions as existing; metadata increments only for new transactions; tests added for rescan behavior.
SPV message handling / logging dash-spv/src/sync/message_handlers.rs	Replace relevant_txids checks with relevant_tx_count() gating; log updated to include total/new/existing counts; iterate via relevant_txids() accessor.
Integration & unit tests key-wallet-manager/tests/spv_integration_tests.rs, key-wallet/src/transaction_checking/wallet_checker.rs (tests)	Tests updated to assert new_txids and existing_txids semantics; added test verifying rescans mark transactions as existing and do not increase stored history/metadata.
Managed account lookup helper key-wallet/src/managed_account/managed_account_collection.rs	Add pub fn get_by_account_type_match(&AccountTypeMatch) -> Option<&ManagedAccount> to resolve account targets from AccountTypeMatch; note: method was inserted twice (duplicate).

Sequence Diagram(s)

sequenceDiagram
  participant SPV as SPV
  participant WM as WalletManager
  participant WC as WalletChecker
  participant MA as ManagedAccounts
  participant DB as Database

  SPV->>WM: process_block(block)
  WM->>WC: check_transaction(tx, update_state)
  WC->>MA: determine affected accounts (get_by_account_type_match)
  MA-->>WC: affected_accounts
  WC->>DB: query if tx exists in affected_accounts
  DB-->>WC: exists? (yes/no)
  WC-->>WM: TransactionCheckResult { is_new_transaction, details }
  WM->>DB: update accounts & tx history (only if is_new_transaction == true)
  WM-->>SPV: BlockProcessingResult { new_txids, existing_txids, ... }
  SPV->>SPV: log using relevant_tx_count() and iterate relevant_txids()

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I bounded through blocks, two piles to keep,
New hops that sparkle, old footprints deep.
Rescans whisper, "Seen this before,"
New ones hop in while old ones stay more.
My ledger carrots — tidy, counted, neat. 🥕

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: distinguishing between new and existing transactions in wallet processing, which is the core objective of the PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@key-wallet-manager/src/wallet_manager/mod.rs`:
- Around line 493-498: The aggregation currently uses AND semantics: when
iterating a wallet's CheckResult (check_result) you push wallet_id into
result.affected_wallets and set result.is_new_transaction = false if any
check_result.is_new_transaction is false; change this so
newly-imported/watch-only wallets don't force the global transaction to be
treated as existing. Detect a wallet's import state (e.g., via wallet metadata
like Wallet::is_fresh_import or a flag on check_result such as
check_result.is_from_fresh_import), and for those wallets use OR semantics
(result.is_new_transaction ||= check_result.is_new_transaction) or explicitly
treat their checks as always "new" for that wallet; for non-imported wallets
keep the existing AND behavior. Update the logic around result.affected_wallets,
check_result, wallet_id, and result.is_new_transaction accordingly.

In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 79-139: The code currently checks existing transactions and sets
result.is_new_transaction=false but still increments
self.metadata.total_transactions and any "new tx" metrics unconditionally;
update the logic so that the increment of self.metadata.total_transactions (and
any other metrics or counters for new transactions) only occurs when
result.is_new_transaction is true. Locate the block that does
self.metadata.total_transactions += 1 (and any related metric updates) and wrap
those operations behind an if result.is_new_transaction { ... } guard (keeping
the existing behavior when true and skipping increments when false).

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@key-wallet/src/managed_account/managed_account_collection.rs`:
- Around line 680-733: get_by_account_type_match currently returns None for
AccountTypeMatch::PlatformPayment (and Dashpay variants); implement the
PlatformPayment branch to construct a PlatformPaymentAccountKey from the
AccountTypeMatch's account_index and key_class and use it to lookup the account
in the platform_payment_accounts collection (mirror the logic from
AccountCollection::get_by_account_type_match and reference
PlatformPaymentAccountKey and platform_payment_accounts). For Dashpay variants,
either extend AccountTypeMatch to carry user_identity_id/friend_identity_id so
you can resolve them directly or implement a fallback search over dashpay
collections to find a matching account; ensure get_by_account_type_match,
AccountTypeMatch::PlatformPayment,
AccountTypeMatch::DashpayReceivingFunds/DashpayExternalAccount,
PlatformPaymentAccountKey, and platform_payment_accounts are the focal symbols
when making the change so transactions picked up by wallet_checker will not be
missed.

🧹 Nitpick comments (2)

key-wallet/src/transaction_checking/account_checker.rs (1)

286-294: Avoid defaulting is_new_transaction to true when no match is found.

is_relevant can remain false while is_new_transaction stays true, which is an inconsistent state if any caller forgets to check relevance. Consider defaulting to false here and letting the wallet layer set it once history is checked.

♻️ Suggested tweak

         let mut result = TransactionCheckResult {
             is_relevant: false,
-            is_new_transaction: true,
+            is_new_transaction: false,
             affected_accounts: Vec::new(),
             total_received: 0,
             total_sent: 0,
             total_received_for_credit_conversion: 0,
             new_addresses: Vec::new(),
         };

key-wallet-manager/src/wallet_interface.rs (1)

23-33: Document or preserve ordering in relevant_txids().

The iterator concatenates new then existing, which may differ from the original per-block order previously implied by relevant_txids. If ordering matters, consider storing a combined sequence during processing or documenting the new ordering semantics.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@key-wallet/src/managed_account/managed_account_collection.rs`:
- Around line 723-750: The lookup for Dashpay accounts currently only compares
the account_index (in AccountTypeMatch::DashpayReceivingFunds and
AccountTypeMatch::DashpayExternalAccount) which can return the wrong account
because DashpayAccountKey is (index, user_identity_id, friend_identity_id);
update the search over dashpay_receival_accounts and dashpay_external_accounts
to use the provided involved_addresses from the AccountTypeMatch payload as the
primary selector: in the .find closure for each
ManagedAccountType::DashpayReceivingFunds /
ManagedAccountType::DashpayExternalAccount check that the index matches AND that
the account.account_type.addresses() intersects (or contains) the
involved_addresses from the AccountTypeMatch; use that combined index+addresses
match instead of index-only to reliably identify the correct Dashpay account.

♻️ Duplicate comments (1)

key-wallet/src/managed_account/managed_account_collection.rs (1)

751-754: PlatformPayment lookup returns None — previously flagged.

The past review raised this concern and suggested implementing the lookup using PlatformPaymentAccountKey. The added comment explains DIP-17 rationale, but if this is intentional, consider adding a more detailed doc comment explaining why lookups are unsupported here while the accounts are stored in the collection.