refactor dedup protection logic (#9764)

contracts/src/v0.8/dev/automation/2_1/KeeperRegistry2_1.sol

@@ -96,13 +96,10 @@ contract KeeperRegistry2_1 is KeeperRegistryBase2_1, OCR2Abstract, Chainable, ER
         true
       );
       upkeepTransmitInfo[i].triggerID = _triggerID(report.upkeepIds[i], report.triggers[i]);
-      upkeepTransmitInfo[i].earlyChecksPassed = _prePerformChecks(
+      (upkeepTransmitInfo[i].earlyChecksPassed, upkeepTransmitInfo[i].dedupID) = _prePerformChecks(
         report.upkeepIds[i],
-        upkeepTransmitInfo[i].triggerType,
-        upkeepTransmitInfo[i].triggerID,
         report.triggers[i],
-        upkeepTransmitInfo[i].upkeep,
-        upkeepTransmitInfo[i].maxLinkPayment
+        upkeepTransmitInfo[i]
       );
 
       if (upkeepTransmitInfo[i].earlyChecksPassed) {
@@ -121,8 +118,8 @@ contract KeeperRegistry2_1 is KeeperRegistryBase2_1, OCR2Abstract, Chainable, ER
       // Deduct that gasUsed by upkeep from our running counter
       gasOverhead -= upkeepTransmitInfo[i].gasUsed;
 
-      // Store last perform block number for upkeep
-      _updateLastPerformed(report.upkeepIds[i], upkeepTransmitInfo[i].triggerType);
+      // Store last perform block number / deduping key for upkeep
+      _updateTriggerMarker(report.upkeepIds[i], upkeepTransmitInfo[i]);
     }
     // No upkeeps to be performed in this report
     if (numUpkeepsPassedChecks == 0) {
@@ -401,51 +398,52 @@ contract KeeperRegistry2_1 is KeeperRegistryBase2_1, OCR2Abstract, Chainable, ER
 
   /**
    * @dev Does some early sanity checks before actually performing an upkeep
+   * @return bool whether the upkeep should be performed
+   * @return bytes32 dedupID for preventing duplicate performances of this trigger
    */
   function _prePerformChecks(
     uint256 upkeepId,
-    Trigger triggerType,
-    bytes32 triggerID,
     bytes memory rawTrigger,
-    Upkeep memory upkeep,
-    uint96 maxLinkPayment
-  ) internal returns (bool) {
-    if (triggerType == Trigger.CONDITION) {
-      if (!_validateConditionalTrigger(upkeepId, triggerID, rawTrigger, upkeep)) return false;
-    } else if (triggerType == Trigger.LOG) {
-      if (!_validateLogTrigger(upkeepId, triggerID, rawTrigger)) return false;
+    UpkeepTransmitInfo memory transmitInfo
+  ) internal returns (bool, bytes32) {
+    bytes32 dedupID;
+    if (transmitInfo.triggerType == Trigger.CONDITION) {
+      if (!_validateConditionalTrigger(upkeepId, rawTrigger, transmitInfo)) return (false, dedupID);
+    } else if (transmitInfo.triggerType == Trigger.LOG) {
+      bool valid;
+      (valid, dedupID) = _validateLogTrigger(upkeepId, rawTrigger, transmitInfo);
+      if (!valid) return (false, dedupID);
     } else {
       revert InvalidTriggerType();
     }
-    if (upkeep.maxValidBlocknumber <= _blockNum()) {
+    if (transmitInfo.upkeep.maxValidBlocknumber <= _blockNum()) {
       // Can happen when an upkeep got cancelled after report was generated.
       // However we have a CANCELLATION_DELAY of 50 blocks so shouldn't happen in practice
-      emit CancelledUpkeepReport(upkeepId, triggerID);
-      return false;
+      emit CancelledUpkeepReport(upkeepId, transmitInfo.triggerID);
+      return (false, dedupID);
     }
 
-    if (upkeep.balance < maxLinkPayment) {
+    if (transmitInfo.upkeep.balance < transmitInfo.maxLinkPayment) {
       // Can happen due to flucutations in gas / link prices
-      emit InsufficientFundsUpkeepReport(upkeepId, triggerID);
-      return false;
+      emit InsufficientFundsUpkeepReport(upkeepId, transmitInfo.triggerID);
+      return (false, dedupID);
     }
 
-    return true;
+    return (true, dedupID);
   }
 
   /**
    * @dev Does some early sanity checks before actually performing an upkeep
    */
   function _validateConditionalTrigger(
     uint256 upkeepId,
-    bytes32 triggerID,
     bytes memory rawTrigger,
-    Upkeep memory upkeep
+    UpkeepTransmitInfo memory transmitInfo
   ) internal returns (bool) {
     ConditionalTrigger memory trigger = abi.decode(rawTrigger, (ConditionalTrigger));
-    if (trigger.blockNum < upkeep.lastPerformedBlockNumber) {
+    if (trigger.blockNum < transmitInfo.upkeep.lastPerformedBlockNumber) {
       // Can happen when another report performed this upkeep after this report was generated
-      emit StaleUpkeepReport(upkeepId, triggerID);
+      emit StaleUpkeepReport(upkeepId, transmitInfo.triggerID);
       return false;
     }
     if (
@@ -458,29 +456,32 @@ contract KeeperRegistry2_1 is KeeperRegistryBase2_1, OCR2Abstract, Chainable, ER
       // 2. blockHash at trigger block number was same as trigger time. This is an optional check which is
       // applied if DON sends non empty trigger.blockHash. Note: It only works for last 256 blocks on chain
       // when it is sent
-      emit ReorgedUpkeepReport(upkeepId, triggerID);
+      emit ReorgedUpkeepReport(upkeepId, transmitInfo.triggerID);
       return false;
     }
     return true;
   }
 
-  function _validateLogTrigger(uint256 upkeepId, bytes32 triggerID, bytes memory rawTrigger) internal returns (bool) {
+  function _validateLogTrigger(
+    uint256 upkeepId,
+    bytes memory rawTrigger,
+    UpkeepTransmitInfo memory transmitInfo
+  ) internal returns (bool, bytes32) {
     LogTrigger memory trigger = abi.decode(rawTrigger, (LogTrigger));
+    bytes32 dedupID = keccak256(abi.encodePacked(upkeepId, trigger.txHash, trigger.logIndex));
     if (
       (trigger.blockHash != bytes32("") && _blockHash(trigger.blockNum) != trigger.blockHash) ||
       trigger.blockNum >= _blockNum()
     ) {
       // Reorg protection is same as conditional trigger upkeeps
-      emit ReorgedUpkeepReport(upkeepId, triggerID);
-      return false;
+      emit ReorgedUpkeepReport(upkeepId, transmitInfo.triggerID);
+      return (false, dedupID);
     }
-    bytes32 logDedupID = keccak256(abi.encodePacked(upkeepId, trigger.txHash, trigger.logIndex));
-    if (s_observedLogTriggers[logDedupID]) {
-      emit StaleUpkeepReport(upkeepId, triggerID);
-      return false;
+    if (s_dedupKeys[dedupID]) {
+      emit StaleUpkeepReport(upkeepId, transmitInfo.triggerID);
+      return (false, dedupID);
     }
-    s_observedLogTriggers[logDedupID] = true;
-    return true;
+    return (true, dedupID);
   }
 
   /**
@@ -512,11 +513,14 @@ contract KeeperRegistry2_1 is KeeperRegistryBase2_1, OCR2Abstract, Chainable, ER
   }
 
   /**
-   * @dev we don't update anything for log triggers because log triggered txs can be performed out of order
+   * @notice updates a storage marker for this upkeep to prevent duplicate and out of order performances
+   * @dev for conditional triggers we set the latest block number, for log triggers we store a dedupID
    */
-  function _updateLastPerformed(uint256 upkeepID, Trigger triggerType) private {
-    if (triggerType == Trigger.CONDITION) {
+  function _updateTriggerMarker(uint256 upkeepID, UpkeepTransmitInfo memory upkeepTransmitInfo) private {
+    if (upkeepTransmitInfo.triggerType == Trigger.CONDITION) {
       s_upkeep[upkeepID].lastPerformedBlockNumber = uint32(_blockNum());
+    } else if (upkeepTransmitInfo.triggerType == Trigger.LOG) {
+      s_dedupKeys[upkeepTransmitInfo.dedupID] = true;
     }
   }
 

contracts/src/v0.8/dev/automation/2_1/KeeperRegistryBase2_1.sol

@@ -76,7 +76,7 @@ abstract contract KeeperRegistryBase2_1 is ConfirmedOwner, ExecutionPrevention {
   mapping(uint256 => address) internal s_upkeepAdmin;
   mapping(uint256 => address) internal s_proposedAdmin;
   mapping(uint256 => bytes) internal s_checkData;
-  mapping(bytes32 => bool) internal s_observedLogTriggers;
+  mapping(bytes32 => bool) internal s_dedupKeys;
   // Registry config and state
   EnumerableSet.AddressSet internal s_registrars;
   mapping(address => Transmitter) internal s_transmitters;
@@ -350,6 +350,9 @@ abstract contract KeeperRegistryBase2_1 is ConfirmedOwner, ExecutionPrevention {
    * @member paymentParams the paymentParams for this upkeep
    * @member performSuccess whether the perform was successful
    * @member gasUsed gasUsed by this upkeep in perform
+   * @member gasOverhead gasOverhead for this upkeep
+   * @member triggerID unique ID used to identify an upkeep/trigger combo
+   * @member dedupID unique ID used to dedup an upkeep/trigger combo
    */
   struct UpkeepTransmitInfo {
     Upkeep upkeep;
@@ -360,6 +363,7 @@ abstract contract KeeperRegistryBase2_1 is ConfirmedOwner, ExecutionPrevention {
     uint256 gasUsed;
     uint256 gasOverhead;
     bytes32 triggerID;
+    bytes32 dedupID;
   }
 
   struct Transmitter {

contracts/test/v0.8/automation/KeeperRegistry2_1.test.ts

@@ -1145,6 +1145,26 @@ describe('KeeperRegistry2_1', () => {
       assert.equal(insufficientFundsUpkeepReportLogs.length, 1)
     })
 
+    it('permits retrying log triggers after funds are added', async () => {
+      const txHash = ethers.utils.randomBytes(32)
+      let tx = await getTransmitTx(registry, keeper1, [logUpkeepId], {
+        txHash,
+        logIndex: 0,
+      })
+      let receipt = await tx.wait()
+      const insufficientFundsLogs =
+        parseInsufficientFundsUpkeepReportLogs(receipt)
+      assert.equal(insufficientFundsLogs.length, 1)
+      registry.connect(admin).addFunds(logUpkeepId, toWei('100'))
+      tx = await getTransmitTx(registry, keeper1, [logUpkeepId], {
+        txHash,
+        logIndex: 0,
+      })
+      receipt = await tx.wait()
+      const performedLogs = parseUpkeepPerformedLogs(receipt)
+      assert.equal(performedLogs.length, 1)
+    })
+
     context('When the upkeep is funded', async () => {
       beforeEach(async () => {
         // Fund the upkeep