-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Separate type for unaggregated network attestations #14659
Changes from all commits
f9232df
76de5f4
e741580
df5338a
c8ecac2
a32321f
4215a95
fd296d9
67e92e0
12795fb
9d70083
b04ccbe
cb58bd1
d3387b3
a9f3844
abf7e6d
023c99d
ce39492
0772e04
bd10a5d
9345e66
bd82335
00f579e
cb59940
82eeb8d
c13b7a1
4704dd3
324d6aa
4053c78
3a2b677
12c80f1
d3228b6
0c2da4d
41e5368
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -308,6 +308,16 @@ func TestValidateNilAttestation(t *testing.T) { | |
}, | ||
errString: "", | ||
}, | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A case with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but this is because there is no additional logic for single attestations. I can still add these tests if you feel strongly about it, but I don't see that much value in copy-pasting tests for different types when the logic is exactly the same |
||
name: "single attestation", | ||
attestation: ðpb.SingleAttestation{ | ||
Data: ðpb.AttestationData{ | ||
Target: ðpb.Checkpoint{}, | ||
Source: ðpb.Checkpoint{}, | ||
}, | ||
}, | ||
errString: "", | ||
}, | ||
} | ||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -285,8 +285,11 @@ func (s *Server) SubmitAttestationsV2(w http.ResponseWriter, r *http.Request) { | |
} | ||
} | ||
|
||
func (s *Server) handleAttestationsElectra(ctx context.Context, data json.RawMessage) (attFailures []*server.IndexedVerificationFailure, failedBroadcasts []string, err error) { | ||
var sourceAttestations []*structs.AttestationElectra | ||
func (s *Server) handleAttestationsElectra( | ||
ctx context.Context, | ||
data json.RawMessage, | ||
) (attFailures []*server.IndexedVerificationFailure, failedBroadcasts []string, err error) { | ||
var sourceAttestations []*structs.SingleAttestation | ||
|
||
if err = json.Unmarshal(data, &sourceAttestations); err != nil { | ||
return nil, nil, errors.Wrap(err, "failed to unmarshal attestation") | ||
|
@@ -296,7 +299,7 @@ func (s *Server) handleAttestationsElectra(ctx context.Context, data json.RawMes | |
return nil, nil, errors.New("no data submitted") | ||
} | ||
|
||
var validAttestations []*eth.AttestationElectra | ||
var validAttestations []*eth.SingleAttestation | ||
for i, sourceAtt := range sourceAttestations { | ||
att, err := sourceAtt.ToConsensus() | ||
if err != nil { | ||
|
@@ -316,31 +319,32 @@ func (s *Server) handleAttestationsElectra(ctx context.Context, data json.RawMes | |
validAttestations = append(validAttestations, att) | ||
} | ||
|
||
for i, att := range validAttestations { | ||
// Broadcast the unaggregated attestation on a feed to notify other services in the beacon node | ||
// of a received unaggregated attestation. | ||
// Note we can't send for aggregated att because we don't have selection proof. | ||
if !att.IsAggregated() { | ||
s.OperationNotifier.OperationFeed().Send(&feed.Event{ | ||
Type: operation.UnaggregatedAttReceived, | ||
Data: &operation.UnAggregatedAttReceivedData{ | ||
Attestation: att, | ||
}, | ||
}) | ||
for i, singleAtt := range validAttestations { | ||
targetState, err := s.AttestationStateFetcher.AttestationTargetState(ctx, singleAtt.Data.Target) | ||
if err != nil { | ||
return nil, nil, errors.Wrap(err, "could not get target state for attestation") | ||
} | ||
committee, err := corehelpers.BeaconCommitteeFromState(ctx, targetState, singleAtt.Data.Slot, singleAtt.CommitteeId) | ||
if err != nil { | ||
return nil, nil, errors.Wrap(err, "could not get committee for attestation") | ||
} | ||
att := singleAtt.ToAttestationElectra(committee) | ||
|
||
s.OperationNotifier.OperationFeed().Send(&feed.Event{ | ||
Type: operation.UnaggregatedAttReceived, | ||
Data: &operation.UnAggregatedAttReceivedData{ | ||
Attestation: att, | ||
}, | ||
}) | ||
|
||
wantedEpoch := slots.ToEpoch(att.Data.Slot) | ||
vals, err := s.HeadFetcher.HeadValidatorsIndices(ctx, wantedEpoch) | ||
if err != nil { | ||
failedBroadcasts = append(failedBroadcasts, strconv.Itoa(i)) | ||
continue | ||
} | ||
committeeIndex, err := att.GetCommitteeIndex() | ||
if err != nil { | ||
return nil, nil, errors.Wrap(err, "failed to retrieve attestation committee index") | ||
} | ||
subnet := corehelpers.ComputeSubnetFromCommitteeAndSlot(uint64(len(vals)), committeeIndex, att.Data.Slot) | ||
if err = s.Broadcaster.BroadcastAttestation(ctx, subnet, att); err != nil { | ||
subnet := corehelpers.ComputeSubnetFromCommitteeAndSlot(uint64(len(vals)), att.GetCommitteeIndex(), att.Data.Slot) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated to this PR but calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add this comment to my follow-up list |
||
if err = s.Broadcaster.BroadcastAttestation(ctx, subnet, singleAtt); err != nil { | ||
log.WithError(err).Errorf("could not broadcast attestation at index %d", i) | ||
failedBroadcasts = append(failedBroadcasts, strconv.Itoa(i)) | ||
continue | ||
|
@@ -350,13 +354,9 @@ func (s *Server) handleAttestationsElectra(ctx context.Context, data json.RawMes | |
if err = s.AttestationCache.Add(att); err != nil { | ||
log.WithError(err).Error("could not save attestation") | ||
} | ||
} else if att.IsAggregated() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. single attestation can't be aggregated |
||
if err = s.AttestationsPool.SaveAggregatedAttestation(att); err != nil { | ||
log.WithError(err).Error("could not save aggregated attestation") | ||
} | ||
} else { | ||
if err = s.AttestationsPool.SaveUnaggregatedAttestation(att); err != nil { | ||
log.WithError(err).Error("could not save unaggregated attestation") | ||
log.WithError(err).Error("could not save attestation") | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need the structure
AttestationElectra
? shouldn't this be removed? I understand that we still need it in consensus, but how about in the API here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
AttestationElectra
is still part of beacon block so it's needed for block related api end points right?