[Account Storage Maps] Refactor storage into V1 and V2

[turbolent]

Description

• Refactor the existing mixed V1/V2 code into separate files. This simplifies the logic, and more easily allows it to be feature-flagged

  • Simplify the bookkeeping: only keep a simple maps, ordered maps are unnecessary:
    We can simply sort the new indices at commit time.

• Put the V2 format behind a feature flag, and ensure that the behaviour with the flag disabled is exactly the same as it is currently. This allows us to perform a rolling/non-HCU deployment

• Expose functions in storage to schedule accounts to be migrated to V2.
  We'll discus which accounts and when to migrate in the upcoming sync with the Execution team.

---

• Targeted PR against master branch
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work
• Code follows the standards mentioned here
• Updated relevant documentation
• Re-reviewed Files changed in the Github PR explorer
• Added appropriate labels

[github-actions]

Cadence Benchstat comparison

This branch with compared with the base branch onflow:feature/combine-domain-payloads-and-domain-storage-maps commit 0abfa9a
The command for i in {1..N}; do go test ./... -run=XXX -bench=. -benchmem -shuffle=on; done was used.
Bench tests were run a total of 7 times on each branch.

Collapsed results for better readability

[fxamacker]

Nice! Since this is DRAFT, I only left one comment about how V1 and V2 are used.

By changing how V1 and V2 are used, we can have consistent account format for existing accounts regardless of StorageFormatV2Enabled settings.

[SupunS]

I mostly reviewed for general Go code. LGTM!

TestRuntimeStorageForUnmigratedAccount seems to be failing though.

[fxamacker]

Nice! Thanks for incorporating feedback and also for adding tests for feature flag.

I left a comment about extra register reads we can avoid for new account when createIfNotExists is false and V2 is enabled.

[turbolent]

@SupunS As mentioned in the description, that and the other test related to the migration are not yet adjusted, but in the follow-up PR, #3680.

@fxamacker Some of your suggestions actually already exist in #3680 – should I maybe just merge #3680 into this PR, and then we can address everything here? (I initially tried to keep the PRs small)

[fxamacker]

@fxamacker Some of your suggestions actually already exist in #3680 – should I maybe just merge #3680 into this PR, and then we can address everything here? (I initially tried to keep the PRs small)

@turbolent Yes. I started reviewing PR 3680. I can review the differences here after you merge.

[turbolent]

@fxamacker @SupunS Merged the migration-related follow-up PR #3680 into this, so everything is now in one PR. Can you please take another look? Sorry for the inconvenience

[fxamacker]

Nice! 👍 I just reviewed updates and new commits from PR #3680.

To confirm my understanding of the current implementation of feature flag & migration:

• With feature flag disabled, all accounts are assumed as in V1 format and continue to be in V1 format. Register reads & writes are consistent with deployed code.

• With feature flag enabled,

  • V1 accounts (with write ops) continue to be stored in V1 unless storage.ScheduleV2MigrationForModifiedAccounts() is called before storage.Commit().

  • New accounts are stored in V2 format automatically without storage.ScheduleV2MigrationForModifiedAccounts().

[turbolent]

@fxamacker exactly, great summary 👍

[fxamacker]

Nice! I left a minor suggestion.

BTW, what are your thoughts about reducing register reads for new accounts when createIfNotExists is false:

• [Account Storage Maps] Refactor storage into V1 and V2 #3678 (comment)

I think we can avoid reading all domain registers for the scenario that doesn't need that.

[turbolent]

@fxamacker Sorry, I had missed the comment reply due to the follow-up commits, I'll reply there

[codecov]

Codecov Report

Attention: Patch coverage is 88.30313% with 71 lines in your changes missing coverage. Please review.

Please upload report for BASE (feature/combine-domain-payloads-and-domain-storage-maps@0abfa9a). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
runtime/storage.go	85.25%	18 Missing and 5 partials ⚠️
runtime/account_storage_v2.go	91.39%	10 Missing and 6 partials ⚠️
runtime/slabindex.go	60.52%	12 Missing and 3 partials ⚠️
runtime/account_storage_v1.go	89.43%	8 Missing and 5 partials ⚠️
interpreter/storage.go	88.88%	2 Missing ⚠️
runtime/migrate_domain_registers.go	92.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@                                    Coverage Diff                                     @@
##             feature/combine-domain-payloads-and-domain-storage-maps    #3678   +/-   ##
==========================================================================================
  Coverage                                                           ?   80.20%           
==========================================================================================
  Files                                                              ?      419           
  Lines                                                              ?    96907           
  Branches                                                           ?        0           
==========================================================================================
  Hits                                                               ?    77728           
  Misses                                                             ?    16441           
  Partials                                                           ?     2738           

Flag	Coverage Δ
unittests	80.20% <88.30%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles