From 90ed37a655cedbfc2c2ee9ca9ecf9f2d4351c7a0 Mon Sep 17 00:00:00 2001 From: Ivan Martinez Date: Thu, 20 Feb 2020 09:56:37 -0500 Subject: [PATCH] Cleanup detection code (#4915) --- slasher/detection/attestations/BUILD.bazel | 2 +- .../detection/attestations/attestations.go | 96 ++++++++++--------- .../attestations/attestations_bench_test.go | 8 +- ...testation_test.go => attestations_test.go} | 8 +- slasher/rpc/server.go | 28 ++---- 5 files changed, 69 insertions(+), 73 deletions(-) rename slasher/detection/attestations/{attestation_test.go => attestations_test.go} (95%) diff --git a/slasher/detection/attestations/BUILD.bazel b/slasher/detection/attestations/BUILD.bazel index 74495bccbd..90bf15e79b 100644 --- a/slasher/detection/attestations/BUILD.bazel +++ b/slasher/detection/attestations/BUILD.bazel @@ -17,8 +17,8 @@ go_library( go_test( name = "go_default_test", srcs = [ - "attestation_test.go", "attestations_bench_test.go", + "attestations_test.go", ], embed = [":go_default_library"], deps = [ diff --git a/slasher/detection/attestations/attestations.go b/slasher/detection/attestations/attestations.go index a021badffa..9df2785672 100644 --- a/slasher/detection/attestations/attestations.go +++ b/slasher/detection/attestations/attestations.go @@ -13,21 +13,21 @@ import ( // Detector is a function type used to implement the slashable surrounding/surrounded // vote detection methods. -type detectFn = func(attestationEpochSpan uint64, recorderEpochSpan *slashpb.MinMaxEpochSpan, sourceEpoch uint64) uint64 +type detectFn = func(span *slashpb.MinMaxEpochSpan, sourceEpoch uint64, attDistance uint64) uint64 // detectMax is a function for maxDetector used to detect surrounding attestations. -func detectMax(attEpochSpan uint64, recorderEpochSpan *slashpb.MinMaxEpochSpan, attSourceEpoch uint64) uint64 { - maxSpan := uint64(recorderEpochSpan.MaxEpochSpan) - if maxSpan > attEpochSpan { +func detectMax(span *slashpb.MinMaxEpochSpan, attSourceEpoch uint64, attDistance uint64) uint64 { + maxSpan := uint64(span.MaxEpochSpan) + if maxSpan > attDistance { return maxSpan + attSourceEpoch } return 0 } // detectMin is a function for minDetector used to detect surrounded attestations. -func detectMin(attEpochSpan uint64, recorderEpochSpan *slashpb.MinMaxEpochSpan, attSourceEpoch uint64) uint64 { - minSpan := uint64(recorderEpochSpan.MinEpochSpan) - if minSpan > 0 && minSpan < attEpochSpan { +func detectMin(span *slashpb.MinMaxEpochSpan, attSourceEpoch uint64, attDistance uint64) uint64 { + minSpan := uint64(span.MinEpochSpan) + if minSpan > 0 && minSpan < attDistance { return minSpan + attSourceEpoch } return 0 @@ -38,20 +38,25 @@ func detectMin(attEpochSpan uint64, recorderEpochSpan *slashpb.MinMaxEpochSpan, // Detailed here: https://github.com/protolambda/eth2-surround/blob/master/README.md#min-max-surround func DetectAndUpdateSpans( ctx context.Context, - att *ethpb.IndexedAttestation, spanMap *slashpb.EpochSpanMap, -) (*slashpb.EpochSpanMap, uint64, uint64, error) { + att *ethpb.IndexedAttestation, +) (*slashpb.EpochSpanMap, uint64, error) { ctx, span := trace.StartSpan(ctx, "Detection.DetectAndUpdateSpans") defer span.End() - minTargetEpoch, spanMap, err := detectAndUpdateMinEpochSpan(ctx, att.Data.Source.Epoch, att.Data.Target.Epoch, spanMap) + spanMap, minTargetEpoch, err := detectAndUpdateMinEpochSpan(ctx, spanMap, att.Data.Source.Epoch, att.Data.Target.Epoch) if err != nil { - return nil, 0, 0, errors.Wrap(err, "failed to update min spans") + return nil, 0, errors.Wrap(err, "failed to update min spans") } - maxTargetEpoch, spanMap, err := detectAndUpdateMaxEpochSpan(ctx, att.Data.Source.Epoch, att.Data.Target.Epoch, spanMap) + spanMap, maxTargetEpoch, err := detectAndUpdateMaxEpochSpan(ctx, spanMap, att.Data.Source.Epoch, att.Data.Target.Epoch) if err != nil { - return nil, 0, 0, errors.Wrap(err, "failed to update max spans") + return nil, 0, errors.Wrap(err, "failed to update max spans") } - return spanMap, minTargetEpoch, maxTargetEpoch, nil + + slashableEpoch := minTargetEpoch + if slashableEpoch == 0 { + slashableEpoch = maxTargetEpoch + } + return spanMap, slashableEpoch, nil } // detectAndUpdateMaxEpochSpan is used to detect and update the max span of an incoming attestation. @@ -62,25 +67,29 @@ func DetectAndUpdateSpans( // Detailed here: https://github.com/protolambda/eth2-surround/blob/master/README.md#min-max-surround func detectAndUpdateMaxEpochSpan( ctx context.Context, + spanMap *slashpb.EpochSpanMap, source uint64, target uint64, - spanMap *slashpb.EpochSpanMap, -) (uint64, *slashpb.EpochSpanMap, error) { +) (*slashpb.EpochSpanMap, uint64, error) { ctx, span := trace.StartSpan(ctx, "Detection.detectAndUpdateMaxEpochSpan") defer span.End() - if target < source { - return 0, nil, fmt.Errorf("target: %d < source: %d ", target, source) + if source > target { + return nil, 0, fmt.Errorf( + "source cannot be greater than target, received source %d, target %d", + source, + target, + ) } - targetEpoch, minMaxSpan, spanMap, err := detectSlashingByEpochSpan(ctx, source, target, spanMap, detectMax) + spanMap, distance, targetEpoch, err := detectSlashingByEpochSpan(ctx, spanMap, source, target, detectMax) if err != nil { - return 0, nil, err + return nil, 0, err } if targetEpoch > 0 { - return targetEpoch, spanMap, nil + return spanMap, targetEpoch, nil } for i := uint64(1); i < target-source; i++ { - val := uint32(minMaxSpan - i) + val := uint32(distance - i) if _, ok := spanMap.EpochSpanMap[source+i]; !ok { spanMap.EpochSpanMap[source+i] = &slashpb.MinMaxEpochSpan{} } @@ -90,7 +99,7 @@ func detectAndUpdateMaxEpochSpan( break } } - return 0, spanMap, nil + return spanMap, 0, nil } // detectAndUpdateMinEpochSpan is used to detect surrounded votes and update the min epoch span @@ -102,28 +111,28 @@ func detectAndUpdateMaxEpochSpan( // Detailed here: https://github.com/protolambda/eth2-surround/blob/master/README.md#min-max-surround func detectAndUpdateMinEpochSpan( ctx context.Context, + spanMap *slashpb.EpochSpanMap, source uint64, target uint64, - spanMap *slashpb.EpochSpanMap, -) (uint64, *slashpb.EpochSpanMap, error) { +) (*slashpb.EpochSpanMap, uint64, error) { ctx, span := trace.StartSpan(ctx, "Detection.detectAndUpdateMinEpochSpan") defer span.End() - if target < source { - return 0, nil, fmt.Errorf( - "target: %d < source: %d ", - target, + if source > target { + return nil, 0, fmt.Errorf( + "source cannot be greater than target, received source %d, target %d", source, + target, ) } - targetEpoch, _, spanMap, err := detectSlashingByEpochSpan(ctx, source, target, spanMap, detectMin) + spanMap, _, targetEpoch, err := detectSlashingByEpochSpan(ctx, spanMap, source, target, detectMin) if err != nil { - return 0, nil, err + return nil, 0, err } if targetEpoch > 0 { - return targetEpoch, spanMap, nil + return spanMap, targetEpoch, nil } if source == 0 { - return 0, spanMap, nil + return spanMap, 0, nil } for i := source - 1; i > 0; i-- { @@ -137,7 +146,7 @@ func detectAndUpdateMinEpochSpan( break } } - return 0, spanMap, nil + return spanMap, 0, nil } // detectSlashingByEpochSpan is used to detect if a slashable event is present @@ -146,23 +155,22 @@ func detectAndUpdateMinEpochSpan( // for both surrounding and surrounded vote cases. func detectSlashingByEpochSpan( ctx context.Context, + spanMap *slashpb.EpochSpanMap, source uint64, target uint64, - spanMap *slashpb.EpochSpanMap, detector detectFn, -) (uint64, uint64, *slashpb.EpochSpanMap, error) { +) (*slashpb.EpochSpanMap, uint64, uint64, error) { ctx, span := trace.StartSpan(ctx, "Detection.detectSlashingByEpochSpan") defer span.End() - minMaxSpan := target - source - if minMaxSpan > params.BeaconConfig().WeakSubjectivityPeriod { - return 0, minMaxSpan, nil, fmt.Errorf( - "target: %d - source: %d > weakSubjectivityPeriod", - params.BeaconConfig().WeakSubjectivityPeriod, - minMaxSpan, + distance := target - source + if distance > params.BeaconConfig().WeakSubjectivityPeriod { + return nil, distance, 0, fmt.Errorf( + "attestation span was greater than waek subjectivity period, received: %d", + distance, ) } if _, ok := spanMap.EpochSpanMap[source]; ok { - return detector(minMaxSpan, spanMap.EpochSpanMap[source], source), minMaxSpan, spanMap, nil + return spanMap, distance, detector(spanMap.EpochSpanMap[source], source, distance), nil } - return 0, minMaxSpan, spanMap, nil + return spanMap, distance, 0, nil } diff --git a/slasher/detection/attestations/attestations_bench_test.go b/slasher/detection/attestations/attestations_bench_test.go index 2cca710a2b..26e2444662 100644 --- a/slasher/detection/attestations/attestations_bench_test.go +++ b/slasher/detection/attestations/attestations_bench_test.go @@ -27,7 +27,7 @@ func BenchmarkMinSpan(b *testing.B) { if err != nil { b.Fatal(err) } - _, _, err = detectAndUpdateMinEpochSpan(ctx, i, i+diff, spanMap) + _, _, err = detectAndUpdateMinEpochSpan(ctx, spanMap, i, i+diff) if err != nil { b.Fatal(err) } @@ -52,7 +52,7 @@ func BenchmarkMaxSpan(b *testing.B) { if err != nil { b.Fatal(err) } - _, _, err = detectAndUpdateMaxEpochSpan(ctx, diff, diff+i, spanMap) + _, _, err = detectAndUpdateMaxEpochSpan(ctx, spanMap, diff, diff+i) if err != nil { b.Fatal(err) } @@ -77,7 +77,7 @@ func BenchmarkDetectSpan(b *testing.B) { if err != nil { b.Fatal(err) } - _, _, _, err = detectSlashingByEpochSpan(ctx, i, i+diff, spanMap, detectMax) + _, _, _, err = detectSlashingByEpochSpan(ctx, spanMap, i, i+diff, detectMax) if err != nil { b.Fatal(err) } @@ -91,7 +91,7 @@ func BenchmarkDetectSpan(b *testing.B) { if err != nil { b.Fatal(err) } - _, _, _, err = detectSlashingByEpochSpan(ctx, i, i+diff, spanMap, detectMin) + _, _, _, err = detectSlashingByEpochSpan(ctx, spanMap, i, i+diff, detectMin) if err != nil { b.Fatal(err) } diff --git a/slasher/detection/attestations/attestation_test.go b/slasher/detection/attestations/attestations_test.go similarity index 95% rename from slasher/detection/attestations/attestation_test.go rename to slasher/detection/attestations/attestations_test.go index 79a09faeac..f36c477ddc 100644 --- a/slasher/detection/attestations/attestation_test.go +++ b/slasher/detection/attestations/attestations_test.go @@ -206,7 +206,7 @@ func TestServer_UpdateMaxEpochSpan(t *testing.T) { if err != nil { t.Fatal(err) } - st, spanMap, err := detectAndUpdateMaxEpochSpan(ctx, tt.sourceEpoch, tt.targetEpoch, spanMap) + spanMap, st, err := detectAndUpdateMaxEpochSpan(ctx, spanMap, tt.sourceEpoch, tt.targetEpoch) if err != nil { t.Fatalf("Failed to update span: %v", err) } @@ -239,7 +239,7 @@ func TestServer_UpdateMinEpochSpan(t *testing.T) { if err != nil { t.Fatal(err) } - st, spanMap, err := detectAndUpdateMinEpochSpan(ctx, tt.sourceEpoch, tt.targetEpoch, spanMap) + spanMap, st, err := detectAndUpdateMinEpochSpan(ctx, spanMap, tt.sourceEpoch, tt.targetEpoch) if err != nil { t.Fatalf("Failed to update span: %v", err) } @@ -282,10 +282,10 @@ func TestServer_FailToUpdate(t *testing.T) { if err != nil { t.Fatal(err) } - if _, _, err := detectAndUpdateMinEpochSpan(ctx, spanTestsFail.sourceEpoch, spanTestsFail.targetEpoch, spanMap); err == nil { + if _, _, err := detectAndUpdateMinEpochSpan(ctx, spanMap, spanTestsFail.sourceEpoch, spanTestsFail.targetEpoch); err == nil { t.Fatalf("Update should not support diff greater then weak subjectivity period: %v ", params.BeaconConfig().WeakSubjectivityPeriod) } - if _, _, err := detectAndUpdateMaxEpochSpan(ctx, spanTestsFail.sourceEpoch, spanTestsFail.targetEpoch, spanMap); err == nil { + if _, _, err := detectAndUpdateMaxEpochSpan(ctx, spanMap, spanTestsFail.sourceEpoch, spanTestsFail.targetEpoch); err == nil { t.Fatalf("Update should not support diff greater then weak subjectivity period: %v ", params.BeaconConfig().WeakSubjectivityPeriod) } diff --git a/slasher/rpc/server.go b/slasher/rpc/server.go index 7738386805..76604339c8 100644 --- a/slasher/rpc/server.go +++ b/slasher/rpc/server.go @@ -111,7 +111,7 @@ func (ss *Server) UpdateSpanMaps(ctx context.Context, req *ethpb.IndexedAttestat wg.Done() return } - spanMap, _, _, err = attestations.DetectAndUpdateSpans(ctx, req, spanMap) + spanMap, _, err = attestations.DetectAndUpdateSpans(ctx, spanMap, req) if err != nil { er <- err wg.Done() @@ -188,7 +188,7 @@ func (ss *Server) DetectSurroundVotes(ctx context.Context, validatorIdx uint64, if err != nil { return nil, errors.Wrap(err, "failed to get validator spans map") } - spanMap, minTargetEpoch, maxTargetEpoch, err := attestations.DetectAndUpdateSpans(ctx, req, spanMap) + spanMap, slashableEpoch, err := attestations.DetectAndUpdateSpans(ctx, spanMap, req) if err != nil { return nil, errors.Wrap(err, "failed to update spans") } @@ -197,30 +197,18 @@ func (ss *Server) DetectSurroundVotes(ctx context.Context, validatorIdx uint64, } var as []*ethpb.AttesterSlashing - if minTargetEpoch > 0 { - attestations, err := ss.SlasherDB.IdxAttsForTargetFromID(ctx, minTargetEpoch, validatorIdx) + if slashableEpoch > 0 { + atts, err := ss.SlasherDB.IdxAttsForTargetFromID(ctx, slashableEpoch, validatorIdx) if err != nil { return nil, err } - for _, ia := range attestations { + for _, ia := range atts { if ia.Data == nil { continue } - if ia.Data.Source.Epoch > req.Data.Source.Epoch && ia.Data.Target.Epoch < req.Data.Target.Epoch { - as = append(as, ðpb.AttesterSlashing{ - Attestation_1: req, - Attestation_2: ia, - }) - } - } - } - if maxTargetEpoch > 0 { - attestations, err := ss.SlasherDB.IdxAttsForTargetFromID(ctx, maxTargetEpoch, validatorIdx) - if err != nil { - return nil, err - } - for _, ia := range attestations { - if ia.Data.Source.Epoch < req.Data.Source.Epoch && ia.Data.Target.Epoch > req.Data.Target.Epoch { + surrounding := ia.Data.Source.Epoch < req.Data.Source.Epoch && ia.Data.Target.Epoch > req.Data.Target.Epoch + surrounded := ia.Data.Source.Epoch > req.Data.Source.Epoch && ia.Data.Target.Epoch < req.Data.Target.Epoch + if surrounding || surrounded { as = append(as, ðpb.AttesterSlashing{ Attestation_1: req, Attestation_2: ia,