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

Measure consensus verification time #6089

Merged
merged 2 commits into from
Jul 16, 2024
Merged

Measure consensus verification time #6089

merged 2 commits into from
Jul 16, 2024

Conversation

michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented Jul 15, 2024 •
edited
Loading

Issue Addressed

We added a metric for "execution verification time" in a previous PR, but it actually captured consensus verification time as well:

Proposed Changes

  • Add a new metric to the block timing cache, the consensus_verification_time which is the delay between the block being observed and it being fully validated from Lighthouse's PoV (parent state loaded & state transition complete).
  • Fix the execution time metric so it measures the time between the request to the EL starting and finishing.

Additional Info

We should ideally update the missing attestations blog post:

As well as the dashboard in lighthouse-metrics:

@michaelsproul michaelsproul added work-in-progress PR is a work-in-progress UX-and-logs labels Jul 15, 2024
@michaelsproul michaelsproul added ready-for-review The code is ready for review v5.3.0 Q3 2024 release with database changes! and removed work-in-progress PR is a work-in-progress labels Jul 15, 2024
jimmygchen
jimmygchen approved these changes Jul 16, 2024
View reviewed changes
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

The changes make sense to me 👍

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jul 16, 2024
@michaelsproul
Copy link
Member Author

@mergify queue

@mergify Mergify
Copy link

mergify bot commented Jul 16, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 26c2e62

mergify bot added a commit that referenced this pull request Jul 16, 2024
@mergify
Merge of #6089
b9a455b
This was referenced Jul 16, 2024
Closed
Merged
Merged
@mergify mergify bot merged commit 26c2e62 into sigp:unstable Jul 16, 2024
28 checks passed
@michaelsproul michaelsproul deleted the consensus-verification-time branch July 16, 2024 04:59
ThreeHrSleep pushed a commit to ThreeHrSleep/lighthouse that referenced this pull request Aug 1, 2024
@michaelsproul @ThreeHrSleep
Measure consensus verification time (sigp#6089)
1bda942 
* Measure consensus verification time

* Track execution time properly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimmygchen jimmygchen jimmygchen approved these changes

Assignees
No one assigned
Labels
ready-for-merge This PR is ready to merge. UX-and-logs v5.3.0 Q3 2024 release with database changes!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@michaelsproul @jimmygchen