Beacon node slasher improvement #13549
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nalepae
force-pushed
the
slasher-memory-leak-13430
branch
from
1f23ec6 to
39b37e6
Compare
nalepae
added a commit
that referenced
this pull request
Jan 29, 2024
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 #13549
nalepae
force-pushed
the
slasher-memory-leak-13430
branch
2 times, most recently
from
44bbbe1 to
c33ec19
Compare
saolyn
reviewed
Jan 29, 2024
beacon-chain/db/slasherkv/slasher.go
Outdated
| bkt := tx.Bucket(attestedEpochsByValidator) | ||
|
|
||
| min := i + batchSize |
There was a problem hiding this comment.
'min' collides with the 'builtin' function, could we consider renaming this variable?
beacon-chain/slasher/receive.go
Outdated
| @@ -12,10 +12,18 @@ import ( | |||
| "github.com/sirupsen/logrus" | |||
| ) | |||
|
|
|||
| const ( | |||
| couldNotSaveAttRecord = "Could not save attestation records to DB" | |||
| couldNotSaveSlashableAtt = "Could not check slashable attestations" | |||
There was a problem hiding this comment.
Suggested change
| couldNotSaveSlashableAtt = "Could not check slashable attestations" | |
| couldNotSaveSlashableAtt = "Could not save slashable attestations" |
There was a problem hiding this comment.
Actually the message is the OK but the name of the variable is bad. Will fix it.
Fixes #13550. In tests, `exitChan` are now useless since waitgroup are used to wait for all goroutines to be stopped.
- Rename some variables. - Add comments. - Use second element of `range` when possible.
Avoid mixes between `indice(s)`and `index(es)`.
- Same target with different signing roots - Same target with same signing roots
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`.
So we can add new proposals later.
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
Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com>
Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com>
nalepae
force-pushed
the
slasher-memory-leak-13430
branch
from
a23ed6f to
8d740aa
Compare
Well, even if, in our case, "happy path" mean slashing.
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.
Not to conflict with the new `min` built-in function.
nalepae
force-pushed
the
slasher-memory-leak-13430
branch
from
8d740aa to
e9afe03
Compare
rauljordan
approved these changes
Jan 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please read this PR commit by commit. Some commit messages contain a lot of information.
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
Commits with commit message ending by
NFC(no functional changes) do not contain any functional change.Which issues(s) does this PR fix?
Fixes:
--slasherpanics at shutdown. #13550 in commit 2756590