Prepare Slasher for Production #5020
Conversation
| @@ -177,6 +187,9 @@ func (s *SpanDetector) updateMinSpan(ctx context.Context, source uint64, target | |||
| } else { | |||
| break | |||
| } | |||
| if epoch == 0 { | |||
|
We should be careful not to merge it IMO . We can miss detecting an attack on ths network if old epoch spans are not populated |
Codecov Report
@@ Coverage Diff @@
## master #5020 +/- ##
===========================================
- Coverage 42.99% 14.93% -28.07%
===========================================
Files 267 97 -170
Lines 20111 7407 -12704
===========================================
- Hits 8647 1106 -7541
+ Misses 9987 6171 -3816
+ Partials 1477 130 -1347 |
rauljordan
changed the title
@shayzluf what's the alternative here? Make the number a configurable flag with a default of 128? I feel like this is a parameter that depends on how comfortable you are with the resource requirements of your slasher instance, but making it way too big will make it prohibitively slow to run slasher at all. |
| return err | ||
| } | ||
| } else { | ||
| break |
There was a problem hiding this comment.
I made this change because I thought it could cause an issue and not properly update spans, wdyt?
slasher/detection/detect.go
Outdated
| @@ -31,10 +35,12 @@ func (ds *Service) detectAttesterSlashings( | |||
| switch result.Kind { | |||
| case types.DoubleVote: | |||
| slashing, err = ds.detectDoubleVote(ctx, att, result) | |||
| logrus.Debugf("Detected a possible double vote for val idx %d", valIdx) | |||
There was a problem hiding this comment.
Are we gonna keep this logs? Maybe we should remove them since we know detection works now.
slasher/detection/listeners.go
Outdated
| slashings, err := ds.detectAttesterSlashings(ctx, indexedAtt) | ||
| if err != nil { | ||
| log.WithError(err).Error("Could not detect attester slashings") | ||
| continue | ||
| } | ||
| if len(slashings) < 1 { | ||
| log.Debug("Updating spans for incoming attestation") |
There was a problem hiding this comment.
These logs are gonna be really spammy, maybe we should just log out the updating spans log?
…cleanup-slasher
Part of #4836
Description
Write why you are making the changes in this pull request
We have now confirmed slasher functions in production, albeit it struggles a lot with intense processing of incoming attestations. This PR cleans up aspects of slasher to ensure we have naming consistency, good tracing, and general tidyness.