fix: fix scanner service when connectivity offline#7223
fix: fix scanner service when connectivity offline#7223SWvheerden merged 2 commits intotari-project:developmentfrom
Conversation
WalkthroughThis update refactors several wallet-related modules. It removes an unnecessary Changes
Sequence Diagram(s)sequenceDiagram
participant UtxoScannerService
participant CommsConnectivity
UtxoScannerService->>CommsConnectivity: get_connectivity_status().await
CommsConnectivity-->>UtxoScannerService: ConnectivityStatus (Online/Offline)
alt If Offline
UtxoScannerService->>UtxoScannerService: Log "Waiting for connectivity"
UtxoScannerService->>UtxoScannerService: Sleep 5 seconds
UtxoScannerService->>CommsConnectivity: Retry get_connectivity_status().await
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
base_layer/wallet/src/utxo_scanner_service/service.rs (1)
160-161: Polling-sleep loop could be replaced by connectivity eventsA fixed 5 s sleep introduces unnecessary latency once the node comes back online.
Consider subscribing to the connectivity event stream (ConnectivityEvent::ConnectivityStateOnline) and usingtokio::select!so the scanner is resumed immediately.base_layer/wallet/tests/utxo_scanner/mod.rs (2)
142-144: Potentially redundant connectivity status update
add_active_connection(..)already bumps the mock into an online state.
The explicitset_connectivity_status(ConnectivityStatus::Online(1))immediately after it is usually unnecessary and could diverge from the real implementation if the behaviour ofadd_active_connectionchanges.- comms_connectivity_mock_state - .set_connectivity_status(ConnectivityStatus::Online(1)) - .await; + // Probably not needed – `add_active_connection` sets the state to `Online`.
307-308: Remove commented-out logger initialisationThe commented
env_loggerlines add noise and are unlikely to be re-enabled once committed.
If verbose logging is occasionally useful, prefer gating it behind an env-flag (e.g.RUST_LOG) rather than leaving commented code around.Also applies to: 864-865
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (3)
92-95: Blocking DB write performed on the async executor
set_recovery_mode()is now synchronous, but it still performs a write against the wallet DB.
Invoking it directly from the async context can block the reactor thread for slow back-ends (e.g. SQLite on a busy FS).- if self.mode == UtxoScannerMode::Recovery { - self.set_recovery_mode()?; - } + if self.mode == UtxoScannerMode::Recovery { + // Off-load potential blocking I/O + tokio::task::spawn_blocking({ + let this = self.clone(); // or Arc-clone the DB handle + move || this.set_recovery_mode() + }) + .await??; + }
291-294: Repeated DB look-ups inside tight scanning loop
check_recovery_mode()hits the DB every iteration while scanning blocks.
During large syncs this executes thousands of times per minute and needlessly increases contention.Cache the result for the current run or throttle the check (e.g. once every N blocks).
703-721: Public interface now synchronous – clarify intent & naming
set_recovery_mode,check_recovery_mode,clear_recovery_modeare now sync, but they still interact with the DB.
Consider:
- Renaming
check_recovery_mode→is_recovery_in_progressto reflect the boolean return.- Documenting that the methods are expected to be cheap / non-blocking, or wrap DB work in
spawn_blockingas noted above.No functional issue, but tightening semantics helps future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(1 hunks)base_layer/wallet/src/utxo_scanner_service/service.rs(1 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(4 hunks)base_layer/wallet/tests/utxo_scanner/mod.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: file licenses
- GitHub Check: ci
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (1)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
1235-1236: Removed.clone()is correct and preferable
TransactionStatusisCopy, so using it directly avoids an unnecessary heap copy and simplifies the expression without altering semantics.
Test Results (CI) 3 files 126 suites 39m 44s ⏱️ Results for commit de7e226. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests) 2 files 10 suites 2h 20m 59s ⏱️ For more details on these failures, see this check. Results for commit de7e226. |
* development: chore: new release v4.5.0-pre.0 (tari-project#7228) chore: better logging (tari-project#7226) feat!: ensure payref persists during recovery (tari-project#7225) chore(ci): split out amd64 and arm64 docker builds into native runners (tari-project#7206) fix: fix scanner service when connectivity offline (tari-project#7223) feat: add payref to grpc outputs (tari-project#7216)
2 participants
Description
Fixed the scanner service when connectivity is offline - the connectivity status will now be queried to start scanning, instead of relying on a based node change to initiate the process. The logical error was introduced in a recent PR.
Motivation and Context
Unit test
async fn test_utxo_scanner_one_sided_payments()failed due to logic implementation error.How Has This Been Tested?
Unit test
async fn test_utxo_scanner_one_sided_payments()pass.What process can a PR reviewer use to test or verify this change?
Code review.
Breaking Changes
Summary by CodeRabbit
Refactor
Tests
No changes to user-facing features or functionality.