Merge pull request #569 from graphprotocol/pcv/rewards-supply-fixes

Rewards: fix calculation by using a snapshot of token supply at the time that signal was last updated

Author: abarmat

config/graph.mainnet.yml

@@ -106,7 +106,9 @@ contracts:
     proxy: true
     init:
       controller: "${{Controller.address}}"
-      issuanceRate: "1000000012184945188" # per block increase of total supply, blocks in a year = 365*60*60*24/13
+    calls:
+      - fn: "setIssuanceRate"
+        _issuanceRate: "1000000012184945188" # per block increase of total supply, blocks in a year = 365*60*60*24/13
   AllocationExchange:
     init:
       graphToken: "${{GraphToken.address}}"

contracts/rewards/RewardsManager.sol

@@ -27,7 +27,7 @@ import "./IRewardsManager.sol";
  * These functions may overestimate the actual rewards due to changes in the total supply
  * until the actual takeRewards function is called.
  */
-contract RewardsManager is RewardsManagerV2Storage, GraphUpgradeable, IRewardsManager {
+contract RewardsManager is RewardsManagerV3Storage, GraphUpgradeable, IRewardsManager {
     using SafeMath for uint256;
 
     uint256 private constant TOKEN_DECIMALS = 1e18;
@@ -68,11 +68,8 @@ contract RewardsManager is RewardsManagerV2Storage, GraphUpgradeable, IRewardsMa
     /**
      * @dev Initialize this contract.
      */
-    function initialize(address _controller, uint256 _issuanceRate) external onlyImpl {
+    function initialize(address _controller) external onlyImpl {
         Managed._initialize(_controller);
-
-        // Settings
-        _setIssuanceRate(_issuanceRate);
     }
 
     // -- Config --
@@ -224,7 +221,7 @@ contract RewardsManager is RewardsManagerV2Storage, GraphUpgradeable, IRewardsMa
         }
 
         uint256 r = issuanceRate;
-        uint256 p = graphToken.totalSupply();
+        uint256 p = tokenSupplySnapshot;
         uint256 a = p.mul(_pow(r, t, TOKEN_DECIMALS)).div(TOKEN_DECIMALS);
 
         // New issuance of tokens during time steps
@@ -315,6 +312,7 @@ contract RewardsManager is RewardsManagerV2Storage, GraphUpgradeable, IRewardsMa
     function updateAccRewardsPerSignal() public override returns (uint256) {
         accRewardsPerSignal = getAccRewardsPerSignal();
         accRewardsPerSignalLastBlockUpdated = block.number;
+        tokenSupplySnapshot = graphToken().totalSupply();
         return accRewardsPerSignal;
     }
 

contracts/rewards/RewardsManagerStorage.sol

@@ -26,3 +26,8 @@ contract RewardsManagerV2Storage is RewardsManagerV1Storage {
     // Minimum amount of signaled tokens on a subgraph required to accrue rewards
     uint256 public minimumSubgraphSignal;
 }
+
+contract RewardsManagerV3Storage is RewardsManagerV2Storage {
+    // Snapshot of the total supply of GRT when accRewardsPerSignal was last updated
+    uint256 public tokenSupplySnapshot;
+}

test/lib/deployment.ts

@@ -226,7 +226,7 @@ export async function deployRewardsManager(
   return network.deployContractWithProxy(
     proxyAdmin,
     'RewardsManager',
-    [controller, defaults.rewards.issuanceRate],
+    [controller],
     deployer,
   ) as unknown as RewardsManager
 }

test/lib/fixtures.ts

@@ -75,6 +75,7 @@ export class NetworkFixture {
     await staking.connect(deployer).setSlasher(slasherAddress, true)
     await grt.connect(deployer).addMinter(rewardsManager.address)
     await gns.connect(deployer).approveAll()
+    await rewardsManager.connect(deployer).setIssuanceRate(deployment.defaults.rewards.issuanceRate)
 
     // Unpause the protocol
     await controller.connect(deployer).setPaused(false)
@@ -95,6 +96,7 @@ export class NetworkFixture {
 
   async setUp(): Promise<void> {
     this.lastSnapshotId = await evmSnapshot()
+    provider().send('evm_setAutomine', [true])
   }
 
   async tearDown(): Promise<void> {

test/rewards/rewards.test.ts

@@ -23,6 +23,7 @@ import {
   formatGRT,
   Account,
   advanceToNextEpoch,
+  provider,
 } from '../lib/testHelpers'
 
 const MAX_PPM = 1000000
@@ -50,12 +51,15 @@ describe('Rewards', () => {
   let rewardsManagerMock: RewardsManagerMock
 
   // Derive some channel keys for each indexer used to sign attestations
-  const channelKey = deriveChannelKey()
+  const channelKey1 = deriveChannelKey()
+  const channelKey2 = deriveChannelKey()
 
   const subgraphDeploymentID1 = randomHexBytes()
   const subgraphDeploymentID2 = randomHexBytes()
 
-  const allocationID = channelKey.address
+  const allocationID1 = channelKey1.address
+  const allocationID2 = channelKey2.address
+
   const metadata = HashZero
 
   const ISSUANCE_RATE_PERIODS = 4 // blocks required to issue 5% rewards
@@ -97,6 +101,10 @@ describe('Rewards', () => {
 
     async accrued() {
       const nBlocks = await this.elapsedBlocks()
+      return this.accruedByElapsed(nBlocks)
+    }
+
+    async accruedByElapsed(nBlocks: BigNumber | number) {
       const n = getRewardsPerSignal(
         new BN(this.totalSupply.toString()),
         new BN(ISSUANCE_RATE_PER_BLOCK.toString()).div(1e18),
@@ -395,9 +403,9 @@ describe('Rewards', () => {
             indexer1.address,
             subgraphDeploymentID1,
             tokensToAllocate,
-            allocationID,
+            allocationID1,
             metadata,
-            await channelKey.generateProof(indexer1.address),
+            await channelKey1.generateProof(indexer1.address),
           )
 
         // Jump
@@ -433,9 +441,9 @@ describe('Rewards', () => {
             indexer1.address,
             subgraphDeploymentID1,
             tokensToAllocate,
-            allocationID,
+            allocationID1,
             metadata,
-            await channelKey.generateProof(indexer1.address),
+            await channelKey1.generateProof(indexer1.address),
           )
 
         // Jump
@@ -477,16 +485,16 @@ describe('Rewards', () => {
             indexer1.address,
             subgraphDeploymentID1,
             tokensToAllocate,
-            allocationID,
+            allocationID1,
             metadata,
-            await channelKey.generateProof(indexer1.address),
+            await channelKey1.generateProof(indexer1.address),
           )
 
         // Jump
         await advanceBlocks(ISSUANCE_RATE_PERIODS)
 
         // Rewards
-        const contractRewards = await rewardsManager.getRewards(allocationID)
+        const contractRewards = await rewardsManager.getRewards(allocationID1)
 
         // We trust using this function in the test because we tested it
         // standalone in a previous test
@@ -523,9 +531,9 @@ describe('Rewards', () => {
             indexer1.address,
             subgraphDeploymentID1,
             tokensToAllocate,
-            allocationID,
+            allocationID1,
             metadata,
-            await channelKey.generateProof(indexer1.address),
+            await channelKey1.generateProof(indexer1.address),
           )
       }
 
@@ -566,9 +574,9 @@ describe('Rewards', () => {
             indexer1.address,
             subgraphDeploymentID1,
             tokensToAllocate,
-            allocationID,
+            allocationID1,
             metadata,
-            await channelKey.generateProof(indexer1.address),
+            await channelKey1.generateProof(indexer1.address),
           )
       }
 
@@ -599,11 +607,11 @@ describe('Rewards', () => {
         // Close allocation. At this point rewards should be collected for that indexer
         const tx = await staking
           .connect(indexer1.signer)
-          .closeAllocation(allocationID, randomHexBytes())
+          .closeAllocation(allocationID1, randomHexBytes())
         const receipt = await tx.wait()
         const event = rewardsManager.interface.parseLog(receipt.logs[1]).args
         expect(event.indexer).eq(indexer1.address)
-        expect(event.allocationID).eq(allocationID)
+        expect(event.allocationID).eq(allocationID1)
         expect(event.epoch).eq(await epochManager.currentEpoch())
         expect(toRound(event.amount)).eq(toRound(expectedIndexingRewards))
 
@@ -658,11 +666,11 @@ describe('Rewards', () => {
         // Close allocation. At this point rewards should be collected for that indexer
         const tx = await staking
           .connect(indexer1.signer)
-          .closeAllocation(allocationID, randomHexBytes())
+          .closeAllocation(allocationID1, randomHexBytes())
         const receipt = await tx.wait()
         const event = rewardsManager.interface.parseLog(receipt.logs[1]).args
         expect(event.indexer).eq(indexer1.address)
-        expect(event.allocationID).eq(allocationID)
+        expect(event.allocationID).eq(allocationID1)
         expect(event.epoch).eq(await epochManager.currentEpoch())
         expect(toRound(event.amount)).eq(toRound(expectedIndexingRewards))
 
@@ -710,7 +718,7 @@ describe('Rewards', () => {
         const beforeIndexer1Stake = await staking.getIndexerStakedTokens(indexer1.address)
 
         // Close allocation. At this point rewards should be collected for that indexer
-        await staking.connect(indexer1.signer).closeAllocation(allocationID, randomHexBytes())
+        await staking.connect(indexer1.signer).closeAllocation(allocationID1, randomHexBytes())
 
         // After state
         const afterTokenSupply = await grt.totalSupply()
@@ -756,10 +764,10 @@ describe('Rewards', () => {
         await advanceToNextEpoch(epochManager)
 
         // Close allocation. At this point rewards should be collected for that indexer
-        const tx = staking.connect(indexer1.signer).closeAllocation(allocationID, randomHexBytes())
+        const tx = staking.connect(indexer1.signer).closeAllocation(allocationID1, randomHexBytes())
         await expect(tx)
           .emit(rewardsManager, 'RewardsDenied')
-          .withArgs(indexer1.address, allocationID, await epochManager.currentEpoch())
+          .withArgs(indexer1.address, allocationID1, await epochManager.currentEpoch())
       })
     })
   })
@@ -791,9 +799,9 @@ describe('Rewards', () => {
           indexer1.address,
           subgraphDeploymentID1,
           tokensToAllocate,
-          allocationID,
+          allocationID1,
           metadata,
-          await channelKey.generateProof(indexer1.address),
+          await channelKey1.generateProof(indexer1.address),
         )
 
       // Jump
@@ -804,7 +812,107 @@ describe('Rewards', () => {
       await curation.connect(curator1.signer).burn(subgraphDeploymentID1, curatorShares, 0)
 
       // Close allocation. At this point rewards should be collected for that indexer
-      await staking.connect(indexer1.signer).closeAllocation(allocationID, randomHexBytes())
+      await staking.connect(indexer1.signer).closeAllocation(allocationID1, randomHexBytes())
+    })
+  })
+
+  describe('multiple allocations', function () {
+    it('two allocations in the same block with a GRT burn in the middle should succeed', async function () {
+      // If rewards are not monotonically increasing, this can trigger
+      // a subtraction overflow error as seen in mainnet tx:
+      // 0xb6bf7bbc446720a7409c482d714aebac239dd62e671c3c94f7e93dd3a61835ab
+      await advanceToNextEpoch(epochManager)
+
+      // Setup
+      await epochManager.setEpochLength(10)
+
+      // Update total signalled
+      const signalled1 = toGRT('1500')
+      await curation.connect(curator1.signer).mint(subgraphDeploymentID1, signalled1, 0)
+
+      // Stake
+      const tokensToStake = toGRT('12500')
+      await staking.connect(indexer1.signer).stake(tokensToStake)
+
+      // Allocate simultaneously, burning in the middle
+      const tokensToAlloc = toGRT('5000')
+      await provider().send('evm_setAutomine', [false])
+      const tx1 = await staking
+        .connect(indexer1.signer)
+        .allocateFrom(
+          indexer1.address,
+          subgraphDeploymentID1,
+          tokensToAlloc,
+          allocationID1,
+          metadata,
+          await channelKey1.generateProof(indexer1.address),
+        )
+      const tx2 = await grt.connect(indexer1.signer).burn(toGRT(1))
+      const tx3 = await staking
+        .connect(indexer1.signer)
+        .allocateFrom(
+          indexer1.address,
+          subgraphDeploymentID1,
+          tokensToAlloc,
+          allocationID2,
+          metadata,
+          await channelKey2.generateProof(indexer1.address),
+        )
+
+      await provider().send('evm_mine', [])
+      await provider().send('evm_setAutomine', [true])
+
+      await expect(tx1).emit(staking, 'AllocationCreated')
+      await expect(tx2).emit(grt, 'Transfer')
+      await expect(tx3).emit(staking, 'AllocationCreated')
+    })
+    it('two simultanous-similar allocations should get same amount of rewards', async function () {
+      await advanceToNextEpoch(epochManager)
+
+      // Setup
+      await epochManager.setEpochLength(10)
+
+      // Update total signalled
+      const signalled1 = toGRT('1500')
+      await curation.connect(curator1.signer).mint(subgraphDeploymentID1, signalled1, 0)
+
+      // Stake
+      const tokensToStake = toGRT('12500')
+      await staking.connect(indexer1.signer).stake(tokensToStake)
+
+      // Allocate simultaneously
+      const tokensToAlloc = toGRT('5000')
+      const tx1 = await staking.populateTransaction.allocateFrom(
+        indexer1.address,
+        subgraphDeploymentID1,
+        tokensToAlloc,
+        allocationID1,
+        metadata,
+        await channelKey1.generateProof(indexer1.address),
+      )
+      const tx2 = await staking.populateTransaction.allocateFrom(
+        indexer1.address,
+        subgraphDeploymentID1,
+        tokensToAlloc,
+        allocationID2,
+        metadata,
+        await channelKey2.generateProof(indexer1.address),
+      )
+      await staking.connect(indexer1.signer).multicall([tx1.data, tx2.data])
+
+      // Jump
+      await advanceToNextEpoch(epochManager)
+
+      // Close allocations simultaneously
+      const tx3 = await staking.populateTransaction.closeAllocation(allocationID1, randomHexBytes())
+      const tx4 = await staking.populateTransaction.closeAllocation(allocationID2, randomHexBytes())
+      const tx5 = await staking.connect(indexer1.signer).multicall([tx3.data, tx4.data])
+
+      // Both allocations should receive the same amount of rewards
+      const receipt = await tx5.wait()
+      const event1 = rewardsManager.interface.parseLog(receipt.logs[1]).args
+      const event2 = rewardsManager.interface.parseLog(receipt.logs[5]).args
+      expect(event1.amount).eq(event2.amount)
     })
   })
 })

test/staking/allocation.test.ts

@@ -18,7 +18,7 @@ import {
   advanceEpochs,
 } from '../lib/testHelpers'
 
-const { AddressZero, HashZero } = constants
+const { AddressZero } = constants
 
 const MAX_PPM = toBN('1000000')