Skip to content
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

Merged
merged 34 commits into from
Jan 13, 2025
Merged

Separate type for unaggregated network attestations #14659

merged 34 commits into from
Jan 13, 2025

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented Nov 22, 2024

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Implements ethereum/consensus-specs#3900

New changes after initial reviews

  • I created new signed and unsigned AggregateAndProof types that hold SingleAttestation, which is needed to be able to save single attestations into pending atts, and simplified GetCommitteeIndex so that it doesn't return an error. Returning errors on getters is always very annoying to deal with, and we shouldn't get into the error scenario in the first place (most, if not all, potential errors are already handled in gossip checks).
  • I save the new SignedAggregateAttestationAndProofSingle type to pending atts and convert the underlying attestation into an AttestationElectra when processing this attestation. This is after validateUnaggregatedAttWithState.
    validateUnaggregatedAttWithState returns the committee so that we can convert the attestation.
  • I removed the validateBitLength helper because I need the committee outside of this function too, and it's already available before the function call. So calling VerifyBitfieldLength directly is simpler.
  • I renamed validateCommitteeIndexAndCount to validateCommitteeIndex as that's how it was called before Electra changes. We don't need the Electra validateCommitteeIndex and validateCommitteeIndexElectra functions because their main purpose was to validate the Electra version, and it's no longer needed with single attestation (there is validateAttestingIndex instead)

Original description

The above consensus change introduces a new SingleAttestation type for unaggregated attestations. AttestationElectra will still be used for aggregated atts. Using the single version in all of our code would be a huge undertaking because many components, such as the attestation pool and attestation caches, require attestations to be of the old format. Modifying these components to work both with pre-Electra attestations and the single attestation is risky and too much work for Pectra.

The main reason why SingleAttestation was introduced is to prevent a DoS attack, which means it's main value is at the level of gossip. Once we pass gossip validation checks, there is no downside to using the AttestationElectra for unaggregated attestations too. This allows us to leave most attestation-related code untouched. To have the best of both worlds, we convert the single attestation to an Electra attestation as quickly as possible and use the latter from that point on.

When converting from SingleAttestation to ElectraAttestation we need to find the index within the committee of the attester_index from the single attestation, because attester_index is the validator index within the whole state. To give an example:

Validator 9999 is an attester and the committee index to which it is assigned is 5. Within committee number 5, the attester's position/index is 7. SingleAttestation's values will be attester_index = 9999 and committee_index = 5. When converting to an ElectraAttestation, we need to set bit number 7 in attestation_bits. We need to obtain the number 7 somehow as it is not present in the SingleAttestation.

The way it's done is by using the AttestationStateFetcher to get the attestation's target state, then get the appropriate committee from that state and lastly call ToAttestationElectra on the SingleAttestation, passing the committee to the function.

Other notes for review

  • Obtaining the target state, getting the committee and converting takes less than 1ms. I presume it's because both the state and committee are cached.
  • Tested on Kurtosis with Prysm-only nodes and and Prysm+Lodestar.
  • The same "convert quickly to Electra attestation" design is what other clients are doing too to minimize disruption.

Acknowledgements

  • I have read CONTRIBUTING.md.
  • I have made an appropriate entry to CHANGELOG.md.
  • I have added a description to this PR with sufficient context for reviewers to understand this PR.

@rkapka rkapka marked this pull request as ready for review December 10, 2024 22:00
@rkapka rkapka requested a review from a team as a code owner December 10, 2024 22:00
@rkapka rkapka changed the title Single att Separate type for unaggregated network attestations Dec 10, 2024
Comment on lines 73 to 98

preState, err := s.cfg.chain.AttestationTargetState(ctx, data.Target)
if err != nil {
tracing.AnnotateError(span, err)
return pubsub.ValidationIgnore, err
}
committeeIndex, err := att.GetCommitteeIndex()
if err != nil {
tracing.AnnotateError(span, err)
return pubsub.ValidationIgnore, err
}
committee, err := helpers.BeaconCommitteeFromState(ctx, preState, att.GetData().Slot, committeeIndex)
if err != nil {
tracing.AnnotateError(span, err)
return pubsub.ValidationIgnore, err
}

var singleAtt *eth.SingleAttestation
if att.Version() >= version.Electra {
singleAtt, ok = att.(*eth.SingleAttestation)
if !ok {
return pubsub.ValidationIgnore, fmt.Errorf("attestation has wrong type (expected %T, got %T)", &eth.SingleAttestation{}, att)
}
att = singleAtt.ToAttestationElectra(committee)
}

Copy link
Member

Choose a reason for hiding this comment

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

Is there a DOS vector for calling AttestationTargetState and BeaconCommitteeFromState this early?

Comment on lines -309 to -318
name: "nil signature",
attestation: &ethpb.AttestationElectra{
AggregationBits: testhelpers.FillByteSlice(4, 74),
Data: &ethpb.AttestationData{
Source: &ethpb.Checkpoint{},
Target: &ethpb.Checkpoint{},
},
CommitteeBits: testhelpers.FillByteSlice(8, 83),
},
expectedErrorMessage: "attestation signature can't be nil",
Copy link
Member

Choose a reason for hiding this comment

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

Why nil signature test removed?

Copy link
Contributor Author

@rkapka rkapka Dec 18, 2024

Choose a reason for hiding this comment

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

I removed it because we had two functions in the codebase checking if an attestation is nil. One is helpers.ValidateNilAttestation and the other one is validateNilAttestation on the VC side. The only difference between them was signature validation which is missing in the helpers package. I decided it's better to just have a single implementation and I expect the one in helpers to be more accurate. I am not exactly sure what happens though when the BN receives an attestation without a signature if the function in helpers doesn't fail (most likely signature validation fails in some other place).

if !ok {
return pubsub.ValidationIgnore, fmt.Errorf("attestation has wrong type (expected %T, got %T)", &eth.SingleAttestation{}, att)
}
att = singleAtt.ToAttestationElectra(committee)
Copy link
Member

Choose a reason for hiding this comment

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

This is the biggest additional performance penalty as far as I can see. For a node that subscribe to several subnets. Is this a concern? We will call this 10-30k times per slot

@@ -71,6 +70,32 @@ func (s *Service) validateCommitteeIndexBeaconAttestation(ctx context.Context, p
if data.Slot == 0 {
return pubsub.ValidationIgnore, nil
}

preState, err := s.cfg.chain.AttestationTargetState(ctx, data.Target)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we calling this here ? There must be strong justification for

  • fetching a state
  • fetching the committee from it

As these are additional bottlenecks on our highest traffic gossip path

Copy link
Member

Choose a reason for hiding this comment

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

Also why is this being called before the current gossip checks

beacon-chain/sync/validate_aggregate_proof.go Show resolved Hide resolved
committee, result, err := s.validateBitLength(ctx, bs, a.GetData().Slot, committeeIndex, a.GetAggregationBits())
if result != pubsub.ValidationAccept {
return result, err
committee, err := helpers.BeaconCommitteeFromState(ctx, bs, a.GetData().Slot, committeeIndex)
Copy link
Member

Choose a reason for hiding this comment

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

Same here, why is validateBitLength removed

@@ -115,6 +104,48 @@ func (s *Service) validateCommitteeIndexBeaconAttestation(ctx context.Context, p
}
}

if !s.cfg.chain.InForkchoice(bytesutil.ToBytes32(data.BeaconBlockRoot)) {
Copy link
Member

Choose a reason for hiding this comment

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

Executing all these methods before we have validated data.BeaconBlockRoot is wrong. As we might have not processed the block yet (and it has to be inserted into our pending queue)

return pubsub.ValidationIgnore, blockchain.ErrNotDescendantOfFinalized
}

if err = s.cfg.chain.VerifyLmdFfgConsistency(ctx, att); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Same here and for the rest of the methods that follow it. If it is difficult enough, you should just stream the attestations after

terencechain
terencechain previously approved these changes Dec 22, 2024
Copy link
Contributor

@potuz potuz left a comment

Choose a reason for hiding this comment

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

First pass, will continue in 30 minutes

Comment on lines 35 to 38
if !attestation.IsSingle() && attestation.GetAggregationBits() == nil {
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

@@ -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

@@ -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

@@ -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
Member

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?

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 !att.IsAggregated() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This check is useless, the structure SingleAttestation always returns false here.

if err = s.Broadcaster.BroadcastAttestation(ctx, subnet, att); err != nil {
log.WithError(err).Errorf("could not broadcast attestation at index %d", i)
failedBroadcasts = append(failedBroadcasts, strconv.Itoa(i))
continue
}

if features.Get().EnableExperimentalAttestationPool {
if err = s.AttestationCache.Add(att); err != nil {
if err = s.AttestationCache.Add(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.

The conversion to ToAttestationElectra happens unconditionally above, I would keep this object and reuse it here.

} else {
if err = s.AttestationsPool.SaveUnaggregatedAttestation(att); err != nil {
log.WithError(err).Error("could not save unaggregated attestation")
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.

@@ -205,11 +205,7 @@ func matchingAtts(atts []ethpbalpha.Att, slot primitives.Slot, attDataRoot []byt
// compare the committee index separately.
if postElectra {
if att.Version() >= version.Electra {
Copy link
Contributor

Choose a reason for hiding this comment

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

The whole block reads better as follows

        root, err := att.GetData().HashTreeRoot()
		if err != nil {
			return nil, errors.Wrap(err, "could not get attestation data root")
		}
		if !bytes.Equal(root[:], attDataRoot) {
			continue
		}
        if att.Version() < version.Electra || (postElectra && att.GetCommitteeIndex()  == index) {
			result = append(result, att)
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but we need to have (!postElectra && att.Version() < version.Electra) || ...

Copy link
Contributor

Choose a reason for hiding this comment

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

that should be covered for att.Version() < version.Electra since we should clean the pool at the fork, but anyway you can change the first side of the || to include the check

        if (!postElectra && att.Version() < version.Electra) || (postElectra && att.GetCommitteeIndex()  == index) {

return nil, nil, errors.Wrap(err, "failed to retrieve attestation committee index")
}
subnet := corehelpers.ComputeSubnetFromCommitteeAndSlot(uint64(len(vals)), committeeIndex, att.Data.Slot)
subnet := corehelpers.ComputeSubnetFromCommitteeAndSlot(uint64(len(vals)), att.GetCommitteeIndex(), att.Data.Slot)
Copy link
Member

@terencechain terencechain Jan 7, 2025

Choose a reason for hiding this comment

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

Unrelated to this PR but calling s.HeadFetcher.HeadValidatorsIndices(ctx, wantedEpoch) in the event of a cache miss will allocate a new slice of active validator indices, it's expensive to just get the active validator count. ActiveValidatorCount is more suitable for this

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

if err != nil {
return nil, err
}

if features.Get().EnableExperimentalAttestationPool {
if err = vs.AttestationCache.Add(att); err != nil {
if err = vs.AttestationCache.Add(att.ToAttestationElectra(committee)); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

may be able to reuse att.ToAttestationElectra(committee) here

func (vs *Server) proposeAtt(
ctx context.Context,
att ethpb.Att,
committee []primitives.ValidatorIndex, // required post-Electra
Copy link
Member

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
Member

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
Member

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

Comment on lines +129 to +144
message SignedAggregateAttestationAndProofSingle {
// The aggregated attestation and selection proof itself.
AggregateAttestationAndProofSingle message = 1;

// 96 byte BLS aggregate signature signed by the aggregator over the message.
bytes signature = 2 [(ethereum.eth.ext.ssz_size) = "96"];
}

message AggregateAttestationAndProofSingle {
// The aggregator index that submitted this aggregated attestation and proof.
uint64 aggregator_index = 1 [(ethereum.eth.ext.cast_type) = "github.com/prysmaticlabs/prysm/v5/consensus-types/primitives.ValidatorIndex"];

// The aggregated attestation that was submitted.
SingleAttestation aggregate = 3;

// 96 byte selection proof signed by the aggregator, which is the signature of the slot to aggregate.
Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that this is required because of the savePendingAtt API. It seems annoying to define new proto types just to satisfy an API for the cache savePendingAtt. I wonder if there's something we can do to fix the cache for the API; probably a subsequent PR is okay

Copy link
Contributor Author

@rkapka rkapka Jan 8, 2025

Choose a reason for hiding this comment

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

The pending atts queue would have to be redesigned so that you don't have to save AggregateAttestationAndProof objects. We currently construct dummy objects just to satisfy the interface. I will add this to my todo list.

rkapka added 3 commits January 8, 2025 18:03
# Conflicts:
#	CHANGELOG.md
#	proto/prysm/v1alpha1/validator.pb.go
if att.Version() >= version.Electra {
var ok bool
singleAtt, ok = att.(*ethpb.SingleAttestation)
if !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a question on this path. According to the Electra spec AggregateAndProof here contains an Attestation object inside which is never a SingleAttestation according to this definition in the Electra spec

Copy link
Contributor

@potuz potuz left a comment

Choose a reason for hiding this comment

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

Have a concern in the pending attestation queue

Copy link
Contributor

@potuz potuz left a comment

Choose a reason for hiding this comment

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

Another quick pass, logic looks good to me except some nasty hacks to deal with the pending queue

}
s.savePendingAtt(&eth.SignedAggregateAttestationAndProofElectra{Message: &eth.AggregateAttestationAndProofElectra{Aggregate: a}})
s.savePendingAtt(&eth.SignedAggregateAttestationAndProofSingle{Message: &eth.AggregateAttestationAndProofSingle{Aggregate: a}})
Copy link
Contributor

Choose a reason for hiding this comment

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

Document this hack please

@@ -115,17 +99,19 @@ func (s *Service) validateCommitteeIndexBeaconAttestation(ctx context.Context, p
}
}

var validationRes pubsub.ValidationResult

// Verify the block being voted and the processed state is in beaconDB and the block has passed validation if it's in the beaconDB.
blockRoot := bytesutil.ToBytes32(data.BeaconBlockRoot)
if !s.hasBlockAndState(ctx, blockRoot) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this whole block can be extracted from this function

if !ok {
return pubsub.ValidationIgnore, fmt.Errorf("attestation has wrong type (expected %T, got %T)", &eth.SingleAttestation{}, a)
}
result, err := validateAttestingIndex(ctx, singleAtt.AttesterIndex, committee)
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
result, err := validateAttestingIndex(ctx, singleAtt.AttesterIndex, committee)
return validateAttestingIndex(ctx, singleAtt.AttesterIndex, committee)

And then remove the else branch

@rkapka rkapka added this pull request to the merge queue Jan 10, 2025
@rkapka rkapka removed this pull request from the merge queue due to a manual request Jan 10, 2025
@rkapka rkapka added this pull request to the merge queue Jan 13, 2025
Merged via the queue into develop with commit 153d187 Jan 13, 2025
15 checks passed
@rkapka rkapka deleted the single-att branch January 13, 2025 17:11
nalepae added a commit that referenced this pull request Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants