Skip to content

Reject attestations to blocks prior to the split #7084

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Mar 19, 2025
Merged

Reject attestations to blocks prior to the split #7084

merged 10 commits into from
Mar 19, 2025

Conversation

eserilev
Copy link
Member

@eserilev eserilev commented Mar 6, 2025

Closes #7083

Reject and penalize peers for gossiping attestations for blocks prior to the current split point

@eserilev eserilev requested a review from jxs as a code owner March 6, 2025 06:44
@eserilev eserilev added UX-and-logs v7.0.0-beta.clean Clean release post Holesky rescue labels Mar 6, 2025
@eserilev eserilev linked an issue Mar 6, 2025 that may be closed by this pull request
Reject attestations to blocks prior to the split #7083
Closed
@michaelsproul michaelsproul mentioned this pull request Mar 10, 2025
Open
michaelsproul
michaelsproul requested changes Mar 10, 2025
View reviewed changes
beacon_node/beacon_chain/src/attestation_verification.rs Outdated
let is_descendant_from_split_block =
split.slot == 0 || fork_choice.is_descendant(split.block_root, attestation_block_root);

if fork_choice.is_finalized_checkpoint_or_descendant(attestation_block_root)
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be better to fold these checks into verify_head_block_is_known, which is already trying to do some checks for descent from finalization. That function currently makes the (wrong) assumption that a block present in fork choice descends from finalization (this isn't true in general because fork choice is sometimes pruned on a delay).

So I think we can add these checks in the case where block_opt is Some

Copy link
Member Author

@eserilev eserilev Mar 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Added the check in verify_head_block_is_known when block_opt is Some

@michaelsproul michaelsproul added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Mar 10, 2025
@eserilev eserilev force-pushed the reject-attestations-prior-to-split branch from a2d3380 to efaab87 Compare March 10, 2025 21:17
@eserilev eserilev changed the base branch from holesky-rescue to release-v7.0.0 March 10, 2025 21:17
@eserilev eserilev added the v7.0.0 New release c. Q1 2025 label Mar 10, 2025
michaelsproul
michaelsproul reviewed Mar 11, 2025
View reviewed changes
@eserilev eserilev added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Mar 13, 2025
michaelsproul added a commit that referenced this pull request Mar 18, 2025
@michaelsproul
Reject attestations to blocks prior to the split (#7084)
4f8d4be 
Squashed commit of the following:

commit 0a53a24
Merge: 5c56a83 27aabe8
Author: Michael Sproul <[email protected]>
Date:   Tue Mar 18 16:28:25 2025 +1100

    Merge branch 'release-v7.0.0' into reject-attestations-prior-to-split

commit 5c56a83
Merge: fb9e049 3a555f5
Author: Eitan Seri-Levi <[email protected]>
Date:   Thu Mar 13 00:29:08 2025 -0600

    Merge branch 'release-v7.0.0' into reject-attestations-prior-to-split

commit fb9e049
Merge: f121c4a 9c4fc6e
Author: Eitan Seri-Levi <[email protected]>
Date:   Wed Mar 12 10:35:38 2025 -0600

    Merge branch 'release-v7.0.0' into reject-attestations-prior-to-split

commit f121c4a
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Mar 11 11:00:40 2025 -0600

    lump into

commit b8d2a8c
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Mar 11 10:58:46 2025 -0600

    add is descendant to fork-choice

commit 72ef42b
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Mar 11 10:53:16 2025 -0600

    fix descent from split check

commit 9d2ebf7
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Mar 11 10:46:00 2025 -0600

    remove unused error variant, lump finalization check into

commit 21478e5
Author: Eitan Seri-Levi <[email protected]>
Date:   Mon Mar 10 15:25:13 2025 -0600

    remove unneeded error variant

commit efaab87
Author: Eitan Seri-Levi <[email protected]>
Date:   Sat Mar 8 11:25:49 2025 -0700

    fix test

commit b911048
Author: Eitan Seri-Levi <[email protected]>
Date:   Wed Mar 5 23:42:46 2025 -0700

    Reject attestations on gossip that attest to blocks prior to the current split point
@michaelsproul michaelsproul mentioned this pull request Mar 18, 2025
Merged
michaelsproul added a commit that referenced this pull request Mar 18, 2025
@michaelsproul
Reject attestations to blocks prior to the split (#7084)
12937be 
Squashed commit of the following:

commit 0a53a24
Merge: 5c56a83 27aabe8
Author: Michael Sproul <[email protected]>
Date:   Tue Mar 18 16:28:25 2025 +1100

    Merge branch 'release-v7.0.0' into reject-attestations-prior-to-split

commit 5c56a83
Merge: fb9e049 3a555f5
Author: Eitan Seri-Levi <[email protected]>
Date:   Thu Mar 13 00:29:08 2025 -0600

    Merge branch 'release-v7.0.0' into reject-attestations-prior-to-split

commit fb9e049
Merge: f121c4a 9c4fc6e
Author: Eitan Seri-Levi <[email protected]>
Date:   Wed Mar 12 10:35:38 2025 -0600

    Merge branch 'release-v7.0.0' into reject-attestations-prior-to-split

commit f121c4a
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Mar 11 11:00:40 2025 -0600

    lump into

commit b8d2a8c
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Mar 11 10:58:46 2025 -0600

    add is descendant to fork-choice

commit 72ef42b
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Mar 11 10:53:16 2025 -0600

    fix descent from split check

commit 9d2ebf7
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Mar 11 10:46:00 2025 -0600

    remove unused error variant, lump finalization check into

commit 21478e5
Author: Eitan Seri-Levi <[email protected]>
Date:   Mon Mar 10 15:25:13 2025 -0600

    remove unneeded error variant

commit efaab87
Author: Eitan Seri-Levi <[email protected]>
Date:   Sat Mar 8 11:25:49 2025 -0700

    fix test

commit b911048
Author: Eitan Seri-Levi <[email protected]>
Date:   Wed Mar 5 23:42:46 2025 -0700

    Reject attestations on gossip that attest to blocks prior to the current split point
@michaelsproul michaelsproul removed the ready-for-review The code is ready for review label Mar 19, 2025
@michaelsproul michaelsproul added the ready-for-merge This PR is ready to merge. label Mar 19, 2025
mergify bot added a commit that referenced this pull request Mar 19, 2025
@mergify
Merge of #7084
cf5ff45
@mergify mergify bot mentioned this pull request Mar 19, 2025
Closed
6 tasks
@mergify mergify bot merged commit e4c9805 into sigp:release-v7.0.0 Mar 19, 2025
31 checks passed
@jimmygchen jimmygchen mentioned this pull request Apr 1, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelsproul michaelsproul michaelsproul approved these changes

@dapplion dapplion dapplion approved these changes

@jxs jxs Awaiting requested review from jxs

Assignees
No one assigned
Labels
ready-for-merge This PR is ready to merge. UX-and-logs v7.0.0-beta.clean Clean release post Holesky rescue v7.0.0 New release c. Q1 2025
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reject attestations to blocks prior to the split
3 participants
@eserilev @michaelsproul @dapplion