Add Slasher double block detection to E2E #5936
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5936 +/- ##
==========================================
- Coverage 59.95% 59.49% -0.47%
==========================================
Files 320 319 -1
Lines 27312 26819 -493
==========================================
- Hits 16376 15955 -421
+ Misses 8741 8705 -36
+ Partials 2195 2159 -36 |
endtoend/evaluators/slashing.go
Outdated
| return err | ||
| } | ||
| client := eth.NewBeaconNodeValidatorClient(conns[0]) | ||
| _, err = client.ProposeAttestation(ctx, att) |
There was a problem hiding this comment.
if _, err = client.ProposeAttestation
| } | ||
| slashedIndices = append(slashedIndices, committee[i]) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func proposeDoubleBlock(conns ...*grpc.ClientConn) error { |
There was a problem hiding this comment.
Maybe we can return more informative errors in each of the conditionals in this func, something like errors.Wrap(err, "could not do X")
endtoend/evaluators/slashing.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func to32BytesSlice(byteArray []byte) []byte { |
There was a problem hiding this comment.
We don't have this in shared? I think we use this in other packages as well
| } | ||
| slashedIndices = append(slashedIndices, committee[i]) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func proposeDoubleBlock(conns ...*grpc.ClientConn) error { | ||
| conn := conns[0] |
There was a problem hiding this comment.
what happen if len(conns) != 1 is empty? Probably best to check the length and return error
There was a problem hiding this comment.
It will always be something here, the test would fail much earlier elsewhere if it wasn't. That would mean we don't have any beacon nodes to connect to. At least 1 node is a guarantee here
There was a problem hiding this comment.
Agree. But better to be defensive. Someone brand new can be writing an e2e test and use proposeDoubleBlock but 100% unaware conns[0] vs [1], [2] will be used
There was a problem hiding this comment.
Also, why pass in conns, why do we need a list? why not just use conn?
There was a problem hiding this comment.
Putting the check here would be very odd, these style of tests follow an interface for all of E2E. Only conns[0] is used here but in the dozen or so other E2E tests, the other conns are used. Using multiple conns here just adds little benefit. E2E is designed with the assumption that there will always be at least 1 beacon node, because if there isn't, the test wouldn't progress.
There was a problem hiding this comment.
Tests are at least run with 1 node, so pinging for any chain data that shouldn't be node dependent is done with the node index that we know is running.
There was a problem hiding this comment.
Also, only using one beacon node here helps test --broadcast-slashing, its why I removed the connection to other nodes from injectDoubleVote.
There was a problem hiding this comment.
I see. I got confused. Perhaps we should add a comment to mention only con0 is explicitly used but input takes in a list of conns.
There was a problem hiding this comment.
Sure thing, that sounds good 👍
What type of PR is this?
Testing improvement
What does this PR do? Why is it needed?
This PR adds a test for double block detection to E2E, in order to test the slashers detection capabilities for broadcasted blocks.
This PR also adds
--broadcast-slashingto E2E, in order to conveniently test slashing broadcasting as well.