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

Do not settle after the deadline #3116

Merged
merged 8 commits into from
Nov 15, 2024
Merged

Conversation

squadgazzz
Copy link
Contributor

@squadgazzz squadgazzz commented Nov 8, 2024

Description

2 colocated solvers reported that /settle requests were sent after the deadline. The expected behaviour is that on each new block a new auction is cut. So for each new settlement the deadline should be the auction.block + settlement_deadline. It turned out that this is not the case and after the auction cut, the deadline is calculated as current_block + settlement_deadline. E.g(deadline config is 3):

  • Auction cut at block 10 in the end of the round.
  • New block 11 is mined.
  • Only then the deadline is calculated as 11+3=14.
  • The next auction is being cut at block 11 and the deadline for the next settlement is also 11+3=14.
  • The first settlement gets executed in the end of the block 13 round.
  • A new block 14 gets mined and the next settle call is sent after the deadline.

This should probably explain how the settle call could be sent after the deadline.

Changes

  • Calculate the deadline based on the auction block, not the current.
  • To properly validate the approach and make sure this never happens again, the proposal is to not send the /settle request once the deadline is reached.
  • Another observation is that a log in the wait_for_settlement_transaction function uses a newly calculated deadline block number for some reason. The PR fixes that and uses the same value as was used in the /settle request. That should help better understand the root cause.

So in the end the expected behaviour should be observed, where each subsequent settle request has a deadline for at least 1 block ahead the previous deadline. So if a settlement gets executed in the end of the round 13 or gets discarded at block 14, the next settlement still have the deadline for block 15 which should be sufficient.

How to test

Existing tests. This might require increasing the settlement deadline by 1.

@squadgazzz squadgazzz requested a review from a team as a code owner November 8, 2024 11:07
Copy link
Contributor

@mstrug mstrug left a comment

Choose a reason for hiding this comment

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

Looks ok for me.

@squadgazzz squadgazzz marked this pull request as draft November 8, 2024 16:20
@squadgazzz squadgazzz marked this pull request as ready for review November 8, 2024 17:06
@squadgazzz squadgazzz marked this pull request as draft November 8, 2024 18:38
@squadgazzz squadgazzz marked this pull request as ready for review November 8, 2024 18:55
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

A bit unfortunate that we are running into these issues with very competitive solvers that win multiple auctions in a row.
Maybe we should consider moving the queuing logic into the driver itself. That way at least teams running their own driver could implement a nonce management solution that properly allows for submitting multiple solutions in parallel. This seems to fit the overall system better.

crates/autopilot/src/run_loop.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@m-lord-renkse m-lord-renkse left a comment

Choose a reason for hiding this comment

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

LGTM

@squadgazzz
Copy link
Contributor Author

squadgazzz commented Nov 15, 2024

A bit unfortunate that we are running into these issues with very competitive solvers that win multiple auctions in a row.
Maybe we should consider moving the queuing logic into the driver itself. That way at least teams running their own driver could implement a nonce management solution that properly allows for submitting multiple solutions in parallel. This seems to fit the overall system better.

After thinking more about it, I think that would be a much better approach. So, drivers should maintain the settlement order. I am not happy with the current solution since it requires artificially increasing the block deadline, which could lead to a longer auction execution time. Will try to tackle it.

@squadgazzz squadgazzz enabled auto-merge (squash) November 15, 2024 15:33
@squadgazzz squadgazzz merged commit 1fe547f into main Nov 15, 2024
11 checks passed
@squadgazzz squadgazzz deleted the do-not-settle-after-deadline branch November 15, 2024 15:37
@github-actions github-actions bot locked and limited conversation to collaborators Nov 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants