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

Fix IncorrectAttestationSource error in attn. simulator #5048

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

paulhauner
Copy link
Member

Issue Addressed

Resolves #5046

Proposed Changes

Introduces both of the fixes suggested in #5046:

  1. Handles the IncorrectAttestationSource error so that a missed source attestation can be identified.
  2. Doesn't try to simulate attestations when the head block is more than 2 epochs older than the wall-clock slot.

My reasoning for (2) is twofold:

  1. To prevent the simulator from trying to compute future, uncached shufflings from old states while syncing. Although we're careful with this regard when verifying simulated attestations, we're not so careful when producing them.
  2. To avoid bad results when syncing. This is a bit philosophical, but I intended to use this feature to gauge the performance of different hardware. Getting bad "benchmarks" during sync wouldn't help my cause. I'm open to alternative views, though!

Additional Info

NA

@paulhauner paulhauner added the v4.6.0 ETA Q1 2024 label Jan 9, 2024
paulhauner added a commit to paulhauner/lighthouse that referenced this pull request Jan 9, 2024
Squashed commit of the following:

commit a1fa5db
Author: Paul Hauner <paul@paulhauner.com>
Date:   Tue Jan 9 14:31:19 2024 +1100

    Handle `IncorrectAttestationSource`

commit bab026f
Author: Paul Hauner <paul@paulhauner.com>
Date:   Tue Jan 9 14:31:04 2024 +1100

    Don't produce attestations when syncing
@paulhauner paulhauner mentioned this pull request Jan 9, 2024
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

V nice

I think not producing attestations during sync is fine. It could be an issue if Lighthouse were crashing or de-synchronising regularly and we wanted to see that reflected in the attestation simulation metrics, but I also think it's OK to consider that out of scope. We would probably notice such an issue looking at other metrics/logs.

@michaelsproul michaelsproul added the ready-for-merge This PR is ready to merge. label Jan 9, 2024
@michaelsproul michaelsproul merged commit be79f74 into sigp:unstable Jan 9, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge. v4.6.0 ETA Q1 2024
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants