Repository: ObolNetwork/charon

Issue:
Batch duplicate logs
# 🎯 Problem to be solved
We log on a lot of places the same line multiple times for distinct validators in a cluster.
```
08:03:32.005 DEBG fetcher    Attester not selected for aggregation duty {"pubkey": "a5a_d41", "duty": "632367/aggregator"}
08:03:32.005 DEBG fetcher    Attester not selected for aggregation duty {"pubkey": "a63_35a", "duty": "632367/aggregator"}
...
08:03:32.002 INFO fetcher    Resolved attester aggregation duty       {"pubkey": "b00_1dd", "duty": "632367/aggregator"}
08:03:32.005 DEBG fetcher    Attester not selected for aggregation duty {"pubkey": "96c_049", "duty": "632367/aggregator"}
```

We can simplify our logging and make it more readable for certain log lines.

# 🛠️ Proposed solution
Log for multiple validators, instead of a single one.

```
08:03:32.005 DEBG fetcher    Attester not selected for aggregation duty {"pubkeys": "a63_35a, a5a_d41, ..., 96c_049", "duty": "632367/aggregator"}
08:03:32.002 INFO fetcher    Resolved attester aggregation duty       {"pubkey": "b00_1dd", "duty": "632367/aggregator"}
```


Hints from issue comments:
@pinebit: Just to clarify this needs to be done at the application level, not in zap/logging layer.

Solution patch:
diff --git a/core/fetcher/fetcher.go b/core/fetcher/fetcher.go
index 935d6dec..4b8276c1 100644
--- a/core/fetcher/fetcher.go
+++ b/core/fetcher/fetcher.go
@@ -172,6 +172,9 @@ func (f *Fetcher) fetchAttesterData(ctx context.Context, slot uint64, defSet cor
 
 // fetchAggregatorData fetches the attestation aggregation data.
 func (f *Fetcher) fetchAggregatorData(ctx context.Context, slot uint64, defSet core.DutyDefinitionSet) (core.UnsignedDataSet, error) {
+	pt := newPubkeysTracker("attester aggregation")
+	defer pt.log(ctx)
+
 	// We may have multiple aggregators in the same committee, use the same aggregated attestation in that case.
 	aggAttByCommIdx := make(map[eth2p0.CommitteeIndex]*eth2spec.VersionedAttestation)
 
@@ -197,10 +200,10 @@ func (f *Fetcher) fetchAggregatorData(ctx context.Context, slot uint64, defSet c
 		if err != nil {
 			return core.UnsignedDataSet{}, err
 		} else if !ok {
-			log.Debug(ctx, "Attester not selected for aggregation duty", z.Any("pubkey", pubkey))
+			pt.addNotSelected(pubkey.String())
 			continue
 		}
-		log.Info(ctx, "Resolved attester aggregation duty", z.Any("pubkey", pubkey))
+		pt.addResolved(pubkey.String())
 
 		aggAtt, ok := aggAttByCommIdx[attDef.CommitteeIndex]
 		if ok {
@@ -298,6 +301,9 @@ func (f *Fetcher) fetchProposerData(ctx context.Context, slot uint64, defSet cor
 
 // fetchContributionData fetches the sync committee contribution data.
 func (f *Fetcher) fetchContributionData(ctx context.Context, slot uint64, defSet core.DutyDefinitionSet) (core.UnsignedDataSet, error) {
+	pt := newPubkeysTracker("sync committee contribution")
+	defer pt.log(ctx)
+
 	resp := make(core.UnsignedDataSet)
 	for pubkey := range defSet {
 		// Query AggSigDB for DutyPrepareSyncContribution to get sync committee selection.
@@ -318,7 +324,7 @@ func (f *Fetcher) fetchContributionData(ctx context.Context, slot uint64, defSet
 		if err != nil {
 			return core.UnsignedDataSet{}, err
 		} else if !ok {
-			log.Debug(ctx, "Sync committee member not selected for contribution aggregation duty", z.Any("pubkey", pubkey))
+			pt.addNotSelected(pubkey.String())
 			continue
 		}
 
@@ -352,7 +358,7 @@ func (f *Fetcher) fetchContributionData(ctx context.Context, slot uint64, defSet
 			// This could happen if the beacon node didn't subscribe to the correct subnet.
 			return core.UnsignedDataSet{}, errors.New("sync committee contribution not found by root (retryable)", z.U64("subcommidx", subcommIdx), z.Hex("root", blockRoot[:]))
 		}
-		log.Info(ctx, "Resolved sync committee contribution duty", z.Any("pubkey", pubkey))
+		pt.addResolved(pubkey.String())
 
 		resp[pubkey] = core.SyncContribution{
 			SyncCommitteeContribution: *contribution,
@@ -401,3 +407,36 @@ func verifyFeeRecipient(ctx context.Context, proposal *eth2api.VersionedProposal
 			z.Str("expected", feeRecipientAddress), z.Str("actual", actualAddr))
 	}
 }
+
+type pubkeysTracker struct {
+	title              string
+	notSelectedPubKeys []string
+	resolvedPubKeys    []string
+}
+
+func newPubkeysTracker(title string) *pubkeysTracker {
+	return &pubkeysTracker{
+		title:              title,
+		notSelectedPubKeys: make([]string, 0),
+		resolvedPubKeys:    make([]string, 0),
+	}
+}
+
+func (pt *pubkeysTracker) addNotSelected(pubkey string) {
+	pt.notSelectedPubKeys = append(pt.notSelectedPubKeys, pubkey)
+}
+
+func (pt *pubkeysTracker) addResolved(pubkey string) {
+	pt.resolvedPubKeys = append(pt.resolvedPubKeys, pubkey)
+}
+
+func (pt *pubkeysTracker) log(ctx context.Context) {
+	if len(pt.notSelectedPubKeys) > 0 {
+		s := strings.Join(pt.notSelectedPubKeys, ",")
+		log.Debug(ctx, pt.title+": not selected pubkeys", z.Str("pubkeys", s))
+	}
+	if len(pt.resolvedPubKeys) > 0 {
+		s := strings.Join(pt.resolvedPubKeys, ",")
+		log.Info(ctx, pt.title+": resolved pubkeys", z.Str("pubkeys", s))
+	}
+}
diff --git a/core/scheduler/scheduler.go b/core/scheduler/scheduler.go
index a88bb383..34b3969b 100644
--- a/core/scheduler/scheduler.go
+++ b/core/scheduler/scheduler.go
@@ -408,7 +408,7 @@ func (s *Scheduler) resolveAttDuties(ctx context.Context, slot core.Slot, vals v
 	return nil
 }
 
-// resolveProposerDuties resolves proposer duties for the given validators.
+// resolveProDuties resolves proposer duties for the given validators.
 func (s *Scheduler) resolveProDuties(ctx context.Context, slot core.Slot, vals validators) error {
 	opts := &eth2api.ProposerDutiesOpts{
 		Epoch:   eth2p0.Epoch(slot.Epoch()),
diff --git a/core/signeddata.go b/core/signeddata.go
index fafddd3a..44e6620b 100644
--- a/core/signeddata.go
+++ b/core/signeddata.go
@@ -1207,7 +1207,7 @@ type SyncCommitteeSelection struct {
 	eth2exp.SyncCommitteeSelection
 }
 
-// MessageRoots returns the signing roots for the provided SyncCommitteeSelection.
+// MessageRoot returns the signing roots for the provided SyncCommitteeSelection.
 // Refer https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/validator.md#syncaggregatorselectiondata
 func (s SyncCommitteeSelection) MessageRoot() ([32]byte, error) {
 	data := altair.SyncAggregatorSelectionData{
@@ -1784,7 +1784,7 @@ type SyncContributionAndProof struct {
 	altair.ContributionAndProof
 }
 
-// MessageRoots returns the signing roots for the provided SyncContributionAndProof.
+// MessageRoot returns the signing roots for the provided SyncContributionAndProof.
 // Refer: https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/validator.md#aggregation-selection.
 func (s SyncContributionAndProof) MessageRoot() ([32]byte, error) {
 	data := altair.SyncAggregatorSelectionData{
@@ -1876,7 +1876,7 @@ type SignedSyncContributionAndProof struct {
 	altair.SignedContributionAndProof
 }
 
-// MessageRoots returns the signing roots for the provided SignedSyncContributionAndProof.
+// MessageRoot returns the signing roots for the provided SignedSyncContributionAndProof.
 // Refer get_contribution_and_proof_signature from https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/validator.md#broadcast-sync-committee-contribution.
 func (s SignedSyncContributionAndProof) MessageRoot() ([32]byte, error) {
 	return s.Message.HashTreeRoot()



Test patch:
diff --git a/core/validatorapi/router_internal_test.go b/core/validatorapi/router_internal_test.go
index 4b8b8fae..d5d9af56 100644
--- a/core/validatorapi/router_internal_test.go
+++ b/core/validatorapi/router_internal_test.go
@@ -1924,7 +1924,7 @@ func testRawRouter(t *testing.T, handler testHandler, callback func(context.Cont
 	testRawRouterEx(t, handler, callback, true)
 }
 
-// testRawRouterEX is a helper function same as testRawRouter() but accepts GetBuilderAPIFlagFunc.
+// testRawRouterEx is a helper function same as testRawRouter() but accepts GetBuilderAPIFlagFunc.
 func testRawRouterEx(t *testing.T, handler testHandler, callback func(context.Context, string), builderEnabled bool) {
 	t.Helper()
 
diff --git a/testutil/verifypr/verify.go b/testutil/verifypr/verify.go
index 48555389..511f04d9 100644
--- a/testutil/verifypr/verify.go
+++ b/testutil/verifypr/verify.go
@@ -196,8 +196,8 @@ func verifyBody(body string) error {
 				// URL is fine
 			} else if ticket == "none" {
 				// None is also fine
-			} else if strings.HasPrefix(ticket, "#") {
-				_, err := strconv.Atoi(strings.TrimPrefix(ticket, "#"))
+			} else if after, ok := strings.CutPrefix(ticket, "#"); ok {
+				_, err := strconv.Atoi(after)
 				if err != nil {
 					return errors.New("ticket tag not a valid github link, #123")
 				}