fix: duplicate tx when importing completed tx#7064
fix: duplicate tx when importing completed tx#7064SWvheerden merged 5 commits intotari-project:developmentfrom
Conversation
WalkthroughThe changes introduce the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant OutputManagerService
participant TransactionServiceHandle
participant StandardUtxoRecoverer
App->>OutputManagerService: initialize (with TransactionServiceHandle)
OutputManagerService->>TransactionServiceHandle: store handle for later use
App->>OutputManagerService: ScanForRecoverableOutputs
OutputManagerService->>StandardUtxoRecoverer: new(..., TransactionServiceHandle)
StandardUtxoRecoverer->>TransactionServiceHandle: get_completed_transactions_by_addresses (filter by block/height)
TransactionServiceHandle-->>StandardUtxoRecoverer: matching transactions or none
StandardUtxoRecoverer->>OutputManagerService: return recovered outputs
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
base_layer/wallet/src/output_manager_service/mod.rs(3 hunks)base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs(2 hunks)base_layer/wallet/src/output_manager_service/resources.rs(2 hunks)base_layer/wallet/src/output_manager_service/service.rs(4 hunks)base_layer/wallet/tests/output_manager_service_tests/service.rs(2 hunks)base_layer/wallet/tests/transaction_service_tests/service.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: ci
🔇 Additional comments (15)
base_layer/wallet/tests/output_manager_service_tests/service.rs (2)
185-185: Align test setup with updatedOutputManagerServiceconstructor
Addedts_handle.clone()to theOutputManagerService::newcall, ensuring the service is initialized with the requiredTransactionServiceHandle.
255-255: Align secondary test setup with updated constructor
Similarly addedts_handle.clone()insetup_oms_with_bn_state, keeping both test helpers consistent with the new service signature.base_layer/wallet/tests/transaction_service_tests/service.rs (1)
410-424: Test setup updated to match implementation changesThe change correctly passes a cloned
transaction_service_handleto theOutputManagerService::newconstructor, aligning with the implementation changes that introduced the handle as a new dependency for the output manager. This enables the output manager to access transaction service data, which is essential for the fix that prevents duplicate transactions when importing completed transactions.base_layer/wallet/src/output_manager_service/resources.rs (2)
33-33: LGTM: Added TransactionServiceHandle import.The import of
TransactionServiceHandleis necessary for the fix to prevent duplicate transactions when importing completed transactions.
51-51: LGTM: Added TransactionServiceHandle as a resource dependency.Adding the transaction service handle to
OutputManagerResourcesenables the output manager to check if transactions already exist before creating duplicates during UTXO scanning. This addition follows the existing pattern of dependency injection in the codebase and directly addresses the PR objective of preventing duplicate transactions when importing completed transactions.base_layer/wallet/src/output_manager_service/mod.rs (3)
65-65: LGTM: Added TransactionServiceHandle import.The import is necessary for retrieving and using the transaction service handle in the initialization process.
130-130: LGTM: Added retrieval of TransactionServiceHandle.Retrieving the TransactionServiceHandle from the available service handles is necessary to pass it to the OutputManagerService constructor.
145-145: LGTM: Added TransactionServiceHandle to the constructor.Passing the transaction service handle to the OutputManagerService constructor completes the dependency injection pattern and provides the service with access to transaction data. This enables checking for existing transactions before creating duplicates during UTXO scanning, directly addressing the PR objective of preventing duplicate transactions when importing completed transactions.
base_layer/wallet/src/output_manager_service/service.rs (4)
118-118: Added new import for TransactionServiceHandle.This import is required for the new functionality that checks for existing transactions during output recovery.
160-160: Added TransactionServiceHandle parameter to service constructor.This parameter allows the OutputManagerService to access transaction data during output recovery operations.
191-191: Added transaction_service_handle to the OutputManagerResources struct.The handle is properly stored in the resources to be accessible by the service operations.
492-499: Added transaction_service_handle to StandardUtxoRecoverer.This change properly passes the transaction service handle to the StandardUtxoRecoverer, enabling it to check for existing transactions during output recovery, which addresses the duplicate transaction issue mentioned in the PR description.
base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs (3)
45-56: Appropriate import update for new dependency.The imports have been properly updated to include the
TransactionServiceHandlewhich is needed for the new functionality to prevent duplicate transactions.
63-63: Good addition of TransactionServiceHandle field.Adding this field to the
StandardUtxoRecovererstruct is necessary to access transaction history during output recovery, which aligns with the PR objective of preventing duplicate transactions.
71-80: Constructor properly updated to include the new dependency.The constructor has been correctly modified to accept and store the
TransactionServiceHandleparameter, maintaining proper initialization of the struct.
| None => { | ||
| let last_block_txs = self | ||
| .transaction_service_handle | ||
| .get_completed_transactions(None, db_output.mined_in_block, db_output.mined_height) | ||
| .await | ||
| .unwrap_or_default(); | ||
| let tx_id = last_block_txs.iter().find_map(|tx| { | ||
| tx.transaction | ||
| .body | ||
| .outputs() | ||
| .iter() | ||
| .find(|tx| tx.commitment == db_output.commitment) | ||
| .map(|_| tx.tx_id) | ||
| }); | ||
| tx_id.unwrap_or_else(TxId::new_random) | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Well-implemented fix for duplicate transaction issue.
This change directly addresses the PR objective by checking for existing transactions with matching outputs before generating a new random TxId. When importing completed transactions, it will now reuse the original transaction ID instead of creating duplicates.
The implementation appropriately:
- Queries completed transactions from the block where the output was mined
- Searches for transactions containing the same output commitment
- Falls back to the original random ID generation only when necessary
Let's verify this implementation with a simple test case:
🏁 Script executed:
#!/bin/bash
# Find test code that validates this behavior
echo "Searching for test cases that verify the duplicate transaction fix:"
rg -A 5 "StandardUtxoRecoverer.*new.*transaction_service_handle" --type rust
echo "Searching for test cases involving transaction import/export:"
rg -A 5 "import.*transaction|export.*transaction" --type rustLength of output: 47879
Add missing tests for TxId reuse in StandardOutputsRecoverer
There are currently no tests covering the new “reuse existing TxId” logic in standard_outputs_recoverer.rs. Please add unit or integration tests to verify both outcomes:
- When a matching output commitment is found in
get_completed_transactions, the originaltx_idis returned. - When no matching commitment exists, a new random
TxIdis generated.
Suggested locations for new tests:
base_layer/wallet/tests/(unit tests for the recoverer)integration_tests/tests/(end-to-end test that imports outputs and checks the returnedTxId)
Target file and lines needing coverage:
base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs:160–175
🤖 Prompt for AI Agents
In
base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs
around lines 160 to 175, add unit or integration tests to cover the new logic
that reuses existing TxId when a matching output commitment is found in
get_completed_transactions, and generates a new random TxId when no match
exists. Create tests that mock or simulate get_completed_transactions returning
transactions with and without matching commitments, then assert that the
returned TxId matches the expected behavior. Place these tests in
base_layer/wallet/tests/ for unit tests or integration_tests/tests/ for
end-to-end coverage.
Test Results (CI) 2 files 60 suites 22m 26s ⏱️ For more details on these failures, see this check. Results for commit dcce63a. ♻️ This comment has been updated with latest results. |
mmrrnn
force-pushed
the
mp_import_completed_tx_duplicate
branch
from
f2f8cee to
d968129
Compare
Test Results (Integration tests) 2 files + 2 11 suites +11 1h 29m 54s ⏱️ + 1h 29m 54s For more details on these failures, see this check. Results for commit 54e3785. ± Comparison against base commit f593638. ♻️ This comment has been updated with latest results. |
2 participants
Description
When importing a completed transaction (not yet broadcasted), the wallet creates a duplicate of this transaction while scanning UTXOs.
Motivation and Context
Check if the corresponding transaction hasn’t been imported. Use existing tx's
tx_idinstead of creating a duplicate record.How Has This Been Tested?
Manually
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Tests