Skip to content

Conversation

@random-zebra
Copy link

Introduce the concepts of "transaction type" and "extra payload" inspired by Dash's DIP2 and integrate them in our customized serialization for SAPLING_VERSION transactions.

Even though we won't probably activate any special type with v5.0, having the new serialization already in place allows us to enable them in a future upgrade, without needing to increment the transaction version again.

This PR includes basic non-contextual validation rules (that can only be safely enforced after Sapling NU):

  • nVersion=1 supports only nType=0
  • A tx can have extra payload only if nType!=0 (thus, due to the previous rule, only if nVersion>=2)
  • A tx with nType!=0 cannot have empty payload.
  • The serialized payload cannot be bigger than a fixed max size (valid for all payload types, currently set at 10 kB).

This also prepares the method stubs for the specific per-payload-type validation rules, and for the tiertwo data processing, that will be added in the future.

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 c1061e66 👍 .

Initial comment, need to add the extraPayload member to the CMutableTransaction class, the CMutableTransaction(const CTransaction& tx) constructor and serialization. Otherwise special txs cannot be created for testing purposes (at the end, CTransaction is an immutable class).

@random-zebra
Copy link
Author

Rebased on master, and cherry-picked a couple commits from @furszy, adding a unit-test for the basic validation rules of special transactions.

furszy
furszy previously approved these changes Nov 21, 2020
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.

utACK b6d7bc3 after rebase, nice work.

There is a commit with an [SQUASH] label that could be squashed.

@random-zebra
Copy link
Author

Rebased on master and commit squashed.

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.

utACK 2fbb932 after rebase.

@furszy
Copy link

furszy commented Nov 24, 2020

testnet is getting closer, merging..

@furszy furszy merged commit 0ad68a3 into PIVX-Project:master Nov 24, 2020
@Fuzzbawls Fuzzbawls added the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Dec 14, 2020
@random-zebra random-zebra removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Jan 3, 2021
random-zebra added a commit that referenced this pull request Jul 18, 2021
9f43405 [Doc] Add DIP3 rpc commands to the release notes (random-zebra)

Pull request description:

  This is the current state of my DMN branch.
  Deterministic masternodes fully working on regtest (except for proof of service, which will come later), with a good number of tests covering registration, update, revocation, revival, collaterals auto-locking, consistency after reorgs, compatibility with the old system, payments, and governance voting. ☕

  There are several differences with Dash code, mostly in the RPC, mempool, block assembler, compatibility code, wallet, deployment logic, and unit/functional testing (which led to the discovery of a few bugs, including a non-trivial one, reported upstream).
  But, overall, this remains respectful of the specification given here (at least for now... we'll have to modify the spec a bit when introducing shield rewards later):
  - [DIP3 — Deterministic Masternode Lists](https://github.com/dashpay/dips/blob/master/dip-0003.md)

  Huge props and thanks to Codablock and the Dash Core developers for the design of this awesome protocol.

  In order to follow the specification, implementing it in our PoS system, while still allowing the cold-stake script, a critical consensus change was needed: the masternode/budget payment is no longer included in the *coinstake* transaction. It is, instead, an output of the *coinbase* transaction, which, in turn, is no longer required to be empty.
  As a side effect, this enables the definition and use of a new P2CS script, with no "free" outputs.
  The block version is bumped to `10`, with the new rules.

  This PR serves as general container for tracking, and will now be divided in more manageable sub-PRs, for better review.

  ### Introduction

  Deterministic Masternode lists are lists of masternodes, built at every block, relying only on on-chain data (previous list, and transactions included in the current block).
  All nodes derive (and verify) their masternode lists independently, from the same on-chain transactions, thus they immediately reach consensus on the tier-two state (number of masternodes, properties and status of each one).
  As clearly explained in the "motivation" part of the DIP document, this is crucially different from the previous system:

  > The previous system was maintained with consensus mechanisms that predated Satoshi Nakamoto’s solution to the Byzantine Generals Problem. This meant that each node needed to maintain their own individual masternode list with P2P messages and not a blockchain based solution. Due to the nature of the P2P system, there was no guarantee that nodes would come to the same conclusion on what the masternode list ought to look like. Discrepancies might, for example, occur due to a different order of message reception or if messages had not been received at all. This posed some risks in regard to consensus and limited the possible uses of quorums by the system.
  >
  > As a concrete example, the previous system required implementing workarounds such as "masternode reward voting" which was performed multiple blocks in advance for each block to make sure that consensus would be found and agreed on. Enforcing this consensus however still posed a risk which could have resulted in network wide forking, so a spork to turn off masternode payment enforcement was added to prevent this issue from occurring. The spork was used sporadically after major network updates.

  This is a major overhaul, which brings also a good number of improvements in the user experience, while removing the shortcomings of the previous system.
  All reviewers are encouraged to take a deep dive in the DIP3 document, which describes perfectly the advantages of the new system.

  ### New Roles

  For each masternode, three different "roles" are defined. Each role is represented by a private/public keypair.

  1. **Owner**: Must be unique on the network. Can update the other two roles, and the masternode payout address.
  2. **Operator**: Must be unique on the network. The operator key is saved in the `pivx.conf` of the remote node, and it is used to sign masternode-related P2P messages (e.g. budget finalisations, or masternode winners in the compatibility code). It can also be used to update the masternode IP-address, or the operator payout address (if the masternode is configured to allow a percentage of the reward to be paid to operator).
  3. **Voting**: Doesn't have to be unique (multiple masternodes can share the same voting key). It is used to cast budget votes.

  The same keypair can be used for all three roles (at least for now, the operator key will be changed to a BLS key soon), but they must be different from the key of the collateral address.

  ### New Transaction Types

  Special transactions ([DIP2](https://github.com/dashpay/dips/blob/master/dip-0002.md)) were introduced in #1966, but no new transaction type has been defined in PIVX yet.
  Here we introduce four new types, each identifying a particular transaction payload, with its own validation rules:

  - `PROREG` (*provider-register*): this is the main special transaction. Used for the registration of a new masternode, setting all of its properties (such as the keys for each role). It creates the masternode collateral, as one of its outputs, or it references a 10000 PIV unspent output on chain (in which case, it must include a signature with its keys, as proof of ownership).
  - `PROUPSERV` (*provider-update-service*): sent by the mn **operator** to update the properties related to the service (IP address, operator payout address)
  - `PROUPREG` (*provider-update-registrar*): sent by the mn **owner** to update the operator key, the voting key, or the payout address.
  - `PROUPREV` (*provider-update-revoke*): sent by the mn **operator** to revoke the service, and put the mn in PoSe-banned state (e.g. in case of compromised keys). The masternode can be "revived" later, by sending a ProUpReg tx, which sets a new operator key, and then a ProUpServ tx (signed with the new key), which sets the new IP address for the masternode.

  ### Code Architecture

  Deterministic masternodes are represented as objects of the class `CDeterministicMN`.
  This class includes a member variable that stores a shared pointer to a constant `CDeterministicMNState` object, which encapsulates the dmn state (updated properties and status).

  A list of masternodes is represented by the class `CDeterministicMNList`, which uses immutable functional maps (https://github.com/arximboldi/immer) to hold the actual information about each entry.
  A new list is built at every block and mantained by `CDeterministicMNManager`.

  The use of immutable functional maps is, in my opinion, an elegant solution, devised by Codablock, to reduce the memory overhead required for the masternode list update at every block, by adopting a [copy-on-write](https://en.wikipedia.org/wiki/Copy-on-write) approach.
  Immutable data structures are provided by default on functional-programming oriented languages, such as Clojure or Scala, but for C++ we unfortunately need to rely on third party libraries.
  We could switch to an implementation based on `std::map`s, but that would severly impact the performance and require hundreds of MB in the ram, just for MN list housekeeping.

  The code still contains a lot of ugly parts, which we need to keep for compatibility with the old system. Most of them are properly commented. My first task, after the full deployment on mainnet, will be to do a general cleanup and refactoring, removing all the legacy system code.

  ### Deployment

  This an hard-forking update, slated for **PIVX v6.0**.
  It will be deployed with a combination of `nuparams` activation height, and a spork message.
  For this purpose, a new height-based (rather than time-based) spork, `SPORK_21_LEGACY_MNS_MAX_HEIGHT`, is introduced.
  It will be used only during the transition to the new system, and can be removed afterwards.

  As in the previous mandatory upgrades, there will be a protocol bump, shortly before the enforcement block.
  All nodes must be updated before the protobump.
  Masternodes, as usual, will need to send a new MNB start message, after upgrading, with the new protocol number.

  After the enforcement, new consensus rules apply (such as block v10), and new special transactions are accepted on the network (with the exception of `PROUPREG`).
  Deterministic masternodes can be registered, but have a few limitations, in order to be compatible with the legacy system (which is still active): for example, they cannot pay a portion of the reward to the operator, yet.
  The system still uses the old logic for masternode payments (mnw signed messages), with new compatibility code, in order to take into account also nodes from the deterministic list.

  After the activation of `SPORK_21_LEGACY_MNS_MAX_HEIGHT`, the legacy system is disabled, and the transition to deterministic masternode lists is complete.
  At this point, mno can also send `PROUPREG` transactions, and use all the features of DMNs. The new payment logic applies.

  The reasons for using a spork, instead of pre-set number of blocks after enforcement, are multiple:

  - It allows for easier testing on testnet, with the possibility of disabling and re-enabling legacy masternodes multiple times, in order to test different scenarios
  - It allows for safer deployment on mainnet: we might want to wait until the network has a good percentage of dmn, before disabling the legacy system. Or, conversely, there could be an unforseen bug in the compatibility code, which would require a transition faster than expected.

  ### TODO

  These are the next milestones:

  - Proof of service for masternodes (with [DIP6](https://github.com/dashpay/dips/blob/master/dip-0006.md) and [DIP7](https://github.com/dashpay/dips/blob/master/dip-0007.md))
  - kickass GUI
  - Budget system overhaul
  - Chain-locks ([DIP8](https://github.com/dashpay/dips/blob/master/dip-0008.md))
  - SHIELD masternode rewards

ACKs for top commit:
  furszy:
    utACK 9f43405
  Fuzzbawls:
    utACK 9f43405

Tree-SHA512: 0c23228522cf021147cb66affcceb8b453c952e9db433c4513ebfcdc3328243383742e8a18eea985036a107506ceb10b39e96d4e0fde87ea16686d2f7039e0bc
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.

3 participants