Monitor service #9933

potuz · 2021-11-24T13:18:43Z

This PR implements a validator monitor within the beacon node. It adds a feature flag --monitor-indices to the beacon-chain command taking a list of validator indices (not necessarily connected to the beacon). The beacon then logs messages and emits metrics in the following events

A beacon block is proposed by a tracked validator
A beacon block includes an attestation by a tracked validator
A beacon block includes a slashing of a tracked validator
A beacon block includes a voluntary exit of a tracked validator
A beacon block includes a sync-committee contribution by a tracked validator
An unaggregated attestation by a tracked validator was processed by the beacon node
An aggregated attestation by a tracked validator was processed by the beacon node
An aggregate attestation where the aggregator was a tracked validator, was processed by the beacon node
A voluntary exit by a tracked validator was processed by the beacon node
It also keeps a structure for aggregated performance since launch, but currently it does not log them

Collaboration with @terencechain

Notes for reviewers:

context and cancel usage in this service may be completely bogus and unnecessary.

beacon-chain/monitor/service.go

rkapka · 2021-11-30T12:49:17Z

beacon-chain/monitor/service.go

+	log.WithFields(logrus.Fields{
+		"ValidatorIndices": tracked,
+	}).Info("Started service")
+	s.isRunning = false


We log out that the service started, but s.isRunning = false. I find this a bit contradictory.

The service starts, but it is not logging anything until the beacon syncs. The error message on localhost:3500/healtz is "not running". I don't mind changing this though.

+1 to Radek's comment

I changed the log to "Starting service" and the boolean to "isLogging", would that be more accurate?

beacon-chain/monitor/service.go

rkapka · 2021-11-30T13:02:18Z

beacon-chain/monitor/service.go

+		trackedSyncCommitteeIndices: make(map[types.ValidatorIndex][]types.CommitteeIndex),
+	}
+	for _, idx := range tracked {
+		r.TrackedValidators[idx] = nil


This map is not filled with real data in this PR. I presume it's done somewhere else?

The fact that the key is there is used to check that the idx is tracked.

Instead of assigning nil, why don't we assign the actual struct

You mean one of the performance structures? because those take a long time to be available, this would block service start until we are fully synced at the minimum.

I think we should just change make(map[types.ValidatorIndex]interface{} to make(map[types.ValidatorIndex]bool. bool is used most of the time in prysm for tracking existence

beacon-chain/node/node.go

terencechain

Required changes:

Feedbacks with start function (ie refactor, go routine... etc)

Optional changes:

Tests

terencechain · 2021-11-30T16:06:44Z

beacon-chain/monitor/service.go

+func (s *Service) Start() {
+	tracked := make([]types.ValidatorIndex, len(s.TrackedValidators))
+	i := 0
+	for idx := range s.TrackedValidators {


for i:=0; i < len(s.TrackedValidators ).... is better for this imo

I'm sorry didn't get this, how do I grab the keys from the map?

terencechain · 2021-11-30T16:07:31Z

beacon-chain/monitor/service.go

+	log.WithFields(logrus.Fields{
+		"ValidatorIndices": tracked,
+	}).Info("Started service")
+	s.isRunning = false


+1 to Radek's comment

terencechain · 2021-11-30T16:09:33Z

beacon-chain/monitor/service.go

+		if err := s.waitForSync(stateChannel, stateSub); err != nil {
+			log.WithError(err)
+			return
+		}
+		state, err := s.config.HeadFetcher.HeadState(s.ctx)
+		if err != nil {
+			log.WithError(err).Error("Could not get head state")
+			return
+		}
+		if state == nil {
+			log.Error("Head state is nil")
+			return
+		}
+		epoch := slots.ToEpoch(state.Slot())
+		log.WithField("Epoch", epoch).Debug("Synced to head epoch, starting reporting performance")
+
+		s.Lock()
+		for idx := range s.TrackedValidators {
+			balance, err := state.BalanceAtIndex(idx)
+			if err != nil {
+				log.WithError(err).WithField("ValidatorIndex", idx).Error(
+					"Could not fetch starting balance, skipping aggregated logs.")
+				balance = 0
+			}
+			s.aggregatedPerformance[idx] = ValidatorAggregatedPerformance{
+				startEpoch:   epoch,
+				startBalance: balance,
+			}
+			s.latestPerformance[idx] = ValidatorLatestPerformance{
+				balance: balance,
+			}
+		}
+		s.Unlock()
+		s.updateSyncCommitteeTrackedVals(state)
+		s.Lock()
+		s.isRunning = true
+		s.Unlock()


I dont think these should be in the background routine.

I couldn't solve the tests without sending these to the background, there are lots of races when subscribing to the channels.

terencechain · 2021-11-30T16:11:53Z

beacon-chain/monitor/service.go

+		trackedSyncCommitteeIndices: make(map[types.ValidatorIndex][]types.CommitteeIndex),
+	}
+	for _, idx := range tracked {
+		r.TrackedValidators[idx] = nil


Instead of assigning nil, why don't we assign the actual struct

terencechain · 2021-11-30T16:13:10Z

beacon-chain/monitor/service.go

+		for idx := range s.TrackedValidators {
+			balance, err := state.BalanceAtIndex(idx)
+			if err != nil {
+				log.WithError(err).WithField("ValidatorIndex", idx).Error(
+					"Could not fetch starting balance, skipping aggregated logs.")
+				balance = 0
+			}
+			s.aggregatedPerformance[idx] = ValidatorAggregatedPerformance{
+				startEpoch:   epoch,
+				startBalance: balance,
+			}
+			s.latestPerformance[idx] = ValidatorLatestPerformance{
+				balance: balance,
+			}
+		}


Consider refactoring this into its own helper

Nice, moving this to it's own helper allows me to add an easy test for it.

terencechain · 2021-11-30T16:13:54Z

beacon-chain/monitor/service.go

+	log.WithFields(logrus.Fields{
+		"ValidatorIndices": tracked,
+	}).Info("Started service")


Should we log this at the end?

In my runtime tests it was reassuring getting the confirmation at launch that I was tracking the right set of validators. Vide the reply above about changing this message.

beacon-chain/monitor/service.go

terencechain

💯

terencechain · 2021-11-30T17:21:10Z

beacon-chain/monitor/service.go

+		trackedSyncCommitteeIndices: make(map[types.ValidatorIndex][]types.CommitteeIndex),
+	}
+	for _, idx := range tracked {
+		r.TrackedValidators[idx] = nil


I think we should just change make(map[types.ValidatorIndex]interface{} to make(map[types.ValidatorIndex]bool. bool is used most of the time in prysm for tracking existence

terencechain · 2021-11-30T21:50:23Z

beacon-chain/monitor/process_block.go

@@ -35,6 +35,7 @@ func (s *Service) processBlock(ctx context.Context, b block.SignedBeaconBlock) {
 	}
 	state := s.config.StateGen.StateByRootIfCachedNoCopy(root)
 	if state == nil {
+		log.Info("Pingo")


potuz added 2 commits November 24, 2021 08:59

Add a service for the monitor

614c1b5

Do not block service start

1347a7b

potuz requested a review from a team as a code owner November 24, 2021 13:18

potuz requested review from rauljordan, prestonvanloon and terencechain November 24, 2021 13:18

potuz added 3 commits November 24, 2021 10:19

gaz

a94e36a

move channel subscription outide go routine

de6cea2

add service start test

db6bc61

potuz added the Ready For Review A pull request ready for code review label Nov 24, 2021

fix panic on node tests

6cf228a

potuz mentioned this pull request Nov 27, 2021

Validator monitor #9882

Closed

5 tasks

rkapka reviewed Nov 30, 2021

View reviewed changes

potuz added 2 commits November 30, 2021 10:48

Radek's first pass

13074f0

Radek's take 2

fb6c18a

rkapka previously approved these changes Nov 30, 2021

View reviewed changes

potuz dismissed rkapka’s stale review via 0f2ce65 November 30, 2021 15:37

uncap error messages

15f91ea

potuz force-pushed the monitor_service branch from 0f2ce65 to 15f91ea Compare November 30, 2021 15:39

revert reversal

e3b7b15

terencechain requested changes Nov 30, 2021

View reviewed changes

potuz and others added 9 commits November 30, 2021 13:57

Terence take 1

052696f

gaz

4fea849

Missing locks found by Terence

a943edb

Track via bool not empty interface

2fa1ca8

Bool for existence

520f0e8

Add tests for every function

edaf3e0

fix allocation of slice

c7dd937

Minor cleanups

57a15ef

Merge branch 'develop' into monitor_service

32f0a67

terencechain approved these changes Nov 30, 2021

View reviewed changes

potuz merged commit afbe026 into develop Nov 30, 2021

delete-merged-branch bot deleted the monitor_service branch November 30, 2021 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Monitor service #9933

Monitor service #9933

potuz commented Nov 24, 2021 •

edited

rkapka Nov 30, 2021

potuz Nov 30, 2021

terencechain Nov 30, 2021

potuz Nov 30, 2021

rkapka Nov 30, 2021

potuz Nov 30, 2021

terencechain Nov 30, 2021

potuz Nov 30, 2021

terencechain Nov 30, 2021

terencechain left a comment

terencechain Nov 30, 2021

potuz Nov 30, 2021

terencechain Nov 30, 2021

terencechain Nov 30, 2021

potuz Nov 30, 2021

terencechain Nov 30, 2021

terencechain Nov 30, 2021

potuz Nov 30, 2021

terencechain Nov 30, 2021

potuz Nov 30, 2021

terencechain left a comment

terencechain Nov 30, 2021

terencechain Nov 30, 2021

Monitor service #9933

Monitor service #9933

Conversation

potuz commented Nov 24, 2021 • edited

Notes for reviewers:

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

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

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

potuz commented Nov 24, 2021 •

edited