core/tracker: implement analyse cluster participation #846

dB2510 · 2022-07-23T11:44:31Z

Implements analyse cluster participation and participation reporter to calculate cluster participation based on partial signatures exchanged and instruments the results.

category: feature
ticket: #821

codecov · 2022-07-23T11:47:56Z

Codecov Report

Merging #846 (eb45b19) into main (7e5431c) will decrease coverage by 0.45%.
The diff coverage is 59.61%.

@@            Coverage Diff             @@
##             main     #846      +/-   ##
==========================================
- Coverage   55.30%   54.85%   -0.46%     
==========================================
  Files         111      112       +1     
  Lines       11539    11666     +127     
==========================================
+ Hits         6382     6399      +17     
- Misses       4240     4328      +88     
- Partials      917      939      +22

Impacted Files	Coverage Δ
p2p/peer.go	`48.14% <0.00%> (-3.86%)`	⬇️
core/tracker/tracker.go	`70.40% <62.00%> (-3.97%)`	⬇️
core/qbft/qbft.go	`71.67% <0.00%> (-10.31%)`	⬇️
app/app.go	`54.86% <0.00%> (-4.74%)`	⬇️
core/aggsigdb/memory.go	`79.62% <0.00%> (-0.93%)`	⬇️
core/priority/calculate.go	`70.78% <0.00%> (ø)`
core/dutydb/memory.go	`71.91% <0.00%> (+1.27%)`	⬆️

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 7e5431c...eb45b19. Read the comment docs.

corverroos · 2022-07-24T17:16:55Z

core/tracker/metrics.go

+	"github.com/prometheus/client_golang/prometheus/promauto"
+)
+
+var participationGauge = promauto.NewGaugeVec(prometheus.GaugeOpts{


Namespace: core
Subsystem: tracker
Name: duty_participation (or just participation)

Align label to "duty", "peer" and "pubkey".

corverroos · 2022-07-24T17:17:51Z

core/tracker/tracker.go

@@ -42,11 +46,15 @@ const (
 	sentinel
 )

+// shareIdx represents a peer's share index.
+type shareIdx int


rather not introduce this type, naming var shareIdx int is what we use through the rest of the codebase.

yeah, i introduced this type to avoid any confusion with int and shareIdx since shareIdx is used in this file very extensively

corverroos · 2022-07-24T17:19:25Z

core/tracker/tracker.go

+}
+
+// newParticipationReporter returns a new participation reporter function which logs and instruments peer participation
+func newParticipationReporter(peers []p2p.Peer) func(context.Context, core.Duty, map[core.PubKey]map[shareIdx]bool, map[core.PubKey]map[shareIdx]bool) {


internalise state my keeping last reported state inside this function, do not require it to kept outside

corverroos · 2022-07-24T17:21:03Z

core/tracker/tracker.go

+			var absentPeers []string
+			for _, peer := range peers {
+				// Peer index is 0 indexed while shareIdx is 1 indexed.
+				if dvPeers[shareIdx(peer.Index+1)] {


Add a ShareIdx method to Peer maybe

corverroos · 2022-07-24T17:23:39Z

core/tracker/tracker.go

+			}
+
+			// Avoid spamming from identical logs.
+			if len(absentPeers) > 0 && !reflect.DeepEqual(currentParticipation[pubKey], lastParticipation[pubKey]) {


best not use reflect in production code.

Also you should also log if some peers didn't participate last time, and all participated this time, e.g. "all peers participated".

Note this is still going to spam the logs if we have 1000 validators in a cluster.

corverroos · 2022-07-24T17:25:04Z

core/tracker/tracker.go

 // event represents an event emitted by a core workflow component.
 type event struct {
 	duty      core.Duty
 	component component
 	pubkey    core.PubKey
+	shareIdx  shareIdx


make a note that this is optional field only set for some component events and list them.

corverroos · 2022-07-24T17:26:42Z

core/tracker/tracker.go

+		eventsPerDV[e.pubkey] = append(eventsPerDV[e.pubkey], e)
+	}
+
+	for pubKey, dvEvents := range eventsPerDV {


I'm not sure about including pubkey in the whole analysis. Since we group pubkeys by duty, either all the pubkeys will be there that should be there for the peer or none will be there for the peer. It doesn't add any value. I would say we should exclude pubkey from this. Just do duty and peer, not pubkey.

corverroos · 2022-07-25T11:42:46Z

core/tracker/metrics.go

+	Namespace: "core",
+	Subsystem: "tracker",
+	Name:      "participation",
+	Help:      "Set to 1 if peer participated successfully for the given duty and Distributed Validator public key or else 0",


remove pubkey stuff from help

corverroos · 2022-07-25T11:43:10Z

core/tracker/tracker.go

@@ -47,6 +50,7 @@ type event struct {
 	duty      core.Duty
 	component component
 	pubkey    core.PubKey
+	shareIdx  int // This is an optional field only set by parSigEx and parSigDBInternal events.


and validatorapi

Also maybe add: ShareIdx is 1 indexed, so 0 indicates unset.

corverroos · 2022-07-25T11:44:40Z

p2p/peer.go

@@ -46,6 +46,11 @@ type Peer struct {
 	Name string
 }

+// ShareIdx returns share index of this Peer.
+func (p Peer) ShareIdx() int {
+	return p.Index + 1


add comment about 1-indexed

corverroos · 2022-07-25T11:45:26Z

core/tracker/tracker.go

 			t.failedDutyReporter(duty, failed, failedComponent.String(), failedMsg)

-			// TODO(dhruv): Case of cluster participation
-			// analyseParticipation(duty, t.events[duty])
+			currentParticipation := analyseParticipation(t.events[duty])


nit: participatedShares := ...

corverroos · 2022-07-25T11:46:50Z

core/tracker/tracker.go

+	for _, e := range events {
+		// If we get a parSigDBInternal event, then the current node participated successfully.
+		// If we get a parSigEx event, then the corresponding peer with e.shareIdx participated successfully.
+		if e.shareIdx > 0 && (e.component == parSigEx || e.component == parSigDBInternal) {


rather be defensive, error if shareIdx 0 for expected components.

corverroos · 2022-07-25T11:48:09Z

core/tracker/tracker.go

+// newParticipationReporter returns a new participation reporter function which logs and instruments peer participation.
+func newParticipationReporter(peers []p2p.Peer) func(context.Context, core.Duty, map[int]bool) {
+	// lastParticipation is the set of peers who participated in the last duty.
+	lastParticipation := make(map[int]bool)


nit: prevAbsent := make(...)

can just make this []string and compare against fmt.Sprint(absent) != fmt.Sprint(prevAbsent)

corverroos · 2022-07-25T11:48:52Z

core/tracker/tracker.go

+	// lastParticipation is the set of peers who participated in the last duty.
+	lastParticipation := make(map[int]bool)
+
+	return func(ctx context.Context, duty core.Duty, currentParticipation map[int]bool) {


nit: participatedShares

corverroos · 2022-07-25T11:57:40Z

core/tracker/tracker.go

+		}
+
+		// Avoid identical logs if same set of peers didn't participated.
+		if len(absentPeers) > 0 && !uniqueParticipation {


if fmt.Sprint(absent) != fmt.Sprint(prevAbsent) { if len(absent) == 0 { log.Info(ctx, "All peers participated in duty", z.Any("duty")) else { log.Info(ctx, "Not all peers participated in duty", z.Any("duty"), z.Any("absent", absentPeers)) } } }

corverroos · 2022-07-25T14:13:33Z

core/tracker/tracker.go

-			// analyseParticipation(duty, t.events[duty])
+			participatedShares, err := analyseParticipation(t.events[duty])
+			if err != nil {
+				log.Error(ctx, "Invalid participated shares", err)


btw, you need to add tracker log topic to ctx

corverroos · 2022-07-25T14:14:01Z

core/tracker/tracker.go

+		// If we get a parSigEx event, then the corresponding peer with e.shareIdx participated successfully.
+		if e.component == parSigEx || e.component == parSigDBInternal {
+			if e.shareIdx == 0 {
+				return nil, errors.New("invalid shareIdx", z.Any("component", e.component))


"shareIdx empty"

dB2510 added 4 commits July 23, 2022 17:16

implement analyse participation

c36de04

update comments

5528144

refactor tests

9740273

add test

417af6a

dB2510 force-pushed the dhruv/participation branch from 2650393 to 417af6a Compare July 23, 2022 11:47

cleanup

3978dc2

corverroos reviewed Jul 24, 2022

View reviewed changes

feedback

a9e6885

corverroos approved these changes Jul 25, 2022

View reviewed changes

cleanup

eb45b19

corverroos approved these changes Jul 25, 2022

View reviewed changes

cleanup

395e6ee

dB2510 added the merge when ready Indicates bulldozer bot may merge when all checks pass label Jul 25, 2022

obol-bulldozer bot merged commit 8f25c30 into main Jul 25, 2022

obol-bulldozer bot deleted the dhruv/participation branch July 25, 2022 14:25

dB2510 mentioned this pull request Jul 26, 2022

Analyse Cluster Participation #821

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/tracker: implement analyse cluster participation #846

core/tracker: implement analyse cluster participation #846

dB2510 commented Jul 23, 2022 •

edited

Loading

codecov bot commented Jul 23, 2022 •

edited

Loading

corverroos Jul 24, 2022

corverroos Jul 24, 2022

dB2510 Jul 25, 2022

corverroos Jul 24, 2022

corverroos Jul 24, 2022

corverroos Jul 24, 2022

corverroos Jul 24, 2022

corverroos Jul 24, 2022

corverroos Jul 25, 2022

corverroos Jul 25, 2022

corverroos Jul 25, 2022

corverroos Jul 25, 2022

corverroos Jul 25, 2022 •

edited

Loading

corverroos Jul 25, 2022

corverroos Jul 25, 2022

corverroos Jul 25, 2022

corverroos Jul 25, 2022

corverroos Jul 25, 2022

corverroos Jul 25, 2022

dB2510 Jul 25, 2022

corverroos Jul 25, 2022

core/tracker: implement analyse cluster participation #846

core/tracker: implement analyse cluster participation #846

Conversation

dB2510 commented Jul 23, 2022 • edited Loading

codecov bot commented Jul 23, 2022 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

corverroos Jul 25, 2022 • edited Loading

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

dB2510 commented Jul 23, 2022 •

edited

Loading

codecov bot commented Jul 23, 2022 •

edited

Loading

corverroos Jul 25, 2022 •

edited

Loading