Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Dynamically adjust proposal block ranges #1139

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/clients/BundleDataClient.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import assert from "assert";
import * as _ from "lodash";
import {
ProposedRootBundle,
Expand All @@ -16,7 +17,6 @@ import {
getRefundInformationFromFill,
queryHistoricalDepositForFill,
assign,
assert,
fixedPointAdjustment,
isDefined,
toBN,
Expand Down Expand Up @@ -45,6 +45,15 @@ import {
import { BundleDataSS } from "../utils/SuperstructUtils";
import { PoolRebalanceRoot } from "../dataworker/Dataworker";

// type DataCacheValue = {
// unfilledDeposits: UnfilledDeposit[];
// fillsToRefund: FillsToRefund;
// allValidFills: FillWithBlock[];
// allInvalidFills: InvalidFill[];
// deposits: DepositWithBlock[];
// earlyDeposits: typechain.FundsDepositedEvent[];
// };
// type DataCache = Record<string, DataCacheValue>;
type DataCache = Record<string, Promise<LoadDataReturnValue>>;

// V3 dictionary helper functions
Expand Down
112 changes: 107 additions & 5 deletions src/dataworker/Dataworker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
ZERO_ADDRESS,
} from "../utils";
import {
DepositWithBlock,
InvalidFill,
ProposedRootBundle,
RootBundleRelayWithBlock,
SpokePoolClientsByChain,
Expand Down Expand Up @@ -338,7 +340,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;
}

getNextHubChainBundleStartBlock(chainIdList = this.chainIdListForBundleEvaluationBlockNumbers): number {
Expand Down Expand Up @@ -467,6 +471,104 @@ export class Dataworker {
return rootBundleData.dataToPersistToDALayer;
}

async narrowProposalBlockRanges(
blockRanges: number[][],
spokePoolClients: SpokePoolClientsByChain
): Promise<number[][]> {
const chainIds = this.chainIdListForBundleEvaluationBlockNumbers;
const updatedBlockRanges = Object.fromEntries(chainIds.map((chainId, idx) => [chainId, [...blockRanges[idx]]]));

const { bundleInvalidFillsV3: invalidFills } = await this.clients.bundleDataClient.loadData(
blockRanges,
spokePoolClients
);

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// 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] =
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.
invalidFills
.filter(({ code }) => code === InvalidFill.DepositIdNotFound)
.forEach(({ fill: { depositId, originChainId, destinationChainId, blockNumber } }) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we sort these by block number by chain to allow us to exit the following map earlier?

Copy link
Contributor Author

@pxrl pxrl Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@pxrl pxrl Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 previousDeposit resolves to undefined. In this case, the updateEndBlock() helper detects that the updated endBlock is undefined and implements a soft-pause of the chain by proposing over the previous endBlock (as we do for Boba).

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@pxrl pxrl Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

This can return undefined if there is no known preceding deposit in range.
That will result in the chain being soft-paused by setting the endBlock equal to the previous endBlock, making this bundle cover no blocks on that chain.

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];

// The blockNumber is iteratively narrowed in this loop so this fill might already be excluded.
if (blockNumber <= endBlock) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: wouldn't blockNumbers for invalid fills always be within the bundle block range by definition when calling loadData?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily, because we source endBlock from updatedBlockRanges, so it is subject to being iteratively updated within this loop. So if we are examining a fill where the blockNumber has already been excluded by narrowing then we should just skip over that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@pxrl pxrl Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

originalStartBlock is normally previousEndBlock + 1, so if startBlock === endBlock is true then we're implementing a soft-pause on the chain, and will propose over the range of [previousEndBlock, previousEndBlock]. This is consistent with the way that chains are "hard-paused" (i.e. disabled via the ConfigStore), so this check is simply verifying the invariant.

To be more correct, rather than assuming that previousEndBlock === originalStartBlock - 1, this check should resolve the previous ending block via HubPoolClient.getLatestBundleEndBlockForChain().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. That makes sense.

);
});
assert(coherentBlockRanges, "Updated proposal block ranges are incoherent");

return finalBlockRanges;
}

async _proposeRootBundle(
blockRangesForProposal: number[][],
spokePoolClients: SpokePoolClientsByChain,
Expand All @@ -475,12 +577,12 @@ export class Dataworker {
logData = false
): Promise<ProposeRootBundleReturnType> {
const timerStart = Date.now();

pxrl marked this conversation as resolved.
Show resolved Hide resolved
const { bundleDepositsV3, bundleFillsV3, bundleSlowFillsV3, unexecutableSlowFills, expiredDepositsToRefundV3 } =
await this.clients.bundleDataClient.loadData(blockRangesForProposal, spokePoolClients, loadDataFromArweave);
// Prepare information about what we need to store to
// Arweave for the bundle. We will be doing this at a
// later point so that we can confirm that this data is
// worth storing.

// Prepare information about what we need to store to Arweave for the bundle.
// We will be doing this at a later point so that we can confirm that this data is worth storing.
const dataToPersistToDALayer = {
bundleBlockRanges: blockRangesForProposal,
bundleDepositsV3,
Expand Down
11 changes: 10 additions & 1 deletion src/interfaces/SpokePool.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
import { SpokePoolClient } from "../clients";
import { FillWithBlock, SpokePoolClient } from "../clients";
import * as utils from "../utils/SDKUtils";

export interface SpokePoolClientsByChain {
[chainId: number]: SpokePoolClient;
}

export const { InvalidFill } = utils;

export type InvalidFill = {
fill: FillWithBlock;
code: utils.InvalidFillEnum;
reason: string;
};
1 change: 1 addition & 0 deletions src/interfaces/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,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;
Expand Down
3 changes: 3 additions & 0 deletions src/utils/SDKUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -23,6 +25,7 @@ export const {
toGWei,
toBNWei,
formatFeePct,
InvalidFill,
shortenHexStrings,
convertFromWei,
max,
Expand Down
1 change: 1 addition & 0 deletions test/Dataworker.loadData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ describe("Dataworker: Load data used in all functions", async function () {
bundleDepositsV3: {},
expiredDepositsToRefundV3: {},
bundleFillsV3: {},
bundleInvalidFillsV3: [],
unexecutableSlowFills: {},
bundleSlowFillsV3: {},
});
Expand Down
Loading
Loading