From b53f2a28fb118a774c3caf2c2c9b59c0c28fe740 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Fri, 3 May 2024 09:50:26 -0500 Subject: [PATCH 1/8] Add reproducer as test for Cadence 1.0 issue #3288 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: https://github.com/onflow/cadence/issues/3288 --- migrations/migration_test.go | 323 ++++++++++++++++++++++ runtime/tests/runtime_utils/testledger.go | 40 +++ 2 files changed, 363 insertions(+) diff --git a/migrations/migration_test.go b/migrations/migration_test.go index 0665f4eecc..b62c149c56 100644 --- a/migrations/migration_test.go +++ b/migrations/migration_test.go @@ -25,6 +25,8 @@ import ( "encoding/hex" "errors" "fmt" + "strconv" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -2761,3 +2763,324 @@ func TestFixLoadedBrokenReferences(t *testing.T) { err = storage.CheckHealth() require.NoError(t, err) } + +// TestMigrateNestedValue is a reproducer for issue #3288. +// https://github.com/onflow/cadence/issues/3288 +// The reproducer uses a simplified data structure. +func TestMigrateNestedValue(t *testing.T) { + + account := common.Address{0x42} + + elaboration := sema.NewElaboration(nil) + + const s1QualifiedIdentifier = "S1" + + elaboration.SetCompositeType( + utils.TestLocation.TypeID(nil, s1QualifiedIdentifier), + &sema.CompositeType{ + Location: utils.TestLocation, + Members: &sema.StringMemberOrderedMap{}, + Identifier: s1QualifiedIdentifier, + Kind: common.CompositeKindStructure, + }, + ) + + storageDomain := "storage" + storageMapKey := interpreter.StringStorageMapKey("foo") + + createData := func(storageDomain string, storageMapKey interpreter.StorageMapKey) map[string][]byte { + ledger := NewTestLedger(nil, nil) + storage := runtime.NewStorage(ledger, nil) + + inter, err := interpreter.NewInterpreter( + &interpreter.Program{ + Elaboration: elaboration, + }, + utils.TestLocation, + &interpreter.Config{ + Storage: storage, + AtreeValueValidationEnabled: true, + // NOTE: disabled, as storage is not expected to be always valid _during_ migration + AtreeStorageValidationEnabled: false, + }, + ) + require.NoError(t, err) + + dictionaryAnyStructStaticType := + interpreter.NewDictionaryStaticType( + nil, + interpreter.PrimitiveStaticTypeAnyStruct, + interpreter.PrimitiveStaticTypeAnyStruct, + ) + + // Nested data structure in testnet account 0xa47a2d3a3b7e9133: + // dictionary (not inlined) -> + // composite (inlined) -> + // dictionary (inlined) -> + // composite (inlined) -> + // dictionary (not inlined) + + // Nested data stucture used to reproduce issue #3288: + // "parentDict" (not inlined) -> + // "childComposite" (inlined) -> + // "gchildDict" (not inlined) + + // Create a dictionary value with 10 elements: + // { + // "grand_child_dict_key_0":"grand_child_dict_value_0", + // ..., + // "grand_child_dict_key_9":"grand_child_dict_value_9" + // } + const gchildDictCount = 10 + gchildDictElements := make([]interpreter.Value, 0, 2*gchildDictCount) + for i := 0; i < gchildDictCount; i++ { + k := interpreter.NewUnmeteredStringValue("grand_child_dict_key_" + strconv.Itoa(i)) + v := interpreter.NewUnmeteredStringValue("grand_child_dict_value_" + strconv.Itoa(i)) + gchildDictElements = append(gchildDictElements, k, v) + } + + gchildDict := interpreter.NewDictionaryValue( + inter, + emptyLocationRange, + dictionaryAnyStructStaticType, + gchildDictElements..., + ) + + // Create a composite value with 1 field "bar": + // { + // bar:{ + // "grand_child_dict_key_0":"grand_child_dict_value_0", + // ..., + // "grand_child_dict_key_9":"grand_child_dict_value_9" + // } + // } + // Under the hood, nested dictionary is referenced by atree SlabID (not inlined). + childComposite := interpreter.NewCompositeValue( + inter, + emptyLocationRange, + utils.TestLocation, + s1QualifiedIdentifier, + common.CompositeKindStructure, + []interpreter.CompositeField{ + { + Name: "bar", + Value: gchildDict, + }, + }, + common.ZeroAddress, + ) + + // Create a dictionary value with 20 elements: + // { + // "parent_dict_key_0": {bar:{"grand_child_dict_key_0":"grand_child_dict_value_0", ...}}, + // ..., + // "parent_dict_key_19":"parent_dict_value_19" + // } + // Under the hood, nested composite (childComposite) is inlined, while gchildDict remains to be not inlined. + const parentDictCount = 20 + parentDictElements := make([]interpreter.Value, 0, 2*parentDictCount) + for i := 0; i < parentDictCount; i++ { + var k, v interpreter.Value + + k = interpreter.NewUnmeteredStringValue("parent_dict_key_" + strconv.Itoa(i)) + + if i == 0 { + v = childComposite + } else { + v = interpreter.NewUnmeteredStringValue("parent_dict_value_" + strconv.Itoa(i)) + } + + parentDictElements = append(parentDictElements, k, v) + } + + parentDict := interpreter.NewDictionaryValueWithAddress( + inter, + emptyLocationRange, + dictionaryAnyStructStaticType, + account, + parentDictElements..., + ) + + // Create storage map under "storage" domain. + storageMap := storage.GetStorageMap(account, storageDomain, true) + + // Add parentDict (not inlined) to storage map. + exist := storageMap.WriteValue(inter, storageMapKey, parentDict) + require.False(t, exist) + + err = storage.Commit(inter, true) + require.NoError(t, err) + + // Expect 4 registers: + // - register contains slab index for storage map of "storage" domain + // - register for storage map of "storage" domain + // - register for parentDict + // - register for gchildDict + const expectedNonEmptyRegisterCount = 4 + + // Verify that not empty registers + storedValues := ledger.StoredValues + nonEmptyRegisterCount := 0 + for _, v := range storedValues { + if len(v) > 0 { + nonEmptyRegisterCount++ + } + } + require.Equal(t, expectedNonEmptyRegisterCount, nonEmptyRegisterCount) + + return storedValues + } + + ledgerData := createData(storageDomain, storageMapKey) + + // Check health of ledger data before migration. + checkHealth(t, account, ledgerData) + + ledger := NewTestLedgerWithData( + nil, + nil, + ledgerData, + map[string]uint64{string(account[:]): uint64(len(ledgerData))}, + ) + + storage := runtime.NewStorage(ledger, nil) + + inter, err := interpreter.NewInterpreter( + &interpreter.Program{ + Elaboration: elaboration, + }, + utils.TestLocation, + &interpreter.Config{ + Storage: storage, + AtreeValueValidationEnabled: true, + // NOTE: disabled, as storage is not expected to be always valid _during_ migration + AtreeStorageValidationEnabled: false, + }, + ) + + storageMap := storage.GetStorageMap(account, storageDomain, false) + require.NotNil(t, storageMap) + + value := storageMap.ReadValue(inter, storageMapKey) + require.NotNil(t, value) + + migration, err := NewStorageMigration( + inter, + storage, + "test", + account, + ) + require.NoError(t, err) + + reporter := newTestReporter() + + // Migration migrates all gchildDict element values from "grand_child_dict_value_x" to 0. + // This causes gchildDict (was not inlined) to be inlined in its parent childComposite. + // So after migration, number of registers should be decreased by 1 (from not inlined to inlined). + migration.MigrateNestedValue( + interpreter.StorageKey{ + Key: storageDomain, + Address: account, + }, + storageMapKey, + value, + []ValueMigration{ + testMigration{inter: inter}, + }, + reporter, + ) + + err = migration.Commit() + require.NoError(t, err) + + // Check health of ledger data after migration. + checkHealth(t, account, ledger.StoredValues) +} + +func checkHealth(t *testing.T, account common.Address, storedValues map[string][]byte) { + ledger := NewTestLedgerWithData(nil, nil, storedValues, nil) + + storage := runtime.NewStorage(ledger, nil) + + // Load storage maps + for _, domain := range common.AllPathDomains { + _ = storage.GetStorageMap(account, domain.Identifier(), false) + } + + // Load atree slabs + err := loadAtreeSlabsInStorge(storage, account, storedValues) + require.NoError(t, err) + + err = storage.CheckHealth() + require.NoError(t, err) +} + +type testMigration struct { + inter *interpreter.Interpreter +} + +var _ ValueMigration = testMigration{} + +func (testMigration) Name() string { + return "Test Migration" +} + +func (m testMigration) Migrate( + key interpreter.StorageKey, + mapKey interpreter.StorageMapKey, + value interpreter.Value, + inter *interpreter.Interpreter, +) ( + interpreter.Value, + error, +) { + switch value := value.(type) { + case *interpreter.StringValue: + if strings.HasPrefix(value.Str, "grand_child_dict_value_") { + return interpreter.Int64Value(0), nil + } + } + + return nil, nil +} + +func (m testMigration) CanSkip(_ interpreter.StaticType) bool { + return false +} + +func (testMigration) Domains() map[string]struct{} { + return nil +} + +func loadAtreeSlabsInStorge(storage *runtime.Storage, account common.Address, storedValues map[string][]byte) error { + splitKey := func(s string) (owner string, key string, err error) { + results := strings.Split(s, "|") + if len(results) != 2 { + return "", "", fmt.Errorf("failed to split key into owner and key: expected 2 elements, got %d elements", len(results)) + } + return results[0], results[1], nil + } + + for k := range storedValues { + owner, key, err := splitKey(k) + if err != nil { + return err + } + + if key[0] != '$' { + continue + } + + slabID := atree.NewSlabID( + atree.Address([]byte(owner[:])), + atree.SlabIndex([]byte(key[1:]))) + + // Retrieve the slab. + _, _, err = storage.Retrieve(slabID) + if err != nil { + return fmt.Errorf("failed to retrieve slab %s: %w", slabID, err) + } + } + + return nil +} diff --git a/runtime/tests/runtime_utils/testledger.go b/runtime/tests/runtime_utils/testledger.go index f82d716c9e..a2c8f93eec 100644 --- a/runtime/tests/runtime_utils/testledger.go +++ b/runtime/tests/runtime_utils/testledger.go @@ -129,3 +129,43 @@ func NewTestLedger( }, } } + +func NewTestLedgerWithData( + onRead func(owner, key, value []byte), + onWrite func(owner, key, value []byte), + storedValues map[string][]byte, + storageIndices map[string]uint64, +) TestLedger { + + storageKey := func(owner, key string) string { + return strings.Join([]string{owner, key}, "|") + } + + return TestLedger{ + StoredValues: storedValues, + OnValueExists: func(owner, key []byte) (bool, error) { + value := storedValues[storageKey(string(owner), string(key))] + return len(value) > 0, nil + }, + OnGetValue: func(owner, key []byte) (value []byte, err error) { + value = storedValues[storageKey(string(owner), string(key))] + if onRead != nil { + onRead(owner, key, value) + } + return value, nil + }, + OnSetValue: func(owner, key, value []byte) (err error) { + storedValues[storageKey(string(owner), string(key))] = value + if onWrite != nil { + onWrite(owner, key, value) + } + return nil + }, + OnAllocateSlabIndex: func(owner []byte) (result atree.SlabIndex, err error) { + index := storageIndices[string(owner)] + 1 + storageIndices[string(owner)] = index + binary.BigEndian.PutUint64(result[:], index) + return + }, + } +} From f75d6b42c5fcb48304be75c3f97a54196db44f0d Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Fri, 3 May 2024 10:19:18 -0500 Subject: [PATCH 2/8] Lint --- migrations/migration_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/migrations/migration_test.go b/migrations/migration_test.go index b62c149c56..b483b46a58 100644 --- a/migrations/migration_test.go +++ b/migrations/migration_test.go @@ -2820,7 +2820,7 @@ func TestMigrateNestedValue(t *testing.T) { // composite (inlined) -> // dictionary (not inlined) - // Nested data stucture used to reproduce issue #3288: + // Nested data structure used to reproduce issue #3288: // "parentDict" (not inlined) -> // "childComposite" (inlined) -> // "gchildDict" (not inlined) @@ -2957,6 +2957,7 @@ func TestMigrateNestedValue(t *testing.T) { AtreeStorageValidationEnabled: false, }, ) + require.NoError(t, err) storageMap := storage.GetStorageMap(account, storageDomain, false) require.NotNil(t, storageMap) From 59166f43d7a9d38802217526933f3e85e632c84c Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Fri, 3 May 2024 14:19:57 -0500 Subject: [PATCH 3/8] Add TestMigrateNestedComposite for migration --- migrations/migration_test.go | 333 ++++++++++++++++++++++++++++++++++- 1 file changed, 324 insertions(+), 9 deletions(-) diff --git a/migrations/migration_test.go b/migrations/migration_test.go index b483b46a58..0dbd41cf06 100644 --- a/migrations/migration_test.go +++ b/migrations/migration_test.go @@ -2766,7 +2766,10 @@ func TestFixLoadedBrokenReferences(t *testing.T) { // TestMigrateNestedValue is a reproducer for issue #3288. // https://github.com/onflow/cadence/issues/3288 -// The reproducer uses a simplified data structure. +// The reproducer uses a simplified data structure: +// dict (not inlined) -> composite (inlined) -> dict (not inlined) +// After migration, data structure is changed to: +// dict (not inlined) -> composite (inlined) -> dict (inlined) func TestMigrateNestedValue(t *testing.T) { account := common.Address{0x42} @@ -2986,7 +2989,24 @@ func TestMigrateNestedValue(t *testing.T) { storageMapKey, value, []ValueMigration{ - testMigration{inter: inter}, + newTestMigration(inter, func( + key interpreter.StorageKey, + mapKey interpreter.StorageMapKey, + value interpreter.Value, + inter *interpreter.Interpreter, + ) ( + interpreter.Value, + error, + ) { + switch value := value.(type) { + case *interpreter.StringValue: + if strings.HasPrefix(value.Str, "grand_child_dict_value_") { + return interpreter.Int64Value(0), nil + } + } + + return nil, nil + }), }, reporter, ) @@ -2996,6 +3016,290 @@ func TestMigrateNestedValue(t *testing.T) { // Check health of ledger data after migration. checkHealth(t, account, ledger.StoredValues) + + // Expect 3 registers: + // - register contains slab index for storage map of "storage" domain + // - register for storage map of "storage" domain + // - register for parentDict (childComposite and gchildDict are inlined) + const expectedNonEmptyRegisterCount = 3 + + // Verify that not empty registers + storedValues := ledger.StoredValues + nonEmptyRegisterCount := 0 + for _, v := range storedValues { + if len(v) > 0 { + nonEmptyRegisterCount++ + } + } + require.Equal(t, expectedNonEmptyRegisterCount, nonEmptyRegisterCount) +} + +// TestMigrateNestedComposite is counterpart to TestMigrateNestedValue by +// using a slightly different data structure: +// composite (not inlined) -> composite (inlined) -> dict (not inlined) +// After migration, data structure is changed to: +// composite (not inlined) -> composite (inlined) -> dict (inlined) +func TestMigrateNestedComposite(t *testing.T) { + + account := common.Address{0x42} + + elaboration := sema.NewElaboration(nil) + + const s1QualifiedIdentifier = "S1" + + elaboration.SetCompositeType( + utils.TestLocation.TypeID(nil, s1QualifiedIdentifier), + &sema.CompositeType{ + Location: utils.TestLocation, + Members: &sema.StringMemberOrderedMap{}, + Identifier: s1QualifiedIdentifier, + Kind: common.CompositeKindStructure, + }, + ) + + storageDomain := "storage" + storageMapKey := interpreter.StringStorageMapKey("foo") + + createData := func(storageDomain string, storageMapKey interpreter.StorageMapKey) map[string][]byte { + ledger := NewTestLedger(nil, nil) + storage := runtime.NewStorage(ledger, nil) + + inter, err := interpreter.NewInterpreter( + &interpreter.Program{ + Elaboration: elaboration, + }, + utils.TestLocation, + &interpreter.Config{ + Storage: storage, + AtreeValueValidationEnabled: true, + // NOTE: disabled, as storage is not expected to be always valid _during_ migration + AtreeStorageValidationEnabled: false, + }, + ) + require.NoError(t, err) + + dictionaryAnyStructStaticType := + interpreter.NewDictionaryStaticType( + nil, + interpreter.PrimitiveStaticTypeAnyStruct, + interpreter.PrimitiveStaticTypeAnyStruct, + ) + + // Nested data structure used in this test (no dictionary): + // "parentComposite" (not inlined) -> + // "childComposite" (inlined) -> + // "gchildDict" (not inlined) + + // Create a dictionary value with 10 elements: + // { + // "grand_child_dict_key_0":"grand_child_dict_value_0", + // ..., + // "grand_child_dict_key_9":"grand_child_dict_value_9" + // } + const gchildDictCount = 10 + gchildDictElements := make([]interpreter.Value, 0, 2*gchildDictCount) + for i := 0; i < gchildDictCount; i++ { + k := interpreter.NewUnmeteredStringValue("grand_child_dict_key_" + strconv.Itoa(i)) + v := interpreter.NewUnmeteredStringValue("grand_child_dict_value_" + strconv.Itoa(i)) + gchildDictElements = append(gchildDictElements, k, v) + } + + gchildDict := interpreter.NewDictionaryValue( + inter, + emptyLocationRange, + dictionaryAnyStructStaticType, + gchildDictElements..., + ) + + // Create a composite value with 1 field "bar": + // { + // bar:{ + // "grand_child_dict_key_0":"grand_child_dict_value_0", + // ..., + // "grand_child_dict_key_9":"grand_child_dict_value_9" + // } + // } + // Under the hood, nested composite is referenced by atree SlabID (not inlined). + childComposite := interpreter.NewCompositeValue( + inter, + emptyLocationRange, + utils.TestLocation, + s1QualifiedIdentifier, + common.CompositeKindStructure, + []interpreter.CompositeField{ + { + Name: "bar", + Value: gchildDict, + }, + }, + common.ZeroAddress, + ) + + // Create a composite value with 20 fields: + // { + // parent_field_0: {bar:{"grand_child_dict_key_0":"grand_child_dict_value_0", ...}}, + // ..., + // parent_field_19:"parent_field_value_19" + // } + // Under the hood, nested composite (childComposite) is inlined, while gchildDict remains to be not inlined. + const parentCompositeFieldCount = 20 + parentCompositeFields := make([]interpreter.CompositeField, 0, parentCompositeFieldCount) + for i := 0; i < parentCompositeFieldCount; i++ { + name := fmt.Sprintf("parent_field_%d", i) + + var value interpreter.Value + if i == 0 { + value = childComposite + } else { + value = interpreter.NewUnmeteredStringValue("parent_field_value_" + strconv.Itoa(i)) + } + + parentCompositeFields = append( + parentCompositeFields, + interpreter.CompositeField{ + Name: name, + Value: value, + }) + } + + parentComposite := interpreter.NewCompositeValue( + inter, + emptyLocationRange, + utils.TestLocation, + s1QualifiedIdentifier, + common.CompositeKindStructure, + parentCompositeFields, + account, + ) + + // Create storage map under "storage" domain. + storageMap := storage.GetStorageMap(account, storageDomain, true) + + // Add parentComposite (not inlined) to storage map. + exist := storageMap.WriteValue(inter, storageMapKey, parentComposite) + require.False(t, exist) + + err = storage.Commit(inter, true) + require.NoError(t, err) + + // Expect 4 registers: + // - register contains slab index for storage map of "storage" domain + // - register for storage map of "storage" domain + // - register for parentComposite + // - register for gchildDict + const expectedNonEmptyRegisterCount = 4 + + // Verify that not empty registers + storedValues := ledger.StoredValues + nonEmptyRegisterCount := 0 + for _, v := range storedValues { + if len(v) > 0 { + nonEmptyRegisterCount++ + } + } + require.Equal(t, expectedNonEmptyRegisterCount, nonEmptyRegisterCount) + + return storedValues + } + + ledgerData := createData(storageDomain, storageMapKey) + + // Check health of ledger data before migration. + checkHealth(t, account, ledgerData) + + ledger := NewTestLedgerWithData( + nil, + nil, + ledgerData, + map[string]uint64{string(account[:]): uint64(len(ledgerData))}, + ) + + storage := runtime.NewStorage(ledger, nil) + + inter, err := interpreter.NewInterpreter( + &interpreter.Program{ + Elaboration: elaboration, + }, + utils.TestLocation, + &interpreter.Config{ + Storage: storage, + AtreeValueValidationEnabled: true, + // NOTE: disabled, as storage is not expected to be always valid _during_ migration + AtreeStorageValidationEnabled: false, + }, + ) + require.NoError(t, err) + + storageMap := storage.GetStorageMap(account, storageDomain, false) + require.NotNil(t, storageMap) + + value := storageMap.ReadValue(inter, storageMapKey) + require.NotNil(t, value) + + migration, err := NewStorageMigration( + inter, + storage, + "test", + account, + ) + require.NoError(t, err) + + reporter := newTestReporter() + + // Migration migrates all gchildComposite element values from "grand_child_dict_value_x" to 0. + // This causes gchildDict (was not inlined) to be inlined in its parent childComposite. + // So after migration, number of registers should be decreased by 1 (from not inlined to inlined). + migration.MigrateNestedValue( + interpreter.StorageKey{ + Key: storageDomain, + Address: account, + }, + storageMapKey, + value, + []ValueMigration{ + newTestMigration(inter, func( + key interpreter.StorageKey, + mapKey interpreter.StorageMapKey, + value interpreter.Value, + inter *interpreter.Interpreter, + ) ( + interpreter.Value, + error, + ) { + switch value := value.(type) { + case *interpreter.StringValue: + if strings.HasPrefix(value.Str, "grand_child_dict_value_") { + return interpreter.Int64Value(0), nil + } + } + + return nil, nil + }), + }, + reporter, + ) + + err = migration.Commit() + require.NoError(t, err) + + // Check health of ledger data after migration. + checkHealth(t, account, ledger.StoredValues) + + // Expect 3 registers: + // - register contains slab index for storage map of "storage" domain + // - register for storage map of "storage" domain + // - register for parentComposite (childComposite and gchildDict are inlined) + const expectedNonEmptyRegisterCount = 3 + + // Verify that not empty registers + storedValues := ledger.StoredValues + nonEmptyRegisterCount := 0 + for _, v := range storedValues { + if len(v) > 0 { + nonEmptyRegisterCount++ + } + } + require.Equal(t, expectedNonEmptyRegisterCount, nonEmptyRegisterCount) } func checkHealth(t *testing.T, account common.Address, storedValues map[string][]byte) { @@ -3016,12 +3320,27 @@ func checkHealth(t *testing.T, account common.Address, storedValues map[string][ require.NoError(t, err) } +type migrateFunc func( + interpreter.StorageKey, + interpreter.StorageMapKey, + interpreter.Value, + *interpreter.Interpreter, +) (interpreter.Value, error) + type testMigration struct { - inter *interpreter.Interpreter + inter *interpreter.Interpreter + migrate migrateFunc } var _ ValueMigration = testMigration{} +func newTestMigration(inter *interpreter.Interpreter, migrate migrateFunc) testMigration { + return testMigration{ + inter: inter, + migrate: migrate, + } +} + func (testMigration) Name() string { return "Test Migration" } @@ -3035,13 +3354,9 @@ func (m testMigration) Migrate( interpreter.Value, error, ) { - switch value := value.(type) { - case *interpreter.StringValue: - if strings.HasPrefix(value.Str, "grand_child_dict_value_") { - return interpreter.Int64Value(0), nil - } + if m.migrate != nil { + return m.migrate(key, mapKey, value, inter) } - return nil, nil } From 891e9db7c93eb6d8e03c59673c31be58e57f2c13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Fri, 3 May 2024 13:31:28 -0700 Subject: [PATCH 4/8] reduce test case --- migrations/migration_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/migrations/migration_test.go b/migrations/migration_test.go index 0dbd41cf06..410d87aeaf 100644 --- a/migrations/migration_test.go +++ b/migrations/migration_test.go @@ -2828,13 +2828,13 @@ func TestMigrateNestedValue(t *testing.T) { // "childComposite" (inlined) -> // "gchildDict" (not inlined) - // Create a dictionary value with 10 elements: + // Create a dictionary value with 8 elements: // { // "grand_child_dict_key_0":"grand_child_dict_value_0", // ..., - // "grand_child_dict_key_9":"grand_child_dict_value_9" + // "grand_child_dict_key_7":"grand_child_dict_value_7" // } - const gchildDictCount = 10 + const gchildDictCount = 8 gchildDictElements := make([]interpreter.Value, 0, 2*gchildDictCount) for i := 0; i < gchildDictCount; i++ { k := interpreter.NewUnmeteredStringValue("grand_child_dict_key_" + strconv.Itoa(i)) @@ -2873,14 +2873,14 @@ func TestMigrateNestedValue(t *testing.T) { common.ZeroAddress, ) - // Create a dictionary value with 20 elements: + // Create a dictionary value with 2 elements: // { // "parent_dict_key_0": {bar:{"grand_child_dict_key_0":"grand_child_dict_value_0", ...}}, // ..., - // "parent_dict_key_19":"parent_dict_value_19" + // "parent_dict_key_1":"parent_dict_value_1" // } // Under the hood, nested composite (childComposite) is inlined, while gchildDict remains to be not inlined. - const parentDictCount = 20 + const parentDictCount = 2 parentDictElements := make([]interpreter.Value, 0, 2*parentDictCount) for i := 0; i < parentDictCount; i++ { var k, v interpreter.Value @@ -2919,7 +2919,7 @@ func TestMigrateNestedValue(t *testing.T) { // - register for storage map of "storage" domain // - register for parentDict // - register for gchildDict - const expectedNonEmptyRegisterCount = 4 + const expectedNonEmptyRegisterCount = 3 // Verify that not empty registers storedValues := ledger.StoredValues From 6f8923c9707a8e2f8949f456419560a3e87e1180 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Mon, 6 May 2024 15:09:32 -0500 Subject: [PATCH 5/8] Fix test broken by commit 891e9db --- migrations/migration_test.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/migrations/migration_test.go b/migrations/migration_test.go index 410d87aeaf..5ab8e7850a 100644 --- a/migrations/migration_test.go +++ b/migrations/migration_test.go @@ -2914,10 +2914,9 @@ func TestMigrateNestedValue(t *testing.T) { err = storage.Commit(inter, true) require.NoError(t, err) - // Expect 4 registers: + // Expect 3 registers: // - register contains slab index for storage map of "storage" domain - // - register for storage map of "storage" domain - // - register for parentDict + // - register for storage map of "storage" domain with parentDict inlined // - register for gchildDict const expectedNonEmptyRegisterCount = 3 @@ -3017,11 +3016,10 @@ func TestMigrateNestedValue(t *testing.T) { // Check health of ledger data after migration. checkHealth(t, account, ledger.StoredValues) - // Expect 3 registers: + // Expect 2 registers: // - register contains slab index for storage map of "storage" domain - // - register for storage map of "storage" domain - // - register for parentDict (childComposite and gchildDict are inlined) - const expectedNonEmptyRegisterCount = 3 + // - register for storage map of "storage" domain with parentDict, childComposite and gchildDict inlined + const expectedNonEmptyRegisterCount = 2 // Verify that not empty registers storedValues := ledger.StoredValues From 48afb37ab3da04c0605348844dbf48b0b4999684 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Mon, 6 May 2024 15:42:27 -0500 Subject: [PATCH 6/8] Fix MigrateNestedValue() for dictionary value This commit fixes Cadence 1.0 migration when using atree inlined data. See issue: https://github.com/onflow/cadence/issues/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. --- migrations/migration.go | 142 +++++++++++++++++++++++++++------------- 1 file changed, 97 insertions(+), 45 deletions(-) diff --git a/migrations/migration.go b/migrations/migration.go index bc757eac9c..bc5c27e7de 100644 --- a/migrations/migration.go +++ b/migrations/migration.go @@ -318,31 +318,23 @@ func (m *StorageMigration) MigrateNestedValue( key, value interpreter.Value } - // Read the keys first, so the iteration wouldn't be affected - // by the modification of the nested values. - var existingKeysAndValues []keyValuePair + // Migrate keys first. + + var existingKeys []interpreter.Value dictionary.IterateReadOnly( inter, emptyLocationRange, - func(key, value interpreter.Value) (resume bool) { + func(key, _ interpreter.Value) (resume bool) { - existingKeysAndValues = append( - existingKeysAndValues, - keyValuePair{ - key: key, - value: value, - }, - ) + existingKeys = append(existingKeys, key) // Continue iteration return true }, ) - for _, existingKeyAndValue := range existingKeysAndValues { - existingKey := existingKeyAndValue.key - existingValue := existingKeyAndValue.value + for _, existingKey := range existingKeys { newKey := m.MigrateNestedValue( storageKey, @@ -352,26 +344,11 @@ func (m *StorageMigration) MigrateNestedValue( reporter, ) - newValue := m.MigrateNestedValue( - storageKey, - storageMapKey, - existingValue, - valueMigrations, - reporter, - ) - - if newKey == nil && newValue == nil { + if newKey == nil { continue } - // We only reach here at least one of key or value has been migrated. - var keyToSet, valueToSet interpreter.Value - - if newKey == nil { - keyToSet = existingKey - } else { - keyToSet = newKey - } + // We only reach here because key needs to be migrated. // Remove the old key-value pair @@ -388,16 +365,12 @@ func (m *StorageMigration) MigrateNestedValue( )) } - if newValue == nil { - valueToSet = existingValue - } else { - // Value was migrated - valueToSet = newValue + // Remove existing key since old key is migrated + interpreter.StoredValue(inter, existingKeyStorable, m.storage).DeepRemove(inter, false) + inter.RemoveReferencedSlab(existingKeyStorable) - interpreter.StoredValue(inter, existingValueStorable, m.storage). - DeepRemove(inter, false) - inter.RemoveReferencedSlab(existingValueStorable) - } + // Convert removed value storable to Value. + existingValue := interpreter.StoredValue(inter, existingValueStorable, m.storage) // Handle dictionary key conflicts. // @@ -417,8 +390,28 @@ func (m *StorageMigration) MigrateNestedValue( if dictionary.ContainsKey( inter, emptyLocationRange, - keyToSet, + newKey, ) { + + newValue := m.MigrateNestedValue( + storageKey, + storageMapKey, + existingValue, + valueMigrations, + reporter, + ) + + var valueToSet interpreter.Value + if newValue == nil { + valueToSet = existingValue + } else { + valueToSet = newValue + + // Remove existing value since value is migrated. + existingValue.DeepRemove(inter, false) + inter.RemoveReferencedSlab(existingValueStorable) + } + owner := dictionary.GetOwner() pathDomain := common.PathDomainStorage @@ -433,7 +426,7 @@ func (m *StorageMigration) MigrateNestedValue( conflictDictionary.InsertWithoutTransfer( inter, emptyLocationRange, - keyToSet, + newKey, valueToSet, ) @@ -463,17 +456,76 @@ func (m *StorageMigration) MigrateNestedValue( } else { - // No conflict, insert the new key-value pair + // No conflict, insert the new key and existing value pair + // Don't migrate value here because we are going to migrate all values in the dictionary next. dictionary.InsertWithoutTransfer( inter, emptyLocationRange, - keyToSet, - valueToSet, + newKey, + existingValue, ) } } + // Migrate values next. + + var existingKeysAndValues []keyValuePair + + dictionary.Iterate( + inter, + emptyLocationRange, + func(key, value interpreter.Value) (resume bool) { + + existingKeysAndValues = append( + existingKeysAndValues, + keyValuePair{ + key: key, + value: value, + }, + ) + + // Continue iteration + return true + }, + ) + + for _, existingKeyAndValue := range existingKeysAndValues { + existingKey := existingKeyAndValue.key + existingValue := existingKeyAndValue.value + + newValue := m.MigrateNestedValue( + storageKey, + storageMapKey, + existingValue, + valueMigrations, + reporter, + ) + + if newValue == nil { + continue + } + + // Set new value with existing key in the dictionary. + existingValueStorable := dictionary.InsertWithoutTransfer( + inter, + emptyLocationRange, + existingKey, + newValue, + ) + if existingValueStorable == nil { + panic(errors.NewUnexpectedError( + "failed to set migrated value for key: %s", + existingKey, + )) + } + + // Remove existing value since value is migrated + interpreter.StoredValue(inter, existingValueStorable, m.storage). + DeepRemove(inter, false) + inter.RemoveReferencedSlab(existingValueStorable) + } + case *interpreter.PublishedValue: publishedValue := typedValue newInnerValue := m.MigrateNestedValue( From 47da8f33457ced51df0e86a979cda77ab5325478 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Tue, 7 May 2024 11:09:28 -0500 Subject: [PATCH 7/8] Update Cadence version --- version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.go b/version.go index cf42ff0c06..05743bdd9d 100644 --- a/version.go +++ b/version.go @@ -21,4 +21,4 @@ package cadence -const Version = "v1.0.0-preview-atree-register-inlining.24" +const Version = "v1.0.0-preview-atree-register-inlining.25" From 7053cd6cd1c673baa451c352366804561a62aa2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Tue, 7 May 2024 13:03:33 -0700 Subject: [PATCH 8/8] Update migrations/migration.go --- migrations/migration.go | 1 - 1 file changed, 1 deletion(-) diff --git a/migrations/migration.go b/migrations/migration.go index 23f1a461a4..3c9cb38a90 100644 --- a/migrations/migration.go +++ b/migrations/migration.go @@ -313,7 +313,6 @@ func (m *StorageMigration) MigrateNestedValue( existingValue, valueMigrations, reporter, - allowMutation, )