Make metrics based bad order detection order specific

[MartinquaXD]

Description

Currently the bad token detection assumes that we are perfectly able to detect "broken" orders and only orders that trade specific tokens that a particular solver is not able to handle cause problems. However this assumption does not work well with the increasing complexity of new order types that can suddenly start failing for any number of reasons.
The most prominent recent example were flashloan orders where the EIP 1271 signature verified correctly but transferring the tokens into the settlement contract failed because the user's Aave debt position was not healthy enough.
Our current logic caused a lot of collateral damage because such orders could cause many reasonable tokens to be flagged as unsupported although the tokens themselves were perfectly fine and only that particular order was problematic.

Changes

To address this this PR change the metrics based detection mechanism to only flag on an order by order basis instead flagging all orders trading specific tokens. The change itself is relatively simple (collect metrics keyed by Uid instead of token`) but came with a few related changes:

• the name bad_token_detection is now incorrect in most (but not all!) cases so many things were renamed
  • this includes a few config parameters so they must be updated in the infra repo as well!
• caching uids has a lot more potential to bloat the cache so a cache eviction task was introduced, this required 2 new config parameters (max_age, gc_interval)

How to test

• adjusted existing unit to make sure the metrics logic still works correctly with Uid
• added a new unit test for the cache eviction

Related issues

Fixes #4019

[MartinquaXD]

github unfortunately marks this whole file as new when it actually was actually just moved and modified slightly. The new parts are:

• added last_seen_at
• added spawn_gc_task (and the associated unit test)

[squadgazzz]

The refactoring/renaming should probably be separated from the logic changes to avoid this.

[squadgazzz]

This diff is much better https://www.diffchecker.com/iMNVcpf7/

[m-sz]

Infra PR https://github.com/cowprotocol/infrastructure/pull/4302

[jmg-duarte]

LGTM

[m-sz]

Moved bad_token and bad_order detection as submodules to common detector

The public facing api (Quality and the Detector) now reside inside of it. Then its submodules are bad_tokens::simulation and bad_orders::metrics which exactly describe how we rule out the specific offending trades. The rest of the crate uses only the main detector module and detector::Detector struct which makes it less confusing.

Also removed the hand rolled current_unix_timestamp() in favour of now_in_epoch_seconds()

[m-sz]

I am now left wondering how should the configuration struct be called. It's currently named BadOrderDetectionConfig and configures both the bad token detection based on simulation and bad order detection based on metrics. Calling it simply DetectorConfig could be confusing.

[jmg-duarte]

I am now left wondering how should the configuration struct be called. It's currently named BadOrderDetectionConfig and configures both the bad token detection based on simulation and bad order detection based on metrics. Calling it simply DetectorConfig could be confusing.

Suggestions:

1. FaultDetectorConfig
2. Split the configs into token/order or metrics/simulation (or both) and then create a separate one called DetectionConfigurations — its no longer ambiguous because you can open it and see both explicit ones

[squadgazzz]

I am now left wondering how should the configuration struct be called. It's currently named BadOrderDetectionConfig and configures both the bad token detection based on simulation and bad order detection based on metrics. Calling it simply DetectorConfig could be confusing.

We now have 2 detectors that live in different modules. Can we simply split their configs or do they share some config params?

[m-sz]

I am now left wondering how should the configuration struct be called. It's currently named BadOrderDetectionConfig and configures both the bad token detection based on simulation and bad order detection based on metrics. Calling it simply DetectorConfig could be confusing.

We now have 2 detectors that live in different modules. Can we simply split their configs or do they share some config params?

They could be split into two if there was not the common hardcoded token statuses:

Here is the overall struct, I'll call it "DetectorConfig" to avoid confusion

pub struct DetectorConfig {
    pub token_supported: HashMap<eth::Address, bool>,
    pub enable_simulation_strategy: bool,
    pub enable_metrics_strategy: bool,
    pub metrics_strategy_failure_ratio: f64,
    pub metrics_strategy_required_measurements: u32,
    pub metrics_strategy_log_only: bool,
    pub metrics_strategy_freeze_time: Duration,
    pub metrics_strategy_gc_interval: Duration,
    pub metrics_strategy_gc_max_age: Duration,
}

It could be split into 2 configs

pub struct BadTokenDetectorConfig {
    pub enable_simulation_strategy: bool,
}

pub struct BardOrderDetectorConfig {
    pub enable_metrics_strategy: bool,
    pub metrics_strategy_failure_ratio: f64,
    pub metrics_strategy_required_measurements: u32,
    pub metrics_strategy_log_only: bool,
    pub metrics_strategy_freeze_time: Duration,
    pub metrics_strategy_gc_interval: Duration,
    pub metrics_strategy_gc_max_age: Duration,
}

with the remaining field: pub token_supported: HashMap<eth::Address, bool> which truly applies to neither, as the overall Detector that combines inside the BadToken and BadOrder short-circuits the check for token quality based on this field.

I am inclined to leave it as-is, keeping the name as BadOrderDetectionConfig since this is truly used at the order level of an auction.

The Competition strategy uses the detector on an per-order basis to ask if it is unsupported. The underlying mechanism makes a decision based either on the token itself or on the specific order, which might be considered an implementation detail. Thus the BadOrderDetector has inside of it an order-level detector based on metrics an a token-level one based on simulation that together provide us the answer if an order is unsupported or not.

Let's keep it as is and move forward with the PR.

[squadgazzz]

Is this config still relevant?

[m-sz]

Yes, the simulation-bad-token-detection retains its previous name: https://github.com/cowprotocol/services/pull/4021/changes/BASE..1cdb7f81f9a4c2607acab68c108c39acd5307638#diff-892459cd473d2f4681aa36029b2d7967cac7604c93c0f780cb917dd597fd1719R839-R840

[m-sz]

Thanks for the reviews. I will wait for @MartinquaXD to provide his thoughts as he is the original author and merge only then.

[MartinquaXD]

Looks alright to me. I like the discussion about proper naming and where to move stuff.
One more nit from my side: detector is a very generic name for the module. risk_detector would give the reader at least some information on what it's supposed to be.
BTW GH doesn't let me approve my own PR.

[m-sz]

I'll change the detector naming to risk_detector and merge