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

[kwvg]

Additional Information

This pull request is motivated by 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 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 consting CTxMemPool functions were possible (courtesy of the presence of const_iterators), 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*Indexes.
• 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)

[PastaPastaPasta]

I'm still confused by all the added cs_mains. You say

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)

and I don't see where cs_main locks are being acquired in develop for these cases. Can you help me figure that out?

[github-actions[bot]]

This pull request has conflicts, please rebase.

[kwvg]

I don't see where cs_main locks are being acquired in develop for these cases. Can you help me figure that out?

@PastaPastaPasta

pblocktree is commented in Dash Core as being protected by ::cs_main in develop (source) but this isn't enforced (lack of EXCLUSIVE_LOCKS_REQUIRED).

In Bitcoin Core, the usage of pblocktree is mostly limited to functions like LoadBlockIndex{DB}, which are protected by ::cs_main (source, source) and FlushStateToDisk, which locks ::cs_main (source), Dash Core differs in that it utilizes pblocktree to manage its indexes (which for the most part are covered by the locks mentioned above) but also uses it in RPC code (via Get*Index, which aren't).

The lack of explicit annotations meant that the safety assumed from the annotations given to functions utilizing pblocktree doesn't extend to RPC calls as no annotations are present there at all.

The annotation is made explicit in https://github.com/bitcoin/bitcoin/pull/22371 (source, renamed as m_block_tree_db), which is part of a series of backports currently undergoing testing. In preparation of those series of backports, this pull request was opened to add explicit annotations (with added cleanups).