Skip to content

Discovery v5 #189

Merged
mkalinin merged 80 commits intodevelopfrom
feature/discv5-client
Nov 27, 2019
Merged

Discovery v5 #189
mkalinin merged 80 commits intodevelopfrom
feature/discv5-client

Conversation

@zilm13
Copy link
Copy Markdown
Member

@zilm13 zilm13 commented Sep 16, 2019

Implementation of this:
https://github.com/ethereum/devp2p/blob/master/discv5/discv5.md

Still in work, need to finish some things, add network, plus refactoring especially around messageHandler <-> Context, the system is raw.

@zilm13
Copy link
Copy Markdown
Member Author

zilm13 commented Sep 17, 2019 •
edited
Loading

Network using Netty added.

TODO:

  • make it work w/o network (network looks not an issue)
  • polish

@mkalinin mkalinin mentioned this pull request Sep 23, 2019
Closed
11 tasks
@zilm13
Copy link
Copy Markdown
Member Author

zilm13 commented Sep 24, 2019

TODO: Buckets according to spec
TODO: handshake crypto

mkalinin
mkalinin requested changes Sep 25, 2019
View reviewed changes
Comment thread discovery/src/main/java/org/ethereum/beacon/discovery/enr/NodeRecord.java Outdated
Comment thread discovery/src/main/java/org/ethereum/beacon/discovery/enr/NodeRecord.java Outdated
Comment thread discovery/src/main/java/org/ethereum/beacon/discovery/enr/NodeRecord.java Outdated
Comment thread discovery/src/main/java/org/ethereum/beacon/discovery/enr/NodeRecord.java Outdated
Comment thread discovery/src/main/java/org/ethereum/beacon/discovery/enr/NodeRecord.java Outdated
Comment thread discovery/src/main/java/org/ethereum/beacon/discovery/enr/NodeRecord.java Outdated
zilm13 added 11 commits September 25, 2019 16:47
@zilm13
discovery: noderecordv4, v5 refactoring. Typo fixes, redundant conver…
1e775ea 
…sions cleanup
@zilm13
discovery: NodeRecord: add guaranteed fields to the interface, fix se…
840ac96 
…q size
@zilm13
util: remove Bytes33 type
59180c9
@zilm13
discovery: make all node record fields accessible. Remove ssz usage f…
930e994 
…or serialization
@zilm13
discovery: fix logDistance to use bit index instead of logarithm
e6f734e
@zilm13
discovery: make NODES answer use Buckets
159457e
@zilm13
discover: fix ECDSA signature sign and recover
7e90aef
@zilm13
discovery: remove NodeRecordV5 as V4 is the only available scheme
f593c22
@zilm13
discovery: refactor Ethereum Node Record to further detach it from ot…
25f222f 
…her entities
@zilm13
discovery: make udp client independent of server
bfd48b1
@zilm13
discovery: use enr scheme interpreter instead of full enr abstractions
84dabf5
zilm13 added 12 commits November 3, 2019 23:05
@zilm13
discovery: finish community tests + fixes
599108f
@zilm13
discovery: update idNonce signing test with correct community vector
66fcb7f
@zilm13
discovery: fix node encoding in AuthHeaderMessagePacket and tests
2eb78e2
@zilm13
discovery: add sign to the enr scheme interface as it's a part of scheme
524833e
@zilm13
discovery: spec fixes, interop test with geth started but not finished
1c4d0b6
@zilm13
discovery: fixed maximum packet size verification + nodes sent in fin…
f29395f 
…dnodes reply
@zilm13
discovery: fixes encoding flow, plus new task api
2bf36a7
@zilm13
discovery: fix and add appropriate asserts in Geth interop test
f3412e3
@zilm13
discovery: interop test with geth fixed
310c825
@zilm13
discovery: refactor ENR routines to have abstractions according to sp…
000a312 
…ecification
@zilm13
discovery: better interface for NodeRecord encoding routines
2c61b55
@zilm13
discovery: javadocs and final cleanup
ccc23a1
@zilm13 zilm13 requested a review from mkalinin November 22, 2019 14:08
@zilm13
Copy link
Copy Markdown
Member Author

zilm13 commented Nov 22, 2019

@mkalinin Finished it. Please, review.

@shahankhatch
Copy link
Copy Markdown
Contributor

shahankhatch commented Nov 23, 2019

lgtm. I have a minor request to review/accept #210 which renames two methods.

@zilm13
Merge pull request #210 from shahankhatch/feature/discv5-client
9985d2e 
Refactor method name to distinguish by parameter name
mkalinin
mkalinin reviewed Nov 26, 2019
View reviewed changes
Comment thread discovery/src/test/java/org/ethereum/beacon/discovery/DiscoveryInteropTest.java Outdated
System.out.println(String.format("Node %s started", nodePair1.getValue1().getNodeId()));
NodeRecord nodeRecord1 = nodePair1.getValue1();
NodeRecord nodeRecord2 =
NODE_RECORD_FACTORY_NO_VERIFICATION.fromBase64(
Copy link
Copy Markdown
Contributor

@mkalinin mkalinin Nov 26, 2019

Choose a reason for hiding this comment

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

It would be convenient if one may change parameters of this node like IP address. I had to replace this base64 line to make test run on Mac OS as it handles docker in a docker-machine instance with its own network interface and addresses.

Copy link
Copy Markdown
Member Author

@zilm13 zilm13 Nov 26, 2019

Choose a reason for hiding this comment

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

Yeah, did this, plus refactored a bit NodeRecord creation

mkalinin
mkalinin reviewed Nov 26, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@mkalinin mkalinin left a comment

Choose a reason for hiding this comment

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

I am in favor of merging this PR and making further refactor/updates via separate requests. I have a suggestion to rename discovery module to discv5 for clarity.
@shahankhatch @zilm13 WDYT?

P.S. interop test looks fantastic! There is a little fix that could increase its usability.

@zilm13
Copy link
Copy Markdown
Member Author

zilm13 commented Nov 26, 2019

@mkalinin I don't like discv5 as a lot of code could be reused in, say, discv6

@shahankhatch
Copy link
Copy Markdown
Contributor

shahankhatch commented Nov 26, 2019

@mkalinin I don't like discv5 as a lot of code could be reused in, say, discv6

I agree with this reasoning.

@mkalinin
Copy link
Copy Markdown
Contributor

mkalinin commented Nov 26, 2019

This particular implementation is discv5. I don't think it's gonna be re-used for discvX in the future. Anyway, it doesn't matter much :)

@mkalinin mkalinin changed the title WIP: discovery v5 Discovery v5 Nov 27, 2019
@mkalinin mkalinin merged commit b2ab7fe into develop Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mkalinin mkalinin mkalinin approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zilm13 @shahankhatch @mkalinin