Closed rstormsf closed 4 years ago
The threshold must be changeable, right? If so, who has to be allowed to change that?
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
Oh, I initially misunderstood. It's clear now, thanks.
@rstormsf @vbaranov @igorbarinov
The first version of the contracts is ready. There are totally two separate contracts:
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?
1) 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.
1) VotingToManageEmissionFunds
address cannot be changed in EmissionFunds
contract. It is initialized only once in constructor. Is it better to make it changeable?
1) Should we check msg.sender
inside EmissionFunds
fallback function to allow receiving the funds only from a specified address?
1) VotingToManageEmissionFunds
finalize method performs one of the following actions:
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
.
@varasev My comments:
VotingToManageEmissionFunds.sol:
maxOldMiningKeysDeepCheck
is repeated for every Voting contract. My suggestion is to store all limits in BallotStorage.areOldMiningKeysVoted
- the same function in all Voting contracts. Let’s move it to BallotStorage too.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?finalize
function can be called. I guess it should be called in the frame between ballot end time and distribution thresholdemit
keyword when smart-contract emits eventsEmissionFunds.sol:
sendFundsTo
should check that receiver is not empty, otherwise, coins will be burnedfunction EmissionFunds
- let’s use constructor notation instead. We can use it from ^0.4.22emit
keyword when smart-contract emits events http://solidity.readthedocs.io/en/v0.4.23/contracts.html?highlight=emit#events@vbaranov
VotingToManageEmissionFunds.sol:
Yeah, it seems that it's better to make a base VotingTo
contract. The current VotingTo* contracts will inherit all repeated methods from VotingTo
.
Yes, good idea. I'll do it.
I think it's better to move this function into the base VotingTo
contract as I wrote above in point 1.
No, every voting contract uses its own storage.
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 lessdistribution 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?
Yes, I will implement that.
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:
VotingToManageEmissionFunds
contract: https://github.com/varasev/poa-network-consensus-contracts/blob/e2fc2234fe50594261913c7080cdb3d20039f6d7/contracts/VotingToManageEmissionFunds.sol#L111I 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
.
Yes, I will do as I described above in the point 7.
The same.
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.
EmissionFunds
and VotingToManageEmissionFunds.sol
contracts have been refactored and updated to solidity v0.4.23: https://github.com/varasev/poa-network-consensus-contracts/commit/17e14900470f96b9dea8c462ff36480c755b0360
Now I'm working on these contracts to improve finalization function.
Parity team has implemented an ability to calculate the block rewards through a smart contract in Parity v1.11: https://github.com/paritytech/parity/pull/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:
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.
Finalization function in VotingToManageEmissionFunds
has been improved: https://github.com/varasev/poa-network-consensus-contracts/commit/a992e29d456bc6b138db4727ce54b97698515004
I'm gonna work on BlockReward
contract.
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:
Install Parity 1.11 and Node.js 8.11 LTS (with npm) if they are not installed.
Perform the next commands:
$ git clone https://github.com/varasev/test-block-reward.git
$ cd test-block-reward
$ npm i
$ npm run start
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.logPayout key is read from another contract: https://github.com/varasev/test-block-reward/blob/2c8135a951fbaf34953e4ed05661b6e7e19833ab/contracts/TestBlockReward.sol#L52
$ npm run clear
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.
@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.
@phahulin Yes, good idea. I'll think about it. Perhaps, we could use ProxyStorage
to store the address of that contract.
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
).
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.
So, we'll make BlockReward
contract upgradable.
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
BlockReward
was renamed to RewardByBlock
in https://github.com/poanetwork/poa-network-consensus-contracts/pull/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.
Title
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:
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