Skip to content
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

Merged
merged 14 commits into from Apr 22, 2020
Merged

More concise duty logging #5397

merged 14 commits into from Apr 22, 2020

Conversation

mcdee
Copy link
Contributor

@mcdee mcdee commented Apr 12, 2020

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):

{"attesters":32,"level":"info","msg":"Attestation schedule","prefix":"validator","pubKeys":["0xb45ee2d02237","0x84398f539a64","0x95d668e77761","0x870bd2434747","0xb090aff6e74d","0xad6017ff33ec","0x91ad339cd616","0xa6d3e4f2b3e5","0xb90a59e63d66","0x851faec0a50c","0xadf6fceba8b4","0x80f0109a647b","0x939f65151b9f","0xa2a576a9ead4","0x980cab0a4aa2","0x8428f1acb3bf","0x928d2b6b1e8b","0x906af7add69c","0xb4c37ed40d2d","0xab032b66e09b","0x83dbae1b08ee","0xb9f5c800dbe8","0x893f6809ed7b","0x94c1a664baed","0x836f2b3e00df","0xa3a32b0f8b4d","0x846f3af6ac23","0xae363ffe8d6c","0x82d62ceb1242","0xa069d8c83f71","0x98f65cbdd29f","0xb8ad8a758c02"],"slot":26,"time":"2020-04-12T11:59:14+01:00"}
{"level":"info","msg":"Proposal schedule","prefix":"validator","pubKey":"0xb7d1eb9f5038","slot":26,"time":"2020-04-12T11:59:14+01:00"}

@mcdee mcdee requested a review from a team as a code owner April 12, 2020 11:20
@codecov
Copy link

codecov bot commented Apr 12, 2020

Codecov Report

Merging #5397 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5397   +/-   ##
=======================================
  Coverage   22.44%   22.44%           
=======================================
  Files         268      268           
  Lines       22711    22711           
=======================================
  Hits         5098     5098           
  Misses      16646    16646           
  Partials      967      967           

proposerKeys[proposerIndex] = validatorKey
}
}
// 0.11 has multiple proposer slots; use this code in place of the above when merged.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix

Copy link
Contributor Author

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.

@mcdee
Copy link
Contributor Author

mcdee commented Apr 16, 2020

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: if duty.Status == ethpb.ValidatorStatus_ACTIVE that I've changed to if duty.Status == ethpb.ValidatorStatus_ACTIVE || duty.Status == ethpb.ValidatorStatus_EXITING, as exiting validators still have assignments and so their subnets should be included. @terencechain could you confirm you're happy with this change?

(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).

@prestonvanloon prestonvanloon added the Ready For Review A pull request ready for code review label Apr 16, 2020
attesterKeys[i] = make([]string, 0)
}
proposerKeys := make([]string, params.BeaconConfig().SlotsPerEpoch)
slotOffset := (slot / params.BeaconConfig().SlotsPerEpoch) * params.BeaconConfig().SlotsPerEpoch
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

helpers.StartSlot()

Copy link
Contributor Author

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.

@prylabs-bulldozer prylabs-bulldozer bot merged commit db6dbdc into prysmaticlabs:master Apr 22, 2020
@mcdee mcdee deleted the concise-duties branch April 22, 2020 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants