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

Cache note commitment tree roots #2584

Merged
merged 6 commits into from
Aug 10, 2021
Merged

Cache note commitment tree roots #2584

merged 6 commits into from
Aug 10, 2021

Conversation

conradoplg
Copy link
Collaborator

@conradoplg conradoplg commented Aug 6, 2021

Motivation

Syncing became really slow after the note commitment trees where added: it's 13 hours after history trees were merged. This is making regenerating the cached state cumbersome (when it finished we had already changed the format again...). It is also making the state tests slower, and we had to reduce the number of proptests to compensate which can arguably reduce our test coverage.

A good part of that is spend recomputing the tree roots, even when nothing was added to them. This is particularly bad for the Orchard tree that we instantiate from genesis just to make the code simpler, but for each block pushed it recompute the same root over and over again. (It didn't help that we were calling root() twice in a row in the finalized state instead of saving to a variable 😅)

Specifications

N/A

Designs

N/A

Solution

Cache the roots using Cell. Also write the cached root to disk when writing to state (which required another state version increase). This is because we read and write the tree for each block pushed to the state. This could be avoided by keeping the trees in memory but that would require much more code and testing.

Review

I think anyone who worked with the state before can review this.

I already started a VM using this code (instance 5402661260596673825 / 85f2cd7) it was another branch with the same logic, I just cleaned up comments and tests). It synced mainnet in 3,5 hours and it's syncing testnet now (height ~750K).

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

N/A


This change is Reviewable

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.

Let's merge after we document the impact of the cache on immutable reference sharing.

zebra-chain/src/orchard/tree.rs Show resolved Hide resolved
teor2345
teor2345 previously approved these changes Aug 9, 2021
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.

Just wanted to add another alternative, feel free to take it or leave it.

zebra-chain/src/orchard/tree.rs Outdated Show resolved Hide resolved
zebra-chain/src/sapling/tree.rs Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor

teor2345 commented Aug 9, 2021

I'm not going to merge this PR now, because it increments the state version.

Whoever merges it should start a state rebuild straight away. (I think @conradoplg might have done one already!)

@conradoplg conradoplg merged commit eac95bd into main Aug 10, 2021
@conradoplg conradoplg deleted the cache-tree-root branch August 10, 2021 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants