From 129a8e644978a9337ecd58a80f9e904fe937858c Mon Sep 17 00:00:00 2001 From: DanS Date: Sun, 5 Jul 2026 13:09:23 -0500 Subject: [PATCH] =?UTF-8?q?fix:=20Tier-2=20UX=20robustness=20=E2=80=94=20a?= =?UTF-8?q?ction=20guards,=20in-progress=20guards,=20input=20trimming?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The app-wide "import-key pattern" fixes (missing guards / untrimmed input / no re-entrancy protection), from the robustness audit: - Import-key dialog: unify the type indicator and the RPC dispatch on one classifier (classifyPrivateKey is now prefix-aware — an "SK..." shielded key no longer misroutes to the transparent RPC), trim manual input (not just Paste), and gate both the indicator and the Import button on a shared isRecognizedPrivateKey() so unrecognized/empty input can't be submitted. - Shield: guard the submit button on connected + !syncing (like Send), with a disabled tooltip, instead of firing into a raw daemon error. - Mining: disable the pool Mine button when the payout address is empty ("enter a payout address first"), and trim the pool URL/worker on persist. - Console: track in-flight RPC commands (atomic counter + busy() override) so the input is disabled while a command runs instead of piling up on the worker. - Maintenance (rescan/repair/delete-chain/reinstall-daemon): re-entrancy guard — bail with "already in progress" instead of launching a duplicate destructive op. - Bootstrap dialog: re-attach to an in-flight download on reopen instead of resetting state and orphaning the running worker. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/app.cpp | 61 +++++++++++++++------ src/app_network.cpp | 15 ++++- src/services/wallet_security_controller.cpp | 17 +++++- src/services/wallet_security_controller.h | 3 + src/ui/windows/bootstrap_download_dialog.h | 9 ++- src/ui/windows/console_command_executor.cpp | 2 + src/ui/windows/console_command_executor.h | 4 ++ src/ui/windows/mining_controls.cpp | 8 ++- src/ui/windows/mining_tab.cpp | 15 ++++- src/ui/windows/shield_dialog.cpp | 16 ++++-- 10 files changed, 122 insertions(+), 28 deletions(-) diff --git a/src/app.cpp b/src/app.cpp index beb9697..254121a 100644 --- a/src/app.cpp +++ b/src/app.cpp @@ -2650,20 +2650,24 @@ void App::renderImportKeyDialog() memset(import_key_input_, 0, sizeof(import_key_input_)); } + // Trimmed view for validation (importPrivateKey trims again before the RPC). The indicator and the + // Import-button guard below both derive from the SAME shared classifier, so they can't disagree. + std::string keyTrim(import_key_input_); + while (!keyTrim.empty() && (keyTrim.front()==' '||keyTrim.front()=='\t'||keyTrim.front()=='\n'||keyTrim.front()=='\r')) keyTrim.erase(keyTrim.begin()); + while (!keyTrim.empty() && (keyTrim.back()==' '||keyTrim.back()=='\t'||keyTrim.back()=='\n'||keyTrim.back()=='\r')) keyTrim.pop_back(); + bool keyRecognized = services::WalletSecurityController::isRecognizedPrivateKey(keyTrim); + bool keyIsZ = services::WalletSecurityController::classifyPrivateKey(keyTrim) + == services::WalletSecurityController::KeyKind::Shielded; + // Key validation indicator - if (import_key_input_[0] != '\0') { - std::string k(import_key_input_); - bool isZKey = (k.substr(0, 20) == "secret-extended-key-") || - (k.length() >= 2 && k[0] == 'S' && k[1] == 'K'); - bool isTKey = (k.length() >= 51 && k.length() <= 52 && - (k[0] == '5' || k[0] == 'K' || k[0] == 'L' || k[0] == 'U')); - if (isZKey || isTKey) { + if (!keyTrim.empty()) { + if (keyRecognized) { ImGui::PushFont(ui::material::Type().iconSmall()); ImGui::TextColored(ImVec4(0.3f, 0.8f, 0.3f, 1.0f), ICON_MD_CHECK_CIRCLE); ImGui::PopFont(); ImGui::SameLine(0, 4.0f); ImGui::TextColored(ImVec4(0.3f, 0.8f, 0.3f, 1.0f), "%s", - isZKey ? "Shielded spending key" : "Transparent private key"); + keyIsZ ? "Shielded spending key" : "Transparent private key"); } else { ImGui::PushFont(ui::material::Type().iconSmall()); ImGui::TextColored(ImVec4(0.8f, 0.6f, 0.0f, 1.0f), ICON_MD_HELP); @@ -2686,18 +2690,17 @@ void App::renderImportKeyDialog() ImGui::Spacing(); ImGui::Separator(); + ImGui::BeginDisabled(!keyRecognized); // only a recognized Z/T key can be imported if (ui::material::StyledButton("Import", ImVec2(btnW, 0), ui::material::resolveButtonFont(btnFont))) { - std::string key(import_key_input_); - if (!key.empty()) { - importPrivateKey(key, [this](bool success, const std::string& msg) { - import_success_ = success; - import_status_ = msg; - if (success) { - memset(import_key_input_, 0, sizeof(import_key_input_)); - } - }); - } + importPrivateKey(std::string(import_key_input_), [this](bool success, const std::string& msg) { + import_success_ = success; + import_status_ = msg; + if (success) { + memset(import_key_input_, 0, sizeof(import_key_input_)); + } + }); } + ImGui::EndDisabled(); ImGui::SameLine(); if (ui::material::StyledButton("Close", ImVec2(btnW, 0), ui::material::resolveButtonFont(btnFont))) { show_import_key_ = false; @@ -3187,6 +3190,13 @@ void App::rescanBlockchain() return; } + // Re-entrancy guard: a rescan/repair (both drive state_.sync.rescanning) or this exact task already + // running would stomp each other — a second confirm must not launch a duplicate operation. + if (state_.sync.rescanning || async_tasks_.isRunning(decision.taskName)) { + ui::Notifications::instance().warning("A blockchain maintenance operation is already in progress."); + return; + } + DEBUG_LOGF("[App] Starting blockchain rescan - stopping daemon first\n"); ui::Notifications::instance().info("Restarting daemon with -rescan flag..."); @@ -3229,6 +3239,11 @@ void App::repairWallet() return; } + if (state_.sync.rescanning || async_tasks_.isRunning(decision.taskName)) { + ui::Notifications::instance().warning("A blockchain maintenance operation is already in progress."); + return; + } + DEBUG_LOGF("[App] Starting wallet repair (-zapwallettxes=2) - stopping daemon first\n"); ui::Notifications::instance().info("Restarting daemon with -zapwallettxes=2 (wallet repair)..."); @@ -3270,6 +3285,11 @@ void App::reinstallBundledDaemon() return; } + if (async_tasks_.isRunning("reinstall-daemon")) { + ui::Notifications::instance().warning("The daemon reinstall is already in progress."); + return; + } + DEBUG_LOGF("[App] Reinstalling bundled daemon binary — stopping daemon first\n"); ui::Notifications::instance().info("Installing bundled daemon — the node will stop, update, and restart..."); @@ -3309,6 +3329,11 @@ void App::deleteBlockchainData() return; } + if (state_.sync.rescanning || async_tasks_.isRunning(decision.taskName)) { + ui::Notifications::instance().warning("A blockchain maintenance operation is already in progress."); + return; + } + DEBUG_LOGF("[App] Deleting blockchain data - stopping daemon first\n"); ui::Notifications::instance().info("Stopping daemon and deleting blockchain data..."); diff --git a/src/app_network.cpp b/src/app_network.cpp index 6e5e0b3..d41f635 100644 --- a/src/app_network.cpp +++ b/src/app_network.cpp @@ -2374,13 +2374,26 @@ void App::exportAllKeys(std::function callba } } -void App::importPrivateKey(const std::string& key, std::function callback) +void App::importPrivateKey(const std::string& rawKey, std::function callback) { if (!state_.connected || !rpc_ || !worker_) { if (callback) callback(false, "Not connected"); return; } + // Trim whitespace/newlines — manual entry doesn't go through the dialog's Paste trimmer, and a + // stray character makes the daemon reject the key with a cryptic error. + std::string key(rawKey); + while (!key.empty() && (key.front()==' '||key.front()=='\t'||key.front()=='\n'||key.front()=='\r')) key.erase(key.begin()); + while (!key.empty() && (key.back()==' '||key.back()=='\t'||key.back()=='\n'||key.back()=='\r')) key.pop_back(); + + // Reject anything that doesn't look like a Z/T private key before handing it to the daemon (the + // dialog's indicator and this guard now share isRecognizedPrivateKey, so they can't disagree). + if (!services::WalletSecurityController::isRecognizedPrivateKey(key)) { + if (callback) callback(false, "Unrecognized private-key format."); + return; + } + const bool shielded = services::WalletSecurityController::classifyPrivateKey(key) == services::WalletSecurityController::KeyKind::Shielded; // Run on the worker thread — import requests a full rescan (rescan=true), so the diff --git a/src/services/wallet_security_controller.cpp b/src/services/wallet_security_controller.cpp index 368a441..2dec9b1 100644 --- a/src/services/wallet_security_controller.cpp +++ b/src/services/wallet_security_controller.cpp @@ -97,7 +97,22 @@ WalletSecurityController::KeyKind WalletSecurityController::classifyAddress(cons WalletSecurityController::KeyKind WalletSecurityController::classifyPrivateKey(const std::string& key) { - return !key.empty() && key[0] == 's' ? KeyKind::Shielded : KeyKind::Transparent; + // Shielded: Sapling extended spending key (secret-extended-key-...) or legacy Sprout (SK...). + // (The old `key[0]=='s'` test misrouted an uppercase "SK..." shielded key to the transparent RPC.) + if (key.rfind("secret-extended-key-", 0) == 0) return KeyKind::Shielded; + if (key.size() >= 2 && key[0] == 'S' && key[1] == 'K') return KeyKind::Shielded; + if (!key.empty() && key[0] == 's') return KeyKind::Shielded; + return KeyKind::Transparent; +} + +bool WalletSecurityController::isRecognizedPrivateKey(const std::string& key) +{ + if (key.rfind("secret-extended-key-", 0) == 0) return true; // Sapling z spending key + if (key.size() >= 2 && key[0] == 'S' && key[1] == 'K') return true; // Sprout z spending key + // Transparent WIF: base58, ~51-52 chars, common version prefixes. + if (key.size() >= 51 && key.size() <= 52 && + (key[0] == '5' || key[0] == 'K' || key[0] == 'L' || key[0] == 'U')) return true; + return false; } const char* WalletSecurityController::importSuccessMessage(KeyKind kind) diff --git a/src/services/wallet_security_controller.h b/src/services/wallet_security_controller.h index 08be111..f300434 100644 --- a/src/services/wallet_security_controller.h +++ b/src/services/wallet_security_controller.h @@ -74,6 +74,9 @@ public: std::size_t minLength = 4); static KeyKind classifyAddress(const std::string& address); static KeyKind classifyPrivateKey(const std::string& key); + // True if `key` looks like a recognized Z (Sapling/Sprout spending) or T (WIF) private key. + // Single source of truth for the import dialog's indicator AND its submit guard. + static bool isRecognizedPrivateKey(const std::string& key); static const char* importSuccessMessage(KeyKind kind); static std::string decryptExportFileName(std::uint64_t timestampSeconds); static void secureClear(std::string& value); diff --git a/src/ui/windows/bootstrap_download_dialog.h b/src/ui/windows/bootstrap_download_dialog.h index e2bab61..d1d4a8c 100644 --- a/src/ui/windows/bootstrap_download_dialog.h +++ b/src/ui/windows/bootstrap_download_dialog.h @@ -30,8 +30,15 @@ class BootstrapDownloadDialog { public: static void show(App* app) { if (!app || !app->supportsFullNodeLifecycleActions()) return; - s_open = true; s_app = app; + // If a download is already in flight, RE-ATTACH to it (just reopen showing progress) instead of + // resetting state and orphaning the running worker / restarting an unverified download. + if (app->isBootstrapDownloading()) { + s_open = true; + if (s_state == State::Confirm) s_state = State::Downloading; + return; + } + s_open = true; s_state = State::Confirm; s_bootstrap.reset(); s_errorMsg.clear(); diff --git a/src/ui/windows/console_command_executor.cpp b/src/ui/windows/console_command_executor.cpp index 11249d5..47238ec 100644 --- a/src/ui/windows/console_command_executor.cpp +++ b/src/ui/windows/console_command_executor.cpp @@ -54,6 +54,7 @@ void FullNodeConsoleExecutor::submit(const std::string& cmd) rpc::RPCWorker* worker = app_->consoleWorker(); if (worker) { + in_flight_.fetch_add(1); // gates busy() so the input is disabled until this returns worker->post([rpc, method, params, this]() -> rpc::RPCWorker::MainCb { std::string result_str; bool is_error = false; @@ -65,6 +66,7 @@ void FullNodeConsoleExecutor::submit(const std::string& cmd) is_error = true; } return [this, result_str, is_error]() { + in_flight_.fetch_sub(1); std::lock_guard lk(results_mutex_); results_.push_back({result_str, is_error}); }; diff --git a/src/ui/windows/console_command_executor.h b/src/ui/windows/console_command_executor.h index fd59f69..82fbace 100644 --- a/src/ui/windows/console_command_executor.h +++ b/src/ui/windows/console_command_executor.h @@ -12,6 +12,7 @@ #include "console_channel.h" #include "imgui.h" +#include #include #include #include @@ -91,9 +92,12 @@ public: ConsoleLogFilterCaps logFilterCaps() const override { return {true, true, true, true}; } void printHelp(const ConsoleAddLineFn& add) override; ConsoleStatusLine toolbarStatus() const override; + // A command is in flight on the RPC worker — lets the UI disable the input (no queued pile-up). + bool busy() const override { return in_flight_.load() > 0; } private: App* app_; + std::atomic in_flight_{0}; size_t last_daemon_output_size_ = 0; size_t last_xmrig_output_size_ = 0; int last_daemon_state_ = -1; // daemon::EmbeddedDaemon::State as int diff --git a/src/ui/windows/mining_controls.cpp b/src/ui/windows/mining_controls.cpp index 92846c6..877e1eb 100644 --- a/src/ui/windows/mining_controls.cpp +++ b/src/ui/windows/mining_controls.cpp @@ -690,8 +690,12 @@ void RenderMiningControls(App* app, const WalletState& state, const MiningInfo& // If pool mining is still shutting down after switching to solo, // keep the button enabled so user can stop it. bool poolStillRunning = !s_pool_mode && state.pool_mining.xmrig_running; + // Can't start pool mining without a payout address (blank for a new wallet with no z-address); + // only blocks starting — stopping a running miner stays enabled. + bool poolNeedsPayout = s_pool_mode && !state.pool_mining.xmrig_running && + std::string(s_pool_worker).empty(); bool disabled = s_pool_mode - ? (isToggling || poolBlockedBySolo) + ? (isToggling || poolBlockedBySolo || poolNeedsPayout) : (poolStillRunning ? false : (!app->isConnected() || isToggling || isSyncing)); // Glass panel background with state-dependent tint @@ -821,6 +825,8 @@ void RenderMiningControls(App* app, const WalletState& state, const MiningInfo& material::Tooltip(TR("mining_syncing_tooltip"), state.sync.verification_progress * 100.0); else if (poolBlockedBySolo) material::Tooltip("%s", TR("mining_stop_solo_for_pool")); + else if (poolNeedsPayout) + material::Tooltip("%s", "Enter a payout address first (generate a Z address)"); else material::Tooltip("%s", isMiningActive ? TR("stop_mining") : TR("start_mining")); } diff --git a/src/ui/windows/mining_tab.cpp b/src/ui/windows/mining_tab.cpp index afe480a..b7296bc 100644 --- a/src/ui/windows/mining_tab.cpp +++ b/src/ui/windows/mining_tab.cpp @@ -199,8 +199,19 @@ static void RenderMiningTabContent(App* app) // Persist pool settings when dirty and no field is active if (s_pool_settings_dirty && !ImGui::IsAnyItemActive()) { - app->settings()->setPoolUrl(s_pool_url); - app->settings()->setPoolWorker(s_pool_worker); + // Trim whitespace/newlines (a pasted URL or payout address often carries a trailing newline) + // before persisting and feeding it to xmrig; write the trimmed value back so the field agrees. + auto trimmed = [](const char* b) { + std::string s(b); + while (!s.empty() && (s.front()==' '||s.front()=='\t'||s.front()=='\n'||s.front()=='\r')) s.erase(s.begin()); + while (!s.empty() && (s.back()==' '||s.back()=='\t'||s.back()=='\n'||s.back()=='\r')) s.pop_back(); + return s; + }; + std::string poolUrl = trimmed(s_pool_url), poolWorker = trimmed(s_pool_worker); + snprintf(s_pool_url, sizeof(s_pool_url), "%s", poolUrl.c_str()); + snprintf(s_pool_worker, sizeof(s_pool_worker), "%s", poolWorker.c_str()); + app->settings()->setPoolUrl(poolUrl); + app->settings()->setPoolWorker(poolWorker); app->settings()->save(); s_pool_settings_dirty = false; diff --git a/src/ui/windows/shield_dialog.cpp b/src/ui/windows/shield_dialog.cpp index d351a18..6a867d7 100644 --- a/src/ui/windows/shield_dialog.cpp +++ b/src/ui/windows/shield_dialog.cpp @@ -166,9 +166,12 @@ void ShieldDialog::render(App* app) ImGui::Spacing(); } - // Buttons - bool can_submit = !s_operation_pending && s_to_address[0] != '\0'; - + // Buttons — guard on connection/sync like the Send tab (a disconnected or mid-sync submit just + // fails at the daemon with a raw error). + bool sh_connected = app->isConnected(); + bool sh_syncing = state.sync.syncing; + bool can_submit = !s_operation_pending && s_to_address[0] != '\0' && sh_connected && !sh_syncing; + if (!can_submit) ImGui::BeginDisabled(); const char* btn_label = (s_mode == Mode::ShieldCoinbase) ? TR("shield_funds") : TR("merge_funds"); @@ -244,7 +247,12 @@ void ShieldDialog::render(App* app) } if (!can_submit) ImGui::EndDisabled(); - + if (!can_submit && ImGui::IsItemHovered(ImGuiHoveredFlags_AllowWhenDisabled)) { + if (!sh_connected) material::Tooltip("%s", TR("send_tooltip_not_connected")); + else if (sh_syncing) material::Tooltip("%s", TR("send_tooltip_syncing")); + else if (s_to_address[0]=='\0') material::Tooltip("%s", TR("shield_select_z")); + } + ImGui::SameLine(); if (material::StyledButton(TR("cancel"), ImVec2(cancelBtn.width, 0), S.resolveFont(cancelBtn.font))) {