Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge winners with participants #3042

Merged
merged 8 commits into from
Oct 10, 2024
Merged

Merge winners with participants #3042

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8eb8030
Merge winners with participants
sunce86 Oct 8, 2024
e63cfec
Small refactor
sunce86 Oct 8, 2024
84e2120
cr fixes
sunce86 Oct 9, 2024
1e91d9f
find winner
sunce86 Oct 9, 2024
f29ab45
remove clones
sunce86 Oct 9, 2024
1fe93f3
encapsulate Participant
sunce86 Oct 9, 2024
1567ce3
refactor Participant
sunce86 Oct 10, 2024
1cc83f7
Merge branch 'main' into merge-winners-with-participants
sunce86 Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions crates/autopilot/src/domain/competition/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use {
super::auction::order,
crate::{
domain,
domain::{auction, eth},
domain::{self, auction, eth},
infra,
},
derive_more::Display,
std::collections::HashMap,
std::{collections::HashMap, sync::Arc},
};

type SolutionId = u64;
Expand Down Expand Up @@ -91,6 +91,19 @@ impl Score {
}
}

#[derive(Clone)]
sunce86 marked this conversation as resolved.
Show resolved Hide resolved
pub struct RawParticipant {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we gain much from this extra type. Instead we could initialize Participant with winner: false and have a second step that sets the value to true for the winners.

Copy link
Contributor Author

@sunce86 sunce86 Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, using the same struct to keep two or more different states of the object is an invitation for bugs to join the party (as we don't know which code now and in the future will use the object before second step).

If code duplication is issue here, I would rather have

#[derive(Clone)]
pub struct Participant {
    inner: RawParticipant,
    winner: bool,
}

impl Participant {
    pub fn driver(&self) -> &Arc<infra::Driver> {
        &self.inner.driver
    }

    pub fn solution(&self) -> &Solution {
        &self.inner.solution
    }

    pub fn is_winner(&self) -> bool {
        self.winner
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also more inclined not to deal with mutating objects. In my exp, that often results in errors and more complex code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored according to example to remove code duplication

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see how turning RawParticipant into a Participant (i.e. adding the winner field) should not be considered mutating objects. Before Dusan introduced the accessor methods both solutions were equally prone to misuse because you could just change the public winner field of the Participant anyway.
Is the concept of a RawParticipant really so important that we should have a domain object for it?

I really want to avoid introducing more and more types which seem the same but are not quite the same. Otherwise you'll have the mental overhead of having to know which subtle detail is different in this particular version of the type.
WDYT about this to reduce the ways mutation can lead to issues (again I feel like what we did with the 2 types should also be considered mutating). That way you can only mutate the structs once in a predictable manner that makes sense in the domain:

#[derive(Clone)]
pub struct Participant {
    solution: Solution,
    driver: Arc<infra::Driver>,
    winner: bool,
}

impl Participant {
    pub fn driver(&self) -> &Arc<infra::Driver> {
        &self.inner.driver
    }

    pub fn solution(&self) -> &Solution {
        &self.inner.solution
    }

    pub fn is_winner(&self) -> bool {
        self.winner
    }
    
    pub fn mark_as_winner(&mut self) {
        self.is_winner = true;
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are really adamant about type safety (i.e. having 2 types) I would at least make the names of the 2 structs more obvious: UnrankedParticipant and (Ranked)Participant.

Copy link
Contributor

@MartinquaXD MartinquaXD Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could even play around with the typestate pattern. Although this is admittedly quite a lot of boiler plate code to achieve a simple thing and should probably even have it's own file to limit the scope of the new state types:

struct Participant<State> {
    solution: Solution,
    driver: Arc<infra::Driver>,
    state: State,
}

struct Unranked;
struct Ranked {
    is_winner: bool
}

impl <T> Participant<T> {
    pub fn solution(&self) -> &Solution {
        &self.solution
    }

    pub fn driver(&self) -> &Arc<infra::Driver> {
        &self.driver
    }
}

impl Participant<Unranked> {
    pub fn new(solution: Solution, driver: Arc<infra::Driver>) -> Self {
        Self {
            solution,
            driver,
            state: Unranked
        }
    }

    pub fn rank(self, is_winner: bool) -> Participant<Ranked> {
        Participant::<Ranked> {
            state: Ranked {
                is_winner,
            },
            solution: self.solution,
            driver: self.driver
        }
    }
}

impl Participant<Ranked> {
    pub fn is_winner(&self) -> bool {
        self.state.is_winner
    }
}

Copy link
Contributor Author

@sunce86 sunce86 Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The biggest concern would be if we use non-initialized is_winner while assuming it's initialized. I think I would be fine with initializing in two steps, if the whole capturing of solve response and competition is done in a single step, for example, if we encapsulate that in a Competition::new() constructor or something.

But I found you typestate pattern most adequate for this use case so I took it. 👍

pub driver: Arc<infra::Driver>,
pub solution: Solution,
}

#[derive(Clone)]
sunce86 marked this conversation as resolved.
Show resolved Hide resolved
pub struct Participant {
pub driver: Arc<infra::Driver>,
pub solution: Solution,
pub winner: bool,
}

#[derive(Debug, thiserror::Error)]
#[error("the solver proposed a 0-score solution")]
pub struct ZeroScore;
Expand Down
181 changes: 86 additions & 95 deletions crates/autopilot/src/run_loop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,28 +246,16 @@ impl RunLoop {
let auction = self.remove_in_flight_orders(auction).await;

// Mark all auction orders as `Ready` for competition
self.persistence.store_order_events(
auction.orders.iter().map(|o| OrderUid(o.uid.0)),
OrderEventLabel::Ready,
);
self.persistence
.store_order_events(auction.orders.iter().map(|o| o.uid), OrderEventLabel::Ready);

// Collect valid solutions from all drivers
let solutions = self.competition(&auction).await;
observe::solutions(&solutions);

// Pick winners for execution
let winners = self.select_winners(&solutions);
if winners.is_empty() {
tracing::info!("no winners for auction");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would still be good to keep this log around IMO.

Copy link
Contributor Author

@sunce86 sunce86 Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, if there are no valid solutions, we have a log for that.

If there is at least one valid solution, there is automatically at least one winner (and it's always a first one), so it can't happen that we have no winners in this case.

if solutions.is_empty() {
return;
}

// Mark all non-winning orders as `Considered` for execution
self.persistence.store_order_events(
non_winning_orders(&solutions, &winners),
OrderEventLabel::Considered,
);

let competition_simulation_block = self.eth.current_block().borrow().number;
let block_deadline = competition_simulation_block + self.config.submission_deadline;

Expand All @@ -277,9 +265,6 @@ impl RunLoop {
.post_processing(
&auction,
competition_simulation_block,
// TODO: Support multiple winners
// https://github.com/cowprotocol/services/issues/3021
&winners.first().expect("must exist").solution,
&solutions,
block_deadline,
)
Expand All @@ -289,23 +274,37 @@ impl RunLoop {
return;
}

observe::unsettled(&solutions, &winners, &auction);
for Participant { driver, solution } in winners {
tracing::info!(driver = %driver.name, solution = %solution.id(), "winner");
// Mark all winning orders as `Executing`
let winning_orders = solutions
.iter()
.filter(|p| p.winner)
.flat_map(|p| p.solution.order_ids().copied())
.collect::<HashSet<_>>();
self.persistence
.store_order_events(winning_orders.clone(), OrderEventLabel::Executing);

// Mark all winning orders as `Executing`
self.persistence
.store_order_events(solution.order_ids().copied(), OrderEventLabel::Executing);
// Mark the rest as `Considered` for execution
self.persistence.store_order_events(
solutions
.iter()
.flat_map(|p| p.solution.order_ids().copied())
.filter(|order_id| !winning_orders.contains(order_id)),
OrderEventLabel::Considered,
);

for winner in solutions.iter().filter(|participant| participant.winner) {
tracing::info!(driver = %winner.driver.name, solution = %winner.solution.id(), "winner");

self.start_settlement_execution(
auction.id,
single_run_start,
driver,
solution,
&winner.driver,
&winner.solution,
block_deadline,
)
.await;
}
observe::unsettled(&solutions, &auction);
}

/// Starts settlement execution in a background task. The function is async
Expand Down Expand Up @@ -398,16 +397,23 @@ impl RunLoop {
}
}

#[allow(clippy::too_many_arguments)]
async fn post_processing(
&self,
auction: &domain::Auction,
competition_simulation_block: u64,
winning_solution: &competition::Solution,
solutions: &[Participant],
solutions: &[competition::Participant],
block_deadline: u64,
) -> Result<()> {
let start = Instant::now();
// TODO: Support multiple winners
// https://github.com/cowprotocol/services/issues/3021
let Some(winning_solution) = solutions
.iter()
.find(|participant| participant.winner)
.map(|participant| &participant.solution)
else {
return Err(anyhow::anyhow!("no winners found"));
};
let winner = winning_solution.solver().into();
let winning_score = winning_solution.score().get().0;
let reference_score = solutions
Expand Down Expand Up @@ -522,7 +528,7 @@ impl RunLoop {

/// Runs the solver competition, making all configured drivers participate.
/// Returns all fair solutions sorted by their score (best to worst).
async fn competition(&self, auction: &domain::Auction) -> Vec<Participant> {
async fn competition(&self, auction: &domain::Auction) -> Vec<competition::Participant> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, can you add unit test for these changes? it is not trivial code and small bug could be hidden 🙏

Copy link
Contributor Author

@sunce86 sunce86 Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did not envision implementing unit tests for the runloop functionality (although I agree it could be useful for these small logically interesting parts).

For now I am happy if the changes do not break the current setup (running with 1 winner) which I regularly check on staging.

Once the whole feature is finished, I think having complete e2e test would be useful before activating the feature gradually on each network.

let request = solve::Request::new(
auction,
&self.trusted_tokens.all(),
Expand Down Expand Up @@ -560,45 +566,45 @@ impl RunLoop {
);
None
}
});

// Winners are selected one by one, starting from the best solution,
// until `max_winners_per_auction` are selected. The solution is a winner
// if it swaps tokens that are not yet swapped by any other already
// selected winner.
let mut already_swapped_tokens = HashSet::new();
let mut winners = 0;
let solutions = solutions
.map(|participant| {
let swapped_tokens = participant
.solution
.orders()
.iter()
.flat_map(|(_, order)| [order.sell.token, order.buy.token])
.collect::<HashSet<_>>();

let is_winner = swapped_tokens.is_disjoint(&already_swapped_tokens)
sunce86 marked this conversation as resolved.
Show resolved Hide resolved
&& winners < self.config.max_winners_per_auction;

if is_winner {
already_swapped_tokens.extend(swapped_tokens);
winners += 1;
}
competition::Participant {
driver: participant.driver.clone(),
solution: participant.solution.clone(),
winner: is_winner,
}
})
.collect();

solutions
}

/// Chooses the winners from the given participants.
///
/// Participants are already sorted by their score (best to worst).
///
/// Winners are selected one by one, starting from the best solution,
/// until `max_winners_per_auction` are selected. The solution is a winner
/// if it swaps tokens that are not yet swapped by any other already
/// selected winner.
fn select_winners<'a>(&self, participants: &'a [Participant]) -> Vec<&'a Participant> {
let mut winners = Vec::new();
let mut already_swapped_tokens = HashSet::new();
for participant in participants.iter() {
let swapped_tokens = participant
.solution
.orders()
.iter()
.flat_map(|(_, order)| vec![order.sell.token, order.buy.token])
.collect::<HashSet<_>>();
if swapped_tokens.is_disjoint(&already_swapped_tokens) {
winners.push(participant);
already_swapped_tokens.extend(swapped_tokens);
if winners.len() >= self.config.max_winners_per_auction {
break;
}
}
}
winners
}

/// Returns true if solution is fair to other solutions
fn is_solution_fair(
solution: &Participant,
others: &[Participant],
solution: &competition::RawParticipant,
others: &[competition::RawParticipant],
auction: &domain::Auction,
) -> bool {
let Some(fairness_threshold) = solution.driver.fairness_threshold else {
Expand Down Expand Up @@ -687,7 +693,7 @@ impl RunLoop {
&self,
driver: Arc<infra::Driver>,
request: &solve::Request,
) -> Vec<Participant> {
) -> Vec<competition::RawParticipant> {
let start = Instant::now();
let result = self.try_solve(&driver, request).await;
let solutions = match result {
Expand All @@ -711,7 +717,7 @@ impl RunLoop {
.filter_map(|solution| match solution {
Ok(solution) => {
Metrics::solution_ok(&driver);
Some(Participant {
Some(competition::RawParticipant {
driver: driver.clone(),
solution,
})
Expand Down Expand Up @@ -874,27 +880,6 @@ impl RunLoop {
}
}

fn non_winning_orders(solutions: &[Participant], winners: &[&Participant]) -> HashSet<OrderUid> {
let proposed_orders: HashSet<_> = solutions
.iter()
.flat_map(|participant| participant.solution.order_ids().copied())
.collect();
let winning_orders: HashSet<_> = winners
.iter()
.flat_map(|participant| participant.solution.order_ids().copied())
.collect();
proposed_orders
.difference(&winning_orders)
.cloned()
.collect()
}

#[derive(Clone)]
pub struct Participant {
driver: Arc<infra::Driver>,
solution: competition::Solution,
}

#[derive(Debug, thiserror::Error)]
enum SolveError {
#[error("the solver timed out")]
Expand Down Expand Up @@ -1033,7 +1018,7 @@ impl Metrics {
.observe(elapsed.as_secs_f64());
}

fn matched_unsettled(winning: &infra::Driver, unsettled: HashSet<domain::OrderUid>) {
fn matched_unsettled(winning: &infra::Driver, unsettled: HashSet<&domain::OrderUid>) {
if !unsettled.is_empty() {
tracing::debug!(?unsettled, "some orders were matched but not settled");
}
Expand Down Expand Up @@ -1097,7 +1082,7 @@ pub mod observe {
);
}

pub fn solutions(solutions: &[super::Participant]) {
pub fn solutions(solutions: &[domain::competition::Participant]) {
if solutions.is_empty() {
tracing::info!("no solutions for auction");
}
Expand All @@ -1112,21 +1097,27 @@ pub mod observe {
}

/// Records metrics for the matched but unsettled orders.
pub fn unsettled(
solutions: &[super::Participant],
winners: &[&super::Participant],
auction: &domain::Auction,
) {
let Some(winner) = winners.first() else {
pub fn unsettled(solutions: &[domain::competition::Participant], auction: &domain::Auction) {
let Some(winner) = solutions.first() else {
// no solutions means nothing to report
return;
};

let auction_uids = auction.orders.iter().map(|o| o.uid).collect::<HashSet<_>>();

let mut non_winning_orders = super::non_winning_orders(solutions, winners);
let mut non_winning_orders = {
let winning_orders = solutions
.iter()
.filter(|p| p.winner)
.flat_map(|p| p.solution.order_ids())
.collect::<HashSet<_>>();
solutions
.iter()
.flat_map(|p| p.solution.order_ids())
.filter(|uid| !winning_orders.contains(uid))
.collect::<HashSet<_>>()
};
// Report orders that were part of a non-winning solution candidate
// but only if they were part of the auction (filter out jit orders)
let auction_uids = auction.orders.iter().map(|o| o.uid).collect::<HashSet<_>>();
non_winning_orders.retain(|uid| auction_uids.contains(uid));
super::Metrics::matched_unsettled(&winner.driver, non_winning_orders);
}
Expand Down
2 changes: 1 addition & 1 deletion crates/autopilot/src/shadow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ impl RunLoop {
let swapped_tokens = solution
.orders()
.iter()
.flat_map(|(_, order)| vec![order.sell.token, order.buy.token])
.flat_map(|(_, order)| [order.sell.token, order.buy.token])
.collect::<HashSet<_>>();
if swapped_tokens.is_disjoint(&already_swapped_tokens) {
winners.push(participant);
Expand Down
Loading