Refactor attestationutil.AttestingIndices (#5159)

* Refactor AttestingIndices to not return any error. Add tests. Add shortcut for fully attested attestation
* attestationutil.ConvertToIndexed never returned error either
* fix test
* remove shortcut
* revert test...

beacon-chain/blockchain/process_attestation_helpers.go

@@ -121,11 +121,7 @@ func (s *Service) verifyAttestation(ctx context.Context, baseState *stateTrie.Be
 	if err != nil {
 		return nil, err
 	}
-	indexedAtt, err := attestationutil.ConvertToIndexed(ctx, a, committee)
-	if err != nil {
-		return nil, errors.Wrap(err, "could not convert attestation to indexed attestation")
-	}
-
+	indexedAtt := attestationutil.ConvertToIndexed(ctx, a, committee)
 	if err := blocks.VerifyIndexedAttestation(ctx, baseState, indexedAtt); err != nil {
 		if err == blocks.ErrSigFailedToVerify {
 			// When sig fails to verify, check if there's a differences in committees due to

beacon-chain/blockchain/process_block.go

@@ -349,10 +349,7 @@ func (s *Service) insertBlockToForkChoiceStore(ctx context.Context, blk *ethpb.B
 		if err != nil {
 			return err
 		}
-		indices, err := attestationutil.AttestingIndices(a.AggregationBits, committee)
-		if err != nil {
-			return err
-		}
+		indices := attestationutil.AttestingIndices(a.AggregationBits, committee)
 		s.forkChoiceStore.ProcessAttestation(ctx, indices, bytesutil.ToBytes32(a.Data.BeaconBlockRoot), a.Data.Target.Epoch)
 	}
 

beacon-chain/core/blocks/block_operations.go

@@ -856,10 +856,7 @@ func VerifyAttestation(ctx context.Context, beaconState *stateTrie.BeaconState,
 	if err != nil {
 		return err
 	}
-	indexedAtt, err := attestationutil.ConvertToIndexed(ctx, att, committee)
-	if err != nil {
-		return errors.Wrap(err, "could not convert to indexed attestation")
-	}
+	indexedAtt := attestationutil.ConvertToIndexed(ctx, att, committee)
 	return VerifyIndexedAttestation(ctx, beaconState, indexedAtt)
 }
 

beacon-chain/core/blocks/block_operations_test.go

@@ -943,7 +943,7 @@ func TestProcessAttestations_OK(t *testing.T) {
 	if err != nil {
 		t.Error(err)
 	}
-	attestingIndices, err := attestationutil.AttestingIndices(att.AggregationBits, committee)
+	attestingIndices := attestationutil.AttestingIndices(att.AggregationBits, committee)
 	if err != nil {
 		t.Error(err)
 	}
@@ -1004,7 +1004,7 @@ func TestProcessAggregatedAttestation_OverlappingBits(t *testing.T) {
 	if err != nil {
 		t.Error(err)
 	}
-	attestingIndices1, err := attestationutil.AttestingIndices(att1.AggregationBits, committee)
+	attestingIndices1 := attestationutil.AttestingIndices(att1.AggregationBits, committee)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1032,7 +1032,7 @@ func TestProcessAggregatedAttestation_OverlappingBits(t *testing.T) {
 	if err != nil {
 		t.Error(err)
 	}
-	attestingIndices2, err := attestationutil.AttestingIndices(att2.AggregationBits, committee)
+	attestingIndices2 := attestationutil.AttestingIndices(att2.AggregationBits, committee)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1082,7 +1082,7 @@ func TestProcessAggregatedAttestation_NoOverlappingBits(t *testing.T) {
 	if err != nil {
 		t.Error(err)
 	}
-	attestingIndices1, err := attestationutil.AttestingIndices(att1.AggregationBits, committee)
+	attestingIndices1 := attestationutil.AttestingIndices(att1.AggregationBits, committee)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1109,7 +1109,7 @@ func TestProcessAggregatedAttestation_NoOverlappingBits(t *testing.T) {
 	if err != nil {
 		t.Error(err)
 	}
-	attestingIndices2, err := attestationutil.AttestingIndices(att2.AggregationBits, committee)
+	attestingIndices2 := attestationutil.AttestingIndices(att2.AggregationBits, committee)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1240,10 +1240,7 @@ func TestConvertToIndexed_OK(t *testing.T) {
 		if err != nil {
 			t.Error(err)
 		}
-		ia, err := attestationutil.ConvertToIndexed(context.Background(), attestation, committee)
-		if err != nil {
-			t.Errorf("failed to convert attestation to indexed attestation: %v", err)
-		}
+		ia := attestationutil.ConvertToIndexed(context.Background(), attestation, committee)
 		if !reflect.DeepEqual(wanted, ia) {
 			diff, _ := messagediff.PrettyDiff(ia, wanted)
 			t.Log(diff)

beacon-chain/core/epoch/epoch_processing.go

@@ -336,10 +336,7 @@ func unslashedAttestingIndices(state *stateTrie.BeaconState, atts []*pb.PendingA
 		if err != nil {
 			return nil, err
 		}
-		attestingIndices, err := attestationutil.AttestingIndices(att.AggregationBits, committee)
-		if err != nil {
-			return nil, errors.Wrap(err, "could not get attester indices")
-		}
+		attestingIndices := attestationutil.AttestingIndices(att.AggregationBits, committee)
 		// Create a set for attesting indices
 		set := make([]uint64, 0, len(attestingIndices))
 		for _, index := range attestingIndices {

beacon-chain/core/epoch/precompute/attestation.go

@@ -47,10 +47,7 @@ func ProcessAttestations(
 		if err != nil {
 			return nil, nil, err
 		}
-		indices, err := attestationutil.AttestingIndices(a.AggregationBits, committee)
-		if err != nil {
-			return nil, nil, err
-		}
+		indices := attestationutil.AttestingIndices(a.AggregationBits, committee)
 		vp = UpdateValidator(vp, v, indices, a, a.Data.Slot)
 	}
 

beacon-chain/core/epoch/precompute/attestation_test.go

@@ -219,7 +219,7 @@ func TestProcessAttestations(t *testing.T) {
 	if err != nil {
 		t.Error(err)
 	}
-	indices, _ := attestationutil.AttestingIndices(att1.AggregationBits, committee)
+	indices := attestationutil.AttestingIndices(att1.AggregationBits, committee)
 	for _, i := range indices {
 		if !vp[i].IsPrevEpochAttester {
 			t.Error("Not a prev epoch attester")
@@ -229,7 +229,7 @@ func TestProcessAttestations(t *testing.T) {
 	if err != nil {
 		t.Error(err)
 	}
-	indices, _ = attestationutil.AttestingIndices(att2.AggregationBits, committee)
+	indices = attestationutil.AttestingIndices(att2.AggregationBits, committee)
 	for _, i := range indices {
 		if !vp[i].IsPrevEpochAttester {
 			t.Error("Not a prev epoch attester")

beacon-chain/core/helpers/committee_test.go

@@ -134,7 +134,7 @@ func TestAttestationParticipants_NoCommitteeCache(t *testing.T) {
 		if err != nil {
 			t.Error(err)
 		}
-		result, err := attestationutil.AttestingIndices(tt.bitfield, committee)
+		result := attestationutil.AttestingIndices(tt.bitfield, committee)
 		if err != nil {
 			t.Errorf("Failed to get attestation participants: %v", err)
 		}
@@ -167,7 +167,7 @@ func TestAttestationParticipants_EmptyBitfield(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	indices, err := attestationutil.AttestingIndices(bitfield.NewBitlist(128), committee)
+	indices := attestationutil.AttestingIndices(bitfield.NewBitlist(128), committee)
 	if err != nil {
 		t.Fatalf("attesting indices failed: %v", err)
 	}

beacon-chain/core/state/transition_test.go

@@ -428,7 +428,7 @@ func TestProcessBlock_PassesProcessingConditions(t *testing.T) {
 	if err != nil {
 		t.Error(err)
 	}
-	attestingIndices, err := attestationutil.AttestingIndices(blockAtt.AggregationBits, committee)
+	attestingIndices := attestationutil.AttestingIndices(blockAtt.AggregationBits, committee)
 	if err != nil {
 		t.Error(err)
 	}
@@ -743,7 +743,7 @@ func TestProcessBlk_AttsBasedOnValidatorCount(t *testing.T) {
 		if err != nil {
 			t.Error(err)
 		}
-		attestingIndices, err := attestationutil.AttestingIndices(att.AggregationBits, committee)
+		attestingIndices := attestationutil.AttestingIndices(att.AggregationBits, committee)
 		if err != nil {
 			t.Error(err)
 		}

beacon-chain/rpc/beacon/attestations.go

@@ -185,15 +185,7 @@ func (bs *Server) ListIndexedAttestations(
 			continue
 		}
 		committee := committeesBySlot[att.Data.Slot].Committees[att.Data.CommitteeIndex]
-		idxAtt, err := attestationutil.ConvertToIndexed(ctx, atts[i], committee.ValidatorIndices)
-		if err != nil {
-			return nil, status.Errorf(
-				codes.Internal,
-				"Could not convert attestation with slot %d to indexed form: %v",
-				att.Data.Slot,
-				err,
-			)
-		}
+		idxAtt := attestationutil.ConvertToIndexed(ctx, atts[i], committee.ValidatorIndices)
 		indexedAtts[i] = idxAtt
 	}
 
@@ -311,15 +303,7 @@ func (bs *Server) StreamIndexedAttestations(
 					continue
 				}
 				committee := committeesForSlot.Committees[att.Data.CommitteeIndex]
-				idxAtt, err := attestationutil.ConvertToIndexed(stream.Context(), att, committee.ValidatorIndices)
-				if err != nil {
-					return status.Errorf(
-						codes.Internal,
-						"Could not convert attestation with slot %d to indexed form: %v",
-						att.Data.Slot,
-						err,
-					)
-				}
+				idxAtt := attestationutil.ConvertToIndexed(stream.Context(), att, committee.ValidatorIndices)
 				if err := stream.Send(idxAtt); err != nil {
 					return status.Errorf(codes.Unavailable, "Could not send over stream: %v", err)
 				}

beacon-chain/rpc/beacon/attestations_test.go

@@ -600,7 +600,7 @@ func TestServer_ListIndexedAttestations_GenesisEpoch(t *testing.T) {
 	for i := 0; i < len(indexedAtts); i++ {
 		att := atts[i]
 		committee := committees[att.Data.Slot].Committees[att.Data.CommitteeIndex]
-		idxAtt, err := attestationutil.ConvertToIndexed(ctx, atts[i], committee.ValidatorIndices)
+		idxAtt := attestationutil.ConvertToIndexed(ctx, atts[i], committee.ValidatorIndices)
 		if err != nil {
 			t.Fatalf("Could not convert attestation to indexed: %v", err)
 		}
@@ -697,7 +697,7 @@ func TestServer_ListIndexedAttestations_ArchivedEpoch(t *testing.T) {
 	for i := 0; i < len(indexedAtts); i++ {
 		att := atts[i]
 		committee := committees[att.Data.Slot].Committees[att.Data.CommitteeIndex]
-		idxAtt, err := attestationutil.ConvertToIndexed(ctx, atts[i], committee.ValidatorIndices)
+		idxAtt := attestationutil.ConvertToIndexed(ctx, atts[i], committee.ValidatorIndices)
 		if err != nil {
 			t.Fatalf("Could not convert attestation to indexed: %v", err)
 		}
@@ -989,7 +989,7 @@ func TestServer_StreamIndexedAttestations_OK(t *testing.T) {
 	for i := 0; i < len(indexedAtts); i++ {
 		att := aggAtts[i]
 		committee := committees[att.Data.Slot].Committees[att.Data.CommitteeIndex]
-		idxAtt, err := attestationutil.ConvertToIndexed(ctx, att, committee.ValidatorIndices)
+		idxAtt := attestationutil.ConvertToIndexed(ctx, att, committee.ValidatorIndices)
 		if err != nil {
 			t.Fatalf("Could not convert attestation to indexed: %v", err)
 		}

beacon-chain/rpc/validator/aggregator_test.go

@@ -221,7 +221,7 @@ func generateAtt(state *beaconstate.BeaconState, index uint64, privKeys []*bls.S
 		AggregationBits: aggBits,
 	}
 	committee, _ := helpers.BeaconCommitteeFromState(state, att.Data.Slot, att.Data.CommitteeIndex)
-	attestingIndices, _ := attestationutil.AttestingIndices(att.AggregationBits, committee)
+	attestingIndices := attestationutil.AttestingIndices(att.AggregationBits, committee)
 	domain, err := helpers.Domain(state.Fork(), 0, params.BeaconConfig().DomainBeaconAttester)
 	if err != nil {
 		return nil, err

beacon-chain/rpc/validator/proposer_test.go

@@ -1307,7 +1307,7 @@ func TestFilterAttestation_OK(t *testing.T) {
 		if err != nil {
 			t.Error(err)
 		}
-		attestingIndices, err := attestationutil.AttestingIndices(atts[i].AggregationBits, committee)
+		attestingIndices := attestationutil.AttestingIndices(atts[i].AggregationBits, committee)
 		if err != nil {
 			t.Error(err)
 		}

beacon-chain/sync/pending_attestations_queue_test.go

@@ -128,7 +128,7 @@ func TestProcessPendingAtts_HasBlockSaveAggregatedAtt(t *testing.T) {
 	if err != nil {
 		t.Error(err)
 	}
-	attestingIndices, err := attestationutil.AttestingIndices(att.AggregationBits, committee)
+	attestingIndices := attestationutil.AttestingIndices(att.AggregationBits, committee)
 	if err != nil {
 		t.Error(err)
 	}

beacon-chain/sync/validate_aggregate_proof.go

@@ -148,10 +148,7 @@ func validateIndexInCommittee(ctx context.Context, s *stateTrie.BeaconState, a *
 	if err != nil {
 		return err
 	}
-	attestingIndices, err := attestationutil.AttestingIndices(a.AggregationBits, committee)
-	if err != nil {
-		return err
-	}
+	attestingIndices := attestationutil.AttestingIndices(a.AggregationBits, committee)
 	var withinCommittee bool
 	for _, i := range attestingIndices {
 		if validatorIndex == i {

beacon-chain/sync/validate_aggregate_proof_test.go

@@ -48,7 +48,7 @@ func TestVerifyIndexInCommittee_CanVerify(t *testing.T) {
 	if err != nil {
 		t.Error(err)
 	}
-	indices, err := attestationutil.AttestingIndices(att.AggregationBits, committee)
+	indices := attestationutil.AttestingIndices(att.AggregationBits, committee)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -330,7 +330,7 @@ func TestValidateAggregateAndProof_CanValidate(t *testing.T) {
 	if err != nil {
 		t.Error(err)
 	}
-	attestingIndices, err := attestationutil.AttestingIndices(att.AggregationBits, committee)
+	attestingIndices := attestationutil.AttestingIndices(att.AggregationBits, committee)
 	if err != nil {
 		t.Error(err)
 	}

shared/attestationutil/BUILD.bazel

@@ -1,4 +1,4 @@
-load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "go_default_library",
@@ -6,9 +6,15 @@ go_library(
     importpath = "github.com/prysmaticlabs/prysm/shared/attestationutil",
     visibility = ["//visibility:public"],
     deps = [
-        "@com_github_pkg_errors//:go_default_library",
         "@com_github_prysmaticlabs_ethereumapis//eth/v1alpha1:go_default_library",
         "@com_github_prysmaticlabs_go_bitfield//:go_default_library",
         "@io_opencensus_go//trace:go_default_library",
     ],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["attestation_utils_test.go"],
+    embed = [":go_default_library"],
+    deps = ["@com_github_prysmaticlabs_go_bitfield//:go_default_library"],
+)

shared/attestationutil/attestation_utils.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"sort"
 
-	"github.com/pkg/errors"
 	ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
 	"github.com/prysmaticlabs/go-bitfield"
 	"go.opencensus.io/trace"
@@ -28,14 +27,11 @@ import (
 //        data=attestation.data,
 //        signature=attestation.signature,
 //    )
-func ConvertToIndexed(ctx context.Context, attestation *ethpb.Attestation, committee []uint64) (*ethpb.IndexedAttestation, error) {
+func ConvertToIndexed(ctx context.Context, attestation *ethpb.Attestation, committee []uint64) *ethpb.IndexedAttestation {
 	ctx, span := trace.StartSpan(ctx, "core.ConvertToIndexed")
 	defer span.End()
 
-	attIndices, err := AttestingIndices(attestation.AggregationBits, committee)
-	if err != nil {
-		return nil, errors.Wrap(err, "could not get attesting indices")
-	}
+	attIndices := AttestingIndices(attestation.AggregationBits, committee)
 
 	sort.Slice(attIndices, func(i, j int) bool {
 		return attIndices[i] < attIndices[j]
@@ -45,7 +41,7 @@ func ConvertToIndexed(ctx context.Context, attestation *ethpb.Attestation, commi
 		Signature:        attestation.Signature,
 		AttestingIndices: attIndices,
 	}
-	return inAtt, nil
+	return inAtt
 }
 
 // AttestingIndices returns the attesting participants indices from the attestation data. The
@@ -61,7 +57,7 @@ func ConvertToIndexed(ctx context.Context, attestation *ethpb.Attestation, commi
 //    """
 //    committee = get_beacon_committee(state, data.slot, data.index)
 //    return set(index for i, index in enumerate(committee) if bits[i])
-func AttestingIndices(bf bitfield.Bitfield, committee []uint64) ([]uint64, error) {
+func AttestingIndices(bf bitfield.Bitfield, committee []uint64) []uint64 {
 	indices := make([]uint64, 0, len(committee))
 	indicesSet := make(map[uint64]bool, len(committee))
 	for i, idx := range committee {
@@ -72,5 +68,5 @@ func AttestingIndices(bf bitfield.Bitfield, committee []uint64) ([]uint64, error
 		}
 		indicesSet[idx] = true
 	}
-	return indices, nil
+	return indices
 }

shared/attestationutil/attestation_utils_test.go

@@ -0,0 +1,46 @@
+package attestationutil_test
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/prysmaticlabs/go-bitfield"
+	"github.com/prysmaticlabs/prysm/shared/attestationutil"
+)
+
+func TestAttestingIndices(t *testing.T) {
+	type args struct {
+		bf        bitfield.Bitfield
+		committee []uint64
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    []uint64
+	}{
+		{
+			name: "Full committee attested",
+			args: args{
+				bf: bitfield.Bitlist{0b1111},
+				committee: []uint64{0, 1, 2},
+			},
+			want: []uint64{0, 1, 2},
+		},
+		{
+			name: "Partial committee attested",
+			args: args{
+				bf: bitfield.Bitlist{0b1101},
+				committee: []uint64{0, 1, 2},
+			},
+			want: []uint64{0, 2},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := attestationutil.AttestingIndices(tt.args.bf, tt.args.committee)
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("AttestingIndices() got = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

tools/blocktree/main.go

@@ -100,10 +100,7 @@ func main() {
 			if err != nil {
 				panic(err)
 			}
-			indices, err := attestationutil.AttestingIndices(att.AggregationBits, committee)
-			if err != nil {
-				panic(err)
-			}
+			indices := attestationutil.AttestingIndices(att.AggregationBits, committee)
 			for _, i := range indices {
 				m[r].score[i] = true
 			}