Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

P2P 2.3 incompatibility with older version - Closes #4060 #4065

Merged
merged 4 commits into from
Aug 12, 2019

Conversation

ishantiw
Copy link
Contributor

@ishantiw ishantiw commented Aug 9, 2019

What was the problem?

The 2.3 node should work with old 2.0 and 1.x peers. There is an incompatibility issue related to ipAddress vs ip properties in old versions. Also, the old version only supports list RPC to get peers list.

How did I solve it?

  • Created incoming and outgoing peerInfo sanitization function.
  • To fetch peers call RPC based on the version if its lower than 2.3

How to manually test it?

npm t and also connect with older versions and see if it's fetching peers

Review checklist

@ishantiw ishantiw self-assigned this Aug 9, 2019
@ishantiw ishantiw requested review from jondubois, diego-G, shuse2 and mitsuaki-u and removed request for diego-G August 9, 2019 15:47
@ishantiw ishantiw marked this pull request as ready for review August 9, 2019 15:48
@@ -39,6 +43,7 @@ import { constructPeerIdFromPeerInfo } from './utils';

const IPV4_NUMBER = 4;
const IPV6_NUMBER = 6;
export const IPADDRESS_FIELD_SUPPORT_VERSION = '2.3.0';
Copy link
Contributor

Choose a reason for hiding this comment

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

IP_ADDRESS_FIELD_SUPPORT_VERSION is better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this SDK version or Core version?
I think this version doesnt relates to this library, so Prefer not to use hardcoded version , too.

procedure: REMOTE_RPC_GET_PEERS_LIST,
procedure: isLowerVersionPeer(this._peerInfo as P2PDiscoveredPeerInfo)
? REMOTE_RPC_GET_PEERS_LIST
: REMOTE_RPC_GET_MINIMAL_PEERS_LIST,
});

return validateBasicPeersInfoList(response.data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should always remove other properties (except for ip/ipAddress and wsPort), even if the peer is the old version and it gives us the full list. The idea for the original feature was that we can't trust PeerInfo which we haven't connected to so that is why we don't store the other fields. Maybe we should keep this aspect.

@@ -39,6 +43,7 @@ import { constructPeerIdFromPeerInfo } from './utils';

const IPV4_NUMBER = 4;
const IPV6_NUMBER = 6;
export const IPADDRESS_FIELD_SUPPORT_VERSION = '2.3.0';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this SDK version or Core version?
I think this version doesnt relates to this library, so Prefer not to use hardcoded version , too.

Copy link
Collaborator

@shuse2 shuse2 left a comment

Choose a reason for hiding this comment

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

Lets re-open the issue to update the RPC endpoint and tackle on 3.0?

@jondubois jondubois changed the base branch from development to release/2.3.0 August 12, 2019 10:16
@jondubois jondubois force-pushed the 4060-p2p_2.3_incompatibility_with_older_version branch from 0512b8f to db5bde0 Compare August 12, 2019 10:21
@shuse2 shuse2 requested a review from jondubois August 12, 2019 11:04
@shuse2 shuse2 merged commit 452986f into release/2.3.0 Aug 12, 2019
@shuse2 shuse2 deleted the 4060-p2p_2.3_incompatibility_with_older_version branch August 12, 2019 11:59
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.

None yet

4 participants