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

[WIP] ETH2.0 Wire protocol #195

Closed
wants to merge 19 commits into from
Closed

Conversation

Mikerah
Copy link
Contributor

@Mikerah Mikerah commented May 7, 2019

I have started an initial implementation of the ETH2.0 Wire protocol.

@GregTheGreek
Copy link
Member

Looks like the linter failed

src/rpc/api/wire/wire.ts Outdated Show resolved Hide resolved
src/p2p/index.ts Outdated
} catch (err) {
this.log.error(err);
}
});

this.node.on('peer:disconnect', (peerInfo) => {
try {
try {

Copy link
Member

Choose a reason for hiding this comment

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

white space, also not sure if this needs a try/catch theres no async functionality happening

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pull-stream is there because that's what used in the example I was given. I will be changing this to an async iterator. Also, you can still use try/catch whether or not there's async functionality. Is there a preferred way for catching errors that would be better in this case?

Copy link
Member

Choose a reason for hiding this comment

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

The way I look at It, those functions should return a promise or callback if they have an error.

I'm on mobile but I'll check the api later

src/p2p/index.ts Outdated Show resolved Hide resolved
src/p2p/index.ts Outdated
@@ -89,18 +95,45 @@ export class P2PNetwork extends EventEmitter implements Service {

});

// 2-way handshake
const protocol: string = "/eth/serenity/beacon/rpc/1";
Copy link
Member

Choose a reason for hiding this comment

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

probably put this into constants file

src/p2p/index.ts Outdated Show resolved Hide resolved
src/p2p/wire/messages.ts Outdated Show resolved Hide resolved
src/p2p/wire/messages.ts Outdated Show resolved Hide resolved
src/rpc/api/wire/messages.ts Show resolved Hide resolved
slot: Slot;
}

export interface HashTreeRoot {
Copy link
Member

Choose a reason for hiding this comment

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

I think we could make a Root type in src/types/primitive.ts that would be useful across the codebase, used in the wire messages.

It would look like:

export type Root = bytes32;

and since we would be using it in ssz-ed types, we'd need a export const Root = bytes32; too.

Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

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

It seems fine, it will probably be easier to merge this and improve parts with iteratitions :)

@Mikerah
Copy link
Contributor Author

Mikerah commented May 21, 2019

@mpetrunic I'm not done yet. I'm still trying to setup the code to make it easier to refactor so that implementing the sync module is easier.

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

We don't want to be using protobuf. We should just be using ssz.

@Mikerah
Copy link
Contributor Author

Mikerah commented May 23, 2019

@wemeetagain we can easily interface between the two. The main issue with ssz is that it's not fit to for messages sent over the wire like something like RLP or Protobufs are. That's why other clients are adopting the dual Protobuf-ssz scheme I'm attempting to go with

@wemeetagain
Copy link
Member

Could you elaborate on ssz not being fit for messages over the wire?

The networking rpc spec mentions that data is serialized using ssz, which makes it seem it is fit for messages over the wire.

I/m under the impression that the reason other clients use protobuf is because they rely on gRPC which requires the use of protobufs.

@Mikerah
Copy link
Contributor Author

Mikerah commented May 23, 2019

I think that spec is going to be changed soon. The reason for using ssz on the wire is simplicity. There were some discussions a while back about using RLP instead. This issue from a while ago sums it up. There's also a google doc floating around somewhere that goes into details. I think that doc is probably just the current discv5 spec, though.

@wemeetagain
Copy link
Member

So why are we implementing a protobuf wire protocol?
It seems from the linked issue that consensus landed on using some minimal custom wire layout, with the interior message being encoded using rlp or ssz.

@GregTheGreek
Copy link
Member

I'm going to have to agree with @wemeetagain I don't see a compelling argument for including protobuf.

Minimalistic is always better imo.

@Mikerah
Copy link
Contributor Author

Mikerah commented May 27, 2019

Closing in favor of waiting until ETH2.0 Wire Protocol is more certain.

@Mikerah Mikerah closed this May 27, 2019
@GregTheGreek GregTheGreek deleted the mikerah/eth2-wire-protocol branch September 11, 2019 02:49
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.

4 participants