retains chained Merkle root across leader slots #1057
Conversation
behzadnouri
force-pushed
the
retain-chained-merkle-root-flat
branch
from
c39a28f to
e9d29f6
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1057 +/- ##
=========================================
- Coverage 82.1% 82.1% -0.1%
=========================================
Files 868 868
Lines 234074 234054 -20
=========================================
- Hits 192373 192338 -35
- Misses 41701 41716 +15 |
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.
behzadnouri
force-pushed
the
retain-chained-merkle-root-flat
branch
from
e9d29f6 to
c9937d1
Compare
|
anyone interested to review this change? |
|
@carllin @AshwinSekar @bw-solana can any one of you review this change? |
| return Err(Error::DuplicateSlotBroadcast(bank.slot())); | ||
| } | ||
| // Reinitialize state for this slot. | ||
| let chained_merkle_root = (self.slot == bank.parent_slot()) |
There was a problem hiding this comment.
This is the only logical change right?
There was a problem hiding this comment.
This and maintaining self.chained_merkle_root across slots.
| shreds.extend(coding_shreds); | ||
| shreds | ||
| } | ||
| if let Some(shred) = shreds.iter().max_by_key(|shred| shred.index()) { |
There was a problem hiding this comment.
we should always generate 1 shred for the LAST_SHRED_IN_SLOT flag right? better to panic here?
There was a problem hiding this comment.
yeah, but we don't need to rely on that here, and kind of hesitant if the panic here is worth it.
I can split changes to |
Actually trying this locally, the changes separate from |
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
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)
`if/else` seems easier to follow, as it produces less nesting. Assignment of `self.chained_merkle_root` into `chained_merkle_root`, that is then assigned back into `self.chained_merkle_root` is a no-op. While I can see how it is separating the computation of the `chained_merkle_root` for the current step from the update of the `self`, I think, overall, it is more confusing then helpful. The `Finish previous slot ...` comment is actually part of a multi-paragraph sequence of steps, that document the whole `process_receive_results()`. Step number was accidentally removed in `anza-xyz#1057`: commit 6b45dc9 Author: behzad nouri <behzadnouri@gmail.com> Date: Fri May 3 20:20:10 2024 +0000 retains chained Merkle root across leader slots (anza-xyz#1057)
`if/else` seems easier to follow, as it produces less nesting. Assignment of `self.chained_merkle_root` into `chained_merkle_root`, that is then assigned back into `self.chained_merkle_root` is a no-op. While I can see how it is separating the computation of the `chained_merkle_root` for the current step from the update of the `self`, I think, overall, it is more confusing then helpful. The `Finish previous slot ...` comment is actually part of a multi-paragraph sequence of steps, that document the whole `process_receive_results()`. Step number was accidentally removed in `anza-xyz#1057`: commit 6b45dc9 Author: behzad nouri <behzadnouri@gmail.com> Date: Fri May 3 20:20:10 2024 +0000 retains chained Merkle root across leader slots (anza-xyz#1057)
`if/else` seems easier to follow, as it produces less nesting. Assignment of `self.chained_merkle_root` into `chained_merkle_root`, that is then assigned back into `self.chained_merkle_root` is a no-op. While I can see how it is separating the computation of the `chained_merkle_root` for the current step from the update of the `self`, I think, overall, it is more confusing then helpful. The `Finish previous slot ...` comment is actually part of a multi-paragraph sequence of steps, that document the whole `process_receive_results()`. Step number was accidentally removed in `anza-xyz#1057`: commit 6b45dc9 Author: behzad nouri <behzadnouri@gmail.com> Date: Fri May 3 20:20:10 2024 +0000 retains chained Merkle root across leader slots (anza-xyz#1057)
`if/else` seems easier to follow, as it produces less nesting. Assignment of `self.chained_merkle_root` into `chained_merkle_root`, that is then assigned back into `self.chained_merkle_root` is a no-op. While I can see how it is separating the computation of the `chained_merkle_root` for the current step from the update of the `self`, I think, overall, it is more confusing then helpful. The `Finish previous slot ...` comment is actually part of a multi-paragraph sequence of steps, that document the whole `process_receive_results()`. Step number was accidentally removed in `anza-xyz#1057`: commit 6b45dc9 Author: behzad nouri <behzadnouri@gmail.com> Date: Fri May 3 20:20:10 2024 +0000 retains chained Merkle root across leader slots (anza-xyz#1057)
…1057) (#1182) retains chained Merkle root across leader slots (#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>
…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>
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.