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

create unified slashing cache #5033

Merged
merged 7 commits into from
Jan 8, 2024
Merged

Conversation

realbigsean
Copy link
Member

@realbigsean realbigsean commented Jan 3, 2024

Issue Addressed

#5017

Changes

  • added a new slashing cache, to track slashing across all messages
  • simplified the blob gossip cache, so it no longer tracks block roots
  • I initially simplified the block gossip cache as well, but it turns out we still need to distinguish between "blocks failing gossip that are slashable" and "blocks failing gossip that are duplicates" so that we can correctly suppress the duplicate error code
  • Additionally, this makes sure we add slashable headers from blob sidecars received via RPC. And checks the signatures. (I don't think we were checking the signature before sending it to the slasher before).
    • I've also opted not to throw error when the signature is invalid here (which would result in not importing the blobs even if the data is correct, and would lead us to downscore the peer), because on an RPC request we only really care whether the data is available or not

@realbigsean realbigsean added work-in-progress PR is a work-in-progress deneb labels Jan 3, 2024
@realbigsean realbigsean added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Jan 4, 2024
@realbigsean realbigsean added the v4.6.0 ETA Q1 2024 label Jan 5, 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.

Nice, I can't spot any issues with this.

I'm trying to think of ways to simplify it further, as it would be great to remove observed_block_producers and just use observed_slashable for all blocks. I think this would require tweaking the spec conditions however, and a resolution to questions like should we propagate slashable messages in a limited way? (ethereum/consensus-specs#3561).

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jan 8, 2024
paulhauner added a commit to paulhauner/lighthouse that referenced this pull request Jan 8, 2024
Squashed commit of the following:

commit 924bdc9
Author: realbigsean <seananderson33@gmail.com>
Date:   Thu Jan 4 19:17:04 2024 -0500

    don't throw error on RPC signature invalie

commit acfa187
Author: realbigsean <seananderson33@gmail.com>
Date:   Thu Jan 4 19:05:04 2024 -0500

    check header signatures for RPC blobs

commit 33d51d5
Author: realbigsean <seananderson33@gmail.com>
Date:   Thu Jan 4 18:13:39 2024 -0500

    clean up slashable cache test

commit 8bd9d85
Author: realbigsean <seananderson33@gmail.com>
Date:   Thu Jan 4 17:47:22 2024 -0500

    revert block seen cache changes

commit 4c3239d
Author: realbigsean <seananderson33@gmail.com>
Date:   Thu Jan 4 17:18:38 2024 -0500

    fix broadcast validation tests

commit 208bf1e
Author: realbigsean <seananderson33@gmail.com>
Date:   Wed Jan 3 18:04:53 2024 -0500

    add observed slashable file

commit 3142635
Author: realbigsean <seananderson33@gmail.com>
Date:   Wed Jan 3 17:58:15 2024 -0500

    create unified slashing cache
@paulhauner paulhauner mentioned this pull request Jan 8, 2024
@realbigsean
Copy link
Member Author

Thanks for the review!

I'm trying to think of ways to simplify it further, as it would be great to remove observed_block_producers and just use observed_slashable for all blocks. I think this would require tweaking the spec conditions however, and a resolution to questions like should we propagate slashable messages in a limited way? (ethereum/consensus-specs#3561).

I agree. If we can re-use the definition of "equivocation" from broadcast_validation in gossip we could simplify this pretty well. But I brought it up on a call and I don't think there's appetite for it until post-deneb.

@realbigsean realbigsean merged commit f70c32e into sigp:unstable Jan 8, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deneb 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