Fix Attester Slashing Validation In Gossip #12295
Conversation
nisdas
added
Ready For Review
| @@ -50,7 +51,7 @@ func (s *Service) validateAttesterSlashing(ctx context.Context, pid peer.ID, msg | |||
| if err != nil { | |||
| return pubsub.ValidationIgnore, err | |||
| } | |||
| if err := blocks.VerifyAttesterSlashing(ctx, headState, slashing); err != nil { | |||
| if _, err := blocks.ProcessAttesterSlashing(ctx, headState, slashing, v.SlashValidator); err != nil { | |||
There was a problem hiding this comment.
This mutates a beacon state and slashes a validator in vain, since the function VerifyAttesterSlashing anyway is called in a few other places (like RPC methods) and in particular is called within ProcessAttesterSlashing I believe that the right thing to do would be to perform the extra check within VerifyAttesterSlashing and if performance is an issue, this function can be refactored to return the validators that need to be slashed in the state (so that ProcessAttesterSlashing can use them)
| @@ -53,7 +57,20 @@ func (s *Service) validateAttesterSlashing(ctx context.Context, pid peer.ID, msg | |||
| if err := blocks.VerifyAttesterSlashing(ctx, headState, slashing); err != nil { | |||
| return pubsub.ValidationReject, err | |||
| } | |||
|
|
|||
| slashedVals := blocks.SlashableAttesterIndices(slashing) | |||
There was a problem hiding this comment.
Better to move this up a little and check if slashedVals == nil then you can return the nil message error above and avoid the double check for nil
| return pubsub.ValidationIgnore, err | ||
| } | ||
| if helpers.IsSlashableValidator(val.ActivationEpoch(), val.WithdrawableEpoch(), val.Slashed(), slots.ToEpoch(headState.Slot())) { | ||
| isSlashable = true |
There was a problem hiding this comment.
Don't we need to break the loop here?
What type of PR is this?
Bug Fix
What does this PR do? Why is it needed?
Fixes a missing check for slashable validators in our gossip pipeline. This adds in the check and also adds in
a respective regression check for it.
Which issues(s) does this PR fix?
N.A
Other notes for review