From e7f5ee91f214f3810781ddcaafddf4e35d879353 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Wed, 29 May 2024 13:58:29 -0700 Subject: [PATCH 1/2] add reproducer test case --- migrations/entitlements/migration_test.go | 218 ++++++++++++++++++++++ 1 file changed, 218 insertions(+) diff --git a/migrations/entitlements/migration_test.go b/migrations/entitlements/migration_test.go index 37716f3b1..8dd21a7b3 100644 --- a/migrations/entitlements/migration_test.go +++ b/migrations/entitlements/migration_test.go @@ -3516,3 +3516,221 @@ func TestIntersectionTypeWithIntersectionLegacyType(t *testing.T) { require.Equal(t, expectedType, typeValue.Type) })() } + +func TestUseAfterMigrationFailure(t *testing.T) { + + t.Parallel() + + locationRange := interpreter.EmptyLocationRange + + ledger := NewTestLedger(nil, nil) + + storageMapKey := interpreter.StringStorageMapKey("dict") + newTestValue := func() interpreter.Value { + return interpreter.NewUnmeteredStringValue("test") + } + + const fooBarQualifiedIdentifier = "Foo.Bar" + testAddress := common.Address{0x42} + fooAddressLocation := common.NewAddressLocation(nil, testAddress, "Foo") + + newStorageAndInterpreter := func(t *testing.T) (*runtime.Storage, *interpreter.Interpreter) { + storage := runtime.NewStorage(ledger, nil) + inter, err := interpreter.NewInterpreter( + nil, + utils.TestLocation, + &interpreter.Config{ + Storage: storage, + // NOTE: disabled, because encoded and decoded values are expected to not match + AtreeValueValidationEnabled: false, + AtreeStorageValidationEnabled: true, + }, + ) + require.NoError(t, err) + + return storage, inter + } + + newCompositeType := func() *interpreter.CompositeStaticType { + return interpreter.NewCompositeStaticType( + nil, + fooAddressLocation, + fooBarQualifiedIdentifier, + common.NewTypeIDFromQualifiedName( + nil, + fooAddressLocation, + fooBarQualifiedIdentifier, + ), + ) + } + + entitlementSetAuthorization := sema.NewEntitlementSetAccess( + []*sema.EntitlementType{ + sema.NewEntitlementType( + nil, + fooAddressLocation, + "E", + ), + }, + sema.Conjunction, + ) + + // Prepare + (func() { + + storage, inter := newStorageAndInterpreter(t) + + dictionaryStaticType := interpreter.NewDictionaryStaticType( + nil, + interpreter.PrimitiveStaticTypeMetaType, + interpreter.PrimitiveStaticTypeString, + ) + dictValue := interpreter.NewDictionaryValue(inter, locationRange, dictionaryStaticType) + + refType := interpreter.NewReferenceStaticType( + nil, + interpreter.UnauthorizedAccess, + newCompositeType(), + ) + refType.HasLegacyIsAuthorized = true + refType.LegacyIsAuthorized = true + + legacyRefType := &migrations.LegacyReferenceType{ + ReferenceStaticType: refType, + } + + optType := interpreter.NewOptionalStaticType( + nil, + legacyRefType, + ) + + legacyOptType := &migrations.LegacyOptionalType{ + OptionalStaticType: optType, + } + + typeValue := interpreter.NewUnmeteredTypeValue(legacyOptType) + + dictValue.Insert( + inter, + locationRange, + typeValue, + newTestValue(), + ) + + // Note: ID is in the old format + assert.Equal(t, + common.TypeID("auth&A.4200000000000000.Foo.Bar"), + legacyRefType.ID(), + ) + + storageMap := storage.GetStorageMap( + testAddress, + common.PathDomainStorage.Identifier(), + true, + ) + + storageMap.SetValue(inter, + storageMapKey, + dictValue.Transfer( + inter, + locationRange, + atree.Address(testAddress), + false, + nil, + nil, + true, // dictValue is standalone + ), + ) + + err := storage.Commit(inter, false) + require.NoError(t, err) + + err = storage.CheckHealth() + require.NoError(t, err) + })() + + // Migrate + (func() { + + storage, inter := newStorageAndInterpreter(t) + + const importErrorMessage = "cannot import" + + inter.SharedState.Config.ImportLocationHandler = + func(inter *interpreter.Interpreter, location common.Location) interpreter.Import { + panic(importErrorMessage) + } + + migration, err := migrations.NewStorageMigration(inter, storage, "test", testAddress) + require.NoError(t, err) + + reporter := newTestReporter() + + migration.Migrate( + migration.NewValueMigrationsPathMigrator( + reporter, + NewEntitlementsMigration(inter), + ), + ) + + err = migration.Commit() + require.NoError(t, err) + + // Assert + + err = storage.CheckHealth() + require.NoError(t, err) + + require.Len(t, reporter.errors, 2) + + assert.ErrorContains(t, reporter.errors[0], importErrorMessage) + assert.ErrorContains(t, reporter.errors[1], "key (Type<&A.4200000000000000.Foo.Bar?>()) not found") + + require.Empty(t, reporter.migrated) + })() + + // Load + (func() { + + storage, inter := newStorageAndInterpreter(t) + + err := storage.CheckHealth() + require.NoError(t, err) + + storageMap := storage.GetStorageMap( + testAddress, + common.PathDomainStorage.Identifier(), + false, + ) + storedValue := storageMap.ReadValue(inter, storageMapKey) + + require.IsType(t, &interpreter.DictionaryValue{}, storedValue) + + dictValue := storedValue.(*interpreter.DictionaryValue) + + refType := interpreter.NewReferenceStaticType( + nil, + interpreter.ConvertSemaAccessToStaticAuthorization(nil, entitlementSetAuthorization), + newCompositeType(), + ) + + optType := interpreter.NewOptionalStaticType( + nil, + refType, + ) + + typeValue := interpreter.NewUnmeteredTypeValue(optType) + + // Note: ID is in the new format + assert.Equal(t, + common.TypeID("(auth(A.4200000000000000.E)&A.4200000000000000.Foo.Bar)?"), + optType.ID(), + ) + + assert.Equal(t, 1, dictValue.Count()) + + // Key did not get migrated, so is inaccessible + _, ok := dictValue.Get(inter, locationRange, typeValue) + require.False(t, ok) + })() +} From 943b9b3f8bc79ad28c7411e8ef39e50b11092d0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Wed, 29 May 2024 14:41:49 -0700 Subject: [PATCH 2/2] gather dictionary keys during migration and use them when migrating dictionary values avoid re-iterating, as e.g. the iteration using the inlining version of atree will fail for old keys --- migrations/entitlements/migration_test.go | 34 ++++++------ migrations/migration.go | 64 ++++++++++++----------- migrations/migration_test.go | 10 ++-- 3 files changed, 53 insertions(+), 55 deletions(-) diff --git a/migrations/entitlements/migration_test.go b/migrations/entitlements/migration_test.go index 8dd21a7b3..6d6e1f04e 100644 --- a/migrations/entitlements/migration_test.go +++ b/migrations/entitlements/migration_test.go @@ -3564,17 +3564,6 @@ func TestUseAfterMigrationFailure(t *testing.T) { ) } - entitlementSetAuthorization := sema.NewEntitlementSetAccess( - []*sema.EntitlementType{ - sema.NewEntitlementType( - nil, - fooAddressLocation, - "E", - ), - }, - sema.Conjunction, - ) - // Prepare (func() { @@ -3619,8 +3608,8 @@ func TestUseAfterMigrationFailure(t *testing.T) { // Note: ID is in the old format assert.Equal(t, - common.TypeID("auth&A.4200000000000000.Foo.Bar"), - legacyRefType.ID(), + common.TypeID("auth&A.4200000000000000.Foo.Bar?"), + legacyOptType.ID(), ) storageMap := storage.GetStorageMap( @@ -3638,7 +3627,6 @@ func TestUseAfterMigrationFailure(t *testing.T) { false, nil, nil, - true, // dictValue is standalone ), ) @@ -3681,10 +3669,9 @@ func TestUseAfterMigrationFailure(t *testing.T) { err = storage.CheckHealth() require.NoError(t, err) - require.Len(t, reporter.errors, 2) + require.Len(t, reporter.errors, 1) assert.ErrorContains(t, reporter.errors[0], importErrorMessage) - assert.ErrorContains(t, reporter.errors[1], "key (Type<&A.4200000000000000.Foo.Bar?>()) not found") require.Empty(t, reporter.migrated) })() @@ -3710,9 +3697,11 @@ func TestUseAfterMigrationFailure(t *testing.T) { refType := interpreter.NewReferenceStaticType( nil, - interpreter.ConvertSemaAccessToStaticAuthorization(nil, entitlementSetAuthorization), + interpreter.UnauthorizedAccess, newCompositeType(), ) + refType.HasLegacyIsAuthorized = true + refType.LegacyIsAuthorized = true optType := interpreter.NewOptionalStaticType( nil, @@ -3723,14 +3712,21 @@ func TestUseAfterMigrationFailure(t *testing.T) { // Note: ID is in the new format assert.Equal(t, - common.TypeID("(auth(A.4200000000000000.E)&A.4200000000000000.Foo.Bar)?"), + common.TypeID("(&A.4200000000000000.Foo.Bar)?"), optType.ID(), ) assert.Equal(t, 1, dictValue.Count()) - // Key did not get migrated, so is inaccessible + // Key did not get migrated, so is inaccessible using the "new" type value _, ok := dictValue.Get(inter, locationRange, typeValue) require.False(t, ok) + + // But the key is still accessible using the "old" type value + legacyKey := migrations.LegacyKey(typeValue) + + value, ok := dictValue.Get(inter, locationRange, legacyKey) + require.True(t, ok) + require.Equal(t, newTestValue(), value) })() } diff --git a/migrations/migration.go b/migrations/migration.go index 00e6896c1..efc21e1a6 100644 --- a/migrations/migration.go +++ b/migrations/migration.go @@ -363,7 +363,7 @@ func (m *StorageMigration) MigrateNestedValue( // The mutating iterator is only able to read new keys, // as it recalculates the stored values' hashes. - m.migrateDictionaryKeys( + keys := m.migrateDictionaryKeys( storageKey, storageMapKey, dictionary, @@ -379,6 +379,7 @@ func (m *StorageMigration) MigrateNestedValue( valueMigrations, reporter, allowMutation, + keys, ) case *interpreter.PublishedValue: @@ -468,6 +469,11 @@ func (m *StorageMigration) MigrateNestedValue( } +type migratedDictionaryKey struct { + key interpreter.Value + migrated bool +} + func (m *StorageMigration) migrateDictionaryKeys( storageKey interpreter.StorageKey, storageMapKey interpreter.StorageMapKey, @@ -475,7 +481,7 @@ func (m *StorageMigration) migrateDictionaryKeys( valueMigrations []ValueMigration, reporter Reporter, allowMutation bool, -) { +) (migratedKeys []migratedDictionaryKey) { inter := m.interpreter var existingKeys []interpreter.Value @@ -504,6 +510,11 @@ func (m *StorageMigration) migrateDictionaryKeys( ) if newKey == nil { + migratedKeys = append(migratedKeys, migratedDictionaryKey{ + key: existingKey, + migrated: false, + }) + continue } @@ -522,7 +533,7 @@ func (m *StorageMigration) migrateDictionaryKeys( // Remove the old key-value pair - existingKey = legacyKey(existingKey) + existingKey = LegacyKey(existingKey) existingKeyStorable, existingValueStorable := dictionary.RemoveWithoutTransfer( inter, emptyLocationRange, @@ -644,8 +655,15 @@ func (m *StorageMigration) migrateDictionaryKeys( newKey, existingValue, ) + + migratedKeys = append(migratedKeys, migratedDictionaryKey{ + key: newKey, + migrated: true, + }) } } + + return } func (m *StorageMigration) migrateDictionaryValues( @@ -655,37 +673,21 @@ func (m *StorageMigration) migrateDictionaryValues( valueMigrations []ValueMigration, reporter Reporter, allowMutation bool, + migratedDictionaryKeys []migratedDictionaryKey, ) { - inter := m.interpreter - type keyValuePair struct { - key, value interpreter.Value - } + for _, migratedDictionaryKey := range migratedDictionaryKeys { - var existingKeysAndValues []keyValuePair - - dictionary.Iterate( - inter, - func(key, value interpreter.Value) (resume bool) { - - existingKeysAndValues = append( - existingKeysAndValues, - keyValuePair{ - key: key, - value: value, - }, - ) - - // Continue iteration - return true - }, - emptyLocationRange, - ) + existingKey := migratedDictionaryKey.key + if !migratedDictionaryKey.migrated { + existingKey = LegacyKey(existingKey) + } - for _, existingKeyAndValue := range existingKeysAndValues { - existingKey := existingKeyAndValue.key - existingValue := existingKeyAndValue.value + existingValue, ok := dictionary.Get(inter, emptyLocationRange, existingKey) + if !ok { + panic(errors.NewUnexpectedError("failed to get existing value for key: %s", existingKey)) + } newValue := m.MigrateNestedValue( storageKey, @@ -810,8 +812,8 @@ func (m *StorageMigration) migrate( ) } -// legacyKey return the same type with the "old" hash/ID generation function. -func legacyKey(key interpreter.Value) interpreter.Value { +// LegacyKey return the same type with the "old" hash/ID generation function. +func LegacyKey(key interpreter.Value) interpreter.Value { switch key := key.(type) { case interpreter.TypeValue: legacyType := legacyType(key.Type) diff --git a/migrations/migration_test.go b/migrations/migration_test.go index f37daa396..eb4c246ba 100644 --- a/migrations/migration_test.go +++ b/migrations/migration_test.go @@ -2480,12 +2480,12 @@ func TestDictionaryKeyConflict(t *testing.T) { dictionaryValue, ) - // NOTE: use legacyKey to ensure the key is encoded in old format + // NOTE: use LegacyKey to ensure the key is encoded in old format dictionaryValue.InsertWithoutTransfer( inter, emptyLocationRange, - legacyKey(dictionaryKey1), + LegacyKey(dictionaryKey1), interpreter.NewArrayValue( inter, emptyLocationRange, @@ -2498,7 +2498,7 @@ func TestDictionaryKeyConflict(t *testing.T) { dictionaryValue.InsertWithoutTransfer( inter, emptyLocationRange, - legacyKey(dictionaryKey2), + LegacyKey(dictionaryKey2), interpreter.NewArrayValue( inter, emptyLocationRange, @@ -2511,7 +2511,7 @@ func TestDictionaryKeyConflict(t *testing.T) { oldValue1, ok := dictionaryValue.Get( inter, emptyLocationRange, - legacyKey(dictionaryKey1), + LegacyKey(dictionaryKey1), ) require.True(t, ok) @@ -2530,7 +2530,7 @@ func TestDictionaryKeyConflict(t *testing.T) { oldValue2, ok := dictionaryValue.Get( inter, emptyLocationRange, - legacyKey(dictionaryKey2), + LegacyKey(dictionaryKey2), ) require.True(t, ok)