[ON HOLD] Reject solution containing outside-of-auction order #2712
Conversation
sunce86
added
the
E:7.1 Ext. solvers operating driver
| let mut ex = self | ||
| .postgres | ||
| .pool | ||
| .begin() |
There was a problem hiding this comment.
There is only a single DB call so we don't need a transaction here.
| .begin() | |
| .acquire() |
| @@ -755,6 +755,27 @@ pub async fn user_orders_with_quote( | |||
| .await | |||
| } | |||
|
|
|||
| pub async fn orders_do_not_exist( | |||
| ex: &mut PgConnection, | |||
| uids: &[OrderUid], | |||
There was a problem hiding this comment.
Could take an impl IntoIterator<Item = OrderUid> instead to avoid double allocating the orders vector.
| let mut separated = query_builder.separated(", "); | ||
| for value_type in uids { | ||
| separated.push_bind(value_type); | ||
| } | ||
| separated.push_unseparated(")) "); |
There was a problem hiding this comment.
Looks other than the usual query builder code. Is the explicit use of .separated() and push_unseparated() here necessary because we push into the inner query here?
|
|
||
| if !self.validate_settlement(&auction, &settlement).await { | ||
| tracing::error!(driver = %driver.name, "invalid settlement"); | ||
| return; |
There was a problem hiding this comment.
Shouldn't we still store all the relevant data for this competition when the solution fails some checks?
| .orders_do_not_exist(&non_auction_orders) | ||
| .await | ||
| .unwrap_or_else(|err| { | ||
| tracing::warn!(?err, "failed to check if all orders exist"); |
There was a problem hiding this comment.
These checks are pretty important so they should fail loudly IMO (e.g. tracing::error!)
| tracing::warn!(?err, "failed to check if all orders exist"); | |
| tracing::warn!(?err, "failed to check if all unexpected orders are JIT orders"); |
| } | ||
| }; | ||
|
|
||
| if !self.validate_settlement(&auction, &settlement).await { |
There was a problem hiding this comment.
I think before we enforce any checks like this we should ensure that we await the correct settlement transaction. It's possible that a malicious losing solver would settle a solution with the expected auction_id out of competition.
Currently we just await the next tx that has the expected auction_id attached to its calldata. What we should probably do instead is to make it so that we only await a solution with the expected auction_id that also came from the winning solver.
That way this verification always applies to "real" solution and a malicious solver can't disrupt the main loop.
This disrupting solver would then easily be removed from the next competitions by the circuit breaker. Unfortunately this likely requires a change in the DB because we have a 1:1 mapping between tx_hash and auction_id at the moment whereas it should probably be a 1:1 mapping between tx_hash and (auction_id, solver).
sunce86
changed the title
|
This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed. |
ON HOLD since I want to get back to this once I work more on fetching the offchain data from db that will be used in
OnSettlementEventUpdater. I already see that I will need a different variation of theorders_do_not_existfunctionality for that as well.Description
Implements first part of the #2667 (comment)
Rejects the winning solution if it contains an order that is not part of the auction. Since JIT orders do not satisfy this condition, we also need to make sure that those that are not in the auction, are also not in the database.
How to test
e2e tests