Conversation
knst
left a comment
There was a problem hiding this comment.
please check my comments for PR.
That's not a final version of my review, but that's what I have seen at the moment
|
please address these warnings from CI also: https://gitlab.com/dashpay/dash/-/jobs/5871076186 |
|
This pull request has conflicts, please rebase. |
|
I haven't reviewed this yet; but thank you for your contribution! |
Expand CProRegTx and CProUpRegTx according to DIP0026 standard, change serialization and deserialization methods in order to take in account the new field, Add consensus rules specified in the DIP0026 (max 32 payees, sum of payment shares cannot be greater than 10000,...) change dmnstate in order to take in account the new field change consensus rules in such a way that masternodes will pay all the payees add multiple payees in the qt masternode table
In this way any deployment that is activated with DIP0023 will be deployed on regtest after activating spork 24
And improve dynamically_prepare_masternode in such a way to support multiple payees
| } | ||
| // TODO: should do this for all not-yet-signied bits | ||
| trySignEHFSignal(Params().GetConsensus().vDeployments[Consensus::DEPLOYMENT_MN_RR].bit, pindexNew); | ||
|
|
There was a problem hiding this comment.
e24cb23 feat(consensus): Generalize regtest ehf should be in its own separate PR imo. Thoughts @knst @PastaPastaPasta ?
There was a problem hiding this comment.
Unlikely we are going to have any other EHF fork before dip0026 is merged.
But to speed up process review, merging new code and reduce conflicts (dip0026 goes for v21, not v20.x) it's reasonable to split
| if (payeeReward > 0) { | ||
| voutMasternodePaymentsRet.emplace_back(payeeReward, payoutShare.scriptPayout); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a set of rules which really makes you want to keep Operator related things separated I think:
- only Operator must be able to change his payout script
- Operator must not be able to change anyone else's payouts
|
Looks very solid. Needs some cleanups and small fixes but great job overall @panleone ! 👍 |
A helper class has been added which has the role to wrap and correctly serialize and deserialize a vector of payoutShare.
|
@panleone @PastaPastaPasta @UdjinM6 Therefore, once the mn_rr is activated (in conjunction with the platform release witch is expected before DIP0026 hard fork), we will require the platform to also support an array of payees. |
Good point, then we must also implement dip0026 on platform. At the moment due to exams I don't have enough free time to do it, but since v21 is still far away it should not be a problem |
|
This pull request has conflicts, please rebase. |
changes to isTriviallyValid and if formatting
1821d92 test: add activate_ehf_by_name (Alessandro Rezzi) de38dca feat(consensus): Generalize ehf activation (Alessandro Rezzi) Pull request description: ## Issue being fixed or feature implemented Try to sign/mine any ehf deployment and not only mn_rr. As asked in the review this decouples (and improves) commit [e24cb23](e24cb23) from PR #5799 ## What was done? See commit description ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone Top commit has no ACKs. Tree-SHA512: 7112a5214b473dea29a892563e6598eca089d2e17fdff8bda08c7777738f1054d2cb07e0a7dccf66214911fec64a6441e84100273ac2ed52abe1d33fb960e8cc
ℹ️ Backport Verification - Not ApplicableThis PR implements DIP0026 (masternode payout shares), which is a Dash-specific feature, not a Bitcoin backport. Original Bitcoin commit: N/A - This is a Dash-native implementation Why verification was skipped:
Recommendation: This PR should be reviewed through the standard Dash development process, not the backport verification system. Note: The backport verification system only applies to PRs that port changes from Bitcoin Core to Dash Core. |
Implement DIP0026 following the official documentation https://github.com/dashpay/dips/blob/master/dip-0026.md
With this PR it is possible to create a masternode that havs up to 32 payees and it is a first step toward trustless masternode shares
What was done?
In ProTxs the field
CScript scriptPayouthas been changed tostd::vector<PayoutShare> payoutShares;which is a vector of scripts/ rewards shares.Serialiation and Deserialization of ProTxs has been changed accordingly
Finally I added the regtest parameters to activate DIP0026 with a soft fork
How Has This Been Tested?
I have added unit tests that test creation, consensus and payment of masternodes with more than one payee
Checklist: