search

ethereumjs / ethereumjs-devp2p

Project is in active development and has been moved to the EthereumJS VM monorepo.
https://github.com/ethereumjs/ethereumjs-vm/tree/master/packages/devp2p
141 stars 62 forks source link

Node discovery v5 #19

Closed holgerd77 closed 5 years ago

holgerd77 commented 6 years ago

Introduction

Node discovery v5 is on it's way, which should make it a lot easier to discover suitable nodes. Though not standardized yet, ~it would be good to have some early experimentation/draft implementation to get closer to a feeling how the protocol (could) behave~ it would be good to have this library updated with a working implementation to support the practical usage in terms of a light client implementation.

Here are some draft documents on this. Also valuable in this regard: Talk from Felix at Devcon3.

Current py-evm implementation work: https://github.com/ethereum/py-evm/pull/209

Current Situation

Currently the library supports Node discovery v4 implemented in src/dpt, where kbucket.js manages the list of peers in a Kademlia-like DHT, message.js implements the different message types like ping, pong or neighbours to discover new peers and server.js sets up a server to handle the discovery communication.

There are static tests for the message types in ./test/dpt-message.js, the integrated communication process is tested in ./test/integration/dpt-simulator.js as well as the corresponding eth and les files.

With the two peer-communication example scripts in the examples folder both ETH and LES (so full- and light client wire) communication can be tested in a real-world scenario by connecting to an actual network of in-production clients.

Task Description

Node discovery v5 updated the structure of Node records, brings changes to the communication flow by adding new packet types and introduces the concept of topics for exchanging on the capability of peers.

A first step into this task would be to work out some concept/idea where these additional/changed components fit into the existing v4 implementation and where current files/classes can be extended and where additional structures have to be set up. Since network communication implementation is very error-prone it will probably ease implementation to early on think along how to extend existing tests and add new tests for added functionality.

Goals

Though being still marked as "experimental" Geth has a working v5 discovery implementation for some time. Sufficient requirement for this issue to be completed is a working peer-communication example where it is possible to set the discovery to v5 - e.g. by CL parameter or example internal constant - and get a working LES and/or (optimally both) ETH connection (working means here: e.g. LES packets are actually passed through) in a reasonable amount of time (repeatedly < 5 min until first connection occurs).

Necessary side goals are:

Required Skills

Everyone is invited to join and take on this task. Since this an advanced task requiring deeper knowledge of the Ethereum networking stack it is probably more likely that you succeed if you bring along skills/experience in the following fields:

Guidance

For questions helping to understand the existing code base and the scope of this issue you will get active guidance by the creator of this issue (@holgerd77, EthereumJS team).

holgerd77 commented 6 years ago

Sounds good.

timxor commented 6 years ago

Update coming tonight, hoping to have it finished today/tomorrow =)

timxor commented 6 years ago

Yesterday I gave a go at the discv5-enr per Felix's suggestion. You can view it here. Upon further reflection this morning it looks like I didn't notice the discv4-enr-extension bridge. So I will work on that today. Though I think I may put it on a new branch because it looks like it just adds two new packets ENRRequest Packet (0x05) and ENRResponse Packet (0x06) that do not interfere with the v4 packet namespace. cc: @holgerd77 @vs77bb @ceresstation

vs77bb commented 6 years ago

Thanks again for the detailed updates here, @tcsiwula!

holgerd77 commented 6 years ago

Hi @tcsiwula, just for information: I was somewhat unresponsive/unavailable in the last weeks due to family reasons, this is getting better now, if you need anything or are waiting for some answers let me know.

spm32 commented 5 years ago

Hey @tcsiwula what's the latest update here? Anything @holgerd77 or our team can to help?

holgerd77 commented 5 years ago

Yes, I would be interested in that as well! 😄 🐨 I think you have very well done enough overall work for having deserved the bounty. We nevertheless have to find some consistent cut so that we can include the work you have done into the library.

So one first-thought idea would be, that we include the structural parts, mark all this as Experimental and leave all the remaining testing work for another bounty. Then we should have minimally one example working, so that it's at least possible for some general try-out.

Would this be a way to go? You can very-well make an alternative suggestion. I will also try to keep this up again and further think along within the coming 2 weeks.

Cheers Holger

jwasinger commented 5 years ago

If it sounds good to you @holgerd77 @tcsiwula , I would be interested in picking up the remainder of the work on this issue.

timxor commented 5 years ago

@ceresstation @holgerd77 yeah that sounds good to me, thank you. How should I mark them as Experimental? I think I had one of the examples (/discv5/simple.js | 🚧 Wip| | /discv5/peer-communication.js | 🚧 Wip ) working last but not sure if it was able to communicate properly with the corresponding geth v5 impl. Yeah that also sounds good @jwasinger .

holgerd77 commented 5 years ago

@tcsiwula I am also not completely sure yet and have to take some more time with a closer look. Let's come up with some clear cut concept until mid next week (Wednesday, 5th), and then do eventual last necessary changes and some clean up to get things finalized and closed before Christmas. Does this make sense and is realistic? I will also have an active look during the week and propose something, think I will need 1-2 hours though.

@jwasinger You are very welcome to jump in! 👍 Just to make things clear: I have now proposed @tcsiwula the whole bounty for the work - think this is also deserved - and bounties are generally not applicable for people with a contract with EF. So this would be work within your general contract conditions.

jwasinger commented 5 years ago

Sounds good @holgerd77 . It may be several weeks before I get a chance to start tackling this (so anyone is welcome to pick it up in the meantime). Thanks for the contribution @tcsiwula . the PR looks very substantial and we very much appreciate your help.

holgerd77 commented 5 years ago

@tcsiwula Sorry I got over that one, will also circle in @vpulim since he is deeper in the the whole networking topic since he is working on this on the client. Maybe he has some good ideas on demarcation/integration of the current code state of the Discovery v5 features into the existing code base.

jwasinger commented 5 years ago

So I realized that I actually don't have the bandwidth to take this on :/

holgerd77 commented 5 years ago

@jwasinger Ok, thanks, feedback is really appreciated, better to realize early on then to always postpone or something! Thanks for the initial offer!

vpulim commented 5 years ago

@tcsiwula Sorry I got over that one, will also circle in @vpulim since he is deeper in the the whole networking topic since he is working on this on the client. Maybe he has some good ideas on demarcation/integration of the current code state of the Discovery v5 features into the existing code base.

@holgerd77 The PR isn't ready for merging into master as-is. src/discv5/node_record.js has syntax errors that prevent lib from building and there are console.logs throughout the code that need to be removed. However, after commenting out the breaking code in node_record.js I was able to confirm the the discv4 code is still working (I was able to download blocks using the client). Without diving more deeply into the code, my initial instinct is that the PR shouldn't be merged into master until it's cleaned up and tested more thoroughly. Also, I would recommend holding off until the v5 spec is finalized or a real urgent need for v5 arises. Currently, discv4 seems to be working just fine for client needs and both geth and parity have no plans (that I know of) to stop supporting v4 any time soon.

holgerd77 commented 5 years ago

Ok, thanks @vpulim for the analysis, then let's do the following:

@tcsiwula I've send you a collaborator invite for the repository. Can you do some last basic cleanups mentioned in the comment above and then do a final re-pushing directly to the repository so that we have administrative control over this?

I would then say that we'll leave this as an open PR as a future working basis for now and I would recommend @ceresstation that we pay out a bounty of 1.000 DAI for this (with the lesson also learned to never again do ETH bounties). How does this sound for everyone?

holgerd77 commented 5 years ago

reminder ping (see message above): @ceresstation @tcsiwula

spm32 commented 5 years ago

@tcsiwula is there anything I can do on my end to help get this across the finish line? It's been outstanding for quite a while. We can of course adjust the bounty amount to be equal to the amount 2 ETH was worth when the bounty began.

holgerd77 commented 5 years ago

Hi @tcsiwula, see the edited comment from @ceresstation above with a note on adjustment of the bounty value. Please let me know if it is possible to do the last adjustments on this I mentioned above until the end of the year, or if you have a significant reason why this is not possible (you can also PM me on Gitter). I would otherwise close/cancel this bounty.

timxor commented 5 years ago

Hello @holgerd77 @ceresstation sorry have been behind on my email. Let me process the above thread. So was the amount 1.000 DAI equal to $1000 or what was the USD value?

holgerd77 commented 5 years ago

1 DAI = 1 US $, so yes.

holgerd77 commented 5 years ago

Ok, just re-read the thread, seems that we have two a bit conflicting statements here with @ceresstation proposing a 2 ETH adjustment and me some conversion to 1.000 DAI.

I would find the 1.000 DAI version a bit more appropriate TBH, since we would leave this task in a very open state with the suggestion I made above, this has exceeded the originally planned time by some order of magnitude and lastly the bounty actually had been defined in ETH with everyone knowing about the risk of ETH price fluctuation.

Anyhow, it's Christmas 🎄 😄 and at the end it's up to Gitcoin to decide on the bounty level (since they have to pay! 😄). So let's take the 2 ETH adjustment version, this would be 712,63 $/ETH (price at bounty opening, see Gitcoin Bot comment above) * 2 ETH = 1.425,26 $ ~ 1.425 DAI. (I would now really stick to DAI here, since ETH price is very fluctuant right now and we very well might end up with another 2x difference in 2 weeks or so).

I will stick to the end-of-year deadline though.

So please do the following:

We can then pay out the bounty.

Best Holger

spm32 commented 5 years ago

Hey @holgerd77 sorry we're on the same page, I was just saying I'd be willing to pay a bounty amount equal to the amount 2 ETH was worth when the bounty began in DAI. In other words I can confirm that we'd be able to pay $1,425 DAI. @tcsiwula please confirm that this is good on your end :)

timxor commented 5 years ago

Okay sounds good to me @holgerd77 @ceresstation will get it done by Sunday for review!

holgerd77 commented 5 years ago

Sounds good! 😄

timxor commented 5 years ago

working on right now, a bit late but expect soon.

holgerd77 commented 5 years ago

That's ok.

timxor commented 5 years ago

Just wrapping up. Created a new PR & branch #48 summary here compiles and runs now successfully should be okay to close out the issue. Let me know if you need me to submit or close the bounty manually @holgerd77 @ceresstation

Further work still needs to be done with regards to the Node Record Specification and testing. I can try to work on this some more in 2019 or at least define some subset pr's. Kinda in the middle of moving, interviewing and spending holidays with my family (just a fyi). Normally I would be more responsive!

Cheers~ 🤓😄

holgerd77 commented 5 years ago

We can very well see this as accomplished, Tim @tcsiwula thanks for doing all the work on this, we've learned to do more fine-grained Gitcoin issues in the future. Hope it was not too overwhelming, would be great if you can continue with some work in the time to come. 😄 👍

@ceresstation Can you initiate the bounty payout (the last comment I made on the PR shouldn't be a blocker, this is just some test run fixes and the PR will stay open anyhow)?

spm32 commented 5 years ago

@tcsiwula thanks for getting that done, @holgerd77 initiating the payout now!

gitcoinbot commented 5 years ago

⚡️ A tip worth 2.00000 ETH (301.9 USD @ $150.95/ETH) has been granted to @tcsiwula for this issue from @vs77bb. ⚡️

Nice work @tcsiwula! Your tip has automatically been deposited in the ETH address we have on file.

gitcoinbot commented 5 years ago

⚡️ A tip worth 700.00000 DAI (700.0 USD @ $1.0/DAI) has been granted to @tcsiwula for this issue from @vs77bb. ⚡️

Nice work @tcsiwula! Your tip has automatically been deposited in the ETH address we have on file.

vs77bb commented 5 years ago

@tcsiwula Please confirm you've received the payment! I remember when this task began - glad to see it across the finish line. Thanks for your patient and thoughtful reviews along the way, @holgerd77 🙂

gitcoinbot commented 5 years ago

⚡️ A tip worth 425.00000 DAI (425.0 USD @ $1.0/DAI) has been granted to @tcsiwula for this issue from @ceresstation. ⚡️

Nice work @tcsiwula! Your tip has automatically been deposited in the ETH address we have on file.

kuhnchris commented 5 years ago

Hi there @tcsiwula , would you mind finishing this on the gitcoin side aswell, so we can finish this bounty? Thank you!

Chris

timxor commented 5 years ago

Pretty sure got the payout, will confirm when I get access to my desktop next. Also will close out on gitcoin side soon!