dex: fix frontier cache incoherence #4283
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
conorsch
added
the
consensus-breaking
Added label. This means we'll need to write a migration in order to release this, and provide validators with instructions, right? |
|
Ooops thanks I missed that, no migration needed, it's an internal structure and only used during execution |
zbuc
reviewed
Apr 29, 2024
| // | ||
| // If so, skip the write, but more importantly, skip emitting an event, | ||
| // so tooling doesn't get confused about a no-op execution. | ||
| if prev_state == new_state { |
There was a problem hiding this comment.
Nit: state is a bit overridden here and it took me a couple reads to understand that prev_state/new_state aren't instances of the position::State enum but instead Positions.
|
Can we fix #4123 as part of these changes? |
hdevalence
requested changes
Apr 29, 2024
There was a problem hiding this comment.
LGTM but we should fix #4123 while we're making these changes.
erwanor
force-pushed
the
erwan/dex_frontier_inspect
branch
from
0fa692e to
d088689
Compare
|
@hdevalence added a fix/event for #4123 |
hdevalence
approved these changes
Apr 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fix a bug in the
Frontierposition cache that caused a position execution guard to be hit.Context
During routing execution, the DEX engine assemble frontiers of positions to execute against. It maintains an internal cache with a position stream for each hop in the path, along with the ids and state of the current loaded set. The position cache is persisted in its entirety when a round of routing execution has completed (via
Frontier::save), and partially, if a position needs to be replaced (Frontier::replace_position).Bug
The bug occurs when the following conditions align:
Closedwhen recording its executionIn that case, since the position rotation logic does not update the internal state of the position, it will attempt to record execution of a stale position when saving the frontier.
Fix
The fix is two-fold, first,
PositionManager::position_executionshould make it clear that it can mutate its input and return the updated position state to allow callers to inspect its state. Once that's done, the position rotation logic will be able to immediately update its internal cache with the actual position state so that there are no incoherences between its internal view and the actual chain state.Checklist before requesting a review
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: