search

Consensys / protocol-BountiedWork

Repository to track bounties across all of PegaSys's repositories
Apache License 2.0
10 stars 7 forks source link

Rust Hobbits implementation #10

Closed jrhea closed 5 years ago

jrhea commented 5 years ago

Hobbits is a network protocol for distributed systems meant for simple interactions between peers. Hobbits is used to explore and test interactions between Ethereum 2.0 clients as they firm up network requirements.

This bounty will be awarded to the team or individual implementing hobbits as defined by the specification of the wire protocol and the specification of the gossip protocol in Rust language.

Acceptance criteria:

Additional Info:

Hobbits specifications: https://github.com/deltap2p/hobbits Reference Implementation: https://github.com/apache/incubator-tuweni/tree/master/hobbits

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 2.0 ETH (487.57 USD @ $243.78/ETH) attached to it as part of the @PegaSysEng fund.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Workers have applied to start work.

These users each claimed they can complete the work by 2 months, 4 weeks from now. Please review their action plans below:

1) nanspro has applied to start work _(Funders only: approve worker | reject worker)_.

I want to code in rust since i have been learning it from last week and this issue looks interesting. The company where i am working as an intern plans to use pority-bridge to connect two quorum chains so i am learning rust now to modify the bridge according to our needs and i think i can work on this along the way to improve my understanding of using rust as an systems prog language.

Learn more on the Gitcoin Issue Details page.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 1 month, 2 weeks from now. Please review their action plans below:

1) kevinkelley has been approved to start work.

Interesting... I'd like to work on this. I'm good in Kotlin, where I see the reference implementation. And I'm good in Rust, I've been doing Rust stuff for a few years when I can, maintaining a vector-graphics binding (nanovg-rs) among other things.

I've got a lot of background in wire protocols. I'm a little nervous about this bounty platform... but I'd like to get involved.

Learn more on the Gitcoin Issue Details page.

AgeManning commented 5 years ago

I think this implementation should be built using the Tokio rust library.

The implementation should behave like a tokio Stream which when polled returns hobbits events. This will allow users to asynchronously receive messages from the Hobbits protocol.

jrhea commented 5 years ago

@AgeManning i'm not seeing a way to add u as an admin. I am going to poke the GitCoin guys tomorrow and see what we can do.

AgeManning commented 5 years ago

A good example of implementing a service that opens a port, decodes packets and sends them upwards using tokio is mdns in libp2p: https://github.com/libp2p/rust-libp2p/blob/master/misc/mdns/src/service.rs

This example uses a UDP socket, whereas hobbits would require a TCP socket.

KevinKelley commented 5 years ago

@AgeManning I'm not seeing where you get "events". This seems to be packet based, there's a header and a payload... are you talking about streaming thru the payload, parsing and generating events from it?

Or do you just mean, at a higher level, to provide a stream, that can be iterated thru or waited on, for packets to come in.

TCP is basically send/receive... polling on that is kind of a hack. And I don't know if it fits with hobbits... which seems to be, send a message, get a response.

Anyway... more tomorrow. Maybe I'm missing something obvious

AgeManning commented 5 years ago

@KevinKelley - I imagine this to be built as a "service" which can be run asynchronously. This is effectively building a service that behaves like a Stream that provides decoded events and can be waited on, as you say in the second paragraph.

The poll() function which provides the output of the Stream, also drives the service, as it processes (decodes and sends) any messages that need to be sent. Thus this service, at a high level, looks like a Stream which can be driven by the poll() function.

Let me know if this doesn't make sense

KevinKelley commented 5 years ago

@AgeManning makes sense, thanks

jrhea commented 5 years ago

@AgeManning & @KevinKelley here is a link to a Hobbits discourse topic. https://discourse.deltap2p.com/t/hobbits-bounties/54

AgeManning commented 5 years ago

@KevinKelley - You can find me on gitter or here, if you have any questions/concerns :)

KevinKelley commented 5 years ago

@AgeManning +1 thanks :)

jrhea commented 5 years ago

@KevinKelley fyi, i will also be running this conformance test against your implementation:

https://github.com/deltap2p/hobbits/tree/master/test/conformance

Can you include a sample app with your implementation so I can run it? Here is an example one that was created for the go implementation:

https://github.com/renaynay/go-hobbits/blob/master/demo/sample.go

Thanks!

KevinKelley commented 5 years ago

@jrhea Yes, no problem.

KevinKelley commented 5 years ago

Work in progress; project layout is set up; am working to implement everything now.

repo will be at https://github.com/KevinKelley/hobbits-rs ...at the moment it's nowhere near worth looking at unless you just like to see something's happening.

No issues yet to ask about. One question; the docs call out only TCP; is there likely a need for different transport mechanism? Wondering how much to modularize

jrhea commented 5 years ago

One question; the docs call out only TCP; is there likely a need for different transport mechanism? Wondering how much to modularize

Modular is good, but not a hard requirement. Follow your conscience 😁. FWIW, there is potential for other transports being tested in the future, but we are sticking with TCP for now.

gitcoinbot commented 5 years ago

@kevinkelley Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

jrhea commented 5 years ago

@KevinKelley how's the implementation going?

KevinKelley commented 5 years ago

@jrhea sorry, I got swamped last week, and didn't get much farther. Positive note, this week is open, I'm working on it now, and I expect to have something to look at over the next couple days...

KevinKelley commented 5 years ago

progress report... marshal and unmarshal are working now, with tests. Still finishing up more tests, and need to do "proper" idiomatic error handling; then on the the networking. More later.

jrhea commented 5 years ago

fyi, we are incorporating some feedback that was received into a more coherent/organized spec. here is a link to the PR https://github.com/deltap2p/hobbits/pull/15 feel free to comment or open issues/PRs

KevinKelley commented 5 years ago

I'm working now on a tokio::codec for this; what I have already works fine for single-message tests. Where the remote opens a tcp connection, sends a EWP message, and waits a response. But to handle a stream of messages, simplest thing is to implement codec, use tokio futures to keep it async and nonblocking. Check the envelope header first, to get payload size; if enough bytes are available on the stream to complete, then parse and handle the message; otherwise poll.

@jrhea Is the conformance-test app considered complete? Is that still work-in-progress?

gitcoinbot commented 5 years ago

@kevinkelley Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

KevinKelley commented 5 years ago

status update... conformance test passing. I'm still cleaning/documenting, refactoring for the "best practices" requirement, making it look like it should.

@jrhea are there more tests/requirements I should know about?

kevin@leopard:~/go/src/github.com/deltap2p/hobbits/test/conformance$ sudo ./conformance --port 12345
[sudo] password for kevin: 
2019/07/04 08:29:19 Starting Ping Test....
2019/07/04 08:29:19 IP is valid. Pinging...
2019/07/04 08:29:19 PING localhost (127.0.0.1):
2019/07/04 08:29:19 24 bytes from 127.0.0.1: icmp_seq=0 time=172.744µs
2019/07/04 08:29:20 24 bytes from 127.0.0.1: icmp_seq=1 time=137.395µs
2019/07/04 08:29:21 24 bytes from 127.0.0.1: icmp_seq=2 time=128.07µs
2019/07/04 08:29:22 24 bytes from 127.0.0.1: icmp_seq=3 time=150.978µs
2019/07/04 08:29:23 24 bytes from 127.0.0.1: icmp_seq=4 time=154.668µs
2019/07/04 08:29:23 
--- localhost ping statistics ---
2019/07/04 08:29:23 5 packets transmitted, 5 packets received, 0% packet loss
2019/07/04 08:29:23 round-trip min/avg/max/stddev = 128.07µs/148.771µs/172.744µs/15.312µs
Ping test passed. Testing port...
2019/07/04 08:29:23 TCP Port "12345" is available
Port test passed. Testing message delivery...
Message delivery passed.
gitcoinbot commented 5 years ago

@kevinkelley Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

jrhea commented 5 years ago

@KevinKelley sorry for the delay. Had some health issues last week. Sounds like you are close (if not already done). Let me get back to you on the tests.

atoulme commented 5 years ago

Hello Kevin,

I took a first look at the repo:

KevinKelley commented 5 years ago

@jrhea okay.

@atoulme yes... noted. I'll finish up cleanup and documenting, have a reviewable version ready tomorrow

gitcoinbot commented 5 years ago

@kevinkelley Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

KevinKelley commented 5 years ago

@atoulme Hey... hi. Sorry so slow; I keep thinking of new things to try, instead of just cleaning up what already works. Anyway, I have now pushed a clean, updated version to KevinKelley/hobbits-rs I'll appreciate a review and any suggestions, if/when you have time.

@jrhea Anything new on the conformance front? Reason I brought it up, is that the conformance test doesn't seem too extensive, and I can think of several things that seem likely to need to be added.

Anyway... I feel like the version here as it stands, is reasonable according to requirements. Let me know, anyone, with comments and suggestions

AgeManning commented 5 years ago

Hey! I've had a quick skim through this. Nice work! I have a few small comments which may be helpful.

Ideally, we would like to return errors where possible rather than panic-ing. I notice there are a few cases where the unwrap() could be replaced for improved readability. For example: https://github.com/KevinKelley/hobbits-rs/blob/master/src/encoding/unmarshal.rs#L7-L8 Here the option could be mapped to an error. For example:

let index = index.ok_or( EWPError::new(...) )?;

the ok_or() could be employed on line 6.

Similarly: https://github.com/KevinKelley/hobbits-rs/blob/master/src/encoding/unmarshal.rs#L27-L31 Could be 

let msg_hdr_len = hdr_parts[3].parse().map_err(|_| EWPError::new(...))?;

There are a few other areas in this file where this style could be applied.

I had a quick look at the hobbits spec (I'm not all that familiar with it, but I think it mentioned a version of 3). Perhaps we include it here: https://github.com/KevinKelley/hobbits-rs/blob/master/src/encoding/unmarshal.rs#L27 or allow it as an input when creating a new Envelope.

https://github.com/KevinKelley/hobbits-rs/blob/master/src/encoding/codec.rs#L58-L67 I think in our use cases the entire envelope would be sent. I think other implementations are not dealing with partial sends. I would imagine the best approach would be to unmarshal the header. If that errors, then return an error. If it succeeds, then we know how many bytes need to be read. Once those bytes are received, we can either unmarshal the body, or error. I'm not sure we need the sophistication yet however.

If we assume entire packets are sent, then if we fail to unmarshal the entire packet I think we should return an error rather than None. If we cannot unmarshal we shouldn't hope more bytes are coming, rather, assume the encoding is invalid.

Similarly: https://github.com/KevinKelley/hobbits-rs/blob/master/src/encoding/codec.rs#L74 If we are not dealing with partial packet transfer, I think it would probably be best to Error if a packet doesn't contain a new-line delimiter, rather than wait for more bytes and potentially corrupt future packets.

In principle the whole decode could be simply something like:

let msg = unmarshal(buf).map_err(std::io::Error::new(...))?;
// success, we got a whole Envelope.
 let bytes_used = offset + 1 + msg.header.len() + msg.body.len();
 // Cut out the used bytes from the buffer so we don't return it again.
 let _ = buf.split_to(bytes_used);
Some(msg)

Typically if decode() handles things correctly we shouldn't need to implement decode_eof(), tokio should handle it for us.

Hopefully these points are somewhat helpful :)

KevinKelley commented 5 years ago

Hopefully these points are somewhat helpful :)

@AgeManning Thank you! Yes, very helpful :) You answered a couple questions that I had.

re: panic() ...yes, for sure. I must have overlooked; will review

re: returning EwpError() instead of None, on partial input... That's definitely easier, and makes sense as you say; in this context, partial is most likely to be a bad envelope, I'd guess.

re: Hobbits spec version: I'm not clear on the intention of that field in the Envelope. Is it meant to be locked to the Hobbits spec version? Or, could it be arbitrary, or tied to the spec of the protocol being carried (GOSSIP|RPC|PING)? Likely you're correct, it's meant to be the EWP spec level, since that's where the envelope format is defined...

Will make changes and repost soon.

AgeManning commented 5 years ago

I'm not sure about the version either. Perhaps it's a better question for @atoulme

atoulme commented 5 years ago

Yes, this specifies the version of the Hobbits spec version, which versions all the protocols under it.

KevinKelley commented 5 years ago

I've made these suggested changes; ready to review. (repo is KevinKelley/hobbits-rs )

About the version field: I'm using some tests that I found in another implementation, which allow for any version number as long as it matches the (digit+)(.)(digit*) regex. So I fixed by adding a check for that format of version-number; rather than the brittle if version != "0.2"

Comments welcome :)

jrhea commented 5 years ago

@KevinKelley I will review tomorrow. Thanks! Also, @atoulme and @AgeManning u guys might wanna look as well. Thanks everyone

atoulme commented 5 years ago

LGTM. One small thing: there is no PONG protocol so you can remove protocol != "PONG" in your code. Otherwise let's get this in. Good work Kevin!

KevinKelley commented 5 years ago

Got it... and done . Thanks!

jrhea commented 5 years ago

@KevinKelley thanks for sticking with this. i will approve as soon as you submit.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 2.0 ETH (437.52 USD @ $218.76/ETH) has been submitted by:

  1. @kevinkelley

@jrhea please take a look at the submitted work:

KevinKelley commented 5 years ago

@jrhea Submitted on gitcoin. This was an interesting project! I'd love to do more with it.

Thanks to all..

AgeManning commented 5 years ago

@jrhea @atoulme - Just wanted to confirm that the hobbit spec uses the '\n' to delimitate headers from message data. This doesn't seem all that logical as hashes are likely sent which I imagine will often contain the byte equivalent of \n.

jrhea commented 5 years ago

@AgeManning I believe an earlier version used \n but v3 doesn't. There were some upgrades made to the spec as we began trying to Interop with prysmatic. @atoulme and I felt bad about it and didn't want to ask @KevinKelley to do it since he had already begun work. We will bite the bullet and update it to conform.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

The funding of 2.0 ETH (452.9 USD @ $226.45/ETH) attached to this issue has been approved & issued to @KevinKelley.

KevinKelley commented 5 years ago

(sorry to spam an old thread...)

@jrhea (or anyone) is there any ongoing activity here? I'm having some free time and want to keep on with some ideas related to this project, but far as I can tell they've stopped updating since July or so.

Where's a good active project similar to this, Hobbits and p2p and Ethereum (and Rust) that maybe I could get involved in?