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

Add Migration to Update Storage Used #1033

Merged
merged 14 commits into from
Aug 23, 2021
Merged

Conversation

janezpodhostnik
Copy link
Contributor

@janezpodhostnik janezpodhostnik commented Jul 27, 2021

ref: https://github.com/dapperlabs/flow-internal/issues/1539

Added a migration that updates accounts storageUsed to the actual value. A de-sync in accounts storageUsed and the actual storage it is using when a migration changes registers directly.

a (minor) de-sync did happen in the past. Running this migration on a mainnet state from a month ago shows that no accounts are using more storage than their storageUsed and that on average accounts are using 7.2% less storage than their storageUsed.

@janezpodhostnik janezpodhostnik self-assigned this Jul 27, 2021
@janezpodhostnik janezpodhostnik marked this pull request as ready for review July 28, 2021 15:47
@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2021

Codecov Report

Merging #1033 (8daaccb) into master (73d6864) will increase coverage by 0.05%.
The diff coverage is 67.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1033      +/-   ##
==========================================
+ Coverage   55.71%   55.76%   +0.05%     
==========================================
  Files         481      482       +1     
  Lines       29387    29501     +114     
==========================================
+ Hits        16373    16452      +79     
- Misses      10781    10803      +22     
- Partials     2233     2246      +13     
Flag Coverage Δ
unittests 55.76% <67.54%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
...execution-state-extract/execution_state_extract.go 76.47% <50.00%> (-1.09%) ⬇️
...ledger/migrations/storage_used_update_migration.go 67.85% <67.85%> (ø)
...sus/approvals/assignment_collector_statemachine.go 50.00% <0.00%> (-1.93%) ⬇️
...d/util/ledger/migrations/storage_fees_migration.go 12.50% <0.00%> (+12.50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73d6864...8daaccb. Read the comment docs.

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.

Nice, looks good!

In the future we should run this after each state migration. There are two upcoming state migrations: one state migration will add type information to all arrays and dictionaries, likely in the next spork; and the other state mogration will switch arrays, dictionaries, and composite values to the new optimized data structures (a few sporks from now)

if _, ok := storageUsed[payloadSize.Address]; !ok {
storageUsed[payloadSize.Address] = 0
}
storageUsed[payloadSize.Address] = storageUsed[payloadSize.Address] + payloadSize.StorageUsed
Copy link
Member

Choose a reason for hiding this comment

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

does this assumes that an address exist that has two storage_used registers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No.

the storageUsedChan gets 1 input for every single register. That input is the address of a register and the storage used by that register. That is then just summed up to get the storage used per address.

This is basically just a way to prevent concurrent access to the storageUsed dictionary.

@janezpodhostnik
Copy link
Contributor Author

bors merge

@janezpodhostnik
Copy link
Contributor Author

If no other data migrations will be made the next spork, this line should be removed before the spork https://github.com/onflow/flow-go/pull/1033/files#diff-944cfc2e92587c43f3648956f2e94ed3b755acabfd1e39cbc08c2a503cde6a9eR64
@janezpodhostnik

@bors
Copy link
Contributor

bors bot commented Aug 23, 2021

@bors bors bot merged commit 0e5b3f3 into master Aug 23, 2021
@bors bors bot deleted the janez/storage-used-update-migration branch August 23, 2021 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants