Don't consider inflight orders invalid

[fafk]

Description

When I was working on another feature (the debug endpoint) I noticed that there is a race condition where we sometimes insert "invalid" order event to the DB.

From logs

09:19:13.717  stored order events label=Executing count=1
09:19:13.733  filtered orders reason=insufficient_balance count=1 orders=[0x060a...]
09:19:13.737  stored order events label=Invalid count=1
09:19:13.756  stored order events label=Traded count=1

Order is in executing state, then settles onchain. If the indexing runs now and we set the order to traded all is well. However if the in cache update runs first it's possible the wallet no longer has the necessary balance as it was traded away in the just executed swap and we consider it invalid and we insert the invalid event for an order that went through perfectly well.

Changes

• Don't consider in-flight orders invalid

How to test

I had an e2e test to reproduce this which failed 6/10 times. After the fix it fails 0/10.

[gemini-code-assist]

Code Review

This pull request introduces a fix for a race condition where an in-flight order could be incorrectly marked as invalid. The change involves fetching in-flight orders from the database and filtering them out from the sets of invalid and filtered orders before storing order events. The implementation is correct and effectively addresses the described issue. The error handling for fetching in-flight orders is graceful, falling back to the previous behavior in case of failure. No critical issues were found in this change.

_{Note: Security Review did not run due to the size of the PR.}

[jmg-duarte]

The code and the idea make sense to me, but the comment makes everything confusing again

[jmg-duarte]

This is a bit hard to read. Do you mean "in-flight order that already won"? And what filters do you mean?

If the order is submitted on chain and fails, why would placing an invalid be misleading?

[fafk]

If the order is submitted on chain and fails, why would placing an invalid be misleading?

Because there is nothing wrong with the order. The solver probably set too low a gas price so the submitted order didn't settle.

Let me tweak the wording a bit though.

[squadgazzz]

Should we exclude in-flight orders from the cache before we even store any events?

[fafk]

Should we exclude in-flight orders from the cache before we even store any events?

A just question. I don't know; I think we might want to filter them, but I imagine we might want solvers to keep producing solutions in case the settlement doesn't land on chain.

[squadgazzz]

I mean, something seems off to me. I see 2 options:

1. Move the store events operation and do that only after the in-flight orders are filtered out.
2. Move the filtering logic before the event storing.

I am more in favor of the first option, but I haven't dived deeper to understand all the implications.

[fafk]

I mean, something seems off to me. I see 2 options:

1. Move the store events operation and do that only after the in-flight orders are filtered out.
2. Move the filtering logic before the event storing.

I am more in favor of the first option, but I haven't dived deeper to understand all the implications.

Okay, I think we can do this: https://github.com/cowprotocol/services/pull/4241/changes – instead of filtering in-flight orders later when cutting the auction we can just omit them when building the live orders cache. That change feels a bit less safe, but it's the healthier approach for the codebase & performance (we don't look up in-flight orders twice). What say you?