GetBlock: Refactor attestation packing (#5632)

* Refactor attestation packing slightly to reduce the skip slot / HTR of process slots
* Merge branch 'master' into refactor-attestation-packing
* gofmt
* Merge branch 'refactor-attestation-packing' of github.com:prysmaticlabs/prysm into refactor-attestation-packing

beacon-chain/rpc/validator/proposer.go

@@ -61,20 +61,9 @@ func (vs *Server) GetBlock(ctx context.Context, req *ethpb.BlockRequest) (*ethpb
 	}
 
 	// Pack aggregated attestations which have not been included in the beacon chain.
-	atts := vs.AttPool.AggregatedAttestations()
-	atts, err = vs.filterAttestationsForBlockInclusion(ctx, req.Slot, atts)
+	atts, err := vs.packAttestations(ctx, req.Slot)
 	if err != nil {
-		return nil, status.Errorf(codes.Internal, "Could not filter attestations: %v", err)
-	}
-
-	// If there is any room left in the block, consider unaggregated attestations as well.
-	if len(atts) < int(params.BeaconConfig().MaxAttestations) {
-		uAtts := vs.AttPool.UnaggregatedAttestations()
-		uAtts, err = vs.filterAttestationsForBlockInclusion(ctx, req.Slot, uAtts)
-		if len(uAtts)+len(atts) > int(params.BeaconConfig().MaxAttestations) {
-			uAtts = uAtts[:int(params.BeaconConfig().MaxAttestations)-len(atts)]
-		}
-		atts = append(atts, uAtts...)
+		return nil, status.Errorf(codes.Internal, "Could not get attestations to pack into block: %v", err)
 	}
 
 	// Use zero hash as stub for state root to compute later.
@@ -392,32 +381,20 @@ func (vs *Server) defaultEth1DataResponse(ctx context.Context, currentHeight *bi
 }
 
 // This filters the input attestations to return a list of valid attestations to be packaged inside a beacon block.
-func (vs *Server) filterAttestationsForBlockInclusion(ctx context.Context, slot uint64, atts []*ethpb.Attestation) ([]*ethpb.Attestation, error) {
+func (vs *Server) filterAttestationsForBlockInclusion(ctx context.Context, state *stateTrie.BeaconState, atts []*ethpb.Attestation) ([]*ethpb.Attestation, error) {
 	ctx, span := trace.StartSpan(ctx, "ProposerServer.filterAttestationsForBlockInclusion")
 	defer span.End()
 
 	validAtts := make([]*ethpb.Attestation, 0, len(atts))
 	inValidAtts := make([]*ethpb.Attestation, 0, len(atts))
 
-	bState, err := vs.HeadFetcher.HeadState(ctx)
-	if err != nil {
-		return nil, errors.New("could not head state from DB")
-	}
-
-	if bState.Slot() < slot {
-		bState, err = state.ProcessSlots(ctx, bState, slot)
-		if err != nil {
-			return nil, errors.Wrapf(err, "could not process slots up to %d", slot)
-		}
-	}
-
 	// TODO(3916): Insert optimizations to sort out the most profitable attestations
 	for i, att := range atts {
 		if i == int(params.BeaconConfig().MaxAttestations) {
 			break
 		}
 
-		if _, err := blocks.ProcessAttestation(ctx, bState, att); err != nil {
+		if _, err := blocks.ProcessAttestation(ctx, state, att); err != nil {
 			inValidAtts = append(inValidAtts, att)
 			continue
 
@@ -460,3 +437,35 @@ func constructMerkleProof(trie *trieutil.SparseMerkleTrie, index int, deposit *e
 	deposit.Proof = proof
 	return deposit, nil
 }
+
+func (vs *Server) packAttestations(ctx context.Context, slot uint64) ([]*ethpb.Attestation, error) {
+	ctx, span := trace.StartSpan(ctx, "validatorServer.packAttestations")
+	defer span.End()
+	st, err := vs.HeadFetcher.HeadState(ctx)
+	if err != nil {
+		return nil, errors.Wrap(err, "could fetch head state")
+	}
+	if st.Slot() < slot {
+		st, err = state.ProcessSlots(ctx, st, slot)
+		if err != nil {
+			return nil, errors.Wrap(err, "could not advance state")
+		}
+	}
+
+	atts := vs.AttPool.AggregatedAttestations()
+	atts, err = vs.filterAttestationsForBlockInclusion(ctx, st, atts)
+	if err != nil {
+		return nil, errors.Wrap(err, "could not filter attestations")
+	}
+
+	// If there is any room left in the block, consider unaggregated attestations as well.
+	if len(atts) < int(params.BeaconConfig().MaxAttestations) {
+		uAtts := vs.AttPool.UnaggregatedAttestations()
+		uAtts, err = vs.filterAttestationsForBlockInclusion(ctx, st, uAtts)
+		if len(uAtts)+len(atts) > int(params.BeaconConfig().MaxAttestations) {
+			uAtts = uAtts[:int(params.BeaconConfig().MaxAttestations)-len(atts)]
+		}
+		atts = append(atts, uAtts...)
+	}
+	return atts, nil
+}

beacon-chain/rpc/validator/proposer_test.go

@@ -1305,6 +1305,9 @@ func TestFilterAttestation_OK(t *testing.T) {
 	if err := state.SetGenesisValidatorRoot(params.BeaconConfig().ZeroHash[:]); err != nil {
 		t.Fatal(err)
 	}
+	if err := state.SetSlot(1); err != nil {
+		t.Error(err)
+	}
 
 	genesisRoot, err := ssz.HashTreeRoot(genesis.Block)
 	if err != nil {
@@ -1330,7 +1333,7 @@ func TestFilterAttestation_OK(t *testing.T) {
 			Target:         &ethpb.Checkpoint{}},
 		}
 	}
-	received, err := proposerServer.filterAttestationsForBlockInclusion(context.Background(), 1, atts)
+	received, err := proposerServer.filterAttestationsForBlockInclusion(context.Background(), state, atts)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1374,7 +1377,7 @@ func TestFilterAttestation_OK(t *testing.T) {
 		atts[i].Signature = bls.AggregateSignatures(sigs).Marshal()[:]
 	}
 
-	received, err = proposerServer.filterAttestationsForBlockInclusion(context.Background(), 1, atts)
+	received, err = proposerServer.filterAttestationsForBlockInclusion(context.Background(), state, atts)
 	if err != nil {
 		t.Fatal(err)
 	}

beacon-chain/rpc/validator/server.go

@@ -2,14 +2,10 @@ package validator
 
 import (
 	"context"
-	"errors"
 	"time"
 
-	"github.com/prysmaticlabs/prysm/beacon-chain/state/stategen"
-	"github.com/prysmaticlabs/prysm/shared/bytesutil"
-	"github.com/prysmaticlabs/prysm/shared/params"
-
 	ptypes "github.com/gogo/protobuf/types"
+	"github.com/pkg/errors"
 	ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
 	"github.com/prysmaticlabs/prysm/beacon-chain/blockchain"
 	"github.com/prysmaticlabs/prysm/beacon-chain/cache"
@@ -25,8 +21,11 @@ import (
 	"github.com/prysmaticlabs/prysm/beacon-chain/operations/voluntaryexits"
 	"github.com/prysmaticlabs/prysm/beacon-chain/p2p"
 	"github.com/prysmaticlabs/prysm/beacon-chain/powchain"
+	"github.com/prysmaticlabs/prysm/beacon-chain/state/stategen"
 	"github.com/prysmaticlabs/prysm/beacon-chain/sync"
 	pbp2p "github.com/prysmaticlabs/prysm/proto/beacon/p2p/v1"
+	"github.com/prysmaticlabs/prysm/shared/bytesutil"
+	"github.com/prysmaticlabs/prysm/shared/params"
 	"github.com/sirupsen/logrus"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"