Add reproducer as test for Cadence 1.0 migration issue #3288 #3314
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
The problem was initially detected in testnet migration of atree inlined data with Cadence 1.0 migration. The bug happened with 5 nested levels of data while this reproducer is simplified to use 3 nested levels. More info at: #3288
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:feature/atree-register-inlining-v1.0 commit fac39f8 Collapsed results for better readability
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
fxamacker
changed the title
fxamacker
commented
May 3, 2024
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.
6 tasks
Fix Cadence 1.0 migration of dictionary values when using atree inlined data
…er/add-reproducer-for-issue-3288
turbolent
changed the base branch from
feature/atree-register-inlining-v1.0
to
bastian/update-atree-register-inlining-v1.0-3
turbolent
changed the base branch from
bastian/update-atree-register-inlining-v1.0-3
to
bastian/sync-atree-register-inlining-v1.0
turbolent
reviewed
May 7, 2024
SupunS
approved these changes
May 7, 2024
Base automatically changed from
bastian/sync-atree-register-inlining-v1.0
to
feature/atree-register-inlining-v1.0
May 7, 2024 21:04
…d-reproducer-for-issue-3288
dsainati1
approved these changes
May 7, 2024
turbolent
merged commit 6d49c8c
into
feature/atree-register-inlining-v1.0
7 of 9 checks passed
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.
Closes #3288
This PR adds 2 tests with a reproducer for Cadence 1.0 migration (using atree inlined data) issue #3288.
TestMigrateNestedValue()(reproducer that currently fails until issue Testnet state migration preview.22 health check after migration error #3288 is fixed)TestMigrateNestedComposite()(test that currently passes using slightly different data structure)TestMigrateNestedValue()is a reproducer that uses data structure similar to testnet account:TestMigrateNestedComposite()passes and it uses a slightly different data structure:The bug happened with testnet data on 5 nested levels. This reproducer is simplified to use 3 nested levels.
I think the problem is with dictionary using ReadOnly iterator to migrate element in-place. If you change this line https://github.com/onflow/cadence/blob/feature/atree-register-inlining-v1.0/migrations/migration.go#L325 from dictionary.IterateReadOnly to dictionary.Iterate, then TestMigrateNestedValue passes. However, other migration tests will then fail.
Discord 🧵 https://discord.com/channels/613813861610684416/1235939015958593578/1236039340816072754
masterbranchFiles changedin the Github PR explorer