Properly clean up attestations during state transitions (#705)

beacon-chain/blockchain/service.go

@@ -215,6 +215,23 @@ func (c *ChainService) updateHead(processedBlock <-chan *types.Block) {
 	}
 }
 
+func (c *ChainService) executeStateTransition(
+	cState *types.CrystallizedState,
+	aState *types.ActiveState,
+	block *types.Block) (*types.CrystallizedState, error) {
+
+	var err error
+	log.Infof("Executing state transition for slot: %d", block.SlotNumber())
+	for cState.IsCycleTransition(block.SlotNumber()) {
+		cState, err = cState.NewStateRecalculations(aState, block, c.enableCrossLinks, c.enableRewardChecking)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	return cState, nil
+}
+
 func (c *ChainService) blockProcessing(processedBlock chan<- *types.Block) {
 	subBlock := c.incomingBlockFeed.Subscribe(c.incomingBlockChan)
 	defer subBlock.Unsubscribe()
@@ -271,16 +288,11 @@ func (c *ChainService) blockProcessing(processedBlock chan<- *types.Block) {
 			// If the block is valid, we compute its associated state tuple (active, crystallized)
 			// and apply a block scoring function.
 			var didCycleTransition bool
-			for cState.IsCycleTransition(block.SlotNumber()) {
-				log.Infof("executing state transition for slot %d", block.SlotNumber())
-				cState, err = cState.NewStateRecalculations(
-					aState,
-					block,
-					c.enableCrossLinks,
-					c.enableRewardChecking,
-				)
+			if cState.IsCycleTransition(block.SlotNumber()) {
+				cState, err = c.executeStateTransition(cState, aState, block)
 				if err != nil {
 					log.Errorf("Initialize new cycle transition failed: %v", err)
+					continue
 				}
 				didCycleTransition = true
 			}

beacon-chain/types/active_state.go

@@ -161,16 +161,19 @@ func (a *ActiveState) appendNewSpecialObject(record *pb.SpecialRecord) []*pb.Spe
 	return append(existing, record)
 }
 
-// cleanUpAttestations removes attestations older than last state recalc slot.
-func (a *ActiveState) cleanUpAttestations(lastStateRecalc uint64) []*pb.AggregatedAttestation {
+// updateAttestations removes attestations older than last state recalc slot.
+func (a *ActiveState) updateAttestations(lastStateRecalc uint64, newAttestations []*pb.AggregatedAttestation) {
 	existing := a.data.PendingAttestations
-	var update []*pb.AggregatedAttestation
-	for i := 0; i < len(existing); i++ {
-		if existing[i].GetSlot() >= lastStateRecalc {
-			update = append(update, existing[i])
+	update := make([]*pb.AggregatedAttestation, 0, len(existing)+len(newAttestations))
+	for _, a := range existing {
+		if a.GetSlot() >= lastStateRecalc {
+			update = append(update, a)
 		}
 	}
-	return update
+
+	update = append(update, newAttestations...)
+
+	a.data.PendingAttestations = update
 }
 
 // calculateNewBlockHashes builds a new slice of recent block hashes with the
@@ -272,20 +275,6 @@ func (a *ActiveState) calculateNewVoteCache(block *Block, cState *CrystallizedSt
 	return update, nil
 }
 
-// CleanUpActiveState removes the old attestations going from a cycle length behind
-// from the last state recalc and then generates the new active state. This is run after
-// a crystallized state transition.
-func (a *ActiveState) CleanUpActiveState(lastStateRecalc uint64) *ActiveState {
-	slot := lastStateRecalc - params.GetConfig().CycleLength
-	newPendingAttestations := a.cleanUpAttestations(slot)
-
-	// Construct new active state after clean up pending attestations.
-	return NewActiveState(&pb.ActiveState{
-		PendingAttestations: newPendingAttestations,
-		RecentBlockHashes:   a.data.RecentBlockHashes,
-	}, a.blockVoteCache)
-}
-
 // CalculateNewActiveState returns the active state for `block` based on its own state.
 // This method should not modify its own state.
 func (a *ActiveState) CalculateNewActiveState(
@@ -296,11 +285,8 @@ func (a *ActiveState) CalculateNewActiveState(
 	var err error
 
 	newState := a.CopyState()
-	// Cleans up old attestations.
-	newState.CleanUpActiveState(cState.LastStateRecalculationSlot())
 
-	// Derive the new set of pending attestations.
-	newState.data.PendingAttestations = newState.appendNewAttestations(block.data.Attestations)
+	newState.updateAttestations(cState.LastStateRecalculationSlot(), block.Attestations())
 
 	// Derive the new set of recent block hashes.
 	newState.data.RecentBlockHashes, err = newState.calculateNewBlockHashes(block, parentSlot)

beacon-chain/types/active_state_test.go

@@ -162,11 +162,9 @@ func TestUpdateAttestationsAfterRecalc(t *testing.T) {
 		},
 	}
 
-	updatedAttestations := aState.appendNewAttestations(newAttestations)
-	aState.data.PendingAttestations = updatedAttestations
-	updatedAttestations = aState.cleanUpAttestations(8)
-	if len(updatedAttestations) != 2 {
-		t.Fatalf("Updated attestations should be length 2: %d", len(updatedAttestations))
+	aState.updateAttestations(8, newAttestations)
+	if len(aState.PendingAttestations()) != 2 {
+		t.Fatalf("Updated attestations should be length 2: %d", len(aState.PendingAttestations()))
 	}
 }