refactor: parallelize unsupported-order detection and defer filtering by metalurgical · Pull Request #4309 · cowprotocol/services

metalurgical · 2026-04-05T20:23:25Z

Description

Parallelize unsupported-order detection and defer filtering

Changes

Switch risk detection to return UIDs,
run it concurrently with preprocessing,
apply filtering after updating orders.
~~- Replace in-place sorting with permutation-based ordering to avoid cloning orders.~~

How to test

As normal.

Related Issues

Fixes #3516

Note

~~Built on top of #4308~~ Split from PR

github-actions · 2026-04-05T20:23:38Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

metalurgical · 2026-04-05T20:23:57Z

I have read the CLA Document and I hereby sign the CLA

metalurgical · 2026-04-05T20:24:10Z

recheck

gemini-code-assist

Code Review

This pull request enhances cross-platform support and optimizes the auction preprocessing pipeline. Significant changes include making memory allocators and system signals platform-specific, updating database configuration for Windows compatibility, and refactoring order sorting and filtering to minimize cloning and improve parallelism. No critical issues found.

jmg-duarte · 2026-04-06T08:33:35Z

I believe this change is built on top of your other PR, please separate them for easier review

jmg-duarte

The typo fixes and import shortening changes are appreciated overall but in this PR they bring in noise to a review that must be done carefully

The parallelism change is also semi-buried in the sorting order refactor

The sorting order refactor is non-trivial and it lacks safety justifications as to why it works. It could also be abstracted/simplified since it doesn't matter if a vector contains T, Z, or A if we're ignoring the type and using another slice as "sorting reference"

Finally, I don't understand why there's so much documentation being removed

metalurgical · 2026-04-06T20:26:05Z

@jmg-duarte You raised valid points. Have simplified it to minimal correction for the issue, restored the documentation comments and removed the permutation based sorting.

Move unsupported-order detection to a parallel, read-only stage in the auction preprocessing pipeline. Detection now operates on a snapshot of orders and returns UIDs to discard, rather than mutating the auction early.

jmg-duarte · 2026-04-09T09:00:13Z

Hey @metalurgical thanks for the changes! It looks much better now, I haven't had a chance of giving them a through review yet; if you don't mind resolving the conflicts, that would be great.

Full disclosure: The PR will need to go through internal testing to ensure there's no performance regressions, I'll take care of that and either bring comments or iterate directly on it myself (depends on outcomes); however, it might take a bit to do so due to this not being a top priority now.

Once more, thanks for your contribution

jmg-duarte

This review is mostly for myself as I've already addressed these issues in a local branch, I just can't forget to actually push the fixes.
I'll push the changes, I'll also revert the import pathing changes as it distracts from the important parts of the PR

jmg-duarte · 2026-04-13T13:09:44Z

+    /// Filters unsupported orders out of the auction.
+    pub async fn filter_unsupported_orders_in_auction(
+        &self,
+        mut auction: crate::domain::competition::Auction,
+    ) -> crate::domain::competition::Auction {
+        let removed_uids = self.unsupported_order_uids(&auction.orders).await;
+        if !removed_uids.is_empty() {
+            auction
+                .orders
+                .retain(|order| !removed_uids.contains(&order.uid));
+        }
+        auction
+    }


jmg-duarte · 2026-04-13T13:09:59Z

+        let orders_for_unsupported_filter: Vec<competition::Order> = auction.orders.clone();
+        let unsupported_uids = risk_detector
+            .unsupported_order_uids(&orders_for_unsupported_filter)
            .await;


Suggested change

let orders_for_unsupported_filter: Vec<competition::Order> = auction.orders.clone();

let unsupported_uids = risk_detector

.unsupported_order_uids(&orders_for_unsupported_filter)

.await;

let unsupported_uids = risk_detector

.unsupported_order_uids(&auction.orders)

.await;

jmg-duarte · 2026-04-13T13:10:40Z

+        let mut auction = auction;
+        if !unsupported_uids.is_empty() {
+            auction
+                .orders
+                .retain(|order| !unsupported_uids.contains(&order.uid));
        }


nit to restrict mutability's "lifetime"

Suggested change

let mut auction = auction;

if !unsupported_uids.is_empty() {

auction

.orders

.retain(|order| !unsupported_uids.contains(&order.uid));

}

let auction = {

let mut auction = auction;

if !unsupported_uids.is_empty() {

auction

.orders

.retain(|order| !unsupported_uids.contains(&order.uid));

};

auction

};

jmg-duarte · 2026-04-13T13:11:32Z

+use std::sync::Arc;
 use {


This won't pass CI, be sure to test stuff locally, we have the just scripts etc

jmg-duarte · 2026-04-13T13:13:47Z

        }

-        auction
+        removed_uids


Suggested change

removed_uids

unsupported_uids

jmg-duarte · 2026-04-13T13:17:16Z

+                    let order = order.clone();
+                    let check_tokens_fut = async move {
+                        let quality = detector.determine_sell_token_quality(&order, now).await;
+                        (order.uid, quality)
+                    };


Suggested change

let order = order.clone();

let check_tokens_fut = async move {

let quality = detector.determine_sell_token_quality(&order, now).await;

(order.uid, quality)

};

let check_tokens_fut = async move {

let quality = detector.determine_sell_token_quality(&order, now).await;

(order.uid, quality)

};

jmg-duarte · 2026-04-13T13:23:06Z

 /// which is significantly faster than the
 /// [num::BigRational] we used before.
 pub struct OrdFloat(f64);
+


Once you format the file correctly this disappears so lets just remove this line (even though I'm also a fan of this separation) to minimize the diff

jmg-duarte · 2026-04-13T13:38:34Z

        let cow_amm_orders = tasks.cow_amm_orders.await;
        auction.orders.extend(cow_amm_orders.iter().cloned());

+        // Start unsupported-order detection immediately


This is wrong, creating the future doesn't drive it

jmg-duarte · 2026-04-13T15:37:33Z

+        let unsupported_orders_future =
+            async move { self.unsupported_order_uids(&orders_for_unsupported).await };


This filters flashloans before orders were loaded with app_data, meaning that the flashloan check ends up being wrong

jmg-duarte · 2026-04-13T16:55:23Z

+        // We can sort the orders, determine UIDS to filter and fetch auction data in
+        // parallel.
+        let (auction, balances, app_data, unsupported_uids) = tokio::join!(
+            sort_orders_future,
+            tasks.balances,
+            tasks.app_data,
+            unsupported_orders_future,
+        );


The way flashloan filtering is handled here, it will break the actual flashloan filtering
It must be done strictly after the update_orders is done

metalurgical requested a review from a team as a code owner April 5, 2026 20:23

github-actions bot added a commit that referenced this pull request Apr 5, 2026

@metalurgical has signed the CLA in #4309

6058779

gemini-code-assist bot reviewed Apr 5, 2026

View reviewed changes

metalurgical force-pushed the issue_3516 branch from c497d0b to ae16fdc Compare April 6, 2026 14:27

jmg-duarte requested changes Apr 6, 2026

View reviewed changes

metalurgical force-pushed the issue_3516 branch from 02bb4e4 to c5e1c03 Compare April 6, 2026 20:26

metalurgical force-pushed the issue_3516 branch from 02a6534 to 4c651d8 Compare April 6, 2026 20:35

metalurgical changed the title ~~refactor: parallelize unsupported-order detection and refactor sorting to avoid order cloning~~ refactor: parallelize unsupported-order detection and defer filtering Apr 6, 2026

parallelize unsupported-order detection and defer filtering

96a0a00

Move unsupported-order detection to a parallel, read-only stage in the auction preprocessing pipeline. Detection now operates on a snapshot of orders and returns UIDs to discard, rather than mutating the auction early.

metalurgical force-pushed the issue_3516 branch from 4c651d8 to 96a0a00 Compare April 6, 2026 20:50

metalurgical added 2 commits April 9, 2026 15:35

Merge branch 'main' into issue_3516

d0b9b7d

fix: extra arguments

3a0f1bd

jmg-duarte mentioned this pull request Apr 13, 2026

Add E2E test to ensure flashloan filtering is performed correctly #4329

Merged

jmg-duarte reviewed Apr 13, 2026

View reviewed changes

metalurgical closed this Apr 13, 2026

metalurgical deleted the issue_3516 branch April 13, 2026 18:57

github-actions bot locked and limited conversation to collaborators Apr 13, 2026

		let unsupported_orders_future =
		async move { self.unsupported_order_uids(&orders_for_unsupported).await };

Conversation

metalurgical commented Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

How to test

Related Issues

Note

Uh oh!

github-actions bot commented Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metalurgical commented Apr 5, 2026

Uh oh!

metalurgical commented Apr 5, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

jmg-duarte commented Apr 6, 2026

Uh oh!

jmg-duarte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metalurgical commented Apr 6, 2026

Uh oh!

jmg-duarte commented Apr 9, 2026

Uh oh!

jmg-duarte left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

metalurgical commented Apr 5, 2026 •

edited

Loading

github-actions bot commented Apr 5, 2026 •

edited

Loading

jmg-duarte left a comment •

edited

Loading