reduce code duplication in process_attestations
#4141
Conversation
…ttesting_indicies, moving validation checks from process_attestation to get_attesting_indicies reverting print removing more prints
| @@ -603,18 +603,23 @@ def get_attesting_indices(state: BeaconState, attestation: Attestation) -> Set[V | |||
| Return the set of attesting indices corresponding to ``aggregation_bits`` and ``committee_bits``. | |||
| """ | |||
| output: Set[ValidatorIndex] = set() | |||
| assert attestation.data.index == 0 # [Modified in Electra:EIP7549] | |||
There was a problem hiding this comment.
do i need these markers? # [Modified in Electra:EIP7549]
| @@ -32,8 +31,6 @@ def test_invalid_attestation_data_index_not_zero(spec, state): | |||
| assert committee_index == spec.get_committee_indices(attestation.committee_bits)[0] | |||
| attestation.data.index = committee_index | |||
|
|
|||
| sign_attestation(spec, state, attestation) | |||
There was a problem hiding this comment.
sign_attestation calls get_attesting_indicies internally and fails here with it
|
Note that |
@mkalinin Are there potentially several unintended consequences in client implementations using a non pure getter in this case? |
Yes, I see the appeal to this. It does make
This is a good point. These checks are pretty minimal, but they are unnecessary in the accessor. The most efficient thing would be to only perform these checks once in I'm a bit torn on what I think we should do. I think I'm slightly in favor of leaving it how it is, just so that it's clear to clients that they do not need to perform these checks in the beacon state accessor. |
I noticed some differences in the implementation of process attestations for Prysm but still passed spec tests. We can reduce code duplication in
process_attestationsby replacing the component withget_attesting_indiciesand moving some checks there. The proposed change as well as test updates are below. As far as I could tell it should be ok to have those validations inget_attesting_indiciesbut please let me know if I am off base.a thank you to @jtraglia for the guidance