-
Notifications
You must be signed in to change notification settings - Fork 745
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
[Merged by Bors] - Metrics for sync aggregate fullness #2439
Conversation
commit 8847bd0 Author: Paul Hauner <paul@paulhauner.com> Date: Thu Jul 8 18:24:00 2021 +1000 Add metrics for sync committee fullness
Blocked on #2279. After it mergers I'll change the base to |
8847bd0
to
fb084ef
Compare
This is unblocked and ready for review :) |
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.
Happy to merge with or without suggested fix
block.body().attestations().len() as f64, | ||
); | ||
|
||
if let Ok(block) = block.as_altair() { |
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.
This would be good as block.body().sync_aggregate()
so that it works for all post-Altair block types, but I realise that the sync_aggregate
method doesn't exist yet in unstable
.
When we implement the next fork we should audit all uses of as_altair
anyway, so I'm happy to merge without fixing this.
The definition of sync_aggregate
is:
impl<'a, T: EthSpec> BeaconBlockBodyRef<'a, T> {
/// Access the sync aggregate from the block's body, if one exists.
pub fn sync_aggregate(self) -> Option<&'a SyncAggregate<T>> {
match self {
BeaconBlockBodyRef::Base(_) => None,
BeaconBlockBodyRef::Altair(inner) => Some(&inner.sync_aggregate),
}
}
}
in beacon_block_body.rs
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.
Oh, great idea. I hadn't considered this problem before. I've added this function in 0dc02e5. Thanks for laying it all out nicely for me 🙂
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.
Noice 😎
bors r+ |
## Issue Addressed NA ## Proposed Changes Adds a metric to see how many set bits are in the sync aggregate for each beacon block being imported. ## Additional Info NA
Pull request successfully merged into unstable. Build succeeded: |
Issue Addressed
NA
Proposed Changes
Adds a metric to see how many set bits are in the sync aggregate for each beacon block being imported.
Additional Info
NA