UpdateDuties improvement 1 #5662

mcdee · 2020-04-28T13:21:55Z

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

This PR stops some non-attesting calculation from being in the critical path for attesting.

At current UpdateDuties() calculates attestation subnets and aggregation function for current and next epoch before returning. It is possible for this to take a very long time if there is a high number of validators or slow CPU, due to the requirement to carry out two signatures per validator. At worst case it goes on for multiple slots, meaning the validator is not validating when it should.

This PR separates the subnet and aggregation piece in to a separate goroutine, to move it out of the critical path.

Which issues(s) does this PR fix?

There are a number of issues raised that show poor attestation numbers. This may or may not fix those individual issues, but it definitely fixes the situation where if the subnet calculation code takes too long to run it would result in missed proposals/attestations.

Other notes for review

This is part 1 of a three-part series. The three parts are:

move calculation for subnets out of the critical path
optimise calculation for subnets
cache 'next epoch' subnet results and reuse them if appropriate

codecov · 2020-04-28T13:45:28Z

Codecov Report

Merging #5662 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5662   +/-   ##
=======================================
  Coverage   59.87%   59.87%           
=======================================
  Files         312      312           
  Lines       26786    26786           
=======================================
  Hits        16039    16039           
  Misses       8604     8604           
  Partials     2143     2143

terencechain

Would you say that current_epoch is still a critical part? I think we all can agree next_epoch is not as critical

mcdee · 2020-04-28T14:18:22Z

I wouldn't call it critical. I looked at it this way:

Proposals:

if UpdateDuties() doesn't finish until after the end of the first slot, any proposal will be missed
if the goroutine doesn't finish until after the end of the first slot, no worries

Attestations:

if UpdateDuties() doesn't finish until after the end of the first slot, any attestation will be late, and possibly not included depending on how late
if the goroutine doesn't finish until after the end of the first slot, the attestation will still be made and broadcast but relies on there being either a pre-existing subnet or another node one hop away that is subscribed to the subnet for it to make its way to the relevant nodes for aggregation

The consideration for proposals is easy. For attestations, I figure it is better to create the attestation and then potentially wait on the delivery of it than definitely wait on the creation (and even then have no guarantee that the delivery will be any better, given it can take time for the node to become a member of the subnet).

terencechain · 2020-04-28T14:46:08Z

I wouldn't call it critical. I looked at it this way:

Proposals:

if UpdateDuties() doesn't finish until after the end of the first slot, any proposal will be missed

if the goroutine doesn't finish until after the end of the first slot, no worries

Attestations:

if UpdateDuties() doesn't finish until after the end of the first slot, any attestation will be late, and possibly not included depending on how late

if the goroutine doesn't finish until after the end of the first slot, the attestation will still be made and broadcast but relies on there being either a pre-existing subnet or another node one hop away that is subscribed to the subnet for it to make its way to the relevant nodes for aggregation

The consideration for proposals is easy. For attestations, I figure it is better to create the attestation and then potentially wait on the delivery of it than definitely wait on the creation (and even then have no guarantee that the delivery will be any better, given it can take time for the node to become a member of the subnet).

I agree with your reasoning. One last question, have you tested this change with attestant's validators?
This is something that can pass canary but fail run time in production

mcdee · 2020-04-28T14:51:49Z

Great question. I've been running with a variant of this since day 2 of the new testnet, so I'm happy with the general idea. What I haven't done is run the code in this PR in isolation from some other changes (pretty much part 2 of the above). I'll set that up now and let it tick along for a day or so and report back.

terencechain · 2020-04-28T14:55:58Z

Great question. I've been running with a variant of this since day 2 of the new testnet, so I'm happy with the general idea. What I haven't done is run the code in this PR in isolation from some other changes (pretty much part 2 of the above). I'll set that up now and let it tick along for a day or so and report back.

Sounds good, thanks! Ping us and we'll merge this

terencechain · 2020-04-28T16:37:00Z

I also worked on steaming validator duty, see:
#5306
https://hackmd.io/KWBj0PCDQUuPKpLQnIiZbw?both

Unfortunately I just haven't had the bandwidth to drive it to completion so anyone is more than welcome to jump in, just let me know which part so we dont duplicate work here

prestonvanloon · 2020-04-28T21:37:22Z

validator/client/validator.go

-		CommitteeIds: subscribeCommitteeIDs,
-		IsAggregator: subscribeIsAggregator,
-	})
+		_, err = v.validatorClient.SubscribeCommitteeSubnets(ctx, &ethpb.CommitteeSubnetsSubscribeRequest{


Please handle this error

Will do; thanks for the spot.

prestonvanloon · 2020-04-28T21:42:32Z

validator/client/validator.go

-			attesterSlot := duty.AttesterSlot
-			committeeIndex := duty.CommitteeIndex
+	go func() {
+		// Create a new context as the existing one will be canceled when the parent function exits.


Can you break all of this out into another method? (All of the stuff in this anonymous go func

Can do; if I break it out simply the function call will look a little messy, i.e. v.subscribeToSubnets(parentCtx, slot, validatingKeys, req) I was planning on a proper rewrite of this code to functions for the next part (with the removal of logging to a separate call there are two pieces of code that are virtually identical). Three options:

leave the code inline; deal with it in the next PR

use the function call as outlined above; tidy up in the next PR

bring forward the changes for the second phase in to this PR

Happy with any of these; let me know which you prefer.

@prestonvanloon any thoughts on the above?

nisdas · 2020-05-12T13:41:49Z

@prestonvanloon can you take a look at this again ?

rauljordan · 2020-05-15T17:30:34Z

Hi @mcdee, we have modified our duties implementation to be a server-side stream #5867, which the client listens to in the background. This new streaming approach solves the same problem as this PR, so we can close. Apologies for the long feedback cycle on this and thank you for the contribution nonetheless.

rauljordan · 2020-05-15T19:48:38Z

These are still crucial:

1. optimise calculation for subnets
2. cache 'next epoch' subnet results and reuse them if appropriate

we won't be touching the duties code after the stream duties functionality, so those additions would be very much welcome. For now, closing this PR

mcdee added 2 commits April 28, 2020 12:28

Calculate attestation subscriptions in a goroutine

bd8bf67

Update test to take in to account async nature of goroutine

24f3f74

mcdee requested a review from a team as a code owner April 28, 2020 13:21

mcdee requested review from shayzluf, terencechain and nisdas April 28, 2020 13:21

terencechain reviewed Apr 28, 2020

View reviewed changes

Merge branch 'master' into duties-improvement-1

e708e2d

rauljordan approved these changes Apr 28, 2020

View reviewed changes

terencechain added the Blocked Blocked by research or external factors label Apr 28, 2020

prestonvanloon requested changes Apr 28, 2020

View reviewed changes

prestonvanloon reviewed Apr 28, 2020

View reviewed changes

mcdee added 7 commits April 29, 2020 08:34

Merge branch 'master' into duties-improvement-1

8c158c9

Merge branch 'master' into duties-improvement-1

e9af7d5

Merge branch 'master' into duties-improvement-1

d475e41

Merge branch 'master' into duties-improvement-1

9fd58ce

Merge branch 'master' into duties-improvement-1

df80cfc

Merge branch 'master' into duties-improvement-1

23dc0a7

Merge branch 'master' into duties-improvement-1

b3a6ba8

nisdas requested a review from prestonvanloon May 12, 2020 13:42

Merge branch 'master' into duties-improvement-1

09d6d7c

rauljordan closed this May 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UpdateDuties improvement 1 #5662

UpdateDuties improvement 1 #5662

mcdee commented Apr 28, 2020

codecov bot commented Apr 28, 2020 •

edited

terencechain left a comment

mcdee commented Apr 28, 2020

terencechain commented Apr 28, 2020

mcdee commented Apr 28, 2020

terencechain commented Apr 28, 2020

terencechain commented Apr 28, 2020

prestonvanloon Apr 28, 2020

mcdee Apr 29, 2020

prestonvanloon Apr 28, 2020 •

edited

mcdee Apr 29, 2020

mcdee May 8, 2020

nisdas commented May 12, 2020

rauljordan commented May 15, 2020

rauljordan commented May 15, 2020

UpdateDuties improvement 1 #5662

UpdateDuties improvement 1 #5662

Conversation

mcdee commented Apr 28, 2020

codecov bot commented Apr 28, 2020 • edited

Codecov Report

terencechain left a comment

Choose a reason for hiding this comment

mcdee commented Apr 28, 2020

terencechain commented Apr 28, 2020

mcdee commented Apr 28, 2020

terencechain commented Apr 28, 2020

terencechain commented Apr 28, 2020

prestonvanloon Apr 28, 2020

Choose a reason for hiding this comment

mcdee Apr 29, 2020

Choose a reason for hiding this comment

prestonvanloon Apr 28, 2020 • edited

Choose a reason for hiding this comment

mcdee Apr 29, 2020

Choose a reason for hiding this comment

mcdee May 8, 2020

Choose a reason for hiding this comment

nisdas commented May 12, 2020

rauljordan commented May 15, 2020

rauljordan commented May 15, 2020

codecov bot commented Apr 28, 2020 •

edited

prestonvanloon Apr 28, 2020 •

edited