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

fix(db): use the correct state version for databases without a state version file #7385

Merged
merged 2 commits into from
Aug 28, 2023

Conversation

teor2345
Copy link
Collaborator

Motivation

When Zebra opens a state cache that has never been upgraded, it was reporting the cache as a newly created state, then marking it with the current state version. This incorrectly skips all format upgrades.

Instead, all upgrades should be performed on the state.

Specifications

https://doc.rust-lang.org/std/fs/struct.Metadata.html#method.is_dir

Complex Code or Requirements

We missed this bug because there weren't enough tests for state version changes or state upgrades. But there are tests in PR #7379 now.

Solution

If there's a database directory wit no version file, report its version as 25.0.0. (Compatible but with no upgrades.)

Review

This bug fix blocks all the finalized state format changes.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@teor2345 teor2345 added C-bug Category: This is a bug P-Medium ⚡ I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data A-state Area: State / database changes labels Aug 25, 2023
@teor2345 teor2345 requested a review from a team as a code owner August 25, 2023 02:23
@teor2345 teor2345 self-assigned this Aug 25, 2023
@teor2345 teor2345 requested review from oxarbitrage and removed request for a team August 25, 2023 02:23
@teor2345 teor2345 changed the title fix(db): fix reported version for databases without a state version file fix(db): fix default state version for databases without a state version file Aug 25, 2023
@teor2345 teor2345 changed the title fix(db): fix default state version for databases without a state version file fix(db): use the correct state version for databases without a state version file Aug 25, 2023
upbqdn
upbqdn previously approved these changes Aug 25, 2023
@upbqdn
Copy link
Member

upbqdn commented Aug 25, 2023

@Mergifyio update.

@mergify
Copy link
Contributor

mergify bot commented Aug 25, 2023

update .

✅ Branch has been successfully updated

@teor2345
Copy link
Collaborator Author

This assert fails zcashfoundation/zebra@f08cd9f/zebra-state/src/service/finalized_state/disk_format/upgrade.rs#L481-L486.

It looks like database_format_version_on_disk returns None here zcashfoundation/zebra@f08cd9f/zebra-state/src/service/finalized_state/zebra_db.rs#L65-L66, but then Some here zcashfoundation/zebra@f08cd9f/zebra-state/src/service/finalized_state/disk_format/upgrade.rs#L477-L478, likely due to the new refactor. I don't see why this happens since database_format_version_on_disk should return consistent results.

That's my mistake. As soon as RocksDB creates the database directory, we provide 25.0.0 as the database version, even though the database is empty and newly created. We need to open RocksDB first, because we depend on its lock to protect the version file.

We could delete the "newly created" code path entirely to simplify things, if you want. Because now it's equivalent to an upgrade from 25.0.0 to the current version, but without any data in the database. (We might want to keep logging it differently, to avoid confusing users.) But that's out of scope for this PR, so let's open a ticket if there's enough interest.

Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

We could delete the "newly created" code path entirely to simplify things, if you want.

I'm fine with leaving it as it is.

zebra-state/src/config.rs Show resolved Hide resolved
mergify bot added a commit that referenced this pull request Aug 28, 2023
@mergify mergify bot merged commit c116cff into main Aug 28, 2023
290 checks passed
@mergify mergify bot deleted the fix-missing-state-version-file branch August 28, 2023 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-state Area: State / database changes C-bug Category: This is a bug I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants