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

Feature/DB repair on corruption #4911

Merged
merged 5 commits into from
Nov 17, 2022
Merged

Conversation

asdacap
Copy link
Contributor

@asdacap asdacap commented Nov 15, 2022

  • Added logic similar to parity where it create a marker if db corruption is detected, then repair on restart.

Changes:

  • Added corrupted marker when corruption exception was encountered.
  • Call repair if marker is detected.

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Testing

Requires testing

  • Yes
  • No

In case you checked yes, did you write tests??

  • Yes
  • No

Comments about testing , should you have some (optional)

  • On mainnet db, does not seems to work. It would failed with force_consistency_checks error. Same error if using ldb tool directly. Tried on state and block db.
  • On chiado db, seems to work if I delete an sst file in blocks db.
  • So it does not always work.

Further comments (optional)

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@asdacap asdacap requested a review from LukaszRozmej November 15, 2022 14:03
@LukaszRozmej
Copy link
Member

Can we break some DB with restarts and apply this PR to recover? @kamilchodola

@asdacap
Copy link
Contributor Author

asdacap commented Nov 15, 2022

Tried killing the node while processing. Does not cause corruption for me.

@kamilchodola
Copy link
Contributor

@LukaszRozmej Would with biggest pleasure break some DBs for You :) I just need an Ayman PR on master or merged into this PR as well for this debug rpc endpoint.

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

Question: Should we potentially move this to the internal package? Especially at least not to touch rocksDbNative here. @rubo

@asdacap
Copy link
Contributor Author

asdacap commented Nov 15, 2022

Question: Should we potentially move this to the internal package? Especially at least not to touch rocksDbNative here. @rubo

Probably, it wont make much different though as its only one call.

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

I would still prefer hiding Native in package ( https://github.com/NethermindEth/rocksdb-sharp )

@kamilchodola
Copy link
Contributor

@asdacap @LukaszRozmej as for now no corruptions on any of el-cl-chain combination but will wait for a bit longer to see

@asdacap asdacap merged commit 664a345 into master Nov 17, 2022
@asdacap asdacap deleted the feature/db-repair-on-corruption branch November 17, 2022 06:19
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.

3 participants