driver: parallelize unsupported order detection#4347
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the competition logic to parallelize unsupported order detection by updating the risk detector API to return a set of UIDs instead of modifying the auction in place. A critical logic error was identified in the new unsupported_order_uids method where orders flagged as unsupported by metrics are filtered out of the processing loop but never added to the removal set, effectively bypassing the intended filtering logic.
|
@jmg-duarte This is minimal, easily reviewable and I think in line with what is expected for resolving the linked issue. Just finishing off what I started. |
metalurgical
force-pushed
the
fix_chore_3516
branch
from
16e3345 to
dcfb3b8
Compare
metalurgical
force-pushed
the
fix_chore_3516
branch
from
a1b49e8 to
e5919f9
Compare
Make unsupported order detection read-only and execute it in parallel with sorting and data fetching. Apply filtering after update_orders to preserve existing ordering.
metalurgical
force-pushed
the
fix_chore_3516
branch
from
e5919f9 to
8ae2e9b
Compare
|
@AryanGodara Thank you for the comments. It should be ready to be reviewed again. Excuse the force push, had to rebase a branch in to ensure it was working and then rebase it out again. |
There was a problem hiding this comment.
Code Review
This pull request refactors the risk detection logic by introducing a SellQualityDetector trait to decouple the simulation detector and improve testability. It updates the Competition and Quote domains to use the refactored filter_unsupported_orders_in_auction and unsupported_order_uids methods, enabling in-place filtering of unsupported orders. Additionally, comprehensive unit tests were added for the Detector logic. I have no feedback to provide as the review comments provided did not meet the high severity threshold required by the style guide.
AryanGodara
left a comment
AryanGodara
left a comment
There was a problem hiding this comment.
Description nit: "Run unsupported detection in parallel in solve()" fit the original version, but not entirely accurate imo after the prev commits
The real wins here are the read-only unsupported_order_uids API, so quote.rs can reuse detection without faking an Auction, and the SellQualityDetector trait that enables the new tests. Worth surfacing those in the description instead.
otherwise lgtm
Yes, this is true, with the latest commits it is effectively concurrent with liquidity fetching. Edit: I have updated this to use |
metalurgical
changed the title
| solver::Liquidity::Fetch => tasks.liquidity.await, | ||
| solver::Liquidity::Skip => Arc::new(Vec::new()), | ||
| let (auction, liquidity) = tokio::join!( | ||
| tokio::spawn( |
There was a problem hiding this comment.
tokio::spawn returns detached JoinHandles. tokio::join! awaits them, but if the outer solve() future is dropped (e.g. client disconnect, overload shed), the spawned tasks keep running to completion. The previous inline tokio::join!(async { … }, future) would have been cancelled with the parent.
There was a problem hiding this comment.
I see your point. Though this behavior is not new here, it already exists with run_blocking_with_timer.
I would like to suggest that you have a look at #4379. The same concept there can be used for this as well, just couple it with a scopeguard. This is also probably better implemented there as well, else it is duplicating efforts.
There was a problem hiding this comment.
Wouldn't this be solveable by simply using a cancellationtoken + dropguard?
The PR you're pointing to is huge and the scope/steps need to be discussed
There was a problem hiding this comment.
Yes that is it.
The PR I am pointing to already has the cancellation token threaded through. It appears large but much of it is tests and plumbing. Did subsequently open an issue for it, #4380. So it just needs the drop guard added to it (and an patched in here) then the above behavior mentioned here will be handled as well.
Happy to discuss scope and steps further.
squadgazzz
left a comment
squadgazzz
left a comment
There was a problem hiding this comment.
Two notes on the c06963a refactor.
4 participants
Description
Allow unsupported order detection be executed in parallel and optimize unsupported order detection.
Changes
without_unsupported_ordersto risk detector.without_unsupported_ordersto do all unsupported order filtering (including flashloans) in one pass.DetectorApitrait to decouple the simulation detector for tests.tokio::spawninsolve(), allowing parallel execution.quote.rsto usewithout_unsupported_orders.How to test
cargo test -p driver
Related Issues
Fixes #3516, follow up PR to #4309 and taking into account #4329