Validator: Safer pending attestation records flushing #8433
Conversation
| @@ -26,6 +27,37 @@ type AttestationRecord struct { | |||
| SigningRoot [32]byte | |||
| } | |||
|
|
|||
| func NewPendingAttestationRecords() *PendingAttestationRecords { | |||
There was a problem hiding this comment.
This conflicts with PendingAttestation used as part of prysm's core package. Throughout our code base, they refer to attestations that are queued to enter the chain, but perhaps we could choose a different name
There was a problem hiding this comment.
It's actually an official eth2 object https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/beacon-chain.md#pendingattestation
There was a problem hiding this comment.
Alright, I will rename
| @@ -258,13 +294,28 @@ func (s *Store) batchAttestationWrites(ctx context.Context) { | |||
| // and resets the list of batched attestations for future writes. | |||
| // This function notifies all subscribers for flushed attestations | |||
| // of the result of the save operation. | |||
| func (s *Store) flushAttestationRecords(ctx context.Context) { | |||
| func (s *Store) flushAttestationRecords(ctx context.Context, records []*AttestationRecord) { | |||
| if s.batchedAttestationsFlushInProgress { | |||
There was a problem hiding this comment.
Atomic bool could be better here, we have it in the shared/ package
There was a problem hiding this comment.
Definitely, thanks!
| func (s *Store) flushAttestationRecords(ctx context.Context) { | ||
| func (s *Store) flushAttestationRecords(ctx context.Context, records []*AttestationRecord) { | ||
| if s.batchedAttestationsFlushInProgress.IsSet() { | ||
| log.Error("Attempted to flush attestation records when already in progress") |
There was a problem hiding this comment.
Should this be at error level? maybe debug is appropriate. We should only have error logs when they are actionable to the user, and this is opaque from a user's perspective
There was a problem hiding this comment.
This should never ever happen. If it is happening, then it's a development bug that was recently introduced. I will add a comment to reflect the significance
| s.batchedAttestations = make([]*AttestationRecord, 0, attestationBatchCapacity) | ||
| } else { | ||
| // This should never happen. | ||
| log.WithError(err).Error("Failed to batch save attestation records, retrying in queue") |
What type of PR is this?
Other
What does this PR do? Why is it needed?
Better locking and flushing of this slice can help prevent dropped saves, in the event that there is already some batch save in progress.
Which issues(s) does this PR fix?
N/A
Other notes for review