feat: Dynamically adjust proposal block ranges #1139
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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, | ||
|
|
@@ -263,10 +266,10 @@ export class Dataworker { | |
| * of log level | ||
| * @returns Array of blocks ranges to propose for next bundle. | ||
| */ | ||
| async _getNextProposalBlockRanges( | ||
|
There was a problem hiding this comment. It looks like this function only has one await and it's at the very last line. Thoughts on removing this |
||
| spokePoolClients: SpokePoolClientsByChain, | ||
| earliestBlocksInSpokePoolClients: { [chainId: number]: number } = {} | ||
| ): Promise<number[][] | undefined> { | ||
| const { configStoreClient, hubPoolClient } = this.clients; | ||
|
|
||
| // Check if a bundle is pending. | ||
|
|
@@ -326,7 +329,9 @@ export class Dataworker { | |
| return; | ||
| } | ||
|
|
||
| // Narrow the block range depending on potential data incoherencies. | ||
| const safeBlockRanges = await this.narrowProposalBlockRanges(blockRangesForProposal, spokePoolClients); | ||
| return safeBlockRanges; | ||
| } | ||
|
|
||
| async proposeRootBundle( | ||
|
|
@@ -344,7 +349,7 @@ export class Dataworker { | |
| // If we are forcing a bundle range, then we should use that instead of the next proposal block ranges. | ||
| const blockRangesForProposal = isDefined(this.forceBundleRange) | ||
| ? this.forceBundleRange | ||
| : await this._getNextProposalBlockRanges(spokePoolClients, earliestBlocksInSpokePoolClients); | ||
|
|
||
| if (!blockRangesForProposal) { | ||
| return; | ||
|
|
@@ -453,6 +458,105 @@ export class Dataworker { | |
| } | ||
| } | ||
|
|
||
| async narrowProposalBlockRanges( | ||
| blockRanges: number[][], | ||
| spokePoolClients: SpokePoolClientsByChain, | ||
| logData = true, | ||
| isUBA = false | ||
|
There was a problem hiding this comment. Will this need to be changed for the UBA removal changes? |
||
| ): Promise<number[][]> { | ||
| const chainIds = this.chainIdListForBundleEvaluationBlockNumbers; | ||
| const updatedBlockRanges = Object.fromEntries(chainIds.map((chainId, idx) => [chainId, [...blockRanges[idx]]])); | ||
|
|
||
| const { allInvalidFills } = await this.clients.bundleDataClient._loadData( | ||
| blockRanges, | ||
| spokePoolClients, | ||
| isUBA, | ||
| 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}` | ||
| ); | ||
|
Comment on lines
+496
to
+499
There was a problem hiding this comment. Since these updates can come in out of order, I would expect this case to happen, no? I find a missing deposit at block 7, then later I see a missing deposit at block 12. Maybe I'm missing the logic that prevents this. |
||
|
|
||
| updatedBlockRanges[chainId] = | ||
pxrl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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. | ||
| allInvalidFills | ||
| .filter(({ code }) => code === InvalidFill.DepositIdNotFound) | ||
| .forEach(({ fill: { depositId, originChainId, destinationChainId, blockNumber } }) => { | ||
|
There was a problem hiding this comment. Can we sort these by block number by chain to allow us to exit the following map earlier? There was a problem hiding this comment. I'm not totally confident on this, but I'm not sure that we can actually simply sort and exit earlier. This is because we need to avoid making assumptions about the ordering of fills vs. deposits. For example, the first invalid fill on a destination chain might correspond to the last missing deposit on the origin chain. Then, some later invalid fill on the destination chain might actually fill an earlier missing deposit on the origin chain. This also seems more likely to occur in the case that the RPCs are serving inconsistent data. If we group/sort by destinationChainId and destination block number, we might not narrow the originChainId block range correctly. If we group/sort originChainId and origin block number, we might not narrow the destinationChainid block range correctly. In general, even when it's really bad, the number of missing deposits we typically see is about 20 - 30. In order to identify both the earliest block for both origin and destination chains we might end up looping multiple times over all of the invalid fills. In the case where we only have tens missing events, it seems like it might be cheaper and simpler to just process them one by one. Full disclosure: I might have overlooked something really obvious here. wdyt? There was a problem hiding this comment. this is a good point, that the latest deposit might match with the earliest fill and vice versra |
||
| 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 resolve a deposit before the immediately preceding depositId. | ||
| const previousDeposit = originSpokePoolClient | ||
| .getDepositsForDestinationChain(destinationChainId) | ||
|
There was a problem hiding this comment. I don't think these are guaranteed to be sorted, which might mean this deposit would be too early, right? |
||
| .filter((deposit: DepositWithBlock) => deposit.blockNumber < blockNumber) | ||
| .at(-1); | ||
|
Comment on lines
+520
to
+523
There was a problem hiding this comment. What if the deposit is from a gap between spoke pool deployments or something? So there actually is no deposit for this id? Will this always return undefined in that case, which would mean no change? If there are edge cases like that that are handled in a subtle way here, we may want to add a brief comment explaining them. There was a problem hiding this comment. The SpokePoolClient only has the ability to handle a single SpokePool deployment, so in the case that we don't find any preceding deposit then I'll update the comment on line 509 to clarify this. There's arguably a scenario where the first deposit on a new chain goes missing. This would implicitly result in proposing over the range [0,0] for that chain because the HubPoolClient supplies previousEndBlock 0 in that scenario. I think this is an extremely remote probability, and activating a new chain requires multiple responsible adults to test the deployment and monitor the proposal, so I'm probably not inclined to handle it explicitly in the code. wdyt? There was a problem hiding this comment. what would a bundle block range look like for a new spoke pool address? whether it be a new chain or an updated spoke pool address for an existing chain? There was a problem hiding this comment. The bundlEndBlock is sourced from HubPoolClient.getLatestBundleEndBlockForChain(), so we inherit its behaviour. For a new deployment on an existing chain, we're bound to continue from the previous bundleEndBlock. This should work as expected. For a new chain, the chainId index isn't found in the previous bundle, so we default to proposing from 0. This is also as was the case for the activation of zkSync and Base - so we inherit the existing behaviour. The only change that I can foresee here is that if that initial proposal contains invalid fills due to missing deposits on the new chain, or fills for missing deposits on another chain, then we'd revert to proposing over [0,0]. I'm not sure whether that's a problem in itself, but it's a pretty extreme edge case because the number of deposits and fills for the new chain in the initial proposal are likely to be very low. and we'd detect it immediately because activating a new chain implies manual review of the proposal block ranges. There was a problem hiding this comment. I think the 0,0 case is remote and doesn't cause an obvious problem, so I wouldn't worry about handling it explicitly. This all makes sense. I think a comment that says something like: Would be really helpful to the reader. |
||
|
|
||
| updateEndBlock(originChainId, previousDeposit?.blockNumber); | ||
nicholaspai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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) { | ||
|
There was a problem hiding this comment. nit: wouldn't There was a problem hiding this comment. Not necessarily, because we source There was a problem hiding this comment. Clarified here: 7559948 |
||
| // Find the fill immediately preceding this invalid fill. | ||
| const previousFill = destSpokePoolClient | ||
| .getFillsWithBlockInRange(startBlock, Math.max(blockNumber - 1, startBlock)) | ||
nicholaspai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| .at(-1); | ||
|
|
||
| // Wind back to the bundle end block number to that of the previous fill. | ||
| updateEndBlock(destinationChainId, previousFill?.blockNumber); | ||
| 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], | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| // 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]); | ||
nicholaspai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const coherentBlockRanges = finalBlockRanges.every(([startBlock, endBlock], idx) => { | ||
| const [originalStartBlock] = blockRanges[idx]; | ||
| return ( | ||
| (endBlock > startBlock && startBlock === originalStartBlock) || | ||
| (startBlock === endBlock && startBlock === originalStartBlock - 1) // soft-pause | ||
|
There was a problem hiding this comment. Why would the new start block be one behind the original start block in this case? And is it okay to leave it that way? There was a problem hiding this comment.
To be more correct, rather than assuming that There was a problem hiding this comment. Understood. That makes sense. |
||
| ); | ||
| }); | ||
| assert(coherentBlockRanges, "Updated proposal block ranges are incoherent"); | ||
|
|
||
| return finalBlockRanges; | ||
| } | ||
|
|
||
| async Legacy_proposeRootBundle( | ||
| blockRangesForProposal: number[][], | ||
| spokePoolClients: SpokePoolClientsByChain, | ||
|
|
@@ -462,6 +566,7 @@ export class Dataworker { | |
| const timerStart = Date.now(); | ||
| const { fillsToRefund, deposits, allValidFills, unfilledDeposits, earlyDeposits } = | ||
| await this.clients.bundleDataClient._loadData(blockRangesForProposal, spokePoolClients, false, logData); | ||
|
|
||
| const allValidFillsInRange = getFillsInRange( | ||
| allValidFills, | ||
| blockRangesForProposal, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is annoying -- ts really should not require runtime code to force it to get that it must be this type. It won't compile without this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OOC @pxrl what is the error that is thrown?