-
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
[Consensus] New masternode payment logic #2309
[Consensus] New masternode payment logic #2309
Conversation
b9b6664
to
b2bef3f
Compare
1b99c7c
to
c3c3202
Compare
Rebased on master. Next in line for the DIP3 list. |
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.
Code ACK c3c3202
few non-blocking whitespace nits, but otherwise looks good
c3c3202
to
69e6ab5
Compare
Fixed whitespaces. |
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.
re-Code ACK 69e6ab5
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.
Code review completed
Spent some time here, have expanded the test coverage to verify that an invalid block paying to a DMN that shouldn't be paying is being rejected. Plus, found out that the new DMN payments validation flow is not being tested, it is just skipped due the not finished masternode sync process and the disabled MN payments spork in the test (the IsBlockPayeeValid
function is just returning true without performing any validation at line 245).
The work is here https://github.com/furszy/PIVX/commits/202103-dmn-payments, all yours ☕.
69e6ab5
to
b39a504
Compare
Thanks for the review @furszy . Added a commit at the end, as, rebasing on master, I noticed that I broke this test in #2360. |
b39a504
to
ed39000
Compare
Bug introduced in PIVX-Project@0b670c8 When the coinbase includes a masternode payment, if the entire block value is given to the miner (first coinbase output), the block over-mints.
ed39000
to
6b7b9df
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.
All good👌 , nice work 🍺.
ACK 6b7b9df
4fd564c [Tests] Add EvoNotificationInterface in the TestingSetup fixture (random-zebra) f6aa6ad [BUG] Check masternode/budget payments during block connection (random-zebra) 57fa99f [Tests] Introduce tiertwo_reorg_mempool test (random-zebra) 9a1bf27 [BUG] Add special tx processing to RollforwardBlock (random-zebra) 5d6bcef [RPC][Tests] Include proTx data in json formatted transactions (random-zebra) 93e8453 [Tests] Update deterministicmns test checking collateral locking (random-zebra) cc132a8 [Wallet] Implement auto-locking of masternode collaterals (random-zebra) c857636 [Tests] Introduce new functional test tiertwo_deterministicmns.py (random-zebra) Pull request description: Another one coming from #2267 This PR introduces a "auto-locking" feature for masternode collaterals: - at startup, the wallet is scanned, and any masternode collateral coin found gets locked. Can be disabled setting `-mnconflock=0` (although, there is no need for a `masternode.conf` anymore, so we might want to consider defining and using a new flag here, and removing `mnconflock` after 6.0). - during runtime, when a ProRegTx is processed by `CWallet::AddToWalletIfInvolvingMe`, the wallet checks for ownership of the collateral, and automatically locks it in case. Here, we also add two more functional tests (`tiertwo_deterministicmns.py`, `tiertwo_reorg_mempool.py`), update the other tests, checking the auto-locking feature, and fix a couple bugs: - Missing special tx processing in RollforwardBlock - Validity of tiertwo payments can (and must) be verified only during block connection, as we keep the deterministic state only for the active chain, not for all possible chaintips. Builds on top of: - [x] #2308 - [x] #2309 ACKs for top commit: furszy: re-ACK 4fd564c after the tiny fix and merging.. Tree-SHA512: b0f05d9265c02fe90baf48e5dcf66b34f078b90ffd0eef1488606013f84996b2ebaa7d70725c59db2f7456105a6eff2ba08da966858f81c4f19d014a72cd7692
Estracted from #2267
This implements the new payment logic for deterministic masternodes, to be used after the deactivation of the legacy system with SPORK_21.
Based on top of: