Update Attester to Use Beacon Node RPC, Replacing SMC Completely #365

rauljordan · 2018-08-01T22:32:53Z

Hi all,

This PR is part of the greater effort of #315 and we are beginning with updating the attester package in our client project to interact with the beacon chain completely via RPC for shuffling and determining attestation height responsibility. This eliminates the need for the Sharding Manager Contract and completely deprecates it within the attester package. Similar work will need to be done for the proposer package.

This PR ensures attester is aligned with the latest beacon chain + sharding specification.

Design Decisions

This package defines server-side RPC methods for the beacon chain and leverages gomock for testing the client side requests to these methods. The attester service has two responsibilities:

Listen for the latest crystallized state from a beacon node via gRPC stream, use the active validator list, cutoffs, and more to determine the height the attester should perform its responsibility
Listen for the latest canonical beacon block from a beacon node via a gRPC stream, check the height against the assigned attestation height from step 1 and perform an attestation

Mocks

This package leverages gRPC mocks through gomock and mockgen for testing. We check in the mocks into client/internal for now until we are able to generate them with Bazel. To regenerate the mocks, run

mockgen github.com/prysmaticlabs/prysm/proto/beacon/rpc/v1 BeaconServiceClient,BeaconService_LatestBeaconBlockClient,BeaconService_LatestCrystallizedStateClient > ./client/internal/beacon_service_mock.go

Requirements for Merge

Tests
Fetching Beacon Block Height + Hash Via RPC Stream
Fetch Crystallized State and Determine Attester Assigned Height via RPC

Use ethereum prefix for proto package

prestonvanloon · 2018-08-08T02:42:17Z

client/BUILD.bazel

@@ -18,34 +16,6 @@ go_library(
    ],
 )

-go_image(


Why did you remove the docker image support for the client?

Please revert this change

prestonvanloon · 2018-08-08T02:46:06Z

client/attester/service.go

+		activeValidators := crystallizedState.GetActiveValidators()
+		validatorCount := len(activeValidators)
+
+		validatorIndexSet := false


Please change to a Boolean like name: isValidatorIndexSet.

Otherwise I think of this as a set of validator indices.

prestonvanloon · 2018-08-08T02:50:34Z

client/attester/service.go

+		for i, val := range activeValidators {
+			// TODO: Check the public key instead of withdrawal address. This will
+			// use BLS.
+			if bytes.Equal(val.GetWithdrawalAddress(), []byte{}) {


Can you create a constant for this so it’s been easier to understand?

var ZERO_ADDRESS = []byte{}

Or something like that

And/or maybe a helper on ValidatorRecord IsWithdrawalAddressSet()

prestonvanloon · 2018-08-08T02:53:22Z

client/attester/service.go

+	}
+	for {
+		block, err := stream.Recv()
+		if err == io.EOF {


Please add comment for clarity. This means the stream was closed but it is not intuitive.

prestonvanloon · 2018-08-08T02:54:54Z

client/attester/service.go

+		// its responsibilities.
+
+		if at.isHeightAssigned && block.GetSlotNumber() == at.assignedHeight {
+			log.Info("Assigned attestation height reached, performing attestation responsibility")


Should there be a TODO here?

prestonvanloon · 2018-08-08T02:58:52Z

beacon-chain/rpc/service.go

@@ -67,8 +71,8 @@ func (s *Service) Stop() error {
 }

 // ShuffleValidators shuffles the validators into attesters/proposers.
-func (s *Service) ShuffleValidators(req *pb.ShuffleRequest, stream pb.BeaconService_ShuffleValidatorsServer) error {
-	return stream.Send(&pb.ShuffleResponse{IsProposer: true, IsAttester: false})
+func (s *Service) ShuffleValidators(ctx context.Context, req *pb.ShuffleRequest) (*pb.ShuffleResponse, error) {


So why this no longer a stream? Are validators shuffled on demand or periodically? Doesn't a client want a stream of events?

Could you add some more detail on what this service method is supposed to do?

prestonvanloon · 2018-08-08T03:01:03Z

client/attester/service.go

 		return
 	}
+	for {
+		crystallizedState, err := stream.Recv()
+		if err == io.EOF {


Can you add a comment here to clarify that this is a closed stream.

prestonvanloon · 2018-08-08T03:02:18Z

client/rpcclient/service.go

+		log.Fatalf("Could not parse endpoint URL: %s, %v", s.endpoint, err)
+		return
+	}
+	conn, err := grpc.Dial(fmt.Sprintf("localhost:%s", endpointURL.Port()), grpc.WithInsecure())


prestonvanloon

Why move Travis file?

prestonvanloon

Also, please do not generate a mock with the_test.go suffix. Otherwise this leaks into the production binary.

prestonvanloon · 2018-08-08T11:39:25Z

client/rpcclient/service.go

@@ -36,9 +38,14 @@ func NewRPCClient(ctx context.Context, cfg *Config) *Service {
 // Start the grpc connection.
 func (s *Service) Start() {
 	log.Info("Starting service")
-	conn, err := grpc.Dial(s.endpoint, grpc.WithInsecure())
+	endpointURL, err := url.Parse(s.endpoint)


Is this necessary?

codecov · 2018-08-09T15:44:37Z

Codecov Report

Merging #365 into master will increase coverage by 2.96%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##           master     #365      +/-   ##
==========================================
+ Coverage   49.15%   52.12%   +2.96%     
==========================================
  Files          35       34       -1     
  Lines        2836     2826      -10     
==========================================
+ Hits         1394     1473      +79     
+ Misses       1245     1163      -82     
+ Partials      197      190       -7

Impacted Files	Coverage Δ
beacon-chain/utils/shuffle.go	`100% <100%> (ø)`	⬆️
beacon-chain/node/node.go	`40.17% <100%> (ø)`	⬆️
client/attester/service.go	`100% <100%> (+100%)`	⬆️
beacon-chain/rpc/service.go	`100% <100%> (+31.7%)`	⬆️
beacon-chain/blockchain/core.go	`65.78% <100%> (-1.06%)`	⬇️
client/node/node.go	`36.49% <14.28%> (+0.46%)`	⬆️
beacon-chain/blockchain/service.go	`22% <28.57%> (-0.69%)`	⬇️
client/rpcclient/service.go	`87.09% <50%> (+29.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 390ac62...9d2e409. Read the comment docs.

prestonvanloon

LGTM

I was hesitant about putting the mocks in internal as it might be used in production code, but that target is marked as test only so I am happy.

Thanks

prestonvanloon · 2018-08-09T21:08:14Z

beacon-chain/rpc/service_test.go

+func TestRPCMethods(t *testing.T) {
+	s := &Service{}
+	if _, err := s.FetchShuffledValidatorIndices(context.Background(), nil); err == nil {
+		t.Error("Wanted error: unimplemented, received nil")


This is OK, but a better pattern is to check that the error returned was the error you expect.

This assertion is that any error returns.

This will be changed very soon once we implement the logic of the beacon RPC server

terencechain

just one typo, we can fix it in the next pr.

terencechain · 2018-08-09T22:44:58Z

beacon-chain/rpc/service.go

+// FetchShuffledValidatorIndices retrieves the shuffled validator indices, cutoffs, and
+// assigned attestation heights at a given crystallized state hash.
+// This function can be called by clients to fetch a historical list of shuffled
+// validators ata point in time corresponding to a certain crystallized state.


rauljordan and others added 30 commits July 31, 2018 12:05

proto beacon RPC messages

e971dd5

rename protos, fix import paths

444b9fd

check in readme

688c318

added proto service def

8608635

bazel run

356b298

Merge branch 'rpc-services' into beacon-rpc

ae18249

added services

b38202e

Merge branch 'master' into beacon-rpc

e2e511b

stream response

d21462a

rpc server

a8e6b57

Merge branch 'beacon-rpc' into beacon-rpc-server

bec0389

define methods on beacon RPC service

ba869f7

built

b935a6e

fix proto package

8fe228b

Merge pull request #2 from prestonvanloon/beacon-rpc

08034db

Use ethereum prefix for proto package

fixed all confs

96d3a76

modified proto gens

56877f2

Merge branch 'beacon-rpc' into beacon-rpc-server

bef0c7e

mod

9950846

Merge branch 'master' into beacon-rpc-server

66a4b72

added RPC server with lifecycle tests

ac50ff3

Merge branch 'master' into beacon-rpc-server

22f2f04

fix

b8f3852

all tests pass

83b3bff

address feedback

6ea245b

comments

09583be

mod proto

8f272ae

rpc port flag

8215acd

Merge branch 'beacon-rpc-server' into replace-smc

28073ce

replacing attester with beacon RPC methods

227b2d3

prestonvanloon requested changes Aug 8, 2018

View reviewed changes

rauljordan added 2 commits August 7, 2018 23:53

address preston comments

5c692a9

gomock controller

a3b5cc4

prestonvanloon reviewed Aug 8, 2018

View reviewed changes

rauljordan added 8 commits August 8, 2018 16:37

gomock

8b900fa

gomock recv

31a9008

expectd num calls

c6802e8

lifecycle test

a023315

coverage, tests fetch crystallized state, mockgen regen

9e72c6f

put travis back in top folder

506b474

merge master

2576dc4

fully covered fetch latest beacon block

02493fc

rauljordan added 2 commits August 9, 2018 13:04

100% code coverage

aee8a9a

todo implement shuffled

164fd7f

rauljordan removed the In Progress label Aug 9, 2018

rauljordan added 5 commits August 9, 2018 14:07

100% diff coverage

e3560db

merge master

9b7cf43

add todo

a586ba9

coverage

e471554

gazelle fix

9d2e409

prestonvanloon approved these changes Aug 9, 2018

View reviewed changes

rauljordan merged commit cd90598 into prysmaticlabs:master Aug 9, 2018

terencechain reviewed Aug 9, 2018

View reviewed changes

terencechain mentioned this pull request Aug 9, 2018

Deprecate SMC, Put SMC in Contract Repo and Update Readme #392

Closed

rauljordan mentioned this pull request Aug 10, 2018

Update Proposer to Use Beacon RPC, Remove SMC Dependency #393

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Attester to Use Beacon Node RPC, Replacing SMC Completely #365

Update Attester to Use Beacon Node RPC, Replacing SMC Completely #365

rauljordan commented Aug 1, 2018 •

edited

Loading

prestonvanloon Aug 8, 2018

prestonvanloon Aug 8, 2018

prestonvanloon Aug 8, 2018

prestonvanloon Aug 8, 2018

prestonvanloon Aug 8, 2018

prestonvanloon Aug 8, 2018

prestonvanloon Aug 8, 2018

prestonvanloon Aug 8, 2018

prestonvanloon left a comment

prestonvanloon left a comment

prestonvanloon Aug 8, 2018

codecov bot commented Aug 9, 2018 •

edited

Loading

prestonvanloon left a comment

prestonvanloon Aug 9, 2018

rauljordan Aug 9, 2018

terencechain left a comment

terencechain Aug 9, 2018

Update Attester to Use Beacon Node RPC, Replacing SMC Completely #365

Update Attester to Use Beacon Node RPC, Replacing SMC Completely #365

Conversation

rauljordan commented Aug 1, 2018 • edited Loading

Design Decisions

Mocks

Requirements for Merge

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

prestonvanloon left a comment

Choose a reason for hiding this comment

prestonvanloon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 9, 2018 • edited Loading

Codecov Report

prestonvanloon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauljordan commented Aug 1, 2018 •

edited

Loading

codecov bot commented Aug 9, 2018 •

edited

Loading