Refactoring / cleanup / fixes for deferred masternode starts #50
Conversation
domob1812
force-pushed
the
refactor-for-mnvault
branch
from
87e0275 to
dde2c37
Compare
divi/src/masternode.cpp
Outdated
| return nullptr; | ||
|
|
||
| assert(conf->GetAncestor(mi->second->nHeight) == mi->second); | ||
| return conf; |
There was a problem hiding this comment.
Return value should be mi->second->nHeight in order to make sense with it's usage inside GetMasternodeInputAge() which may be better renamed to GetNumberOfConfirmationBlocksForCollateral() ?
There was a problem hiding this comment.
The height alone is not enough. We need also the block time of the block (conf->GetBlockTime()) later in CheckInputs. That is why the method returns the CBlockIndex* of when the collateral got 15 confirmations, rather than just the collateral height.
divi/src/masternode.cpp
Outdated
| LogPrint("masternode","mnb - Input must have at least %d confirmations\n", MASTERNODE_MIN_CONFIRMATIONS); | ||
| return false; | ||
| } | ||
| collateralHeight = confBlock->nHeight - MASTERNODE_MIN_CONFIRMATIONS + 1; |
There was a problem hiding this comment.
This should just be collateralHeight = confBlock->nHeight as per the previous comment
There was a problem hiding this comment.
Also curious about the masternode input age indirectly being cached by virtue of the collateralHeight having a fixed value except when re-adding the node through CheckInputs().
Since we're accessing chainActive directly, might be worth just caching the blockhash as well as the height and checking it matches ?
There was a problem hiding this comment.
About the first comment, see my reply above. About the second comment, that indeed makes sense. I will update the code to cache the block hash instead, which is more robust against chain reorgs.
There was a problem hiding this comment.
I've updated the change now accordingly. The GetCollateralBlock function now returns directly the block where the collateral was confirmed, not 14 blocks after - this is certainly less confusing.
And the cache is now directly on the hash of that block, so that we can verify if it is still contained in chainActive and update the cache if not (to handle reorgs). Since the expensive part is looking up the block from transaction hash, the cache will still help with that.
createDelayedMasternodePing is a very implementation-detaily function. This removes it from the header and from being a static method inside CMasternodePing and just turns it into a helper function inside masternode.cpp in an anonymous namespace.
GetTimeMicros and GetTimeMillis are used in some of the networking code (e.g. for triggering periodic pings), but didn't respect mocktime previously. This causes issues like spurious node disconnects in regtest tests using mocktime. This change adds support for mocktime to those methods, which seems to solve those issues.
domob1812
force-pushed
the
refactor-for-mnvault
branch
2 times, most recently
from
ef73ed0 to
ddc5c10
Compare
This simplifies and consolidates the logic used to verify confirmations and age of the collateral UTXO for masternodes. Instead of various functions we now have just one main function, which looks up the block index when the UTXO was confirmed. With this, we can check that it actually has the required confirmations, that the sigtime matches (is later than the block when it got the final confirmation necessary), and also cache the block hash for later input age computation.
When startmasternode is called with defer set to true, do not even attempt to register the masternode; just create the broadcast data and return it. This makes it possible to use the function even when the masternode is not yet valid, e.g. because the collateral does not have sufficient confirmations yet.
domob1812
force-pushed
the
refactor-for-mnvault
branch
from
ddc5c10 to
9732e80
Compare
| if (!collateralBlock.IsNull()) { | ||
| const auto mi = mapBlockIndex.find(collateralBlock); | ||
| if (mi != mapBlockIndex.end() && chainActive.Contains(mi->second)) | ||
| return mi->second; | ||
| } |
There was a problem hiding this comment.
This could potentially cause some issues if there was some race condition following a fork, as it is possible that it was set before the fork but then not found on the active chain, no?
There was a problem hiding this comment.
The cache takes effect only if a block is cached and part of the active chain at the moment the request happens (per the code quoted above).
If this is not the case, e.g. because the cached block was reorged and is no longer part of the active chain, then we simply invalidate and regenerate the cache. So I don't think anything can go wrong here.
2 participants
This contains some general refactoring / cleanup commits split out of #45, related to how deferred masternode broadcasts are created and handled.