Skip to content
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

[Consensus] New block rewards #2763

Merged
merged 11 commits into from Sep 23, 2022
Merged

Conversation

tohsnoom
Copy link

@tohsnoom tohsnoom commented Jul 6, 2022

Adjust block rewards for staking, masternode and budget to change to new values when V5.5 upgrade block is reached.

Adds UPGRADE_V5.5 block height value to chainparams, currently set to placeholder values for mainnet and testnet.

Adds nNewMNBlockReward value to chainparams--note the planned refactoring to call Params().GetConsensus().nMNBlockReward directly would need to be rethought or wait until after change has taken effect since there are now two parameters which is selected in GetMasternodePayment.

Unit tests for these changes are still WIP.

@Fuzzbawls
Copy link
Collaborator

some additional changes to address the issues with the regression test suite:

Index: test/functional/test_framework/test_framework.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
--- a/test/functional/test_framework/test_framework.py	(revision fe1c0adfaff413b7ce03793e8d012e0334dab0d7)
+++ b/test/functional/test_framework/test_framework.py	(date 1657598250506)
@@ -1630,7 +1630,7 @@
         self.minerPos = 4
         self.remoteDMN1Pos = 5
 
-        self.extra_args = [["-nuparams=v5_shield:249", "-nuparams=v6_evo:250", "-whitelist=127.0.0.1"]] * self.num_nodes
+        self.extra_args = [["-nuparams=v5_shield:249", "-nuparams=PIVX_v5.5:250", "-nuparams=v6_evo:250", "-whitelist=127.0.0.1"]] * self.num_nodes
         for i in [self.remoteOnePos, self.remoteTwoPos, self.remoteDMN1Pos]:
             self.extra_args[i] += ["-listen", "-externalip=127.0.0.1"]
         self.extra_args[self.minerPos].append("-sporkkey=932HEevBSujW2ud7RfB1YF91AFygbBRQj3de3LyaCRqNzKKgWXi")
Index: src/consensus/upgrades.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/consensus/upgrades.cpp b/src/consensus/upgrades.cpp
--- a/src/consensus/upgrades.cpp	(revision fe1c0adfaff413b7ce03793e8d012e0334dab0d7)
+++ b/src/consensus/upgrades.cpp	(date 1657591085736)
@@ -62,6 +62,10 @@
                 /*.strInfo =*/ "New staking rules",
         },
         {
+                /*.strName =*/ "PIVX_v5.5",
+                /*.strInfo =*/ "New rewards structure",
+        },
+        {
                 /*.strName =*/ "v6_evo",
                 /*.strInfo =*/ "Deterministic Masternodes",
         },
Index: test/functional/tiertwo_dkg_errors.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/functional/tiertwo_dkg_errors.py b/test/functional/tiertwo_dkg_errors.py
--- a/test/functional/tiertwo_dkg_errors.py	(revision fe1c0adfaff413b7ce03793e8d012e0334dab0d7)
+++ b/test/functional/tiertwo_dkg_errors.py	(date 1657597464929)
@@ -15,7 +15,7 @@
 
     def set_test_params(self):
         self.set_base_test_params()
-        self.extra_args = [["-nuparams=v5_shield:1", "-nuparams=v6_evo:130", "-debug=llmq", "-debug=dkg", "-debug=net"]] * self.num_nodes
+        self.extra_args = [["-nuparams=v5_shield:1", "-nuparams=PIVX_v5.5:130", "-nuparams=v6_evo:130", "-debug=llmq", "-debug=dkg", "-debug=net"]] * self.num_nodes
         self.extra_args[0].append("-sporkkey=932HEevBSujW2ud7RfB1YF91AFygbBRQj3de3LyaCRqNzKKgWXi")
 
     def reset_simerror(self, node):

@Fuzzbawls Fuzzbawls added Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes Protocol-Update Consensus labels Jul 15, 2022
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

After some more internal testing, here is some additional feedback:

  • A couple minor logic issues lead to inconsistency between block reward, budget value, and MN payment amount in terms of at which block they change.
  • Suggest to include regtest in the rewards change process by transitioning away from the static 250 PIV block reward to the new block reward at the suggested changeover height (+1).
  • During testing, after the new reward structure was activated, the GUI started labeling MN rewards as Budget payments, as the logic for that assumes any MN payment above consensus.nMNBlockReward must be a budget payment.

src/budget/budgetmanager.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/chainparams.cpp Outdated Show resolved Hide resolved
@tohsnoom
Copy link
Author

tohsnoom commented Aug 4, 2022

I've added budget unit test cases for v5.5 activation.

@tohsnoom tohsnoom requested a review from Fuzzbawls August 9, 2022 20:43
@tohsnoom
Copy link
Author

Commits have been added for switching the protocol enforcement spork and adding a masternode payment amount unit test.

@Fuzzbawls Fuzzbawls added this to the 5.5.0 milestone Sep 11, 2022
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK 7887920

Copy link

@DeanSparrow DeanSparrow left a comment

Choose a reason for hiding this comment

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

ACK 7887920

I have reviewed these changes.

@yenachar
Copy link

Downloaded, built (in release and debug), and reviewed the code without material issues.

@Fuzzbawls Fuzzbawls changed the title [Consensus] New block rewards - preliminary PR for review [Consensus] New block rewards Sep 23, 2022
@Fuzzbawls Fuzzbawls merged commit f849bea into PIVX-Project:master Sep 23, 2022
@Fuzzbawls Fuzzbawls removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Dec 11, 2022
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.

None yet

4 participants