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

Use Custom SSZ for P2P Types #7436

Merged
merged 30 commits into from
Oct 14, 2020
Merged

Use Custom SSZ for P2P Types #7436

merged 30 commits into from
Oct 14, 2020

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Oct 5, 2020

What type of PR is this?

Feature Addition And Bug Fix

What does this PR do? Why is it needed?

  • This adds ssz support for non composite types. Previously we used go-ssz for non composite type which
    led our node to be susceptible to any go-ssz specific bugs. This PR instead implements custom ssz for our
    required p2p types, so that they do not need to use go-ssz anymore
  • Add in relevant tests.
  • Fix usage of this across the repo.

Which issues(s) does this PR fix?

Fixes #7425

Other notes for review

@nisdas nisdas added Ready For Review A pull request ready for code review Networking P2P related items and removed Ready For Review A pull request ready for code review labels Oct 6, 2020
@nisdas nisdas marked this pull request as ready for review October 6, 2020 16:05
@nisdas nisdas requested a review from a team as a code owner October 6, 2020 16:05
@@ -26,8 +26,6 @@ ssz_gen_marshal(
],
objs = [
"BeaconBlocksByRangeRequest",
"BeaconBlocksByRootRequest",
Copy link
Member

Choose a reason for hiding this comment

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

Delete BeaconBlocksByRootRequest in messages.proto?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its been deleted form before, this is just cleanup

name = "go_default_library",
srcs = ["types.go"],
importpath = "github.com/prysmaticlabs/prysm/beacon-chain/p2p/types",
visibility = ["//visibility:public"],
Copy link
Member

Choose a reason for hiding this comment

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

Should the visibility be public here?

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch ! that is why the CI was complaining lol

prestonvanloon
prestonvanloon previously approved these changes Oct 14, 2020
@prestonvanloon
Copy link
Member

Conflicts 😭 PTAL @nisdas

prestonvanloon
prestonvanloon previously approved these changes Oct 14, 2020
prestonvanloon
prestonvanloon previously approved these changes Oct 14, 2020
@prylabs-bulldozer prylabs-bulldozer bot merged commit 022b666 into master Oct 14, 2020
@delete-merged-branch delete-merged-branch bot deleted the useCustomSSZ branch October 14, 2020 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Networking P2P related items Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Req-resp Error responses are decoded incorrectly.
3 participants