Improve vc logs #13573
Merged
Improve vc logs #13573
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rkapka
force-pushed
the
improve-vc-logs
branch
from
f6dcc91 to
aede437
Compare
terencechain
reviewed
Feb 1, 2024
validator/client/attest.go
Outdated
Comment on lines
237
to
241
| h, err := hash.Proto(data) | ||
| r, err := data.HashTreeRoot() |
There was a problem hiding this comment.
Have we benchmarked against the two?
|
What's the point of |
|
Also, I'm wondering if the |
This reverts commit 0820c870d2627950179b0edf7ce62ee4fa4a03a3.
|
Also: We have both |
|
Sometimes, it's |
rkapka
commented
Feb 8, 2024
| @@ -258,7 +258,7 @@ func performRoles(slotCtx context.Context, allRoles map[[48]byte][]iface.Validat | |||
| defer span.End() | |||
| defer func() { | |||
| if err := recover(); err != nil { // catch any panic in logging | |||
| log.WithField("err", err). | |||
| log.WithField("error", err). | |||
There was a problem hiding this comment.
Can't use WithError because err is of type any
rkapka
commented
Feb 8, 2024
| "slot": epochStartSlot + i, | ||
| "slotInEpoch": (epochStartSlot + i) % params.BeaconConfig().SlotsPerEpoch, | ||
| "attesterCount": len(attesterKeys[i]), | ||
| "attesterPubkeys": attesterKeys[i], |
There was a problem hiding this comment.
we have a proposerPubkey field too, so can't use pubkeys
rkapka
commented
Feb 8, 2024
| "status": status.status.Status.String(), | ||
| } | ||
| if status.index != nonexistentIndex { | ||
| fields["index"] = status.index | ||
| fields["validatorIndex"] = status.index |
There was a problem hiding this comment.
We have validatorIndex in other places. I prefer this way rather than index because it might be unclear what kind of index it is (maybe not in this exact place, but somewhere else).
nalepae
previously approved these changes
Feb 8, 2024
validator/client/aggregate.go
Outdated
| for _, log := range v.attLogs { | ||
| if duty.CommitteeIndex == log.data.CommitteeIndex { | ||
| log.aggregatorIndices = append(log.aggregatorIndices, duty.ValidatorIndex) | ||
| for _, l := range v.submittedAtts { |
There was a problem hiding this comment.
Why using l as a variable name?
(the letter l is not in submittedAtts).
Why not using something like att?
nalepae
approved these changes
Feb 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Other
What does this PR do? Why is it needed?
The purpose of this PR is to make validator logs more clean/consistent/useful:
Schedule for epochlog before logs for individual slot duties, which allows to extract thetotalAttestersInEpochlog that was repeated for every slot. The new log also displays the proposer count.timeTillDutybecause I don't think it's useful.Attestation scheduleandProposal scheduleinto a singleDuties schedulelog.