-
Notifications
You must be signed in to change notification settings - Fork 332
Add peerDAS kzg batch verification metric #9415
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
Add peerDAS kzg batch verification metric #9415
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 2 places where we use kzg verify batch in DataColumnSidecarGossipValidator
also
metricsSystem, | ||
timeProvider, | ||
TekuMetricCategory.BEACON, | ||
"kzg_verification_data_column_batch_seconds", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need one metric in several places, please define it only once and reuse, see
Line 56 in 9cd9e8a
public static final BiFunction<MetricsSystem, TimeProvider, MetricsHistogram> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just 2 cases where we should reject like kzg verification failed (it's actually failed, we don't know in which part)
return completedFuture(reject("DataColumnSidecar does not pass kzg validation")); | ||
} | ||
} catch (final Throwable t) { | ||
throw new RuntimeException(t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the same: return completedFuture(reject("DataColumnSidecar does not pass kzg validation"));
return completedFuture(reject("DataColumnSidecar does not pass kzg validation")); | ||
} | ||
} catch (final Throwable t) { | ||
throw new RuntimeException(t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reject by the same reason if it didn't pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR Description
Add PeerDAS
beacon_kzg_verification_data_column_batch_seconds
metric to align with beacon metrics specsDocumentation
doc-change-required
label to this PR if updates are required.Changelog