[entropy] Entropy gas benchmarks and optimizations (#1153)

* add gas benchmark

* fix benchmark

* fix benchmark

* fix benchmark

* optimization 1: remove provider

* move to u128 for fees

* update benchmark

* comment

* reduce commitment storage

* optimize storage more

* fix fee fields in state

* hmm

* ok

* fix

* cleanup

* this got out of hand

* test overflow conditions

* fix bad merge

* doc

Author: jayantk

target_chains/ethereum/contracts/README.md

@@ -89,7 +89,7 @@ A gas report should have a couple of tables like this:
 
 For most of the methods, the minimum gas usage is an indication of our desired gas usage. Because the calls that store something in the storage
 for the first time in `setUp` use significantly more gas. For example, in the above table, there are two calls to `updatePriceFeeds`. The first
-call has happend in the `setUp` method and costed over a million gas and is not intended for our Benchmark. So our desired value is the
+call has happened in the `setUp` method and costed over a million gas and is not intended for our Benchmark. So our desired value is the
 minimum value which is around 380k gas.
 
 If you like to optimize the contract and measure the gas optimization you can get gas snapshots using `forge snapshot` and evaluate your

target_chains/ethereum/contracts/contracts/entropy/Entropy.sol

@@ -6,6 +6,7 @@ import "@pythnetwork/entropy-sdk-solidity/EntropyStructs.sol";
 import "@pythnetwork/entropy-sdk-solidity/EntropyErrors.sol";
 import "@pythnetwork/entropy-sdk-solidity/EntropyEvents.sol";
 import "@pythnetwork/entropy-sdk-solidity/IEntropy.sol";
+import "@openzeppelin/contracts/utils/math/SafeCast.sol";
 import "./EntropyState.sol";
 
 // Entropy implements a secure 2-party random number generation procedure. The protocol
@@ -74,10 +75,26 @@ import "./EntropyState.sol";
 // cases where the user chooses not to reveal.
 contract Entropy is IEntropy, EntropyState {
     // TODO: Use an upgradeable proxy
-    constructor(uint pythFeeInWei, address defaultProvider) {
+    constructor(
+        uint128 pythFeeInWei,
+        address defaultProvider,
+        bool prefillRequestStorage
+    ) {
         _state.accruedPythFeesInWei = 0;
         _state.pythFeeInWei = pythFeeInWei;
         _state.defaultProvider = defaultProvider;
+
+        if (prefillRequestStorage) {
+            // Write some data to every storage slot in the requests array such that new requests
+            // use a more consistent amount of gas.
+            // Note that these requests are not live because their sequenceNumber is 0.
+            for (uint8 i = 0; i < NUM_REQUESTS; i++) {
+                EntropyStructs.Request storage req = _state.requests[i];
+                req.provider = address(1);
+                req.blockNumber = 1234;
+                req.commitment = hex"0123";
+            }
+        }
     }
 
     // Register msg.sender as a randomness provider. The arguments are the provider's configuration parameters
@@ -86,7 +103,7 @@ contract Entropy is IEntropy, EntropyState {
     //
     // chainLength is the number of values in the hash chain *including* the commitment, that is, chainLength >= 1.
     function register(
-        uint feeInWei,
+        uint128 feeInWei,
         bytes32 commitment,
         bytes calldata commitmentMetadata,
         uint64 chainLength,
@@ -121,7 +138,7 @@ contract Entropy is IEntropy, EntropyState {
     // Withdraw a portion of the accumulated fees for the provider msg.sender.
     // Calling this function will transfer `amount` wei to the caller (provided that they have accrued a sufficient
     // balance of fees in the contract).
-    function withdraw(uint256 amount) public override {
+    function withdraw(uint128 amount) public override {
         EntropyStructs.ProviderInfo storage providerInfo = _state.providers[
             msg.sender
         ];
@@ -166,24 +183,33 @@ contract Entropy is IEntropy, EntropyState {
         providerInfo.sequenceNumber += 1;
 
         // Check that fees were paid and increment the pyth / provider balances.
-        uint requiredFee = getFee(provider);
+        uint128 requiredFee = getFee(provider);
         if (msg.value < requiredFee) revert EntropyErrors.InsufficientFee();
         providerInfo.accruedFeesInWei += providerInfo.feeInWei;
-        _state.accruedPythFeesInWei += (msg.value - providerInfo.feeInWei);
+        _state.accruedPythFeesInWei += (SafeCast.toUint128(msg.value) -
+            providerInfo.feeInWei);
 
         // Store the user's commitment so that we can fulfill the request later.
-        EntropyStructs.Request storage req = _state.requests[
-            requestKey(provider, assignedSequenceNumber)
-        ];
+        // Warning: this code needs to overwrite *every* field in the request, because the returned request can be
+        // filled with arbitrary data.
+        EntropyStructs.Request storage req = allocRequest(
+            provider,
+            assignedSequenceNumber
+        );
         req.provider = provider;
         req.sequenceNumber = assignedSequenceNumber;
-        req.userCommitment = userCommitment;
-        req.providerCommitment = providerInfo.currentCommitment;
-        req.providerCommitmentSequenceNumber = providerInfo
-            .currentCommitmentSequenceNumber;
+        req.numHashes = SafeCast.toUint32(
+            assignedSequenceNumber -
+                providerInfo.currentCommitmentSequenceNumber
+        );
+        req.commitment = keccak256(
+            bytes.concat(userCommitment, providerInfo.currentCommitment)
+        );
 
         if (useBlockHash) {
-            req.blockNumber = block.number;
+            req.blockNumber = SafeCast.toUint96(block.number);
+        } else {
+            req.blockNumber = 0;
         }
 
         emit Requested(req);
@@ -202,24 +228,24 @@ contract Entropy is IEntropy, EntropyState {
         bytes32 userRandomness,
         bytes32 providerRevelation
     ) public override returns (bytes32 randomNumber) {
-        // TODO: do we need to check that this request exists?
         // TODO: this method may need to be authenticated to prevent griefing
-        bytes32 key = requestKey(provider, sequenceNumber);
-        EntropyStructs.Request storage req = _state.requests[key];
-        // This invariant should be guaranteed to hold by the key construction procedure above, but check it
-        // explicitly to be extra cautious.
-        if (req.sequenceNumber != sequenceNumber)
-            revert EntropyErrors.AssertionFailure();
-
-        bool valid = isProofValid(
-            req.providerCommitmentSequenceNumber,
-            req.providerCommitment,
-            sequenceNumber,
+        EntropyStructs.Request storage req = findRequest(
+            provider,
+            sequenceNumber
+        );
+        // Check that there is a request for the given provider / sequence number.
+        if (req.provider != provider || req.sequenceNumber != sequenceNumber)
+            revert EntropyErrors.NoSuchRequest();
+
+        bytes32 providerCommitment = constructProviderCommitment(
+            req.numHashes,
             providerRevelation
         );
-        if (!valid) revert EntropyErrors.IncorrectProviderRevelation();
-        if (constructUserCommitment(userRandomness) != req.userCommitment)
-            revert EntropyErrors.IncorrectUserRevelation();
+        bytes32 userCommitment = constructUserCommitment(userRandomness);
+        if (
+            keccak256(bytes.concat(userCommitment, providerCommitment)) !=
+            req.commitment
+        ) revert EntropyErrors.IncorrectRevelation();
 
         bytes32 blockHash = bytes32(uint256(0));
         if (req.blockNumber != 0) {
@@ -240,7 +266,7 @@ contract Entropy is IEntropy, EntropyState {
             randomNumber
         );
 
-        delete _state.requests[key];
+        clearRequest(provider, sequenceNumber);
 
         EntropyStructs.ProviderInfo storage providerInfo = _state.providers[
             provider
@@ -270,21 +296,20 @@ contract Entropy is IEntropy, EntropyState {
         address provider,
         uint64 sequenceNumber
     ) public view override returns (EntropyStructs.Request memory req) {
-        bytes32 key = requestKey(provider, sequenceNumber);
-        req = _state.requests[key];
+        req = findRequest(provider, sequenceNumber);
     }
 
     function getFee(
         address provider
-    ) public view override returns (uint feeAmount) {
+    ) public view override returns (uint128 feeAmount) {
         return _state.providers[provider].feeInWei + _state.pythFeeInWei;
     }
 
     function getAccruedPythFees()
         public
         view
         override
-        returns (uint accruedPythFeesInWei)
+        returns (uint128 accruedPythFeesInWei)
     {
         return _state.accruedPythFeesInWei;
     }
@@ -305,31 +330,90 @@ contract Entropy is IEntropy, EntropyState {
         );
     }
 
-    // Create a unique key for an in-flight randomness request (to store it in the contract state)
+    // Create a unique key for an in-flight randomness request. Returns both a long key for use in the requestsOverflow
+    // mapping and a short key for use in the requests array.
     function requestKey(
         address provider,
         uint64 sequenceNumber
-    ) internal pure returns (bytes32 hash) {
+    ) internal pure returns (bytes32 hash, uint8 shortHash) {
         hash = keccak256(abi.encodePacked(provider, sequenceNumber));
+        shortHash = uint8(hash[0] & NUM_REQUESTS_MASK);
     }
 
-    // Validate that revelation at sequenceNumber is the correct value in the hash chain for a provider whose
-    // last known revealed random number was lastRevelation at lastSequenceNumber.
-    function isProofValid(
-        uint64 lastSequenceNumber,
-        bytes32 lastRevelation,
-        uint64 sequenceNumber,
+    // Construct a provider's commitment given their revealed random number and the distance in the hash chain
+    // between the commitment and the revealed random number.
+    function constructProviderCommitment(
+        uint64 numHashes,
         bytes32 revelation
-    ) internal pure returns (bool valid) {
-        if (sequenceNumber <= lastSequenceNumber)
-            revert EntropyErrors.AssertionFailure();
-
-        bytes32 currentHash = revelation;
-        while (sequenceNumber > lastSequenceNumber) {
+    ) internal pure returns (bytes32 currentHash) {
+        currentHash = revelation;
+        while (numHashes > 0) {
             currentHash = keccak256(bytes.concat(currentHash));
-            sequenceNumber -= 1;
+            numHashes -= 1;
         }
+    }
+
+    // Find an in-flight request.
+    // Note that this method can return requests that are not currently active. The caller is responsible for checking
+    // that the returned request is active (if they care).
+    function findRequest(
+        address provider,
+        uint64 sequenceNumber
+    ) internal view returns (EntropyStructs.Request storage req) {
+        (bytes32 key, uint8 shortKey) = requestKey(provider, sequenceNumber);
+
+        req = _state.requests[shortKey];
+        if (req.provider == provider && req.sequenceNumber == sequenceNumber) {
+            return req;
+        } else {
+            req = _state.requestsOverflow[key];
+        }
+    }
+
+    // Clear the storage for an in-flight request, deleting it from the hash table.
+    function clearRequest(address provider, uint64 sequenceNumber) internal {
+        (bytes32 key, uint8 shortKey) = requestKey(provider, sequenceNumber);
+
+        EntropyStructs.Request storage req = _state.requests[shortKey];
+        if (req.provider == provider && req.sequenceNumber == sequenceNumber) {
+            req.sequenceNumber = 0;
+        } else {
+            delete _state.requestsOverflow[key];
+        }
+    }
+
+    // Allocate storage space for a new in-flight request. This method returns a pointer to a storage slot
+    // that the caller should overwrite with the new request. Note that the memory at this storage slot may
+    // -- and will -- be filled with arbitrary values, so the caller *must* overwrite every field of the returned
+    // struct.
+    function allocRequest(
+        address provider,
+        uint64 sequenceNumber
+    ) internal returns (EntropyStructs.Request storage req) {
+        (, uint8 shortKey) = requestKey(provider, sequenceNumber);
+
+        req = _state.requests[shortKey];
+        if (isActive(req)) {
+            // There's already a prior active request in the storage slot we want to use.
+            // Overflow the prior request to the requestsOverflow mapping.
+            // It is important that this code overflows the *prior* request to the mapping, and not the new request.
+            // There is a chance that some requests never get revealed and remain active forever. We do not want such
+            // requests to fill up all of the space in the array and cause all new requests to incur the higher gas cost
+            // of the mapping.
+            //
+            // This operation is expensive, but should be rare. If overflow happens frequently, increase
+            // the size of the requests array to support more concurrent active requests.
+            (bytes32 reqKey, ) = requestKey(req.provider, req.sequenceNumber);
+            _state.requestsOverflow[reqKey] = req;
+        }
+    }
 
-        valid = currentHash == lastRevelation;
+    // Returns true if a request is active, i.e., its corresponding random value has not yet been revealed.
+    function isActive(
+        EntropyStructs.Request storage req
+    ) internal view returns (bool) {
+        // Note that a provider's initial registration occupies sequence number 0, so there is no way to construct
+        // a randomness request with sequence number 0.
+        return req.sequenceNumber != 0;
     }
 }

target_chains/ethereum/contracts/contracts/entropy/EntropyState.sol

@@ -6,14 +6,39 @@ import "@pythnetwork/entropy-sdk-solidity/EntropyStructs.sol";
 
 contract EntropyInternalStructs {
     struct State {
-        uint pythFeeInWei;
-        uint accruedPythFeesInWei;
+        // Fee charged by the pyth protocol in wei.
+        uint128 pythFeeInWei;
+        // Total quantity of fees (in wei) earned by the pyth protocol that are currently stored in the contract.
+        // This quantity is incremented when fees are paid and decremented when fees are withdrawn.
+        // Note that u128 can store up to ~10^36 wei, which is ~10^18 in native base tokens, which should be plenty.
+        uint128 accruedPythFeesInWei;
+        // The protocol sets a provider as default to simplify integration for developers.
         address defaultProvider;
+        // Hash table for storing in-flight requests. Table keys are hash(provider, sequenceNumber), and the value is
+        // the current request (if one is currently in-flight).
+        //
+        // Due to the vagaries of EVM opcode costs, it is inefficient to simply use a mapping here. Overwriting zero-valued
+        // storage slots with non-zero values is expensive in EVM (21k gas). Using a mapping, each new request starts
+        // from all-zero values, and thus incurs a substantial write cost. Deleting non-zero values does refund gas, but
+        // unfortunately the refund is not substantial enough to matter.
+        //
+        // This data structure is a two-level hash table. It first tries to store new requests in the requests array at
+        // an index determined by a few bits of the request's key. If that slot in the array is already occupied by a
+        // prior request, the prior request is evicted into the requestsOverflow mapping. Requests in the array are
+        // considered active if their sequenceNumber is > 0.
+        //
+        // WARNING: the number of requests must be kept in sync with the constants below
+        EntropyStructs.Request[32] requests;
+        mapping(bytes32 => EntropyStructs.Request) requestsOverflow;
+        // Mapping from randomness providers to information about each them.
         mapping(address => EntropyStructs.ProviderInfo) providers;
-        mapping(bytes32 => EntropyStructs.Request) requests;
     }
 }
 
 contract EntropyState {
+    // The size of the requests hash table. Must be a power of 2.
+    uint8 public constant NUM_REQUESTS = 32;
+    bytes1 public constant NUM_REQUESTS_MASK = 0x1f;
+
     EntropyInternalStructs.State _state;
 }