Implement proof-of-stake changes #1177

asaj · 2019-10-02T00:50:33Z

Description

This PR implements changes to the proof of stake design.

Most notably, it:

Removes notice periods, and their effect on voting weight
Introduces the concept of voting and non-voting locked gold
Allows users to vote for multiple validator groups
Introduces the concept of pending and active votes in validator elections, so that epoch rewards can be paid out to voters more efficiently
Introduces a cap on the number of votes a validator group can receive
Factors out validator elections into a separate contract

Tested

Unit tests, manual tests of the CLI with ganache (with proof-of-possession disabled):

celocli lockedgold:register --from 0x5409ED021D9299bf6814279A6A1411A7e866A631
celocli lockedgold:register --from 0x6Ecbe1DB9EF729CBe972C83Fb886247691Fb6beb
celocli lockedgold:lock --from 0x5409ED021D9299bf6814279A6A1411A7e866A631 --goldAmount 1000000000000000000
celocli lockedgold:lock --from 0x6Ecbe1DB9EF729CBe972C83Fb886247691Fb6beb --goldAmount 1000000000000000000
celocli validatorgroup:register --from 0x5409ED021D9299bf6814279A6A1411A7e866A631 --name testGroup --url celo.org --commission 0.1
celocli validator:register --from 0x6Ecbe1DB9EF729CBe972C83Fb886247691Fb6beb --name testValidator --url celo.org --publicKey 0xc52f3fab06e22a54915a8765c4f6826090cfac5e40282b43844bf1c0df83aaa632e55b67869758f2291d1aabe0ebecc7cbf4236aaa45e3e0cfbf997eda082ae19d3e1d8f49f6b0d8e9a03d80ca07b1d24cf1cc0557bdcc04f5e17a46e35d02d0d411d956dbd5d2d2464eebd7b74ae30005d223780d785d2abc5644fac7ac29fb0e302bdc80c81a5d45018b68b1045068a4b3a4861c93037685fd0d252d7405011220a66a6257562d0c26dabf64485a1d96bad27bb1c0fd6080a75b0ec9f75b50298a2a8e04b02b2688c8104fca61fb00
celocli validator:affiliation --from 0x6Ecbe1DB9EF729CBe972C83Fb886247691Fb6beb --set 0x5409ED021D9299bf6814279A6A1411A7e866A631
celocli validatorgroup:member --accept 0x6Ecbe1DB9EF729CBe972C83Fb886247691Fb6beb --from 0x5409ED021D9299bf6814279A6A1411A7e866A631
celocli election:vote --from 0x5409ED021D9299bf6814279A6A1411A7e866A631 --for 0x5409ED021D9299bf6814279A6A1411A7e866A631 --value 9

Other changes

CLI
ContractKit
Typos
UsingRegistry

Related issues

Fixes Users SBAT withdraw locked gold in latest proof-of-stake design #969, Validator Groups SBAT specify a commission #975, Users SNBAT withdraw locked gold if they have been a validator group in the past 60 days #977, Users SBAT lock gold in new Locked Gold contract #967, Users SNBAT withdraw locked gold if they have been a validator in the past 60 days #972, Developers SBAT set different requirements for Validators vs Validator Groups #301, Users SBAT vote on governance proposals in new proof-of-stake design #970

Backwards compatibility

Not backwards compatible

m-chrzan

Got through Governance.sol and LockedGold.sol.

packages/protocol/contracts/governance/Governance.sol

m-chrzan · 2019-10-11T19:33:20Z

packages/protocol/contracts/governance/LockedGold.sol

-    uint128 value;
-    uint128 index;
+  struct Authorizations {
+    // The address that is authorized to vote on behalf of the account.


Is this always set to something? I imagine if it's 0, the account itself is authorized to vote. If so, might be worth adding info on this edge case to the comment. Ditto below.

Good call, this behavior is unclear, made it more detailed in the comments.

m-chrzan · 2019-10-11T19:36:10Z

packages/protocol/contracts/governance/LockedGold.sol

+  }
+
+  struct Balances {
+    // This contract does not store an account's locked gold that is being used in electing


This feels like a weird location to put this comment - comments above struct fields usually describe what a field represents. This one I guess is more of a "what this field does not represent" comment?

Not sure what a better place would be. Maybe just prefixing it with "NOTE:" would clear up the confusion for me personally?

Also, describing what exactly nonvoting's value represents might be helpful.

packages/protocol/contracts/governance/LockedGold.sol

m-chrzan · 2019-10-11T20:28:37Z

packages/protocol/contracts/governance/LockedGold.sol

-   * @param _account The address of the account.
-   * @param noticePeriod The notice period of the Locked Gold commitment.
-   * @return The value and index of the specified Locked Gold commitment.
+   * @notice Withdraws a gold that has been unlocked after the unlocking period has passed.


Typo: unnecessary "a".

packages/protocol/contracts/governance/LockedGold.sol

m-chrzan · 2019-10-11T20:38:14Z

packages/protocol/contracts/governance/LockedGold.sol

  )
-    private
+    public


Can this be external?

m-chrzan · 2019-10-11T20:43:38Z

packages/protocol/contracts/governance/LockedGold.sol

+   * @param r Output value r of the ECDSA signature.
+   * @param s Output value s of the ECDSA signature.
+   * @dev Fails if the address is already authorized or is an account.
+   * @dev v, r, s constitute `authorize`'s signature on `msg.sender`.


There's no variable called authorize. Presume this should be current?

Danger of search and replace

packages/protocol/contracts/governance/LockedGold.sol

m-chrzan

Got through contracts and migrations, started looking at unit tests.

m-chrzan · 2019-10-12T13:17:11Z

packages/protocol/contracts/governance/Validators.sol

+  // minimum amount of Locked Gold required in order to earn epoch rewards. Furthermore, the
+  // account will not be able to unlock Gold if it would cause the account to fall below
+  // these values.
+  // If an account has deregistered a validator or validator group and is still subject to the


Hmm, this doesn't sound like the best contract for a validator. One could be afraid of a situation in which, after registering, DeregistrationLockup goes up and BalanceRequirements is increased above one's balance. Sounds like with the current setup, a validator's locked funds could be made unexpectedly unavailable to them?

Maybe this is something we just have to live with, rely on the governance process to not have these move unfairly. The other option would be to store historical values per validator and honor the contract they made when registering.

Yup, that's exactly correct. If I, as a validator, lock up more than the required gold, and then the requirement goes up, that extra gold which was previously relatively accessible to me becomes locked up until I deregister and wait out the lockup period.

How comfortable validators are with this I suppose is dependent on their faith in the governance process.

The other option would be to store historical values per validator and honor the contract they made when registering.

This would work, but IMO the ability to adjust the requirements/lockups is a feature and not a bug.

m-chrzan · 2019-10-12T13:23:58Z

packages/protocol/contracts/governance/Validators.sol

+  }
+
+  // After deregistering a validator or validator group, the account will remain subject to the
+  // current balance requirements for this long (in seconds).


Related to the above, this comment could be read as "whatever the current requirement is at time of deregistration, that is the requirement that will be enforced when trying to unlock", which I believe is not the case, right?

Correct, Validators are subject to whatever the current requirements are. How would you suggest phrasing this?

packages/protocol/contracts/governance/Validators.sol

m-chrzan · 2019-10-12T15:04:39Z

packages/protocol/test/governance/election.ts

+          resp = await election.markGroupIneligible(group)
+        })
+
+        describe('when the group has votes', () => {})


Empty describe. Did you mean to put the its below inside of it?

Nope, not sure where this came from

m-chrzan · 2019-10-12T15:32:14Z

packages/protocol/test/governance/election.ts

+            election.revokeActive(group, value + 1, NULL_ADDRESS, NULL_ADDRESS, index)
+          )
+        })
+      })


Agreed. A whole scenario where a voters cast votes, (mock?) rewards are distributed, more voters cast votes, and then ensuring everyone ends up with the correct balance would be nice.

packages/protocol/test/governance/election.ts

m-chrzan · 2019-10-12T15:37:48Z

packages/protocol/test/governance/election.ts

+
+      it('should elect only n members from that group', async () => {
+        assertSameAddresses(await election.electValidators(), [
+          validator7,


Did you mean to have these out of order?

I believe they're ordered in the order that they would be elected by d'hondt

mcortesi

final comments. Maybe only one to review, still approving it so can be merged after solving the comments

packages/cli/src/commands/lockedgold/lock.ts

packages/cli/src/commands/validatorgroup/member.ts

mcortesi · 2019-10-15T11:47:54Z

packages/contractkit/src/wrappers/Election.ts

+  */
+
+  async markGroupEligible(validatorGroup: Address): Promise<CeloTransactionObject<boolean>> {
+    if (this.kit.defaultAccount == null) {


no. Maybe add a TODO so it's addresses on another PR

mcortesi · 2019-10-15T11:48:12Z

packages/contractkit/src/wrappers/Election.ts

+
+  async markGroupEligible(validatorGroup: Address): Promise<CeloTransactionObject<boolean>> {
+    if (this.kit.defaultAccount == null) {
+      throw new Error(`missing from at new ValdidatorUtils()`)


same... add a todo?

packages/contractkit/src/wrappers/LockedGold.ts

mcortesi · 2019-10-15T12:52:43Z

packages/protocol/contracts/governance/LockedGold.sol

-  function isValidating(address validator) external view returns (bool) {
-    IValidators validators = IValidators(registry.getAddressFor(VALIDATORS_REGISTRY_ID));
-    return validators.isValidating(validator);
+  function _decrementNonvotingAccountBalance(address account, uint256 value) private {


I though solidity linter expects all internal methods to go after public/external ones

Looks like the new version of solhint has this check disabled by default. I'll reorder these in a follow-up PR to avoid merge conflicts in asaj/pos-2

We can disable that if we want. Not sure what we prefer.

I think it would be good to enable, the function ordering in my PR is a bit nonsensical and hard to read. Only keeping it this way to minimize merge conflicts with asaj/pos-2

packages/protocol/contracts/governance/LockedGold.sol

packages/protocol/contracts/governance/Election.sol

mcortesi · 2019-10-15T14:19:03Z

packages/protocol/contracts/governance/Election.sol

+   */
+  function getActiveVotesUnitsDelta(address group, uint256 value) private view returns (uint256) {
+    // Preserve unitsDelta * total = value * totalUnits
+    return value.mul(votes.active.forGroup[group].totalUnits.add(1)).div(


Not sure to follow the math here.
So the relation is:

accountUnits / totalUnits = accountValue / totalValue

where value = votes

The question is: how much units would i gain if a add X value/votes

that means i need to solve this equation:

(newUnits) / (totalUnits + newUnits) = (addedValue) / (totalValue + addedValue)

where we need to get newUnits and the rest is given.

which doesn't look similar to what we are doing here... plus we are adding 1 to both unit and totalValue. And is not like we are (a) creating only one unit. (b) adding only 1 to totalValue.

The equation you wrote simplifies to totalUnits * addedValue = newUnits * totalValue, which, solving for newUnits, gives us totalUnits * addedValue / totalValue, i.e. the equation in the code.

If we don't add 1 to those values no one can ever place a vote, since the numerator and denominator will both be zero

Ok, thanks. I see the math now. The +1 were confusing.
I understand why you add them, but wouldn't be easier (and mathematically accurate) to first check if totalUnits == 0; and if make delta = value

So that we end with:

totalUnits = value totalValue = value delta = value userUnits = value

This is much better, thanks!

packages/protocol/contracts/governance/Election.sol

* master: (37 commits) [Wallet] Add support for social wallet (Safeguards) import (#1414) [Wallet] Implement new backup flows including social backup (#1399) Use validator set precompiles in Attestations (#1248) E2E Attestations test + various e2e improvements (#1417) Update Footer (#1331) [Wallet] New camera permission flow (#1398) [Wallet] Enable push notifications on iOS (#1389) [Wallet] Add Celo Lite toggle (UI only, zeroSync on/off in other PR) (#1369) Document npm inter-repo dependencies instructions (#1370) [wallet]Add more documentation on ZeroSync mode (#1367) [wallet]Add documentation for jndcrash (#1364) Point end-to-end tests back to master (#1372) Alfajores changes & comment on unlocking accounts (#1297) Reconfigure terraform local configuration during init to allow multiple envs (#773) Implement proof-of-stake changes (#1177) [Celotool] Update blockchain-api deploy script to automatically update faucet address (#1347) Allow most recently reporting oracle to report again (#1288) [wallet]Add documentation for ZeroSync mode (#1361) Fix Metadata registration during contract deploy (#1346) [Wallet] Enable firebase on iOS (#1344) ...

* master: (128 commits) [Wallet] Add support for social wallet (Safeguards) import (#1414) [Wallet] Implement new backup flows including social backup (#1399) Use validator set precompiles in Attestations (#1248) E2E Attestations test + various e2e improvements (#1417) Update Footer (#1331) [Wallet] New camera permission flow (#1398) [Wallet] Enable push notifications on iOS (#1389) [Wallet] Add Celo Lite toggle (UI only, zeroSync on/off in other PR) (#1369) Document npm inter-repo dependencies instructions (#1370) [wallet]Add more documentation on ZeroSync mode (#1367) [wallet]Add documentation for jndcrash (#1364) Point end-to-end tests back to master (#1372) Alfajores changes & comment on unlocking accounts (#1297) Reconfigure terraform local configuration during init to allow multiple envs (#773) Implement proof-of-stake changes (#1177) [Celotool] Update blockchain-api deploy script to automatically update faucet address (#1347) Allow most recently reporting oracle to report again (#1288) [wallet]Add documentation for ZeroSync mode (#1361) Fix Metadata registration during contract deploy (#1346) [Wallet] Enable firebase on iOS (#1344) ...

* master: (37 commits) [Wallet] Add support for social wallet (Safeguards) import (#1414) [Wallet] Implement new backup flows including social backup (#1399) Use validator set precompiles in Attestations (#1248) E2E Attestations test + various e2e improvements (#1417) Update Footer (#1331) [Wallet] New camera permission flow (#1398) [Wallet] Enable push notifications on iOS (#1389) [Wallet] Add Celo Lite toggle (UI only, zeroSync on/off in other PR) (#1369) Document npm inter-repo dependencies instructions (#1370) [wallet]Add more documentation on ZeroSync mode (#1367) [wallet]Add documentation for jndcrash (#1364) Point end-to-end tests back to master (#1372) Alfajores changes & comment on unlocking accounts (#1297) Reconfigure terraform local configuration during init to allow multiple envs (#773) Implement proof-of-stake changes (#1177) [Celotool] Update blockchain-api deploy script to automatically update faucet address (#1347) Allow most recently reporting oracle to report again (#1288) [wallet]Add documentation for ZeroSync mode (#1361) Fix Metadata registration during contract deploy (#1346) [Wallet] Enable firebase on iOS (#1344) ...

This reverts commit 17ff683.

Asa Oines added 15 commits September 18, 2019 17:44

Most things that don't touch incentives

02f39db

Trying to get things to compile

0f226c8

More work

721b9da

More changes

17a624e

Compiling

6ce7f7e

Updated validators test

c19b8b6

LockedGold tests passing

a6921d3

Validators tests passing

871d06d

Fix governance tests

e0e8cf7

Add election test file

223536d

Election tests passing

651009d

Fix governance test

a795125

Most e2e governance tests passing

9b4d431

end to end tests passing

e866ca7

Point to celo-blockchain branch

3a31670

asaj requested a review from m-chrzan as a code owner October 2, 2019 00:50

Asa Oines added 10 commits October 2, 2019 12:49

Merge master

3f35c70

Add some natspecs

c466d78

Linting

f02b05a

Small fix

70d7478

ContractKit building

69f6ab2

cli builds

15d2e8c

Update oclif

d6ef3ec

Checkpoint

615ec4f

Merge master

1bbf1e0

CLI seems to be working

3ff2fe4

asaj requested a review from mcortesi as a code owner October 3, 2019 19:29

cleanup

d3b83c8

asaj assigned timmoreton and mcortesi Oct 3, 2019

asaj mentioned this pull request Oct 11, 2019

Allow validators to specify an attestationKey with which they sign attestations #1313

Merged

Asa Oines added 2 commits October 11, 2019 12:10

Beef up documentation

64016a7

Merge branch 'master' into asaj/pos

6ffdd36

m-chrzan reviewed Oct 11, 2019

View reviewed changes

Asa Oines added 3 commits October 11, 2019 14:07

Expire previously upvoted proposals

1f18108

Address comments

506a77b

Merge branch 'master' into asaj/pos

23054fc

m-chrzan reviewed Oct 12, 2019

View reviewed changes

Asa Oines added 2 commits October 14, 2019 12:10

Address comments

90e8b58

Merge master

32557cf

mcortesi approved these changes Oct 15, 2019

View reviewed changes

mcortesi assigned asaj and unassigned mcortesi Oct 15, 2019

Asa Oines added 6 commits October 15, 2019 17:30

Address comments

b8e1797

Merge master

631894a

Address comments

4a4d6e2

Merge branch 'master' into asaj/pos

1d36857

Fix tests, linting

f3515ac

Merge branch 'master' into asaj/pos

c697213

asaj added the automerge Have PR merge automatically when checks pass label Oct 16, 2019

Asa Oines added 4 commits October 16, 2019 11:01

Remove bondeddeposits test

9aa6ec3

Merge branch 'master' into asaj/pos

ed8e5a0

Fix test

89364ab

Merge branch 'master' into asaj/pos

98d8feb

celo-ci-bot-user merged commit 17ff683 into master Oct 16, 2019

cmcewen added a commit that referenced this pull request Oct 25, 2019

Revert "Implement proof-of-stake changes (#1177)"

da60237

This reverts commit 17ff683.

mcortesi deleted the asaj/pos branch December 4, 2019 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement proof-of-stake changes #1177

Implement proof-of-stake changes #1177

asaj commented Oct 2, 2019 •

edited

Loading

m-chrzan left a comment

m-chrzan Oct 11, 2019

asaj Oct 11, 2019

m-chrzan Oct 11, 2019

m-chrzan Oct 11, 2019

asaj Oct 11, 2019

m-chrzan Oct 11, 2019

asaj Oct 11, 2019

m-chrzan Oct 11, 2019

asaj Oct 11, 2019

m-chrzan left a comment

m-chrzan Oct 12, 2019

asaj Oct 14, 2019

m-chrzan Oct 12, 2019

asaj Oct 14, 2019

m-chrzan Oct 12, 2019

asaj Oct 14, 2019

m-chrzan Oct 12, 2019

m-chrzan Oct 12, 2019

asaj Oct 14, 2019

mcortesi left a comment

mcortesi Oct 15, 2019

mcortesi Oct 15, 2019

mcortesi Oct 15, 2019

asaj Oct 16, 2019

mcortesi Oct 16, 2019

asaj Oct 16, 2019

mcortesi Oct 15, 2019

asaj Oct 16, 2019

mcortesi Oct 16, 2019

asaj Oct 16, 2019

Implement proof-of-stake changes #1177

Implement proof-of-stake changes #1177

Conversation

asaj commented Oct 2, 2019 • edited Loading

Description

Tested

Other changes

Related issues

Backwards compatibility

m-chrzan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-chrzan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcortesi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asaj commented Oct 2, 2019 •

edited

Loading