From 7f64df008ecd78d530b42fb7d75469df07d25ba2 Mon Sep 17 00:00:00 2001 From: DanS Date: Fri, 9 Oct 2026 13:31:01 -0500 Subject: [PATCH] rpc: answer z_getoperationstatus without cs_main and cs_wallet z_getoperationstatus took LOCK2(cs_main, cs_wallet) for a pure read of the async operation queue, so a GUI polling it every few seconds queued behind every wallet scan and block connect. It also sorted UniValues with a by-value comparator. - z_getoperationstatus_IMPL takes no wallet locks. z_getoperationresult takes LOCK2 itself, so the removing path stays serialised with the wallet's swap of automated operations (cancel then remove, under cs_wallet). - Sort (creation_time, index) pairs comparing the time only. libstdc++'s sort depends only on the comparator results, so the order, ties included, is the same as before. - AsyncRPCQueue::popOperationForId finds and erases under one lock. - z_listoperationids takes no wallet locks; the status-name map is const and read with at(). Output is unchanged. Co-Authored-By: Claude Opus 5.5 --- src/asyncrpcoperation.cpp | 6 ++-- src/asyncrpcqueue.cpp | 12 +++++--- src/wallet/rpcwallet.cpp | 61 +++++++++++++++++++++++++++------------ 3 files changed, 53 insertions(+), 26 deletions(-) diff --git a/src/asyncrpcoperation.cpp b/src/asyncrpcoperation.cpp index 0579ca1f6..723a41d52 100644 --- a/src/asyncrpcoperation.cpp +++ b/src/asyncrpcoperation.cpp @@ -32,7 +32,7 @@ using namespace std; static boost::uuids::random_generator uuidgen; -static std::map OperationStatusMap = { +static const std::map OperationStatusMap = { {OperationStatus::READY, "queued"}, {OperationStatus::EXECUTING, "executing"}, {OperationStatus::CANCELLED, "cancelled"}, @@ -194,7 +194,7 @@ UniValue AsyncRPCOperation::getStatus() const { OperationStatus status = this->getState(); UniValue obj(UniValue::VOBJ); obj.push_back(Pair("id", this->id_)); - obj.push_back(Pair("status", OperationStatusMap[status])); + obj.push_back(Pair("status", OperationStatusMap.at(status))); obj.push_back(Pair("creation_time", this->creation_time_)); // TODO: Issue #1354: There may be other useful metadata to return to the user. UniValue err = this->getError(); @@ -218,5 +218,5 @@ UniValue AsyncRPCOperation::getStatus() const { */ std::string AsyncRPCOperation::getStateAsString() const { OperationStatus status = this->getState(); - return OperationStatusMap[status]; + return OperationStatusMap.at(status); } diff --git a/src/asyncrpcqueue.cpp b/src/asyncrpcqueue.cpp index ce356c146..ffde51391 100644 --- a/src/asyncrpcqueue.cpp +++ b/src/asyncrpcqueue.cpp @@ -129,14 +129,18 @@ std::shared_ptr AsyncRPCQueue::getOperationForId(AsyncRPCOper /** * Return the operation for a given operation id and then remove the operation from internal storage. + * The lookup and the erase happen under one hold of lock_, so when several threads pop the same id + * exactly one of them receives the operation and the others get a null pointer. */ std::shared_ptr AsyncRPCQueue::popOperationForId(AsyncRPCOperationId id) { - std::shared_ptr ptr = getOperationForId(id); - if (ptr) { - std::lock_guard guard(lock_); + std::shared_ptr ptr; // declared before guard, so it is released after unlock + std::lock_guard guard(lock_); + AsyncRPCOperationMap::iterator iter = operation_map_.find(id); + if (iter != operation_map_.end()) { + ptr = iter->second; // Note: if the id still exists in the operationIdQueue, when it gets processed by a worker // there will no operation in the map to execute, so nothing will happen. - operation_map_.erase(id); + operation_map_.erase(iter); } return ptr; } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index d673ac974..d89f356ea 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -5079,7 +5079,12 @@ UniValue z_getoperationresult(const UniValue& params, bool fHelp, const CPubKey& + HelpExampleRpc("z_getoperationresult", "'[\"operationid\", ... ]'") ); - // This call will remove finished operations + // This call will remove finished operations. Unlike z_getoperationstatus it keeps cs_main + + // cs_wallet: CWallet::RunSaplingSweep/RunSaplingConsolidation/RunAutoShieldCoinbase cancel and + // remove the previous automated operation inside one cs_wallet critical section, and holding + // cs_wallet here keeps this removal serialised with that and with other z_getoperationresult + // callers, exactly as before. + LOCK2(cs_main, pwalletMain->cs_wallet); return z_getoperationstatus_IMPL(params, true); } @@ -5108,7 +5113,12 @@ UniValue z_getoperationstatus(const UniValue& params, bool fHelp, const CPubKey& UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedOperations=false) { - LOCK2(cs_main, pwalletMain->cs_wallet); + // No cs_main/cs_wallet here: nothing below reads wallet or chain state. AsyncRPCQueue guards + // its operation map with its own mutex, and AsyncRPCOperation::getStatus() (and every + // override) reads only the atomic state_, members fixed at construction (id_, creation_time_, + // contextinfo_, targetHeight_) and result/error under the operation's own lock_. Holding the + // wallet locks only made this poll wait behind wallet scans and block connection. The removing + // form is serialised by its caller, z_getoperationresult, which still holds cs_main + cs_wallet. std::set filter; if (params.size()==1) { @@ -5119,11 +5129,18 @@ UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedO } bool useFilter = (filter.size()>0); - UniValue ret(UniValue::VARR); std::shared_ptr q = getAsyncRPCQueue(); std::vector ids = q->getAllOperationIds(); - for (auto id : ids) { + // Status objects in enumeration order, and one (creation_time, index) key per object for the + // sort. UniValue has no move operations, so sorting the objects themselves deep-copied them on + // every comparison and every element move. + std::vector objs; + objs.reserve(ids.size()); + std::vector > order; + order.reserve(ids.size()); + + for (const AsyncRPCOperationId& id : ids) { if (useFilter && !filter.count(id)) continue; @@ -5139,26 +5156,32 @@ UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedO if (fRemoveFinishedOperations) { // Caller is only interested in retrieving finished results if ("success"==s || "failed"==s || "cancelled"==s) { - ret.push_back(obj); - q->popOperationForId(id); + objs.push_back(obj); + // Report the operation only if this call is the one that removed it. + if (q->popOperationForId(id)) { + order.push_back(std::make_pair(operation->getCreationTime(), objs.size() - 1)); + } else { + objs.pop_back(); + } } } else { - ret.push_back(obj); + objs.push_back(obj); + order.push_back(std::make_pair(operation->getCreationTime(), objs.size() - 1)); } } - std::vector arrTmp = ret.getValues(); - - // sort results chronologically by creation_time - std::sort(arrTmp.begin(), arrTmp.end(), [](UniValue a, UniValue b) -> bool { - const int64_t t1 = find_value(a.get_obj(), "creation_time").get_int64(); - const int64_t t2 = find_value(b.get_obj(), "creation_time").get_int64(); - return t1 < t2; + // sort results chronologically by creation_time. Compare creation_time only, never the index: + // std::sort then makes exactly the comparisons and moves it made on the status objects before, + // so operations created in the same second keep the order earlier releases returned. + std::sort(order.begin(), order.end(), + [](const std::pair& a, const std::pair& b) -> bool { + return a.first < b.first; }); - ret.clear(); - ret.setArray(); - ret.push_backV(arrTmp); + UniValue ret(UniValue::VARR); + for (size_t i = 0; i < order.size(); i++) { + ret.push_back(objs[order[i].second]); + } return ret; } @@ -6245,8 +6268,8 @@ UniValue z_listoperationids(const UniValue& params, bool fHelp, const CPubKey& m + HelpExampleRpc("z_listoperationids", "") ); - LOCK2(cs_main, pwalletMain->cs_wallet); - + // No cs_main/cs_wallet: like z_getoperationstatus this only reads the self-locking async + // operation queue and each operation's atomic state. std::string filter; bool useFilter = false; if (params.size()==1) {