Improve dictionary migration #3318
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
This commit fixes Cadence 1.0 migration when using atree inlined data. See issue: #3288 Previously, MigrateNestedValue() migrates dictionary by using readonly iterator and migrating values in place. This commit migrates keys first using readonly iterator and then migrates values using mutable iterator.
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit 665d604 Collapsed results for better readability
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
6 tasks
SupunS
approved these changes
May 7, 2024
|
Added one last small improvement: c4bed07: Avoid variable shadowing Consistently use |
SupunS
approved these changes
May 7, 2024
dsainati1
approved these changes
May 7, 2024
fxamacker
reviewed
May 7, 2024
There was a problem hiding this comment.
@turbolent Nice! Thanks for porting #3316 to master and adding mutation checks etc.
I only left a comment to use a different iterator function for cadence v1.0 + atree inlining migration because dictionary.IterateKeys() in branch feature/atree-register-inlining-v1.0 uses mutable iterators.
6 tasks
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.
Work towards #3288
Info
This PR is a result of investigating and fixing #3288, the broken migration of dictionaries in the atree register inlining version, https://github.com/onflow/cadence/tree/feature/atree-register-inlining-v1.0.
Kudos to @fxamacker for investigating the issue, finding the cause, and coming up with this non-intrusive solution! 👏
This PR mostly just ports #3316 to
master, so that the atree register inlining feature branch only contains additional changes required for atree register inlining, and the branch does not drift too far off of master.The PR is best reviewed by viewing each commit individually.
Description
02ebd1e: Fix MigrateNestedValue() for dictionary value
The original bug was the use of the read-only iterator when migrating dictionary keys and values. Using the read-only iterator is wrong, as the migration might mutate dictionary keys and values.
However, simply switching from the read-only iterator to the iterator that allows mutations of children is not possible, as the iterator that allows mutations to children does not support reading old dictionary keys that will get a different hash value in Cadence 1.0.
@fxamacker realized that we can split the migration of dictionaries from the current "interleaved" approach of iterating over all key-value pairs, into two phases, one that migrates all keys, and then another that migrates all values.
This was implemented in Fix Cadence 1.0 migration of dictionary values when using atree inlined data #3316, but on top of the atree register inlining feature branch. This PR ports the commit, 02ebd1e, to
master. Even though the change isn't strictly needed onmaster, porting it anyways keeps the approach of how dictionaries are migrated the same across the different branches.412780e: Add comments
Explain why the migration of dictionaries is now in two phases.
Also add a note to not attempt to refactor the conflict handling code – I caught myself attempting to "improve" it, just to realize it was working correctly, even after the changes of the previous commit.
274cfce: Refactors the dictionary migration phases into separate functions
The
MigrateNestedValuefunction had become very long, so this commit refactors the dictionary case of the switch into two functions. The code got moved as-is and no changes were made to it.76e337c: Adds a test case for migrating dictionaries with enum keys
We did not previously have a test case for it, so add one to ensure this is still supported.
5d2501a: Prevents mutation of dictionary keys
Testnet state migration preview.22 health check after migration error #3288 was hard to debug and fix, because the invalid mutation of child values returned from the read-only mutation did not produce any errors. Only the defensive storage health check after the migration identified there was something wrong.
Improve the migration of nested values by adding assertions that ensure that migrations never mutate dictionary keys. A mutation may replace a dictionary key (e.g. enum with a different enum), but it may never mutate a dictionary key in place (e.g. change the raw value of an enum).
Add a test that has such an invalid migration, and ensure it is immediately rejected.
c4bed07: Avoid variable shadowing
Consistently use
newValueas the name of the variable that is the migrated version ofexistingValue, a child values that is being migrated. Avoid shadowing the final result variable of the whole function,migratedValue.masterbranchFiles changedin the Github PR explorer