feat!: change wallet to use http calls for utxo syncing #7151
Conversation
…ode-wallet-service-as-http
…ode-wallet-service-as-http
…-as-http' into feature/base-node-wallet-service-as-http
…-as-http' into feature/base-node-wallet-service-as-http
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (1)
197-578: 🛠️ Refactor suggestionRefactor the overly complex
attempt_syncmethod.The method is marked with
#[allow(clippy::too_many_lines)]indicating it's too complex. Consider breaking it down into smaller, more focused methods for better maintainability.Consider extracting the following into separate methods:
- Header fetching logic
- Block scanning logic
- Progress reporting logic
- Error handling and retry logic
♻️ Duplicate comments (2)
base_layer/core/src/base_node/rpc/models/get_utxos_by_block.rs (1)
29-132: Consider deriving the schema automatically to improve maintainability.The manual schema definition is error-prone and will require updates whenever
TransactionOutputchanges. This could lead to documentation drift.Consider implementing
ToSchemaderive macro onTransactionOutputdirectly, or using a schema generation utility that can introspect the struct. This would ensure the OpenAPI documentation stays in sync with the actual implementation.If manual schema is necessary due to serialization customizations, consider adding tests to verify the schema matches the actual struct serialization.
clients/rust/base_node_wallet_client/src/client/http.rs (1)
78-136:⚠️ Potential issueAddress previously flagged error handling and configurability issues.
The implementation still has the error handling issues mentioned in previous reviews:
- The page size is hardcoded (now to 10 instead of 5)
- Error handling continues the loop indefinitely on failure
- No proper cancellation mechanism beyond the shutdown signal
Consider implementing the improvements suggested in the previous review to:
- Make the page size configurable
- Break the loop on errors instead of continuing indefinitely
- Add proper error handling with retry limits
🧹 Nitpick comments (6)
base_layer/core/src/base_node/rpc/models/sync_utxos_by_block.rs (1)
38-38: Fix missing space in struct name.Apply this diff to fix the formatting:
-pub struct MinimalUtxoInfo{ +pub struct MinimalUtxoInfo {base_layer/wallet/src/utxo_scanner_service/uxto_scanner_service_builder.rs (2)
23-23: Remove unused import.The
std::anyimport is not used anywhere in this file.-use std::any; -
162-162: Clean up extra whitespace and formatting.There are unnecessary blank lines and inconsistent formatting.
- let http_client_url = match &self.node_url { + let http_client_url = match &self.node_url { Some(url) => url.clone(), None => return Err(anyhow::anyhow!("Node URL must be set before building the UTXO scanner service.")), }; let resources = UtxoScannerResources { // ... other fields ... http_client_url }; - - Ok( UtxoScannerService::new( + Ok(UtxoScannerService::new(Also applies to: 180-182
base_layer/core/src/base_node/rpc/query_service.rs (2)
148-148: Remove unnecessary blank lines.Multiple extra blank lines that should be removed for consistency.
Also applies to: 160-160, 169-170
235-235: Document or make configurable the chunk size for outputs.The code chunks outputs into groups of 2000 without explanation. This magic number should be documented or made configurable.
+ // Chunk outputs to avoid excessive memory usage and message size + const OUTPUT_CHUNK_SIZE: usize = 2000; - for output_chunk in outputs.chunks(2000) { + for output_chunk in outputs.chunks(OUTPUT_CHUNK_SIZE) {base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (1)
474-476: Fix awkward line breaking in while loop condition.The while loop condition has poor formatting that reduces readability.
- while let Some(response) = - utxo_stream.recv().await - { + while let Some(response) = utxo_stream.recv().await {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
applications/minotari_console_wallet/log4rs_sample.yml(6 hunks)applications/minotari_console_wallet/src/recovery.rs(3 hunks)applications/minotari_node/src/http/handler/get_utxos_by_block.rs(1 hunks)applications/minotari_node/src/http/handler/mod.rs(1 hunks)applications/minotari_node/src/http/server.rs(3 hunks)base_layer/core/src/base_node/rpc/mod.rs(2 hunks)base_layer/core/src/base_node/rpc/models/get_utxos_by_block.rs(1 hunks)base_layer/core/src/base_node/rpc/models/mod.rs(1 hunks)base_layer/core/src/base_node/rpc/models/sync_utxos_by_block.rs(1 hunks)base_layer/core/src/base_node/rpc/query_service.rs(4 hunks)base_layer/core/src/transactions/transaction_components/encrypted_data.rs(2 hunks)base_layer/core/src/transactions/transaction_key_manager/inner.rs(2 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/lib.rs(1 hunks)base_layer/wallet/src/output_manager_service/recovery/mod.rs(1 hunks)base_layer/wallet/src/output_manager_service/recovery/output_recovery_info.rs(1 hunks)base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs(1 hunks)base_layer/wallet/src/utxo_scanner_service/initializer.rs(6 hunks)base_layer/wallet/src/utxo_scanner_service/service.rs(6 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(16 hunks)base_layer/wallet/src/utxo_scanner_service/uxto_scanner_service_builder.rs(9 hunks)base_layer/wallet_ffi/src/lib.rs(5 hunks)base_layer/wallet_ffi/wallet.h(1 hunks)clients/rust/base_node_wallet_client/Cargo.toml(1 hunks)clients/rust/base_node_wallet_client/src/client/http.rs(2 hunks)clients/rust/base_node_wallet_client/src/client/mod.rs(2 hunks)
✅ Files skipped from review due to trivial changes (6)
- base_layer/wallet/src/output_manager_service/recovery/mod.rs
- base_layer/wallet/src/output_manager_service/recovery/output_recovery_info.rs
- base_layer/wallet/src/lib.rs
- base_layer/core/src/transactions/transaction_key_manager/inner.rs
- applications/minotari_console_wallet/log4rs_sample.yml
- base_layer/wallet_ffi/wallet.h
🚧 Files skipped from review as they are similar to previous changes (9)
- clients/rust/base_node_wallet_client/Cargo.toml
- base_layer/core/src/base_node/rpc/models/mod.rs
- applications/minotari_console_wallet/src/recovery.rs
- base_layer/wallet/src/utxo_scanner_service/initializer.rs
- applications/minotari_node/src/http/server.rs
- base_layer/wallet/src/utxo_scanner_service/service.rs
- applications/minotari_node/src/http/handler/mod.rs
- base_layer/wallet_ffi/src/lib.rs
- base_layer/core/src/base_node/rpc/mod.rs
🧰 Additional context used
🧠 Learnings (1)
base_layer/core/src/base_node/rpc/query_service.rs (1)
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (9)
base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs (1)
297-297: LGTM! Correctly updated method call to match new interface.The change aligns with the updated
try_output_key_recoverymethod signature that now accepts separatecommitmentandencrypted_dataparameters instead of the fullTransactionOutput. This improves modularity by only passing the specific data needed for key recovery.base_layer/core/src/transactions/transaction_components/encrypted_data.rs (1)
619-624: Code organization improvement.The
EncryptedDatastruct was repositioned within the file with no functional changes. This improves code organization by moving the struct definition closer to its implementation.base_layer/core/src/transactions/transaction_key_manager/interface.rs (1)
390-391: Good API improvement that enhances modularity.The method signature change from accepting a full
TransactionOutputto separatecommitmentandencrypted_dataparameters improves modularity by requiring only the specific data needed for key recovery. This aligns with the broader refactoring to support HTTP-based UTXO synchronization with minimal data structures.Note: This is a breaking API change, but appears to be consistently updated across implementations.
clients/rust/base_node_wallet_client/src/client/mod.rs (2)
5-10: New imports support HTTP-based UTXO syncing functionality.The imports are well-organized and necessary for the new UTXO syncing methods:
- RPC models for request/response types
ShutdownSignalfor graceful terminationmpscfor streaming responses
23-33:✅ Verification successful
Well-designed API for HTTP-based UTXO synchronization.
The new trait methods provide a clean interface for UTXO syncing:
get_utxos_by_blockfor single block queriessync_utxos_by_blockfor streaming across block ranges with shutdown supportThe API design is sound with appropriate error handling and streaming capabilities for large datasets.
Verify that implementations of these trait methods exist and handle errors appropriately:
🏁 Script executed:
#!/bin/bash # Description: Check for implementations of the new trait methods # Expected: Find implementations in http.rs or other implementation files # Search for implementations of the new methods rg -A 10 "async fn get_utxos_by_block" rg -A 10 "async fn sync_utxos_by_block"Length of output: 9953
Implementations Verified and Errors Properly Propagated
- The methods are implemented in
clients/rust/base_node_wallet_client/src/client/http.rs:
get_utxos_by_blockconstructs the URL, sends the HTTP request, and deserializes the response, using?to propagate any URL‐parsing, network, or JSON errors asClientError.sync_utxos_by_blockspawns a background task that iterates over the block range, streams responses into anmpsc::Receiver, and forwards any errors back through theClientErrorchannel.All requested trait implementations exist and properly bubble up errors. Approving these changes.
applications/minotari_node/src/http/handler/get_utxos_by_block.rs (1)
49-62: LGTM! Well-structured handler implementation.The handler implementation follows best practices with proper error handling, logging, and async/await usage. The parameter conversion and service call are correctly implemented.
base_layer/core/src/transactions/transaction_key_manager/wrapper.rs (1)
411-423: LGTM! Improved modularity through parameter separation.The refactor to accept separate
CompressedCommitmentandEncryptedDataparameters instead of a fullTransactionOutputimproves modularity and aligns well with the new minimal UTXO representation introduced in the RPC models. This change supports more efficient output key recovery.base_layer/core/src/base_node/rpc/models/sync_utxos_by_block.rs (1)
16-18: LGTM! Limit increase addresses previous feedback.The maximum limit has been appropriately increased from 5 to 2000, which addresses the previous review comment about enabling more efficient synchronization by reducing the number of HTTP requests required.
base_layer/core/src/base_node/rpc/models/get_utxos_by_block.rs (1)
14-17: LGTM! Simple and clean request model.The request structure is appropriately minimal and well-defined for single block UTXO queries.
| #[utoipa::path( | ||
| get, | ||
| operation_id = "sync_utxos_by_block", | ||
| params(GetUtxosByBlockQueryParams), | ||
| path = "/sync_utxos_by_block", | ||
| responses( | ||
| (status = 200, description = "UTXOs returned successfully in the given headers' hash range", body = SyncUtxosByBlockResponse), | ||
| (status = NOT_FOUND, description = "Header not found", body = ErrorResponse, example = json!({"error": "Header not found at height: 10"})), | ||
| (status = INTERNAL_SERVER_ERROR, description = "Start/end header hash not found or header height mismatch", body = ErrorResponse), | ||
| ), | ||
| )] |
There was a problem hiding this comment.
Fix inconsistent OpenAPI operation metadata.
The operation metadata contains inconsistencies that could confuse API consumers:
operation_id = "sync_utxos_by_block"doesn't match the actual functionality of getting UTXOs for a single block- The response description mentions "given headers' hash range" but this endpoint handles a single block hash
Apply this diff to fix the inconsistencies:
#[utoipa::path(
get,
- operation_id = "sync_utxos_by_block",
+ operation_id = "get_utxos_by_block",
params(GetUtxosByBlockQueryParams),
- path = "/sync_utxos_by_block",
+ path = "/get_utxos_by_block",
responses(
- (status = 200, description = "UTXOs returned successfully in the given headers' hash range", body = SyncUtxosByBlockResponse),
+ (status = 200, description = "UTXOs returned successfully for the given block", body = GetUtxosByBlockResponse),
(status = NOT_FOUND, description = "Header not found", body = ErrorResponse, example = json!({"error": "Header not found at height: 10"})),
(status = INTERNAL_SERVER_ERROR, description = "Start/end header hash not found or header height mismatch", body = ErrorResponse),
),
)]🤖 Prompt for AI Agents
In applications/minotari_node/src/http/handler/get_utxos_by_block.rs around
lines 38 to 48, the OpenAPI operation metadata has inconsistencies: the
operation_id "sync_utxos_by_block" does not reflect that the endpoint fetches
UTXOs for a single block, and the response description incorrectly mentions a
range of headers instead of a single block hash. Update the operation_id to
something like "get_utxos_by_block" and revise the response description to
accurately state that UTXOs are returned for the specified block hash.
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs
Outdated
Show resolved
Hide resolved
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs
Outdated
Show resolved
Hide resolved
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (3)
242-242:⚠️ Potential issueRemove all debug print statements.
Multiple debug log statements with "here" should be removed before merging.
Remove all instances of:
-info!(target: LOG_TARGET, "here");Also applies to: 286-286, 445-445, 455-455, 468-468, 473-473, 485-485, 501-501, 506-506, 544-544
607-661:⚠️ Potential issueImplement the TODO for importing UTXOs to transaction service.
The
import_utxos_to_transaction_servicemethod has a TODO comment and the entire implementation is commented out. This appears to be critical functionality that's missing.Would you like me to help implement the UTXO import logic or create an issue to track this missing functionality?
613-659: 🛠️ Refactor suggestionRemove large block of commented out code.
The commented implementation should be removed. If this functionality is needed, it should be properly implemented or tracked in an issue.
Remove lines 614-659 which contain the commented out implementation.
🧹 Nitpick comments (1)
base_layer/core/src/base_node/rpc/models/sync_utxos_by_block.rs (1)
41-41: Remove the commented out code.If the
scriptfield is not needed, remove the commented line to keep the code clean.- // pub script: Vec<u8>,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
applications/minotari_node/src/http/server.rs(3 hunks)base_layer/core/src/base_node/rpc/models/sync_utxos_by_block.rs(1 hunks)base_layer/core/src/base_node/rpc/query_service.rs(4 hunks)base_layer/wallet/src/lib.rs(2 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- base_layer/wallet/src/lib.rs
- applications/minotari_node/src/http/server.rs
- base_layer/core/src/base_node/rpc/query_service.rs
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
🔇 Additional comments (4)
base_layer/core/src/base_node/rpc/models/sync_utxos_by_block.rs (1)
12-21: Good improvement on the limit value.The maximum limit has been increased from 5 to 2000, which allows for more efficient synchronization as suggested in previous reviews.
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (3)
481-499: Verify the incomplete scanning logic.The code checks for found outputs but the actual import logic seems incomplete. After finding outputs, it fetches the block but then calls
import_utxos_to_transaction_servicewhich only has a TODO.This scanning logic appears to be incomplete. The flow is:
- Search for owned outputs
- If found, fetch the full block
- Call import_utxos_to_transaction_service (which is not implemented)
Please verify that this partial implementation is intentional or if it needs to be completed before merging.
509-534: Clarify the commented out recovery tracking logic.Lines 511-512 and 531-532 contain commented out code for tracking recovery counts and amounts. This seems like important functionality for the scanner.
- // count += scanned_block.num_outputs.unwrap_or(0); - // amount += scanned_block.amount.unwrap_or_else(|| 0.into())and
- // num_recovered = num_recovered.saturating_add(count); - // total_amount += amount;Are these recovery metrics no longer needed, or should they be properly implemented?
557-605: Good refactoring of the output search logic.The method has been properly renamed from
scan_for_outputstosearch_for_owned_outputsand updated to work withMinimalUtxoSyncInfo. The async key recovery integration looks correct.
base_layer/core/src/base_node/rpc/models/sync_utxos_by_block.rs
Outdated
Show resolved
Hide resolved
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs
Outdated
Show resolved
Hide resolved
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (3)
265-265: Remove debug print statements.Multiple instances of debug print statements that should be removed before merging.
Remove all instances of:
-info!(target: LOG_TARGET, "here");Also applies to: 311-311, 470-470, 481-481, 494-494, 498-498, 526-526, 532-532, 536-536, 576-576
206-211: Remove commented out code.This commented code block should be removed to keep the codebase clean.
- // let address = rpc_client.get_wallet_query_http_service_address().await?; - // if address.http_address.is_empty() { - // Err(UtxoScannerError::BaseNodeWalletServiceUrlEmpty) - // } else { Ok(http::Client::new(self.resources.http_client_url.clone())) - // }
813-817: Remove commented out method.The
get_next_peermethod is no longer needed with the HTTP client approach and should be removed.- // fn get_next_peer(&mut self) -> Option<NodeId> { - // let peer = self.peer_seeds.get(self.peer_index).map(NodeId::from_public_key); - // self.peer_index += 1; - // peer - // }
🧹 Nitpick comments (2)
base_layer/wallet/src/utxo_scanner_service/uxto_scanner_service_builder.rs (1)
23-23: Remove unused imports.The imports
std::anyandcrate::schema::client_key_values::keyare not used anywhere in this file and should be removed to keep the code clean.-use std::any;- schema::client_key_values::key,Also applies to: 39-39
base_layer/core/src/base_node/rpc/query_service.rs (1)
194-194: Use consistent database access pattern.Line 194 uses
self.dbdirectly while the rest of the method usesself.db(). For consistency, use the helper method throughout.- let end_header = self - .db + let end_header = self + .db()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
applications/minotari_console_wallet/src/automation/commands.rs(4 hunks)applications/minotari_console_wallet/src/init/mod.rs(2 hunks)applications/minotari_console_wallet/src/recovery.rs(3 hunks)applications/minotari_console_wallet/src/wallet_modes.rs(1 hunks)applications/minotari_node/src/http/handler/mod.rs(1 hunks)base_layer/core/src/base_node/rpc/models/get_utxos_by_block.rs(1 hunks)base_layer/core/src/base_node/rpc/models/mod.rs(1 hunks)base_layer/core/src/base_node/rpc/models/sync_utxos_by_block.rs(1 hunks)base_layer/core/src/base_node/rpc/query_service.rs(4 hunks)base_layer/core/src/transactions/transaction_components/encrypted_data.rs(1 hunks)base_layer/core/src/transactions/transaction_key_manager/inner.rs(1 hunks)base_layer/core/src/transactions/transaction_key_manager/wrapper.rs(1 hunks)base_layer/wallet/src/lib.rs(2 hunks)base_layer/wallet/src/output_manager_service/recovery/mod.rs(1 hunks)base_layer/wallet/src/output_manager_service/recovery/output_recovery_info.rs(1 hunks)base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs(1 hunks)base_layer/wallet/src/utxo_scanner_service/initializer.rs(6 hunks)base_layer/wallet/src/utxo_scanner_service/service.rs(6 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(14 hunks)base_layer/wallet/src/utxo_scanner_service/uxto_scanner_service_builder.rs(10 hunks)base_layer/wallet/src/wallet.rs(2 hunks)base_layer/wallet_ffi/src/lib.rs(5 hunks)clients/rust/base_node_wallet_client/src/client/http.rs(2 hunks)clients/rust/base_node_wallet_client/src/client/mod.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- base_layer/wallet/src/output_manager_service/recovery/output_recovery_info.rs
🚧 Files skipped from review as they are similar to previous changes (20)
- base_layer/wallet/src/output_manager_service/recovery/mod.rs
- base_layer/core/src/base_node/rpc/models/mod.rs
- base_layer/wallet/src/wallet.rs
- base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs
- applications/minotari_console_wallet/src/recovery.rs
- base_layer/core/src/transactions/transaction_components/encrypted_data.rs
- base_layer/wallet/src/lib.rs
- base_layer/wallet/src/utxo_scanner_service/initializer.rs
- applications/minotari_console_wallet/src/init/mod.rs
- applications/minotari_console_wallet/src/automation/commands.rs
- clients/rust/base_node_wallet_client/src/client/mod.rs
- base_layer/wallet_ffi/src/lib.rs
- base_layer/core/src/base_node/rpc/models/get_utxos_by_block.rs
- base_layer/core/src/base_node/rpc/models/sync_utxos_by_block.rs
- base_layer/core/src/transactions/transaction_key_manager/inner.rs
- clients/rust/base_node_wallet_client/src/client/http.rs
- applications/minotari_node/src/http/handler/mod.rs
- applications/minotari_console_wallet/src/wallet_modes.rs
- base_layer/core/src/transactions/transaction_key_manager/wrapper.rs
- base_layer/wallet/src/utxo_scanner_service/service.rs
🧰 Additional context used
🧠 Learnings (1)
base_layer/core/src/base_node/rpc/query_service.rs (1)
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
🔇 Additional comments (3)
base_layer/wallet/src/utxo_scanner_service/uxto_scanner_service_builder.rs (1)
104-107: HTTP URL validation and error handling looks good.The builder correctly validates that the HTTP node URL is set before building the service and provides clear error messages. The transition from peer-based to HTTP-based architecture is well implemented.
Also applies to: 116-123, 175-182
base_layer/core/src/base_node/rpc/query_service.rs (1)
88-147: New query methods are well implemented.The
fetch_kernel,fetch_utxos_by_block, andfetch_utxosmethods provide comprehensive blockchain and mempool querying capabilities with proper validation, error handling, and pagination logic. The mempool integration for transaction status queries is particularly well designed.Also applies to: 149-177, 179-289
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (1)
205-212: HTTP client transition is well implemented.The refactor from peer-based RPC to HTTP client is properly executed. The new methods for HTTP client creation, block scanning logic, streaming API usage, and output scanning with key manager integration are all correctly implemented.
Also applies to: 214-255, 471-477, 589-639
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
clients/rust/base_node_wallet_client/src/client/http.rs (2)
107-107: Remove JSON body from GET request.Sending a JSON body with a GET request is non-standard. The header_hash is already included in the query string, making the JSON body redundant.
122-181: Address hardcoded limit and error handling issues.The implementation still has issues previously identified:
- The limit is hardcoded to 10 (line 135)
- Error handling doesn't break the loop - it will continue indefinitely on failure
Consider making the page size configurable and breaking the loop on errors:
+const DEFAULT_PAGE_SIZE: usize = 10; async fn sync_utxos_by_block( &self, start_header_hash: Vec<u8>, end_header_hash: Vec<u8>, shutdown: ShutdownSignal, ) -> Result<mpsc::Receiver<Result<SyncUtxosByBlockResponse, anyhow::Error>>, anyhow::Error> { debug!(target: LOG_TARGET, "Starting UTXO sync from {} to {}", start_header_hash.to_hex(), end_header_hash.to_hex()); let mut target_url = self.api_address.join("/sync_utxos_by_block")?; let (resp_tx, resp_rx) = mpsc::channel(1000); let start_header_hash_hex = start_header_hash.to_hex(); let end_header_hash_hex = end_header_hash.to_hex(); let client = self.http_client.clone(); - let limit = 10; + let limit = DEFAULT_PAGE_SIZE; tokio::spawn(async move { let mut page = 0; let mut has_next_page = true; while has_next_page { if shutdown.is_triggered() { info!(target: LOG_TARGET, "UTXO sync task shutdown triggered, exiting loop"); break; } // ... existing code ... match client.get(target_url.clone()).send().await { Ok(response) => match response.json::<SyncUtxosByBlockResponse>().await { Ok(response) => { has_next_page = response.has_next_page; debug!(target: LOG_TARGET, "Received UTXOs for page {}", page); if let Err(send_error) = resp_tx.send(Ok(response)).await { error!(target: LOG_TARGET, "Error sending utxo response: {:?}", send_error); + break; // Channel closed } }, Err(error) => { if let Err(send_error) = resp_tx.send(Err(error.into())).await { error!(target: LOG_TARGET, "Error sending error result: {:?}", send_error); } + break; // Stop on error }, }, Err(error) => { if let Err(send_error) = resp_tx.send(Err(error.into())).await { error!(target: LOG_TARGET, "Error sending error result: {:?}", send_error); } + break; // Stop on error }, }base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (2)
274-592: Remove all debug print statements.Multiple instances of uninformative debug print statements need to be removed throughout the file.
Remove all occurrences of:
-info!(target: LOG_TARGET, "here");This applies to lines: 274, 276, 278, 282, 299, 305, 323, 471, 486, 497, 510, 514, 532, 542, 548, 553, 592
824-828: Remove commented out method.The
get_next_peermethod is obsolete with the HTTP client approach and should be removed.- // fn get_next_peer(&mut self) -> Option<NodeId> { - // let peer = self.peer_seeds.get(self.peer_index).map(NodeId::from_public_key); - // self.peer_index += 1; - // peer - // }
🧹 Nitpick comments (1)
applications/minotari_console_wallet/log4rs_sample.yml (1)
96-96: Remove trailing spaces.- encoder: - pattern: "{d(%Y-%m-%d %H:%M:%S.%f)} [{t}] [Thread:{I}] {l:5} {m} // {f}:{L}{n}" + encoder: + pattern: "{d(%Y-%m-%d %H:%M:%S.%f)} [{t}] [Thread:{I}] {l:5} {m} // {f}:{L}{n}"🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 96-96: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.toml(2 hunks)applications/minotari_console_wallet/log4rs_sample.yml(6 hunks)base_layer/core/src/base_node/rpc/models/sync_utxos_by_block.rs(1 hunks)base_layer/wallet/src/error.rs(2 hunks)base_layer/wallet/src/utxo_scanner_service/error.rs(0 hunks)base_layer/wallet/src/utxo_scanner_service/mod.rs(0 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(20 hunks)clients/rust/base_node_wallet_client/Cargo.toml(1 hunks)clients/rust/base_node_wallet_client/src/client/http.rs(2 hunks)clients/rust/base_node_wallet_client/src/client/mod.rs(1 hunks)clients/rust/base_node_wallet_client/src/error.rs(0 hunks)clients/rust/base_node_wallet_client/src/lib.rs(0 hunks)
💤 Files with no reviewable changes (4)
- base_layer/wallet/src/utxo_scanner_service/mod.rs
- clients/rust/base_node_wallet_client/src/lib.rs
- clients/rust/base_node_wallet_client/src/error.rs
- base_layer/wallet/src/utxo_scanner_service/error.rs
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- clients/rust/base_node_wallet_client/Cargo.toml
- base_layer/wallet/src/error.rs
- base_layer/core/src/base_node/rpc/models/sync_utxos_by_block.rs
- clients/rust/base_node_wallet_client/src/client/mod.rs
🧰 Additional context used
🪛 YAMLlint (1.37.1)
applications/minotari_console_wallet/log4rs_sample.yml
[error] 96-96: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
applications/minotari_console_wallet/log4rs_sample.yml (2)
66-96: LGTM!The new
utxo_scannerandhttpappenders are well-configured and follow the established pattern of other appenders in the file.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 96-96: trailing spaces
(trailing-spaces)
146-157: LGTM!The new logger configurations for
wallet::utxo_scanningandtari::wallet::client::httpare properly set up with appropriate appenders and non-additive settings.base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (1)
605-650: LGTM!The
search_for_owned_outputsimplementation correctly handles output decryption using both view key and recovery key approaches, with appropriate error handling.
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
clients/rust/base_node_wallet_client/src/client/http.rs (2)
112-133:⚠️ Potential issueRemove JSON body from GET request.
Sending a JSON body with a GET request (line 119) is non-standard. The header hash is already included in the query parameters, so the JSON body should be removed.
Apply this diff to fix the issue:
let res = self .http_client .get(target_url) - .json(&models::GetUtxosByBlockRequest { header_hash }) .send() .await?;
135-194: 🛠️ Refactor suggestionMake page size configurable and improve error handling.
The implementation has several areas for improvement:
- The page size is hardcoded to 10 (line 148)
- Error handling continues the loop indefinitely on failures
Consider these improvements:
+const DEFAULT_PAGE_SIZE: usize = 10; + async fn sync_utxos_by_block( &self, start_header_hash: Vec<u8>, end_header_hash: Vec<u8>, shutdown: ShutdownSignal, ) -> Result<mpsc::Receiver<Result<SyncUtxosByBlockResponse, anyhow::Error>>, anyhow::Error> { debug!(target: LOG_TARGET, "Starting UTXO sync from {} to {}", start_header_hash.to_hex(), end_header_hash.to_hex()); let mut target_url = self.api_address.join("/sync_utxos_by_block")?; let (resp_tx, resp_rx) = mpsc::channel(1000); let start_header_hash_hex = start_header_hash.to_hex(); let end_header_hash_hex = end_header_hash.to_hex(); let client = self.http_client.clone(); - let limit = 10; + let limit = DEFAULT_PAGE_SIZE; tokio::spawn(async move { let mut page = 0; let mut has_next_page = true; + let mut consecutive_errors = 0; + const MAX_CONSECUTIVE_ERRORS: usize = 3; while has_next_page { if shutdown.is_triggered() { info!(target: LOG_TARGET, "UTXO sync task shutdown triggered, exiting loop"); break; } // ... rest of the code ... match client.get(target_url.clone()).send().await { Ok(response) => match response.json::<SyncUtxosByBlockResponse>().await { Ok(response) => { + consecutive_errors = 0; // Reset error counter on success has_next_page = response.has_next_page; // ... rest of the code ... }, Err(error) => { + consecutive_errors += 1; + if consecutive_errors >= MAX_CONSECUTIVE_ERRORS { + error!(target: LOG_TARGET, "Too many consecutive errors, stopping sync"); + break; + } if let Err(send_error) = resp_tx.send(Err(error.into())).await { error!(target: LOG_TARGET, "Error sending error result: {:?}", send_error); } }, }, Err(error) => { + consecutive_errors += 1; + if consecutive_errors >= MAX_CONSECUTIVE_ERRORS { + error!(target: LOG_TARGET, "Too many consecutive errors, stopping sync"); + break; + } if let Err(send_error) = resp_tx.send(Err(error.into())).await { error!(target: LOG_TARGET, "Error sending error result: {:?}", send_error); } }, }base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (4)
207-214: Remove commented out code.This commented code block should be removed to keep the codebase clean.
- // let address = rpc_client.get_wallet_query_http_service_address().await?; - // if address.http_address.is_empty() { - // Err(anyhow::Error::BaseNodeWalletServiceUrlEmpty) - // } else { Ok(http::Client::new(self.resources.http_client_url.clone())) - // }
823-827: Remove commented out method.The
get_next_peermethod is no longer needed with the HTTP client approach and should be removed.- // fn get_next_peer(&mut self) -> Option<NodeId> { - // let peer = self.peer_seeds.get(self.peer_index).map(NodeId::from_public_key); - // self.peer_index += 1; - // peer - // }
274-275:⚠️ Potential issueRemove all debug print statements.
Multiple instances of debug print statements need to be removed before merging.
Remove all instances of:
-info!(target: LOG_TARGET, "here");Also applies to lines: 483, 494, 507, 511, 520, 539, 545, 549, 567, 589
Also applies to: 276-277, 279-280, 282-283, 299-300, 305-306, 315-316, 323-324
542-543:⚠️ Potential issueRemove or implement incomplete TODO markers.
There are incomplete
todo!()comments that suggest unfinished implementation.Either implement the intended functionality or remove these
todo!()calls if the code is not required:-// todo!();Also applies to: 547-548
🧹 Nitpick comments (1)
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (1)
845-851: Remove commented out code block.This commented code should be removed to maintain code cleanliness.
- // let block_height_birthday = client - // .get_height_at_time(epoch_time_birthday) - // .await - // .unwrap_or_else(|e| { - // warn!(target: LOG_TARGET, "Problem requesting `height_at_time` from Base Node: {}", e); - // 0 - // });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(20 hunks)clients/rust/base_node_wallet_client/src/client/http.rs(2 hunks)clients/rust/base_node_wallet_client/src/client/mod.rs(1 hunks)
🔇 Additional comments (1)
clients/rust/base_node_wallet_client/src/client/mod.rs (1)
5-30: LGTM! Good refactoring to standardize error handling.The changes to use
anyhow::Errorinstead of a custom error type simplifies error handling across the codebase. The modification to returnOption<BlockHeader>forget_header_by_heightis a good design choice that explicitly handles the case where a header might not exist at a given height.
| loop { | ||
| next_header = wallet_service_client | ||
| .get_header_by_height(last_scanned_block.height + 1) | ||
| .await?; | ||
| if !next_header.is_none() { | ||
| break; | ||
| } | ||
| height = height.saturating_sub(1); | ||
| } | ||
| let next_header = next_header.unwrap(); |
There was a problem hiding this comment.
Fix potential infinite loop in header search.
The loop searching for a known header could run indefinitely if no header is found. Add a lower bound to prevent this.
loop {
next_header = wallet_service_client
.get_header_by_height(last_scanned_block.height + 1)
.await?;
if !next_header.is_none() {
break;
}
+ if height == 0 {
+ return Err(anyhow!("No known headers found in base node"));
+ }
height = height.saturating_sub(1);
}📝 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.
| loop { | |
| next_header = wallet_service_client | |
| .get_header_by_height(last_scanned_block.height + 1) | |
| .await?; | |
| if !next_header.is_none() { | |
| break; | |
| } | |
| height = height.saturating_sub(1); | |
| } | |
| let next_header = next_header.unwrap(); | |
| loop { | |
| next_header = wallet_service_client | |
| .get_header_by_height(last_scanned_block.height + 1) | |
| .await?; | |
| if !next_header.is_none() { | |
| break; | |
| } | |
| if height == 0 { | |
| return Err(anyhow!("No known headers found in base node")); | |
| } | |
| height = height.saturating_sub(1); | |
| } | |
| let next_header = next_header.unwrap(); |
🤖 Prompt for AI Agents
In base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs around lines
225 to 234, the loop that searches for the next header can run indefinitely if
no header is found. To fix this, add a lower bound check on the height variable
inside the loop and break or return an error if the height reaches zero or
another minimum value, preventing an infinite loop.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (3)
203-210: Remove commented-out code.The commented-out code should be removed to keep the codebase clean.
- /// Try to instantiate a Base Node Wallet Service client. - fn base_node_wallet_service_client(&self) -> Result<http::Client, anyhow::Error> { - // let address = rpc_client.get_wallet_query_http_service_address().await?; - // if address.http_address.is_empty() { - // Err(anyhow::Error::BaseNodeWalletServiceUrlEmpty) - // } else { - Ok(http::Client::new(self.resources.http_client_url.clone())) - // } - } + /// Try to instantiate a Base Node Wallet Service client. + fn base_node_wallet_service_client(&self) -> Result<http::Client, anyhow::Error> { + Ok(http::Client::new(self.resources.http_client_url.clone())) + }
212-256: Fix potential infinite loop in header search.The loop searching for a known header could run indefinitely if no header is found. This is the same issue identified in past reviews.
loop { next_header = wallet_service_client .get_header_by_height(last_scanned_block.height + 1) .await?; if !next_header.is_none() { break; } + if height == 0 { + return Err(anyhow!("No known headers found in base node")); + } height = height.saturating_sub(1); }
765-769: Remove commented-out method.The
get_next_peermethod is no longer needed with the HTTP client approach and should be removed as previously noted.- // fn get_next_peer(&mut self) -> Option<NodeId> { - // let peer = self.peer_seeds.get(self.peer_index).map(NodeId::from_public_key); - // self.peer_index += 1; - // peer - // }
🧹 Nitpick comments (1)
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (1)
787-815: Remove large commented-out code block.The commented-out code for getting height at time should be removed to clean up the codebase.
- // let block_height_birthday = client - // .get_height_at_time(epoch_time_birthday) - // .await - // .unwrap_or_else(|e| { - // warn!(target: LOG_TARGET, "Problem requesting `height_at_time` from Base Node: {}", e); - // 0 - // });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
applications/minotari_console_wallet/src/automation/commands.rs(5 hunks)applications/minotari_console_wallet/src/recovery.rs(4 hunks)base_layer/wallet/src/storage/sqlite_db/scanned_blocks.rs(2 hunks)base_layer/wallet/src/utxo_scanner_service/handle.rs(1 hunks)base_layer/wallet/src/utxo_scanner_service/service.rs(6 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(15 hunks)base_layer/wallet_ffi/src/tasks.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- applications/minotari_console_wallet/src/automation/commands.rs
- base_layer/wallet/src/utxo_scanner_service/handle.rs
- base_layer/wallet_ffi/src/tasks.rs
- applications/minotari_console_wallet/src/recovery.rs
- base_layer/wallet/src/utxo_scanner_service/service.rs
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (6)
base_layer/wallet/src/storage/sqlite_db/scanned_blocks.rs (2)
54-62: LGTM: Constructor correctly nullifies removed fields.The constructor properly sets both
amountandnum_outputstoNone, which aligns with the broader refactoring to remove these fields from the data model.
103-113: LGTM: Conversion trait properly handles field removal.The
From<ScannedBlock>implementation correctly sets bothamountandnum_outputstoNone, maintaining consistency with the simplifiedScannedBlockdata model that no longer tracks these values.base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (4)
23-72: LGTM: Import updates support the HTTP client refactoring.The new imports correctly support the architectural shift to HTTP-based communication, including the new HTTP client, error handling with
anyhow, and updated RPC models for UTXO synchronization.
90-100: LGTM: Generic type parameter addition enhances flexibility.The addition of the
TKeyManagergeneric type parameter allows for better abstraction and testability of the key management functionality.
544-589: LGTM: Key recovery logic is well-implemented.The
search_for_owned_outputsmethod correctly implements both view key-based detection for change outputs and DH-based detection for received outputs. The error handling and key derivation logic appear sound.
470-501: LGTM: Output processing logic is well-structured.The two-phase approach of first identifying owned outputs with minimal data, then fetching full block data for confirmed matches is an efficient strategy that minimizes bandwidth usage while ensuring complete recovery.
| let mut utxo_stream = client | ||
| .sync_utxos_by_block( | ||
| start_header_hash.to_vec(), | ||
| end_header_hash.to_vec(), | ||
| self.shutdown_signal.clone(), | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify shutdown signal handling in streaming.
Ensure that the shutdown signal is properly handled during UTXO streaming to prevent resource leaks or hanging connections.
The shutdown signal is passed to the streaming method, but verify that the HTTP client properly terminates the stream when the signal is triggered:
🏁 Script executed:
#!/bin/bash
# Description: Check how shutdown signals are handled in the HTTP client streaming implementation
# Search for shutdown signal handling in HTTP client
ast-grep --pattern 'fn sync_utxos_by_block($$$) {
$$$
}'
# Look for shutdown signal usage in HTTP client
rg -A 10 "shutdown.*signal" --type rustLength of output: 70876
🏁 Script executed:
#!/bin/bash
set -e
# Locate the implementation of `sync_utxos_by_block`
rg -n "sync_utxos_by_block" -nLength of output: 4085
🏁 Script executed:
#!/bin/bash
set -e
# Display the HTTP client implementation for `sync_utxos_by_block`
rg -n "async fn sync_utxos_by_block" -A50 clients/rust/base_node_wallet_client/src/client/http.rsLength of output: 3036
🏁 Script executed:
#!/bin/bash
set -e
# Show full async block in sync_utxos_by_block to verify shutdown handling
sed -n '135,260p' clients/rust/base_node_wallet_client/src/client/http.rsLength of output: 2987
🏁 Script executed:
#!/bin/bash
set -e
# Show utxo_stream and shutdown handling in utxo_scanner_task
rg -n "utxo_stream" -n base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs -A10 -B10
rg -n "shutdown_signal" -n base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs -A5 -B5Length of output: 3486
Add cancellation for in-flight UTXO streaming on shutdown
It looks like both the HTTP client’s sync_utxos_by_block and the scanner loop only check shutdown.is_triggered() between pages/messages, so a long-running HTTP request or a stalled recv().await could hang until it completes. To ensure timely shutdown and avoid leaked connections/tasks, introduce tokio::select! around the request and receive futures:
• In clients/rust/base_node_wallet_client/src/client/http.rs (around lines 152–186):
Replace
while has_next_page {
if shutdown.is_triggered() { break; }
let response = client.get(target_url.clone()).send().await?;
…
}with something like:
loop {
tokio::select! {
_ = shutdown.wait() => break,
resp = client.get(target_url.clone()).send() => match resp {
Ok(response) => { /* parse and send */ }
Err(err) => break,
}
}
if !has_next_page { break; }
page += 1;
}• In base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (around lines 447–461):
Wrap the receive in select! so you can abort waiting for the next page:
loop {
tokio::select! {
_ = self.shutdown_signal.wait() => return Ok(total_scanned),
msg = utxo_stream.recv() => match msg {
Some(response) => { … }
None => break,
}
}
…
}This change guarantees that both pending HTTP calls and channel receives are cancelled immediately when shutdown is triggered.
🤖 Prompt for AI Agents
In base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs around lines
447 to 453, the current UTXO streaming loop waits on utxo_stream.recv() without
concurrently checking the shutdown signal, which can cause hangs on shutdown. To
fix this, wrap the receive call in a tokio::select! block that concurrently
awaits either the shutdown_signal.wait() or utxo_stream.recv(), returning early
if shutdown is triggered. Similarly, in
clients/rust/base_node_wallet_client/src/client/http.rs around lines 152 to 186,
refactor the HTTP request loop to use tokio::select! to concurrently await the
shutdown signal or the HTTP request future, breaking the loop immediately if
shutdown occurs. This ensures timely cancellation of in-flight requests and
prevents resource leaks.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (4)
168-175: Remove commented out code and verify error handlingThe HTTP client creation logic is correct, but the commented out code should be removed for cleanliness. Also consider adding validation for the URL configuration.
- // let address = rpc_client.get_wallet_query_http_service_address().await?; - // if address.http_address.is_empty() { - // Err(anyhow::Error::BaseNodeWalletServiceUrlEmpty) - // } else { Ok(http::Client::new(self.resources.http_client_url.clone())) - // }
186-194: Add bounds check to prevent infinite loopThe loop searching for a known header could run indefinitely if no header is found. Add a lower bound to prevent this:
loop { next_header = wallet_service_client .get_header_by_height(last_scanned_block.height + 1) .await?; if !next_header.is_none() { break; } + if height == 0 { + return Err(anyhow!("No known headers found in base node")); + } height = height.saturating_sub(1); }
413-416: Improve shutdown signal handling during streamingThe current implementation only checks the shutdown signal between messages, which could cause delays during long-running HTTP requests. Consider wrapping the receive call in
tokio::select!to enable immediate cancellation:- while let Some(response) = utxo_stream.recv().await { - if self.shutdown_signal.is_triggered() { - return Ok((total_scanned, total_num_recovered, total_value_recovered)); - } + loop { + tokio::select! { + _ = self.shutdown_signal.wait() => return Ok((total_scanned, total_num_recovered, total_value_recovered)), + msg = utxo_stream.recv() => match msg { + Some(response) => { + let response = response?; + // ... process response + } + None => break, + } + } + }
726-730: Remove commented out methodThe
get_next_peermethod is no longer needed with the HTTP client approach and should be removed:- // fn get_next_peer(&mut self) -> Option<NodeId> { - // let peer = self.peer_seeds.get(self.peer_index).map(NodeId::from_public_key); - // self.peer_index += 1; - // peer - // }
🧹 Nitpick comments (3)
base_layer/wallet/tests/transaction_service_tests/service.rs (1)
240-240: Consider making the test URL configurable or use a constant.The hardcoded localhost URL works for tests, but consider extracting it to a constant at the module level for better maintainability and consistency across tests.
+const TEST_HTTP_NODE_URL: &str = "http://127.0.0.1:5434"; + // ... later in the function - let http_node_url = Url::parse("http://127.0.0.1:5434").unwrap(); + let http_node_url = Url::parse(TEST_HTTP_NODE_URL).unwrap();base_layer/wallet/tests/utxo_scanner/mod.rs (1)
727-727: Remove commented out codeThe commented out line
// let num_outputs = if h % 2 == 1 { Some(1) } else { None };should be removed to keep the codebase clean, as it appears to reference fields that no longer exist in theScannedBlockstruct.- // let num_outputs = if h % 2 == 1 { Some(1) } else { None };base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (1)
748-754: Consider removing commented out codeThe commented out code block for
get_height_at_timerequest should be removed if it's no longer needed, or properly implemented if it's required functionality.- // let block_height_birthday = client - // .get_height_at_time(epoch_time_birthday) - // .await - // .unwrap_or_else(|e| { - // warn!(target: LOG_TARGET, "Problem requesting `height_at_time` from Base Node: {}", e); - // 0 - // });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.cargo/config.toml(1 hunks)applications/minotari_console_wallet/src/automation/commands.rs(4 hunks)applications/minotari_console_wallet/src/recovery.rs(4 hunks)base_layer/core/src/transactions/transaction_components/test.rs(2 hunks)base_layer/core/src/transactions/transaction_components/transaction_output.rs(1 hunks)base_layer/core/src/transactions/transaction_components/wallet_output_builder.rs(1 hunks)base_layer/core/src/transactions/transaction_protocol/recipient.rs(1 hunks)base_layer/core/src/transactions/transaction_protocol/sender.rs(1 hunks)base_layer/wallet/src/utxo_scanner_service/handle.rs(1 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(15 hunks)base_layer/wallet/tests/transaction_service_tests/service.rs(4 hunks)base_layer/wallet/tests/utxo_scanner/mod.rs(8 hunks)base_layer/wallet_ffi/src/tasks.rs(2 hunks)base_layer/wallet_ffi/wallet.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .cargo/config.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- base_layer/wallet/src/utxo_scanner_service/handle.rs
- base_layer/wallet_ffi/src/tasks.rs
- applications/minotari_console_wallet/src/recovery.rs
- applications/minotari_console_wallet/src/automation/commands.rs
- base_layer/wallet_ffi/wallet.h
🧰 Additional context used
🧬 Code Graph Analysis (1)
base_layer/wallet/tests/utxo_scanner/mod.rs (3)
base_layer/wallet/src/utxo_scanner_service/initializer.rs (1)
UtxoScannerService(124-128)comms/core/src/test_utils/mocks/connectivity_manager.rs (1)
create_connectivity_mock(43-50)base_layer/wallet/src/utxo_scanner_service/service.rs (1)
builder(107-109)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (18)
base_layer/core/src/transactions/transaction_components/transaction_output.rs (1)
632-632: LGTM! Interface change correctly implemented.The update to pass separate
commitment()andencrypted_data()parameters instead of the entireTransactionOutputaligns with the broader interface refactor described in the AI summary. The accessor methods are used correctly and the functionality remains equivalent.base_layer/core/src/transactions/transaction_protocol/sender.rs (1)
1725-1725: LGTM! Consistent interface adaptation.The update correctly adapts to the new
try_output_key_recoverymethod signature by passing separatecommitment()andencrypted_data()parameters. This change is consistent with the broader interface refactor across the codebase.base_layer/core/src/transactions/transaction_components/wallet_output_builder.rs (1)
396-396: Interface change correctly implemented in test.The update to
try_output_key_recoverycorrectly passescommitment()andencrypted_data()as separate parameters instead of the full output object, maintaining the same test logic while conforming to the updated interface.base_layer/core/src/transactions/transaction_protocol/recipient.rs (1)
250-250: Consistent interface update in recipient protocol test.The change correctly updates the
try_output_key_recoverycall to use separate commitment and encrypted data parameters, maintaining test functionality while conforming to the new interface.base_layer/core/src/transactions/transaction_components/test.rs (2)
104-104: Interface update correctly applied in key manager test.The change properly updates the
try_output_key_recoverycall to pass commitment and encrypted data separately, maintaining the test's validation logic.
572-572: Consistent interface change in output recovery test.The update correctly implements the new method signature while preserving the test's functionality to validate output key recovery.
base_layer/wallet/tests/transaction_service_tests/service.rs (3)
174-174: LGTM: Import addition for URL handling.The import of
url::Urlis correctly added to support the new HTTP-based UTXO syncing functionality.
287-287: LGTM: Correct parameter passing for HTTP-based UTXO scanning.The HTTP node URL is properly passed to the
UtxoScannerServiceInitializer, which aligns with the refactoring from peer-based RPC to HTTP calls.
1563-1563: LGTM: Method signature correctly updated.The call to
try_output_key_recoveryhas been properly updated to pass the output's commitment and encrypted data separately instead of the entire output object, matching the new interface described in the AI summary.base_layer/wallet/tests/utxo_scanner/mod.rs (3)
89-89: LGTM: Generic type parameter correctly updatedThe change from
WalletConnectivityMocktoMemoryDbKeyManagerproperly reflects the architectural shift from peer-based connectivity to key manager-based operations.
177-211: LGTM: Service builder correctly updated for new architectureThe builder configuration properly uses the new
MemoryDbKeyManagergeneric parameter and passes the key manager tobuild_with_resources. The simplified configuration without connectivity mocks aligns with the HTTP client approach.
374-374: LGTM: Event pattern matching correctly updatedThe explicit ignoring of the
time_takenfield withtime_taken: _,properly handles the updatedUtxoScannerEvent::Completedstructure while maintaining test functionality.Also applies to: 522-524, 706-708
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (6)
67-83: LGTM: Struct updates properly support new architectureThe new
SyncResultstruct provides good encapsulation of scanning results, and the addition of theTKeyManagergeneric parameter toUtxoScannerTaskcorrectly supports the key manager-based approach.
89-135: LGTM: Run method correctly updated for new architectureThe updated
run()method properly uses the newSyncResultstruct and maintains appropriate error handling and retry logic for the HTTP client approach.
224-289: LGTM: Sync method correctly implements HTTP client approachThe
attempt_sync()method is properly refactored to use the HTTP client, correctly accumulates scanning results, and maintains appropriate error handling and shutdown signal checking.
503-550: LGTM: Output recovery logic correctly implementedThe
search_for_owned_outputs()method properly implements key-based output recovery using the view key for change outputs and sender offset recovery for received outputs. Error handling for invalid commitments and keys is appropriate.
615-615: LGTM: Performance improvement with slice referenceChanging the parameter from
Vec<(WalletOutput, ImportStatus, TxId, TransactionOutput)>to a slice reference avoids unnecessary cloning and improves performance.
667-685: LGTM: Error handling improved with anyhow::ErrorThe updated return types using
anyhow::Errorprovide better flexibility for error handling while maintaining the same logic for recovery mode management.
| @@ -101,7 +101,7 @@ async fn key_manager_input() { | |||
| .expect("Should be able to create transaction output"); | |||
|
|
|||
| assert_eq!(*input.features().unwrap(), OutputFeatures::default()); | |||
| let (_, value, _) = key_manager.try_output_key_recovery(&output, None).await.unwrap(); | |||
| let (_, value, _) = key_manager.try_output_key_recovery(output.commitment(), output.encrypted_data(), None).await.unwrap(); | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify all usages of try_output_key_recovery have been updated.
Ensure that all calls to try_output_key_recovery throughout the codebase have been updated to use the new signature with separate commitment and encrypted_data parameters.
Also applies to: 572-572
🏁 Script executed:
#!/bin/bash
# Description: Search for any remaining old-style calls to try_output_key_recovery
# Expected: All calls should use the new signature with separate parameters
echo "Searching for try_output_key_recovery method calls..."
rg -A 2 -B 1 "try_output_key_recovery"
echo -e "\n=== Checking for potential old-style calls ==="
# Look for calls that might still pass a full output object
rg "try_output_key_recovery.*output[^.]" --type rust
echo -e "\n=== Checking method definition ==="
# Find the actual method definition to understand the new signature
ast-grep --pattern $'try_output_key_recovery($_, $_, $_) {
$$$
}'Length of output: 6461
Update remaining outdated call in documentation
One legacy usage of try_output_key_recovery remains in the docs. Please update it to the new signature with separate commitment and encrypted_data parameters:
• docs/guide/tari_transaction_creation_overview.md
- let (key, _value, _) = key_manager_alice.try_output_key_recovery(output, None).await.unwrap();
+ let (key, _value, _) = key_manager_alice
+ .try_output_key_recovery(output.commitment(), output.encrypted_data(), None)
+ .await
+ .unwrap();🤖 Prompt for AI Agents
In base_layer/core/src/transactions/transaction_components/test.rs at line 104,
ensure the call to try_output_key_recovery uses the new signature with separate
commitment and encrypted_data parameters, not a single output object. Update the
call to pass output.commitment() and output.encrypted_data() as separate
arguments, and verify all other calls in the codebase follow this pattern. Also,
update the legacy usage in docs/guide/tari_transaction_creation_overview.md
accordingly.
stringhandler
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
base_layer/wallet_ffi/src/lib.rs (1)
10021-10042: URL validation logic is comprehensive and handles edge cases well.The validation covers null pointers, string conversion failures, and URL parsing errors with appropriate error codes for each case.
Note: Previous review comments about formatting issues in this section have been addressed.
🧹 Nitpick comments (1)
base_layer/wallet_ffi/src/lib.rs (1)
6746-6746: Document breaking change in FFI interface.The addition of the
http_base_nodeparameter is a breaking change to the FFI interface. Ensure this is properly documented in release notes and migration guides.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
base_layer/wallet_ffi/src/lib.rs(37 hunks)base_layer/wallet_ffi/wallet.h(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- base_layer/wallet_ffi/wallet.h
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: ci
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
🔇 Additional comments (6)
base_layer/wallet_ffi/src/lib.rs (6)
84-84: Imports look correct for the new HTTP-based functionality.The addition of
WalletConnectivityInterfaceandWalletKeyManagerimports aligns with the architectural changes described in the PR objectives.Also applies to: 112-112
193-193: Good addition of URL parsing capability.The
url::Urlimport is necessary for the new HTTP URL validation logic.
9985-9985: Consistent FFI interface change for recovery function.The addition of
http_base_nodeparameter towallet_start_recoverymaintains consistency with thewallet_createfunction changes.
10008-10008: Architectural change from connectivity handle to key manager.The change in generic parameters from
WalletConnectivityHandletoWalletKeyManagerreflects the shift from peer-based to HTTP-based connectivity as described in the PR objectives.
10045-10045: Consistent method change for HTTP-based connectivity.The change from
.with_peers(peer_public_keys)to.with_http_node_url(http_url)aligns with the overall architectural shift to HTTP-based UTXO syncing.
11611-11612: Test updates are consistent and use appropriate test URL.All test cases have been updated to provide the required
http_base_nodeparameter using a consistent localhost URL for testing.Also applies to: 11665-11666, 11790-11791, 12027-12028, 12100-12101, 12186-12187, 12407-12408, 12553-12554, 12706-12707, 12979-12980, 13261-13262, 13531-13532, 13919-13920, 13990-13991
| let http_base_node = if http_base_node.is_null() { | ||
| None | ||
| } else { | ||
| let base_node = CStr::from_ptr(http_base_node) | ||
| .to_str() | ||
| .expect("A non-null base node should be able to be converted to string") | ||
| .to_owned(); | ||
| Some(base_node) | ||
| }; |
There was a problem hiding this comment.
Replace panic-prone .expect() with proper error handling.
Using .expect() in FFI code can cause crashes in the calling application if string conversion fails. This should be handled gracefully with proper error reporting.
- let http_base_node = if http_base_node.is_null() {
- None
- } else {
- let base_node = CStr::from_ptr(http_base_node)
- .to_str()
- .expect("A non-null base node should be able to be converted to string")
- .to_owned();
- Some(base_node)
- };
+ let http_base_node = if http_base_node.is_null() {
+ None
+ } else {
+ match CStr::from_ptr(http_base_node).to_str() {
+ Ok(s) => Some(s.to_owned()),
+ Err(_) => {
+ *error_out = LibWalletError::from(InterfaceError::PointerError("http_base_node contains invalid UTF-8".to_string())).code;
+ return ptr::null_mut();
+ }
+ }
+ };📝 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.
| let http_base_node = if http_base_node.is_null() { | |
| None | |
| } else { | |
| let base_node = CStr::from_ptr(http_base_node) | |
| .to_str() | |
| .expect("A non-null base node should be able to be converted to string") | |
| .to_owned(); | |
| Some(base_node) | |
| }; | |
| let http_base_node = if http_base_node.is_null() { | |
| None | |
| } else { | |
| match CStr::from_ptr(http_base_node).to_str() { | |
| Ok(s) => Some(s.to_owned()), | |
| Err(_) => { | |
| *error_out = LibWalletError::from( | |
| InterfaceError::PointerError( | |
| "http_base_node contains invalid UTF-8".to_string() | |
| ) | |
| ).code; | |
| return ptr::null_mut(); | |
| } | |
| } | |
| }; |
🤖 Prompt for AI Agents
In base_layer/wallet_ffi/src/lib.rs around lines 6895 to 6903, replace the
panic-prone .expect() call on string conversion with proper error handling.
Instead of unwrapping the result of to_str(), handle the Result explicitly by
checking for an error and returning an appropriate error code or message to the
caller, ensuring the FFI boundary does not cause the application to crash on
invalid input.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
base_layer/wallet/src/utxo_scanner_service/initializer.rs (1)
124-143: Panic-on-error (expect) remains – replace with proper error propagationIdentical feedback was already given earlier; the
expect("Failed to build UTXO scanner service")will still crash the
wallet in production. Convert the enclosing async closure toResultand bubble the error up instead of aborting.base_layer/wallet_ffi/src/lib.rs (1)
6898-6906:⚠️ Potential issueStill using
.expect()– FFI may abort the host processThe panic-prone
.expect()call survived the refactor although it was pointed out in an earlier review. Any invalid UTF-8 inhttp_base_nodewill unwind past the FFI boundary and crash the embedding application.- let base_node = CStr::from_ptr(http_base_node) - .to_str() - .expect("A non-null base node should be able to be converted to string") - .to_owned(); - Some(base_node) + match CStr::from_ptr(http_base_node).to_str() { + Ok(s) => Some(s.to_owned()), + Err(_) => { + *error_out = LibWalletError::from( + InterfaceError::PointerError("http_base_node contains invalid UTF-8".into()), + ) + .code; + return ptr::null_mut(); + } + }base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (1)
415-419: Shutdown still blocks onrecv().await
Previous feedback about wrapping therecv()in atokio::select!with the
shutdown signal hasn’t been addressed. The task will hang indefinitely if the
stream stalls after shutdown is triggered.
(See earlier review thread.)
🧹 Nitpick comments (9)
base_layer/wallet/src/utxo_scanner_service/initializer.rs (1)
49-67: Validatehttp_node_urlduring construction
Url::parsewill accept many “valid‐ish” inputs (e.g. missing scheme defaults tofile:).
Before storing the value, consider validating that
- the scheme is
httporhttps, and- the host part is present.
- pub fn new(backend: WalletDatabase<T>, network: Network, birthday_offset: u16, http_node_url: Url) -> Self { + pub fn new( + backend: WalletDatabase<T>, + network: Network, + birthday_offset: u16, + http_node_url: Url, + ) -> Self { + assert!( + matches!(http_node_url.scheme(), "http" | "https") && http_node_url.has_host(), + "UtxoScannerServiceInitializer: invalid base-node URL: {}", + http_node_url + );This fail-fast check prevents accidentally running against
file:///tmpor similar.base_layer/wallet/tests/utxo_scanner/mod.rs (2)
90-95: Introduce a type alias to tame the 3-parameter generic
UtxoScannerService<WalletSqliteDatabase, MemoryDbKeyManager, MockHttpClientFactory>is verbose and appears dozens of
times. A simple alias improves readability across the test module.type TestScannerService = UtxoScannerService< WalletSqliteDatabase, MemoryDbKeyManager, MockHttpClientFactory, >;Usage then becomes
Option<TestScannerService>.
822-830:1 swatchdog is too tight → intermittent CI flakinessHeavy CI boxes or the Tokio scheduler under load can legitimately take >1 s to spawn the scanner.
Bump the timeout or make it configurable to avoid spurious failures.-let delay = time::sleep(Duration::from_secs(1)); +let delay = time::sleep(Duration::from_secs(10));base_layer/wallet/src/utxo_scanner_service/service.rs (2)
204-208:HttpClientFactoryshould beSend + Sync
UtxoScannerServiceand the task spawn the factory across threads, but the bound is missing on the trait itself.
Adding it prevents accidental non-thread-safe factory implementations.-pub trait HttpClientFactory: Clone { +pub trait HttpClientFactory: Clone + Send + Sync + 'static {Note: existing impls already satisfy these bounds, so this is a zero-cost compile-time safety net.
221-227: Consider caching the HTTP client
DefaultHttpClientFactory::create_http_clientinstantiates a new HTTP client per call.
If the client maintains a connection pool or TLS session, reusing a single instance is cheaper:#[derive(Clone)] pub struct DefaultHttpClientFactory { - node_url: Url, + client: minotari_node_wallet_client::http::Client, } impl DefaultHttpClientFactory { pub fn new(node_url: Url) -> Self { - Self { node_url } + Self { + client: minotari_node_wallet_client::http::Client::new(node_url), + } } } impl HttpClientFactory for DefaultHttpClientFactory { type Client = minotari_node_wallet_client::http::Client; fn create_http_client(&self) -> Self::Client { - minotari_node_wallet_client::http::Client::new(self.node_url.clone()) + self.client.clone() } }(Assumes the upstream client is
Clone.)base_layer/wallet_ffi/src/lib.rs (2)
10030-10037: Minor: unnecessaryto_string()and inconsistent capitalization in error message
format!("Url is not valid: {}", e.to_string())creates an extra allocation; theDisplayimpl is already enough. Also prefer “URL” to keep messages consistent.- "Url is not valid: {}", - e.to_string() + "URL is not valid: {}", + e
7010-7015: Shadowing makes intent opaqueThe local
http_base_node(Option) shadows the raw pointer argument of the same name. While legal, it hampers readability and increases the chance of accidental misuse. Consider renaming tohttp_base_node_optor similar.base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (2)
528-530: Typo:offfset_pub_keyMinor readability nit – triple-f in the variable name.
-let offfset_pub_key = RistrettoPublicKey::from_canonical_bytes(&output.sender_offset_public_key) +let offset_pub_key = RistrettoPublicKey::from_canonical_bytes(&output.sender_offset_public_key)
542-546: Timing metrics are misleading
one_sided_timeis measured immediately after resettingstart, so it is
effectively always ≈ 0 ms. Either remove the second measurement or place it
around an actual workload.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
applications/minotari_console_wallet/src/recovery.rs(4 hunks)base_layer/core/src/base_node/rpc/models/get_tip_info.rs(1 hunks)base_layer/core/src/base_node/rpc/models/sync_utxos_by_block.rs(1 hunks)base_layer/wallet/src/utxo_scanner_service/initializer.rs(4 hunks)base_layer/wallet/src/utxo_scanner_service/service.rs(3 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(14 hunks)base_layer/wallet/src/utxo_scanner_service/uxto_scanner_service_builder.rs(6 hunks)base_layer/wallet/tests/support/base_node_http_service_mock.rs(1 hunks)base_layer/wallet/tests/support/mod.rs(1 hunks)base_layer/wallet/tests/utxo_scanner/mod.rs(13 hunks)base_layer/wallet_ffi/src/lib.rs(37 hunks)
✅ Files skipped from review due to trivial changes (1)
- base_layer/wallet/tests/support/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- base_layer/core/src/base_node/rpc/models/sync_utxos_by_block.rs
- base_layer/core/src/base_node/rpc/models/get_tip_info.rs
- applications/minotari_console_wallet/src/recovery.rs
- base_layer/wallet/src/utxo_scanner_service/uxto_scanner_service_builder.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
🔇 Additional comments (2)
base_layer/wallet/tests/support/base_node_http_service_mock.rs (1)
108-129:⚠️ Potential issueReturn an explicit error when header hash is unknown
get_utxos_by_blocksilently succeeds with an empty vector if neither the block nor the header exists.
Tests depending on a failure signal will pass unintentionally.- let header = state.blocks.values().find(|h| h.hash().to_vec() == header_hash); - let res = if let Some(header) = header { + let header = state.blocks.values().find(|h| h.hash().to_vec() == header_hash); + let res = if let Some(header) = header { ... } else { - return Err(Error::msg("Header not found for the given hash")); + return Err(Error::msg("Unknown header hash")); };Likely an incorrect or invalid review comment.
base_layer/wallet_ffi/src/lib.rs (1)
6748-6751: Update C-header / documentation for the new parameter
wallet_createnow requires anhttp_base_nodeparameter. Ensure the exported C header (wallet_ffi.h) and accompanying docs/examples reflect this breaking change so downstream integrators don’t segfault from mismatched call-sites.
| async fn sync_utxos_by_block( | ||
| &self, | ||
| start_header_hash: Vec<u8>, | ||
| end_header_hash: Vec<u8>, | ||
| shutdown: ShutdownSignal, | ||
| ) -> Result<mpsc::Receiver<Result<SyncUtxosByBlockResponse, Error>>, Error> { | ||
| let (tx, rx) = mpsc::channel(100); | ||
| let state2 = self.state.read().await; | ||
|
|
||
| let start_height = state2 | ||
| .blocks | ||
| .values() | ||
| .find(|b| b.hash().to_vec() == start_header_hash) | ||
| .map_or(0, |b| b.height); | ||
|
|
||
| let end_height = state2 | ||
| .blocks | ||
| .values() | ||
| .find(|b| b.hash().to_vec() == end_header_hash) | ||
| .map_or(0, |b| b.height); | ||
| let state = self.state.clone(); | ||
| tokio::spawn(async move { | ||
| let state = state.read().await; | ||
| let mut current_height = 0; | ||
| let mut blocks = vec![]; | ||
| let page_size = 5; | ||
|
|
||
| for height in start_height..=end_height { | ||
| if shutdown.is_triggered() { | ||
| break; | ||
| } | ||
| if let Some(ub) = state.utxos_by_block.get(&height) { | ||
| let block_header = state.blocks.get(&height).cloned(); | ||
| if let Some(header) = block_header { | ||
| blocks.push(BlockUtxoInfo { | ||
| header_hash: header.hash().to_vec(), | ||
| height: header.height, | ||
| outputs: ub | ||
| .utxos | ||
| .iter() | ||
| .map(|o| models::MinimalUtxoSyncInfo { | ||
| output_hash: o.hash().to_vec(), | ||
| commitment: o.commitment.as_bytes().to_vec(), | ||
| encrypted_data: o.encrypted_data.to_byte_vec(), | ||
| sender_offset_public_key: o.sender_offset_public_key.to_vec(), | ||
| }) | ||
| .collect(), | ||
| mined_timestamp: header.timestamp.as_u64(), | ||
| }); | ||
| current_height = height; | ||
| } | ||
| } | ||
| if blocks.len() >= page_size || height == end_height { | ||
| let has_next_page = height < end_height; | ||
| let response = SyncUtxosByBlockResponse { | ||
| blocks: blocks.clone(), | ||
| has_next_page, | ||
| }; | ||
| blocks.clear(); | ||
|
|
||
| if tx.send(Ok(response)).await.is_err() { | ||
| break; // Channel closed | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| Ok(rx) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
sync_utxos_by_block silently falls back to height 0
When the supplied start/end hashes aren’t found, start_height/end_height default to 0, which can mask test setup
errors. Prefer early validation:
-let start_height = ...map_or(0, |b| b.height);
-let end_height = ...map_or(0, |b| b.height);
+let start_height = ...ok_or_else(|| Error::msg("start_header_hash not found"))?.height;
+let end_height = ...ok_or_else(|| Error::msg("end_header_hash not found"))?.height;📝 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.
| async fn sync_utxos_by_block( | |
| &self, | |
| start_header_hash: Vec<u8>, | |
| end_header_hash: Vec<u8>, | |
| shutdown: ShutdownSignal, | |
| ) -> Result<mpsc::Receiver<Result<SyncUtxosByBlockResponse, Error>>, Error> { | |
| let (tx, rx) = mpsc::channel(100); | |
| let state2 = self.state.read().await; | |
| let start_height = state2 | |
| .blocks | |
| .values() | |
| .find(|b| b.hash().to_vec() == start_header_hash) | |
| .map_or(0, |b| b.height); | |
| let end_height = state2 | |
| .blocks | |
| .values() | |
| .find(|b| b.hash().to_vec() == end_header_hash) | |
| .map_or(0, |b| b.height); | |
| let state = self.state.clone(); | |
| tokio::spawn(async move { | |
| let state = state.read().await; | |
| let mut current_height = 0; | |
| let mut blocks = vec![]; | |
| let page_size = 5; | |
| for height in start_height..=end_height { | |
| if shutdown.is_triggered() { | |
| break; | |
| } | |
| if let Some(ub) = state.utxos_by_block.get(&height) { | |
| let block_header = state.blocks.get(&height).cloned(); | |
| if let Some(header) = block_header { | |
| blocks.push(BlockUtxoInfo { | |
| header_hash: header.hash().to_vec(), | |
| height: header.height, | |
| outputs: ub | |
| .utxos | |
| .iter() | |
| .map(|o| models::MinimalUtxoSyncInfo { | |
| output_hash: o.hash().to_vec(), | |
| commitment: o.commitment.as_bytes().to_vec(), | |
| encrypted_data: o.encrypted_data.to_byte_vec(), | |
| sender_offset_public_key: o.sender_offset_public_key.to_vec(), | |
| }) | |
| .collect(), | |
| mined_timestamp: header.timestamp.as_u64(), | |
| }); | |
| current_height = height; | |
| } | |
| } | |
| if blocks.len() >= page_size || height == end_height { | |
| let has_next_page = height < end_height; | |
| let response = SyncUtxosByBlockResponse { | |
| blocks: blocks.clone(), | |
| has_next_page, | |
| }; | |
| blocks.clear(); | |
| if tx.send(Ok(response)).await.is_err() { | |
| break; // Channel closed | |
| } | |
| } | |
| } | |
| }); | |
| Ok(rx) | |
| } | |
| async fn sync_utxos_by_block( | |
| &self, | |
| start_header_hash: Vec<u8>, | |
| end_header_hash: Vec<u8>, | |
| shutdown: ShutdownSignal, | |
| ) -> Result<mpsc::Receiver<Result<SyncUtxosByBlockResponse, Error>>, Error> { | |
| let (tx, rx) = mpsc::channel(100); | |
| let state2 = self.state.read().await; | |
| let start_height = state2 | |
| .blocks | |
| .values() | |
| .find(|b| b.hash().to_vec() == start_header_hash) | |
| - .map_or(0, |b| b.height); | |
| + .ok_or_else(|| Error::msg("start_header_hash not found"))? | |
| + .height; | |
| let end_height = state2 | |
| .blocks | |
| .values() | |
| .find(|b| b.hash().to_vec() == end_header_hash) | |
| - .map_or(0, |b| b.height); | |
| + .ok_or_else(|| Error::msg("end_header_hash not found"))? | |
| + .height; | |
| let state = self.state.clone(); | |
| tokio::spawn(async move { | |
| let state = state.read().await; | |
| let mut current_height = 0; | |
| let mut blocks = vec![]; | |
| let page_size = 5; | |
| for height in start_height..=end_height { | |
| if shutdown.is_triggered() { | |
| break; | |
| } | |
| if let Some(ub) = state.utxos_by_block.get(&height) { | |
| let block_header = state.blocks.get(&height).cloned(); | |
| if let Some(header) = block_header { | |
| blocks.push(BlockUtxoInfo { | |
| header_hash: header.hash().to_vec(), | |
| height: header.height, | |
| outputs: ub | |
| .utxos | |
| .iter() | |
| .map(|o| models::MinimalUtxoSyncInfo { | |
| output_hash: o.hash().to_vec(), | |
| commitment: o.commitment.as_bytes().to_vec(), | |
| encrypted_data: o.encrypted_data.to_byte_vec(), | |
| sender_offset_public_key: o.sender_offset_public_key.to_vec(), | |
| }) | |
| .collect(), | |
| mined_timestamp: header.timestamp.as_u64(), | |
| }); | |
| current_height = height; | |
| } | |
| } | |
| if blocks.len() >= page_size || height == end_height { | |
| let has_next_page = height < end_height; | |
| let response = SyncUtxosByBlockResponse { | |
| blocks: blocks.clone(), | |
| has_next_page, | |
| }; | |
| blocks.clear(); | |
| if tx.send(Ok(response)).await.is_err() { | |
| break; // Channel closed | |
| } | |
| } | |
| } | |
| }); | |
| Ok(rx) | |
| } |
🤖 Prompt for AI Agents
In base_layer/wallet/tests/support/base_node_http_service_mock.rs around lines
131 to 199, the function sync_utxos_by_block defaults start_height and
end_height to 0 when the provided start or end header hashes are not found,
which can hide test setup errors. Modify the code to validate that both
start_height and end_height are found before proceeding; if either is missing,
return an error early instead of defaulting to 0. This ensures that invalid
input is caught promptly and does not cause silent fallback behavior.
| let http_base_node_address: *const c_char = | ||
| CString::new("http://127.0.1:2222").unwrap().into_raw() as *const c_char; |
There was a problem hiding this comment.
Invalid test URL – missing octet
"http://127.0.1:2222" is not a valid dotted-quad IPv4 address and will fail Url::parse, contradicting the positive-path test intent.
- CString::new("http://127.0.1:2222").unwrap().into_raw() as *const c_char;
+ CString::new("http://127.0.0.1:2222").unwrap().into_raw() as *const c_char;📝 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.
| let http_base_node_address: *const c_char = | |
| CString::new("http://127.0.1:2222").unwrap().into_raw() as *const c_char; | |
| let http_base_node_address: *const c_char = | |
| CString::new("http://127.0.0.1:2222").unwrap().into_raw() as *const c_char; |
🤖 Prompt for AI Agents
In base_layer/wallet_ffi/src/lib.rs around lines 13994 to 13995, the test URL
"http://127.0.1:2222" is invalid due to a missing octet in the IPv4 address.
Replace the URL with a valid IPv4 address such as "http://127.0.0.1:2222" to
ensure the URL parses correctly and the positive-path test behaves as intended.
| use log::*; | ||
| use minotari_node_wallet_client::{http, BaseNodeWalletClient}; |
There was a problem hiding this comment.
Remove unused http import to avoid compilation warnings/errors
http is imported but never used in this file. rustc will emit a warning that may be escalated to an error by #![deny(unused_imports)].
-use minotari_node_wallet_client::{http, BaseNodeWalletClient};
+use minotari_node_wallet_client::BaseNodeWalletClient;📝 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.
| use minotari_node_wallet_client::{http, BaseNodeWalletClient}; | |
| -use minotari_node_wallet_client::{http, BaseNodeWalletClient}; | |
| +use minotari_node_wallet_client::BaseNodeWalletClient; |
🤖 Prompt for AI Agents
In base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs at line 31,
remove the unused import `http` from the `use
minotari_node_wallet_client::{http, BaseNodeWalletClient};` statement to prevent
compilation warnings or errors caused by unused imports.
| let current_header_hash = response.header_hash; | ||
| let mined_timestamp = DateTime::<Utc>::from_timestamp(response.mined_timestamp as i64, 0) | ||
| .unwrap_or(DateTime::<Utc>::MIN_UTC); | ||
| let outputs = response.outputs; |
There was a problem hiding this comment.
Compilation failure: misuse of chrono API
DateTime::<Utc>::from_timestamp() returns an Option<DateTime<Utc>>
beginning with chrono 0.4.23. Calling .unwrap_or(...) on the result of
from_timestamp therefore requires an unwrap_or on the Option, but here
it is applied to the DateTime value, which doesn’t compile.
-let mined_timestamp = DateTime::<Utc>::from_timestamp(response.mined_timestamp as i64, 0)
- .unwrap_or(DateTime::<Utc>::MIN_UTC);
+let mined_timestamp = Utc
+ .timestamp_opt(response.mined_timestamp as i64, 0)
+ .single()
+ .unwrap_or(DateTime::<Utc>::MIN_UTC);📝 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.
| let current_header_hash = response.header_hash; | |
| let mined_timestamp = DateTime::<Utc>::from_timestamp(response.mined_timestamp as i64, 0) | |
| .unwrap_or(DateTime::<Utc>::MIN_UTC); | |
| let outputs = response.outputs; | |
| let current_header_hash = response.header_hash; | |
| - let mined_timestamp = DateTime::<Utc>::from_timestamp(response.mined_timestamp as i64, 0) | |
| - .unwrap_or(DateTime::<Utc>::MIN_UTC); | |
| + let mined_timestamp = Utc | |
| + .timestamp_opt(response.mined_timestamp as i64, 0) | |
| + .single() | |
| + .unwrap_or(DateTime::<Utc>::MIN_UTC); | |
| let outputs = response.outputs; |
🤖 Prompt for AI Agents
In base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs around lines
424 to 427, the code incorrectly calls unwrap_or on the result of
DateTime::<Utc>::from_timestamp(), which now returns an Option<DateTime<Utc>>.
To fix this, apply unwrap_or on the Option returned by from_timestamp, providing
DateTime::<Utc>::MIN_UTC as the default value inside unwrap_or to handle the
None case properly.
| let mut height = last_scanned_block.height; | ||
| let mut next_header; | ||
| // Keep going backwards until we find a header that is known to the base node | ||
| loop { | ||
| next_header = wallet_service_client | ||
| .get_header_by_height(last_scanned_block.height + 1) | ||
| .await?; | ||
| if !next_header.is_none() { | ||
| break; | ||
| } | ||
| height = height.saturating_sub(1); | ||
| } | ||
| let next_header = next_header.unwrap(); |
There was a problem hiding this comment.
Infinite-loop risk: height variable is ignored
The intention is to walk backwards until a header is found, but the call
still uses last_scanned_block.height + 1 on every iteration while height
is mutated but never read. If the next header is unknown the loop never
terminates.
- let mut height = last_scanned_block.height;
+ let mut height = last_scanned_block.height;
// Keep going backwards until we find a header that is known to the base node
loop {
- next_header = wallet_service_client
- .get_header_by_height(last_scanned_block.height + 1)
+ next_header = wallet_service_client
+ .get_header_by_height(height + 1)
.await?;
if !next_header.is_none() {
break;
}
- height = height.saturating_sub(1);
+ if height == 0 {
+ return Err(anyhow!(
+ "Could not find a known header—reached genesis without success"
+ ));
+ }
+ height = height.saturating_sub(1);
}📝 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.
| let mut height = last_scanned_block.height; | |
| let mut next_header; | |
| // Keep going backwards until we find a header that is known to the base node | |
| loop { | |
| next_header = wallet_service_client | |
| .get_header_by_height(last_scanned_block.height + 1) | |
| .await?; | |
| if !next_header.is_none() { | |
| break; | |
| } | |
| height = height.saturating_sub(1); | |
| } | |
| let next_header = next_header.unwrap(); | |
| let mut height = last_scanned_block.height; | |
| let mut next_header; | |
| // Keep going backwards until we find a header that is known to the base node | |
| loop { | |
| next_header = wallet_service_client | |
| .get_header_by_height(height + 1) | |
| .await?; | |
| if !next_header.is_none() { | |
| break; | |
| } | |
| if height == 0 { | |
| return Err(anyhow!( | |
| "Could not find a known header—reached genesis without success" | |
| )); | |
| } | |
| height = height.saturating_sub(1); | |
| } | |
| let next_header = next_header.unwrap(); |
🤖 Prompt for AI Agents
In base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs around lines
183 to 195, the loop intended to find a known header walks backwards by
decrementing the height variable, but the call to get_header_by_height always
uses last_scanned_block.height + 1 instead of the updated height. To fix this,
replace last_scanned_block.height + 1 with the mutable height variable in the
get_header_by_height call so the loop correctly queries decreasing heights and
terminates when a known header is found.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
base_layer/wallet_ffi/src/lib.rs (2)
14231-14233: Invalid IPv4 address in test fixture (repeat of earlier feedback)
"http://127.0.1:2222"is still present; the missing octet makes it an invalid dotted-quad andUrl::parsewill fail on the “happy-path” test.This was flagged previously – please update to
127.0.0.1(or another valid host).
6912-6920:⚠️ Potential issue
expectwill abort the host application across the FFI boundaryCalling
.expect()on the UTF-8 conversion is unsafe here; invalid data from C will trigger an unwind/panic that crosses the FFI boundary and aborts the process.- let base_node = CStr::from_ptr(http_base_node) - .to_str() - .expect("A non-null base node should be able to be converted to string") - .to_owned(); - Some(base_node) + match CStr::from_ptr(http_base_node).to_str() { + Ok(s) => Some(s.to_owned()), + Err(_) => { + *error_out = LibWalletError::from( + InterfaceError::PointerError("http_base_node contains invalid UTF-8".to_string()) + ).code; + return ptr::null_mut(); + } + }
🧹 Nitpick comments (1)
base_layer/wallet_ffi/src/lib.rs (1)
10186-10198: Minor: fix error-string casingNit, but the error string says “
Url is not valid”. Elsewhere in the file you changed similar messages to “URL”. Keeping it consistent helps grep-ability.- "Url is not valid: {}", + "URL is not valid: {}",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.toml(2 hunks)applications/minotari_console_wallet/src/automation/commands.rs(4 hunks)applications/minotari_console_wallet/src/init/mod.rs(2 hunks)applications/minotari_console_wallet/src/wallet_modes.rs(1 hunks)base_layer/common_types/Cargo.toml(1 hunks)base_layer/core/Cargo.toml(1 hunks)base_layer/core/src/transactions/transaction_protocol/sender.rs(1 hunks)base_layer/wallet/Cargo.toml(2 hunks)base_layer/wallet/src/output_manager_service/service.rs(1 hunks)base_layer/wallet/src/transaction_service/service.rs(1 hunks)base_layer/wallet/tests/transaction_service_tests/service.rs(4 hunks)base_layer/wallet_ffi/src/lib.rs(37 hunks)base_layer/wallet_ffi/wallet.h(2 hunks)supply-chain/config.toml(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- base_layer/core/Cargo.toml
- base_layer/wallet/Cargo.toml
- base_layer/common_types/Cargo.toml
- base_layer/core/src/transactions/transaction_protocol/sender.rs
- base_layer/wallet/src/transaction_service/service.rs
- Cargo.toml
- base_layer/wallet/src/output_manager_service/service.rs
- applications/minotari_console_wallet/src/wallet_modes.rs
- applications/minotari_console_wallet/src/init/mod.rs
- supply-chain/config.toml
- applications/minotari_console_wallet/src/automation/commands.rs
- base_layer/wallet/tests/transaction_service_tests/service.rs
- base_layer/wallet_ffi/wallet.h
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
🔇 Additional comments (1)
base_layer/wallet_ffi/src/lib.rs (1)
82-87: Possible unused import –WalletConnectivityInterface
connectivity_service::WalletConnectivityInterfacewas added but I can’t find any usage in the added hunks.
If it’s indeed unused the crate will fail to compile withunused importwarnings (denied by#![deny(warnings)]in this crate).Please run
cargo check --all-featuresto confirm whether the import is required and remove it if not.
Co-Authored-By: @ksrichard
Summary by CodeRabbit
New Features
Improvements
Refactor
Chores
utoipa,anyhow, andurl.BREAKING CHANGE: FFI interface has changed. New config setting needed.