-
Notifications
You must be signed in to change notification settings - Fork 284
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
7549 - VC aggregation #8300
7549 - VC aggregation #8300
Conversation
@@ -170,7 +170,7 @@ private String formatBlockRoots(final Set<Bytes32> blockRoots) { | |||
return blockRoots.stream().map(LogFormatter::formatHashRoot).collect(Collectors.joining(", ")); | |||
} | |||
|
|||
public void aggregationSkipped(final UInt64 slot, final int committeeIndex) { | |||
public void aggregationSkipped(final UInt64 slot, final UInt64 committeeIndex) { |
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.
we only see this used in test code in this PR which is curious... if its only testing code we should probably move it, and Im less concerned with the interface change, otherwise i think committeeIndex would have been better to stay as int :)
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.
I had a back and forth among int<->UInt64. I landed there just to maintain similarity to validatorIndex
.
And i also saw some UInt64 related to committteeIndex in some API params.
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.
why you say it is only used in testing? I see this :)
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.
weird that interface changes and calling point doesnt...
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, one nit about something only used in testing code
ValidatorApiChannel::createAggregate
by adding an optionalcommitteeIndex
to allow BN to know about committee to aggregate for. (Implemented only for in-process VC)MatchingDataAttestationGroup
) to apply the committee filtering if required.Related to #7965
Documentation
doc-change-required
label to this PR if updates are required.Changelog