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.

Sign up for GitHub

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

Jump to bottom

[TierTwo] Deterministic masternode lists #2273

Merged
merged 25 commits into from
Apr 27, 2021
Merged

[TierTwo] Deterministic masternode lists #2273

merged 25 commits into from
Apr 27, 2021

Conversation

random-zebra
Copy link

@random-zebra random-zebra commented Mar 26, 2021 •
edited
Loading

Extracted from #2267.
This is the first fundamental PR.
Here we introduce the PROREG transaction type, the DMN manager (and active-masternode manager), and all the required classes (CDeterministicMN, CDeterministicMNState, CDeterministicMNStateDiff, CDeterministicMNList and CDeterministicMNListDiff).

This contains also the headers-only immer library for persistent immutable data structures. Reasons for the inclusion are explained in #2267 (under "code architecture").
The headers are copied over, at the same commit used by Dash (0a718d2d76bab6ebdcf43de943bd6c7d2dbfe2f9). Later on, we might want to properly include this as a git subtree.

Built on top of:

@random-zebra random-zebra added this to the 6.0.0 milestone Mar 26, 2021
@random-zebra random-zebra self-assigned this Mar 26, 2021
This was referenced Mar 26, 2021
Merged
Merged
@random-zebra random-zebra mentioned this pull request Apr 6, 2021
Merged
1 task
@random-zebra random-zebra force-pushed the 202103-dmn-lists branch 4 times, most recently from 295869b to a6ee11c Compare April 13, 2021 14:15
@random-zebra
Copy link
Author

Rebased on master, ready to go (this is where the fun starts).

Fuzzbawls
Fuzzbawls reviewed Apr 18, 2021
View reviewed changes
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.

Initial code review, some comments left

Also, our policy is to NOT log or return in the RPC interface MN IP addresses. upstream Dash doesn't follow this policy so some regression is introduced here.

Also also, seeing some inconsistent log line standards. In most places we use a format of __func__: message, but here i see things like -- message and __func__ -- message. we should stick to a single formatting standard for log lines.

src/init.cpp Outdated Show resolved Hide resolved
src/evo/deterministicmns.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
@random-zebra random-zebra mentioned this pull request Apr 18, 2021
Merged
@random-zebra
Copy link
Author

Styling/logging nits fixed.

our policy is to NOT log or return in the RPC interface MN IP addresses

If you mean CDeterministicMNState::ToJson, I think it makes very little sense to "hide" some information to the user, when the same information is freely available network-wide (just by reading the chain).

Also also, seeing some inconsistent log line standards.

There is some __func__: --ERROR. I've now removed the --, but our codebase is still a bit inconsistent in this (e.g. see CMasternodeSync::SyncWithNode logs). We could surely fix it in a proper PR, or, even better, we could write a good linter, and automate the process, so PR reviews can focus on the actual code.

furszy
furszy reviewed Apr 19, 2021
View reviewed changes
src/evo/providertx.cpp Outdated Show resolved Hide resolved
@random-zebra
Copy link
Author

Rebased on master after #2322 merge.

furszy
furszy reviewed Apr 22, 2021
View reviewed changes
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Initial code review completed, took some time but done, left two minor comments.
Overall looking great, cool work 🍺.

Final missing topic for me is get deeper over the MN unique properties.

src/evo/evonotificationinterface.cpp Outdated Show resolved Hide resolved
src/activemasternode.cpp Outdated Show resolved Hide resolved
random-zebra and others added 13 commits April 23, 2021 01:01
@random-zebra
[Refactoring] Add evo Notification Interface
c013bf1
@random-zebra
[Refactoring] Add IsDIP3Enforced/LegacyMNObsolete funcs to DMNManager
41869ec
@random-zebra
[Validation] Check duplicate unique-properties for masternodes
ddad010
@random-zebra
[Tests] Check deterministic masternodes unique properties
66302c3
@codablock @random-zebra
Implement CActiveDeterministicMasternodeManager
ca9d10c 
>>> adapted from dashpay/dash@d90b139
@random-zebra
[Refactoring] encapsulate activeMNINfo inside activeMNManager
6cd5b0c 
providing const pointer getter
@random-zebra
[Refactoring] CADMM: GetLocalAddress static
dcb5df9
@random-zebra
[Init] Add -mnoperatorprivatekey to flag the active mn as deterministic
0520ea9
@random-zebra
[BUG] Add proper virtual dtor for CActiveDeterministicMasternodeManager
a1cb227
@random-zebra
[Tests] Check for protx inputs hash in evo_specialtx_tests
170ab92
@random-zebra
[Trivial] Fix styling/logging nits
e659d7e
@random-zebra
[Consensus] Serialize the whole scriptPayout for ProReg sign string
e0ff60f
@random-zebra
[Consensus] Serialize also shield inputs for ProReg inputshash
6dbdf37
@random-zebra
Copy link
Author

Updated as per feedback.

furszy
furszy approved these changes Apr 23, 2021
View reviewed changes
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Code review ACK 6dbdf37.

Ok, this base work looks great.
Have written down some improvements that can do in a subsequent PR, let's move forward with it 👍 .

@furszy
Copy link

furszy commented Apr 26, 2021

Would be good to get this one merged before any other PR so we can continue moving over the v6.0 priorities list (#2267 (comment)).

Fuzzbawls
Fuzzbawls approved these changes Apr 27, 2021
View reviewed changes
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.

Code ACK 6dbdf37

@furszy furszy merged commit ac75379 into PIVX-Project:master Apr 27, 2021
@random-zebra random-zebra mentioned this pull request Apr 27, 2021
Merged
furszy added a commit that referenced this pull request May 5, 2021
@furszy
Merge #2274: [Consensus][Refactoring] Blocks v10: TierTwo payments in…
4e2e094 
… the coinbase transaction

c2060ef [Tests] Add IsCoinbaseValueValid_test unit test case in budget_tests (random-zebra)
5994ac9 [Refactoring] Spork: decouple AddSporkMessage from UpdateSpork (random-zebra)
c5ff712 [Refactoring] Update active state inside IsCoinbaseValueValid (random-zebra)
bfa628e [Refactoring] Cleanup in preparation for compatibility code (random-zebra)
82e186c [Refactoring] Refactor FillBlockPayee in preparation for DMN payments (random-zebra)
b9eacdc [Refactoring] No need to check mint-outputs to define coinbase txes (random-zebra)
c14870b [Consensus] Stricter checks for coinbase for v10+ (random-zebra)
e8c35ee [Consensus] Bump block version to 10: non empty coinbase transaction (random-zebra)
a891d5b [RPC] Don't skip coinbases in ListReceived (random-zebra)
8afc832 [Cleanup] Remove IsZerocoinMint check in FillBlockPayee (random-zebra)
e567f7f [Refactoring] Pass both coinbase and coinstake to FillBlockPayee (random-zebra)
944e793 [Cleanup] LoadStakeInput: no need for the previous block (random-zebra)
c6f71b2 [Refactoring] Decouple coinstake creation/signing (random-zebra)
5f3d3ba [Cleanup] Refactor CPivStake::CreateTxIn into GetTxIn (random-zebra)
1f71993 [Cleanup][Trivial] Remove unused param nMasternodeCountDrift (random-zebra)
58ee29f [Refactoring] Directly return empty DMN list before enforcement (random-zebra)

Pull request description:

  Extracted from #2267.
  This PR introduces an important consensus change. As per title, mn/budget payments are now outputs of the coinbase transaction, rather than the coinstake (which, now, contains only outputs belonging to the staker).
  The block version is bumped to 10.
  Also refactor the budget-payment functions, in preparation for the compatibility code and the new payment logic.

  Built on top of:
  - [x] #2273

ACKs for top commit:
  furszy:
    No code changes after rebase, ACK c2060ef
  Fuzzbawls:
    Code ACK c2060ef

Tree-SHA512: 29d6f729772f88ff908b9a83af8d378ddf6aa2d80e9f684df53d66b04ee44f9eac30d7cae9b911954f1a3bd321bfc0791d2944ebb87610a79f23d426c3702ca6
random-zebra added a commit that referenced this pull request May 6, 2021
@random-zebra
Merge #2296: [RPC] Provider-Register transactions
ddef43d 
e754732 [RPC][Tests] Include proTx data in json formatted transactions (random-zebra)
431693f Missing lock for Masternode::SetSpent() method. (furszy)
fb60da4 [Refactoring][RPC] protx send: avoid extra serializing/unserializing (random-zebra)
4c50f7a [Refactoring][RPC] Decouple atmp/relay functions in sendrawtransaction (random-zebra)
86dcf69 [Consensus] Make masternode collateral amount a consensus param (random-zebra)
503e7da [Consensus] Prevent usage of same collateral for MN and DMN instance (random-zebra)
f218357 [RPC] Add DMN support to listmasternodes (random-zebra)
716e7ed [RPC] Deprecate startmasternode when SPORK_21 is active (random-zebra)
70fe350 [MN] Stop processing legacy masternodes after SPORK_21 is active (random-zebra)
a718654 [RPC] Block EVO rpcs before V6 enforcement (random-zebra)
214dc7f [RPC] Implement protx_list (random-zebra)
cad0f0e [RPC] Implement protx_register RPC functions (random-zebra)

Pull request description:

  Extracted from #2267. Based on top of
  - [x] #2273

  Implement `protx_register` and `protx_list` RPC commands.
  Add DMN support to `listmasternodes`.
  Deprecate `startmasternode` after SPORK_21 activation.

ACKs for top commit:
  furszy:
    code ACK e754732 after cherry-pick.
  Fuzzbawls:
    Code ACK e754732

Tree-SHA512: 92fdb8e9056bcba0dad04db39048147e2650d306c85319a9825fb3d2b50a81bb58686412a3a406bf43809186eb97fa97b449e877757ba2ac5fec7926d198116e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@furszy furszy furszy approved these changes

@Fuzzbawls Fuzzbawls Fuzzbawls approved these changes

Assignees

@random-zebra random-zebra

Labels
Consensus Masternode Tests Upstream Dash Validation
Projects
None yet
Milestone
5.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@random-zebra @furszy @Fuzzbawls @codablock