[Validation] Introduce rolling cache for block hashes in masternode manager #1904
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
random-zebra
added
RPC
Validation
Needs Release Notes
random-zebra
force-pushed
the
202010_mnping_faster-validation
branch
from
af6f11b
to
bb53c17
Compare
|
Rebased on master and added functional test. |
|
Rebase needed. Small conflicts with master |
random-zebra
force-pushed
the
202010_mnping_faster-validation
branch
from
bb53c17
to
3e3059a
Compare
|
Done. |
furszy
reviewed
Oct 19, 2020
| } else { | ||
| // Use chainActive | ||
| LOCK(cs_main); | ||
| return chainActive[nHeight]->GetBlockHash(); |
There was a problem hiding this comment.
Note for the future: this cs_main could be a potential deadlock in GetNextMasternodeInQueueForPayment, where we are locking the internal CMasternodeMan::cs first.
There was a problem hiding this comment.
Yeah. It should be addressed in the future (even before deterministic masternodes, GetNextMasternodeInQueueForPayment can be refactored, rearranging its locks properly and removing the redundancies).
But, at least with this patch, it's only on blocks deeper than 200.
Currently on master we lock it for every block (even twice inside CalculateScore).
which can be used for testing the contents of the cycling vector
If the height is within the cache size. Otherwise get the hash from chainActive. No need to use the convoluted GetBlockHash in masternode.cpp
Not used anymore
removes cs_main lock to access chainActive in CMasternodePing::CheckAndUpdate
to check the validity of the contents of the mnmanager block-hashes rolling cache
random-zebra
force-pushed
the
202010_mnping_faster-validation
branch
from
3e3059a
to
dd67066
Compare
|
Nit addressed and PR rebased on master. |
Fuzzbawls
approved these changes
Oct 20, 2020
furszy
added a commit
that referenced
this pull request
Oct 25, 2020
872da91 Speedup listmasternodes removing a redundant for loop over every MN on the mnnodeman. (furszy) ea434a1 CMasternodeMan::GetMasternodeRanks removed unneeded MN extra validations and totally unnecessary extra vector load. (furszy) Pull request description: Built on top of #1904 . PR starting on 9123d22. `listmasternodes` RPC command was taking a considerable amount of time, checked the flow and discovered a lot of redundancies in the process. This PR aims to correct them. 1) `CMasternodeMan::GetMasternodeRanks` is only used for an RPC call and it was checking and validating each Masternode instead of just retrieve them as it suppose to be doing (the tier two thread is the only one responsible of check and update the masternodes list). 2) `CMasternodeMan::GetMasternodeRanks` had a totally unneeded second vector load. Looping over the entire masternodes list one more time after getting it. 3) `listmasternodes` after calling `GetMasternodeRanks` was looking for each of the retrieved masternodes one more time using the retrieved masternode vin (yes..). ACKs for top commit: random-zebra: ACK 872da91 Fuzzbawls: ACK 872da91 Tree-SHA512: 52db4488ec429594a8fa6cacceaaf494ec0da179929b7de30f8698cbbb2d940c0d859f8a0082dce33ead5227066079011f30480434ab935906e12aa80b4199a3
random-zebra
removed
the
Needs Release Notes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem:
Validation of many objects on the second layer (masternode pings, winners/scores, etc...) requires access to chainActive to get the hashes of blocks at a certain height. This is done very often (especially for masternode pings) and means holding
cs_mainlock every time, making the process slow and possibly leading to locking order inconsistencies.Solution:
Cache the last N block hashes in the masternode manager.
In order to implement this efficiently, we introduce a new data structure called
CyclingVector, which is a fixed-size vector, exposing only a setterSet(i, value)which modifies the indexi % N.The cache is initialized at startup and updated when blocks get connected to / disconnected from the tip.
The size (
CACHED_BLOCK_HASHES) is currently set a default value of 200 making it possible to validate all masternode pings (but not all masternode winners) without accessing chainActive. In the future we could add a startup flag to let the user customize this (higher cache size would, as usual, mean faster execution but higher memory usage).An RPC
getcachedblockhashesis introduced, in order to print the content of the cache. This can be used later in the functional test suite for better testing.This PR also removes the accesses to chainActive to get the current chain-height (using a value cached in the manager).