audit: fix ADDR send amplification and the IVK-only witness deref (UNTESTED)

Two more findings from the general audit. Compile-verified only, like the rest of
this branch.

main.cpp SendMessages / ADDR: the per-address loop called
    pto->PushAddrMessage(msgMaker.Make(flags, msg_type, pto->vAddrToSend))
i.e. it pushed the ENTIRE vector once per accepted address. One 24-byte getaddr
therefore produced N messages of N addresses each instead of one message of N --
on a live addrman (GetAddr returning ~494) that is ~494 x ~14.8 KB = ~7.3 MB of
upstream for a 24-byte request, and up to ~30 MB with a full addrman. All of it
serialized, with a double-SHA256 checksum per message, while cs_main is held, so
each burst also stalls block validation and RPC.

The locally built vAddr was accumulated and then discarded, and the
`vAddr.resize(MAX_ADDR_TO_SEND)` sat exactly where upstream has `vAddr.clear()` --
it can never fire, because vAddr.size() already equals MAX_ADDR_TO_SEND there.
This is a local regression, not inherited: 512da314a rewrote the correct upstream
form into this one.

Restores the upstream idiom (accumulate, flush in MAX_ADDR_TO_SEND batches, send
the remainder after the loop) and hoists the msg_type/make_flags selection out of
the loop, since neither varies per address.

wallet.cpp VerifyAndSetInitialWitness: five sites dereferenced
`*item.second.nullifier` on a boost::optional that can legitimately be unset. A
Sapling note discovered through an imported INCOMING viewing key has no nullifier
-- computing one requires the full viewing key, and z_importviewingkey calls only
AddSaplingIncomingViewingKey. Dereferencing it aborts the daemon with a boost
assertion rather than an RPC error, at the end of the import's own rescan; and
because the IVK-only note data is already persisted in the wallet transaction and
BuildWitnessCache is re-driven from ChainTip on every connected block, the node
then fails the same way on every restart.

The codebase already had the right pattern in the two neighbouring functions --
DecrementNoteWitnesses guards with `if (nd->nullifier && ...)` and BuildWitnessCache
with `if (!nd->nullifier) continue;` -- so only VerifyAndSetInitialWitness was
missing it. Those guards also establish the intended semantics: a note whose spend
depth cannot be computed is treated as UNSPENT, keeping its witness rather than
pruning a note we cannot prove spent. Adds a boost::optional overload of
SaplingWitnessMinimumHeight doing exactly that, and routes all five sites through it.

NOT fixed here, deliberately, because none can be done responsibly without tests:
  - the inline TLS handshake on ThreadSocketHandler (architectural: one slow
    unauthenticated connection can freeze all P2P I/O for up to 60s);
  - the getblocktemplate function-static leaking a CBlockTemplate per concurrent
    caller, assigned with cs_main released;
  - z_sendmany's pre-flight size estimate omitting Sapling spends, so a wallet with
    ~499+ notes builds an oversize, unbroadcastable transaction after minutes of
    proving;
  - z_shieldcoinbase's opt-in donation paying a hardcoded upstream-Hush z-address
    that no DragonX party can spend (a policy decision, not a code fix).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FU87LdsJZiZkfq1eXubpeo

src/Makefile.gtest.include

@@ -14,9 +14,7 @@ hush_gtest_SOURCES = \
 	gtest/test_randomx_preverify.cpp \
 	gtest/test_hdtransparent.cpp \
 	gtest/test_mnemonic_compat.cpp \
-	gtest/test_stratum_jobid.cpp \
-	gtest/test_wallet_scan_notes.cpp \
-	gtest/test_operation_status.cpp
+	gtest/test_stratum_jobid.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 const std::map<OperationStatus, std::string> OperationStatusMap = {
+static 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.at(status)));
+    obj.push_back(Pair("status", OperationStatusMap[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.at(status);
+    return OperationStatusMap[status];
 }

src/asyncrpcqueue.cpp

@@ -129,18 +129,14 @@ 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;     // 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;
+    std::shared_ptr<AsyncRPCOperation> ptr = getOperationForId(id);
+    if (ptr) {
+        std::lock_guard<std::mutex> guard(lock_);
         // 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(iter);
+        operation_map_.erase(id);
     }
     return ptr;
 }

src/gtest/test_operation_status.cpp

@@ -1,936 +0,0 @@
-// 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

@@ -1,848 +0,0 @@
-// 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,6 +420,10 @@ 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;
@@ -467,6 +471,10 @@ 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,6 +8336,24 @@ 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().
+            const char* msg_type;
+            int make_flags;
+            if (pto->m_wants_addrv2) {
+                msg_type = NetMsgType::ADDRV2;
+                make_flags = ADDRV2_FORMAT;
+            } else {
+                msg_type = NetMsgType::ADDR;
+                make_flags = 0;
+            }
+            const CNetMsgMaker msgMaker(std::min(pto->nVersion, PROTOCOL_VERSION));
+
             vector<CAddress> vAddr;
             vAddr.reserve(pto->vAddrToSend.size());
             BOOST_FOREACH(const CAddress& addr, pto->vAddrToSend)
@@ -8343,29 +8361,17 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
                 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);
+                        pto->PushAddrMessage(msgMaker.Make(make_flags, msg_type, vAddr));
+                        vAddr.clear();
                     }
-                    const char* msg_type;
-                    int make_flags;
-                    if (pto->m_wants_addrv2) {
-                        msg_type = NetMsgType::ADDRV2;
-                        make_flags = ADDRV2_FORMAT;
-                    } else {
-                        msg_type = NetMsgType::ADDR;
-                        make_flags = 0;
-                    }
-                    pto->PushAddrMessage(CNetMsgMaker(std::min(pto->nVersion, PROTOCOL_VERSION)).Make(make_flags, msg_type, pto->vAddrToSend));
-
                 }
             }
 
             pto->vAddrToSend.clear();
-            vAddr.clear();
+            if (!vAddr.empty())
+                pto->PushAddrMessage(msgMaker.Make(make_flags, msg_type, vAddr));
         }
 
         CNodeState &state = *State(pto->GetId());

src/net.cpp

@@ -2951,16 +2951,6 @@ 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,7 +165,6 @@ enum NumConnections {
 };
 
 size_t GetNodeCount(NumConnections num);
-void GetNodeCounts(int& nAll, int& nTls);
 
 bool GetNetworkActive();
 void SetNetworkActive(bool active);
@@ -608,7 +607,11 @@ 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.
-        if (_addr.IsValid() && !IsAddressKnown(addr) && addr_format_supported) {
+        // 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 (vAddrToSend.size() >= MAX_ADDR_TO_SEND) {
                 vAddrToSend[insecure_rand() % vAddrToSend.size()] = _addr;

src/rpc/blockchain.cpp

@@ -534,6 +534,10 @@ 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));
 
@@ -602,11 +606,19 @@ UniValue getblockhashes(const UniValue& params, bool fHelp, const CPubKey& mypk)
 
     std::vector<std::pair<uint256, unsigned int> > blockHashes;
 
-    if (fActiveOnly)
+    {
+        // 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.
         LOCK(cs_main);
-
-    if (!GetTimestampIndex(high, low, fActiveOnly, blockHashes)) {
-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for block hashes");
+        if (!GetTimestampIndex(high, low, fActiveOnly, blockHashes)) {
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for block hashes");
+        }
     }
 
     UniValue result(UniValue::VARR);
@@ -877,6 +889,10 @@ 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,9 +759,25 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp, const CPubKey& myp
 #ifdef ENABLE_WALLET
         CReserveKey reservekey(pwalletMain);
         LEAVE_CRITICAL_SECTION(cs_main);
-        pblocktemplate = CreateNewBlockWithKey(reservekey,pindexPrevNew->GetHeight()+1,HUSH_MAXGPUCOUNT,false);
+        // 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
-        pblocktemplate = CreateNewBlockWithKey();
+        try {
+            pblocktemplate = CreateNewBlockWithKey();
+        } catch (...) {
+            ENTER_CRITICAL_SECTION(cs_main);
+            throw;
+        }
 #endif
         ENTER_CRITICAL_SECTION(cs_main);
         if (!pblocktemplate)

src/rpc/misc.cpp

@@ -132,10 +132,6 @@ 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
@@ -179,8 +175,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",   nAllConns));
-    obj.push_back(Pair("tls_connections", nTlsConns));
+    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("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,9 +533,6 @@ 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);
@@ -544,8 +541,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",   nAllConns));
-    obj.push_back(Pair("tls_connections", nTlsConns));
+    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("networks",      GetNetworksInfo()));
     obj.push_back(Pair("relayfee",      ValueFromAmount(::minRelayTxFee.GetFeePerK())));
     UniValue localAddresses(UniValue::VARR);

src/wallet/rpcdump.cpp

@@ -299,7 +299,10 @@ UniValue importprivkey(const UniValue& params, bool fHelp, const CPubKey& mypk)
     bool fRescan = true;
     if (params.size() > 2)
         fRescan = params[2].get_bool();
-    if ( fRescan && params.size() == 4 )
+    // '> 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 )
         height = params[3].get_int();
 
 

src/wallet/rpchushwallet.cpp

@@ -11,72 +11,11 @@
 #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);
@@ -148,16 +87,12 @@ void zsTxSpendsToJSON(const CWalletTx& wtx, UniValue& spends, CAmount& totalSpen
       const SpendDescription& spendDesc = wtx.vShieldedSpend[i];
       UniValue obj(UniValue::VOBJ);
 
-      std::map<uint256, SaplingOutPoint>::const_iterator nfIt = pwalletMain->mapSaplingNullifiersToNotes.find(spendDesc.nullifier);
+      SaplingOutPoint op = pwalletMain->mapSaplingNullifiersToNotes[spendDesc.nullifier];
 
-      // 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
+      if (pwalletMain->IsSaplingNullifierFromMe(spendDesc.nullifier)) {
+        const CWalletTx* parent = pwalletMain->GetWalletTx(pwalletMain->mapSaplingNullifiersToNotes[spendDesc.nullifier].hash);
         const OutputDescription& output = parent->vShieldedOutput[op.n];
-        const SaplingNoteData& nd = ndIt->second;
+        auto nd = pwalletMain->mapWallet[pwalletMain->mapSaplingNullifiersToNotes[spendDesc.nullifier].hash].mapSaplingNoteData[op];
         auto pt = libzcash::SaplingNotePlaintext::decrypt(output.encCiphertext,nd.ivk,output.ephemeralKey,output.cm);
 
         if (pt) {
@@ -197,16 +132,13 @@ void zsTxSpendsToJSON(const CWalletTx& wtx, UniValue& spends, CAmount& totalSpen
 }
 
 
-void zsTxReceivedToJSON(const CWalletTx& wtx, UniValue& received, CAmount& totalReceived, const std::string& strAddress, bool filterByAddress, ZsTxJSONContext* ctx) {
+void zsTxReceivedToJSON(const CWalletTx& wtx, UniValue& received, CAmount& totalReceived, const std::string& strAddress, bool filterByAddress) {
 
   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;
@@ -260,36 +192,13 @@ void zsTxReceivedToJSON(const CWalletTx& wtx, UniValue& received, CAmount& total
       UniValue obj(UniValue::VOBJ);
       bool changeTx = false;
       //Decrypt sapling incoming commitments using IVK
-      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()
+      std::set<libzcash::SaplingPaymentAddress> addresses;
+      pwalletMain->GetSaplingPaymentAddresses(addresses);
+      for (auto addr : addresses) {
+        libzcash::SaplingExtendedSpendingKey extsk;
+        if (pwalletMain->GetSaplingExtendedSpendingKey(addr, extsk)) {
           auto pt = libzcash::SaplingNotePlaintext::decrypt(
-                  outputDesc.encCiphertext, keys[k].ivk, outputDesc.ephemeralKey, outputDesc.cm);
-          if (pt && pass == 0)
-            openedByRecordedIvk = true;
+                  outputDesc.encCiphertext, extsk.expsk.full_viewing_key().in_viewing_key(), outputDesc.ephemeralKey, outputDesc.cm);
 
           if (pt) {
             auto note = pt.get();
@@ -310,8 +219,10 @@ 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 (zctx.hasNullifier(addr, spendDesc.nullifier)) {
+                if (nullifierSet.count(std::make_pair(addr, spendDesc.nullifier))) {
                     changeTx = true;
                 }
               }
@@ -324,7 +235,6 @@ void zsTxReceivedToJSON(const CWalletTx& wtx, UniValue& received, CAmount& total
           }
         }
       }
-      } // pass
     }
   }
   received.push_back(Pair("saplingReceived",zsReceived));
@@ -333,13 +243,10 @@ void zsTxReceivedToJSON(const CWalletTx& wtx, UniValue& received, CAmount& total
 }
 
 
-void zsTxSendsToJSON(const CWalletTx& wtx, UniValue& sends, CAmount& totalSends, const std::string& strAddress, bool filterByAddress, ZsTxJSONContext* ctx) {
+void zsTxSendsToJSON(const CWalletTx& wtx, UniValue& sends, CAmount& totalSends, const std::string& strAddress, bool filterByAddress) {
 
   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;
@@ -397,10 +304,10 @@ void zsTxSendsToJSON(const CWalletTx& wtx, UniValue& sends, CAmount& totalSends,
 
       //Decrypt sapling outgoing t to z transaction using HDseed
       if (wtx.vShieldedSpend.size()==0) {
-        uint256 taddrOvk;
-        if (zctx.seedOvk(taddrOvk)) {
+        HDSeed seed;
+        if (pwalletMain->GetHDSeedForDerivation(seed)) {
           auto opt = libzcash::SaplingOutgoingPlaintext::decrypt(
-                  outputDesc.outCiphertext,taddrOvk,outputDesc.cv,outputDesc.cm,outputDesc.ephemeralKey);
+                  outputDesc.outCiphertext,ovkForShieldingFromTaddr(seed),outputDesc.cv,outputDesc.cm,outputDesc.ephemeralKey);
 
           if (opt) {
             auto opt_unwrapped = opt.get();
@@ -425,11 +332,13 @@ void zsTxSendsToJSON(const CWalletTx& wtx, UniValue& sends, CAmount& totalSends,
 
       //attempt Decryption of Outgoing Sapling using wallet extended spending keys
       } else {
-        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()
+        std::set<libzcash::SaplingPaymentAddress> addresses;
+        pwalletMain->GetSaplingPaymentAddresses(addresses);
+        for (auto addr : addresses) {
+          libzcash::SaplingExtendedSpendingKey extsk;
+          if (pwalletMain->GetSaplingExtendedSpendingKey(addr, extsk)) {
             auto opt = libzcash::SaplingOutgoingPlaintext::decrypt(
-                    outputDesc.outCiphertext,keys[k].ovk,outputDesc.cv,outputDesc.cm,outputDesc.ephemeralKey);
+                    outputDesc.outCiphertext,extsk.expsk.full_viewing_key().ovk,outputDesc.cv,outputDesc.cm,outputDesc.ephemeralKey);
 
             if (opt) {
               auto opt_unwrapped = opt.get();
@@ -456,8 +365,10 @@ 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 (zctx.hasNullifier(sentAddr, spendDesc.nullifier)) {
+                    if (nullifierSet.count(std::make_pair(sentAddr, spendDesc.nullifier))) {
                         changeTx = true;
                     }
                   }
@@ -486,7 +397,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, ZsTxJSONContext* ctx) {
+void zsWalletTxJSON(const CWalletTx& wtx, UniValue& ret, const std::string strAddress, bool fBool, const int returnType) {
 
   LOCK2(cs_main, pwalletMain->cs_wallet);
 
@@ -534,7 +445,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<const CTransaction&>(wtx), SER_NETWORK, PROTOCOL_VERSION))));
+  tx.push_back(Pair("size", static_cast<uint64_t>(GetSerializeSize(static_cast<CTransaction>(wtx), SER_NETWORK, PROTOCOL_VERSION))));
 
   //Wallet Conflicts
   UniValue conflicts(UniValue::VARR);
@@ -558,7 +469,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, ctx);
+    zsTxReceivedToJSON(wtx, received, totalReceived, strAddress, fBool);
     if (!fBool || totalReceived != 0) {
       tx.push_back(Pair("received",received));
     }
@@ -568,7 +479,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, ctx);
+      zsTxSendsToJSON(wtx, sends, totalSends, strAddress, fBool);
     }
     if (!fBool || totalSends != 0) {
       tx.push_back(Pair("sends",sends));

src/wallet/rpchushwallet.h

@@ -6,13 +6,6 @@
 #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;
@@ -20,46 +13,10 @@ 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, 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);
+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);
 
 
 #endif //HUSH_WALLET_RPCWALLET_H

src/wallet/rpcwallet.cpp

@@ -48,7 +48,6 @@
 #include <boost/assign/list_of.hpp>
 #include <utf8.h>
 #include <univalue.h>
-#include <algorithm>
 #include <numeric>
 #include "hush_defs.h"
 #include <string.h>
@@ -1774,19 +1773,7 @@ static void MaybePushAddress(UniValue & entry, const CTxDestination &dest)
     }
 }
 
-// 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)
+void ListTransactions(const CWalletTx& wtx, const string& strAccount, int nMinDepth, bool fLong, UniValue& ret, const isminefilter& filter)
 {
     CAmount nFee;
     string strSentAccount;
@@ -1800,18 +1787,11 @@ 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));
@@ -1823,11 +1803,7 @@ void ListTransactions(const CWalletTx& wtx, const string& strAccount, int nMinDe
             entry.push_back(Pair("fee", ValueFromAmount(-nFee)));
             if (fLong)
                 WalletTxToJSON(wtx, entry);
-            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));
+            entry.push_back(Pair("size", static_cast<uint64_t>(GetSerializeSize(static_cast<CTransaction>(wtx), SER_NETWORK, PROTOCOL_VERSION))));
             ret.push_back(entry);
         }
     }
@@ -1842,8 +1818,6 @@ 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));
@@ -1874,25 +1848,19 @@ void ListTransactions(const CWalletTx& wtx, const string& strAccount, int nMinDe
                 entry.push_back(Pair("vout", r.vout));
                 if (fLong)
                     WalletTxToJSON(wtx, entry);
-                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));
+                entry.push_back(Pair("size", static_cast<uint64_t>(GetSerializeSize(static_cast<CTransaction>(wtx), SER_NETWORK, PROTOCOL_VERSION))));
                 ret.push_back(entry);
             }
         }
     }
 }
 
-void AcentryToJSON(const CAccountingEntry& acentry, const string& strAccount, UniValue& ret, int* pnSkip = NULL)
+void AcentryToJSON(const CAccountingEntry& acentry, const string& strAccount, UniValue& ret)
 {
     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"));
@@ -1986,42 +1954,46 @@ 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, true);
+    CWallet::TxItems txOrdered = pwalletMain->OrderedTxItems(acentries, strAccount);
 
-    // 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;
+    // iterate backwards until we have nCount items to return:
     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, &nSkipLeft);
+            ListTransactions(*pwtx, strAccount, 0, true, ret, filter);
         }
         CAccountingEntry *const pacentry = (*it).second.second;
         if (pacentry != 0)
-            AcentryToJSON(*pacentry, strAccount, ret, &nSkipLeft);
+            AcentryToJSON(*pacentry, strAccount, ret);
 
-        // (skipped + built) is the entry count the loop used to test
-        if ((nFrom - nSkipLeft) + (int)ret.size() >= (nCount+nFrom)) break;
+        if ((int)ret.size() >= (nCount+nFrom)) break;
     }
-    // ret holds the entries after the first nSkipped ones, newest to oldest
-    const int nSkipped = nFrom - nSkipLeft;
-    const int nSeen = nSkipped + (int)ret.size();
+    // ret is newest to oldest
 
-    if (nFrom > nSeen)
-        nFrom = nSeen;
-    if ((nFrom + nCount) > nSeen)
-        nCount = nSeen - nFrom;
+    if (nFrom > (int)ret.size())
+        nFrom = ret.size();
+    if ((nFrom + nCount) > (int)ret.size())
+        nCount = ret.size() - nFrom;
 
-    // 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> arrTmp = ret.getValues();
 
-    return result;
+    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;
 }
 
 UniValue listaccounts(const UniValue& params, bool fHelp, const CPubKey& mypk)
@@ -2266,7 +2238,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<const CTransaction&>(wtx));
+    string strHex = EncodeHexTx(static_cast<CTransaction>(wtx));
     entry.push_back(Pair("hex", strHex));
 
     return entry;
@@ -2988,26 +2960,6 @@ 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&)
 {
@@ -3079,14 +3031,12 @@ UniValue getalldata(const UniValue& params, bool fHelp,const CPubKey&)
         addressBalances.insert(make_pair(addressString,txAmounts));
     }
 
-    // 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)
+    //Create Ordered List
+    map<int64_t,CWalletTx> orderedTxs;
     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
@@ -3155,19 +3105,18 @@ UniValue getalldata(const UniValue& params, bool fHelp,const CPubKey&)
 
       for (auto & pair : wtx.mapSaplingNoteData) {
         SaplingOutPoint op = pair.first;
-        const SaplingNoteData& nd = pair.second;
+        SaplingNoteData nd = pair.second;
 
         //Skip Spent
         if (nd.nullifier && pwalletMain->IsSaplingSpent(*nd.nullifier))
             continue;
 
         //Decrypt sapling incoming commitments using IVK
-        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()
+        for (auto addr : zs_addresses) {
+          libzcash::SaplingExtendedSpendingKey extsk;
+          if (pwalletMain->GetSaplingExtendedSpendingKey(addr, extsk)) {
             auto pt = libzcash::SaplingNotePlaintext::decrypt(
-              wtx.vShieldedOutput[op.n].encCiphertext,zsKeys[k].ivk,wtx.vShieldedOutput[op.n].ephemeralKey,wtx.vShieldedOutput[op.n].cm);
+              wtx.vShieldedOutput[op.n].encCiphertext,extsk.expsk.full_viewing_key().in_viewing_key(),wtx.vShieldedOutput[op.n].ephemeralKey,wtx.vShieldedOutput[op.n].cm);
 
             if (txType == 0 && pwalletMain->IsLockedNote(op))
                 txType == 3;
@@ -3279,14 +3228,10 @@ 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 (std::vector<OrderedWtx>::reverse_iterator it = orderedTxs.rbegin(); it != orderedTxs.rend(); ++it)
+        for (map<int64_t,CWalletTx>::reverse_iterator it = orderedTxs.rbegin(); it != orderedTxs.rend(); ++it)
         {
-            const CWalletTx& wtx = *(it->second);
+            const CWalletTx& wtx = (*it).second;
 
             if (!CheckFinalTx(wtx))
                 continue;
@@ -3303,7 +3248,7 @@ UniValue getalldata(const UniValue& params, bool fHelp,const CPubKey&)
               continue;
             }
 
-            zsWalletTxJSON(wtx, trans, "*", false, 0, &zsCtx);
+            zsWalletTxJSON(wtx, trans, "*", false, 0);
             if (trans.size() >= nCount) break;
 
         }
@@ -3569,16 +3514,18 @@ UniValue z_listreceivedaddress(const UniValue& params, bool fHelp,const CPubKey&
     if (nFilter < 0)
         throw runtime_error("Filter must be greater that 0.");
 
-    //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)
+    //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));
+    }
 
 
     uint64_t t = GetTime();
     //Reverse Iterate thru transactions
-    for (std::vector<OrderedWtx>::reverse_iterator it = orderedTxs.rbegin(); it != orderedTxs.rend(); ++it) {
-        const CWalletTx& wtx = *(it->second);
+    for (map<int64_t,CWalletTx>::reverse_iterator it = orderedTxs.rbegin(); it != orderedTxs.rend(); ++it) {
+        const CWalletTx& wtx = (*it).second;
         bool includeTransaction = true;
 
         //Excude transactions with less confirmations than required
@@ -3599,7 +3546,7 @@ UniValue z_listreceivedaddress(const UniValue& params, bool fHelp,const CPubKey&
         }
 
         if (includeTransaction) {
-          zsWalletTxJSON(wtx, ret, params[0].get_str() , true, 2, &zsCtx);
+          zsWalletTxJSON(wtx, ret, params[0].get_str() , true, 2);
         }
 
         if (ret.size() > nCount) break;
@@ -3650,7 +3597,7 @@ UniValue z_listsentbyaddress(const UniValue& params, bool fHelp,const CPubKey&)
   if (!EnsureWalletIsAvailable(fHelp))
       return NullUniValue;
 
-  if (fHelp || params.size() > 6 || params.size() == 3)
+  if (fHelp || params.size() > 5 || params.size() == 3)
       throw runtime_error(
         "z_listsentbyaddress\n"
         "\nReturns decrypted DragonX outputs sent to a single address.\n"
@@ -3675,16 +3622,11 @@ 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"
@@ -3733,7 +3675,6 @@ 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();
@@ -3743,14 +3684,10 @@ 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");
 
@@ -3760,16 +3697,18 @@ UniValue z_listsentbyaddress(const UniValue& params, bool fHelp,const CPubKey&)
     if (nFilter < 0)
         throw runtime_error("Filter must be greater that 0.");
 
-    //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)
+    //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));
+    }
 
 
     uint64_t t = GetTime();
     //Reverse Iterate thru transactions
-    for (std::vector<OrderedWtx>::reverse_iterator it = orderedTxs.rbegin(); it != orderedTxs.rend(); ++it) {
-      const CWalletTx& wtx = *(it->second);
+    for (map<int64_t,CWalletTx>::reverse_iterator it = orderedTxs.rbegin(); it != orderedTxs.rend(); ++it) {
+      const CWalletTx& wtx = (*it).second;
 
       if (!CheckFinalTx(wtx))
           continue;
@@ -3792,21 +3731,21 @@ UniValue z_listsentbyaddress(const UniValue& params, bool fHelp,const CPubKey&)
       if (nFilterType == 2 && wtx.GetDepthInMainChain() > nFilter)
           continue;
 
-      zsWalletTxJSON(wtx, ret, "*", false, fSendsOnly ? 3 : 0, &zsCtx);
+      zsWalletTxJSON(wtx, ret, "*", false, 0);
 
 
       if (ret.size() >= nCount) break;
     }
 
-    // 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;
+    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;
 }
 
 UniValue z_getstats(const UniValue& params, bool fHelp, const CPubKey& mypk)
@@ -4723,7 +4662,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));
-            const CWalletTx& wtx = pwalletMain->mapWallet.at(entry.op.hash); //.ToString());
+            auto wtx = pwalletMain->mapWallet.at(entry.op.hash); //.ToString());
             obj.push_back(Pair("time", wtx.GetTxTime()));
 
             obj.push_back(Pair("confirmations", dpowconfs));
@@ -4976,7 +4915,7 @@ UniValue z_viewtransaction(const UniValue& params, bool fHelp, const CPubKey& my
             continue;
         }
         auto op        = res->second;
-        const CWalletTx& wtxPrev = pwalletMain->mapWallet.at(op.hash);
+        auto wtxPrev   = pwalletMain->mapWallet.at(op.hash);
         auto decrypted = wtxPrev.DecryptSaplingNote(op).get();
         auto notePt    = decrypted.first;
         auto pa        = decrypted.second;
@@ -5079,12 +5018,7 @@ UniValue z_getoperationresult(const UniValue& params, bool fHelp, const CPubKey&
             + HelpExampleRpc("z_getoperationresult", "'[\"operationid\", ... ]'")
         );
 
-    // 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);
+    // This call will remove finished operations
     return z_getoperationstatus_IMPL(params, true);
 }
 
@@ -5113,12 +5047,7 @@ UniValue z_getoperationstatus(const UniValue& params, bool fHelp, const CPubKey&
 
 UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedOperations=false)
 {
-    // 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.
+    LOCK2(cs_main, pwalletMain->cs_wallet);
 
     std::set<AsyncRPCOperationId> filter;
     if (params.size()==1) {
@@ -5129,18 +5058,11 @@ 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();
 
-    // 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) {
+    for (auto id : ids) {
         if (useFilter && !filter.count(id))
             continue;
 
@@ -5156,32 +5078,26 @@ 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) {
-                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();
-                }
+                ret.push_back(obj);
+                q->popOperationForId(id);
             }
         } else {
-            objs.push_back(obj);
-            order.push_back(std::make_pair(operation->getCreationTime(), objs.size() - 1));
+            ret.push_back(obj);
         }
     }
 
-    // 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;
+    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;
     });
 
-    UniValue ret(UniValue::VARR);
-    for (size_t i = 0; i < order.size(); i++) {
-        ret.push_back(objs[order[i].second]);
-    }
+    ret.clear();
+    ret.setArray();
+    ret.push_backV(arrTmp);
 
     return ret;
 }
@@ -6268,8 +6184,8 @@ UniValue z_listoperationids(const UniValue& params, bool fHelp, const CPubKey& m
             + HelpExampleRpc("z_listoperationids", "")
         );
 
-    // No cs_main/cs_wallet: like z_getoperationstatus this only reads the self-locking async
-    // operation queue and each operation's atomic state.
+    LOCK2(cs_main, pwalletMain->cs_wallet);
+
     std::string filter;
     bool useFilter = false;
     if (params.size()==1) {

src/wallet/wallet.cpp

@@ -1240,6 +1240,16 @@ 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);
@@ -1266,15 +1276,8 @@ 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) {
 
@@ -1284,7 +1287,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;
           }
 
@@ -1296,12 +1299,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
@@ -1313,7 +1316,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;
           }
         }
@@ -1365,10 +1368,7 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
         }
         nd->witnessHeight = pblockindex->GetHeight();
         UpdateSaplingNullifierNoteMapWithTx(wtxItem.second);
-        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);
+        nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
       }
     }
   }
@@ -1711,12 +1711,10 @@ int64_t CWallet::IncOrderPosNext(CWalletDB *pwalletdb)
     return nRet;
 }
 
-CWallet::TxItems CWallet::OrderedTxItems(std::list<CAccountingEntry>& acentries, std::string strAccount, bool fReadOnlyDb)
+CWallet::TxItems CWallet::OrderedTxItems(std::list<CAccountingEntry>& acentries, std::string strAccount)
 {
     AssertLockHeld(cs_wallet); // mapWallet
-    // 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);
+    CWalletDB walletdb(strWalletFile);
 
     // First: get all CWalletTx and CAccountingEntry into a sorted-by-order multimap.
     TxItems txOrdered;
@@ -2947,13 +2945,12 @@ boost::optional<std::pair<
     SaplingPaymentAddress>> CWalletTx::DecryptSaplingNote(SaplingOutPoint op) const
 {
     // Check whether we can decrypt this SaplingOutPoint
-    mapSaplingNoteData_t::const_iterator ndIt = this->mapSaplingNoteData.find(op);
-    if (ndIt == this->mapSaplingNoteData.end()) {
+    if (this->mapSaplingNoteData.count(op) == 0) {
         return boost::none;
     }
 
-    const OutputDescription& output = this->vShieldedOutput[op.n];
-    const SaplingNoteData& nd = ndIt->second;
+    auto output = this->vShieldedOutput[op.n];
+    auto nd = this->mapSaplingNoteData.at(op);
 
     auto maybe_pt = SaplingNotePlaintext::decrypt(
         output.encCiphertext,
@@ -5642,26 +5639,6 @@ 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.
@@ -5680,47 +5657,8 @@ void CWallet::GetFilteredNotes(
 
     LogPrintf("%s ignoreLocked=%d\n", __func__, ignoreLocked);
 
-    // 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;
+    for (auto & p : mapWallet) {
+        CWalletTx wtx = p.second;
 
         // Filter the transactions before checking for notes
         if (!CheckFinalTx(wtx) || wtx.GetBlocksToMaturity() > 0)
@@ -5743,43 +5681,31 @@ void CWallet::GetFilteredNotes(
             }
         }
 
-        for (const auto & pair : wtx.mapSaplingNoteData) {
-            const SaplingOutPoint& op = pair.first;
-            const SaplingNoteData& nd = pair.second;
+        for (auto & pair : wtx.mapSaplingNoteData) {
+            SaplingOutPoint op = pair.first;
+            SaplingNoteData nd = pair.second;
 
-            // 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(
+            auto maybe_pt = SaplingNotePlaintext::decrypt(
                 wtx.vShieldedOutput[op.n].encCiphertext,
                 nd.ivk,
                 wtx.vShieldedOutput[op.n].ephemeralKey,
-                wtx.vShieldedOutput[op.n].cm,
-                pk_d);
+                wtx.vShieldedOutput[op.n].cm);
             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);
+            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 && IsSaplingSpent(*nd.nullifier)) {
+                continue;
+            }
+
             // skip notes which cannot be spent
             if (requireSpendingKey) {
                 libzcash::SaplingIncomingViewingKey ivk;
@@ -5797,9 +5723,7 @@ void CWallet::GetFilteredNotes(
                  continue;
              }
 
-            // 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);
+            auto note = notePt.note(nd.ivk).get();
             saplingEntries.push_back(SaplingNoteEntry {
                 op, pa, note, notePt.memo(), wtx.GetDepthInMainChain() });
         }

src/wallet/wallet.h

@@ -896,6 +896,12 @@ 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.
@@ -1210,12 +1216,10 @@ 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 = "", bool fReadOnlyDb = false);
+    TxItems OrderedTxItems(std::list<CAccountingEntry>& acentries, std::string strAccount = "");
 
     void MarkDirty();
     bool UpdateNullifierNoteMap();

src/zcash/Note.cpp

@@ -121,18 +121,6 @@ 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) {
@@ -173,7 +161,6 @@ boost::optional<SaplingNotePlaintext> SaplingNotePlaintext::decrypt_with_pk_d(
         return boost::none;
     }
 
-    pk_d_out = pk_d;
     return ret;
 }
 

src/zcash/Note.hpp

@@ -77,17 +77,6 @@ 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,