fix(BundleDataClient): Don't mark deposits in the future as invalid (#…

…1785)

* fix(BundleDataClient): Don't mark deposits in the future as invalid

Bundle data client incorrectly marks a deposit as invalid when its origin chain block number is greater than the origin chain bundle end block

Separately, increase lookback required for spoke pool clients to better accomodate situation where a fill is sent well after a deposit was mined but before it expired.

* fix tests

src/clients/BundleDataClient.ts

@@ -762,43 +762,40 @@ export class BundleDataClient {
           continue;
         }
 
-        originClient
-          .getDepositsForDestinationChain(destinationChainId)
-          .filter((deposit) => deposit.blockNumber <= originChainBlockRange[1])
-          .forEach((deposit) => {
-            depositCounter++;
-            const relayDataHash = this.getRelayHashFromEvent(deposit);
-            if (v3RelayHashes[relayDataHash]) {
-              // If we've seen this deposit before, then skip this deposit. This can happen if our RPC provider
-              // gives us bad data.
-              return;
-            }
-            // Even if deposit is not in bundle block range, store all deposits we can see in memory in this
-            // convenient dictionary.
-            v3RelayHashes[relayDataHash] = {
-              deposit: deposit,
-              fill: undefined,
-              slowFillRequest: undefined,
-            };
-
-            // If deposit block is within origin chain bundle block range, then save as bundle deposit.
-            // If deposit is in bundle and it has expired, additionally save it as an expired deposit.
-            // If deposit is not in the bundle block range, then save it as an older deposit that
-            // may have expired.
-            if (deposit.blockNumber >= originChainBlockRange[0]) {
-              // Deposit is a V3 deposit in this origin chain's bundle block range and is not a duplicate.
-              updateBundleDepositsV3(bundleDepositsV3, deposit);
-              // We don't check that fillDeadline >= bundleBlockTimestamps[destinationChainId][0] because
-              // that would eliminate any deposits in this bundle with a very low fillDeadline like equal to 0
-              // for example. Those should be impossible to create but technically should be included in this
-              // bundle of refunded deposits.
-              if (deposit.fillDeadline < bundleBlockTimestamps[destinationChainId][1]) {
-                expiredBundleDepositHashes.add(relayDataHash);
-              }
-            } else {
-              olderDepositHashes.add(relayDataHash);
+        originClient.getDepositsForDestinationChain(destinationChainId).forEach((deposit) => {
+          depositCounter++;
+          const relayDataHash = this.getRelayHashFromEvent(deposit);
+          if (v3RelayHashes[relayDataHash]) {
+            // If we've seen this deposit before, then skip this deposit. This can happen if our RPC provider
+            // gives us bad data.
+            return;
+          }
+          // Even if deposit is not in bundle block range, store all deposits we can see in memory in this
+          // convenient dictionary.
+          v3RelayHashes[relayDataHash] = {
+            deposit: deposit,
+            fill: undefined,
+            slowFillRequest: undefined,
+          };
+
+          // If deposit block is within origin chain bundle block range, then save as bundle deposit.
+          // If deposit is in bundle and it has expired, additionally save it as an expired deposit.
+          // If deposit is not in the bundle block range, then save it as an older deposit that
+          // may have expired.
+          if (deposit.blockNumber >= originChainBlockRange[0] && deposit.blockNumber <= originChainBlockRange[1]) {
+            // Deposit is a V3 deposit in this origin chain's bundle block range and is not a duplicate.
+            updateBundleDepositsV3(bundleDepositsV3, deposit);
+            // We don't check that fillDeadline >= bundleBlockTimestamps[destinationChainId][0] because
+            // that would eliminate any deposits in this bundle with a very low fillDeadline like equal to 0
+            // for example. Those should be impossible to create but technically should be included in this
+            // bundle of refunded deposits.
+            if (deposit.fillDeadline < bundleBlockTimestamps[destinationChainId][1]) {
+              expiredBundleDepositHashes.add(relayDataHash);
             }
-          });
+          } else if (deposit.blockNumber < originChainBlockRange[0]) {
+            olderDepositHashes.add(relayDataHash);
+          }
+        });
       }
     }
     this.logger.debug({

src/common/Constants.ts

@@ -579,3 +579,5 @@ export const DEFAULT_GAS_MULTIPLIER: { [chainId: number]: number } = {
   [CHAIN_IDs.REDSTONE]: 1.5,
   [CHAIN_IDs.ZORA]: 1.5,
 };
+
+export const CONSERVATIVE_BUNDLE_FREQUENCY_SECONDS = 3 * 60 * 60; // 3 hours is a safe assumption for the time

src/dataworker/DataworkerUtils.ts

@@ -1,7 +1,7 @@
 import assert from "assert";
 import { utils, interfaces, caching } from "@across-protocol/sdk";
 import { SpokePoolClient } from "../clients";
-import { spokesThatHoldEthAndWeth } from "../common/Constants";
+import { CONSERVATIVE_BUNDLE_FREQUENCY_SECONDS, spokesThatHoldEthAndWeth } from "../common/Constants";
 import { CONTRACT_ADDRESSES } from "../common/ContractAddresses";
 import {
   PoolRebalanceLeaf,
@@ -131,9 +131,19 @@ export async function blockRangesAreInvalidForSpokeClients(
       // Skip this check if the spokePoolClient.fromBlock is less than or equal to the spokePool deployment block.
       // In this case, we have all the information for this SpokePool possible so there are no older deposits
       // that might have expired that we might miss.
+      const conservativeBundleFrequencySeconds = Number(
+        process.env.CONSERVATIVE_BUNDLE_FREQUENCY_SECONDS ?? CONSERVATIVE_BUNDLE_FREQUENCY_SECONDS
+      );
       if (
         spokePoolClient.eventSearchConfig.fromBlock > spokePoolClient.deploymentBlock &&
-        endBlockTimestamps[chainId] - spokePoolClient.getOldestTime() < maxFillDeadlineBufferInBlockRange
+        // @dev The maximum lookback window we need to evaluate expired deposits is the max fill deadline buffer,
+        // which captures all deposits that newly expired, plus the bundle time (e.g. 1 hour) to account for the
+        // maximum time it takes for a newly expired deposit to be included in a bundle. A conservative value for
+        // this bundle time is 3 hours. This `conservativeBundleFrequencySeconds` buffer also ensures that all deposits
+        // that are technically "expired", but have fills in the bundle, are also included. This can happen if a fill
+        // is sent pretty late into the deposit's expiry period.
+        endBlockTimestamps[chainId] - spokePoolClient.getOldestTime() <
+          maxFillDeadlineBufferInBlockRange + conservativeBundleFrequencySeconds
       ) {
         return true;
       }

test/Dataworker.blockRangeUtils.ts

@@ -11,6 +11,7 @@ import { getDeployedBlockNumber } from "@across-protocol/contracts";
 import { MockHubPoolClient, MockSpokePoolClient } from "./mocks";
 import { getTimestampsForBundleEndBlocks } from "../src/utils/BlockUtils";
 import { assert } from "../src/utils";
+import { CONSERVATIVE_BUNDLE_FREQUENCY_SECONDS } from "../src/common";
 
 let dataworkerClients: DataworkerClients;
 let spokePoolClients: { [chainId: number]: SpokePoolClient };
@@ -314,8 +315,9 @@ describe("Dataworker block range-related utility methods", async function () {
     ).to.equal(false);
 
     // Set oldest time older such that fill deadline buffer now exceeds the time between the end block and the oldest
-    // time. Block ranges should now be valid.
-    const oldestBlockTimestampOverride = endBlockTimestamps[originChainId] - fillDeadlineOverride - 1;
+    // time plus the conservative bundle time. Block ranges should now be valid.
+    const oldestBlockTimestampOverride =
+      endBlockTimestamps[originChainId] - fillDeadlineOverride - CONSERVATIVE_BUNDLE_FREQUENCY_SECONDS - 1;
     assert(oldestBlockTimestampOverride > 0, "unrealistic oldest block timestamp");
     mockSpokePoolClient.setOldestBlockTimestampOverride(oldestBlockTimestampOverride);
     expect(

test/Dataworker.loadData.fill.ts

@@ -666,6 +666,23 @@ describe("Dataworker: Load data used in all functions", async function () {
       expect(data2.bundleDepositsV3).to.deep.equal({});
       expect(data2.expiredDepositsToRefundV3[originChainId][erc20_1.address].length).to.equal(1);
     });
+    it("Handles when deposit is greater than origin bundle end block but fill is within range", async function () {
+      // Send deposit after origin chain block range.
+      const blockRanges = getDefaultBlockRange(5);
+      const futureDeposit = generateV3Deposit();
+      const originChainIndex = dataworkerInstance.chainIdListForBundleEvaluationBlockNumbers.indexOf(originChainId);
+      blockRanges[originChainIndex] = [blockRanges[0][0], futureDeposit.blockNumber - 1];
+
+      await mockOriginSpokePoolClient.update(["V3FundsDeposited"]);
+
+      generateV3FillFromDepositEvent(futureDeposit);
+      await mockDestinationSpokePoolClient.update(["FilledV3Relay"]);
+
+      const data1 = await dataworkerInstance.clients.bundleDataClient.loadData(blockRanges, spokePoolClients);
+      expect(data1.bundleDepositsV3).to.deep.equal({});
+      expect(data1.bundleFillsV3[repaymentChainId][l1Token_1.address].fills.length).to.equal(1);
+      expect(spyLogIncludes(spy, -2, "invalid V3 fills in range")).to.be.false;
+    });
     it("Does not count prior bundle expired deposits that were filled", async function () {
       // Send deposit that expires in this bundle.
       const bundleBlockTimestamps = await dataworkerInstance.clients.bundleDataClient.getBundleBlockTimestamps(