CIP-21: Governable LookbackWindow Smart Contract changes (#4747)

Implementation of CIP-21 smart contract changes. celo-blockchain related PR: celo-org/celo-blockchain#1136 Adds the uptimeLookbackWindow to the BlockchainParameters contract. This is only ever used on the celo-blockchain client to determine the size of the uptime lookback window which is used to compute the validator uptimeScore An important invariant, is that UptimeLookbackWindow must remain constant during an epoch. Thus, changes to the parameter only take effect on the next epoch. There's an exception to this rule during migration/deployment of this change; there uptimeLookbackWindow should be initialized to chain's current value for the parameter which is part of the ChainConfig on the genesis block. To initialize the value: On a new testnet: As part of the migrations, BlockchainParamets.initialize() will do the initialization On an existing testnet: After upgrading the contract, first call to setLookbackWindow() will the value for the current epoch. Since BlockchainParameters won't be reinitialized when this change is deployed, a patch was made to the getUptimteLookbackWindow() contract to return 12 (mainnet, baklava & alfajores value for lookbackWindow) in the case of the value not being initalized. This patch can be removed in the future, since it's only necessary until setLoobackWindow() is called and first epoch since deployment has passed Co-authored-by: Mariano Cortesi <[email protected]>
celo-org · Jan 20, 2021 · fa29d23 · fa29d23
1 parent 382b61e
commit fa29d23
Show file tree

Hide file tree

Showing 10 changed files with 228 additions and 54 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -31,7 +31,7 @@ e2e-defaults: &e2e-defaults
   docker:
     - image: celohq/circleci:geth1.9-2
   environment:
-    CELO_BLOCKCHAIN_BRANCH_TO_TEST: master
+    CELO_BLOCKCHAIN_BRANCH_TO_TEST: andres-dg/loopbackwindow-refactor
 general:
   artifacts:
     - 'mobile/coverage'

diff --git a/packages/celotool/src/e2e-tests/governance_tests.ts b/packages/celotool/src/e2e-tests/governance_tests.ts
@@ -117,41 +117,49 @@ async function calculateUptime(
   epochSize: number,
   lookbackWindow: number
 ): Promise<BigNumber[]> {
-  // The parentAggregateSeal is not counted for the first or last blocks of the epoch
-  const blocks = await concurrentMap(10, [...Array(epochSize - 2).keys()], (i) =>
-    kit.connection.getBlock(lastBlockNumberOfEpoch - epochSize + 2 + i)
+  const firstBlockNumberOfEpoch = lastBlockNumberOfEpoch - epochSize + 1
+
+  const monitoringWindow: [number, number] = [
+    firstBlockNumberOfEpoch + lookbackWindow - 1, // last block of first lookbackWindow
+    lastBlockNumberOfEpoch - 2, // we ignore last 2 block fo epoch
+  ]
+  const monitoringWindowSize = monitoringWindow[1] - monitoringWindow[0] + 1
+
+  // we need to obtain monitoring window blocks shifted by 1, since
+  // we are interested in parentAggregatedSeal that lives on the next block
+  const blocks = await concurrentMap(10, [...Array(monitoringWindowSize).keys()], (i) =>
+    kit.connection.getBlock(monitoringWindow[0] + 1 + i)
   )
   const lastSignedBlock: number[] = new Array(validatorSetSize).fill(0)
-  const tally: number[] = new Array(validatorSetSize).fill(0)
+  const upBlocks: number[] = new Array(validatorSetSize).fill(0)
 
   // Follows updateUptime() in core/blockchain.go
-  let windowBlocks = 1
   for (const block of blocks) {
+    const blockNumber = block.number - 1 // we are actually monitoring prev block signatures
     const bitmap = parseBlockExtraData(block.extraData).parentAggregatedSeal.bitmap
 
+    const isMonitoredBlock =
+      blockNumber >= monitoringWindow[0] && blockNumber <= monitoringWindow[1]
+
+    const currentLookbackWindow: [number, number] = [blockNumber - lookbackWindow + 1, blockNumber]
+
     for (let signerIndex = 0; signerIndex < validatorSetSize; signerIndex++) {
       if (bitIsSet(bitmap, signerIndex)) {
-        lastSignedBlock[signerIndex] = block.number - 1
-      }
-      if (windowBlocks < lookbackWindow) {
-        continue
+        lastSignedBlock[signerIndex] = blockNumber
       }
-      const signedBlockWindowLastBlockNum = block.number - 1
-      const signedBlockWindowFirstBlockNum = signedBlockWindowLastBlockNum - (lookbackWindow - 1)
-      if (
-        signedBlockWindowFirstBlockNum <= lastSignedBlock[signerIndex] &&
-        lastSignedBlock[signerIndex] <= signedBlockWindowLastBlockNum
-      ) {
-        tally[signerIndex]++
-      }
-    }
 
-    if (windowBlocks < lookbackWindow) {
-      windowBlocks++
+      const lastSignedWithinLookbackwindow =
+        lastSignedBlock[signerIndex] >= currentLookbackWindow[0] &&
+        lastSignedBlock[signerIndex] <= currentLookbackWindow[1]
+
+      if (isMonitoredBlock && lastSignedWithinLookbackwindow) {
+        upBlocks[signerIndex]++
+      }
     }
   }
-  const denominator = epochSize - lookbackWindow - 1
-  return tally.map((signerTally) => new BigNumber(signerTally / denominator))
+
+  const maxPotentialUpBlocks = monitoringWindowSize
+  return upBlocks.map((x) => new BigNumber(x / maxPotentialUpBlocks))
 }
 
 // TODO(asa): Test independent rotation of ecdsa, bls keys.
@@ -286,9 +294,8 @@ describe('governance tests', () => {
     return decryptedKeystore.privateKey
   }
 
-  const isLastBlockOfEpoch = (blockNumber: number, epochSize: number) => {
-    return blockNumber % epochSize === 0
-  }
+  const isLastBlockOfEpoch = (blockNumber: number, epochSize: number) =>
+    blockNumber % epochSize === 0
 
   const assertBalanceChanged = async (
     address: string,
@@ -336,18 +343,16 @@ describe('governance tests', () => {
     }
   }
 
-  const assertTargetVotingYieldUnchanged = async (blockNumber: number) => {
-    await assertTargetVotingYieldChanged(blockNumber, new BigNumber(0))
-  }
+  const assertTargetVotingYieldUnchanged = (blockNumber: number) =>
+    assertTargetVotingYieldChanged(blockNumber, new BigNumber(0))
 
   const getLastEpochBlock = (blockNumber: number, epoch: number) => {
     const epochNumber = Math.floor((blockNumber - 1) / epoch)
     return epochNumber * epoch
   }
 
-  const assertGoldTokenTotalSupplyUnchanged = async (blockNumber: number) => {
-    await assertGoldTokenTotalSupplyChanged(blockNumber, new BigNumber(0))
-  }
+  const assertGoldTokenTotalSupplyUnchanged = (blockNumber: number) =>
+    assertGoldTokenTotalSupplyChanged(blockNumber, new BigNumber(0))
 
   const assertGoldTokenTotalSupplyChanged = async (blockNumber: number, expected: BigNumber) => {
     const currentSupply = new BigNumber(await goldToken.methods.totalSupply().call({}, blockNumber))
@@ -564,7 +569,7 @@ describe('governance tests', () => {
           expectChangedScores = await getValidatorSetAccountsAtBlock(blockNumber)
           expectUnchangedScores = validatorAccounts.filter((x) => !expectChangedScores.includes(x))
           electedValidators = await getValidatorSetAccountsAtBlock(blockNumber)
-          uptime = await calculateUptime(kit, electedValidators.length, blockNumber, epoch, 2)
+          uptime = await calculateUptime(kit, electedValidators.length, blockNumber, epoch, 3)
         } else {
           expectUnchangedScores = validatorAccounts
           expectChangedScores = []
@@ -909,6 +914,7 @@ describe('governance tests', () => {
 
       await connectPeers([...gethConfig.instances, validatorGroup], verbose)
 
+      console.log('wait for validatorGroup to finish syncing')
       await waitToFinishInstanceSyncing(validatorGroup)
 
       // Connect the validating nodes to the non-validating nodes, to test that announce messages
@@ -946,13 +952,15 @@ describe('governance tests', () => {
 
       await connectValidatorPeers([...gethConfig.instances, ...additionalValidatingNodes])
 
+      console.log('wait for new validators to sync')
       await Promise.all(additionalValidatingNodes.map((i) => waitToFinishInstanceSyncing(i)))
 
       validatorAccounts = await getValidatorGroupMembers()
       assert.equal(validatorAccounts.length, 5)
       epoch = new BigNumber(await validators.methods.getEpochSize().call()).toNumber()
       assert.equal(epoch, 10)
 
+      console.log('wait for end of epoch')
       // Wait for an epoch transition to ensure everyone is connected to one another.
       await waitForEpochTransition(web3, epoch)
 

diff --git a/packages/celotool/src/e2e-tests/validator_order_tests.ts b/packages/celotool/src/e2e-tests/validator_order_tests.ts
@@ -50,7 +50,7 @@ describe('governance tests', () => {
     })
 
     it('properly orders validators randomly', async function(this: any) {
-      this.timeout(160000 /* 160 seconds */)
+      this.timeout(320000 /* 320 seconds */)
       // If a consensus round fails during this test, the results are inconclusive.
       // Retry up to two times to mitigate this issue. Restarting the nodes is not needed.
       this.retries(2)

diff --git a/packages/celotool/src/lib/geth.ts b/packages/celotool/src/lib/geth.ts
@@ -892,15 +892,6 @@ export async function startGeth(
   // TODO(ponti): add flag after Donut fork
   // const txFeeRecipient = instance.txFeeRecipient || minerValidator
   const verbosity = gethConfig.verbosity ? gethConfig.verbosity : '3'
-  let blocktime: number = 1
-
-  if (
-    gethConfig.genesisConfig &&
-    gethConfig.genesisConfig.blockTime !== undefined &&
-    gethConfig.genesisConfig.blockTime >= 0
-  ) {
-    blocktime = gethConfig.genesisConfig.blockTime
-  }
 
   const gethArgs = [
     '--datadir',
@@ -920,8 +911,6 @@ export async function startGeth(
     'extip:127.0.0.1',
     '--allow-insecure-unlock', // geth1.9 to use http w/unlocking
     '--gcmode=archive', // Needed to retrieve historical state
-    '--istanbul.blockperiod',
-    blocktime.toString()
   ]
 
   if (minerValidator) {
@@ -1058,8 +1047,9 @@ export async function startGeth(
 export function writeGenesis(gethConfig: GethRunConfig, validators: Validator[], verbose: boolean) {
   const genesis: string = generateGenesis({
     validators,
+    blockTime: 1,
     epoch: 10,
-    lookbackwindow: 2,
+    lookbackwindow: 3,
     requestTimeout: 3000,
     chainId: gethConfig.networkId,
     ...gethConfig.genesisConfig,
@@ -1212,6 +1202,9 @@ export async function migrateContracts(
         validatorKeys: validatorPrivateKeys.map(ensure0x),
         attestationKeys: attestationKeys.map(ensure0x),
       },
+      blockchainParameters: {
+        uptimeLookbackWindow: 3, // same as our default in `writeGenesis()`
+      },      
     },
     overrides
   )

diff --git a/packages/protocol/contracts/governance/BlockchainParameters.sol b/packages/protocol/contracts/governance/BlockchainParameters.sol
@@ -1,25 +1,40 @@
 pragma solidity ^0.5.13;
 
+import "openzeppelin-solidity/contracts/math/SafeMath.sol";
 import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
 import "../common/Initializable.sol";
+import "../common/UsingPrecompiles.sol";
 
 /**
  * @title Contract for storing blockchain parameters that can be set by governance.
  */
-contract BlockchainParameters is Ownable, Initializable {
+contract BlockchainParameters is Ownable, Initializable, UsingPrecompiles {
+  using SafeMath for uint256;
+
   struct ClientVersion {
     uint256 major;
     uint256 minor;
     uint256 patch;
   }
 
+  struct LookbackWindow {
+    // Value for lookbackWindow before `nextValueActivationBlock`
+    uint256 oldValue;
+    // Value for lookbackWindow after `nextValueActivationBlock`
+    uint256 nextValue;
+    // Epoch where next value is activated
+    uint256 nextValueActivationEpoch;
+  }
+
   ClientVersion private minimumClientVersion;
   uint256 public blockGasLimit;
   uint256 public intrinsicGasForAlternativeFeeCurrency;
+  LookbackWindow public uptimeLookbackWindow;
 
   event MinimumClientVersionSet(uint256 major, uint256 minor, uint256 patch);
   event IntrinsicGasForAlternativeFeeCurrencySet(uint256 gas);
   event BlockGasLimitSet(uint256 limit);
+  event UptimeLookbackWindowSet(uint256 window, uint256 activationEpoch);
 
   /**
    * @notice Used in place of the constructor to allow the contract to be upgradable via proxy.
@@ -28,18 +43,29 @@ contract BlockchainParameters is Ownable, Initializable {
    * @param patch Minimum client version that can be used in the chain, patch level.
    * @param _gasForNonGoldCurrencies Intrinsic gas for non-gold gas currencies.
    * @param gasLimit Block gas limit.
+   * @param lookbackWindow Lookback window for measuring validator uptime.
    */
   function initialize(
     uint256 major,
     uint256 minor,
     uint256 patch,
     uint256 _gasForNonGoldCurrencies,
-    uint256 gasLimit
+    uint256 gasLimit,
+    uint256 lookbackWindow
   ) external initializer {
     _transferOwnership(msg.sender);
     setMinimumClientVersion(major, minor, patch);
     setBlockGasLimit(gasLimit);
     setIntrinsicGasForAlternativeFeeCurrency(_gasForNonGoldCurrencies);
+    setUptimeLookbackWindow(lookbackWindow);
+  }
+
+  /**
+   * @notice Returns the storage, major, minor, and patch version of the contract.
+   * @return The storage, major, minor, and patch version of the contract.
+   */
+  function getVersionNumber() external pure returns (uint256, uint256, uint256, uint256) {
+    return (1, 2, 0, 0);
   }
 
   /**
@@ -75,6 +101,45 @@ contract BlockchainParameters is Ownable, Initializable {
     emit IntrinsicGasForAlternativeFeeCurrencySet(gas);
   }
 
+  /**
+   * @notice Sets the uptime lookback window.
+   * @param window New window.
+   */
+  function setUptimeLookbackWindow(uint256 window) public onlyOwner {
+    require(window >= 3 && window <= 720, "UptimeLookbackWindow must be within safe range");
+    require(
+      window <= getEpochSize().sub(2),
+      "UptimeLookbackWindow must be smaller or equal to epochSize - 2"
+    );
+
+    uptimeLookbackWindow.oldValue = _getUptimeLookbackWindow();
+
+    // changes only take place on the next epoch
+    uptimeLookbackWindow.nextValueActivationEpoch = getEpochNumber().add(1);
+    uptimeLookbackWindow.nextValue = window;
+
+    emit UptimeLookbackWindowSet(window, uptimeLookbackWindow.nextValueActivationEpoch);
+  }
+
+  /**
+   * @notice Gets the uptime lookback window.
+   */
+  function getUptimeLookbackWindow() public view returns (uint256 lookbackWindow) {
+    lookbackWindow = _getUptimeLookbackWindow();
+    require(lookbackWindow != 0, "UptimeLookbackWindow is not initialized");
+  }
+
+  /**
+   * @notice Gets the uptime lookback window.
+   */
+  function _getUptimeLookbackWindow() internal view returns (uint256 lookbackWindow) {
+    if (getEpochNumber() >= uptimeLookbackWindow.nextValueActivationEpoch) {
+      return uptimeLookbackWindow.nextValue;
+    } else {
+      return uptimeLookbackWindow.oldValue;
+    }
+  }
+
   /**
    * @notice Query minimum client version.
    * @return Returns major, minor, and patch version numbers.

diff --git a/packages/protocol/contracts/governance/test/BlockchainParametersTest.sol b/packages/protocol/contracts/governance/test/BlockchainParametersTest.sol
@@ -0,0 +1,6 @@
+pragma solidity ^0.5.13;
+
+import "../BlockchainParameters.sol";
+import "./MockUsingPrecompiles.sol";
+
+contract BlockchainParametersTest is BlockchainParameters, MockUsingPrecompiles {}
diff --git a/packages/protocol/migrations/19_blockchainparams.ts b/packages/protocol/migrations/19_blockchainparams.ts
@@ -11,6 +11,7 @@ const initializeArgs = async (_: string): Promise<any[]> => {
     version.patch,
     config.blockchainParameters.gasForNonGoldCurrencies,
     config.blockchainParameters.deploymentBlockGasLimit,
+    config.blockchainParameters.uptimeLookbackWindow,
   ]
 }
 

diff --git a/packages/protocol/migrationsConfig.js b/packages/protocol/migrationsConfig.js
@@ -42,6 +42,7 @@ const DefaultConfig = {
     },
     deploymentBlockGasLimit: 20000000,
     blockGasLimit: 13000000,
+    uptimeLookbackWindow: 12,
   },
   doubleSigningSlasher: {
     reward: '1000000000000000000000', // 1000 cGLD