From 416533858d88aa986256d78ac59f474c55a2ef44 Mon Sep 17 00:00:00 2001 From: rauljordan Date: Fri, 22 May 2020 14:20:18 -0500 Subject: [PATCH 1/5] align to spec --- beacon-chain/sync/validate_aggregate_proof.go | 13 ++++++------ ...date_committee_index_beacon_attestation.go | 21 +++++++++++++++++-- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/beacon-chain/sync/validate_aggregate_proof.go b/beacon-chain/sync/validate_aggregate_proof.go index 2ed7b27c5572..bcf7ea4f580d 100644 --- a/beacon-chain/sync/validate_aggregate_proof.go +++ b/beacon-chain/sync/validate_aggregate_proof.go @@ -8,18 +8,18 @@ import ( pubsub "github.com/libp2p/go-libp2p-pubsub" "github.com/pkg/errors" ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" + "go.opencensus.io/trace" + "github.com/prysmaticlabs/prysm/beacon-chain/core/blocks" "github.com/prysmaticlabs/prysm/beacon-chain/core/helpers" "github.com/prysmaticlabs/prysm/beacon-chain/core/state" stateTrie "github.com/prysmaticlabs/prysm/beacon-chain/state" - "github.com/prysmaticlabs/prysm/shared/attestationutil" "github.com/prysmaticlabs/prysm/shared/bls" "github.com/prysmaticlabs/prysm/shared/bytesutil" "github.com/prysmaticlabs/prysm/shared/featureconfig" "github.com/prysmaticlabs/prysm/shared/params" "github.com/prysmaticlabs/prysm/shared/roughtime" "github.com/prysmaticlabs/prysm/shared/traceutil" - "go.opencensus.io/trace" ) // validateAggregateAndProof verifies the aggregated signature and the selection proof is valid before forwarding to the @@ -106,7 +106,7 @@ func (r *Service) validateAggregatedAtt(ctx context.Context, signed *ethpb.Signe } } - // Verify validator index is within the aggregate's committee. + // Verify validator index is within the beacon committee. if err := validateIndexInCommittee(ctx, s, signed.Message.Aggregate, signed.Message.AggregatorIndex); err != nil { traceutil.AnnotateError(span, errors.Wrapf(err, "Could not validate index in committee")) return false @@ -167,7 +167,7 @@ func (r *Service) setAggregatorIndexEpochSeen(epoch uint64, aggregatorIndex uint r.seenAttestationCache.Add(string(b), true) } -// This validates the aggregator's index in state is within the attesting indices of the attestation. +// This validates the aggregator's index in state is within the beacon committee. func validateIndexInCommittee(ctx context.Context, s *stateTrie.BeaconState, a *ethpb.Attestation, validatorIndex uint64) error { ctx, span := trace.StartSpan(ctx, "sync.validateIndexInCommittee") defer span.End() @@ -176,9 +176,8 @@ func validateIndexInCommittee(ctx context.Context, s *stateTrie.BeaconState, a * if err != nil { return err } - attestingIndices := attestationutil.AttestingIndices(a.AggregationBits, committee) var withinCommittee bool - for _, i := range attestingIndices { + for _, i := range committee { if validatorIndex == i { withinCommittee = true break @@ -186,7 +185,7 @@ func validateIndexInCommittee(ctx context.Context, s *stateTrie.BeaconState, a * } if !withinCommittee { return fmt.Errorf("validator index %d is not within the committee: %v", - validatorIndex, attestingIndices) + validatorIndex, committee) } return nil } diff --git a/beacon-chain/sync/validate_committee_index_beacon_attestation.go b/beacon-chain/sync/validate_committee_index_beacon_attestation.go index 11b839d64207..01af40838bdc 100644 --- a/beacon-chain/sync/validate_committee_index_beacon_attestation.go +++ b/beacon-chain/sync/validate_committee_index_beacon_attestation.go @@ -9,7 +9,9 @@ import ( "github.com/libp2p/go-libp2p-core/peer" pubsub "github.com/libp2p/go-libp2p-pubsub" eth "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" + "github.com/prysmaticlabs/prysm/beacon-chain/core/helpers" "github.com/prysmaticlabs/prysm/beacon-chain/p2p" + "github.com/prysmaticlabs/prysm/shared/attestationutil" "github.com/prysmaticlabs/prysm/shared/bytesutil" "github.com/prysmaticlabs/prysm/shared/featureconfig" "github.com/prysmaticlabs/prysm/shared/traceutil" @@ -18,7 +20,7 @@ import ( // Validation // - The attestation's committee index (attestation.data.index) is for the correct subnet. -// - The attestation is unaggregated -- that is, it has exactly one participating validator (len([bit for bit in attestation.aggregation_bits if bit == 0b1]) == 1). +// - The attestation is unaggregated -- that is, it has exactly one participating validator (len(get_attesting_indices(state, attestation.data, attestation.aggregation_bits)) == 1). // - The block being voted for (attestation.data.beacon_block_root) passes validation. // - attestation.data.slot is within the last ATTESTATION_PROPAGATION_SLOT_RANGE slots (attestation.data.slot + ATTESTATION_PROPAGATION_SLOT_RANGE >= current_slot >= attestation.data.slot). // - The signature of attestation is valid. @@ -56,6 +58,7 @@ func (s *Service) validateCommitteeIndexBeaconAttestation(ctx context.Context, p if att.Data == nil { return false } + // Verify this the first attestation received for the participating validator for the slot. if s.hasSeenCommitteeIndicesSlot(att.Data.Slot, att.Data.CommitteeIndex, att.AggregationBits) { return false @@ -72,8 +75,22 @@ func (s *Service) validateCommitteeIndexBeaconAttestation(ctx context.Context, p return false } + // Attestation aggregation bits must exist. + if att.AggregationBits == nil { + return false + } + st, err := s.chain.AttestationPreState(ctx, att) + if err != nil { + traceutil.AnnotateError(span, err) + return false + } + committee, err := helpers.BeaconCommitteeFromState(st, att.Data.Slot, att.Data.CommitteeIndex) + if err != nil { + return false + } + // Attestation must be unaggregated. - if att.AggregationBits == nil || att.AggregationBits.Count() != 1 { + if len(attestationutil.AttestingIndices(att.AggregationBits, committee)) != 1 { return false } From 52febbd73ec56f2f4aa9ffdc51d541aae95d1f91 Mon Sep 17 00:00:00 2001 From: Raul Jordan Date: Fri, 22 May 2020 14:30:47 -0500 Subject: [PATCH 2/5] Update beacon-chain/sync/validate_aggregate_proof.go --- beacon-chain/sync/validate_aggregate_proof.go | 1 - 1 file changed, 1 deletion(-) diff --git a/beacon-chain/sync/validate_aggregate_proof.go b/beacon-chain/sync/validate_aggregate_proof.go index bcf7ea4f580d..943565a62143 100644 --- a/beacon-chain/sync/validate_aggregate_proof.go +++ b/beacon-chain/sync/validate_aggregate_proof.go @@ -9,7 +9,6 @@ import ( "github.com/pkg/errors" ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" "go.opencensus.io/trace" - "github.com/prysmaticlabs/prysm/beacon-chain/core/blocks" "github.com/prysmaticlabs/prysm/beacon-chain/core/helpers" "github.com/prysmaticlabs/prysm/beacon-chain/core/state" From 313334178806a34050401dfd04bfaf71153c72f9 Mon Sep 17 00:00:00 2001 From: rauljordan Date: Fri, 22 May 2020 15:12:24 -0500 Subject: [PATCH 3/5] ensure sync tests pass --- .../sync/validate_aggregate_proof_test.go | 31 ++++++++++++++ ...date_committee_index_beacon_attestation.go | 1 + ...committee_index_beacon_attestation_test.go | 41 ++++++++++++------- 3 files changed, 58 insertions(+), 15 deletions(-) diff --git a/beacon-chain/sync/validate_aggregate_proof_test.go b/beacon-chain/sync/validate_aggregate_proof_test.go index a4331497d94f..8c4ba5f0d48f 100644 --- a/beacon-chain/sync/validate_aggregate_proof_test.go +++ b/beacon-chain/sync/validate_aggregate_proof_test.go @@ -65,6 +65,37 @@ func TestVerifyIndexInCommittee_CanVerify(t *testing.T) { } } +func TestVerifyIndexInCommittee_ExistsInBeaconCommittee(t *testing.T) { + ctx := context.Background() + params.UseMinimalConfig() + defer params.UseMainnetConfig() + + validators := uint64(64) + s, _ := testutil.DeterministicGenesisState(t, validators) + if err := s.SetSlot(params.BeaconConfig().SlotsPerEpoch); err != nil { + t.Fatal(err) + } + + bf := []byte{0xff} + att := ðpb.Attestation{Data: ðpb.AttestationData{ + Target: ðpb.Checkpoint{Epoch: 0}}, + AggregationBits: bf} + + committee, err := helpers.BeaconCommitteeFromState(s, att.Data.Slot, att.Data.CommitteeIndex) + if err != nil { + t.Error(err) + } + + if err := validateIndexInCommittee(ctx, s, att, committee[0]); err != nil { + t.Fatal(err) + } + + wanted := "validator index 1000 is not within the committee" + if err := validateIndexInCommittee(ctx, s, att, 1000); err == nil || !strings.Contains(err.Error(), wanted) { + t.Error("Did not receive wanted error") + } +} + func TestVerifySelection_NotAnAggregator(t *testing.T) { ctx := context.Background() params.UseMinimalConfig() diff --git a/beacon-chain/sync/validate_committee_index_beacon_attestation.go b/beacon-chain/sync/validate_committee_index_beacon_attestation.go index 01af40838bdc..7a14d9641f05 100644 --- a/beacon-chain/sync/validate_committee_index_beacon_attestation.go +++ b/beacon-chain/sync/validate_committee_index_beacon_attestation.go @@ -86,6 +86,7 @@ func (s *Service) validateCommitteeIndexBeaconAttestation(ctx context.Context, p } committee, err := helpers.BeaconCommitteeFromState(st, att.Data.Slot, att.Data.CommitteeIndex) if err != nil { + traceutil.AnnotateError(span, err) return false } diff --git a/beacon-chain/sync/validate_committee_index_beacon_attestation_test.go b/beacon-chain/sync/validate_committee_index_beacon_attestation_test.go index 725b54a14603..fd248894543a 100644 --- a/beacon-chain/sync/validate_committee_index_beacon_attestation_test.go +++ b/beacon-chain/sync/validate_committee_index_beacon_attestation_test.go @@ -12,6 +12,7 @@ import ( pubsubpb "github.com/libp2p/go-libp2p-pubsub/pb" ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" "github.com/prysmaticlabs/go-bitfield" + mockChain "github.com/prysmaticlabs/prysm/beacon-chain/blockchain/testing" "github.com/prysmaticlabs/prysm/beacon-chain/cache" dbtest "github.com/prysmaticlabs/prysm/beacon-chain/db/testing" @@ -28,7 +29,8 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { p := p2ptest.NewTestP2P(t) db := dbtest.SetupDB(t) chain := &mockChain.ChainService{ - Genesis: time.Now().Add(time.Duration(-64*int64(params.BeaconConfig().SecondsPerSlot)) * time.Second), // 64 slots ago + // 1 slot ago. + Genesis: time.Now().Add(time.Duration(-1*int64(params.BeaconConfig().SecondsPerSlot)) * time.Second), ValidatorsRoot: [32]byte{'A'}, ValidAttestation: true, } @@ -53,7 +55,7 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { blk := ðpb.SignedBeaconBlock{ Block: ðpb.BeaconBlock{ - Slot: 55, + Slot: 1, }, } if err := db.SaveBlock(ctx, blk); err != nil { @@ -65,10 +67,15 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { t.Fatal(err) } - savedState := testutil.NewBeaconState() + validators := uint64(64) + savedState, _ := testutil.DeterministicGenesisState(t, validators) + if err := savedState.SetSlot(1); err != nil { + t.Fatal(err) + } if err := db.SaveState(context.Background(), savedState, validBlockRoot); err != nil { t.Fatal(err) } + chain.State = savedState tests := []struct { name string @@ -78,30 +85,34 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { want bool }{ { - name: "validAttestationSignature", + name: "valid attestation signature", msg: ðpb.Attestation{ AggregationBits: bitfield.Bitlist{0b1010}, Data: ðpb.AttestationData{ BeaconBlockRoot: validBlockRoot[:], - CommitteeIndex: 1, - Slot: 63, + CommitteeIndex: 0, + Slot: 1, + Target: ðpb.Checkpoint{ + Epoch: 0, + Root: validBlockRoot[:], + }, }, }, - topic: fmt.Sprintf("/eth2/%x/committee_index1_beacon_attestation", digest), + topic: fmt.Sprintf("/eth2/%x/committee_index0_beacon_attestation", digest), validAttestationSignature: true, want: true, }, { - name: "alreadySeen", + name: "already seen", msg: ðpb.Attestation{ AggregationBits: bitfield.Bitlist{0b1010}, Data: ðpb.AttestationData{ BeaconBlockRoot: validBlockRoot[:], - CommitteeIndex: 1, - Slot: 63, + CommitteeIndex: 0, + Slot: 1, }, }, - topic: fmt.Sprintf("/eth2/%x/committee_index1_beacon_attestation", digest), + topic: fmt.Sprintf("/eth2/%x/committee_index0_beacon_attestation", digest), validAttestationSignature: true, want: false, }, @@ -112,7 +123,7 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { Data: ðpb.AttestationData{ BeaconBlockRoot: validBlockRoot[:], CommitteeIndex: 2, - Slot: 63, + Slot: 1, }, }, topic: fmt.Sprintf("/eth2/%x/committee_index3_beacon_attestation", digest), @@ -126,7 +137,7 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { Data: ðpb.AttestationData{ BeaconBlockRoot: validBlockRoot[:], CommitteeIndex: 1, - Slot: 63, + Slot: 1, }, }, topic: fmt.Sprintf("/eth2/%x/committee_index1_beacon_attestation", digest), @@ -140,7 +151,7 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { Data: ðpb.AttestationData{ BeaconBlockRoot: bytesutil.PadTo([]byte("missing"), 32), CommitteeIndex: 1, - Slot: 63, + Slot: 1, }, }, topic: fmt.Sprintf("/eth2/%x/committee_index1_beacon_attestation", digest), @@ -154,7 +165,7 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { Data: ðpb.AttestationData{ BeaconBlockRoot: validBlockRoot[:], CommitteeIndex: 1, - Slot: 63, + Slot: 1, }, }, topic: fmt.Sprintf("/eth2/%x/committee_index1_beacon_attestation", digest), From d42a1dec791f0e09097123e178c843821195c9c7 Mon Sep 17 00:00:00 2001 From: Raul Jordan Date: Fri, 22 May 2020 15:13:06 -0500 Subject: [PATCH 4/5] Update beacon-chain/sync/validate_committee_index_beacon_attestation_test.go --- .../sync/validate_committee_index_beacon_attestation_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/beacon-chain/sync/validate_committee_index_beacon_attestation_test.go b/beacon-chain/sync/validate_committee_index_beacon_attestation_test.go index fd248894543a..378093ff4ea5 100644 --- a/beacon-chain/sync/validate_committee_index_beacon_attestation_test.go +++ b/beacon-chain/sync/validate_committee_index_beacon_attestation_test.go @@ -12,7 +12,6 @@ import ( pubsubpb "github.com/libp2p/go-libp2p-pubsub/pb" ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" "github.com/prysmaticlabs/go-bitfield" - mockChain "github.com/prysmaticlabs/prysm/beacon-chain/blockchain/testing" "github.com/prysmaticlabs/prysm/beacon-chain/cache" dbtest "github.com/prysmaticlabs/prysm/beacon-chain/db/testing" From 1395c3cff02613c75b5c5753af2813f148e893b9 Mon Sep 17 00:00:00 2001 From: rauljordan Date: Tue, 26 May 2020 20:28:19 -0500 Subject: [PATCH 5/5] pass test --- beacon-chain/sync/validate_beacon_attestation_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon-chain/sync/validate_beacon_attestation_test.go b/beacon-chain/sync/validate_beacon_attestation_test.go index fb7cef2b1c91..ba7d599286b4 100644 --- a/beacon-chain/sync/validate_beacon_attestation_test.go +++ b/beacon-chain/sync/validate_beacon_attestation_test.go @@ -97,7 +97,7 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { }, }, }, - topic: fmt.Sprintf("/eth2/%x/beacon_attestation_1", digest), + topic: fmt.Sprintf("/eth2/%x/beacon_attestation_0", digest), validAttestationSignature: true, want: true, },