Aggregator selection from RPC to validator client #4071

terencechain · 2019-11-20T22:12:04Z

Part of #3865

This PR implements aggregator selection for RPC server and hardness the aggregator connection between beacon node and validator client. There's no breaking logic changes. Validator client simply queries beacon node on per slot basis to check if it's an aggregator. You should see these 2 logs if the validator is also an aggregator

Beacon node:

INFO rpc/aggregator: Broadcasting aggregated attestation and proof committeeIndex=0 slot=3 validatorIndex=30

Validator client:

INFO validator: Assigned and submitted aggregation and proof request committeeIndex=0 pubKey=0x82cd058276c7 slot=3

Change list:

Added AttestationsBySlotCommittee, it returns to-be-aggregated attestations based on slot and committee
Started rpc/aggregator/server.go, it has one RPC call SubmitAggregateAndProof, it's for validator to submit slot signature, and check if the validator is an aggregator then broadcast aggregation signature and proof
Started /client/validator_aggregate.go, it implements SubmitAggregateAndProof, the validator computes slot signature to send it to beacon node's aggregator server
Added PB definitions for service AggregatorService and call SubmitAggregateAndProof

Context for reviewers: https://github.com/ethereum/eth2.0-specs/blob/dev/specs/validator/0_beacon-chain-validator.md#attestation-aggregation

codecov · 2019-11-20T22:57:49Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #4071   +/-   ##
=======================================
  Coverage   57.99%   57.99%           
=======================================
  Files         203      203           
  Lines       13042    13042           
=======================================
  Hits         7564     7564           
  Misses       4398     4398           
  Partials     1080     1080

rauljordan

Comin' along! I'm excited for this feature

rauljordan · 2019-11-20T23:02:01Z

beacon-chain/operations/attestation.go

@@ -137,6 +138,23 @@ func (s *Service) AttestationPoolNoVerify(ctx context.Context) ([]*ethpb.Attesta
 	return atts, nil
 }

+// AttestationsBySlotCommittee returns the attestations from the attestations pool that's filtered


Suggested change

// AttestationsBySlotCommittee returns the attestations from the attestations pool that's filtered

// AttestationsBySlotCommittee returns the attestations from the attestations poo filtered

rauljordan · 2019-11-20T23:02:32Z

beacon-chain/operations/attestation.go

+// AttestationsBySlotCommittee returns the attestations from the attestations pool that's filtered
+// by slot and committee index.
+func (s *Service) AttestationsBySlotCommittee(ctx context.Context, slot uint64, index uint64) ([]*ethpb.Attestation, error) {
+	s.attestationPoolLock.Lock()


Can this be a read lock?

rauljordan · 2019-11-20T23:03:02Z

beacon-chain/rpc/aggregator/server.go

+
+// SubmitAggregateAndProof is called by a validator at every slot to check whether
+// it's assigned to be an aggregator. If yes, server will broadcast aggregated attestation
+// and proof on the validators behave.


Suggested change

// and proof on the validators behave.

// and proof regarding the validators' behavior.

rauljordan · 2019-11-20T23:03:43Z

beacon-chain/rpc/aggregator/server.go

+	}
+
+	validatorIndex, exists, err := as.BeaconDB.ValidatorIndex(ctx, bytesutil.ToBytes48(req.PublicKey))
+	if err != nil || !exists {


These should be two separate if's. One should throw an internal code error if err != nil and the other should through NotFound if !exists

…aggregator

prestonvanloon

Needs tests.

Is there a E2E evaluator you can add to test this logic at runtime?
Maybe in a follow up PR, but it would be nice to see this tested E2E in addition to the unit tests.

beacon-chain/operations/attestation.go

beacon-chain/rpc/aggregator/server.go

prestonvanloon · 2019-11-21T18:04:37Z

validator/client/validator_aggregate.go

+// via gRPC. Beacon node will verify the slot signature and determine if the validagtor is also
+// an aggregator. If yes, then beacon node will broadcast aggregated signature and
+// proof on the validator's behave.
+func (v *validator) SubmitAggregateAndProof(ctx context.Context, slot uint64, pubKey [48]byte) {


Please add a test

terencechain · 2019-11-21T19:01:30Z

Needs tests.

Is there a E2E evaluator you can add to test this logic at runtime?
Maybe in a follow up PR, but it would be nice to see this tested E2E in addition to the unit tests.

definitely, that is coming

prestonvanloon · 2019-11-21T19:06:47Z

validator/client/runner.go

 					switch role {
 					case pb.ValidatorRole_BOTH:
 						go v.SubmitAttestation(slotCtx, slot, id)
+						v.SubmitAggregateAndProof(ctx, slot, id)


Can this be in a go routine so it doesn't block proposing a block?

Also, wouldn't this be part of SubmitAttestation?

Ha. Yeah. Was going to add it next PR when we implement the logic to Wait till 3/4 of the slot and submit aggregated attestation and proof I'll add it now :)

prestonvanloon · 2019-11-21T19:12:43Z

proto/beacon/rpc/v1/services.proto

@@ -16,6 +16,10 @@ service ProposerService {
  rpc ProposeBlock(ethereum.eth.v1alpha1.BeaconBlock) returns (ProposeResponse);
 }

+service AggregatorService {


Why create a new service? Shouldn't this be part of the validator service?

I think Aggregator should have its own role (ie. similar to proposer and attester). It makes it more extensible as the aggregating spec gets more complicated and we adopt real-life aggregation protocol. Another reason is aggregator perform time based role (ie. submits signed slot signature when it's 2/3 of the slot)

prestonvanloon · 2019-11-21T19:13:38Z

beacon-chain/rpc/aggregator/server.go

+		return nil, status.Errorf(codes.Internal, "Could not get validator index from DB: %v", err)
+	}
+	if !exists {
+		return nil, errors.New("could not locate validator index in DB")


status.Error?

prestonvanloon · 2019-11-21T19:14:41Z

beacon-chain/rpc/aggregator/server.go

+	SyncChecker sync.Checker
+}
+
+// SubmitAggregateAndProof is called by a validator at every slot to check whether


Every slot? Is there no way to determine this in assignments or at epoch level?

Updated comment. Should be every slot if you are part of the committee

prestonvanloon · 2019-11-21T19:15:00Z

beacon-chain/rpc/aggregator/server.go

+			"slot":           req.Slot,
+			"validatorIndex": validatorIndex,
+			"committeeIndex": req.CommitteeIndex,
+		}).Info("Broadcasting aggregated attestation and proof")


What broadcasting? This doesn't broadcast anything

This PR is just for selection. Will handle broadcast next. Updated log to reflect that

prestonvanloon · 2019-11-21T19:15:30Z

shared/params/config.go

@@ -246,6 +246,7 @@ func MinimalSpecConfig() *BeaconChainConfig {
 	minimalConfig.ShuffleRoundCount = 10
 	minimalConfig.MinGenesisActiveValidatorCount = 64
 	minimalConfig.MinGenesisTime = 0
+	minimalConfig.TargetAggregatorsPerCommittee = 1


Why 1? Just wondering

For 64 validators, you get 4 validators per committee. 1 is the easiest number to see it work live given the modulus selection approach

prestonvanloon · 2019-11-21T19:16:31Z

validator/client/runner.go

 					switch role {
 					case pb.ValidatorRole_BOTH:
 						go v.SubmitAttestation(slotCtx, slot, id)
+						v.SubmitAggregateAndProof(ctx, slot, id)


Also, wouldn't this be part of SubmitAttestation?

prestonvanloon · 2019-11-21T19:17:07Z

validator/client/validator_aggregate.go

+			"committeeIndex": assignment.CommitteeIndex,
+			"pubKey":         fmt.Sprintf("%#x", bytesutil.Trunc(pubKey[:])),
+		}).Info("Assigned and submitted aggregation and proof request")
+	}


It feels like something is missing here...

validator/client/validator_aggregate.go

prestonvanloon · 2019-11-21T19:40:29Z

Additional context for reviewers: https://github.com/ethereum/eth2.0-specs/blob/dev/specs/validator/0_beacon-chain-validator.md#attestation-aggregation

prestonvanloon · 2019-11-21T19:43:23Z

validator/client/runner.go

 						v.ProposeBlock(slotCtx, slot, id)
 					case pb.ValidatorRole_ATTESTER:
+						v.SubmitAggregateAndProof(ctx, slot, id)


If both of these wait until different times in the slot, you're going to have problems unless they run in goroutines

Agree. Switched to go routines

Co-Authored-By: Preston Van Loon <preston@prysmaticlabs.com>

…iclabs/prysm into refactor-validator-roles

…aggregator

prestonvanloon · 2019-11-22T03:54:03Z

beacon-chain/rpc/aggregator/server.go

+	SyncChecker sync.Checker
+}
+
+// SubmitAggregateAndProof is called by a validator at every slot if you are part of the committee


Please update comment

* Config * Updated proto * Updated pool * Updated RPC * Updated validator client * run time works * Clean ups * Fix tests * Visibility * Merge branch 'master' of https://github.com/prysmaticlabs/prysm into aggregator * Raul's feedback * Tests for RPC server * Tests for validator client * Span * More tests * Use go routine for SubmitAggregateAndProof * Go routines * Updated comments * Use array of roles * Fixed tests * Build * Update validator/client/runner.go Co-Authored-By: Preston Van Loon <preston@prysmaticlabs.com> * Update validator/client/runner.go Co-Authored-By: Preston Van Loon <preston@prysmaticlabs.com> * If * Merge branch 'refactor-validator-roles' of https://github.com/prysmaticlabs/prysm into refactor-validator-roles * Empty * Feedback * Merge branch 'master' of https://github.com/prysmaticlabs/prysm into aggregator * Removed proto/eth/v1alpha1/shard_chain.pb.go? * Cleaned up * Revert * Comments * Lint * Comment * Merge branch 'master' into aggregator

terencechain added 8 commits November 20, 2019 12:34

Config

db40f03

Updated proto

bda471d

Updated pool

004ea57

Updated RPC

6e44e5e

Updated validator client

d173008

run time works

0f314b0

Clean ups

88ae4a9

Fix tests

b9f5444

terencechain changed the title ~~Aggregator part 1~~ Aggregator selection from RPC to validator client Nov 20, 2019

terencechain self-assigned this Nov 20, 2019

Visibility

c4c1c86

rauljordan reviewed Nov 20, 2019

View reviewed changes

terencechain added 2 commits November 21, 2019 06:36

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

31604b3

…aggregator

Raul's feedback

52e9f84

prestonvanloon requested changes Nov 21, 2019

View reviewed changes

terencechain added 4 commits November 21, 2019 10:24

Tests for RPC server

d7ed01c

Tests for validator client

394bcfe

Span

4db47de

More tests

d8b1e69

terencechain added the Ready For Review A pull request ready for code review label Nov 21, 2019

prestonvanloon reviewed Nov 21, 2019

View reviewed changes

Use go routine for SubmitAggregateAndProof

1590620

prestonvanloon requested changes Nov 21, 2019

View reviewed changes

prestonvanloon reviewed Nov 21, 2019

View reviewed changes

terencechain added 3 commits November 21, 2019 11:47

Go routines

0f976c0

Updated comments

9bcc35f

Use array of roles

5ea3b1d

terencechain and others added 14 commits November 21, 2019 13:36

Fixed tests

ef8d8e1

Build

7e89c7c

Update validator/client/runner.go

0f7ce7a

Co-Authored-By: Preston Van Loon <preston@prysmaticlabs.com>

Update validator/client/runner.go

4c4384f

Co-Authored-By: Preston Van Loon <preston@prysmaticlabs.com>

If

6a0b639

Merge branch 'refactor-validator-roles' of https://github.com/prysmat…

b1144ae

…iclabs/prysm into refactor-validator-roles

Empty

952c29a

Feedback

86bc69a

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

3582620

…aggregator

Removed proto/eth/v1alpha1/shard_chain.pb.go?

2665342

Cleaned up

125e66e

Revert

5e52f78

Comments

9a90a65

Lint

b13c112

prestonvanloon reviewed Nov 22, 2019

View reviewed changes

Comment

9947b89

prestonvanloon approved these changes Nov 22, 2019

View reviewed changes

Merge branch 'master' into aggregator

04ffcdd

terencechain added the OK to merge label Nov 22, 2019

prylabs-bulldozer bot merged commit f5cb040 into master Nov 22, 2019

delete-merged-branch bot deleted the aggregator branch November 22, 2019 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggregator selection from RPC to validator client #4071

Aggregator selection from RPC to validator client #4071

terencechain commented Nov 20, 2019 •

edited

codecov bot commented Nov 20, 2019 •

edited

rauljordan left a comment

rauljordan Nov 20, 2019

rauljordan Nov 20, 2019

terencechain Nov 21, 2019

rauljordan Nov 20, 2019

rauljordan Nov 20, 2019

prestonvanloon left a comment

prestonvanloon Nov 21, 2019

terencechain commented Nov 21, 2019

prestonvanloon Nov 21, 2019

prestonvanloon Nov 21, 2019

terencechain Nov 21, 2019

prestonvanloon Nov 21, 2019

terencechain Nov 22, 2019

prestonvanloon Nov 22, 2019

prestonvanloon Nov 21, 2019

terencechain Nov 21, 2019

prestonvanloon Nov 21, 2019 •

edited

terencechain Nov 21, 2019

prestonvanloon Nov 21, 2019

terencechain Nov 21, 2019

prestonvanloon Nov 21, 2019

terencechain Nov 21, 2019

prestonvanloon Nov 21, 2019

prestonvanloon Nov 21, 2019

prestonvanloon commented Nov 21, 2019

prestonvanloon Nov 21, 2019

terencechain Nov 21, 2019

prestonvanloon Nov 22, 2019

	// AttestationsBySlotCommittee returns the attestations from the attestations pool that's filtered
	// AttestationsBySlotCommittee returns the attestations from the attestations poo filtered

	// and proof on the validators behave.
	// and proof regarding the validators' behavior.

Aggregator selection from RPC to validator client #4071

Aggregator selection from RPC to validator client #4071

Conversation

terencechain commented Nov 20, 2019 • edited

codecov bot commented Nov 20, 2019 • edited

Codecov Report

rauljordan 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

prestonvanloon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain commented Nov 21, 2019

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 Nov 21, 2019 • edited

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 commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain commented Nov 20, 2019 •

edited

codecov bot commented Nov 20, 2019 •

edited

prestonvanloon Nov 21, 2019 •

edited