Merge solutions not settlements #2471
Conversation
| merged.extend(extension); | ||
| } | ||
| // Sort by "simplest", ie least merged solution. | ||
| // Maybe we should sort by score instead? |
There was a problem hiding this comment.
@harisang brought up at some point that we have no way of knowing the score of a merged solution so using that for determining a sort order is probably not wise.
Prioritizing solutions that are the closest to what the solver originally intended seems like the most reasonable choice here.
But since we now have reth nodes we might not have to sweat it anyway. ;)
There was a problem hiding this comment.
The reason we only knew the score for settlements (not solutions) was that they required simulation to get the gas costs. Thanks to CiP38 (rank by surplus) this goes away and so we can now actually compute the score from the solution (cf. #2448).
So I think we could still debate if we want to start with the highest scored solution (even if it's more risky to fail because it'll likely consist of many merged settlements.
There was a problem hiding this comment.
Oh, right. With CIP 38 things change. Then I'd say let's be greedy (highest score first). That way we could stop at the first solution that doesn't revert.
| // Solution prices need to congruent, i.e. there needs to be a unique factor to | ||
| // scale all common tokens from one solution into the other. | ||
| let scaling_factors = scaling_factors(&self.prices, &other.prices); | ||
| let factor = match scaling_factors.len() { |
There was a problem hiding this comment.
This logic seems pretty awkward. I think calling the function common_scaling_factor() -> Result<BigRational> or sth like that and moving that check into the function would be easier to understand.
I would also move if factor < 1 there.
There was a problem hiding this comment.
Wasn't able to get the if factor < 1 logic abstracted but I think this is much simpler to read now.
| if *entry.get() != scaled { | ||
| return Err(MergeError::IncongruentPrices); | ||
| } |
There was a problem hiding this comment.
Since the single common scaling factor is already chosen to satisfy this condition I assume this is supposed to detect rounding errors when converting back to U256?
There was a problem hiding this comment.
Yes, I just don't want to override values that don't match and be on the safe side, but you are right this shouldn't really happen.
There was a problem hiding this comment.
Since this is now possible, and since some solvers raised the question of skipping merging, and since we discussed it before, have you considered completely moving the merging to solvers?
| } | ||
| // Sort by "simplest", ie least merged solution. | ||
| // Maybe we should sort by score instead? |
There was a problem hiding this comment.
I think we should capture issue to change sorting (by score) once the CIP38 is live.
|
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. |
|
still planning to merge this (next week hopefully) |
fleupold
force-pushed
the
merge_solutions
branch
from
de0baf8 to
fb6e7d4
Compare
|
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
Currently merging multiple solutions into a larger one happens in the
Settlementtype and reuses the legacy settlement encoder. As part of our initiative to fully transition away from the legacy code base this PR moves solution merging logic into our solution domain model.This means we will now merge solutions before simulating and encoding them. This can potentially lead to a lot more solution candidates being generated and requiring simulation. I don't expect this to be an issue in practice as encoding is already part of a stream which will yield the best solution candidate found so far as soon as we approach the solving deadline (thought may have to go into in which order solution candidate should be attempted to be simulated).
Changes
mergemethod onSolutionIdtype onSolutionto be able to represent when a solution was generated from multiple sub-solutions.settlement.mergewithsolution.merge(this means merging now happens before first simulation)scoreinstead once Rank by surplus driver V3 #2448 is merged)Depending on how risky we think this change is, we can also keep the existing merging logic and have a feature flag guard the two codepaths.
How to test
Existing unit test
Related Issues
Fixes #1478