From c795bfc13fe6db729410b877c45de3a69632a657 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Tue, 19 Nov 2024 11:10:44 -0600 Subject: [PATCH 1/8] Reduce register reads with StorageFormatV2Enabled Currently, all domain registers are being read for a scenario that does not require it. When StorageFormatV2 feature flag is enabled, Storage.GetDomainStorageMap() reads all domain registers to determine if account is new account or in V1 format. These register reads are unnecessary for V1 accounts when: - the given domain register exists, or - the given domain register doesn't exist and createIfNotExists param is false. This commit modifies Storage.GetDomainStorageMap() to read the given domain register first (before iterating all domain registers) to reduce register reads. - If the given domain register exists, then account format is V1 and no more register reading is needed. - If the given domain register doesn't exist and createIfNotExists is false, then nil domain storage map is returned and no more register reading is needed. - Otherwise, account format (V1 account or new account) is determined by iterating all domain registers. --- runtime/sharedstate_test.go | 8 +- runtime/storage.go | 172 +++++++++++++++++++++++++++++++----- 2 files changed, 157 insertions(+), 23 deletions(-) diff --git a/runtime/sharedstate_test.go b/runtime/sharedstate_test.go index 52dd68f00..b2cd5cc49 100644 --- a/runtime/sharedstate_test.go +++ b/runtime/sharedstate_test.go @@ -255,12 +255,18 @@ func TestRuntimeSharedState(t *testing.T) { test( true, []ownerKeyPair{ - // Read account domain register to check if it is a migrated account + // Read account register to check if it is a migrated account // Read returns no value. { owner: signerAddress[:], key: []byte(AccountStorageKey), }, + // Read contract domain register. + // Read returns no value. + { + owner: signerAddress[:], + key: []byte(common.StorageDomainContract.Identifier()), + }, // Read all available domain registers to check if it is a new account // Read returns no value. { diff --git a/runtime/storage.go b/runtime/storage.go index 8198e69fa..aab2f1dab 100644 --- a/runtime/storage.go +++ b/runtime/storage.go @@ -40,6 +40,15 @@ type StorageConfig struct { StorageFormatV2Enabled bool } +type storageFormat uint8 + +const ( + storageFormatUnknown storageFormat = iota + storageFormatNew + storageFormatV1 + StorageFormatV2 +) + type Storage struct { *atree.PersistentSlabStorage @@ -160,7 +169,10 @@ func (s *Storage) GetDomainStorageMap( } }() - if !s.Config.StorageFormatV2Enabled || s.IsV1Account(address) { + if !s.Config.StorageFormatV2Enabled { + + // When StorageFormatV2 is disabled, handle all accounts as v1 accounts. + domainStorageMap = s.AccountStorageV1.GetDomainStorageMap( address, domain, @@ -172,44 +184,160 @@ func (s *Storage) GetDomainStorageMap( } } else { - domainStorageMap = s.AccountStorageV2.GetDomainStorageMap( - inter, - address, - domain, - createIfNotExists, - ) + + // StorageFormatV2 is enabled. + + onlyCheckSpecifiedDomainForV1 := !createIfNotExists + format := s.getAccountStorageFormat(address, domain, onlyCheckSpecifiedDomainForV1) + + switch format { + + case storageFormatUnknown: + // storageFormatUnknown is returned when !createIfNotExists + // and domain register doesn't exist. + + if createIfNotExists { + panic(errors.NewUnreachableError()) + } + + domainStorageMap = nil + + case storageFormatV1: + domainStorageMap = s.AccountStorageV1.GetDomainStorageMap( + address, + domain, + createIfNotExists, + ) + + s.cacheIsV1Account(address, true) + + case StorageFormatV2, storageFormatNew: + domainStorageMap = s.AccountStorageV2.GetDomainStorageMap( + inter, + address, + domain, + createIfNotExists, + ) + + s.cacheIsV1Account(address, false) + + default: + panic(errors.NewUnreachableError()) + } } return domainStorageMap } -// IsV1Account returns true if given account is in account storage format v1. -func (s *Storage) IsV1Account(address common.Address) (isV1 bool) { +// getAccountStorageFormat returns storageFormat for the given account. +// This function determines account format by reading registers. +// If onlyCheckSpecifiedDomainForV1 is true, only the given domain +// register is checked to determine if account format is v1. If +// domain register doesn't exist, StorageFormatUnknown is returned. +// +// When StorageFormatV2 is disabled: +// - No register reading (accounts are assumed to be in v1 format). +// +// When StorageFormatV2 is enabled: +// - For v2 accounts, "stored" register is read. +// +// - For v1 accounts, +// - If onlyCheckSpecifiedDomainForV1 is true, +// "stored" register and given domain register are read. +// - If onlyCheckSpecifiedDomainForV1 is false and given domain exists, +// "stored" register and given domain register are read. +// - If onlyCheckSpecifiedDomainForV1 is false and given domain doesn't exist, +// "stored" register, given domain register, and all domain registers are read. +// +// - For new accounts, "stored" register, given domain register, and all domain registers are read. +func (s *Storage) getAccountStorageFormat( + address common.Address, + domain common.StorageDomain, + onlyCheckSpecifiedDomainForV1 bool, +) (format storageFormat) { - // Check cache + // All accounts are assumed to be in v1 format when StorageFormatV2 is disabled. - if isV1, present := s.cachedV1Accounts[address]; present { - return isV1 + if !s.Config.StorageFormatV2Enabled { + return storageFormatV1 } - // Cache result + // Return cached account format (no register reading). - defer func() { - s.cacheIsV1Account(address, isV1) - }() + isCachedV1, isCachedV2 := s.getCachedAccountFormat(address) - // First check if account storage map exists. - // In that case the account was already migrated to account storage format v2, - // and we do not need to check the domain storage map registers. + if isCachedV1 { + return storageFormatV1 + } + + if isCachedV2 { + return StorageFormatV2 + } + + // Check if account is v2 (by reading "stored" register). + + if s.isV2Account(address) { + return StorageFormatV2 + } + + // Check if account is v1 (by reading given domain register). + + if s.hasDomainRegister(address, domain) { + return storageFormatV1 + } + + // Return early if onlyCheckSpecifiedDomainForV1 to prevent more register reading. + + if onlyCheckSpecifiedDomainForV1 { + return storageFormatUnknown + } + // At this point, account is either new account or v1 account without given domain register. + + if s.isV1Account(address) { + return storageFormatV1 + } + + return storageFormatNew +} + +func (s *Storage) getCachedAccountFormat(address common.Address) (isV1 bool, isV2 bool) { + isV1, cached := s.cachedV1Accounts[address] + if !cached { + return false, false + } + return isV1, !isV1 +} + +// isV2Account returns true if given account is in account storage format v2. +func (s *Storage) isV2Account(address common.Address) bool { accountStorageMapExists, err := hasAccountStorageMap(s.Ledger, address) if err != nil { panic(err) } - if accountStorageMapExists { - return false + + return accountStorageMapExists +} + +// hasDomainRegister returns true if given account has given domain register. +// NOTE: account storage format v1 has domain registers. +func (s *Storage) hasDomainRegister(address common.Address, domain common.StorageDomain) (domainExists bool) { + _, domainExists, err := readSlabIndexFromRegister( + s.Ledger, + address, + []byte(domain.Identifier()), + ) + if err != nil { + panic(err) } + return domainExists +} + +// isV1Account returns true if given account is in account storage format v1 +// by checking if any of the domain registers exist. +func (s *Storage) isV1Account(address common.Address) (isV1 bool) { + // Check if a storage map register exists for any of the domains. // Check the most frequently used domains first, such as storage, public, private. for _, domain := range common.AllStorageDomains { @@ -492,7 +620,7 @@ func (s *Storage) CheckHealth() error { // Only accounts in storage format v1 store domain storage maps // directly at the root of the account - if !s.IsV1Account(address) { + if !s.isV1Account(address) { continue } From f56272754ac0c867f15059476bdacab38c65ab3b Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Wed, 20 Nov 2024 10:13:18 -0600 Subject: [PATCH 2/8] Add tests for register reads for GetDomainStorageMap() This commit adds tests for register reads from GetStorageMap() for account storage format v1 and v2. --- runtime/storage_test.go | 829 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 829 insertions(+) diff --git a/runtime/storage_test.go b/runtime/storage_test.go index 4e4dc261b..ab5640fe8 100644 --- a/runtime/storage_test.go +++ b/runtime/storage_test.go @@ -23,12 +23,14 @@ import ( "encoding/hex" "fmt" "math/rand" + "runtime" "slices" "sort" "strconv" "strings" "testing" + "github.com/fxamacker/cbor/v2" "github.com/onflow/atree" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -8081,6 +8083,801 @@ func TestDomainRegisterMigrationForLargeAccount(t *testing.T) { checkAccountStorageMapData(t, ledger.StoredValues, ledger.StorageIndices, address, accountValues) } +func TestGetDomainStorageMapRegisterReadsForNewAccount(t *testing.T) { + t.Parallel() + + address := common.MustBytesToAddress([]byte{0x1}) + + testCases := []struct { + name string + storageFormatV2Enabled bool + domain common.StorageDomain + createIfNotExists bool + expectedDomainStorageMapIsNil bool + expectedReadsFor1stGetDomainStorageMapCall []ownerKeyPair + expectedReadsFor2ndGetDomainStorageMapCall []ownerKeyPair + }{ + // Test cases with storageFormatV2Enabled = false + { + name: "storageFormatV2Enabled = false, domain storage map does not exist, createIfNotExists = false", + storageFormatV2Enabled: false, + domain: common.StorageDomainPathStorage, + createIfNotExists: false, + expectedDomainStorageMapIsNil: true, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Read domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // Read domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + }, + }, + { + name: "storageFormatV2Enabled = false, domain storage map does not exist, createIfNotExists = true", + storageFormatV2Enabled: false, + domain: common.StorageDomainPathStorage, + createIfNotExists: true, + expectedDomainStorageMapIsNil: false, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Read domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // No register reads from the second GetDomainStorageMap() because + // domain storage map is created and cached in the first GetDomainStorageMap(). + }, + }, + // Test cases with storageFormatV2Enabled = true + { + name: "storageFormatV2Enabled = true, domain storage map does not exist, createIfNotExists = false", + storageFormatV2Enabled: true, + domain: common.StorageDomainPathStorage, + createIfNotExists: false, + expectedDomainStorageMapIsNil: true, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Check if account is v2 + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Check domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // Second GetDomainStorageMap() has the same register reading as the first GetDomainStorageMap() + // because account status can't be cached in previous call. + + // Check if account is v2 + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Check domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + }, + }, + { + name: "storageFormatV2Enabled = true, domain storage map does not exist, createIfNotExists = true", + storageFormatV2Enabled: true, + domain: common.StorageDomainPathStorage, + createIfNotExists: true, + expectedDomainStorageMapIsNil: false, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Check if account is v2 + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Check domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + // Check all domain registers + { + owner: address[:], + key: []byte(common.PathDomainStorage.Identifier()), + }, + { + owner: address[:], + key: []byte(common.PathDomainPrivate.Identifier()), + }, + { + owner: address[:], + key: []byte(common.PathDomainPublic.Identifier()), + }, + { + owner: address[:], + key: []byte(common.StorageDomainContract.Identifier()), + }, + { + owner: address[:], + key: []byte(common.StorageDomainInbox.Identifier()), + }, + { + owner: address[:], + key: []byte(common.StorageDomainCapabilityController.Identifier()), + }, + { + owner: address[:], + key: []byte(common.StorageDomainCapabilityControllerTag.Identifier()), + }, + { + owner: address[:], + key: []byte(common.StorageDomainPathCapability.Identifier()), + }, + { + owner: address[:], + key: []byte(common.StorageDomainAccountCapability.Identifier()), + }, + // Read account register to load account storage map + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // No register reads from the second GetDomainStorageMap() because + // domain storage map is created and cached in the first GetDomainStorageMap(). + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + + var ledgerReads []ownerKeyPair + + // Create empty storage + ledger := NewTestLedger( + func(owner, key, _ []byte) { + ledgerReads = append( + ledgerReads, + ownerKeyPair{ + owner: owner, + key: key, + }, + ) + }, + nil) + + storage := NewStorage( + ledger, + nil, + StorageConfig{ + StorageFormatV2Enabled: tc.storageFormatV2Enabled, + }, + ) + + inter := NewTestInterpreterWithStorage(t, storage) + + domainStorageMap := storage.GetDomainStorageMap(inter, address, tc.domain, tc.createIfNotExists) + require.Equal(t, tc.expectedDomainStorageMapIsNil, domainStorageMap == nil) + require.Equal(t, tc.expectedReadsFor1stGetDomainStorageMapCall, ledgerReads) + + ledgerReads = ledgerReads[:0] + + // Call GetDomainStorageMap() again to test account status is cached and no register reading is needed. + + domainStorageMap = storage.GetDomainStorageMap(inter, address, tc.domain, tc.createIfNotExists) + require.Equal(t, tc.expectedDomainStorageMapIsNil, domainStorageMap == nil) + require.Equal(t, tc.expectedReadsFor2ndGetDomainStorageMapCall, ledgerReads) + }) + } +} + +func TestGetDomainStorageMapRegisterReadsForV1Account(t *testing.T) { + + t.Parallel() + + address := common.MustBytesToAddress([]byte{0x1}) + + type getStorageDataFunc func() (storedValues map[string][]byte, StorageIndices map[string]uint64) + + createV1AccountWithDomain := func( + address common.Address, + domain common.StorageDomain, + ) getStorageDataFunc { + return func() (storedValues map[string][]byte, StorageIndices map[string]uint64) { + ledger := NewTestLedger(nil, nil) + + persistentSlabStorage := newSlabStorage(ledger) + + orderedMap, err := atree.NewMap( + persistentSlabStorage, + atree.Address(address), + atree.NewDefaultDigesterBuilder(), + interpreter.EmptyTypeInfo{}, + ) + require.NoError(t, err) + + slabIndex := orderedMap.SlabID().Index() + + for i := range 3 { + + key := interpreter.StringStorageMapKey(strconv.Itoa(i)) + + value := interpreter.NewUnmeteredIntValueFromInt64(int64(i)) + + existingStorable, err := orderedMap.Set( + key.AtreeValueCompare, + key.AtreeValueHashInput, + key.AtreeValue(), + value, + ) + require.NoError(t, err) + require.Nil(t, existingStorable) + } + + // Commit domain storage map + err = persistentSlabStorage.FastCommit(runtime.NumCPU()) + require.NoError(t, err) + + // Create domain register + err = ledger.SetValue(address[:], []byte(domain.Identifier()), slabIndex[:]) + require.NoError(t, err) + + return ledger.StoredValues, ledger.StorageIndices + } + } + + testCases := []struct { + name string + getStorageData getStorageDataFunc + storageFormatV2Enabled bool + domain common.StorageDomain + createIfNotExists bool + expectedDomainStorageMapIsNil bool + expectedReadsFor1stGetDomainStorageMapCall []ownerKeyPair + expectedReadsFor2ndGetDomainStorageMapCall []ownerKeyPair + }{ + // Test cases with storageFormatV2Enabled = false + { + name: "storageFormatV2Enabled = false, domain storage map does not exist, createIfNotExists = false", + storageFormatV2Enabled: false, + getStorageData: createV1AccountWithDomain(address, common.StorageDomainPathPublic), + domain: common.StorageDomainPathStorage, + createIfNotExists: false, + expectedDomainStorageMapIsNil: true, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Read domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // Read domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + }, + }, + { + name: "storageFormatV2Enabled = false, domain storage map does not exist, createIfNotExists = true", + storageFormatV2Enabled: false, + getStorageData: createV1AccountWithDomain(address, common.StorageDomainPathPublic), + domain: common.StorageDomainPathStorage, + createIfNotExists: true, + expectedDomainStorageMapIsNil: false, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Read domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // No register reading in second GetDomainStorageMap() because + // domain storage map is created and cached in the first + // GetDomainStorageMap(0). + }, + }, + { + name: "storageFormatV2Enabled = false, domain storage map exists, createIfNotExists = false", + storageFormatV2Enabled: false, + getStorageData: createV1AccountWithDomain(address, common.StorageDomainPathStorage), + domain: common.StorageDomainPathStorage, + createIfNotExists: false, + expectedDomainStorageMapIsNil: false, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Read domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + // Read domain storage map register + { + owner: address[:], + key: []byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}, + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // No register reading in second GetDomainStorageMap() because + // domain storage map is loaded and cached in the first + // GetDomainStorageMap(0). + }, + }, + { + name: "storageFormatV2Enabled = false, domain storage map exists, createIfNotExists = true", + storageFormatV2Enabled: false, + getStorageData: createV1AccountWithDomain(address, common.StorageDomainPathStorage), + domain: common.StorageDomainPathStorage, + createIfNotExists: true, + expectedDomainStorageMapIsNil: false, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Read domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + // Read domain storage map register + { + owner: address[:], + key: []byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}, + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // No register reading in second GetDomainStorageMap() because + // domain storage map is loaded and cached in the first + // GetDomainStorageMap(0). + }, + }, + // Test cases with storageFormatV2Enabled = true + { + name: "storageFormatV2Enabled = true, domain storage map does not exist, createIfNotExists = false", + storageFormatV2Enabled: true, + getStorageData: createV1AccountWithDomain(address, common.StorageDomainPathPublic), + domain: common.StorageDomainPathStorage, + createIfNotExists: false, + expectedDomainStorageMapIsNil: true, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Check if account is v2 + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Check domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // Check if account is v2 + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Check domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + }, + }, + { + name: "storageFormatV2Enabled = true, domain storage map does not exist, createIfNotExists = true", + storageFormatV2Enabled: true, + getStorageData: createV1AccountWithDomain(address, common.StorageDomainPathPublic), + domain: common.StorageDomainPathStorage, + createIfNotExists: true, + expectedDomainStorageMapIsNil: false, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Check if account is v2 + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Check domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + // Check all domain registers until any existing domain is checked + { + owner: address[:], + key: []byte(common.PathDomainStorage.Identifier()), + }, + { + owner: address[:], + key: []byte(common.PathDomainPrivate.Identifier()), + }, + { + owner: address[:], + key: []byte(common.PathDomainPublic.Identifier()), + }, + // Check domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // No register reading from second GetDomainStorageMap() because + // domain storage map is created and cached in the first + // GetDomainStorageMap(). + }, + }, + { + name: "storageFormatV2Enabled = true, domain storage map exists, createIfNotExists = false", + storageFormatV2Enabled: true, + getStorageData: createV1AccountWithDomain(address, common.StorageDomainPathStorage), + domain: common.StorageDomainPathStorage, + createIfNotExists: false, + expectedDomainStorageMapIsNil: false, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Check if account is v2 + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Check domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + // Read domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + // Read domain storage map register + { + owner: address[:], + key: []byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}, + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // No register reading from second GetDomainStorageMap() because + // domain storage map is created and cached in the first + // GetDomainStorageMap(). + }, + }, + { + name: "storageFormatV2Enabled = true, domain storage map exists, createIfNotExists = true", + storageFormatV2Enabled: true, + getStorageData: createV1AccountWithDomain(address, common.StorageDomainPathStorage), + domain: common.StorageDomainPathStorage, + createIfNotExists: true, + expectedDomainStorageMapIsNil: false, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Check if account is v2 + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Check given domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + // Read given domain register + { + owner: address[:], + key: []byte(common.StorageDomainPathStorage.Identifier()), + }, + // Read domain storage map register + { + owner: address[:], + key: []byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}, + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // No register reading from second GetDomainStorageMap() because + // domain storage map is created and cached in the first + // GetDomainStorageMap(). + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + + storedValues, storedIndices := tc.getStorageData() + + var ledgerReads []ownerKeyPair + + ledger := NewTestLedgerWithData( + func(owner, key, _ []byte) { + ledgerReads = append( + ledgerReads, + ownerKeyPair{ + owner: owner, + key: key, + }, + ) + }, + nil, + storedValues, + storedIndices, + ) + + storage := NewStorage( + ledger, + nil, + StorageConfig{ + StorageFormatV2Enabled: tc.storageFormatV2Enabled, + }, + ) + + inter := NewTestInterpreterWithStorage(t, storage) + + domainStorageMap := storage.GetDomainStorageMap(inter, address, tc.domain, tc.createIfNotExists) + require.Equal(t, tc.expectedDomainStorageMapIsNil, domainStorageMap == nil) + require.Equal(t, tc.expectedReadsFor1stGetDomainStorageMapCall, ledgerReads) + + ledgerReads = ledgerReads[:0] + + domainStorageMap = storage.GetDomainStorageMap(inter, address, tc.domain, tc.createIfNotExists) + require.Equal(t, tc.expectedDomainStorageMapIsNil, domainStorageMap == nil) + require.Equal(t, tc.expectedReadsFor2ndGetDomainStorageMapCall, ledgerReads) + }) + } +} + +func TestGetDomainStorageMapRegisterReadsForV2Account(t *testing.T) { + t.Parallel() + + address := common.MustBytesToAddress([]byte{0x1}) + + type getStorageDataFunc func() (storedValues map[string][]byte, StorageIndices map[string]uint64) + + createV2AccountWithDomain := func( + address common.Address, + domain common.StorageDomain, + ) getStorageDataFunc { + return func() (storedValues map[string][]byte, StorageIndices map[string]uint64) { + ledger := NewTestLedger(nil, nil) + + persistentSlabStorage := newSlabStorage(ledger) + + accountOrderedMap, err := atree.NewMap( + persistentSlabStorage, + atree.Address(address), + atree.NewDefaultDigesterBuilder(), + interpreter.EmptyTypeInfo{}, + ) + require.NoError(t, err) + + slabIndex := accountOrderedMap.SlabID().Index() + + domainOrderedMap, err := atree.NewMap( + persistentSlabStorage, + atree.Address(address), + atree.NewDefaultDigesterBuilder(), + interpreter.EmptyTypeInfo{}, + ) + require.NoError(t, err) + + domainKey := interpreter.Uint64StorageMapKey(domain) + + existingDomain, err := accountOrderedMap.Set( + domainKey.AtreeValueCompare, + domainKey.AtreeValueHashInput, + domainKey.AtreeValue(), + domainOrderedMap, + ) + require.NoError(t, err) + require.Nil(t, existingDomain) + + for i := range 3 { + + key := interpreter.StringStorageMapKey(strconv.Itoa(i)) + + value := interpreter.NewUnmeteredIntValueFromInt64(int64(i)) + + existingStorable, err := domainOrderedMap.Set( + key.AtreeValueCompare, + key.AtreeValueHashInput, + key.AtreeValue(), + value, + ) + require.NoError(t, err) + require.Nil(t, existingStorable) + } + + // Commit domain storage map + err = persistentSlabStorage.FastCommit(runtime.NumCPU()) + require.NoError(t, err) + + // Create account register + err = ledger.SetValue(address[:], []byte(AccountStorageKey), slabIndex[:]) + require.NoError(t, err) + + return ledger.StoredValues, ledger.StorageIndices + } + } + + testCases := []struct { + name string + getStorageData getStorageDataFunc + domain common.StorageDomain + createIfNotExists bool + expectedDomainStorageMapIsNil bool + expectedReadsFor1stGetDomainStorageMapCall []ownerKeyPair + expectedReadsFor2ndGetDomainStorageMapCall []ownerKeyPair + }{ + { + name: "domain storage map does not exist, createIfNotExists = false", + getStorageData: createV2AccountWithDomain(address, common.StorageDomainPathPublic), + domain: common.StorageDomainPathStorage, + createIfNotExists: false, + expectedDomainStorageMapIsNil: true, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Check if account is v2 + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Read account register + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Read account storage map + { + owner: address[:], + key: []byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}, + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // No register reading from second GetDomainStorageMap because + // account storage map is loaded and cached from first + // GetDomainStorageMap(). + }, + }, + { + name: "domain storage map does not exist, createIfNotExists = true", + getStorageData: createV2AccountWithDomain(address, common.StorageDomainPathPublic), + domain: common.StorageDomainPathStorage, + createIfNotExists: true, + expectedDomainStorageMapIsNil: false, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Check if account is v2 + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Read account register + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Read account storage map + { + owner: address[:], + key: []byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}, + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // No register reading from second GetDomainStorageMap() because + // domain storage map is created and cached in the first + // GetDomainStorageMap(). + }, + }, + { + name: "domain storage map exists, createIfNotExists = false", + getStorageData: createV2AccountWithDomain(address, common.StorageDomainPathStorage), + domain: common.StorageDomainPathStorage, + createIfNotExists: false, + expectedDomainStorageMapIsNil: false, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Check if account is v2 + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Read account register + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Read account storage map + { + owner: address[:], + key: []byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}, + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // No register reading from second GetDomainStorageMap() because + // domain storage map is created and cached in the first + // GetDomainStorageMap(). + }, + }, + { + name: "domain storage map exists, createIfNotExists = true", + getStorageData: createV2AccountWithDomain(address, common.StorageDomainPathStorage), + domain: common.StorageDomainPathStorage, + createIfNotExists: true, + expectedDomainStorageMapIsNil: false, + expectedReadsFor1stGetDomainStorageMapCall: []ownerKeyPair{ + // Check if account is v2 + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Read account register + { + owner: address[:], + key: []byte(AccountStorageKey), + }, + // Read account storage map + { + owner: address[:], + key: []byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}, + }, + }, + expectedReadsFor2ndGetDomainStorageMapCall: []ownerKeyPair{ + // No register reading from second GetDomainStorageMap() because + // domain storage map is created and cached in the first + // GetDomainStorageMap(). + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + + storedValues, storedIndices := tc.getStorageData() + + var ledgerReads []ownerKeyPair + + ledger := NewTestLedgerWithData( + func(owner, key, _ []byte) { + ledgerReads = append( + ledgerReads, + ownerKeyPair{ + owner: owner, + key: key, + }, + ) + }, + nil, + storedValues, + storedIndices, + ) + + storage := NewStorage( + ledger, + nil, + StorageConfig{ + StorageFormatV2Enabled: true, + }, + ) + + inter := NewTestInterpreterWithStorage(t, storage) + + domainStorageMap := storage.GetDomainStorageMap(inter, address, tc.domain, tc.createIfNotExists) + require.Equal(t, tc.expectedDomainStorageMapIsNil, domainStorageMap == nil) + require.Equal(t, tc.expectedReadsFor1stGetDomainStorageMapCall, ledgerReads) + + ledgerReads = ledgerReads[:0] + + domainStorageMap = storage.GetDomainStorageMap(inter, address, tc.domain, tc.createIfNotExists) + require.Equal(t, tc.expectedDomainStorageMapIsNil, domainStorageMap == nil) + require.Equal(t, tc.expectedReadsFor2ndGetDomainStorageMapCall, ledgerReads) + }) + } +} + // createAndWriteAccountStorageMap creates account storage map with given domains and writes random values to domain storage map. func createAndWriteAccountStorageMap( t testing.TB, @@ -8207,3 +9004,35 @@ func checkAccountStorageMapData( require.Equal(tb, 1, len(rootSlabIDs)) require.Contains(tb, rootSlabIDs, accountSlabID) } + +func newSlabStorage(ledger atree.Ledger) *atree.PersistentSlabStorage { + decodeStorable := func( + decoder *cbor.StreamDecoder, + slabID atree.SlabID, + inlinedExtraData []atree.ExtraData, + ) ( + atree.Storable, + error, + ) { + return interpreter.DecodeStorable( + decoder, + slabID, + inlinedExtraData, + nil, + ) + } + + decodeTypeInfo := func(decoder *cbor.StreamDecoder) (atree.TypeInfo, error) { + return interpreter.DecodeTypeInfo(decoder, nil) + } + + ledgerStorage := atree.NewLedgerBaseStorage(ledger) + + return atree.NewPersistentSlabStorage( + ledgerStorage, + interpreter.CBOREncMode, + interpreter.CBORDecMode, + decodeStorable, + decodeTypeInfo, + ) +} From 9af92f66fac1926eea058c06c2f9bcc7cd3bd7a3 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Mon, 25 Nov 2024 10:38:15 -0600 Subject: [PATCH 3/8] Update unit tests to check unique register reads Currently, tests for register reads only check sequence of register reading. However, same register can be read multiple times and all register reads are included in the sequence. This commit updates tests to explicitly checks unique registers being read in addition to sequence of registers being read. --- runtime/storage_test.go | 99 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/runtime/storage_test.go b/runtime/storage_test.go index ab5640fe8..9b0022384 100644 --- a/runtime/storage_test.go +++ b/runtime/storage_test.go @@ -8096,6 +8096,7 @@ func TestGetDomainStorageMapRegisterReadsForNewAccount(t *testing.T) { expectedDomainStorageMapIsNil bool expectedReadsFor1stGetDomainStorageMapCall []ownerKeyPair expectedReadsFor2ndGetDomainStorageMapCall []ownerKeyPair + expectedReadsSet map[string]struct{} }{ // Test cases with storageFormatV2Enabled = false { @@ -8118,6 +8119,9 @@ func TestGetDomainStorageMapRegisterReadsForNewAccount(t *testing.T) { key: []byte(common.StorageDomainPathStorage.Identifier()), }, }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + common.StorageDomainPathStorage.Identifier(): {}, + }, }, { name: "storageFormatV2Enabled = false, domain storage map does not exist, createIfNotExists = true", @@ -8136,6 +8140,9 @@ func TestGetDomainStorageMapRegisterReadsForNewAccount(t *testing.T) { // No register reads from the second GetDomainStorageMap() because // domain storage map is created and cached in the first GetDomainStorageMap(). }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + common.StorageDomainPathStorage.Identifier(): {}, + }, }, // Test cases with storageFormatV2Enabled = true { @@ -8171,6 +8178,10 @@ func TestGetDomainStorageMapRegisterReadsForNewAccount(t *testing.T) { key: []byte(common.StorageDomainPathStorage.Identifier()), }, }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + AccountStorageKey: {}, + string(address[:]) + "|" + common.StorageDomainPathStorage.Identifier(): {}, + }, }, { name: "storageFormatV2Enabled = true, domain storage map does not exist, createIfNotExists = true", @@ -8236,6 +8247,18 @@ func TestGetDomainStorageMapRegisterReadsForNewAccount(t *testing.T) { // No register reads from the second GetDomainStorageMap() because // domain storage map is created and cached in the first GetDomainStorageMap(). }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + AccountStorageKey: {}, + string(address[:]) + "|" + common.StorageDomainPathStorage.Identifier(): {}, + string(address[:]) + "|" + common.StorageDomainPathPrivate.Identifier(): {}, + string(address[:]) + "|" + common.StorageDomainPathPublic.Identifier(): {}, + string(address[:]) + "|" + common.StorageDomainContract.Identifier(): {}, + string(address[:]) + "|" + common.StorageDomainInbox.Identifier(): {}, + string(address[:]) + "|" + common.StorageDomainCapabilityController.Identifier(): {}, + string(address[:]) + "|" + common.StorageDomainCapabilityControllerTag.Identifier(): {}, + string(address[:]) + "|" + common.StorageDomainPathCapability.Identifier(): {}, + string(address[:]) + "|" + common.StorageDomainAccountCapability.Identifier(): {}, + }, }, } @@ -8243,6 +8266,7 @@ func TestGetDomainStorageMapRegisterReadsForNewAccount(t *testing.T) { t.Run(tc.name, func(t *testing.T) { var ledgerReads []ownerKeyPair + ledgerReadsSet := make(map[string]struct{}) // Create empty storage ledger := NewTestLedger( @@ -8254,6 +8278,7 @@ func TestGetDomainStorageMapRegisterReadsForNewAccount(t *testing.T) { key: key, }, ) + ledgerReadsSet[string(owner)+"|"+string(key)] = struct{}{} }, nil) @@ -8278,6 +8303,12 @@ func TestGetDomainStorageMapRegisterReadsForNewAccount(t *testing.T) { domainStorageMap = storage.GetDomainStorageMap(inter, address, tc.domain, tc.createIfNotExists) require.Equal(t, tc.expectedDomainStorageMapIsNil, domainStorageMap == nil) require.Equal(t, tc.expectedReadsFor2ndGetDomainStorageMapCall, ledgerReads) + + // Check underlying ledger reads + require.Equal(t, len(ledgerReadsSet), len(tc.expectedReadsSet)) + for k := range ledgerReadsSet { + require.Contains(t, tc.expectedReadsSet, k) + } }) } } @@ -8346,6 +8377,7 @@ func TestGetDomainStorageMapRegisterReadsForV1Account(t *testing.T) { expectedDomainStorageMapIsNil bool expectedReadsFor1stGetDomainStorageMapCall []ownerKeyPair expectedReadsFor2ndGetDomainStorageMapCall []ownerKeyPair + expectedReadsSet map[string]struct{} }{ // Test cases with storageFormatV2Enabled = false { @@ -8369,6 +8401,9 @@ func TestGetDomainStorageMapRegisterReadsForV1Account(t *testing.T) { key: []byte(common.StorageDomainPathStorage.Identifier()), }, }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + common.StorageDomainPathStorage.Identifier(): {}, + }, }, { name: "storageFormatV2Enabled = false, domain storage map does not exist, createIfNotExists = true", @@ -8389,6 +8424,9 @@ func TestGetDomainStorageMapRegisterReadsForV1Account(t *testing.T) { // domain storage map is created and cached in the first // GetDomainStorageMap(0). }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + common.StorageDomainPathStorage.Identifier(): {}, + }, }, { name: "storageFormatV2Enabled = false, domain storage map exists, createIfNotExists = false", @@ -8414,6 +8452,10 @@ func TestGetDomainStorageMapRegisterReadsForV1Account(t *testing.T) { // domain storage map is loaded and cached in the first // GetDomainStorageMap(0). }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + common.StorageDomainPathStorage.Identifier(): {}, + string(address[:]) + "|" + string([]byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}): {}, + }, }, { name: "storageFormatV2Enabled = false, domain storage map exists, createIfNotExists = true", @@ -8439,6 +8481,10 @@ func TestGetDomainStorageMapRegisterReadsForV1Account(t *testing.T) { // domain storage map is loaded and cached in the first // GetDomainStorageMap(0). }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + common.StorageDomainPathStorage.Identifier(): {}, + string(address[:]) + "|" + string([]byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}): {}, + }, }, // Test cases with storageFormatV2Enabled = true { @@ -8472,6 +8518,10 @@ func TestGetDomainStorageMapRegisterReadsForV1Account(t *testing.T) { key: []byte(common.StorageDomainPathStorage.Identifier()), }, }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + AccountStorageKey: {}, + string(address[:]) + "|" + common.StorageDomainPathStorage.Identifier(): {}, + }, }, { name: "storageFormatV2Enabled = true, domain storage map does not exist, createIfNotExists = true", @@ -8515,6 +8565,12 @@ func TestGetDomainStorageMapRegisterReadsForV1Account(t *testing.T) { // domain storage map is created and cached in the first // GetDomainStorageMap(). }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + AccountStorageKey: {}, + string(address[:]) + "|" + common.StorageDomainPathStorage.Identifier(): {}, + string(address[:]) + "|" + common.StorageDomainPathPrivate.Identifier(): {}, + string(address[:]) + "|" + common.StorageDomainPathPublic.Identifier(): {}, + }, }, { name: "storageFormatV2Enabled = true, domain storage map exists, createIfNotExists = false", @@ -8550,6 +8606,11 @@ func TestGetDomainStorageMapRegisterReadsForV1Account(t *testing.T) { // domain storage map is created and cached in the first // GetDomainStorageMap(). }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + AccountStorageKey: {}, + string(address[:]) + "|" + common.StorageDomainPathStorage.Identifier(): {}, + string(address[:]) + "|" + string([]byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}): {}, + }, }, { name: "storageFormatV2Enabled = true, domain storage map exists, createIfNotExists = true", @@ -8585,6 +8646,11 @@ func TestGetDomainStorageMapRegisterReadsForV1Account(t *testing.T) { // domain storage map is created and cached in the first // GetDomainStorageMap(). }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + AccountStorageKey: {}, + string(address[:]) + "|" + common.StorageDomainPathStorage.Identifier(): {}, + string(address[:]) + "|" + string([]byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}): {}, + }, }, } @@ -8594,6 +8660,7 @@ func TestGetDomainStorageMapRegisterReadsForV1Account(t *testing.T) { storedValues, storedIndices := tc.getStorageData() var ledgerReads []ownerKeyPair + ledgerReadsSet := make(map[string]struct{}) ledger := NewTestLedgerWithData( func(owner, key, _ []byte) { @@ -8604,6 +8671,7 @@ func TestGetDomainStorageMapRegisterReadsForV1Account(t *testing.T) { key: key, }, ) + ledgerReadsSet[string(owner)+"|"+string(key)] = struct{}{} }, nil, storedValues, @@ -8629,6 +8697,12 @@ func TestGetDomainStorageMapRegisterReadsForV1Account(t *testing.T) { domainStorageMap = storage.GetDomainStorageMap(inter, address, tc.domain, tc.createIfNotExists) require.Equal(t, tc.expectedDomainStorageMapIsNil, domainStorageMap == nil) require.Equal(t, tc.expectedReadsFor2ndGetDomainStorageMapCall, ledgerReads) + + // Check underlying ledger reads + require.Equal(t, len(ledgerReadsSet), len(tc.expectedReadsSet)) + for k := range ledgerReadsSet { + require.Contains(t, tc.expectedReadsSet, k) + } }) } } @@ -8714,6 +8788,7 @@ func TestGetDomainStorageMapRegisterReadsForV2Account(t *testing.T) { expectedDomainStorageMapIsNil bool expectedReadsFor1stGetDomainStorageMapCall []ownerKeyPair expectedReadsFor2ndGetDomainStorageMapCall []ownerKeyPair + expectedReadsSet map[string]struct{} }{ { name: "domain storage map does not exist, createIfNotExists = false", @@ -8743,6 +8818,10 @@ func TestGetDomainStorageMapRegisterReadsForV2Account(t *testing.T) { // account storage map is loaded and cached from first // GetDomainStorageMap(). }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + AccountStorageKey: {}, + string(address[:]) + "|" + string([]byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}): {}, + }, }, { name: "domain storage map does not exist, createIfNotExists = true", @@ -8772,6 +8851,10 @@ func TestGetDomainStorageMapRegisterReadsForV2Account(t *testing.T) { // domain storage map is created and cached in the first // GetDomainStorageMap(). }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + AccountStorageKey: {}, + string(address[:]) + "|" + string([]byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}): {}, + }, }, { name: "domain storage map exists, createIfNotExists = false", @@ -8801,6 +8884,10 @@ func TestGetDomainStorageMapRegisterReadsForV2Account(t *testing.T) { // domain storage map is created and cached in the first // GetDomainStorageMap(). }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + AccountStorageKey: {}, + string(address[:]) + "|" + string([]byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}): {}, + }, }, { name: "domain storage map exists, createIfNotExists = true", @@ -8830,6 +8917,10 @@ func TestGetDomainStorageMapRegisterReadsForV2Account(t *testing.T) { // domain storage map is created and cached in the first // GetDomainStorageMap(). }, + expectedReadsSet: map[string]struct{}{ + string(address[:]) + "|" + AccountStorageKey: {}, + string(address[:]) + "|" + string([]byte{'$', 0, 0, 0, 0, 0, 0, 0, 1}): {}, + }, }, } @@ -8839,6 +8930,7 @@ func TestGetDomainStorageMapRegisterReadsForV2Account(t *testing.T) { storedValues, storedIndices := tc.getStorageData() var ledgerReads []ownerKeyPair + ledgerReadsSet := make(map[string]struct{}) ledger := NewTestLedgerWithData( func(owner, key, _ []byte) { @@ -8849,6 +8941,7 @@ func TestGetDomainStorageMapRegisterReadsForV2Account(t *testing.T) { key: key, }, ) + ledgerReadsSet[string(owner)+"|"+string(key)] = struct{}{} }, nil, storedValues, @@ -8874,6 +8967,12 @@ func TestGetDomainStorageMapRegisterReadsForV2Account(t *testing.T) { domainStorageMap = storage.GetDomainStorageMap(inter, address, tc.domain, tc.createIfNotExists) require.Equal(t, tc.expectedDomainStorageMapIsNil, domainStorageMap == nil) require.Equal(t, tc.expectedReadsFor2ndGetDomainStorageMapCall, ledgerReads) + + // Check underlying ledger reads + require.Equal(t, len(ledgerReadsSet), len(tc.expectedReadsSet)) + for k := range ledgerReadsSet { + require.Contains(t, tc.expectedReadsSet, k) + } }) } } From c7cf0127ec09a18d10715ac456248855a4f4a82a Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Mon, 25 Nov 2024 13:11:36 -0600 Subject: [PATCH 4/8] Refactor getAccountStorageFormat() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Müller --- runtime/storage.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/runtime/storage.go b/runtime/storage.go index aab2f1dab..9bef9d5e7 100644 --- a/runtime/storage.go +++ b/runtime/storage.go @@ -301,12 +301,16 @@ func (s *Storage) getAccountStorageFormat( return storageFormatNew } -func (s *Storage) getCachedAccountFormat(address common.Address) (isV1 bool, isV2 bool) { +func (s *Storage) getCachedAccountFormat(address common.Address) (format storageFormat, known bool) { isV1, cached := s.cachedV1Accounts[address] if !cached { - return false, false + return storageFormatUnknown, false + } + if isV1 { + return storageFormatV1, true + } else { + return StorageFormatV2, true } - return isV1, !isV1 } // isV2Account returns true if given account is in account storage format v2. From 426a5489e36305d86763e55641144cf7611027f6 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Mon, 25 Nov 2024 13:14:48 -0600 Subject: [PATCH 5/8] Use refactored getCachedAccountFormat() --- runtime/storage.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/runtime/storage.go b/runtime/storage.go index 9bef9d5e7..6f811f2e6 100644 --- a/runtime/storage.go +++ b/runtime/storage.go @@ -264,14 +264,9 @@ func (s *Storage) getAccountStorageFormat( // Return cached account format (no register reading). - isCachedV1, isCachedV2 := s.getCachedAccountFormat(address) - - if isCachedV1 { - return storageFormatV1 - } - - if isCachedV2 { - return StorageFormatV2 + cachedFormat, known := s.getCachedAccountFormat(address) + if known { + return cachedFormat } // Check if account is v2 (by reading "stored" register). From 42d6b261f0e0048c66059a1f5a28b4ec95b51183 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Mon, 25 Nov 2024 15:44:50 -0600 Subject: [PATCH 6/8] Rename storageFormat constant --- runtime/storage.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/runtime/storage.go b/runtime/storage.go index 6f811f2e6..0b8c8ec49 100644 --- a/runtime/storage.go +++ b/runtime/storage.go @@ -46,7 +46,7 @@ const ( storageFormatUnknown storageFormat = iota storageFormatNew storageFormatV1 - StorageFormatV2 + storageFormatV2 ) type Storage struct { @@ -211,7 +211,7 @@ func (s *Storage) GetDomainStorageMap( s.cacheIsV1Account(address, true) - case StorageFormatV2, storageFormatNew: + case storageFormatV2, storageFormatNew: domainStorageMap = s.AccountStorageV2.GetDomainStorageMap( inter, address, @@ -272,7 +272,7 @@ func (s *Storage) getAccountStorageFormat( // Check if account is v2 (by reading "stored" register). if s.isV2Account(address) { - return StorageFormatV2 + return storageFormatV2 } // Check if account is v1 (by reading given domain register). @@ -304,7 +304,7 @@ func (s *Storage) getCachedAccountFormat(address common.Address) (format storage if isV1 { return storageFormatV1, true } else { - return StorageFormatV2, true + return storageFormatV2, true } } From 1568414631ed1926dce972de443e981f70cd809d Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Mon, 25 Nov 2024 16:23:06 -0600 Subject: [PATCH 7/8] Refactor Storage.GetDomainStorageMap() --- runtime/storage.go | 194 ++++++++++++++++++++++++--------------------- 1 file changed, 105 insertions(+), 89 deletions(-) diff --git a/runtime/storage.go b/runtime/storage.go index 0b8c8ec49..9f6e30132 100644 --- a/runtime/storage.go +++ b/runtime/storage.go @@ -44,7 +44,6 @@ type storageFormat uint8 const ( storageFormatUnknown storageFormat = iota - storageFormatNew storageFormatV1 storageFormatV2 ) @@ -173,127 +172,144 @@ func (s *Storage) GetDomainStorageMap( // When StorageFormatV2 is disabled, handle all accounts as v1 accounts. - domainStorageMap = s.AccountStorageV1.GetDomainStorageMap( + // Only read requested domain register. + + domainStorageMap = s.getDomainStorageMapForV1Account( address, domain, createIfNotExists, ) - if domainStorageMap != nil { - s.cacheIsV1Account(address, true) - } + return + } - } else { + // StorageFormatV2 is enabled. - // StorageFormatV2 is enabled. + // Check if cached account format is available. - onlyCheckSpecifiedDomainForV1 := !createIfNotExists - format := s.getAccountStorageFormat(address, domain, onlyCheckSpecifiedDomainForV1) + cachedFormat, known := s.getCachedAccountFormat(address) + if known { + return s.getDomainStorageMap( + cachedFormat, + inter, + address, + domain, + createIfNotExists, + ) + } - switch format { + // Check if account is v2 (by reading "stored" register). - case storageFormatUnknown: - // storageFormatUnknown is returned when !createIfNotExists - // and domain register doesn't exist. + if s.isV2Account(address) { + return s.getDomainStorageMapForV2Account( + inter, + address, + domain, + createIfNotExists, + ) + } - if createIfNotExists { - panic(errors.NewUnreachableError()) - } + // Check if account is v1 (by reading requested domain register). - domainStorageMap = nil + if s.hasDomainRegister(address, domain) { + return s.getDomainStorageMapForV1Account( + address, + domain, + createIfNotExists, + ) + } - case storageFormatV1: - domainStorageMap = s.AccountStorageV1.GetDomainStorageMap( - address, - domain, - createIfNotExists, - ) + // Domain register doesn't exist. - s.cacheIsV1Account(address, true) + // Return early if !createIfNotExists to avoid more register reading. - case storageFormatV2, storageFormatNew: - domainStorageMap = s.AccountStorageV2.GetDomainStorageMap( - inter, - address, - domain, - createIfNotExists, - ) + if !createIfNotExists { + return nil + } - s.cacheIsV1Account(address, false) + // At this point, account is either new account or v1 account without requested domain register. - default: - panic(errors.NewUnreachableError()) - } + // Check if account is v1 (by reading more domain registers) + + if s.isV1Account(address) { + return s.getDomainStorageMapForV1Account( + address, + domain, + createIfNotExists, + ) } - return domainStorageMap + // New account is treated as v2 account when feature flag is enabled. + + return s.getDomainStorageMapForV2Account( + inter, + address, + domain, + createIfNotExists, + ) } -// getAccountStorageFormat returns storageFormat for the given account. -// This function determines account format by reading registers. -// If onlyCheckSpecifiedDomainForV1 is true, only the given domain -// register is checked to determine if account format is v1. If -// domain register doesn't exist, StorageFormatUnknown is returned. -// -// When StorageFormatV2 is disabled: -// - No register reading (accounts are assumed to be in v1 format). -// -// When StorageFormatV2 is enabled: -// - For v2 accounts, "stored" register is read. -// -// - For v1 accounts, -// - If onlyCheckSpecifiedDomainForV1 is true, -// "stored" register and given domain register are read. -// - If onlyCheckSpecifiedDomainForV1 is false and given domain exists, -// "stored" register and given domain register are read. -// - If onlyCheckSpecifiedDomainForV1 is false and given domain doesn't exist, -// "stored" register, given domain register, and all domain registers are read. -// -// - For new accounts, "stored" register, given domain register, and all domain registers are read. -func (s *Storage) getAccountStorageFormat( +func (s *Storage) getDomainStorageMapForV1Account( address common.Address, domain common.StorageDomain, - onlyCheckSpecifiedDomainForV1 bool, -) (format storageFormat) { - - // All accounts are assumed to be in v1 format when StorageFormatV2 is disabled. - - if !s.Config.StorageFormatV2Enabled { - return storageFormatV1 - } - - // Return cached account format (no register reading). + createIfNotExists bool, +) *interpreter.DomainStorageMap { + domainStorageMap := s.AccountStorageV1.GetDomainStorageMap( + address, + domain, + createIfNotExists, + ) - cachedFormat, known := s.getCachedAccountFormat(address) - if known { - return cachedFormat - } + s.cacheIsV1Account(address, true) - // Check if account is v2 (by reading "stored" register). + return domainStorageMap +} - if s.isV2Account(address) { - return storageFormatV2 - } +func (s *Storage) getDomainStorageMapForV2Account( + inter *interpreter.Interpreter, + address common.Address, + domain common.StorageDomain, + createIfNotExists bool, +) *interpreter.DomainStorageMap { + domainStorageMap := s.AccountStorageV2.GetDomainStorageMap( + inter, + address, + domain, + createIfNotExists, + ) - // Check if account is v1 (by reading given domain register). + s.cacheIsV1Account(address, false) - if s.hasDomainRegister(address, domain) { - return storageFormatV1 - } + return domainStorageMap +} - // Return early if onlyCheckSpecifiedDomainForV1 to prevent more register reading. +func (s *Storage) getDomainStorageMap( + format storageFormat, + inter *interpreter.Interpreter, + address common.Address, + domain common.StorageDomain, + createIfNotExists bool, +) *interpreter.DomainStorageMap { + switch format { - if onlyCheckSpecifiedDomainForV1 { - return storageFormatUnknown - } + case storageFormatV1: + return s.getDomainStorageMapForV1Account( + address, + domain, + createIfNotExists, + ) - // At this point, account is either new account or v1 account without given domain register. + case storageFormatV2: + return s.getDomainStorageMapForV2Account( + inter, + address, + domain, + createIfNotExists, + ) - if s.isV1Account(address) { - return storageFormatV1 + default: + panic(errors.NewUnreachableError()) } - - return storageFormatNew } func (s *Storage) getCachedAccountFormat(address common.Address) (format storageFormat, known bool) { From b51053d112dea40d7e176793fa84534a404cf1ba Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Mon, 25 Nov 2024 17:09:52 -0600 Subject: [PATCH 8/8] Refactor Storage.isV1Account() --- runtime/storage.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/runtime/storage.go b/runtime/storage.go index 9f6e30132..d6eb222f0 100644 --- a/runtime/storage.go +++ b/runtime/storage.go @@ -356,14 +356,7 @@ func (s *Storage) isV1Account(address common.Address) (isV1 bool) { // Check if a storage map register exists for any of the domains. // Check the most frequently used domains first, such as storage, public, private. for _, domain := range common.AllStorageDomains { - _, domainExists, err := readSlabIndexFromRegister( - s.Ledger, - address, - []byte(domain.Identifier()), - ) - if err != nil { - panic(err) - } + domainExists := s.hasDomainRegister(address, domain) if domainExists { return true }