feat: allow block import after NUMBER_OF_COLUMNS / 2 #8408
Conversation
This reverts commit 0be6543.
…hainSafe/lodestar into mkeil/refactor-block-input-on-unstable
**Motivation** - clean up of block-input branch **Description** - bix build and unit test issues (many tests no longer relevant)
…c logic in a test case
**Motivation** - make e2e CI green for #8200 **Description** - fix e2e for unknownBlockSync - support fulu for other e2e tests --------- Co-authored-by: Tuyen Nguyen <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @wemeetagain, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the block processing mechanism by allowing blocks to be considered 'complete' and imported earlier, specifically once half of the required data columns are received. It also introduces a more flexible, consumer-driven approach to managing reconstruction delays, which is then leveraged to strategically delay reconstruction until after the 4-second mark in a slot. This change aims to optimize network load, improve block propagation efficiency, and align reconstruction efforts with critical protocol timings, ultimately contributing to a more robust and performant beacon node.
Highlights
- Reconstruction Trigger Condition: The
blockInput.hasAllDatacondition now triggers if at leastNUMBER_OF_COLUMNS / 2are available, enabling earlier block processing and import once sufficient data is present. - Consumer-Driven Reconstruction Delay: The
ColumnReconstructionTrackerhas been refactored to accept adelayparameter, allowing external callers to precisely specify the base delay before reconstruction occurs, enhancing control over timing. - Delayed Reconstruction in Gossip Handler: Reconstruction is now explicitly delayed until after the 4-second mark within a slot in the gossip handler, aligning with the head vote timing, and includes a small random component to desynchronize nodes and reduce peak load.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to allow block import to proceed when at least half of the data columns are available, which is enough for reconstruction. It also makes the reconstruction delay configurable. The changes are a good step forward. My review focuses on a potential bug in determining data availability for reconstruction and a minor code redundancy that could be improved for clarity.
| chain.columnReconstructionTracker.triggerColumnReconstruction(blockInput); | ||
| chain.columnReconstructionTracker.triggerColumnReconstruction( | ||
| // wait to reconstruct until after head vote | ||
| getCutoffTimeMs(chain, dataColumnSlot, 4000), |
There was a problem hiding this comment.
shouldn't be hard coding 4000 here, either SECONDS_PER_SLOT / INTERVALS_PER_SLOT or something similar to what we do in validator client
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## mkeil/refactor-block-input-on-unstable #8408 +/- ##
==========================================================================
- Coverage 54.42% 54.35% -0.07%
==========================================================================
Files 853 853
Lines 63935 64085 +150
Branches 4787 4789 +2
==========================================================================
+ Hits 34794 34833 +39
- Misses 29070 29181 +111
Partials 71 71 🚀 New features to boost your workflow:
|
Co-authored-by: Nico Flaig <[email protected]>
|
Discussion from voice chat:
|
|
Notes related to having all custodied columns before persisting everything to db:
|
| // gossip block is validated, we want to process it asap | ||
| eagerPersistBlock: true, | ||
| // however, due to other optimizations, we don't eagerly persist the block | ||
| eagerPersistBlock: false, |
There was a problem hiding this comment.
should we do this for postfulu only? need to update comments as well if we do
| // if we've received at least half of the columns, trigger reconstruction of the rest | ||
| if (blockInput.columnCount >= NUMBER_OF_COLUMNS / 2) { | ||
| chain.columnReconstructionTracker.triggerColumnReconstruction(blockInput); | ||
| chain.columnReconstructionTracker.triggerColumnReconstruction( |
There was a problem hiding this comment.
doing triggerColumnReconstruction in writeBlockInputToDeb.ts could be enough because data columns could be from different sources, not just gossip
| // block is validated with correct root, we want to process it as soon as possible | ||
| eagerPersistBlock: true, | ||
| // however, due to other optimizations, we don't eagerly persist the block | ||
| eagerPersistBlock: false, |
There was a problem hiding this comment.
same to above, if we do this postfulu only, we don't have to worry about the current validator performance of electra
| } | ||
|
|
||
| // Await writeBlockInputToDb if it was started above | ||
| await writeBlockInputPromise; |
There was a problem hiding this comment.
could you move this to after "Imported block" log but before "Block processed" log
that's give us more detail about timing
| const delay = | ||
| RECONSTRUCTION_DELAY_MIN_MS + Math.random() * (RECONSTRUCTION_DELAY_MAX_MS - RECONSTRUCTION_DELAY_MIN_MS); | ||
| sleep(delay).then(() => { | ||
| sleep(delay + Math.random() * RECONSTRUCTION_RANDOM_DELAY_MAX_MS).then(() => { |
There was a problem hiding this comment.
I'm in favor of having a mechanism to delay more based on number of columns arrived instead of a random()
similar to lighthouse
may want to revisit this later
|
needs to be rebased onto unstable |
5 participants
Motivation
Description