From bc2c20683215c9e879bfc772042f557d32b296e8 Mon Sep 17 00:00:00 2001 From: terence tsao Date: Tue, 19 Jan 2021 13:41:44 -0800 Subject: [PATCH] Add verify slot target epoch function and apply all (#8273) * Add verify slot target epoch function and apply all * Fix TestProcessAttestationsNoVerify_IncorrectSlotTargetEpoch Co-authored-by: Raul Jordan --- .../blockchain/process_attestation.go | 17 ++------ .../blockchain/process_attestation_test.go | 18 ++++---- beacon-chain/core/blocks/attestation.go | 4 +- beacon-chain/core/blocks/attestation_test.go | 2 +- beacon-chain/core/helpers/attestation.go | 9 ++++ beacon-chain/core/helpers/attestation_test.go | 41 +++++++++++++++++++ beacon-chain/sync/validate_aggregate_proof.go | 2 +- .../sync/validate_beacon_attestation.go | 2 +- 8 files changed, 68 insertions(+), 27 deletions(-) diff --git a/beacon-chain/blockchain/process_attestation.go b/beacon-chain/blockchain/process_attestation.go index 08dbbc769..86f4a8d0c 100644 --- a/beacon-chain/blockchain/process_attestation.go +++ b/beacon-chain/blockchain/process_attestation.go @@ -2,7 +2,6 @@ package blockchain import ( "context" - "fmt" "github.com/pkg/errors" ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" @@ -45,22 +44,14 @@ func (s *Service) onAttestation(ctx context.Context, a *ethpb.Attestation) ([]ui ctx, span := trace.StartSpan(ctx, "blockChain.onAttestation") defer span.End() - if a == nil { - return nil, errors.New("nil attestation") + if err := helpers.ValidateNilAttestation(a); err != nil { + return nil, err } - if a.Data == nil { - return nil, errors.New("nil attestation.Data field") + if err := helpers.ValidateSlotTargetEpoch(a.Data); err != nil { + return nil, err } - if a.Data.Target == nil { - return nil, errors.New("nil attestation.Data.Target field") - } - tgt := stateTrie.CopyCheckpoint(a.Data.Target) - if helpers.SlotToEpoch(a.Data.Slot) != a.Data.Target.Epoch { - return nil, fmt.Errorf("data slot is not in the same epoch as target %d != %d", helpers.SlotToEpoch(a.Data.Slot), a.Data.Target.Epoch) - } - // Verify beacon node has seen the target block before. if !s.hasBlock(ctx, bytesutil.ToBytes32(tgt.Root)) { return nil, ErrTargetRootNotInDB diff --git a/beacon-chain/blockchain/process_attestation_test.go b/beacon-chain/blockchain/process_attestation_test.go index ff123e071..26dc9fd2c 100644 --- a/beacon-chain/blockchain/process_attestation_test.go +++ b/beacon-chain/blockchain/process_attestation_test.go @@ -72,34 +72,34 @@ func TestStore_OnAttestation(t *testing.T) { }{ { name: "attestation's data slot not aligned with target vote", - a: ðpb.Attestation{Data: ðpb.AttestationData{Slot: params.BeaconConfig().SlotsPerEpoch, Target: ðpb.Checkpoint{Root: make([]byte, 32)}}}, - wantedErr: "data slot is not in the same epoch as target 1 != 0", + a: testutil.HydrateAttestation(ðpb.Attestation{Data: ðpb.AttestationData{Slot: params.BeaconConfig().SlotsPerEpoch, Target: ðpb.Checkpoint{Root: make([]byte, 32)}}}), + wantedErr: "slot 32 does not match target epoch 0", }, { name: "attestation's target root not in db", - a: ðpb.Attestation{Data: ðpb.AttestationData{Target: ðpb.Checkpoint{Root: bytesutil.PadTo([]byte{'A'}, 32)}}}, + a: testutil.HydrateAttestation(ðpb.Attestation{Data: ðpb.AttestationData{Target: ðpb.Checkpoint{Root: bytesutil.PadTo([]byte{'A'}, 32)}}}), wantedErr: "target root does not exist in db", }, { name: "no pre state for attestations's target block", - a: ðpb.Attestation{Data: ðpb.AttestationData{Target: ðpb.Checkpoint{Root: BlkWithOutStateRoot[:]}}}, + a: testutil.HydrateAttestation(ðpb.Attestation{Data: ðpb.AttestationData{Target: ðpb.Checkpoint{Root: BlkWithOutStateRoot[:]}}}), wantedErr: "could not get pre state for epoch 0", }, { name: "process attestation doesn't match current epoch", - a: ðpb.Attestation{Data: ðpb.AttestationData{Slot: 100 * params.BeaconConfig().SlotsPerEpoch, Target: ðpb.Checkpoint{Epoch: 100, - Root: BlkWithStateBadAttRoot[:]}}}, + a: testutil.HydrateAttestation(ðpb.Attestation{Data: ðpb.AttestationData{Slot: 100 * params.BeaconConfig().SlotsPerEpoch, Target: ðpb.Checkpoint{Epoch: 100, + Root: BlkWithStateBadAttRoot[:]}}}), wantedErr: "target epoch 100 does not match current epoch", }, { name: "process nil attestation", a: nil, - wantedErr: "nil attestation", + wantedErr: "attestation can't be nil", }, { name: "process nil field (a.Data) in attestation", a: ðpb.Attestation{}, - wantedErr: "nil attestation.Data field", + wantedErr: "attestation's data can't be nil", }, { name: "process nil field (a.Target) in attestation", @@ -112,7 +112,7 @@ func TestStore_OnAttestation(t *testing.T) { AggregationBits: make([]byte, 1), Signature: make([]byte, 96), }, - wantedErr: "nil attestation.Data.Target field", + wantedErr: "attestation's target can't be nil", }, } diff --git a/beacon-chain/core/blocks/attestation.go b/beacon-chain/core/blocks/attestation.go index 969011846..272dd124f 100644 --- a/beacon-chain/core/blocks/attestation.go +++ b/beacon-chain/core/blocks/attestation.go @@ -128,8 +128,8 @@ func ProcessAttestationNoVerifySignature( currEpoch, ) } - if helpers.SlotToEpoch(data.Slot) != data.Target.Epoch { - return nil, fmt.Errorf("data slot is not in the same epoch as target %d != %d", helpers.SlotToEpoch(data.Slot), data.Target.Epoch) + if err := helpers.ValidateSlotTargetEpoch(att.Data); err != nil { + return nil, err } s := att.Data.Slot diff --git a/beacon-chain/core/blocks/attestation_test.go b/beacon-chain/core/blocks/attestation_test.go index d149bcfa3..6d5273bcd 100644 --- a/beacon-chain/core/blocks/attestation_test.go +++ b/beacon-chain/core/blocks/attestation_test.go @@ -389,7 +389,7 @@ func TestProcessAttestationsNoVerify_IncorrectSlotTargetEpoch(t *testing.T) { Target: ðpb.Checkpoint{Root: make([]byte, 32)}, }, }) - wanted := fmt.Sprintf("data slot is not in the same epoch as target %d != %d", helpers.SlotToEpoch(att.Data.Slot), att.Data.Target.Epoch) + wanted := "slot 32 does not match target epoch 0" _, err := blocks.ProcessAttestationNoVerifySignature(context.TODO(), beaconState, att) assert.ErrorContains(t, wanted, err) } diff --git a/beacon-chain/core/helpers/attestation.go b/beacon-chain/core/helpers/attestation.go index ee3400f3d..7823a36be 100644 --- a/beacon-chain/core/helpers/attestation.go +++ b/beacon-chain/core/helpers/attestation.go @@ -35,6 +35,15 @@ func ValidateNilAttestation(attestation *ethpb.Attestation) error { return nil } +// ValidateSlotTargetEpoch checks if attestation data's epoch matches target checkpoint's epoch. +// It is recommended to run `ValidateNilAttestation` first to ensure `data.Target` can't be nil. +func ValidateSlotTargetEpoch(data *ethpb.AttestationData) error { + if SlotToEpoch(data.Slot) != data.Target.Epoch { + return fmt.Errorf("slot %d does not match target epoch %d", data.Slot, data.Target.Epoch) + } + return nil +} + // IsAggregator returns true if the signature is from the input validator. The committee // count is provided as an argument rather than imported implementation from spec. Having // committee count as an argument allows cheaper computation at run time. diff --git a/beacon-chain/core/helpers/attestation_test.go b/beacon-chain/core/helpers/attestation_test.go index 3eb9d50a6..ae385bed6 100644 --- a/beacon-chain/core/helpers/attestation_test.go +++ b/beacon-chain/core/helpers/attestation_test.go @@ -294,3 +294,44 @@ func TestValidateNilAttestation(t *testing.T) { }) } } + +func TestValidateSlotTargetEpoch(t *testing.T) { + tests := []struct { + name string + attestation *ethpb.Attestation + errString string + }{ + { + name: "incorrect slot", + attestation: ðpb.Attestation{ + Data: ðpb.AttestationData{ + Target: ðpb.Checkpoint{Epoch: 1}, + Source: ðpb.Checkpoint{}, + }, + AggregationBits: []byte{}, + }, + errString: "slot 0 does not match target epoch 1", + }, + { + name: "good attestation", + attestation: ðpb.Attestation{ + Data: ðpb.AttestationData{ + Slot: 2 * params.BeaconConfig().SlotsPerEpoch, + Target: ðpb.Checkpoint{Epoch: 2}, + Source: ðpb.Checkpoint{}, + }, + AggregationBits: []byte{}, + }, + errString: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.errString != "" { + require.ErrorContains(t, tt.errString, helpers.ValidateSlotTargetEpoch(tt.attestation.Data)) + } else { + require.NoError(t, helpers.ValidateSlotTargetEpoch(tt.attestation.Data)) + } + }) + } +} diff --git a/beacon-chain/sync/validate_aggregate_proof.go b/beacon-chain/sync/validate_aggregate_proof.go index 88da76d03..2c10be61f 100644 --- a/beacon-chain/sync/validate_aggregate_proof.go +++ b/beacon-chain/sync/validate_aggregate_proof.go @@ -52,7 +52,7 @@ func (s *Service) validateAggregateAndProof(ctx context.Context, pid peer.ID, ms return pubsub.ValidationReject } - if helpers.SlotToEpoch(m.Message.Aggregate.Data.Slot) != m.Message.Aggregate.Data.Target.Epoch { + if err := helpers.ValidateSlotTargetEpoch(m.Message.Aggregate.Data); err != nil { return pubsub.ValidationReject } if err := helpers.ValidateAttestationTime(m.Message.Aggregate.Data.Slot, s.chain.GenesisTime()); err != nil { diff --git a/beacon-chain/sync/validate_beacon_attestation.go b/beacon-chain/sync/validate_beacon_attestation.go index fa69905b3..5e8f57dbc 100644 --- a/beacon-chain/sync/validate_beacon_attestation.go +++ b/beacon-chain/sync/validate_beacon_attestation.go @@ -68,7 +68,7 @@ func (s *Service) validateCommitteeIndexBeaconAttestation(ctx context.Context, p traceutil.AnnotateError(span, err) return pubsub.ValidationIgnore } - if helpers.SlotToEpoch(att.Data.Slot) != att.Data.Target.Epoch { + if err := helpers.ValidateSlotTargetEpoch(att.Data); err != nil { return pubsub.ValidationReject }