fix(state-snapshots): handle multiple incoming requests correctly #12912
+57
−44
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
With the current code, we handle a DeleteAndMaybeCreateSnapshotRequest message and delete the state snapshot, and then send a CreateSnapshotRequest that will be handled later. But this means that if several requests come in a period of time shorter than the amount of time it takes to create the state snapshot, several deletions might all run in a row before the corresponding creations run, and the extra state snapshots will not get cleaned up (as warned about in the log line "Requested a state snapshot but that is already available with a different hash". There's no real need to have this extra message passing to Self since along with this bug, it adds unnecessary complication to the code. So just have two message types, one for deletions and one for creations, and call self.tries.delete_state_snapshot() in the handler for snapshot creations.
|
I didnt add any testloop tests for this case, because it's a bit tricky to get the timing to trigger it. But I tested it w the hacks in this branch and also ran 2.5.0 with this PR but not #12907 cherry picked, so we can see the fix here get triggered |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12912 +/- ##
==========================================
- Coverage 70.54% 70.52% -0.02%
==========================================
Files 851 851
Lines 174906 174917 +11
Branches 174906 174917 +11
==========================================
- Hits 123382 123355 -27
- Misses 46399 46432 +33
- Partials 5125 5130 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
The state snapshot actor's logic does not work if several requests come in quick succession. There are a couple problems that this PR fixes.
One is that we first handle a
DeleteAndMaybeCreateSnapshotRequestmessage and delete the state snapshot, and then send aCreateSnapshotRequesttoSelfthat will be handled later. But this means that if several requests come in a period of time shorter than the amount of time it takes to create the state snapshot, several deletions might all run in a row before the corresponding creations run, and the extra state snapshots will not get cleaned upThe second more important problem is that we call
snapshot_taken()to allow flat storage updates after the snapshot has been created, but this is done even if there have been other snapshot requests in the meantime. So we end up allowing flat head updates when we shouldn't have, and the second snapshot request will take a snapshot of flat storage that's advanced too far in the chainThis was seen recently because it was triggered by the extra snapshot request we generate before #12907, but it has always been possible in theory in the case of a fork. So even with that PR, this is still worth fixing