From 41f89793c2fe6d8c35e058ab0119bcc7e77dd547 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Wed, 13 Dec 2023 18:39:45 +0100 Subject: [PATCH 1/5] Deduplicate contract names migration --- .../deduplicate_contract_names_migration.go | 134 ++++++++++++++++++ ...duplicate_contract_names_migration_test.go | 90 ++++++++++++ 2 files changed, 224 insertions(+) create mode 100644 cmd/util/ledger/migrations/deduplicate_contract_names_migration.go create mode 100644 cmd/util/ledger/migrations/deduplicate_contract_names_migration_test.go diff --git a/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go b/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go new file mode 100644 index 00000000000..5d79da0590c --- /dev/null +++ b/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go @@ -0,0 +1,134 @@ +package migrations + +import ( + "bytes" + "context" + "fmt" + + "github.com/rs/zerolog" + "github.com/rs/zerolog/log" + + "github.com/fxamacker/cbor/v2" + + "github.com/onflow/cadence/runtime/common" + + "github.com/onflow/flow-go/cmd/util/ledger/util" + "github.com/onflow/flow-go/fvm/environment" + "github.com/onflow/flow-go/fvm/storage/state" + "github.com/onflow/flow-go/ledger" + "github.com/onflow/flow-go/ledger/common/convert" + "github.com/onflow/flow-go/model/flow" +) + +// DeduplicateContractNamesMigration checks if the contract names have been duplicated and +// removes the duplicate ones. +type DeduplicateContractNamesMigration struct { + log zerolog.Logger +} + +func (d *DeduplicateContractNamesMigration) InitMigration( + log zerolog.Logger, + _ []*ledger.Payload, + _ int, +) error { + d.log = log. + With(). + Str("migration", "DeduplicateContractNamesMigration"). + Logger() + + return nil +} + +func (d *DeduplicateContractNamesMigration) MigrateAccount( + ctx context.Context, + address common.Address, + payloads []*ledger.Payload, +) ([]*ledger.Payload, error) { + snapshot, err := util.NewPayloadSnapshot(payloads) + if err != nil { + return nil, fmt.Errorf("failed to create payload snapshot: %w", err) + } + transactionState := state.NewTransactionState(snapshot, state.DefaultParameters()) + accounts := environment.NewAccounts(transactionState) + flowAddress := flow.ConvertAddress(address) + + contractNames, err := accounts.GetContractNames(flowAddress) + if err != nil { + return nil, fmt.Errorf("failed to get contract names: %w", err) + } + if len(contractNames) == 0 { + return payloads, nil + } + + contractNamesSet := make(map[string]struct{}) + removeIndexes := make([]int, 0) + for i, name := range contractNames { + if _, ok := contractNamesSet[name]; ok { + // duplicate contract name + removeIndexes = append(removeIndexes, i) + continue + } + + contractNamesSet[name] = struct{}{} + } + + if len(removeIndexes) == 0 { + return payloads, nil + } + + log.Info(). + Str("address", address.Hex()). + Strs("contract_names", contractNames). + Msg("removing duplicate contract names") + + // remove the duplicate contract names, keeping the original order + for i := len(removeIndexes) - 1; i >= 0; i-- { + contractNames = append(contractNames[:removeIndexes[i]], contractNames[removeIndexes[i]+1:]...) + } + + var buf bytes.Buffer + cborEncoder := cbor.NewEncoder(&buf) + err = cborEncoder.Encode(contractNames) + if err != nil { + return nil, fmt.Errorf( + "cannot encode contract names: %s", + contractNames, + ) + } + newContractNames := buf.Bytes() + + id := flow.ContractNamesRegisterID(flowAddress) + err = accounts.SetValue(id, newContractNames) + + if err != nil { + return nil, fmt.Errorf("setting value failed: %w", err) + } + + // finalize the transaction + result, err := transactionState.FinalizeMainTransaction() + if err != nil { + return nil, fmt.Errorf("failed to finalize main transaction: %w", err) + } + + for id, value := range result.WriteSet { + if value == nil { + delete(snapshot.Payloads, id) + continue + } + + snapshot.Payloads[id] = ledger.NewPayload( + convert.RegisterIDToLedgerKey(id), + value, + ) + } + + newPayloads := make([]*ledger.Payload, 0, len(snapshot.Payloads)) + for _, payload := range snapshot.Payloads { + newPayloads = append(newPayloads, payload) + } + + return newPayloads, nil + +} + +var _ AccountBasedMigration = &DeduplicateContractNamesMigration{} diff --git a/cmd/util/ledger/migrations/deduplicate_contract_names_migration_test.go b/cmd/util/ledger/migrations/deduplicate_contract_names_migration_test.go new file mode 100644 index 00000000000..a826d635ad4 --- /dev/null +++ b/cmd/util/ledger/migrations/deduplicate_contract_names_migration_test.go @@ -0,0 +1,90 @@ +package migrations_test + +import ( + "bytes" + "context" + "testing" + + "github.com/fxamacker/cbor/v2" + "github.com/rs/zerolog" + "github.com/stretchr/testify/require" + + "github.com/onflow/cadence/runtime/common" + + "github.com/onflow/flow-go/cmd/util/ledger/migrations" + "github.com/onflow/flow-go/fvm/environment" + "github.com/onflow/flow-go/ledger" + "github.com/onflow/flow-go/ledger/common/convert" + "github.com/onflow/flow-go/model/flow" +) + +func TestDeduplicateContractNamesMigration(t *testing.T) { + migration := migrations.DeduplicateContractNamesMigration{} + + log := zerolog.New(zerolog.NewTestWriter(t)) + + err := migration.InitMigration(log, nil, 0) + require.NoError(t, err) + + ctx := context.Background() + + address, err := common.HexToAddress("0x1") + require.NoError(t, err) + + contractNames := []string{"test", "test", "test2", "test3", "test3"} + + var buf bytes.Buffer + cborEncoder := cbor.NewEncoder(&buf) + err = cborEncoder.Encode(contractNames) + require.NoError(t, err) + newContractNames := buf.Bytes() + + accountStatus := environment.NewAccountStatus() + + accountStatus.SetStorageUsed(1000) + + payloads, err := migration.MigrateAccount(ctx, address, + []*ledger.Payload{ + + ledger.NewPayload( + convert.RegisterIDToLedgerKey( + flow.RegisterID{ + Owner: string(address.Bytes()), + Key: flow.ContractNamesKey, + }, + ), + newContractNames, + ), + ledger.NewPayload( + convert.RegisterIDToLedgerKey( + flow.AccountStatusRegisterID(flow.ConvertAddress(address)), + ), + accountStatus.ToBytes(), + ), + }, + ) + + require.NoError(t, err) + require.Equal(t, 2, len(payloads)) + + for _, payload := range payloads { + key, err := payload.Key() + require.NoError(t, err) + id, err := convert.LedgerKeyToRegisterID(key) + require.NoError(t, err) + + if id.Key != flow.ContractNamesKey { + continue + } + + value := payload.Value() + + contracts := make([]string, 0) + buf := bytes.NewReader(value) + cborDecoder := cbor.NewDecoder(buf) + err = cborDecoder.Decode(&contracts) + require.NoError(t, err) + + require.Equal(t, 3, len(contracts)) + } +} From 2faa3880086216cc3b871d851082207115f48e46 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Tue, 2 Jan 2024 16:27:43 +0100 Subject: [PATCH 2/5] address review comments --- .../deduplicate_contract_names_migration.go | 43 ++++++++----------- 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go b/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go index 5d79da0590c..80707e39d95 100644 --- a/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go +++ b/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go @@ -1,14 +1,11 @@ package migrations import ( - "bytes" "context" "fmt" - "github.com/rs/zerolog" - "github.com/rs/zerolog/log" - "github.com/fxamacker/cbor/v2" + "github.com/rs/zerolog" "github.com/onflow/cadence/runtime/common" @@ -26,6 +23,10 @@ type DeduplicateContractNamesMigration struct { log zerolog.Logger } +func (d *DeduplicateContractNamesMigration) Close() error { + return nil +} + func (d *DeduplicateContractNamesMigration) InitMigration( log zerolog.Logger, _ []*ledger.Payload, @@ -56,46 +57,40 @@ func (d *DeduplicateContractNamesMigration) MigrateAccount( if err != nil { return nil, fmt.Errorf("failed to get contract names: %w", err) } - if len(contractNames) == 0 { + if len(contractNames) == 1 { return payloads, nil } - contractNamesSet := make(map[string]struct{}) - removeIndexes := make([]int, 0) - for i, name := range contractNames { - if _, ok := contractNamesSet[name]; ok { - // duplicate contract name - removeIndexes = append(removeIndexes, i) + var foundDuplicate bool + i := 1 + for i < len(contractNames) { + if contractNames[i-1] != contractNames[i] { + i++ continue } - - contractNamesSet[name] = struct{}{} + // Found duplicate (contactNames[i-1] == contactNames[i]) + // Remove contractNames[i] + copy(contractNames[i:], contractNames[i+1:]) + contractNames = contractNames[:len(contractNames)-1] + foundDuplicate = true } - if len(removeIndexes) == 0 { + if !foundDuplicate { return payloads, nil } - log.Info(). + d.log.Info(). Str("address", address.Hex()). Strs("contract_names", contractNames). Msg("removing duplicate contract names") - // remove the duplicate contract names, keeping the original order - for i := len(removeIndexes) - 1; i >= 0; i-- { - contractNames = append(contractNames[:removeIndexes[i]], contractNames[removeIndexes[i]+1:]...) - } - - var buf bytes.Buffer - cborEncoder := cbor.NewEncoder(&buf) - err = cborEncoder.Encode(contractNames) + newContractNames, err := cbor.Marshal(contractNames) if err != nil { return nil, fmt.Errorf( "cannot encode contract names: %s", contractNames, ) } - newContractNames := buf.Bytes() id := flow.ContractNamesRegisterID(flowAddress) err = accounts.SetValue(id, newContractNames) From 4a23ca1be0bf47450582299037998970af3e2650 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Tue, 2 Jan 2024 16:52:29 +0100 Subject: [PATCH 3/5] dont use snapshot to get one register --- .../deduplicate_contract_names_migration.go | 70 +++++++------------ 1 file changed, 26 insertions(+), 44 deletions(-) diff --git a/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go b/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go index 80707e39d95..4669f8e4b2b 100644 --- a/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go +++ b/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go @@ -9,9 +9,6 @@ import ( "github.com/onflow/cadence/runtime/common" - "github.com/onflow/flow-go/cmd/util/ledger/util" - "github.com/onflow/flow-go/fvm/environment" - "github.com/onflow/flow-go/fvm/storage/state" "github.com/onflow/flow-go/ledger" "github.com/onflow/flow-go/ledger/common/convert" "github.com/onflow/flow-go/model/flow" @@ -45,21 +42,35 @@ func (d *DeduplicateContractNamesMigration) MigrateAccount( address common.Address, payloads []*ledger.Payload, ) ([]*ledger.Payload, error) { - snapshot, err := util.NewPayloadSnapshot(payloads) - if err != nil { - return nil, fmt.Errorf("failed to create payload snapshot: %w", err) - } - transactionState := state.NewTransactionState(snapshot, state.DefaultParameters()) - accounts := environment.NewAccounts(transactionState) flowAddress := flow.ConvertAddress(address) + contractNamesID := flow.ContractNamesRegisterID(flowAddress) + + var contractNamesPayload *ledger.Payload + contractNamesPayloadIndex := 0 + for i, payload := range payloads { + key, err := payload.Key() + if err != nil { + return nil, err + } + id, err := convert.LedgerKeyToRegisterID(key) + if err != nil { + return nil, err + } + if id == contractNamesID { + contractNamesPayload = payload + contractNamesPayloadIndex = i + break + } + } + if contractNamesPayload == nil { + return payloads, nil + } - contractNames, err := accounts.GetContractNames(flowAddress) + var contractNames []string + err := cbor.Unmarshal(contractNamesPayload.Value(), &contractNames) if err != nil { return nil, fmt.Errorf("failed to get contract names: %w", err) } - if len(contractNames) == 1 { - return payloads, nil - } var foundDuplicate bool i := 1 @@ -92,37 +103,8 @@ func (d *DeduplicateContractNamesMigration) MigrateAccount( ) } - id := flow.ContractNamesRegisterID(flowAddress) - err = accounts.SetValue(id, newContractNames) - - if err != nil { - return nil, fmt.Errorf("setting value failed: %w", err) - } - - // finalize the transaction - result, err := transactionState.FinalizeMainTransaction() - if err != nil { - return nil, fmt.Errorf("failed to finalize main transaction: %w", err) - } - - for id, value := range result.WriteSet { - if value == nil { - delete(snapshot.Payloads, id) - continue - } - - snapshot.Payloads[id] = ledger.NewPayload( - convert.RegisterIDToLedgerKey(id), - value, - ) - } - - newPayloads := make([]*ledger.Payload, 0, len(snapshot.Payloads)) - for _, payload := range snapshot.Payloads { - newPayloads = append(newPayloads, payload) - } - - return newPayloads, nil + payloads[contractNamesPayloadIndex] = ledger.NewPayload(convert.RegisterIDToLedgerKey(contractNamesID), newContractNames) + return payloads, nil } From 05fbf9867febaabf5eaa42304f128a7cc537bc05 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Tue, 2 Jan 2024 19:01:00 +0100 Subject: [PATCH 4/5] add more tests --- .../deduplicate_contract_names_migration.go | 17 +- ...duplicate_contract_names_migration_test.go | 223 ++++++++++++++---- 2 files changed, 193 insertions(+), 47 deletions(-) diff --git a/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go b/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go index 4669f8e4b2b..16bfda771ce 100644 --- a/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go +++ b/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go @@ -66,8 +66,12 @@ func (d *DeduplicateContractNamesMigration) MigrateAccount( return payloads, nil } + value := contractNamesPayload.Value() + if len(value) == 0 { + return payloads, nil + } var contractNames []string - err := cbor.Unmarshal(contractNamesPayload.Value(), &contractNames) + err := cbor.Unmarshal(value, &contractNames) if err != nil { return nil, fmt.Errorf("failed to get contract names: %w", err) } @@ -76,6 +80,17 @@ func (d *DeduplicateContractNamesMigration) MigrateAccount( i := 1 for i < len(contractNames) { if contractNames[i-1] != contractNames[i] { + + if contractNames[i-1] > contractNames[i] { + // this is not a valid state and we should fail. + // Contract names must be sorted by definition. + return nil, fmt.Errorf( + "contract names for account %s are not sorted: %s", + address.Hex(), + contractNames, + ) + } + i++ continue } diff --git a/cmd/util/ledger/migrations/deduplicate_contract_names_migration_test.go b/cmd/util/ledger/migrations/deduplicate_contract_names_migration_test.go index a826d635ad4..ba81bc826cd 100644 --- a/cmd/util/ledger/migrations/deduplicate_contract_names_migration_test.go +++ b/cmd/util/ledger/migrations/deduplicate_contract_names_migration_test.go @@ -1,8 +1,10 @@ package migrations_test import ( - "bytes" "context" + "fmt" + "math/rand" + "sort" "testing" "github.com/fxamacker/cbor/v2" @@ -20,71 +22,200 @@ import ( func TestDeduplicateContractNamesMigration(t *testing.T) { migration := migrations.DeduplicateContractNamesMigration{} - log := zerolog.New(zerolog.NewTestWriter(t)) - err := migration.InitMigration(log, nil, 0) require.NoError(t, err) - ctx := context.Background() - address, err := common.HexToAddress("0x1") require.NoError(t, err) - contractNames := []string{"test", "test", "test2", "test3", "test3"} - - var buf bytes.Buffer - cborEncoder := cbor.NewEncoder(&buf) - err = cborEncoder.Encode(contractNames) - require.NoError(t, err) - newContractNames := buf.Bytes() + ctx := context.Background() accountStatus := environment.NewAccountStatus() - accountStatus.SetStorageUsed(1000) + accountStatusPayload := ledger.NewPayload( + convert.RegisterIDToLedgerKey( + flow.AccountStatusRegisterID(flow.ConvertAddress(address)), + ), + accountStatus.ToBytes(), + ) - payloads, err := migration.MigrateAccount(ctx, address, - []*ledger.Payload{ - - ledger.NewPayload( - convert.RegisterIDToLedgerKey( - flow.RegisterID{ - Owner: string(address.Bytes()), - Key: flow.ContractNamesKey, - }, - ), - newContractNames, + contractNamesPayload := func(contractNames []byte) *ledger.Payload { + return ledger.NewPayload( + convert.RegisterIDToLedgerKey( + flow.RegisterID{ + Owner: string(address.Bytes()), + Key: flow.ContractNamesKey, + }, ), - ledger.NewPayload( - convert.RegisterIDToLedgerKey( - flow.AccountStatusRegisterID(flow.ConvertAddress(address)), - ), - accountStatus.ToBytes(), - ), - }, - ) + contractNames, + ) + } - require.NoError(t, err) - require.Equal(t, 2, len(payloads)) + requireContractNames := func(payloads []*ledger.Payload, f func([]string)) { + for _, payload := range payloads { + key, err := payload.Key() + require.NoError(t, err) + id, err := convert.LedgerKeyToRegisterID(key) + require.NoError(t, err) + + if id.Key != flow.ContractNamesKey { + continue + } + + contracts := make([]string, 0) + err = cbor.Unmarshal(payload.Value(), &contracts) + require.NoError(t, err) + + f(contracts) + + } + } + + t.Run("no contract names", func(t *testing.T) { + payloads, err := migration.MigrateAccount(ctx, address, + []*ledger.Payload{ + accountStatusPayload, + }, + ) + + require.NoError(t, err) + require.Equal(t, 1, len(payloads)) + }) - for _, payload := range payloads { - key, err := payload.Key() + t.Run("one contract", func(t *testing.T) { + contractNames := []string{"test"} + newContractNames, err := cbor.Marshal(contractNames) require.NoError(t, err) - id, err := convert.LedgerKeyToRegisterID(key) + + payloads, err := migration.MigrateAccount(ctx, address, + []*ledger.Payload{ + accountStatusPayload, + contractNamesPayload(newContractNames), + }, + ) + require.NoError(t, err) + require.Equal(t, 2, len(payloads)) - if id.Key != flow.ContractNamesKey { - continue + requireContractNames(payloads, func(contracts []string) { + require.Equal(t, 1, len(contracts)) + require.Equal(t, "test", contracts[0]) + }) + }) + + t.Run("two unique contracts", func(t *testing.T) { + contractNames := []string{"test", "test2"} + newContractNames, err := cbor.Marshal(contractNames) + require.NoError(t, err) + + payloads, err := migration.MigrateAccount(ctx, address, + []*ledger.Payload{ + accountStatusPayload, + contractNamesPayload(newContractNames), + }, + ) + + require.NoError(t, err) + require.Equal(t, 2, len(payloads)) + + requireContractNames(payloads, func(contracts []string) { + require.Equal(t, 2, len(contracts)) + require.Equal(t, "test", contracts[0]) + require.Equal(t, "test2", contracts[1]) + }) + }) + + t.Run("two contracts", func(t *testing.T) { + contractNames := []string{"test", "test"} + newContractNames, err := cbor.Marshal(contractNames) + require.NoError(t, err) + + payloads, err := migration.MigrateAccount(ctx, address, + []*ledger.Payload{ + accountStatusPayload, + contractNamesPayload(newContractNames), + }, + ) + + require.NoError(t, err) + require.Equal(t, 2, len(payloads)) + + requireContractNames(payloads, func(contracts []string) { + require.Equal(t, 1, len(contracts)) + require.Equal(t, "test", contracts[0]) + }) + }) + + t.Run("not sorted contracts", func(t *testing.T) { + contractNames := []string{"test2", "test"} + newContractNames, err := cbor.Marshal(contractNames) + require.NoError(t, err) + + _, err = migration.MigrateAccount(ctx, address, + []*ledger.Payload{ + accountStatusPayload, + contractNamesPayload(newContractNames), + }, + ) + + require.Error(t, err) + }) + + t.Run("duplicate contracts", func(t *testing.T) { + contractNames := []string{"test", "test", "test2", "test3", "test3"} + newContractNames, err := cbor.Marshal(contractNames) + require.NoError(t, err) + + payloads, err := migration.MigrateAccount(ctx, address, + []*ledger.Payload{ + accountStatusPayload, + contractNamesPayload(newContractNames), + }, + ) + + require.NoError(t, err) + require.Equal(t, 2, len(payloads)) + + requireContractNames(payloads, func(contracts []string) { + require.Equal(t, 3, len(contracts)) + require.Equal(t, "test", contracts[0]) + require.Equal(t, "test2", contracts[1]) + require.Equal(t, "test3", contracts[2]) + }) + }) + + t.Run("random contracts", func(t *testing.T) { + contractNames := make([]string, 1000) + uniqueContracts := 1 + for i := 0; i < 1000; i++ { + // i > 0 so it's easier to know how many unique contracts there are + if i > 0 && rand.Float32() < 0.5 { + uniqueContracts++ + } + contractNames[i] = fmt.Sprintf("test%d", uniqueContracts) } - value := payload.Value() + // sort contractNames alphabetically, because they are not sorted + sort.Slice(contractNames, func(i, j int) bool { + return contractNames[i] < contractNames[j] + }) - contracts := make([]string, 0) - buf := bytes.NewReader(value) - cborDecoder := cbor.NewDecoder(buf) - err = cborDecoder.Decode(&contracts) + newContractNames, err := cbor.Marshal(contractNames) require.NoError(t, err) - require.Equal(t, 3, len(contracts)) - } + payloads, err := migration.MigrateAccount(ctx, address, + []*ledger.Payload{ + accountStatusPayload, + contractNamesPayload(newContractNames), + }, + ) + + require.NoError(t, err) + require.Equal(t, 2, len(payloads)) + + requireContractNames(payloads, func(contracts []string) { + require.Equal(t, uniqueContracts, len(contracts)) + }) + }) } From bd7e9199dcedafe1837126d19fd18b418c3dd881 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Tue, 2 Jan 2024 21:50:37 +0100 Subject: [PATCH 5/5] remove empty payload --- .../migrations/deduplicate_contract_names_migration.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go b/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go index 16bfda771ce..ab35e04d8a3 100644 --- a/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go +++ b/cmd/util/ledger/migrations/deduplicate_contract_names_migration.go @@ -16,6 +16,8 @@ import ( // DeduplicateContractNamesMigration checks if the contract names have been duplicated and // removes the duplicate ones. +// +// This migration de-syncs storage used, so it should be run before the StorageUsedMigration. type DeduplicateContractNamesMigration struct { log zerolog.Logger } @@ -68,8 +70,13 @@ func (d *DeduplicateContractNamesMigration) MigrateAccount( value := contractNamesPayload.Value() if len(value) == 0 { + // Remove the empty payload + copy(payloads[contractNamesPayloadIndex:], payloads[contractNamesPayloadIndex+1:]) + payloads = payloads[:len(payloads)-1] + return payloads, nil } + var contractNames []string err := cbor.Unmarshal(value, &contractNames) if err != nil {