Command to fetch validator statuses + MultipleValidatorStatus #5784

michaelhly · 2020-05-08T18:50:32Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?
Add validator accounts status command to fetch validator statuses from the Beacon Node.

Which issues(s) does this PR fix?
Resolves #5578

Other notes for review
Summary of changes:

MultipleValidatorStatus endpoint
Update beacon_node_validator_service_mock.go
status command for the validator client
- Status fetcher
- Status sorter
Tests for the above

TODO:

Runtime Demos:
Connected to syncing node

> bazel run validator accounts status
2020/05/12 15:12:45 maxprocs: Leaving GOMAXPROCS=8: CPU quota undefined
[2020-05-13 08:24:57]  INFO accounts: Please specify the keystore path for your private keys (default: "/Users/username/Library/Eth2Validators"):

[2020-05-13 08:24:59]  INFO accounts: Please enter the password for your private keys
[2020-05-13 08:24:59]  INFO node: Enter a password:
[2020-05-13 08:25:01]  WARN validator: You are using an insecure gRPC connection! Please provide a certificate and key to use a secure connection.
[2020-05-13 08:25:01] FATAL main: Could not fetch account statuses from the beacon node. error=rpc error: code = Unavailable desc = Syncing to latest head, not ready to respond
Failed to fetch validator statuses for 1 key(s)

Node offline

> bazel run validator accounts status
2020/05/12 15:15:09 maxprocs: Leaving GOMAXPROCS=8: CPU quota undefined
[2020-05-12 15:15:09]  INFO accounts: Please specify the keystore path for your private keys (default: "/Users/username/Library/Eth2Validators"):

[2020-05-12 15:15:15]  INFO accounts: Please enter the password for your private keys
[2020-05-12 15:15:15]  INFO node: Enter a password:
[2020-05-12 15:15:21]  WARN validator: You are using an insecure gRPC connection! Please provide a certificate and key to use a secure connection.

// Waits for 10 seconds

[2020-05-12 15:15:31] FATAL main: Failed to dial beacon node endpoint at localhost:4000 error=context deadline exceeded

Connected to synced node
https://gist.github.com/michaelhly/cc00d4329974582abcbbbb19dbed66a0

terencechain · 2020-05-10T00:14:07Z

validator/accounts/status.go

+	}
+
+	req := &ethpb.ValidatorStatusRequest{
+		PublicKey: pubKey,


So if we have N keys then it's N requests to the beacon node. Can we batch this to just 1 request?

Yes. Right now it's like this so I can avoid compile errors.

I'm waiting for prysmaticlabs/ethereumapis#148 to get merged. Once the API for ValidatorStatus is updated, we'll batch public keys into slices of at most C keys. So we'll make k=N/C requests.

And after receiving the responses from the node, we'll run a k-way merge sort and then print the results.

codecov · 2020-05-11T06:52:11Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #5784   +/-   ##
=======================================
  Coverage   59.60%   59.60%           
=======================================
  Files         315      315           
  Lines       26570    26570           
=======================================
  Hits        15836    15836           
  Misses       8590     8590           
  Partials     2144     2144

…validator-client-account-statuses

prestonvanloon · 2020-05-18T17:12:59Z

validator/accounts/status.go

+		grpc.WithStatsHandler(&ocgrpc.ClientHandler{}),
+		grpc.WithBlock(),
+		grpc.WithTimeout(
+			10 * time.Second /* Block for 10 seconds to see if we can connect to beacon node */),


Deprecated: use DialContext instead of Dial and context.WithTimeout instead. Will be supported throughout 1.x.

https://godoc.org/google.golang.org/grpc#WithTimeout

validator/accounts/status.go

…validator-client-account-statuses

…m/michaelhly/prysm into validator-client-account-statuses

rauljordan · 2020-05-19T02:07:24Z

PTAL @terencechain

terencechain

Lgtm. Thanks Michael!

nisdas · 2020-05-19T02:17:38Z

beacon-chain/rpc/validator/status.go

+	filtered := make(map[[48]byte]bool)
+	filtered[[48]byte{}] = true // Filter out keys with all zeros.
+	// Filter out duplicate public keys.
+	for _, pubKey := range req.GetPublicKeys() {


no need to copy here, you can access the field directly

nisdas · 2020-05-19T02:17:47Z

beacon-chain/rpc/validator/status.go

+		}
+	}
+	// Convert indices to public keys.
+	for _, idx := range req.GetIndices() {


same over here

validator/accounts/account.go

nisdas · 2020-05-19T02:21:34Z

validator/accounts/status.go

+
+// MaxRequestLimit specifies the max concurrent grpc requests allowed
+// to fetch account statuses.
+const MaxRequestLimit = 5 // XXX: Should create flag to make parameter configurable.


Remove the XXX comment, or open up an issue in the repo for this as a todo. We use
TODO(#Num) where Num represents the issue number.

nisdas · 2020-05-19T02:21:52Z

validator/accounts/status.go

+
+// MaxRequestKeys specifies the max amount of public keys allowed
+// in a single grpc request, when fetching account statuses.
+const MaxRequestKeys = 2000 // XXX: This is an arbitrary number. Used to limit time complexity


Same here remove XXX

nisdas · 2020-05-19T02:23:11Z

validator/accounts/status.go

+	maxCallRecvMsgSize int,
+	grpcRetries uint,
+	grpcHeaders []string) error {
+	dialOpts, err := constructDialOptions(maxCallRecvMsgSize, withCert, grpcHeaders, grpcRetries)


Can you not construct the GRPC client in this command, this should be done on the outside of the method.

nisdas · 2020-05-19T02:25:56Z

validator/accounts/status.go

+	errorChannel := make(chan error, MaxRequestLimit)
+	statusChannel := make(chan []ValidatorStatusMetadata, MaxRequestLimit)
+	// Launch routines to fetch statuses.
+	i, numBatches := 0, 0


please use scatter from mputils for parallelized requests.
https://github.com/prysmaticlabs/prysm/blob/master/shared/mputil/scatter.go#L21

@nisdas do you think that is going to help here? This isn't doing any computation really, just fetching the requests from the server.

Mostly because it looks like its doing a lot of repeat work which is done by Scatter, its also just splitting up the requests into separate goroutines using a fixed request size.

prestonvanloon · 2020-05-19T03:12:30Z

validator/accounts/status.go

+	errorChannel := make(chan error, MaxRequestLimit)
+	statusChannel := make(chan []ValidatorStatusMetadata, MaxRequestLimit)
+	// Launch routines to fetch statuses.
+	i, numBatches := 0, 0


@nisdas do you think that is going to help here? This isn't doing any computation really, just fetching the requests from the server.

validator/accounts/status.go

nisdas · 2020-05-19T04:45:49Z

validator/accounts/status.go

+				"Eth1DepositBlockNumber":    fieldToString(m.Eth1DepositBlockNumber),
+				"PositionInActivationQueue": fieldToString(m.PositionInActivationQueue),
+			},
+		).Infof("Status=%v\n PublicKey=0x%s\n", m.Status, hex.EncodeToString(key))


pubkey should be above under logrus.Fields . You should be only logging the status here, with the status being an enum type, you can simple represent it using %s

validator/accounts/status.go

nisdas

LGTM !

michaelhly added 3 commits May 8, 2020 13:39

Add method to fetch account statuses

a7a3ae0

Add status command

673c839

gofmt

4e97a2a

michaelhly force-pushed the validator-client-account-statuses branch from 91ce0fc to fdc668b Compare May 8, 2020 19:10

Add comment about sorting

3c357a1

michaelhly force-pushed the validator-client-account-statuses branch from fdc668b to 3c357a1 Compare May 8, 2020 19:13

Close conneciton when finished

1294fdf

michaelhly force-pushed the validator-client-account-statuses branch from 5ea8600 to 1294fdf Compare May 8, 2020 19:28

michaelhly added 8 commits May 8, 2020 15:49

Fix error

462f35f

Refactor FetchAccountStatuses

d355b13

Add status_test.go

a2c2f45

Move sorting out of FetchAccountStatuses

237620d

Remove unnecessary casting

15132b7

Expect ValidatorStatus to be called

e8123b4

Wrap long comment

c0d837e

Comment out sorting

7b7efef

michaelhly force-pushed the validator-client-account-statuses branch from 3e9e770 to 7b7efef Compare May 10, 2020 00:07

terencechain reviewed May 10, 2020

View reviewed changes

michaelhly force-pushed the validator-client-account-statuses branch from 3bc7daf to 2ab3a6b Compare May 10, 2020 23:30

Add all necessary dial options

9635895

michaelhly force-pushed the validator-client-account-statuses branch from 2ab3a6b to 9635895 Compare May 11, 2020 00:05

michaelhly added 2 commits May 10, 2020 19:13

Close connection before evaluating error from fetch

348b799

Small changes

c39e231

michaelhly force-pushed the validator-client-account-statuses branch from 6a4f56f to 8096b80 Compare May 11, 2020 03:43

Fix lint issues

74c315b

michaelhly force-pushed the validator-client-account-statuses branch from 8096b80 to 74c315b Compare May 11, 2020 03:47

Merge branch 'master' into validator-client-account-statuses

dbf8bee

Update dependencies for docker images

c2558ec

michaelhly marked this pull request as ready for review May 11, 2020 14:59

michaelhly force-pushed the validator-client-account-statuses branch 3 times, most recently from dc8e385 to f7237cf Compare May 17, 2020 00:41

michaelhly added 2 commits May 17, 2020 01:43

Update output

4952966

Merge branch 'master' of https://github.com/prysmaticlabs/prysm into …

a1463a1

…validator-client-account-statuses

michaelhly force-pushed the validator-client-account-statuses branch from f7237cf to a1463a1 Compare May 17, 2020 06:45

michaelhly mentioned this pull request May 18, 2020

Docs for validator accounts status command prysmaticlabs/documentation#108

Merged

prestonvanloon reviewed May 18, 2020

View reviewed changes

rauljordan and others added 6 commits May 18, 2020 16:35

Merge branch 'master' into validator-client-account-statuses

f77e5e6

Fix conflicts

1eea671

Merge branch 'master' of https://github.com/prysmaticlabs/prysm into …

c086663

…validator-client-account-statuses

Merge branch 'validator-client-account-statuses' of https://github.co…

3b1be09

…m/michaelhly/prysm into validator-client-account-statuses

Set context timeout for FetchAccountStatuses

a786cb0

Remove deprecated grpc.WithTimeout

1d78e7a

gofmt

9744278

terencechain previously approved these changes May 19, 2020

View reviewed changes

nisdas requested changes May 19, 2020

View reviewed changes

prestonvanloon reviewed May 19, 2020

View reviewed changes

michaelhly added 3 commits May 18, 2020 22:21

Remove getters

9100ecb

Remove parallel stuff

a15de80

Move grpc dialing out of status.go

5472aac

michaelhly dismissed terencechain’s stale review via 5472aac May 19, 2020 04:38

nisdas reviewed May 19, 2020

View reviewed changes

Update logging based on feedback

2f8ce09

nisdas reviewed May 19, 2020

View reviewed changes

validator/accounts/status.go Outdated Show resolved Hide resolved

Update validator/accounts/status.go

7861331

nisdas approved these changes May 19, 2020

View reviewed changes

nisdas added the OK to merge label May 19, 2020

prylabs-bulldozer bot merged commit 62b617f into prysmaticlabs:master May 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Command to fetch validator statuses + MultipleValidatorStatus #5784

Command to fetch validator statuses + MultipleValidatorStatus #5784

michaelhly commented May 8, 2020 •

edited

terencechain May 10, 2020

michaelhly May 10, 2020 •

edited

codecov bot commented May 11, 2020 •

edited

prestonvanloon May 18, 2020

rauljordan commented May 19, 2020

terencechain left a comment

nisdas May 19, 2020

nisdas May 19, 2020

nisdas May 19, 2020

nisdas May 19, 2020

nisdas May 19, 2020

nisdas May 19, 2020

prestonvanloon May 19, 2020

nisdas May 19, 2020

prestonvanloon May 19, 2020

nisdas May 19, 2020

nisdas left a comment

Command to fetch validator statuses + MultipleValidatorStatus #5784

Command to fetch validator statuses + MultipleValidatorStatus #5784

Conversation

michaelhly commented May 8, 2020 • edited

Choose a reason for hiding this comment

michaelhly May 10, 2020 • edited

Choose a reason for hiding this comment

codecov bot commented May 11, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

rauljordan commented May 19, 2020

terencechain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nisdas left a comment

Choose a reason for hiding this comment

michaelhly commented May 8, 2020 •

edited

michaelhly May 10, 2020 •

edited

codecov bot commented May 11, 2020 •

edited