Generate unique Functions requestIds for reorg'ed requests (#10891)

* compute requestId using hash of commitment * fixed tests * prettier * removed extra line * Added test assertions * Update functions.gas-snapshot * Update functions.gas-snapshot * Update functions.gas-snapshot * Reduce gas use for calculating unique requestId * Extend hash to cover more fields * Add solhint ignore for tx.origin * Gas Golf: -86k from maximal request * Regenerate geth wrappers * Restore foundry-lib --------- Co-authored-by: Bolek Kulbabinski <[email protected]> Co-authored-by: Justin Kaseman <[email protected]>
smartcontractkit · Oct 18, 2023 · 231f2ff · 231f2ff
1 parent d9afbbd
commit 231f2ff
Show file tree

Hide file tree

Showing 5 changed files with 63 additions and 80 deletions.
diff --git a/contracts/gas-snapshots/functions.gas-snapshot b/contracts/gas-snapshots/functions.gas-snapshot
@@ -31,8 +31,8 @@ FunctionsRouter_Fulfill:test_Fulfill_RevertIfNotCommittedCoordinator() (gas: 280
 FunctionsRouter_Fulfill:test_Fulfill_RevertIfPaused() (gas: 153900)
 FunctionsRouter_Fulfill:test_Fulfill_SuccessClientNoLongerExists() (gas: 296711)
 FunctionsRouter_Fulfill:test_Fulfill_SuccessFulfilled() (gas: 310303)
-FunctionsRouter_Fulfill:test_Fulfill_SuccessUserCallbackReverts() (gas: 2484625)
-FunctionsRouter_Fulfill:test_Fulfill_SuccessUserCallbackRunsOutOfGas() (gas: 515110)
+FunctionsRouter_Fulfill:test_Fulfill_SuccessUserCallbackReverts() (gas: 2484943)
+FunctionsRouter_Fulfill:test_Fulfill_SuccessUserCallbackRunsOutOfGas() (gas: 515428)
 FunctionsRouter_GetAdminFee:test_GetAdminFee_Success() (gas: 18005)
 FunctionsRouter_GetAllowListId:test_GetAllowListId_Success() (gas: 12926)
 FunctionsRouter_GetConfig:test_GetConfig_Success() (gas: 37136)
@@ -53,23 +53,23 @@ FunctionsRouter_ProposeContractsUpdate:test_ProposeContractsUpdate_RevertIfNotNe
 FunctionsRouter_ProposeContractsUpdate:test_ProposeContractsUpdate_RevertIfNotOwner() (gas: 23369)
 FunctionsRouter_ProposeContractsUpdate:test_ProposeContractsUpdate_Success() (gas: 118456)
 FunctionsRouter_SendRequest:test_SendRequest_RevertIfConsumerNotAllowed() (gas: 59304)
-FunctionsRouter_SendRequest:test_SendRequest_RevertIfDuplicateRequestId() (gas: 192478)
+FunctionsRouter_SendRequest:test_SendRequest_RevertIfDuplicateRequestId() (gas: 192796)
 FunctionsRouter_SendRequest:test_SendRequest_RevertIfEmptyData() (gas: 29405)
 FunctionsRouter_SendRequest:test_SendRequest_RevertIfIncorrectDonId() (gas: 57926)
 FunctionsRouter_SendRequest:test_SendRequest_RevertIfInsufficientSubscriptionBalance() (gas: 186209)
 FunctionsRouter_SendRequest:test_SendRequest_RevertIfInvalidCallbackGasLimit() (gas: 50902)
 FunctionsRouter_SendRequest:test_SendRequest_RevertIfInvalidDonId() (gas: 25061)
 FunctionsRouter_SendRequest:test_SendRequest_RevertIfNoSubscription() (gas: 29111)
 FunctionsRouter_SendRequest:test_SendRequest_RevertIfPaused() (gas: 34247)
-FunctionsRouter_SendRequest:test_SendRequest_Success() (gas: 198072)
+FunctionsRouter_SendRequest:test_SendRequest_Success() (gas: 284999)
 FunctionsRouter_SendRequestToProposed:test_SendRequestToProposed_RevertIfConsumerNotAllowed() (gas: 65800)
 FunctionsRouter_SendRequestToProposed:test_SendRequestToProposed_RevertIfEmptyData() (gas: 35991)
 FunctionsRouter_SendRequestToProposed:test_SendRequestToProposed_RevertIfIncorrectDonId() (gas: 29897)
 FunctionsRouter_SendRequestToProposed:test_SendRequestToProposed_RevertIfInvalidCallbackGasLimit() (gas: 57488)
 FunctionsRouter_SendRequestToProposed:test_SendRequestToProposed_RevertIfInvalidDonId() (gas: 27482)
 FunctionsRouter_SendRequestToProposed:test_SendRequestToProposed_RevertIfNoSubscription() (gas: 35696)
 FunctionsRouter_SendRequestToProposed:test_SendRequestToProposed_RevertIfPaused() (gas: 40766)
-FunctionsRouter_SendRequestToProposed:test_SendRequestToProposed_Success() (gas: 204630)
+FunctionsRouter_SendRequestToProposed:test_SendRequestToProposed_Success() (gas: 291551)
 FunctionsRouter_SendRequestToProposed:test_SendRequest_RevertIfInsufficientSubscriptionBalance() (gas: 192728)
 FunctionsRouter_SetAllowListId:test_SetAllowListId_Success() (gas: 30687)
 FunctionsRouter_SetAllowListId:test_UpdateConfig_RevertIfNotOwner() (gas: 13380)
@@ -83,7 +83,7 @@ FunctionsSubscriptions_AcceptSubscriptionOwnerTransfer:test_AcceptSubscriptionOw
 FunctionsSubscriptions_AcceptSubscriptionOwnerTransfer:test_AcceptSubscriptionOwnerTransfer_RevertIfPaused() (gas: 60962)
 FunctionsSubscriptions_AcceptSubscriptionOwnerTransfer:test_AcceptSubscriptionOwnerTransfer_RevertIfSenderBecomesBlocked() (gas: 94675)
 FunctionsSubscriptions_AcceptSubscriptionOwnerTransfer:test_AcceptSubscriptionOwnerTransfer_RevertIfSenderIsNotNewOwner() (gas: 62691)
-FunctionsSubscriptions_AcceptSubscriptionOwnerTransfer:test_AcceptSubscriptionOwnerTransfer_Success() (gas: 214258)
+FunctionsSubscriptions_AcceptSubscriptionOwnerTransfer:test_AcceptSubscriptionOwnerTransfer_Success() (gas: 214576)
 FunctionsSubscriptions_AddConsumer:test_AddConsumer_RevertIfMaximumConsumers() (gas: 137833)
 FunctionsSubscriptions_AddConsumer:test_AddConsumer_RevertIfMaximumConsumersAfterConfigUpdate() (gas: 164777)
 FunctionsSubscriptions_AddConsumer:test_AddConsumer_RevertIfNoSubscription() (gas: 12926)
@@ -95,10 +95,10 @@ FunctionsSubscriptions_CancelSubscription:test_CancelSubscription_RevertIfNoSubs
 FunctionsSubscriptions_CancelSubscription:test_CancelSubscription_RevertIfNotAllowedSender() (gas: 57929)
 FunctionsSubscriptions_CancelSubscription:test_CancelSubscription_RevertIfNotSubscriptionOwner() (gas: 89316)
 FunctionsSubscriptions_CancelSubscription:test_CancelSubscription_RevertIfPaused() (gas: 20191)
-FunctionsSubscriptions_CancelSubscription:test_CancelSubscription_RevertIfPendingRequests() (gas: 193445)
+FunctionsSubscriptions_CancelSubscription:test_CancelSubscription_RevertIfPendingRequests() (gas: 193763)
 FunctionsSubscriptions_CancelSubscription:test_CancelSubscription_SuccessForfeitAllBalanceAsDeposit() (gas: 114636)
 FunctionsSubscriptions_CancelSubscription:test_CancelSubscription_SuccessForfeitSomeBalanceAsDeposit() (gas: 125891)
-FunctionsSubscriptions_CancelSubscription:test_CancelSubscription_SuccessRecieveDeposit() (gas: 311168)
+FunctionsSubscriptions_CancelSubscription:test_CancelSubscription_SuccessRecieveDeposit() (gas: 311486)
 FunctionsSubscriptions_Constructor:test_Constructor_Success() (gas: 7654)
 FunctionsSubscriptions_CreateSubscriptionWithConsumer:test_CreateSubscriptionWithConsumer_RevertIfNotAllowedSender() (gas: 28637)
 FunctionsSubscriptions_CreateSubscriptionWithConsumer:test_CreateSubscriptionWithConsumer_RevertIfPaused() (gas: 17948)
@@ -128,7 +128,7 @@ FunctionsSubscriptions_OwnerCancelSubscription:test_OwnerCancelSubscription_Reve
 FunctionsSubscriptions_OwnerCancelSubscription:test_OwnerCancelSubscription_Success() (gas: 54867)
 FunctionsSubscriptions_OwnerCancelSubscription:test_OwnerCancelSubscription_SuccessDeletesSubscription() (gas: 49624)
 FunctionsSubscriptions_OwnerCancelSubscription:test_OwnerCancelSubscription_SuccessSubOwnerRefunded() (gas: 50896)
-FunctionsSubscriptions_OwnerCancelSubscription:test_OwnerCancelSubscription_SuccessWhenRequestInFlight() (gas: 164045)
+FunctionsSubscriptions_OwnerCancelSubscription:test_OwnerCancelSubscription_SuccessWhenRequestInFlight() (gas: 164300)
 FunctionsSubscriptions_OwnerWithdraw:test_OwnerWithdraw_RevertIfAmountMoreThanBalance() (gas: 17924)
 FunctionsSubscriptions_OwnerWithdraw:test_OwnerWithdraw_RevertIfBalanceInvariant() (gas: 210)
 FunctionsSubscriptions_OwnerWithdraw:test_OwnerWithdraw_RevertIfNotOwner() (gas: 15533)
@@ -137,7 +137,7 @@ FunctionsSubscriptions_OwnerWithdraw:test_OwnerWithdraw_SuccessIfRecipientAddres
 FunctionsSubscriptions_OwnerWithdraw:test_OwnerWithdraw_SuccessPaysRecipient() (gas: 54413)
 FunctionsSubscriptions_OwnerWithdraw:test_OwnerWithdraw_SuccessSetsBalanceToZero() (gas: 37790)
 FunctionsSubscriptions_PendingRequestExists:test_PendingRequestExists_SuccessFalseIfNoPendingRequests() (gas: 15025)
-FunctionsSubscriptions_PendingRequestExists:test_PendingRequestExists_SuccessTrueIfPendingRequests() (gas: 175579)
+FunctionsSubscriptions_PendingRequestExists:test_PendingRequestExists_SuccessTrueIfPendingRequests() (gas: 175897)
 FunctionsSubscriptions_ProposeSubscriptionOwnerTransfer:test_ProposeSubscriptionOwnerTransfer_RevertIfEmptyNewOwner() (gas: 27610)
 FunctionsSubscriptions_ProposeSubscriptionOwnerTransfer:test_ProposeSubscriptionOwnerTransfer_RevertIfInvalidNewOwner() (gas: 57707)
 FunctionsSubscriptions_ProposeSubscriptionOwnerTransfer:test_ProposeSubscriptionOwnerTransfer_RevertIfNoSubscription() (gas: 15000)
@@ -153,7 +153,7 @@ FunctionsSubscriptions_RemoveConsumer:test_RemoveConsumer_RevertIfNoSubscription
 FunctionsSubscriptions_RemoveConsumer:test_RemoveConsumer_RevertIfNotAllowedSender() (gas: 57778)
 FunctionsSubscriptions_RemoveConsumer:test_RemoveConsumer_RevertIfNotSubscriptionOwner() (gas: 87186)
 FunctionsSubscriptions_RemoveConsumer:test_RemoveConsumer_RevertIfPaused() (gas: 18004)
-FunctionsSubscriptions_RemoveConsumer:test_RemoveConsumer_RevertIfPendingRequests() (gas: 190877)
+FunctionsSubscriptions_RemoveConsumer:test_RemoveConsumer_RevertIfPendingRequests() (gas: 191195)
 FunctionsSubscriptions_RemoveConsumer:test_RemoveConsumer_Success() (gas: 41979)
 FunctionsSubscriptions_SetFlags:test_SetFlags_RevertIfNoSubscription() (gas: 12847)
 FunctionsSubscriptions_SetFlags:test_SetFlags_RevertIfNotOwner() (gas: 15640)
@@ -191,5 +191,5 @@ Gas_AcceptTermsOfService:test_AcceptTermsOfService_Gas() (gas: 84675)
 Gas_AddConsumer:test_AddConsumer_Gas() (gas: 79067)
 Gas_CreateSubscription:test_CreateSubscription_Gas() (gas: 73353)
 Gas_FundSubscription:test_FundSubscription_Gas() (gas: 38501)
-Gas_SendRequest:test_SendRequest_MaximumGas() (gas: 958243)
-Gas_SendRequest:test_SendRequest_MinimumGas() (gas: 156605)
+Gas_SendRequest:test_SendRequest_MaximumGas() (gas: 964209)
+Gas_SendRequest:test_SendRequest_MinimumGas() (gas: 156929)
diff --git a/contracts/src/v0.8/functions/dev/v1_X/FunctionsBilling.sol b/contracts/src/v0.8/functions/dev/v1_X/FunctionsBilling.sol
@@ -208,11 +208,21 @@ abstract contract FunctionsBilling is Routable, IFunctionsBilling {
       revert InsufficientBalance();
     }
 
-    bytes32 requestId = _computeRequestId(
-      address(this),
-      request.requestingContract,
-      request.subscriptionId,
-      request.initiatedRequests + 1
+    uint32 timeoutTimestamp = uint32(block.timestamp + config.requestTimeoutSeconds);
+    bytes32 requestId = keccak256(
+      abi.encode(
+        address(this),
+        request.requestingContract,
+        request.subscriptionId,
+        request.initiatedRequests + 1,
+        keccak256(request.data),
+        request.dataVersion,
+        request.callbackGasLimit,
+        estimatedTotalCostJuels,
+        timeoutTimestamp,
+        // solhint-disable-next-line avoid-tx-origin
+        tx.origin
+      )
     );
 
     commitment = FunctionsResponse.Commitment({
@@ -222,7 +232,7 @@ abstract contract FunctionsBilling is Routable, IFunctionsBilling {
       subscriptionId: request.subscriptionId,
       callbackGasLimit: request.callbackGasLimit,
       estimatedTotalCostJuels: estimatedTotalCostJuels,
-      timeoutTimestamp: uint32(block.timestamp + config.requestTimeoutSeconds),
+      timeoutTimestamp: timeoutTimestamp,
       requestId: requestId,
       donFee: donFee,
       gasOverheadBeforeCallback: config.gasOverheadBeforeCallback,
@@ -234,17 +244,6 @@ abstract contract FunctionsBilling is Routable, IFunctionsBilling {
     return commitment;
   }
 
-  /// @notice Generate a keccak hash request ID
-  /// @dev uses the number of requests that the consumer of a subscription has sent as a nonce
-  function _computeRequestId(
-    address don,
-    address client,
-    uint64 subscriptionId,
-    uint64 nonce
-  ) private pure returns (bytes32) {
-    return keccak256(abi.encode(don, client, subscriptionId, nonce));
-  }
-
   /// @notice Finalize billing process for an Functions request by sending a callback to the Client contract and then charging the subscription
   /// @param requestId identifier for the request that was generated by the Registry in the beginBilling commitment
   /// @param response response data from DON consensus

diff --git a/contracts/src/v0.8/functions/tests/v1_X/FunctionsRouter.t.sol b/contracts/src/v0.8/functions/tests/v1_X/FunctionsRouter.t.sol
@@ -464,10 +464,6 @@ contract FunctionsRouter_SendRequest is FunctionsSubscriptionSetup {
 
     uint32 callbackGasLimit = 5000;
 
-    bytes32 expectedRequestId = keccak256(
-      abi.encode(address(s_functionsCoordinator), OWNER_ADDRESS, s_subscriptionId, 1)
-    );
-
     uint96 costEstimate = s_functionsCoordinator.estimateCost(
       s_subscriptionId,
       requestData,
@@ -477,25 +473,6 @@ contract FunctionsRouter_SendRequest is FunctionsSubscriptionSetup {
 
     vm.recordLogs();
 
-    // topic0 (function signature, always checked), topic1 (true), topic2 (true), topic3 (true), and data (true).
-    bool checkTopic1 = true;
-    bool checkTopic2 = true;
-    bool checkTopic3 = true;
-    bool checkData = true;
-    vm.expectEmit(checkTopic1, checkTopic2, checkTopic3, checkData);
-    emit RequestStart({
-      requestId: expectedRequestId,
-      donId: s_donId,
-      subscriptionId: s_subscriptionId,
-      subscriptionOwner: OWNER_ADDRESS,
-      requestingContract: OWNER_ADDRESS,
-      requestInitiator: OWNER_ADDRESS,
-      data: requestData,
-      dataVersion: FunctionsRequest.REQUEST_DATA_VERSION,
-      callbackGasLimit: callbackGasLimit,
-      estimatedTotalCostJuels: costEstimate
-    });
-
     bytes32 requestIdFromReturn = s_functionsRouter.sendRequest(
       s_subscriptionId,
       requestData,
@@ -506,9 +483,24 @@ contract FunctionsRouter_SendRequest is FunctionsSubscriptionSetup {
 
     // Get requestId from RequestStart event log topic 1
     Vm.Log[] memory entries = vm.getRecordedLogs();
-    bytes32 requestIdFromEvent = entries[2].topics[1];
+    bytes32 requestIdFromEvent = entries[1].topics[1];
+    bytes32 donIdFromEvent = entries[1].topics[2];
+    bytes32 subscriptionIdFromEvent = entries[1].topics[3];
+
+    bytes memory expectedRequestData = abi.encode(
+      OWNER_ADDRESS,
+      OWNER_ADDRESS,
+      OWNER_ADDRESS,
+      requestData,
+      FunctionsRequest.REQUEST_DATA_VERSION,
+      callbackGasLimit,
+      costEstimate
+    );
 
     assertEq(requestIdFromReturn, requestIdFromEvent);
+    assertEq(donIdFromEvent, s_donId);
+    assertEq(subscriptionIdFromEvent, bytes32(uint256(s_subscriptionId)));
+    assertEq(expectedRequestData, entries[1].data);
   }
 }
 
@@ -759,10 +751,6 @@ contract FunctionsRouter_SendRequestToProposed is FunctionsSubscriptionSetup {
 
     uint32 callbackGasLimit = 5000;
 
-    bytes32 expectedRequestId = keccak256(
-      abi.encode(address(s_functionsCoordinator2), OWNER_ADDRESS, s_subscriptionId, 1)
-    );
-
     uint96 costEstimate = s_functionsCoordinator2.estimateCost(
       s_subscriptionId,
       requestData,
@@ -772,25 +760,6 @@ contract FunctionsRouter_SendRequestToProposed is FunctionsSubscriptionSetup {
 
     vm.recordLogs();
 
-    // topic0 (function signature, always checked), topic1 (true), topic2 (true), topic3 (true), and data (true).
-    bool checkTopic1 = true;
-    bool checkTopic2 = true;
-    bool checkTopic3 = true;
-    bool checkData = true;
-    vm.expectEmit(checkTopic1, checkTopic2, checkTopic3, checkData);
-    emit RequestStart({
-      requestId: expectedRequestId,
-      donId: s_donId,
-      subscriptionId: s_subscriptionId,
-      subscriptionOwner: OWNER_ADDRESS,
-      requestingContract: OWNER_ADDRESS,
-      requestInitiator: OWNER_ADDRESS,
-      data: requestData,
-      dataVersion: FunctionsRequest.REQUEST_DATA_VERSION,
-      callbackGasLimit: callbackGasLimit,
-      estimatedTotalCostJuels: costEstimate
-    });
-
     bytes32 requestIdFromReturn = s_functionsRouter.sendRequestToProposed(
       s_subscriptionId,
       requestData,
@@ -801,9 +770,24 @@ contract FunctionsRouter_SendRequestToProposed is FunctionsSubscriptionSetup {
 
     // Get requestId from RequestStart event log topic 1
     Vm.Log[] memory entries = vm.getRecordedLogs();
-    bytes32 requestIdFromEvent = entries[2].topics[1];
+    bytes32 requestIdFromEvent = entries[1].topics[1];
+    bytes32 donIdFromEvent = entries[1].topics[2];
+    bytes32 subscriptionIdFromEvent = entries[1].topics[3];
+
+    bytes memory expectedRequestData = abi.encode(
+      OWNER_ADDRESS,
+      OWNER_ADDRESS,
+      OWNER_ADDRESS,
+      requestData,
+      FunctionsRequest.REQUEST_DATA_VERSION,
+      callbackGasLimit,
+      costEstimate
+    );
 
     assertEq(requestIdFromReturn, requestIdFromEvent);
+    assertEq(donIdFromEvent, s_donId);
+    assertEq(subscriptionIdFromEvent, bytes32(uint256(s_subscriptionId)));
+    assertEq(expectedRequestData, entries[1].data);
   }
 }