Skip to content

Fix attestation validation coding and performance issues #1397

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

Open
ElFantasma opened this issue Mar 17, 2025 · 0 comments
Open

Fix attestation validation coding and performance issues #1397

ElFantasma opened this issue Mar 17, 2025 · 0 comments

Comments

@ElFantasma
Copy link
Contributor

Attestation signature verification is a costly operation. And it seems we are performing it twice per block processing.

Also, during the analysis I found that there is duplicated and difficult to follow code.

Performance issue:

We currently are performing attestation signature verification as part of the state transition in StateTransition.Operations.process_attestation_batch.

And later, similar validations (including signature verification) is performed in process_attestations inside ForkChoice.process_block.

Also, we should be doing the same validation when receiving attestations via gossip, but this was disabled as it seemed to be overwhelming the client.

I did a fast test on branch cache_attestations, caching the validation results (and the conversion from attestation to indexed_attestation) and found some improvement on the second time we validate attestations. See the following chart:

Image
where the yellow line is the second time we perform the validation.

This code should not be merged as it is a fast messy test and it can have memory issues on the number of attestations being cached.

Code duplication issues:

We have almost identical code on ForkChoice.Hanlders.check_valid_indexed_attestation and StateTransition.Operations.check_valid_indexed_attestation. And the code using those functions is pretty similar as well:

See ForkChoice.Hanlders.on_attestation vs StateTransition.Operations.validate_attestation.

This also leads to similar Accessors:

See Accessors.get_indexed_attestation vs. Accessors.get_committee_indexed_attestation that in time also call almost identical functions Accessors.get_attesting_indices and Accessors.get_committee_attesting_indices.

Logic of both functions do the same, except one of them returns the indices in a sorted fashion, and the other returns the indices unsorted (but it is later sorted). And also one of them expect the list of indices and the other expects a beacon_committee to calculate the indices.

Also obtaining the attesting indexes is performed several times in different parts of the code and could be cached: See here, here and here

This code should be cleaned up, and duplication should be removed, but it is not so simple as this code follows the annotated spec, and in order to correct this we should move away from it. It also seems that currently the performance bottleneck is somewhere else so paying the price of moving away from the spec seems unnecessary at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

1 participant