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 command to diff states #5970

Merged
merged 16 commits into from
Jun 3, 2024
Merged

Add command to diff states #5970

merged 16 commits into from
Jun 3, 2024

Conversation

turbolent
Copy link
Member

@turbolent turbolent commented May 22, 2024

Add a new command, diff-states, to the util, which allows diffing two states. The states may be either given in the form of a snapshot directory / state commitment, or a payloads file. The command generates a report of differences in the states on either a low "raw" level, or on a high Cadence-level.

This essentially exposes the existing state loading and Cadence value diffing functionality of the migration.
While at it, optimize how the traces (paths in the object structure) are generated.

@turbolent turbolent self-assigned this May 22, 2024
@turbolent turbolent requested review from a team May 22, 2024 20:29
@codecov-commenter
Copy link

codecov-commenter commented May 22, 2024

Codecov Report

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

Project coverage is 55.80%. Comparing base (7ebb82b) to head (095669b).

Files Patch % Lines
cmd/util/cmd/diff-states/cmd.go 58.99% 114 Missing and 16 partials ⚠️
cmd/util/ledger/migrations/cadence_value_diff.go 37.93% 101 Missing and 7 partials ⚠️
...util/ledger/migrations/cadence_values_migration.go 0.00% 10 Missing ⚠️
cmd/util/cmd/extract-payloads-by-address/cmd.go 0.00% 3 Missing ⚠️
cmd/util/ledger/util/registers/registers.go 0.00% 3 Missing ⚠️
ledger/complete/ledger.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5970      +/-   ##
==========================================
- Coverage   55.83%   55.80%   -0.04%     
==========================================
  Files        1129     1131       +2     
  Lines       89297    89758     +461     
==========================================
+ Hits        49858    50085     +227     
- Misses      34686    34895     +209     
- Partials     4753     4778      +25     
Flag Coverage Δ
unittests 55.80% <51.60%> (-0.04%) ⬇️

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
Copy link
Member Author

turbolent commented May 22, 2024

@fxamacker Any idea why I'd get errors like failed to preload old atree registers: decoding error: data has invalid flag 0x80, want 0x8 or 0xb when running the Cadence value diffing?

Getting this when running the command on cadence-migration-test (flow-benchmark, us-west1-b), with mount -t ext4 -o discard,defaults /dev/disk/by-id/google-cadence-migration-test-temp /var/temp:

$ util diff-states \
    --state-1 /var/temp/migrationtestnet1-may8-inlined-preview20 \
    --state-2 /var/temp/migrationtestnet1-may8-inlined-preview23 \
    --state-commitment-1 5f0ebd52ea07b8a39b5f76d1e981ec92702663dc4ec82a2f7beafba6263d5a8c \
    --state-commitment-2 fd8f5c9d6ef2b13acd0c806284901fad2f510e812d8b8c391913b03143c2cd56 \
    --output-directory /var/temp \
    --raw=false \
    --chain flow-testnet

It's only reported for some of the accounts:

$ cat state-diff_1716412624.json | jq -c '.[] | select(.Kind == "error_diff_failed")' | wc -l
25146

@fxamacker
Copy link
Member

@fxamacker Any idea why I'd get errors like failed to preload old atree registers: decoding error: data has invalid flag 0x80, want 0x8 or 0xb when running the Cadence value diffing?

@turbolent The error message "data has invalid flag" is only present in feature/stable-cadence branch (no atree inlining). Is the input data to the program atree inlined or not inlined?

@turbolent
Copy link
Member Author

@fxamacker Oh, right, good catch! I need to build the binary on top of the atree inlining branch 👍

Copy link
Member

@fxamacker fxamacker left a comment

Choose a reason for hiding this comment

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

Nice! I focused on reviewing non-test code and left some comments.

cmd/util/cmd/diff-states/cmd.go Show resolved Hide resolved
cmd/util/cmd/diff-states/cmd.go Outdated Show resolved Hide resolved
cmd/util/ledger/migrations/cadence_values_migration.go Outdated Show resolved Hide resolved
cmd/util/ledger/migrations/cadence_values_migration.go Outdated Show resolved Hide resolved
@turbolent turbolent requested review from fxamacker and a team May 24, 2024 19:43
Copy link
Member

@fxamacker fxamacker left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for updating PR! I left a few more comments for your consideration.

cmd/util/cmd/diff-states/cmd.go Show resolved Hide resolved
cmd/util/cmd/diff-states/cmd.go Outdated Show resolved Hide resolved
@@ -209,10 +209,22 @@ func (m *CadenceBaseMigration) MigrateAccount(
m.nWorkers,
)

owner := string(address[:])
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use AddressToRegisterOwner to be consistent for global registers.

Suggested change
owner := string(address[:])
owner := AddressToRegisterOwner(address)

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it in 9a2541a, but is it even required? We call this in MigrateAccount, which is only called for accounts, and not for global registers?

@turbolent turbolent requested a review from a team May 28, 2024 23:49
@turbolent
Copy link
Member Author

@onflow/cadence PTAL

@turbolent turbolent requested a review from fxamacker May 29, 2024 01:15
@turbolent
Copy link
Member Author

To address the last part of #3366, an atree failure to iterate over a non-migrated key, I added some granular error recovery code to the Cadence value diffing in 095669b. There isn't much we can do about the problem, other than handling it gracefully when we encounter it and continue

Copy link
Member

@AlexHentschel AlexHentschel left a comment

Choose a reason for hiding this comment

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

Took a brief pass over the PR. There aren't really any algorithmic changes on the trie, so no concerns from my end. Though, don't have a lot of context on the other details.

@turbolent turbolent added this pull request to the merge queue Jun 3, 2024
Merged via the queue into master with commit 21ce1ae Jun 3, 2024
55 checks passed
@turbolent turbolent deleted the bastian/diff-states branch June 3, 2024 23:49
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

5 participants