DragonX/ObsidianDragon
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 4 Wiki Activity

fix: Tier-2 UX robustness — action guards, in-progress guards, input trimming

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
DanS
2026-07-05 13:09:23 -05:00
parent df14533ad3
commit 129a8e6449
10 changed files with 122 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

61
src/app.cpp
View File

@@ -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...");

15
src/app_network.cpp
View File

@@ -2374,13 +2374,26 @@ void App::exportAllKeys(std::function<void(const std::string&, int, int)> callba
}
}
void App::importPrivateKey(const std::string& key, std::function<void(bool, const std::string&)> callback)
void App::importPrivateKey(const std::string& rawKey, std::function<void(bool, const std::string&)> 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

17
src/services/wallet_security_controller.cpp
View File

@@ -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)

3
src/services/wallet_security_controller.h
View File

@@ -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);

9
src/ui/windows/bootstrap_download_dialog.h
View File

@@ -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();

2
src/ui/windows/console_command_executor.cpp
View File

@@ -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<std::mutex> lk(results_mutex_);
results_.push_back({result_str, is_error});
};

4
src/ui/windows/console_command_executor.h
View File

@@ -12,6 +12,7 @@
#include "console_channel.h"
#include "imgui.h"
#include <atomic>
#include <cstdint>
#include <deque>
#include <functional>
@@ -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<int> 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

8
src/ui/windows/mining_controls.cpp
View File

@@ -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"));
}

15
src/ui/windows/mining_tab.cpp
View File

@@ -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;

16
src/ui/windows/shield_dialog.cpp
View File

@@ -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))) {