Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

[PAN-2811] Be more lenient with discovery message deserialization #1580

Merged

Conversation

mbaxter
Copy link
Contributor

@mbaxter mbaxter commented Jun 19, 2019

PR description

EIP-8 specifies that devp2p discovery message deserialization should be lenient in order to make protocol upgrades easier. This PR updates discovery packet deserialization to be more lenient by:

  • ignoring extra fields
  • ignoring the version in PING packets

@@ -52,7 +52,7 @@ public static FindNeighborsPacketData readFrom(final RLPInput in) {
in.enterList();
final BytesValue target = in.readBytesValue();
final long expiration = in.readLongScalar();
in.leaveList();
in.leaveList(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional) make leaveList parameter an enum. Much easier to understand what
in.leaveList(RLPInput.IgnoreRest) does as opposed to in.leaveList(true)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say either @RatanRSur's suggestion or at least leave a comment that (true) means to ignore the rest of the list, for all the leaveList(true) calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

Third option would be to have two methods - the usual leaveList and the new leaveListSkippingRemainingElements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to leaveListLenient()

@@ -52,7 +52,7 @@ public static FindNeighborsPacketData readFrom(final RLPInput in) {
in.enterList();
final BytesValue target = in.readBytesValue();
final long expiration = in.readLongScalar();
in.leaveList();
in.leaveList(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say either @RatanRSur's suggestion or at least leave a comment that (true) means to ignore the rest of the list, for all the leaveList(true) calls.

@mbaxter mbaxter merged commit 12af8bd into PegaSysEng:master Jun 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants