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

change(state): Insert only the first tree in each series of identical trees into finalized state #7266

Merged
merged 21 commits into from
Aug 9, 2023
Merged

change(state): Insert only the first tree in each series of identical trees into finalized state #7266

merged 21 commits into from
Aug 9, 2023

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Jul 19, 2023 •
edited
Loading

Motivation

Address part of #4784.

Solution

  • Insert only the first Sapling and Orchard tree in each series of identical trees into the finalized state.
  • We don't store Sprout trees by height, only a single tree for the tip, so this PR doesn't optimize the storing of Sprout trees.
  • Update snapshots.
  • Simplify the methods that handle the trees.
  • Correctly retrieve the trees when retrieving by hash or height.

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?

@upbqdn upbqdn added A-rust Area: Updates to Rust code I-heavy Problems with excessive memory, disk, or CPU usage I-slow Problems with performance or responsiveness A-state Area: State / database changes C-tech-debt Category: Code maintainability issues labels Jul 19, 2023
@upbqdn upbqdn requested a review from teor2345 July 19, 2023 22:21
@upbqdn upbqdn self-assigned this Jul 19, 2023
@upbqdn upbqdn requested a review from a team as a code owner July 19, 2023 22:21
teor2345
teor2345 previously approved these changes Jul 19, 2023
View reviewed changes
Copy link
Collaborator

@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.

Thanks for making this change, it looks great!

Do you think it's worth splitting this PR so Zebra 1.2.0 states will be compatible with older Zebra 1.2.0 versions?

...tate/src/service/finalized_state/zebra_db/block/tests/snapshots/orchard_trees@mainnet_2.snap Outdated Show resolved Hide resolved
zebra-state/src/service/finalized_state/zebra_db/block.rs Show resolved Hide resolved
zebra-state/src/service/finalized_state/zebra_db/shielded.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 mentioned this pull request Jul 20, 2023
Closed
@arya2 arya2 added the do-not-merge Tells Mergify not to merge this PR label Jul 20, 2023
@arya2 arya2 mentioned this pull request Jul 21, 2023
Merged
@arya2 arya2 removed the do-not-merge Tells Mergify not to merge this PR label Jul 22, 2023
@mpguerra mpguerra linked an issue Jul 26, 2023 that may be closed by this pull request
Store only the first tree state in each series of identical tree states #4784
Closed
@mpguerra
Copy link
Contributor

this one seems ready to merge? @upbqdn I'll leave it up to you how you want to resolve the final comments :)

@upbqdn upbqdn requested a review from arya2 July 26, 2023 22:59
@upbqdn
Copy link
Member Author

upbqdn commented Jul 26, 2023 •
edited
Loading

All comments are addressed. I noticed there's a bug in the new code that upgrades the database. I'll push a fix in a separate PR.

@upbqdn
Copy link
Member Author

upbqdn commented Jul 26, 2023

I added a check that checks if the tree roots of the genesis block match the roots of empty trees, and I noticed our proptests currently generate genesis blocks with shielded data, so the trees aren't empty. I'll push a fix in a separate PR.

@upbqdn upbqdn mentioned this pull request Aug 7, 2023
Merged
6 tasks
@upbqdn upbqdn force-pushed the dedup-tree-insertion-fin-state branch from a30ad15 to 65f1cb5 Compare August 8, 2023 17:10
@upbqdn upbqdn mentioned this pull request Aug 8, 2023
Merged
6 tasks
@arya2
Copy link
Contributor

arya2 commented Aug 8, 2023

This again:

Message: can't upgrade a database that has already been upgraded, or is newer:
disk: 25.0.2
upgrade: 25.0.2
running: 25.1.0
Location: zebra-state/src/service/finalized_state/disk_format/upgrade.rs:390

https://github.com/ZcashFoundation/zebra/actions/runs/5800049030/job/15723531329?pr=7266#step:8:206

@upbqdn
Copy link
Member Author

upbqdn commented Aug 8, 2023

That should stop after #7304.

@arya2
Copy link
Contributor

arya2 commented Aug 8, 2023

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Aug 8, 2023

update

☑️ Nothing to do

  • #commits-behind>0 [:pushpin: update requirement]
  • -closed [:pushpin: update requirement]

mergify bot added a commit that referenced this pull request Aug 8, 2023
@mergify
Merge of #7266
b22b707
@mergify mergify bot mentioned this pull request Aug 8, 2023
Closed
@mergify mergify bot merged commit 57c9249 into main Aug 9, 2023
302 of 304 checks passed
@mergify mergify bot deleted the dedup-tree-insertion-fin-state branch August 9, 2023 00:32
@upbqdn upbqdn mentioned this pull request Aug 9, 2023
Merged
6 tasks
@upbqdn upbqdn mentioned this pull request Aug 29, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpguerra mpguerra mpguerra left review comments

@teor2345 teor2345 teor2345 left review comments

@arya2 arya2 arya2 approved these changes

Assignees

@upbqdn upbqdn

Labels
A-rust Area: Updates to Rust code A-state Area: State / database changes C-tech-debt Category: Code maintainability issues I-heavy Problems with excessive memory, disk, or CPU usage I-slow Problems with performance or responsiveness
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Store only the first tree state in each series of identical tree states
4 participants
@upbqdn @mpguerra @arya2 @teor2345