improve(BundleDataClient,SpokePoolClient): Log about duplicate events and `getLatestProposedBundleData` should rarely load data from scratch #884

nicholaspai · 2025-02-10T17:07:21Z

This PR will make the relayer faster when the current bundle proposal is pending but not published to Arweave and more resilient to errors thrown during bundle reconstruction from scratch.

We want to know when these events happen but we currently believe this is possible with the Indexed spoke pool client so we shouldn't throw. We'll log them in the SpokePoolClient
The relayer reconstructs bundle data to compute the latest running balances in order to choose a repayment chain. We don't want the relayer to ever have to reconstruct data from scratch, instead it should always load it from arweave. This also avoids any safety errors thrown by the BundleDataClient when it detects a duplicate event. Keeping these errors in keeps the bundle data client safe when loadDataFromScratch is called by the proposer and disputer

We want to know when these events happen but we currently believe this is possible with the Indexed spoke pool client so we shouldn't throw.

src/clients/SpokePoolClient.ts

bmzig · 2025-02-10T17:46:09Z

src/clients/SpokePoolClient.ts

+              return e.transactionHash === deposit.transactionHash && e.logIndex === deposit.logIndex;
+            })
+          ) {
+            this.logger.warn({


If this is ever logged, this basically means that there is a bug somewhere. I think this should be an error.

this might take down the fast relayer though, no?

I think the relayers are the only bots which do not stop on errors, since they error a lot when txns revert in simulation. @pxrl could probably confirm that I'm not mistaken, though.

I'm not sure we should crash here. I'd rather just ignore the event and try our best to keep the events stored in memory correct by filtering out duplicates

Ideally, this should never trigger, though. I'd argue that if this is logged, then we introduced a bug somewhere and should look into recent commits. Right now, we know that this would occasionally log in the relayer, but if this happened somewhere else in the future, we'd probably want to look into it. Warn logs just seem to be drowned out by the many other warn logs we emit elsewhere.

hm so if we crashed here, this would bring down the fast relayer which seems to handlese duplicate events ok today. Are we ok with that? yes it'd help us debug, but also be disprutive to production service. WDYT?

If the fast relayer crashes on errors, then I agree, we should not log an error and go with what you currently have. I don't think the fast relayer crashes on logged errors, though. Am I wrong?

Errors result in a page being emitted and they're very visible in Slack, but the bot should continue to run.

Warnings are also reasonably visible in Slack but don't result in anyone getting paged.

Depending on how frequently this gets emitted, it could be a blessing or a curse. It'd be nice to avoid behaviour where it triggered a lot of rapid alerts. Given that it's in the SpokePoolClient update() function, I think it'd probably alert once and then disappear, so I think error should be OK.

pxrl · 2025-02-10T21:17:34Z

src/clients/BundleDataClient/BundleDataClient.ts

+      (d) => d.transactionHash === deposit.transactionHash && d.logIndex === deposit.logIndex
+    )
+  ) {
+    throw new Error("Duplicate deposit in bundleDeposits");


Can we add a log message here to identify the relevant deposits? That will speed up debugging time significantly if we're able to isolate them.

pxrl · 2025-02-10T21:22:23Z

src/clients/BundleDataClient/BundleDataClient.ts

+    let n = hubPoolClient.hasPendingProposal() ? 1 : 2;
+
+    // eslint-disable-next-line no-constant-condition
+    while (true) {


Should we maybe make this conditional on n < something? It'd suck to somehow end up in an infinite loop here.

Suggested change

while (true) {

while (n++ < 4) {

wdyt of faebdb8 where we fallback to just loading from scratch.

pxrl · 2025-02-10T21:32:04Z

src/clients/SpokePoolClient.ts

+        if (
+          this.fills[fill.originChainId] !== undefined &&
+          this.fills[fill.originChainId].some(
+            (f) => f.transactionHash === fill.transactionHash && f.logIndex === fill.logIndex
+          )
+        ) {


Prettier probably complains about this formatting, but it should be possible to drop the first check and simplify the statements.

Suggested change

if (

this.fills[fill.originChainId] !== undefined &&

this.fills[fill.originChainId].some(

(f) => f.transactionHash === fill.transactionHash && f.logIndex === fill.logIndex

)

) {

const duplicate = this.fills[fill.originChainId]?.find((f) => f.transactionHash === fill.transactionHash && f.logIndex === fill.logIndex);

if (duplicate) {

nicholaspai · 2025-02-11T00:07:08Z

@bmzig @pxrl if we do experience duplicate events, I can imagine the logs would get very noisy and be distracting. WDYT about emitting one error level log per update() run? 7545cf2

bmzig · 2025-02-11T15:14:29Z

src/clients/BundleDataClient/BundleDataClient.ts

+    // Check if bundle data exists on arweave, otherwise fallback to last published bundle data. If the
+    // first bundle block range we are trying is the pending proposal, then we'll grab the most recently
+    // validated bundle, otherwise we'll grab the second most recently validated bundle.
+    let n = hubPoolClient.hasPendingProposal() ? 1 : 2;


Why is it OK to do this? This function now won't necessarily return the latest proposed bundle data, and will instead likely return an older bundle's arweave data. Wouldn't this mean that calculating the "latest" pool rebalance root isn't actually going to calculate the latest root, and instead some older one (unless the latest data has been published).

maybe we should re-name this function to getLatestProposedRootWithArweaveData because if you look at how this function is used, its used by the InventoryClient here to get the best approximation of the latest running balances in order to compute which chains are "over-allocated" here.

So, when a bundle is first proposed but doesn't have its bundle data published to arweave, there is really little negative consequence for the InventoryClient. The InventoryClient will use an outdated running balance until the arweave bundle data is published. If we assume the arweave data gets published once a challenge period, then at worst the "latest" pool rebalance root will be one bundle behind. In reality, it only lags by ~15 mins until the bundle data is very likely published to Arweave.

2b6a8dd

Improves relayer and monitor performance by loading more data from arweave optimistically Depends on across-protocol/sdk#884

improve(BundleDataClient): Log about duplicate destination chain events

b0a7a23

We want to know when these events happen but we currently believe this is possible with the Indexed spoke pool client so we shouldn't throw.

nicholaspai requested review from bmzig and pxrl February 10, 2025 17:07

Log duplicate deposits in spoke pool client

55783b7

nicholaspai changed the title ~~improve(BundleDataClient): Log about duplicate destination chain events~~ improve(BundleDataClient,SpokePoolClient): Log about duplicate events Feb 10, 2025

bmzig reviewed Feb 10, 2025

View reviewed changes

src/clients/SpokePoolClient.ts Show resolved Hide resolved

bmzig reviewed Feb 10, 2025

View reviewed changes

nicholaspai added 2 commits February 10, 2025 12:55

getLatestProposedBundleData should never load data from scratch

9850552

lint

1ab05cc

nicholaspai changed the title ~~improve(BundleDataClient,SpokePoolClient): Log about duplicate events~~ improve(BundleDataClient,SpokePoolClient): Log about duplicate events and getLatestProposedBundleData should never load data from scratch Feb 10, 2025

Update BundleDataClient.ts

7c30e1a

nicholaspai requested a review from bmzig February 10, 2025 18:01

Update BundleDataClient.ts

8c066af

pxrl reviewed Feb 10, 2025

View reviewed changes

nicholaspai added 4 commits February 10, 2025 16:34

Update SpokePoolClient.ts

ce0be1d

Update BundleDataClient.ts

bf3bae6

exit after n == 4

faebdb8

Update SpokePoolClient.ts

42fb574

nicholaspai requested a review from pxrl February 10, 2025 21:51

pxrl approved these changes Feb 10, 2025

View reviewed changes

Update BundleDataClient.ts

04d3592

reduce noisiness of logs

7545cf2

Update BundleDataClient.ts

2df77cf

nicholaspai requested a review from pxrl February 11, 2025 00:11

Update SpokePoolClient.ts

3f802dc

bmzig reviewed Feb 11, 2025

View reviewed changes

nicholaspai added 2 commits February 11, 2025 10:16

Clean up logs

f2429b8

Update BundleDataClient.ts

2b6a8dd

nicholaspai requested a review from bmzig February 11, 2025 15:21

nicholaspai added 2 commits February 11, 2025 10:39

Update BundleDataClient.ts

34ce1e0

Update BundleDataClient.ts

ea48db5

nicholaspai mentioned this pull request Feb 11, 2025

improve: Use optimized version of getLatestPoolRebalanceRoot across-protocol/relayer#2085

Draft

Update BundleDataClient.ts

331dd34

bmzig approved these changes Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve(BundleDataClient,SpokePoolClient): Log about duplicate events and `getLatestProposedBundleData` should rarely load data from scratch #884

improve(BundleDataClient,SpokePoolClient): Log about duplicate events and `getLatestProposedBundleData` should rarely load data from scratch #884

nicholaspai commented Feb 10, 2025 •

edited

Loading

bmzig Feb 10, 2025

nicholaspai Feb 10, 2025

bmzig Feb 10, 2025

nicholaspai Feb 10, 2025

bmzig Feb 10, 2025 •

edited

Loading

nicholaspai Feb 10, 2025

bmzig Feb 10, 2025

pxrl Feb 10, 2025

nicholaspai Feb 10, 2025

pxrl Feb 10, 2025

nicholaspai Feb 10, 2025

pxrl Feb 10, 2025 •

edited

Loading

nicholaspai Feb 10, 2025

pxrl Feb 10, 2025

nicholaspai Feb 10, 2025

nicholaspai commented Feb 11, 2025

bmzig Feb 11, 2025

nicholaspai Feb 11, 2025

improve(BundleDataClient,SpokePoolClient): Log about duplicate events and getLatestProposedBundleData should rarely load data from scratch #884

Are you sure you want to change the base?

improve(BundleDataClient,SpokePoolClient): Log about duplicate events and getLatestProposedBundleData should rarely load data from scratch #884

Conversation

nicholaspai commented Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmzig Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pxrl Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholaspai commented Feb 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

improve(BundleDataClient,SpokePoolClient): Log about duplicate events and `getLatestProposedBundleData` should rarely load data from scratch #884

improve(BundleDataClient,SpokePoolClient): Log about duplicate events and `getLatestProposedBundleData` should rarely load data from scratch #884

nicholaspai commented Feb 10, 2025 •

edited

Loading

bmzig Feb 10, 2025 •

edited

Loading

pxrl Feb 10, 2025 •

edited

Loading