Skip to content

Conversation

@random-zebra
Copy link

@random-zebra random-zebra commented May 5, 2021

Built on top of:

This concludes the first DMN-milestone list #2267 (comment).

Here we introduce the last two payloads and transaction types, adding relative RPC commands, and updating the tests:

  • PROUPREG (provider-update-registrar) submitted by the owner (the payload must be signed with the owner key) to update the operator key and/or the voting key and/or the payout address.

  • PROUPREV (provider-update-revoke) submitted by the 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 operator key), which sets the new IP address for the masternode.

@random-zebra
Copy link
Author

Rebased on master. Ready to go.

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.

Reviewed till e5292cf08a86a295983adc33ac08781225a82913. Still moving but looking great

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 ACK fb3f567a2e563caa7d3f0c5f45dfa4df9af39195, only found nits. Really nice work, awesome test coverage👌👌.

@random-zebra random-zebra force-pushed the 202103-dmn-proupreg-rev branch from fb3f567 to 853d8e1 Compare June 26, 2021 13:54
@random-zebra
Copy link
Author

Thanks for the review @furszy. Nits addressed.

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 ACK 853d8e1 after nits.
will start running it and test it live now.

@random-zebra random-zebra force-pushed the 202103-dmn-proupreg-rev branch from 853d8e1 to 663517e Compare June 26, 2021 15:00
@random-zebra random-zebra requested a review from Fuzzbawls June 29, 2021 09:03
furszy
furszy previously approved these changes Jun 30, 2021
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.

re-utACK after rebase 663517e. Last one in the 2267 work path 🍻 .

@furszy
Copy link

furszy commented Jul 1, 2021

Small conflict, needs rebase :\

@random-zebra
Copy link
Author

Rebased...

@random-zebra random-zebra force-pushed the 202103-dmn-proupreg-rev branch 2 times, most recently from 018a474 to 959f595 Compare July 6, 2021 13:55
check:
- valid payload
- payload malleability
- owner script payout change
- duplicate operator key
- duplicate owner key
Otherwise a block with two protx using the same key could be accepted
and then cause an assertion failure in AddUniqueProperty.
check:
- valid payload
- payload malleability
As changing the payout address, could be used maliciously to game the
old payment system.
MalleateProUpServTx shouldn't re-use the ip (port) of an active
masternode. Select the port with a number > 1000
@random-zebra random-zebra force-pushed the 202103-dmn-proupreg-rev branch from 959f595 to 27d9f4e Compare July 6, 2021 14:01
@random-zebra
Copy link
Author

Rebased on master (once again), and serialization updated.

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.

No changes after rebase, only serialization code update. ACK 27d9f4e.

Will need to be rebased on master once #2471 gets merged so GA can run over it again.

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 3c76eaf1048b595f72f812d377ca3be00dc3edf0

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.

have run all the tests locally one more time, ACK 27d9f4e. Merging..

@furszy furszy merged commit 8a683aa into PIVX-Project:master Jul 7, 2021
furszy added a commit that referenced this pull request Oct 8, 2021
d3843cd [Refactor] No need to get the publickey of the active masternode (random-zebra)
483f509 [Refactor] De-duplicate operator BLS key parsing in rpc (random-zebra)
855c094 [Build] Fix CMake builds with chiabls/relic includes (random-zebra)
77ec208 [Tests] Fix TierTwo functional tests with BLS operator key (random-zebra)
dd46f72 [TierTwo] Migration of DMN operator key to BLS (random-zebra)
1dfdcc1 [RPC] Implement generateblskeypair() RPC command (random-zebra)
619c0f8 scripted-diff: Rename keyIDOperator to pubKeyOperator (random-zebra)
b0a7fa5 [QA] Add tests for fbv messages signed with BLS keys (random-zebra)
dd0fb01 [TierTwo] Add functions to sign/verify messages with BLS (random-zebra)

Pull request description:

  This builds on top of:

  - [x] #2363
  - [x] #2419
  - [x] #2420

  As per title, introduce BLS keys/signatures for messages signed by the masternode operator (which is a requirement for [DIP 6](https://github.com/dashpay/dips/blob/master/dip-0006.md), and subsequent upgrades).

  - a new RPC command `generateblskeypair` returns a new publickey/secretkey BLS keypair.
  - `CDeterministicMNState::keyIDOperator` is now a `CBLSLazyPublicKey` (instead of a `CKeyID`), and it's renamed `pubKeyOperator`.
  - `-mnoperatorprivatekey` flag takes a BLS secret key in hex format.
  - `CActiveMasternodeInfo` stores a `CBLSPublicKey`/`CBLSSecretKey`
  - the BLS signature byte vector is serialized for legacy messages (mnw, fbv) in the compatibility code.

ACKs for top commit:
  furszy:
    great, ACK d3843cd
  Fuzzbawls:
    ACK d3843cd

Tree-SHA512: 1caf49615283d10b356caa049b57091dc70805c0bf2c11645d560c81223ce0d9936a46096cc4871c5ec25164518ebdf259d0f23a8c5bca39f2e436cc9e954519
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