Conversation
fafk
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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. |
| 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
| 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.
|
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. |
| let mut scored: Vec<(Option<Solved>, Settlement)> = scores | ||
| .into_iter() | ||
| .max_by_key(|(score, _)| score.to_owned()) | ||
| .sorted_by(|(a, _), (b, _)| b.cmp(a)) |
There was a problem hiding this comment.
Is the reason to switch away from max_by_key to avoid cloning the score? Otherwise I find max_by_key easier to understand by just reading the code.
There was a problem hiding this comment.
max_by_key give 1 element (the max), but now we actually want a vector of N best solutions.
There was a problem hiding this comment.
Ahh, right. Then sorted_by_key would be easier to read, no?
.sorted_by_key(|(score, _)| std::cmp::Reverse(*score))
| { | ||
| let mut lock = self.settlements.lock().unwrap(); | ||
| for (_, settlement) in &scored { | ||
| lock.push_front(settlement.clone()); | ||
| } | ||
| const MAX_SOLUTION_STORAGE: usize = 5; | ||
| lock.truncate(MAX_SOLUTION_STORAGE); | ||
| lock.truncate(max_to_propose * 5); |
There was a problem hiding this comment.
nit: I think a name for this magic number would be helpful. Maybe MAX_CONCURRENT_AUCTIONS?
| let active: Vec<_> = scored_ref | ||
| .iter() | ||
| .filter(|(solved, _)| !voided_ref.contains(&solved.id.get())) | ||
| .collect(); |
There was a problem hiding this comment.
You don't have to .collect() here for join_all to work.
| self.settlements | ||
| .lock() | ||
| .unwrap() | ||
| .retain(|s| s.solution().get() != solution_id); |
There was a problem hiding this comment.
Feels a bit weird to lock settlements multiple times in a loop after all the solutions already got invalidated.
WDYT about doing most of this error handling in the future itself and only return Option<SolutionId> from the future. Then you can add all of them to the voided_ref at once and lock self.settlements as the simulation results trickle in.
let voided_solutions =
futures::future::join_all(active.map(|(solved, settlement)| {
let solution_id = solved.id.get();
async move {
let result = self.simulate_settlement(settlement).await;
let err = match result {
Err(simulator::Error::Revert(err)) => err,
_ => return None,
};
let has_haircut = settlement.has_haircut();
observe::winner_voided(self.solver.name(), block, &err, has_haircut);
self.settlements
.lock()
.unwrap()
.retain(|s| s.solution().get() != solution_id);
if !has_haircut {
notify::simulation_failed(
&self.solver,
auction.id(),
settlement.solution(),
&simulator::Error::Revert(err),
true,
);
}
Some(solution_id)
}
}))
.await;
voided_ref.extend(voided_solutions.into_iter().flatten());| /// discarded and only valid ones are returned. | ||
| #[tokio::test] | ||
| #[ignore] | ||
| async fn all_proposed_with_invalid() { |
There was a problem hiding this comment.
nit: awkward name - why not only_proposes_valid_solutions?
There was a problem hiding this comment.
Take it or leave it: Cover the error conditions in the function doc too
The error message is great though, this would just help when hovering over
| let has_haircut = settlement.has_haircut(); | ||
| let voided_ref = &mut voided; | ||
| let scored_ref = &scored; | ||
| let simulate_on_new_blocks = async move { |
There was a problem hiding this comment.
would it make sense to extract this into a function? the nesting level gets deep and it would become easier to read
| let active: Vec<_> = scored_ref | ||
| .iter() | ||
| .filter(|(solved, _)| !voided_ref.contains(&solved.id.get())) | ||
| .collect(); |
There was a problem hiding this comment.
Couldn't you permanently remove solutions from scored to avoid this?
Or can it be that a solution that fails on block N, can pass again on N+1?
4 participants
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).