Skip to content

feat: implement DIP0026#5799

Open
panleone wants to merge 15 commits intodashpay:developfrom
panleoneDash:dip0026
Open

feat: implement DIP0026#5799
panleone wants to merge 15 commits intodashpay:developfrom
panleoneDash:dip0026

Conversation

@panleone
Copy link

@panleone panleone commented Jan 4, 2024

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 scriptPayout has been changed to std::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:

  • I have performed a self-review of my own code
  • 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
  • I have assigned this pull request to a milestone

@knst knst added this to the 21 milestone Jan 4, 2024
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@knst
Copy link
Collaborator

knst commented Jan 6, 2024

please address these warnings from CI also:

The subject line of commit hash 6dbc490ff109af21ac2611b3b1f4fe4a7e207271 is followed by a non-empty line. Subject lines should always be followed by a blank line.
The subject line of commit hash 9a6f9e9f44d347646a9e47219a532a936ac99e73 is followed by a non-empty line. Subject lines should always be followed by a blank line.
src/evo/deterministicmns.cpp:1711:72: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/evo/providertx.cpp:26:16: warning: Variable 'payoutShare' can be declared as reference to const [constVariable]
src/evo/providertx.cpp:68:16: warning: Variable 'payoutShare' can be declared as reference to const [constVariable]
src/rpc/evo.cpp:1319:71: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/rpc/evo.cpp:1395:84: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]

https://gitlab.com/dashpay/dash/-/jobs/5871076186
they are cause CI tail.

@github-actions
Copy link

github-actions bot commented Jan 7, 2024

This pull request has conflicts, please rebase.

@PastaPastaPasta
Copy link
Member

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e24cb23 feat(consensus): Generalize regtest ehf should be in its own separate PR imo. Thoughts @knst @PastaPastaPasta ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@UdjinM6
Copy link

UdjinM6 commented Jan 8, 2024

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.
@knst
Copy link
Collaborator

knst commented Jan 10, 2024

@panleone @PastaPastaPasta @UdjinM6
I've just observed another crucial aspect. With the "mn_rr" hard fork, a portion of the masternodes' rewards will be reallocated between the masternodes directly and the platform reward (approximately 37%). The platform is responsible for managing its share of the reward and distributing it among evo nodes.

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.

@panleone
Copy link
Author

@panleone @PastaPastaPasta @UdjinM6 I've just observed another crucial aspect. With the "mn_rr" hard fork, a portion of the masternodes' rewards will be reallocated between the masternodes directly and the platform reward (approximately 37%). The platform is responsible for managing its share of the reward and distributing it among evo nodes.

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

@github-actions
Copy link

This pull request has conflicts, please rebase.

@panleone panleone mentioned this pull request Jan 14, 2024
5 tasks
PastaPastaPasta added a commit that referenced this pull request Feb 29, 2024
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
@UdjinM6 UdjinM6 removed this from the 21 milestone Jul 26, 2024
@coderabbitai coderabbitai bot mentioned this pull request Jan 10, 2025
5 tasks
@DashCoreAutoGuix
Copy link

ℹ️ Backport Verification - Not Applicable

This 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
Reviewed commit hash: 4b5c4aaac1-verify-1753723627

Why verification was skipped:

  • This PR introduces new Dash functionality (DIP0026)
  • No corresponding Bitcoin changes to verify against
  • This is legitimate Dash development, not a backport

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants