Cleanup validateAggregateAttTime with a better error message, more tests, and more clear inline comments (#6423)

* Add span for updateRecentCanonicalBlocks
* Cleanup validateAggregateAttTime with a better error message, more tests, and more clear inline comments
* Merge branch 'master' into cleanup-validateAggregateAttTime
* revert beacon-chain/blockchain/head.go
* Merge branch 'cleanup-validateAggregateAttTime' of github.com:prysmaticlabs/prysm into cleanup-validateAggregateAttTime
* Merge refs/heads/master into cleanup-validateAggregateAttTime
* Merge refs/heads/master into cleanup-validateAggregateAttTime

beacon-chain/sync/validate_aggregate_proof.go

@@ -191,20 +191,46 @@ func validateIndexInCommittee(ctx context.Context, bs *stateTrie.BeaconState, a
 	return nil
 }
 
-// Validates that the incoming aggregate attestation is in the desired time range.
+// Validates that the incoming aggregate attestation is in the desired time range. An attestation
+// is valid only if received within the last ATTESTATION_PROPAGATION_SLOT_RANGE slots.
+//
+// Example:
+//   ATTESTATION_PROPAGATION_SLOT_RANGE = 5
+//   current_slot = 100
+//   invalid_attestation_slot = 92
+//   invalid_attestation_slot = 101
+//   valid_attestation_slot = 98
+// In the attestation must be within the range of 95 to 100 in the example above.
 func validateAggregateAttTime(attSlot uint64, genesisTime time.Time) error {
-	// set expected boundaries for attestations
 	attTime := genesisTime.Add(time.Duration(attSlot*params.BeaconConfig().SecondsPerSlot) * time.Second)
-	attSlotRange := attSlot + params.BeaconNetworkConfig().AttestationPropagationSlotRange
-	attTimeRange := genesisTime.Add(time.Duration(attSlotRange*params.BeaconConfig().SecondsPerSlot) * time.Second)
-	currentTime := roughtime.Now()
+	currentSlot := helpers.SlotsSince(genesisTime)
+
+	// A clock disparity allows for minor tolerances outside of the expected range. This value is
+	// usually small, less than 1 second.
 	clockDisparity := params.BeaconNetworkConfig().MaximumGossipClockDisparity
 
-	// Verify attestation slot is within the last ATTESTATION_PROPAGATION_SLOT_RANGE slots.
-	currentSlot := helpers.SlotsSince(genesisTime)
-	if attTime.Add(-clockDisparity).After(currentTime) ||
-		currentTime.Add(-clockDisparity).After(attTimeRange) {
-		return fmt.Errorf("attestation slot out of range %d <= %d <= %d", attSlot, currentSlot, attSlot+params.BeaconNetworkConfig().AttestationPropagationSlotRange)
+	// An attestation cannot be from the future, so the upper bounds is set to now, with a minor
+	// tolerance for peer clock disparity.
+	upperBounds := roughtime.Now().Add(clockDisparity)
+
+	// An attestation cannot be older than the current slot - attestation propagation slot range
+	// with a minor tolerance for peer clock disparity.
+	lowerBoundsSlot := uint64(0)
+	if currentSlot > params.BeaconNetworkConfig().AttestationPropagationSlotRange {
+		lowerBoundsSlot = currentSlot - params.BeaconNetworkConfig().AttestationPropagationSlotRange
+	}
+	lowerBounds := genesisTime.Add(
+		time.Duration(lowerBoundsSlot*params.BeaconConfig().SecondsPerSlot) * time.Second,
+	).Add(-clockDisparity)
+
+	// Verify attestation slot within the time range.
+	if attTime.Before(lowerBounds) || attTime.After(upperBounds) {
+		return fmt.Errorf(
+			"attestation slot %d not within attestation propagation range of %d to %d (current slot)",
+			attSlot,
+			currentSlot-params.BeaconNetworkConfig().AttestationPropagationSlotRange,
+			currentSlot,
+		)
 	}
 	return nil
 }

beacon-chain/sync/validate_aggregate_proof_test.go

@@ -27,6 +27,7 @@ import (
 	"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/testutil"
 )
 
@@ -637,3 +638,89 @@ func TestVerifyIndexInCommittee_SeenAggregatorEpoch(t *testing.T) {
 		t.Fatal("Validated status is true")
 	}
 }
+
+func Test_validateAggregateAttTime(t *testing.T) {
+	if params.BeaconNetworkConfig().MaximumGossipClockDisparity < 200*time.Millisecond {
+		t.Fatal("This test expects the maximum clock disparity to be at least 200ms")
+	}
+
+	type args struct {
+		attSlot     uint64
+		genesisTime time.Time
+	}
+	tests := []struct {
+		name    string
+		args    args
+		wantErr bool
+	}{
+		{
+			name: "attestation.slot == current_slot",
+			args: args{
+				attSlot:     15,
+				genesisTime: roughtime.Now().Add(-15 * time.Duration(params.BeaconConfig().SecondsPerSlot) * time.Second),
+			},
+			wantErr: false,
+		},
+		{
+			name: "attestation.slot == current_slot, received in middle of slot",
+			args: args{
+				attSlot: 15,
+				genesisTime: roughtime.Now().Add(
+					-15 * time.Duration(params.BeaconConfig().SecondsPerSlot) * time.Second,
+				).Add(-(time.Duration(params.BeaconConfig().SecondsPerSlot/2) * time.Second)),
+			},
+			wantErr: false,
+		},
+		{
+			name: "attestation.slot == current_slot, received 200ms early",
+			args: args{
+				attSlot: 16,
+				genesisTime: roughtime.Now().Add(
+					-16 * time.Duration(params.BeaconConfig().SecondsPerSlot) * time.Second,
+				).Add(-200 * time.Millisecond),
+			},
+			wantErr: false,
+		},
+		{
+			name: "attestation.slot > current_slot",
+			args: args{
+				attSlot:     16,
+				genesisTime: roughtime.Now().Add(-15 * time.Duration(params.BeaconConfig().SecondsPerSlot) * time.Second),
+			},
+			wantErr: true,
+		},
+		{
+			name: "attestation.slot < current_slot-ATTESTATION_PROPAGATION_SLOT_RANGE",
+			args: args{
+				attSlot:     100 - params.BeaconNetworkConfig().AttestationPropagationSlotRange - 1,
+				genesisTime: roughtime.Now().Add(-100 * time.Duration(params.BeaconConfig().SecondsPerSlot) * time.Second),
+			},
+			wantErr: true,
+		},
+		{
+			name: "attestation.slot = current_slot-ATTESTATION_PROPAGATION_SLOT_RANGE",
+			args: args{
+				attSlot:     100 - params.BeaconNetworkConfig().AttestationPropagationSlotRange,
+				genesisTime: roughtime.Now().Add(-100 * time.Duration(params.BeaconConfig().SecondsPerSlot) * time.Second),
+			},
+			wantErr: false,
+		},
+		{
+			name: "attestation.slot = current_slot-ATTESTATION_PROPAGATION_SLOT_RANGE, received 200ms late",
+			args: args{
+				attSlot: 100 - params.BeaconNetworkConfig().AttestationPropagationSlotRange,
+				genesisTime: roughtime.Now().Add(
+					-100 * time.Duration(params.BeaconConfig().SecondsPerSlot) * time.Second,
+				).Add(200 * time.Millisecond),
+			},
+			wantErr: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := validateAggregateAttTime(tt.args.attSlot, tt.args.genesisTime); (err != nil) != tt.wantErr {
+				t.Errorf("validateAggregateAttTime() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}