chore: add fork-choice and engine focil metrics #8311
Conversation
KatyaRyazantseva
added
the
scope-metrics
![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 @KatyaRyazantseva, 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 integrates new metrics into the beacon node and fork-choice components to enhance observability of the system's behavior, particularly concerning interactions with the execution engine and the handling of inclusion lists. These additions align with recent updates to the FOCIL specification, providing deeper insights into critical processes like fork-choice updates and block validation.
Highlights
- New Metrics for Fork-Choice Updates: Introduced beacon_engine_forkchoiceUpdatedV4_requests_total and beacon_engine_forkchoiceUpdatedV4_requests_duration_seconds to track the frequency and latency of forkchoiceUpdatedV4 requests sent to the execution client, specifically for EIP-7805 related forks.
- Inclusion List Transaction Metrics: Added beacon_inclusion_list_transactions_sent_to_payload_total to count the total number of inclusion list transactions sent to the execution payload.
- Inclusion List Equivocation Tracking: Implemented beacon_inclusion_lists_equivocating_total to monitor instances where inclusion lists are equivocating.
- Unsatisfied Inclusion List Blocks Metric: Introduced beacon_inclusion_list_unsatisfied_blocks_total to track blocks that fail to satisfy inclusion list requirements.
- Cleanup of TODOs: Removed placeholder comments related to these new metrics, indicating their successful implementation.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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 adds several new metrics related to fork-choice and the FOCIL (EIP-7805) specification. The changes are mostly straightforward, adding metric increments at various points in the codebase. However, I've found a critical issue where a duration metric is calculated incorrectly due to a missing await on an asynchronous call. I've also identified code duplication for metrics gathering logic in two separate files, which should be refactored for better maintainability. My review includes a detailed explanation of the bug with a suggested fix, and a recommendation to address the code duplication.
| if (isForkPostEip7805(forkName)) { | ||
| const requestDurationTime = Date.now() / 1000 - requestStartTime; | ||
| this.metrics?.eip7805.forkchoiceUpdatedV4RequestsDuration.observe(requestDurationTime); | ||
| this.metrics?.eip7805.forkchoiceUpdatedV4Requests.inc(); | ||
| } |
There was a problem hiding this comment.
The duration of the notifyForkchoiceUpdate request is measured incorrectly. The call is asynchronous and not awaited, so this block executes immediately, resulting in a near-zero duration. The metrics logic should be moved into a .then() block to execute after the promise resolves.
Example fix:
const requestStartTime = Date.now() / 1000;
this.executionEngine
.notifyForkchoiceUpdate(
this.config.getForkName(this.forkChoice.getHead().slot),
headBlockHash,
safeBlockHash,
finalizedBlockHash
)
.then(() => {
if (isForkPostEip7805(forkName)) {
const requestDurationTime = Date.now() / 1000 - requestStartTime;
this.metrics?.eip7805.forkchoiceUpdatedV4RequestsDuration.observe(requestDurationTime);
this.metrics?.eip7805.forkchoiceUpdatedV4Requests.inc();
}
})
.catch((e) => {
if (!isErrorAborted(e) && !isQueueErrorAborted(e)) {
this.logger.error("Error pushing notifyForkchoiceUpdate()", {headBlockHash, finalizedBlockHash}, e);
}
});| if (ForkSeq[fork] >= ForkSeq.eip7805) { | ||
| const requestDurationTime = Date.now() / 1000 - requestStartTime; | ||
| chain.metrics?.eip7805.forkchoiceUpdatedV4RequestsDuration.observe(requestDurationTime); | ||
| chain.metrics?.eip7805.forkchoiceUpdatedV4Requests.inc(); | ||
| } |
There was a problem hiding this comment.
This metrics-gathering logic is duplicated in packages/beacon-node/src/chain/blocks/importBlock.ts. To improve maintainability and reduce code duplication, consider extracting this logic into a shared helper function. This would also help in preventing bugs, such as the one currently present in the importBlock.ts implementation.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## focil #8311 +/- ##
==========================================
- Coverage 53.91% 53.90% -0.01%
==========================================
Files 857 857
Lines 65155 65151 -4
Branches 4865 4865
==========================================
- Hits 35126 35120 -6
- Misses 29952 29954 +2
Partials 77 77 🚀 New features to boost your workflow:
|
| finalizedBlockHash, | ||
| attributes | ||
| ); | ||
| if (ForkSeq[fork] >= ForkSeq.eip7805) { |
There was a problem hiding this comment.
this isn't really a focil specific metric anymore and we already track number and duration of fcu calls
Motivation
The focil TODO metrics need to be updated after the latest focil updates have been merged.
Description
The list of updated metrics:
Steps to test or reproduce
The metrics can be reproduced using the Kurtosis focil dashboard.