From 02e2363c0351cded46632186a75c0b626f20d2f8 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Wed, 3 Jan 2024 11:26:50 +0100 Subject: [PATCH 01/18] feat: Dynamically adjust proposal block ranges This commit introduces dynamic adjustment of the block ranges that the proposer will propose. The intention of this change is to make the proposer more robust to incomplete or inconsistent data being supplied by one or more RPC providers. Dynamic adjustment is enacted in the event that any of the following conditions are detected: - An invalid fill traces to a missing deposit (no deposit found for the specified depositId). - A deposit is known to be filled, without any associated FilledRelay event. In the first case, this results in narrowing of the block range on both the origin and destination chains. For the latter, only the destination chain is narrowed. --- package.json | 2 +- src/clients/BundleDataClient.ts | 45 ++++-- src/dataworker/Dataworker.ts | 158 +++++++++++++++++- src/dataworker/DataworkerUtils.ts | 5 +- src/interfaces/SpokePool.ts | 10 ++ src/interfaces/index.ts | 1 + src/utils/SDKUtils.ts | 3 + test/Dataworker.loadData.ts | 2 + test/Dataworker.proposeRootBundle.ts | 232 ++++++++++++++++++++++++++- yarn.lock | 8 +- 10 files changed, 438 insertions(+), 28 deletions(-) diff --git a/package.json b/package.json index 2afe9adc2..0bb636083 100644 --- a/package.json +++ b/package.json @@ -12,7 +12,7 @@ "dependencies": { "@across-protocol/constants-v2": "1.0.8", "@across-protocol/contracts-v2": "2.4.7", - "@across-protocol/sdk-v2": "0.20.0", + "@across-protocol/sdk-v2": "0.20.1", "@arbitrum/sdk": "^3.1.3", "@defi-wonderland/smock": "^2.3.5", "@eth-optimism/sdk": "^3.1.0", diff --git a/src/clients/BundleDataClient.ts b/src/clients/BundleDataClient.ts index 2dcae92dc..c4e4c3be2 100644 --- a/src/clients/BundleDataClient.ts +++ b/src/clients/BundleDataClient.ts @@ -1,8 +1,10 @@ +import assert from "assert"; import * as _ from "lodash"; import { DepositWithBlock, FillsToRefund, FillWithBlock, + InvalidFill, ProposedRootBundle, RefundRequestWithBlock, UnfilledDeposit, @@ -38,6 +40,7 @@ type DataCacheValue = { unfilledDeposits: UnfilledDeposit[]; fillsToRefund: FillsToRefund; allValidFills: FillWithBlock[]; + allInvalidFills: InvalidFill[]; deposits: DepositWithBlock[]; earlyDeposits: typechain.FundsDepositedEvent[]; }; @@ -176,6 +179,7 @@ export class BundleDataClient { unfilledDeposits: UnfilledDeposit[]; fillsToRefund: FillsToRefund; allValidFills: FillWithBlock[]; + allInvalidFills: InvalidFill[]; deposits: DepositWithBlock[]; earlyDeposits: typechain.FundsDepositedEvent[]; }> { @@ -190,6 +194,7 @@ export class BundleDataClient { } return this._loadData(blockRangesForChains, spokePoolClients, isUBA, logData); } + async _loadData( blockRangesForChains: number[][], spokePoolClients: { [chainId: number]: SpokePoolClient }, @@ -199,6 +204,7 @@ export class BundleDataClient { unfilledDeposits: UnfilledDeposit[]; fillsToRefund: FillsToRefund; allValidFills: FillWithBlock[]; + allInvalidFills: InvalidFill[]; deposits: DepositWithBlock[]; earlyDeposits: typechain.FundsDepositedEvent[]; }> { @@ -225,7 +231,7 @@ export class BundleDataClient { const allRelayerRefunds: { repaymentChain: number; repaymentToken: string }[] = []; const deposits: DepositWithBlock[] = []; const allValidFills: FillWithBlock[] = []; - const allInvalidFills: FillWithBlock[] = []; + const allInvalidFills: InvalidFill[] = []; const earlyDeposits: typechain.FundsDepositedEvent[] = []; // Save refund in-memory for validated fill. @@ -275,23 +281,27 @@ export class BundleDataClient { updateTotalRefundAmount(fillsToRefund, fill, chainToSendRefundTo, repaymentToken); }; - const validateFillAndSaveData = async (fill: FillWithBlock, blockRangeForChain: number[]): Promise => { + const validateFillAndSaveData = async (fill: FillWithBlock, blockRangeForChain: number[]): Promise => { const originClient = spokePoolClients[fill.originChainId]; const matchedDeposit = originClient.getDepositForFill(fill); if (matchedDeposit) { addRefundForValidFill(fill, matchedDeposit, blockRangeForChain); - } else { - // Matched deposit for fill was not found in spoke client. This situation should be rare so let's - // send some extra RPC requests to blocks older than the spoke client's initial event search config - // to find the deposit if it exists. - const spokePoolClient = spokePoolClients[fill.originChainId]; - const historicalDeposit = await queryHistoricalDepositForFill(spokePoolClient, fill); - if (historicalDeposit.found) { - addRefundForValidFill(fill, historicalDeposit.deposit, blockRangeForChain); - } else { - allInvalidFills.push(fill); - } + return true; + } + + // Matched deposit for fill was not found in spoke client. This situation should be rare so let's + // send some extra RPC requests to blocks older than the spoke client's initial event search config + // to find the deposit if it exists. + const spokePoolClient = spokePoolClients[fill.originChainId]; + const historicalDeposit = await queryHistoricalDepositForFill(spokePoolClient, fill); + if (historicalDeposit.found) { + addRefundForValidFill(fill, historicalDeposit.deposit, blockRangeForChain); + return true; } + + assert(historicalDeposit.found === false); // Help tsc to narrow the discriminated union type. + allInvalidFills.push({ fill, code: historicalDeposit.code, reason: historicalDeposit.reason }); + return false; }; const _isChainDisabled = (chainId: number): boolean => { @@ -450,7 +460,14 @@ export class BundleDataClient { }); } - this.loadDataCache[key] = { fillsToRefund, deposits, unfilledDeposits, allValidFills, earlyDeposits }; + this.loadDataCache[key] = { + fillsToRefund, + deposits, + unfilledDeposits, + allValidFills, + allInvalidFills, + earlyDeposits, + }; return this.loadDataFromCache(key); } diff --git a/src/dataworker/Dataworker.ts b/src/dataworker/Dataworker.ts index 76d0ee403..fd31803e3 100644 --- a/src/dataworker/Dataworker.ts +++ b/src/dataworker/Dataworker.ts @@ -1,9 +1,11 @@ +import assert from "assert"; import { utils as ethersUtils } from "ethers"; import { winston, EMPTY_MERKLE_ROOT, sortEventsDescending, BigNumber, + getNetworkName, getRefund, MerkleTree, sortEventsAscending, @@ -18,6 +20,7 @@ import { DepositWithBlock, FillsToRefund, FillWithBlock, + InvalidFill, isUbaOutflow, outflowIsFill, ProposedRootBundle, @@ -29,6 +32,7 @@ import { RunningBalances, PoolRebalanceLeaf, RelayerRefundLeaf, + RelayData, } from "../interfaces"; import { DataworkerClients } from "./DataworkerClientHelper"; import { SpokePoolClient, UBAClient, BalanceAllocator } from "../clients"; @@ -453,6 +457,153 @@ export class Dataworker { } } + async narrowProposalBlockRanges( + blockRanges: number[][], + spokePoolClients: SpokePoolClientsByChain, + isUBA = false, + logData = false + ): Promise { + const chainIds = this.chainIdListForBundleEvaluationBlockNumbers; + const updatedBlockRanges = Object.fromEntries(chainIds.map((chainId, idx) => [chainId, [...blockRanges[idx]]])); + + const { deposits, allValidFills, allInvalidFills } = await this.clients.bundleDataClient._loadData( + blockRanges, + spokePoolClients, + isUBA, + logData + ); + + // If invalid fills were detected and they appear to be due to gaps in FundsDeposited events: + // - Narrow the origin block range to exclude the missing deposit, AND + // - Narrow the destination block range to exclude the invalid fill. + allInvalidFills + .filter(({ code }) => code === InvalidFill.DepositIdNotFound) + .forEach(({ fill: { depositId, originChainId, destinationChainId, blockNumber } }) => { + const [originChain, destinationChain] = [getNetworkName(originChainId), getNetworkName(destinationChainId)]; + + // Exclude the missing deposit on the origin chain. + const originSpokePoolClient = spokePoolClients[originChainId]; + let [startBlock, endBlock] = updatedBlockRanges[originChainId]; + + // Find the previous known deposit. This may not be immediastely preceding depositId. + const previousDeposit = originSpokePoolClient + .getDepositsForDestinationChain(destinationChainId) + .filter((deposit: DepositWithBlock) => deposit.blockNumber < blockNumber) + .at(-1); + + if (previousDeposit?.blockNumber ?? startBlock > startBlock) { + updatedBlockRanges[originChainId] = [startBlock, previousDeposit.blockNumber]; + this.logger.debug({ + at: "Dataworker::narrowBlockRanges", + message: `Narrowed proposal block range on ${originChain} due to missing deposit.`, + depositId, + previousBlockRange: [startBlock, endBlock], + newBlockRange: updatedBlockRanges[originChainId], + }); + } + + // Update the endBlock on the destination chain to exclude the invalid fill. + const destSpokePoolClient = spokePoolClients[destinationChainId]; + [startBlock, endBlock] = updatedBlockRanges[destinationChainId]; + + if (blockNumber <= endBlock) { + // Find the fill immediately preceding this invalid fill. + const previousFill = destSpokePoolClient + .getFillsWithBlockInRange(startBlock, Math.max(blockNumber - 1, startBlock)) + .at(-1); + + // Wind back to the bundle end block number to that of the previous fill. + const newEndBlock = previousFill?.blockNumber ?? startBlock; + updatedBlockRanges[destinationChainId] = + newEndBlock > startBlock ? [startBlock, newEndBlock] : [startBlock - 1, startBlock - 1]; // @fix: Must use previous end block! + + this.logger.debug({ + at: "Dataworker::narrowBlockRanges", + message: `Narrowed proposal block range on ${destinationChain} due to missing deposit on ${originChain}.`, + depositId, + previousBlockRange: [startBlock, endBlock], + newBlockRange: updatedBlockRanges[destinationChainId], + }); + } + }); + + // For each deposit within the origin chain block range for which no or only partial fill events are found, + // verify whether the fill has been completed on the destination chain. If the fill has been made to completion + // then this is evidence of potential dropped/suppressed events, so narrow the block range to exclude those blocks. + const newUnfilledDeposits = deposits.filter(({ originChainId, depositId, amount, blockNumber }) => { + if (blockNumber > updatedBlockRanges[originChainId][1]) { + return false; // Fill event is already out of scope due to previous narrowing. + } + return allValidFills.find( + (fill) => + fill.depositId === depositId && fill.originChainId === originChainId && fill.totalFilledAmount.eq(amount) + ) + ? false + : true; + }); + + // For each unfilled or partially filled deposit, verify whether it was actually filled by the proposal end block. + const fillBlocks = await sdk.utils.mapAsync(newUnfilledDeposits, async (deposit: DepositWithBlock) => { + const { spokePool, deploymentBlock } = spokePoolClients[deposit.destinationChainId]; + const blockRange = updatedBlockRanges[deposit.destinationChainId]; + const endBlock = blockRange[1]; + + // @todo: Some tests rely on this; fix! + const startBlock = blockRange[0] > deploymentBlock ? blockRange[0] : deploymentBlock; + + // @todo: Beware the scenario where the fill is completed before the deposit, yet the deposit hash matches 100%. + // This corner case must be ruled out or mitigated before merge, because it will cause the proposer to throw. + return await sdk.utils.findFillBlock(spokePool, deposit as RelayData, startBlock, endBlock); + }); + + // Exclude each un- or partially-filled deposit that resolved to a complete fill. + fillBlocks.forEach((fillBlock, idx) => { + if (isDefined(fillBlock)) { + const deposit = newUnfilledDeposits[idx]; + const { destinationChainId } = deposit; + const chain = getNetworkName(deposit.destinationChainId); + this.logger.warn({ + at: "Dataworker::narrowBlockRange", + message: `Identified probable missing filledRelay event on ${chain} at block ${fillBlock}.`, + deposit, + }); + + const [startBlock, endBlock] = updatedBlockRanges[destinationChainId]; + const newEndBlock = + allValidFills + .filter((fill) => fill.destinationChainId === destinationChainId && fill.blockNumber < fillBlock) + .at(-1)?.blockNumber ?? startBlock; + + if (newEndBlock < endBlock) { + updatedBlockRanges[destinationChainId] = + newEndBlock > startBlock ? [startBlock, newEndBlock] : [startBlock - 1, startBlock - 1]; + + this.logger.debug({ + at: "Dataworker::narrowBlockRanges", + message: `Narrowed proposal block range on ${chain} due to missing fill event at block ${fillBlock}.`, + deposit, + previousBlockRange: [startBlock, endBlock], + newBlockRange: updatedBlockRanges[destinationChainId], + }); + } + } + }); + + // Quick sanity check - make sure that the block ranges are coherent. A chain may be soft-paused if it has ongoing + // RPC issues (block ranges are frozen at the previous proposal endBlock), so ensure that this is also handled. + const finalBlockRanges = chainIds.map((chainId) => updatedBlockRanges[chainId]); + const coherentBlockRanges = finalBlockRanges.every(([startBlock, endBlock], idx) => { + const [originalStartBlock] = blockRanges[idx]; + return ( + (endBlock > startBlock && startBlock === originalStartBlock) || + (startBlock === endBlock && startBlock === originalStartBlock - 1) // soft-pause + ); + }); + assert(coherentBlockRanges, "Updated proposal block ranges are incoherent"); + + return chainIds.map((chainId) => updatedBlockRanges[chainId]); + } + async Legacy_proposeRootBundle( blockRangesForProposal: number[][], spokePoolClients: SpokePoolClientsByChain, @@ -460,8 +611,13 @@ export class Dataworker { logData = false ): Promise { const timerStart = Date.now(); + + // Dry-run the proposal over the input block ranges to identify any data inconsistencies. + const blockRanges = await this.narrowProposalBlockRanges(blockRangesForProposal, spokePoolClients); + const { fillsToRefund, deposits, allValidFills, unfilledDeposits, earlyDeposits } = - await this.clients.bundleDataClient._loadData(blockRangesForProposal, spokePoolClients, false, logData); + await this.clients.bundleDataClient._loadData(blockRanges, spokePoolClients, false, logData); + const allValidFillsInRange = getFillsInRange( allValidFills, blockRangesForProposal, diff --git a/src/dataworker/DataworkerUtils.ts b/src/dataworker/DataworkerUtils.ts index c536a8d4f..cb8317623 100644 --- a/src/dataworker/DataworkerUtils.ts +++ b/src/dataworker/DataworkerUtils.ts @@ -6,6 +6,7 @@ import { DepositWithBlock, FillsToRefund, FillWithBlock, + InvalidFill, PoolRebalanceLeaf, RelayerRefundLeaf, RelayerRefundLeafWithGroup, @@ -104,10 +105,10 @@ export function prettyPrintSpokePoolEvents( allValidFills: FillWithBlock[], allRelayerRefunds: { repaymentChain: number; repaymentToken: string }[], unfilledDeposits: UnfilledDeposit[], - allInvalidFills: FillWithBlock[] + allInvalidFills: InvalidFill[] ): AnyObject { const allInvalidFillsInRange = getFillsInRange( - allInvalidFills, + allInvalidFills.map(({ fill }) => fill), blockRangesForChains, chainIdListForBundleEvaluationBlockNumbers ); diff --git a/src/interfaces/SpokePool.ts b/src/interfaces/SpokePool.ts index e1e0f3bfe..1353e0900 100644 --- a/src/interfaces/SpokePool.ts +++ b/src/interfaces/SpokePool.ts @@ -1,5 +1,15 @@ +import { interfaces } from "@across-protocol/sdk-v2"; import { SpokePoolClient } from "../clients"; +import * as utils from "../utils/SDKUtils"; export interface SpokePoolClientsByChain { [chainId: number]: SpokePoolClient; } + +export const { InvalidFill } = utils; + +export type InvalidFill = { + fill: interfaces.FillWithBlock; + code: utils.InvalidFillEnum; + reason: string; +}; diff --git a/src/interfaces/index.ts b/src/interfaces/index.ts index bee072987..998082efe 100644 --- a/src/interfaces/index.ts +++ b/src/interfaces/index.ts @@ -40,6 +40,7 @@ export type PendingRootBundle = interfaces.PendingRootBundle; // SpokePool interfaces export type FundsDepositedEvent = interfaces.FundsDepositedEvent; +export type RelayData = interfaces.RelayData; export type Deposit = interfaces.Deposit; export type DepositWithBlock = interfaces.DepositWithBlock; export type Fill = interfaces.Fill; diff --git a/src/utils/SDKUtils.ts b/src/utils/SDKUtils.ts index 05952c59e..4fb7da4d8 100644 --- a/src/utils/SDKUtils.ts +++ b/src/utils/SDKUtils.ts @@ -6,6 +6,8 @@ export type BlockFinderHints = sdk.utils.BlockFinderHints; export class PriceClient extends sdk.priceClient.PriceClient {} export const { acrossApi, coingecko, defiLlama } = sdk.priceClient.adapters; +export type InvalidFillEnum = sdk.utils.InvalidFill; + export const { bnZero, bnOne, @@ -19,6 +21,7 @@ export const { toGWei, toBNWei, formatFeePct, + InvalidFill, shortenHexStrings, convertFromWei, max, diff --git a/test/Dataworker.loadData.ts b/test/Dataworker.loadData.ts index 5085b9ffa..9f1e59444 100644 --- a/test/Dataworker.loadData.ts +++ b/test/Dataworker.loadData.ts @@ -103,9 +103,11 @@ describe("Dataworker: Load data used in all functions", async function () { deposits: [], fillsToRefund: {}, allValidFills: [], + allInvalidFills: [], earlyDeposits: [], }); }); + describe("Computing refunds for bundles", function () { let fill1: Fill; let deposit1: Deposit; diff --git a/test/Dataworker.proposeRootBundle.ts b/test/Dataworker.proposeRootBundle.ts index 853283688..32dc9cb63 100644 --- a/test/Dataworker.proposeRootBundle.ts +++ b/test/Dataworker.proposeRootBundle.ts @@ -1,4 +1,8 @@ +import assert from "assert"; +import { utils as sdkUtils } from "@across-protocol/sdk-v2"; +import { Dataworker } from "../src/dataworker/Dataworker"; import { HubPoolClient, MultiCallerClient, SpokePoolClient } from "../src/clients"; +import { Deposit, FillWithBlock } from "../src/interfaces"; import { EMPTY_MERKLE_ROOT, MAX_UINT_VAL, getDepositPath } from "../src/utils"; import { CHAIN_ID_TEST_LIST, amountToDeposit, destinationChainId, originChainId, utf8ToHex } from "./constants"; import { setupFastDataworker } from "./fixtures/Dataworker.Fixture"; @@ -6,24 +10,41 @@ import { Contract, SignerWithAddress, buildDeposit, + buildFill, buildFillForRepaymentChain, ethers, expect, lastSpyLogIncludes, lastSpyLogLevel, + setupTokensForWallet, sinon, toBNWei, } from "./utils"; - // Tested -import { Dataworker } from "../src/dataworker/Dataworker"; -import { FillWithBlock } from "../src/interfaces"; import { MockConfigStoreClient } from "./mocks"; +class TestSpokePoolClient extends SpokePoolClient { + deleteDeposit(depositId: number): void { + const depositHash = this.getDepositHash({ depositId, originChainId: this.chainId }); + delete this.depositHashes[depositHash]; + } + + // Delete the first fill matching the pair of originChainId and depositId. + deleteFill(originChainId: number, depositId: number): void { + const fills = this.fills[originChainId]; + const depositIdx = fills + .map((fill) => `${fill.originChainId}-${fill.depositId}`) + .indexOf(`${originChainId}-${depositId}`); + assert(depositIdx !== -1); + + this.fills[originChainId].splice(depositIdx, 1); + } +} + let spy: sinon.SinonSpy; let spokePool_1: Contract, erc20_1: Contract, spokePool_2: Contract, erc20_2: Contract; let l1Token_1: Contract, hubPool: Contract, configStore: Contract; -let depositor: SignerWithAddress; +let depositor: SignerWithAddress, relayer: SignerWithAddress; let hubPoolClient: HubPoolClient, configStoreClient: MockConfigStoreClient; let dataworkerInstance: Dataworker, multiCallerClient: MultiCallerClient; @@ -34,6 +55,7 @@ let updateAllClients: () => Promise; describe("Dataworker: Propose root bundle", async function () { beforeEach(async function () { ({ + relayer, hubPool, spokePool_1, erc20_1, @@ -52,7 +74,7 @@ describe("Dataworker: Propose root bundle", async function () { } = await setupFastDataworker(ethers)); }); - it("Simple lifecycle", async function () { + it.skip("Simple lifecycle", async function () { await updateAllClients(); const getMostRecentLog = (_spy: sinon.SinonSpy, message: string) => { @@ -220,7 +242,7 @@ describe("Dataworker: Propose root bundle", async function () { dataworkerInstance.clearCache(); expect(dataworkerInstance.rootCache).to.deep.equal({}); }); - it("Exits early if config store version is out of date", async function () { + it.skip("Exits early if config store version is out of date", async function () { // Set up test so that the latest version in the config store contract is higher than // the version in the config store client. const update = await configStore.updateGlobalConfig(utf8ToHex("VERSION"), "1"); @@ -248,4 +270,202 @@ describe("Dataworker: Propose root bundle", async function () { expect(lastSpyLogIncludes(spy, "Skipping proposal because missing updated ConfigStore version")).to.be.true; expect(lastSpyLogLevel(spy)).to.equal("warn"); }); + + describe("Dataworker: Proposal block range narrowing", async function () { + const nDeposits = 50; + const chainIds = [originChainId, destinationChainId]; + let originSpoke: SpokePoolClient, destinationSpoke: SpokePoolClient; + + beforeEach(async function () { + // Construct special SpokePoolClient instances with the ability to delete events. + [originSpoke, destinationSpoke] = chainIds.map((chainId) => { + const { deploymentBlock, logger, spokePool } = spokePoolClients[chainId]; + const spokePoolClient = new TestSpokePoolClient(logger, spokePool, hubPoolClient, chainId, deploymentBlock); + spokePoolClients[chainId] = spokePoolClient; + return spokePoolClient; + }); + await Promise.all(Object.values(spokePoolClients).map((spokePoolClient) => spokePoolClient.update())); + + const weth = undefined; + await setupTokensForWallet(originSpoke.spokePool, depositor, [erc20_1], weth, nDeposits + 1); + await setupTokensForWallet(destinationSpoke.spokePool, relayer, [erc20_2], weth, nDeposits + 1); + }); + + it("Narrows block ranges for fills where no depositId is found (origin chain deposit gaps)", async function () { + // Prime the origin spoke with a single deposit. Some utility functions involved in searching for deposits + // rely on the lowest deposit ID in order to bound the search by. A missing initial deposit ID would always + // be caught during pre-launch testing, so that's not a concern here. + let deposit = await buildDeposit( + hubPoolClient, + originSpoke.spokePool, + erc20_1, + l1Token_1, + depositor, + destinationChainId, + amountToDeposit + ); + + // Since the SpokePoolClient relies on SpokePoolClient.latestDepositIdQueried, we can't currently detect + // the _final_ deposit going missing. Some additional refactoring is needed to add this capability. + const missingDepositIdx = Math.floor(Math.random() * (nDeposits - 2)); + let missingDepositId = -1; + + for (let idx = 0; idx < nDeposits; ++idx) { + deposit = await buildDeposit( + hubPoolClient, + originSpoke.spokePool, + erc20_1, + l1Token_1, + depositor, + destinationChainId, + amountToDeposit + ); + + if (idx === missingDepositIdx) { + missingDepositId = deposit.depositId; + } + + await buildFill(destinationSpoke.spokePool, erc20_2, depositor, relayer, deposit, 1); + } + await hubPool.setCurrentTime(deposit.quoteTimestamp + 1); + await hubPoolClient.update(); + + await sdkUtils.mapAsync([originSpoke, destinationSpoke], async (spokePoolClient) => { + await spokePoolClient.spokePool.setCurrentTime(deposit.quoteTimestamp + 1); + await spokePoolClient.update(); + }); + + // Flush the "missing" deposit, which was made but should not be visible to the SpokePoolClient. + originSpoke.deleteDeposit(missingDepositId); + const deposits = originSpoke.getDeposits(); + expect(deposits.length).equals(nDeposits); // 1 (initial) + nDeposits - 1 (missing). + expect(deposits.map(({ depositId }) => depositId).includes(missingDepositId)).is.false; + + // There must be 1 more fill than deposits. + const fills = destinationSpoke.getFills(); + expect(fills.length).equals(nDeposits); + + // Estimate the starting proposal block range for each chain, based on the blocks it has data for. + const proposalChainIds = configStoreClient.getChainIdIndicesForBlock(); + const blockRanges = proposalChainIds.map((chainId) => { + const { deploymentBlock, latestBlockSearched } = spokePoolClients[chainId]; + return [deploymentBlock, latestBlockSearched]; + }); + + // Read in the valid and invalid fills for the specified block ranges. This should contain an invalid fill. + let { allValidFills, allInvalidFills } = await dataworkerInstance.clients.bundleDataClient.loadData( + blockRanges, + spokePoolClients + ); + expect(allValidFills.length).equals(nDeposits - 1); + expect(allInvalidFills.length).equals(1); + expect(allInvalidFills[0].fill.depositId).equals(missingDepositId); + + // Compute the updated end block on the origin chain (excluding the missing deposit). + const safeDepositIdx = deposits.map(({ depositId }) => depositId).indexOf(allInvalidFills[0]?.fill.depositId - 1); + const { blockNumber: safeOriginEndBlock } = deposits[safeDepositIdx]; + + // Compute the updated end block on the destination chain (excluding the "invalid" fill). + const { blockNumber: safeDestinationEndBlock } = allValidFills.reduce( + (acc, curr) => + curr.blockNumber < allInvalidFills[0]?.fill.blockNumber && curr.blockNumber > acc.blockNumber ? curr : acc, + allValidFills[0] + ); + + // Update the expected proposal block ranges. + const safeBlockRanges = blockRanges.map((blockRange) => [...blockRange]); + const [originChainIdx, destinationChainIdx] = [ + proposalChainIds.indexOf(originChainId), + proposalChainIds.indexOf(destinationChainId), + ]; + safeBlockRanges[originChainIdx][1] = safeOriginEndBlock; + safeBlockRanges[destinationChainIdx][1] = safeDestinationEndBlock; + + const narrowedBlockRanges = await dataworkerInstance.narrowProposalBlockRanges(blockRanges, spokePoolClients); + ({ allValidFills, allInvalidFills } = await dataworkerInstance.clients.bundleDataClient.loadData( + narrowedBlockRanges, + spokePoolClients + )); + expect(allInvalidFills.length).equals(0); + expect(narrowedBlockRanges).to.deep.equal(safeBlockRanges); + }); + + it("Narrows proposal block ranges for missing fills (destination chain fill gaps)", async function () { + // Since the SpokePoolClient relies on SpokePoolClient.latestDepositIdQueried, we can't currently detect + // the _final_ deposit going missing. Some additional refactoring is needed to add this detection. + const missingDepositIdx = Math.floor(Math.random() * (nDeposits - 2)); + let missingDepositId = -1; + let fillBlock = -1; + let deposit: Deposit; + for (let idx = 0; idx < nDeposits; ++idx) { + deposit = await buildDeposit( + hubPoolClient, + originSpoke.spokePool, + erc20_1, + l1Token_1, + depositor, + destinationChainId, + amountToDeposit + ); + + await buildFill(destinationSpoke.spokePool, erc20_2, depositor, relayer, deposit, 1); + if (idx === missingDepositIdx) { + missingDepositId = deposit.depositId; + fillBlock = await destinationSpoke.spokePool.provider.getBlockNumber(); + } + } + assert(fillBlock !== -1); + + await hubPool.setCurrentTime(deposit.quoteTimestamp + 1); + await hubPoolClient.update(); + + await sdkUtils.mapAsync([originSpoke, destinationSpoke], async (spokePoolClient) => { + await spokePoolClient.spokePool.setCurrentTime(deposit.quoteTimestamp + 1); + await spokePoolClient.update(); + }); + + // There must be 1 more deposits than fills. + expect(originSpoke.getDeposits().length).equals(nDeposits); + + // Flush the "missing" fill, which was made but should not be visible to the SpokePoolClient. + destinationSpoke.deleteFill(originChainId, missingDepositId); + const fills = destinationSpoke.getFillsForOriginChain(originChainId); + expect(fills.length).equals(nDeposits - 1); // nDeposits - 1 (missing). + expect(fills.map(({ depositId }) => depositId).includes(missingDepositId)).is.false; + + // Estimate the starting proposal block range for each chain, based on the blocks it has data for. + const proposalChainIds = configStoreClient.getChainIdIndicesForBlock(); + const blockRanges = proposalChainIds.map((chainId) => { + const { deploymentBlock, latestBlockSearched } = spokePoolClients[chainId]; + return [deploymentBlock, latestBlockSearched]; + }); + + // Read in the valid and invalid fills for the specified block ranges. This should contain an invalid fill. + let { allValidFills, allInvalidFills } = await dataworkerInstance.clients.bundleDataClient.loadData( + blockRanges, + spokePoolClients + ); + expect(allValidFills.length).equals(nDeposits - 1); + expect(allInvalidFills.length).equals(0); // One valid fill is _missing_. + + // Compute the updated end block on the destination chain (excluding the "invalid" fill). + const { blockNumber: safeDestinationEndBlock } = allValidFills.reduce( + (acc, curr) => (curr.blockNumber < fillBlock && curr.blockNumber > acc.blockNumber ? curr : acc), + allValidFills[0] + ); + + // Update the expected proposal block ranges. + const safeBlockRanges = blockRanges.map((blockRange) => [...blockRange]); + const destinationChainIdx = proposalChainIds.indexOf(destinationChainId); + safeBlockRanges[destinationChainIdx][1] = safeDestinationEndBlock; + + const narrowedBlockRanges = await dataworkerInstance.narrowProposalBlockRanges(blockRanges, spokePoolClients); + ({ allValidFills, allInvalidFills } = await dataworkerInstance.clients.bundleDataClient.loadData( + narrowedBlockRanges, + spokePoolClients + )); + expect(allInvalidFills.length).equals(0); + expect(narrowedBlockRanges).to.deep.equal(safeBlockRanges); + }); + }); }); diff --git a/yarn.lock b/yarn.lock index 2421d6eea..8444979e0 100644 --- a/yarn.lock +++ b/yarn.lock @@ -48,10 +48,10 @@ "@openzeppelin/contracts" "4.1.0" "@uma/core" "^2.18.0" -"@across-protocol/sdk-v2@0.20.0": - version "0.20.0" - resolved "https://registry.yarnpkg.com/@across-protocol/sdk-v2/-/sdk-v2-0.20.0.tgz#6f7a49f140138c76b5c1e0668c4796ff9d6e8286" - integrity sha512-akWjmm/okF2W2hQN+9GUa1GMWTBcFAxHA7ElXbnGV5h4UDEP1j7Z8kpf3cL+kaz/BqEluuBifu+arehpuhdfBg== +"@across-protocol/sdk-v2@0.20.1": + version "0.20.1" + resolved "https://registry.yarnpkg.com/@across-protocol/sdk-v2/-/sdk-v2-0.20.1.tgz#6dcf18bf1291b0aef936325fcc18cc348dee0c97" + integrity sha512-t1iDG84yBcgRO3AufptKQgOf6lgnWjeLmA8/X2X2nE95G5io1WG7Xz2ymj6FX6Bpk/JcCkBoN8sKIi6N03OwYg== dependencies: "@across-protocol/across-token" "^1.0.0" "@across-protocol/constants-v2" "^1.0.8" From cb9186a96d4340bcda0465d18d717c9ecf9686e7 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Fri, 5 Jan 2024 14:27:45 +0100 Subject: [PATCH 02/18] Re-enable disabled tests --- test/Dataworker.proposeRootBundle.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Dataworker.proposeRootBundle.ts b/test/Dataworker.proposeRootBundle.ts index 32dc9cb63..5550a8452 100644 --- a/test/Dataworker.proposeRootBundle.ts +++ b/test/Dataworker.proposeRootBundle.ts @@ -74,7 +74,7 @@ describe("Dataworker: Propose root bundle", async function () { } = await setupFastDataworker(ethers)); }); - it.skip("Simple lifecycle", async function () { + it("Simple lifecycle", async function () { await updateAllClients(); const getMostRecentLog = (_spy: sinon.SinonSpy, message: string) => { @@ -242,7 +242,7 @@ describe("Dataworker: Propose root bundle", async function () { dataworkerInstance.clearCache(); expect(dataworkerInstance.rootCache).to.deep.equal({}); }); - it.skip("Exits early if config store version is out of date", async function () { + it("Exits early if config store version is out of date", async function () { // Set up test so that the latest version in the config store contract is higher than // the version in the config store client. const update = await configStore.updateGlobalConfig(utf8ToHex("VERSION"), "1"); From b3d31e95d9ba37741f450d8c8b3f59d24987d49d Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Fri, 5 Jan 2024 14:48:56 +0100 Subject: [PATCH 03/18] Return finalBlockRanges --- src/dataworker/Dataworker.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dataworker/Dataworker.ts b/src/dataworker/Dataworker.ts index fd31803e3..6eb26336e 100644 --- a/src/dataworker/Dataworker.ts +++ b/src/dataworker/Dataworker.ts @@ -601,7 +601,7 @@ export class Dataworker { }); assert(coherentBlockRanges, "Updated proposal block ranges are incoherent"); - return chainIds.map((chainId) => updatedBlockRanges[chainId]); + return finalBlockRanges; } async Legacy_proposeRootBundle( From d029d53730f1cf52ec1363b2daf46f889d767236 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Fri, 5 Jan 2024 14:51:14 +0100 Subject: [PATCH 04/18] Undo unnecessary change --- src/clients/BundleDataClient.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/clients/BundleDataClient.ts b/src/clients/BundleDataClient.ts index c4e4c3be2..a98344055 100644 --- a/src/clients/BundleDataClient.ts +++ b/src/clients/BundleDataClient.ts @@ -281,12 +281,12 @@ export class BundleDataClient { updateTotalRefundAmount(fillsToRefund, fill, chainToSendRefundTo, repaymentToken); }; - const validateFillAndSaveData = async (fill: FillWithBlock, blockRangeForChain: number[]): Promise => { + const validateFillAndSaveData = async (fill: FillWithBlock, blockRangeForChain: number[]): Promise => { const originClient = spokePoolClients[fill.originChainId]; const matchedDeposit = originClient.getDepositForFill(fill); if (matchedDeposit) { addRefundForValidFill(fill, matchedDeposit, blockRangeForChain); - return true; + return; } // Matched deposit for fill was not found in spoke client. This situation should be rare so let's @@ -296,12 +296,10 @@ export class BundleDataClient { const historicalDeposit = await queryHistoricalDepositForFill(spokePoolClient, fill); if (historicalDeposit.found) { addRefundForValidFill(fill, historicalDeposit.deposit, blockRangeForChain); - return true; + } else { + assert(historicalDeposit.found === false); // Help tsc to narrow the discriminated union type. + allInvalidFills.push({ fill, code: historicalDeposit.code, reason: historicalDeposit.reason }); } - - assert(historicalDeposit.found === false); // Help tsc to narrow the discriminated union type. - allInvalidFills.push({ fill, code: historicalDeposit.code, reason: historicalDeposit.reason }); - return false; }; const _isChainDisabled = (chainId: number): boolean => { From ee01c7eb726f73196c529fa79174d91f29df9b45 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Fri, 5 Jan 2024 14:54:04 +0100 Subject: [PATCH 05/18] Additional cleanup --- src/clients/BundleDataClient.ts | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/clients/BundleDataClient.ts b/src/clients/BundleDataClient.ts index a98344055..abe16b9b2 100644 --- a/src/clients/BundleDataClient.ts +++ b/src/clients/BundleDataClient.ts @@ -286,19 +286,18 @@ export class BundleDataClient { const matchedDeposit = originClient.getDepositForFill(fill); if (matchedDeposit) { addRefundForValidFill(fill, matchedDeposit, blockRangeForChain); - return; - } - - // Matched deposit for fill was not found in spoke client. This situation should be rare so let's - // send some extra RPC requests to blocks older than the spoke client's initial event search config - // to find the deposit if it exists. - const spokePoolClient = spokePoolClients[fill.originChainId]; - const historicalDeposit = await queryHistoricalDepositForFill(spokePoolClient, fill); - if (historicalDeposit.found) { - addRefundForValidFill(fill, historicalDeposit.deposit, blockRangeForChain); } else { - assert(historicalDeposit.found === false); // Help tsc to narrow the discriminated union type. - allInvalidFills.push({ fill, code: historicalDeposit.code, reason: historicalDeposit.reason }); + // Matched deposit for fill was not found in spoke client. This situation should be rare so let's + // send some extra RPC requests to blocks older than the spoke client's initial event search config + // to find the deposit if it exists. + const spokePoolClient = spokePoolClients[fill.originChainId]; + const historicalDeposit = await queryHistoricalDepositForFill(spokePoolClient, fill); + if (historicalDeposit.found) { + addRefundForValidFill(fill, historicalDeposit.deposit, blockRangeForChain); + } else { + assert(historicalDeposit.found === false); // Help tsc to narrow the discriminated union type. + allInvalidFills.push({ fill, code: historicalDeposit.code, reason: historicalDeposit.reason }); + } } }; From 314073f1f7b86c18b65a577b99b78f998b81f637 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Fri, 5 Jan 2024 15:11:32 +0100 Subject: [PATCH 06/18] Relocate block range shuffling to proposer-only workflow --- src/dataworker/Dataworker.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/dataworker/Dataworker.ts b/src/dataworker/Dataworker.ts index 6eb26336e..26e920cdd 100644 --- a/src/dataworker/Dataworker.ts +++ b/src/dataworker/Dataworker.ts @@ -346,9 +346,11 @@ export class Dataworker { // safe strategy but could lead to new roots failing to be proposed until ALL networks are healthy. // If we are forcing a bundle range, then we should use that instead of the next proposal block ranges. + const widestBlockRanges = this._getNextProposalBlockRanges(spokePoolClients, earliestBlocksInSpokePoolClients); + const safeBlockRanges = await this.narrowProposalBlockRanges(widestBlockRanges, spokePoolClients); const blockRangesForProposal = isDefined(this.forceBundleRange) ? this.forceBundleRange - : this._getNextProposalBlockRanges(spokePoolClients, earliestBlocksInSpokePoolClients); + : safeBlockRanges; if (!blockRangesForProposal) { return; @@ -612,11 +614,8 @@ export class Dataworker { ): Promise { const timerStart = Date.now(); - // Dry-run the proposal over the input block ranges to identify any data inconsistencies. - const blockRanges = await this.narrowProposalBlockRanges(blockRangesForProposal, spokePoolClients); - const { fillsToRefund, deposits, allValidFills, unfilledDeposits, earlyDeposits } = - await this.clients.bundleDataClient._loadData(blockRanges, spokePoolClients, false, logData); + await this.clients.bundleDataClient._loadData(blockRangesForProposal, spokePoolClients, false, logData); const allValidFillsInRange = getFillsInRange( allValidFills, From fb12aa072dd769f12caeb62778c2800bc3a9d567 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Fri, 5 Jan 2024 15:14:50 +0100 Subject: [PATCH 07/18] Relocate block range narrowing further upstream in proposer workflow --- src/dataworker/Dataworker.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/dataworker/Dataworker.ts b/src/dataworker/Dataworker.ts index 26e920cdd..2f2b7163f 100644 --- a/src/dataworker/Dataworker.ts +++ b/src/dataworker/Dataworker.ts @@ -267,10 +267,10 @@ export class Dataworker { * of log level * @returns Array of blocks ranges to propose for next bundle. */ - _getNextProposalBlockRanges( + async _getNextProposalBlockRanges( spokePoolClients: SpokePoolClientsByChain, earliestBlocksInSpokePoolClients: { [chainId: number]: number } = {} - ): number[][] | undefined { + ): Promise { const { configStoreClient, hubPoolClient } = this.clients; // Check if a bundle is pending. @@ -330,7 +330,9 @@ export class Dataworker { return; } - return blockRangesForProposal; + // Narrow the block range depending on potential data incoherencies. + const safeBlockRanges = await this.narrowProposalBlockRanges(blockRangesForProposal, spokePoolClients); + return safeBlockRanges; } async proposeRootBundle( @@ -346,11 +348,9 @@ export class Dataworker { // safe strategy but could lead to new roots failing to be proposed until ALL networks are healthy. // If we are forcing a bundle range, then we should use that instead of the next proposal block ranges. - const widestBlockRanges = this._getNextProposalBlockRanges(spokePoolClients, earliestBlocksInSpokePoolClients); - const safeBlockRanges = await this.narrowProposalBlockRanges(widestBlockRanges, spokePoolClients); const blockRangesForProposal = isDefined(this.forceBundleRange) ? this.forceBundleRange - : safeBlockRanges; + : await this._getNextProposalBlockRanges(spokePoolClients, earliestBlocksInSpokePoolClients); if (!blockRangesForProposal) { return; From ccb5df0f16f253d891715d4ed6c4f6ecb10fd444 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Tue, 9 Jan 2024 01:45:16 +0100 Subject: [PATCH 08/18] Fix tests --- src/dataworker/Dataworker.ts | 4 ++-- test/Dataworker.buildRoots.ts | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/dataworker/Dataworker.ts b/src/dataworker/Dataworker.ts index 17d13a2d7..4aaa6a9c3 100644 --- a/src/dataworker/Dataworker.ts +++ b/src/dataworker/Dataworker.ts @@ -462,8 +462,8 @@ export class Dataworker { async narrowProposalBlockRanges( blockRanges: number[][], spokePoolClients: SpokePoolClientsByChain, - isUBA = false, - logData = false + logData = true, + isUBA = false ): Promise { const chainIds = this.chainIdListForBundleEvaluationBlockNumbers; const updatedBlockRanges = Object.fromEntries(chainIds.map((chainId, idx) => [chainId, [...blockRanges[idx]]])); diff --git a/test/Dataworker.buildRoots.ts b/test/Dataworker.buildRoots.ts index 8281f6d0c..2e8016560 100644 --- a/test/Dataworker.buildRoots.ts +++ b/test/Dataworker.buildRoots.ts @@ -1307,7 +1307,7 @@ describe("Dataworker: Build merkle roots", async function () { }, ]); - const blockRanges = dataworkerInstance._getNextProposalBlockRanges(spokePoolClients); + const blockRanges = await dataworkerInstance._getNextProposalBlockRanges(spokePoolClients); if (!blockRanges) { throw new Error("Can't propose new bundle"); } @@ -1334,7 +1334,7 @@ describe("Dataworker: Build merkle roots", async function () { ).to.be.true; }); it("0 flows", async function () { - const blockRanges = dataworkerInstance._getNextProposalBlockRanges(spokePoolClients); + const blockRanges = await dataworkerInstance._getNextProposalBlockRanges(spokePoolClients); if (!blockRanges) { throw new Error("Can't propose new bundle"); } @@ -1538,7 +1538,7 @@ describe("Dataworker: Build merkle roots", async function () { }, ]); - const blockRanges = dataworkerInstance._getNextProposalBlockRanges(spokePoolClients); + const blockRanges = await dataworkerInstance._getNextProposalBlockRanges(spokePoolClients); if (!blockRanges) { throw new Error("Can't propose new bundle"); } @@ -1744,7 +1744,7 @@ describe("Dataworker: Build merkle roots", async function () { }, ]); - const blockRanges = dataworkerInstance._getNextProposalBlockRanges(spokePoolClients); + const blockRanges = await dataworkerInstance._getNextProposalBlockRanges(spokePoolClients); if (!blockRanges) { throw new Error("Can't propose new bundle"); } From c716ee87503964a7467ccc10b5f076ac02689584 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Tue, 9 Jan 2024 05:56:26 +0100 Subject: [PATCH 09/18] Simplify end block updating --- src/dataworker/Dataworker.ts | 72 +++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 33 deletions(-) diff --git a/src/dataworker/Dataworker.ts b/src/dataworker/Dataworker.ts index 4aaa6a9c3..fe0bb9f34 100644 --- a/src/dataworker/Dataworker.ts +++ b/src/dataworker/Dataworker.ts @@ -475,6 +475,26 @@ export class Dataworker { logData ); + // Helper to update a chain's end block correctly, accounting for soft-pausing if needed. + const updateEndBlock = (chainId: number, endBlock?: number): void => { + const [currentStartBlock, currentEndBlock] = updatedBlockRanges[chainId]; + const previousEndBlock = this.clients.hubPoolClient.getLatestBundleEndBlockForChain( + chainIds, + this.clients.hubPoolClient.latestBlockSearched, + chainId + ); + + endBlock ??= previousEndBlock; + assert( + endBlock < currentEndBlock, + `Invalid block range update for chain ${chainId}: block ${endBlock} >= ${currentEndBlock}` + ); + + updatedBlockRanges[chainId] = endBlock > currentStartBlock + ? [currentStartBlock, endBlock] + : [previousEndBlock, previousEndBlock]; + }; + // If invalid fills were detected and they appear to be due to gaps in FundsDeposited events: // - Narrow the origin block range to exclude the missing deposit, AND // - Narrow the destination block range to exclude the invalid fill. @@ -493,16 +513,14 @@ export class Dataworker { .filter((deposit: DepositWithBlock) => deposit.blockNumber < blockNumber) .at(-1); - if (previousDeposit?.blockNumber ?? startBlock > startBlock) { - updatedBlockRanges[originChainId] = [startBlock, previousDeposit.blockNumber]; - this.logger.debug({ - at: "Dataworker::narrowBlockRanges", - message: `Narrowed proposal block range on ${originChain} due to missing deposit.`, - depositId, - previousBlockRange: [startBlock, endBlock], - newBlockRange: updatedBlockRanges[originChainId], - }); - } + updateEndBlock(originChainId, previousDeposit?.blockNumber); + this.logger.debug({ + at: "Dataworker::narrowBlockRanges", + message: `Narrowed proposal block range on ${originChain} due to missing deposit.`, + depositId, + previousBlockRange: [startBlock, endBlock], + newBlockRange: updatedBlockRanges[originChainId], + }); // Update the endBlock on the destination chain to exclude the invalid fill. const destSpokePoolClient = spokePoolClients[destinationChainId]; @@ -515,10 +533,7 @@ export class Dataworker { .at(-1); // Wind back to the bundle end block number to that of the previous fill. - const newEndBlock = previousFill?.blockNumber ?? startBlock; - updatedBlockRanges[destinationChainId] = - newEndBlock > startBlock ? [startBlock, newEndBlock] : [startBlock - 1, startBlock - 1]; // @fix: Must use previous end block! - + updateEndBlock(destinationChainId, previousFill?.blockNumber); this.logger.debug({ at: "Dataworker::narrowBlockRanges", message: `Narrowed proposal block range on ${destinationChain} due to missing deposit on ${originChain}.`, @@ -564,30 +579,21 @@ export class Dataworker { const deposit = newUnfilledDeposits[idx]; const { destinationChainId } = deposit; const chain = getNetworkName(deposit.destinationChainId); - this.logger.warn({ - at: "Dataworker::narrowBlockRange", - message: `Identified probable missing filledRelay event on ${chain} at block ${fillBlock}.`, - deposit, - }); - const [startBlock, endBlock] = updatedBlockRanges[destinationChainId]; const newEndBlock = allValidFills .filter((fill) => fill.destinationChainId === destinationChainId && fill.blockNumber < fillBlock) - .at(-1)?.blockNumber ?? startBlock; + .at(-1)?.blockNumber; - if (newEndBlock < endBlock) { - updatedBlockRanges[destinationChainId] = - newEndBlock > startBlock ? [startBlock, newEndBlock] : [startBlock - 1, startBlock - 1]; - - this.logger.debug({ - at: "Dataworker::narrowBlockRanges", - message: `Narrowed proposal block range on ${chain} due to missing fill event at block ${fillBlock}.`, - deposit, - previousBlockRange: [startBlock, endBlock], - newBlockRange: updatedBlockRanges[destinationChainId], - }); - } + const previousBlockRange = updatedBlockRanges[destinationChainId]; + updateEndBlock(destinationChainId, newEndBlock); + this.logger.warn({ + at: "Dataworker::narrowBlockRanges", + message: `Narrowed proposal block range on ${chain} due to missing fill event at block ${fillBlock}.`, + deposit, + previousBlockRange, + newBlockRange: updatedBlockRanges[destinationChainId], + }); } }); From a990ee79ec92043bddf012eccef1554f33947b49 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Tue, 9 Jan 2024 06:23:12 +0100 Subject: [PATCH 10/18] lint --- src/dataworker/Dataworker.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/dataworker/Dataworker.ts b/src/dataworker/Dataworker.ts index fe0bb9f34..6d5cad983 100644 --- a/src/dataworker/Dataworker.ts +++ b/src/dataworker/Dataworker.ts @@ -490,9 +490,8 @@ export class Dataworker { `Invalid block range update for chain ${chainId}: block ${endBlock} >= ${currentEndBlock}` ); - updatedBlockRanges[chainId] = endBlock > currentStartBlock - ? [currentStartBlock, endBlock] - : [previousEndBlock, previousEndBlock]; + updatedBlockRanges[chainId] = + endBlock > currentStartBlock ? [currentStartBlock, endBlock] : [previousEndBlock, previousEndBlock]; }; // If invalid fills were detected and they appear to be due to gaps in FundsDeposited events: @@ -580,10 +579,9 @@ export class Dataworker { const { destinationChainId } = deposit; const chain = getNetworkName(deposit.destinationChainId); - const newEndBlock = - allValidFills - .filter((fill) => fill.destinationChainId === destinationChainId && fill.blockNumber < fillBlock) - .at(-1)?.blockNumber; + const newEndBlock = allValidFills + .filter((fill) => fill.destinationChainId === destinationChainId && fill.blockNumber < fillBlock) + .at(-1)?.blockNumber; const previousBlockRange = updatedBlockRanges[destinationChainId]; updateEndBlock(destinationChainId, newEndBlock); From 51df1ca7f700bdfe1dcf67a40774b9a0c4e311f7 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Wed, 10 Jan 2024 06:42:44 +0100 Subject: [PATCH 11/18] Fix comment Suggested by Matt. Co-authored-by: Matt Rice --- src/dataworker/Dataworker.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dataworker/Dataworker.ts b/src/dataworker/Dataworker.ts index 6d5cad983..2293a5054 100644 --- a/src/dataworker/Dataworker.ts +++ b/src/dataworker/Dataworker.ts @@ -506,7 +506,7 @@ export class Dataworker { const originSpokePoolClient = spokePoolClients[originChainId]; let [startBlock, endBlock] = updatedBlockRanges[originChainId]; - // Find the previous known deposit. This may not be immediastely preceding depositId. + // Find the previous known deposit. This may resolve a deposit before the immediately preceding depositId. const previousDeposit = originSpokePoolClient .getDepositsForDestinationChain(destinationChainId) .filter((deposit: DepositWithBlock) => deposit.blockNumber < blockNumber) From 70f65e96e46496bc904c9756a57ec1ec2ef68d0e Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Wed, 10 Jan 2024 07:08:30 +0100 Subject: [PATCH 12/18] Nuke stray newline --- src/dataworker/Dataworker.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/dataworker/Dataworker.ts b/src/dataworker/Dataworker.ts index 2293a5054..3c7cbe7a5 100644 --- a/src/dataworker/Dataworker.ts +++ b/src/dataworker/Dataworker.ts @@ -617,7 +617,6 @@ export class Dataworker { logData = false ): Promise { const timerStart = Date.now(); - const { fillsToRefund, deposits, allValidFills, unfilledDeposits, earlyDeposits } = await this.clients.bundleDataClient._loadData(blockRangesForProposal, spokePoolClients, false, logData); From 093d80bb8ef10a60b9a9b4707981c41139f9be85 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Wed, 17 Jan 2024 11:24:52 +0100 Subject: [PATCH 13/18] Back out "missing fills" change Proceeding with the initial "invalid fills" change alone. The subsequent change can be re-introduced later. --- src/dataworker/Dataworker.ts | 54 +--------------------------- test/Dataworker.proposeRootBundle.ts | 2 +- 2 files changed, 2 insertions(+), 54 deletions(-) diff --git a/src/dataworker/Dataworker.ts b/src/dataworker/Dataworker.ts index 3c7cbe7a5..fa214eb06 100644 --- a/src/dataworker/Dataworker.ts +++ b/src/dataworker/Dataworker.ts @@ -468,7 +468,7 @@ export class Dataworker { const chainIds = this.chainIdListForBundleEvaluationBlockNumbers; const updatedBlockRanges = Object.fromEntries(chainIds.map((chainId, idx) => [chainId, [...blockRanges[idx]]])); - const { deposits, allValidFills, allInvalidFills } = await this.clients.bundleDataClient._loadData( + const { allInvalidFills } = await this.clients.bundleDataClient._loadData( blockRanges, spokePoolClients, isUBA, @@ -543,58 +543,6 @@ export class Dataworker { } }); - // For each deposit within the origin chain block range for which no or only partial fill events are found, - // verify whether the fill has been completed on the destination chain. If the fill has been made to completion - // then this is evidence of potential dropped/suppressed events, so narrow the block range to exclude those blocks. - const newUnfilledDeposits = deposits.filter(({ originChainId, depositId, amount, blockNumber }) => { - if (blockNumber > updatedBlockRanges[originChainId][1]) { - return false; // Fill event is already out of scope due to previous narrowing. - } - return allValidFills.find( - (fill) => - fill.depositId === depositId && fill.originChainId === originChainId && fill.totalFilledAmount.eq(amount) - ) - ? false - : true; - }); - - // For each unfilled or partially filled deposit, verify whether it was actually filled by the proposal end block. - const fillBlocks = await sdk.utils.mapAsync(newUnfilledDeposits, async (deposit: DepositWithBlock) => { - const { spokePool, deploymentBlock } = spokePoolClients[deposit.destinationChainId]; - const blockRange = updatedBlockRanges[deposit.destinationChainId]; - const endBlock = blockRange[1]; - - // @todo: Some tests rely on this; fix! - const startBlock = blockRange[0] > deploymentBlock ? blockRange[0] : deploymentBlock; - - // @todo: Beware the scenario where the fill is completed before the deposit, yet the deposit hash matches 100%. - // This corner case must be ruled out or mitigated before merge, because it will cause the proposer to throw. - return await sdk.utils.findFillBlock(spokePool, deposit as RelayData, startBlock, endBlock); - }); - - // Exclude each un- or partially-filled deposit that resolved to a complete fill. - fillBlocks.forEach((fillBlock, idx) => { - if (isDefined(fillBlock)) { - const deposit = newUnfilledDeposits[idx]; - const { destinationChainId } = deposit; - const chain = getNetworkName(deposit.destinationChainId); - - const newEndBlock = allValidFills - .filter((fill) => fill.destinationChainId === destinationChainId && fill.blockNumber < fillBlock) - .at(-1)?.blockNumber; - - const previousBlockRange = updatedBlockRanges[destinationChainId]; - updateEndBlock(destinationChainId, newEndBlock); - this.logger.warn({ - at: "Dataworker::narrowBlockRanges", - message: `Narrowed proposal block range on ${chain} due to missing fill event at block ${fillBlock}.`, - deposit, - previousBlockRange, - newBlockRange: updatedBlockRanges[destinationChainId], - }); - } - }); - // Quick sanity check - make sure that the block ranges are coherent. A chain may be soft-paused if it has ongoing // RPC issues (block ranges are frozen at the previous proposal endBlock), so ensure that this is also handled. const finalBlockRanges = chainIds.map((chainId) => updatedBlockRanges[chainId]); diff --git a/test/Dataworker.proposeRootBundle.ts b/test/Dataworker.proposeRootBundle.ts index 5550a8452..3587d1aa0 100644 --- a/test/Dataworker.proposeRootBundle.ts +++ b/test/Dataworker.proposeRootBundle.ts @@ -390,7 +390,7 @@ describe("Dataworker: Propose root bundle", async function () { expect(narrowedBlockRanges).to.deep.equal(safeBlockRanges); }); - it("Narrows proposal block ranges for missing fills (destination chain fill gaps)", async function () { + it.skip("Narrows proposal block ranges for missing fills (destination chain fill gaps)", async function () { // Since the SpokePoolClient relies on SpokePoolClient.latestDepositIdQueried, we can't currently detect // the _final_ deposit going missing. Some additional refactoring is needed to add this detection. const missingDepositIdx = Math.floor(Math.random() * (nDeposits - 2)); From 7a17005a93e5fdec20cd1b5fb4cf8c936fb0edb5 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Wed, 17 Jan 2024 11:39:42 +0100 Subject: [PATCH 14/18] lint --- src/dataworker/Dataworker.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/dataworker/Dataworker.ts b/src/dataworker/Dataworker.ts index fa214eb06..6e458d8a7 100644 --- a/src/dataworker/Dataworker.ts +++ b/src/dataworker/Dataworker.ts @@ -32,7 +32,6 @@ import { RunningBalances, PoolRebalanceLeaf, RelayerRefundLeaf, - RelayData, } from "../interfaces"; import { DataworkerClients } from "./DataworkerClientHelper"; import { SpokePoolClient, UBAClient, BalanceAllocator } from "../clients"; From 677c8b9d44835b9a75aedbd889640e1b9570beb0 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Wed, 17 Jan 2024 19:47:06 +0100 Subject: [PATCH 15/18] Add comment Proposed by Nick. Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com> --- src/dataworker/Dataworker.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/dataworker/Dataworker.ts b/src/dataworker/Dataworker.ts index 6e458d8a7..adfd91c1a 100644 --- a/src/dataworker/Dataworker.ts +++ b/src/dataworker/Dataworker.ts @@ -489,6 +489,7 @@ export class Dataworker { `Invalid block range update for chain ${chainId}: block ${endBlock} >= ${currentEndBlock}` ); + // If endBlock is equal to or before the currentEndBlock then we are "soft-pausing" the chain by setting the bundle start and end block equal to the previous end block. This effectively tells the dataworker to not progress on that chain. updatedBlockRanges[chainId] = endBlock > currentStartBlock ? [currentStartBlock, endBlock] : [previousEndBlock, previousEndBlock]; }; From c8673dcbc5b56b1d5cd65ee138cdc8215b5e3668 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Wed, 17 Jan 2024 20:16:43 +0100 Subject: [PATCH 16/18] reflow comment --- src/dataworker/Dataworker.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/dataworker/Dataworker.ts b/src/dataworker/Dataworker.ts index adfd91c1a..78c579fb5 100644 --- a/src/dataworker/Dataworker.ts +++ b/src/dataworker/Dataworker.ts @@ -489,7 +489,8 @@ export class Dataworker { `Invalid block range update for chain ${chainId}: block ${endBlock} >= ${currentEndBlock}` ); - // If endBlock is equal to or before the currentEndBlock then we are "soft-pausing" the chain by setting the bundle start and end block equal to the previous end block. This effectively tells the dataworker to not progress on that chain. + // If endBlock is equal to or before the currentEndBlock then the chain is "soft-paused" by setting the bundle + // start and end block equal to the previous end block. This tells the dataworker to not progress on that chain. updatedBlockRanges[chainId] = endBlock > currentStartBlock ? [currentStartBlock, endBlock] : [previousEndBlock, previousEndBlock]; }; From 75599485dd8c23cd0fa6768133b7fd94f30c3f27 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Wed, 17 Jan 2024 20:14:59 +0100 Subject: [PATCH 17/18] Clarify iterative updates --- src/dataworker/Dataworker.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/dataworker/Dataworker.ts b/src/dataworker/Dataworker.ts index 78c579fb5..94b82a6f2 100644 --- a/src/dataworker/Dataworker.ts +++ b/src/dataworker/Dataworker.ts @@ -526,6 +526,7 @@ export class Dataworker { const destSpokePoolClient = spokePoolClients[destinationChainId]; [startBlock, endBlock] = updatedBlockRanges[destinationChainId]; + // The blockNumber is iteratively narrowed in this loop so this fill might already be excluded. if (blockNumber <= endBlock) { // Find the fill immediately preceding this invalid fill. const previousFill = destSpokePoolClient From 56bef8c11007ae4633fd8cd9ad064ed5f787e39e Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Mon, 10 Jun 2024 12:17:56 +0200 Subject: [PATCH 18/18] lint --- test/Dataworker.proposeRootBundle.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Dataworker.proposeRootBundle.ts b/test/Dataworker.proposeRootBundle.ts index 5816d5498..d092f8f17 100644 --- a/test/Dataworker.proposeRootBundle.ts +++ b/test/Dataworker.proposeRootBundle.ts @@ -253,7 +253,7 @@ describe("Dataworker: Propose root bundle", async function () { erc20_1.address, amountToDeposit, erc20_2.address, - amountToDeposit, + amountToDeposit ); // Since the SpokePoolClient relies on SpokePoolClient.latestDepositIdQueried, we can't currently detect @@ -269,7 +269,7 @@ describe("Dataworker: Propose root bundle", async function () { erc20_1.address, amountToDeposit, erc20_2.address, - amountToDeposit, + amountToDeposit ); if (idx === missingDepositIdx) {