rpc: read vNodes under cs_vNodes in getinfo/getnetworkinfo

getinfo and getnetworkinfo read vNodes.size() and counted the TLS peers
under cs_main (getinfo also cs_wallet), not cs_vNodes, racing the socket
handler that pushes to and erases from vNodes. That is a torn read and
iteration over a vector being reallocated: undefined behaviour, in
practice a rare crash or a stale, inconsistent count.

Add GetNodeCounts(), which takes cs_vNodes once and returns a consistent
snapshot (tls_connections <= connections). Both RPCs now read the counts
through it before taking cs_main, so the two locks are never held at once
(no lock-order change, no extended hold of cs_vNodes) and the
"connections"/"tls_connections" output fields are unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

src/Makefile.gtest.include

@@ -14,7 +14,9 @@ hush_gtest_SOURCES = \
 	gtest/test_randomx_preverify.cpp \
 	gtest/test_hdtransparent.cpp \
 	gtest/test_mnemonic_compat.cpp \
-	gtest/test_stratum_jobid.cpp
+	gtest/test_stratum_jobid.cpp \
+	gtest/test_wallet_scan_notes.cpp \
+	gtest/test_operation_status.cpp
 
 hush_gtest_CPPFLAGS = $(AM_CPPFLAGS) -DMULTICORE -fopenmp -DBINARY_OUTPUT -DCURVE_ALT_BN128 -DSTATIC $(BITCOIN_INCLUDES)
 hush_gtest_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)

src/asyncrpcoperation.cpp

@@ -32,7 +32,7 @@ using namespace std;
 
 static boost::uuids::random_generator uuidgen;
 
-static std::map<OperationStatus, std::string> OperationStatusMap = {
+static const std::map<OperationStatus, std::string> 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);
 }

src/asyncrpcqueue.cpp

@@ -129,14 +129,18 @@ std::shared_ptr<AsyncRPCOperation> 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<AsyncRPCOperation> AsyncRPCQueue::popOperationForId(AsyncRPCOperationId id) {
-    std::shared_ptr<AsyncRPCOperation> ptr = getOperationForId(id);
-    if (ptr) {
+    std::shared_ptr<AsyncRPCOperation> ptr;     // declared before guard, so it is released after unlock
     std::lock_guard<std::mutex> 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;
 }

src/gtest/test_operation_status.cpp

@@ -0,0 +1,936 @@
+// Copyright (c) 2016-2026 The Hush developers
+// Distributed under the GPLv3 software license, see the accompanying
+// file COPYING or https://www.gnu.org/licenses/gpl-3.0.en.html
+//
+// z_getoperationstatus / z_getoperationresult / z_listoperationids after they stopped sorting
+// whole UniValues, and after z_getoperationstatus and z_listoperationids stopped taking
+// cs_main + cs_wallet. These tests show that:
+//   * both status calls return byte for byte what the original algorithm (copied below from
+//     c1040028e) returns, for queued, executing, successful, failed and cancelled operations,
+//     with and without id filters, with unknown and duplicate ids, with more than 16
+//     operations and with creation_time ties. std::sort reorders equal keys once there are
+//     more than 16 of them, so the order of same-second operations is part of the contract;
+//   * z_getoperationresult removes exactly the finished operations that it reports;
+//   * z_getoperationstatus and z_listoperationids answer while another thread holds
+//     cs_main + cs_wallet, and z_getoperationresult still waits for those locks;
+//   * AsyncRPCQueue::popOperationForId hands an operation to exactly one of several concurrent
+//     callers, so concurrent removers never report the same operation twice.
+//
+// No worker runs on the shared async queue in hush-gtest, so every operation keeps the state the
+// test gives it. Without pwalletMain the handlers throw "Method not found", and
+// z_getoperationresult locks pwalletMain->cs_wallet, so each test installs an empty in-memory
+// CWallet. These tests use TEST() and a TestEnv guard rather than a fixture, because a fixture
+// class cannot be named OperationStatus: that name is the operation state enum in
+// asyncrpcoperation.h.
+
+#include <gtest/gtest.h>
+#include <univalue.h>
+
+#include <algorithm>
+#include <atomic>
+#include <chrono>
+#include <condition_variable>
+#include <ctime>
+#include <future>
+#include <map>
+#include <memory>
+#include <mutex>
+#include <set>
+#include <stdexcept>
+#include <string>
+#include <thread>
+#include <vector>
+
+#include "asyncrpcoperation.h"
+#include "asyncrpcqueue.h"
+#include "init.h"
+#include "main.h"
+#include "pubkey.h"
+#include "rpc/server.h"
+#include "sync.h"
+#include "wallet/wallet.h"
+
+// Defined in wallet/rpcwallet.cpp (non-static, no header). Called directly so that removers can
+// run without the cs_main + cs_wallet that z_getoperationresult takes.
+extern UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedOperations);
+
+namespace {
+
+// Bound for every wait that must succeed, so a regression fails instead of hanging.
+const std::chrono::milliseconds kGuardTimeout(5000);
+
+const char* const kQueueNotEmpty = "the shared async queue already holds operations from elsewhere";
+const char* const kPoolFailed = "could not create operations with controlled creation_time ties";
+
+enum FakeKind {
+    FAKE_QUEUED = 0,
+    FAKE_EXECUTING,
+    FAKE_SUCCESS,
+    FAKE_FAILED,
+    FAKE_CANCELLED,
+    FAKE_KINDS
+};
+
+// An operation whose state is set by the test and never changes after it is queued: main() does
+// nothing, and nothing runs it anyway. getStatus() appends method/params exactly the way
+// z_sendmany, z_mergetoaddress and z_shieldcoinbase append their contextinfo_.
+class FixedStateOperation : public AsyncRPCOperation {
+public:
+    virtual void main() {}
+
+    virtual UniValue getStatus() const {
+        UniValue v = AsyncRPCOperation::getStatus();
+        if (contextinfo_.isNull()) {
+            return v;
+        }
+
+        UniValue obj = v.get_obj();
+        obj.push_back(Pair("method", "z_fixedstate"));
+        obj.push_back(Pair("params", contextinfo_));
+        return obj;
+    }
+
+    // Only called before the operation is queued, like a real operation's constructor.
+    void SetContext(const UniValue& context) {
+        contextinfo_ = context;
+    }
+
+    void ForceExecuting() {
+        set_state(OperationStatus::EXECUTING);
+    }
+
+    // Same order as the real operations: stop the clock, then publish the result and SUCCESS.
+    void ForceSuccess(const UniValue& result) {
+        start_execution_clock();
+        stop_execution_clock();
+        set_result(result);
+        set_state(OperationStatus::SUCCESS);
+    }
+
+    void ForceFailed(int code, const std::string& message) {
+        set_error_code(code);
+        set_error_message(message);
+        set_state(OperationStatus::FAILED);
+    }
+
+private:
+    UniValue contextinfo_;
+};
+
+std::shared_ptr<FixedStateOperation> MakeOperation(int kind, int tag)
+{
+    std::shared_ptr<FixedStateOperation> op(new FixedStateOperation());
+    if (tag % 3 == 0) {
+        UniValue context(UniValue::VOBJ);
+        context.push_back(Pair("fromaddress", "zs1fixedstate"));
+        context.push_back(Pair("minconf", tag));
+        op->SetContext(context);
+    }
+    switch (kind) {
+    case FAKE_QUEUED:
+        break;
+    case FAKE_EXECUTING:
+        op->ForceExecuting();
+        break;
+    case FAKE_SUCCESS:
+        if (tag % 2) {
+            UniValue result(UniValue::VOBJ);
+            result.push_back(Pair("txid", "txid-" + std::to_string(tag)));
+            op->ForceSuccess(result);
+        } else {
+            op->ForceSuccess(UniValue("done"));
+        }
+        break;
+    case FAKE_FAILED:
+        op->ForceFailed(-6, "boom " + std::to_string(tag));
+        break;
+    case FAKE_CANCELLED:
+        op->cancel();   // READY -> CANCELLED, the same path shutdown and wallet.cpp use
+        break;
+    }
+    return op;
+}
+
+bool IsFinished(const AsyncRPCOperation& op)
+{
+    return op.isSuccess() || op.isFailed() || op.isCancelled();
+}
+
+// The z_getoperationstatus_IMPL of c1040028e, verbatim except that the LOCK2 and the
+// popOperationForId call are left out. Evaluated on an unchanged queue just before the call under
+// test, it sees the same getAllOperationIds() enumeration and returns what the original returned:
+// the original removing pass popped each operation only after reading it, so it reported exactly
+// what this non-removing pass reports.
+UniValue ReferenceOperationStatus(const UniValue& params, bool fRemoveFinishedOperations)
+{
+    std::set<AsyncRPCOperationId> filter;
+    if (params.size()==1) {
+        UniValue ids = params[0].get_array();
+        for (const UniValue & v : ids.getValues()) {
+            filter.insert(v.get_str());
+        }
+    }
+    bool useFilter = (filter.size()>0);
+
+    UniValue ret(UniValue::VARR);
+    std::shared_ptr<AsyncRPCQueue> q = getAsyncRPCQueue();
+    std::vector<AsyncRPCOperationId> ids = q->getAllOperationIds();
+
+    for (auto id : ids) {
+        if (useFilter && !filter.count(id))
+            continue;
+
+        std::shared_ptr<AsyncRPCOperation> operation = q->getOperationForId(id);
+        if (!operation) {
+            continue;
+        }
+
+        UniValue obj = operation->getStatus();
+        std::string s = obj["status"].get_str();
+        if (fRemoveFinishedOperations) {
+            // Caller is only interested in retrieving finished results
+            if ("success"==s || "failed"==s || "cancelled"==s) {
+                ret.push_back(obj);
+            }
+        } else {
+            ret.push_back(obj);
+        }
+    }
+
+    std::vector<UniValue> 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;
+    });
+
+    ret.clear();
+    ret.setArray();
+    ret.push_backV(arrTmp);
+
+    return ret;
+}
+
+// The body of c1040028e's z_listoperationids after its argument check, verbatim without the LOCK2.
+UniValue ReferenceListOperationIds(const UniValue& params)
+{
+    std::string filter;
+    bool useFilter = false;
+    if (params.size()==1) {
+        filter = params[0].get_str();
+        useFilter = true;
+    }
+
+    UniValue ret(UniValue::VARR);
+    std::shared_ptr<AsyncRPCQueue> q = getAsyncRPCQueue();
+    std::vector<AsyncRPCOperationId> ids = q->getAllOperationIds();
+    for (auto id : ids) {
+        std::shared_ptr<AsyncRPCOperation> operation = q->getOperationForId(id);
+        if (!operation) {
+            continue;
+        }
+        std::string state = operation->getStateAsString();
+        if (useFilter && filter.compare(state)!=0)
+            continue;
+        ret.push_back(id);
+    }
+
+    return ret;
+}
+
+// Results and errors as one comparable string. These helpers make no gtest calls, so helper
+// threads can use them.
+std::string CallOutcome(rpcfn_type fn, const UniValue& params)
+{
+    try {
+        return fn(params, false, CPubKey()).write();
+    } catch (const UniValue& objError) {
+        return "rpc error: " + objError.write();
+    } catch (const std::exception& e) {
+        return std::string("exception: ") + e.what();
+    }
+}
+
+std::string ReferenceOutcome(const UniValue& params, bool fRemoveFinishedOperations)
+{
+    try {
+        return ReferenceOperationStatus(params, fRemoveFinishedOperations).write();
+    } catch (const UniValue& objError) {
+        return "rpc error: " + objError.write();
+    } catch (const std::exception& e) {
+        return std::string("exception: ") + e.what();
+    }
+}
+
+std::string ReferenceListOutcome(const UniValue& params)
+{
+    try {
+        return ReferenceListOperationIds(params).write();
+    } catch (const UniValue& objError) {
+        return "rpc error: " + objError.write();
+    } catch (const std::exception& e) {
+        return std::string("exception: ") + e.what();
+    }
+}
+
+// Byte comparison that reports where two (possibly very long) JSON texts first differ.
+::testing::AssertionResult SameText(const std::string& original, const std::string& patched)
+{
+    if (original == patched)
+        return ::testing::AssertionSuccess();
+    size_t i = 0;
+    while (i < original.size() && i < patched.size() && original[i] == patched[i])
+        i++;
+    const size_t from = i < 40 ? 0 : i - 40;
+    return ::testing::AssertionFailure()
+        << "outputs differ at byte " << i << " (lengths " << original.size() << " / " << patched.size() << ")"
+        << "\n  original: ..." << original.substr(from, 120)
+        << "\n  patched:  ..." << patched.substr(from, 120);
+}
+
+UniValue NoParams()
+{
+    return UniValue(UniValue::VARR);
+}
+
+// [[id, ...]]
+UniValue IdParams(const std::vector<std::string>& ids)
+{
+    UniValue arr(UniValue::VARR);
+    for (size_t i = 0; i < ids.size(); i++)
+        arr.push_back(ids[i]);
+    UniValue params(UniValue::VARR);
+    params.push_back(arr);
+    return params;
+}
+
+// The ids a params array asks for. Empty means "all", as in the handlers.
+std::set<std::string> RequestedIds(const UniValue& params)
+{
+    std::set<std::string> ids;
+    if (params.size() == 1) {
+        const std::vector<UniValue>& v = params[0].get_array().getValues();
+        for (size_t i = 0; i < v.size(); i++)
+            ids.insert(v[i].get_str());
+    }
+    return ids;
+}
+
+std::set<std::string> ListedIds()
+{
+    UniValue listed = z_listoperationids(NoParams(), false, CPubKey());
+    std::set<std::string> ids;
+    for (size_t i = 0; i < listed.size(); i++)
+        ids.insert(listed[i].get_str());
+    return ids;
+}
+
+// Installs an empty in-memory wallet as pwalletMain for one test, and when it goes out of scope
+// removes every operation the test added to the shared queue and restores pwalletMain.
+class TestEnv {
+public:
+    TestEnv() : queue_(getAsyncRPCQueue()), savedWallet_(pwalletMain) {
+        wasEmpty_ = queue_->getAllOperationIds().empty();
+        pwalletMain = &wallet_;
+    }
+
+    ~TestEnv() {
+        RemoveAll();
+        pwalletMain = savedWallet_;
+    }
+
+    bool QueueWasEmpty() const { return wasEmpty_; }
+
+    const std::shared_ptr<AsyncRPCQueue>& Queue() const { return queue_; }
+
+    // False if the queue refused an operation.
+    bool AddAll(const std::vector<std::shared_ptr<FixedStateOperation> >& ops) {
+        for (size_t i = 0; i < ops.size(); i++) {
+            if (!queue_->addOperation(ops[i]))
+                return false;
+            added_.push_back(ops[i]->getId());
+        }
+        return true;
+    }
+
+    void RemoveAll() {
+        for (size_t i = 0; i < added_.size(); i++)
+            queue_->popOperationForId(added_[i]);
+        added_.clear();
+    }
+
+private:
+    std::shared_ptr<AsyncRPCQueue> queue_;
+    CWallet wallet_;
+    CWallet* savedWallet_;
+    bool wasEmpty_;
+    std::vector<AsyncRPCOperationId> added_;
+};
+
+// Holds cs_main and pwalletMain->cs_wallet on a helper thread, as a long wallet scan does, until
+// Release() or destruction.
+class WalletLockHolder {
+public:
+    WalletLockHolder() : held_(false), release_(false), thread_(&WalletLockHolder::Run, this) {}
+
+    ~WalletLockHolder() {
+        Release();
+        thread_.join();
+    }
+
+    bool WaitUntilHeld(std::chrono::milliseconds timeout) {
+        std::unique_lock<std::mutex> lk(m_);
+        return cv_.wait_for(lk, timeout, [this]() { return held_; });
+    }
+
+    void Release() {
+        {
+            std::lock_guard<std::mutex> guard(m_);
+            release_ = true;
+        }
+        cv_.notify_all();
+    }
+
+private:
+    void Run() {
+        LOCK2(cs_main, pwalletMain->cs_wallet);
+        std::unique_lock<std::mutex> lk(m_);
+        held_ = true;
+        cv_.notify_all();
+        cv_.wait(lk, [this]() { return release_; });
+    }
+
+    std::mutex m_;
+    std::condition_variable cv_;
+    bool held_;
+    bool release_;
+    std::thread thread_;    // last, so that everything Run() touches exists before it starts
+};
+
+// creation_time is time(NULL) at construction and cannot be set, so ties and distinct times are
+// made by creating operations inside chosen seconds. Waits in 10 ms steps for the next second.
+void WaitForNextSecond()
+{
+    const time_t start = time(NULL);
+    for (int i = 0; i < 150 && time(NULL) == start; i++)
+        std::this_thread::sleep_for(std::chrono::milliseconds(10));
+}
+
+bool AllCreatedInOneSecond(const std::vector<std::shared_ptr<FixedStateOperation> >& ops)
+{
+    for (size_t i = 1; i < ops.size(); i++) {
+        if (ops[i]->getCreationTime() != ops[0]->getCreationTime())
+            return false;
+    }
+    return true;
+}
+
+const int kPoolGroups = 3;
+const size_t kPoolGroupSize = 300;
+
+// kPoolGroups groups of operations in all five states. Every operation in a group has the same
+// creation_time, and the groups' creation_times strictly increase.
+struct OperationPool {
+    bool ok;
+    std::vector<std::vector<std::shared_ptr<FixedStateOperation> > > groups;
+};
+
+OperationPool BuildPool()
+{
+    OperationPool pool;
+    pool.ok = true;
+    for (int g = 0; g < kPoolGroups && pool.ok; g++) {
+        std::vector<std::shared_ptr<FixedStateOperation> > group;
+        bool tied = false;
+        for (int attempt = 0; attempt < 3 && !tied; attempt++) {
+            if (g > 0 || attempt > 0)
+                WaitForNextSecond();
+            group.clear();
+            for (size_t i = 0; i < kPoolGroupSize; i++) {
+                group.push_back(MakeOperation(static_cast<int>((i + 2 * g) % FAKE_KINDS),
+                                              static_cast<int>(g * kPoolGroupSize + i)));
+            }
+            tied = AllCreatedInOneSecond(group);
+        }
+        const bool later = pool.groups.empty() ||
+                           group.front()->getCreationTime() > pool.groups.back().front()->getCreationTime();
+        pool.ok = tied && later;
+        pool.groups.push_back(group);
+    }
+    return pool;
+}
+
+// Built once (it waits for up to about two second boundaries) and shared by the tests that need
+// it. The operations are never modified after they are built.
+const OperationPool& SharedPool()
+{
+    static const OperationPool pool = BuildPool();
+    return pool;
+}
+
+struct Population {
+    std::string name;
+    std::vector<std::shared_ptr<FixedStateOperation> > ops;
+};
+
+// For each size: "ties-K", K operations from one second, and "mixed-K", K operations spread
+// round robin over three seconds (ties within each second). 16 is the largest size that
+// libstdc++'s std::sort handles with insertion sort alone; from 17 on, introsort reorders equal keys.
+std::vector<Population> Populations(const OperationPool& pool)
+{
+    static const size_t sizes[] = {0, 1, 2, 16, 17, 40, 300};
+    std::vector<Population> pops;
+    for (size_t s = 0; s < sizeof(sizes) / sizeof(sizes[0]); s++) {
+        const size_t k = sizes[s];
+
+        Population ties;
+        ties.name = "ties-" + std::to_string(k);
+        ties.ops.assign(pool.groups[1].begin(), pool.groups[1].begin() + k);
+        pops.push_back(ties);
+
+        Population mixed;
+        mixed.name = "mixed-" + std::to_string(k);
+        for (size_t i = 0; i < k; i++)
+            mixed.ops.push_back(pool.groups[i % kPoolGroups][i / kPoolGroups]);
+        pops.push_back(mixed);
+    }
+    return pops;
+}
+
+// Arguments for z_getoperationstatus/z_getoperationresult over a population: no argument, an
+// empty list (no filter), an unknown id, two ids, a duplicated id, every other id plus an
+// unknown one, and every id.
+std::vector<UniValue> StatusParamSets(const std::vector<std::shared_ptr<FixedStateOperation> >& ops)
+{
+    std::vector<UniValue> sets;
+    sets.push_back(NoParams());
+    sets.push_back(IdParams(std::vector<std::string>()));
+    sets.push_back(IdParams(std::vector<std::string>(1, "opid-unknown")));
+    if (!ops.empty()) {
+        std::vector<std::string> two;
+        two.push_back(ops.front()->getId());
+        two.push_back(ops.back()->getId());
+        sets.push_back(IdParams(two));
+        sets.push_back(IdParams(std::vector<std::string>(2, ops.front()->getId())));
+
+        std::vector<std::string> half, all;
+        for (size_t i = 0; i < ops.size(); i++) {
+            all.push_back(ops[i]->getId());
+            if (i % 2 == 0)
+                half.push_back(ops[i]->getId());
+        }
+        half.push_back("opid-unknown");
+        sets.push_back(IdParams(half));
+        sets.push_back(IdParams(all));
+    }
+    return sets;
+}
+
+// Pops every id once, as fast as it can, after the start signal.
+void PopEach(AsyncRPCQueue* q, const std::vector<AsyncRPCOperationId>* ids,
+             const std::atomic<bool>* go, std::vector<AsyncRPCOperationId>* out)
+{
+    while (!go->load())
+        std::this_thread::yield();
+    for (size_t i = 0; i < ids->size(); i++) {
+        if (q->popOperationForId((*ids)[i]) != nullptr)
+            out->push_back((*ids)[i]);
+    }
+}
+
+// Removes finished operations until the queue is empty (or kGuardTimeout passes) and records
+// every id it was handed. If direct is true it calls the IMPL without cs_main/cs_wallet, so only
+// the queue's atomic pop and the IMPL's checked pop keep it apart from the other removers.
+// Otherwise it calls the z_getoperationresult handler, which takes the locks.
+void DrainFinished(bool direct, const std::atomic<bool>* go, std::vector<std::string>* out)
+{
+    while (!go->load())
+        std::this_thread::yield();
+    const UniValue params(UniValue::VARR);
+    std::shared_ptr<AsyncRPCQueue> q = getAsyncRPCQueue();
+    const std::chrono::steady_clock::time_point deadline = std::chrono::steady_clock::now() + kGuardTimeout;
+    while (std::chrono::steady_clock::now() < deadline) {
+        UniValue ret;
+        try {
+            ret = direct ? z_getoperationstatus_IMPL(params, true)
+                         : z_getoperationresult(params, false, CPubKey());
+            for (size_t i = 0; i < ret.size(); i++)
+                out->push_back(find_value(ret[i], "id").get_str());
+        } catch (...) {
+            out->push_back("<exception>");
+            return;
+        }
+        if (ret.empty() && q->getAllOperationIds().empty())
+            return;
+    }
+}
+
+// Polls z_getoperationstatus while the removers run. Counts results that break its contract
+// (only the test's finished operations exist): an unfinished status, a repeated id, or
+// creation_time out of order.
+void PollStatus(const std::atomic<bool>* go, const std::atomic<bool>* stop, int* faults)
+{
+    while (!go->load())
+        std::this_thread::yield();
+    const UniValue params(UniValue::VARR);
+    while (!stop->load()) {
+        try {
+            UniValue ret = z_getoperationstatus(params, false, CPubKey());
+            std::set<std::string> ids;
+            int64_t last = 0;
+            for (size_t i = 0; i < ret.size(); i++) {
+                const std::string id = find_value(ret[i], "id").get_str();
+                const std::string status = find_value(ret[i], "status").get_str();
+                const int64_t t = find_value(ret[i], "creation_time").get_int64();
+                if (!ids.insert(id).second || t < last ||
+                    (status != "success" && status != "failed" && status != "cancelled"))
+                    (*faults)++;
+                last = t;
+            }
+        } catch (...) {
+            (*faults)++;
+        }
+    }
+}
+
+} // namespace
+
+// The read path returns exactly what the original algorithm returned, including the order of
+// same-second operations, and removes nothing.
+TEST(OperationStatus, StatusMatchesOriginalAlgorithm)
+{
+    const OperationPool& pool = SharedPool();
+    ASSERT_TRUE(pool.ok) << kPoolFailed;
+    TestEnv env;
+    ASSERT_TRUE(env.QueueWasEmpty()) << kQueueNotEmpty;
+
+    const std::vector<Population> pops = Populations(pool);
+    for (size_t p = 0; p < pops.size(); p++) {
+        ASSERT_TRUE(env.AddAll(pops[p].ops));
+        const std::vector<UniValue> paramSets = StatusParamSets(pops[p].ops);
+        for (size_t i = 0; i < paramSets.size(); i++) {
+            EXPECT_TRUE(SameText(ReferenceOutcome(paramSets[i], false),
+                                 CallOutcome(&z_getoperationstatus, paramSets[i])))
+                << pops[p].name << " params=" << paramSets[i].write();
+        }
+        EXPECT_EQ(pops[p].ops.size(), env.Queue()->getAllOperationIds().size()) << pops[p].name;
+        env.RemoveAll();
+    }
+
+#if defined(__GLIBCXX__)
+    // Check that the comparison above can fail. With 40 operations in one second, libstdc++'s
+    // introsort reorders the equal keys, so the original output is not the enumeration order.
+    // A (creation_time, index) comparator would keep the enumeration order and fail above.
+    std::vector<std::shared_ptr<FixedStateOperation> > tied(pool.groups[1].begin(), pool.groups[1].begin() + 40);
+    ASSERT_TRUE(env.AddAll(tied));
+    const UniValue ref = ReferenceOperationStatus(NoParams(), false);
+    std::vector<std::string> reported;
+    for (size_t i = 0; i < ref.size(); i++)
+        reported.push_back(find_value(ref[i], "id").get_str());
+    const std::vector<std::string> enumerated = env.Queue()->getAllOperationIds();
+    EXPECT_EQ(enumerated.size(), reported.size());
+    EXPECT_NE(enumerated, reported) << "40 tied operations kept their enumeration order";
+    env.RemoveAll();
+#endif
+}
+
+// The removing path reports exactly what the original reported, removes exactly the finished
+// operations it was asked about, and leaves the rest visible to the other two calls.
+TEST(OperationStatus, ResultMatchesOriginalAlgorithm)
+{
+    const OperationPool& pool = SharedPool();
+    ASSERT_TRUE(pool.ok) << kPoolFailed;
+    TestEnv env;
+    ASSERT_TRUE(env.QueueWasEmpty()) << kQueueNotEmpty;
+
+    const std::vector<Population> pops = Populations(pool);
+    for (size_t p = 0; p < pops.size(); p++) {
+        const std::vector<UniValue> paramSets = StatusParamSets(pops[p].ops);
+        for (size_t i = 0; i < paramSets.size(); i++) {
+            const UniValue& params = paramSets[i];
+            const std::string where = pops[p].name + " params=" + params.write();
+            ASSERT_TRUE(env.AddAll(pops[p].ops));
+
+            const std::string ref = ReferenceOutcome(params, true);
+            EXPECT_TRUE(SameText(ref, CallOutcome(&z_getoperationresult, params))) << where;
+
+            const std::set<std::string> requested = RequestedIds(params);
+            std::set<std::string> remaining;
+            int wrong = 0;
+            for (size_t j = 0; j < pops[p].ops.size(); j++) {
+                const std::shared_ptr<FixedStateOperation>& op = pops[p].ops[j];
+                const bool selected = requested.empty() || requested.count(op->getId()) > 0;
+                const bool present = env.Queue()->getOperationForId(op->getId()) != nullptr;
+                if (present != !(IsFinished(*op) && selected))
+                    wrong++;
+                if (present)
+                    remaining.insert(op->getId());
+            }
+            EXPECT_EQ(0, wrong) << where << ": operations removed or kept wrongly";
+
+            // Nothing is handed out twice, and the survivors are still listed and reported.
+            EXPECT_EQ("[]", CallOutcome(&z_getoperationresult, params)) << where;
+            EXPECT_EQ(remaining, ListedIds()) << where;
+            EXPECT_TRUE(SameText(ReferenceOutcome(NoParams(), false),
+                                 CallOutcome(&z_getoperationstatus, NoParams()))) << where;
+
+            env.RemoveAll();
+        }
+    }
+}
+
+TEST(OperationStatus, ListOperationIdsMatchesOriginalAlgorithm)
+{
+    const OperationPool& pool = SharedPool();
+    ASSERT_TRUE(pool.ok) << kPoolFailed;
+    TestEnv env;
+    ASSERT_TRUE(env.QueueWasEmpty()) << kQueueNotEmpty;
+
+    const char* const filters[] = {NULL, "queued", "executing", "success", "failed", "cancelled", "bogus"};
+    const std::vector<Population> pops = Populations(pool);
+    for (size_t p = 0; p < pops.size(); p++) {
+        ASSERT_TRUE(env.AddAll(pops[p].ops));
+        for (size_t f = 0; f < sizeof(filters) / sizeof(filters[0]); f++) {
+            UniValue params(UniValue::VARR);
+            if (filters[f] != NULL)
+                params.push_back(filters[f]);
+            EXPECT_TRUE(SameText(ReferenceListOutcome(params), CallOutcome(&z_listoperationids, params)))
+                << pops[p].name << " params=" << params.write();
+        }
+        env.RemoveAll();
+    }
+}
+
+// Malformed arguments fail exactly as before and remove nothing; no wallet still means
+// "Method not found".
+TEST(OperationStatus, ParameterErrorsUnchanged)
+{
+    TestEnv env;
+    ASSERT_TRUE(env.QueueWasEmpty()) << kQueueNotEmpty;
+    std::vector<std::shared_ptr<FixedStateOperation> > ops;
+    for (int k = 0; k < FAKE_KINDS; k++)
+        ops.push_back(MakeOperation(k, k));
+    ASSERT_TRUE(env.AddAll(ops));
+
+    UniValue notArray(UniValue::VARR);
+    notArray.push_back("not_an_array");
+    UniValue inner(UniValue::VARR);
+    inner.push_back(ops[FAKE_SUCCESS]->getId());
+    inner.push_back(1);
+    UniValue notString(UniValue::VARR);
+    notString.push_back(inner);
+
+    EXPECT_EQ("exception: JSON value is not an array as expected", ReferenceOutcome(notArray, false));
+    EXPECT_EQ("exception: JSON value is not a string as expected", ReferenceOutcome(notString, false));
+    EXPECT_EQ(ReferenceOutcome(notArray, false), CallOutcome(&z_getoperationstatus, notArray));
+    EXPECT_EQ(ReferenceOutcome(notString, false), CallOutcome(&z_getoperationstatus, notString));
+    EXPECT_EQ(ReferenceOutcome(notArray, true), CallOutcome(&z_getoperationresult, notArray));
+    EXPECT_EQ(ReferenceOutcome(notString, true), CallOutcome(&z_getoperationresult, notString));
+
+    UniValue tooMany(UniValue::VARR);
+    tooMany.push_back(UniValue(UniValue::VARR));
+    tooMany.push_back("toomanyargs");
+    EXPECT_THROW(z_getoperationstatus(tooMany, false, CPubKey()), std::runtime_error);
+    EXPECT_THROW(z_getoperationresult(tooMany, false, CPubKey()), std::runtime_error);
+    EXPECT_THROW(z_listoperationids(tooMany, false, CPubKey()), std::runtime_error);
+
+    EXPECT_EQ(ops.size(), env.Queue()->getAllOperationIds().size());
+
+    CWallet* wallet = pwalletMain;
+    pwalletMain = NULL;
+    EXPECT_THROW(z_getoperationstatus(NoParams(), false, CPubKey()), UniValue);
+    EXPECT_THROW(z_getoperationresult(NoParams(), false, CPubKey()), UniValue);
+    EXPECT_THROW(z_listoperationids(NoParams(), false, CPubKey()), UniValue);
+    pwalletMain = wallet;
+}
+
+// While another thread holds cs_main + cs_wallet, z_getoperationstatus (unfiltered and filtered,
+// as the GUI polls it) and z_listoperationids still answer, with unchanged output, and
+// z_getoperationresult waits for the locks and then answers as before.
+TEST(OperationStatus, StatusDoesNotWaitForWalletLocks)
+{
+    TestEnv env;
+    ASSERT_TRUE(env.QueueWasEmpty()) << kQueueNotEmpty;
+    std::vector<std::shared_ptr<FixedStateOperation> > ops;
+    for (int k = 0; k < FAKE_KINDS; k++)
+        ops.push_back(MakeOperation(k, k));
+    ASSERT_TRUE(env.AddAll(ops));
+
+    const UniValue filtered = IdParams(std::vector<std::string>(1, ops[FAKE_SUCCESS]->getId()));
+    const std::string refStatus = ReferenceOutcome(NoParams(), false);
+    const std::string refFiltered = ReferenceOutcome(filtered, false);
+    const std::string refList = ReferenceListOutcome(NoParams());
+    const std::string refResult = ReferenceOutcome(NoParams(), true);
+
+    // Declared before the holder: on an early return the holder is destroyed first, which
+    // releases the locks, and only then do the futures' destructors wait for their calls.
+    std::future<std::string> status, statusFiltered, listed, result;
+    WalletLockHolder holder;
+    ASSERT_TRUE(holder.WaitUntilHeld(kGuardTimeout)) << "helper thread could not take cs_main + cs_wallet";
+
+    status = std::async(std::launch::async, CallOutcome, &z_getoperationstatus, NoParams());
+    statusFiltered = std::async(std::launch::async, CallOutcome, &z_getoperationstatus, filtered);
+    listed = std::async(std::launch::async, CallOutcome, &z_listoperationids, NoParams());
+    result = std::async(std::launch::async, CallOutcome, &z_getoperationresult, NoParams());
+
+    const bool statusDone = status.wait_for(kGuardTimeout) == std::future_status::ready;
+    const bool filteredDone = statusFiltered.wait_for(kGuardTimeout) == std::future_status::ready;
+    const bool listDone = listed.wait_for(kGuardTimeout) == std::future_status::ready;
+    // The locks are still held, so a z_getoperationresult that takes them cannot have finished.
+    const bool resultWaited = result.wait_for(std::chrono::milliseconds(100)) == std::future_status::timeout;
+    holder.Release();
+    const bool resultDone = result.wait_for(kGuardTimeout) == std::future_status::ready;
+
+    EXPECT_TRUE(statusDone) << "z_getoperationstatus waited for cs_main/cs_wallet";
+    EXPECT_TRUE(filteredDone) << "filtered z_getoperationstatus waited for cs_main/cs_wallet";
+    EXPECT_TRUE(listDone) << "z_listoperationids waited for cs_main/cs_wallet";
+    EXPECT_TRUE(resultWaited) << "z_getoperationresult no longer waits for cs_main/cs_wallet";
+    EXPECT_TRUE(resultDone) << "z_getoperationresult did not finish after the locks were released";
+    if (statusDone) {
+        EXPECT_EQ(refStatus, status.get());
+    }
+    if (filteredDone) {
+        EXPECT_EQ(refFiltered, statusFiltered.get());
+    }
+    if (listDone) {
+        EXPECT_EQ(refList, listed.get());
+    }
+    if (resultDone) {
+        EXPECT_EQ(refResult, result.get());
+    }
+
+    // The result call removed the three finished operations and kept the queued and executing ones.
+    std::set<std::string> expected;
+    expected.insert(ops[FAKE_QUEUED]->getId());
+    expected.insert(ops[FAKE_EXECUTING]->getId());
+    EXPECT_EQ(expected, ListedIds());
+}
+
+// SC-1: popOperationForId looks up and erases under one hold of the queue lock, so when several
+// threads pop the same ids each operation goes to exactly one of them. The old two-step pop
+// could hand the same operation to two callers; this catches that probabilistically.
+TEST(OperationStatus, PopOperationForIdHandsEachOperationToOneCaller)
+{
+    {
+        std::shared_ptr<AsyncRPCQueue> q = std::make_shared<AsyncRPCQueue>();
+        std::shared_ptr<FixedStateOperation> op = MakeOperation(FAKE_SUCCESS, 1);
+        ASSERT_TRUE(q->addOperation(op));
+        EXPECT_TRUE(q->popOperationForId("opid-unknown") == nullptr);
+        EXPECT_TRUE(q->popOperationForId(op->getId()).get() == op.get());
+        EXPECT_TRUE(q->getOperationForId(op->getId()) == nullptr);
+        EXPECT_TRUE(q->popOperationForId(op->getId()) == nullptr);
+    }
+
+    const int kThreads = 8;
+    const int kOps = 1000;
+    const int kRounds = 10;
+    for (int round = 0; round < kRounds; round++) {
+        // A private queue with no workers.
+        std::shared_ptr<AsyncRPCQueue> q = std::make_shared<AsyncRPCQueue>();
+        std::vector<AsyncRPCOperationId> ids;
+        for (int i = 0; i < kOps; i++) {
+            std::shared_ptr<AsyncRPCOperation> op(new FixedStateOperation());
+            ASSERT_TRUE(q->addOperation(op));
+            ids.push_back(op->getId());
+        }
+
+        std::atomic<bool> go(false);
+        std::vector<std::vector<AsyncRPCOperationId> > popped(kThreads);
+        std::vector<std::thread> threads;
+        for (int t = 0; t < kThreads; t++)
+            threads.push_back(std::thread(PopEach, q.get(), &ids, &go, &popped[t]));
+        go.store(true);
+        for (size_t t = 0; t < threads.size(); t++)
+            threads[t].join();
+
+        std::map<AsyncRPCOperationId, int> handed;
+        size_t total = 0;
+        for (int t = 0; t < kThreads; t++) {
+            for (size_t i = 0; i < popped[t].size(); i++) {
+                handed[popped[t][i]]++;
+                total++;
+            }
+        }
+        int wrong = 0;
+        for (size_t i = 0; i < ids.size(); i++) {
+            std::map<AsyncRPCOperationId, int>::const_iterator it = handed.find(ids[i]);
+            if (it == handed.end() || it->second != 1)
+                wrong++;
+        }
+        EXPECT_EQ(0, wrong) << "round " << round << ": operations not handed to exactly one caller";
+        EXPECT_EQ(static_cast<size_t>(kOps), total) << "round " << round;
+        EXPECT_TRUE(q->getAllOperationIds().empty()) << "round " << round;
+    }
+}
+
+// Removers through the locked handler and straight through the IMPL (no wallet locks) drain the
+// same finished operations while two lock-free z_getoperationstatus pollers run. Every operation
+// is reported exactly once, and no poll returns a malformed or unsorted list.
+TEST(OperationStatus, ConcurrentRemoversReportEachOperationOnce)
+{
+    TestEnv env;
+    ASSERT_TRUE(env.QueueWasEmpty()) << kQueueNotEmpty;
+    const int kOps = 200;
+    std::vector<std::shared_ptr<FixedStateOperation> > ops;
+    std::set<std::string> expected;
+    for (int i = 0; i < kOps; i++) {
+        ops.push_back(MakeOperation(FAKE_SUCCESS + i % 3, i));     // success, failed or cancelled
+        expected.insert(ops.back()->getId());
+    }
+    ASSERT_TRUE(env.AddAll(ops));
+
+    const int kRemovers = 8;
+    const int kReaders = 2;
+    std::atomic<bool> go(false);
+    std::atomic<bool> stop(false);
+    std::vector<std::vector<std::string> > removed(kRemovers);
+    std::vector<int> readerFaults(kReaders, 0);
+    std::vector<std::thread> removers, readers;
+    for (int t = 0; t < kRemovers; t++)
+        removers.push_back(std::thread(DrainFinished, t % 2 == 0, &go, &removed[t]));
+    for (int r = 0; r < kReaders; r++)
+        readers.push_back(std::thread(PollStatus, &go, &stop, &readerFaults[r]));
+    go.store(true);
+    for (size_t t = 0; t < removers.size(); t++)
+        removers[t].join();
+    stop.store(true);
+    for (size_t r = 0; r < readers.size(); r++)
+        readers[r].join();
+
+    std::map<std::string, int> reported;
+    size_t total = 0;
+    for (int t = 0; t < kRemovers; t++) {
+        for (size_t i = 0; i < removed[t].size(); i++) {
+            reported[removed[t][i]]++;
+            total++;
+        }
+    }
+    int duplicated = 0, unknown = 0;
+    for (std::map<std::string, int>::const_iterator it = reported.begin(); it != reported.end(); ++it) {
+        if (it->second != 1)
+            duplicated++;
+        if (!expected.count(it->first))
+            unknown++;
+    }
+    EXPECT_EQ(0, duplicated) << "an operation was reported by more than one remover";
+    EXPECT_EQ(0, unknown) << "a remover reported an unknown id or threw";
+    EXPECT_EQ(static_cast<size_t>(kOps), reported.size());
+    EXPECT_EQ(static_cast<size_t>(kOps), total);
+    EXPECT_TRUE(env.Queue()->getAllOperationIds().empty());
+    for (int r = 0; r < kReaders; r++)
+        EXPECT_EQ(0, readerFaults[r]) << "reader " << r;
+}
+
+// SC-4: the const status map with .at() names every state exactly as before.
+TEST(OperationStatus, StateNamesUnchanged)
+{
+    const char* const names[FAKE_KINDS] = {"queued", "executing", "success", "failed", "cancelled"};
+    for (int k = 0; k < FAKE_KINDS; k++) {
+        std::shared_ptr<FixedStateOperation> op = MakeOperation(k, k + 1);
+        EXPECT_EQ(std::string(names[k]), op->getStateAsString());
+        const UniValue status = op->getStatus();
+        EXPECT_EQ(std::string(names[k]), find_value(status, "status").get_str());
+    }
+}

src/gtest/test_wallet_scan_notes.cpp

@@ -0,0 +1,848 @@
+// Copyright (c) 2016-2026 The Hush developers
+// Distributed under the GPLv3 software license, see the accompanying
+// file COPYING or https://www.gnu.org/licenses/gpl-3.0.en.html
+//
+// Proves that the optimised CWallet::GetFilteredNotes returns exactly what the original
+// did: the same entries, in the same order, with every field equal. The optimisations
+// are references instead of copies, skipping txs without note data, the spent check
+// before trial decryption, the exact ivk pre-filter for address-filtered calls, and
+// building the note and address from decrypt_with_pk_d. RefGetFilteredNotes below is a
+// verbatim copy of the original body (c1040028e), and both run over the same hand-built
+// wallet for a grid of filters and parameters. The file also tests
+// SaplingNotePlaintext::decrypt_with_pk_d against decrypt().
+//
+// No proofs are needed: the wallet never verifies them, so notes are encrypted by hand
+// into fabricated OutputDescriptions and added with AddToWallet(wtx, true, NULL). Depth
+// is faked with a mapBlockIndex chain (depth >= 1), the mempool (depth 0) or neither
+// (depth -1). minDepth > 1 is not covered: that branch calls tx_height(), which reads
+// the tx from disk (the regtest differential covers it).
+
+#include <gtest/gtest.h>
+
+#include "arith_uint256.h"
+#include "chain.h"
+#include "chainparams.h"
+#include "key_io.h"
+#include "main.h"
+#include "primitives/transaction.h"
+#include "txmempool.h"
+#include "uint256.h"
+#include "util.h"
+#include "wallet/crypter.h"
+#include "wallet/wallet.h"
+#include "zcash/Address.hpp"
+#include "zcash/Note.hpp"
+#include "zcash/zip32.h"
+
+#include <algorithm>
+#include <array>
+#include <cassert>
+#include <climits>
+#include <list>
+#include <memory>
+#include <set>
+#include <string>
+#include <vector>
+
+#include <boost/filesystem.hpp>
+
+// The original body calls these; wallet.cpp declares them the same way (wallet.cpp:77-78).
+// Both are defined in main.cpp (via hush.h). Neither is reached here (minDepth <= 1).
+int32_t hush_dpowconfs(int32_t height,int32_t numconfs);
+int tx_height( const uint256 &hash );
+
+/**
+ * Verbatim copy of the original
+ * CWallet::GetFilteredNotes(std::vector<SaplingNoteEntry>&, std::set<PaymentAddress>&,
+ * int, int, bool, bool, bool) at c1040028e (wallet.cpp:5637-5721). The only changes:
+ * it is a free function, so wallet members are `wallet.`-qualified (all of them are
+ * public: cs_wallet, mapWallet, IsSaplingSpent, GetSaplingIncomingViewingKey,
+ * GetSaplingFullViewingKey, HaveSaplingSpendingKey, IsLockedNote), and the two LogPrintf
+ * calls are dropped (they do not affect the output).
+ */
+static void RefGetFilteredNotes(
+    CWallet& wallet,
+    std::vector<SaplingNoteEntry>& saplingEntries,
+    std::set<libzcash::PaymentAddress>& filterAddresses,
+    int minDepth,
+    int maxDepth,
+    bool ignoreSpent,
+    bool requireSpendingKey,
+    bool ignoreLocked)
+{
+    using namespace libzcash;
+
+    LOCK2(cs_main, wallet.cs_wallet);
+
+    for (auto & p : wallet.mapWallet) {
+        CWalletTx wtx = p.second;
+
+        // Filter the transactions before checking for notes
+        if (!CheckFinalTx(wtx) || wtx.GetBlocksToMaturity() > 0)
+            continue;
+
+        if (minDepth > 1) {
+            int nHeight    = tx_height(wtx.GetHash());
+            if ( nHeight == 0 ) {
+                continue;
+            }
+            int nDepth     = wtx.GetDepthInMainChain();
+            int dpowconfs  = hush_dpowconfs(nHeight,nDepth);
+            if ( dpowconfs < minDepth || dpowconfs > maxDepth) {
+                continue;
+            }
+        } else {
+            if ( wtx.GetDepthInMainChain() < minDepth ||
+                wtx.GetDepthInMainChain() > maxDepth) {
+                continue;
+            }
+        }
+
+        for (auto & pair : wtx.mapSaplingNoteData) {
+            SaplingOutPoint op = pair.first;
+            SaplingNoteData nd = pair.second;
+
+            auto maybe_pt = SaplingNotePlaintext::decrypt(
+                wtx.vShieldedOutput[op.n].encCiphertext,
+                nd.ivk,
+                wtx.vShieldedOutput[op.n].ephemeralKey,
+                wtx.vShieldedOutput[op.n].cm);
+            assert(static_cast<bool>(maybe_pt));
+            auto notePt = maybe_pt.get();
+
+            auto maybe_pa = nd.ivk.address(notePt.d);
+            assert(static_cast<bool>(maybe_pa));
+            auto pa = maybe_pa.get();
+
+            // skip notes which belong to a different payment address in the wallet
+            if (!(filterAddresses.empty() || filterAddresses.count(pa))) {
+                continue;
+            }
+
+            if (ignoreSpent && nd.nullifier && wallet.IsSaplingSpent(*nd.nullifier)) {
+                continue;
+            }
+
+            // skip notes which cannot be spent
+            if (requireSpendingKey) {
+                libzcash::SaplingIncomingViewingKey ivk;
+                libzcash::SaplingFullViewingKey fvk;
+                if (!(wallet.GetSaplingIncomingViewingKey(pa, ivk) &&
+                    wallet.GetSaplingFullViewingKey(ivk, fvk) &&
+                    wallet.HaveSaplingSpendingKey(fvk))) {
+                    continue;
+                }
+            }
+
+            // skip locked notes
+             if (ignoreLocked && wallet.IsLockedNote(op)) {
+                 continue;
+             }
+
+            auto note = notePt.note(nd.ivk).get();
+            saplingEntries.push_back(SaplingNoteEntry {
+                op, pa, note, notePt.memo(), wtx.GetDepthInMainChain() });
+        }
+    }
+}
+
+namespace {
+
+// Heights 0..CHAIN_LENGTH-1, so a tx in the block at height h has depth CHAIN_LENGTH - h.
+const int CHAIN_LENGTH = 10;
+
+// The Jubjub subgroup order r_J. An ivk + r_J generates the same addresses as ivk,
+// because librustzcash_ivk_to_pkd multiplies by the raw 256-bit value.
+const char JUBJUB_R_HEX[] = "0e7db4ea6533afa906673b0101343b00a6682093ccc81082d0970e5ed6f72cb7";
+
+// Keys and addresses of the test wallet (see WalletScanNotes::BuildWallet).
+struct TestKeys {
+    libzcash::SaplingFullViewingKey fvk1, fvk2;
+    libzcash::SaplingIncomingViewingKey ivk1, ivk2, ivk3, ivk4, ivkN, aliasIvk1;
+    libzcash::SaplingPaymentAddress a1;      // K1 default address (spending key)
+    libzcash::SaplingPaymentAddress d1;      // K1 diversified address, recorded correctly
+    libzcash::SaplingPaymentAddress u1;      // K1 diversified address, never recorded
+    libzcash::SaplingPaymentAddress x1;      // K1 diversified address, recorded with K2's ivk
+    libzcash::SaplingPaymentAddress y1;      // K1 diversified address, recorded with ivk1 + r_J
+    libzcash::SaplingPaymentAddress a2;      // K2 default address (spending key)
+    libzcash::SaplingPaymentAddress a3;      // K3: incoming viewing key only
+    libzcash::SaplingPaymentAddress a4;      // K4: full viewing key only
+    libzcash::SaplingPaymentAddress aN;      // address of a non-canonical ivk (>= 2^251)
+    libzcash::SaplingPaymentAddress foreign; // not in the wallet at all
+    std::vector<libzcash::SaplingPaymentAddress> empty; // spending keys that hold no notes
+    size_t nNotes;
+};
+
+// One output of a fabricated tx that the wallet owns: the address it pays, the ivk the
+// wallet records for it (SaplingNoteData::ivk) and its cached nullifier, if any.
+struct OwnedOutput {
+    libzcash::SaplingPaymentAddress addr;
+    libzcash::SaplingIncomingViewingKey ivk;
+    uint64_t value;
+    boost::optional<uint256> nullifier;
+};
+
+struct NamedFilter {
+    std::string name;
+    std::set<libzcash::PaymentAddress> addrs;
+};
+
+// CCryptoKeyStore::EncryptKeys is protected, and CWallet::EncryptWallet needs a wallet file.
+class EncryptableWallet : public CWallet {
+public:
+    bool EncryptKeysForTest(CKeyingMaterial& vMasterKeyIn) { return EncryptKeys(vMasterKeyIn); }
+};
+
+libzcash::SaplingExtendedSpendingKey KeyFromSeedByte(unsigned char b)
+{
+    RawHDSeed raw(32, b);
+    HDSeed seed(raw);
+    return libzcash::SaplingExtendedSpendingKey::Master(seed);
+}
+
+// A diversified address of xsk other than its default one: the first valid diversifier
+// index at or after `start` (the default address uses the first one from 0).
+libzcash::SaplingPaymentAddress DiversifiedAddress(
+    const libzcash::SaplingExtendedSpendingKey& xsk, unsigned char start)
+{
+    libzcash::diversifier_index_t j;
+    j.begin()[0] = start;
+    return xsk.ToXFVK().Address(j).get().second;
+}
+
+// A synthetic nullifier. GetFilteredNotes only compares nullifiers, it never recomputes them.
+uint256 Nf(unsigned char n)
+{
+    uint256 nf;
+    nf.begin()[0] = n;
+    nf.begin()[31] = 0x4e;
+    return nf;
+}
+
+// A shielded output paying `value` to `addr`, encrypted the way a real sender does
+// (SaplingNotePlaintext::encrypt), with a random rcm and a memo derived from the value.
+// There is no proof, value commitment or outgoing ciphertext: the wallet reads only cm,
+// ephemeralKey and encCiphertext.
+OutputDescription MakeOutput(const libzcash::SaplingPaymentAddress& addr, uint64_t value)
+{
+    libzcash::SaplingNote note(addr, value);
+    std::array<unsigned char, HUSH_MEMO_SIZE> memo;
+    memo.fill(0);
+    memo[0] = value & 0xff;
+    memo[1] = (value >> 8) & 0xff;
+    libzcash::SaplingNotePlaintext pt(note, memo);
+    libzcash::SaplingNotePlaintextEncryptionResult res = pt.encrypt(note.pk_d).get();
+
+    OutputDescription od;
+    od.cv = uint256();
+    od.cm = note.cm().get();
+    od.ephemeralKey = res.second.get_epk();
+    od.encCiphertext = res.first;
+    od.outCiphertext.fill(0);
+    od.zkproof.fill(0);
+    return od;
+}
+
+std::set<libzcash::PaymentAddress> Filter(const std::vector<libzcash::SaplingPaymentAddress>& addrs)
+{
+    std::set<libzcash::PaymentAddress> s;
+    for (const libzcash::SaplingPaymentAddress& a : addrs) {
+        s.insert(a);
+    }
+    return s;
+}
+
+void ExpectSameEntries(const std::vector<SaplingNoteEntry>& expected,
+                       const std::vector<SaplingNoteEntry>& actual)
+{
+    ASSERT_EQ(expected.size(), actual.size());
+    for (size_t i = 0; i < expected.size(); i++) {
+        SCOPED_TRACE(testing::Message() << "entry " << i);
+        const SaplingNoteEntry& e = expected[i];
+        const SaplingNoteEntry& a = actual[i];
+        EXPECT_EQ(e.op.hash, a.op.hash);
+        EXPECT_EQ(e.op.n, a.op.n);
+        EXPECT_TRUE(e.address == a.address);
+        EXPECT_TRUE(e.note.d == a.note.d);
+        EXPECT_EQ(e.note.pk_d, a.note.pk_d);
+        EXPECT_EQ(e.note.value(), a.note.value());
+        EXPECT_EQ(e.note.r, a.note.r);
+        EXPECT_TRUE(e.memo == a.memo);
+        EXPECT_EQ(e.confirmations, a.confirmations);
+    }
+}
+
+// Runs the original and the optimised GetFilteredNotes with the same arguments and
+// expects the same entries in the same order. Returns the number of entries.
+size_t ExpectMatchesReference(CWallet& wallet, NamedFilter& filter,
+                              int minDepth, int maxDepth,
+                              bool ignoreSpent, bool requireSpendingKey, bool ignoreLocked)
+{
+    SCOPED_TRACE(testing::Message() << "filter=" << filter.name
+                 << " minDepth=" << minDepth << " maxDepth=" << maxDepth
+                 << " ignoreSpent=" << ignoreSpent
+                 << " requireSpendingKey=" << requireSpendingKey
+                 << " ignoreLocked=" << ignoreLocked);
+
+    std::vector<SaplingNoteEntry> expected;
+    std::vector<SaplingNoteEntry> actual;
+    RefGetFilteredNotes(wallet, expected, filter.addrs,
+                        minDepth, maxDepth, ignoreSpent, requireSpendingKey, ignoreLocked);
+    wallet.GetFilteredNotes(actual, filter.addrs,
+                            minDepth, maxDepth, ignoreSpent, requireSpendingKey, ignoreLocked);
+    ExpectSameEntries(expected, actual);
+    return expected.size();
+}
+
+// The reference's result size with maxDepth INT_MAX.
+size_t RefCount(CWallet& wallet, std::set<libzcash::PaymentAddress> filter, int minDepth,
+                bool ignoreSpent, bool requireSpendingKey, bool ignoreLocked)
+{
+    std::vector<SaplingNoteEntry> entries;
+    RefGetFilteredNotes(wallet, entries, filter,
+                        minDepth, INT_MAX, ignoreSpent, requireSpendingKey, ignoreLocked);
+    return entries.size();
+}
+
+// Every filter shape the pre-filter distinguishes.
+std::vector<NamedFilter> AllFilters(const TestKeys& k)
+{
+    // 16 recorded, verifiable addresses: the pre-filter is on (the cap is inclusive).
+    std::vector<libzcash::SaplingPaymentAddress> sixteen = {k.a1, k.d1, k.a2, k.a3, k.a4};
+    sixteen.insert(sixteen.end(), k.empty.begin(), k.empty.begin() + 11);
+    // 17: the pre-filter is off only because of the cap.
+    std::vector<libzcash::SaplingPaymentAddress> seventeen = sixteen;
+    seventeen.push_back(k.empty[11]);
+
+    std::set<libzcash::PaymentAddress> invalidOnly;
+    invalidOnly.insert(libzcash::InvalidEncoding());
+    std::set<libzcash::PaymentAddress> invalidAndA2 = invalidOnly;
+    invalidAndA2.insert(k.a2);
+
+    std::vector<NamedFilter> f;
+    f.push_back(NamedFilter{"{}", Filter({})});
+    f.push_back(NamedFilter{"{a1}", Filter({k.a1})});
+    f.push_back(NamedFilter{"{d1}", Filter({k.d1})});
+    f.push_back(NamedFilter{"{a2}", Filter({k.a2})});
+    f.push_back(NamedFilter{"{a3 ivk-only}", Filter({k.a3})});
+    f.push_back(NamedFilter{"{a4 fvk-only}", Filter({k.a4})});
+    f.push_back(NamedFilter{"{a1,a2,a3,a4,d1}", Filter({k.a1, k.a2, k.a3, k.a4, k.d1})});
+    f.push_back(NamedFilter{"{u1 unrecorded}", Filter({k.u1})});
+    f.push_back(NamedFilter{"{a1,u1 unrecorded}", Filter({k.a1, k.u1})});
+    f.push_back(NamedFilter{"{x1 mismatched}", Filter({k.x1})});
+    f.push_back(NamedFilter{"{x1 mismatched,a2}", Filter({k.x1, k.a2})});
+    f.push_back(NamedFilter{"{y1 aliased}", Filter({k.y1})});
+    f.push_back(NamedFilter{"{aN non-canonical}", Filter({k.aN})});
+    f.push_back(NamedFilter{"{no notes}", Filter({k.empty[0]})});
+    f.push_back(NamedFilter{"{foreign}", Filter({k.foreign})});
+    f.push_back(NamedFilter{"{invalid}", invalidOnly});
+    f.push_back(NamedFilter{"{invalid,a2}", invalidAndA2});
+    f.push_back(NamedFilter{"16 addresses", Filter(sixteen)});
+    f.push_back(NamedFilter{"17 addresses", Filter(seventeen)});
+    return f;
+}
+
+// decrypt_with_pk_d must succeed exactly when decrypt() does, and must leave pk_d_out
+// alone when it fails. Returns whether decryption succeeded.
+bool DecryptBothWays(const libzcash::SaplingEncCiphertext& ct, const uint256& ivk,
+                     const uint256& epk, const uint256& cmu)
+{
+    uint256 sentinel;
+    std::fill(sentinel.begin(), sentinel.end(), 0xa5);
+    uint256 pk_d = sentinel;
+    auto expected = libzcash::SaplingNotePlaintext::decrypt(ct, ivk, epk, cmu);
+    auto actual = libzcash::SaplingNotePlaintext::decrypt_with_pk_d(ct, ivk, epk, cmu, pk_d);
+    EXPECT_EQ(static_cast<bool>(expected), static_cast<bool>(actual));
+    if (!actual) {
+        EXPECT_EQ(pk_d, sentinel);
+    }
+    return static_cast<bool>(actual);
+}
+
+} // namespace
+
+class WalletScanNotes : public ::testing::Test {
+protected:
+    boost::filesystem::path datadir;
+    bool hadDatadirArg;
+    std::string oldDatadirArg;
+    bool oldPrintToDebugLog;
+    CBlockIndex* oldTip;
+    std::vector<std::unique_ptr<CBlockIndex>> blocks;
+    std::vector<CTransaction> mempoolTxs;
+
+    void SetUp() override
+    {
+        SelectParams(CBaseChainParams::REGTEST);
+
+        // GetFilteredNotes logs on every call: keep that away from any real datadir and
+        // out of debug.log.
+        hadDatadirArg = mapArgs.count("-datadir") > 0;
+        if (hadDatadirArg) {
+            oldDatadirArg = mapArgs["-datadir"];
+        }
+        datadir = boost::filesystem::temp_directory_path() / boost::filesystem::unique_path();
+        boost::filesystem::create_directories(datadir);
+        mapArgs["-datadir"] = datadir.string();
+        ClearDatadirCache();
+        oldPrintToDebugLog = fPrintToDebugLog;
+        fPrintToDebugLog = false;
+
+        // A fake active chain. Only what CMerkleTx::GetDepthInMainChain reads is set: the
+        // mapBlockIndex entry, phashBlock, the height and pprev.
+        LOCK(cs_main);
+        oldTip = chainActive.Tip();
+        for (int h = 0; h < CHAIN_LENGTH; h++) {
+            std::unique_ptr<CBlockIndex> pindex(new CBlockIndex());
+            pindex->SetHeight(h);
+            pindex->pprev = blocks.empty() ? NULL : blocks.back().get();
+            uint256 hash;
+            hash.begin()[0] = h;
+            hash.begin()[30] = 0x5c;
+            hash.begin()[31] = 0xa9;
+            auto ret = mapBlockIndex.insert(std::make_pair(hash, pindex.get()));
+            ASSERT_TRUE(ret.second);
+            pindex->phashBlock = &ret.first->first;
+            blocks.push_back(std::move(pindex));
+        }
+        chainActive.SetTip(blocks.back().get());
+    }
+
+    void TearDown() override
+    {
+        {
+            LOCK(cs_main);
+            for (const CTransaction& tx : mempoolTxs) {
+                std::list<CTransaction> removed;
+                mempool.remove(tx, removed, false);
+            }
+            mempoolTxs.clear();
+            chainActive.SetTip(oldTip);
+            for (const std::unique_ptr<CBlockIndex>& pindex : blocks) {
+                mapBlockIndex.erase(pindex->GetBlockHash());
+            }
+            blocks.clear();
+        }
+
+        fPrintToDebugLog = oldPrintToDebugLog;
+        if (hadDatadirArg) {
+            mapArgs["-datadir"] = oldDatadirArg;
+        } else {
+            mapArgs.erase("-datadir");
+        }
+        ClearDatadirCache();
+        boost::system::error_code ec;
+        boost::filesystem::remove_all(datadir, ec);
+
+        // The network every other test in this binary selects for itself.
+        SelectParams(CBaseChainParams::MAIN);
+    }
+
+    // Adds a fabricated tx to the wallet the way LoadWallet does: AddToWallet(wtx, true,
+    // NULL) skips the database and proof checks but fills the nullifier and spend maps.
+    // `owned` become outputs 0..n-1 with wallet note data, `foreign` outputs follow with
+    // none, and `spends` are the nullifiers the tx reveals. depth >= 1 puts the tx in the
+    // fake chain, 0 in the mempool, -1 nowhere (like a conflicted tx).
+    uint256 AddTx(CWallet& wallet, int depth,
+                  const std::vector<OwnedOutput>& owned,
+                  const std::vector<libzcash::SaplingPaymentAddress>& foreign,
+                  const std::vector<uint256>& spends,
+                  bool transparentOutput = false)
+    {
+        assert(depth >= -1 && depth <= CHAIN_LENGTH);
+
+        CMutableTransaction mtx;
+        mtx.fOverwintered = true;
+        mtx.nVersionGroupId = SAPLING_VERSION_GROUP_ID;
+        mtx.nVersion = SAPLING_TX_VERSION;
+        for (const OwnedOutput& o : owned) {
+            mtx.vShieldedOutput.push_back(MakeOutput(o.addr, o.value));
+        }
+        for (const libzcash::SaplingPaymentAddress& addr : foreign) {
+            mtx.vShieldedOutput.push_back(MakeOutput(addr, 1));
+        }
+        for (const uint256& nf : spends) {
+            SpendDescription sd;
+            sd.nullifier = nf;
+            sd.zkproof.fill(0);
+            sd.spendAuthSig.fill(0);
+            mtx.vShieldedSpend.push_back(sd);
+        }
+        if (transparentOutput) {
+            mtx.vout.push_back(CTxOut(1000, CScript() << OP_TRUE));
+        }
+        CTransaction tx(mtx);
+
+        CWalletTx wtx(&wallet, tx);
+        for (size_t i = 0; i < owned.size(); i++) {
+            SaplingNoteData nd(owned[i].ivk);
+            nd.nullifier = owned[i].nullifier;
+            wtx.mapSaplingNoteData[SaplingOutPoint(tx.GetHash(), i)] = nd;
+        }
+        if (depth >= 1) {
+            wtx.hashBlock = blocks[CHAIN_LENGTH - depth]->GetBlockHash();
+            wtx.nIndex = 0;
+            // There is no real block to check a merkle branch against.
+            wtx.fMerkleVerified = true;
+        } else if (depth == 0) {
+            mempool.addUnchecked(tx.GetHash(),
+                CTxMemPoolEntry(tx, 0, GetTime(), 0.0, chainActive.Height(), true, false, SAPLING_BRANCH_ID),
+                false);
+            mempoolTxs.push_back(tx);
+        }
+        wallet.AddToWallet(wtx, true, NULL);
+        return tx.GetHash();
+    }
+
+    // The test wallet. Every key state, every recorded-address state and every note state
+    // GetFilteredNotes distinguishes, spread over several depths:
+    //   K1 spending key: a1 (default), d1 (recorded diversified), u1 (unrecorded),
+    //      x1 (recorded with K2's ivk, as z_importviewingkey allows), y1 (recorded with
+    //      ivk1 + r_J, which still generates y1);
+    //   K2 spending key: a2;  K3 ivk only: a3;  K4 fvk only: a4;
+    //   ivkN: a non-canonical ivk in [2^251, r_J) and its address aN;
+    //   12 more spending keys with no notes; one foreign key.
+    // Notes are spent in a block, spent in the mempool, "spent" by a conflicted tx (so
+    // still unspent), unspent, without a cached nullifier although spent on chain, and
+    // locked. There are also txs without note data (spends only, transparent only, and
+    // a shielded output to a foreign address).
+    void BuildWallet(CWallet& wallet, TestKeys& k)
+    {
+        LOCK2(cs_main, wallet.cs_wallet);
+
+        libzcash::SaplingExtendedSpendingKey xsk1 = KeyFromSeedByte(0x11);
+        libzcash::SaplingExtendedSpendingKey xsk2 = KeyFromSeedByte(0x22);
+        libzcash::SaplingExtendedSpendingKey xsk3 = KeyFromSeedByte(0x33);
+        libzcash::SaplingExtendedSpendingKey xsk4 = KeyFromSeedByte(0x44);
+        libzcash::SaplingExtendedSpendingKey xskN = KeyFromSeedByte(0x55);
+
+        k.fvk1 = xsk1.expsk.full_viewing_key();
+        k.fvk2 = xsk2.expsk.full_viewing_key();
+        libzcash::SaplingFullViewingKey fvk4 = xsk4.expsk.full_viewing_key();
+        k.ivk1 = k.fvk1.in_viewing_key();
+        k.ivk2 = k.fvk2.in_viewing_key();
+        k.ivk3 = xsk3.expsk.full_viewing_key().in_viewing_key();
+        k.ivk4 = fvk4.in_viewing_key();
+
+        k.a1 = xsk1.DefaultAddress();
+        k.d1 = DiversifiedAddress(xsk1, 50);
+        k.u1 = DiversifiedAddress(xsk1, 100);
+        k.x1 = DiversifiedAddress(xsk1, 150);
+        k.y1 = DiversifiedAddress(xsk1, 200);
+        std::set<libzcash::SaplingPaymentAddress> k1addrs = {k.a1, k.d1, k.u1, k.x1, k.y1};
+        ASSERT_EQ(k1addrs.size(), (size_t)5);
+        k.a2 = xsk2.DefaultAddress();
+        k.a3 = xsk3.DefaultAddress();
+        k.a4 = xsk4.DefaultAddress();
+        k.foreign = KeyFromSeedByte(0x66).DefaultAddress();
+
+        // Genuine ivks are canonical (top 5 bits clear, so below 2^251).
+        ASSERT_EQ(k.ivk1.begin()[31] & 0xF8, 0);
+        ASSERT_EQ(k.ivk2.begin()[31] & 0xF8, 0);
+
+        // ivk1 + r_J generates exactly ivk1's addresses. If this ever stops holding, the
+        // y1 case no longer exercises the pre-filter's canonical-ivk guard.
+        k.aliasIvk1 = libzcash::SaplingIncomingViewingKey(ArithToUint256(
+            UintToArith256(k.ivk1) + UintToArith256(uint256S(JUBJUB_R_HEX))));
+        ASSERT_NE(k.aliasIvk1.begin()[31] & 0xF8, 0);
+        auto aliasY1 = k.aliasIvk1.address(k.y1.d);
+        ASSERT_TRUE(static_cast<bool>(aliasY1));
+        EXPECT_TRUE(aliasY1.get() == k.y1);
+
+        // ivkN in [2^251, 2^251 + 2^248), below r_J: ka_agree accepts it, so notes to aN
+        // decrypt normally, but it is not canonical.
+        uint256 rawN = xskN.expsk.full_viewing_key().in_viewing_key();
+        rawN.begin()[31] = 0x08;
+        k.ivkN = libzcash::SaplingIncomingViewingKey(rawN);
+        k.aN = k.ivkN.address(k.a1.d).get();
+
+        ASSERT_TRUE(wallet.AddSaplingZKey(xsk1, k.a1));
+        ASSERT_TRUE(wallet.AddSaplingIncomingViewingKey(k.ivk1, k.d1));
+        ASSERT_TRUE(wallet.AddSaplingZKey(xsk2, k.a2));
+        ASSERT_TRUE(wallet.AddSaplingIncomingViewingKey(k.ivk3, k.a3));
+        ASSERT_TRUE(wallet.AddSaplingFullViewingKey(fvk4, k.a4));
+        ASSERT_TRUE(wallet.AddSaplingIncomingViewingKey(k.ivk2, k.x1));
+        ASSERT_TRUE(wallet.AddSaplingIncomingViewingKey(k.aliasIvk1, k.y1));
+        ASSERT_TRUE(wallet.AddSaplingIncomingViewingKey(k.ivkN, k.aN));
+        for (int i = 0; i < 12; i++) {
+            libzcash::SaplingExtendedSpendingKey xsk = KeyFromSeedByte(0x70 + i);
+            libzcash::SaplingPaymentAddress addr = xsk.DefaultAddress();
+            ASSERT_TRUE(wallet.AddSaplingZKey(xsk, addr));
+            k.empty.push_back(addr);
+        }
+
+        const boost::optional<uint256> noNf;
+        std::vector<uint256> noSpends;
+        std::vector<libzcash::SaplingPaymentAddress> noForeign;
+
+        // Mixed ownership in one tx; Nf(2) is spent in a block.
+        AddTx(wallet, 5, {{k.a1, k.ivk1, 1001, Nf(1)},
+                          {k.a1, k.ivk1, 1002, Nf(2)},
+                          {k.a2, k.ivk2, 2001, Nf(3)}}, noForeign, noSpends);
+        // Nf(4) is spent in the mempool. The IVK-only note has no cached nullifier, but its
+        // on-chain nullifier Nf(5) is spent in a block: it must still count as unspent.
+        AddTx(wallet, 1, {{k.d1, k.ivk1, 1003, Nf(4)},
+                          {k.a3, k.ivk3, 3001, noNf}}, noForeign, noSpends);
+        // u1's note is locked; x1's note is "spent" only by a conflicted tx.
+        uint256 hashUX = AddTx(wallet, 3, {{k.u1, k.ivk1, 1004, Nf(6)},
+                                           {k.x1, k.ivk1, 1005, Nf(7)}}, noForeign, noSpends);
+        // In the mempool (depth 0).
+        AddTx(wallet, 0, {{k.a1, k.ivk1, 1006, Nf(8)},
+                          {k.a4, k.ivk4, 4001, Nf(9)}}, noForeign, noSpends);
+        // Conflicted (depth -1).
+        AddTx(wallet, -1, {{k.a2, k.ivk2, 2002, Nf(10)}}, noForeign, noSpends);
+        // Deepest block; Nf(11) is spent in a block.
+        AddTx(wallet, 10, {{k.y1, k.ivk1, 1007, Nf(11)},
+                           {k.aN, k.ivkN, 5001, noNf}}, noForeign, noSpends);
+        // a1's note is locked; one output goes to a foreign address (no note data).
+        uint256 hashLocked = AddTx(wallet, 2, {{k.a1, k.ivk1, 1008, Nf(12)},
+                                               {k.a2, k.ivk2, 2003, Nf(13)}},
+                                   {k.foreign}, noSpends);
+        // Nf(14) is spent in a block.
+        AddTx(wallet, 4, {{k.a3, k.ivk3, 3002, noNf},
+                          {k.aN, k.ivkN, 5002, Nf(14)},
+                          {k.x1, k.ivk1, 1009, Nf(15)}}, noForeign, noSpends);
+        AddTx(wallet, 1, {{k.d1, k.ivk1, 1010, Nf(16)}}, noForeign, noSpends);
+        k.nNotes = 18;
+
+        // Txs without note data: the spenders (in a block, in the mempool, conflicted), a
+        // transparent-only tx and a tx whose only output pays a foreign address.
+        AddTx(wallet, 2, {}, noForeign, {Nf(2), Nf(5), Nf(11), Nf(14)});
+        AddTx(wallet, 0, {}, noForeign, {Nf(4)});
+        AddTx(wallet, -1, {}, noForeign, {Nf(7)});
+        AddTx(wallet, 3, {}, noForeign, noSpends, true);
+        AddTx(wallet, 1, {}, {k.foreign}, noSpends);
+
+        wallet.LockNote(SaplingOutPoint(hashUX, 0));
+        wallet.LockNote(SaplingOutPoint(hashLocked, 0));
+    }
+};
+
+// The fixture really has what the equivalence tests rely on: every note is visible, the
+// spent, depth, key and lock states are the intended ones, and the regression cases
+// (unrecorded, mismatched, aliased and non-canonical addresses) hold notes, so equivalence
+// on them is not vacuous.
+TEST_F(WalletScanNotes, ReferenceSeesTheFixture)
+{
+    CWallet wallet;
+    TestKeys k;
+    ASSERT_NO_FATAL_FAILURE(BuildWallet(wallet, k));
+
+    std::set<libzcash::PaymentAddress> none;
+    std::vector<SaplingNoteEntry> all;
+    RefGetFilteredNotes(wallet, all, none, -1, INT_MAX, false, false, false);
+    EXPECT_EQ(all.size(), k.nNotes);
+    std::set<int> depths;
+    for (const SaplingNoteEntry& e : all) {
+        depths.insert(e.confirmations);
+    }
+    EXPECT_EQ(depths, (std::set<int>{-1, 0, 1, 2, 3, 4, 5, 10}));
+
+    // Spent: Nf(2), Nf(11) and Nf(14) in a block, Nf(4) in the mempool. Not spent: Nf(7)
+    // (conflicted spender) and the IVK-only note without a cached nullifier.
+    EXPECT_EQ(RefCount(wallet, none, -1, true, false, false), k.nNotes - 4);
+    // minDepth 0 drops the conflicted tx (1 note), minDepth 1 also the mempool tx (2).
+    EXPECT_EQ(RefCount(wallet, none, 0, false, false, false), k.nNotes - 1);
+    EXPECT_EQ(RefCount(wallet, none, 1, false, false, false), k.nNotes - 3);
+    EXPECT_EQ(RefCount(wallet, none, -1, false, false, true), k.nNotes - 2);
+    // Spending key reachable through the recorded address: a1 (4), d1 (2), a2 (3), and
+    // x1 (2), because x1 is recorded with K2's ivk and the wallet has K2's spending key.
+    EXPECT_EQ(RefCount(wallet, none, -1, false, true, false), (size_t)11);
+
+    EXPECT_EQ(RefCount(wallet, Filter({k.a1}), -1, false, false, false), (size_t)4);
+    EXPECT_EQ(RefCount(wallet, Filter({k.d1}), -1, false, false, false), (size_t)2);
+    EXPECT_EQ(RefCount(wallet, Filter({k.u1}), -1, false, false, false), (size_t)1);
+    EXPECT_EQ(RefCount(wallet, Filter({k.x1}), -1, false, false, false), (size_t)2);
+    EXPECT_EQ(RefCount(wallet, Filter({k.y1}), -1, false, false, false), (size_t)1);
+    EXPECT_EQ(RefCount(wallet, Filter({k.a2}), -1, false, false, false), (size_t)3);
+    EXPECT_EQ(RefCount(wallet, Filter({k.a3}), -1, false, false, false), (size_t)2);
+    EXPECT_EQ(RefCount(wallet, Filter({k.a4}), -1, false, false, false), (size_t)1);
+    EXPECT_EQ(RefCount(wallet, Filter({k.aN}), -1, false, false, false), (size_t)2);
+    EXPECT_EQ(RefCount(wallet, Filter({k.empty[0]}), -1, false, false, false), (size_t)0);
+    EXPECT_EQ(RefCount(wallet, Filter({k.foreign}), -1, false, false, false), (size_t)0);
+}
+
+// Every filter shape x ignoreSpent x requireSpendingKey x ignoreLocked x minDepth {0, 1}.
+TEST_F(WalletScanNotes, MatchesReferenceAcrossFilters)
+{
+    CWallet wallet;
+    TestKeys k;
+    ASSERT_NO_FATAL_FAILURE(BuildWallet(wallet, k));
+
+    std::vector<NamedFilter> filters = AllFilters(k);
+    size_t total = 0;
+    for (NamedFilter& f : filters) {
+        for (int minDepth : {0, 1}) {
+            for (bool ignoreSpent : {false, true}) {
+                for (bool requireSpendingKey : {false, true}) {
+                    for (bool ignoreLocked : {false, true}) {
+                        total += ExpectMatchesReference(wallet, f, minDepth, INT_MAX,
+                                                        ignoreSpent, requireSpendingKey, ignoreLocked);
+                    }
+                }
+            }
+        }
+    }
+    EXPECT_GT(total, (size_t)0);
+}
+
+// The depth window: minDepth {-1, 0, 1} x maxDepth {0, 1, 3, INT_MAX} x the three flags,
+// unfiltered and with a single (pre-filtered) address.
+TEST_F(WalletScanNotes, MatchesReferenceAcrossDepths)
+{
+    CWallet wallet;
+    TestKeys k;
+    ASSERT_NO_FATAL_FAILURE(BuildWallet(wallet, k));
+
+    std::vector<NamedFilter> filters;
+    filters.push_back(NamedFilter{"{}", Filter({})});
+    filters.push_back(NamedFilter{"{a1}", Filter({k.a1})});
+    size_t total = 0;
+    for (NamedFilter& f : filters) {
+        for (int minDepth : {-1, 0, 1}) {
+            for (int maxDepth : {0, 1, 3, INT_MAX}) {
+                for (bool ignoreSpent : {false, true}) {
+                    for (bool requireSpendingKey : {false, true}) {
+                        for (bool ignoreLocked : {false, true}) {
+                            total += ExpectMatchesReference(wallet, f, minDepth, maxDepth,
+                                                            ignoreSpent, requireSpendingKey, ignoreLocked);
+                        }
+                    }
+                }
+            }
+        }
+    }
+    EXPECT_GT(total, (size_t)0);
+}
+
+// The single-address string overload (z_getbalance and friends): it decodes the address
+// and calls the set overload with maxDepth INT_MAX and ignoreLocked true.
+TEST_F(WalletScanNotes, MatchesReferenceForAddressString)
+{
+    CWallet wallet;
+    TestKeys k;
+    ASSERT_NO_FATAL_FAILURE(BuildWallet(wallet, k));
+
+    std::vector<std::string> addresses = {
+        "",
+        EncodePaymentAddress(k.a1),
+        EncodePaymentAddress(k.u1),
+        EncodePaymentAddress(k.x1),
+        EncodePaymentAddress(k.foreign),
+        "not-an-address",
+    };
+    size_t total = 0;
+    for (const std::string& address : addresses) {
+        for (int minDepth : {0, 1}) {
+            for (bool ignoreSpent : {false, true}) {
+                for (bool requireSpendingKey : {false, true}) {
+                    SCOPED_TRACE(testing::Message() << "address=\"" << address << "\""
+                                 << " minDepth=" << minDepth << " ignoreSpent=" << ignoreSpent
+                                 << " requireSpendingKey=" << requireSpendingKey);
+                    std::set<libzcash::PaymentAddress> filter;
+                    if (address.length() > 0) {
+                        filter.insert(DecodePaymentAddress(address));
+                    }
+                    std::vector<SaplingNoteEntry> expected;
+                    std::vector<SaplingNoteEntry> actual;
+                    RefGetFilteredNotes(wallet, expected, filter,
+                                        minDepth, INT_MAX, ignoreSpent, requireSpendingKey, true);
+                    wallet.GetFilteredNotes(actual, address, minDepth, ignoreSpent, requireSpendingKey);
+                    ExpectSameEntries(expected, actual);
+                    total += expected.size();
+                }
+            }
+        }
+    }
+    EXPECT_GT(total, (size_t)0);
+}
+
+// An encrypted, locked wallet: crypted spending keys still satisfy requireSpendingKey and
+// the ivk/fvk maps are not encrypted, so both implementations must still agree.
+TEST_F(WalletScanNotes, MatchesReferenceWhenEncryptedAndLocked)
+{
+    EncryptableWallet wallet;
+    TestKeys k;
+    ASSERT_NO_FATAL_FAILURE(BuildWallet(wallet, k));
+
+    CKeyingMaterial masterKey(WALLET_CRYPTO_KEY_SIZE, 0x5a);
+    ASSERT_TRUE(wallet.EncryptKeysForTest(masterKey));
+    ASSERT_TRUE(wallet.Lock());
+    ASSERT_TRUE(wallet.IsCrypted());
+    ASSERT_TRUE(wallet.IsLocked());
+    EXPECT_TRUE(wallet.HaveSaplingSpendingKey(k.fvk1));
+
+    std::vector<NamedFilter> filters = AllFilters(k);
+    size_t total = 0;
+    size_t spendable = 0;
+    for (NamedFilter& f : filters) {
+        for (bool ignoreSpent : {false, true}) {
+            for (bool requireSpendingKey : {false, true}) {
+                for (bool ignoreLocked : {false, true}) {
+                    size_t n = ExpectMatchesReference(wallet, f, 0, INT_MAX,
+                                                      ignoreSpent, requireSpendingKey, ignoreLocked);
+                    total += n;
+                    if (requireSpendingKey) {
+                        spendable += n;
+                    }
+                }
+            }
+        }
+    }
+    EXPECT_GT(total, (size_t)0);
+    EXPECT_GT(spendable, (size_t)0);
+}
+
+// decrypt_with_pk_d returns the same plaintext as decrypt() and the pk_d that
+// ivk.address(d) recomputes, and fails exactly when decrypt() fails.
+TEST_F(WalletScanNotes, DecryptWithPkDMatchesDecrypt)
+{
+    CWallet wallet;
+    TestKeys k;
+    ASSERT_NO_FATAL_FAILURE(BuildWallet(wallet, k));
+
+    LOCK2(cs_main, wallet.cs_wallet);
+    size_t checked = 0;
+    for (const auto& p : wallet.mapWallet) {
+        const CWalletTx& wtx = p.second;
+        for (const auto& item : wtx.mapSaplingNoteData) {
+            SCOPED_TRACE(testing::Message() << "note " << item.first.ToString());
+            const OutputDescription& od = wtx.vShieldedOutput[item.first.n];
+            const libzcash::SaplingIncomingViewingKey& ivk = item.second.ivk;
+
+            uint256 pk_d;
+            auto expected = libzcash::SaplingNotePlaintext::decrypt(
+                od.encCiphertext, ivk, od.ephemeralKey, od.cm);
+            auto actual = libzcash::SaplingNotePlaintext::decrypt_with_pk_d(
+                od.encCiphertext, ivk, od.ephemeralKey, od.cm, pk_d);
+            ASSERT_TRUE(static_cast<bool>(expected));
+            ASSERT_TRUE(static_cast<bool>(actual));
+            EXPECT_TRUE(actual->d == expected->d);
+            EXPECT_EQ(actual->value(), expected->value());
+            EXPECT_EQ(actual->rcm, expected->rcm);
+            EXPECT_TRUE(actual->memo() == expected->memo());
+            auto addr = ivk.address(actual->d);
+            ASSERT_TRUE(static_cast<bool>(addr));
+            EXPECT_EQ(pk_d, addr->pk_d);
+            checked++;
+
+            // Wrong ivk: the AEAD fails.
+            const libzcash::SaplingIncomingViewingKey& wrongIvk = (ivk == k.ivk2) ? k.ivk1 : k.ivk2;
+            EXPECT_FALSE(DecryptBothWays(od.encCiphertext, wrongIvk, od.ephemeralKey, od.cm));
+            // Wrong cmu: decryption and ivk_to_pkd succeed, the commitment check fails, so
+            // pk_d has been computed but must not reach pk_d_out.
+            uint256 badCm = od.cm;
+            badCm.begin()[0] ^= 1;
+            EXPECT_FALSE(DecryptBothWays(od.encCiphertext, ivk, od.ephemeralKey, badCm));
+            // Tampered ciphertext.
+            libzcash::SaplingEncCiphertext badCt = od.encCiphertext;
+            badCt[0] ^= 1;
+            EXPECT_FALSE(DecryptBothWays(badCt, ivk, od.ephemeralKey, od.cm));
+            // ivk1 + r_J: whatever librustzcash decides for a non-canonical scalar, both agree.
+            if (ivk == k.ivk1) {
+                DecryptBothWays(od.encCiphertext, k.aliasIvk1, od.ephemeralKey, od.cm);
+            }
+        }
+    }
+    EXPECT_EQ(checked, k.nNotes);
+}

src/httpserver.cpp

@@ -420,10 +420,6 @@ static void libevent_log_cb(int severity, const char *msg)
         LogPrint("libevent", "libevent: %s\n", msg);
 }
 
-/** Cap on the combined size of an HTTP request line + headers. libevent's default is EV_SIZE_MAX,
- *  i.e. unbounded, and it buffers before any ACL or auth check runs. */
-static const size_t MAX_HEADERS_SIZE = 8192;
-
 bool InitHTTPServer()
 {
     struct evhttp* http = 0;
@@ -471,10 +467,6 @@ bool InitHTTPServer()
 
     evhttp_set_timeout(http, GetArg("-rpcservertimeout", DEFAULT_HTTP_SERVER_TIMEOUT));
     evhttp_set_max_body_size(http, MAX_SIZE);
-    // libevent defaults max_headers_size to EV_SIZE_MAX, so without this a single connection can
-    // stream an unbounded request line / header block and grow RSS ~1:1 with bytes sent, BEFORE the
-    // -rpcallowip ACL or auth check runs (both happen after libevent has parsed the request).
-    evhttp_set_max_headers_size(http, MAX_HEADERS_SIZE);
     evhttp_set_gencb(http, http_request_cb, NULL);
 
     if (!HTTPBindAddresses(http)) {

src/main.cpp

@@ -8336,13 +8336,20 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
         // Message: addr
         if (fSendTrickle)
         {
-            // Accumulate into vAddr and send it ONCE (or in MAX_ADDR_TO_SEND-sized batches).
-            // This loop previously pushed the ENTIRE pto->vAddrToSend on every accepted address,
-            // so a single 24-byte getaddr produced N messages of N addresses each instead of one
-            // message of N -- ~500x the intended bandwidth on a typical addrman, all serialized
-            // (with per-message double-SHA256 checksums) while cs_main is held. The locally built
-            // vAddr was accumulated and then discarded, and the vAddr.resize(MAX_ADDR_TO_SEND) was
-            // a no-op standing where upstream has vAddr.clear().
+            vector<CAddress> vAddr;
+            vAddr.reserve(pto->vAddrToSend.size());
+            BOOST_FOREACH(const CAddress& addr, pto->vAddrToSend)
+            {
+                if (pto->AddAddressIfNotAlreadyKnown(addr))
+                {
+                    vAddr.push_back(addr);
+
+                    if (vAddr.size() >= MAX_ADDR_TO_SEND)
+                    {
+                      // Should be impossible since we always check size before adding to
+                      // vAddrToSend. Recover by trimming the vector.
+                      vAddr.resize(MAX_ADDR_TO_SEND);
+                    }
                     const char* msg_type;
                     int make_flags;
                     if (pto->m_wants_addrv2) {
@@ -8352,26 +8359,13 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
                         msg_type = NetMsgType::ADDR;
                         make_flags = 0;
                     }
-            const CNetMsgMaker msgMaker(std::min(pto->nVersion, PROTOCOL_VERSION));
+                    pto->PushAddrMessage(CNetMsgMaker(std::min(pto->nVersion, PROTOCOL_VERSION)).Make(make_flags, msg_type, pto->vAddrToSend));
 
-            vector<CAddress> vAddr;
-            vAddr.reserve(pto->vAddrToSend.size());
-            BOOST_FOREACH(const CAddress& addr, pto->vAddrToSend)
-            {
-                if (pto->AddAddressIfNotAlreadyKnown(addr))
-                {
-                    vAddr.push_back(addr);
-                    if (vAddr.size() >= MAX_ADDR_TO_SEND)
-                    {
-                        pto->PushAddrMessage(msgMaker.Make(make_flags, msg_type, vAddr));
-                        vAddr.clear();
-                    }
                 }
             }
 
             pto->vAddrToSend.clear();
-            if (!vAddr.empty())
-                pto->PushAddrMessage(msgMaker.Make(make_flags, msg_type, vAddr));
+            vAddr.clear();
         }
 
         CNodeState &state = *State(pto->GetId());

src/net.cpp

@@ -2951,6 +2951,16 @@ void CNode::PushAddrMessage(CSerializedNetMsg&& msg)
     }
 }
 
+void GetNodeCounts(int& nAll, int& nTls)
+{
+    LOCK(cs_vNodes);
+    nAll = (int)vNodes.size();
+    nTls = 0;
+    for (const CNode* n : vNodes)
+        if (n->ssl != NULL)
+            nTls++;
+}
+
 size_t GetNodeCount(NumConnections flags)
 {
     LOCK(cs_vNodes);

src/net.h

@@ -165,6 +165,7 @@ enum NumConnections {
 };
 
 size_t GetNodeCount(NumConnections num);
+void GetNodeCounts(int& nAll, int& nTls);
 
 bool GetNetworkActive();
 void SetNetworkActive(bool active);
@@ -607,11 +608,7 @@ public:
         // Known checking here is only to save space from duplicates.
         // SendMessages will filter it again for knowns that were added
         // after addresses were pushed.
-        // NOTE: _addr (the address being queued), NOT addr (this peer's own address, net.h ~416).
-        // Testing the member made the filter constant for the connection's lifetime: once the peer's
-        // own address entered its addrKnown -- routine, via the remote's AdvertizeLocal -- every
-        // relay path to it silently no-opped until the daily addrKnown.reset().
-        if (_addr.IsValid() && !IsAddressKnown(_addr) && addr_format_supported) {
+        if (_addr.IsValid() && !IsAddressKnown(addr) && addr_format_supported) {
 
             if (vAddrToSend.size() >= MAX_ADDR_TO_SEND) {
                 vAddrToSend[insecure_rand() % vAddrToSend.size()] = _addr;

src/rpc/blockchain.cpp

@@ -534,10 +534,6 @@ UniValue getblockdeltas(const UniValue& params, bool fHelp, const CPubKey& mypk)
     if (fHelp || params.size() != 1)
         throw runtime_error("");
 
-    // Reads mapBlockIndex / chainActive (and, below, pcoinsTip's mutable anchor cache),
-    // all of which are cs_main-guarded. Every sibling RPC in this file locks; this one did not.
-    LOCK(cs_main);
-
     std::string strHash = params[0].get_str();
     uint256 hash(uint256S(strHash));
 
@@ -606,20 +602,12 @@ UniValue getblockhashes(const UniValue& params, bool fHelp, const CPubKey& mypk)
 
     std::vector<std::pair<uint256, unsigned int> > blockHashes;
 
-    {
-        // The lock must SPAN GetTimestampIndex: with fActiveOnly it calls blockOnchainActive() for
-        // every row, which reads mapBlockIndex and chainActive. The previous form was
-        //     if (fActiveOnly)
-        //         LOCK(cs_main);
-        // and LOCK() declares a scoped object, so as an unbraced substatement it was constructed
-        // and destroyed on that line -- the walk then ran completely unsynchronised. Taken
-        // unconditionally here: this RPC is explorer-only and not hot, and a conditional lock is
-        // exactly the shape that produced the bug.
+    if (fActiveOnly)
         LOCK(cs_main);
+
     if (!GetTimestampIndex(high, low, fActiveOnly, blockHashes)) {
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for block hashes");
     }
-    }
 
     UniValue result(UniValue::VARR);
 
@@ -889,10 +877,6 @@ UniValue getblockmerkletree(const UniValue& params, bool fHelp, const CPubKey& m
             + HelpExampleRpc("getblockmerkletree", "290000")
         );
 
-    // Reads mapBlockIndex / chainActive (and, below, pcoinsTip's mutable anchor cache),
-    // all of which are cs_main-guarded. Every sibling RPC in this file locks; this one did not.
-    LOCK(cs_main);
-
     CBlockIndex* phushblockindex;
     uint256 blockRoot;
     SaplingMerkleTree tree;

src/rpc/mining.cpp

@@ -759,25 +759,9 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp, const CPubKey& myp
 #ifdef ENABLE_WALLET
         CReserveKey reservekey(pwalletMain);
         LEAVE_CRITICAL_SECTION(cs_main);
-        // MUST re-enter cs_main before letting an exception escape. The enclosing LOCK(cs_main) is
-        // a scoped CMutexLock whose owns_lock is still true, so if CreateNewBlockWithKey throws
-        // (any wallet/BDB fault: disk full, EMFILE, a corrupt wallet.dat) its destructor unlocks an
-        // already-unlocked mutex during unwinding -> BOOST_VERIFY -> SIGABRT. Asserts cannot be
-        // compiled out here (main.cpp #errors on NDEBUG), so this aborts the daemon instead of
-        // returning the actionable error, and the abort happens inside unwinding so nothing is logged.
-        try {
         pblocktemplate = CreateNewBlockWithKey(reservekey,pindexPrevNew->GetHeight()+1,HUSH_MAXGPUCOUNT,false);
-        } catch (...) {
-            ENTER_CRITICAL_SECTION(cs_main);
-            throw;
-        }
 #else
-        try {
         pblocktemplate = CreateNewBlockWithKey();
-        } catch (...) {
-            ENTER_CRITICAL_SECTION(cs_main);
-            throw;
-        }
 #endif
         ENTER_CRITICAL_SECTION(cs_main);
         if (!pblocktemplate)

src/rpc/misc.cpp

@@ -132,6 +132,10 @@ UniValue getinfo(const UniValue& params, bool fHelp, const CPubKey& mypk)
             + HelpExampleRpc("getinfo", "")
         );
 
+    // C268: snapshot peer counts under cs_vNodes (sequentially, before the cs_main/cs_wallet lock)
+    int nAllConns = 0, nTlsConns = 0;
+    GetNodeCounts(nAllConns, nTlsConns);
+
 #ifdef ENABLE_WALLET
     LOCK2(cs_main, pwalletMain ? &pwalletMain->cs_wallet : NULL);
 #else
@@ -175,8 +179,8 @@ UniValue getinfo(const UniValue& params, bool fHelp, const CPubKey& mypk)
     }
     //obj.push_back(Pair("build_date", BUILD_DATE));
     obj.push_back(Pair("timeoffset",    0));
-    obj.push_back(Pair("connections",   (int)vNodes.size()));
-    obj.push_back(Pair("tls_connections", (int)std::count_if(vNodes.begin(), vNodes.end(), [](CNode* n) {return n->ssl != NULL;})));
+    obj.push_back(Pair("connections",   nAllConns));
+    obj.push_back(Pair("tls_connections", nTlsConns));
     obj.push_back(Pair("proxy",         (proxy.IsValid() ? proxy.proxy.ToStringIPPort() : string())));
     obj.push_back(Pair("testnet",       Params().TestnetToBeDeprecatedFieldRPC()));
     obj.push_back(Pair("relayfee",      ValueFromAmount(::minRelayTxFee.GetFeePerK())));

src/rpc/net.cpp

@@ -533,6 +533,9 @@ UniValue getnetworkinfo(const UniValue& params, bool fHelp, const CPubKey& mypk)
             + HelpExampleRpc("getnetworkinfo", "")
         );
 
+    int nAllConns = 0, nTlsConns = 0;
+    GetNodeCounts(nAllConns, nTlsConns);
+
     LOCK(cs_main);
 
     UniValue obj(UniValue::VOBJ);
@@ -541,8 +544,8 @@ UniValue getnetworkinfo(const UniValue& params, bool fHelp, const CPubKey& mypk)
     obj.push_back(Pair("protocolversion",PROTOCOL_VERSION));
     obj.push_back(Pair("localservices",       strprintf("%016x", nLocalServices)));
     obj.push_back(Pair("timeoffset",    0));
-    obj.push_back(Pair("connections",   (int)vNodes.size()));
-    obj.push_back(Pair("tls_connections", (int)std::count_if(vNodes.begin(), vNodes.end(), [](CNode* n) {return n->ssl != NULL;})));
+    obj.push_back(Pair("connections",   nAllConns));
+    obj.push_back(Pair("tls_connections", nTlsConns));
     obj.push_back(Pair("networks",      GetNetworksInfo()));
     obj.push_back(Pair("relayfee",      ValueFromAmount(::minRelayTxFee.GetFeePerK())));
     UniValue localAddresses(UniValue::VARR);

src/wallet/rpcdump.cpp

@@ -299,10 +299,7 @@ UniValue importprivkey(const UniValue& params, bool fHelp, const CPubKey& mypk)
     bool fRescan = true;
     if (params.size() > 2)
         fRescan = params[2].get_bool();
-    // '> 3', not '== 4': with the optional 5th (secret_key) argument present the equality test
-    // failed and height silently stayed 0, rescanning from genesis. Every sibling RPC in this file
-    // already uses the '>' form.
-    if ( fRescan && params.size() > 3 )
+    if ( fRescan && params.size() == 4 )
         height = params[3].get_int();
 
 

src/wallet/rpchushwallet.cpp

@@ -11,11 +11,72 @@
 #include "utilmoneystr.h"
 #include <utf8.h>
 
+#include <map>
+#include <set>
+#include <utility>
+#include <vector>
+
 using namespace std;
 using namespace libzcash;
 
 bool EnsureWalletIsAvailable(bool avoidException);
 
+const std::vector<ZsTxJSONContext::Key>& ZsTxJSONContext::keys()
+{
+    if (!keysBuilt) {
+        // The calls, order and filter of the per-output key loops this replaces.
+        std::set<libzcash::SaplingPaymentAddress> addresses;
+        pwalletMain->GetSaplingPaymentAddresses(addresses);
+        for (std::set<libzcash::SaplingPaymentAddress>::const_iterator it = addresses.begin(); it != addresses.end(); ++it) {
+            libzcash::SaplingExtendedSpendingKey extsk;
+            if (!pwalletMain->GetSaplingExtendedSpendingKey(*it, extsk))
+                continue;   // viewing-key-only, or encrypted and locked: skipped, as before
+            libzcash::SaplingFullViewingKey fvk = extsk.expsk.full_viewing_key();
+            Key key;
+            key.addr = *it;
+            key.ivk = fvk.in_viewing_key();
+            key.ovk = fvk.ovk;
+            vKeys.push_back(key);
+        }
+        keysBuilt = true;
+    }
+    return vKeys;
+}
+
+bool ZsTxJSONContext::hasNullifier(const libzcash::SaplingPaymentAddress& addr, const uint256& nullifier)
+{
+    if (!nullifiersBuilt) {
+        // The population GetNullifiersForAddresses() scans: every mapWallet note that has a nullifier.
+        for (std::map<uint256, CWalletTx>::const_iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it) {
+            for (mapSaplingNoteData_t::const_iterator nit = it->second.mapSaplingNoteData.begin(); nit != it->second.mapSaplingNoteData.end(); ++nit) {
+                if (nit->second.nullifier)
+                    nfByIvk.insert(std::make_pair(nit->second.ivk, nit->second.nullifier.get()));
+            }
+        }
+        nullifiersBuilt = true;
+    }
+    // GetNullifiersForAddresses() ignores the result of this lookup, so an address the wallet does not
+    // know is matched against the default (null) ivk. Keep that.
+    libzcash::SaplingIncomingViewingKey ivk;
+    pwalletMain->GetSaplingIncomingViewingKey(addr, ivk);
+    return nfByIvk.count(std::make_pair(ivk, nullifier)) > 0;
+}
+
+bool ZsTxJSONContext::seedOvk(uint256& ovkOut)
+{
+    if (!seedOvkTried) {
+        HDSeed seed;
+        if (pwalletMain->GetHDSeedForDerivation(seed)) {
+            seedOvkValue = ovkForShieldingFromTaddr(seed);
+            seedOvkOk = true;
+        }
+        seedOvkTried = true;
+    }
+    if (seedOvkOk)
+        ovkOut = seedOvkValue;
+    return seedOvkOk;
+}
+
 void zsTxSpendsToJSON(const CWalletTx& wtx, UniValue& spends, CAmount& totalSpends, CAmount& filteredSpends, const std::string& strAddress, bool filterByAddress) {
 
   LOCK2(cs_main, pwalletMain->cs_wallet);
@@ -87,12 +148,16 @@ void zsTxSpendsToJSON(const CWalletTx& wtx, UniValue& spends, CAmount& totalSpen
       const SpendDescription& spendDesc = wtx.vShieldedSpend[i];
       UniValue obj(UniValue::VOBJ);
 
-      SaplingOutPoint op = pwalletMain->mapSaplingNullifiersToNotes[spendDesc.nullifier];
+      std::map<uint256, SaplingOutPoint>::const_iterator nfIt = pwalletMain->mapSaplingNullifiersToNotes.find(spendDesc.nullifier);
 
-      if (pwalletMain->IsSaplingNullifierFromMe(spendDesc.nullifier)) {
-        const CWalletTx* parent = pwalletMain->GetWalletTx(pwalletMain->mapSaplingNullifiersToNotes[spendDesc.nullifier].hash);
+      // Same test as IsSaplingNullifierFromMe(), but find() so a read-only RPC never inserts into wallet maps.
+      if (nfIt != pwalletMain->mapSaplingNullifiersToNotes.end() && pwalletMain->mapWallet.count(nfIt->second.hash)) {
+        const SaplingOutPoint op = nfIt->second;
+        const CWalletTx* parent = pwalletMain->GetWalletTx(op.hash);
+        mapSaplingNoteData_t::const_iterator ndIt = parent->mapSaplingNoteData.find(op);
+        if (ndIt == parent->mapSaplingNoteData.end()) continue;  // stale entry: old code inserted an empty note that never decrypts
         const OutputDescription& output = parent->vShieldedOutput[op.n];
-        auto nd = pwalletMain->mapWallet[pwalletMain->mapSaplingNullifiersToNotes[spendDesc.nullifier].hash].mapSaplingNoteData[op];
+        const SaplingNoteData& nd = ndIt->second;
         auto pt = libzcash::SaplingNotePlaintext::decrypt(output.encCiphertext,nd.ivk,output.ephemeralKey,output.cm);
 
         if (pt) {
@@ -132,13 +197,16 @@ void zsTxSpendsToJSON(const CWalletTx& wtx, UniValue& spends, CAmount& totalSpen
 }
 
 
-void zsTxReceivedToJSON(const CWalletTx& wtx, UniValue& received, CAmount& totalReceived, const std::string& strAddress, bool filterByAddress) {
+void zsTxReceivedToJSON(const CWalletTx& wtx, UniValue& received, CAmount& totalReceived, const std::string& strAddress, bool filterByAddress, ZsTxJSONContext* ctx) {
 
   if(fZdebug)
     fprintf(stderr,"%s: txid=%s\n", __func__, wtx.GetHash().ToString().c_str() );
 
   LOCK2(cs_main, pwalletMain->cs_wallet);
 
+  ZsTxJSONContext localCtx;   // used only when the caller passed no per-call memo
+  ZsTxJSONContext& zctx = ctx ? *ctx : localCtx;
+
   //Check address
   bool isTAddress = false;
   bool isZsAddress = false;
@@ -192,13 +260,36 @@ void zsTxReceivedToJSON(const CWalletTx& wtx, UniValue& received, CAmount& total
       UniValue obj(UniValue::VOBJ);
       bool changeTx = false;
       //Decrypt sapling incoming commitments using IVK
-      std::set<libzcash::SaplingPaymentAddress> addresses;
-      pwalletMain->GetSaplingPaymentAddresses(addresses);
-      for (auto addr : addresses) {
-        libzcash::SaplingExtendedSpendingKey extsk;
-        if (pwalletMain->GetSaplingExtendedSpendingKey(addr, extsk)) {
+      const std::vector<ZsTxJSONContext::Key>& keys = zctx.keys();
+      // An output opens under at most one ivk (decrypt() checks the note commitment, which binds
+      // pk_d = [ivk]g_d), and FindMySaplingNotes records that ivk in mapSaplingNoteData. When the recorded
+      // ivk is one of keys(), pass 0 tries only the keys with that ivk: every other key would fail, as in
+      // the old single loop. If pass 0 opens nothing (stale note data), pass 1 tries the remaining keys in
+      // the same order. Without a usable record only pass 1 runs, over every key, exactly as before.
+      libzcash::SaplingIncomingViewingKey recordedIvk;
+      bool narrow = false;
+      {
+        mapSaplingNoteData_t::const_iterator ndIt = wtx.mapSaplingNoteData.find(SaplingOutPoint(wtx.GetHash(), i));
+        if (ndIt != wtx.mapSaplingNoteData.end()) {
+          for (size_t k = 0; k < keys.size() && !narrow; k++) {
+            if (keys[k].ivk == ndIt->second.ivk) {
+              recordedIvk = ndIt->second.ivk;
+              narrow = true;
+            }
+          }
+        }
+      }
+      bool openedByRecordedIvk = false;
+      for (int pass = narrow ? 0 : 1; pass < 2 && !openedByRecordedIvk; pass++) {
+      for (size_t k = 0; k < keys.size(); k++) {
+        if (narrow && ((pass == 0) != (keys[k].ivk == recordedIvk)))
+          continue;   // pass 0: only the recorded ivk; pass 1: only the others
+        const libzcash::SaplingPaymentAddress& addr = keys[k].addr;
+        {   // spending-key availability is resolved once per call, in ZsTxJSONContext::keys()
           auto pt = libzcash::SaplingNotePlaintext::decrypt(
-                  outputDesc.encCiphertext, extsk.expsk.full_viewing_key().in_viewing_key(), outputDesc.ephemeralKey, outputDesc.cm);
+                  outputDesc.encCiphertext, keys[k].ivk, outputDesc.ephemeralKey, outputDesc.cm);
+          if (pt && pass == 0)
+            openedByRecordedIvk = true;
 
           if (pt) {
             auto note = pt.get();
@@ -219,10 +310,8 @@ void zsTxReceivedToJSON(const CWalletTx& wtx, UniValue& received, CAmount& total
 
             //Check Change Status
             if (wtx.vShieldedSpend.size()!=0) {
-              std::set<std::pair<PaymentAddress, uint256>> nullifierSet;
-              nullifierSet = pwalletMain->GetNullifiersForAddresses({addr});
               BOOST_FOREACH(const SpendDescription& spendDesc, wtx.vShieldedSpend) {
-                if (nullifierSet.count(std::make_pair(addr, spendDesc.nullifier))) {
+                if (zctx.hasNullifier(addr, spendDesc.nullifier)) {
                     changeTx = true;
                 }
               }
@@ -235,6 +324,7 @@ void zsTxReceivedToJSON(const CWalletTx& wtx, UniValue& received, CAmount& total
           }
         }
       }
+      } // pass
     }
   }
   received.push_back(Pair("saplingReceived",zsReceived));
@@ -243,10 +333,13 @@ void zsTxReceivedToJSON(const CWalletTx& wtx, UniValue& received, CAmount& total
 }
 
 
-void zsTxSendsToJSON(const CWalletTx& wtx, UniValue& sends, CAmount& totalSends, const std::string& strAddress, bool filterByAddress) {
+void zsTxSendsToJSON(const CWalletTx& wtx, UniValue& sends, CAmount& totalSends, const std::string& strAddress, bool filterByAddress, ZsTxJSONContext* ctx) {
 
   LOCK2(cs_main, pwalletMain->cs_wallet);
 
+  ZsTxJSONContext localCtx;   // used only when the caller passed no per-call memo
+  ZsTxJSONContext& zctx = ctx ? *ctx : localCtx;
+
   //Used to identify incomplete key sets
   int shieldedOutputCount = 0;
   int shieldedOutputDecryptedCount = 0;
@@ -304,10 +397,10 @@ void zsTxSendsToJSON(const CWalletTx& wtx, UniValue& sends, CAmount& totalSends,
 
       //Decrypt sapling outgoing t to z transaction using HDseed
       if (wtx.vShieldedSpend.size()==0) {
-        HDSeed seed;
-        if (pwalletMain->GetHDSeedForDerivation(seed)) {
+        uint256 taddrOvk;
+        if (zctx.seedOvk(taddrOvk)) {
           auto opt = libzcash::SaplingOutgoingPlaintext::decrypt(
-                  outputDesc.outCiphertext,ovkForShieldingFromTaddr(seed),outputDesc.cv,outputDesc.cm,outputDesc.ephemeralKey);
+                  outputDesc.outCiphertext,taddrOvk,outputDesc.cv,outputDesc.cm,outputDesc.ephemeralKey);
 
           if (opt) {
             auto opt_unwrapped = opt.get();
@@ -332,13 +425,11 @@ void zsTxSendsToJSON(const CWalletTx& wtx, UniValue& sends, CAmount& totalSends,
 
       //attempt Decryption of Outgoing Sapling using wallet extended spending keys
       } else {
-        std::set<libzcash::SaplingPaymentAddress> addresses;
-        pwalletMain->GetSaplingPaymentAddresses(addresses);
-        for (auto addr : addresses) {
-          libzcash::SaplingExtendedSpendingKey extsk;
-          if (pwalletMain->GetSaplingExtendedSpendingKey(addr, extsk)) {
+        const std::vector<ZsTxJSONContext::Key>& keys = zctx.keys();
+        for (size_t k = 0; k < keys.size(); k++) {
+          {   // spending-key availability is resolved once per call, in ZsTxJSONContext::keys()
             auto opt = libzcash::SaplingOutgoingPlaintext::decrypt(
-                    outputDesc.outCiphertext,extsk.expsk.full_viewing_key().ovk,outputDesc.cv,outputDesc.cm,outputDesc.ephemeralKey);
+                    outputDesc.outCiphertext,keys[k].ovk,outputDesc.cv,outputDesc.cm,outputDesc.ephemeralKey);
 
             if (opt) {
               auto opt_unwrapped = opt.get();
@@ -365,10 +456,8 @@ void zsTxSendsToJSON(const CWalletTx& wtx, UniValue& sends, CAmount& totalSends,
 
                 //Check Change Status
                 if (wtx.vShieldedSpend.size()!=0) {
-                  std::set<std::pair<PaymentAddress, uint256>> nullifierSet;
-                  nullifierSet = pwalletMain->GetNullifiersForAddresses({sentAddr});
                   BOOST_FOREACH(const SpendDescription& spendDesc, wtx.vShieldedSpend) {
-                    if (nullifierSet.count(std::make_pair(sentAddr, spendDesc.nullifier))) {
+                    if (zctx.hasNullifier(sentAddr, spendDesc.nullifier)) {
                         changeTx = true;
                     }
                   }
@@ -397,7 +486,7 @@ void zsTxSendsToJSON(const CWalletTx& wtx, UniValue& sends, CAmount& totalSends,
 }
 
 
-void zsWalletTxJSON(const CWalletTx& wtx, UniValue& ret, const std::string strAddress, bool fBool, const int returnType) {
+void zsWalletTxJSON(const CWalletTx& wtx, UniValue& ret, const std::string strAddress, bool fBool, const int returnType, ZsTxJSONContext* ctx) {
 
   LOCK2(cs_main, pwalletMain->cs_wallet);
 
@@ -445,7 +534,7 @@ void zsWalletTxJSON(const CWalletTx& wtx, UniValue& ret, const std::string strAd
   tx.push_back(Pair("expiryheight", (int64_t)wtx.nExpiryHeight));
   tx.push_back(Pair("confirmations", confirms));
   tx.push_back(Pair("time", wtx.GetTxTime()));
-  tx.push_back(Pair("size", static_cast<uint64_t>(GetSerializeSize(static_cast<CTransaction>(wtx), SER_NETWORK, PROTOCOL_VERSION))));
+  tx.push_back(Pair("size", static_cast<uint64_t>(GetSerializeSize(static_cast<const CTransaction&>(wtx), SER_NETWORK, PROTOCOL_VERSION))));
 
   //Wallet Conflicts
   UniValue conflicts(UniValue::VARR);
@@ -469,7 +558,7 @@ void zsWalletTxJSON(const CWalletTx& wtx, UniValue& ret, const std::string strAd
   }
   // Get Received
   if (returnType == 0 || returnType == 2) {
-    zsTxReceivedToJSON(wtx, received, totalReceived, strAddress, fBool);
+    zsTxReceivedToJSON(wtx, received, totalReceived, strAddress, fBool, ctx);
     if (!fBool || totalReceived != 0) {
       tx.push_back(Pair("received",received));
     }
@@ -479,7 +568,7 @@ void zsWalletTxJSON(const CWalletTx& wtx, UniValue& ret, const std::string strAd
   if (returnType == 0 || returnType == 3) {
     //Only include sends if there are spends that belong to the wallet.
     if (totalSpends != 0 || fBool) {
-      zsTxSendsToJSON(wtx, sends, totalSends, strAddress, fBool);
+      zsTxSendsToJSON(wtx, sends, totalSends, strAddress, fBool, ctx);
     }
     if (!fBool || totalSends != 0) {
       tx.push_back(Pair("sends",sends));

src/wallet/rpchushwallet.h

@@ -6,6 +6,13 @@
 #ifndef HUSH_WALLET_RPCHUSHWALLET_H
 #define HUSH_WALLET_RPCHUSHWALLET_H
 
+#include "uint256.h"
+#include "zcash/Address.hpp"
+
+#include <set>
+#include <utility>
+#include <vector>
+
 struct balancestruct {
   CAmount confirmed;
   CAmount unconfirmed;
@@ -13,10 +20,46 @@ struct balancestruct {
   CAmount immature;
 };
 
+// Per-RPC-call memo for the zs*ToJSON helpers: the wallet's usable Sapling keys, the HD-seed ovk and a
+// nullifier index, derived once per call instead of once per output. Every member is a copy; nothing
+// points into wallet maps. Valid only while the caller holds LOCK2(cs_main, pwalletMain->cs_wallet) for
+// the whole call: declare it after that lock and never keep it across calls. Passing NULL to the helpers
+// gives each helper call its own memo (the old per-call cost profile, minus the per-output repeats).
+class ZsTxJSONContext {
+public:
+    struct Key {
+        libzcash::SaplingPaymentAddress addr;
+        libzcash::SaplingIncomingViewingKey ivk;
+        uint256 ovk;
+    };
+
+    ZsTxJSONContext() : keysBuilt(false), nullifiersBuilt(false), seedOvkTried(false), seedOvkOk(false) {}
+    ZsTxJSONContext(const ZsTxJSONContext&) = delete;
+    ZsTxJSONContext& operator=(const ZsTxJSONContext&) = delete;
+
+    // Wallet z-addresses (std::set order) whose extended spending key is available now, with that key's
+    // extsk.expsk.full_viewing_key().in_viewing_key() and .ovk. Empty for an encrypted+locked wallet or
+    // one that holds only viewing keys.
+    const std::vector<Key>& keys();
+    // == GetNullifiersForAddresses({addr}).count(std::make_pair(addr, nullifier)) > 0
+    bool hasNullifier(const libzcash::SaplingPaymentAddress& addr, const uint256& nullifier);
+    // == GetHDSeedForDerivation(seed) ? (ovkOut = ovkForShieldingFromTaddr(seed), true) : false
+    bool seedOvk(uint256& ovkOut);
+
+private:
+    bool keysBuilt;
+    std::vector<Key> vKeys;
+    bool nullifiersBuilt;
+    std::set<std::pair<libzcash::SaplingIncomingViewingKey, uint256> > nfByIvk;
+    bool seedOvkTried;
+    bool seedOvkOk;
+    uint256 seedOvkValue;
+};
+
 void zsTxSpendsToJSON(const CWalletTx& wtx, UniValue& spends, CAmount& totalSpends, CAmount& filteredSpends, const std::string& strAddress, bool filterByAddress);
-void zsTxReceivedToJSON(const CWalletTx& wtx, UniValue& received, CAmount& totalReceived, const std::string& strAddress, bool filterByAddress);
-void zsTxSendsToJSON(const CWalletTx& wtx, UniValue& sends, CAmount& totalSends, const std::string& strAddress, bool filterByAddress);
-void zsWalletTxJSON(const CWalletTx& wtx, UniValue& ret, const std::string strAddress, bool fBool, const int returnType);
+void zsTxReceivedToJSON(const CWalletTx& wtx, UniValue& received, CAmount& totalReceived, const std::string& strAddress, bool filterByAddress, ZsTxJSONContext* ctx = NULL);
+void zsTxSendsToJSON(const CWalletTx& wtx, UniValue& sends, CAmount& totalSends, const std::string& strAddress, bool filterByAddress, ZsTxJSONContext* ctx = NULL);
+void zsWalletTxJSON(const CWalletTx& wtx, UniValue& ret, const std::string strAddress, bool fBool, const int returnType, ZsTxJSONContext* ctx = NULL);
 
 
 #endif //HUSH_WALLET_RPCWALLET_H

src/wallet/rpcwallet.cpp

@@ -48,6 +48,7 @@
 #include <boost/assign/list_of.hpp>
 #include <utf8.h>
 #include <univalue.h>
+#include <algorithm>
 #include <numeric>
 #include "hush_defs.h"
 #include <string.h>
@@ -1773,7 +1774,19 @@ static void MaybePushAddress(UniValue & entry, const CTxDestination &dest)
     }
 }
 
-void ListTransactions(const CWalletTx& wtx, const string& strAccount, int nMinDepth, bool fLong, UniValue& ret, const isminefilter& filter)
+// listtransactions skips its first `from` entries without building them: while *pnSkip > 0, an
+// entry that would be emitted only decrements *pnSkip. The counter is shared across calls, so the
+// skipped entries are exactly the first `from` ones in emission order. NULL: emit everything.
+static bool ConsumeSkip(int* pnSkip)
+{
+    if (pnSkip != NULL && *pnSkip > 0) {
+        --*pnSkip;
+        return true;
+    }
+    return false;
+}
+
+void ListTransactions(const CWalletTx& wtx, const string& strAccount, int nMinDepth, bool fLong, UniValue& ret, const isminefilter& filter, int* pnSkip = NULL)
 {
     CAmount nFee;
     string strSentAccount;
@@ -1787,11 +1800,18 @@ void ListTransactions(const CWalletTx& wtx, const string& strAccount, int nMinDe
     bool fAllAccounts = (strAccount == string("*"));
     bool involvesWatchonly = wtx.IsFromMe(ISMINE_WATCH_ONLY);
 
+    // "size" is a property of the transaction, not of an entry: compute it once, when the first
+    // entry is built, through a const reference instead of a by-value CTransaction copy.
+    bool fHaveTxSize = false;
+    uint64_t nTxSize = 0;
+
     // Sent
     if ((!listSent.empty() || nFee != 0) && (fAllAccounts || strAccount == strSentAccount))
     {
         BOOST_FOREACH(const COutputEntry& s, listSent)
         {
+            if (ConsumeSkip(pnSkip))
+                continue;
             UniValue entry(UniValue::VOBJ);
             if(involvesWatchonly || (::IsMine(*pwalletMain, s.destination) & ISMINE_WATCH_ONLY))
                 entry.push_back(Pair("involvesWatchonly", true));
@@ -1803,7 +1823,11 @@ void ListTransactions(const CWalletTx& wtx, const string& strAccount, int nMinDe
             entry.push_back(Pair("fee", ValueFromAmount(-nFee)));
             if (fLong)
                 WalletTxToJSON(wtx, entry);
-            entry.push_back(Pair("size", static_cast<uint64_t>(GetSerializeSize(static_cast<CTransaction>(wtx), SER_NETWORK, PROTOCOL_VERSION))));
+            if (!fHaveTxSize) {
+                nTxSize = static_cast<uint64_t>(GetSerializeSize(static_cast<const CTransaction&>(wtx), SER_NETWORK, PROTOCOL_VERSION));
+                fHaveTxSize = true;
+            }
+            entry.push_back(Pair("size", nTxSize));
             ret.push_back(entry);
         }
     }
@@ -1818,6 +1842,8 @@ void ListTransactions(const CWalletTx& wtx, const string& strAccount, int nMinDe
                 account = pwalletMain->mapAddressBook[r.destination].name;
             if (fAllAccounts || (account == strAccount))
             {
+                if (ConsumeSkip(pnSkip))
+                    continue;
                 UniValue entry(UniValue::VOBJ);
                 if(involvesWatchonly || (::IsMine(*pwalletMain, r.destination) & ISMINE_WATCH_ONLY))
                     entry.push_back(Pair("involvesWatchonly", true));
@@ -1848,19 +1874,25 @@ void ListTransactions(const CWalletTx& wtx, const string& strAccount, int nMinDe
                 entry.push_back(Pair("vout", r.vout));
                 if (fLong)
                     WalletTxToJSON(wtx, entry);
-                entry.push_back(Pair("size", static_cast<uint64_t>(GetSerializeSize(static_cast<CTransaction>(wtx), SER_NETWORK, PROTOCOL_VERSION))));
+                if (!fHaveTxSize) {
+                    nTxSize = static_cast<uint64_t>(GetSerializeSize(static_cast<const CTransaction&>(wtx), SER_NETWORK, PROTOCOL_VERSION));
+                    fHaveTxSize = true;
+                }
+                entry.push_back(Pair("size", nTxSize));
                 ret.push_back(entry);
             }
         }
     }
 }
 
-void AcentryToJSON(const CAccountingEntry& acentry, const string& strAccount, UniValue& ret)
+void AcentryToJSON(const CAccountingEntry& acentry, const string& strAccount, UniValue& ret, int* pnSkip = NULL)
 {
     bool fAllAccounts = (strAccount == string("*"));
 
     if (fAllAccounts || acentry.strAccount == strAccount)
     {
+        if (ConsumeSkip(pnSkip))
+            return;
         UniValue entry(UniValue::VOBJ);
         entry.push_back(Pair("account", acentry.strAccount));
         entry.push_back(Pair("category", "move"));
@@ -1954,46 +1986,42 @@ UniValue listtransactions(const UniValue& params, bool fHelp, const CPubKey& myp
     UniValue ret(UniValue::VARR);
 
     std::list<CAccountingEntry> acentries;
-    CWallet::TxItems txOrdered = pwalletMain->OrderedTxItems(acentries, strAccount);
+    CWallet::TxItems txOrdered = pwalletMain->OrderedTxItems(acentries, strAccount, true);
 
-    // iterate backwards until we have nCount items to return:
+    // iterate backwards until we have nCount items to return. The first nFrom entries are only
+    // counted (nSkipLeft), never built, since they are not returned.
+    int nSkipLeft = nFrom;
     for (CWallet::TxItems::reverse_iterator it = txOrdered.rbegin(); it != txOrdered.rend(); ++it)
     {
         CWalletTx *const pwtx = (*it).second.first;
         if (pwtx != 0)
         {
-            ListTransactions(*pwtx, strAccount, 0, true, ret, filter);
+            ListTransactions(*pwtx, strAccount, 0, true, ret, filter, &nSkipLeft);
         }
         CAccountingEntry *const pacentry = (*it).second.second;
         if (pacentry != 0)
-            AcentryToJSON(*pacentry, strAccount, ret);
+            AcentryToJSON(*pacentry, strAccount, ret, &nSkipLeft);
 
-        if ((int)ret.size() >= (nCount+nFrom)) break;
+        // (skipped + built) is the entry count the loop used to test
+        if ((nFrom - nSkipLeft) + (int)ret.size() >= (nCount+nFrom)) break;
     }
-    // ret is newest to oldest
+    // ret holds the entries after the first nSkipped ones, newest to oldest
+    const int nSkipped = nFrom - nSkipLeft;
+    const int nSeen = nSkipped + (int)ret.size();
 
-    if (nFrom > (int)ret.size())
-        nFrom = ret.size();
-    if ((nFrom + nCount) > (int)ret.size())
-        nCount = ret.size() - nFrom;
+    if (nFrom > nSeen)
+        nFrom = nSeen;
+    if ((nFrom + nCount) > nSeen)
+        nCount = nSeen - nFrom;
 
-    vector<UniValue> arrTmp = ret.getValues();
+    // The reply is entries [nFrom, nFrom+nCount) of the newest-first sequence, oldest to newest.
+    // Entry k of that sequence is ret[k - nSkipped] (nFrom >= nSkipped holds here).
+    UniValue result(UniValue::VARR);
+    const std::vector<UniValue>& vNewestFirst = ret.getValues();
+    for (int i = nFrom + nCount; i > nFrom; --i)
+        result.push_back(vNewestFirst[i - 1 - nSkipped]);
 
-    vector<UniValue>::iterator first = arrTmp.begin();
-    std::advance(first, nFrom);
-    vector<UniValue>::iterator last = arrTmp.begin();
-    std::advance(last, nFrom+nCount);
-
-    if (last != arrTmp.end()) arrTmp.erase(last, arrTmp.end());
-    if (first != arrTmp.begin()) arrTmp.erase(arrTmp.begin(), first);
-
-    std::reverse(arrTmp.begin(), arrTmp.end()); // Return oldest to newest
-
-    ret.clear();
-    ret.setArray();
-    ret.push_backV(arrTmp);
-
-    return ret;
+    return result;
 }
 
 UniValue listaccounts(const UniValue& params, bool fHelp, const CPubKey& mypk)
@@ -2238,7 +2266,7 @@ UniValue gettransaction(const UniValue& params, bool fHelp, const CPubKey& mypk)
     ListTransactions(wtx, "*", 0, false, details, filter);
     entry.push_back(Pair("details", details));
 
-    string strHex = EncodeHexTx(static_cast<CTransaction>(wtx));
+    string strHex = EncodeHexTx(static_cast<const CTransaction&>(wtx));
     entry.push_back(Pair("hex", strHex));
 
     return entry;
@@ -2960,6 +2988,26 @@ UniValue listunspent(const UniValue& params, bool fHelp, const CPubKey& mypk)
     return results;
 }
 
+// Wallet transactions ordered by nOrderPos without copying them (the old code copied every CWalletTx,
+// with every note's witness list, into a map<int64_t,CWalletTx>). The pointers point into
+// pwalletMain->mapWallet and are valid only while cs_wallet is held and nothing erases from mapWallet
+// (std::map insertions do not invalidate them). Reproduces map<int64_t,CWalletTx>::insert exactly:
+// ascending nOrderPos and, for equal nOrderPos, only the first tx in mapWallet (txid) order, because
+// insert() never replaces an existing key; hence stable_sort + unique.
+typedef std::pair<int64_t, const CWalletTx*> OrderedWtx;
+static bool OrderedWtxLess(const OrderedWtx& a, const OrderedWtx& b) { return a.first < b.first; }
+static bool OrderedWtxSamePos(const OrderedWtx& a, const OrderedWtx& b) { return a.first == b.first; }
+static void BuildOrderedWalletTxs(std::vector<OrderedWtx>& out)
+{
+    AssertLockHeld(pwalletMain->cs_wallet);
+    out.clear();
+    out.reserve(pwalletMain->mapWallet.size());
+    for (std::map<uint256, CWalletTx>::const_iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it)
+        out.push_back(OrderedWtx(it->second.nOrderPos, &it->second));
+    std::stable_sort(out.begin(), out.end(), OrderedWtxLess);
+    out.erase(std::unique(out.begin(), out.end(), OrderedWtxSamePos), out.end());
+}
+
 // Return current blockchain status, wallet balance, address balance and the last 200 transactions
 UniValue getalldata(const UniValue& params, bool fHelp,const CPubKey&)
 {
@@ -3031,12 +3079,14 @@ UniValue getalldata(const UniValue& params, bool fHelp,const CPubKey&)
         addressBalances.insert(make_pair(addressString,txAmounts));
     }
 
-    //Create Ordered List
-    map<int64_t,CWalletTx> orderedTxs;
+    // Per-call key/nullifier memo for the note loop below and for zsWalletTxJSON (rpchushwallet.h).
+    // Declared after the LOCK2 above, so it is destroyed before cs_wallet is released.
+    ZsTxJSONContext zsCtx;
+
+    //Classify every wallet tx for the balances (the ordered list for listtransactions is built below)
     for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it) {
       const uint256& wtxid = it->first;
       const CWalletTx& wtx = (*it).second;
-      orderedTxs.insert(std::pair<int64_t,CWalletTx>(wtx.nOrderPos, wtx));
 
       unsigned int txType = 0;
       // 0 Unassigend
@@ -3105,18 +3155,19 @@ UniValue getalldata(const UniValue& params, bool fHelp,const CPubKey&)
 
       for (auto & pair : wtx.mapSaplingNoteData) {
         SaplingOutPoint op = pair.first;
-        SaplingNoteData nd = pair.second;
+        const SaplingNoteData& nd = pair.second;
 
         //Skip Spent
         if (nd.nullifier && pwalletMain->IsSaplingSpent(*nd.nullifier))
             continue;
 
         //Decrypt sapling incoming commitments using IVK
-        for (auto addr : zs_addresses) {
-          libzcash::SaplingExtendedSpendingKey extsk;
-          if (pwalletMain->GetSaplingExtendedSpendingKey(addr, extsk)) {
+        const std::vector<ZsTxJSONContext::Key>& zsKeys = zsCtx.keys();
+        for (size_t k = 0; k < zsKeys.size(); k++) {
+          const libzcash::SaplingPaymentAddress& addr = zsKeys[k].addr;
+          {   // spending-key availability is resolved once per call, in ZsTxJSONContext::keys()
             auto pt = libzcash::SaplingNotePlaintext::decrypt(
-              wtx.vShieldedOutput[op.n].encCiphertext,extsk.expsk.full_viewing_key().in_viewing_key(),wtx.vShieldedOutput[op.n].ephemeralKey,wtx.vShieldedOutput[op.n].cm);
+              wtx.vShieldedOutput[op.n].encCiphertext,zsKeys[k].ivk,wtx.vShieldedOutput[op.n].ephemeralKey,wtx.vShieldedOutput[op.n].cm);
 
             if (txType == 0 && pwalletMain->IsLockedNote(op))
                 txType == 3;
@@ -3228,10 +3279,14 @@ UniValue getalldata(const UniValue& params, bool fHelp,const CPubKey&)
         }
 
 
+        //Ordered list of pointers into mapWallet (cs_wallet is held for the whole call)
+        std::vector<OrderedWtx> orderedTxs;
+        BuildOrderedWalletTxs(orderedTxs);
+
         uint64_t t = GetTime();
-        for (map<int64_t,CWalletTx>::reverse_iterator it = orderedTxs.rbegin(); it != orderedTxs.rend(); ++it)
+        for (std::vector<OrderedWtx>::reverse_iterator it = orderedTxs.rbegin(); it != orderedTxs.rend(); ++it)
         {
-            const CWalletTx& wtx = (*it).second;
+            const CWalletTx& wtx = *(it->second);
 
             if (!CheckFinalTx(wtx))
                 continue;
@@ -3248,7 +3303,7 @@ UniValue getalldata(const UniValue& params, bool fHelp,const CPubKey&)
               continue;
             }
 
-            zsWalletTxJSON(wtx, trans, "*", false, 0);
+            zsWalletTxJSON(wtx, trans, "*", false, 0, &zsCtx);
             if (trans.size() >= nCount) break;
 
         }
@@ -3514,18 +3569,16 @@ UniValue z_listreceivedaddress(const UniValue& params, bool fHelp,const CPubKey&
     if (nFilter < 0)
         throw runtime_error("Filter must be greater that 0.");
 
-    //Created Ordered Transaction Map
-    map<int64_t,CWalletTx> orderedTxs;
-    for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it) {
-      const CWalletTx& wtx = (*it).second;
-      orderedTxs.insert(std::pair<int64_t,CWalletTx>(wtx.nOrderPos, wtx));
-    }
+    //Ordered list of pointers into mapWallet (cs_wallet is held for the whole call)
+    std::vector<OrderedWtx> orderedTxs;
+    BuildOrderedWalletTxs(orderedTxs);
+    ZsTxJSONContext zsCtx;   // per-call key/nullifier memo (rpchushwallet.h)
 
 
     uint64_t t = GetTime();
     //Reverse Iterate thru transactions
-    for (map<int64_t,CWalletTx>::reverse_iterator it = orderedTxs.rbegin(); it != orderedTxs.rend(); ++it) {
-        const CWalletTx& wtx = (*it).second;
+    for (std::vector<OrderedWtx>::reverse_iterator it = orderedTxs.rbegin(); it != orderedTxs.rend(); ++it) {
+        const CWalletTx& wtx = *(it->second);
         bool includeTransaction = true;
 
         //Excude transactions with less confirmations than required
@@ -3546,7 +3599,7 @@ UniValue z_listreceivedaddress(const UniValue& params, bool fHelp,const CPubKey&
         }
 
         if (includeTransaction) {
-          zsWalletTxJSON(wtx, ret, params[0].get_str() , true, 2);
+          zsWalletTxJSON(wtx, ret, params[0].get_str() , true, 2, &zsCtx);
         }
 
         if (ret.size() > nCount) break;
@@ -3597,7 +3650,7 @@ UniValue z_listsentbyaddress(const UniValue& params, bool fHelp,const CPubKey&)
   if (!EnsureWalletIsAvailable(fHelp))
       return NullUniValue;
 
-  if (fHelp || params.size() > 5 || params.size() == 3)
+  if (fHelp || params.size() > 6 || params.size() == 3)
       throw runtime_error(
         "z_listsentbyaddress\n"
         "\nReturns decrypted DragonX outputs sent to a single address.\n"
@@ -3622,11 +3675,16 @@ UniValue z_listsentbyaddress(const UniValue& params, bool fHelp,const CPubKey&)
         "5. \"Count:\"                 (numeric, optional, default=9999999) \n"
         "                               Last n number of transactions returned\n"
         "\n"
+        "6. \"Sends only:\"             (boolean, optional, default=false) \n"
+        "                               true: omit the \"spends\" and \"received\" keys of every entry and skip the\n"
+        "                               trial decryption they need. false: the full historic output.\n"
+        "\n"
         "Default Parameters:\n"
         "2. 0 - O confimations required\n"
         "3. 0 - Returns all transactions\n"
         "4. 9999999 - Ignored\n"
         "5. 9999999 - Return the last 9,999,999 transactions.\n"
+        "6. false - Include the spends and received sections.\n"
         "\n"
         "\nResult:\n"
         "   \"txid\":  \"transactionid\",           (string) The transaction id.\n"
@@ -3675,6 +3733,7 @@ UniValue z_listsentbyaddress(const UniValue& params, bool fHelp,const CPubKey&)
     int64_t nFilterType = 0;
     int64_t nFilter = 9999999;
     int64_t nCount = 9999999;
+    bool fSendsOnly = false;   // opt-in, see help: omits the "spends"/"received" keys
 
     if (params.size() >= 2)
       nMinConfirms = params[1].get_int64();
@@ -3684,10 +3743,14 @@ UniValue z_listsentbyaddress(const UniValue& params, bool fHelp,const CPubKey&)
       nFilter = params[3].get_int64();
     }
 
-    if (params.size() == 5) {
+    if (params.size() >= 5) {
       nCount = params[4].get_int64();
     }
 
+    if (params.size() == 6) {
+      fSendsOnly = params[5].get_bool();
+    }
+
     if (nMinConfirms < 0)
       throw runtime_error("Minimum confimations must be greater that 0");
 
@@ -3697,18 +3760,16 @@ UniValue z_listsentbyaddress(const UniValue& params, bool fHelp,const CPubKey&)
     if (nFilter < 0)
         throw runtime_error("Filter must be greater that 0.");
 
-    //Created Ordered Transaction Map
-    map<int64_t,CWalletTx> orderedTxs;
-    for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it) {
-      const CWalletTx& wtx = (*it).second;
-      orderedTxs.insert(std::pair<int64_t,CWalletTx>(wtx.nOrderPos, wtx));
-    }
+    //Ordered list of pointers into mapWallet (cs_wallet is held for the whole call)
+    std::vector<OrderedWtx> orderedTxs;
+    BuildOrderedWalletTxs(orderedTxs);
+    ZsTxJSONContext zsCtx;   // per-call key/nullifier memo (rpchushwallet.h)
 
 
     uint64_t t = GetTime();
     //Reverse Iterate thru transactions
-    for (map<int64_t,CWalletTx>::reverse_iterator it = orderedTxs.rbegin(); it != orderedTxs.rend(); ++it) {
-      const CWalletTx& wtx = (*it).second;
+    for (std::vector<OrderedWtx>::reverse_iterator it = orderedTxs.rbegin(); it != orderedTxs.rend(); ++it) {
+      const CWalletTx& wtx = *(it->second);
 
       if (!CheckFinalTx(wtx))
           continue;
@@ -3731,21 +3792,21 @@ UniValue z_listsentbyaddress(const UniValue& params, bool fHelp,const CPubKey&)
       if (nFilterType == 2 && wtx.GetDepthInMainChain() > nFilter)
           continue;
 
-      zsWalletTxJSON(wtx, ret, "*", false, 0);
+      zsWalletTxJSON(wtx, ret, "*", false, fSendsOnly ? 3 : 0, &zsCtx);
 
 
       if (ret.size() >= nCount) break;
     }
 
-    vector<UniValue> arrTmp = ret.getValues();
-
-    std::reverse(arrTmp.begin(), arrTmp.end()); // Return oldest to newest
-
-    ret.clear();
-    ret.setArray();
-    ret.push_backV(arrTmp);
-
-    return ret;
+    // Return oldest to newest (ret was filled newest first): one copy of each entry instead of
+    // getValues() + std::reverse + push_backV
+    UniValue oldestFirst(UniValue::VARR);
+    {
+        const std::vector<UniValue>& newestFirst = ret.getValues();
+        for (std::vector<UniValue>::const_reverse_iterator rit = newestFirst.rbegin(); rit != newestFirst.rend(); ++rit)
+            oldestFirst.push_back(*rit);
+    }
+    return oldestFirst;
 }
 
 UniValue z_getstats(const UniValue& params, bool fHelp, const CPubKey& mypk)
@@ -4662,7 +4723,7 @@ UniValue z_listreceivedbyaddress(const UniValue& params, bool fHelp, const CPubK
             }
             obj.push_back(Pair("outindex", (int)entry.op.n));
             obj.push_back(Pair("rawconfirmations", entry.confirmations));
-            auto wtx = pwalletMain->mapWallet.at(entry.op.hash); //.ToString());
+            const CWalletTx& wtx = pwalletMain->mapWallet.at(entry.op.hash); //.ToString());
             obj.push_back(Pair("time", wtx.GetTxTime()));
 
             obj.push_back(Pair("confirmations", dpowconfs));
@@ -4915,7 +4976,7 @@ UniValue z_viewtransaction(const UniValue& params, bool fHelp, const CPubKey& my
             continue;
         }
         auto op        = res->second;
-        auto wtxPrev   = pwalletMain->mapWallet.at(op.hash);
+        const CWalletTx& wtxPrev = pwalletMain->mapWallet.at(op.hash);
         auto decrypted = wtxPrev.DecryptSaplingNote(op).get();
         auto notePt    = decrypted.first;
         auto pa        = decrypted.second;
@@ -5018,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);
 }
 
@@ -5047,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<AsyncRPCOperationId> filter;
     if (params.size()==1) {
@@ -5058,11 +5129,18 @@ UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedO
     }
     bool useFilter = (filter.size()>0);
 
-    UniValue ret(UniValue::VARR);
     std::shared_ptr<AsyncRPCQueue> q = getAsyncRPCQueue();
     std::vector<AsyncRPCOperationId> 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<UniValue> objs;
+    objs.reserve(ids.size());
+    std::vector<std::pair<int64_t, size_t> > order;
+    order.reserve(ids.size());
+
+    for (const AsyncRPCOperationId& id : ids) {
         if (useFilter && !filter.count(id))
             continue;
 
@@ -5078,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<UniValue> 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<int64_t, size_t>& a, const std::pair<int64_t, size_t>& 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;
 }
@@ -6184,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) {

src/wallet/wallet.cpp

@@ -1240,16 +1240,6 @@ int CWallet::SaplingWitnessMinimumHeight(const uint256& nullifier, int nWitnessH
     return nMinimumHeight;
 }
 
-int CWallet::SaplingWitnessMinimumHeight(const boost::optional<uint256>& nullifier, int nWitnessHeight, int nMinimumHeight)
-{
-    // No nullifier => an incoming-viewing-key-only note (z_importviewingkey without the full
-    // viewing key). Spend depth is unknowable, so treat it as unspent and keep its witness.
-    if (!nullifier) {
-        return min(nWitnessHeight, nMinimumHeight);
-    }
-    return SaplingWitnessMinimumHeight(*nullifier, nWitnessHeight, nMinimumHeight);
-}
-
 int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessOnly)
 {
   LOCK2(cs_main, cs_wallet);
@@ -1276,8 +1266,15 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
         uint256 blockRoot;
         uint256 witnessRoot;
 
-        if (!nd->nullifier)
+        if (!nd->nullifier) {
           ::ClearSingleNoteWitnessCache(nd);
+          // A note found with only an incoming viewing key (z_importviewingkey) never gets a
+          // nullifier (UpdateSaplingNullifierNoteMapWithTx needs the full viewing key) and can't be
+          // spent, so it needs no witness. Building one re-read its block on every call and then
+          // aborted on *item.second.nullifier below.
+          if (!HaveSaplingFullViewingKey(nd->ivk))
+            continue;
+        }
 
         if (!nd->witnesses.empty() && nd->witnessHeight > 0) {
 
@@ -1287,7 +1284,7 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
 
           //Skip Validation when witness root has been validated
           if (nd->witnessRootValidated) {
-            nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+            nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
             continue;
           }
 
@@ -1299,12 +1296,12 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
             CBlockIndex* whIndex = chainActive[nd->witnessHeight];
             if (whIndex == NULL) {
               //witnessHeight strictly above the active chain (transient catch-up): cannot validate yet
-              nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+              nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
               continue;
             }
             if (nd->witnesses.front().root() == whIndex->hashFinalSaplingRoot) {
               nd->witnessRootValidated = true;
-              nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+              nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
               continue;
             }
             //root mismatch on the active chain -> desynced; fall through to rebuild below
@@ -1316,7 +1313,7 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
           blockRoot = pblockindex->hashFinalSaplingRoot;
           if (witnessRoot == blockRoot) {
             nd->witnessRootValidated = true;
-            nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+            nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
             continue;
           }
         }
@@ -1368,7 +1365,10 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
         }
         nd->witnessHeight = pblockindex->GetHeight();
         UpdateSaplingNullifierNoteMapWithTx(wtxItem.second);
-        nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+        if (item.second.nullifier)
+          nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+        else
+          LogPrintf("%s: no witness found for note %s:%d in its block; it stays unspendable until a rescan\n", __func__, wtxHash.ToString(), op.n);
       }
     }
   }
@@ -1711,10 +1711,12 @@ int64_t CWallet::IncOrderPosNext(CWalletDB *pwalletdb)
     return nRet;
 }
 
-CWallet::TxItems CWallet::OrderedTxItems(std::list<CAccountingEntry>& acentries, std::string strAccount)
+CWallet::TxItems CWallet::OrderedTxItems(std::list<CAccountingEntry>& acentries, std::string strAccount, bool fReadOnlyDb)
 {
     AssertLockHeld(cs_wallet); // mapWallet
-    CWalletDB walletdb(strWalletFile);
+    // Only a read cursor over the accounting entries runs on this handle. Pure readers open it
+    // read-only and without flush-on-close, so the call does not end in a txn_checkpoint.
+    CWalletDB walletdb(strWalletFile, fReadOnlyDb ? "r" : "r+", !fReadOnlyDb);
 
     // First: get all CWalletTx and CAccountingEntry into a sorted-by-order multimap.
     TxItems txOrdered;
@@ -2945,12 +2947,13 @@ boost::optional<std::pair<
     SaplingPaymentAddress>> CWalletTx::DecryptSaplingNote(SaplingOutPoint op) const
 {
     // Check whether we can decrypt this SaplingOutPoint
-    if (this->mapSaplingNoteData.count(op) == 0) {
+    mapSaplingNoteData_t::const_iterator ndIt = this->mapSaplingNoteData.find(op);
+    if (ndIt == this->mapSaplingNoteData.end()) {
         return boost::none;
     }
 
-    auto output = this->vShieldedOutput[op.n];
-    auto nd = this->mapSaplingNoteData.at(op);
+    const OutputDescription& output = this->vShieldedOutput[op.n];
+    const SaplingNoteData& nd = ndIt->second;
 
     auto maybe_pt = SaplingNotePlaintext::decrypt(
         output.encCiphertext,
@@ -5639,6 +5642,26 @@ void CWallet::GetFilteredNotes(
     GetFilteredNotes(saplingEntries, filterAddresses, minDepth, INT_MAX, ignoreSpent, requireSpendingKey);
 }
 
+namespace {
+
+// GetFilteredNotes' decrypt-free address pre-filter is only built for at most this many
+// filter addresses. Verifying each one costs a Jubjub scalar multiplication, which buys
+// nothing when the filter is "every address in the wallet" (z_listunspent, z_getbalances).
+// Setting it to 0 disables the pre-filter entirely.
+const size_t GETFILTEREDNOTES_PREFILTER_MAX_ADDRESSES = 16;
+
+// True if ivk < 2^251. A genuine Sapling ivk is CRH_ivk output with its top 5 bits cleared
+// (librustzcash_crh_ivk), so it is below 2^251 < r_J (the Jubjub subgroup order). Below r_J,
+// pk_d = [ivk] g_d determines ivk uniquely. librustzcash_ivk_to_pkd multiplies by the raw
+// 256-bit value without reducing it, so a value >= r_J aliases ivk - r_J. Byte 31 of the
+// little-endian representation is the most significant byte.
+bool IsCanonicalSaplingIvk(const libzcash::SaplingIncomingViewingKey& ivk)
+{
+    return (ivk.begin()[31] & 0xF8) == 0;
+}
+
+} // namespace
+
 /**
  * Find notes in the wallet filtered by payment addresses, min depth, max depth,
  * if the note is spent, if a spending key is required, and if the notes are locked.
@@ -5657,8 +5680,47 @@ void CWallet::GetFilteredNotes(
 
     LogPrintf("%s ignoreLocked=%d\n", __func__, ignoreLocked);
 
-    for (auto & p : mapWallet) {
-        CWalletTx wtx = p.second;
+    // Decrypt-free address pre-filter. A note matches a Sapling filter address a only if
+    // nd.ivk.address(a.d) == a. If a's recorded ivk r is canonical and provably generates a
+    // (r.address(a.d) == a), then any other canonical ivk cannot generate a, so a note whose
+    // canonical nd.ivk is not one of these r can be skipped without decrypting it. Recorded
+    // pairs are not trusted as-is (z_importviewingkey stores any (ivk, zaddr) pair the user
+    // supplies), so each one is verified. If any Sapling filter address has no recorded ivk,
+    // a non-canonical one, or one that does not generate it, the pre-filter is off and every
+    // note takes the old decrypt path. InvalidEncoding entries never equal a note's address.
+    std::set<libzcash::SaplingIncomingViewingKey> prefilterIvks;
+    bool usePrefilter = !filterAddresses.empty() &&
+                        filterAddresses.size() <= GETFILTEREDNOTES_PREFILTER_MAX_ADDRESSES;
+    if (usePrefilter) {
+        for (const libzcash::PaymentAddress& fa : filterAddresses) {
+            const libzcash::SaplingPaymentAddress* sa = boost::get<libzcash::SaplingPaymentAddress>(&fa);
+            if (sa == nullptr) {
+                continue;
+            }
+            libzcash::SaplingIncomingViewingKey rivk;
+            if (!GetSaplingIncomingViewingKey(*sa, rivk) || !IsCanonicalSaplingIvk(rivk)) {
+                usePrefilter = false;
+                break;
+            }
+            auto derived = rivk.address(sa->d);
+            if (!derived || !(derived.get() == *sa)) {
+                usePrefilter = false;
+                break;
+            }
+            prefilterIvks.insert(rivk);
+        }
+    }
+
+    for (const auto & p : mapWallet) {
+        // A reference, not a copy: copying a CWalletTx deep-copies every note's witness list.
+        // Nothing in this loop mutates mapWallet or any CWalletTx, and cs_main + cs_wallet are
+        // held for the whole function, so the reference stays valid.
+        const CWalletTx& wtx = p.second;
+
+        // A tx without Sapling note data can never yield an entry (the note loop below runs
+        // zero times), so skip it before the finality, maturity and depth work.
+        if (wtx.mapSaplingNoteData.empty())
+            continue;
 
         // Filter the transactions before checking for notes
         if (!CheckFinalTx(wtx) || wtx.GetBlocksToMaturity() > 0)
@@ -5681,31 +5743,43 @@ void CWallet::GetFilteredNotes(
             }
         }
 
-        for (auto & pair : wtx.mapSaplingNoteData) {
-            SaplingOutPoint op = pair.first;
-            SaplingNoteData nd = pair.second;
+        for (const auto & pair : wtx.mapSaplingNoteData) {
+            const SaplingOutPoint& op = pair.first;
+            const SaplingNoteData& nd = pair.second;
 
-            auto maybe_pt = SaplingNotePlaintext::decrypt(
-                wtx.vShieldedOutput[op.n].encCiphertext,
-                nd.ivk,
-                wtx.vShieldedOutput[op.n].ephemeralKey,
-                wtx.vShieldedOutput[op.n].cm);
-            assert(static_cast<bool>(maybe_pt));
-            auto notePt = maybe_pt.get();
-
-            auto maybe_pa = nd.ivk.address(notePt.d);
-            assert(static_cast<bool>(maybe_pa));
-            auto pa = maybe_pa.get();
-
-            // skip notes which belong to a different payment address in the wallet
-            if (!(filterAddresses.empty() || filterAddresses.count(pa))) {
+            // Address pre-filter (see prefilterIvks above): a canonical ivk that is not one of
+            // the verified filter ivks cannot generate any filter address, so the
+            // filterAddresses check after decryption would reject this note anyway.
+            if (usePrefilter && IsCanonicalSaplingIvk(nd.ivk) && prefilterIvks.count(nd.ivk) == 0) {
                 continue;
             }
 
+            // Spent check before trial decryption: it reads only nd.nullifier and wallet
+            // state (not the plaintext or the address), so spent notes are never decrypted.
             if (ignoreSpent && nd.nullifier && IsSaplingSpent(*nd.nullifier)) {
                 continue;
             }
 
+            uint256 pk_d;
+            auto maybe_pt = SaplingNotePlaintext::decrypt_with_pk_d(
+                wtx.vShieldedOutput[op.n].encCiphertext,
+                nd.ivk,
+                wtx.vShieldedOutput[op.n].ephemeralKey,
+                wtx.vShieldedOutput[op.n].cm,
+                pk_d);
+            assert(static_cast<bool>(maybe_pt));
+            auto notePt = maybe_pt.get();
+
+            // Same value as nd.ivk.address(notePt.d).get(): decryption succeeded, so
+            // librustzcash_ivk_to_pkd(nd.ivk, d) returned true (d is a valid diversifier, which
+            // is all check_diversifier tests) and wrote pk_d, the value address() recomputes.
+            SaplingPaymentAddress pa(notePt.d, pk_d);
+
+            // skip notes which belong to a different payment address in the wallet
+            if (!(filterAddresses.empty() || filterAddresses.count(pa))) {
+                continue;
+            }
+
             // skip notes which cannot be spent
             if (requireSpendingKey) {
                 libzcash::SaplingIncomingViewingKey ivk;
@@ -5723,7 +5797,9 @@ void CWallet::GetFilteredNotes(
                  continue;
              }
 
-            auto note = notePt.note(nd.ivk).get();
+            // Same value as notePt.note(nd.ivk).get(), which is
+            // SaplingNote(d, nd.ivk.address(d)->pk_d, value, rcm): pa is nd.ivk.address(d).
+            libzcash::SaplingNote note(notePt.d, pa.pk_d, notePt.value(), notePt.rcm);
             saplingEntries.push_back(SaplingNoteEntry {
                 op, pa, note, notePt.memo(), wtx.GetDepthInMainChain() });
         }

src/wallet/wallet.h

@@ -896,12 +896,6 @@ public:
 protected:
 
     int SaplingWitnessMinimumHeight(const uint256& nullifier, int nWitnessHeight, int nMinimumHeight);
-    //! Overload for a note whose nullifier may be unset. A note discovered through an imported
-    //! INCOMING viewing key has no nullifier (computing one needs the full viewing key), so
-    //! dereferencing the optional aborts the daemon. Treats such a note as unspent, which is the
-    //! conservative direction: it keeps the witness alive rather than pruning a note we cannot
-    //! prove spent.
-    int SaplingWitnessMinimumHeight(const boost::optional<uint256>& nullifier, int nWitnessHeight, int nMinimumHeight);
 
     /**
      * pindex is the new tip being connected.
@@ -1216,10 +1210,12 @@ public:
 
     /**
      * Get the wallet's activity log
+     * @param fReadOnlyDb open wallet.dat read-only and without flush-on-close (pure readers such as
+     *                    listtransactions: no txn_checkpoint per call); false = read-write, flush-on-close
      * @return multimap of ordered transactions and accounting entries
      * @warning Returned pointers are *only* valid within the scope of passed acentries
      */
-    TxItems OrderedTxItems(std::list<CAccountingEntry>& acentries, std::string strAccount = "");
+    TxItems OrderedTxItems(std::list<CAccountingEntry>& acentries, std::string strAccount = "", bool fReadOnlyDb = false);
 
     void MarkDirty();
     bool UpdateNullifierNoteMap();

src/zcash/Note.cpp

@@ -121,6 +121,18 @@ boost::optional<SaplingNotePlaintext> SaplingNotePlaintext::decrypt(
     const uint256 &epk,
     const uint256 &cmu
 )
+{
+    uint256 pk_d;
+    return decrypt_with_pk_d(ciphertext, ivk, epk, cmu, pk_d);
+}
+
+boost::optional<SaplingNotePlaintext> SaplingNotePlaintext::decrypt_with_pk_d(
+    const SaplingEncCiphertext &ciphertext,
+    const uint256 &ivk,
+    const uint256 &epk,
+    const uint256 &cmu,
+    uint256 &pk_d_out
+)
 {
     auto pt = AttemptSaplingEncDecryption(ciphertext, ivk, epk);
     if (!pt) {
@@ -161,6 +173,7 @@ boost::optional<SaplingNotePlaintext> SaplingNotePlaintext::decrypt(
         return boost::none;
     }
 
+    pk_d_out = pk_d;
     return ret;
 }
 

src/zcash/Note.hpp

@@ -77,6 +77,17 @@ public:
         const uint256 &cmu
     );
 
+    // Same as decrypt(ciphertext, ivk, epk, cmu). On success it also stores in pk_d_out the
+    // pk_d = [ivk] g_d that decryption computed, which equals ivk.address(d)->pk_d, so the
+    // caller does not have to recompute it. pk_d_out is not written on failure.
+    static boost::optional<SaplingNotePlaintext> decrypt_with_pk_d(
+        const SaplingEncCiphertext &ciphertext,
+        const uint256 &ivk,
+        const uint256 &epk,
+        const uint256 &cmu,
+        uint256 &pk_d_out
+    );
+
     static boost::optional<SaplingNotePlaintext> decrypt(
         const SaplingEncCiphertext &ciphertext,
         const uint256 &epk,