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

Applied snapshot balances #1280

Merged
merged 24 commits into from
Jan 10, 2024
Merged

Applied snapshot balances #1280

merged 24 commits into from
Jan 10, 2024

Conversation

esuwu
Copy link
Contributor

@esuwu esuwu commented Dec 15, 2023

No description provided.

@nickeskov nickeskov force-pushed the snapshots-apply-balances branch from 5f83ee5 to f4bde2e Compare December 17, 2023 21:42
@esuwu
Copy link
Contributor Author

esuwu commented Dec 24, 2023

2658378 mainnet ok

@esuwu esuwu added awaiting-release Ready to be a part of a new release and removed awaiting-release Ready to be a part of a new release labels Dec 27, 2023
@nickeskov nickeskov self-requested a review December 27, 2023 23:10
@nickeskov nickeskov force-pushed the snapshots-apply-balances branch 3 times, most recently from 984ca1e to 6ab40a9 Compare December 28, 2023 02:49
pkg/state/appender.go Outdated Show resolved Hide resolved
pkg/state/appender.go Outdated Show resolved Hide resolved
pkg/state/snapshot_generator.go Outdated Show resolved Hide resolved
pkg/state/snapshot_generator.go Outdated Show resolved Hide resolved
return nil, nil, errors.Errorf("more than one balance diff for the same address in the same block")
}
if len(balanceChange.key) > wavesBalanceKeySize {
err := sg.addAssetBalanceDiffFromTxDiff(balanceChange.balanceDiffs[0], balanceChange.key,
Copy link
Member

Choose a reason for hiding this comment

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

Why are you taking first diff, but not latest?

Copy link
Contributor Author

@esuwu esuwu Dec 28, 2023

Choose a reason for hiding this comment

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

balanceDiffs struct contains proto.BlockID, it means that while inside a block, there's only one element in this array.
It used to be a diff for multiple blocks, but not anymore. The array should be replaced with a variable, but it would take a bit extra time to refactor code.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, for now we can leave it without changes.
I've just submitted a refactoring issue #1297

pkg/state/snapshot_generator.go Outdated Show resolved Hide resolved
@esuwu
Copy link
Contributor Author

esuwu commented Dec 30, 2023

f8b0ab8 mainnet ok

@nickeskov nickeskov deleted the branch master January 9, 2024 22:00
@nickeskov nickeskov closed this Jan 9, 2024
@nickeskov nickeskov reopened this Jan 9, 2024
@nickeskov nickeskov changed the base branch from save-tx-in-snapshot-applier to master January 9, 2024 22:20
@nickeskov nickeskov force-pushed the snapshots-apply-balances branch from 58af430 to f8b0ab8 Compare January 9, 2024 22:32
Comment on lines +1063 to +1064
// for compatibility with the legacy state hashes
sg.stor.balances.addAssetBalanceChangeLegacySH(address, asset, change.balance)
Copy link
Member

Choose a reason for hiding this comment

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

This trick won't work in the light node mode.

Comment on lines +1082 to +1083
// for compatibility with the legacy state hashes
sg.stor.balances.addWavesBalanceChangeLegacySH(address, change)
Copy link
Member

Choose a reason for hiding this comment

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

And this one too

Comment on lines +790 to +791
// clean up legacy state hash records with zero diffs
a.stor.balances.filterZeroDiffsSHOut(blockID)
Copy link
Member

Choose a reason for hiding this comment

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

It's important to do these actions in snapshotApplier because only this entity (mostly) can change the state in the light node mode.

nickeskov and others added 2 commits January 10, 2024 07:50
This reverts commit 12b2a6a.

According to the scala node implementation:
- Saved block snapshot must not contain initial block snapshot.
- Light node can calculate all initial data by its own.
@nickeskov
Copy link
Member

These issues will be fixed in a different PR.

@nickeskov nickeskov merged commit 7a4fbf8 into master Jan 10, 2024
14 checks passed
@nickeskov nickeskov deleted the snapshots-apply-balances branch January 10, 2024 08:47
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.

2 participants