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): Refactor the structure of finalizable blocks #7035

Merged
merged 11 commits into from
Jun 27, 2023
Merged

change(state): Refactor the structure of finalizable blocks #7035

merged 11 commits into from
Jun 27, 2023

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Jun 21, 2023 •
edited
Loading

Motivation

Close #6912.

Solution

  • Create FinalizableBlock and use it instead of ContextuallyVerifiedBlockWithTrees in commit_finalized_direct().
  • Pass ContextuallyVerifiedBlockWithTrees instead of passing separate finalized, history_tree, and note_commitment_trees when storing blocks in the finalized state. This also simplifies the internal API when storing blocks since the parameter list is shorter.

This PR is based on PR #7025.

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 C-bug Category: This is a bug A-consensus Area: Consensus rule updates P-Medium ⚡ A-state Area: State / database changes C-audit Category: Issues arising from audit findings C-tech-debt Category: Code maintainability issues labels Jun 21, 2023
@upbqdn upbqdn requested a review from a team as a code owner June 21, 2023 12:10
@upbqdn upbqdn self-assigned this Jun 21, 2023
@upbqdn upbqdn requested review from teor2345 and removed request for a team June 21, 2023 12:10
@github-actions github-actions bot added the C-feature Category: New features label Jun 21, 2023
@upbqdn upbqdn changed the title Add finalizable block change(state): Refactor the structure of finalizable blocks Jun 21, 2023
@upbqdn upbqdn marked this pull request as draft June 21, 2023 12:14
@upbqdn
Copy link
Member Author

upbqdn commented Jun 21, 2023 •
edited
Loading

The initial base of this PR is #7025, so it would be suitable if we merge #7025 first, and then rebase this PR onto main. I, therefore, marked this PR as a draft, even though it's complete.

@mpguerra mpguerra linked an issue Jun 21, 2023 that may be closed by this pull request
Change ContextuallyVerifiedBlockWithTrees into an enum to correctly represent verification statuses #6912
Closed
@upbqdn upbqdn mentioned this pull request Jun 21, 2023
Merged
6 tasks
Base automatically changed from refactor-ContextuallyVerifiedBlockWithTrees to main June 21, 2023 16:58
@upbqdn upbqdn force-pushed the add-FinalizableBlock branch from 2a79ee2 to b7131e1 Compare June 21, 2023 17:40
@github-actions github-actions bot added the C-enhancement Category: This is an improvement label Jun 21, 2023
@upbqdn upbqdn force-pushed the add-FinalizableBlock branch 2 times, most recently from 7069a86 to a279938 Compare June 21, 2023 17:56
upbqdn added 2 commits June 21, 2023 19:59
@upbqdn
Add and use FinalizableBlock
584ad32 
This commit adds `FinalizableBlock`, and uses it instead of
`ContextuallyVerifiedBlockWithTrees` in `commit_finalized_direct()`
@upbqdn
Use ContextuallyVerifiedBlockWithTrees
dac739f 
This commit passes `ContextuallyVerifiedBlockWithTrees` instead of
passing separate `finalized`, `history_tree` and `note_commitment_trees`
when storing blocks in the finalized state.
@upbqdn upbqdn force-pushed the add-FinalizableBlock branch from a279938 to dac739f Compare June 21, 2023 17:59
@upbqdn
Copy link
Member Author

upbqdn commented Jun 21, 2023 •
edited
Loading

Rebased onto main.

@upbqdn upbqdn marked this pull request as ready for review June 21, 2023 18:01
teor2345
teor2345 previously approved these changes Jun 21, 2023
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.

Looks great, thanks!

All my comments are optional, but it would be nice to fix the docs and the type name.

zebra-state/src/request.rs Show resolved Hide resolved
zebra-state/src/request.rs Show resolved Hide resolved
zebra-state/src/service/finalized_state.rs Outdated Show resolved Hide resolved
zebra-state/src/service/finalized_state.rs Outdated Show resolved Hide resolved
zebra-state/src/service/finalized_state.rs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 22, 2023 •
edited
Loading

Codecov Report

Merging #7035 (3bae36e) into main (a6f35af) will decrease coverage by 0.16%.
The diff coverage is 81.08%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7035      +/-   ##
==========================================
- Coverage   77.52%   77.37%   -0.16%     
==========================================
  Files         310      310              
  Lines       41694    41690       -4     
==========================================
- Hits        32325    32259      -66     
- Misses       9369     9431      +62

@oxarbitrage oxarbitrage mentioned this pull request Jun 25, 2023
Merged
6 tasks
teor2345
teor2345 previously approved these changes Jun 26, 2023
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.

Let's merge this as it is, then open tickets for any further changes.

@teor2345
Copy link
Contributor

Here's the link to the long discussion about what to call "block with trees":
#7035 (comment)

@teor2345
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jun 27, 2023

update

✅ Branch has been successfully updated

@teor2345
Copy link
Contributor

Failed due to bug #7077

mergify bot added a commit that referenced this pull request Jun 27, 2023
@mergify
Merge of #7035
5c1a0c0
@mergify mergify bot mentioned this pull request Jun 27, 2023
Closed
@mergify mergify bot merged commit 1f1d04b into main Jun 27, 2023
@mergify mergify bot deleted the add-FinalizableBlock branch June 27, 2023 08:58
@teor2345 teor2345 mentioned this pull request Jun 28, 2023
Merged
44 tasks
@upbqdn upbqdn mentioned this pull request Jul 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxarbitrage oxarbitrage oxarbitrage left review comments

@teor2345 teor2345 teor2345 approved these changes

Assignees

@upbqdn upbqdn

Labels
A-consensus Area: Consensus rule updates A-state Area: State / database changes C-audit Category: Issues arising from audit findings C-bug Category: This is a bug C-enhancement Category: This is an improvement C-feature Category: New features C-tech-debt Category: Code maintainability issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change ContextuallyVerifiedBlockWithTrees into an enum to correctly represent verification statuses
3 participants
@upbqdn @teor2345 @oxarbitrage