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

Add proposer_slashing and attester_slashing to SSE #376

Merged
merged 5 commits into from Dec 12, 2023

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented Dec 1, 2023

Add proposer_slashing and attester_slashing to SSE. There's a non-zero delay between slashable messages seen on gossip, and them being included on-chain. These new events allows consumers to learn about slashable events faster than just tracking blocks, specially in the case of large amounts of slashable messages.

One relevant use-case

The total cost of a slashing event for large operators is highly dependant on their response time. If many keys are involved in an operational error it can take some time for those slashable messages to be included on chain.

There are monitoring tools available that stop your setup in response to slashable messages seen on-chain. For example https://github.com/attestantio/esd. Their response time could be minimized to zero slots by watching gossip directly.

@mehdi-aouadi
Copy link
Contributor

I think it would be better and simpler to consolidate the two events into a simple one:

  • Event: slashed_validator
  • Data: {"pubkey": "0x...."}

That would be sufficient for this purpose. I don't see the point of having all the current attester and proposer slashing messages payload, plus it's more usefull for the VC to have the public keys instead of the index (let the BN do the translation) as mentioned by @rolfyone here

@rolfyone
Copy link
Collaborator

rolfyone commented Dec 1, 2023

I would have thought it'd be less work all round if there was just a stream of pubkeys or pubkeys and indices, rather than publishing the whole event for the 1 field...
This would satisfy what's been asked for, just wondering if we're ever going to use the rest of that data... It's admittedly nice to just publish standard spec events, I do see that side of things...

@dapplion
Copy link
Collaborator Author

dapplion commented Dec 6, 2023

I would have thought it'd be less work all round if there was just a stream of pubkeys or pubkeys and indices, rather than publishing the whole event for the 1 field... This would satisfy what's been asked for, just wondering if we're ever going to use the rest of that data... It's admittedly nice to just publish standard spec events, I do see that side of things...

Right the intent of this specification was to expose this data in the most generic way possible, instead of satisfy a very specific demand. However if we are confident that the full events will always be useless, just emitting pubkeys is more succint. Validators should already have a internal cached map of index -> pubkey, plus dealing with a few extra kB of data over the wire isn't that bad.

Do you see a big downside in complexity for having to parse the full events? Sending the full event also opens the possibility for trustless verification on the validator client side

@mcdee
Copy link
Contributor

mcdee commented Dec 6, 2023

I'd be inclined to send the full spec struct, probably simpler in codebases to do this than creating a new struct with a subset of the data, and also sets us up if we ever move towards sending events as SSZ.

@g11tech
Copy link
Contributor

g11tech commented Dec 6, 2023

I'd be inclined to send the full spec struct, probably simpler in codebases to do this than creating a new struct with a subset of the data, and also sets us up if we ever move towards sending events as SSZ.

+1

also its better to have it as separate events

rkapka
rkapka previously approved these changes Dec 8, 2023
apis/eventstream/index.yaml Outdated Show resolved Hide resolved
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
@rolfyone
Copy link
Collaborator

Lets add these to the change doc:)

@james-prysm
Copy link
Contributor

james-prysm commented Dec 11, 2023

Biggest concern we have (prysm side) that still needs investigating is if this would clog up our event notifier based on its current design. may be a bigger task to add if its the case.

@rolfyone
Copy link
Collaborator

Biggest concern we have (prysm side) that still needs investigating is if this would clog up our event notifier based on its current design. may be a bigger task to add if its the case.

if we have that many slashings that this becomes an issue, that would be a concern in itself...

Copy link
Collaborator

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rolfyone rolfyone merged commit f82fff2 into master Dec 12, 2023
5 checks passed
@rolfyone rolfyone deleted the dapplion-slashing-event branch December 12, 2023 22:12
etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Dec 22, 2023
Add support for slashings on the beacon-API event stream for compat with
beacon-API specs.

- ethereum/beacon-APIs#376
etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Dec 22, 2023
Add support for slashings on the beacon-API event stream for compat with
beacon-API specs.

- ethereum/beacon-APIs#376
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants