Skip to content
This repository has been archived by the owner on Dec 10, 2020. It is now read-only.

Node discovery v5 #19

Closed
holgerd77 opened this issue Dec 15, 2017 · 86 comments
Closed

Node discovery v5 #19

holgerd77 opened this issue Dec 15, 2017 · 86 comments

Comments

@holgerd77
Copy link
Member

holgerd77 commented Dec 15, 2017

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: ethereum/py-evm#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:

  • An extended code base which...
    • follows the style principles and coding best practices of the existing code base
    • is readable/maintainable for other developers
    • introduces no or minimal code repetitions
  • An extended test suite which keeps code coverage for the DHT on more-or-less the current level
  • Some additions to the README docs which gives some usage guides and reflect the API changes

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:

  • Experience with working on larger code bases in Node.js/modern ES6 Javascript
  • Experience with the Node.js toolchain / ecosystem
  • Knowledge/understanding of generic p2p networking concepts
  • Some understanding of the Ethereum networking mechanics
  • Experience with assuring code quality/testing

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).

@Silur Silur self-assigned this Dec 18, 2017
@Silur
Copy link

Silur commented Dec 18, 2017

I guess I'll just claim this then

Edited (@holgerd77): Open again since @Silur seems to not have enough free resources on that.

@holgerd77
Copy link
Member Author

Cool. :-)

@holgerd77
Copy link
Member Author

For reference: ethereum/go-ethereum#2117 (comment)
("p2p/discover: from address in ping is IPv6 unspecified in some cases")

@holgerd77
Copy link
Member Author

Py-EVM implementation [in progress]: gsalgado/py-evm@f31fbfc

@holgerd77
Copy link
Member Author

@Silur Are you still open/do you have time to work on this one? Otherwise I would put this back in "help wanted", with the current comments people interested will think this is already being worked on.

@gitcoinbot
Copy link

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


This issue now has a funding of 1.5 ETH (1068.95 USD @ $712.63/ETH) attached to it.

@rauljordan
Copy link

Hey @holgerd77 interested in this, as we are currently exploring p2p in my team, Prysmatic Labs. Can you elaborate more on the requirements needed to get the bounty accepted? What would an acceptable PR look like? Any specific features you have in mind to close this?

@alexanderbez
Copy link

Interested in helping out with this as well.

@abitrolly
Copy link

Is there an abstract describing current situation and the problem with discovery?

@holgerd77
Copy link
Member Author

I'll update the issue tomorrow with a more specific work description and requirements for a bounty to be accepted.

@holgerd77
Copy link
Member Author

Hi everyone, I updated the work specification, this should now hopefully accurately enough describe what is expected here, if you still have questions let me know. This might sound a bit tighter then before (didn't realize that I had this "experimental implementation" in the introduction of the issue description, I replaced this pointing to an actual working implementation).

Please re-read, take a breath and maybe sleep over one night and think, if this is a good issue for you. This is an advanced task, requiring some intrinsic motivation as well as some pre-knowledge to not make this too demanding.

Thanks everyone to expressing interest in this so far, pretty amazed about the feedback! 😄

@vs77bb
Copy link

vs77bb commented May 22, 2018

Hi all, Vivek from Gitcoin here. It looks like @brianherman was first to 'Start Work' here on Gitcoin and thus has precedence to give this a go + @alexanderbez has offered his help as well.

We'd encourage a collaborative effort here if it makes sense, as in line with open source ethos 😄perhaps @brianherman gives it a go and comes back around for feedback as needed?

@rauljordan good to see you here; @holgerd77 has updated with acceptance criteria above!

@holgerd77
Copy link
Member Author

@brianheram Hi Brian, does this still fit your expectations after the updated spec description and do you want to start on this? If you have any questions let me know!

@vs77bb
Copy link

vs77bb commented May 23, 2018

@brianherman see above^ 🙂

@gitcoinbot
Copy link

@brianherman 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!

  • warning (3 days)
  • escalation to mods (6 days)

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

@vs77bb
Copy link

vs77bb commented May 29, 2018

Hi @abitrolly @alexanderbez @rauljordan are any of you interested in taking on this bounty? @holgerd77 and I have discussed and increased the contribution to 2ETH (blockchain syncing now).

As an FYI, we understand the expiry date is near on Gitcoin, but will be able to pay this out upon completion, even if it takes beyond the expiration.

@alexanderbez
Copy link

I'm certainly happy to help on this. Seems there are three major topics: packets, node records, and topics. Perhaps we could come up with a plan/design and tackle these in parallel?

@rauljordan
Copy link

rauljordan commented May 29, 2018 via email

@holgerd77
Copy link
Member Author

@alexanderbez @rauljordan Cool guys, that sounds great!

@jvmaia
Copy link

jvmaia commented Jun 1, 2018

Hey i'm interested in help in this issue, i'm not experienced in devp2p but i want to learn more about.

@vs77bb
Copy link

vs77bb commented Jun 11, 2018

Hi @jvmaia @rauljordan @alexanderbez I wanted to follow up as the bounty funder; is there anything I can do to help coordinate? We're happy to add to the bounty as it looks like it may become a collaborative effort.

@abitrolly
Copy link

The bounty is good, but I won't have the time to research current p2p protocols and their weaknesses.

@timxor
Copy link

timxor commented Jun 17, 2018

@holgerd77 @vs77bb @gitcoinbot can we get an extension on this? I could get it done by next week, pretty familiar with this code base -- somewhat.

@holgerd77
Copy link
Member Author

@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
Copy link

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

@holgerd77
Copy link
Member Author

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

@vpulim
Copy link
Contributor

vpulim commented Dec 9, 2018

@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
Copy link
Member Author

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
Copy link
Member Author

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

@spm32
Copy link

spm32 commented Dec 18, 2018

@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
Copy link
Member Author

holgerd77 commented Dec 18, 2018

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
Copy link

timxor commented Dec 20, 2018

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
Copy link
Member Author

1 DAI = 1 US $, so yes.

@holgerd77
Copy link
Member Author

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:

  • Do a new PR with your work directly to the original repository so that we gain administrative control on the work
  • Do the syntax error and console.log() cleanup mentioned in the comment from @vpulim above
  • Optimally make sure that we can at least run one example to test the new functionality (won't make this a blocker though)

We can then pay out the bounty.

Best
Holger

@spm32
Copy link

spm32 commented Dec 23, 2018

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
Copy link

timxor commented Dec 27, 2018

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

@holgerd77
Copy link
Member Author

Sounds good! 😄

@timxor
Copy link

timxor commented Dec 31, 2018

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

@holgerd77
Copy link
Member Author

That's ok.

@timxor
Copy link

timxor commented Dec 31, 2018

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
Copy link
Member Author

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
Copy link

spm32 commented Jan 7, 2019

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

@gitcoinbot
Copy link

⚡️ 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
Copy link

⚡️ 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
Copy link

vs77bb commented Jan 7, 2019

@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
Copy link

⚡️ 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
Copy link

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

Chris

@timxor
Copy link

timxor commented Jan 30, 2019

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

@timxor timxor closed this as completed Jan 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests