fix: fetch contributions only if there are aggregator duties #251
Conversation
| if len(selectionProofs) == 0 { | ||
| // there aren't any aggregators |
There was a problem hiding this comment.
as far as I understand we hit this code as long len(roots) == 0.
Else, even if an empty sig is created, we will still have a non-empty selectionProofs slice...
But this can only happen if we don't have quorum...
So nothing makes sense to me...
There was a problem hiding this comment.
@GalRogozinski
We hit this code if none of the duties are aggregator duties:
if !aggregator {
continue
}
| return errors.Wrap(err, "can't start new duty runner instance for duty") | ||
| } | ||
| } else { | ||
| r.BaseRunner.State.Finished = true |
There was a problem hiding this comment.
this is the fix right?
So we don't mark it as finished too early?
There was a problem hiding this comment.
Fix is to not call GetSyncCommitteeContribution when you have no aggregator duties.
So the "if no aggregators, set Finished to true" is moved up before that call.
Background: Before this PR, it tried to fetch contributions even if the duties aren't aggregator duties, which was pointless. |
|
Not sure I understand, we do check if aggregator |
Indeed, while we do check for an aggregator, in the current implementation we still proceed to call Updated PR description with help from GPT-4, I think it's much better now 😅 |
This PR addresses a situation in the
SyncCommitteeAggregatorRunner'sProcessPreConsensusmethod where attempts were made to fetch SyncCommitteeContributions even when there were no aggregator duties assigned. These attempts were unnecessary and resulted in inefficiencies in the code execution.Previously, the code was structured such that after checking for aggregators, it proceeded to
GetSyncCommitteeContributioneven when no aggregator duties were present. This resulted in failure since theGetSyncCommitteeContributionmethod couldn't process without aggregator duties (Beacon node recognizes this), and the subsequent check foranyIsAggregatorwas never reached.The changes in this PR include:
The code now only appends signatures to
selectionProofswhen an aggregator duty is identified, eliminating the need for theanyIsAggregatorcheck.The
ProcessPreConsensusmethod will exit early if no aggregator duties are identified, before attempting to callGetSyncCommitteeContribution.This way, the code is prevented from attempting to fetch
SyncCommitteeContributionwhen no aggregator duties are assigned, simplifying the flow and reducing unnecessary operations.