-
Notifications
You must be signed in to change notification settings - Fork 741
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 phase0 block reward in rewards API #5101
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
71ef86b
Added Block Rewards
Gua00va 9bcaa83
added new type
Gua00va 4e9725c
added enum
Gua00va 4ce8f2a
Fix phase0 block reward in rewards API (#4929)
dknopik 4449c54
Merge 'guav00a/proposer-rewards-api'
dknopik 1462dc4
Merge unstable
dknopik 3204172
Revamp phase0 reward API tests
dknopik d3e2493
Merge branch 'unstable' into fix-4929
dknopik df16707
Merge branch 'unstable' into fix-4929
dknopik 614465d
Merge remote-tracking branch 'origin/unstable' into fix-4929
michaelsproul e45de97
Fix merge fallout
michaelsproul 4fd0e50
Remove junk revived in merge
michaelsproul 2007fd4
Address review
dknopik fec0996
Merge branch 'unstable' into fix-4929
dknopik fdb4eca
Merge branch 'unstable' into fix-4929
dknopik File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
This algorithm is not quite right because it pays out for attestations regardless of whether this proposer was the the first one to include them.
In the spec, the epoch processing function iterates the
previous_epoch_attestations
and pays the proposer reward to the proposer who includes each attester's attestation with minimalinclusion_delay
:It's quite common that a validator's attestation will be included multiple times in different aggregates, e.g. first at slot 10 and then again at slot 11. Only the proposer of the block of slot 10 gets the reward in this case (the slot 11 proposer either gets nothing for the attestation if it covered no new validators, or just the rewards for the newly covered validators for which this is their first inclusion on chain).
I think to remedy this the simplest fix would be to:
inclusion_delay
(usually the first inclusion, but not always). This would involve checking first againststate.previous_epoch_attestations
/state.current_epoch_attestations
as appropriate and then checking against attestations already processed in this block (we don't want to double pay for multi inclusions).One complication is the
min
byinclusion_delay
. It would be helpful if a property like this held:Unfortunately this property does not hold in the case of slashable attestations made by validator
V
when the chain is not finalising promptly, in which case it could be that:source
checkpoint (this requires finality to lapse for >2 epochs as shufflings only diverge if their common ancestor block is >2 epochs ago).V
signs two slashable attestations in epochN // 32
: one on each chain. LetM = N + 1
for simplicity. On the first chain they attest at e.g. slotN
and on the second chain they attest at slotN - 5
. The attestation from slotN - 5
is included atN
for aninclusion_delay
of 5, then the attestation from slotN
is included at slotM = N + 1
for aninclusion_delay
of 1. So it's possible for an attestation included at a later block to have a lowerinclusion_delay
, i.e. the property is false.Therefore in the general case it's not safe to just use the state at the block's slot (
state
) to infer the rewards. I had hoped that if this property held we could use it to avoid loading thecurrent_epoch_end
/next_epoch_end
states.Instead I think we should just proceed with loading all 3 states most of the time and rely on the fact that all the phase0 states are quite small and should load quickly with the soon-to-be-merged hierarchical state diffs:
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 for the review!
I've implemented a potential fix and a (rather messy) test for this.
I have a question about the second part: While such a pair of attestations would be obviously slashable, would they be really be includable into the "other" chain, respectively? As the shuffling is different, the attesting validator would likely be in a different committee for a different slot in each chain and therefore the validation would fail (for one of the attestations), as the signature does not match the expected validator, as per my understanding.
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.
The beacon node is able to verify attestations from multiple chains, it will load the relevant state & committees and use those to verify the signature. Only in the case where it views the blocks from the other chain as completely invalid will it fail to process the blocks & attestations from that chain.
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.
Oh no actually you're right. We can't include the attestations from the other fork on chain once the shufflings diverge, because
process_attestation
in the spec uses the committees from the current chainI think this means the property I stated does hold. Which means we could use the passed in
state
for all of the calculations 🤔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.
While I currently also think that your statement holds, I don't see why we can avoid the state loads.
In phase0, attestation rewards are processed at the epoch boundary. Therefore, we need the state, as a validator might be slashed at the epoch boundary but not at slot
N
where the attestation is included. Furthermore, we need the effective balance and total effective balance from the epoch boundary to properly calculate rewards.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.
Oooh yes, the slashing thing definitely prevents us from using the pre-state. Good catch.
The effective balances are immutable within an epoch, but you're right they'd also be a problem for the current epoch attestations we include which don't get rewarded until the end of the next epoch. Let's leave it as-is 😅