From 52e680ac2b59f343c4a7b75072fdfac506422e3c Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Fri, 28 Mar 2025 10:17:48 -0700 Subject: [PATCH 1/7] add configurable gas limits and failure on calls --- .../contracts/contracts/entropy/Entropy.sol | 84 +++++++++++-- target_chains/ethereum/contracts/package.json | 2 +- .../entropy_sdk/solidity/EntropyEvents.sol | 13 ++ .../entropy_sdk/solidity/EntropyStructs.sol | 10 +- .../solidity/abis/EntropyEvents.json | 101 +++++++++++++++ .../entropy_sdk/solidity/abis/IEntropy.json | 116 ++++++++++++++++++ 6 files changed, 312 insertions(+), 14 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index ede2e39976..2187997d6c 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -470,6 +470,15 @@ abstract contract Entropy is IEntropy, EntropyState { if (!req.isRequestWithCallback) { revert EntropyErrors.InvalidRevealCall(); } + // Invariant check: all callback requests should have useBlockhash set to false. + if (req.useBlockhash) { + revert EntropyErrors.InvalidRevealCall(); + } + + if (req.reentryGuard) { + revert EntropyErrors.InvalidRevealCall(); + } + bytes32 blockHash; bytes32 randomNumber; (randomNumber, blockHash) = revealHelper( @@ -480,27 +489,60 @@ abstract contract Entropy is IEntropy, EntropyState { address callAddress = req.requester; - emit RevealedWithCallback( - req, - userRandomNumber, - providerRevelation, - randomNumber - ); - - clearRequest(provider, sequenceNumber); - // Check if the callAddress is a contract account. + // TODO: this can probably be deleted. uint len; assembly { len := extcodesize(callAddress) } + + bool success; + bytes memory ret; if (len != 0) { - IEntropyConsumer(callAddress)._entropyCallback( - sequenceNumber, - provider, + uint64 gas; + if (req.blockNumber != 0) { + gas = req.blockNumber; + } else { + gas = gasLeft(); + } + + req.reentryGuard = true; + (success, ret) == callAddress.excessivelySafeCall( + gas, + 32, + abi.encodeWithSelector( + IEntropyConsumer._entropyCallback.selector, + sequenceNumber, + provider, + randomNumber + ) + ); + req.reentryGuard = false; + } + + if (success) { + emit RevealedWithCallback( + req, + userRandomNumber, + providerRevelation, randomNumber ); + clearRequest(provider, sequenceNumber); + } else { + bytes32 errorReason; + assembly { + errorReason := mload(add(ret, 32)); + } + + emit CallbackFailed( + provider, + req.requester, + sequenceNumber, + errorReason + ) + req.callbackAttempted = true; } + } function getProviderInfo( @@ -625,6 +667,24 @@ abstract contract Entropy is IEntropy, EntropyState { ); } + // Set the default gas limit for a request. + function setDefaultGasLimit(uint64 gasLimit) external override { + EntropyStructs.ProviderInfo storage provider = _state.providers[ + msg.sender + ]; + if (provider.sequenceNumber == 0) { + revert EntropyErrors.NoSuchProvider(); + } + + uint64 oldGasLimit = provider.defaultGasLimit; + provider.defaultGasLimit = gasLimit; + emit ProviderDefaultGasLimitUpdated( + msg.sender, + oldGasLimit, + gasLimit + ); + } + function constructUserCommitment( bytes32 userRandomness ) public pure override returns (bytes32 userCommitment) { diff --git a/target_chains/ethereum/contracts/package.json b/target_chains/ethereum/contracts/package.json index 14243cbfc0..296bd002bc 100644 --- a/target_chains/ethereum/contracts/package.json +++ b/target_chains/ethereum/contracts/package.json @@ -23,7 +23,7 @@ "migrate": "truffle migrate", "receiver-submit-guardian-sets": "truffle exec scripts/receiverSubmitGuardianSetUpgrades.js", "verify": "truffle run verify $npm_config_module@$npm_config_contract_address --network $npm_config_network", - "install-forge-deps": "forge install foundry-rs/forge-std@v1.7.6 --no-git --no-commit", + "install-forge-deps": "forge install foundry-rs/forge-std@v1.7.6 nomad-xyz/ExcessivelySafeCall@81cd99c --no-git --no-commit", "coverage": "./coverage.sh", "test:format": "prettier --check .", "fix:format": "prettier --write ." diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol index 59b595fb13..3090dcfb9f 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol @@ -28,6 +28,13 @@ interface EntropyEvents { bytes32 providerRevelation, bytes32 randomNumber ); + // TODO: indexing, other fields?? + event CallbackFailed( + address indexed provider, + address indexed requestor, + uint64 indexed sequenceNumber, + bytes32 errorCode + ); event ProviderFeeUpdated(address provider, uint128 oldFee, uint128 newFee); @@ -44,6 +51,12 @@ interface EntropyEvents { uint32 newMaxNumHashes ); + event ProviderDefaultGasLimitUpdated( + address provider, + uint64 oldDefaultGasLimit, + uint64 newDefaultGasLimit + ); + event Withdrawal( address provider, address recipient, diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol index 15c1d1511a..5a5f8f5940 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol @@ -37,6 +37,8 @@ contract EntropyStructs { // Maximum number of hashes to record in a request. This should be set according to the maximum gas limit // the provider supports for callbacks. uint32 maxNumHashes; + // Default gas limit to use for callbacks. + uint64 defaultGasLimit; } struct Request { @@ -54,6 +56,9 @@ contract EntropyStructs { // Note that we're using a uint64 such that we have an additional space for an address and other fields in // this storage slot. Although block.number returns a uint256, 64 bits should be plenty to index all of the // blocks ever generated. + // + // Note: We are overloading this storage slot to also store a gas limit for callbacks, as we do not support + // blockhashes in the callback case. uint64 blockNumber; // The address that requested this random number. address requester; @@ -61,6 +66,9 @@ contract EntropyStructs { bool useBlockhash; // If true, the requester will be called back with the generated random value. bool isRequestWithCallback; - // There are 2 remaining bytes of free space in this slot. + // If true, the callback has been attempted by the provider (and failed for some reason). + bool callbackAttempted; + // If true, a fulfillment request for this is already in-flight + bool reentryGuard; } } diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json index b34dffcf59..d0de247eaa 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json @@ -1,4 +1,60 @@ [ + { + "anonymous": false, + "inputs": [ + { + "indexed": true, + "internalType": "address", + "name": "provider", + "type": "address" + }, + { + "indexed": true, + "internalType": "address", + "name": "requestor", + "type": "address" + }, + { + "indexed": true, + "internalType": "uint64", + "name": "sequenceNumber", + "type": "uint64" + }, + { + "indexed": false, + "internalType": "bytes32", + "name": "errorCode", + "type": "bytes32" + } + ], + "name": "CallbackFailed", + "type": "event" + }, + { + "anonymous": false, + "inputs": [ + { + "indexed": false, + "internalType": "address", + "name": "provider", + "type": "address" + }, + { + "indexed": false, + "internalType": "uint64", + "name": "oldDefaultGasLimit", + "type": "uint64" + }, + { + "indexed": false, + "internalType": "uint64", + "name": "newDefaultGasLimit", + "type": "uint64" + } + ], + "name": "ProviderDefaultGasLimitUpdated", + "type": "event" + }, { "anonymous": false, "inputs": [ @@ -163,6 +219,11 @@ "internalType": "uint32", "name": "maxNumHashes", "type": "uint32" + }, + { + "internalType": "uint64", + "name": "defaultGasLimit", + "type": "uint64" } ], "indexed": false, @@ -218,6 +279,16 @@ "internalType": "bool", "name": "isRequestWithCallback", "type": "bool" + }, + { + "internalType": "bool", + "name": "callbackAttempted", + "type": "bool" + }, + { + "internalType": "bool", + "name": "reentryGuard", + "type": "bool" } ], "indexed": false, @@ -297,6 +368,16 @@ "internalType": "bool", "name": "isRequestWithCallback", "type": "bool" + }, + { + "internalType": "bool", + "name": "callbackAttempted", + "type": "bool" + }, + { + "internalType": "bool", + "name": "reentryGuard", + "type": "bool" } ], "indexed": false, @@ -352,6 +433,16 @@ "internalType": "bool", "name": "isRequestWithCallback", "type": "bool" + }, + { + "internalType": "bool", + "name": "callbackAttempted", + "type": "bool" + }, + { + "internalType": "bool", + "name": "reentryGuard", + "type": "bool" } ], "indexed": false, @@ -431,6 +522,16 @@ "internalType": "bool", "name": "isRequestWithCallback", "type": "bool" + }, + { + "internalType": "bool", + "name": "callbackAttempted", + "type": "bool" + }, + { + "internalType": "bool", + "name": "reentryGuard", + "type": "bool" } ], "indexed": false, diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json b/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json index 61a4a6be2e..8ee6319651 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json @@ -1,4 +1,60 @@ [ + { + "anonymous": false, + "inputs": [ + { + "indexed": true, + "internalType": "address", + "name": "provider", + "type": "address" + }, + { + "indexed": true, + "internalType": "address", + "name": "requestor", + "type": "address" + }, + { + "indexed": true, + "internalType": "uint64", + "name": "sequenceNumber", + "type": "uint64" + }, + { + "indexed": false, + "internalType": "bytes32", + "name": "errorCode", + "type": "bytes32" + } + ], + "name": "CallbackFailed", + "type": "event" + }, + { + "anonymous": false, + "inputs": [ + { + "indexed": false, + "internalType": "address", + "name": "provider", + "type": "address" + }, + { + "indexed": false, + "internalType": "uint64", + "name": "oldDefaultGasLimit", + "type": "uint64" + }, + { + "indexed": false, + "internalType": "uint64", + "name": "newDefaultGasLimit", + "type": "uint64" + } + ], + "name": "ProviderDefaultGasLimitUpdated", + "type": "event" + }, { "anonymous": false, "inputs": [ @@ -163,6 +219,11 @@ "internalType": "uint32", "name": "maxNumHashes", "type": "uint32" + }, + { + "internalType": "uint64", + "name": "defaultGasLimit", + "type": "uint64" } ], "indexed": false, @@ -218,6 +279,16 @@ "internalType": "bool", "name": "isRequestWithCallback", "type": "bool" + }, + { + "internalType": "bool", + "name": "callbackAttempted", + "type": "bool" + }, + { + "internalType": "bool", + "name": "reentryGuard", + "type": "bool" } ], "indexed": false, @@ -297,6 +368,16 @@ "internalType": "bool", "name": "isRequestWithCallback", "type": "bool" + }, + { + "internalType": "bool", + "name": "callbackAttempted", + "type": "bool" + }, + { + "internalType": "bool", + "name": "reentryGuard", + "type": "bool" } ], "indexed": false, @@ -352,6 +433,16 @@ "internalType": "bool", "name": "isRequestWithCallback", "type": "bool" + }, + { + "internalType": "bool", + "name": "callbackAttempted", + "type": "bool" + }, + { + "internalType": "bool", + "name": "reentryGuard", + "type": "bool" } ], "indexed": false, @@ -431,6 +522,16 @@ "internalType": "bool", "name": "isRequestWithCallback", "type": "bool" + }, + { + "internalType": "bool", + "name": "callbackAttempted", + "type": "bool" + }, + { + "internalType": "bool", + "name": "reentryGuard", + "type": "bool" } ], "indexed": false, @@ -672,6 +773,11 @@ "internalType": "uint32", "name": "maxNumHashes", "type": "uint32" + }, + { + "internalType": "uint64", + "name": "defaultGasLimit", + "type": "uint64" } ], "internalType": "struct EntropyStructs.ProviderInfo", @@ -738,6 +844,16 @@ "internalType": "bool", "name": "isRequestWithCallback", "type": "bool" + }, + { + "internalType": "bool", + "name": "callbackAttempted", + "type": "bool" + }, + { + "internalType": "bool", + "name": "reentryGuard", + "type": "bool" } ], "internalType": "struct EntropyStructs.Request", From 330b8ee58bdf7184404b350c3b0c51b52a06241a Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Tue, 1 Apr 2025 08:34:10 -0700 Subject: [PATCH 2/7] fix this --- .../contracts/contracts/entropy/Entropy.sol | 167 +++++++++++++----- .../contracts/forge-test/Entropy.t.sol | 6 +- .../ethereum/contracts/remappings.txt | 1 + .../entropy_sdk/solidity/IEntropy.sol | 14 ++ .../entropy_sdk/solidity/abis/IEntropy.json | 66 +++++++ 5 files changed, 210 insertions(+), 44 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index 2187997d6c..1db343a652 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -8,6 +8,7 @@ import "@pythnetwork/entropy-sdk-solidity/EntropyEvents.sol"; import "@pythnetwork/entropy-sdk-solidity/IEntropy.sol"; import "@pythnetwork/entropy-sdk-solidity/IEntropyConsumer.sol"; import "@openzeppelin/contracts/utils/math/SafeCast.sol"; +import "ExcessivelySafeCall/ExcessivelySafeCall.sol"; import "./EntropyState.sol"; // Entropy implements a secure 2-party random number generation procedure. The protocol @@ -76,6 +77,8 @@ import "./EntropyState.sol"; // the user is always incentivized to reveal their random number, and that the protocol has an escape hatch for // cases where the user chooses not to reveal. abstract contract Entropy is IEntropy, EntropyState { + using ExcessivelySafeCall for address; + function _initialize( address admin, uint128 pythFeeInWei, @@ -203,7 +206,8 @@ abstract contract Entropy is IEntropy, EntropyState { address provider, bytes32 userCommitment, bool useBlockhash, - bool isRequestWithCallback + bool isRequestWithCallback, + uint64 callbackGasLimit ) internal returns (EntropyStructs.Request storage req) { EntropyStructs.ProviderInfo storage providerInfo = _state.providers[ provider @@ -220,7 +224,10 @@ abstract contract Entropy is IEntropy, EntropyState { // Check that fees were paid and increment the pyth / provider balances. uint128 requiredFee = getFee(provider); if (msg.value < requiredFee) revert EntropyErrors.InsufficientFee(); - providerInfo.accruedFeesInWei += providerInfo.feeInWei; + providerInfo.accruedFeesInWei += getProviderFee( + provider, + callbackGasLimit + ); _state.accruedPythFeesInWei += (SafeCast.toUint128(msg.value) - providerInfo.feeInWei); @@ -245,9 +252,21 @@ abstract contract Entropy is IEntropy, EntropyState { ); req.requester = msg.sender; - req.blockNumber = SafeCast.toUint64(block.number); - req.useBlockhash = useBlockhash; - req.isRequestWithCallback = isRequestWithCallback; + if (useBlockhash && isRequestWithCallback) { + revert EntropyErrors.AssertionFailure(); + } else if (isRequestWithCallback) { + req.isRequestWithCallback = isRequestWithCallback; + if (callbackGasLimit == 0) { + req.blockNumber = providerInfo.defaultGasLimit; + } else { + req.blockNumber = callbackGasLimit; + } + req.useBlockhash = false; + } else { + req.isRequestWithCallback = false; + req.blockNumber = SafeCast.toUint64(block.number); + req.useBlockhash = useBlockhash; + } } // As a user, request a random number from `provider`. Prior to calling this method, the user should @@ -269,7 +288,8 @@ abstract contract Entropy is IEntropy, EntropyState { provider, userCommitment, useBlockHash, - false + false, + 0 ); assignedSequenceNumber = req.sequenceNumber; emit Requested(req); @@ -294,7 +314,35 @@ abstract contract Entropy is IEntropy, EntropyState { // If we remove the blockHash from this, the provider would have no choice but to provide its committed // random number. Hence, useBlockHash is set to false. false, - true + true, + 0 + ); + + emit RequestedWithCallback( + provider, + req.requester, + req.sequenceNumber, + userRandomNumber, + req + ); + + return req.sequenceNumber; + } + + function requestWithCallbackAndGas( + address provider, + bytes32 userRandomNumber, + uint64 gasLimit + ) public payable override returns (uint64) { + EntropyStructs.Request storage req = requestHelper( + provider, + constructUserCommitment(userRandomNumber), + // If useBlockHash is set to true, it allows a scenario in which the provider and miner can collude. + // If we remove the blockHash from this, the provider would have no choice but to provide its committed + // random number. Hence, useBlockHash is set to false. + false, + true, + gasLimit ); emit RequestedWithCallback( @@ -489,26 +537,21 @@ abstract contract Entropy is IEntropy, EntropyState { address callAddress = req.requester; - // Check if the callAddress is a contract account. - // TODO: this can probably be deleted. - uint len; - assembly { - len := extcodesize(callAddress) - } + if (req.blockNumber != 0 && !req.callbackAttempted) { + // TODO: need to validate that we have enough gas left to forward (?) + // Or at least that we forwarded enough gas before marking the callback as failed + /* + if (gasleft() < req.blockNumber) { - bool success; - bytes memory ret; - if (len != 0) { - uint64 gas; - if (req.blockNumber != 0) { - gas = req.blockNumber; - } else { - gas = gasLeft(); } + */ req.reentryGuard = true; - (success, ret) == callAddress.excessivelySafeCall( - gas, + bool success; + bytes memory ret; + (success, ret) = callAddress.excessivelySafeCall( + req.blockNumber, + 0, 32, abi.encodeWithSelector( IEntropyConsumer._entropyCallback.selector, @@ -518,9 +561,30 @@ abstract contract Entropy is IEntropy, EntropyState { ) ); req.reentryGuard = false; - } - if (success) { + if (success) { + emit RevealedWithCallback( + req, + userRandomNumber, + providerRevelation, + randomNumber + ); + clearRequest(provider, sequenceNumber); + } else { + bytes32 errorReason; + assembly { + errorReason := mload(add(ret, 32)) + } + + emit CallbackFailed( + provider, + req.requester, + sequenceNumber, + errorReason + ); + req.callbackAttempted = true; + } + } else { emit RevealedWithCallback( req, userRandomNumber, @@ -528,21 +592,21 @@ abstract contract Entropy is IEntropy, EntropyState { randomNumber ); clearRequest(provider, sequenceNumber); - } else { - bytes32 errorReason; + + // Check if the callAddress is a contract account. + uint len; assembly { - errorReason := mload(add(ret, 32)); + len := extcodesize(callAddress) } - emit CallbackFailed( - provider, - req.requester, - sequenceNumber, - errorReason - ) - req.callbackAttempted = true; + if (len != 0) { + IEntropyConsumer(callAddress)._entropyCallback( + sequenceNumber, + provider, + randomNumber + ); + } } - } function getProviderInfo( @@ -570,7 +634,30 @@ abstract contract Entropy is IEntropy, EntropyState { function getFee( address provider ) public view override returns (uint128 feeAmount) { - return _state.providers[provider].feeInWei + _state.pythFeeInWei; + return getFeeForGas(provider, 0); + } + + function getFeeForGas( + address provider, + uint64 gasLimit + ) public view override returns (uint128 feeAmount) { + return getProviderFee(provider, gasLimit) + _state.pythFeeInWei; + } + + function getProviderFee( + address providerAddr, + uint64 gasLimit + ) internal view returns (uint128 feeAmount) { + EntropyStructs.ProviderInfo memory provider = _state.providers[ + providerAddr + ]; + if (gasLimit > provider.defaultGasLimit) { + uint128 additionalFee = ((gasLimit - provider.defaultGasLimit) * + provider.feeInWei) / provider.defaultGasLimit; + return provider.feeInWei + additionalFee; + } else { + return provider.feeInWei; + } } function getPythFee() public view returns (uint128 feeAmount) { @@ -678,11 +765,7 @@ abstract contract Entropy is IEntropy, EntropyState { uint64 oldGasLimit = provider.defaultGasLimit; provider.defaultGasLimit = gasLimit; - emit ProviderDefaultGasLimitUpdated( - msg.sender, - oldGasLimit, - gasLimit - ); + emit ProviderDefaultGasLimitUpdated(msg.sender, oldGasLimit, gasLimit); } function constructUserCommitment( diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index d088bb0c37..a755efb53a 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -801,10 +801,12 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { providerInfo.currentCommitment ) ), - blockNumber: 1234, + blockNumber: 0, requester: user1, useBlockhash: false, - isRequestWithCallback: true + isRequestWithCallback: true, + callbackAttempted: false, + reentryGuard: false }) ); vm.roll(1234); diff --git a/target_chains/ethereum/contracts/remappings.txt b/target_chains/ethereum/contracts/remappings.txt index 4b2c7905c0..a6ccbf917d 100644 --- a/target_chains/ethereum/contracts/remappings.txt +++ b/target_chains/ethereum/contracts/remappings.txt @@ -3,4 +3,5 @@ @pythnetwork/=./node_modules/@pythnetwork/ ds-test/=lib/forge-std/lib/ds-test/src/ forge-std/=lib/forge-std/src/ +ExcessivelySafeCall=lib/ExcessivelySafeCall/src/ truffle/=./node_modules/truffle/ diff --git a/target_chains/ethereum/entropy_sdk/solidity/IEntropy.sol b/target_chains/ethereum/entropy_sdk/solidity/IEntropy.sol index ce446ff44b..2d4dfbbdaf 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/IEntropy.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/IEntropy.sol @@ -56,6 +56,12 @@ interface IEntropy is EntropyEvents { bytes32 userRandomNumber ) external payable returns (uint64 assignedSequenceNumber); + function requestWithCallbackAndGas( + address provider, + bytes32 userRandomNumber, + uint64 gasLimit + ) external payable returns (uint64 assignedSequenceNumber); + // Fulfill a request for a random number. This method validates the provided userRandomness and provider's proof // against the corresponding commitments in the in-flight request. If both values are validated, this function returns // the corresponding random number. @@ -100,6 +106,11 @@ interface IEntropy is EntropyEvents { function getFee(address provider) external view returns (uint128 feeAmount); + function getFeeForGas( + address provider, + uint64 gasLimit + ) external view returns (uint128 feeAmount); + function getAccruedPythFees() external view @@ -124,6 +135,9 @@ interface IEntropy is EntropyEvents { // the provider supports for callbacks. function setMaxNumHashes(uint32 maxNumHashes) external; + // Set the default gas limit for a request. If 0, no + function setDefaultGasLimit(uint64 gasLimit) external; + // Advance the provider commitment and increase the sequence number. // This is used to reduce the `numHashes` required for future requests which leads to reduced gas usage. function advanceProviderCommitment( diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json b/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json index 8ee6319651..b7039abdcb 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json @@ -702,6 +702,30 @@ "stateMutability": "view", "type": "function" }, + { + "inputs": [ + { + "internalType": "address", + "name": "provider", + "type": "address" + }, + { + "internalType": "uint64", + "name": "gasLimit", + "type": "uint64" + } + ], + "name": "getFeeForGas", + "outputs": [ + { + "internalType": "uint128", + "name": "feeAmount", + "type": "uint128" + } + ], + "stateMutability": "view", + "type": "function" + }, { "inputs": [ { @@ -950,6 +974,35 @@ "stateMutability": "payable", "type": "function" }, + { + "inputs": [ + { + "internalType": "address", + "name": "provider", + "type": "address" + }, + { + "internalType": "bytes32", + "name": "userRandomNumber", + "type": "bytes32" + }, + { + "internalType": "uint64", + "name": "gasLimit", + "type": "uint64" + } + ], + "name": "requestWithCallbackAndGas", + "outputs": [ + { + "internalType": "uint64", + "name": "assignedSequenceNumber", + "type": "uint64" + } + ], + "stateMutability": "payable", + "type": "function" + }, { "inputs": [ { @@ -1012,6 +1065,19 @@ "stateMutability": "nonpayable", "type": "function" }, + { + "inputs": [ + { + "internalType": "uint64", + "name": "gasLimit", + "type": "uint64" + } + ], + "name": "setDefaultGasLimit", + "outputs": [], + "stateMutability": "nonpayable", + "type": "function" + }, { "inputs": [ { From 1c27dda6cdda5daa69ad35041d03466c1409d6ce Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Tue, 1 Apr 2025 09:11:22 -0700 Subject: [PATCH 3/7] tests --- .../contracts/contracts/entropy/Entropy.sol | 46 +++--- .../contracts/forge-test/Entropy.t.sol | 142 +++++++++++++++++- .../entropy_sdk/solidity/EntropyStructs.sol | 16 +- .../solidity/abis/EntropyEvents.json | 16 +- .../entropy_sdk/solidity/abis/IEntropy.json | 20 +-- 5 files changed, 191 insertions(+), 49 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index 1db343a652..0aa4171900 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -103,7 +103,7 @@ abstract contract Entropy is IEntropy, EntropyState { for (uint8 i = 0; i < NUM_REQUESTS; i++) { EntropyStructs.Request storage req = _state.requests[i]; req.provider = address(1); - req.blockNumber = 1234; + req.blockNumberOrGasLimit = 1234; req.commitment = hex"0123"; } } @@ -222,14 +222,12 @@ abstract contract Entropy is IEntropy, EntropyState { providerInfo.sequenceNumber += 1; // Check that fees were paid and increment the pyth / provider balances. - uint128 requiredFee = getFee(provider); + uint128 requiredFee = getFeeForGas(provider, callbackGasLimit); if (msg.value < requiredFee) revert EntropyErrors.InsufficientFee(); - providerInfo.accruedFeesInWei += getProviderFee( - provider, - callbackGasLimit - ); + uint128 providerFee = getProviderFee(provider, callbackGasLimit); + providerInfo.accruedFeesInWei += providerFee; _state.accruedPythFeesInWei += (SafeCast.toUint128(msg.value) - - providerInfo.feeInWei); + providerFee); // Store the user's commitment so that we can fulfill the request later. // Warning: this code needs to overwrite *every* field in the request, because the returned request can be @@ -257,16 +255,19 @@ abstract contract Entropy is IEntropy, EntropyState { } else if (isRequestWithCallback) { req.isRequestWithCallback = isRequestWithCallback; if (callbackGasLimit == 0) { - req.blockNumber = providerInfo.defaultGasLimit; + req.blockNumberOrGasLimit = providerInfo.defaultGasLimit; } else { - req.blockNumber = callbackGasLimit; + req.blockNumberOrGasLimit = callbackGasLimit; } req.useBlockhash = false; } else { req.isRequestWithCallback = false; - req.blockNumber = SafeCast.toUint64(block.number); + req.blockNumberOrGasLimit = SafeCast.toUint64(block.number); req.useBlockhash = useBlockhash; } + + req.callbackFailed = false; + req.reentryGuard = false; } // As a user, request a random number from `provider`. Prior to calling this method, the user should @@ -376,9 +377,9 @@ abstract contract Entropy is IEntropy, EntropyState { blockHash = bytes32(uint256(0)); if (req.useBlockhash) { - bytes32 _blockHash = blockhash(req.blockNumber); + bytes32 _blockHash = blockhash(req.blockNumberOrGasLimit); - // The `blockhash` function will return zero if the req.blockNumber is equal to the current + // The `blockhash` function will return zero if the req.blockNumberOrGasLimit is equal to the current // block number, or if it is not within the 256 most recent blocks. This allows the user to // select between two random numbers by executing the reveal function in the same block as the // request, or after 256 blocks. This gives each user two chances to get a favorable result on @@ -451,7 +452,7 @@ abstract contract Entropy is IEntropy, EntropyState { } // Fulfill a request for a random number. This method validates the provided userRandomness and provider's proof - // against the corresponding commitments in the in-flight request. If both values are validated, this function returns + // against the corresponding commitments in the in-flight request. If both values are validated, this method returns // the corresponding random number. // // Note that this function can only be called once per in-flight request. Calling this function deletes the stored @@ -520,7 +521,7 @@ abstract contract Entropy is IEntropy, EntropyState { } // Invariant check: all callback requests should have useBlockhash set to false. if (req.useBlockhash) { - revert EntropyErrors.InvalidRevealCall(); + revert EntropyErrors.AssertionFailure(); } if (req.reentryGuard) { @@ -537,11 +538,16 @@ abstract contract Entropy is IEntropy, EntropyState { address callAddress = req.requester; - if (req.blockNumber != 0 && !req.callbackAttempted) { + // blockNumberOrGasLimit holds the gas limit in the callback case. + // If the gas limit is 0, then the provider hasn't configured their default limit, + // so we default to the prior entropy flow (where there is no failure state). + // Similarly, if the request has already failed, we fall back to the prior flow so that + // recovery attempts can provide more gas / directly see the revert reason. + if (req.blockNumberOrGasLimit != 0 && !req.callbackFailed) { // TODO: need to validate that we have enough gas left to forward (?) // Or at least that we forwarded enough gas before marking the callback as failed /* - if (gasleft() < req.blockNumber) { + if (gasleft() < req.blockNumberOrGasLimit) { } */ @@ -550,7 +556,7 @@ abstract contract Entropy is IEntropy, EntropyState { bool success; bytes memory ret; (success, ret) = callAddress.excessivelySafeCall( - req.blockNumber, + req.blockNumberOrGasLimit, 0, 32, abi.encodeWithSelector( @@ -582,7 +588,7 @@ abstract contract Entropy is IEntropy, EntropyState { sequenceNumber, errorReason ); - req.callbackAttempted = true; + req.callbackFailed = true; } } else { emit RevealedWithCallback( @@ -600,11 +606,13 @@ abstract contract Entropy is IEntropy, EntropyState { } if (len != 0) { + req.reentryGuard = true; IEntropyConsumer(callAddress)._entropyCallback( sequenceNumber, provider, randomNumber ); + req.reentryGuard = false; } } } @@ -652,6 +660,8 @@ abstract contract Entropy is IEntropy, EntropyState { providerAddr ]; if (gasLimit > provider.defaultGasLimit) { + // This calculation rounds down the fee, which means that users can get some gas in the callback for free. + // However, the value of the free gas is < 1 wei, which is insignificant. uint128 additionalFee = ((gasLimit - provider.defaultGasLimit) * provider.feeInWei) / provider.defaultGasLimit; return provider.feeInWei + additionalFee; diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index a755efb53a..b3034c21a4 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -203,7 +203,10 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { function testBasicFlow() public { vm.roll(17); uint64 sequenceNumber = request(user2, provider1, 42, false); - assertEq(random.getRequest(provider1, sequenceNumber).blockNumber, 17); + assertEq( + random.getRequest(provider1, sequenceNumber).blockNumberOrGasLimit, + 17 + ); assertEq( random.getRequest(provider1, sequenceNumber).useBlockhash, false @@ -243,7 +246,10 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { 42, false ); - assertEq(random.getRequest(provider1, sequenceNumber).blockNumber, 20); + assertEq( + random.getRequest(provider1, sequenceNumber).blockNumberOrGasLimit, + 20 + ); assertEq( random.getRequest(provider1, sequenceNumber).useBlockhash, false @@ -405,7 +411,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { uint64 sequenceNumber = request(user2, provider1, 42, true); assertEq( - random.getRequest(provider1, sequenceNumber).blockNumber, + random.getRequest(provider1, sequenceNumber).blockNumberOrGasLimit, 1234 ); assertEq( @@ -801,11 +807,11 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { providerInfo.currentCommitment ) ), - blockNumber: 0, + blockNumberOrGasLimit: 0, requester: user1, useBlockhash: false, isRequestWithCallback: true, - callbackAttempted: false, + callbackFailed: false, reentryGuard: false }) ); @@ -1150,6 +1156,132 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { vm.expectRevert(); random.setProviderFeeAsFeeManager(provider1, 1000); } + + function testSetDefaultGasLimit() public { + uint64 newGasLimit = 100000; + + vm.prank(provider1); + random.setDefaultGasLimit(newGasLimit); + + EntropyStructs.ProviderInfo memory info = random.getProviderInfo( + provider1 + ); + assertEq(info.defaultGasLimit, newGasLimit); + } + + function testSetDefaultGasLimitRevertIfNotFromProvider() public { + vm.expectRevert(EntropyErrors.NoSuchProvider.selector); + random.setDefaultGasLimit(100000); + } + + function testRequestWithCallbackUsesDefaultGasLimit() public { + uint64 defaultGasLimit = 100000; + vm.prank(provider1); + random.setDefaultGasLimit(defaultGasLimit); + + bytes32 userRandomNumber = bytes32(uint(42)); + uint fee = random.getFee(provider1); + + vm.deal(user1, fee); + vm.prank(user1); + uint64 sequenceNumber = random.requestWithCallback{value: fee}( + provider1, + userRandomNumber + ); + + EntropyStructs.Request memory req = random.getRequest( + provider1, + sequenceNumber + ); + assertEq(req.blockNumberOrGasLimit, defaultGasLimit); + } + + function testRequestWithCallbackAndCustomGasLimit() public { + uint64 defaultGasLimit = 100000; + uint64 customGasLimit = 200000; + + vm.prank(provider1); + random.setDefaultGasLimit(defaultGasLimit); + + bytes32 userRandomNumber = bytes32(uint(42)); + uint fee = random.getFeeForGas(provider1, customGasLimit); + + vm.deal(user1, fee); + vm.prank(user1); + uint64 sequenceNumber = random.requestWithCallbackAndGas{value: fee}( + provider1, + userRandomNumber, + customGasLimit + ); + + EntropyStructs.Request memory req = random.getRequest( + provider1, + sequenceNumber + ); + assertEq(req.blockNumberOrGasLimit, customGasLimit); + } + + function testRequestWithCallbackAndGasLimitFeeScaling() public { + uint64 defaultGasLimit = 100000; + uint64 doubleGasLimit = 200000; + + vm.prank(provider1); + random.setDefaultGasLimit(defaultGasLimit); + + uint baseFee = random.getFee(provider1); + assertEq(baseFee, provider1FeeInWei + pythFeeInWei); + + // Fee scales proportionally with gas limit + uint scaledFee = random.getFeeForGas(provider1, doubleGasLimit); + assertEq(scaledFee, 2 * provider1FeeInWei + pythFeeInWei); + } + + function testRequestWithCallbackAndGasLimitInsufficientFee() public { + uint64 defaultGasLimit = 100000; + uint64 doubleGasLimit = 200000; + + vm.prank(provider1); + random.setDefaultGasLimit(defaultGasLimit); + + bytes32 userRandomNumber = bytes32(uint(42)); + uint baseFee = random.getFee(provider1); // This is insufficient for double gas + + vm.deal(user1, baseFee); + vm.prank(user1); + vm.expectRevert(EntropyErrors.InsufficientFee.selector); + random.requestWithCallbackAndGas{value: baseFee}( + provider1, + userRandomNumber, + doubleGasLimit + ); + } + + function testRequestWithCallbackAndGasLimitLowerThanDefault() public { + uint64 defaultGasLimit = 100000; + uint64 lowerGasLimit = 50000; + + vm.prank(provider1); + random.setDefaultGasLimit(defaultGasLimit); + + bytes32 userRandomNumber = bytes32(uint(42)); + uint fee = random.getFeeForGas(provider1, lowerGasLimit); + + vm.deal(user1, fee); + vm.prank(user1); + uint64 sequenceNumber = random.requestWithCallbackAndGas{value: fee}( + provider1, + userRandomNumber, + lowerGasLimit + ); + + EntropyStructs.Request memory req = random.getRequest( + provider1, + sequenceNumber + ); + assertEq(req.blockNumberOrGasLimit, lowerGasLimit); + // Fee should be the same as base fee since we're using less gas than default + assertEq(fee, random.getFee(provider1)); + } } contract EntropyConsumer is IEntropyConsumer { diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol index 5a5f8f5940..8d32664964 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol @@ -52,22 +52,22 @@ contract EntropyStructs { // eliminating 1 store. bytes32 commitment; // Storage slot 3 // - // The number of the block where this request was created. - // Note that we're using a uint64 such that we have an additional space for an address and other fields in + // The number of the block where this request was created OR the gas limit for callbacks. + // (isRequestedWithCallback toggles between these two cases. Block numbers are not required + // in the callback case, as these are used to determine blockhashes, which aren't used.) + // + // Note that we're using a uint64 for the blockNumber such that we have an additional space for an address and other fields in // this storage slot. Although block.number returns a uint256, 64 bits should be plenty to index all of the // blocks ever generated. - // - // Note: We are overloading this storage slot to also store a gas limit for callbacks, as we do not support - // blockhashes in the callback case. - uint64 blockNumber; + uint64 blockNumberOrGasLimit; // The address that requested this random number. address requester; // If true, incorporate the blockhash of blockNumber into the generated random value. bool useBlockhash; // If true, the requester will be called back with the generated random value. bool isRequestWithCallback; - // If true, the callback has been attempted by the provider (and failed for some reason). - bool callbackAttempted; + // If true, the callback has been attempted by the provider and failed for some reason. + bool callbackFailed; // If true, a fulfillment request for this is already in-flight bool reentryGuard; } diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json index d0de247eaa..50d5b5cdec 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json @@ -262,7 +262,7 @@ }, { "internalType": "uint64", - "name": "blockNumber", + "name": "blockNumberOrGasLimit", "type": "uint64" }, { @@ -282,7 +282,7 @@ }, { "internalType": "bool", - "name": "callbackAttempted", + "name": "callbackFailed", "type": "bool" }, { @@ -351,7 +351,7 @@ }, { "internalType": "uint64", - "name": "blockNumber", + "name": "blockNumberOrGasLimit", "type": "uint64" }, { @@ -371,7 +371,7 @@ }, { "internalType": "bool", - "name": "callbackAttempted", + "name": "callbackFailed", "type": "bool" }, { @@ -416,7 +416,7 @@ }, { "internalType": "uint64", - "name": "blockNumber", + "name": "blockNumberOrGasLimit", "type": "uint64" }, { @@ -436,7 +436,7 @@ }, { "internalType": "bool", - "name": "callbackAttempted", + "name": "callbackFailed", "type": "bool" }, { @@ -505,7 +505,7 @@ }, { "internalType": "uint64", - "name": "blockNumber", + "name": "blockNumberOrGasLimit", "type": "uint64" }, { @@ -525,7 +525,7 @@ }, { "internalType": "bool", - "name": "callbackAttempted", + "name": "callbackFailed", "type": "bool" }, { diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json b/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json index b7039abdcb..f2b96d7e02 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json @@ -262,7 +262,7 @@ }, { "internalType": "uint64", - "name": "blockNumber", + "name": "blockNumberOrGasLimit", "type": "uint64" }, { @@ -282,7 +282,7 @@ }, { "internalType": "bool", - "name": "callbackAttempted", + "name": "callbackFailed", "type": "bool" }, { @@ -351,7 +351,7 @@ }, { "internalType": "uint64", - "name": "blockNumber", + "name": "blockNumberOrGasLimit", "type": "uint64" }, { @@ -371,7 +371,7 @@ }, { "internalType": "bool", - "name": "callbackAttempted", + "name": "callbackFailed", "type": "bool" }, { @@ -416,7 +416,7 @@ }, { "internalType": "uint64", - "name": "blockNumber", + "name": "blockNumberOrGasLimit", "type": "uint64" }, { @@ -436,7 +436,7 @@ }, { "internalType": "bool", - "name": "callbackAttempted", + "name": "callbackFailed", "type": "bool" }, { @@ -505,7 +505,7 @@ }, { "internalType": "uint64", - "name": "blockNumber", + "name": "blockNumberOrGasLimit", "type": "uint64" }, { @@ -525,7 +525,7 @@ }, { "internalType": "bool", - "name": "callbackAttempted", + "name": "callbackFailed", "type": "bool" }, { @@ -851,7 +851,7 @@ }, { "internalType": "uint64", - "name": "blockNumber", + "name": "blockNumberOrGasLimit", "type": "uint64" }, { @@ -871,7 +871,7 @@ }, { "internalType": "bool", - "name": "callbackAttempted", + "name": "callbackFailed", "type": "bool" }, { From 95490fbd6ecdc3fdaa68f90a24bc582809da3e03 Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Tue, 1 Apr 2025 09:21:05 -0700 Subject: [PATCH 4/7] tests --- .../contracts/forge-test/Entropy.t.sol | 133 ++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index b3034c21a4..55d604a361 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -946,6 +946,139 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { ); } + function testRequestWithCallbackAndRevealWithCallbackWithGasLimit() public { + uint64 defaultGasLimit = 100000; + vm.prank(provider1); + random.setDefaultGasLimit(defaultGasLimit); + + bytes32 userRandomNumber = bytes32(uint(42)); + uint fee = random.getFee(provider1); + EntropyConsumer consumer = new EntropyConsumer(address(random)); + vm.deal(address(consumer), fee); + vm.prank(user1); + uint64 assignedSequenceNumber = consumer.requestEntropy{value: fee}( + userRandomNumber + ); + EntropyStructs.Request memory req = random.getRequest( + provider1, + assignedSequenceNumber + ); + + // Verify the gas limit was set correctly + assertEq(req.blockNumberOrGasLimit, defaultGasLimit); + + vm.expectEmit(false, false, false, true, address(random)); + emit RevealedWithCallback( + req, + userRandomNumber, + provider1Proofs[assignedSequenceNumber], + random.combineRandomValues( + userRandomNumber, + provider1Proofs[assignedSequenceNumber], + 0 + ) + ); + vm.prank(user1); + random.revealWithCallback( + provider1, + assignedSequenceNumber, + userRandomNumber, + provider1Proofs[assignedSequenceNumber] + ); + + assertEq(consumer.sequence(), assignedSequenceNumber); + assertEq(consumer.provider(), provider1); + assertEq( + consumer.randomness(), + random.combineRandomValues( + userRandomNumber, + provider1Proofs[assignedSequenceNumber], + 0 + ) + ); + + EntropyStructs.Request memory reqAfterReveal = random.getRequest( + provider1, + assignedSequenceNumber + ); + assertEq(reqAfterReveal.sequenceNumber, 0); + } + + function testRequestWithCallbackAndRevealWithCallbackWithGasLimitAndFailure() public { + uint64 defaultGasLimit = 100000; + vm.prank(provider1); + random.setDefaultGasLimit(defaultGasLimit); + + bytes32 userRandomNumber = bytes32(uint(42)); + uint fee = random.getFee(provider1); + EntropyConsumerFails consumer = new EntropyConsumerFails(address(random)); + vm.deal(address(consumer), fee); + vm.prank(user1); + uint64 assignedSequenceNumber = consumer.requestEntropy{value: fee}( + userRandomNumber + ); + EntropyStructs.Request memory req = random.getRequest( + provider1, + assignedSequenceNumber + ); + + // Verify the gas limit was set correctly + assertEq(req.blockNumberOrGasLimit, defaultGasLimit); + + // First attempt should fail and emit CallbackFailed event + vm.expectEmit(false, false, false, true, address(random)); + emit CallbackFailed( + provider1, + address(consumer), + assignedSequenceNumber, + bytes32(bytes("Callback failed")) + ); + vm.prank(user1); + random.revealWithCallback( + provider1, + assignedSequenceNumber, + userRandomNumber, + provider1Proofs[assignedSequenceNumber] + ); + + // Verify request is still active after failure + EntropyStructs.Request memory reqAfterFailure = random.getRequest( + provider1, + assignedSequenceNumber + ); + assertEq(reqAfterFailure.sequenceNumber, assignedSequenceNumber); + assertTrue(reqAfterFailure.callbackFailed); + +/* + // Second attempt should succeed and emit RevealedWithCallback event + vm.expectEmit(false, false, false, true, address(random)); + emit RevealedWithCallback( + req, + userRandomNumber, + provider1Proofs[assignedSequenceNumber], + random.combineRandomValues( + userRandomNumber, + provider1Proofs[assignedSequenceNumber], + 0 + ) + ); + vm.prank(user1); + random.revealWithCallback( + provider1, + assignedSequenceNumber, + userRandomNumber, + provider1Proofs[assignedSequenceNumber] + ); + + // Verify request is cleared after successful reveal + EntropyStructs.Request memory reqAfterReveal = random.getRequest( + provider1, + assignedSequenceNumber + ); + assertEq(reqAfterReveal.sequenceNumber, 0); + */ + } + function testRequestWithCallbackAndRevealWithCallbackFailing() public { bytes32 userRandomNumber = bytes32(uint(42)); uint fee = random.getFee(provider1); From 916353b2736ae7058e88ceda400120071770ce99 Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Tue, 1 Apr 2025 09:36:10 -0700 Subject: [PATCH 5/7] test callback flows --- .../contracts/contracts/entropy/Entropy.sol | 9 ++------- .../ethereum/contracts/forge-test/Entropy.t.sol | 15 ++++++++++++--- .../entropy_sdk/solidity/EntropyEvents.sol | 2 +- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index 0aa4171900..034688b76f 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -558,7 +558,7 @@ abstract contract Entropy is IEntropy, EntropyState { (success, ret) = callAddress.excessivelySafeCall( req.blockNumberOrGasLimit, 0, - 32, + 256, abi.encodeWithSelector( IEntropyConsumer._entropyCallback.selector, sequenceNumber, @@ -577,16 +577,11 @@ abstract contract Entropy is IEntropy, EntropyState { ); clearRequest(provider, sequenceNumber); } else { - bytes32 errorReason; - assembly { - errorReason := mload(add(ret, 32)) - } - emit CallbackFailed( provider, req.requester, sequenceNumber, - errorReason + ret ); req.callbackFailed = true; } diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index 55d604a361..0a43ec8eec 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -954,7 +954,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { bytes32 userRandomNumber = bytes32(uint(42)); uint fee = random.getFee(provider1); EntropyConsumer consumer = new EntropyConsumer(address(random)); - vm.deal(address(consumer), fee); + vm.deal(user1, fee); vm.prank(user1); uint64 assignedSequenceNumber = consumer.requestEntropy{value: fee}( userRandomNumber @@ -1012,7 +1012,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { bytes32 userRandomNumber = bytes32(uint(42)); uint fee = random.getFee(provider1); EntropyConsumerFails consumer = new EntropyConsumerFails(address(random)); - vm.deal(address(consumer), fee); + vm.deal(user1, fee); vm.prank(user1); uint64 assignedSequenceNumber = consumer.requestEntropy{value: fee}( userRandomNumber @@ -1031,7 +1031,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { provider1, address(consumer), assignedSequenceNumber, - bytes32(bytes("Callback failed")) + abi.encodeWithSelector(0x08c379a0, "Callback failed") ); vm.prank(user1); random.revealWithCallback( @@ -1460,6 +1460,15 @@ contract EntropyConsumerFails is IEntropyConsumer { entropy = _entropy; } + function requestEntropy( + bytes32 randomNumber + ) public payable returns (uint64 sequenceNumber) { + address _provider = IEntropy(entropy).getDefaultProvider(); + sequenceNumber = IEntropy(entropy).requestWithCallback{ + value: msg.value + }(_provider, randomNumber); + } + function getEntropy() internal view override returns (address) { return entropy; } diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol index 3090dcfb9f..282c6c88c3 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol @@ -33,7 +33,7 @@ interface EntropyEvents { address indexed provider, address indexed requestor, uint64 indexed sequenceNumber, - bytes32 errorCode + bytes errorCode ); event ProviderFeeUpdated(address provider, uint128 oldFee, uint128 newFee); From 293c8a6d401ab4b767e695cde079e5d748c4581c Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Tue, 1 Apr 2025 10:01:35 -0700 Subject: [PATCH 6/7] revert flag --- .../contracts/forge-test/Entropy.t.sol | 89 +++++++++---------- .../solidity/abis/EntropyEvents.json | 4 +- .../entropy_sdk/solidity/abis/IEntropy.json | 4 +- 3 files changed, 48 insertions(+), 49 deletions(-) diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index 0a43ec8eec..a6e21d86fd 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -843,7 +843,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { function testRequestWithCallbackAndRevealWithCallbackByContract() public { bytes32 userRandomNumber = bytes32(uint(42)); uint fee = random.getFee(provider1); - EntropyConsumer consumer = new EntropyConsumer(address(random)); + EntropyConsumer consumer = new EntropyConsumer(address(random), false); vm.deal(user1, fee); vm.prank(user1); uint64 assignedSequenceNumber = consumer.requestEntropy{value: fee}( @@ -953,7 +953,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { bytes32 userRandomNumber = bytes32(uint(42)); uint fee = random.getFee(provider1); - EntropyConsumer consumer = new EntropyConsumer(address(random)); + EntropyConsumer consumer = new EntropyConsumer(address(random), false); vm.deal(user1, fee); vm.prank(user1); uint64 assignedSequenceNumber = consumer.requestEntropy{value: fee}( @@ -978,7 +978,6 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { 0 ) ); - vm.prank(user1); random.revealWithCallback( provider1, assignedSequenceNumber, @@ -1011,7 +1010,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { bytes32 userRandomNumber = bytes32(uint(42)); uint fee = random.getFee(provider1); - EntropyConsumerFails consumer = new EntropyConsumerFails(address(random)); + EntropyConsumer consumer = new EntropyConsumer(address(random), true); vm.deal(user1, fee); vm.prank(user1); uint64 assignedSequenceNumber = consumer.requestEntropy{value: fee}( @@ -1025,15 +1024,15 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { // Verify the gas limit was set correctly assertEq(req.blockNumberOrGasLimit, defaultGasLimit); - // First attempt should fail and emit CallbackFailed event + // On the first attempt, the transaction should succeed and emit CallbackFailed event. + bytes memory revertReason = abi.encodeWithSelector(0x08c379a0, "Callback failed"); vm.expectEmit(false, false, false, true, address(random)); emit CallbackFailed( provider1, address(consumer), assignedSequenceNumber, - abi.encodeWithSelector(0x08c379a0, "Callback failed") + revertReason ); - vm.prank(user1); random.revealWithCallback( provider1, assignedSequenceNumber, @@ -1049,11 +1048,29 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { assertEq(reqAfterFailure.sequenceNumber, assignedSequenceNumber); assertTrue(reqAfterFailure.callbackFailed); -/* - // Second attempt should succeed and emit RevealedWithCallback event + // Second attempt should revert + vm.expectRevert(); + random.revealWithCallback( + provider1, + assignedSequenceNumber, + userRandomNumber, + provider1Proofs[assignedSequenceNumber] + ); + + // Again, request stays active after failure + reqAfterFailure = random.getRequest( + provider1, + assignedSequenceNumber + ); + assertEq(reqAfterFailure.sequenceNumber, assignedSequenceNumber); + assertTrue(reqAfterFailure.callbackFailed); + + // If the callback starts succeeding, we can invoke it and + // it emits RevealedWithCallback event. + consumer.setReverts(false); vm.expectEmit(false, false, false, true, address(random)); emit RevealedWithCallback( - req, + reqAfterFailure, userRandomNumber, provider1Proofs[assignedSequenceNumber], random.combineRandomValues( @@ -1062,7 +1079,6 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { 0 ) ); - vm.prank(user1); random.revealWithCallback( provider1, assignedSequenceNumber, @@ -1076,14 +1092,14 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { assignedSequenceNumber ); assertEq(reqAfterReveal.sequenceNumber, 0); - */ } function testRequestWithCallbackAndRevealWithCallbackFailing() public { bytes32 userRandomNumber = bytes32(uint(42)); uint fee = random.getFee(provider1); - EntropyConsumerFails consumer = new EntropyConsumerFails( - address(random) + EntropyConsumer consumer = new EntropyConsumer( + address(random), + true ); vm.deal(address(consumer), fee); vm.startPrank(address(consumer)); @@ -1422,9 +1438,11 @@ contract EntropyConsumer is IEntropyConsumer { bytes32 public randomness; address public entropy; address public provider; + bool public reverts; - constructor(address _entropy) { + constructor(address _entropy, bool _reverts) { entropy = _entropy; + reverts = _reverts; } function requestEntropy( @@ -1440,40 +1458,21 @@ contract EntropyConsumer is IEntropyConsumer { return entropy; } + function setReverts(bool _reverts) public { + reverts = _reverts; + } + function entropyCallback( uint64 _sequence, address _provider, bytes32 _randomness ) internal override { - sequence = _sequence; - provider = _provider; - randomness = _randomness; - } -} - -contract EntropyConsumerFails is IEntropyConsumer { - uint64 public sequence; - bytes32 public randomness; - address public entropy; - - constructor(address _entropy) { - entropy = _entropy; - } - - function requestEntropy( - bytes32 randomNumber - ) public payable returns (uint64 sequenceNumber) { - address _provider = IEntropy(entropy).getDefaultProvider(); - sequenceNumber = IEntropy(entropy).requestWithCallback{ - value: msg.value - }(_provider, randomNumber); - } - - function getEntropy() internal view override returns (address) { - return entropy; - } - - function entropyCallback(uint64, address, bytes32) internal pure override { - revert("Callback failed"); + if (!reverts) { + sequence = _sequence; + provider = _provider; + randomness = _randomness; + } else { + revert("Callback failed"); + } } } diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json index 50d5b5cdec..16fa80895a 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json @@ -22,9 +22,9 @@ }, { "indexed": false, - "internalType": "bytes32", + "internalType": "bytes", "name": "errorCode", - "type": "bytes32" + "type": "bytes" } ], "name": "CallbackFailed", diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json b/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json index f2b96d7e02..ff99f95530 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json @@ -22,9 +22,9 @@ }, { "indexed": false, - "internalType": "bytes32", + "internalType": "bytes", "name": "errorCode", - "type": "bytes32" + "type": "bytes" } ], "name": "CallbackFailed", From d08454e2892d95f400c0a6b709db04e94b8ae923 Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Wed, 2 Apr 2025 08:00:48 -0700 Subject: [PATCH 7/7] gas tests --- .../contracts/forge-test/Entropy.t.sol | 145 ++++++++++++++++-- 1 file changed, 133 insertions(+), 12 deletions(-) diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index a6e21d86fd..13668fdff3 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -1003,7 +1003,9 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { assertEq(reqAfterReveal.sequenceNumber, 0); } - function testRequestWithCallbackAndRevealWithCallbackWithGasLimitAndFailure() public { + function testRequestWithCallbackAndRevealWithCallbackWithGasLimitAndFailure() + public + { uint64 defaultGasLimit = 100000; vm.prank(provider1); random.setDefaultGasLimit(defaultGasLimit); @@ -1025,7 +1027,10 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { assertEq(req.blockNumberOrGasLimit, defaultGasLimit); // On the first attempt, the transaction should succeed and emit CallbackFailed event. - bytes memory revertReason = abi.encodeWithSelector(0x08c379a0, "Callback failed"); + bytes memory revertReason = abi.encodeWithSelector( + 0x08c379a0, + "Callback failed" + ); vm.expectEmit(false, false, false, true, address(random)); emit CallbackFailed( provider1, @@ -1058,15 +1063,11 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { ); // Again, request stays active after failure - reqAfterFailure = random.getRequest( - provider1, - assignedSequenceNumber - ); + reqAfterFailure = random.getRequest(provider1, assignedSequenceNumber); assertEq(reqAfterFailure.sequenceNumber, assignedSequenceNumber); assertTrue(reqAfterFailure.callbackFailed); - // If the callback starts succeeding, we can invoke it and - // it emits RevealedWithCallback event. + // If the callback starts succeeding, we can invoke it and it emits the usual RevealedWithCallback event. consumer.setReverts(false); vm.expectEmit(false, false, false, true, address(random)); emit RevealedWithCallback( @@ -1097,10 +1098,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { function testRequestWithCallbackAndRevealWithCallbackFailing() public { bytes32 userRandomNumber = bytes32(uint(42)); uint fee = random.getFee(provider1); - EntropyConsumer consumer = new EntropyConsumer( - address(random), - true - ); + EntropyConsumer consumer = new EntropyConsumer(address(random), true); vm.deal(address(consumer), fee); vm.startPrank(address(consumer)); uint64 assignedSequenceNumber = random.requestWithCallback{value: fee}( @@ -1431,6 +1429,108 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { // Fee should be the same as base fee since we're using less gas than default assertEq(fee, random.getFee(provider1)); } + + function testRequestWithCallbackAndRevealWithCallbackWithGasLimitAndFailureWithGasUsage() + public + { + uint64 defaultGasLimit = 100000; + vm.prank(provider1); + random.setDefaultGasLimit(defaultGasLimit); + + bytes32 userRandomNumber = bytes32(uint(42)); + uint fee = random.getFee(provider1); + EntropyConsumer consumer = new EntropyConsumer(address(random), false); + // Consumer callback uses ~10% more gas than the provider's default + consumer.setTargetGasUsage((defaultGasLimit * 110) / 100); + + vm.deal(user1, fee); + vm.prank(user1); + uint64 assignedSequenceNumber = consumer.requestEntropy{value: fee}( + userRandomNumber + ); + EntropyStructs.Request memory req = random.getRequest( + provider1, + assignedSequenceNumber + ); + + // Verify the gas limit was set correctly + assertEq(req.blockNumberOrGasLimit, defaultGasLimit); + + // The transaction reverts if the provider does not provide enough gas to forward + // the gasLimit to the callback transaction. + vm.expectRevert(); + random.revealWithCallback{gas: defaultGasLimit - 10000}( + provider1, + assignedSequenceNumber, + userRandomNumber, + provider1Proofs[assignedSequenceNumber] + ); + + // If called with enough gas, the transaction should succeed, but the callback should fail because + // it uses too much gas. + vm.expectEmit(false, false, false, true, address(random)); + emit CallbackFailed( + provider1, + address(consumer), + assignedSequenceNumber, + // out-of-gas reverts have an empty bytes array as the return value. + "" + ); + random.revealWithCallback( + provider1, + assignedSequenceNumber, + userRandomNumber, + provider1Proofs[assignedSequenceNumber] + ); + + // Verify request is still active after failure + EntropyStructs.Request memory reqAfterFailure = random.getRequest( + provider1, + assignedSequenceNumber + ); + assertEq(reqAfterFailure.sequenceNumber, assignedSequenceNumber); + assertTrue(reqAfterFailure.callbackFailed); + + // A subsequent attempt passing insufficient gas should also revert + vm.expectRevert(); + random.revealWithCallback{gas: defaultGasLimit - 10000}( + provider1, + assignedSequenceNumber, + userRandomNumber, + provider1Proofs[assignedSequenceNumber] + ); + + // Again, request stays active after failure + reqAfterFailure = random.getRequest(provider1, assignedSequenceNumber); + assertEq(reqAfterFailure.sequenceNumber, assignedSequenceNumber); + assertTrue(reqAfterFailure.callbackFailed); + + // Calling without a gas limit should succeed + vm.expectEmit(false, false, false, true, address(random)); + emit RevealedWithCallback( + reqAfterFailure, + userRandomNumber, + provider1Proofs[assignedSequenceNumber], + random.combineRandomValues( + userRandomNumber, + provider1Proofs[assignedSequenceNumber], + 0 + ) + ); + random.revealWithCallback( + provider1, + assignedSequenceNumber, + userRandomNumber, + provider1Proofs[assignedSequenceNumber] + ); + + // Verify request is cleared after successful reveal + EntropyStructs.Request memory reqAfterReveal = random.getRequest( + provider1, + assignedSequenceNumber + ); + assertEq(reqAfterReveal.sequenceNumber, 0); + } } contract EntropyConsumer is IEntropyConsumer { @@ -1439,10 +1539,13 @@ contract EntropyConsumer is IEntropyConsumer { address public entropy; address public provider; bool public reverts; + uint256 public gasUsed; + uint256 public targetGasUsage; constructor(address _entropy, bool _reverts) { entropy = _entropy; reverts = _reverts; + targetGasUsage = 0; // Default target } function requestEntropy( @@ -1462,11 +1565,29 @@ contract EntropyConsumer is IEntropyConsumer { reverts = _reverts; } + function setTargetGasUsage(uint256 _targetGasUsage) public { + targetGasUsage = _targetGasUsage; + } + function entropyCallback( uint64 _sequence, address _provider, bytes32 _randomness ) internal override { + uint256 startGas = gasleft(); + uint256 currentGasUsed = 0; + + // Keep consuming gas until we reach our target + while (currentGasUsed < targetGasUsage) { + // Consume gas with a hash operation + bytes32 hash = keccak256( + abi.encodePacked(currentGasUsed, _randomness) + ); + currentGasUsed = startGas - gasleft(); + } + + gasUsed = currentGasUsed; + if (!reverts) { sequence = _sequence; provider = _provider;