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

change(state): Check database format is valid on shutdown, to catch format errors in new block code #7606

Merged
merged 5 commits into from
Sep 22, 2023

Conversation

teor2345
Copy link
Contributor

Motivation

We want to catch format errors in new block code. But currently that code is only tested when Zebra adds new blocks, then shuts down, then opens the same state. So PRs can merge with bugs in that code.

Instead, we check the format is valid before Zebra shuts down. This tells users early if they need to re-sync their state, rather than surprising them the next time they launch Zebra.

It also prevents us writing invalid cached states in CI.

Close #7570.

Complex Code or Requirements

I think a panic on shutdown should fail CI, but I haven't checked to make sure. Do you think it's worth doing that manually?

Solution

Related Refactors:

Review

This is optional for the release.

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?

Follow Up Work

We could replace a whole lot of config arguments with the database and version paths, but that can happen later if needed. Storing an Arc<Config> isn't that expensive.

@teor2345 teor2345 added A-consensus Area: Consensus rule updates P-Medium ⚡ C-testing Category: These are tests A-state Area: State / database changes labels Sep 21, 2023
@teor2345 teor2345 requested a review from a team as a code owner September 21, 2023 23:34
@teor2345 teor2345 self-assigned this Sep 21, 2023
@teor2345 teor2345 requested review from arya2 and removed request for a team September 21, 2023 23:34
@upbqdn
Copy link
Member

upbqdn commented Sep 21, 2023

I think a panic on shutdown should fail CI, but I haven't checked to make sure. Do you think it's worth doing that manually?

I don't think so.

Base automatically changed from flsync-valid to main September 22, 2023 01:33
@mergify mergify bot dismissed upbqdn’s stale review September 22, 2023 01:33

The base branch was changed.

@mergify mergify bot merged commit 7348d08 into main Sep 22, 2023
@mergify mergify bot deleted the shutdown-valid branch September 22, 2023 13:58
@upbqdn upbqdn mentioned this pull request Oct 13, 2023
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-state Area: State / database changes C-testing Category: These are tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test that "sync from empty" and "upgrade from existing" state code produces the same data
2 participants