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.

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

Optimize GetDuties VC action #13789

Merged
merged 6 commits into from Mar 22, 2024
Merged

Optimize GetDuties VC action #13789

merged 6 commits into from Mar 22, 2024

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented Mar 22, 2024

What type of PR is this?

Other

What does this PR do? Why is it needed?

This PR reduces the time to fetch duties from ~8-9s to ~2.5s by utilizing wait groups and making concurrent requests. Concurrency is spread between two layers:

  1. Fetch duties for current and next epoch concurrently
  2. Within each epoch, fetch attester duties, proposer duties, sync committee duties and committees concurrently

Another piece of this PR is a getValidatorsForDuties function that is used instead of multipleValidatorStatus, This is because the latter does many things that we don't need for duties, including a call to GetValidatorCount which currently takes around 250ms on Holesky.

@rkapka rkapka requested a review from a team as a code owner March 22, 2024 06:49
@rkapka rkapka added Validator Client API Api related tasks Ready For Review A pull request ready for code review labels Mar 22, 2024
nalepae
nalepae previously approved these changes Mar 22, 2024

nextEpochDuties, err := c.getDutiesForEpoch(ctx, in.Epoch+1, known, fetchSyncDuties)
var currentEpochDuties []*ethpb.DutiesResponse_Duty
Copy link
Contributor

Choose a reason for hiding this comment

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

Question:

Why using an errgroup here for only one goroutine?
(In general errgroups are used when several goroutines run in parallel, and we want manage the case one of them returns an error.)

A pattern without errgroup could be the following:

errCh := make(chan error)  // Create a channel for errors
 go faultyFunction(errCh) // Start the faulty function in a new goroutine

 // Do other stuffs

 err := <-errCh  // Wait for an error
 fmt.Println("Received an error:", err)

With faultyFunction sending the error at the end in errCh.

However, I admit errgroup totally works here.

}
slotCommittees[c.Slot] = n + 1
}
var wg errgroup.Group
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as a safety measure, I would define:

  • attesterDutiesMapping,
  • syncDutiesMapping,
  • proposerDutySlots and
  • committeeMapping

at the same place in the code, and add a commentary saying that the main routine cannot use these variables before wg.Wait().

(Such a fact sounds obvious now, but in a few months/years an other developer may want to use these variables before the wg.Wait() and some bad things may happen in this case.)

Comment on lines 208 to 210
var attesterSlot primitives.Slot
var committeeIndex primitives.CommitteeIndex
var committeeValidatorIndices []primitives.ValidatorIndex
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var attesterSlot primitives.Slot
var committeeIndex primitives.CommitteeIndex
var committeeValidatorIndices []primitives.ValidatorIndex
var (
attesterSlot primitives.Slot
committeeIndex primitives.CommitteeIndex
committeeValidatorIndices []primitives.ValidatorIndex
)

@rkapka rkapka enabled auto-merge March 22, 2024 09:44
@rkapka rkapka added this pull request to the merge queue Mar 22, 2024
Merged via the queue into develop with commit 63c2b35 Mar 22, 2024
16 of 17 checks passed
@rkapka rkapka deleted the optimize-GetDuties branch March 22, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Api related tasks Ready For Review A pull request ready for code review Validator Client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants