Better block attestation inclusion (#4838)

* Fill blocks with unaggregated atts when possible, don't delete from pool until the block is submitted

* Add back invalid atts removal

* Don't delete attestations unless they can be aggregated

* comment and test

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

beacon-chain/operations/attestations/aggregate.go

@@ -46,10 +46,14 @@ func (s *Service) aggregateAttestations(ctx context.Context, attsToBeAggregated
 			return err
 		}
 		attsByRoot[attDataRoot] = append(attsByRoot[attDataRoot], att)
+	}
 
-		if !helpers.IsAggregated(att) {
-			if err := s.pool.DeleteUnaggregatedAttestation(att); err != nil {
-				return err
+	for _, atts := range attsByRoot {
+		for _, att := range atts {
+			if !helpers.IsAggregated(att) && len(atts) > 1 {
+				if err := s.pool.DeleteUnaggregatedAttestation(att); err != nil {
+					return err
+				}
 			}
 		}
 	}

beacon-chain/rpc/validator/proposer.go

@@ -60,6 +60,16 @@ func (vs *Server) GetBlock(ctx context.Context, req *ethpb.BlockRequest) (*ethpb
 		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...)
+	}
+
 	// Use zero hash as stub for state root to compute later.
 	stateRoot := params.BeaconConfig().ZeroHash[:]
 

beacon-chain/rpc/validator/proposer_test.go

@@ -109,6 +109,128 @@ func TestGetBlock_OK(t *testing.T) {
 	}
 }
 
+func TestGetBlock_AddsUnaggregatedAtts(t *testing.T) {
+	db := dbutil.SetupDB(t)
+	defer dbutil.TeardownDB(t, db)
+	ctx := context.Background()
+
+	beaconState, privKeys := testutil.DeterministicGenesisState(t, params.BeaconConfig().MinGenesisActiveValidatorCount)
+
+	stateRoot, err := beaconState.HashTreeRoot()
+	if err != nil {
+		t.Fatalf("Could not hash genesis state: %v", err)
+	}
+
+	genesis := b.NewGenesisBlock(stateRoot[:])
+	if err := db.SaveBlock(ctx, genesis); err != nil {
+		t.Fatalf("Could not save genesis block: %v", err)
+	}
+
+	parentRoot, err := ssz.HashTreeRoot(genesis.Block)
+	if err != nil {
+		t.Fatalf("Could not get signing root %v", err)
+	}
+	if err := db.SaveState(ctx, beaconState, parentRoot); err != nil {
+		t.Fatalf("Could not save genesis state: %v", err)
+	}
+	if err := db.SaveHeadBlockRoot(ctx, parentRoot); err != nil {
+		t.Fatalf("Could not save genesis state: %v", err)
+	}
+
+	proposerServer := &Server{
+		BeaconDB:          db,
+		HeadFetcher:       &mock.ChainService{State: beaconState, Root: parentRoot[:]},
+		SyncChecker:       &mockSync.Sync{IsSyncing: false},
+		BlockReceiver:     &mock.ChainService{},
+		ChainStartFetcher: &mockPOW.POWChain{},
+		Eth1InfoFetcher:   &mockPOW.POWChain{},
+		Eth1BlockFetcher:  &mockPOW.POWChain{},
+		MockEth1Votes:     true,
+		AttPool:           attestations.NewPool(),
+		ExitPool:          voluntaryexits.NewPool(),
+	}
+
+
+	// Generate a bunch of random attestations at slot. These would be considered double votes, but
+	// we don't care for the purpose of this test.
+	var atts []*ethpb.Attestation
+	for i := uint64(0); len(atts) < int(params.BeaconConfig().MaxAttestations); i++ {
+		a, err := testutil.GenerateAttestations(beaconState, privKeys, 2, 1, true)
+		if err != nil {
+			t.Fatal(err)
+		}
+		atts = append(atts, a...)
+	}
+	// Max attestations minus one so we can almost fill the block and then include 1 unaggregated
+	// att to maximize inclusion.
+	atts = atts[:params.BeaconConfig().MaxAttestations-1]
+	if err := proposerServer.AttPool.SaveAggregatedAttestations(atts); err != nil {
+		t.Fatal(err)
+	}
+
+	// Generate some more random attestations with a larger spread so that we can capture at least
+	// one unaggregated attestation.
+	if atts, err := testutil.GenerateAttestations(beaconState, privKeys, 8, 1, true); err != nil {
+		t.Fatal(err)
+	} else {
+		found := false
+		for _, a := range atts {
+			if !helpers.IsAggregated(a) {
+				found = true
+				if err := proposerServer.AttPool.SaveUnaggregatedAttestation(a); err != nil {
+					t.Fatal(err)
+				}
+			}
+		}
+		if !found {
+			t.Fatal("No unaggregated attestations were generated")
+		}
+	}
+
+	randaoReveal, err := testutil.RandaoReveal(beaconState, 0, privKeys)
+	if err != nil {
+		t.Error(err)
+	}
+
+	graffiti := bytesutil.ToBytes32([]byte("eth2"))
+	req := &ethpb.BlockRequest{
+		Slot:         1,
+		RandaoReveal: randaoReveal,
+		Graffiti:     graffiti[:],
+	}
+
+	block, err := proposerServer.GetBlock(ctx, req)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if block.Slot != req.Slot {
+		t.Fatal("Expected block to have slot of 1")
+	}
+	if !bytes.Equal(block.ParentRoot, parentRoot[:]) {
+		t.Fatal("Expected block to have correct parent root")
+	}
+	if !bytes.Equal(block.Body.RandaoReveal, randaoReveal) {
+		t.Fatal("Expected block to have correct randao reveal")
+	}
+	if !bytes.Equal(block.Body.Graffiti, req.Graffiti) {
+		t.Fatal("Expected block to have correct graffiti")
+	}
+	if len(block.Body.Attestations) != int(params.BeaconConfig().MaxAttestations) {
+		t.Fatalf("Expected a full block of attestations, only received %d", len(block.Body.Attestations))
+	}
+	hasUnaggregatedAtt := false
+	for _, a := range block.Body.Attestations {
+		if !helpers.IsAggregated(a) {
+			hasUnaggregatedAtt = true
+			break
+		}
+	}
+	if !hasUnaggregatedAtt {
+		t.Fatal("Expected block to contain at least one unaggregated attestation")
+	}
+}
+
 func TestProposeBlock_OK(t *testing.T) {
 	db := dbutil.SetupDB(t)
 	defer dbutil.TeardownDB(t, db)

shared/testutil/block.go

@@ -81,7 +81,7 @@ func GenerateFullBlock(
 	numToGen = conf.NumAttestations
 	atts := []*ethpb.Attestation{}
 	if numToGen > 0 {
-		atts, err = GenerateAttestations(bState, privs, numToGen, slot)
+		atts, err = GenerateAttestations(bState, privs, numToGen, slot, false)
 		if err != nil {
 			return nil, errors.Wrapf(err, "failed generating %d attestations:", numToGen)
 		}
@@ -291,19 +291,11 @@ func generateAttesterSlashings(
 // for the same data with their aggregation bits split uniformly.
 //
 // If you request 4 attestations, but there are 8 committees, you will get 4 fully aggregated attestations.
-func GenerateAttestations(
-	bState *stateTrie.BeaconState,
-	privs []*bls.SecretKey,
-	numToGen uint64,
-	slot uint64,
-) ([]*ethpb.Attestation, error) {
+func GenerateAttestations(bState *stateTrie.BeaconState, privs []*bls.SecretKey, numToGen uint64, slot uint64, randomRoot bool, ) ([]*ethpb.Attestation, error) {
 	currentEpoch := helpers.SlotToEpoch(slot)
 	attestations := []*ethpb.Attestation{}
 	generateHeadState := false
-	bState, err := stateTrie.InitializeFromProtoUnsafe(bState.CloneInnerState())
-	if err != nil {
-		return nil, err
-	}
+	bState = bState.Copy()
 	if slot > bState.Slot() {
 		// Going back a slot here so there's no inclusion delay issues.
 		slot--
@@ -312,6 +304,7 @@ func GenerateAttestations(
 
 	targetRoot := make([]byte, 32)
 	headRoot := make([]byte, 32)
+	var err error
 	// Only calculate head state if its an attestation for the current slot or future slot.
 	if generateHeadState || slot == bState.Slot() {
 		headState, err := stateTrie.InitializeFromProtoUnsafe(bState.CloneInnerState())
@@ -336,6 +329,14 @@ func GenerateAttestations(
 			return nil, err
 		}
 	}
+	if randomRoot {
+		b := make([]byte, 32)
+		_, err := rand.Read(b)
+		if err != nil {
+			return nil, err
+		}
+		headRoot = b
+	}
 
 	activeValidatorCount, err := helpers.ActiveValidatorCount(bState, currentEpoch)
 	if err != nil {
@@ -401,6 +402,11 @@ func GenerateAttestations(
 				sigs = append(sigs, privs[committee[b]].Sign(dataRoot[:], domain))
 			}
 
+			// bls.AggregateSignatures will return nil if sigs is 0.
+			if len(sigs) == 0 {
+				continue
+			}
+
 			att := &ethpb.Attestation{
 				Data:            attData,
 				AggregationBits: aggregationBits,

tools/benchmark-files-gen/main.go

@@ -115,7 +115,7 @@ func generateMarshalledFullStateAndBlock() error {
 
 	atts := []*ethpb.Attestation{}
 	for i := slotOffset + 1; i < slotsPerEpoch+slotOffset; i++ {
-		attsForSlot, err := testutil.GenerateAttestations(beaconState, privs, attConfig.NumAttestations, i)
+		attsForSlot, err := testutil.GenerateAttestations(beaconState, privs, attConfig.NumAttestations, i, false)
 		if err != nil {
 			return err
 		}