Broadcast Proposer Slashing on equivocation #14693
Conversation
|
I think this should probably be done in the gossip validation of beacon blocks for two reasons:
Fwiw I am just an interested bystander using this as a learning opportunity. Feel free to ignore or let me know if this is unhelpful |
There was a problem hiding this comment.
When moving this check to the right spot instead after the call to ReceiveBlock we don't have the root yet (since we only hash after we know we haven't seen the block), we should check if the signatures are different instead, as a different root will result in a different signature.
shyam-patel-kira
changed the title
|
Hey @potuz, Thanks for the review, addressed the changes that were requested |
…nto broadcast-equivocating-blocks
… broadcast-equivocating-blocks
…nto broadcast-equivocating-blocks
…nto broadcast-equivocating-blocks
…yam-patel-kira/prysm into broadcast-equivocating-blocks
|
not sure why but the review got dismissed after I committed |
shyam-patel-kira
changed the title
|
|
||
| // Verify the slashing against current state | ||
| if err := blocks.VerifyProposerSlashing(headState, slashing); err != nil { | ||
| if strings.Contains(err.Error(), "could not verify beacon block header") { |
There was a problem hiding this comment.
It is very risky to compare errors like this because if someone modifies the original error message, this may never be true anymore. Can you extract the original message to an exported variable and use errors.Is?
| ### Added | ||
| - Added immediate broadcasting of proposer slashings when equivocating blocks are detected during block processing. | ||
| - Added signature verification for proposer slashing detection. | ||
| - Added a new `HeadStateErr` for testing equivocations | ||
|
|
||
| ### Changed | ||
| - Improved equivocation detection by comparing blocks with the same slot and proposer against the head block. | ||
| - Removed redundant slashing verification checks in favor of using existing VerifyProposerSlashing function. |
There was a problem hiding this comment.
| ### Added | |
| - Added immediate broadcasting of proposer slashings when equivocating blocks are detected during block processing. | |
| - Added signature verification for proposer slashing detection. | |
| - Added a new `HeadStateErr` for testing equivocations | |
| ### Changed | |
| - Improved equivocation detection by comparing blocks with the same slot and proposer against the head block. | |
| - Removed redundant slashing verification checks in favor of using existing VerifyProposerSlashing function. | |
| ### Added | |
| - Added immediate broadcasting of proposer slashings when equivocating blocks are detected during block processing. |
I would include only this in the changelog because that's the only thing that really matters. Adding too many entries to a changelog fragment bloats the changelog
| @@ -1495,3 +1496,225 @@ func Test_validateDenebBeaconBlock(t *testing.T) { | |||
| require.NoError(t, err) | |||
| require.ErrorIs(t, validateDenebBeaconBlock(bdb.Block()), errRejectCommitmentLen) | |||
| } | |||
|
|
|||
| func TestDetectAndBroadcastEquivocation_NoEquivocation(t *testing.T) { | |||
There was a problem hiding this comment.
Can you merge all these tests into one, using t.Run for each test case? That way you can set up things like p := p2ptest.NewTestP2P(t) only once, at the start of the test, and reduce test times.
Pseudocode:
func TestDetectAndBroadcastEquivocation() {
p := p2ptest.NewTestP2P(t)
(...)
t.Run("no equivocation", ...
t.Run("equivocation detected", ...
}
|
@shyam-patel-kira I left a few comments |
… broadcast-equivocating-blocks
Thanks for the review @rkapka, I've pushed the changes let me know if anything needs to be changed |
|
@shyam-patel-kira |
… broadcast-equivocating-blocks
…yam-patel-kira/prysm into broadcast-equivocating-blocks
…yam-patel-kira/prysm into broadcast-equivocating-blocks
fixed it |
fixes #13088
The PR implements immediate broadcasting of slashing messages when detecting equivocating blocks, helping the network react more quickly to malicious behavior without relying on the full slasher service processing.