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

Verify Slashing Signatures Before Putting Into Blocks #5071

Merged
merged 27 commits into from Mar 12, 2020

Conversation

rauljordan
Copy link
Contributor

@rauljordan rauljordan commented Mar 11, 2020

No tracking issue.


Description

Write why you are making the changes in this pull request

Currently, our slashing pool does not verify signatures, causing some bad slashings to be attempted to put into blocks, causing issues. Bad slashings are not cleared from pool today.

Write a summary of the changes you are making

This PR ensures we verify the attester/proposer slashing before giving it to a proposer. If it fails for any reason, we clear it from the pool.

Link anything that would be helpful or relevant to the reviewers

This PR fixes old tests and adds regression tests.

@prestonvanloon prestonvanloon added Priority: Critical Highest, immediate priority item Bug Something isn't working labels Mar 11, 2020
@codecov
Copy link

codecov bot commented Mar 11, 2020

Codecov Report

Merging #5071 into master will decrease coverage by 1.27%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master    #5071      +/-   ##
==========================================
- Coverage   44.79%   43.52%   -1.28%     
==========================================
  Files         216      215       -1     
  Lines       16811    16749      -62     
==========================================
- Hits         7531     7290     -241     
- Misses       7982     8195     +213     
+ Partials     1298     1264      -34

@rauljordan rauljordan marked this pull request as ready for review March 11, 2020 19:32
@rauljordan rauljordan added the Ready For Review A pull request ready for code review label Mar 11, 2020
"errors"
"fmt"
"sort"

ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
"go.opencensus.io/trace"

Copy link
Member

Choose a reason for hiding this comment

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

Please remove space

shayzluf
shayzluf previously approved these changes Mar 11, 2020
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.

Maybe we should move verify to the rpc endpoint?

@rauljordan rauljordan changed the title Verify Slashing Signatures Before Putting Into Blocks WIP: Verify Slashing Signatures Before Putting Into Blocks Mar 11, 2020
@rauljordan rauljordan changed the title WIP: Verify Slashing Signatures Before Putting Into Blocks Verify Slashing Signatures Before Putting Into Blocks Mar 11, 2020
prestonvanloon
prestonvanloon previously approved these changes Mar 11, 2020
0xKiwi
0xKiwi previously approved these changes Mar 11, 2020
defer span.End()

if err := blocks.VerifyProposerSlashing(state, slashing); err != nil {
numPendingAttesterSlashingFailedSigVerify.Inc()
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call with the metrics

@rauljordan rauljordan dismissed stale reviews from 0xKiwi and prestonvanloon via 87dbb4d March 12, 2020 00:28
@prylabs-bulldozer prylabs-bulldozer bot merged commit f1a42eb into master Mar 12, 2020
@delete-merged-branch delete-merged-branch bot deleted the verify-slash-sig branch March 12, 2020 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Priority: Critical Highest, immediate priority item Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants