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

feat(state): Store history trees by height in the non-finalized state #4928

Merged
merged 5 commits into from
Aug 29, 2022

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Aug 22, 2022

Motivation

In order to implement #4824, the non-finalized state needs to store history
trees by their height, so that it can send the history tree corresponding to the
root block to the finalized state when committing the root block.

Solution

This PR adds history_trees_by_height to Chain in the non-finalized state.

Closes #4837.

Follow Up Work

Finish #4721.

@upbqdn upbqdn requested review from a team as code owners August 22, 2022 23:34
@upbqdn upbqdn requested review from conradoplg and removed request for a team August 22, 2022 23:34
@upbqdn upbqdn added the A-state Area: State / database changes label Aug 22, 2022
@upbqdn upbqdn self-assigned this Aug 22, 2022
@upbqdn upbqdn added C-bug Category: This is a bug P-High 🔥 C-enhancement Category: This is an improvement I-slow Problems with performance or responsiveness and removed C-bug Category: This is a bug labels Aug 22, 2022
@codecov
Copy link

codecov bot commented Aug 23, 2022

Codecov Report

Merging #4928 (2bef36f) into main (4cda4ee) will decrease coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4928      +/-   ##
==========================================
- Coverage   79.18%   78.95%   -0.24%     
==========================================
  Files         309      309              
  Lines       38783    38789       +6     
==========================================
- Hits        30710    30625      -85     
- Misses       8073     8164      +91     

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.

Thanks, nice work on the PartialEq refactor.

@upbqdn
Copy link
Member Author

upbqdn commented Aug 23, 2022

@teor2345 if the CI doesn't pass, do you think you'd be able to merge this manually? It would be handy to have this PR in place for #4721.

@teor2345
Copy link
Contributor

teor2345 commented Aug 23, 2022

@teor2345 if the CI doesn't pass, do you think you'd be able to merge this manually? It would be handy to have this PR in place for #4721.

I think we've fixed all the outstanding bugs, so I'm hoping that we're done with admin merges.
(And we should really only be skipping CI for high priority PRs.)

Can you start your next PR by rebasing it on the add-hist-trees-by-height branch?
You might want to rebase add-hist-trees-by-height on main first.

If that won't work, here's what I expect will happen with CI:

If PR #4918 fails, you can try doing a @mergifyio update, and increasing the timeouts in that PR, or you can ask @oxarbitrage to fix the PR.

If all that doesn't work out, I'll see what I can do tomorrow morning.

@teor2345
Copy link
Contributor

This PR is a low priority, but I'm marking it with Medium so it merges a bit earlier, so that Marek can use it for other work.

@teor2345
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Aug 24, 2022

update

✅ Branch has been successfully updated

@gustavovalverde
Copy link
Member

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Aug 28, 2022

update

✅ Branch has been successfully updated

@teor2345 teor2345 removed the request for review from conradoplg August 29, 2022 02:08
mergify bot added a commit that referenced this pull request Aug 29, 2022
@mergify mergify bot merged commit a87b119 into main Aug 29, 2022
@mergify mergify bot deleted the add-hist-trees-by-height branch August 29, 2022 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-state Area: State / database changes C-enhancement Category: This is an improvement I-slow Problems with performance or responsiveness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store history trees by height in the non-finalized state
3 participants