Merge #6083: refactor: move Get*Index to rpc/index_util.cpp, const-ify functions and arguments, add lock annotations and some minor housekeeping

8fb8630 refactor: inline sorting and make available through argument (Kittywhiskers Van Gogh)
3e0fcf4 refactor: move accessing CBlockTreeDB global out of Get*Index (Kittywhiskers Van Gogh)
ee9d112 refactor: move pair value swapping out of CTxMemPool::getAddressIndex() (Kittywhiskers Van Gogh)
808842b refactor: define all key/value pairings as *Entry and use them instead (Kittywhiskers Van Gogh)
488f047 rpc: extend error-on-failure to GetSpentIndex (Kittywhiskers Van Gogh)
9a6503d refactor: make CBlockTreeDB::Read*Index arguments const (Kittywhiskers Van Gogh)
625982e refactor: make CTxMemPool::get*Index functions and arguments const (Kittywhiskers Van Gogh)
5ad49ad refactor: move Get{Address*, Timestamp, Spent}Index out of validation (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  This pull request is motivated by [bitcoin#22371](bitcoin#22371), which gets rid of the `pblocktree` global.

  The sole usage of `pblocktree` introduced by Dash is for managing our {address, spent, timestamp} indexes with most of invocations within `BlockManager` or `CChainState`, granting them internal access to `pblocktree` (now `m_block_tree_db`). The sole exception being `Get*Index`, that relies on accessing the global and has no direct internal access.

  This pull request aims to refactor code associated with `Get*Index` with the eventual aim of moving gaining access to the global out of the function. `Get*Index` is called exclusively called through RPC, which makes giving it access to `ChainstateManager` quite easy, which makes switching from the global to accessing it through `ChainstateManager` when backporting  [bitcoin#22371](bitcoin#22371) a drop-in replacement.

  Alongside that, the surrounding code has been given some TLC:

  * Moving code out of `validation.cpp` and into `rpc/index_util.cpp`. The code is exclusively used in RPC logic and doesn't aid in validation.
  * Add lock annotations for accessing `pblocktree` (while already protected by `::cs_main`, said protection is currently not enforced but will be once moved into `BlockManager` in the backport)
  * `const`-ing input arguments and using pass-by-value for input arguments that can be written inline (i.e. types like `CSpentIndexKey` are still pass-by-ref).
    * While `const`ing `CTxMemPool` functions were possible (courtesy of the presence of `const_iterator`s), the same is currently not possible with `CBlockTreeDB` functions as the iterator is non-`const`.
  * Extend error messages to `GetSpentIndex` to bring it line with other `Get*Index`es.
  * Define key-value pairings as a `*Entry` typedef and replacing all explicit type constructions with it.
  * Make `CTxMemPool::getAddressIndex` indifferent to how `CMempoolAddressDeltaKey` is constructed.
    * Current behaviour is to accept a `std::pair<uint160, AddressType>` and construct the `CMempoolAddressDeltaKey` internally, this was presumably done to account for the return type of `getAddressesFromParams` in the sole call for the `CTxMemPool::getAddressIndex`.
    * This has been changed, moving the construction into the RPC call.
   * Moving {height, timestamp} sorting out of RPC and into the applicable `Get*Index` functions.

  ## Breaking Changes

  None expected.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas (note: N/A)
  - [x] I have added or updated relevant unit/integration/functional/e2e tests (note: N/A)
  - [x] I have made corresponding changes to the documentation (note: N/A)
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 8fb8630
  UdjinM6:
    utACK 8fb8630

Tree-SHA512: 425a383e8284bbd74a5e9bcda4a9d7988221197055f43faa591e6f0d579625cee28f6a6046dab951e7afa0c3e33af1778fb4bb5f0a2e1e5792fe0d9396897a14

Author: PastaPastaPasta

src/Makefile.am

@@ -286,6 +286,7 @@ BITCOIN_CORE_H = \
   reverse_iterator.h \
   rpc/blockchain.h \
   rpc/client.h \
+  rpc/index_util.h \
   rpc/mining.h \
   rpc/protocol.h \
   rpc/rawtransaction_util.h \
@@ -502,6 +503,7 @@ libbitcoin_server_a_SOURCES = \
   rpc/blockchain.cpp \
   rpc/coinjoin.cpp \
   rpc/evo.cpp \
+  rpc/index_util.cpp \
   rpc/masternode.cpp \
   rpc/governance.cpp \
   rpc/mining.cpp \

src/addressindex.h

@@ -17,6 +17,9 @@
 #include <tuple>
 
 class CScript;
+struct CAddressIndexKey;
+struct CMempoolAddressDelta;
+struct CMempoolAddressDeltaKey;
 
 enum class AddressType : uint8_t {
     P2PK_OR_P2PKH = 1,
@@ -26,6 +29,9 @@ enum class AddressType : uint8_t {
 };
 template<> struct is_serializable_enum<AddressType> : std::true_type {};
 
+using CAddressIndexEntry = std::pair<CAddressIndexKey, CAmount>;
+using CMempoolAddressDeltaEntry = std::pair<CMempoolAddressDeltaKey, CMempoolAddressDelta>;
+
 struct CMempoolAddressDelta
 {
 public:

src/rpc/blockchain.cpp

@@ -30,6 +30,7 @@
 #include <policy/fees.h>
 #include <policy/policy.h>
 #include <primitives/transaction.h>
+#include <rpc/index_util.h>
 #include <rpc/server.h>
 #include <rpc/server_util.h>
 #include <rpc/util.h>
@@ -834,7 +835,7 @@ static RPCHelpMan getblockhashes()
     unsigned int low = request.params[1].get_int();
     std::vector<uint256> blockHashes;
 
-    if (!GetTimestampIndex(high, low, blockHashes)) {
+    if (LOCK(::cs_main); !GetTimestampIndex(*pblocktree, high, low, blockHashes)) {
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for block hashes");
     }
 

src/rpc/index_util.cpp

@@ -0,0 +1,98 @@
+// Copyright (c) 2016 BitPay, Inc.
+// Copyright (c) 2024 The Dash Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <rpc/index_util.h>
+
+#include <txmempool.h>
+#include <uint256.h>
+#include <validation.h>
+
+bool GetAddressIndex(CBlockTreeDB& block_tree_db, const uint160& addressHash, const AddressType type,
+                     std::vector<CAddressIndexEntry>& addressIndex,
+                     const int32_t start, const int32_t end)
+{
+    AssertLockHeld(::cs_main);
+
+    if (!fAddressIndex)
+        return error("Address index not enabled");
+
+    if (!block_tree_db.ReadAddressIndex(addressHash, type, addressIndex, start, end))
+        return error("Unable to get txids for address");
+
+    return true;
+}
+
+bool GetAddressUnspentIndex(CBlockTreeDB& block_tree_db, const uint160& addressHash, const AddressType type,
+                            std::vector<CAddressUnspentIndexEntry>& unspentOutputs, const bool height_sort)
+{
+    AssertLockHeld(::cs_main);
+
+    if (!fAddressIndex)
+        return error("Address index not enabled");
+
+    if (!block_tree_db.ReadAddressUnspentIndex(addressHash, type, unspentOutputs))
+        return error("Unable to get txids for address");
+
+    if (height_sort) {
+        std::sort(unspentOutputs.begin(), unspentOutputs.end(),
+                  [](const CAddressUnspentIndexEntry &a, const CAddressUnspentIndexEntry &b) {
+                        return a.second.m_block_height < b.second.m_block_height;
+                  });
+    }
+
+    return true;
+}
+
+bool GetMempoolAddressDeltaIndex(const CTxMemPool& mempool,
+                                 const std::vector<CMempoolAddressDeltaKey>& addressDeltaIndex,
+                                 std::vector<CMempoolAddressDeltaEntry>& addressDeltaEntries,
+                                 const bool timestamp_sort)
+{
+    if (!fAddressIndex)
+        return error("Address index not enabled");
+
+    if (!mempool.getAddressIndex(addressDeltaIndex, addressDeltaEntries))
+        return error("Unable to get address delta information");
+
+    if (timestamp_sort) {
+        std::sort(addressDeltaEntries.begin(), addressDeltaEntries.end(),
+                  [](const CMempoolAddressDeltaEntry &a, const CMempoolAddressDeltaEntry &b) {
+                        return a.second.m_time < b.second.m_time;
+                  });
+    }
+
+    return true;
+}
+
+bool GetSpentIndex(CBlockTreeDB& block_tree_db, const CTxMemPool& mempool, const CSpentIndexKey& key,
+                   CSpentIndexValue& value)
+{
+    AssertLockHeld(::cs_main);
+
+    if (!fSpentIndex)
+        return error("Spent index not enabled");
+
+    if (mempool.getSpentIndex(key, value))
+        return true;
+
+    if (!block_tree_db.ReadSpentIndex(key, value))
+        return error("Unable to get spend information");
+
+    return true;
+}
+
+bool GetTimestampIndex(CBlockTreeDB& block_tree_db, const uint32_t high, const uint32_t low,
+                       std::vector<uint256>& hashes)
+{
+    AssertLockHeld(::cs_main);
+
+    if (!fTimestampIndex)
+        return error("Timestamp index not enabled");
+
+    if (!block_tree_db.ReadTimestampIndex(high, low, hashes))
+        return error("Unable to get hashes for timestamps");
+
+    return true;
+}

src/rpc/index_util.h

@@ -0,0 +1,45 @@
+// Copyright (c) 2016 BitPay, Inc.
+// Copyright (c) 2024 The Dash Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_RPC_INDEX_UTIL_H
+#define BITCOIN_RPC_INDEX_UTIL_H
+
+#include <cstdint>
+#include <vector>
+
+#include <amount.h>
+#include <addressindex.h>
+#include <spentindex.h>
+#include <sync.h>
+#include <threadsafety.h>
+
+class CBlockTreeDB;
+class CTxMemPool;
+class uint160;
+class uint256;
+
+enum class AddressType : uint8_t;
+
+extern RecursiveMutex cs_main;
+
+bool GetAddressIndex(CBlockTreeDB& block_tree_db, const uint160& addressHash, const AddressType type,
+                     std::vector<CAddressIndexEntry>& addressIndex,
+                     const int32_t start = 0, const int32_t end = 0)
+    EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+bool GetAddressUnspentIndex(CBlockTreeDB& block_tree_db, const uint160& addressHash, const AddressType type,
+                            std::vector<CAddressUnspentIndexEntry>& unspentOutputs, const bool height_sort = false)
+    EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+bool GetMempoolAddressDeltaIndex(const CTxMemPool& mempool,
+                                 const std::vector<CMempoolAddressDeltaKey>& addressDeltaIndex,
+                                 std::vector<CMempoolAddressDeltaEntry>& addressDeltaEntries,
+                                 const bool timestamp_sort = false);
+bool GetSpentIndex(CBlockTreeDB& block_tree_db, const CTxMemPool& mempool, const CSpentIndexKey& key,
+                   CSpentIndexValue& value)
+    EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+bool GetTimestampIndex(CBlockTreeDB& block_tree_db, const uint32_t high, const uint32_t low,
+                       std::vector<uint256>& hashes)
+    EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+
+#endif // BITCOIN_RPC_CLIENT_H

src/rpc/misc.cpp

@@ -19,6 +19,7 @@
 #include <net.h>
 #include <node/context.h>
 #include <rpc/blockchain.h>
+#include <rpc/index_util.h>
 #include <rpc/server.h>
 #include <rpc/server_util.h>
 #include <rpc/util.h>
@@ -700,16 +701,6 @@ static bool getAddressesFromParams(const UniValue& params, std::vector<std::pair
     return true;
 }
 
-static bool heightSort(std::pair<CAddressUnspentKey, CAddressUnspentValue> a,
-                std::pair<CAddressUnspentKey, CAddressUnspentValue> b) {
-    return a.second.m_block_height < b.second.m_block_height;
-}
-
-static bool timestampSort(std::pair<CMempoolAddressDeltaKey, CMempoolAddressDelta> a,
-                   std::pair<CMempoolAddressDeltaKey, CMempoolAddressDelta> b) {
-    return a.second.m_time < b.second.m_time;
-}
-
 static RPCHelpMan getaddressmempool()
 {
     return RPCHelpMan{"getaddressmempool",
@@ -741,22 +732,22 @@ static RPCHelpMan getaddressmempool()
         },
     [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
+    CTxMemPool& mempool = EnsureAnyMemPool(request.context);
 
-    std::vector<std::pair<uint160, AddressType> > addresses;
-
+    std::vector<std::pair<uint160, AddressType>> addresses;
     if (!getAddressesFromParams(request.params, addresses)) {
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
     }
 
-    std::vector<std::pair<CMempoolAddressDeltaKey, CMempoolAddressDelta> > indexes;
-
-    CTxMemPool& mempool = EnsureAnyMemPool(request.context);
-    if (!mempool.getAddressIndex(addresses, indexes)) {
+    std::vector<CMempoolAddressDeltaKey> input_addresses;
+    std::vector<CMempoolAddressDeltaEntry> indexes;
+    for (const auto& [hash, type] : addresses) {
+        input_addresses.push_back({type, hash});
+    }
+    if (!GetMempoolAddressDeltaIndex(mempool, input_addresses, indexes, /* timestamp_sort = */ true)) {
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for address");
     }
 
-    std::sort(indexes.begin(), indexes.end(), timestampSort);
-
     UniValue result(UniValue::VARR);
 
     for (const auto& [mempoolAddressKey, mempoolAddressDelta] : indexes) {
@@ -820,16 +811,18 @@ static RPCHelpMan getaddressutxos()
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
     }
 
-    std::vector<std::pair<CAddressUnspentKey, CAddressUnspentValue> > unspentOutputs;
+    std::vector<CAddressUnspentIndexEntry> unspentOutputs;
 
-    for (const auto& address : addresses) {
-        if (!GetAddressUnspent(address.first, address.second, unspentOutputs)) {
-            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for address");
+    {
+        LOCK(::cs_main);
+        for (const auto& address : addresses) {
+            if (!GetAddressUnspentIndex(*pblocktree, address.first, address.second, unspentOutputs,
+                                        /* height_sort = */ true)) {
+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for address");
+            }
         }
     }
 
-    std::sort(unspentOutputs.begin(), unspentOutputs.end(), heightSort);
-
     UniValue result(UniValue::VARR);
 
     for (const auto& [unspentKey, unspentValue] : unspentOutputs) {
@@ -905,16 +898,19 @@ static RPCHelpMan getaddressdeltas()
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
     }
 
-    std::vector<std::pair<CAddressIndexKey, CAmount> > addressIndex;
+    std::vector<CAddressIndexEntry> addressIndex;
 
-    for (const auto& address : addresses) {
-        if (start > 0 && end > 0) {
-            if (!GetAddressIndex(address.first, address.second, addressIndex, start, end)) {
-                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for address");
-            }
-        } else {
-            if (!GetAddressIndex(address.first, address.second, addressIndex)) {
-                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for address");
+    {
+        LOCK(::cs_main);
+        for (const auto& address : addresses) {
+            if (start > 0 && end > 0) {
+                if (!GetAddressIndex(*pblocktree, address.first, address.second, addressIndex, start, end)) {
+                    throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for address");
+                }
+            } else {
+                if (!GetAddressIndex(*pblocktree, address.first, address.second, addressIndex)) {
+                    throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for address");
+                }
             }
         }
     }
@@ -974,16 +970,21 @@ static RPCHelpMan getaddressbalance()
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
     }
 
-    std::vector<std::pair<CAddressIndexKey, CAmount> > addressIndex;
+    std::vector<CAddressIndexEntry> addressIndex;
 
-    for (const auto& address : addresses) {
-        if (!GetAddressIndex(address.first, address.second, addressIndex)) {
-            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for address");
+    ChainstateManager& chainman = EnsureAnyChainman(request.context);
+
+    int nHeight;
+    {
+        LOCK(::cs_main);
+        for (const auto& address : addresses) {
+            if (!GetAddressIndex(*pblocktree, address.first, address.second, addressIndex)) {
+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for address");
+            }
         }
+        nHeight = chainman.ActiveChain().Height();
     }
 
-    ChainstateManager& chainman = EnsureAnyChainman(request.context);
-    int nHeight = WITH_LOCK(cs_main, return chainman.ActiveChain().Height());
 
     CAmount balance = 0;
     CAmount balance_spendable = 0;
@@ -1053,16 +1054,19 @@ static RPCHelpMan getaddresstxids()
         }
     }
 
-    std::vector<std::pair<CAddressIndexKey, CAmount> > addressIndex;
+    std::vector<CAddressIndexEntry> addressIndex;
 
-    for (const auto& address : addresses) {
-        if (start > 0 && end > 0) {
-            if (!GetAddressIndex(address.first, address.second, addressIndex, start, end)) {
-                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for address");
-            }
-        } else {
-            if (!GetAddressIndex(address.first, address.second, addressIndex)) {
-                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for address");
+    {
+        LOCK(::cs_main);
+        for (const auto& address : addresses) {
+            if (start > 0 && end > 0) {
+                if (!GetAddressIndex(*pblocktree, address.first, address.second, addressIndex, start, end)) {
+                    throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for address");
+                }
+            } else {
+                if (!GetAddressIndex(*pblocktree, address.first, address.second, addressIndex)) {
+                    throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for address");
+                }
             }
         }
     }
@@ -1134,7 +1138,7 @@ static RPCHelpMan getspentinfo()
     CSpentIndexValue value;
 
     CTxMemPool& mempool = EnsureAnyMemPool(request.context);
-    if (!GetSpentIndex(mempool, key, value)) {
+    if (LOCK(::cs_main); !GetSpentIndex(*pblocktree, mempool, key, value)) {
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Unable to get spent info");
     }