-
Notifications
You must be signed in to change notification settings - Fork 146
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't done a deep review, but the general spec portion looks good other than the bug noted below
attester_slashing.slashable_attestation_2.validator_indices and | ||
state.validator_registry[index].slashed_epoch > current_epoch | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to validate that len(slashable_indices) > 0
here.
"Verify that len(slashable_indices) >= 1" in the spec
32e41e5
to
05c5f64
Compare
05c5f64
to
84bf7ac
Compare
assert ( | ||
new_state.validator_balances[attester_index] < state.validator_balances[attester_index] | ||
) | ||
# else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we fill some failure cases in before merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh thanks! That was some leftover. Added a light test!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks great -- had a few comments/questions and i think there is a typo that should be fixed (see comment about double assignment)
i wouldn't hate having more tests a la danny's comment but would lean towards being light on tests as we move to implement the full state transition as quickly as possible :)
batched_block_roots=updated_batched_block_roots, | ||
) | ||
return state | ||
return process_slot_transition(state, self.config, previous_block_root) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 i like the symmetry here to move this logic to its own file (and match the structure of the operation processing)
(and also in general prefer many small files over few big files :D)
1. Add test of `is_surround_vote` case 2. Enable `process_attester_slashings`
1. Add test for `process_attester_slashings` 2. Fix the missing `validate_slashable_indices` (thanks to djrtwo) 3. Fix `create_mock_slashable_attestation` - should have used the same attester to mock slashable_attestation
84bf7ac
to
9e088c8
Compare
What was wrong?
process_attester_slashings
and related validationsHow was it fixed?
Implement
process_attester_slashings
Implement
validate_attester_slashing
for validating eachAttesterSlashing
Add
validate_attester_slashing
tests intests/eth2/beacon/state_machines/forks/test_serenity_block_attester_slashing_validation.py
Address verify_slashable_attestation with custody bits all 0 OR all 1 consensus-specs#651
TODO:
process_attester_slashing
Cute Animal Picture