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
Fix Gossip Message ID #7624
Fix Gossip Message ID #7624
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7624 +/- ##
=======================================
Coverage 61.87% 61.87%
=======================================
Files 422 422
Lines 29723 29723
=======================================
Hits 18390 18390
Misses 8418 8418
Partials 2915 2915 |
h := hashutil.Hash(pmsg.Data) | ||
return string(h[:]) | ||
decodedData, err := snappy.Decode(nil /*dst*/, pmsg.Data) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err != nil
feels a bit generic. On a safer side, do you know what specific errors are ok and are not ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaik, all errors returned from it should signify it is an invalid snappy object.
https://github.com/golang/snappy/blob/master/decode.go#L57
None of the errors there are 'ok', all point to a corrupted/invalid snappy object as of the spec. If it cant be decoded
it should be appended with the invalid domain and then hashed.
What type of PR is this?
Feature Addition
What does this PR do? Why is it needed?
Which issues(s) does this PR fix?
Follows on from ethereum/consensus-specs#2089
Other notes for review