-
Notifications
You must be signed in to change notification settings - Fork 130
feat(l1): avoid opening DB if schema version doesn't match #5597
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the schema version validation mechanism to use a backend-agnostic JSON metadata file instead of storing the version inside the database. This change allows checking the schema version before opening the database, preventing potential data loss from incompatible schema versions.
Key changes:
- Schema version is now stored in a
metadata.jsonfile in the store directory - Validation occurs before opening any database backend
- Error messages now include both found and expected schema versions for clarity
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/storage/store.rs | Added JSON-based metadata validation function and StoreMetadata struct; moved validation before store initialization |
| crates/storage/error.rs | Enhanced error type with structured fields showing both found and expected versions; added I/O and JSON serialization error variants |
| crates/storage/api.rs | Removed set_store_schema_version and get_store_schema_version methods from StoreEngine trait |
| crates/storage/store_db/rocksdb.rs | Removed RocksDB-specific schema version implementation and unused write_sync helper method |
| crates/storage/store_db/in_memory.rs | Removed no-op schema version methods that were irrelevant for in-memory storage |
| cmd/ethrex/initializers.rs | Updated error handling to use new structured IncompatibleDBVersion error format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Lines of code reportTotal lines added: Detailed view |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Err(err @ StoreError::IncompatibleDBVersion { .. }) | ||
| | Err(err @ StoreError::NotFoundDBVersion { .. }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We start at 1 so we can treat 0 and missing as equivalent, so I don't think the new error kind is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes the distinction clear and also simplifies the formatting of the error here.
| pub const STORE_SCHEMA_VERSION: u64 = 1; | ||
|
|
||
| /// Name of the file storing the metadata about the database | ||
| pub const STORE_METADATA_FILENAME: &str = "metadata.json"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the place for the schema versioning is the DB. Decoupling it is asking for trouble. E.g., I don't see the removedb command deleting this properly.
If we don't want it to be data, we may follow a scheme similar to what Postgres does, which is encoding versioning in file names (e.g., the internal directory might be data-{schema_version}, with failure indicating incompatibility).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removedb removes the whole datadir, including the metadata file, so there should be no problem there.
The main reason to move the schema versioning to another file is to make it backend-agnostic. Keeping it inside the DB means we need to open a Store before checking if we support its version. Right now, this means we may have some data loss due to unknown columns being dropped by RocksDB. This can be solved, but it would require the backend itself to check the version.
It also has the benefit that we can include more information, like the engine type we're using, and we can easily inspect it because we use plaintext. Having it in filenames is a good alternative, but I don't see the benefits in moving to that.
Motivation
We're currently opening the Store backend before checking the DB schema version matches. This can lead to data loss in case the RocksDB schema is changed.
Description
This PR changes the metadata to be stored in JSON format in a backend-agnostic way. This allows for inspecting the DB version without starting ethrex. Example:
It also allows for more easily extending the check with more validations, for example, for backend type.
Additionally, it also fails with a clear error when trying to run a new ethrex version on an old DB without this versioning scheme:
Closes #5541