Update slasher service to Electra #14812
Conversation
|
|
||
| // Ensure the attestation with the lower data root is the first attestation. | ||
| if bytes.Compare(existingAttWrapper.DataRoot[:], incomingAttWrapper.DataRoot[:]) > 0 { | ||
| slashing = ðpb.AttesterSlashingElectra{ |
There was a problem hiding this comment.
Not covered (while the equivalent for pre-electra is covered).
There was a problem hiding this comment.
I don't think pre-Electra was covered. I added a Phase0 test too
beacon-chain/slasher/chunks_test.go
Outdated
| @@ -239,6 +283,49 @@ func TestMaxSpanChunksSlice_CheckSlashable(t *testing.T) { | |||
| require.NotEqual(t, (*ethpb.AttesterSlashing)(nil), slashing) | |||
There was a problem hiding this comment.
I think this assert is not valid any more:
If slashing is nil but with (*ethpb.AttesterSlashingElectra) type, then this require.NotEqual will be successful.
beacon-chain/slasher/chunks_test.go
Outdated
| slashing, err := chunk.CheckSlashable(ctx, slasherDB, validatorIdx, surroundedVote) | ||
| require.NoError(t, err) | ||
| // The old record should be converted to Electra and the resulting slashing should be an Electra slashing. | ||
| require.NotEqual(t, (*ethpb.AttesterSlashingElectra)(nil), slashing) |
There was a problem hiding this comment.
I think this assert is not valid any more:
If slashing is nil but with (*ethpb.AttesterSlashing) type, then this require.NotEqual will be successful.
(And it seems it is actually the case with this test.)
There was a problem hiding this comment.
You're right. I decided to use the reflect package because I think it's the easiest way to check for a non-nil interface. Simply checking against nil doesn't work
There was a problem hiding this comment.
Did this test change?
I still see require.NotEqual(t, (*ethpb.AttesterSlashingElectra)(nil), slashing) and slashing being actually nil of type AttSlashing in the test.
| unifyAttWrapperVersion(existingAttWrapper, incomingAttWrapper) | ||
|
|
||
| postElectra := existingAttWrapper.IndexedAttestation.Version() >= version.Electra | ||
| if postElectra { |
There was a problem hiding this comment.
Not covered. (Probably because of #14812 (comment))
| // Ensure the attestation with the lower data root is the first attestation. | ||
| if bytes.Compare(wrapper_1.DataRoot[:], wrapper_2.DataRoot[:]) > 0 { | ||
| postElectra := wrapper_1.IndexedAttestation.Version() >= version.Electra | ||
| if postElectra { |
Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
What type of PR is this?
Feature
What does this PR do? Why is it needed?
Updates the slasher service and the slasher simulator to handle
IndexedAttestationElectra. Because it is possible for an attester slashing to have one offending attestation pre-Electra and the other offending attestation post-Electra, we convert the pre-Electra attestation to a post-Electra one and construct anAttesterSlashingElectra. Because the only difference betweenIndexedAttestationandIndexedAttestationElectrais the maximum number of attesting indices (increased in Electra), the signature is still valid after the conversion.