feat!: 4k collateral high performance masternode implementation by ogabrielides · Pull Request #5039 · dashpay/dash

ogabrielides · 2022-10-10T18:12:41Z

Issue being fixed or feature implemented

What was done?

Implementation of 4k collateral HPMN.

How Has This Been Tested?

Breaking Changes

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

For repository code-owners and collaborators only

I have assigned this pull request to a milestone

github-actions · 2022-10-21T18:10:37Z

This pull request has conflicts, please rebase.

github-actions · 2022-12-27T01:01:54Z

This pull request has conflicts, please rebase.

github-actions · 2022-12-30T05:47:51Z

This pull request has conflicts, please rebase.

thephez · 2023-01-04T16:43:25Z

@PastaPastaPasta @UdjinM6 We need to make sure this doesn't create any issues with sentinel. Please consider that when reviewing. @nmarley You may know best about that.

github-actions · 2023-01-13T14:26:09Z

This pull request has conflicts, please rebase.

github-actions · 2023-01-20T05:36:47Z

This pull request has conflicts, please rebase.

it was picking the wrong DMN as a payee...

Co-Authored-By: PastaPastaPasta <6443210+pastapastapasta@users.noreply.github.com>

ogabrielides · 2023-02-13T13:16:29Z

@PastaPastaPasta @UdjinM6 Applied suggestions (+fixed tests).
In addition, as discussed with @HashEngineering, platformNodeID is included in SML entries.
Please review again

PastaPastaPasta · 2023-02-13T15:27:35Z

Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>

PastaPastaPasta

utACK for squash merge

knst

some nits only. Not a blocker to merge! Can be done never or as spare PR someday.

knst · 2023-02-13T20:03:55Z

    uint256 proTxHash;
    COutPoint collateralOutpoint;
    uint16_t nOperatorReward{0};
+    uint16_t nType{MnType::Regular.index};


why index is uint8_t but here is uint16_t? btw, better to use enum class here as more error prune (see other related comment)

knst · 2023-02-13T20:04:05Z

+class CDeterministicMNType
+{
+public:
+    uint8_t index;


may be change uint8_t index to enum class MNType : uint8_t ?

Pros:
Firstly, switch inside GetMnType() and any similar code will show a compiler warning if any type is missing.
Secondly, instead "MnType::HighPerformance.index" you can write MnType::HighPerformance
Thirdly, you can remove .index from MnType
Cons:
Instead (nType == MnType::HighPerformance.index) need to write nType == static_cast<int16_t>(MnType::HighPerformance)

knst · 2023-02-13T20:14:59Z

+    if (dmn->nType == MnType::HighPerformance.index) {
+        if (!UpdateUniqueProperty(*dmn, oldState->platformNodeID, dmn->pdmnState->platformNodeID)) {
+            mnUniquePropertyMap = mnUniquePropertyMapSaved;
+            throw(std::runtime_error(strprintf("%s: Can't update a masternode %s with a duplicate platformNodeID=%s", __func__,


I though that () is not needed after throw.

If remove them, the line will be more beautiful because no "))))" at the end of line but only ")))"

ogabrielides · 2023-02-13T20:31:37Z

some nits only. Not a blocker to merge! Can be done never or as spare PR someday.

Sure. Can be applied in a separate PR. Thanks @knst !

UdjinM6 · 2023-02-14T09:57:01Z

pls see https://github.com/UdjinM6/dash/commits/pr5039. also, I think expressions like nType == MnType::HighPerformance.index look pretty confusing. I like the suggestion made by @knst above, ~~I would suggest to implement it here.~~ ok, let's keep it as is for now and refactor it later.

Co-Authored-By: UdjinM6 <1935069+Udjinm6@users.noreply.github.com>

Co-Authored-By: Konstantin Akimov <545784+knst@users.noreply.github.com>

UdjinM6

utACK

PastaPastaPasta

re-utACK for squash merge

…b upgrade logic 39f76c8 chore: drop legacy `CDeterministicMNState` formats, evodb upgrade logic (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * `CDeterministicMNState_`{`Oldformat`, `mntype_format`} was introduced in [dash#5039](#5039) and [dash#5403](#5403), included in Dash Core v19 and v20 respectively. The above change has been in the codebase for more than two major releases and does not concern databases with a long shelf life (like wallets), this pull request removes them entirely. ## Breaking Changes Users upgrading from these versions are now expected to `-reindex`. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 39f76c8 knst: utACK 39f76c8 Tree-SHA512: 3fabe0aeb7f3d01a858b0b19d49a22523f462157597a1ade93181288e4ba7b133a5ee9272a8abe41e40cff177c0ddf94123274f0427693db6fdb90c4b8e613fa

ogabrielides marked this pull request as draft October 10, 2022 18:12

github-actions Bot added the needs rebase label Oct 21, 2022

github-actions Bot removed the needs rebase label Dec 14, 2022

ogabrielides changed the title ~~Introduction of HighPerformanceMasternode~~ feat: 4k collateral masternode implementation Dec 17, 2022

ogabrielides changed the title ~~feat: 4k collateral masternode implementation~~ feat: 4k collateral high performance masternode implementation Dec 17, 2022

thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label Dec 20, 2022

github-actions Bot added the needs rebase label Dec 27, 2022

github-actions Bot removed the needs rebase label Dec 27, 2022

ogabrielides changed the title ~~feat: 4k collateral high performance masternode implementation~~ feat!: 4k collateral high performance masternode implementation Dec 28, 2022

ogabrielides added this to the 20 milestone Dec 29, 2022

github-actions Bot added the needs rebase label Dec 30, 2022

github-actions Bot removed the needs rebase label Dec 30, 2022

ogabrielides modified the milestones: 20, 19 Jan 4, 2023

github-actions Bot added the needs rebase label Jan 13, 2023

github-actions Bot added needs rebase and removed needs rebase labels Jan 13, 2023

github-actions Bot removed the needs rebase label Jan 20, 2023

thephez added P2P Some notable changes on p2p level and removed P2P Some notable changes on p2p level labels Jan 26, 2023

ogabrielides marked this pull request as ready for review January 26, 2023 18:06

ogabrielides requested review from PastaPastaPasta, UdjinM6 and thephez January 26, 2023 18:06

UdjinM6 and others added 3 commits February 12, 2023 12:03

fix: tweak masternodelist output to match dmnstate

50816b8

fix/tests: fix masternode payments rpc, add tests

26d2d10

it was picking the wrong DMN as a payee...

rebase adjustement

a633718

github-actions Bot removed the needs rebase label Feb 12, 2023

ogabrielides and others added 5 commits February 12, 2023 15:27

Include PlatformNodeID in SML

0448ac4

refac: consolidate MasternodeType logic

86440d4

add dmn_types.h

9e51c24

adjustements and cleanups

2d51efb

Co-Authored-By: PastaPastaPasta <6443210+pastapastapasta@users.noreply.github.com>

clang formater

d1c1bce

PastaPastaPasta reviewed Feb 13, 2023

View reviewed changes

ogabrielides and others added 2 commits February 13, 2023 18:20

Apply suggestions from code review

7281e21

Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>

check dmn

f7ab291

PastaPastaPasta previously approved these changes Feb 13, 2023

View reviewed changes

knst reviewed Feb 13, 2023

View reviewed changes

ogabrielides and others added 2 commits February 14, 2023 12:38

various refactorin

45849e3

Co-Authored-By: UdjinM6 <1935069+Udjinm6@users.noreply.github.com>

various refactoring

82d9f60

Co-Authored-By: Konstantin Akimov <545784+knst@users.noreply.github.com>

ogabrielides dismissed PastaPastaPasta’s stale review via 82d9f60 February 14, 2023 11:54

UdjinM6 approved these changes Feb 14, 2023

View reviewed changes

ogabrielides requested a review from PastaPastaPasta February 14, 2023 12:34

PastaPastaPasta approved these changes Feb 14, 2023

View reviewed changes

PastaPastaPasta merged commit aa8462b into dashpay:develop Feb 14, 2023

ogabrielides deleted the 4k_hpmn branch February 15, 2023 08:26

ogabrielides mentioned this pull request Feb 21, 2023

HPMN Implementation #5134

Closed

thephez mentioned this pull request Mar 21, 2023

DIP27: high-performance masternodes dashpay/dips#125

Merged

This was referenced Feb 10, 2025

chore: drop deprecated RPCs masternode current, masternode winner and getpoolinfo #6567

Merged

chore: drop legacy CDeterministicMNState formats, evodb upgrade logic #6579

Merged

Conversation

ogabrielides commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actions Bot commented Oct 21, 2022

Uh oh!

github-actions Bot commented Dec 27, 2022

Uh oh!

github-actions Bot commented Dec 30, 2022

Uh oh!

thephez commented Jan 4, 2023

Uh oh!

github-actions Bot commented Jan 13, 2023

Uh oh!

github-actions Bot commented Jan 20, 2023

Uh oh!

ogabrielides commented Feb 13, 2023

Uh oh!

Uh oh!

Uh oh!

PastaPastaPasta Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

knst left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knst Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

knst Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

knst Feb 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

knst Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ogabrielides commented Feb 13, 2023

Uh oh!

UdjinM6 commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ogabrielides commented Oct 10, 2022 •

edited

Loading

knst left a comment •

edited

Loading

UdjinM6 commented Feb 14, 2023 •

edited

Loading