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

Handle case where peers advertise a listening port of 0 #1391

Merged
merged 2 commits into from
May 2, 2019

Conversation

mbaxter
Copy link
Contributor

@mbaxter mbaxter commented May 2, 2019

PR description

It is common for peers to advertise a listening port of 0. Handle this case by using a default listening port of 30303.

Copy link
Contributor

@AbdelStark AbdelStark left a comment

Choose a reason for hiding this comment

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

LGTM

int port = peerInfo.getPort();
if (port == 0) {
// Most peers advertise a port of "0", just set a default best guess in this case
port = EnodeURL.DEFAULT_LISTENING_PORT;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of the default port perhaps we could use the remote port of the current communication? We are already doing this with the remoteAddress.

Also, can these two values ever differ? Such as if we have an eth loadbalancer or proxy in front of us (in case those ever exist for devp2p).

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 experimented with trying to use the remote port, but it doesn't match the listening port. When the other nodes are initiating outbound connections, they're free to use whatever port they want for their side of the connection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Un-resolving this comment because I think its a good question and will be helpful to have it visible here as a kind of documentation. Actually, I think I might add a comment in the codebase as well :D

@mbaxter mbaxter merged commit dadaad2 into PegaSysEng:master May 2, 2019
ajsutton pushed a commit to ajsutton/pantheon that referenced this pull request May 2, 2019
@xavierlepretre
Copy link

As a side note, my Geth reports its own admin.nodeInfo.enode as "enode://3dffa8485a02fcb969e8afa47ba7134c785c3b04be00086907b3766df7280dfe1b9d38813be7f7f90043215c9fffca5f7778e6a1c403fb4cac087c13b908a9b5@127.0.0.1:30303?discport=0". If I don't truncate the ?discport=0 before I add the string to Pantheon's bootnodes, then Pantheon complains that:

picocli.CommandLine$ParameterException: Invalid enode URL syntax. Enode URL should have the following format 'enode://<node_id>@<ip>:<listening_port>[?discport=<discovery_port>]'. Invalid discovery port.  Port should be between 1 - 65535.
	at tech.pegasys.pantheon.cli.PantheonCommand.setBootnodes(PantheonCommand.java:187)

@mbaxter
Copy link
Contributor Author

mbaxter commented May 3, 2019

Thanks for reporting @xavierlepretre ! I've added a ticket and we will address this soon: https://pegasys1.atlassian.net/browse/PAN-2618

notlesh pushed a commit to notlesh/pantheon that referenced this pull request May 4, 2019
notlesh pushed a commit to notlesh/pantheon that referenced this pull request May 14, 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