fix: various fixes for BlockhashRNG and RNGWillFallback, use block timestamp

jaybuidl · jaybuidl · commit f0746bdb4a38 · 2025-07-24T19:12:50.000+02:00
diff --git a/contracts/deploy/00-home-chain-arbitration.ts b/contracts/deploy/00-home-chain-arbitration.ts
@@ -43,7 +43,11 @@ const deployArbitration: DeployFunction = async (hre: HardhatRuntimeEnvironment)
 
   const blockhashRng = await getContractOrDeploy(hre, "BlockHashRNG", {
     from: deployer,
-    args: [],
+    args: [
+      deployer, // governor
+      deployer, // consumer (configured to SortitionModule later)
+      600, // lookaheadTime: 10 minutes in seconds
+    ],
     log: true,
   });
 
diff --git a/contracts/deploy/00-rng.ts b/contracts/deploy/00-rng.ts
@@ -8,7 +8,7 @@ import { getContractOrDeploy } from "./utils/getContractOrDeploy";
 const deployArbitration: DeployFunction = async (hre: HardhatRuntimeEnvironment) => {
   const { deployments, getNamedAccounts, getChainId, ethers } = hre;
   const { deploy } = deployments;
-  const RNG_LOOKAHEAD = 20;
+  const RNG_LOOKAHEAD_TIME = 30 * 60; // 30 minutes in seconds
 
   // fallback to hardhat node signers on local network
   const deployer = (await getNamedAccounts()).deployer ?? (await hre.ethers.getSigners())[0].address;
@@ -32,11 +32,15 @@ const deployArbitration: DeployFunction = async (hre: HardhatRuntimeEnvironment)
 
   const rng2 = await deploy("BlockHashRNG", {
     from: deployer,
-    args: [],
+    args: [
+      deployer, // governor
+      sortitionModule.target, // consumer
+      RNG_LOOKAHEAD_TIME,
+    ],
     log: true,
   });
 
-  await sortitionModule.changeRandomNumberGenerator(rng2.address, RNG_LOOKAHEAD);
+  await sortitionModule.changeRandomNumberGenerator(rng2.address);
 };
 
 deployArbitration.tags = ["RNG"];
diff --git a/contracts/src/rng/BlockhashRNG.sol b/contracts/src/rng/BlockhashRNG.sol
@@ -7,42 +7,114 @@ import "./IRNG.sol";
 /// @title Random Number Generator using blockhash with fallback.
 /// @dev
 ///  Random Number Generator returning the blockhash with a fallback behaviour.
-///  In case no one called it within the 256 blocks, it returns the previous blockhash.
-///  This contract must be used when returning 0 is a worse failure mode than returning another blockhash.
-///  Allows saving the random number for use in the future. It allows the contract to still access the blockhash even after 256 blocks.
+///  On L2 like Arbitrum block production is sporadic so block timestamp is more reliable than block number.
+///  Returns 0 when no random number is available.
+///  Allows saving the random number for use in the future. It allows the contract to retrieve the blockhash even after the time window.
 contract BlockHashRNG is IRNG {
-    uint256 public immutable lookahead; // Minimal block distance between requesting and obtaining a random number.
-    uint256 public requestBlock; // Block number of the current request
-    mapping(uint256 block => uint256 number) public randomNumbers; // randomNumbers[block] is the random number for this block, 0 otherwise.
+    // ************************************* //
+    // *             Storage               * //
+    // ************************************* //
 
-    constructor(uint256 _lookahead) {
-        lookahead = _lookahead + lookahead;
+    address public governor; // The address that can withdraw funds.
+    address public consumer; // The address that can request random numbers.
+    uint256 public immutable lookaheadTime; // Minimal time in seconds between requesting and obtaining a random number.
+    uint256 public requestTimestamp; // Timestamp of the current request
+    mapping(uint256 timestamp => uint256 number) public randomNumbers; // randomNumbers[timestamp] is the random number for this timestamp, 0 otherwise.
+
+    // ************************************* //
+    // *        Function Modifiers         * //
+    // ************************************* //
+
+    modifier onlyByGovernor() {
+        require(governor == msg.sender, "Governor only");
+        _;
+    }
+
+    modifier onlyByConsumer() {
+        require(consumer == msg.sender, "Consumer only");
+        _;
+    }
+
+    // ************************************* //
+    // *            Constructor            * //
+    // ************************************* //
+
+    /// @dev Constructor.
+    /// @param _governor The Governor of the contract.
+    /// @param _consumer The address that can request random numbers.
+    /// @param _lookaheadTime The time lookahead in seconds for the random number.
+    constructor(address _governor, address _consumer, uint256 _lookaheadTime) {
+        governor = _governor;
+        consumer = _consumer;
+        lookaheadTime = _lookaheadTime;
+    }
+
+    // ************************************* //
+    // *             Governance            * //
+    // ************************************* //
+
+    /// @dev Changes the governor of the contract.
+    /// @param _governor The new governor.
+    function changeGovernor(address _governor) external onlyByGovernor {
+        governor = _governor;
+    }
+
+    /// @dev Changes the consumer of the RNG.
+    /// @param _consumer The new consumer.
+    function changeConsumer(address _consumer) external onlyByGovernor {
+        consumer = _consumer;
     }
 
+    // ************************************* //
+    // *         State Modifiers           * //
+    // ************************************* //
+
     /// @dev Request a random number.
-    function requestRandomness() external override {
-        requestBlock = block.number;
+    function requestRandomness() external override onlyByConsumer {
+        requestTimestamp = block.timestamp;
     }
 
     /// @dev Return the random number. If it has not been saved and is still computable compute it.
     /// @return randomNumber The random number or 0 if it is not ready or has not been requested.
-    function receiveRandomness() external override returns (uint256 randomNumber) {
-        uint256 expectedBlock = requestBlock;
-        randomNumber = randomNumbers[expectedBlock];
+    function receiveRandomness() external override onlyByConsumer returns (uint256 randomNumber) {
+        if (requestTimestamp == 0) return 0; // No request made
+
+        uint256 expectedTimestamp = requestTimestamp + lookaheadTime;
+
+        // Check if enough time has passed
+        if (block.timestamp < expectedTimestamp) {
+            return 0; // Not ready yet
+        }
+
+        // Check if we already have a saved random number for this timestamp window
+        randomNumber = randomNumbers[expectedTimestamp];
         if (randomNumber != 0) {
             return randomNumber;
         }
 
-        if (expectedBlock < block.number) {
-            // The random number is not already set and can be.
-            if (blockhash(expectedBlock) != 0x0) {
-                // Normal case.
-                randomNumber = uint256(blockhash(expectedBlock));
-            } else {
-                // The contract was not called in time. Fallback to returning previous blockhash.
-                randomNumber = uint256(blockhash(block.number - 1));
-            }
+        // Use last block hash for randomness
+        randomNumber = uint256(blockhash(block.number - 1));
+        if (randomNumber != 0) {
+            randomNumbers[expectedTimestamp] = randomNumber;
         }
-        randomNumbers[expectedBlock] = randomNumber;
+        return randomNumber;
+    }
+
+    // ************************************* //
+    // *             View Functions        * //
+    // ************************************* //
+
+    /// @dev Check if randomness is ready to be received.
+    /// @return ready True if randomness can be received.
+    function isRandomnessReady() external view returns (bool ready) {
+        if (requestTimestamp == 0) return false;
+        return block.timestamp >= requestTimestamp + lookaheadTime;
+    }
+
+    /// @dev Get the timestamp when randomness will be ready.
+    /// @return readyTimestamp The timestamp when randomness will be available.
+    function getRandomnessReadyTimestamp() external view returns (uint256 readyTimestamp) {
+        if (requestTimestamp == 0) return 0;
+        return requestTimestamp + lookaheadTime;
     }
 }
diff --git a/contracts/src/rng/RNGWithFallback.sol b/contracts/src/rng/RNGWithFallback.sol
@@ -13,9 +13,10 @@ contract RNGWithFallback is IRNG {
     // ************************************* //
 
     address public governor; // Governor address
+    address public consumer; // Consumer address
     IRNG[] public rngs; // List of RNG implementations
-    uint256 public fallbackTimeout; // Number of blocks to wait before falling back to next RNG
-    uint256 public requestBlock; // Block number of the current request
+    uint256 public fallbackTimeoutSeconds; // Time in seconds to wait before falling back to next RNG
+    uint256 public requestTimestamp; // Timestamp of the current request
     uint256 public currentRngIndex; // Index of the current RNG
     bool public isRequesting; // Whether a request is in progress
 
@@ -25,6 +26,7 @@ contract RNGWithFallback is IRNG {
 
     event RNGDefaultChanged(address indexed _newDefaultRng);
     event RNGFallback(uint256 _fromIndex, uint256 _toIndex);
+    event RNGFailure();
     event RNGFallbackAdded(address indexed _rng);
     event RNGFallbackRemoved(address indexed _rng);
     event FallbackTimeoutChanged(uint256 _newTimeout);
@@ -34,14 +36,15 @@ contract RNGWithFallback is IRNG {
     // ************************************* //
 
     /// @param _governor Governor address
-    /// @param _fallbackTimeout Number of blocks to wait before falling back to next RNG
+    /// @param _consumer Consumer address
+    /// @param _fallbackTimeoutSeconds Time in seconds to wait before falling back to next RNG
     /// @param _defaultRng The default RNG
-    constructor(address _governor, uint256 _fallbackTimeout, IRNG _defaultRng) {
+    constructor(address _governor, address _consumer, uint256 _fallbackTimeoutSeconds, IRNG _defaultRng) {
         require(address(_defaultRng) != address(0), "Invalid default RNG");
-        require(_fallbackTimeout > 0, "Invalid fallback timeout");
 
         governor = _governor;
-        fallbackTimeout = _fallbackTimeout;
+        consumer = _consumer;
+        fallbackTimeoutSeconds = _fallbackTimeoutSeconds;
         rngs.push(_defaultRng);
     }
 
@@ -54,26 +57,31 @@ contract RNGWithFallback is IRNG {
         _;
     }
 
+    modifier onlyByConsumer() {
+        require(msg.sender == consumer, "Consumer only");
+        _;
+    }
+
     // ************************************* //
     // *         State Modifiers          * //
     // ************************************* //
 
     /// @dev Request a random number from the default RNG
-    function requestRandomness() external override {
+    function requestRandomness() external override onlyByConsumer {
         require(!isRequesting, "Request already in progress");
-        _requestRandomness();
+        _requestRandomness(DEFAULT_RNG);
     }
 
-    function _requestRandomness() internal {
+    function _requestRandomness(uint256 _rngIndex) internal {
         isRequesting = true;
-        requestBlock = block.number;
-        currentRngIndex = DEFAULT_RNG;
-        rngs[DEFAULT_RNG].requestRandomness();
+        requestTimestamp = block.timestamp;
+        currentRngIndex = _rngIndex;
+        rngs[_rngIndex].requestRandomness();
     }
 
     /// @dev Receive the random number with fallback logic
     /// @return randomNumber Random Number
-    function receiveRandomness() external override returns (uint256 randomNumber) {
+    function receiveRandomness() external override onlyByConsumer returns (uint256 randomNumber) {
         // Try to get random number from current RNG
         randomNumber = rngs[currentRngIndex].receiveRandomness();
 
@@ -84,14 +92,17 @@ contract RNGWithFallback is IRNG {
         }
 
         // If the timeout is exceeded, try next RNG
-        if (block.number > requestBlock + fallbackTimeout) {
+        if (block.timestamp > requestTimestamp + fallbackTimeoutSeconds) {
             uint256 nextIndex = currentRngIndex + 1;
 
             // If we have another RNG to try, switch to it and request again
             if (nextIndex < rngs.length) {
                 emit RNGFallback(currentRngIndex, nextIndex);
                 currentRngIndex = nextIndex;
-                rngs[nextIndex].requestRandomness();
+                _requestRandomness(nextIndex);
+            } else {
+                // No more RNGs to try
+                emit RNGFailure();
             }
         }
         return randomNumber;
@@ -104,10 +115,15 @@ contract RNGWithFallback is IRNG {
     /// @dev Change the governor
     /// @param _newGovernor Address of the new governor
     function changeGovernor(address _newGovernor) external onlyByGovernor {
-        require(_newGovernor != address(0), "Invalid governor");
         governor = _newGovernor;
     }
 
+    /// @dev Change the consumer
+    /// @param _consumer Address of the new consumer
+    function changeConsumer(address _consumer) external onlyByGovernor {
+        consumer = _consumer;
+    }
+
     /// @dev Change the default RNG
     /// @param _newDefaultRng Address of the new default RNG
     function changeDefaultRng(IRNG _newDefaultRng) external onlyByGovernor {
@@ -116,7 +132,7 @@ contract RNGWithFallback is IRNG {
         emit RNGDefaultChanged(address(_newDefaultRng));
 
         // Take over any pending request
-        _requestRandomness();
+        _requestRandomness(DEFAULT_RNG);
     }
 
     /// @dev Add a new RNG fallback
@@ -142,17 +158,18 @@ contract RNGWithFallback is IRNG {
     }
 
     /// @dev Change the fallback timeout
-    /// @param _newTimeout New timeout in blocks
-    function changeFallbackTimeout(uint256 _newTimeout) external onlyByGovernor {
-        require(_newTimeout > 0, "Invalid timeout");
-        fallbackTimeout = _newTimeout;
-        emit FallbackTimeoutChanged(_newTimeout);
+    /// @param _fallbackTimeoutSeconds New timeout in seconds
+    function changeFallbackTimeout(uint256 _fallbackTimeoutSeconds) external onlyByGovernor {
+        fallbackTimeoutSeconds = _fallbackTimeoutSeconds;
+        emit FallbackTimeoutChanged(_fallbackTimeoutSeconds);
     }
 
-    /// @dev Drop the pending request.
+    /// @dev Emergency reset the RNG.
     /// Useful for the governor to ensure that re-requesting a random number will not be blocked by a previous request.
-    function dropPendingRequest() external onlyByGovernor {
+    function emergencyReset() external onlyByGovernor {
         isRequesting = false;
+        requestTimestamp = 0;
+        currentRngIndex = DEFAULT_RNG;
     }
 
     // ************************************* //
diff --git a/contracts/test/rng/index.ts b/contracts/test/rng/index.ts
@@ -3,7 +3,6 @@ import { deployments, ethers, network } from "hardhat";
 import { IncrementalNG, BlockHashRNG, ChainlinkRNG, ChainlinkVRFCoordinatorV2Mock } from "../../typechain-types";
 
 const initialNg = 424242;
-const abiCoder = ethers.AbiCoder.defaultAbiCoder();
 
 describe("IncrementalNG", async () => {
   let rng: IncrementalNG;
@@ -28,15 +27,48 @@ describe("BlockHashRNG", async () => {
   let rng: BlockHashRNG;
 
   beforeEach("Setup", async () => {
-    const rngFactory = await ethers.getContractFactory("BlockHashRNG");
-    rng = (await rngFactory.deploy(1)) as BlockHashRNG;
+    const [deployer] = await ethers.getSigners();
+    await deployments.delete("BlockHashRNG");
+    await deployments.deploy("BlockHashRNG", {
+      from: deployer.address,
+      args: [deployer.address, deployer.address, 10], // governor, consumer, lookaheadTime (seconds)
+    });
+    rng = await ethers.getContract<BlockHashRNG>("BlockHashRNG");
   });
 
-  it("Should return a non-zero number for a block number", async () => {
-    const tx = await rng.receiveRandomness();
-    const trace = await network.provider.send("debug_traceTransaction", [tx.hash]);
-    const [rn] = abiCoder.decode(["uint"], ethers.getBytes(`${trace.returnValue}`));
-    expect(rn).to.not.equal(0);
+  it("Should return a non-zero number after requesting and waiting", async () => {
+    // First request randomness
+    await rng.requestRandomness();
+
+    // Check that it's not ready yet
+    expect(await rng.isRandomnessReady()).to.be.false;
+
+    // Advance time by 10 seconds (the lookahead time)
+    await network.provider.send("evm_increaseTime", [10]);
+    await network.provider.send("evm_mine");
+
+    // Now it should be ready
+    expect(await rng.isRandomnessReady()).to.be.true;
+
+    // Get the random number
+    const randomNumber = await rng.receiveRandomness.staticCall();
+    expect(randomNumber).to.not.equal(0);
+  });
+
+  it("Should return 0 if randomness not requested", async () => {
+    const randomNumber = await rng.receiveRandomness.staticCall();
+    expect(randomNumber).to.equal(0);
+  });
+
+  it("Should return 0 if not enough time has passed", async () => {
+    await rng.requestRandomness();
+
+    // Don't advance time enough
+    await network.provider.send("evm_increaseTime", [5]); // Only 5 seconds
+    await network.provider.send("evm_mine");
+
+    const randomNumber = await rng.receiveRandomness.staticCall();
+    expect(randomNumber).to.equal(0);
   });
 });
 
