lambdaclass / lambdaclass/lambda_ethereum_consensus

Fix attestation validation coding and performance issues

Open
#1,397 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Elixir
Stars
109
Forks
41
PR merge metrics
No merged PRs in 30d

Description

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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading StateTransition.Operations.process_attestation_batch and validate_attestation alongside ForkChoice.process_block, process_attestations, and Handlers.on_attestation. Compare the duplicated validation and accessor paths in operations.ex, handlers.ex, and accessors.ex, then inspect the disabled gossip validation in operations_collector.ex. Done should mean the duplicated work is cleaned up and attestation validation performance is improved without losing the required block and gossip checks.

Written by the indexing model from the issue text.

Assessment

Tech stack
elixir
Domain
backend, distributed-systems, performance
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.