backport: feat(wallet): add Mine Blocks dialog for Regtest/Devnet dev mode#638
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a dev-mode "Mine Blocks" feature: UI dialog for selecting a core address and block count, backend CoreTask::MineBlocks to generate blocks and refresh wallet balances, and documentation with 24 manual test scenarios. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Mine Dialog (UI)
participant Backend as AppContext / Backend Task
participant Core as Core Client
participant Wallet as Wallet
User->>UI: Open Mine Dialog
UI->>Wallet: Request core addresses & balances
Wallet-->>UI: Return addresses
User->>UI: Select address & enter block count
UI->>Backend: Dispatch CoreTask::MineBlocks(block_count, address, wallet)
Backend->>Core: generate_to_address(block_count, address)
Core-->>Backend: Return mined blocks
Backend->>Wallet: Refresh balances
Wallet-->>Backend: Balances updated
Backend-->>UI: MineBlocksSuccess(mined_count)
UI->>User: Display success message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4 tasks
The dialog stayed open (in a broken state) after clicking Mine or Cancel because the local `open` variable was written back to `is_open` after the dialog state had already been reset. Pass `is_open` directly to egui's `.open()` and use a separate `close` flag for button-triggered dismissal. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend_task/core/mod.rs (1)
284-306: Movegenerate_to_addressintospawn_blockingto prevent blocking the async runtime.The RPC call is synchronous and will block until all blocks are generated. For large block counts, this blocks a tokio worker thread. The codebase already uses
spawn_blockingfor other blocking RPC operations (wallet refresh, asset lock recovery); this should follow the same pattern.♻️ Proposed adjustment
- let mined = self - .core_client - .read() - .expect("Core client lock was poisoned") - .generate_to_address(block_count, &address) - .map_err(|e| e.to_string())?; + let core_client = self.core_client.clone(); + let address = address.clone(); + let mined = tokio::task::spawn_blocking(move || { + core_client + .read() + .expect("Core client lock was poisoned") + .generate_to_address(block_count, &address) + }) + .await + .map_err(|e| format!("Task join error: {}", e))? + .map_err(|e| e.to_string())?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/core/mod.rs` around lines 284 - 306, The synchronous RPC call generate_to_address inside CoreTask::MineBlocks blocks the async runtime; move the generate_to_address call into a tokio::task::spawn_blocking closure so the blocking RPC runs off the async executor. Inside the closure acquire the core_client read lock, call generate_to_address(block_count, &address), and return the mined result (or its length) and any error; then await the spawn_blocking result and map errors as before. After that, perform the wallet refresh (calling ctx.refresh_wallet_info(wallet)) either in a subsequent spawn_blocking or in the same blocking closure before returning, preserving the existing error mappings and ultimately returning BackendTaskSuccessResult::MineBlocksSuccess(mined_count).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/backend_task/core/mod.rs`:
- Around line 284-306: The synchronous RPC call generate_to_address inside
CoreTask::MineBlocks blocks the async runtime; move the generate_to_address call
into a tokio::task::spawn_blocking closure so the blocking RPC runs off the
async executor. Inside the closure acquire the core_client read lock, call
generate_to_address(block_count, &address), and return the mined result (or its
length) and any error; then await the spawn_blocking result and map errors as
before. After that, perform the wallet refresh (calling
ctx.refresh_wallet_info(wallet)) either in a subsequent spawn_blocking or in the
same blocking closure before returning, preserving the existing error mappings
and ultimately returning
BackendTaskSuccessResult::MineBlocksSuccess(mined_count).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/ai-design/2026-02-24-mine-blocks-dialog/manual-test-scenarios.mdsrc/backend_task/core/mod.rssrc/backend_task/mod.rssrc/ui/wallets/wallets_screen/dialogs.rssrc/ui/wallets/wallets_screen/mod.rs
Contributor
Author
🔍 Audit Report: PR #638 — Mine Blocks DialogReviewed by: Claude Code with a 3-agent team:
PR scope (from GitHub base): 5 files, +701 / -1 lines Findings Summary
✅ = Fixed in follow-up commit Pre-existing / Outside-Diff IssuesThese are in the
Positive Observations
🤖 Reviewed by Claudius the Magnificent AI Agent |
- Wrap `generate_to_address` in `spawn_blocking` to avoid blocking the async runtime thread (HIGH) - Replace `.expect()` on core client lock with `.map_err()?` for graceful error handling instead of panic (HIGH) - Cap block count at 1000 to prevent resource exhaustion on the Core node (HIGH) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/ui/wallets/wallets_screen/dialogs.rs (3)
1374-1377: Error display is inconsistent with the send dialog's styled error frame.The send dialog renders errors inside a colored
Framewith a "Dismiss" button (lines 206–223). The mine dialog just renders a plain colored label. If unifying error presentation across dialogs is desirable, the sameFrame-with-dismiss pattern can be reused here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/wallets_screen/dialogs.rs` around lines 1374 - 1377, The mine dialog currently shows errors as a plain colored label (self.mine_dialog.error); change it to match the send dialog's styled error Frame-with-"Dismiss" button pattern by rendering the error inside the same Frame (using the same color/styling helpers like DashColors::error_color(dark_mode)), include a "Dismiss" button identical to the send dialog's behavior, and wire the button to clear the error (e.g., set self.mine_dialog.error = None or call the same dismiss helper used by the send dialog) so the UI and dismissal behavior are consistent across dialogs.
1236-1241: Dialog opens in an error state when no wallet is selected.When
selected_walletisNone, the dialog opens immediately showing "Select a wallet first". Since the Mine button is gated by dev mode + RPC + Regtest/Devnet network, a wallet should always be selected by the time this dialog is reachable. If the intent is a defensive guard, consider not opening the dialog at all and instead surfacing the message through aMessageBanneror tooltip so the dialog stays closed until a wallet is selected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/wallets_screen/dialogs.rs` around lines 1236 - 1241, The open_mine_dialog currently opens the dialog and sets mine_dialog.error when selected_wallet is None; change this so the dialog is not opened in that case: in open_mine_dialog, if self.selected_wallet is None, do not set mine_dialog.is_open or mine_dialog.error but instead surface the feedback via the existing MessageBanner/tooltip mechanism (or set a banner state like self.message_banner = Some("Select a wallet first")) so the dialog remains closed until a wallet is selected; update references to selected_wallet, mine_dialog.error, and mine_dialog.is_open in open_mine_dialog accordingly.
1253-1293: Extract a shared helper to eliminate near-identical address-loading logic.
load_core_addresses_for_mineis a near-verbatim copy ofload_core_addresses_for_receive(lines 1132–1175). The only differences are which dialog fields receive the results. A helper that returns the loaded addresses can serve both:fn load_bip44_external_addresses( &self, wallet: &Arc<RwLock<Wallet>>, ) -> Result<Vec<(String, u64)>, String> { let wallet_guard = wallet.read().map_err(|e| e.to_string())?; let network = self.app_context.network; let addresses: Vec<(String, u64)> = wallet_guard .watched_addresses .iter() .filter(|(path, _)| path.is_bip44_external(network)) .map(|(_, info)| { let balance = wallet_guard .address_balances .get(&info.address) .copied() .unwrap_or(0); (info.address.to_string(), balance) }) .collect(); Ok(addresses) }Both
load_core_addresses_for_*methods can then call this and handle the "empty → generate new" fallback independently.There is also a minor inconsistency:
load_core_addresses_for_receiveexplicitly callsself.receive_dialog.core_addresses.clear()on the generation-error path (line 1168), butload_core_addresses_for_mineomits the equivalentself.mine_dialog.core_addresses.clear()(line 1286). This is currently harmless sinceopen_mine_dialoginitialises the dialog with..Default::default()before calling this function, but it should be made consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/wallets_screen/dialogs.rs` around lines 1253 - 1293, Extract a shared helper load_bip44_external_addresses(&self, wallet: &Arc<RwLock<Wallet>>) -> Result<Vec<(String, u64)>, String> that acquires wallet.read(), filters watched_addresses by path.is_bip44_external(self.app_context.network), maps to (address.to_string(), balance) using address_balances, and returns the Vec or an Err(String) on lock failure; then update both load_core_addresses_for_mine and load_core_addresses_for_receive to call this helper and keep their individual "if empty -> call generate_new_core_receive_address" fallback logic and dialog assignments (leave generate_new_core_receive_address usage unchanged). Also make the error path consistent by clearing self.mine_dialog.core_addresses on generate_new_core_receive_address Err (matching receive_dialog behavior) and set selected_address_index appropriately. Ensure references to load_core_addresses_for_mine, load_core_addresses_for_receive, load_bip44_external_addresses, generate_new_core_receive_address, mine_dialog.core_addresses, receive_dialog.core_addresses, and mine_dialog.error are used to locate and modify the code.src/backend_task/core/mod.rs (1)
289-310: Add a network guard before dispatchinggenerate_to_address.The backend has no server-side check that the active network is Regtest or Devnet before calling
generate_to_address. The UI enforces this at dispatch time, but if aMineBlockstask is ever constructed on the wrong network, the error surfaces only after the RPC round-trip.generate_to_addressis a regtest-only command; adding an explicit guard here satisfies defense-in-depth and keeps the error message clear.🛡️ Proposed guard
CoreTask::MineBlocks { block_count, address, wallet, } => { + match self.network { + Network::Regtest | Network::Devnet => {} + net => { + return Err(format!( + "generate_to_address is only available on Regtest/Devnet, current network: {}", + net + )); + } + } let ctx = self.clone();
Rename the second
ctxbinding to avoid confusion.The first
ctxis moved into the initialspawn_blockingclosure and consumed; the secondlet ctx = self.clone()at line 303 re-creates it for the wallet-refresh step. The reuse of the same name is correct but can be confusing when reading the diff.♻️ Suggested rename
- let ctx = self.clone(); - tokio::task::spawn_blocking(move || ctx.refresh_wallet_info(wallet)) + let refresh_ctx = self.clone(); + tokio::task::spawn_blocking(move || refresh_ctx.refresh_wallet_info(wallet))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/core/mod.rs` around lines 289 - 310, Before calling generate_to_address, add an explicit guard that verifies the node is on Regtest (or Devnet) and return a clear error if not; perform this check using the same context used to access the RPC (e.g., via self or self.core_client.read()'s network/state) so the regtest-only command is never dispatched on a public network. After adding the network check, proceed to call generate_to_address only when the guard passes. Also rename the second let ctx = self.clone() (used before refresh_wallet_info) to a distinct name like ctx_refresh to avoid shadowing/confusion with the earlier ctx moved into the first spawn_blocking closure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/wallets/wallets_screen/dialogs.rs`:
- Around line 1436-1443: Replace the unsafe assume_checked() call with
require_network(...) to enforce the address network; specifically, when parsing
addr_str via addr_str.parse::<Address<NetworkUnchecked>>(), call
require_network(self.network) (or the appropriate Network value held by the
dialog/state) instead of assume_checked(), and if require_network returns an
Err, set self.mine_dialog.error = Some(format!("Invalid address: {}", err)) and
return — this makes the network invariant explicit and surfaces a clear error on
mismatch rather than silently accepting any network.
---
Nitpick comments:
In `@src/backend_task/core/mod.rs`:
- Around line 289-310: Before calling generate_to_address, add an explicit guard
that verifies the node is on Regtest (or Devnet) and return a clear error if
not; perform this check using the same context used to access the RPC (e.g., via
self or self.core_client.read()'s network/state) so the regtest-only command is
never dispatched on a public network. After adding the network check, proceed to
call generate_to_address only when the guard passes. Also rename the second let
ctx = self.clone() (used before refresh_wallet_info) to a distinct name like
ctx_refresh to avoid shadowing/confusion with the earlier ctx moved into the
first spawn_blocking closure.
In `@src/ui/wallets/wallets_screen/dialogs.rs`:
- Around line 1374-1377: The mine dialog currently shows errors as a plain
colored label (self.mine_dialog.error); change it to match the send dialog's
styled error Frame-with-"Dismiss" button pattern by rendering the error inside
the same Frame (using the same color/styling helpers like
DashColors::error_color(dark_mode)), include a "Dismiss" button identical to the
send dialog's behavior, and wire the button to clear the error (e.g., set
self.mine_dialog.error = None or call the same dismiss helper used by the send
dialog) so the UI and dismissal behavior are consistent across dialogs.
- Around line 1236-1241: The open_mine_dialog currently opens the dialog and
sets mine_dialog.error when selected_wallet is None; change this so the dialog
is not opened in that case: in open_mine_dialog, if self.selected_wallet is
None, do not set mine_dialog.is_open or mine_dialog.error but instead surface
the feedback via the existing MessageBanner/tooltip mechanism (or set a banner
state like self.message_banner = Some("Select a wallet first")) so the dialog
remains closed until a wallet is selected; update references to selected_wallet,
mine_dialog.error, and mine_dialog.is_open in open_mine_dialog accordingly.
- Around line 1253-1293: Extract a shared helper
load_bip44_external_addresses(&self, wallet: &Arc<RwLock<Wallet>>) ->
Result<Vec<(String, u64)>, String> that acquires wallet.read(), filters
watched_addresses by path.is_bip44_external(self.app_context.network), maps to
(address.to_string(), balance) using address_balances, and returns the Vec or an
Err(String) on lock failure; then update both load_core_addresses_for_mine and
load_core_addresses_for_receive to call this helper and keep their individual
"if empty -> call generate_new_core_receive_address" fallback logic and dialog
assignments (leave generate_new_core_receive_address usage unchanged). Also make
the error path consistent by clearing self.mine_dialog.core_addresses on
generate_new_core_receive_address Err (matching receive_dialog behavior) and set
selected_address_index appropriately. Ensure references to
load_core_addresses_for_mine, load_core_addresses_for_receive,
load_bip44_external_addresses, generate_new_core_receive_address,
mine_dialog.core_addresses, receive_dialog.core_addresses, and mine_dialog.error
are used to locate and modify the code.
- Replace assume_checked() with require_network() for address validation (CodeRabbit #2) - Use styled Frame-with-dismiss error display matching Send dialog pattern (CodeRabbit #3) - Don't open dialog when no wallet selected; show MessageBanner instead (CodeRabbit #4) - Extract shared load_bip44_external_addresses() helper to eliminate near-duplicate code between mine and receive dialogs (CodeRabbit #5) - Add backend-side network guard (Regtest/Devnet) for defense-in-depth (CodeRabbit #6) - Rename shadowed ctx binding to refresh_ctx for clarity (CodeRabbit #7) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
commented
Feb 24, 2026
- Change MineBlocksSuccess(usize) to MineBlocksSuccess(u64) for type consistency with block_count parameter (Claudius #5) - Align dialog close pattern with Send/Receive: use local `open` variable for egui X button, reset state inside closure for Cancel/Mine buttons (Claudius #6) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CoreTask::MineBlocksvariant callsgenerate_to_address()and refreshes wallet balancesTest plan
Manual test scenarios:
docs/ai-design/2026-02-24-mine-blocks-dialog/manual-test-scenarios.md🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Tests