-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More concise duty logging #5397
More concise duty logging #5397
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5397 +/- ##
=======================================
Coverage 22.44% 22.44%
=======================================
Files 268 268
Lines 22711 22711
=======================================
Hits 5098 5098
Misses 16646 16646
Partials 967 967 |
validator/client/validator.go
Outdated
proposerKeys[proposerIndex] = validatorKey | ||
} | ||
} | ||
// 0.11 has multiple proposer slots; use this code in place of the above when merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated; will run locally for a bit to ensure the manual bits of the merge are correct.
Ran this overnight and it seems fine. Note that this does change a couple of functional lines now, rather than just the logging. There were a couple of checks that said: (Also, the separation of the logging code does mean that there are two chunks of code now that are basically the same and could be spun in to a function now, but that's outside of the scope of this patch). |
validator/client/validator.go
Outdated
attesterKeys[i] = make([]string, 0) | ||
} | ||
proposerKeys := make([]string, params.BeaconConfig().SlotsPerEpoch) | ||
slotOffset := (slot / params.BeaconConfig().SlotsPerEpoch) * params.BeaconConfig().SlotsPerEpoch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont quite understand (slot / params.BeaconConfig().SlotsPerEpoch) * params.BeaconConfig().SlotsPerEpoch
Do you mean to do slot - epoch_start_slot
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes; slot at the beginning of this slot's epoch. Is there a helper for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
helpers.StartSlot()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks; updated to use the helpers.
At current the validator logs a duty entry for every active key each epoch, which is both noisy and somewhat hard to comprehend when a validator is running many keys. This patch changes the output from key-based to slot-based, which bounds the maximum number of entries to 64 (one for attesters at each slot, one for proposer at each slot).
The only information that is lost with this more concise format is the committee index of the attester, but it shows up when the (aggregated) attestation is made so it actually just avoids duplication.
Sample output from the new format (from an interop run with 1,024 validators):