-
Notifications
You must be signed in to change notification settings - Fork 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
base: develop
Are you sure you want to change the base?
Changes from 15 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
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 |
---|---|---|
|
@@ -36,6 +36,13 @@ type AttestationElectra struct { | |
CommitteeBits string `json:"committee_bits"` | ||
} | ||
|
||
type SingleAttestation struct { | ||
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. Do we still need the structure 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 think |
||
CommitteeIndex string `json:"committee_index"` | ||
AttesterIndex string `json:"attester_index"` | ||
Data *AttestationData `json:"data"` | ||
Signature string `json:"signature"` | ||
} | ||
|
||
type AttestationData struct { | ||
Slot string `json:"slot"` | ||
CommitteeIndex string `json:"index"` | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -32,7 +32,7 @@ func ValidateNilAttestation(attestation ethpb.Att) error { | |||||||||||||||||||||||
if attestation.GetData().Target == nil { | ||||||||||||||||||||||||
return errors.New("attestation's target can't be nil") | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
if attestation.GetAggregationBits() == nil { | ||||||||||||||||||||||||
if !attestation.IsSingle() && attestation.GetAggregationBits() == nil { | ||||||||||||||||||||||||
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 think |
||||||||||||||||||||||||
return errors.New("attestation's bitfield can't be nil") | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
return nil | ||||||||||||||||||||||||
Comment on lines
+35
to
38
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.
Suggested change
|
||||||||||||||||||||||||
|
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 |
||
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 |
---|---|---|
|
@@ -274,8 +274,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") | ||
|
@@ -285,7 +288,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 { | ||
|
@@ -306,14 +309,23 @@ func (s *Server) handleAttestationsElectra(ctx context.Context, data json.RawMes | |
} | ||
|
||
for i, att := range validAttestations { | ||
targetState, err := s.AttestationStateFetcher.AttestationTargetState(ctx, att.Data.Target) | ||
if err != nil { | ||
return nil, nil, errors.Wrap(err, "could not get target state for attestation") | ||
} | ||
committee, err := corehelpers.BeaconCommitteeFromState(ctx, targetState, att.Data.Slot, att.CommitteeId) | ||
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. Not part of this PR but the bad design here is that we can only access the beacon committee while holding the state, when most of the time this committee is already cached. We should have a method to access the beacon committee from the checkpoint and only handle the failure withing that method, that is, the function would be |
||
if err != nil { | ||
return nil, nil, errors.Wrap(err, "could not get committee for attestation") | ||
} | ||
|
||
// 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 !corehelpers.IsAggregated(att) { | ||
s.OperationNotifier.OperationFeed().Send(&feed.Event{ | ||
Type: operation.UnaggregatedAttReceived, | ||
Data: &operation.UnAggregatedAttReceivedData{ | ||
Attestation: att, | ||
Attestation: att.ToAttestationElectra(committee), | ||
}, | ||
}) | ||
} | ||
|
@@ -336,11 +348,11 @@ func (s *Server) handleAttestationsElectra(ctx context.Context, data json.RawMes | |
} | ||
|
||
if corehelpers.IsAggregated(att) { | ||
if err = s.AttestationsPool.SaveAggregatedAttestation(att); err != nil { | ||
if err = s.AttestationsPool.SaveAggregatedAttestation(att.ToAttestationElectra(committee)); err != nil { | ||
log.WithError(err).Error("could not save aggregated attestation") | ||
} | ||
} else { | ||
if err = s.AttestationsPool.SaveUnaggregatedAttestation(att); err != nil { | ||
if err = s.AttestationsPool.SaveUnaggregatedAttestation(att.ToAttestationElectra(committee)); err != nil { | ||
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. Same here. |
||
log.WithError(err).Error("could not save unaggregated attestation") | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ import ( | |
"github.com/prysmaticlabs/prysm/v5/crypto/bls" | ||
"github.com/prysmaticlabs/prysm/v5/monitoring/tracing/trace" | ||
ethpb "github.com/prysmaticlabs/prysm/v5/proto/prysm/v1alpha1" | ||
"github.com/prysmaticlabs/prysm/v5/runtime/version" | ||
"github.com/prysmaticlabs/prysm/v5/time/slots" | ||
"google.golang.org/grpc/codes" | ||
"google.golang.org/grpc/status" | ||
|
@@ -44,7 +45,7 @@ func (vs *Server) ProposeAttestation(ctx context.Context, att *ethpb.Attestation | |
ctx, span := trace.StartSpan(ctx, "AttesterServer.ProposeAttestation") | ||
defer span.End() | ||
|
||
resp, err := vs.proposeAtt(ctx, att, att.GetData().CommitteeIndex) | ||
resp, err := vs.proposeAtt(ctx, att, nil, att.GetData().CommitteeIndex) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
@@ -62,24 +63,31 @@ func (vs *Server) ProposeAttestation(ctx context.Context, att *ethpb.Attestation | |
|
||
// ProposeAttestationElectra is a function called by an attester to vote | ||
// on a block via an attestation object as defined in the Ethereum specification. | ||
func (vs *Server) ProposeAttestationElectra(ctx context.Context, att *ethpb.AttestationElectra) (*ethpb.AttestResponse, error) { | ||
func (vs *Server) ProposeAttestationElectra(ctx context.Context, att *ethpb.SingleAttestation) (*ethpb.AttestResponse, error) { | ||
ctx, span := trace.StartSpan(ctx, "AttesterServer.ProposeAttestationElectra") | ||
defer span.End() | ||
|
||
targetState, err := vs.AttestationStateFetcher.AttestationTargetState(ctx, att.Data.Target) | ||
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. Any concerns with this line: for every attestation we propose, we need to retrieve the target state. What are the implications of this? 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. Why not just return the committee? This is what you need anyway instead of the full state. 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. Take a look at my other comment, the bad design here is that the committee requires the state when most times it won't even use it. We need to write a helper that returns the committee from the cache and if it's not cached then it takes the state. This function should take a checkpoint |
||
if err != nil { | ||
return nil, status.Error(codes.Internal, "Could not get target state") | ||
} | ||
committeeIndex, err := att.GetCommitteeIndex() | ||
if err != nil { | ||
return nil, err | ||
return nil, status.Error(codes.Internal, "Could not get committee index") | ||
} | ||
committee, err := helpers.BeaconCommitteeFromState(ctx, targetState, att.Data.Slot, committeeIndex) | ||
if err != nil { | ||
return nil, status.Error(codes.Internal, "Could not get committee") | ||
} | ||
|
||
resp, err := vs.proposeAtt(ctx, att, committeeIndex) | ||
resp, err := vs.proposeAtt(ctx, att, committee, committeeIndex) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
go func() { | ||
ctx = trace.NewContext(context.Background(), trace.FromContext(ctx)) | ||
attCopy := att.Copy() | ||
if err := vs.AttPool.SaveUnaggregatedAttestation(attCopy); err != nil { | ||
if err := vs.AttPool.SaveUnaggregatedAttestation(att.ToAttestationElectra(committee)); err != nil { | ||
log.WithError(err).Error("Could not save unaggregated attestation") | ||
return | ||
} | ||
|
@@ -136,14 +144,29 @@ func (vs *Server) SubscribeCommitteeSubnets(ctx context.Context, req *ethpb.Comm | |
return &emptypb.Empty{}, nil | ||
} | ||
|
||
func (vs *Server) proposeAtt(ctx context.Context, att ethpb.Att, committee primitives.CommitteeIndex) (*ethpb.AttestResponse, error) { | ||
func (vs *Server) proposeAtt( | ||
ctx context.Context, | ||
att ethpb.Att, | ||
committee []primitives.ValidatorIndex, // required post-Electra | ||
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 don't understand as to why 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. you need the committee to convert the attestation to an electra attestation to send the feed... perhaps that can be taken away 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. That's what I thought initially, but after looking closer, the feed takes Example, this will build:
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. Discussed offline, we'll take the internal type to track a bit field such that we wont have to be passing committee around |
||
committeeIndex primitives.CommitteeIndex, | ||
) (*ethpb.AttestResponse, error) { | ||
if _, err := bls.SignatureFromBytes(att.GetSignature()); err != nil { | ||
return nil, status.Error(codes.InvalidArgument, "Incorrect attestation signature") | ||
} | ||
|
||
root, err := att.GetData().HashTreeRoot() | ||
if err != nil { | ||
return nil, status.Errorf(codes.Internal, "Could not tree hash attestation: %v", err) | ||
return nil, status.Errorf(codes.Internal, "Could not get attestation root: %v", err) | ||
} | ||
|
||
var singleAtt *ethpb.SingleAttestation | ||
if att.Version() >= version.Electra { | ||
var ok bool | ||
singleAtt, ok = att.(*ethpb.SingleAttestation) | ||
if !ok { | ||
return nil, status.Errorf(codes.Internal, "Attestation has wrong type (expected %T, got %T)", ðpb.SingleAttestation{}, att) | ||
} | ||
att = singleAtt.ToAttestationElectra(committee) | ||
} | ||
|
||
// Broadcast the unaggregated attestation on a feed to notify other services in the beacon node | ||
|
@@ -161,10 +184,16 @@ func (vs *Server) proposeAtt(ctx context.Context, att ethpb.Att, committee primi | |
if err != nil { | ||
return nil, err | ||
} | ||
subnet := helpers.ComputeSubnetFromCommitteeAndSlot(uint64(len(vals)), committee, att.GetData().Slot) | ||
subnet := helpers.ComputeSubnetFromCommitteeAndSlot(uint64(len(vals)), committeeIndex, att.GetData().Slot) | ||
|
||
// Broadcast the new attestation to the network. | ||
if err := vs.P2P.BroadcastAttestation(ctx, subnet, att); err != nil { | ||
var attToBroadcast ethpb.Att | ||
if singleAtt != nil { | ||
attToBroadcast = singleAtt | ||
} else { | ||
attToBroadcast = att | ||
} | ||
if err := vs.P2P.BroadcastAttestation(ctx, subnet, attToBroadcast); err != nil { | ||
return nil, status.Errorf(codes.Internal, "Could not broadcast attestation: %v", err) | ||
} | ||
|
||
|
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.
Please put in unreleased section