Redesign the pending attestation queue #15024
Conversation
| // This processes pending attestation queues on every `processPendingAttsPeriod`. | ||
| func (s *Service) processPendingAttsQueue() { | ||
| // This processes pending attestation queues on every processPendingAttsPeriod. | ||
| func (s *Service) runPendingAttsQueue() { |
There was a problem hiding this comment.
I think run is a better name given that attestations are processed in a continuous manner
| if err := s.cfg.attPool.SaveAggregatedAttestation(aggregate); err != nil { | ||
| log.WithError(err).Debug("Could not save aggregate attestation") | ||
| return | ||
| if att.IsAggregated() { |
There was a problem hiding this comment.
A SignedAggregateAttAndProof can hold an attestation with just one bit set, in which case saving the attestation through SaveAggregatedAttestation will fail
| @@ -352,31 +352,3 @@ func (s *Service) hasBlockAndState(ctx context.Context, blockRoot [32]byte) bool | |||
| hasState := hasStateSummary || s.cfg.beaconDB.HasState(ctx, blockRoot) | |||
| return hasState && s.cfg.chain.HasBlock(ctx, blockRoot) | |||
| } | |||
|
|
|||
| func (s *Service) saveToPendingAttPool(att eth.Att) (pubsub.ValidationResult, error) { | |||
There was a problem hiding this comment.
This function is no longer needed. Its only purpose was to create a dummy SignedAggregateAttestationAndProof and send it to the queue.
| @@ -259,54 +252,72 @@ func TestProcessPendingAtts_HasBlockSaveUnAggregatedAttElectra(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestProcessPendingAtts_NoBroadcastWithBadSignature(t *testing.T) { | |||
There was a problem hiding this comment.
This test was badly broken. The "wrong signature" attestations failed validation because of a different reason, not because of the signature. There was also a lot of unnecessary duplication such as creating two services.
What type of PR is this?
Other
Was this tested?
Yes, on Kurtosis. I sent 25% of attestations to the queue and they got processed successfully.
What does this PR do? Why is it needed?
Our current pending attestation queue's design is messy. The queue accepts only
SignedAggregateAttAndProofobjects but can handle both aggregates as well as unaggregated attestations, which are not part of anySignedAggregateAttAndProof. Because of this whenever we want to save an unaggregated attestation, we create a dummy aggregate object with the attestation inside. This has a few issues:SingleAttestationdoesn't have an associatedSignedAggregateAttAndProof, so we created aSignedAggregateAttestationAndProofSingleonly to be able to save single attestations to the queue. This type is not useful for anything else.IsAggregated()on the attestation to check whether we should process it as an aggregate or as an attestation. But if a "real" aggregate's attestation has only one bit set, it will be processed as an attestation when it should have been processed as an aggregate. This could be fixed by checking if the aggregate is dummy or not, but that requires keeping the dummy objects and the aim of this PR is to get rid of them.The solution in this PR changes the type of the field that holds pending atts from
map[[32]byte][]ethpb.SignedAggregateAttAndProoftomap[[32]byte][]any. That way we can save both types in the queue without having to resort to creating dummy objects. The biggest disadvantage of this approach is that you can technically save anything in the map, but there are several defensive measures in the pending att queue:savePendingAggregateandsavePendingAttassert typesprocessAttestationshas a type switch and will not process incorrect typesvalidatePendingAttshas a type switch and will remove incorrect typesWhat's more, all functions that deal with pending attestations are unexported.
Alternative designs
An alternative option would be to create an interface for both aggregates and attestations, but because attestations and aggregates are processed very differently (for example aggregate processing uses
validateAggregatedAttwhich requires aSignedAggregateAttAndProof), there would still be a need for type switching, and the interface would probably end up being just an empty marker interface.Another alternative would be to create a separate pending queue for each type, but that would warrant a larger redesign to remove duplication. For now we would have 2 queues, but in the future there could be more (I think ePBS introduces a concept of payload attestations, which could also be queued this way).
Acknowledgements