Add a migration which detects and filters out unreferenced slabs

[turbolent]

Work towards #5634

[turbolent]

@onflow/cadence PTAL 🙏

[fxamacker]

Nice! I left a comment about needing to filter out child slabs and referenced slabs as well.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.65%. Comparing base (7b89544) to head (ff4932d).
Report is 1 commits behind head on feature/atree-inlining-cadence-v0.42.

Additional details and impacted files

@@                            Coverage Diff                            @@
##           feature/atree-inlining-cadence-v0.42    #5653       +/-   ##
=========================================================================
+ Coverage                                 53.39%   64.65%   +11.26%     
=========================================================================
  Files                                        16       91       +75     
  Lines                                      2270    12233     +9963     
=========================================================================
+ Hits                                       1212     7909     +6697     
- Misses                                      971     3624     +2653     
- Partials                                     87      700      +613     

Flag	Coverage Δ
unittests	64.65% <ø> (+11.26%)	⬆️

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.

[turbolent]

Forgot we also need to still integrate this new migration into the whole pipeline.

I guess it should be behind a flag, disabled by default?

[fxamacker]

Like I mentioned above in the thread, we can maybe also extend it to take care of fixing values with missing slabs in the known affected Testnet accounts. Alternatively we could have yet another separate migration, but that would also incur some overhead. What do you think is best?

@turbolent It would be cleaner to have separate migrations fixing different problems if overhead is small.

• fix old broken data: 10 registers with references to nonexistent registers (9 testnet accounts)
• fix old unreferenced slabs

We can make overhead small, e.g. by only fixing registers in the 9 testnet accounts.

Also, if other accounts have these problems, then it might be better to be alerted about those instead of silently fixing them.

In any case, I'd prefer to get this PR an, and then take care of the additional functionality in a separate PR.
Merging this PR in it's current form would allow us to port it to the feature branch which has the v0.42 atree register inlining migration in parallel.

I agree.

Forgot we also need to still integrate this new migration into the whole pipeline.
I guess it should be behind a flag, disabled by default?

Yes, that would be great.

[turbolent]

Integrated the new migration into the pipeline in commit 652410e.

@fxamacker To avoid the overhead of grouping payloads by account, maybe we can add the discussed migration, another account-based migration, into this sub-pipeline: https://github.com/onflow/flow-go/pull/5653/files#diff-efc0583bcfd2aa19725211a31ed4ceed33b7d0ab5228c4ab533acf0d7f474d63R422-R424

[fxamacker]

To avoid the overhead of grouping payloads by account, maybe we can add the discussed migration, another account-based migration, into this sub-pipeline: https://github.com/onflow/flow-go/pull/5653/files#diff-efc0583bcfd2aa19725211a31ed4ceed33b7d0ab5228c4ab533acf0d7f474d63R422-R424

@turbolent Yes, that was the plan. I will include the new migration in an existing account based migration when I open PR.

[turbolent]

@fxamacker Should I port this to feature/atree-inlining-cadence-v0.42 now?

[fxamacker]

@fxamacker Should I port this to feature/atree-inlining-cadence-v0.42 now?

@turbolent Not yet, I'm currently taking another look at this PR since it was updated.

BTW, I just opened PR #5755 for new migration (to fix refs to nonexistent slabs) targeting stable-cadence.
PTAL at #5755 🙏

[fxamacker]

Nice! 👍 LGTM!

It would be nice to write filtered slabs in payload file format (instead of JSON) to be consistent. However, current API to create payload file doesn't support incremental writes yet. We can revisit this aspect later if needed.

[fxamacker]

@fxamacker Should I port this to feature/atree-inlining-cadence-v0.42 now?

@turbolent Yes but If you don't have time, I can port this first thing in the morning (CDT).

BTW, thanks for fast review on PR #5755!

[turbolent]

@fxamacker Can we use the reporter infrastructure to create a payloads file? Or do we have to create one payloads file per account?

[fxamacker]

@fxamacker Can we use the reporter infrastructure to create a payloads file?

@turbolent No, the current reporter infrastructure can't create a payload file because it serializes received objects in JSON.

Or do we have to create one payloads file per account?

One payloads file per account might be overkill since we don't treat them differently.

One approach to create payload files for unreferenced slabs in this migration is to:

• aggregate unreferenced slabs in FilterUnreferencedSlabsMigration from all accounts
• call CreatePayloadFile() with all unreferenced slabs at FilterUnreferencedSlabsMigration.Close()

This approach uses existing code and it doesn't break any API.

Maybe report should only summarize by listing accounts having any unreferenced slabs (along with undreferenced slab count). Payload data can be in a payloads file instead of the report.

[fxamacker]

@turbolent I opened PR #5758 to port this PR to feature/atree-inlining-cadence-v0.42. PTAL 🙏

[turbolent]

@fxamacker How does 7e3d598 look?

[fxamacker]

Commit 7e3d598 looks good! Thanks for writing payloads to payload file.

I left some comments.

[turbolent]

@fxamacker Addressed the feedback in f5a46ee and 52175ea. If they look good, we'll want to port them to #5758

[fxamacker]

@turbolent Updates look good! I ported new commits to #5758.