Increased Emission Supply

[rstormsf]

Title

  Title: Increased Emission Supply
  Layer: Consensus

Abstract

Emission supply needs to be increased by 2.5%. All these supply are coming from POA block rewards and should go to a specific smart contract.
This change requires modifications from Rust(AuRa) implementation and additional work to implement in Solidity.

Specification

Smart contract:

The smart contract should be able to accept POA coin.
When a certain threshold reaches, validators should be able to start new ballot on what to do with these funds.
Once the new ballot has started, every validator needs to decide what should be done with the fund:

• Send somewhere(some address to send it to)
• Burn
• Freeze til next threshold(keep it in the contract)
• If no decision has been made the funds should be returned back upon completion of a ballot
• Ballot has to have some expiration time.
• There has to be a minimum amount of votes read from
  https://github.com/poanetwork/poa-network-consensus-contracts/blob/master/contracts/BallotsStorage.sol#L35

ballotsStorage.getBallotThreshold(1)

Deployed addresses could be found here:
Sokol
Core

Only validators voting keys could be used to vote:
https://github.com/poanetwork/poa-network-consensus-contracts/blob/master/contracts/KeysManager.sol#L148

[varasev]

The threshold must be changeable, right? If so, who has to be allowed to change that?

[igorbarinov]

For know the emission release threshold is defined as 3 months.

Ballot has to have some expiration time.

Validators have distribution threshold of 7 days max when they can make a decision about distribution.

E.g. validator started a ballot 2 days after emission release threshold then ballot may have 5 days or less distribution threshold

[varasev]

Oh, I initially misunderstood. It's clear now, thanks.

[varasev]

@rstormsf @vbaranov @igorbarinov

The first version of the contracts is ready. There are totally two separate contracts:

• https://github.com/varasev/poa-network-consensus-contracts/blob/master/contracts/VotingToManageEmissionFunds.sol
• https://github.com/varasev/poa-network-consensus-contracts/blob/master/contracts/EmissionFunds.sol

These are draft versions for discussion. They don't have any tests yet.

VotingToManageEmissionFunds contract is for ballots. EmissionFunds contract will receive and keep POA coins and will be controlled only by VotingToManageEmissionFunds.

Please take a look at the code. Did I get everything correctly?

Some notes and questions:

1. The burning method sends coins to 0x0 address: https://github.com/varasev/poa-network-consensus-contracts/blob/master/contracts/EmissionFunds.sol#L43 - is it correct or it's better to think about another possible approach?

2. I've used send method instead of transfer to prevent contract blocking due to revert in case of any exception: https://github.com/varasev/poa-network-consensus-contracts/blob/master/contracts/EmissionFunds.sol#L31 - this is because of VotingToManageEmissionFunds finalize method that must be completed successfully in any case.

3. VotingToManageEmissionFunds address cannot be changed in EmissionFunds contract. It is initialized only once in constructor. Is it better to make it changeable?

4. Should we check msg.sender inside EmissionFunds fallback function to allow receiving the funds only from a specified address?

5. VotingToManageEmissionFunds finalize method performs one of the following actions:

• send coins to a specified address
• burn coins
• keep coins till the next emission release threshold

The action is performed that has received the maximum number of votes: https://github.com/varasev/poa-network-consensus-contracts/blob/master/contracts/VotingToManageEmissionFunds.sol#L278-L305

In a controversial situation it keeps coins till the next emission release threshold.

[vbaranov]

@varasev My comments:

VotingToManageEmissionFunds.sol:

1. As we don’t use different VotingData structs since we are migrating to upgradable storage why do we need duplicated definitions of common properties of ballots of all types such as getProgress, getTotalVoters, getStartTime, getEndTime, getIsFinalized etc?
2. maxOldMiningKeysDeepCheck is repeated for every Voting contract. My suggestion is to store all limits in BallotStorage.
3. areOldMiningKeysVoted - the same function in all Voting contracts. Let’s move it to BallotStorage too.
4. require(_id == nextBallotId().sub(1)); https://github.com/varasev/poa-network-consensus-contracts/blob/master/contracts/VotingToManageEmissionFunds.sol#L148 - all Voting contracts use the same storage and the same pointer to nextBallotId property, right? Thus, every ballot has a different id in the new model (just to note, that the counter is unique for every ballot type in the current production architecture). How can we know in finalize method, that we finalize ballot of proper type?
5. There is no time restriction when finalize function can be called. I guess it should be called in the frame between ballot end time and distribution threshold
6. If the last validator has voted, the finalization method could start automatically.
7. Let’s use emit keyword when smart-contract emits events

EmissionFunds.sol:

1. sendFundsTo should check that receiver is not empty, otherwise, coins will be burned
2. function EmissionFunds - let’s use constructor notation instead. We can use it from ^0.4.22
3. Let’s use emit keyword when smart-contract emits events http://solidity.readthedocs.io/en/v0.4.23/contracts.html?highlight=emit#events
4. I would suggest to check that balance is positive in _balance() method `require(address(this).balance > 0);

[varasev]

@vbaranov

VotingToManageEmissionFunds.sol:

1. Yeah, it seems that it's better to make a base VotingTo contract. The current VotingTo* contracts will inherit all repeated methods from VotingTo.

2. Yes, good idea. I'll do it.

3. I think it's better to move this function into the base VotingTo contract as I wrote above in point 1.

4. No, every voting contract uses its own storage.

5. As far as I understand, the ballot end time and distribution threshold are the same. @igorbarinov wrote above:

Validators have distribution threshold of 7 days max when they can make a decision about distribution.

E.g. validator started a ballot 2 days after emission release threshold then ballot may have 5 days or less distribution threshold.

So, the finalize method may be called any time after the distribution threshold because it must be enough time to call it. The next ballot may be started only after the previous ballot is finalized.

So, I think there is no problem here?

6. Yes, I will implement that.

7. I've already tried to use a new version in the previous commit: https://github.com/varasev/poa-network-consensus-contracts/blob/d30d0e5df2b6f97f079f5f3dfe0634ec5e29dd3b/contracts/VotingToManageEmissionFunds.sol#L110

But I can't use emit now because the rest smart contracts require v0.4.18 and use truffle 4.0.1. For that reason the Travis cannot run tests with the new v0.4.22 keywords.

I will do npm update truffle in the repository and update the code of all contracts to use emit, constructor and other v0.4.22 improvements.

EmissionFunds.sol:

1. The receiver address is already checked in the VotingToManageEmissionFunds contract: https://github.com/varasev/poa-network-consensus-contracts/blob/e2fc2234fe50594261913c7080cdb3d20039f6d7/contracts/VotingToManageEmissionFunds.sol#L111

I think it's better not to make require(_receiver != address(0)); in sendFundsTo because if some developer remove require(_sendTo != address(0)); from VotingToManageEmissionFunds in the future there may be a case when the vote with _sendTo == 0 will block finalization forever due to revert in sendFundsTo.

2. Yes, I will do as I described above in the point 7.

3. The same.

4. It's a bad idea to revert in sendFundsTo or burnFunds because of finalization in VotingToManageEmissionFunds as it was described above in point 1.

We cannot revert in these methods because they are used by _finalizeBallot: https://github.com/varasev/poa-network-consensus-contracts/blob/e2fc2234fe50594261913c7080cdb3d20039f6d7/contracts/VotingToManageEmissionFunds.sol#L310-L314

If we revert there then finalization will never be completed: https://github.com/varasev/poa-network-consensus-contracts/blob/e2fc2234fe50594261913c7080cdb3d20039f6d7/contracts/VotingToManageEmissionFunds.sol#L152

The next ballot may be started only after the previous ballot is finalized. So, if the finalize method is blocked the entire contract is blocked.

[varasev]

EmissionFunds and VotingToManageEmissionFunds.sol contracts have been refactored and updated to solidity v0.4.23: varasev/poa-network-consensus-contracts@17e1490

Now I'm working on these contracts to improve finalization function.

[varasev]

Parity team has implemented an ability to calculate the block rewards through a smart contract in Parity v1.11: openethereum/parity-ethereum#8419 (thanks!)

@phahulin thank you for your test repo.

Now we don't need to change an implementation of AuRa to achieve the goals described in the comments above.

Besides, it is possible to assign the reward directly to validator's payoutKey address.

In addition to the previously described smart contracts (see other comments above) we need to implement one more contract (let's call it BlockReward).

That contract has to keep two types of value:

• block reward value;
• emission value of the coins which will be assigned to EmissionFunds contract.

Also, the BlockReward contract will need to call KeysManager.getPayoutByMining method in case of using payoutKey instead of miningKey to assign the reward.

[varasev]

Finalization function in VotingToManageEmissionFunds has been improved: varasev/poa-network-consensus-contracts@a992e29

I'm gonna work on BlockReward contract.

[varasev]

A test BlockReward contract has been rechecked. I've created a test repo based on @phahulin's repo: https://github.com/varasev/test-block-reward/tree/f55aa24362c83102af2c124a2fe68461e4868679 (thanks again!)

Usage:

1. Install Parity 1.11 and Node.js 8.11 LTS (with npm) if they are not installed.

2. Perform the next commands:

$ git clone https://github.com/varasev/test-block-reward.git
$ cd test-block-reward
$ npm i
$ npm run start

3. Watch a log in the console. It should be seen that payoutKey balances are being increased alternately by 10 eth and the vault's balance is being increased by 5 eth every 5 seconds. An example log is available here: https://github.com/varasev/test-block-reward/blob/2c8135a951fbaf34953e4ed05661b6e7e19833ab/example.log

Payout key is read from another contract: https://github.com/varasev/test-block-reward/blob/2c8135a951fbaf34953e4ed05661b6e7e19833ab/contracts/TestBlockReward.sol#L52

4. To stop and clear this setup, perform the next command:

$ npm run clear

[varasev]

The BlockReward contract has been created and added to security-audit branch: https://github.com/poanetwork/poa-network-consensus-contracts/pull/83/files

Now I'm working on unit tests for VotingToManageEmissionFunds, EmissionFunds and BlockReward contracts.

[phahulin]

@varasev note that there is no way to change BlockReward contract in the same way as we can change address of network consensus contract - there is no multi option, and simply changing blockRewardContractAddress won't work. So without updating parity there is no way to change reward algorithm once we deploy it.

Would it be more flexible if we use BlockReward as a proxy to another "actual" contract that will calculate rewards? And add a method in BlockReward to change that contract's address.

[varasev]

@phahulin Yes, good idea. I'll think about it. Perhaps, we could use ProxyStorage to store the address of that contract.

[varasev]

We could use upgradable BlockReward contract. Today's successful tests are here: https://github.com/varasev/test-block-reward/tree/637dcac17f9a563bdb1d6e363a439372689b7bf9

BlockReward contract's address will be constant in spec.json, but its implementation will be changeable through VotingToChangeProxyAddress contract.

BlockReward will read EmissionFunds contract's address from ProxyStorage and read mining/payout keys from KeysManager (its address is also stored in ProxyStorage).

[varasev]

Is it OK, if validators will be able to vote for changing of BlockReward implementation which contains the values of rewards? In fact, they could increase reward value in this way.

[varasev]

So, we'll make BlockReward contract upgradable.

[varasev]

Smart contracts and relevant unit tests for Increased Emission Supply are ready and can be viewed here: https://github.com/poanetwork/poa-network-consensus-contracts/tree/f53f0dce1dfac8b2fb37e73f654edff1b8187397

[varasev]

BlockReward was renamed to RewardByBlock in poanetwork/poa-network-consensus-contracts#140.

RewardByBlock is upgradable, but I think we shouldn't add it to ProxyStorage.setContractAddress in advance due to security reasons. The new ProxyStorage is also upgradable, so we could add RewardByBlock implementation changing to ProxyStorage.setContractAddress in the future when it's necessary.