feat: improve scanning performance#7371
Conversation
WalkthroughThis update introduces a new asynchronous method, Changes
Sequence Diagram(s)sequenceDiagram
participant Wallet
participant KeyManagerWrapper
participant KeyManagerInner
Wallet->>KeyManagerWrapper: is_this_output_ours(commitment, encrypted_data, custom_recovery_key_id)
KeyManagerWrapper->>KeyManagerInner: is_this_output_ours(commitment, encrypted_data, custom_recovery_key_id)
KeyManagerInner->>KeyManagerInner: Attempt decryption with keys
KeyManagerInner->>KeyManagerInner: Verify range proof mask
KeyManagerInner-->>KeyManagerWrapper: Result<bool, TransactionError>
KeyManagerWrapper-->>Wallet: Result<bool, TransactionError>
sequenceDiagram
participant UTXOScanner
participant KeyManager
participant OutputManager
UTXOScanner->>OutputManager: scan_outputs_for_one_sided_payments(outputs)
OutputManager-->>UTXOScanner: one_sided_results
UTXOScanner->>UTXOScanner: Filter out one-sided outputs
UTXOScanner->>OutputManager: scan_for_recoverable_outputs(remaining_outputs)
OutputManager-->>UTXOScanner: recoverable_results
UTXOScanner->>UTXOScanner: Combine results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
base_layer/core/src/transactions/transaction_key_manager/inner.rs(3 hunks)base_layer/core/src/transactions/transaction_key_manager/interface.rs(1 hunks)base_layer/core/src/transactions/transaction_key_manager/wrapper.rs(1 hunks)base_layer/wallet/src/base_node_service/monitor.rs(1 hunks)base_layer/wallet/src/output_manager_service/service.rs(1 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(3 hunks)base_layer/wallet_ffi/wallet.h(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy.
base_layer/wallet/src/base_node_service/monitor.rs (1)
Learnt from: hansieodendaal
PR: #7216
File: applications/minotari_app_grpc/src/conversions/aggregate_body.rs:31-46
Timestamp: 2025-06-14T04:46:31.966Z
Learning: When a type implements the Copy trait (like FixedHash/BlockHash), it can be used multiple times in iterator closures without being consumed or moved. The compiler automatically copies Copy types, so no explicit .clone() is needed and will trigger clippy::clone_on_copy warnings.
base_layer/wallet_ffi/wallet.h (2)
Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy. The wallet.proto file confirms that timelocked_balance is defined as the fourth field in the GetBalanceResponse message.
Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy.
base_layer/wallet/src/output_manager_service/service.rs (3)
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: hansieodendaal
PR: #6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in output_to_self method in the Tari wallet, use an accurate fee estimate instead of fee_per_gram. The input_selection.as_final_fee() method provides a good initial estimate, and the final fee can be obtained later from stp.get_fee_amount().
Learnt from: SWvheerden
PR: #7300
File: base_layer/wallet/src/transaction_service/service.rs:1663-1663
Timestamp: 2025-07-10T13:21:46.666Z
Learning: In the Tari wallet codebase, scanned height persistence is handled by the UTXO scanner service, not individual services like TransactionService. Services that need current scanned height should read it via get_last_scanned_height() rather than persisting it themselves. This follows separation of concerns architecture pattern.
base_layer/core/src/transactions/transaction_key_manager/wrapper.rs (2)
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (2)
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: SWvheerden
PR: #7300
File: base_layer/wallet/src/transaction_service/service.rs:1663-1663
Timestamp: 2025-07-10T13:21:46.666Z
Learning: In the Tari wallet codebase, scanned height persistence is handled by the UTXO scanner service, not individual services like TransactionService. Services that need current scanned height should read it via get_last_scanned_height() rather than persisting it themselves. This follows separation of concerns architecture pattern.
base_layer/core/src/transactions/transaction_key_manager/inner.rs (2)
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: hansieodendaal
PR: #7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
base_layer/core/src/transactions/transaction_key_manager/interface.rs (1)
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: ci
- GitHub Check: ledger build tests
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / FFI
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (9)
base_layer/wallet/src/base_node_service/monitor.rs (1)
138-144: Dropped the superfluousclone()– nice micro-optimisationReplacing
last_checked_hash = tip_hash.clone();withlast_checked_hash = *tip_hash;cleanly exploitsFixedHash: Copy, avoids theclippy::clone_on_copylint, and saves an unnecessary heap allocation.
Looks good.base_layer/wallet/src/output_manager_service/service.rs (2)
384-387: LGTM - Efficient refactor removes redundant key derivation.The direct creation of
TariKeyId::Derivedusing the already availablecommitment_mask_key_idis more efficient than re-deriving it from the private key. This change maintains the same functionality while improving performance.
392-392: LGTM - Consistent with the refactor approach.Using
commitment_mask_key_id.clone()directly is consistent with the optimization in lines 384-387 and avoids redundant key derivation operations.base_layer/core/src/transactions/transaction_key_manager/interface.rs (1)
394-399: Well-designed method addition for performance optimization.The new
is_this_output_oursmethod provides an efficient way to check output ownership without performing full key recovery. The method signature is consistent with existing patterns and appropriately positioned near related recovery methods in the trait.base_layer/core/src/transactions/transaction_key_manager/wrapper.rs (1)
423-434: Correct implementation following established wrapper pattern.The implementation properly delegates to the inner key manager using a read lock, which is consistent with other read-only operations in the wrapper. The method signature matches the trait definition exactly.
base_layer/core/src/transactions/transaction_key_manager/inner.rs (2)
682-683: Good optimization for sequential key discoveryStarting the search at
current_index + 1is a smart optimization that reduces unnecessary checks when keys are used sequentially, which is the common case.
95-95: Confirm safe key recovery with reduced search depthThe constant at
base_layer/core/src/transactions/transaction_key_manager/inner.rs:95was changed from:- const TRANSACTION_KEY_MANAGER_MAX_SEARCH_DEPTH: u64 = 1_000_000; + const TRANSACTION_KEY_MANAGER_MAX_SEARCH_DEPTH: u64 = 2_000;While this improves performance, no documentation or tests reference gaps larger than the new limit, and existing tests only cover indices up to 7. This may prevent recovery of keys if the derivation index gap ever exceeds 2,000.
• Verify that in all branches the maximum key index gap will remain below 2,000.
• Consider makingTRANSACTION_KEY_MANAGER_MAX_SEARCH_DEPTHconfigurable (e.g., via a const or runtime parameter).
• Add tests forfind_key_indexandfind_private_key_indexwith index gaps near the 2,000 boundary.base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (2)
568-595: Efficient two-phase output ownership checkThe refactored logic correctly prioritizes received outputs (using DH shared secret) before checking for change outputs, avoiding redundant decryption attempts.
614-671: Well-structured two-phase scanning improves performanceThe refactored scanning logic efficiently separates one-sided payments from recoverable outputs, avoiding redundant processing by filtering out already-found outputs between phases.
| pub async fn is_this_output_ours( | ||
| &self, | ||
| commitment: &CompressedCommitment, | ||
| encrypted_data: &EncryptedData, | ||
| custom_recovery_key_id: Option<&TariKeyId>, | ||
| ) -> Result<bool, TransactionError> { | ||
| let recovery_key = if let Some(key_id) = custom_recovery_key_id { | ||
| self.get_private_key(key_id).await? | ||
| } else { | ||
| self.get_private_view_key().await? | ||
| }; | ||
| let (value, private_key, _payment_id) = EncryptedData::decrypt_data(&recovery_key, commitment, encrypted_data)?; | ||
| self.crypto_factories | ||
| .range_proof | ||
| .verify_mask(&commitment.to_commitment()?, &private_key, value.into())?; | ||
| Ok(true) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to eliminate code duplication
The is_this_output_ours method duplicates most of the logic from try_output_key_recovery. Consider refactoring to use a shared internal method.
+ async fn decrypt_and_verify_output(
+ &self,
+ commitment: &CompressedCommitment,
+ encrypted_data: &EncryptedData,
+ custom_recovery_key_id: Option<&TariKeyId>,
+ ) -> Result<(MicroMinotari, PrivateKey, MemoField), TransactionError> {
+ let recovery_key = if let Some(key_id) = custom_recovery_key_id {
+ self.get_private_key(key_id).await?
+ } else {
+ self.get_private_view_key().await?
+ };
+ let (value, private_key, payment_id) = EncryptedData::decrypt_data(&recovery_key, commitment, encrypted_data)?;
+ self.crypto_factories
+ .range_proof
+ .verify_mask(&commitment.to_commitment()?, &private_key, value.into())?;
+ Ok((value, private_key, payment_id))
+ }
+
pub async fn try_output_key_recovery(
&self,
commitment: &CompressedCommitment,
encrypted_data: &EncryptedData,
custom_recovery_key_id: Option<&TariKeyId>,
) -> Result<(TariKeyId, MicroMinotari, MemoField), TransactionError> {
- let recovery_key = if let Some(key_id) = custom_recovery_key_id {
- self.get_private_key(key_id).await?
- } else {
- self.get_private_view_key().await?
- };
- let (value, private_key, payment_id) = EncryptedData::decrypt_data(&recovery_key, commitment, encrypted_data)?;
- self.crypto_factories
- .range_proof
- .verify_mask(&commitment.to_commitment()?, &private_key, value.into())?;
+ let (value, private_key, payment_id) = self.decrypt_and_verify_output(commitment, encrypted_data, custom_recovery_key_id).await?;
let branch = TransactionKeyManagerBranch::CommitmentMask.get_branch_key();
// ... rest of the method
}
pub async fn is_this_output_ours(
&self,
commitment: &CompressedCommitment,
encrypted_data: &EncryptedData,
custom_recovery_key_id: Option<&TariKeyId>,
) -> Result<bool, TransactionError> {
- let recovery_key = if let Some(key_id) = custom_recovery_key_id {
- self.get_private_key(key_id).await?
- } else {
- self.get_private_view_key().await?
- };
- let (value, private_key, _payment_id) = EncryptedData::decrypt_data(&recovery_key, commitment, encrypted_data)?;
- self.crypto_factories
- .range_proof
- .verify_mask(&commitment.to_commitment()?, &private_key, value.into())?;
+ self.decrypt_and_verify_output(commitment, encrypted_data, custom_recovery_key_id).await?;
Ok(true)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub async fn is_this_output_ours( | |
| &self, | |
| commitment: &CompressedCommitment, | |
| encrypted_data: &EncryptedData, | |
| custom_recovery_key_id: Option<&TariKeyId>, | |
| ) -> Result<bool, TransactionError> { | |
| let recovery_key = if let Some(key_id) = custom_recovery_key_id { | |
| self.get_private_key(key_id).await? | |
| } else { | |
| self.get_private_view_key().await? | |
| }; | |
| let (value, private_key, _payment_id) = EncryptedData::decrypt_data(&recovery_key, commitment, encrypted_data)?; | |
| self.crypto_factories | |
| .range_proof | |
| .verify_mask(&commitment.to_commitment()?, &private_key, value.into())?; | |
| Ok(true) | |
| } | |
| // Shared helper to decrypt and verify an output | |
| async fn decrypt_and_verify_output( | |
| &self, | |
| commitment: &CompressedCommitment, | |
| encrypted_data: &EncryptedData, | |
| custom_recovery_key_id: Option<&TariKeyId>, | |
| ) -> Result<(MicroMinotari, PrivateKey, MemoField), TransactionError> { | |
| let recovery_key = if let Some(key_id) = custom_recovery_key_id { | |
| self.get_private_key(key_id).await? | |
| } else { | |
| self.get_private_view_key().await? | |
| }; | |
| let (value, private_key, payment_id) = | |
| EncryptedData::decrypt_data(&recovery_key, commitment, encrypted_data)?; | |
| self.crypto_factories | |
| .range_proof | |
| .verify_mask(&commitment.to_commitment()?, &private_key, value.into())?; | |
| Ok((value, private_key, payment_id)) | |
| } | |
| pub async fn try_output_key_recovery( | |
| &self, | |
| commitment: &CompressedCommitment, | |
| encrypted_data: &EncryptedData, | |
| custom_recovery_key_id: Option<&TariKeyId>, | |
| ) -> Result<(TariKeyId, MicroMinotari, MemoField), TransactionError> { | |
| let (value, private_key, payment_id) = | |
| self.decrypt_and_verify_output(commitment, encrypted_data, custom_recovery_key_id).await?; | |
| let branch = TransactionKeyManagerBranch::CommitmentMask.get_branch_key(); | |
| // ... rest of the method | |
| } | |
| pub async fn is_this_output_ours( | |
| &self, | |
| commitment: &CompressedCommitment, | |
| encrypted_data: &EncryptedData, | |
| custom_recovery_key_id: Option<&TariKeyId>, | |
| ) -> Result<bool, TransactionError> { | |
| self.decrypt_and_verify_output(commitment, encrypted_data, custom_recovery_key_id).await?; | |
| Ok(true) | |
| } |
🤖 Prompt for AI Agents
In base_layer/core/src/transactions/transaction_key_manager/inner.rs around
lines 1691 to 1708, the is_this_output_ours method duplicates logic found in
try_output_key_recovery. Refactor by extracting the common logic into a private
helper function that both methods can call, passing necessary parameters and
returning shared results to avoid code duplication and improve maintainability.
There was a problem hiding this comment.
@stringhandler Perfect! I'll implement the refactoring to eliminate the code duplication between is_this_output_ours and try_output_key_recovery.
Task: Refactor to eliminate code duplication in transaction key manager
- Extract common decryption and verification logic into a shared helper method
- Update both
try_output_key_recoveryandis_this_output_oursto use the helper - Maintain the same public API while improving code maintainability
Another agent will implement these changes for you. The refactoring will create a clean, maintainable solution that eliminates the duplication while preserving the exact same functionality.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
⏳ Preparing changes...
There was a problem hiding this comment.
A summary of the changes CodeRabbit can apply:
Refactor the file by adding a new helper method to decrypt and verify outputs, then replace duplicated decryption and verification code in the
try_output_key_recoveryandis_this_output_oursmethods with calls to this helper for improved code reuse and reliability.Refactor the file base_layer/core/src/transactions/transaction_key_manager/inner.rs by extracting duplicated decryption and verification code from two methods into a new shared async helper method, then replace the original duplicated code blocks with calls to this helper to improve code clarity and maintainability.
Insert a shared helper method for decrypting and verifying outputs before the existing function to eliminate duplicated code, then replace the duplicated code blocks in two methods with calls to this new helper, adjusting line numbers accordingly to maintain correct code structure.
- ✅ Create PR with these edits
- 📌 Create commit in current branch
- 📋 Get copyable edits
| pub async fn is_this_output_ours( | ||
| &self, | ||
| commitment: &CompressedCommitment, | ||
| encrypted_data: &EncryptedData, | ||
| custom_recovery_key_id: Option<&TariKeyId>, | ||
| ) -> Result<bool, TransactionError> { | ||
| let recovery_key = if let Some(key_id) = custom_recovery_key_id { | ||
| self.get_private_key(key_id).await? | ||
| } else { | ||
| self.get_private_view_key().await? | ||
| }; | ||
| let (value, private_key, _payment_id) = EncryptedData::decrypt_data(&recovery_key, commitment, encrypted_data)?; | ||
| self.crypto_factories | ||
| .range_proof | ||
| .verify_mask(&commitment.to_commitment()?, &private_key, value.into())?; | ||
| Ok(true) | ||
| } | ||
|
|
hansieodendaal
left a comment
hansieodendaal
left a comment
There was a problem hiding this comment.
Nice improvement.
General question: Will wallets with a bigger ratio of change outputs not be worse off during recovery?
Some more comments.
3 participants
Description
greatly improves the scanning recovery performance
Summary by CodeRabbit
New Features
Improvements
Documentation