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.

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

Beacon node slasher improvement #13549

Merged
merged 29 commits into from Jan 31, 2024
Merged

Beacon node slasher improvement #13549

merged 29 commits into from Jan 31, 2024

Commits on Jan 30, 2024

  1. Slasher: Ensure all gorouting are stopped before running Stop actions. 

    Fixes #13550.
In tests, `exitChan` are now useless since waitgroup are used to wait
for all goroutines to be stopped.
    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    9270c52 View commit details
    Browse the repository at this point in the history

  2. slasher.go: Add comments and rename some variables. - NFC

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    d1d287b View commit details
    Browse the repository at this point in the history

  3. detect_blocks.go: Improve. - NFC 

    - Rename some variables.
- Add comments.
- Use second element of `range` when possible.
    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    45ce8ed View commit details
    Browse the repository at this point in the history

  4. chunks.go: Remove _receivers. - NFC

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    0189027 View commit details
    Browse the repository at this point in the history

  5. validateAttestationIntegrity: Improve documentation. - NFC

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    5add622 View commit details
    Browse the repository at this point in the history

  6. filterAttestations: Avoid elseand rename variable. - NFC

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    d8462fd View commit details
    Browse the repository at this point in the history

  7. slasher.go: Fix and add comments.

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    0f53312 View commit details
    Browse the repository at this point in the history

  8. SaveAttestationRecordsForValidators: Remove unused code.

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    b6bb491 View commit details
    Browse the repository at this point in the history

  9. LastEpochWrittenForValidators: Name variables consistently. - NFC 

    Avoid mixes between `indice(s)`and `index(es)`.
    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    5c4180c View commit details
    Browse the repository at this point in the history

  10. SaveLastEpochsWrittenForValidators: Name variables consistently. - NFC

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    7049d77 View commit details
    Browse the repository at this point in the history

  11. CheckAttesterDoubleVotes: Rename variables and add comments. - NFC

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    2508142 View commit details
    Browse the repository at this point in the history

  12. schema.go: Add comments. - NFC

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    eefb8ae View commit details
    Browse the repository at this point in the history

  13. processQueuedAttestations: Add comments. - NFC

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    c19b8ee View commit details
    Browse the repository at this point in the history

  14. checkDoubleVotes: Rename variable. - NFC

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    f6b96ed View commit details
    Browse the repository at this point in the history

  15. Test_processQueuedAttestations: Ensure there is no error log.

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    104e794 View commit details
    Browse the repository at this point in the history

  16. shouldNotBeSlashable => shouldBeSlashable

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    257b0cb View commit details
    Browse the repository at this point in the history

  17. Test_processQueuedAttestations: Add 2 test cases: 

    - Same target with different signing roots
- Same target with same signing roots
    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    efdbeb0 View commit details
    Browse the repository at this point in the history

  18. checkDoubleVotesOnDisk ==> checkDoubleVotes. 

    Before this commit, `checkDoubleVotes` did two tasks:
- Checking if there are any slashable double votes in the input
  list of attestations with respect to each other.
- Checking if there are any slashable double votes in the input
  list of attestations with respect to our database.

However, `checkDoubleVotes` is called only in
`checkSlashableAttestations`.

And `checkSlashableAttestations` is called only in:
- `processQueuedAttestations`, and in
- `IsSlashableAttestation`

Study of case `processQueuedAttestations`:
---------------------------------------------
In `processQueuedAttestations`, `checkSlashableAttestations`
is ALWAYS called after
`Database.SaveAttestationRecordsForValidators`.

It means that, when calling `checkSlashableAttestations`,
`validAtts` are ALREADY stored in the DB.

Each attestation of `validAtts` will be checked twice:
- Against the other attestations of `validAtts` (the portion of
  deleted code)
- Against the content of the database.

One of those two checks is redundent.
==> We can remove the check against other attestations in `validAtts`.

Study of case `Database.SaveAttestationRecordsForValidators`:
----------------------------------------------------------------
In `Database.SaveAttestationRecordsForValidators`,
`checkSlashableAttestations` is ALWAYS called with a list of
attestations containing only ONE attestation.

This only attestaion will be checked twice:
- Against itself, and an attestation cannot conflict with itself.
- Against the content of the database.

==> We can remove the check against other attestations in `validAtts`.

=========================

In both cases, we showed that we can remove the check of attestation
against the content of `validAtts`, and the corresponding test
`Test_checkDoubleVotes_SlashableInputAttestations`.
    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    72f108b View commit details
    Browse the repository at this point in the history

  19. Test_processQueuedBlocks_DetectsDoubleProposals: Wrap proposals. 

    So we can add new proposals later.
    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    e171085 View commit details
    Browse the repository at this point in the history

  20. Fix slasher multiple proposals false negative. 

    If a first batch of blocks is sent with:
- validator 1 - slot 4 - signing root 1
- validator 1 - slot 5 - signing root 1

Then, if a second batch of blocks is sent with:
- validator 1 - slot 4 - signing root 2

Because we have two blocks proposed by the same validator (1) and for
the same slot (4), but with two different signing roots (1 and 2), the
validator 1 should be slashed.

This is not the case before this commit.
A new test case has been added as well to check this.

Fixes #13551
    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    d546115 View commit details
    Browse the repository at this point in the history

  21. params.go: Change comments. - NFC

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    9f1c451 View commit details
    Browse the repository at this point in the history

  22. CheckSlashable: Keep the happy path without indentation.

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    f19225d View commit details
    Browse the repository at this point in the history

  23. detectAllAttesterSlashings => checkSurrounds.

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    025110e View commit details
    Browse the repository at this point in the history

  24. Update beacon-chain/db/slasherkv/slasher.go 

    Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com>
    @nalepae @saolyn
    nalepae and saolyn committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    6e42331 View commit details
    Browse the repository at this point in the history

  25. Update beacon-chain/db/slasherkv/slasher.go 

    Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com>
    @nalepae @saolyn
    nalepae and saolyn committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    084cd08 View commit details
    Browse the repository at this point in the history

  26. CheckAttesterDoubleVotes: Keep happy path without indentation. 

    Well, even if, in our case, "happy path" mean slashing.
    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    67235ae View commit details
    Browse the repository at this point in the history

  27. 'SaveAttestationRecordsForValidators': Save the first attestation. 

    In case of multiple votes, arbitrarily save the first attestation.
Saving the first one in particular has no functional impact,
since in any case all attestations will be tested against
the content of the database. So all but the first one will be
detected as slashable.

However, saving the first one and not an other one let us not
to modify the end to end tests, since they expect the first one
to be saved in the database.
    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    74fe7af View commit details
    Browse the repository at this point in the history

  28. Rename min => minimum. 

    Not to conflict with the new `min` built-in function.
    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    fec9d56 View commit details
    Browse the repository at this point in the history

  29. couldNotSaveSlashableAtt ==> couldNotCheckSlashableAtt

    @nalepae
    nalepae committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    e9afe03 View commit details
    Browse the repository at this point in the history