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

Node Discovery v4 ENR Extension (EIP-868) #11540

Merged
merged 1 commit into from
Apr 4, 2020
Merged

Node Discovery v4 ENR Extension (EIP-868) #11540

merged 1 commit into from
Apr 4, 2020

Conversation

vorot93
Copy link

@vorot93 vorot93 commented Mar 4, 2020

This PR introduces integration with Ethereum Node Record standard as described in EIP-868.

This is really more a proof of concept because of the following caveats:

  • The very first application of ENR is the storage of FORK_ID and FORK_HASH as described by EIP-2124/EIP-2364. Basically this means leaking the state of blockchain into Discovery (!) on a continuous basis with the addition of every block (!). It breaks the current separation of layers between BlockChainClient, sync and devp2p.
  • On top of that, ENR requires versioning which means rich object with disk persistence. It's currently a part of Host but is this the best solution?
  • ENR looks like NodeEndpoint - what will be the relationship between these two going forward?

All in all, this implementation is partially workable, but the full implementation will require rethinking sync design.

@vorot93 vorot93 requested review from tomusdrw, ordian and niklasad1 March 4, 2020 12:53
dvdplm
dvdplm reviewed Mar 5, 2020
View reviewed changes
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Initial review, will be back.

util/network-devp2p/src/discovery.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/host.rs Outdated Show resolved Hide resolved
util/network-devp2p/Cargo.toml Outdated Show resolved Hide resolved
niklasad1
niklasad1 reviewed Mar 5, 2020
View reviewed changes
Cargo.lock Show resolved Hide resolved
niklasad1
niklasad1 reviewed Mar 5, 2020
View reviewed changes
Cargo.lock Outdated Show resolved Hide resolved
@niklasad1 niklasad1 mentioned this pull request Mar 5, 2020
Merged
@ordian
Copy link
Member

ordian commented Mar 5, 2020

While it's nice to be able to reuse the enr code, this particular library seems a bit immature and tailored for eth 2.0 client. EIP-778 (see prior #11354) doesn't seem complicated and IMHO it's better to have our own implementation here and avoid depending on libraries we don't need and not depend on a third-party if we need to change something quickly.

tomusdrw
tomusdrw suggested changes Mar 5, 2020
View reviewed changes
util/network-devp2p/src/discovery.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/discovery.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/host.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/host.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/node_table.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/host.rs Outdated
let key = H256::random().into();
save_key(tempdir.path(), &key);
let r = load_key(tempdir.path());
let key = Secret::from(H256::random());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a test for ENR entry too?

util/network-devp2p/src/host.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/host.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/host.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/host.rs Outdated Show resolved Hide resolved
@vorot93 vorot93 mentioned this pull request Mar 17, 2020
Merged
@vorot93 vorot93 force-pushed the vorot93/enr branch 4 times, most recently from 6155317 to d46c9b3 Compare March 24, 2020 07:01
@vorot93
Copy link
Author

vorot93 commented Mar 24, 2020

I have completely redone the PR from scratch using the architecture proposed by @tomusdrw.

dvdplm
dvdplm reviewed Mar 24, 2020
View reviewed changes
util/network-devp2p/Cargo.toml Outdated Show resolved Hide resolved
util/network-devp2p/src/discovery.rs Show resolved Hide resolved
util/network-devp2p/src/disk.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/disk.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/disk.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/node_record.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/disk.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/lib.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/node_record.rs Outdated Show resolved Hide resolved
tomusdrw
tomusdrw reviewed Mar 24, 2020
View reviewed changes
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm! Few minor things, would be also good to add some docs (although I know the items are not really public nor we enforce docs here, but still it might be a good time to start adding them :))

util/network-devp2p/src/discovery.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/discovery.rs Show resolved Hide resolved
util/network-devp2p/src/host.rs Show resolved Hide resolved
util/network-devp2p/src/host.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/node_record.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/node_record.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/discovery.rs Outdated Show resolved Hide resolved
@vorot93 vorot93 force-pushed the vorot93/enr branch 3 times, most recently from b0a8da0 to fad711b Compare March 31, 2020 11:18
@vorot93 vorot93 requested review from dvdplm, ordian and niklasad1 March 31, 2020 23:17
dvdplm
dvdplm approved these changes Apr 2, 2020
View reviewed changes
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Mustn't grumble.

@ordian ordian merged commit e047bb4 into master Apr 4, 2020
@ordian ordian deleted the vorot93/enr branch April 4, 2020 08:52
dvdplm added a commit that referenced this pull request Apr 14, 2020
@dvdplm
Merge branch 'master' into dp/chore/remove-accounts-bloom
88b0225 
* master: (25 commits)
  Update .gitmodules (#11628)
  ethcore/res: activate ecip-1088 phoenix on classic (#11598)
  Upgrade parity-common deps to latest (#11620)
  Fix Goerli syncing (#11604)
  deps: switch to upstream ctrlc (#11617)
  Deduplicating crate dependencies (part 3 of n) (#11614)
  Deduplicating crate dependencies (part 2 of n, `slab`) (#11613)
  Actually save ENR on creation and modification (#11602)
  Activate POSDAO on xDai chain and update bootnodes (#11610)
  Activate on-chain randomness in POA Core (#11609)
  Deduplicating crate dependencies (part 1 of n) (#11606)
  Update enodes for POA Sokol (#11611)
  Remove .git folder from dogerignore file so vergen library can get build date and commit hash in the binary generatio vergen library can get build date and commit hash in the binary generation (#11608)
  Reduced gas cost for static calls made to precompiles EIP2046/1352 (#11583)
  [easy] `ethcore-bloom-journal` was renamed to `accounts-bloom` (#11605)
  Use serde_json to export hardcoded sync (#11601)
  Node Discovery v4 ENR Extension (EIP-868) (#11540)
  Fix compile warnings (#11595)
  Update version to 3.0.0-alpha.1 (#11592)
  ethcore/res: bump canon fork hash for mordor and kotti testnets (#11584)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@niklasad1 niklasad1 niklasad1 left review comments

@ordian ordian ordian approved these changes

@tomusdrw tomusdrw tomusdrw requested changes

@dvdplm dvdplm dvdplm 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.
5 participants
@vorot93 @ordian @dvdplm @tomusdrw @niklasad1