Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disc v5.1 implementation #72

Merged
merged 46 commits into from
Sep 30, 2020
Merged

Disc v5.1 implementation #72

merged 46 commits into from
Sep 30, 2020

Conversation

Nashatyrev
Copy link
Contributor

@Nashatyrev Nashatyrev commented Sep 16, 2020 •
edited
Loading

PR Description

Disc v5.1 implementation (ethereum/devp2p#157)

  • 5.1 packet messages
  • Test vectors for new messages
  • Refactor Pipeline approach (planning as a separate PR)
  • Unit tests for new messages
  • Integrate new messages
  • Update negotiation
  • implement TALKREQ/TALKRESP (planning as a separate PR)
  • check interop with Lighthouse
  • (?) check interop with Prysm (not sure Prysm is ready now)

Fixed Issue(s)

Fixes Consensys/teku#2797

@Nashatyrev Nashatyrev mentioned this pull request Sep 17, 2020
Closed
@Nashatyrev Nashatyrev marked this pull request as ready for review September 24, 2020 18:40
mbaxter
mbaxter reviewed Sep 28, 2020
View reviewed changes
src/main/java/org/ethereum/beacon/discovery/message/FindNodeMessage.java Show resolved Hide resolved
src/main/java/org/ethereum/beacon/discovery/message/FindNodeMessage.java Show resolved Hide resolved
src/main/java/org/ethereum/beacon/discovery/message/handler/FindNodeHandler.java Show resolved Hide resolved
src/main/java/org/ethereum/beacon/discovery/packet/impl/StaticHeaderImpl.java Outdated Show resolved Hide resolved
src/main/java/org/ethereum/beacon/discovery/message/handler/FindNodeHandler.java
.collect(Collectors.toList());

List<List<NodeRecord>> nodeRecordsList =
Lists.partition(nodeRecordInfos, MAX_NODES_PER_MESSAGE);
Copy link

Choose a reason for hiding this comment

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

Do we need any limits on this? The spec says: "The recommended result limit for FINDNODE queries is 16 nodes."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, with this respect the MAX_NODES_PER_MESSAGE size is set to 4 here. This satisfies any limits except of Handshake message. This is highly unlikely situation for a honest remote peer to send us back WhoAreYou on a response packet but this is not a protocol violation and we should normally respond with Handshake including the original Nodes message.
Need to think about it more cause that would require more complex handling - you don't know in advance which packet your message would be delivered with :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a check on outgoing messages on their max size for this case. I think just dropping an outgoing Nodes message if it for some reason is delivering with a Handshake packet would be enough for now.
Also logged this issue #74

Copy link

Choose a reason for hiding this comment

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

Do we need a limit on the overall total though? Wasn't sure if the limit of 16 was per response or overall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sound like the 16 is recommended as a total ENRs returned (in one or more Nodes packets) for a single FindNode request.
Added the code which limits the number of ENRs
Also suggested to make this statement and the limit itself more explicit: https://github.com/ethereum/devp2p/pull/157/files#r497382489

src/main/java/org/ethereum/beacon/discovery/message/handler/FindNodeHandler.java Outdated Show resolved Hide resolved
src/main/java/org/ethereum/beacon/discovery/packet/AuthData.java Outdated
return new AuthDataImpl(gcmNonce);
}

static Header<AuthData> createHeader(Bytes32 srcNodeId, Bytes12 gcmNonce) {
Copy link

Choose a reason for hiding this comment

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

Looks like this is only used in tests - should we move this to a test util?

Suggested change
static Header<AuthData> createHeader(Bytes32 srcNodeId, Bytes12 gcmNonce) {
static Header<AuthData> createOrdinaryHeader(Bytes32 srcNodeId, Bytes12 gcmNonce) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there are too many interface factory methods left from the very beginning. I have cleaned them up here: a77ff29

  • moved all create*Header static methods to the Header class
  • Left *Packet.create() methods
  • Removed almost all others

src/main/java/org/ethereum/beacon/discovery/packet/HandshakeMessagePacket.java Outdated Show resolved Hide resolved
src/main/java/org/ethereum/beacon/discovery/packet/impl/RawPacketImpl.java Outdated Show resolved Hide resolved
src/main/java/org/ethereum/beacon/discovery/util/Utils.java Outdated Show resolved Hide resolved
@Nashatyrev
The Nodes.total field should contain the total number of packets but …
6221320 
…not records
@Nashatyrev
Assign the method with more explanatory name
12cfd65
@Nashatyrev
Fix typo in class name
bd2f54d
@Nashatyrev
Fix param name srcNodeId -> destNodeId. Name params more explicitly (…
10fa187 
…destNodeId instead of nodeId)
@Nashatyrev
Narrow compareBytes() param type from Bytes to Bytes32 to avoid incor…
d65bcdf 
…rect usage
@Nashatyrev
Apply spotless
091a919
@Nashatyrev
Reduce the number of static factory methods in 5.1 Packet interfaces.…
a77ff29 
… Leave just Header.create* and *Packet.create()
@Nashatyrev
Fix the test: empty FindNode distances list is not allowed
0bb80a7
@Nashatyrev
Apply spotless
7d627af
@Nashatyrev
Increase retryTimeout for slow machines (like CircleCI docker)
84a8118
@Nashatyrev
Increase also life check interval
91d7e4d
@Nashatyrev
Check outgoing packet against max size. Just drop it if too large
640b118
@Nashatyrev Nashatyrev mentioned this pull request Sep 29, 2020
Open
mbaxter
mbaxter approved these changes Sep 29, 2020
View reviewed changes
src/main/java/org/ethereum/beacon/discovery/message/handler/FindNodeHandler.java
.collect(Collectors.toList());

List<List<NodeRecord>> nodeRecordsList =
Lists.partition(nodeRecordInfos, MAX_NODES_PER_MESSAGE);
Copy link

Choose a reason for hiding this comment

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

Do we need a limit on the overall total though? Wasn't sure if the limit of 16 was per response or overall.

src/main/java/org/ethereum/beacon/discovery/pipeline/handler/OutgoingParcelHandler.java Outdated Show resolved Hide resolved
src/main/java/org/ethereum/beacon/discovery/pipeline/handler/OutgoingParcelHandler.java Outdated Show resolved Hide resolved
Nashatyrev and others added 6 commits September 30, 2020 12:24
@Nashatyrev @mbaxter
Fix wording
3e7ee9a 
Co-authored-by: mbaxter <meredith.baxter@consensys.net>
@Nashatyrev @mbaxter
Large outbound packet size is rather the error
50748e4 
Co-authored-by: mbaxter <meredith.baxter@consensys.net>
@Nashatyrev
Limit the total number of returned ENRs for a single FindNode request
69e5ee9
@Nashatyrev
Limit the total number of returned ENRs for a single FindNode request
c7b5611
@Nashatyrev
Merge remote-tracking branch 'origin/feature/disc-v5.1' into feature/…
7f648c5 
…disc-v5.1

# Conflicts:
#	src/main/java/org/ethereum/beacon/discovery/message/handler/FindNodeHandler.java
@Nashatyrev
Apply spotless
f1b879c
@Nashatyrev Nashatyrev merged commit 53cb77f into Consensys:master Sep 30, 2020
@Nashatyrev
Copy link
Contributor Author

'Umbrella' issue for all implementers: ethereum/consensus-specs#2059

@Nashatyrev Nashatyrev mentioned this pull request Oct 1, 2020
Merged
@macfarla macfarla mentioned this pull request Jul 19, 2022
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbaxter mbaxter mbaxter 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.

Update to discv5.1
2 participants
@Nashatyrev @mbaxter