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

kwvg · 2024-06-26T19:31:53Z

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:

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

UdjinM6

LGTM, one suggestion

src/rpc/index_util.cpp

UdjinM6

utACK 13abc0f

src/rpc/blockchain.cpp

src/rpc/misc.cpp

PastaPastaPasta · 2024-06-27T21:47:48Z

src/rpc/misc.cpp

+    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();
    }


Suggested change

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();

}

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");

}

}

return chainman.ActiveChain().Height();

}();

Fetching nHeight and calling GetAddressIndex are two independent actions that happen to access variables protected by the same RecursiveMutex, for that reason they were placed within the same scope. The proposed refactor makes it appears as if running GetAddressIndex is part of the process for fetching nHeight, when it's not.

disagree but nit

src/rpc/misc.cpp

PastaPastaPasta · 2024-06-27T21:49:33Z

src/txmempool.cpp

 {
    LOCK(cs);
-    mapSpentIndex::iterator it;
+    mapSpentIndex::const_iterator it = mapSpent.find(key);


maybe?

Suggested change

mapSpentIndex::const_iterator it = mapSpent.find(key);

const auto it = mapSpent.find(key);

PastaPastaPasta · 2024-06-27T21:51:49Z

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 · 2024-06-27T22:39:30Z

This pull request has conflicts, please rebase.

With bonus const'ing of CTxMemPool::get*Index

With bonus code formatting. We cannot make the functions themselves const as the iterators are non-const.

kwvg · 2024-06-28T09:53:42Z

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 bitcoin#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).

PastaPastaPasta

utACK 8fb8630

UdjinM6

utACK 8fb8630

, bitcoin#23174, bitcoin#23785, bitcoin#23581, bitcoin#23974, bitcoin#22932, bitcoin#24050, bitcoin#24515 (blockstorage backports) 1bf0bf4 merge bitcoin#24515: Only load BlockMan in BlockMan member functions (Kittywhiskers Van Gogh) 5c1eb67 merge bitcoin#24050: Give m_block_index ownership of CBlockIndexes (Kittywhiskers Van Gogh) c440304 merge bitcoin#22932: Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main (Kittywhiskers Van Gogh) e303a4e merge bitcoin#23974: Make blockstorage globals private members of BlockManager (Kittywhiskers Van Gogh) 301163c merge bitcoin#23581: Move BlockManager to node/blockstorage (Kittywhiskers Van Gogh) 732e871 merge bitcoin#23785: Move stuff to ChainstateManager (Kittywhiskers Van Gogh) b402fd5 merge bitcoin#23174: have LoadBlockIndex account for snapshot use (Kittywhiskers Van Gogh) a08f2f4 merge bitcoin#21526: UpdateTip/CheckBlockIndex assumeutxo support (Kittywhiskers Van Gogh) 472caa0 merge bitcoin#22371: Move pblocktree global to BlockManager (Kittywhiskers Van Gogh) d69ca83 merge bitcoin#21727: Move more stuff to blockstorage (Kittywhiskers Van Gogh) 6df927f chore: exclude underscore placeholder from shadowing linter warnings (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6078 * Dependent on #6074 * Dependent on #6083 * Dependent on #6119 * Dependency for #6138 * In [bitcoin#24050](bitcoin#24050), `BlockMap` is given ownership of the `CBlockIndex` instance contained within the `unordered_map`. The same has not been done for `PrevBlockMap` as `PrevBlockMap` is populated with `pprev` pointers and doing so seems to break validation logic. * Dash has a specific linter for all Dash-specific code present in Core. The introduction of `util/translation.h` into `validation.h` has caused the linter to trigger shadowing warnings due to a conflict between the common use of `_` as a placeholder/throwaway name ([source](https://github.com/dashpay/dash/blob/37e026a038a60313214e01b6aba029809ea7ad39/src/spork.cpp#L44)) and upstream's usage of it to process translatable strings ([source](https://github.com/dashpay/dash/blob/37e026a038a60313214e01b6aba029809ea7ad39/src/util/translation.h#L55-L62)). Neither C++17 nor C++20 have an _official_ placeholder/throwaway term or annotation for structured bindings (which cannot use `[[maybe_unused]` or `std::ignore`) but [P2169](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2169r4.pdf) is a proposal put forth to make it the official placeholder, in that light, the linter will silence shadowing warnings involving an underscore. ## 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 - [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: UdjinM6: utACK 1bf0bf4 (with one nit) knst: utACK 1bf0bf4 PastaPastaPasta: utACK 1bf0bf4 Tree-SHA512: 875fff34fe91916722f017526135697466e521d7179c473a5c0c444e3aa873369019b804dee9f5f795fc7ebed5c2481b5ce2d895b2950782a37de7b098157ad4

kwvg added this to the 21 milestone Jun 26, 2024

kwvg marked this pull request as ready for review June 27, 2024 05:10

kwvg requested review from knst, UdjinM6 and PastaPastaPasta June 27, 2024 05:10

UdjinM6 reviewed Jun 27, 2024

View reviewed changes

src/rpc/index_util.cpp Outdated Show resolved Hide resolved

kwvg force-pushed the idx_upd branch from 1daa2e5 to 13abc0f Compare June 27, 2024 18:20

kwvg requested a review from UdjinM6 June 27, 2024 18:36

UdjinM6 previously approved these changes Jun 27, 2024

View reviewed changes

PastaPastaPasta reviewed Jun 27, 2024

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

PastaPastaPasta reviewed Jun 27, 2024

View reviewed changes

src/rpc/misc.cpp Outdated Show resolved Hide resolved

PastaPastaPasta reviewed Jun 27, 2024

View reviewed changes

src/rpc/misc.cpp Outdated Show resolved Hide resolved

PastaPastaPasta reviewed Jun 27, 2024

View reviewed changes

github-actions bot added the needs rebase label Jun 27, 2024

kwvg added 8 commits June 28, 2024 08:14

refactor: move Get{Address*, Timestamp, Spent}Index out of validation

5ad49ad

With bonus const'ing of CTxMemPool::get*Index

refactor: make CTxMemPool::get*Index functions and arguments const

625982e

refactor: make CBlockTreeDB::Read*Index arguments const

9a6503d

With bonus code formatting. We cannot make the functions themselves const as the iterators are non-const.

rpc: extend error-on-failure to GetSpentIndex

488f047

refactor: define all key/value pairings as *Entry and use them instead

808842b

refactor: move pair value swapping out of CTxMemPool::getAddressIndex()

ee9d112

refactor: move accessing CBlockTreeDB global out of Get*Index

3e0fcf4

refactor: inline sorting and make available through argument

8fb8630

kwvg dismissed UdjinM6’s stale review via 8fb8630 June 28, 2024 09:35

kwvg force-pushed the idx_upd branch from 13abc0f to 8fb8630 Compare June 28, 2024 09:35

github-actions bot removed the needs rebase label Jun 28, 2024

kwvg requested a review from UdjinM6 June 28, 2024 15:23

kwvg requested a review from PastaPastaPasta June 28, 2024 15:23

kwvg mentioned this pull request Jun 28, 2024

backport: merge bitcoin#21727, #22371, #21526, #23174, #23785, #23581, #23974, #22932, #24050, #24515 (blockstorage backports) #6085

Merged

5 tasks

PastaPastaPasta approved these changes Jun 28, 2024

View reviewed changes

UdjinM6 approved these changes Jun 28, 2024

View reviewed changes

PastaPastaPasta merged commit c1de83b into dashpay:develop Jun 29, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kwvg commented Jun 26, 2024

UdjinM6 left a comment

UdjinM6 left a comment •

edited

Loading

PastaPastaPasta Jun 27, 2024

kwvg Jun 28, 2024

PastaPastaPasta Jun 28, 2024

PastaPastaPasta Jun 27, 2024

PastaPastaPasta commented Jun 27, 2024

github-actions bot commented Jun 27, 2024

kwvg commented Jun 28, 2024 •

edited

Loading

PastaPastaPasta left a comment

UdjinM6 left a comment

	mapSpentIndex::const_iterator it = mapSpent.find(key);
	const auto it = mapSpent.find(key);

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

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

Conversation

kwvg commented Jun 26, 2024

Additional Information

Breaking Changes

Checklist:

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment • edited Loading

Choose a reason for hiding this comment

PastaPastaPasta Jun 27, 2024

Choose a reason for hiding this comment

kwvg Jun 28, 2024

Choose a reason for hiding this comment

PastaPastaPasta Jun 28, 2024

Choose a reason for hiding this comment

PastaPastaPasta Jun 27, 2024

Choose a reason for hiding this comment

PastaPastaPasta commented Jun 27, 2024

github-actions bot commented Jun 27, 2024

kwvg commented Jun 28, 2024 • edited Loading

PastaPastaPasta left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment •

edited

Loading

kwvg commented Jun 28, 2024 •

edited

Loading