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

v1.18: retains chained Merkle root across leader slots (backport of #1057) #1182

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented May 3, 2024

Problem

Looking up Merkle root of the last erasure batch for the parent block can fail if the slot meta are not yet available in blockstore.
When a leader generates blocks for its consecutive leader slots, instead of using blockstore, it can retain chained Merkle root across its leader slots.

Summary of Changes

Retain chained Merkle roots across leader slots.


This is an automatic backport of pull request #1057 done by Mergify.

Looking up Merkle root of the last erasure batch for the parent block
can fail if the slot-meta is not yet available in blockstore.
This commit instead retains chained Merkle root across leader slots. If
the parent of the current block is the previous leader slot, then the
chained Merkle root is readily available and we can bypass blockstore
lookup.

(cherry picked from commit 6b45dc9)
@mergify mergify bot requested a review from a team as a code owner May 3, 2024 20:22
@mergify mergify bot requested a review from a team as a code owner May 3, 2024 20:22
@behzadnouri behzadnouri requested a review from AshwinSekar May 3, 2024 21:51
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 94.87179% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 81.6%. Comparing base (c8e6fd0) to head (5ab1b32).

Additional details and impacted files
@@            Coverage Diff            @@
##            v1.18    #1182     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         827      827             
  Lines      225352   225332     -20     
=========================================
- Hits       184011   183994     -17     
+ Misses      41341    41338      -3     

@behzadnouri
Copy link

@sakridge @CriesofCarrots @t-nelson

This change has been pending for more than 2 weeks.

@behzadnouri
Copy link

@sakridge @CriesofCarrots @t-nelson
This has been pending for 1 month now.
This is part of the chained merkle shreds and we definitely want this change so that to monitor err_unknown_chained_merkle_root before rolling out chained merkle shreds.

@CriesofCarrots
Copy link

This has been pending for 1 month now.

Yes, pending without any context posted on the PR.
As Trent mentioned, this diff is quite difficult to parse. It also seems to do a lot more than adjust err_unknown_chained_merkle_root; perhaps multiple commits in the original PR would have made it easier to distinguish new behavior from refactoring. I don't feel comfortable rubber-stamping atm.

@behzadnouri
Copy link

behzadnouri commented Jun 4, 2024

This has been pending for 1 month now.

Yes, pending without any context posted on the PR. As Trent mentioned, this diff is quite difficult to parse. It also seems to do a lot more than adjust err_unknown_chained_merkle_root; perhaps multiple commits in the original PR would have made it easier to distinguish new behavior from refactoring. I don't feel comfortable rubber-stamping atm.

@CriesofCarrots if you have specific questions about the code and change please ask.
ignoring the code review and pings for one month is not going to help with that.
after all, you have been forcing yourself to be the code reviewer on the code you don't have context on.

Copy link

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review coordinated offline. i'm ok with these changes now

@behzadnouri behzadnouri merged commit 34464d9 into v1.18 Jun 6, 2024
35 checks passed
@behzadnouri behzadnouri deleted the mergify/bp/v1.18/pr-1057 branch June 6, 2024 20:41
anwayde pushed a commit to firedancer-io/agave that referenced this pull request Jul 23, 2024
…nza-xyz#1057) (anza-xyz#1182)

retains chained Merkle root across leader slots (anza-xyz#1057)

Looking up Merkle root of the last erasure batch for the parent block
can fail if the slot-meta is not yet available in blockstore.
This commit instead retains chained Merkle root across leader slots. If
the parent of the current block is the previous leader slot, then the
chained Merkle root is readily available and we can bypass blockstore
lookup.

(cherry picked from commit 6b45dc9)

Co-authored-by: behzad nouri <behzadnouri@gmail.com>
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.

5 participants