-
Notifications
You must be signed in to change notification settings - Fork 716
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Refactor] Masternode Budget first refactoring and cleanup #1826
[Refactor] Masternode Budget first refactoring and cleanup #1826
Conversation
d6489c3
to
be97b28
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, great work!, really needed ☕ .
Code review ACK with a concern and an improvement topic:
-
Concern:
cs_main
lock change from TRY_LOCK to LOCK inCheckAndRemove
(it's written in the comments). -
Improvement topic:
We can get rid of thefSynced
flag member in CBudgetVote and clean up the code a lot there.
It seems to only be used to determine whether broadcast the vote or not (set when the node is sync) when the same functionality can be achieved with a single flag in the manager and not inside every stored vote.
src/masternode-budget.h
Outdated
@@ -85,6 +88,18 @@ class CBudgetVote : public CSignedMessage | |||
std::string GetStrMessage() const override; | |||
const CTxIn GetVin() const override { return vin; }; | |||
|
|||
UniValue ToJSON() const; | |||
|
|||
int GetDirection() const { return nVote; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't be more understandable to name it GetVote
or GetVoteType
? .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence. Didn't like this "direction" name either... but I think that neither GetVote
nor GetVoteType
truly catch the meaning. Open to hear more opinions.
5644aaf
to
69c4ee9
Compare
d80fa6a
to
68829f4
Compare
68829f4
to
08d65f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- make critical variables private, providing interfaces - make functions const where appropriate
Add interfaces: - SyncVotes: to send votes to a specific CNode - SetSynced: to mark the fSynced flag of votes - GetVotesArray(Object): to return univalue objects for budget rpc
encapsulate those variables for which we already have Getters/Setters. Mark getters as const.
And properly rename the one looking for the highest yes count
- rename UpdateValid - make fValid private - add StrInvalid and its getter - make IsValid return fValid.
- make nFeeTXHash private, add GetFeeTXHash() - mark some function as 'const'
The chain height is not needed inside the function. Only check the height in init, before reading the budget database, and modify the fDryRun flag if needed.
Save a cached block height in the budget manager (update with ABC) and use it instead of calling chainActive everywhere.
08d65f0
to
5564002
Compare
|
||
if (masternodeSync.RequestedMasternodeAssets <= MASTERNODE_SYNC_BUDGET) return; | ||
|
||
if (strBudgetMode == "suggest") { //suggest the budget we see | ||
SubmitFinalBudget(); | ||
} | ||
|
||
int nCurrentHeight = GetBestHeight(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: int nCurrentHeight = height;
or just rename height
to nCurrentHeight
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 5564002 and merging..
Otherwise all proposals/budgets are removed by `CheckAndRemove` (negative confirmations for collateral txes). Introduced in PIVX-Project#1826
…ring init 683fd6b [BUG] Set budget best height before reading budget DB during init (random-zebra) Pull request description: Fix a bug Introduced in #1826, which makes the node erase all proposals and finalized budgets, after reading them from the DB during startup (and thus having to ask everything again from the peers). **Cause:** The budget manager best height is set after reading the budgetdb. As non-dry runs of `CBudgetDB::Read` include a call to `CheckAndRemove()` at the end (to delete expired objects), the manager's best-height needs to be already set. ACKs for top commit: furszy: Good catch, ACK 683fd6b. Fuzzbawls: utACK 683fd6b Tree-SHA512: e66dd4e8a33eac7a92559fea545a9d8df5aeda27a8d5e490ab7948733320e2d17e105b66dabcd9603c17503838fe505310b844f52b056cecd58f2038e2901651
…+ MN activation functional test. 9a1e5e2 Including functional test coverage for budget finalization sync process. (furszy) 7222705 Simple masternodes final budget suggest RPC command implementation for regtest-only. (furszy) f485be4 rename masternodes/budget functional test name to use the tiertwo_xxx prefix. (furszy) 3bffe86 masternodes_governance_basic.py test migrated to use mock time. (furszy) 9fab353 test_framework: implement sync_tiertwo util function (furszy) 58eefbd New functional test masternodes_governance_basic.py, added coverage for: (furszy) 2038f2f added test coverage for the following masternode status: expiration, re initialization after expiration, removal, re initialization after removal and collateral spent. (furszy) 4e3d7a3 Move masternodes consensus time definitions to .cpp . (furszy) b598734 new masternode activation functional test :) . (furszy) ee44f0c CMasternode added missing internal cs locks. (furszy) 8414747 regtest-only: enable different ports usage for MNs. (furszy) 84dca1a end spork message to let the remote peer know when the sporks broadcast ends. (furszy) 6ed644e Added regtest support for masternode validation values: min confirmations, min ping time, min mnb time, expiration time, removal time. (furszy) cd7bb34 Introducing new tier two network initial sync architecture. (furszy) c7b03c0 refactor: abstracted out ProcessGetMNList from CMasternodeMan::ProcessMesssage. (furszy) 4a67a4b refactored ProcessSpork code into ProcessSporkMsg and ProcessGetSporks. (furszy) Pull request description: -- Only running on regtest -- Covering the following points: #### 1) A new synchronization structure for the tier two network. It has no redundant checks nor redundant requests (there by less network bloat, faster synchronization) and the code is by far more readable and maintainable than the previous version. #### 2) Tier two network regtest support. Adding Masternodes and Governance capabilities to the regtest network. #### 3) Sync and masternode activation test. Functional test validating the correct operation of the tier two network sync and the masternode broadcasting & activation process. #### TODO: * Clean peer state on peer disconnection (connect signal) * Spam filter + spam functional test. * Not responding peers timeout test + spam functional test. * Sporks broadcasting end message (currently it's using the SPORK_INVALID as end message). * Data verification from several peers (currently running, only need to implement a sync threshold to move to the next level) + spam functional test. * Further decoupling from the old structure. * Previous version cleanup. ------------ Have pushed it under WIP to be able to test other great PRs like #1826. ACKs for top commit: random-zebra: Left few more comments (mostly around the use of mocktime which seems a bit funky), but they can be addressed in a later PR, so ACK 9a1e5e2 Fuzzbawls: ACK 9a1e5e2 Tree-SHA512: 0808b26d997d6ee76a6c0217e0321a7dbcf3a4a42d25dd142bd84058f486a9ba19868269f755eb10e05cfea41a8532e0d88593bfc4dbc957fd480e8ea7e6d7eb
Refactoring of the masternode budget classes (
CBudgetManager
,CBudgetProposal
,CBudgetVote
,CFinalizedBudget
,CFinalizedBudgetVote
), providing better encapsulation of the data, removing unused functions, and giving a first round of styling cleanup.