Skip to content

StorageDumper: make dump format compatible with StatesDumper #3908

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

AnnaShaleva
Copy link
Member

@AnnaShaleva AnnaShaleva commented Apr 20, 2025

Description

Problem:

#3281 removes StatesDumper plugin, however, the output format of state dump files generated by StorageDumper is incompatible with StatesDumper format. We have a set of existing tools that rely on output format of StatesDumper.

Note that it was said in #3271 (comment) that:

we have some tooling that uses StorageDumper output.

But it's not true, because all our tools use the output of StatesDumper actually.

Solution:

Keep the StatesDumper output format compatible with StorageDumper.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Node synchronisation and resulting dump formats comparison.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@AnnaShaleva AnnaShaleva added the Bug Used to tag confirmed bugs label Apr 20, 2025
@AnnaShaleva AnnaShaleva added this to the v3.8.0 milestone Apr 20, 2025
Problem:

#3281 removes StatesDumper
plugin, however, the output format of state dump files generated by StorageDumper
is incompatible with StatesDumper format. We have a set of existing tools that
rely on output format of StatesDumper.

Solution:

Get back to the old format of output dump files.

Signed-off-by: Anna Shaleva <[email protected]>
@vncoelho
Copy link
Member

We can adapt our tools, but check with NGD as well.

@NGDAdmin @superboyiii

cschuchardt88
cschuchardt88 previously approved these changes Apr 20, 2025
}

private void InitFileWriter(uint network, IReadOnlyStore snapshot)
void OnCommitStorage(uint network, NeoSystem system)
Copy link
Contributor

@Wi1l-B0t Wi1l-B0t Apr 21, 2025

Choose a reason for hiding this comment

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

Why change IReadOnlyStore to NeoSystem?
There is only one system.GetSnapshotCache()

Copy link
Member

Choose a reason for hiding this comment

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

The event receive NeoSystem

@superboyiii
Copy link
Member

I strongly dislike this PR. I rarely say sth like this, but This PR is a real BUG. StorageDumper is StorageDumper, it's not StatesDumper. We remove StatesDumper because it has bug and we have a better choice->StorageDumper. Now we are going back... StorageDumper has clear format, each block data in a row, easy to read and compare. And it will not lose data when CLI is interrupted. But now see what we're doing... The bug is back again because some tool is not compatible... I don't understand why A PLUGIN has to be compatible with tools? Shouldn't tools adapt for plugin?
@AnnaShaleva You can try to install this Plugin and resync data. Randomly interrupt CLI for some times and export these dump data. You will see lots of data are lost.

@superboyiii
Copy link
Member

1745308924659

@roman-khimov
Copy link
Contributor

I don't understand why A PLUGIN has to be compatible with tools? Shouldn't tools adapt for plugin?

It's an API/format that is publicly available, so it's not that easy. Take RPC for example, it's a plugin, but this doesn't mean you can change its behavior easily, a lot of other code depends on it. This one of course is not that popular, but still.

Now if there is a real bug with the format (but I'd check if it's a format problem rather than implementation) we can consider switching to StatesDumper one, but IIRC it's not perfect either. This one is not release critical, we can solve it after 3.8 in any direction.

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

@AnnaShaleva ,create another plugin for now. We need to release soon, leave this plugin as it is. Later we decide how to merge.

@superboyiii
Copy link
Member

I don't understand why A PLUGIN has to be compatible with tools? Shouldn't tools adapt for plugin?

It's an API/format that is publicly available, so it's not that easy. Take RPC for example, it's a plugin, but this doesn't mean you can change its behavior easily, a lot of other code depends on it. This one of course is not that popular, but still.

Now if there is a real bug with the format (but I'd check if it's a format problem rather than implementation) we can consider switching to StatesDumper one, but IIRC it's not perfect either. This one is not release critical, we can solve it after 3.8 in any direction.

At least we shouldn't bring bug back. Or I think you need is just making StatesDumper back. Then just revert it but don't change StorageDumper.

@AnnaShaleva AnnaShaleva removed this from the v3.8.0 milestone Apr 22, 2025
@shargon shargon self-requested a review April 22, 2025 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Used to tag confirmed bugs Need Update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants