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

Re-enable Slasher E2E Test #13420

Conversation

brightredchilli
Copy link
Contributor

@brightredchilli brightredchilli commented Jan 5, 2024

What does this PR do? Why is it needed?
Bugfix, Re-enables the Slasher E2E Test.

In a nutshell, slashing was flaky because the other beacon node doesnt get a chance to propose. See #12415 (comment) for more details.

Which issues(s) does this PR fix?
Fixes #12415

Other notes for review

Tests ran 50 times locally:

[57 / 107] 1 / 1 tests; Testing //testing/endtoend:go_default_test (run 50 of 50); 115s darwin-sandbox
Target //testing/endtoend:go_default_test up-to-date:
  bazel-bin/testing/endtoend/go_default_test_/go_default_test
INFO: Elapsed time: 5804.892s, Critical Path: 120.36s

There are 2 commits to this PR:

  1. Commit 1 introduces the bare minimum fix by copy-pasting and duplicating the relevant parts in the test
  2. Commit 2 goes a bit further by refactoring and keeping the evaluators/slashing.go file cleaner. I didn't refactor the double block evalulator. This commit can be discarded or modified as necessary.

Also, I want to point out a couple of things that are different from before:

  1. This test used to double attest for the slot at currentslot-1, but that is no longer possible due to the chanage here. Double attesting for the current slot works just fine.
  2. Because the P2P network filters out messages that it has seen before, the double attestation sent to each node must vary slightly - I did this by attaching some random bytes to the block root, but something simpler like a counter could work too, or just taking the arg directly from the test callsite.

On another side note, I was able to run tests locally successfully with SecondsPerSlot reduced from 10 to 3. Might be worth looking into to speed up the integration tests.

@brightredchilli brightredchilli requested a review from a team as a code owner January 5, 2024 02:28
@CLAassistant
Copy link

CLAassistant commented Jan 5, 2024

CLA assistant check
All committers have signed the CLA.

nisdas
nisdas previously approved these changes Jan 18, 2024
@nisdas nisdas enabled auto-merge January 18, 2024 08:04
@nisdas
Copy link
Member

nisdas commented Jan 18, 2024

@brightredchilli Lint fails here

  testing/endtoend/evaluators/slashing.go:127: File is not `gofmt`-ed with `-s` (gofmt)
  
  testing/endtoend/evaluators/slashing_helper.go:20: File is not `gofmt`-ed with `-s` (gofmt)
  	valClient eth.BeaconNodeValidatorClient

Please run gofmt on the branch

auto-merge was automatically disabled January 18, 2024 15:38

Head branch was pushed to by a user without write access

@brightredchilli brightredchilli force-pushed the ying/reenable-slasher-e2e-issue12415 branch from 99cc5c8 to 64b356f Compare January 18, 2024 15:38
@brightredchilli
Copy link
Contributor Author

@nisdas ran gofmt on the files that i changed. gofmt also changes some other files in the repo but i didnt check those changes in.

@nisdas nisdas enabled auto-merge January 19, 2024 04:24
@nisdas nisdas added this pull request to the merge queue Jan 19, 2024
Merged via the queue into prysmaticlabs:develop with commit bfb6480 Jan 19, 2024
17 checks passed
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.

Re-enable Slasher E2E Test
3 participants