Skip to content

Conversation

@dknopik
Copy link
Member

@dknopik dknopik commented Jul 23, 2025

Thiiiis one is a bit spicy:

Currently, the slashing database only exposes functions to atomically check slashability of an attestation/block and then insert the corresponding data into the database. As this is done atomically, we can fearlessly sign the thing as other threads will definitely see the inserted data.

In Anchor, we first decide with the SSV committee which attestation/block we should sign, and then create a partial signature and broadcast it. Until now, we did the slashing check-and-insert right before creating the partial signature, and will continue to do so. However, we want to be able to check whether a thing is slashable in the first step, in order to not decide on a invalid value and then sign nothing, as the the slashing check fails.

Basically, we want to move from:

flowchart LR
    A@{ shape: sm-circ, label: "Small start" } --> Z[Wait for block]
    Z --> B[Decide block]
    B --> C{Is safe?}
    C -->|Yes| D[Create partial sig]
    C -->|No| E[Sign nothing]
Loading

to

flowchart LR
    A@{ shape: sm-circ, label: "Small start" } --> Z[Wait for block]
    Z --> C{"Is safe?<br>(check only)"}
    C -->|Yes| B[Decide block]
    C -->|No| Z
    B --> D{"Is safe?<br>(check and insert)"}
    D -->|Yes| H[Create partial sig]
    D -->|No| I[Sign nothing]
Loading

For this, we need to expose check only functions in the slashing database. This introduces a way for the user of the slashing database to shoot themselves in the foot.

jimmygchen added a commit that referenced this pull request Jul 24, 2025
@michaelsproul michaelsproul added val-client Relates to the validator client binary code-quality security labels Jul 25, 2025
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

I support these changes.

@michaelsproul suggested a good modification.

We probably should disallow these functions in lighthouse by doing something like this:
https://github.com/sigp/lighthouse/blob/stable/consensus/types/clippy.toml

and this:
https://github.com/sigp/lighthouse/blob/stable/consensus/types/src/lib.rs#L3-L10

@AgeManning AgeManning added the v8.0.0-rc.0 Q3 2025 release for Fusaka on Holesky label Sep 10, 2025
Copy link
Member Author

@dknopik dknopik left a comment

Choose a reason for hiding this comment

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

@michaelsproul good idea, done :)

*.tar.gz
/bin
genesis.ssz
/clippy.toml
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why this was here

block_header: &BeaconBlockHeader,
domain: Hash256,
) -> Result<Safe, NotSafe> {
#[allow(clippy::disallowed_methods)]
Copy link
Member Author

Choose a reason for hiding this comment

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

needed, as even disallowed methods may not call disallowed methods

@michaelsproul
Copy link
Member

CI should pass now, we've bumped the HTTP test timeout on unstable

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.

LGTM, thanks!

@michaelsproul michaelsproul added the ready-for-merge This PR is ready to merge. label Sep 11, 2025
@mergify
Copy link

mergify bot commented Sep 11, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@michaelsproul
Copy link
Member

@mergify requeue

@mergify
Copy link

mergify bot commented Sep 11, 2025

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@mergify
Copy link

mergify bot commented Sep 11, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@mergify mergify bot merged commit 5815681 into sigp:unstable Sep 11, 2025
46 of 47 checks passed
PoulavBhowmick03 pushed a commit to PoulavBhowmick03/lighthouse that referenced this pull request Sep 12, 2025
kevaundray pushed a commit to kevaundray/lighthouse that referenced this pull request Sep 13, 2025
jtraglia pushed a commit to jtraglia/lighthouse that referenced this pull request Sep 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-quality ready-for-merge This PR is ready to merge. security v8.0.0-rc.0 Q3 2025 release for Fusaka on Holesky val-client Relates to the validator client binary

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants