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.

Sign up for GitHub

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

Jump to bottom

fix(state): Update column family names to match Zebra's database design #4639

Merged
merged 14 commits into from
Jun 30, 2022
Merged

fix(state): Update column family names to match Zebra's database design #4639

merged 14 commits into from
Jun 30, 2022

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Jun 17, 2022 •
edited by teor2345
Loading

Motivation

The documentation at https://github.com/ZcashFoundation/zebra/blob/main/book/src/dev/rfcs/0005-state-updates.md#rocksdb-data-structures does not currently match the source code. This PR makes the source code consistent with the docs.

Depends-On: #4586
Depends-On: #4669
Depends-On: #4704

Solution

This PR:

  • Renames all column families that didn't match the docs so that they match the docs. It also renames some variables in the non-finalized state to match the new names.
  • Reorders the column families so that the order matches the docs.
  • Updates struct Chain in the docs for the non-finalized state.
  • Increments DATABASE_FORMAT_VERSION to 25.
  • Removes obsolete docs.
  • Deletes unused snapshots.

Addresses #4150.

Review

I have not ran all tests yet, and some tests were failing, so I'm labelling this PR as a draft until I make the tests pass.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour

@upbqdn upbqdn added C-cleanup Category: This is a cleanup P-Low ❄️ A-state Area: State / database changes lightwalletd any work associated with lightwalletd labels Jun 17, 2022
@upbqdn upbqdn self-assigned this Jun 17, 2022
@upbqdn upbqdn force-pushed the update-column-family-names branch from 0ad51f4 to 68f097f Compare June 17, 2022 20:35
@codecov
Copy link

codecov bot commented Jun 17, 2022 •
edited
Loading

Codecov Report

Merging #4639 (928b56a) into main (11dcc13) will decrease coverage by 0.00%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             main    #4639      +/-   ##
==========================================
- Coverage   78.78%   78.78%   -0.01%     
==========================================
  Files         306      306              
  Lines       37552    37549       -3     
==========================================
- Hits        29586    29582       -4     
- Misses       7966     7967       +1

@upbqdn upbqdn changed the title Update column family names fix(state): Update column family names to match Zebra's database design Jun 18, 2022
@teor2345
Copy link
Contributor

We want to use this state version change to test old database deletion in PR #4586, so I've marked that as a dependency.

teor2345
teor2345 reviewed Jun 20, 2022
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

These all look pretty good!

I have a few suggestions for cleaning things up a bit.

book/src/dev/rfcs/0004-asynchronous-script-verification.md Outdated Show resolved Hide resolved
zebra-state/src/service/finalized_state/disk_db.rs Outdated Show resolved Hide resolved
...e/finalized_state/disk_format/tests/snapshots/block_header_by_height_raw_data@mainnet_0.snap Show resolved Hide resolved
...src/service/finalized_state/disk_format/tests/snapshots/empty_column_families@mainnet_0.snap Show resolved Hide resolved
@upbqdn
Copy link
Member Author

upbqdn commented Jun 20, 2022

I updated the description of the PR to match the newly added commits. I'll wait for PR #4586, and then increment DATABASE_FORMAT_VERSION to 26.

@teor2345
Copy link
Contributor

I updated the description of the PR to match the newly added commits. I'll wait for PR #4586, and then increment DATABASE_FORMAT_VERSION to 26.

PR #4586 doesn't change the database format, it just changes how we deal with outdated versions that are left behind on disk.

So this PR can stay at version 25.

teor2345
teor2345 approved these changes Jun 20, 2022
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

PR #4586 is about to merge, so we can merge this one next if you want.

@teor2345
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jun 21, 2022

update

✅ Branch has been successfully updated

@upbqdn upbqdn marked this pull request as ready for review June 21, 2022 15:18
@upbqdn upbqdn requested a review from a team as a code owner June 21, 2022 15:18
@upbqdn upbqdn removed the request for review from a team June 21, 2022 15:18
@upbqdn upbqdn requested a review from conradoplg June 21, 2022 15:18
@teor2345
Copy link
Contributor

No cached state disk available
Expected zebrad-cache-(branch)-[0-9a-f]+-v25-mainnet-checkpoint

https://github.com/ZcashFoundation/zebra/runs/6991264216?check_suite_focus=true#step:6:97

It looks like we skipped the "create state image" job, but we should have run it when the state version was updated:
https://github.com/ZcashFoundation/zebra/runs/6991230027?check_suite_focus=true

I'll see if I can fix this in another PR.

@teor2345
Copy link
Contributor

We haven't been creating cached state disks for about a month, I think that's fixed by:

@teor2345 teor2345 mentioned this pull request Jun 23, 2022
Merged
2 tasks
@upbqdn
Copy link
Member Author

upbqdn commented Jun 27, 2022 •
edited
Loading

@Mergifyio update.

@mergify
Copy link
Contributor

mergify bot commented Jun 27, 2022

update .

✅ Branch has been successfully updated

@teor2345 teor2345 mentioned this pull request Jun 28, 2022
Merged
1 task
@teor2345 teor2345 removed the request for review from conradoplg June 28, 2022 22:49
@upbqdn
Copy link
Member Author

upbqdn commented Jun 30, 2022

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jun 30, 2022

update

✅ Branch has been successfully updated

@mergify mergify bot merged commit 32faa94 into main Jun 30, 2022
@mergify mergify bot deleted the update-column-family-names branch June 30, 2022 23:00
@upbqdn upbqdn mentioned this pull request Jul 4, 2022
Closed
@teor2345 teor2345 mentioned this pull request Jun 5, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 approved these changes

Assignees

@upbqdn upbqdn

Labels
A-state Area: State / database changes C-cleanup Category: This is a cleanup lightwalletd any work associated with lightwalletd
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@upbqdn @teor2345