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

Move history tree and value balance to typed column families #8115

Merged
merged 18 commits into from
Dec 20, 2023

Conversation

teor2345
Copy link
Contributor

Motivation

This is another part of ticket #7833, making sure that the new typed column families work for more complex situations in the state. These commits or PRs will become examples in the documentation.

This PR converts the history tree and value balance to typed column families, and cleans up some risky and redundant code.

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?

If a checkbox isn't relevant to the PR, mark it as done.

Database Format Docs

Please check the types in the PR match the existing types in the database:
https://github.com/ZcashFoundation/zebra/blob/main/book/src/dev/state-db-upgrades.md#current-state-database-format

Complex Code or Requirements

This PR removes complexity by using consistent types for reading and writing column families.

Solution

  • Convert history trees to typed column families
    • Create methods for legacy (compatibility with 1.2.0 and earlier) and raw types (upgrade from 1.2.0 to 1.3.0)
    • Delete redundant format code that makes risky assumptions about history tree formats on disk
  • Convert value balance to typed column families
  • Add typed methods for writing into existing batches (owned and borrowed)
  • Make it easier to pass references when writing typed column families (by taking &Key and &Value and relying on auto-deref)
  • Always pass ZebraDb to database methods

Related changes:

  • Clean up and document zebra-state exports

Unrelated cleanups:

  • Remove a 1.72 lint exception now 1.74 is stable

Testing

Review

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

Follow Up Work

@teor2345 teor2345 added A-docs Area: Documentation C-cleanup Category: This is a cleanup P-Medium ⚡ A-state Area: State / database changes labels Dec 18, 2023
@teor2345 teor2345 self-assigned this Dec 18, 2023
@teor2345 teor2345 requested review from a team as code owners December 18, 2023 02:50
@teor2345 teor2345 requested review from upbqdn and removed request for a team December 18, 2023 02:50
Base automatically changed from scan-cleanups to main December 18, 2023 16:33
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.

The solution in this PR contains this item:

  • Always pass ZebraDb to database methods

I reverted this change because it caused a very tricky bug where Zebra would compile with cargo build, but not, for example, with cargo build --package zebrad or in other cases. We'll need a bigger refactor if we want to do that change.

I also manually resolved merge conflicts and fixed some API bugs that the merge introduced.

Otherwise, this PR looks good.

@upbqdn
Copy link
Member

upbqdn commented Dec 20, 2023

The cause of the bug was that passing ZebraDb to functions that expect DiskDb created a reliance on a Deref impl for ZebraDb targeting DiskDb, which is conditionally compiled if the proptest-impl or shielded-scan features are enabled during the compilation of zebra-state. By a weird coincidence, running cargo build or cargo build --release seems to enable shielded-scan when compiling zebra-state, so the Deref impl gets compiled in that case. It looks like this is a bug on its own because shielded-scan should not be enabled. Running, for example, cargo build --package zebrad doesn't enable the feature, so the Deref was missing, and Zebra didn't compile.

@mergify mergify bot merged commit ad015e0 into main Dec 20, 2023
102 checks passed
@mergify mergify bot deleted the state-typed-cf branch December 20, 2023 23:20
@teor2345
Copy link
Contributor Author

teor2345 commented Jan 1, 2024

The cause of the bug was that passing ZebraDb to functions that expect DiskDb created a reliance on a Deref impl for ZebraDb targeting DiskDb, which is conditionally compiled if the proptest-impl or shielded-scan features are enabled during the compilation of zebra-state. By a weird coincidence, running cargo build or cargo build --release seems to enable shielded-scan when compiling zebra-state, so the Deref impl gets compiled in that case. It looks like this is a bug on its own because shielded-scan should not be enabled. Running, for example, cargo build --package zebrad doesn't enable the feature, so the Deref was missing, and Zebra didn't compile.

We could have enabled the Deref impl in all builds, or called the db() method where needed.

But we'll deal with this when we split ZebraDb and DiskDb properly in #7937.

@oxarbitrage oxarbitrage mentioned this pull request Jan 17, 2024
41 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation A-state Area: State / database changes C-cleanup Category: This is a cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants