From 8a7d6a9932a0ea8ebc3d0e499a3f91bc6953fbf7 Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Wed, 2 Apr 2025 10:54:03 -0700 Subject: [PATCH 01/15] moving stuff around --- .../contracts/contracts/entropy/Entropy.sol | 78 ++++++- .../contracts/forge-test/Entropy.t.sol | 194 +++++++++++++++++- .../entropy_sdk/solidity/EntropyEvents.sol | 6 + .../entropy_sdk/solidity/EntropyStructs.sol | 4 + .../entropy_sdk/solidity/IEntropy.sol | 14 ++ 5 files changed, 281 insertions(+), 15 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index 589fa7eeee..17e253dc21 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -207,7 +207,8 @@ abstract contract Entropy is IEntropy, EntropyState { address provider, bytes32 userCommitment, bool useBlockhash, - bool isRequestWithCallback + bool isRequestWithCallback, + uint32 callbackGasLimit ) internal returns (EntropyStructs.Request storage req) { EntropyStructs.ProviderInfo storage providerInfo = _state.providers[ provider @@ -222,11 +223,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 += providerInfo.feeInWei; + 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 @@ -251,9 +253,17 @@ abstract contract Entropy is IEntropy, EntropyState { req.blockNumber = SafeCast.toUint64(block.number); req.useBlockhash = useBlockhash; +<<<<<<< HEAD req.callbackStatus = isRequestWithCallback ? EntropyStatusConstants.CALLBACK_NOT_STARTED : EntropyStatusConstants.CALLBACK_NOT_NECESSARY; +======= + req.status = isRequestWithCallback + ? EntropyConstants.STATUS_CALLBACK_NOT_STARTED + : EntropyConstants.STATUS_NO_CALLBACK; + // TODO: rounding! + req.gasLimit10k = SafeCast.toUint16(callbackGasLimit / 10000); +>>>>>>> 185f7de53 (moving stuff around) } // As a user, request a random number from `provider`. Prior to calling this method, the user should @@ -275,7 +285,8 @@ abstract contract Entropy is IEntropy, EntropyState { provider, userCommitment, useBlockHash, - false + false, + 0 ); assignedSequenceNumber = req.sequenceNumber; emit Requested(req); @@ -292,6 +303,14 @@ abstract contract Entropy is IEntropy, EntropyState { function requestWithCallback( address provider, bytes32 userRandomNumber + ) public payable override returns (uint64) { + return requestWithCallbackAndGasLimit(provider, userRandomNumber, _state.providers[provider].defaultGasLimit); + } + + function requestWithCallbackAndGasLimit( + address provider, + bytes32 userRandomNumber, + uint32 gasLimit ) public payable override returns (uint64) { EntropyStructs.Request storage req = requestHelper( provider, @@ -300,7 +319,8 @@ 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, + gasLimit ); emit RequestedWithCallback( @@ -310,7 +330,6 @@ abstract contract Entropy is IEntropy, EntropyState { userRandomNumber, req ); - return req.sequenceNumber; } @@ -497,12 +516,12 @@ abstract contract Entropy is IEntropy, EntropyState { // any reverts will be reported as an event. Any failing requests move to a failure state // at which point they can be recovered. The recovery flow invokes the callback directly // (no catching errors) which allows callers to easily see the revert reason. - if (req.callbackStatus == EntropyStatusConstants.CALLBACK_NOT_STARTED) { + if (req.gasLimit10k != 0 && req.callbackStatus == EntropyStatusConstants.CALLBACK_NOT_STARTED) { req.callbackStatus = EntropyStatusConstants.CALLBACK_IN_PROGRESS; bool success; bytes memory ret; (success, ret) = callAddress.excessivelySafeCall( - gasleft(), // TODO: providers need to be able to configure this in the future. + req.gasLimit10k * 10000, 256, // copy at most 256 bytes of the return value into ret. abi.encodeWithSelector( IEntropyConsumer._entropyCallback.selector, @@ -590,7 +609,32 @@ 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, + uint32 gasLimit + ) public view override returns (uint128 feeAmount) { + return getProviderFee(provider, gasLimit) + _state.pythFeeInWei; + } + + function getProviderFee( + address providerAddr, + uint32 gasLimit + ) internal view returns (uint128 feeAmount) { + EntropyStructs.ProviderInfo memory provider = _state.providers[ + 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; + } else { + return provider.feeInWei; + } } function getPythFee() public view returns (uint128 feeAmount) { @@ -687,6 +731,20 @@ abstract contract Entropy is IEntropy, EntropyState { ); } + // Set the default gas limit for a request. + function setDefaultGasLimit(uint32 gasLimit) external override { + EntropyStructs.ProviderInfo storage provider = _state.providers[ + msg.sender + ]; + if (provider.sequenceNumber == 0) { + revert EntropyErrors.NoSuchProvider(); + } + + uint32 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/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index 720fd092d0..1a497089d7 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -806,6 +806,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { requester: user1, useBlockhash: false, callbackStatus: EntropyStatusConstants.CALLBACK_NOT_STARTED + gasLimit10k: 0 }) ); vm.roll(1234); @@ -939,9 +940,6 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { ); } - // TODO: restore this test once providers have custom gas limits, which allow us to toggle between - // the old and new failure behavior. - /* function testRequestWithCallbackAndRevealWithCallbackFailing() public { bytes32 userRandomNumber = bytes32(uint(42)); uint fee = random.getFee(provider1); @@ -961,9 +959,69 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { provider1Proofs[assignedSequenceNumber] ); } - */ + + function testRequestWithCallbackGasLimit() 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); + 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); + + vm.expectEmit(false, false, false, true, address(random)); + emit RevealedWithCallback( + req, + userRandomNumber, + provider1Proofs[assignedSequenceNumber], + random.combineRandomValues( + userRandomNumber, + provider1Proofs[assignedSequenceNumber], + 0 + ) + ); + 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 testRequestWithRevertingCallback() 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), true); @@ -1056,7 +1114,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { } function testRequestWithCallbackUsingTooMuchGas() public { - uint64 defaultGasLimit = 100000; + uint32 defaultGasLimit = 100000; bytes32 userRandomNumber = bytes32(uint(42)); uint fee = random.getFee(provider1); @@ -1299,6 +1357,132 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { vm.expectRevert(); random.setProviderFeeAsFeeManager(provider1, 1000); } + + function testSetDefaultGasLimit() public { + uint32 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 { + uint32 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.gasLimit10k, defaultGasLimit); + } + + function testRequestWithCallbackAndCustomGasLimit() public { + uint32 defaultGasLimit = 100000; + uint32 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.requestWithCallbackAndGasLimit{value: fee}( + provider1, + userRandomNumber, + customGasLimit + ); + + EntropyStructs.Request memory req = random.getRequest( + provider1, + sequenceNumber + ); + assertEq(req.gasLimit10k, customGasLimit); + } + + function testRequestWithCallbackAndGasLimitFeeScaling() public { + uint32 defaultGasLimit = 100000; + uint32 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 { + uint32 defaultGasLimit = 100000; + uint32 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.requestWithCallbackAndGasLimit{value: baseFee}( + provider1, + userRandomNumber, + doubleGasLimit + ); + } + + function testRequestWithCallbackAndGasLimitLowerThanDefault() public { + uint32 defaultGasLimit = 100000; + uint32 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.requestWithCallbackAndGasLimit{value: fee}( + provider1, + userRandomNumber, + lowerGasLimit + ); + + EntropyStructs.Request memory req = random.getRequest( + provider1, + sequenceNumber + ); + assertEq(req.gasLimit10k, 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/EntropyEvents.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol index 6587cefe41..b502f6d397 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol @@ -41,6 +41,12 @@ interface EntropyEvents { event ProviderFeeUpdated(address provider, uint128 oldFee, uint128 newFee); + event ProviderDefaultGasLimitUpdated( + address indexed provider, + uint64 oldDefaultGasLimit, + uint64 newDefaultGasLimit + ); + event ProviderUriUpdated(address provider, bytes oldUri, bytes newUri); event ProviderFeeManagerUpdated( diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol index c0092ff1c6..5fd1b50c7e 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. + uint32 defaultGasLimit; } struct Request { @@ -62,5 +64,7 @@ contract EntropyStructs { // Status flag for requests with callbacks. See EntropyConstants for the possible values of this flag. uint8 callbackStatus; // 2 bytes of space left in this struct. + // The gasLimit in units of 10k gas. (i.e., 2 = 20k gas) + uint16 gasLimit10k; } } diff --git a/target_chains/ethereum/entropy_sdk/solidity/IEntropy.sol b/target_chains/ethereum/entropy_sdk/solidity/IEntropy.sol index ce446ff44b..9b0b69dc8a 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 requestWithCallbackAndGasLimit( + address provider, + bytes32 userRandomNumber, + uint32 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, + uint32 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(uint32 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( From 959f50e24b7a1b99ac7e6189b9fcf01d2ab0a885 Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Wed, 2 Apr 2025 11:01:06 -0700 Subject: [PATCH 02/15] getting tests in place --- .../contracts/contracts/entropy/Entropy.sol | 16 +-- .../contracts/forge-test/Entropy.t.sol | 85 +++++++++--- .../solidity/abis/EntropyEvents.json | 50 +++++++ .../entropy_sdk/solidity/abis/IEntropy.json | 126 ++++++++++++++++++ 4 files changed, 253 insertions(+), 24 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index 17e253dc21..b56b296a38 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -253,17 +253,12 @@ abstract contract Entropy is IEntropy, EntropyState { req.blockNumber = SafeCast.toUint64(block.number); req.useBlockhash = useBlockhash; -<<<<<<< HEAD + req.callbackStatus = isRequestWithCallback ? EntropyStatusConstants.CALLBACK_NOT_STARTED : EntropyStatusConstants.CALLBACK_NOT_NECESSARY; -======= - req.status = isRequestWithCallback - ? EntropyConstants.STATUS_CALLBACK_NOT_STARTED - : EntropyConstants.STATUS_NO_CALLBACK; - // TODO: rounding! + // TODO: rounding! req.gasLimit10k = SafeCast.toUint16(callbackGasLimit / 10000); ->>>>>>> 185f7de53 (moving stuff around) } // As a user, request a random number from `provider`. Prior to calling this method, the user should @@ -304,7 +299,12 @@ abstract contract Entropy is IEntropy, EntropyState { address provider, bytes32 userRandomNumber ) public payable override returns (uint64) { - return requestWithCallbackAndGasLimit(provider, userRandomNumber, _state.providers[provider].defaultGasLimit); + return + requestWithCallbackAndGasLimit( + provider, + userRandomNumber, + _state.providers[provider].defaultGasLimit + ); } function requestWithCallbackAndGasLimit( diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index 1a497089d7..5444c040c4 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -961,7 +961,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { } function testRequestWithCallbackGasLimit() public { - uint64 defaultGasLimit = 100000; + uint32 defaultGasLimit = 100000; vm.prank(provider1); random.setDefaultGasLimit(defaultGasLimit); @@ -979,7 +979,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { ); // Verify the gas limit was set correctly - assertEq(req.blockNumberOrGasLimit, defaultGasLimit); + assertEq(req.gasLimit10k, defaultGasLimit); vm.expectEmit(false, false, false, true, address(random)); emit RevealedWithCallback( @@ -1018,7 +1018,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { } function testRequestWithRevertingCallback() public { - uint64 defaultGasLimit = 100000; + uint32 defaultGasLimit = 100000; vm.prank(provider1); random.setDefaultGasLimit(defaultGasLimit); @@ -1115,6 +1115,8 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { function testRequestWithCallbackUsingTooMuchGas() public { uint32 defaultGasLimit = 100000; + vm.prank(provider1); + random.setDefaultGasLimit(defaultGasLimit); bytes32 userRandomNumber = bytes32(uint(42)); uint fee = random.getFee(provider1); @@ -1132,20 +1134,75 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { assignedSequenceNumber ); + // Verify the gas limit was set correctly + assertEq(req.gasLimit10k, 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}( + 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, + userRandomNumber, + provider1Proofs[assignedSequenceNumber], + random.combineRandomValues( + userRandomNumber, + provider1Proofs[assignedSequenceNumber], + 0 + ), + // 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); + assertEq( + reqAfterFailure.status, + EntropyConstants.STATUS_CALLBACK_FAILED + ); + + // 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); + assertEq( + reqAfterFailure.status, + EntropyConstants.STATUS_CALLBACK_FAILED + ); + // Calling without a gas limit should succeed vm.expectEmit(false, false, false, true, address(random)); emit RevealedWithCallback( - req, + reqAfterFailure, userRandomNumber, provider1Proofs[assignedSequenceNumber], random.combineRandomValues( @@ -1409,11 +1466,9 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { vm.deal(user1, fee); vm.prank(user1); - uint64 sequenceNumber = random.requestWithCallbackAndGasLimit{value: fee}( - provider1, - userRandomNumber, - customGasLimit - ); + uint64 sequenceNumber = random.requestWithCallbackAndGasLimit{ + value: fee + }(provider1, userRandomNumber, customGasLimit); EntropyStructs.Request memory req = random.getRequest( provider1, @@ -1469,11 +1524,9 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { vm.deal(user1, fee); vm.prank(user1); - uint64 sequenceNumber = random.requestWithCallbackAndGasLimit{value: fee}( - provider1, - userRandomNumber, - lowerGasLimit - ); + uint64 sequenceNumber = random.requestWithCallbackAndGasLimit{ + value: fee + }(provider1, userRandomNumber, lowerGasLimit); EntropyStructs.Request memory req = random.getRequest( provider1, @@ -1482,7 +1535,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { assertEq(req.gasLimit10k, 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/abis/EntropyEvents.json b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json index 164c3d4b19..a2c4e7d4f4 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json @@ -48,6 +48,31 @@ "name": "CallbackFailed", "type": "event" }, + { + "anonymous": false, + "inputs": [ + { + "indexed": true, + "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": [ @@ -212,6 +237,11 @@ "internalType": "uint32", "name": "maxNumHashes", "type": "uint32" + }, + { + "internalType": "uint32", + "name": "defaultGasLimit", + "type": "uint32" } ], "indexed": false, @@ -267,6 +297,11 @@ "internalType": "uint8", "name": "callbackStatus", "type": "uint8" + }, + { + "internalType": "uint16", + "name": "gasLimit10k", + "type": "uint16" } ], "indexed": false, @@ -346,6 +381,11 @@ "internalType": "uint8", "name": "callbackStatus", "type": "uint8" + }, + { + "internalType": "uint16", + "name": "gasLimit10k", + "type": "uint16" } ], "indexed": false, @@ -401,6 +441,11 @@ "internalType": "uint8", "name": "callbackStatus", "type": "uint8" + }, + { + "internalType": "uint16", + "name": "gasLimit10k", + "type": "uint16" } ], "indexed": false, @@ -480,6 +525,11 @@ "internalType": "uint8", "name": "callbackStatus", "type": "uint8" + }, + { + "internalType": "uint16", + "name": "gasLimit10k", + "type": "uint16" } ], "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 1c6de9d2d3..4a344385a9 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json @@ -48,6 +48,31 @@ "name": "CallbackFailed", "type": "event" }, + { + "anonymous": false, + "inputs": [ + { + "indexed": true, + "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": [ @@ -212,6 +237,11 @@ "internalType": "uint32", "name": "maxNumHashes", "type": "uint32" + }, + { + "internalType": "uint32", + "name": "defaultGasLimit", + "type": "uint32" } ], "indexed": false, @@ -267,6 +297,11 @@ "internalType": "uint8", "name": "callbackStatus", "type": "uint8" + }, + { + "internalType": "uint16", + "name": "gasLimit10k", + "type": "uint16" } ], "indexed": false, @@ -346,6 +381,11 @@ "internalType": "uint8", "name": "callbackStatus", "type": "uint8" + }, + { + "internalType": "uint16", + "name": "gasLimit10k", + "type": "uint16" } ], "indexed": false, @@ -401,6 +441,11 @@ "internalType": "uint8", "name": "callbackStatus", "type": "uint8" + }, + { + "internalType": "uint16", + "name": "gasLimit10k", + "type": "uint16" } ], "indexed": false, @@ -480,6 +525,11 @@ "internalType": "uint8", "name": "callbackStatus", "type": "uint8" + }, + { + "internalType": "uint16", + "name": "gasLimit10k", + "type": "uint16" } ], "indexed": false, @@ -650,6 +700,30 @@ "stateMutability": "view", "type": "function" }, + { + "inputs": [ + { + "internalType": "address", + "name": "provider", + "type": "address" + }, + { + "internalType": "uint32", + "name": "gasLimit", + "type": "uint32" + } + ], + "name": "getFeeForGas", + "outputs": [ + { + "internalType": "uint128", + "name": "feeAmount", + "type": "uint128" + } + ], + "stateMutability": "view", + "type": "function" + }, { "inputs": [ { @@ -721,6 +795,11 @@ "internalType": "uint32", "name": "maxNumHashes", "type": "uint32" + }, + { + "internalType": "uint32", + "name": "defaultGasLimit", + "type": "uint32" } ], "internalType": "struct EntropyStructs.ProviderInfo", @@ -787,6 +866,11 @@ "internalType": "uint8", "name": "callbackStatus", "type": "uint8" + }, + { + "internalType": "uint16", + "name": "gasLimit10k", + "type": "uint16" } ], "internalType": "struct EntropyStructs.Request", @@ -883,6 +967,35 @@ "stateMutability": "payable", "type": "function" }, + { + "inputs": [ + { + "internalType": "address", + "name": "provider", + "type": "address" + }, + { + "internalType": "bytes32", + "name": "userRandomNumber", + "type": "bytes32" + }, + { + "internalType": "uint32", + "name": "gasLimit", + "type": "uint32" + } + ], + "name": "requestWithCallbackAndGasLimit", + "outputs": [ + { + "internalType": "uint64", + "name": "assignedSequenceNumber", + "type": "uint64" + } + ], + "stateMutability": "payable", + "type": "function" + }, { "inputs": [ { @@ -945,6 +1058,19 @@ "stateMutability": "nonpayable", "type": "function" }, + { + "inputs": [ + { + "internalType": "uint32", + "name": "gasLimit", + "type": "uint32" + } + ], + "name": "setDefaultGasLimit", + "outputs": [], + "stateMutability": "nonpayable", + "type": "function" + }, { "inputs": [ { From 52309936cd7e403964b07ea5744392a2af885e35 Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Wed, 2 Apr 2025 12:59:19 -0700 Subject: [PATCH 03/15] fixing stuff --- .../contracts/contracts/entropy/Entropy.sol | 31 ++++++++++----- .../contracts/forge-test/Entropy.t.sol | 38 ++++++++++++++++--- .../entropy_sdk/solidity/EntropyEvents.sol | 11 ++++++ 3 files changed, 66 insertions(+), 14 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index b56b296a38..47c560f4c4 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -257,8 +257,7 @@ abstract contract Entropy is IEntropy, EntropyState { req.callbackStatus = isRequestWithCallback ? EntropyStatusConstants.CALLBACK_NOT_STARTED : EntropyStatusConstants.CALLBACK_NOT_NECESSARY; - // TODO: rounding! - req.gasLimit10k = SafeCast.toUint16(callbackGasLimit / 10000); + req.gasLimit10k = roundGas(callbackGasLimit); } // As a user, request a random number from `provider`. Prior to calling this method, the user should @@ -520,8 +519,9 @@ abstract contract Entropy is IEntropy, EntropyState { req.callbackStatus = EntropyStatusConstants.CALLBACK_IN_PROGRESS; bool success; bytes memory ret; + uint256 startingGas = gasleft(); (success, ret) = callAddress.excessivelySafeCall( - req.gasLimit10k * 10000, + uint256(req.gasLimit10k) * 10000, 256, // copy at most 256 bytes of the return value into ret. abi.encodeWithSelector( IEntropyConsumer._entropyCallback.selector, @@ -530,6 +530,7 @@ abstract contract Entropy is IEntropy, EntropyState { randomNumber ) ); + uint256 endingGas = gasleft(); // Reset status to not started here in case the transaction reverts. req.callbackStatus = EntropyStatusConstants.CALLBACK_NOT_STARTED; @@ -541,8 +542,8 @@ abstract contract Entropy is IEntropy, EntropyState { randomNumber ); clearRequest(provider, sequenceNumber); - } else if (ret.length > 0) { - // Callback reverted for some reason that is *not* out-of-gas. + } else if (ret.length > 0 || startingGas - endingGas >= uint256(req.gasLimit10k) * 10000) { + // Callback reverted for some reason. Note ret.length == 0 implies out of gas. emit CallbackFailed( provider, req.requester, @@ -554,8 +555,6 @@ abstract contract Entropy is IEntropy, EntropyState { ); req.callbackStatus = EntropyStatusConstants.CALLBACK_FAILED; } else { - // The callback ran out of gas - // TODO: this case will go away once we add provider gas limits, so we're not putting in a custom error type. require(false, "provider needs to send more gas"); } } else { @@ -626,10 +625,12 @@ abstract contract Entropy is IEntropy, EntropyState { EntropyStructs.ProviderInfo memory provider = _state.providers[ providerAddr ]; - if (gasLimit > provider.defaultGasLimit) { + + uint32 roundedGasLimit = uint32(roundGas(gasLimit)) * 10000; + if (roundedGasLimit > 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) * + uint128 additionalFee = ((roundedGasLimit - provider.defaultGasLimit) * provider.feeInWei) / provider.defaultGasLimit; return provider.feeInWei + additionalFee; } else { @@ -761,6 +762,18 @@ abstract contract Entropy is IEntropy, EntropyState { ); } + // Rounds the provided quantity of gas into units of 10k gas. + // If gas is not evenly divisible by 10k, rounds up. + function roundGas( + uint32 gas + ) public pure returns (uint16) { + uint32 gas10k = gas / 10000; + if (gas10k * 10000 < gas) { + gas10k += 1; + } + return SafeCast.toUint16(gas10k); + } + // 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( diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index 5444c040c4..615f7fb296 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -979,7 +979,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { ); // Verify the gas limit was set correctly - assertEq(req.gasLimit10k, defaultGasLimit); + assertEq(req.gasLimit10k, 10); vm.expectEmit(false, false, false, true, address(random)); emit RevealedWithCallback( @@ -1135,7 +1135,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { ); // Verify the gas limit was set correctly - assertEq(req.gasLimit10k, defaultGasLimit); + assertEq(req.gasLimit10k, 10); // The transaction reverts if the provider does not provide enough gas to forward // the gasLimit to the callback transaction. @@ -1451,7 +1451,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { provider1, sequenceNumber ); - assertEq(req.gasLimit10k, defaultGasLimit); + assertEq(req.gasLimit10k, 10); } function testRequestWithCallbackAndCustomGasLimit() public { @@ -1474,7 +1474,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { provider1, sequenceNumber ); - assertEq(req.gasLimit10k, customGasLimit); + assertEq(req.gasLimit10k, 20); } function testRequestWithCallbackAndGasLimitFeeScaling() public { @@ -1532,10 +1532,38 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { provider1, sequenceNumber ); - assertEq(req.gasLimit10k, lowerGasLimit); + assertEq(req.gasLimit10k, 5); // Fee should be the same as base fee since we're using less gas than default assertEq(fee, random.getFee(provider1)); } + + function testRoundGas() public { + // TODO: test this with the full request/reveal flow and update the max value + + // Test exact multiples of 10,000 + assertEq(random.roundGas(0), 0); + assertEq(random.roundGas(10000), 1); + assertEq(random.roundGas(20000), 2); + assertEq(random.roundGas(100000), 10); + + // Test values just below multiples of 10,000 + assertEq(random.roundGas(9999), 1); + assertEq(random.roundGas(19999), 2); + assertEq(random.roundGas(99999), 10); + + // Test values just above multiples of 10,000 + assertEq(random.roundGas(10001), 2); + assertEq(random.roundGas(20001), 3); + assertEq(random.roundGas(100001), 11); + + // Test middle values + assertEq(random.roundGas(5000), 1); + assertEq(random.roundGas(15000), 2); + assertEq(random.roundGas(25000), 3); + + // Test maximum uint32 value + assertEq(random.roundGas(type(uint32).max), 429497); // (2^32 - 1) / 10000 rounded up + } } contract EntropyConsumer is IEntropyConsumer { diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol index b502f6d397..df42c5dc09 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol @@ -39,8 +39,19 @@ interface EntropyEvents { bytes errorCode ); + event CallbackOutOfGas( + address indexed provider, + address indexed requestor, + uint64 indexed sequenceNumber, + bytes32 userRandomNumber, + bytes32 providerRevelation, + bytes32 randomNumber, + bytes errorCode + ); + event ProviderFeeUpdated(address provider, uint128 oldFee, uint128 newFee); + // TODO: uint32 event ProviderDefaultGasLimitUpdated( address indexed provider, uint64 oldDefaultGasLimit, From 0c12829c407ac7e03cefa78957dc0ee6e371f6e2 Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Wed, 2 Apr 2025 13:02:24 -0700 Subject: [PATCH 04/15] fixing stuff --- .../contracts/forge-test/Entropy.t.sol | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index 615f7fb296..9dbbd45f6e 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -1226,6 +1226,120 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { assertEq(reqAfterReveal.sequenceNumber, 0); } + function testRequestWithCallbackUsingTooMuchGas2() public { + uint32 defaultGasLimit = 64000000; + vm.prank(provider1); + random.setDefaultGasLimit(defaultGasLimit); + + bytes32 userRandomNumber = bytes32(uint(42)); + uint fee = random.getFee(provider1); + EntropyConsumer consumer = new EntropyConsumer(address(random), false); + consumer.setTargetGasUsage(defaultGasLimit); + + 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.gasLimit10k, 10); + + // 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, + userRandomNumber, + provider1Proofs[assignedSequenceNumber], + random.combineRandomValues( + userRandomNumber, + provider1Proofs[assignedSequenceNumber], + 0 + ), + // 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); + assertEq( + reqAfterFailure.status, + EntropyConstants.STATUS_CALLBACK_FAILED + ); + + // 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); + assertEq( + reqAfterFailure.status, + EntropyConstants.STATUS_CALLBACK_FAILED + ); + + // 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); + */ + } + function testLastRevealedTooOld() public { for (uint256 i = 0; i < provider1MaxNumHashes; i++) { request(user1, provider1, 42, false); From 80fc09af3cdab090f3eff8a76282b5638f7f6596 Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Wed, 2 Apr 2025 20:29:54 -0700 Subject: [PATCH 05/15] more stuff --- .../contracts/contracts/entropy/Entropy.sol | 17 +++- .../contracts/forge-test/Entropy.t.sol | 90 ++----------------- .../entropy_sdk/solidity/EntropyErrors.sol | 2 + 3 files changed, 23 insertions(+), 86 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index 47c560f4c4..bc63ab3ba5 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -521,6 +521,11 @@ abstract contract Entropy is IEntropy, EntropyState { bytes memory ret; uint256 startingGas = gasleft(); (success, ret) = callAddress.excessivelySafeCall( + // Warning: the provided gas limit below is only an *upper bound* on the gas provided to the call. + // If the current context has less gas the desired value, 63/64ths of the current context's gas will + // be provided instead. (See CALL opcode docs here https://www.evm.codes/?fork=cancun#f1) + // Consequently, out-of-gas reverts need to be handled carefully to ensure that the callback + // was truly provided with a sufficient amount of gas. uint256(req.gasLimit10k) * 10000, 256, // copy at most 256 bytes of the return value into ret. abi.encodeWithSelector( @@ -543,7 +548,9 @@ abstract contract Entropy is IEntropy, EntropyState { ); clearRequest(provider, sequenceNumber); } else if (ret.length > 0 || startingGas - endingGas >= uint256(req.gasLimit10k) * 10000) { - // Callback reverted for some reason. Note ret.length == 0 implies out of gas. + // The callback reverted for some reason. + // If ret.length == 0, then the callback ran out of gas. In this case, ensure that the + // callback was provided with sufficient gas. emit CallbackFailed( provider, req.requester, @@ -555,7 +562,13 @@ abstract contract Entropy is IEntropy, EntropyState { ); req.callbackStatus = EntropyStatusConstants.CALLBACK_FAILED; } else { - require(false, "provider needs to send more gas"); + // Callback reverted by running out of gas, but the calling context did not have enough gas + // to run the callback. This is a corner case that can happen due to the nuances of gas passing + // in calls (see the comment on the call above). + // + // (Note that reverting here plays nicely with the estimateGas RPC method, which binary searches for + // the smallest gas value that causes the transaction to *succeed*. See https://github.com/ethereum/go-ethereum/pull/3587 ) + revert EntropyErrors.InsufficientGas(); } } else { // This case uses the checks-effects-interactions pattern to avoid reentry attacks diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index 9dbbd45f6e..697c107174 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -1226,7 +1226,12 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { assertEq(reqAfterReveal.sequenceNumber, 0); } + // Test the corner case caused by the CALL opcode passing at most 63/64ths of the current gas + // to the sub-call. function testRequestWithCallbackUsingTooMuchGas2() public { + // With a 64M gas limit, we will pass ~63M gas to the callback (which is insufficient), but still + // have ~1M gas to execute code within the revealWithCallback method, which should be enough to + // run all of the logic subsequent to the excessivelySafeCall. uint32 defaultGasLimit = 64000000; vm.prank(provider1); random.setDefaultGasLimit(defaultGasLimit); @@ -1246,98 +1251,15 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { assignedSequenceNumber ); - // Verify the gas limit was set correctly - // assertEq(req.gasLimit10k, 10); - // The transaction reverts if the provider does not provide enough gas to forward // the gasLimit to the callback transaction. - vm.expectRevert(); + vm.expectRevert(EntropyErrors.InsufficientGas.selector); 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, - userRandomNumber, - provider1Proofs[assignedSequenceNumber], - random.combineRandomValues( - userRandomNumber, - provider1Proofs[assignedSequenceNumber], - 0 - ), - // 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); - assertEq( - reqAfterFailure.status, - EntropyConstants.STATUS_CALLBACK_FAILED - ); - - // 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); - assertEq( - reqAfterFailure.status, - EntropyConstants.STATUS_CALLBACK_FAILED - ); - - // 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); - */ } function testLastRevealedTooOld() public { diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol index 0511daae9a..56bc273d58 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol @@ -43,4 +43,6 @@ library EntropyErrors { error LastRevealedTooOld(); // A more recent commitment is already revealed on-chain error UpdateTooOld(); + // Not enough gas was provided to the function to execute the callback with the desired amount of gas. + error InsufficientGas(); } From 6e8898d8bbc2f03521d3db3225d256adefd9e7f4 Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Fri, 4 Apr 2025 07:27:46 -0700 Subject: [PATCH 06/15] rebase --- .../contracts/forge-test/Entropy.t.sol | 10 ++-- .../solidity/abis/EntropyErrors.json | 5 ++ .../solidity/abis/EntropyEvents.json | 49 +++++++++++++++++++ .../entropy_sdk/solidity/abis/IEntropy.json | 49 +++++++++++++++++++ 4 files changed, 108 insertions(+), 5 deletions(-) diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index 697c107174..cb92f04c61 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -805,7 +805,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { blockNumber: 1234, requester: user1, useBlockhash: false, - callbackStatus: EntropyStatusConstants.CALLBACK_NOT_STARTED + callbackStatus: EntropyStatusConstants.CALLBACK_NOT_STARTED, gasLimit10k: 0 }) ); @@ -1178,8 +1178,8 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { ); assertEq(reqAfterFailure.sequenceNumber, assignedSequenceNumber); assertEq( - reqAfterFailure.status, - EntropyConstants.STATUS_CALLBACK_FAILED + reqAfterFailure.callbackStatus, + EntropyStatusConstants.CALLBACK_FAILED ); // A subsequent attempt passing insufficient gas should also revert @@ -1195,8 +1195,8 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { reqAfterFailure = random.getRequest(provider1, assignedSequenceNumber); assertEq(reqAfterFailure.sequenceNumber, assignedSequenceNumber); assertEq( - reqAfterFailure.status, - EntropyConstants.STATUS_CALLBACK_FAILED + reqAfterFailure.callbackStatus, + EntropyStatusConstants.CALLBACK_FAILED ); // Calling without a gas limit should succeed diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json index 24a6ef2548..65d64c324a 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json @@ -19,6 +19,11 @@ "name": "InsufficientFee", "type": "error" }, + { + "inputs": [], + "name": "InsufficientGas", + "type": "error" + }, { "inputs": [], "name": "InvalidRevealCall", diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json index a2c4e7d4f4..7149128c7d 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json @@ -48,6 +48,55 @@ "name": "CallbackFailed", "type": "event" }, + { + "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": "userRandomNumber", + "type": "bytes32" + }, + { + "indexed": false, + "internalType": "bytes32", + "name": "providerRevelation", + "type": "bytes32" + }, + { + "indexed": false, + "internalType": "bytes32", + "name": "randomNumber", + "type": "bytes32" + }, + { + "indexed": false, + "internalType": "bytes", + "name": "errorCode", + "type": "bytes" + } + ], + "name": "CallbackOutOfGas", + "type": "event" + }, { "anonymous": false, "inputs": [ diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json b/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json index 4a344385a9..44f9e98802 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json @@ -48,6 +48,55 @@ "name": "CallbackFailed", "type": "event" }, + { + "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": "userRandomNumber", + "type": "bytes32" + }, + { + "indexed": false, + "internalType": "bytes32", + "name": "providerRevelation", + "type": "bytes32" + }, + { + "indexed": false, + "internalType": "bytes32", + "name": "randomNumber", + "type": "bytes32" + }, + { + "indexed": false, + "internalType": "bytes", + "name": "errorCode", + "type": "bytes" + } + ], + "name": "CallbackOutOfGas", + "type": "event" + }, { "anonymous": false, "inputs": [ From 90bc88df2948f0abcd41396544098a45258724e3 Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Fri, 4 Apr 2025 08:35:31 -0700 Subject: [PATCH 07/15] cleaning up --- .../contracts/contracts/entropy/Entropy.sol | 43 ++++++----- .../contracts/forge-test/Entropy.t.sol | 74 +++++++++++++------ 2 files changed, 78 insertions(+), 39 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index bc63ab3ba5..2c5c8861c2 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -515,17 +515,20 @@ abstract contract Entropy is IEntropy, EntropyState { // any reverts will be reported as an event. Any failing requests move to a failure state // at which point they can be recovered. The recovery flow invokes the callback directly // (no catching errors) which allows callers to easily see the revert reason. - if (req.gasLimit10k != 0 && req.callbackStatus == EntropyStatusConstants.CALLBACK_NOT_STARTED) { + if ( + req.gasLimit10k != 0 && + req.callbackStatus == EntropyStatusConstants.CALLBACK_NOT_STARTED + ) { req.callbackStatus = EntropyStatusConstants.CALLBACK_IN_PROGRESS; bool success; bytes memory ret; uint256 startingGas = gasleft(); (success, ret) = callAddress.excessivelySafeCall( // Warning: the provided gas limit below is only an *upper bound* on the gas provided to the call. - // If the current context has less gas the desired value, 63/64ths of the current context's gas will - // be provided instead. (See CALL opcode docs here https://www.evm.codes/?fork=cancun#f1) + // At most 63/64ths of the current context's gas will be provided to a call, which may be less + // than the indicated gas limit. (See CALL opcode docs here https://www.evm.codes/?fork=cancun#f1) // Consequently, out-of-gas reverts need to be handled carefully to ensure that the callback - // was truly provided with a sufficient amount of gas. + // was truly provided with a sufficient amount of gas. uint256(req.gasLimit10k) * 10000, 256, // copy at most 256 bytes of the return value into ret. abi.encodeWithSelector( @@ -535,7 +538,6 @@ abstract contract Entropy is IEntropy, EntropyState { randomNumber ) ); - uint256 endingGas = gasleft(); // Reset status to not started here in case the transaction reverts. req.callbackStatus = EntropyStatusConstants.CALLBACK_NOT_STARTED; @@ -547,10 +549,15 @@ abstract contract Entropy is IEntropy, EntropyState { randomNumber ); clearRequest(provider, sequenceNumber); - } else if (ret.length > 0 || startingGas - endingGas >= uint256(req.gasLimit10k) * 10000) { + } else if ( + ret.length > 0 || + (startingGas * 31) / 32 > uint256(req.gasLimit10k) * 10000 + ) { // The callback reverted for some reason. - // If ret.length == 0, then the callback ran out of gas. In this case, ensure that the - // callback was provided with sufficient gas. + // If ret.length > 0, then we know the callback manually triggered a revert, so it's safe to mark it as failed. + // If ret.length == 0, then the callback might have run out of gas (though there are other ways to trigger a revert with ret.length == 0). + // In this case, ensure that the callback was provided with sufficient gas. Technically, 63/64ths of the startingGas is forwarded, + // but we're using 31/32 to introduce a margin of safety. emit CallbackFailed( provider, req.requester, @@ -562,11 +569,11 @@ abstract contract Entropy is IEntropy, EntropyState { ); req.callbackStatus = EntropyStatusConstants.CALLBACK_FAILED; } else { - // Callback reverted by running out of gas, but the calling context did not have enough gas + // Callback reverted by (potentially) running out of gas, but the calling context did not have enough gas // to run the callback. This is a corner case that can happen due to the nuances of gas passing // in calls (see the comment on the call above). - // - // (Note that reverting here plays nicely with the estimateGas RPC method, which binary searches for + // + // (Note that reverting here plays nicely with the estimateGas RPC method, which binary searches for // the smallest gas value that causes the transaction to *succeed*. See https://github.com/ethereum/go-ethereum/pull/3587 ) revert EntropyErrors.InsufficientGas(); } @@ -640,11 +647,15 @@ abstract contract Entropy is IEntropy, EntropyState { ]; uint32 roundedGasLimit = uint32(roundGas(gasLimit)) * 10000; - if (roundedGasLimit > provider.defaultGasLimit) { + if ( + provider.defaultGasLimit > 0 && + roundedGasLimit > 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 = ((roundedGasLimit - provider.defaultGasLimit) * - provider.feeInWei) / provider.defaultGasLimit; + uint128 additionalFee = ((roundedGasLimit - + provider.defaultGasLimit) * provider.feeInWei) / + provider.defaultGasLimit; return provider.feeInWei + additionalFee; } else { return provider.feeInWei; @@ -777,9 +788,7 @@ abstract contract Entropy is IEntropy, EntropyState { // Rounds the provided quantity of gas into units of 10k gas. // If gas is not evenly divisible by 10k, rounds up. - function roundGas( - uint32 gas - ) public pure returns (uint16) { + function roundGas(uint32 gas) internal pure returns (uint16) { uint32 gas10k = gas / 10000; if (gas10k * 10000 < gas) { gas10k += 1; diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index cb92f04c61..a4be3dfcfc 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -1229,7 +1229,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { // Test the corner case caused by the CALL opcode passing at most 63/64ths of the current gas // to the sub-call. function testRequestWithCallbackUsingTooMuchGas2() public { - // With a 64M gas limit, we will pass ~63M gas to the callback (which is insufficient), but still + // With a 64M gas limit, we will pass ~63M gas to the callback (which is insufficient), but still // have ~1M gas to execute code within the revealWithCallback method, which should be enough to // run all of the logic subsequent to the excessivelySafeCall. uint32 defaultGasLimit = 64000000; @@ -1246,10 +1246,6 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { uint64 assignedSequenceNumber = consumer.requestEntropy{value: fee}( userRandomNumber ); - EntropyStructs.Request memory req = random.getRequest( - provider1, - assignedSequenceNumber - ); // The transaction reverts if the provider does not provide enough gas to forward // the gasLimit to the callback transaction. @@ -1573,32 +1569,66 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { assertEq(fee, random.getFee(provider1)); } - function testRoundGas() public { - // TODO: test this with the full request/reveal flow and update the max value + function testGasLimitsAndFeeRounding() public { + vm.prank(provider1); + random.setDefaultGasLimit(20000); + vm.prank(provider1); + random.setProviderFee(1); // Test exact multiples of 10,000 - assertEq(random.roundGas(0), 0); - assertEq(random.roundGas(10000), 1); - assertEq(random.roundGas(20000), 2); - assertEq(random.roundGas(100000), 10); + assertGasLimit10k(0, 0, 1); + assertGasLimit10k(10000, 1, 1); + assertGasLimit10k(20000, 2, 1); + assertGasLimit10k(100000, 10, 5); // Test values just below multiples of 10,000 - assertEq(random.roundGas(9999), 1); - assertEq(random.roundGas(19999), 2); - assertEq(random.roundGas(99999), 10); + assertGasLimit10k(9999, 1, 1); + assertGasLimit10k(19999, 2, 1); + assertGasLimit10k(39999, 4, 2); + assertGasLimit10k(99999, 10, 5); // Test values just above multiples of 10,000 - assertEq(random.roundGas(10001), 2); - assertEq(random.roundGas(20001), 3); - assertEq(random.roundGas(100001), 11); + assertGasLimit10k(10001, 2, 1); + assertGasLimit10k(20001, 3, 1); + assertGasLimit10k(100001, 11, 5); + assertGasLimit10k(110001, 12, 6); // Test middle values - assertEq(random.roundGas(5000), 1); - assertEq(random.roundGas(15000), 2); - assertEq(random.roundGas(25000), 3); + assertGasLimit10k(5000, 1, 1); + assertGasLimit10k(15000, 2, 1); + assertGasLimit10k(25000, 3, 1); - // Test maximum uint32 value - assertEq(random.roundGas(type(uint32).max), 429497); // (2^32 - 1) / 10000 rounded up + // Test maximum value + assertGasLimit10k( + uint32(type(uint16).max) * 10000, + type(uint16).max, + uint128(type(uint16).max) / 2 + ); + } + + // Helper method to create a request with a specific gas limit and check the gasLimit10k field + function assertGasLimit10k( + uint32 gasLimit, + uint16 expectedGasLimit10k, + uint128 expectedProviderFee + ) internal { + // Create a request with callback + bytes32 userRandomNumber = bytes32(uint(42)); + uint fee = random.getFeeForGas(provider1, gasLimit); + assertEq(fee - random.getPythFee(), expectedProviderFee); + + vm.deal(user1, fee); + vm.prank(user1); + uint64 sequenceNumber = random.requestWithCallbackAndGasLimit{ + value: fee + }(provider1, userRandomNumber, gasLimit); + + // Check the gasLimit10k field in the request + EntropyStructs.Request memory req = random.getRequest( + provider1, + sequenceNumber + ); + assertEq(req.gasLimit10k, expectedGasLimit10k); } } From 1fa8c14564ad25954380b6bb0e14f108e7e4ab7a Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Fri, 4 Apr 2025 08:44:43 -0700 Subject: [PATCH 08/15] cleaning up possible error case --- .../ethereum/contracts/contracts/entropy/Entropy.sol | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index 2c5c8861c2..ddf0d036bf 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -257,7 +257,13 @@ abstract contract Entropy is IEntropy, EntropyState { req.callbackStatus = isRequestWithCallback ? EntropyStatusConstants.CALLBACK_NOT_STARTED : EntropyStatusConstants.CALLBACK_NOT_NECESSARY; - req.gasLimit10k = roundGas(callbackGasLimit); + if (providerInfo.defaultGasLimit == 0) { + // Provider doesn't support the new callback failure state flow (toggled by setting the gas limit field). + // Set gasLimit10k to 0 to disable. + req.gasLimit10k = 0; + } else { + req.gasLimit10k = roundGas(callbackGasLimit); + } } // As a user, request a random number from `provider`. Prior to calling this method, the user should From 57d5ae20aad445bbc560e64979220e7c86781c9e Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Fri, 4 Apr 2025 09:50:53 -0700 Subject: [PATCH 09/15] improve tests --- .../contracts/contracts/entropy/Entropy.sol | 13 +- .../contracts/forge-test/Entropy.t.sol | 264 ++++++++++++------ .../entropy_sdk/solidity/EntropyErrors.sol | 2 + .../solidity/abis/EntropyErrors.json | 5 + 4 files changed, 196 insertions(+), 88 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index ddf0d036bf..f324c9201a 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -262,8 +262,17 @@ abstract contract Entropy is IEntropy, EntropyState { // Set gasLimit10k to 0 to disable. req.gasLimit10k = 0; } else { - req.gasLimit10k = roundGas(callbackGasLimit); - } + // This check does two important things: + // 1. Providers have a minimum fee set for their defaultGasLimit. If users request less gas than that, + // they still pay for the full gas limit. So we may as well give them the full limit here. + // 2. If a provider has a defaultGasLimit != 0, we need to ensure that all requests have a >0 gas limit + // so that we opt-in to the new callback failure state flow. + req.gasLimit10k = roundGas( + callbackGasLimit < providerInfo.defaultGasLimit + ? providerInfo.defaultGasLimit + : callbackGasLimit + ); + } } // As a user, request a random number from `provider`. Prior to calling this method, the user should diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index a4be3dfcfc..07aaf415a5 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -1457,6 +1457,12 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { provider1 ); assertEq(info.defaultGasLimit, newGasLimit); + + // Can reset back to 0. + vm.prank(provider1); + random.setDefaultGasLimit(0); + info = random.getProviderInfo(provider1); + assertEq(info.defaultGasLimit, 0); } function testSetDefaultGasLimitRevertIfNotFromProvider() public { @@ -1509,136 +1515,210 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { assertEq(req.gasLimit10k, 20); } - function testRequestWithCallbackAndGasLimitFeeScaling() public { - uint32 defaultGasLimit = 100000; - uint32 doubleGasLimit = 200000; - + function testGasLimitsAndFeeRounding() public { vm.prank(provider1); - random.setDefaultGasLimit(defaultGasLimit); + random.setDefaultGasLimit(20000); + vm.prank(provider1); + random.setProviderFee(1); - uint baseFee = random.getFee(provider1); - assertEq(baseFee, provider1FeeInWei + pythFeeInWei); + // Test exact multiples of 10,000 + assertGasLimitAndFee(0, 2, 1); + assertGasLimitAndFee(10000, 2, 1); + assertGasLimitAndFee(20000, 2, 1); + assertGasLimitAndFee(100000, 10, 5); - // Fee scales proportionally with gas limit - uint scaledFee = random.getFeeForGas(provider1, doubleGasLimit); - assertEq(scaledFee, 2 * provider1FeeInWei + pythFeeInWei); - } + // Test values just below multiples of 10,000 + assertGasLimitAndFee(9999, 2, 1); + assertGasLimitAndFee(19999, 2, 1); + assertGasLimitAndFee(39999, 4, 2); + assertGasLimitAndFee(99999, 10, 5); - function testRequestWithCallbackAndGasLimitInsufficientFee() public { - uint32 defaultGasLimit = 100000; - uint32 doubleGasLimit = 200000; + // Test values just above multiples of 10,000 + assertGasLimitAndFee(10001, 2, 1); + assertGasLimitAndFee(20001, 3, 1); + assertGasLimitAndFee(100001, 11, 5); + assertGasLimitAndFee(110001, 12, 6); + + // Test middle values + assertGasLimitAndFee(5000, 2, 1); + assertGasLimitAndFee(15000, 2, 1); + assertGasLimitAndFee(25000, 3, 1); + + // Test maximum value + assertGasLimitAndFee( + uint32(type(uint16).max) * 10000, + type(uint16).max, + uint128(type(uint16).max) / 2 + ); + // A provider with a 0 gas limit is opted-out of the failure state flow, indicated by + // a 0 gas limit on all requests. vm.prank(provider1); - random.setDefaultGasLimit(defaultGasLimit); + random.setDefaultGasLimit(0); + assertGasLimitAndFee(0, 0, 1); + assertGasLimitAndFee(10000, 0, 1); + assertGasLimitAndFee(20000, 0, 1); + assertGasLimitAndFee(100000, 0, 1); + } + + // Helper method to create a request with a specific gas limit and check the gasLimit10k / provider fees + function assertGasLimitAndFee( + uint32 gasLimit, + uint16 expectedGasLimit10k, + uint128 expectedProviderFee + ) internal { + // Create a request with callback bytes32 userRandomNumber = bytes32(uint(42)); - uint baseFee = random.getFee(provider1); // This is insufficient for double gas + uint fee = random.getFeeForGas(provider1, gasLimit); + assertEq(fee - random.getPythFee(), expectedProviderFee); - vm.deal(user1, baseFee); + // Passing 1 wei less than the expected fee causes a revert. + vm.deal(user1, fee); vm.prank(user1); vm.expectRevert(EntropyErrors.InsufficientFee.selector); - random.requestWithCallbackAndGasLimit{value: baseFee}( + random.requestWithCallbackAndGasLimit{value: fee - 1}( provider1, userRandomNumber, - doubleGasLimit + gasLimit ); - } - function testRequestWithCallbackAndGasLimitLowerThanDefault() public { - uint32 defaultGasLimit = 100000; - uint32 lowerGasLimit = 50000; - - vm.prank(provider1); - random.setDefaultGasLimit(defaultGasLimit); - - bytes32 userRandomNumber = bytes32(uint(42)); - uint fee = random.getFeeForGas(provider1, lowerGasLimit); - - vm.deal(user1, fee); + uint128 startingAccruedProviderFee = random + .getProviderInfo(provider1) + .accruedFeesInWei; vm.prank(user1); uint64 sequenceNumber = random.requestWithCallbackAndGasLimit{ value: fee - }(provider1, userRandomNumber, lowerGasLimit); + }(provider1, userRandomNumber, gasLimit); + assertEq( + random.getProviderInfo(provider1).accruedFeesInWei - + startingAccruedProviderFee, + expectedProviderFee + ); + + // Check the gasLimit10k field in the request EntropyStructs.Request memory req = random.getRequest( provider1, sequenceNumber ); - assertEq(req.gasLimit10k, 5); - // Fee should be the same as base fee since we're using less gas than default - assertEq(fee, random.getFee(provider1)); + assertEq(req.gasLimit10k, expectedGasLimit10k); } - function testGasLimitsAndFeeRounding() public { - vm.prank(provider1); - random.setDefaultGasLimit(20000); + function testCallbackProvidedGas() public { vm.prank(provider1); - random.setProviderFee(1); - - // Test exact multiples of 10,000 - assertGasLimit10k(0, 0, 1); - assertGasLimit10k(10000, 1, 1); - assertGasLimit10k(20000, 2, 1); - assertGasLimit10k(100000, 10, 5); + random.setDefaultGasLimit(200000); - // Test values just below multiples of 10,000 - assertGasLimit10k(9999, 1, 1); - assertGasLimit10k(19999, 2, 1); - assertGasLimit10k(39999, 4, 2); - assertGasLimit10k(99999, 10, 5); + assertCallbackResult(0, 190000, true); + assertCallbackResult(0, 210000, false); + assertCallbackResult(300000, 290000, true); + assertCallbackResult(300000, 310000, false); - // Test values just above multiples of 10,000 - assertGasLimit10k(10001, 2, 1); - assertGasLimit10k(20001, 3, 1); - assertGasLimit10k(100001, 11, 5); - assertGasLimit10k(110001, 12, 6); - - // Test middle values - assertGasLimit10k(5000, 1, 1); - assertGasLimit10k(15000, 2, 1); - assertGasLimit10k(25000, 3, 1); + // A provider that hasn't upgraded to the callback failure flow + // can never cause a callback to fail because it runs out of gas. + vm.prank(provider1); + random.setDefaultGasLimit(0); - // Test maximum value - assertGasLimit10k( - uint32(type(uint16).max) * 10000, - type(uint16).max, - uint128(type(uint16).max) / 2 - ); + assertCallbackResult(0, 190000, true); + assertCallbackResult(0, 210000, true); + assertCallbackResult(300000, 290000, true); + assertCallbackResult(300000, 310000, true); } - // Helper method to create a request with a specific gas limit and check the gasLimit10k field - function assertGasLimit10k( + // Helper method to assert whether a request with a specific gas limit / a callback with a specific gas cost + // should be successful or not. + function assertCallbackResult( uint32 gasLimit, - uint16 expectedGasLimit10k, - uint128 expectedProviderFee + uint32 callbackGasUsage, + bool expectSuccess ) internal { // Create a request with callback bytes32 userRandomNumber = bytes32(uint(42)); uint fee = random.getFeeForGas(provider1, gasLimit); - assertEq(fee - random.getPythFee(), expectedProviderFee); vm.deal(user1, fee); vm.prank(user1); - uint64 sequenceNumber = random.requestWithCallbackAndGasLimit{ - value: fee - }(provider1, userRandomNumber, gasLimit); + EntropyConsumer consumer = new EntropyConsumer(address(random), false); + uint64 sequenceNumber = consumer.requestEntropyWithGasLimit{value: fee}( + userRandomNumber, + gasLimit + ); + + consumer.setTargetGasUsage(callbackGasUsage); - // Check the gasLimit10k field in the request EntropyStructs.Request memory req = random.getRequest( provider1, sequenceNumber ); - assertEq(req.gasLimit10k, expectedGasLimit10k); + + if (!expectSuccess) { + vm.expectEmit(false, false, false, true, address(random)); + emit CallbackFailed( + provider1, + address(consumer), + sequenceNumber, + userRandomNumber, + provider1Proofs[sequenceNumber], + random.combineRandomValues( + userRandomNumber, + provider1Proofs[sequenceNumber], + 0 + ), + // out-of-gas reverts have an empty bytes array as the return value. + "" + ); + random.revealWithCallback( + provider1, + sequenceNumber, + userRandomNumber, + provider1Proofs[sequenceNumber] + ); + + // Verify request is still active after failure + EntropyStructs.Request memory reqAfterFailure = random.getRequest( + provider1, + sequenceNumber + ); + assertEq(reqAfterFailure.sequenceNumber, sequenceNumber); + assertEq( + reqAfterFailure.callbackStatus, + EntropyStatusConstants.CALLBACK_FAILED + ); + } else { + vm.expectEmit(false, false, false, true, address(random)); + emit RevealedWithCallback( + req, + userRandomNumber, + provider1Proofs[sequenceNumber], + random.combineRandomValues( + userRandomNumber, + provider1Proofs[sequenceNumber], + 0 + ) + ); + random.revealWithCallback( + provider1, + sequenceNumber, + userRandomNumber, + provider1Proofs[sequenceNumber] + ); + + // Verify request is cleared after successful callback + EntropyStructs.Request memory reqAfterSuccess = random.getRequest( + provider1, + sequenceNumber + ); + assertEq(reqAfterSuccess.sequenceNumber, 0); + } } } contract EntropyConsumer is IEntropyConsumer { uint64 public sequence; bytes32 public randomness; - address public entropy; address public provider; + address public entropy; bool public reverts; - uint256 public gasUsed; uint256 public targetGasUsage; constructor(address _entropy, bool _reverts) { @@ -1656,6 +1736,16 @@ contract EntropyConsumer is IEntropyConsumer { }(_provider, randomNumber); } + function requestEntropyWithGasLimit( + bytes32 randomNumber, + uint32 gasLimit + ) public payable returns (uint64 sequenceNumber) { + address _provider = IEntropy(entropy).getDefaultProvider(); + sequenceNumber = IEntropy(entropy).requestWithCallbackAndGasLimit{ + value: msg.value + }(_provider, randomNumber, gasLimit); + } + function getEntropy() internal view override returns (address) { return entropy; } @@ -1665,6 +1755,10 @@ contract EntropyConsumer is IEntropyConsumer { } function setTargetGasUsage(uint256 _targetGasUsage) public { + require( + _targetGasUsage > 60000, + "Target gas usage cannot be below 60k (~the cost of storing callback results)" + ); targetGasUsage = _targetGasUsage; } @@ -1674,22 +1768,20 @@ contract EntropyConsumer is IEntropyConsumer { bytes32 _randomness ) internal override { uint256 startGas = gasleft(); - uint256 currentGasUsed = 0; + + sequence = _sequence; + provider = _provider; + randomness = _randomness; // Keep consuming gas until we reach our target + uint256 currentGasUsed = startGas - gasleft(); while (currentGasUsed < targetGasUsage) { // Consume gas with a hash operation keccak256(abi.encodePacked(currentGasUsed, _randomness)); currentGasUsed = startGas - gasleft(); } - gasUsed = currentGasUsed; - - if (!reverts) { - sequence = _sequence; - provider = _provider; - randomness = _randomness; - } else { + if (reverts) { revert("Callback failed"); } } diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol index 56bc273d58..6122df8c10 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol @@ -45,4 +45,6 @@ library EntropyErrors { error UpdateTooOld(); // Not enough gas was provided to the function to execute the callback with the desired amount of gas. error InsufficientGas(); + // An argument to the function call was invalid or out of the expected range. + error InvalidArgument(); } diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json index 65d64c324a..36d202e6af 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json @@ -24,6 +24,11 @@ "name": "InsufficientGas", "type": "error" }, + { + "inputs": [], + "name": "InvalidArgument", + "type": "error" + }, { "inputs": [], "name": "InvalidRevealCall", From 6ec328a91419b2873abffc944093d9949090702c Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Fri, 4 Apr 2025 10:11:33 -0700 Subject: [PATCH 10/15] cleanup --- .../contracts/contracts/entropy/Entropy.sol | 2 +- .../entropy_sdk/solidity/EntropyErrors.sol | 2 -- .../entropy_sdk/solidity/EntropyEvents.sol | 5 ++--- .../entropy_sdk/solidity/EntropyStructs.sol | 5 +++-- .../entropy_sdk/solidity/IEntropy.sol | 21 ++++++++++++++++++- .../solidity/abis/EntropyErrors.json | 5 ----- .../solidity/abis/EntropyEvents.json | 8 +++---- .../entropy_sdk/solidity/abis/IEntropy.json | 8 +++---- 8 files changed, 34 insertions(+), 22 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index f324c9201a..499fd44c91 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -317,7 +317,7 @@ abstract contract Entropy is IEntropy, EntropyState { requestWithCallbackAndGasLimit( provider, userRandomNumber, - _state.providers[provider].defaultGasLimit + 0 // Passing 0 will assign the request the provider's default gas limit ); } diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol index 6122df8c10..56bc273d58 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol @@ -45,6 +45,4 @@ library EntropyErrors { error UpdateTooOld(); // Not enough gas was provided to the function to execute the callback with the desired amount of gas. error InsufficientGas(); - // An argument to the function call was invalid or out of the expected range. - error InvalidArgument(); } diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol index df42c5dc09..8e0b56b3de 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol @@ -51,11 +51,10 @@ interface EntropyEvents { event ProviderFeeUpdated(address provider, uint128 oldFee, uint128 newFee); - // TODO: uint32 event ProviderDefaultGasLimitUpdated( address indexed provider, - uint64 oldDefaultGasLimit, - uint64 newDefaultGasLimit + uint32 oldDefaultGasLimit, + uint32 newDefaultGasLimit ); event ProviderUriUpdated(address provider, bytes oldUri, bytes newUri); diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol index 5fd1b50c7e..a4ca3a6c45 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol @@ -63,8 +63,9 @@ contract EntropyStructs { bool useBlockhash; // Status flag for requests with callbacks. See EntropyConstants for the possible values of this flag. uint8 callbackStatus; - // 2 bytes of space left in this struct. - // The gasLimit in units of 10k gas. (i.e., 2 = 20k gas) + // The gasLimit in units of 10k gas. (i.e., 2 = 20k gas). We're using units of 10k in order to fit this + // field into the remaining 2 bytes of this storage slot. The dynamic range here is 10k - ~65M, which should + // cover all real-world use cases. uint16 gasLimit10k; } } diff --git a/target_chains/ethereum/entropy_sdk/solidity/IEntropy.sol b/target_chains/ethereum/entropy_sdk/solidity/IEntropy.sol index 9b0b69dc8a..9d1f13a8c6 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/IEntropy.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/IEntropy.sol @@ -48,14 +48,30 @@ interface IEntropy is EntropyEvents { // // The address calling this function should be a contract that inherits from the IEntropyConsumer interface. // The `entropyCallback` method on that interface will receive a callback with the generated random number. + // `entropyCallback` will be run with the provider's default gas limit (see `getProviderInfo(provider).defaultGasLimit`). + // If your callback needs additional gas, please use `requestWithCallbackAndGasLimit`. // - // This method will revert unless the caller provides a sufficient fee (at least getFee(provider)) as msg.value. + // This method will revert unless the caller provides a sufficient fee (at least `getFee(provider)`) as msg.value. // Note that excess value is *not* refunded to the caller. function requestWithCallback( address provider, bytes32 userRandomNumber ) external payable returns (uint64 assignedSequenceNumber); + // Request a random number from `provider`, getting a callback with the result. + // The caller must specify a provider to fulfill the request -- `getDefaultProvider()` is a sane default -- + // and a `userRandomNumber` to combine into the result. The method returns a sequence number which callers + // should save to correlate the request with the callback. + // + // The address calling this function should be a contract that inherits from the IEntropyConsumer interface. + // The `entropyCallback` method on that interface will receive a callback with the returned sequence number and + // the generated random number. `entropyCallback` will be run with the `gasLimit` provided to this function. + // The `gasLimit` will be rounded up to a multiple of 10k (e.g., 19000 -> 20000), and furthermore is lower bounded + // by the provider's configured default limit. + // + // This method will revert unless the caller provides a sufficient fee (at least `getFeeForGas(provider, gasLimit)`) as msg.value. + // Note that provider fees can change over time. Thus, callers of this method should explictly compute `getFeeForGas(provider, gasLimit)` + // prior to each invocation (as opposed to hardcoding a value). Further note that excess value is *not* refunded to the caller. function requestWithCallbackAndGasLimit( address provider, bytes32 userRandomNumber, @@ -104,8 +120,11 @@ interface IEntropy is EntropyEvents { uint64 sequenceNumber ) external view returns (EntropyStructs.Request memory req); + // Get the fee charged by provider for a request with the default gasLimit (`request` or `requestWithCallback`). + // If you are calling `requestWithCallbackAndGasLimit`, please use `getFeeForGas`. function getFee(address provider) external view returns (uint128 feeAmount); + // Get the fee charged by `provider` for a request with a specific `gasLimit` (`requestWithCallbackAndGasLimit`). function getFeeForGas( address provider, uint32 gasLimit diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json index 36d202e6af..65d64c324a 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json @@ -24,11 +24,6 @@ "name": "InsufficientGas", "type": "error" }, - { - "inputs": [], - "name": "InvalidArgument", - "type": "error" - }, { "inputs": [], "name": "InvalidRevealCall", diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json index 7149128c7d..cdbf17c917 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json @@ -108,15 +108,15 @@ }, { "indexed": false, - "internalType": "uint64", + "internalType": "uint32", "name": "oldDefaultGasLimit", - "type": "uint64" + "type": "uint32" }, { "indexed": false, - "internalType": "uint64", + "internalType": "uint32", "name": "newDefaultGasLimit", - "type": "uint64" + "type": "uint32" } ], "name": "ProviderDefaultGasLimitUpdated", diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json b/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json index 44f9e98802..2a2724873f 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json @@ -108,15 +108,15 @@ }, { "indexed": false, - "internalType": "uint64", + "internalType": "uint32", "name": "oldDefaultGasLimit", - "type": "uint64" + "type": "uint32" }, { "indexed": false, - "internalType": "uint64", + "internalType": "uint32", "name": "newDefaultGasLimit", - "type": "uint64" + "type": "uint32" } ], "name": "ProviderDefaultGasLimitUpdated", From 41284e40cf0e19e2627cad78a93d7660ec40d1be Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Fri, 4 Apr 2025 10:14:56 -0700 Subject: [PATCH 11/15] cleanup --- .../ethereum/contracts/contracts/entropy/Entropy.sol | 5 +++++ .../ethereum/entropy_sdk/solidity/EntropyEvents.sol | 10 ---------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index 499fd44c91..a5f5b925bd 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -661,6 +661,11 @@ abstract contract Entropy is IEntropy, EntropyState { providerAddr ]; + // Providers charge a minimum of their configured feeInWei for every request. + // Requests using more than the defaultGasLimit get a proportionally scaled fee. + // This approach may be somewhat simplistic, but it allows us to continue using the + // existing feeInWei parameter for the callback failure flow instead of defining new + // configuration values. uint32 roundedGasLimit = uint32(roundGas(gasLimit)) * 10000; if ( provider.defaultGasLimit > 0 && diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol index 8e0b56b3de..120cb0c858 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyEvents.sol @@ -39,16 +39,6 @@ interface EntropyEvents { bytes errorCode ); - event CallbackOutOfGas( - address indexed provider, - address indexed requestor, - uint64 indexed sequenceNumber, - bytes32 userRandomNumber, - bytes32 providerRevelation, - bytes32 randomNumber, - bytes errorCode - ); - event ProviderFeeUpdated(address provider, uint128 oldFee, uint128 newFee); event ProviderDefaultGasLimitUpdated( From 6e85db979b3d8c20dd0e74afe97370128cd2fdfa Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Fri, 4 Apr 2025 10:17:53 -0700 Subject: [PATCH 12/15] cleanup --- .../contracts/contracts/entropy/Entropy.sol | 6 ++- .../solidity/abis/EntropyEvents.json | 49 ------------------- .../entropy_sdk/solidity/abis/IEntropy.json | 49 ------------------- 3 files changed, 4 insertions(+), 100 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index a5f5b925bd..09ec3608e5 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -526,6 +526,8 @@ abstract contract Entropy is IEntropy, EntropyState { address callAddress = req.requester; + // If the request has an explicit gas limit, then run the new callback failure state flow. + // // Requests that haven't been invoked yet will be invoked safely (catching reverts), and // any reverts will be reported as an event. Any failing requests move to a failure state // at which point they can be recovered. The recovery flow invokes the callback directly @@ -661,10 +663,10 @@ abstract contract Entropy is IEntropy, EntropyState { providerAddr ]; - // Providers charge a minimum of their configured feeInWei for every request. + // Providers charge a minimum of their configured feeInWei for every request. // Requests using more than the defaultGasLimit get a proportionally scaled fee. // This approach may be somewhat simplistic, but it allows us to continue using the - // existing feeInWei parameter for the callback failure flow instead of defining new + // existing feeInWei parameter for the callback failure flow instead of defining new // configuration values. uint32 roundedGasLimit = uint32(roundGas(gasLimit)) * 10000; if ( diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json index cdbf17c917..f257fccf35 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyEvents.json @@ -48,55 +48,6 @@ "name": "CallbackFailed", "type": "event" }, - { - "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": "userRandomNumber", - "type": "bytes32" - }, - { - "indexed": false, - "internalType": "bytes32", - "name": "providerRevelation", - "type": "bytes32" - }, - { - "indexed": false, - "internalType": "bytes32", - "name": "randomNumber", - "type": "bytes32" - }, - { - "indexed": false, - "internalType": "bytes", - "name": "errorCode", - "type": "bytes" - } - ], - "name": "CallbackOutOfGas", - "type": "event" - }, { "anonymous": false, "inputs": [ diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json b/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json index 2a2724873f..3012caf605 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/IEntropy.json @@ -48,55 +48,6 @@ "name": "CallbackFailed", "type": "event" }, - { - "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": "userRandomNumber", - "type": "bytes32" - }, - { - "indexed": false, - "internalType": "bytes32", - "name": "providerRevelation", - "type": "bytes32" - }, - { - "indexed": false, - "internalType": "bytes32", - "name": "randomNumber", - "type": "bytes32" - }, - { - "indexed": false, - "internalType": "bytes", - "name": "errorCode", - "type": "bytes" - } - ], - "name": "CallbackOutOfGas", - "type": "event" - }, { "anonymous": false, "inputs": [ From c9a81ca6f64559358562bae0cd5aab75feb14ca3 Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Fri, 4 Apr 2025 10:23:24 -0700 Subject: [PATCH 13/15] final cleanup --- .../contracts/forge-test/Entropy.t.sol | 26 ++----------------- 1 file changed, 2 insertions(+), 24 deletions(-) diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index 07aaf415a5..57f8d107df 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -1492,29 +1492,6 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { assertEq(req.gasLimit10k, 10); } - function testRequestWithCallbackAndCustomGasLimit() public { - uint32 defaultGasLimit = 100000; - uint32 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.requestWithCallbackAndGasLimit{ - value: fee - }(provider1, userRandomNumber, customGasLimit); - - EntropyStructs.Request memory req = random.getRequest( - provider1, - sequenceNumber - ); - assertEq(req.gasLimit10k, 20); - } - function testGasLimitsAndFeeRounding() public { vm.prank(provider1); random.setDefaultGasLimit(20000); @@ -1768,7 +1745,8 @@ contract EntropyConsumer is IEntropyConsumer { bytes32 _randomness ) internal override { uint256 startGas = gasleft(); - + // These seemingly innocuous instructions are actually quite expensive + // (~60k gas) because they're writes to contract storage. sequence = _sequence; provider = _provider; randomness = _randomness; From 75a3c022a754b2166a43fb6e70e8969c4204bfc3 Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Tue, 8 Apr 2025 12:41:07 -0700 Subject: [PATCH 14/15] address PR comments --- .../contracts/contracts/entropy/Entropy.sol | 28 ++++++++++++++----- .../contracts/forge-test/Entropy.t.sol | 24 ++++++++++++++++ .../entropy_sdk/solidity/EntropyErrors.sol | 2 ++ .../entropy_sdk/solidity/EntropyStructs.sol | 2 +- .../solidity/abis/EntropyErrors.json | 5 ++++ 5 files changed, 53 insertions(+), 8 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index 09ec3608e5..8e9986715d 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -80,6 +80,10 @@ import "@pythnetwork/entropy-sdk-solidity/EntropyStatusConstants.sol"; abstract contract Entropy is IEntropy, EntropyState { using ExcessivelySafeCall for address; + uint32 public constant TEN_THOUSAND = 10000; + uint32 public constant MAX_GAS_LIMIT = + uint32(type(uint16).max) * TEN_THOUSAND; + function _initialize( address admin, uint128 pythFeeInWei, @@ -267,7 +271,7 @@ abstract contract Entropy is IEntropy, EntropyState { // they still pay for the full gas limit. So we may as well give them the full limit here. // 2. If a provider has a defaultGasLimit != 0, we need to ensure that all requests have a >0 gas limit // so that we opt-in to the new callback failure state flow. - req.gasLimit10k = roundGas( + req.gasLimit10k = roundTo10kGas( callbackGasLimit < providerInfo.defaultGasLimit ? providerInfo.defaultGasLimit : callbackGasLimit @@ -546,7 +550,7 @@ abstract contract Entropy is IEntropy, EntropyState { // than the indicated gas limit. (See CALL opcode docs here https://www.evm.codes/?fork=cancun#f1) // Consequently, out-of-gas reverts need to be handled carefully to ensure that the callback // was truly provided with a sufficient amount of gas. - uint256(req.gasLimit10k) * 10000, + uint256(req.gasLimit10k) * TEN_THOUSAND, 256, // copy at most 256 bytes of the return value into ret. abi.encodeWithSelector( IEntropyConsumer._entropyCallback.selector, @@ -568,7 +572,8 @@ abstract contract Entropy is IEntropy, EntropyState { clearRequest(provider, sequenceNumber); } else if ( ret.length > 0 || - (startingGas * 31) / 32 > uint256(req.gasLimit10k) * 10000 + (startingGas * 31) / 32 > + uint256(req.gasLimit10k) * TEN_THOUSAND ) { // The callback reverted for some reason. // If ret.length > 0, then we know the callback manually triggered a revert, so it's safe to mark it as failed. @@ -668,7 +673,7 @@ abstract contract Entropy is IEntropy, EntropyState { // This approach may be somewhat simplistic, but it allows us to continue using the // existing feeInWei parameter for the callback failure flow instead of defining new // configuration values. - uint32 roundedGasLimit = uint32(roundGas(gasLimit)) * 10000; + uint32 roundedGasLimit = uint32(roundTo10kGas(gasLimit)) * TEN_THOUSAND; if ( provider.defaultGasLimit > 0 && roundedGasLimit > provider.defaultGasLimit @@ -787,6 +792,10 @@ abstract contract Entropy is IEntropy, EntropyState { revert EntropyErrors.NoSuchProvider(); } + // Check that we can round the gas limit into the 10k gas. This reverts + // if the provided value exceeds the max. + roundTo10kGas(gasLimit); + uint32 oldGasLimit = provider.defaultGasLimit; provider.defaultGasLimit = gasLimit; emit ProviderDefaultGasLimitUpdated(msg.sender, oldGasLimit, gasLimit); @@ -810,11 +819,16 @@ abstract contract Entropy is IEntropy, EntropyState { // Rounds the provided quantity of gas into units of 10k gas. // If gas is not evenly divisible by 10k, rounds up. - function roundGas(uint32 gas) internal pure returns (uint16) { - uint32 gas10k = gas / 10000; - if (gas10k * 10000 < gas) { + function roundTo10kGas(uint32 gas) internal pure returns (uint16) { + if (gas > MAX_GAS_LIMIT) { + revert EntropyErrors.MaxGasLimitExceeded(); + } + + uint32 gas10k = gas / TEN_THOUSAND; + if (gas10k * TEN_THOUSAND < gas) { gas10k += 1; } + // Note: safe cast here should never revert due to the if statement above. return SafeCast.toUint16(gas10k); } diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index 57f8d107df..36c99de096 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -1463,6 +1463,19 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { random.setDefaultGasLimit(0); info = random.getProviderInfo(provider1); assertEq(info.defaultGasLimit, 0); + + // Can set to maximum value. + uint32 maxLimit = random.MAX_GAS_LIMIT(); + vm.prank(provider1); + random.setDefaultGasLimit(maxLimit); + info = random.getProviderInfo(provider1); + assertEq(info.defaultGasLimit, random.MAX_GAS_LIMIT()); + + // Reverts if max value is exceeded + uint32 exceedsGasLimit = random.MAX_GAS_LIMIT() + 1; + vm.prank(provider1); + vm.expectRevert(EntropyErrors.MaxGasLimitExceeded.selector); + random.setDefaultGasLimit(exceedsGasLimit); } function testSetDefaultGasLimitRevertIfNotFromProvider() public { @@ -1528,6 +1541,17 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { uint128(type(uint16).max) / 2 ); + // Test larger than max value reverts with expected error + uint32 exceedsGasLimit = uint32(type(uint16).max) * 10000 + 1; + vm.expectRevert(EntropyErrors.MaxGasLimitExceeded.selector); + random.getFeeForGas(provider1, exceedsGasLimit); + vm.expectRevert(EntropyErrors.MaxGasLimitExceeded.selector); + random.requestWithCallbackAndGasLimit{value: 10000000000000}( + provider1, + bytes32(uint(42)), + exceedsGasLimit + ); + // A provider with a 0 gas limit is opted-out of the failure state flow, indicated by // a 0 gas limit on all requests. vm.prank(provider1); diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol index 56bc273d58..fb238f8382 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol @@ -45,4 +45,6 @@ library EntropyErrors { error UpdateTooOld(); // Not enough gas was provided to the function to execute the callback with the desired amount of gas. error InsufficientGas(); + // A gas limit value was provided that was greater than the maximum possible limit of 655,350,000 + error MaxGasLimitExceeded(); } diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol index a4ca3a6c45..002ab7be20 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol @@ -64,7 +64,7 @@ contract EntropyStructs { // Status flag for requests with callbacks. See EntropyConstants for the possible values of this flag. uint8 callbackStatus; // The gasLimit in units of 10k gas. (i.e., 2 = 20k gas). We're using units of 10k in order to fit this - // field into the remaining 2 bytes of this storage slot. The dynamic range here is 10k - ~65M, which should + // field into the remaining 2 bytes of this storage slot. The dynamic range here is 10k - 655M, which should // cover all real-world use cases. uint16 gasLimit10k; } diff --git a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json index 65d64c324a..f5a5faf248 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json +++ b/target_chains/ethereum/entropy_sdk/solidity/abis/EntropyErrors.json @@ -39,6 +39,11 @@ "name": "LastRevealedTooOld", "type": "error" }, + { + "inputs": [], + "name": "MaxGasLimitExceeded", + "type": "error" + }, { "inputs": [], "name": "NoSuchProvider", From 790d057e4675595ff03386f3313ac7110cad289b Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Tue, 15 Apr 2025 07:22:03 -0700 Subject: [PATCH 15/15] test emit event --- target_chains/ethereum/contracts/forge-test/Entropy.t.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index 36c99de096..a2638e6de1 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -1451,6 +1451,8 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { uint32 newGasLimit = 100000; vm.prank(provider1); + vm.expectEmit(false, false, false, true, address(random)); + emit ProviderDefaultGasLimitUpdated(provider1, 0, newGasLimit); random.setDefaultGasLimit(newGasLimit); EntropyStructs.ProviderInfo memory info = random.getProviderInfo(