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

Monitor attestations #9901

Merged
merged 11 commits into from
Nov 19, 2021
Merged

Monitor attestations #9901

merged 11 commits into from
Nov 19, 2021

Conversation

potuz
Copy link
Contributor

@potuz potuz commented Nov 13, 2021

This PR is the third in the implementation of the validator monitor, it should be reviewed after #9899

This PR implements logging of performance for attestations viewed by our tracked validators. For attestations included in blocks it logs

  • Timely Head, Source, Target
  • Inclusion Slot
  • Balance and Balance Change

In addition to the basic information

  • Validator Index
  • Attested Slot
  • Source, Target and Head block roots.

For unagregated and aggregated attestations processed by the node (after P2P validation but not necessarily included in a block), we log only the basic information above.

In addition, for P2P aggregated attestations, we log the event that one of our tracked validators was the aggregator.

The P2P logging (namely not included in blocks) is only performed if the appropriate Beacon State is currently cached to avoid expensive lookups. This requires addition of the appropriate function in the stategen package.

info for reviewers

  • This PR includes the most expensive performance hit of the validator monitor: the state copies to log unaggregated and aggregated attestations coming over the wire. I have tested this on Prater and the beacon stays in sync, but this would require testing on testnet to verify if it is a resource intensive operation or not. In principle we are only requesting states from the cache.
  • This PR has a larger diff than the previous two, but as a consolation reward, the next PR will only include the addition of metrics and will be minimal.

@potuz potuz requested a review from a team as a code owner November 13, 2021 20:52
@potuz potuz force-pushed the monitor_attestations branch 2 times, most recently from 2f3ebad to 28c90c9 Compare November 13, 2021 21:30
@potuz potuz force-pushed the monitor_attestations branch 2 times, most recently from 50c24b7 to b4e3494 Compare November 15, 2021 17:21
@potuz potuz force-pushed the monitor_attestations branch from b4e3494 to f8ccb39 Compare November 15, 2021 20:41
Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

👏 First round of feedbacks. Will look again

beacon-chain/monitor/process_attestation.go Outdated Show resolved Hide resolved
beacon-chain/monitor/process_attestation.go Outdated Show resolved Hide resolved
beacon-chain/monitor/process_attestation.go Show resolved Hide resolved
beacon-chain/monitor/process_attestation.go Outdated Show resolved Hide resolved
beacon-chain/monitor/process_attestation.go Outdated Show resolved Hide resolved
beacon-chain/monitor/process_attestation.go Outdated Show resolved Hide resolved
@potuz potuz force-pushed the monitor_attestations branch from c65c2f7 to 5a9e150 Compare November 16, 2021 18:23
@potuz potuz force-pushed the monitor_attestations branch from 5a9e150 to f3d88f8 Compare November 16, 2021 18:29
beacon-chain/monitor/process_attestation.go Outdated Show resolved Hide resolved
beacon-chain/monitor/process_attestation.go Show resolved Hide resolved
beacon-chain/monitor/process_attestation.go Outdated Show resolved Hide resolved
beacon-chain/monitor/process_attestation.go Outdated Show resolved Hide resolved
beacon-chain/monitor/process_attestation.go Outdated Show resolved Hide resolved
beacon-chain/monitor/process_attestation.go Outdated Show resolved Hide resolved
beacon-chain/monitor/service.go Outdated Show resolved Hide resolved
terencechain
terencechain previously approved these changes Nov 18, 2021
beacon-chain/monitor/process_attestation.go Outdated Show resolved Hide resolved
beacon-chain/monitor/process_attestation.go Outdated Show resolved Hide resolved
beacon-chain/monitor/process_attestation.go Outdated Show resolved Hide resolved
@potuz potuz merged commit 50159c2 into prysmaticlabs:develop Nov 19, 2021
@potuz potuz mentioned this pull request Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants