Verify Slashing Signatures Before Putting Into Blocks

beacon-chain/operations/slashings/BUILD.bazel

@@ -11,13 +11,15 @@ go_library(
     importpath = "github.com/prysmaticlabs/prysm/beacon-chain/operations/slashings",
     visibility = ["//beacon-chain:__subpackages__"],
     deps = [
+        "//beacon-chain/core/blocks:go_default_library",
         "//beacon-chain/core/helpers:go_default_library",
         "//beacon-chain/state:go_default_library",
         "//shared/params:go_default_library",
         "//shared/sliceutil:go_default_library",
         "@com_github_prometheus_client_golang//prometheus:go_default_library",
         "@com_github_prometheus_client_golang//prometheus/promauto:go_default_library",
         "@com_github_prysmaticlabs_ethereumapis//eth/v1alpha1:go_default_library",
+        "@io_opencensus_go//trace:go_default_library",
     ],
 )
 
@@ -33,6 +35,7 @@ go_test(
         "//beacon-chain/state:go_default_library",
         "//proto/beacon/p2p/v1:go_default_library",
         "//shared/params:go_default_library",
+        "//shared/testutil:go_default_library",
         "@com_github_gogo_protobuf//proto:go_default_library",
         "@com_github_prysmaticlabs_ethereumapis//eth/v1alpha1:go_default_library",
     ],

beacon-chain/operations/slashings/metrics.go

@@ -6,6 +6,12 @@ import (
 )
 
 var (
+	numPendingAttesterSlashingFailedSigVerify = promauto.NewCounter(
+		prometheus.CounterOpts{
+			Name: "pending_attester_slashing_fail_sig_verify_total",
+			Help: "Times an pending attester slashing fails sig verification",
+		},
+	)
 	numPendingAttesterSlashings = promauto.NewGauge(
 		prometheus.GaugeOpts{
 			Name: "num_pending_attester_slashings",
@@ -24,6 +30,12 @@ var (
 			Help: "Times an attester slashing for an already slashed validator is received",
 		},
 	)
+	numPendingProposerSlashingFailedSigVerify = promauto.NewCounter(
+		prometheus.CounterOpts{
+			Name: "pending_proposer_slashing_fail_sig_verify_total",
+			Help: "Times an pending proposer slashing fails sig verification",
+		},
+	)
 	numPendingProposerSlashings = promauto.NewGauge(
 		prometheus.GaugeOpts{
 			Name: "num_pending_proposer_slashings",

beacon-chain/operations/slashings/service.go

@@ -1,11 +1,15 @@
 package slashings
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"sort"
 
 	ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
+	"go.opencensus.io/trace"
+
+	"github.com/prysmaticlabs/prysm/beacon-chain/core/blocks"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/helpers"
 	beaconstate "github.com/prysmaticlabs/prysm/beacon-chain/state"
 	"github.com/prysmaticlabs/prysm/shared/params"
@@ -23,9 +27,11 @@ func NewPool() *Pool {
 
 // PendingAttesterSlashings returns attester slashings that are able to be included into a block.
 // This method will not return more than the block enforced MaxAttesterSlashings.
-func (p *Pool) PendingAttesterSlashings() []*ethpb.AttesterSlashing {
+func (p *Pool) PendingAttesterSlashings(ctx context.Context, st *beaconstate.BeaconState) []*ethpb.AttesterSlashing {
 	p.lock.RLock()
 	defer p.lock.RUnlock()
+	ctx, span := trace.StartSpan(ctx, "operations.PendingAttesterSlashing")
+	defer span.End()
 
 	// Update prom metric.
 	numPendingAttesterSlashings.Set(float64(len(p.pendingAttesterSlashing)))
@@ -44,17 +50,26 @@ func (p *Pool) PendingAttesterSlashings() []*ethpb.AttesterSlashing {
 		for _, idx := range slashedVal {
 			included[idx] = true
 		}
-		pending = append(pending, attSlashing)
+
+		if err := blocks.VerifyAttesterSlashing(ctx, st, attSlashing); err == nil {
+			pending = append(pending, attSlashing)
+		} else {
+			numPendingAttesterSlashingFailedSigVerify.Inc()
+			// Else, we clear the attester slashing from the pool.
+			p.pendingAttesterSlashing = append(p.pendingAttesterSlashing[:i], p.pendingAttesterSlashing[i+1:]...)
+		}
 	}
 
 	return pending
 }
 
 // PendingProposerSlashings returns proposer slashings that are able to be included into a block.
 // This method will not return more than the block enforced MaxProposerSlashings.
-func (p *Pool) PendingProposerSlashings() []*ethpb.ProposerSlashing {
+func (p *Pool) PendingProposerSlashings(ctx context.Context, st *beaconstate.BeaconState) []*ethpb.ProposerSlashing {
 	p.lock.RLock()
 	defer p.lock.RUnlock()
+	ctx, span := trace.StartSpan(ctx, "operations.PendingProposerSlashing")
+	defer span.End()
 
 	// Update prom metric.
 	numPendingProposerSlashings.Set(float64(len(p.pendingProposerSlashing)))
@@ -64,7 +79,13 @@ func (p *Pool) PendingProposerSlashings() []*ethpb.ProposerSlashing {
 		if i >= int(params.BeaconConfig().MaxProposerSlashings) {
 			break
 		}
-		pending = append(pending, slashing)
+		if err := blocks.VerifyProposerSlashing(st, slashing); err == nil {
+			pending = append(pending, slashing)
+		} else {
+			numPendingProposerSlashingFailedSigVerify.Inc()
+			// Else, we clear the proposer slashing from the pool.
+			p.pendingProposerSlashing = append(p.pendingProposerSlashing[:i], p.pendingProposerSlashing[i+1:]...)
+		}
 	}
 	return pending
 }

beacon-chain/operations/slashings/service_attester_test.go

@@ -1,6 +1,7 @@
 package slashings
 
 import (
+	"context"
 	"reflect"
 	"strings"
 	"testing"
@@ -10,6 +11,7 @@ import (
 	beaconstate "github.com/prysmaticlabs/prysm/beacon-chain/state"
 	p2ppb "github.com/prysmaticlabs/prysm/proto/beacon/p2p/v1"
 	"github.com/prysmaticlabs/prysm/shared/params"
+	"github.com/prysmaticlabs/prysm/shared/testutil"
 )
 
 func attesterSlashingForValIdx(valIdx ...uint64) *ethpb.AttesterSlashing {
@@ -450,6 +452,20 @@ func TestPool_PendingAttesterSlashings(t *testing.T) {
 	type fields struct {
 		pending []*PendingAttesterSlashing
 	}
+	beaconState, privKeys := testutil.DeterministicGenesisState(t, 64)
+	pendingSlashings := make([]*PendingAttesterSlashing, 20)
+	slashings := make([]*ethpb.AttesterSlashing, 20)
+	for i := 0; i < len(pendingSlashings); i++ {
+		sl, err := testutil.GenerateAttesterSlashingForValidator(beaconState, privKeys[i], uint64(i))
+		if err != nil {
+			t.Fatal(err)
+		}
+		pendingSlashings[i] = &PendingAttesterSlashing{
+			attesterSlashing: sl,
+			validatorToSlash: uint64(i),
+		}
+		slashings[i] = sl
+	}
 	tests := []struct {
 		name   string
 		fields fields
@@ -465,105 +481,102 @@ func TestPool_PendingAttesterSlashings(t *testing.T) {
 		{
 			name: "All eligible",
 			fields: fields{
-				pending: generateNPendingSlashings(1),
+				pending: pendingSlashings,
 			},
-			want: generateNAttSlashings(1),
+			want: slashings[0:1],
 		},
 		{
 			name: "Multiple indices",
 			fields: fields{
-				pending: []*PendingAttesterSlashing{
-					pendingSlashingForValIdx(1, 5, 8),
-				},
-			},
-			want: []*ethpb.AttesterSlashing{
-				attesterSlashingForValIdx(1, 5, 8),
+				pending: pendingSlashings[3:6],
 			},
-		},
-		{
-			name: "All eligible, over max",
-			fields: fields{
-				pending: generateNPendingSlashings(6),
-			},
-			want: generateNAttSlashings(1),
-		},
-		{
-			name: "No duplicate slashings for grouped",
-			fields: fields{
-				pending: generateNPendingSlashings(16),
-			},
-			want: generateNAttSlashings(1),
+			want: slashings[3:4],
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			p := &Pool{
 				pendingAttesterSlashing: tt.fields.pending,
 			}
-			if got := p.PendingAttesterSlashings(); !reflect.DeepEqual(tt.want, got) {
+			if got := p.PendingAttesterSlashings(
+				context.Background(),
+				beaconState,
+			); !reflect.DeepEqual(tt.want, got) {
 				t.Errorf("Unexpected return from PendingAttesterSlashings, wanted %v, received %v", tt.want, got)
 			}
 		})
 	}
 }
 
-func TestPool_PendingAttesterSlashings_2Max(t *testing.T) {
+func TestPool_PendingAttesterSlashings_NoDuplicates(t *testing.T) {
 	conf := params.BeaconConfig()
 	conf.MaxAttesterSlashings = 2
 	params.OverrideBeaconConfig(conf)
+	beaconState, privKeys := testutil.DeterministicGenesisState(t, 64)
+	pendingSlashings := make([]*PendingAttesterSlashing, 3)
+	slashings := make([]*ethpb.AttesterSlashing, 3)
+	for i := 0; i < 2; i++ {
+		sl, err := testutil.GenerateAttesterSlashingForValidator(beaconState, privKeys[i], uint64(i))
+		if err != nil {
+			t.Fatal(err)
+		}
+		pendingSlashings[i] = &PendingAttesterSlashing{
+			attesterSlashing: sl,
+			validatorToSlash: uint64(i),
+		}
+		slashings[i] = sl
+	}
+	// We duplicate the last slashing.
+	pendingSlashings[2] = pendingSlashings[1]
+	slashings[2] = slashings[1]
+	p := &Pool{
+		pendingAttesterSlashing: pendingSlashings,
+	}
+	want := slashings[0:2]
+	if got := p.PendingAttesterSlashings(
+		context.Background(),
+		beaconState,
+	); !reflect.DeepEqual(want, got) {
+		t.Errorf("Unexpected return from PendingAttesterSlashings, wanted %v, received %v", want, got)
+	}
+}
 
-	type fields struct {
-		pending []*PendingAttesterSlashing
+func TestPool_PendingAttesterSlashings_SigFailsVerify_ClearPool(t *testing.T) {
+	conf := params.BeaconConfig()
+	conf.MaxAttesterSlashings = 2
+	params.OverrideBeaconConfig(conf)
+	beaconState, privKeys := testutil.DeterministicGenesisState(t, 64)
+	pendingSlashings := make([]*PendingAttesterSlashing, 2)
+	slashings := make([]*ethpb.AttesterSlashing, 2)
+	for i := 0; i < 2; i++ {
+		sl, err := testutil.GenerateAttesterSlashingForValidator(beaconState, privKeys[i], uint64(i))
+		if err != nil {
+			t.Fatal(err)
+		}
+		pendingSlashings[i] = &PendingAttesterSlashing{
+			attesterSlashing: sl,
+			validatorToSlash: uint64(i),
+		}
+		slashings[i] = sl
 	}
-	tests := []struct {
-		name   string
-		fields fields
-		want   []*ethpb.AttesterSlashing
-	}{
-		{
-			name: "No duplicates with grouped att slashings",
-			fields: fields{
-				pending: []*PendingAttesterSlashing{
-					{
-						attesterSlashing: attesterSlashingForValIdx(4, 12, 40),
-						validatorToSlash: 4,
-					},
-					{
-						attesterSlashing: attesterSlashingForValIdx(6, 8, 24),
-						validatorToSlash: 6,
-					},
-					{
-						attesterSlashing: attesterSlashingForValIdx(6, 8, 24),
-						validatorToSlash: 8,
-					},
-					{
-						attesterSlashing: attesterSlashingForValIdx(4, 12, 40),
-						validatorToSlash: 12,
-					},
-					{
-						attesterSlashing: attesterSlashingForValIdx(6, 8, 24),
-						validatorToSlash: 24,
-					},
-					{
-						attesterSlashing: attesterSlashingForValIdx(4, 12, 40),
-						validatorToSlash: 40,
-					},
-				},
-			},
-			want: []*ethpb.AttesterSlashing{
-				attesterSlashingForValIdx(4, 12, 40),
-				attesterSlashingForValIdx(6, 8, 24),
-			},
-		},
+	// We mess up the signature of the second slashing.
+	badSig := make([]byte, 96)
+	copy(badSig, "muahaha")
+	pendingSlashings[1].attesterSlashing.Attestation_1.Signature = badSig
+	slashings[1].Attestation_1.Signature = badSig
+	p := &Pool{
+		pendingAttesterSlashing: pendingSlashings,
 	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			p := &Pool{
-				pendingAttesterSlashing: tt.fields.pending,
-			}
-			if got := p.PendingAttesterSlashings(); !reflect.DeepEqual(tt.want, got) {
-				t.Errorf("Unexpected return from PendingAttesterSlashings, wanted %v, received %v", tt.want, got)
-			}
-		})
+	// We only want a single attester slashing to remain.
+	want := slashings[0:1]
+	if got := p.PendingAttesterSlashings(
+		context.Background(),
+		beaconState,
+	); !reflect.DeepEqual(want, got) {
+		t.Errorf("Unexpected return from PendingAttesterSlashings, wanted %v, received %v", want, got)
+	}
+	// We expect to only have 1 pending attester slashing in the pool.
+	if len(p.pendingAttesterSlashing) != 1 {
+		t.Error("Expected failed attester slashing to have been cleared from pool")
 	}
 }