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

fix(Relayer): exclusivityDeadline is inclusive not exclusive #2026

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nicholaspai
Copy link
Member

No description provided.

bmzig
bmzig previously approved these changes Jan 29, 2025
pxrl
pxrl previously approved these changes Jan 29, 2025
@@ -294,7 +294,7 @@ export class Relayer {

if (
deposit.exclusiveRelayer !== ZERO_ADDRESS &&
deposit.exclusivityDeadline > currentTime &&
deposit.exclusivityDeadline >= currentTime &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I am fwiw curious about how simulation works here, since if exclusivityDeadline === currentTime then you'd expect that by the time our transaction is simulated & submitted, the exclusivityDeadline should be in the past.

So I'm wondering whether we're seeing some quirk of simulation here, where the block timestamp used in simulation is from the previous block. I have to admit that I'm generally a bit vague about exactly what chain state is used when we are simulating.

In the worst case, this change might cause us to be slower to make fills with lapsed exclusivity, or given current SpokePool implementation, all fills (i.e. it might implicitly impose a 1 block confirmation delay). This would be especially noticeable where the destination chain has long block-ish times - i.e. mainnet, Linea, Scroll.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's an example of something we might consider: #2027

We could alternatively consider adding "NotExclusiveRelayer" to the set of "acceptable failure" cases and suppress the simulation error, as we to for "RelayFilled" errors. The "NotExclusiveRelayer" scenario isn't triggering as frequently as it was before the SpokePool upgrade and associated config tweaks, so I think any adjustment here is now mostly about making the logs a bit quieter.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you recommend we do?

Copy link
Member Author

Choose a reason for hiding this comment

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

however, I still think this PR is correct and it shouldn't really delay deposits that much. How often are we finding an exclusivityDeadline exactly equal to currentTime? Seems like it'd be small

Copy link
Member Author

Choose a reason for hiding this comment

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

@nicholaspai nicholaspai dismissed stale reviews from pxrl and bmzig via 3518ff7 January 29, 2025 14:22
@nicholaspai nicholaspai requested a review from pxrl January 29, 2025 14:38
Copy link
Contributor

@james-a-morris james-a-morris left a comment

Choose a reason for hiding this comment

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

My gut feel is that this is a relatively small edge case given how unlikely it would be to be in this situation. LGTM good catch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants