Miner variable rewards I #399

kronosapiens · 2018-10-27T07:15:08Z

Partially addresses #164

Variable miner rewards as per WP sec 7.8 equations 3 & 4
Token staking timestamp updates as average of existing and new stake.
Currently total reward is fixed at 1200 CLNY.

elenadimitrova · 2018-11-08T15:00:02Z

contracts/ColonyNetworkMining.sol

@@ -113,29 +114,61 @@ contract ColonyNetworkMining is ColonyNetworkStorage {
    }
  }

-  function rewardStakers(address[] stakers) internal {
+  // Constants for miner weight calculations
+  uint256 constant T = 7776000 * WAD; // Seconds in 90 days


Update comment to be clear it is Seconds in 90 days * WAD

elenadimitrova · 2018-11-08T15:00:20Z

contracts/ColonyNetworkMining.sol

-  function rewardStakers(address[] stakers) internal {
+  // Constants for miner weight calculations
+  uint256 constant T = 7776000 * WAD; // Seconds in 90 days
+  uint256 constant N = 24 * WAD; // 2x maximum number of miners


2x maximum number of miners * WAD

elenadimitrova · 2018-11-08T15:03:04Z

contracts/IColonyNetwork.sol

+  /// @param _timeStaked Amount of time (in seconds) that the miner has staked their CLNY
+  /// @param _submissonIndex Index of reputation hash submission (between 1 and 12)
+  /// @return minerWeight The weight of miner reward
+  function calculateMinerWeight(uint256 _timeStaked, uint256 _submissonIndex) public view returns (uint256 minerWeight);


Function can be set to pure (both here and in the implementation)

elenadimitrova · 2018-11-08T15:05:54Z

contracts/IReputationMiningCycle.sol

@@ -199,12 +199,13 @@ contract IReputationMiningCycle {

  /// @notice Start the reputation log with the rewards for the stakers who backed the accepted new reputation root hash.
  /// @param stakers The array of stakers addresses to receive the reward.
-  /// @param commonColonyAddress The address of the common colony, which the special mining skill is earned in


aaa we still had this leftover common colony terminology around! Glad you found it.

elenadimitrova · 2018-11-08T15:14:17Z

contracts/IReputationMiningCycle.sol

  /// @param reward The amount of reputation to be rewarded to each staker
  /// @dev Only callable by colonyNetwork
  /// @dev Note that the same address might be present multiple times in `stakers` - this is acceptable, and indicates the
  /// same address backed the same hash multiple times with different entries.
-  function rewardStakersWithReputation(address[] stakers, address commonColonyAddress, uint reward, uint miningSkillId) public;
+  function rewardStakersWithReputation(address[] stakers, uint256[] weights, address metaColonyAddress, uint reward, uint miningSkillId) public;


I know this was there beforehand but can we update the uint parameters to be explicitly uint256

elenadimitrova · 2018-11-08T15:19:40Z

contracts/TokenLocking.sol

+
+    uint256 timestamp = add(mul(prevWeight, lock.timestamp), mul(currWeight, now)) / add(prevWeight, currWeight);
+
+    userLocks[_token][msg.sender] = Lock(totalLockCount[_token], add(lock.balance, _amount), timestamp);


The above lock variable is a storage reference so you can set any of it properties directly e.g.

lock.balance = add(lock.balance, _amount); lock.timestamp = timestamp;

True, but since we're setting all of the fields simultaneously it seems justified to do it in one go.

I don't see us setting all the fields here, totalLockCount is not changed just the other two.

Hrm, could there be a situation where totalLockCount[_token] is different than it was when the last deposit was made?

@elenadimitrova This test fails if we don't also set totalLockCount[_token]:

should be able to set user lock count equal to total lock count when depositing if user had 0 deposited tokens:

Yes, you're right. Glad I dug in this function to find another issue (#437), unrelated to the changes here.
At least I'd like to reuse the lock storage reference and do

lock = Lock(totalLockCount[_token], add(lock.balance, _amount), newTimestamp);

elenadimitrova · 2018-11-08T17:35:03Z

contracts/ColonyNetworkMining.sol

+
+    for (i = 0; i < stakers.length; i++) {
+      (,,timeStaked) = ITokenLocking(tokenLocking).getUserLock(clnyToken, stakers[i]);
+      minerWeights[i] = calculateMinerWeight(now - timeStaked, i + 1);


If the submissionIndex in calculateMinerWeight is 0-based then there won't be need to do i + 1 here and submissionIndex - 1 there.

elenadimitrova · 2018-11-08T18:17:48Z

contracts/ReputationMiningCycle.sol

@@ -188,7 +190,8 @@ contract ReputationMiningCycle is ReputationMiningCycleStorage, PatriciaTreeProo
    IColonyNetwork(colonyNetworkAddress).setReputationRootHash(
      submission.proposedNewRootHash,
      submission.nNodes,
-      submittedHashes[submission.proposedNewRootHash][submission.nNodes]
+      submittedHashes[submission.proposedNewRootHash][submission.nNodes],
+      1200 * WAD // TODO: Make this a function of reputation state


Can we avoid going round the houses and passing the reward to IColonyNetwork. setReputationRootHash which calls IColonyNetwork.rewardStakers which in turns circles back to here IReputationMiningCycle.rewardStakersWithReputation and make it a separate internal contract call?

If that 👆 works we'll avoid importing ITokenLocking in ColonyNetworkMining too and reuse the import here.

Rewarding miners with tokens can stay in ColonyNetworkMining but reputation rewards can likely be done here.

Hmm. I would say in that case let's move the whole reward flow into ReputationMiningCycle. Having the reward logic split over two contracts seems unnecessary; we can get metaColonyAddress via IColonyNetwork(colonyNetworkAddress).

Erm wait, I'm noticing that we're not technically going in a circle -- the original contract is the activeReputationMiningCycle and we call back into the inactiveReputationMiningCycle. So this circuitous route is not so circuitous after all.

You're right. I still experimented a little with refactoring this but got no significant benefits clarity wise. Considering confirmNewHash gas cost of 687,823 which is less than a dollar at current prices this isn't a priority atm so happy to leave as is, for now at least.

area · 2018-11-12T10:57:57Z

test/token-locking.js

+      const info2 = await tokenLocking.getUserLock(token.address, userAddress);
+
+      const avgTime = (time1 + time2) / 2;
+      assert.closeTo(info2[2].toNumber(), avgTime, 1); // Tolerance of 1 second


This test can be made deterministic if you query the most recent block after each deposit, getting the actual timestamp the transaction was executed at.

Shouldn't there also be tests where the deposit amount is unequal?

I can do that

area · 2018-11-12T11:09:31Z

test/colony-network-mining.js

+      assert.isTrue(balance1Updated.sub(REWARD.divn(2)).gtn(0), "Account was not rewarded properly");
+      // Less than half of the reward
+      assert.isTrue(balance2Updated.sub(REWARD.divn(2)).ltn(0), "Account was not rewarded properly");
+      // Sum is total reward within 100 wei of precision error


This 100 wei figure is completely arbitrary?

Was it a conscious decision to only be checking that the total awarded was at least the right amount, rather than no more than the right amount? Are we enable to ensure we always land on one side or the other?

It's somewhat arbitrary, as a "very small round number" (although I just tried it with 10 wei and it passed). And I believe that closeTo test is double-sided, looking for whether the amount is within 100 of the target in either direction.

You know, turns out even equal works. But part of me still prefers to write the test in terms of tolerance, since someone could change the token values later and the test could then fail for unrelated precision reasons.

Will write test in terms of stakers.length wei.

area · 2018-11-12T11:29:56Z

contracts/ColonyNetworkMining.sol

-      // Also give them some newly minted tokens.
-      ERC20Extended(IColony(metaColony).getToken()).transfer(stakers[i], reward);
+    for (i = 0; i < stakers.length; i++) {
+      ERC20Extended(clnyToken).transfer(stakers[i], wmul(reward, minerWeights[i]));


There's going to be some dust left at the end of this, no? Can we explicitly burn it, or something, rather than leaving it in an address that we can't recover it from?

There's also the issue with rounding up 0.5s with wmul/wdiv... We could calculate how much we should mint, and mint that? But then the total reward is not exactly 1200 CLNY...

Mm I like the idea of working backwards to figure out the amount to mint. Saying "we'd like to give out around X CLNY, but given some slight imprecision we're going to figure out what the rewards should be and mint that amount".

I'm proposing we introduce a realReward value which is calculated as a sum of the weighted reward, which avoids dust issues. Given that the reputation amount is meant to be variable anyway (i.e. a function of the reputation state), I think this approach is reasonable for now. If we need stronger guarantees we can circle back around to this issue.

kronosapiens · 2018-11-13T09:51:15Z

@elenadimitrova @area Have updated in response to feedback

elenadimitrova assigned kronosapiens Oct 27, 2018

kronosapiens force-pushed the feature/miner-rewards branch from be707ca to e39a0d7 Compare October 27, 2018 07:32

kronosapiens added the in-progress label Oct 27, 2018

kronosapiens force-pushed the feature/miner-rewards branch 2 times, most recently from 49c4730 to adac96b Compare October 27, 2018 15:24

kronosapiens changed the title ~~Miner variable rewards~~ Miner variable rewards I Oct 27, 2018

kronosapiens added the ready-for-review label Oct 27, 2018

kronosapiens requested review from elenadimitrova and area October 27, 2018 16:26

elenadimitrova removed the in-progress label Oct 27, 2018

area mentioned this pull request Nov 6, 2018

Integer overflow in ColonyNetworkMining #413

Closed

elenadimitrova added under-review and removed ready-for-review labels Nov 8, 2018

elenadimitrova reviewed Nov 8, 2018

View reviewed changes

elenadimitrova added this to the Sprint 12 milestone Nov 8, 2018

area reviewed Nov 12, 2018

View reviewed changes

kronosapiens force-pushed the feature/miner-rewards branch from adac96b to e198242 Compare November 12, 2018 13:55

elenadimitrova modified the milestones: Sprint 12, Sprint 13 Nov 12, 2018

kronosapiens force-pushed the feature/miner-rewards branch from e198242 to 57012e6 Compare November 13, 2018 09:50

kronosapiens force-pushed the feature/miner-rewards branch from 57012e6 to 058aa3c Compare November 13, 2018 11:02

kronosapiens added 4 commits November 15, 2018 11:44

Add Lock.timestamp and minerWeight calculation (#164 I)

13834d4

Implement variable miner rewards (#164 II)

5a30000

Respond to reviewer feedback I

d464918

Respond to reviewer feedback II

2ff6421

kronosapiens force-pushed the feature/miner-rewards branch from 058aa3c to 2ff6421 Compare November 15, 2018 09:44

elenadimitrova approved these changes Nov 15, 2018

View reviewed changes

kronosapiens merged commit 98522b6 into develop Nov 15, 2018

kronosapiens deleted the feature/miner-rewards branch November 15, 2018 17:35

elenadimitrova removed under-review labels Nov 16, 2018


		uint256 timestamp = add(mul(prevWeight, lock.timestamp), mul(currWeight, now)) / add(prevWeight, currWeight);

		userLocks[_token][msg.sender] = Lock(totalLockCount[_token], add(lock.balance, _amount), timestamp);

Miner variable rewards I #399

Miner variable rewards I #399

Conversation

kronosapiens commented Oct 27, 2018 • edited Loading

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

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

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

kronosapiens Nov 12, 2018 • edited Loading

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

kronosapiens Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

kronosapiens commented Nov 13, 2018

kronosapiens commented Oct 27, 2018 •

edited

Loading

kronosapiens Nov 12, 2018 •

edited

Loading

kronosapiens Nov 13, 2018 •

edited

Loading