Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for multiple solution proposals from the driver, which is a key feature for enabling EIP-7702 parallel submissions. The changes to Competition::solve to handle multiple solutions, including sorting, caching, and re-simulation, are well-implemented. The new configuration flag propose-all-solutions and associated validation are also correctly added. I've found one issue related to configuration validation for EIP-7702 setup that should be addressed to prevent runtime failures from misconfigurations.
|
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. |
| // When multi-solution proposals are disabled keep only the best one. | ||
| if !self.solver.propose_all_solutions() { | ||
| scored.truncate(1); | ||
| } |
There was a problem hiding this comment.
alternatively: you could .take(1) above and drop the whole thing, avoiding processing more solutions
| for (_, settlement) in &scored { | ||
| lock.push_front(settlement.clone()); | ||
| } |
There was a problem hiding this comment.
i wonder if the following would be faster:
- keeping scored as a vecdeque from the start
- extending scored with settlements
- replacing settlements with scored
| if let Err(infra::simulator::Error::Revert(err)) = | ||
| self.simulate_settlement(settlement).await | ||
| { |
There was a problem hiding this comment.
nit: this helps reduce the indentation and make this a bit more readable
| if let Err(infra::simulator::Error::Revert(err)) = | |
| self.simulate_settlement(settlement).await | |
| { | |
| let err = match self.simulate_settlement(settlement).await { | |
| Err(infra::simulator::Error::Revert(err)) => err, | |
| _ => continue | |
| }; | |
| ... |
| /// delegation. Required when `submission_accounts` is non-empty. | ||
| forwarder_contract: Option<eth::Address>, | ||
|
|
||
| /// If enabled the driver proposes all valid solutions to the autopilot |
There was a problem hiding this comment.
nit:
| /// If enabled the driver proposes all valid solutions to the autopilot | |
| /// If enabled, the driver proposes all valid solutions to the autopilot |
| /// Extracts the first solution from the response. Asserts that exactly one | ||
| /// solution was returned (i.e. `propose-all-solutions` is disabled). |
There was a problem hiding this comment.
| /// Extracts the first solution from the response. Asserts that exactly one | |
| /// solution was returned (i.e. `propose-all-solutions` is disabled). | |
| /// Extracts the first solution from the response. Panics if more than one | |
| /// solution was returned (i.e. `propose-all-solutions` is disabled). |
| for (_, settlement) in &scored { | ||
| lock.push_front(settlement.clone()); | ||
| } | ||
| const MAX_SOLUTION_STORAGE: usize = 5; |
There was a problem hiding this comment.
But scored can hold more than 5 items, right? Should we truncate scored to MAX_SOLUTION_STORAGE before caching, so what is stored and what is returned are always in sync.
| } | ||
| return; | ||
| if let Err(infra::simulator::Error::Revert(err)) = | ||
| self.simulate_settlement(settlement).await |
There was a problem hiding this comment.
All simulations are running sequentially. Any reason for that? Should we run them in parallel?
| // Sort all scored settlements descending by score (best first). | ||
| let mut scored: Vec<(Option<Solved>, Settlement)> = scores | ||
| .into_iter() | ||
| .max_by_key(|(score, _)| score.to_owned()) | ||
| .sorted_by(|(a, _), (b, _)| b.cmp(a)) | ||
| .map(|(score, settlement)| { | ||
| ( | ||
| Solved { | ||
| id: settlement.solution().clone(), | ||
| score, | ||
| trades: settlement.orders(), | ||
| prices: settlement.prices(), | ||
| gas: Some(settlement.gas.estimate), | ||
| }, | ||
| settlement, | ||
| ) | ||
| let solved = Solved { | ||
| id: settlement.solution().clone(), | ||
| score, | ||
| trades: settlement.orders(), | ||
| prices: settlement.prices(), | ||
| gas: Some(settlement.gas.estimate), | ||
| }; | ||
| (Some(solved), settlement) |
There was a problem hiding this comment.
Why not something like the following?
// proposed
let scored: Vec<(Solved, Settlement)> = scores
.into_iter()
.sorted_by(|(a, _), (b, _)| b.cmp(a))
.map(|(score, settlement)| {
let solved = Solved { ... };
(solved, settlement)
})
.collect();
// ...
let mut voided_ids: HashSet<_> = HashSet::new();
// In the re-simulation loop:
if let Err(infra::simulator::Error::Revert(err)) = ... {
voided_ids.insert(settlement.solution().get());
// retain / notify as before...
}
// Return:
Ok(scored
.into_iter()
.filter_map(|(solved, _)| {
if voided_ids.contains(&solved.id.get()) { None } else { Some(solved) }
})
.collect())| .solutions | ||
| .with_label_values(&[solver, "SolutionNotFound"]) | ||
| .with_label_values(&[solver, "Success"]) | ||
| .inc(); |
There was a problem hiding this comment.
Should it be now incremented by the number of solved solutions?
|
|
||
| let solve = test.solve().await.ok(); | ||
| let solutions = solve.solutions(); | ||
| assert_eq!(solutions.len(), 2); |
There was a problem hiding this comment.
The test should also validate the sorting logic.
|
|
||
| /// Test that when `propose-all-solutions` is enabled, all valid solutions are | ||
| /// returned (sorted best-first) and each can be revealed. | ||
| #[tokio::test] |
There was a problem hiding this comment.
Another test is missing: voided solution removed from multi-solution response.
| .map(|a| format!("\"{a}\"")) | ||
| .collect::<Vec<_>>() | ||
| .join(", "); | ||
| writeln!(file, " submission-accounts = [{accounts}]").unwrap(); |
|
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. |
Description
The driver currently proposes only the single highest-scoring solution to the autopilot. With EIP-7702 parallel submission in place, the autopilot's combinatorial auction can now benefit from receiving all valid solutions from a driver to find the optimal set of winners.
Changes
[x] Driver's solve() now returns Vec instead of Option with all valid solutions sorted best-first
[x] Block re-simulation loop now monitors all proposed solutions individually, voiding only those that revert
[x] New per-solver config flag propose-all-solutions (default: false) keeps existing behavior until EIP-7702 infrastructure is ready
How to test
To enable in production, add to the solver config:
propose-all-solutions = true(requires submission-accounts to also be configured along with the forwarder contract).