Decompose Electra block attestations

[rkapka]

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Prysm nodes on Electra devnets experience attestation packing issues. Every block is filled up to the brim with 8 attestations. Except for attestations for the current slot, which should go into the block, it includes redundant attestations from previous slots that have been already included by proposers of older blocks.

The reason why this happens is that we don't delete aggregated attestations from the pool correctly when we receive a block. Redundant packing is mostly a "cosmetic" bug, but not deleting attestations means that we keep much more of them in the pool than necessary, bloating the size of the pool needlessly. And we don't delete them because we can't compare Electra block attestations with aggregated attestations in the same way we compare pre-Electra block attestations. Because in Electra block attestations can combine aggregates for multiple committees, comparing attestations bits of a block attestation with attestation bits of an aggregate can cause unexpected results, leading to covered aggregates not being removed from the pool. To make sure aggregates are removed, we decompose the block attestation into dummy aggregates, with each aggregate accounting for one committee. This allows us to compare aggregates in the same way it's done pre-Electra.

Other notes for review

Another suggested approach was to use the SlotCommitteeCount helper and not creating dummy aggregates, but methods that delete aggregates expect attestations as parameters, so that would require changing how these interfaces work, and that's much more invasive than the current approach.

Acknowledgements

• I have read CONTRIBUTING.md.
• I have included a uniquely named changelog fragment file.
• I have added a description to this PR with sufficient context for reviewers to understand this PR.

[saolyn]

Should we check whether committeeIndices and committees have the same length?

[james-prysm]

at first I thought maybe seconding this but are there some guarantees due to already being from the head state/headblock?

[rkapka]

I will add a sanity check

[james-prysm]

what about breaking this out into its own function and using a switch statement instead? it'll read easier than seeing continue several times here

func (s *Service) pruneAttsFromPool(ctx context.Context, headState state.BeaconState, headBlock interfaces.ReadOnlySignedBeaconBlock) error {
    atts := headBlock.Block().Body().Attestations()
    for _, att := range atts {
        if err := s.handleAttestation(ctx, headState, att); err != nil {
            return err
        }
    }
    return nil
}

func (s *Service) handleAttestation(ctx context.Context, headState state.BeaconState, att ethpb.Att) error {
    switch {
    case !att.IsAggregated():
        return s.cfg.AttPool.DeleteUnaggregatedAttestation(att)

    case att.Version() == version.Phase0:
        if features.Get().EnableExperimentalAttestationPool {
            return s.cfg.AttestationCache.DeleteCovered(att)
        }
        return s.cfg.AttPool.DeleteAggregatedAttestation(att)

    default:
        return s.pruneElectraAttsFromPool(ctx, headState, att)
    }
}

[james-prysm]

would it be helpful to wrap these errors to know if it's unaggregated or based on electra?

[james-prysm]

is something like the below needed for safety?
if offset+uint64(len(c)) > uint64(att.GetAggregationBits().Len()) {
return errors.New("committee bits exceed available aggregation bits")
}

I think you can also do for j := range len(c), don't have a strong preference for it but it can be useful.

is it possible to have a test for the check I suggested?

[rkapka]

is something like the below needed for safety?

This can never happen because it means that the attestation bits have wrong lengths, and such a block will get rejected.

I think you can also do for j := range len(c)

This requires j to be an int, and then I will have to do casting two times below.

is it possible to have a test for the check I suggested?

As per above, I don't think it's necessary because it can never happen (unless we have a bug in some other place of the code, which is another story)

[potuz]

Why returning here instead of continuing and removing the rest?

[potuz]

I have several issues/questions with this function

1. Are we certain these committees will be cached already? I wonder if this is not a DOS when importing an old block that will require to do a computation.
2. This function is called in a loop for att but the committees are all taken from the same state. Are the successive calls No-ops or if they aren't you should pass the committees as parameters.
3. Is there a way of knowing if we have the attestation in the pool already before doing any processing on the block ones?