From 81f803948c3c1675ef5a5d7f2da0704126e3c884 Mon Sep 17 00:00:00 2001 From: DanS Date: Thu, 27 Aug 2026 15:55:00 -0500 Subject: [PATCH] =?UTF-8?q?hygiene:=20Phase=206=20(partial)=20=E2=80=94=20?= =?UTF-8?q?fix=207=20verified=20bugs=20+=20quick-wins=20+=20dedup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the Phase-6 structural scoping: land the verified behavioral bugs and the safe quick-wins/dedups now; the large refactors (monolith splits, ~180 header globals) and consensus-adjacent items stay deferred. Built clean; self-mined; verifychain=true. BUGS (verified by reading the code): - net.cpp CNode::Ban: a braceless `if (subNet.Match(...))` left `pnode->fDisconnect = true;` OUTSIDE the guard, so banning any one subnet marked EVERY connected peer for disconnect (dropped the whole peer set). Wrapped the two statements in braces. (LIVE, high severity.) - rpcdump.cpp importwallet: the `!fGood -> throw "Error adding some keys"` check was trapped inside the `if (fRescan)` block, so importwallet with rescan=false silently reported success when key import failed. Hoisted the check before the rescan branch and cleaned the garbled braces/indentation. - wallet.cpp CommitTransaction: ignored AddToWallet()'s return, so a failed disk-persist of a just-signed spend was swallowed while the tx broadcast. Now logs a hard error on failure. - hush_nSPV_fullnode.h: the UTXOS branch declared `uint8_t filter` while the twin TXIDS branch uses `uint32_t filter`; dragon_rwnum switches on sizeof(filter), so the utxos path parsed only 1 of 4 wire filter bytes. Widened to uint32_t. - asyncrpcoperation_sweep.cpp: LogPrintf("%s ... %s", one-arg) read a missing vararg; added the __func__ argument. - rpcdump.cpp importprivkey: inner `auto secret_key` shadowed the outer uint8_t and changed the type into DecodeCustomSecret; dropped the shadow. - rpcdump.cpp getrescaninfo: char[8] + sprintf("%.4f") overflows when the ratio >= 10.0 (transient reorg); widened to char[16] + snprintf. QUICK WINS: removed the duplicate DRAGON_MAXSCRIPTSIZE #define; pinned the dead HUSH3-branch NOTARISATION_SCAN_LIMIT_BLOCKS to 1440; fixed init typos (fRequestShutdown, RPC warmup). DEDUP: extracted the 19-line try/catch error-mapping block — copy-pasted identically into all six async operations — into AsyncRPCOperation::set_error_from_current_exception(), so the mapping is edited in one place. Behavior-identical (verified all six blocks were byte- identical first). Deferred (endorsed by the scoping, better as their own PRs): addrman Select_ dedup, the pow.cpp powLimit helper (consensus file), the miner CreateNewBlock lock-asymmetry, the wallet monolith splits, and the ~180-global / consensus- retarget / Komodo-heritage work. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/asyncrpcoperation.cpp | 26 ++++++++++++ src/asyncrpcoperation.h | 5 +++ src/crosschain.cpp | 4 +- src/hush_defs.h | 2 +- src/hush_nSPV_fullnode.h | 2 +- src/init.cpp | 4 +- src/net.cpp | 3 +- .../asyncrpcoperation_autoshieldcoinbase.cpp | 17 +------- .../asyncrpcoperation_mergetoaddress.cpp | 17 +------- ...asyncrpcoperation_saplingconsolidation.cpp | 17 +------- src/wallet/asyncrpcoperation_sendmany.cpp | 17 +------- .../asyncrpcoperation_shieldcoinbase.cpp | 17 +------- src/wallet/asyncrpcoperation_sweep.cpp | 19 +-------- src/wallet/rpcdump.cpp | 40 ++++++++++--------- src/wallet/wallet.cpp | 3 +- 15 files changed, 70 insertions(+), 123 deletions(-) diff --git a/src/asyncrpcoperation.cpp b/src/asyncrpcoperation.cpp index c0f5f4c0a..0579ca1f6 100644 --- a/src/asyncrpcoperation.cpp +++ b/src/asyncrpcoperation.cpp @@ -18,6 +18,7 @@ ******************************************************************************/ #include "asyncrpcoperation.h" +#include #include #include @@ -58,6 +59,31 @@ AsyncRPCOperation::AsyncRPCOperation(const AsyncRPCOperation& o) : { } +// Shared error mapping for every async op's main(): rethrow the in-flight +// exception and translate it to this operation's error code/message. Keeping +// it here means the mapping is edited in one place, not copy-pasted into six. +void AsyncRPCOperation::set_error_from_current_exception() +{ + try { + throw; + } catch (const UniValue& objError) { + set_error_code(find_value(objError, "code").get_int()); + set_error_message(find_value(objError, "message").get_str()); + } catch (const runtime_error& e) { + set_error_code(-1); + set_error_message("runtime error: " + string(e.what())); + } catch (const logic_error& e) { + set_error_code(-1); + set_error_message("logic error: " + string(e.what())); + } catch (const exception& e) { + set_error_code(-1); + set_error_message("general exception: " + string(e.what())); + } catch (...) { + set_error_code(-2); + set_error_message("unknown error"); + } +} + AsyncRPCOperation& AsyncRPCOperation::operator=( const AsyncRPCOperation& other ) { this->id_ = other.id_; this->creation_time_ = other.creation_time_; diff --git a/src/asyncrpcoperation.h b/src/asyncrpcoperation.h index 8bef0ec08..f3a5dbf10 100644 --- a/src/asyncrpcoperation.h +++ b/src/asyncrpcoperation.h @@ -147,6 +147,11 @@ protected: std::lock_guard guard(lock_); this->error_message_ = errorMessage; } + + // Map the in-flight (rethrown) exception to error_code_/error_message_. Called from + // every async op's main() catch(...) so the UniValue/runtime/logic/exception mapping + // lives in one place instead of being copy-pasted into all six operations. + void set_error_from_current_exception(); void set_result(UniValue v) { std::lock_guard guard(lock_); diff --git a/src/crosschain.cpp b/src/crosschain.cpp index 4acb91f1a..2ac298e99 100644 --- a/src/crosschain.cpp +++ b/src/crosschain.cpp @@ -41,7 +41,9 @@ // XXX: There are potential crashes wherever we access chainActive without a lock, // because it might be disconnecting blocks at the same time. // TODO: this assumes a blocktime of 75 seconds for HUSH and 60 seconds for other chains -int NOTARISATION_SCAN_LIMIT_BLOCKS = strncmp(SMART_CHAIN_SYMBOL, "HUSH3",5) == 0 ? 1152 : 1440; +// DragonX: the HUSH3 (1152) branch is dead — SMART_CHAIN_SYMBOL is always "DRAGONX", and at +// static-init time it is empty, so this already always resolved to 1440. Pinned to 1440. +int NOTARISATION_SCAN_LIMIT_BLOCKS = 1440; CBlockIndex *hush_getblockindex(uint256 hash); /* On HUSH */ diff --git a/src/hush_defs.h b/src/hush_defs.h index 6fd33a8f9..2264a7160 100644 --- a/src/hush_defs.h +++ b/src/hush_defs.h @@ -597,7 +597,7 @@ void hush_netevent(std::vector payload); int32_t getacseason(uint32_t timestamp); int32_t gethushseason(int32_t height); -#define DRAGON_MAXSCRIPTSIZE 10001 +// DRAGON_MAXSCRIPTSIZE is defined once near the top of this header; the duplicate here was removed. #define HUSH_KVDURATION 1440 #define HUSH_KVBINARY 2 #define PRICES_SMOOTHWIDTH 1 diff --git a/src/hush_nSPV_fullnode.h b/src/hush_nSPV_fullnode.h index 54756e259..9287c2103 100644 --- a/src/hush_nSPV_fullnode.h +++ b/src/hush_nSPV_fullnode.h @@ -660,7 +660,7 @@ void hush_nSPVreq(CNode *pfrom,std::vector request) // received a reque struct NSPV_utxosresp U; if ( len >= 2 && len < 64+5 && request[1] < 64 && (request[1] == len-3 || request[1] == len-7 || request[1] == len-11) ) { - int32_t skipcount = 0; char coinaddr[64]; uint8_t filter; uint8_t isCC = 0; + int32_t skipcount = 0; char coinaddr[64]; uint32_t filter; uint8_t isCC = 0; memcpy(coinaddr,&request[2],request[1]); // request[1] < 64 bounds the copy + the terminator write coinaddr[request[1]] = 0; if ( request[1] == len-3 ) diff --git a/src/init.cpp b/src/init.cpp index e726e6623..d0c947bef 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -153,7 +153,7 @@ std::atomic fRequestShutdown(false); void StartShutdown() { if(fDebug) { - fprintf(stderr,"%s: fRequestShudown=true\n", __FUNCTION__); + fprintf(stderr,"%s: fRequestShutdown=true\n", __FUNCTION__); } fRequestShutdown = true; } @@ -2816,7 +2816,7 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) SetRPCWarmupFinished(); if(fDebug) - fprintf(stderr,"RPC warmump finished\n"); + fprintf(stderr,"RPC warmup finished\n"); uiInterface.InitMessage(_("Full Node Done Loading! :)")); #ifdef ENABLE_WALLET diff --git a/src/net.cpp b/src/net.cpp index 7fc4e7669..73e147c1a 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -691,9 +691,10 @@ void CNode::Ban(const CSubNet& subNet, const BanReason &banReason, int64_t banti { LOCK(cs_vNodes); for (CNode* pnode : vNodes) { - if (subNet.Match(static_cast(pnode->addr))) + if (subNet.Match(static_cast(pnode->addr))) { LogPrint("net", "%s: disconnecting from banned node %s\n", __func__, pnode->addr.ToString().c_str() ); pnode->fDisconnect = true; + } } } diff --git a/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp b/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp index 94db62e97..4300dc176 100644 --- a/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp +++ b/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp @@ -63,23 +63,8 @@ void AsyncRPCOperation_autoshieldcoinbase::main() { try { success = main_impl(); - } catch (const UniValue& objError) { - int code = find_value(objError, "code").get_int(); - std::string message = find_value(objError, "message").get_str(); - set_error_code(code); - set_error_message(message); - } catch (const runtime_error& e) { - set_error_code(-1); - set_error_message("runtime error: " + string(e.what())); - } catch (const logic_error& e) { - set_error_code(-1); - set_error_message("logic error: " + string(e.what())); - } catch (const exception& e) { - set_error_code(-1); - set_error_message("general exception: " + string(e.what())); } catch (...) { - set_error_code(-2); - set_error_message("unknown error"); + set_error_from_current_exception(); } stop_execution_clock(); diff --git a/src/wallet/asyncrpcoperation_mergetoaddress.cpp b/src/wallet/asyncrpcoperation_mergetoaddress.cpp index 6d92a44ad..dea3b8e11 100644 --- a/src/wallet/asyncrpcoperation_mergetoaddress.cpp +++ b/src/wallet/asyncrpcoperation_mergetoaddress.cpp @@ -133,23 +133,8 @@ void AsyncRPCOperation_mergetoaddress::main() try { success = main_impl(); - } catch (const UniValue& objError) { - int code = find_value(objError, "code").get_int(); - std::string message = find_value(objError, "message").get_str(); - set_error_code(code); - set_error_message(message); - } catch (const runtime_error& e) { - set_error_code(-1); - set_error_message("runtime error: " + string(e.what())); - } catch (const logic_error& e) { - set_error_code(-1); - set_error_message("logic error: " + string(e.what())); - } catch (const exception& e) { - set_error_code(-1); - set_error_message("general exception: " + string(e.what())); } catch (...) { - set_error_code(-2); - set_error_message("unknown error"); + set_error_from_current_exception(); } #ifdef ENABLE_MINING diff --git a/src/wallet/asyncrpcoperation_saplingconsolidation.cpp b/src/wallet/asyncrpcoperation_saplingconsolidation.cpp index 5a08f8cc7..6096d3d19 100644 --- a/src/wallet/asyncrpcoperation_saplingconsolidation.cpp +++ b/src/wallet/asyncrpcoperation_saplingconsolidation.cpp @@ -52,23 +52,8 @@ void AsyncRPCOperation_saplingconsolidation::main() { try { success = main_impl(); - } catch (const UniValue& objError) { - int code = find_value(objError, "code").get_int(); - std::string message = find_value(objError, "message").get_str(); - set_error_code(code); - set_error_message(message); - } catch (const runtime_error& e) { - set_error_code(-1); - set_error_message("runtime error: " + string(e.what())); - } catch (const logic_error& e) { - set_error_code(-1); - set_error_message("logic error: " + string(e.what())); - } catch (const exception& e) { - set_error_code(-1); - set_error_message("general exception: " + string(e.what())); } catch (...) { - set_error_code(-2); - set_error_message("unknown error"); + set_error_from_current_exception(); } stop_execution_clock(); diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 9c8fafa95..adbc78f40 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -155,23 +155,8 @@ void AsyncRPCOperation_sendmany::main() { try { success = main_impl(); - } catch (const UniValue& objError) { - int code = find_value(objError, "code").get_int(); - std::string message = find_value(objError, "message").get_str(); - set_error_code(code); - set_error_message(message); - } catch (const runtime_error& e) { - set_error_code(-1); - set_error_message("runtime error: " + string(e.what())); - } catch (const logic_error& e) { - set_error_code(-1); - set_error_message("logic error: " + string(e.what())); - } catch (const exception& e) { - set_error_code(-1); - set_error_message("general exception: " + string(e.what())); } catch (...) { - set_error_code(-2); - set_error_message("unknown error"); + set_error_from_current_exception(); } unlock_notes(); diff --git a/src/wallet/asyncrpcoperation_shieldcoinbase.cpp b/src/wallet/asyncrpcoperation_shieldcoinbase.cpp index e179f4476..16e175340 100644 --- a/src/wallet/asyncrpcoperation_shieldcoinbase.cpp +++ b/src/wallet/asyncrpcoperation_shieldcoinbase.cpp @@ -116,23 +116,8 @@ void AsyncRPCOperation_shieldcoinbase::main() { try { success = main_impl(); - } catch (const UniValue& objError) { - int code = find_value(objError, "code").get_int(); - std::string message = find_value(objError, "message").get_str(); - set_error_code(code); - set_error_message(message); - } catch (const runtime_error& e) { - set_error_code(-1); - set_error_message("runtime error: " + string(e.what())); - } catch (const logic_error& e) { - set_error_code(-1); - set_error_message("logic error: " + string(e.what())); - } catch (const exception& e) { - set_error_code(-1); - set_error_message("general exception: " + string(e.what())); } catch (...) { - set_error_code(-2); - set_error_message("unknown error"); + set_error_from_current_exception(); } #ifdef ENABLE_MINING diff --git a/src/wallet/asyncrpcoperation_sweep.cpp b/src/wallet/asyncrpcoperation_sweep.cpp index 5c94bf4e5..75a5b20c2 100644 --- a/src/wallet/asyncrpcoperation_sweep.cpp +++ b/src/wallet/asyncrpcoperation_sweep.cpp @@ -45,23 +45,8 @@ void AsyncRPCOperation_sweep::main() { try { success = main_impl(); - } catch (const UniValue& objError) { - int code = find_value(objError, "code").get_int(); - std::string message = find_value(objError, "message").get_str(); - set_error_code(code); - set_error_message(message); - } catch (const runtime_error& e) { - set_error_code(-1); - set_error_message("runtime error: " + string(e.what())); - } catch (const logic_error& e) { - set_error_code(-1); - set_error_message("logic error: " + string(e.what())); - } catch (const exception& e) { - set_error_code(-1); - set_error_message("general exception: " + string(e.what())); } catch (...) { - set_error_code(-2); - set_error_message("unknown error"); + set_error_from_current_exception(); } stop_execution_clock(); @@ -111,7 +96,7 @@ bool IsExcludedAddress(libzcash::SaplingPaymentAddress zaddr) { } } else { // This is an invalid sapling zaddr - LogPrintf("%s: Invalid zsweepexclude zaddr %s, ignoring\n", sweepExcludeAddress); + LogPrintf("%s: Invalid zsweepexclude zaddr %s, ignoring\n", __func__, sweepExcludeAddress); continue; } diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 6c507780b..069404ca7 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -196,9 +196,9 @@ UniValue getrescaninfo(const UniValue& params, bool fHelp, const CPubKey& mypk) auto startHeight = pwalletMain->rescanStartHeight; auto currentHeight = chainActive.Height(); // if current height is 0, progress=1 since there is nothing to rescan - char progress[8]; + char progress[16]; if (currentHeight != 0) { - sprintf(progress, "%.4f", (double) rescanHeight / (double) currentHeight ); + snprintf(progress, sizeof(progress), "%.4f", (double) rescanHeight / (double) currentHeight ); ret.push_back(Pair("rescan_progress", progress)); } ret.push_back(Pair("rescan_start_height", startHeight)); @@ -305,7 +305,7 @@ UniValue importprivkey(const UniValue& params, bool fHelp, const CPubKey& mypk) if (params.size() > 4) { - auto secret_key = AmountFromValue(params[4])/100000000; + secret_key = AmountFromValue(params[4])/100000000; key = DecodeCustomSecret(strSecret, secret_key); } else { key = DecodeSecret(strSecret); @@ -585,25 +585,27 @@ UniValue importwallet_impl(const UniValue& params, bool fHelp, bool fImportZKeys } } - if (fRescan) { - CBlockIndex *pindex = chainActive.LastTip(); - while (pindex && pindex->pprev && pindex->GetBlockTime() > nTimeBegin - 7200) - pindex = pindex->pprev; + // A failed key/address add must surface on BOTH paths; previously this + // check lived inside the fRescan block, so importwallet with rescan=false + // silently reported success even when some keys failed to import. + if (!fGood) + throw JSONRPCError(RPC_WALLET_ERROR, "Error adding some keys to wallet"); - LogPrintf("Rescanning last %i blocks\n", chainActive.Height() - pindex->GetHeight() + 1); - if (!pwalletMain->nTimeFirstKey || nTimeBegin < pwalletMain->nTimeFirstKey) - pwalletMain->nTimeFirstKey = nTimeBegin; - pwalletMain->ScanForWalletTransactions(pindex); - pwalletMain->MarkDirty(); + if (fRescan) { + CBlockIndex *pindex = chainActive.LastTip(); + while (pindex && pindex->pprev && pindex->GetBlockTime() > nTimeBegin - 7200) + pindex = pindex->pprev; - if (!fGood) - throw JSONRPCError(RPC_WALLET_ERROR, "Error adding some keys to wallet"); + LogPrintf("Rescanning last %i blocks\n", chainActive.Height() - pindex->GetHeight() + 1); + if (!pwalletMain->nTimeFirstKey || nTimeBegin < pwalletMain->nTimeFirstKey) + pwalletMain->nTimeFirstKey = nTimeBegin; + pwalletMain->ScanForWalletTransactions(pindex); + pwalletMain->MarkDirty(); + } else { + LogPrintf("Importwallet without rescan successful\n"); + } - return NullUniValue; } - - else{ - LogPrintf("Importwallet without Rescan successfull\n"); - return NullUniValue;} + return NullUniValue; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 1c84243a8..e0cd0d926 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4703,7 +4703,8 @@ bool CWallet::CommitTransaction(CWalletTx& wtxNew, CReserveKey& reservekey) // Add tx to wallet, because if it has change it's also ours, // otherwise just for transaction history. - AddToWallet(wtxNew, false, pwalletdb); + if (!AddToWallet(wtxNew, false, pwalletdb)) + LogPrintf("CommitTransaction(): Error: failed to persist wallet tx %s to disk; wallet may be out of sync with the ledger\n", wtxNew.GetHash().ToString()); // Notify that old coins are spent set setCoins;