Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change span representation to a struct in Slasher #4981

Merged
merged 8 commits into from Mar 3, 2020

Conversation

0xKiwi
Copy link
Contributor

@0xKiwi 0xKiwi commented Mar 2, 2020

Part of #4836

This PR changes the [3]uint16 of the spanner to a

type Span struct {
	MinSpan     uint16
	MaxSpan     uint16
	SigBytes    [2]byte
	HasAttested bool
}

Changes include making a distinct field for the bytes to be stored, and changing the bloom filter to a [2]byte, that can later be used to append to the target epoch for searching the DB for the slashable attestation.

By storing part of the DB key in the spanner, we can retrieve the attestation we need to find faster. Will plugin to detection through another PR.

@@ -23,7 +19,7 @@ var _ = iface.SpanDetector(&SpanDetector{})
// spans from validators and attestation data roots.
type SpanDetector struct {
// Slice of epochs for valindex => min-max span + double vote filter
spans []map[uint64][3]uint16
spans []map[uint64]types.Span
Copy link
Contributor Author

@0xKiwi 0xKiwi Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not using a reference since the actual struct nearly the same size as the pointer would be, just an extra bool. There are also no nested/dynamic types. Size is static.
If this should be changed let me know.

@0xKiwi 0xKiwi requested review from nisdas, har00ga and terencechain and removed request for har00ga March 2, 2020 20:59
@0xKiwi 0xKiwi self-assigned this Mar 2, 2020
@0xKiwi 0xKiwi requested a review from shayzluf March 2, 2020 22:41
Copy link
Contributor

@shayzluf shayzluf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review .didn't look at tests yet. Will look at it all in few hours

}, nil
}
}

if sp := s.spans[targetEpoch%numSpans]; sp != nil {
filterNum := sp[validatorIdx][2]
if filterNum == 0 {
// Check if the validator has attested for this epoch or not.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to check if it's the same dataroot in order to decide if its double. Doing it in another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that will be handled by detection. This just makes sure the attestation triggers a hard DB check.

}
filterNum = binary.LittleEndian.Uint16(attFilter)

// Set the bloom filter back into the span for the epoch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a bloom filter any more

@0xKiwi 0xKiwi requested a review from shayzluf March 3, 2020 04:43
@codecov
Copy link

codecov bot commented Mar 3, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@7e8fd0e). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master   #4981   +/-   ##
========================================
  Coverage          ?   3.99%           
========================================
  Files             ?      71           
  Lines             ?    6258           
  Branches          ?       0           
========================================
  Hits              ?     250           
  Misses            ?    5988           
  Partials          ?      20

SlashableEpoch: targetEpoch,
}, nil
}
return &types.DetectionResult{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems strange to return double vote when it is not verified

Copy link
Contributor Author

@0xKiwi 0xKiwi Mar 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just for the detection, it lets the detection service to do a hard check, which we always need to do anyways for checking double votes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using 2 bytes of the signature isn't safe enough to just compare the receiving attestation with it IMO, db checks are really quick with this strategy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats true

Copy link
Contributor

@shayzluf shayzluf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@shayzluf shayzluf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please change to reference

@0xKiwi 0xKiwi requested a review from shayzluf March 3, 2020 07:44
Copy link
Contributor

@shayzluf shayzluf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
lets merge it and i will use this struct in my spanner db

@shayzluf shayzluf merged commit 703ce63 into prysmaticlabs:master Mar 3, 2020
@0xKiwi 0xKiwi deleted the slasher-change-filter branch March 15, 2020 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants