Merge pull request #645 from HorizenOfficial/dr/listunspent_improved

Refactor listunspent

doc/release-notes/release-notes-current.md

@@ -6,5 +6,9 @@ zend v5.0.99
 ## New Features and Improvements
 
 ## Bugfixes and Minor Changes
+- PR [#645](https://github.com/HorizenOfficial/zen/pull/645) Refactored listunspent rpc implementation for improved performance
 
 ## Contributors
+* [@ptagl](https://github.com/ptagl)
+* [@drgora](https://github.com/drgora)
+* [@JackPiri](https://github.com/JackPiri)

qa/pull-tester/rpc-tests.sh

@@ -169,6 +169,7 @@ testScripts=(
   'mempool_hard_fork_cleaning.py',156,344
   'shieldedpoolremoval.py',377,855
   'sc_mempool_conflict.py',32,45
+  'listunspent.py',26,41
 );
 
 testScriptsExt=(

qa/rpc-tests/listunspent.py

@@ -0,0 +1,87 @@
+#!/usr/bin/env python3
+# Copyright (c) 2024 The Horizen Foundation
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+from decimal import Decimal
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal, start_nodes, initialize_chain_clean, connect_nodes
+
+ADDITIONAL_TXS = 10
+
+class ListUnspentTest(BitcoinTestFramework):
+
+    def setup_chain(self):
+        print("Initializing test directory "+self.options.tmpdir)
+        initialize_chain_clean(self.options.tmpdir, 4)
+
+    def setup_network(self, split=False):
+        self.nodes = start_nodes(2, self.options.tmpdir)
+        connect_nodes(self.nodes,0,1)
+        self.is_network_split=False
+        self.sync_all()
+
+    def run_test(self):
+        [alice, miner] = self.nodes
+
+        addr_alice = alice.getnewaddress()
+        addr_alice_2 = alice.getnewaddress()
+        addr_miner = miner.getnewaddress()
+
+        miner.generate(101) # need to have spendeable coinbase
+        txid = miner.sendtoaddress(addr_alice, Decimal('1.0'))
+        miner.generate(10)
+        self.sync_all()
+
+        def have_tx(txid, listunspent):
+            for tx in listunspent:
+                if tx['txid'] == txid:
+                    return True
+            return False
+
+        # Check that txid is the only tx in alice's wallet
+        unspent = alice.listunspent()
+        assert(len(unspent) == 1 and unspent[0]['txid'] == txid)
+
+        # Check that parameters are used correctly 
+        unspent = alice.listunspent(10, 10, [addr_alice])
+        assert(len(unspent) == 1 and unspent[0]['txid'] == txid)
+
+        # Check that minDepth is respected (maxDepth is 9999999 as per documentation)
+        unspent = alice.listunspent(11, 9999999, [])
+        assert_equal(len(unspent), 0)
+
+        # Check that maxDepth is respected (minDepth is 1 as per documentation)
+        unspent = alice.listunspent(1, 9, [])
+        assert_equal(len(unspent), 0)
+
+        # Check that address is respected
+        unspent = alice.listunspent(1, 9999999, [addr_alice_2])
+        assert_equal(len(unspent), 0)
+
+        for tx in range(ADDITIONAL_TXS):
+            txid = miner.sendtoaddress(addr_alice_2, Decimal('1.0'))
+            miner.generate(1)
+            self.sync_all()
+            assert(have_tx(txid, alice.listunspent()))
+
+        unspent = alice.listunspent()
+        assert_equal(len(unspent), ADDITIONAL_TXS + 1)
+        assert(have_tx(txid, unspent))
+
+        # Check that locked transactions are not returned by listunspent
+        lock_tx = {"txid": unspent[0]['txid'], "vout": unspent[0]['vout']}
+        assert(alice.lockunspent(False, [lock_tx]))
+        unspent = alice.listunspent()
+        assert_equal(len(unspent), ADDITIONAL_TXS)
+        assert(not have_tx(lock_tx['txid'], unspent))
+
+        # Check that spent transactions are not returned by listunspent
+        alice.lockunspent(True, [lock_tx])
+        alice.sendtoaddress(addr_miner, Decimal(alice.getbalance()), "", "", True)
+        unspent = alice.listunspent()
+        assert_equal(len(unspent), 0)
+
+
+if __name__ == '__main__':
+    ListUnspentTest().main()

src/keystore.cpp

@@ -1,6 +1,7 @@
 // Copyright (c) 2009-2010 Satoshi Nakamoto
 // Copyright (c) 2009-2014 The Bitcoin Core developers
 // Copyright (c) 2018 Zen Blockchain Foundation
+// Copyright (c) 2024 The Horizen Foundation
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -63,6 +64,7 @@ bool CBasicKeyStore::AddWatchOnly(const CScript &dest)
 {
     LOCK(cs_KeyStore);
     setWatchOnly.insert(dest);
+    setWatchOnlyLengths.insert(dest.size());
     return true;
 }
 
@@ -79,13 +81,14 @@ bool CBasicKeyStore::HaveWatchOnly(const CScript &dest) const
 
     /* We assume that dest could be a script with OP_CHECKBLOCKATHEIGHT. In this case we cant search
      * for full match with watchonly scripts, cause OP_CHECKBLOCKATHEIGHT arguments are different all the time.
-     * So, instead, check that dest starts with some of the scripts from setWatchOnly */
-    auto predicate = [&dest](const CScript& script) {
-        auto res = search(begin(dest), end(dest), begin(script), end(script));
-        return res == begin(dest);
-    };
+     * So, instead, check that dest starts with some of the script from setWatchOnly */
+    for (size_t len: setWatchOnlyLengths) {
+        if (len <= dest.size() && setWatchOnly.find(CScript(dest.begin(), dest.begin()+len)) != setWatchOnly.end()) {
+            return true;
+        }
+    }
 
-    return std::find_if(setWatchOnly.begin(), setWatchOnly.end(), predicate) != setWatchOnly.end();
+    return false;
 }
 
 bool CBasicKeyStore::HaveWatchOnly() const

src/keystore.h

@@ -1,5 +1,6 @@
 // Copyright (c) 2009-2010 Satoshi Nakamoto
 // Copyright (c) 2009-2014 The Bitcoin Core developers
+// Copyright (c) 2024 The Horizen Foundation
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -14,6 +15,7 @@
 #include "zcash/Address.hpp"
 #include "zcash/NoteEncryption.hpp"
 
+#include <unordered_set>
 #include <boost/signals2/signal.hpp>
 #include <boost/variant.hpp>
 
@@ -63,9 +65,19 @@ class CKeyStore
     virtual bool GetViewingKey(const libzcash::PaymentAddress &address, libzcash::ViewingKey& vkOut) const =0;
 };
 
+template<> struct std::hash<CScript> {
+    std::size_t operator()(CScript const& s) const noexcept {
+        size_t res = 0;
+        for (size_t i = 0; i < s.size(); i++) {
+            boost::hash_combine(res, s[i]);
+        }
+        return res;
+    }
+};
+
 typedef std::map<CKeyID, CKey> KeyMap;
 typedef std::map<CScriptID, CScript > ScriptMap;
-typedef std::set<CScript> WatchOnlySet;
+typedef std::unordered_set<CScript> WatchOnlySet;
 typedef std::map<libzcash::PaymentAddress, libzcash::SpendingKey> SpendingKeyMap;
 typedef std::map<libzcash::PaymentAddress, libzcash::ViewingKey> ViewingKeyMap;
 typedef std::map<libzcash::PaymentAddress, ZCNoteDecryption> NoteDecryptorMap;
@@ -77,6 +89,7 @@ class CBasicKeyStore : public CKeyStore
     KeyMap mapKeys;
     ScriptMap mapScripts;
     WatchOnlySet setWatchOnly;
+    std::unordered_set<size_t> setWatchOnlyLengths;
     SpendingKeyMap mapSpendingKeys;
     ViewingKeyMap mapViewingKeys;
     NoteDecryptorMap mapNoteDecryptors;

src/univalue/Makefile.am

@@ -19,7 +19,7 @@ libunivalue_la_SOURCES = \
 libunivalue_la_LDFLAGS = \
 	-version-info $(LIBUNIVALUE_CURRENT):$(LIBUNIVALUE_REVISION):$(LIBUNIVALUE_AGE) \
 	-no-undefined
-libunivalue_la_CXXFLAGS = -I$(top_srcdir)/include
+libunivalue_la_CXXFLAGS = -std=c++11 -I$(top_srcdir)/include
 
 TESTS = test/object test/unitester test/no_nul
 
@@ -40,22 +40,22 @@ TEST_DATA_DIR=test
 
 test_unitester_SOURCES = test/unitester.cpp
 test_unitester_LDADD = libunivalue.la
-test_unitester_CXXFLAGS = -I$(top_srcdir)/include -DJSON_TEST_SRC=\"$(srcdir)/$(TEST_DATA_DIR)\"
+test_unitester_CXXFLAGS = $(libunivalue_la_CXXFLAGS) -DJSON_TEST_SRC=\"$(srcdir)/$(TEST_DATA_DIR)\"
 test_unitester_LDFLAGS = -static $(LIBTOOL_APP_LDFLAGS)
 
 test_test_json_SOURCES = test/test_json.cpp
 test_test_json_LDADD = libunivalue.la
-test_test_json_CXXFLAGS = -I$(top_srcdir)/include
+test_test_json_CXXFLAGS = $(libunivalue_la_CXXFLAGS)
 test_test_json_LDFLAGS = -static $(LIBTOOL_APP_LDFLAGS)
 
 test_no_nul_SOURCES = test/no_nul.cpp
 test_no_nul_LDADD = libunivalue.la
-test_no_nul_CXXFLAGS = -I$(top_srcdir)/include
+test_no_nul_CXXFLAGS = $(libunivalue_la_CXXFLAGS)
 test_no_nul_LDFLAGS = -static $(LIBTOOL_APP_LDFLAGS)
 
 test_object_SOURCES = test/object.cpp
 test_object_LDADD = libunivalue.la
-test_object_CXXFLAGS = -I$(top_srcdir)/include
+test_object_CXXFLAGS = $(libunivalue_la_CXXFLAGS)
 test_object_LDFLAGS = -static $(LIBTOOL_APP_LDFLAGS)
 
 TEST_FILES = \

src/univalue/include/univalue.h

@@ -1,5 +1,6 @@
 // Copyright 2014 BitPay Inc.
 // Copyright 2015 Bitcoin Core Developers
+// Copyright (c) 2024 The Horizen Foundation
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or https://opensource.org/licenses/mit-license.php.
 
@@ -99,6 +100,7 @@ class UniValue {
 
     bool push_back(const UniValue& val);
     bool push_backV(const std::vector<UniValue>& vec);
+    UniValue& emplace_back(VType type) { values.emplace_back(type); return values.back(); };
 
     void _pushKV(const std::string& key, const UniValue& val);
     bool pushKV(const std::string& key, const UniValue& val);

src/wallet/gtest/test_wallet.cpp

@@ -1,4 +1,5 @@
 // Copyright (c) 2018-2023 Zen Blockchain Foundation
+// Copyright (c) 2024 The Horizen Foundation
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 #include <gmock/gmock.h>
@@ -1199,3 +1200,62 @@ TEST_F(WalletTest, NoteLocking) {
     EXPECT_FALSE(wallet.IsLockedNote(jsoutpt.hash, jsoutpt.js, jsoutpt.n));
     EXPECT_FALSE(wallet.IsLockedNote(jsoutpt2.hash, jsoutpt2.js, jsoutpt2.n));
 }
+
+TEST_F(WalletTest, HaveWatchOnly) {
+    TestWallet wallet;
+
+    EXPECT_FALSE(wallet.HaveWatchOnly());
+
+    // Add watch only scripts for multiple types
+
+    // P2SH
+    CScriptID script_id;
+    GetRandBytes((unsigned char*)&script_id, sizeof(script_id));
+    CScript p2sh_scriptPubKey_wo_replay;
+    p2sh_scriptPubKey_wo_replay << ToByteVector(script_id) << OP_EQUAL;
+    EXPECT_TRUE(wallet.AddWatchOnly(p2sh_scriptPubKey_wo_replay));
+
+    // P2PK
+    std::vector<unsigned char> rnd_bytes(PUBLIC_KEY_SIZE);
+    GetRandBytes(rnd_bytes.data(), PUBLIC_KEY_SIZE);
+    CPubKey pubkey(rnd_bytes.begin(), rnd_bytes.end());
+    CScript p2pk_scriptPubKey_wo_replay;
+    p2pk_scriptPubKey_wo_replay << OP_HASH160 << ToByteVector(pubkey) << OP_CHECKSIG;
+    EXPECT_TRUE(wallet.AddWatchOnly(p2pk_scriptPubKey_wo_replay));
+
+    // P2PKH
+    CScript p2pkh_scriptPubKey_wo_replay;
+    p2pkh_scriptPubKey_wo_replay << OP_DUP << OP_HASH160 << ToByteVector(pubkey.GetID()) << OP_EQUALVERIFY << OP_CHECKSIG;
+    EXPECT_TRUE(wallet.AddWatchOnly(p2pkh_scriptPubKey_wo_replay));
+
+    EXPECT_TRUE(wallet.HaveWatchOnly());
+
+    // Add replay protection to each of the previous scripts
+    uint256 random_block_hash = GetRandHash();
+
+    CScript p2sh_scriptPubKey_w_replay = p2sh_scriptPubKey_wo_replay;
+    p2sh_scriptPubKey_w_replay << ToByteVector(random_block_hash) << 21 << OP_CHECKBLOCKATHEIGHT;
+
+    CScript p2pk_scriptPubKey_w_replay = p2pk_scriptPubKey_wo_replay;
+    p2pk_scriptPubKey_w_replay << ToByteVector(random_block_hash) << 21 << OP_CHECKBLOCKATHEIGHT;
+
+    CScript p2pkh_scriptPubKey_w_replay = p2pkh_scriptPubKey_wo_replay;
+    p2pkh_scriptPubKey_w_replay << ToByteVector(random_block_hash) << 21 << OP_CHECKBLOCKATHEIGHT;
+
+    // Check that scripts with replay protection are found
+    EXPECT_TRUE(wallet.HaveWatchOnly(p2sh_scriptPubKey_w_replay));
+    EXPECT_TRUE(wallet.HaveWatchOnly(p2pk_scriptPubKey_w_replay));
+    EXPECT_TRUE(wallet.HaveWatchOnly(p2pkh_scriptPubKey_w_replay));
+
+    // Check that scripts without replay protection are found
+    EXPECT_TRUE(wallet.HaveWatchOnly(p2sh_scriptPubKey_wo_replay));
+    EXPECT_TRUE(wallet.HaveWatchOnly(p2pk_scriptPubKey_wo_replay));
+    EXPECT_TRUE(wallet.HaveWatchOnly(p2pkh_scriptPubKey_wo_replay));
+
+    // Check that another script is not found
+    CScriptID another_script_id;
+    GetRandBytes((unsigned char*)&another_script_id, sizeof(another_script_id));
+    CScript another_p2sh_scriptPubKey_w_replay;
+    another_p2sh_scriptPubKey_w_replay << OP_HASH160 << ToByteVector(another_script_id) << OP_EQUAL << ToByteVector(random_block_hash) << 21 << OP_CHECKBLOCKATHEIGHT;
+    EXPECT_FALSE(wallet.HaveWatchOnly(another_p2sh_scriptPubKey_w_replay));
+}

src/wallet/rpcwallet.cpp

@@ -3997,59 +3997,52 @@ UniValue listunspent(const UniValue& params, bool fHelp)
             CBitcoinAddress address(input.get_str());
             if (!address.IsValid())
                 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, string("Invalid horizen address: ")+input.get_str());
-            if (setAddress.count(address))
+            if (!setAddress.insert(address).second)
                 throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, duplicated address: ")+input.get_str());
-           setAddress.insert(address);
         }
     }
 
     UniValue results(UniValue::VARR);
     vector<COutput> vecOutputs;
-    assert(pwalletMain != NULL);
+    assert(pwalletMain);
     LOCK2(cs_main, pwalletMain->cs_wallet);
-    pwalletMain->AvailableCoins(vecOutputs, false, NULL, true, true);
-    for(const COutput& out: vecOutputs) {
-        if (out.nDepth < nMinDepth || out.nDepth > nMaxDepth)
-            continue;
+    auto utxo_map = pwalletMain->AvailableCoinsByAddress(setAddress, nMinDepth, nMaxDepth, false, NULL, true, true);
 
-        if (setAddress.size()) {
-            CTxDestination address;
-            if (!ExtractDestination(out.tx->getTxBase()->GetVout()[out.pos].scriptPubKey, address))
-                continue;
+    // Find unspent coinbase utxos and update estimated size
+    for (const auto& [address, utxo_vec]: utxo_map) {
+        std::string addr_str = CBitcoinAddress(address).ToString();
+        std::string account_name;
 
-            if (!setAddress.count(address))
-                continue;
-        }
+        auto map_entry = pwalletMain->mapAddressBook.find(address);
+        if (map_entry != pwalletMain->mapAddressBook.end())
+            account_name = map_entry->second.name;
 
-        CAmount nValue = out.tx->getTxBase()->GetVout()[out.pos].nValue;
-        const CScript& pk = out.tx->getTxBase()->GetVout()[out.pos].scriptPubKey;
-        UniValue entry(UniValue::VOBJ);
-        entry.pushKV("txid", out.tx->getTxBase()->GetHash().GetHex());
-        entry.pushKV("vout", out.pos);
-        entry.pushKV("isCert", out.tx->getTxBase()->IsCertificate());
-        entry.pushKV("generated", out.tx->getTxBase()->IsCoinBase());
 
-        CTxDestination address;
-        if (ExtractDestination(out.tx->getTxBase()->GetVout()[out.pos].scriptPubKey, address)) {
-            entry.pushKV("address", CBitcoinAddress(address).ToString());
-            if (pwalletMain->mapAddressBook.count(address))
-                entry.pushKV("account", pwalletMain->mapAddressBook[address].name);
-        }
-        entry.pushKV("scriptPubKey", HexStr(pk.begin(), pk.end()));
-        if (pk.IsPayToScriptHash()) {
-            CTxDestination address;
-            if (ExtractDestination(pk, address)) {
-                const CScriptID& hash = boost::get<CScriptID>(address);
+        for (const COutput& out: utxo_vec) {
+            CAmount nValue = out.tx->getTxBase()->GetVout()[out.pos].nValue;
+            const CScript& pk = out.tx->getTxBase()->GetVout()[out.pos].scriptPubKey;
+            UniValue& entry = results.emplace_back(UniValue::VOBJ);
+            entry.pushKV("txid", out.tx->getTxBase()->GetHash().GetHex());
+            entry.pushKV("vout", out.pos);
+            entry.pushKV("isCert", out.tx->getTxBase()->IsCertificate());
+            entry.pushKV("generated", out.tx->getTxBase()->IsCoinBase());
+
+            entry.pushKV("address", addr_str);
+            if (!account_name.empty())
+                entry.pushKV("account", account_name);
+
+            entry.pushKV("scriptPubKey", HexStr(pk.begin(), pk.end()));
+            if (pk.IsPayToScriptHash()) {
+                const CScriptID& script_hash = boost::get<CScriptID>(address);
                 CScript redeemScript;
-                if (pwalletMain->GetCScript(hash, redeemScript))
+                if (pwalletMain->GetCScript(script_hash, redeemScript))
                     entry.pushKV("redeemScript", HexStr(redeemScript.begin(), redeemScript.end()));
             }
+            entry.pushKV("amount",ValueFromAmount(nValue));
+            entry.pushKV("satoshis", nValue);
+            entry.pushKV("confirmations",out.nDepth);
+            entry.pushKV("spendable", out.fSpendable);
         }
-        entry.pushKV("amount",ValueFromAmount(nValue));
-        entry.pushKV("satoshis", nValue);
-        entry.pushKV("confirmations",out.nDepth);
-        entry.pushKV("spendable", out.fSpendable);
-        results.push_back(entry);
     }
 
     return results;