GetValidatorPerformance only return its status info if the validator is active #5568

ShawkiS · 2020-04-21T19:03:36Z

Resolves #5485

Description
Write why you are making the changes in this pull request:
The problem here is that the user has 5 validator keys. Only 2 of these are active. The feature request is to only log "previous epoch voting summary" for activated validators as this log is not helpful for pending ones. A pending validator had no duties in the last epoch and there is really nothing to report on their performance.

Write a summary of the changes you are making:
This PR changes one file that is rpc/beacon/validators.go and it adds a condition to the GetValidatorPerformance to make sure it only returns its status info if the validator is active

CLAassistant · 2020-04-21T19:03:42Z

All committers have signed the CLA.

prestonvanloon · 2020-04-21T19:46:56Z

Please fix build


/root/.cache/bazel/_bazel_root/5c04516d923b21f838ef18b7f8ed7f88/execroot/prysm/beacon-chain/rpc/beacon/validators.go:817:16: undefined: errors
--
  | /root/.cache/bazel/_bazel_root/5c04516d923b21f838ef18b7f8ed7f88/execroot/prysm/beacon-chain/rpc/beacon/validators.go:819:32: not enough arguments in call to helpers.IsActiveValidator
  | have (*eth.Validator)
  | want (*eth.Validator, uint64)

terencechain · 2020-04-21T21:32:48Z

beacon-chain/rpc/beacon/validators.go

+			return nil, errors.Wrap(err, "could not get validator")
+		}
+		if !helpers.IsActiveValidator(val) {
+		// If the validator isn't active, no need to get its performance


Not sure if the comment here is necessary. It's pretty self explanatory imo

terencechain · 2020-04-21T21:33:22Z

Please also add a description to the PR

ShawkiS · 2020-04-22T05:38:05Z

@prestonvanloon Okay will fix it,

I saw two exapmples: one passing currentEpoch as helpers.SlotToEpoch(currentSlot) and the other helpers.PrevEpoch(state)

What shall I pass in our current example?

@terencechain okay will remove it

nisdas · 2020-04-22T06:56:34Z

beacon-chain/rpc/beacon/validators.go

-	if err != nil {
-		return nil, status.Error(codes.Internal, "Could not get head state")
-	}
+    headState, err := bs.HeadFetcher.HeadState(ctx)


please use gofmt here the indentation is off.

nisdas · 2020-04-22T06:57:25Z

beacon-chain/rpc/beacon/validators.go

+		val, err := headState.ValidatorAtIndex(idx)
+		currentEpoch := helpers.CurrentEpoch(headState)
+		if err != nil {
+			return nil, status.Errorf(codes.Internal, "could not get validator: %v", err)


fix the error here

could not get current epoch

Oh, why that is happening I saw this code working in other places?
Is the way I am getting it wrong?

ok so your order of code here is vulnerable to errors here. if you look at

idx, ok := headState.ValidatorIndexByPubkey(pubkeyBytes)

you need to handle the case where the index doesn't exist.
so immediately after that you need to do:

if !ok { missingValidators = append(missingValidators, key) continue }

Then doing this:

val, err := headState.ValidatorAtIndex(idx) currentEpoch := helpers.CurrentEpoch(headState) if err != nil {

is wrong you need to handle the error first

val, err := headState.ValidatorAtIndex(idx) if err != nil { return nil, status.Errorf(codes.Internal, "could not get validator: %v", err) } currentEpoch := helpers.CurrentEpoch(headState)

nisdas · 2020-04-22T07:22:07Z

beacon-chain/rpc/beacon/validators.go

@@ -804,8 +804,9 @@ func (bs *Server) GetValidatorPerformance(

 	headState, err := bs.HeadFetcher.HeadState(ctx)
 	if err != nil {
-		return nil, status.Error(codes.Internal, "Could not get head state")
+		return nil, status.Errorf(codes.Internal, "could not get head state: %v", err)


Suggested change

return nil, status.Errorf(codes.Internal, "could not get head state: %v", err)

return nil, status.Errorf(codes.Internal, "Could not get head state: %v", err)

This is a server error returned to our rpc clients, so it should be capitalized.

codecov · 2020-04-22T07:36:35Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #5568   +/-   ##
=======================================
  Coverage   27.46%   27.46%           
=======================================
  Files         239      239           
  Lines       20771    20771           
=======================================
  Hits         5705     5705           
  Misses      14070    14070           
  Partials      996      996

rauljordan · 2020-04-22T13:53:17Z

Hi @ShawkiS would also recommend always adding a PR description and a good title that gives us enough context about why this is needed (if it solves an open issue, type resolves #ISSUE_NUM or mention the issue it is related to in the description). Also use labels as needed, such as when the PR is ready for review or when it is still in progress.

ShawkiS · 2020-04-22T14:45:45Z

Hey @rauljordan, okay will make sure to do that next time because it seems that I can't edit the description now, yeah that's better I changed the name. Does it seem that I can't set a label?

nisdas

@ShawkiS Please add in a proper title with a PR description.

ShawkiS requested a review from a team as a code owner April 21, 2020 19:03

ShawkiS added 4 commits April 21, 2020 22:52

Task #1

24d2048

First commit

e2496fb

Fix

ac370a1

Clean

eb94867

terencechain reviewed Apr 21, 2020

View reviewed changes

ShawkiS added 2 commits April 22, 2020 00:36

Clean

d5ac521

Clean

3f07ab4

Merge branch 'master' into only-active-validators-log

2f34bf6

nisdas reviewed Apr 22, 2020

View reviewed changes

ShawkiS added 6 commits April 22, 2020 10:37

Clean

2df0279

Fix

50436dc

Fix

c9da8b8

Fix

03a647f

Fix

caa9534

Fix

c6254bc

ShawkiS changed the title ~~Only active validators log~~ resolves #5485 Apr 22, 2020

terencechain approved these changes Apr 22, 2020

View reviewed changes

nisdas requested changes Apr 23, 2020

View reviewed changes

ShawkiS changed the title ~~resolves #5485~~ GetValidatorPerformance only return its status info if the validator is active Apr 23, 2020

nisdas approved these changes Apr 23, 2020

View reviewed changes

Merge branch 'master' into only-active-validators-log

e2c7558

nisdas added the OK to merge label Apr 23, 2020

prylabs-bulldozer bot merged commit 49ca075 into prysmaticlabs:master Apr 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GetValidatorPerformance only return its status info if the validator is active #5568

GetValidatorPerformance only return its status info if the validator is active #5568

ShawkiS commented Apr 21, 2020 •

edited

CLAassistant commented Apr 21, 2020 •

edited

prestonvanloon commented Apr 21, 2020

terencechain Apr 21, 2020

terencechain commented Apr 21, 2020

ShawkiS commented Apr 22, 2020

nisdas Apr 22, 2020

ShawkiS Apr 22, 2020

nisdas Apr 22, 2020

ShawkiS Apr 22, 2020

nisdas Apr 22, 2020

nisdas Apr 22, 2020

codecov bot commented Apr 22, 2020 •

edited

rauljordan commented Apr 22, 2020

ShawkiS commented Apr 22, 2020

nisdas left a comment

	return nil, status.Errorf(codes.Internal, "could not get head state: %v", err)
	return nil, status.Errorf(codes.Internal, "Could not get head state: %v", err)

GetValidatorPerformance only return its status info if the validator is active #5568

GetValidatorPerformance only return its status info if the validator is active #5568

Conversation

ShawkiS commented Apr 21, 2020 • edited

Resolves #5485

CLAassistant commented Apr 21, 2020 • edited

prestonvanloon commented Apr 21, 2020

terencechain Apr 21, 2020

Choose a reason for hiding this comment

terencechain commented Apr 21, 2020

ShawkiS commented Apr 22, 2020

nisdas Apr 22, 2020

Choose a reason for hiding this comment

ShawkiS Apr 22, 2020

Choose a reason for hiding this comment

nisdas Apr 22, 2020

Choose a reason for hiding this comment

ShawkiS Apr 22, 2020

Choose a reason for hiding this comment

nisdas Apr 22, 2020

Choose a reason for hiding this comment

nisdas Apr 22, 2020

Choose a reason for hiding this comment

codecov bot commented Apr 22, 2020 • edited

Codecov Report

rauljordan commented Apr 22, 2020

ShawkiS commented Apr 22, 2020

nisdas left a comment

Choose a reason for hiding this comment

ShawkiS commented Apr 21, 2020 •

edited

CLAassistant commented Apr 21, 2020 •

edited

codecov bot commented Apr 22, 2020 •

edited