Skip to content

Separate type for unaggregated network attestations #14659

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

Merged
merged 34 commits into from
Jan 13, 2025
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
f9232df
definitions and gossip
rkapka Nov 22, 2024
76de5f4
Merge branch '__develop' into single-att
rkapka Nov 22, 2024
e741580
validator
rkapka Nov 22, 2024
df5338a
broadcast
rkapka Nov 25, 2024
c8ecac2
broadcast the correct att depending on version
rkapka Nov 26, 2024
a32321f
Merge branch '__develop' into single-att
rkapka Nov 27, 2024
4215a95
small updates
rkapka Nov 27, 2024
fd296d9
don't check bits after Electra
rkapka Nov 29, 2024
67e92e0
nitpick
rkapka Nov 29, 2024
12795fb
Merge branch '__develop' into single-att
rkapka Dec 10, 2024
9d70083
tests
rkapka Dec 10, 2024
b04ccbe
changelog <3
rkapka Dec 11, 2024
cb58bd1
Merge branch 'develop' into single-att
rkapka Dec 11, 2024
d3387b3
review
rkapka Dec 13, 2024
a9f3844
more review
rkapka Dec 18, 2024
abf7e6d
review yet again
rkapka Dec 23, 2024
023c99d
try a different design
rkapka Dec 23, 2024
ce39492
Merge branch '__develop' into single-att
rkapka Dec 26, 2024
0772e04
fix gossip issues
rkapka Dec 27, 2024
bd10a5d
Merge branch '__develop' into single-att
rkapka Dec 30, 2024
9345e66
cleanup
rkapka Dec 31, 2024
bd82335
tests
rkapka Dec 31, 2024
00f579e
Merge branch '__develop' into single-att
rkapka Jan 2, 2025
cb59940
reduce cognitive complexity
rkapka Jan 2, 2025
82eeb8d
Preston's review
rkapka Jan 7, 2025
c13b7a1
Merge branch '__develop' into single-att
rkapka Jan 7, 2025
4704dd3
move changelog entry to unreleased section
rkapka Jan 7, 2025
324d6aa
fix pending atts pool issues
rkapka Jan 8, 2025
4053c78
reviews
rkapka Jan 8, 2025
3a2b677
Merge branch '__develop' into single-att
rkapka Jan 8, 2025
12c80f1
Potuz's comments
rkapka Jan 9, 2025
d3228b6
test fixes
rkapka Jan 9, 2025
0c2da4d
Merge branch '__develop' into single-att
rkapka Jan 9, 2025
41e5368
Merge branch 'develop' into single-att
rkapka Jan 9, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve
- Added a Prometheus error counter metric for SSE requests.
- Save light client updates and bootstraps in DB.
- Added more comprehensive tests for `BlockToLightClientHeader`. [PR](https://github.com/prysmaticlabs/prysm/pull/14699)
- Separate type for unaggregated network attestations. [PR](https://github.com/prysmaticlabs/prysm/pull/14659)
Copy link
Member

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


### Changed

Expand Down
26 changes: 26 additions & 0 deletions api/server/structs/conversions.go
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,32 @@ func (a *AttestationElectra) ToConsensus() (*eth.AttestationElectra, error) {
}, nil
}

func (a *SingleAttestation) ToConsensus() (*eth.SingleAttestation, error) {
ci, err := strconv.ParseUint(a.CommitteeIndex, 10, 64)
if err != nil {
return nil, server.NewDecodeError(err, "CommitteeIndex")
}
ai, err := strconv.ParseUint(a.AttesterIndex, 10, 64)
if err != nil {
return nil, server.NewDecodeError(err, "AttesterIndex")
}
data, err := a.Data.ToConsensus()
if err != nil {
return nil, server.NewDecodeError(err, "Data")
}
sig, err := bytesutil.DecodeHexWithLength(a.Signature, fieldparams.BLSSignatureLength)
if err != nil {
return nil, server.NewDecodeError(err, "Signature")
}

return &eth.SingleAttestation{
CommitteeId: primitives.CommitteeIndex(ci),
AttesterIndex: primitives.ValidatorIndex(ai),
Data: data,
Signature: sig,
}, nil
}

func AttElectraFromConsensus(a *eth.AttestationElectra) *AttestationElectra {
return &AttestationElectra{
AggregationBits: hexutil.Encode(a.AggregationBits),
Expand Down
7 changes: 7 additions & 0 deletions api/server/structs/other.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ type AttestationElectra struct {
CommitteeBits string `json:"committee_bits"`
}

type SingleAttestation struct {
Copy link
Contributor

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?

Copy link
Collaborator

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?

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"`
Expand Down
2 changes: 1 addition & 1 deletion beacon-chain/core/helpers/attestation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think attestation.IsNil should handle this check and this function ValidateNilAttestation should not even exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add this comment to my follow-up list

return errors.New("attestation's bitfield can't be nil")
}
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if !attestation.IsSingle() && attestation.GetAggregationBits() == nil {
return errors.New("attestation's bitfield can't be nil")
}
return nil
if attestation.IsSingle() {
return nil
}
if attestation.GetAggregationBits() == nil {
return errors.New("attestation's bitfield can't be nil")
}
return nil

Expand Down
10 changes: 10 additions & 0 deletions beacon-chain/core/helpers/attestation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,16 @@ func TestValidateNilAttestation(t *testing.T) {
},
errString: "",
},
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A case with a ethpb.SingleAttestation and nil failures is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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: &ethpb.SingleAttestation{
Data: &ethpb.AttestationData{
Target: &ethpb.Checkpoint{},
Source: &ethpb.Checkpoint{},
},
},
errString: "",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
8 changes: 8 additions & 0 deletions beacon-chain/core/helpers/beacon_committee_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,14 @@ func TestVerifyBitfieldLength_OK(t *testing.T) {
assert.NoError(t, helpers.VerifyBitfieldLength(bf, committeeSize), "Bitfield is not validated when it was supposed to be")
}

func TestVerifyBitfieldLength_WrongLength(t *testing.T) {
helpers.ClearCache()

bf := bitfield.Bitlist{0xFF, 0x01}
committeeSize := uint64(4)
require.ErrorContains(t, "wanted participants bitfield length 4, got: 8", helpers.VerifyBitfieldLength(bf, committeeSize))
}

func TestCommitteeAssignments_CannotRetrieveFutureEpoch(t *testing.T) {
helpers.ClearCache()

Expand Down
4 changes: 2 additions & 2 deletions beacon-chain/p2p/gossip_topic_mappings.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func GossipTopicMappings(topic string, epoch primitives.Epoch) proto.Message {
return gossipMessage(topic)
case AttestationSubnetTopicFormat:
if epoch >= params.BeaconConfig().ElectraForkEpoch {
return &ethpb.AttestationElectra{}
return &ethpb.SingleAttestation{}
}
return gossipMessage(topic)
case AttesterSlashingSubnetTopicFormat:
Expand Down Expand Up @@ -101,7 +101,7 @@ func init() {
GossipTypeMapping[reflect.TypeOf(&ethpb.SignedBeaconBlockDeneb{})] = BlockSubnetTopicFormat
// Specially handle Electra objects.
GossipTypeMapping[reflect.TypeOf(&ethpb.SignedBeaconBlockElectra{})] = BlockSubnetTopicFormat
GossipTypeMapping[reflect.TypeOf(&ethpb.AttestationElectra{})] = AttestationSubnetTopicFormat
GossipTypeMapping[reflect.TypeOf(&ethpb.SingleAttestation{})] = AttestationSubnetTopicFormat
GossipTypeMapping[reflect.TypeOf(&ethpb.AttesterSlashingElectra{})] = AttesterSlashingSubnetTopicFormat
GossipTypeMapping[reflect.TypeOf(&ethpb.SignedAggregateAttestationAndProofElectra{})] = AggregateAndProofSubnetTopicFormat
}
2 changes: 1 addition & 1 deletion beacon-chain/p2p/gossip_topic_mappings_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func TestGossipTopicMappings_CorrectType(t *testing.T) {
_, ok = pMessage.(*ethpb.SignedBeaconBlockElectra)
assert.Equal(t, true, ok)
pMessage = GossipTopicMappings(AttestationSubnetTopicFormat, electraForkEpoch)
_, ok = pMessage.(*ethpb.AttestationElectra)
_, ok = pMessage.(*ethpb.SingleAttestation)
assert.Equal(t, true, ok)
pMessage = GossipTopicMappings(AttesterSlashingSubnetTopicFormat, electraForkEpoch)
_, ok = pMessage.(*ethpb.AttesterSlashingElectra)
Expand Down
2 changes: 1 addition & 1 deletion beacon-chain/p2p/types/object_mapping.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ func InitializeDataMaps() {
return &ethpb.Attestation{}, nil
},
bytesutil.ToBytes4(params.BeaconConfig().ElectraForkVersion): func() (ethpb.Att, error) {
return &ethpb.AttestationElectra{}, nil
return &ethpb.SingleAttestation{}, nil
},
}

Expand Down
1 change: 1 addition & 0 deletions beacon-chain/rpc/endpoints.go
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,7 @@ func (s *Service) beaconEndpoints(
FinalizationFetcher: s.cfg.FinalizationFetcher,
ForkchoiceFetcher: s.cfg.ForkchoiceFetcher,
CoreService: coreService,
AttestationStateFetcher: s.cfg.AttestationReceiver,
}

const namespace = "beacon"
Expand Down
24 changes: 18 additions & 6 deletions beacon-chain/rpc/eth/beacon/handlers_pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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 {
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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 BeaconCommitteeFromCheckpoint(cp, committeeId) and within that function we should check the committee cache, if it fails, then it should request a state and try to get it from the state.

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),
},
})
}
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

log.WithError(err).Error("could not save unaggregated attestation")
}
}
Expand Down
31 changes: 18 additions & 13 deletions beacon-chain/rpc/eth/beacon/handlers_pool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -498,13 +498,17 @@ func TestSubmitAttestations(t *testing.T) {
c.SlotsPerEpoch = 1
params.OverrideBeaconConfig(c)

_, keys, err := util.DeterministicDepositsAndKeys(1)
_, keys, err := util.DeterministicDepositsAndKeys(2)
require.NoError(t, err)
validators := []*ethpbv1alpha1.Validator{
{
PublicKey: keys[0].PublicKey().Marshal(),
ExitEpoch: params.BeaconConfig().FarFutureEpoch,
},
{
PublicKey: keys[1].PublicKey().Marshal(),
ExitEpoch: params.BeaconConfig().FarFutureEpoch,
},
}
bs, err := util.NewBeaconState(func(state *ethpbv1alpha1.BeaconState) error {
state.Validators = validators
Expand All @@ -521,9 +525,10 @@ func TestSubmitAttestations(t *testing.T) {

chainService := &blockchainmock.ChainService{State: bs}
s := &Server{
HeadFetcher: chainService,
ChainInfoFetcher: chainService,
OperationNotifier: &blockchainmock.MockOperationNotifier{},
HeadFetcher: chainService,
ChainInfoFetcher: chainService,
OperationNotifier: &blockchainmock.MockOperationNotifier{},
AttestationStateFetcher: chainService,
}
t.Run("V1", func(t *testing.T) {
t.Run("single", func(t *testing.T) {
Expand Down Expand Up @@ -732,7 +737,7 @@ func TestSubmitAttestations(t *testing.T) {
assert.Equal(t, http.StatusOK, writer.Code)
assert.Equal(t, true, broadcaster.BroadcastCalled.Load())
assert.Equal(t, 1, broadcaster.NumAttestations())
assert.Equal(t, "0x03", hexutil.Encode(broadcaster.BroadcastAttestations[0].GetAggregationBits()))
assert.Equal(t, primitives.ValidatorIndex(1), broadcaster.BroadcastAttestations[0].GetAttestingIndex())
assert.Equal(t, "0x8146f4397bfd8fd057ebbcd6a67327bdc7ed5fb650533edcb6377b650dea0b6da64c14ecd60846d5c0a0cd43893d6972092500f82c9d8a955e2b58c5ed3cbe885d84008ace6bd86ba9e23652f58e2ec207cec494c916063257abf285b9b15b15", hexutil.Encode(broadcaster.BroadcastAttestations[0].GetSignature()))
assert.Equal(t, primitives.Slot(0), broadcaster.BroadcastAttestations[0].GetData().Slot)
assert.Equal(t, primitives.CommitteeIndex(0), broadcaster.BroadcastAttestations[0].GetData().CommitteeIndex)
Expand Down Expand Up @@ -2344,8 +2349,8 @@ var (
]`
singleAttElectra = `[
{
"aggregation_bits": "0x03",
"committee_bits": "0x0100000000000000",
"committee_index": "0",
"attester_index": "1",
"signature": "0x8146f4397bfd8fd057ebbcd6a67327bdc7ed5fb650533edcb6377b650dea0b6da64c14ecd60846d5c0a0cd43893d6972092500f82c9d8a955e2b58c5ed3cbe885d84008ace6bd86ba9e23652f58e2ec207cec494c916063257abf285b9b15b15",
"data": {
"slot": "0",
Expand All @@ -2364,8 +2369,8 @@ var (
]`
multipleAttsElectra = `[
{
"aggregation_bits": "0x03",
"committee_bits": "0x0100000000000000",
"committee_index": "0",
"attester_index": "0",
"signature": "0x8146f4397bfd8fd057ebbcd6a67327bdc7ed5fb650533edcb6377b650dea0b6da64c14ecd60846d5c0a0cd43893d6972092500f82c9d8a955e2b58c5ed3cbe885d84008ace6bd86ba9e23652f58e2ec207cec494c916063257abf285b9b15b15",
"data": {
"slot": "0",
Expand All @@ -2382,8 +2387,8 @@ var (
}
},
{
"aggregation_bits": "0x03",
"committee_bits": "0x0100000000000000",
"committee_index": "0",
"attester_index": "1",
"signature": "0x8146f4397bfd8fd057ebbcd6a67327bdc7ed5fb650533edcb6377b650dea0b6da64c14ecd60846d5c0a0cd43893d6972092500f82c9d8a955e2b58c5ed3cbe885d84008ace6bd86ba9e23652f58e2ec207cec494c916063257abf285b9b15b15",
"data": {
"slot": "0",
Expand All @@ -2403,8 +2408,8 @@ var (
// signature is invalid
invalidAttElectra = `[
{
"aggregation_bits": "0x03",
"committee_bits": "0x0100000000000000",
"committee_index": "0",
"attester_index": "0",
"signature": "0x000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
"data": {
"slot": "0",
Expand Down
1 change: 1 addition & 0 deletions beacon-chain/rpc/eth/beacon/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,5 @@ type Server struct {
BLSChangesPool blstoexec.PoolManager
ForkchoiceFetcher blockchain.ForkchoiceFetcher
CoreService *core.Service
AttestationStateFetcher blockchain.AttestationStateFetcher
}
49 changes: 39 additions & 10 deletions beacon-chain/rpc/prysm/v1alpha1/validator/attester.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand All @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
}
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand as to why committee is needed here.
ProposeAttestationElectra calls this function and is already passing in SingleAttestation
BroadcastAttestation doesn't require committee

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 ethpb.Att, which a single attestation already satisfies. It should theoretically work if I just pass in SingleAttestation to the feed, unless there's something on the receiver side that specifically requires electra attestation and not single attestation. In that case, I would argue that the receiver should fix that.

Example, this will build:

func (vs *Server) proposeAtt(
	ctx context.Context,
	att ethpb.Att,
	committeeIndex primitives.CommitteeIndex,
) (*ethpb.AttestResponse, error) {

	vs.OperationNotifier.OperationFeed().Send(&feed.Event{
		Type: operation.UnaggregatedAttReceived,
		Data: &operation.UnAggregatedAttReceivedData{
			Attestation: att,
		},
	})

	subnet := helpers.ComputeSubnetFromCommitteeAndSlot(uint64(len(vals)), committeeIndex, att.GetData().Slot)
	if err := vs.P2P.BroadcastAttestation(ctx, subnet, att); err != nil {
		return nil, status.Errorf(codes.Internal, "Could not broadcast attestation: %v", err)
	}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The 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)", &ethpb.SingleAttestation{}, att)
}
att = singleAtt.ToAttestationElectra(committee)
}

// Broadcast the unaggregated attestation on a feed to notify other services in the beacon node
Expand All @@ -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)
}

Expand Down
Loading