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

Solver participation guard e2e test #3280

Open
wants to merge 12 commits into
base: db-statistics-based-guard
Choose a base branch
from

Conversation

squadgazzz
Copy link
Contributor

@squadgazzz squadgazzz commented Feb 13, 2025

Implements e2e tests for all the solver participation guards from #3263 and #3257. Simulates the settlement failures by altering the DB. Another alternative is to use a broken MEV blocker URL, which is considered less flexible since some of the solutions need to be successfully settled to test at least the statistic-based approach.

The approach is a bit awkward due to the async nature of the guards implementation. Thinking more about the sync approach, there will still be the same behavior since a new auction is being cut while the previous one is being settled, meaning the sync approach will also allow the non-settling solver to submit one more solution before getting a ban.

@squadgazzz squadgazzz marked this pull request as ready for review February 13, 2025 21:06
@squadgazzz squadgazzz requested a review from a team as a code owner February 13, 2025 21:06
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.

There is a dozen of repeated lines in the setup sections in each of the three tests, so it could be extracted to one function like setup().

.await
.is_err()
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICS this e2e test only tests half of the feature - disabling the solver. Could we also test that after a while the solver is allowed to participate again?

Copy link
Contributor Author

@squadgazzz squadgazzz Feb 18, 2025

Choose a reason for hiding this comment

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

That requires tokio::pause() + advance() usage since making the TTL for the banned solvers cache too low will make the test flaky. Waiting for too long in real-time is not acceptable. Will try to play with the pause/advance to make it work with anvil, etc.

My bad, this doesn't require pause/advance since the cache is not involved in checking the Auth contract.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can start a test with already disabled solver and check if it is enabled for the next auction (this will required some state solver guard setup which I'm not sure is possible to initialize).

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, this doesn't require pause/advance since the cache is not involved in checking the Auth contract.

But the e2e should not just test the auth contract check, right?
To test the cache itself couldn't you also just set a very low TTL (e.g. 5s) and actually wait for the necessary amount of wall clock time?

Copy link
Contributor Author

@squadgazzz squadgazzz Feb 19, 2025

Choose a reason for hiding this comment

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

The cache is not involved when calling the is_solver smart contract function.

To test the db-based approaches cache expiration, I think it is impossible to find a proper value. The cache gets updated in a background task when autopilot already received solutions from the driver, so it will participate in that competition. Once the cache contains the problematic solver, only on the next auction round it won't be able to participate. The test also needs to make sure that the solver is banned, it waits for some amount of time that there are no trades. All this time spent is unpredictable.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a pretty big problem. Calling is_solver is trivial compared to the DB query and caching logic and since the e2e was supposed to give confidence of the correctness of the solution it's not doing its job AFAICS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can create a long-running test, which will be excluded from the local_node tests run. Will think about other workarounds again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to find a reasonable TTL to both not delay the execution and avoid flakiness: 8082206

@squadgazzz squadgazzz force-pushed the solvers-guard-e2e-test branch from 1ac6b91 to a4c14a2 Compare February 18, 2025 15:52
@squadgazzz squadgazzz force-pushed the solvers-guard-e2e-test branch from a4c14a2 to 930a80a Compare February 18, 2025 15:52
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