[Merged by Bors] - Update state before producing attestation #1596
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
|
Update: no missed attestations after running for 100 more epochs! |
AgeManning
approved these changes
Sep 8, 2020
michaelsproul
approved these changes
Sep 8, 2020
paulhauner
approved these changes
Sep 8, 2020
There was a problem hiding this comment.
Great work! Like @michaelsproul (I assume) I have actually reviewed this and approve it.
bors r+
bors bot
pushed a commit
that referenced
this pull request
Sep 8, 2020
## Issue Addressed Partly addresses #1547 ## Proposed Changes This fix addresses the missing attestations at slot 0 of an epoch (also sometimes slot 1 when slot 0 was skipped). There are 2 cases: 1. BN receives the block for the attestation slot after 4 seconds (1/3rd of the slot). 2. No block is proposed for this slot. In both cases, when we produce the attestation, we pass the head state to the `produce_unaggregated_attestation_for_block` function here https://github.com/sigp/lighthouse/blob/9833eca02485ef06abef4144f071ea3b79002f30/beacon_node/beacon_chain/src/beacon_chain.rs#L845-L850 Since we don't advance the state in this function, we set `attestation.data.source = state.current_justified_checkpoint` which is atleast 2 epochs lower than current_epoch(wall clock epoch). This attestation is invalid and cannot be included in a block because of this assert from the spec: ```python if data.target.epoch == get_current_epoch(state): assert data.source == state.current_justified_checkpoint state.current_epoch_attestations.append(pending_attestation) ``` https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/beacon-chain.md#attestations This PR changes the `produce_unaggregated_attestation_for_block` function to ensure that it advances the state before producing the attestation at the new epoch. Running this on my node, have missed 0 attestations across all 8 of my validators in a 100 epoch period 🎉 To compare, I was missing ~14 attestations across all 8 validators in the same 100 epoch period before the fix. Will report missed attestations if any after running for another 100 epochs tomorrow.
|
Pull request successfully merged into master. Build succeeded: |
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.
Issue Addressed
Partly addresses #1547
Proposed Changes
This fix addresses the missing attestations at slot 0 of an epoch (also sometimes slot 1 when slot 0 was skipped).
There are 2 cases:
In both cases, when we produce the attestation, we pass the head state to the
produce_unaggregated_attestation_for_blockfunction herelighthouse/beacon_node/beacon_chain/src/beacon_chain.rs
Lines 845 to 850 in 9833eca
Since we don't advance the state in this function, we set
attestation.data.source = state.current_justified_checkpointwhich is atleast 2 epochs lower than current_epoch(wall clock epoch).This attestation is invalid and cannot be included in a block because of this assert from the spec:
https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/beacon-chain.md#attestations
This PR changes the
produce_unaggregated_attestation_for_blockfunction to ensure that it advances the state before producing the attestation at the new epoch.Running this on my node, have missed 0 attestations across all 8 of my validators in a 100 epoch period 🎉
To compare, I was missing ~14 attestations across all 8 validators in the same 100 epoch period before the fix.
Will report missed attestations if any after running for another 100 epochs tomorrow.