Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix storage health check in Cadence 1.0 migration #5622

Merged

Conversation

fxamacker
Copy link
Member

@fxamacker fxamacker commented Apr 3, 2024

Currently, storage health check always fails with "slabs not referenced from account storage" in pre-migration because storage maps are not loaded in Cadence runtime storage even though payloads are loaded.

This PR fixes this problem by loading storage map explicitly after loading payloads in storage.

Updates onflow/cadence#3192

cc: @turbolent

Currently, storage health check always fails with
"slabs not referenced from account storage" because storage
maps are not loaded in Cadence runtime storage even though
payloads are loaded.

This commit fixes this problem by loading storage map explicitly
after loading payloads in storage.
@fxamacker fxamacker requested review from turbolent and a team April 3, 2024 22:31
@fxamacker fxamacker self-assigned this Apr 3, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 57.33%. Comparing base (c258474) to head (be8b26d).
Report is 3 commits behind head on feature/stable-cadence.

Files Patch % Lines
...util/ledger/migrations/cadence_values_migration.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                     @@
##           feature/stable-cadence    #5622      +/-   ##
==========================================================
+ Coverage                   56.95%   57.33%   +0.37%     
==========================================================
  Files                         390      817     +427     
  Lines                       43837    83101   +39264     
==========================================================
+ Hits                        24968    47644   +22676     
- Misses                      16782    31709   +14927     
- Partials                     2087     3748    +1661     
Flag Coverage Δ
unittests 57.33% <0.00%> (+0.37%) ⬆️

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.

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Do you have a concrete error from the logs where this occurs?

Why would we need this in addition to already loading all payloads before?

@fxamacker
Copy link
Member Author

fxamacker commented Apr 3, 2024

@turbolent

Do you have a concrete error from the logs where this occurs?

Yes, this storage check error happens before migration starts when I run locally modified atree migration.

For context, in my local copy, I am adding storage health check in atree migration and testing it with testnet data. Most of the storage health check code are the same with the storage health check in Cadence 1.0 migration.

4:00PM WRN storage health check before migration failed error="internal error: slabs not referenced from account storage: [0x75318eaf00edbc9d.11 0x75318eaf00edbc9d.12 0x75318eaf00edbc9d.13 0x75318eaf00edbc9d.1122]
goroutine 352 [running]:
runtime/debug.Stack()
  /usr/lib/golang/src/runtime/debug/stack.go:24 +0x5e
github.com/onflow/cadence/runtime/errors.NewUnexpectedError({0x1a25f70?, 0x4?}, {0xc0003b9890?, 0x7f5346fd83c8?, 0x50?})
  /home/fa/go/src/github.com/onflow/cadence-master/cadence/runtime/errors/errors.go:145 +0x45\ngithub.com/onflow/cadence/runtime.(*Storage).CheckHealth(0xc0003658c0)
  /home/fa/go/src/github.com/onflow/cadence-master/cadence/runtime/storage.go:349 +0x6f0\ngithub.com/onflow/flow-go/cmd/util/ledger/migrations.(*AtreeRegisterMigrator).MigrateAccount(0xc00072b0e0, {0x1dbdb90?, 0xc00045d6d0?}, {0x75, 0x31, 0x8e, 0xaf, 0x0, 0xed, 0xbc, ...}, ...)
  /home/fa/go/src/github.com/onflow/flow-go-atree-migration/flow-go/cmd/util/ledger/migrations/atree_register_migration.go:127 +0xcc7\ngithub.com/onflow/flow-go/cmd/util/ledger/migrations.MigrateGroupConcurrently.func1()
  /home/fa/go/src/github.com/onflow/flow-go-atree-migration/flow-go/cmd/util/ledger/migrations/account_based_migration.go:195 +0x4d2\ncreated by github.com/onflow/flow-go/cmd/util/ledger/migrations.MigrateGroupConcurrently in goroutine 1
  /home/fa/go/src/github.com/onflow/flow-go-atree-migration/flow-go/cmd/util/ledger/migrations/account_based_migration.go:143 +0x1ff" 
account=75318eaf00edbc9d migration=atree-register-migration migration_index=0

Why would we need this in addition to already loading all payloads before?

In Cadence runtime Storage.CheckHealth(), we compare root slab IDs from atree storage in atree.CheckStorageHealth() with root slab IDs of loaded storage maps from s.storageMaps. So if storage map isn't loaded, all root slabs from atree storage are reported in error log as "not referenced".

https://github.com/onflow/cadence/blob/master/runtime/storage.go#L330-L351

@turbolent
Copy link
Member

@fxamacker Ah, thank you for the explanation, I see now!

@turbolent turbolent merged commit 94a745a into feature/stable-cadence Apr 4, 2024
54 of 55 checks passed
@turbolent turbolent deleted the fxamacker/fix-migration-storage-check branch April 4, 2024 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants