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

[RLPx] add option to connect to remote hosts from a predefined local port #83

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jochem-brouwer
Copy link
Member

When playing around with this library and trying to connect to my local node, I noticed that even when I set RLPx's listenPort it would connect from random local ports to my node. I would expect that it would use listenPort to connect from, but this is apparently not done.

This is handy, because for instance in Geth you can then set this peer as a "trusted peer" and keep this peer in the peer pool. However, you need the IP and the port for this to add the trusted peer: if this keeps changing this is annoying.

I tried not adding this connectPort option and instead just using listenPort but this breaks the tests. I am not sure if this is due to the tests or that there is a fundamental problem by defaulting it to listenPort (I think it is due to the tests?).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 87.338% when pulling a6babb3 on fix-connect-port into 826a06b on master.

@holgerd77
Copy link
Member

@jochem-brouwer I am always a bit on thin ice when reasoning about networking questions. My first intuition says that we should try to really avoid another port option connectPort and instead just try to fix the tests failing respectively adopt the test runner.

I would very much assume that every peer connection would need its own port to connect from on the local side? So I have to say that I very much don't understand this whole listenPort setup with the same listenPort given to every new peer created. Maybe we should ground our overall knowledge here before we continue.

@holgerd77
Copy link
Member

Ah ok, think I got it, the listenPort is just the port to listen for incoming peer connections (normally on our current setup/use cases we rather go out than letting in for connection). The connection itself to a peer is then started on a separate port, so that the listenPort remains the same to continue to listen for other incoming connections.

Does this make sense?

@jochem-brouwer
Copy link
Member Author

It does make sense, but without this change it always means that a RLP connection is made from a random local port. I don't think that this is correct. If you check out the peers you get in Geth all of them connect from the current port (30303 default) to the remote hosts' port (in most cases also 30303). This does not seem to be reflected here. I have not checked but also assume that our client thus also connects from random local ports.

@holgerd77
Copy link
Member

Hi @jochem-brouwer, I was just looking over the open PRs here on the library to see which one we should merge before the monorepo transition. On this one particular I have to say that I don't have enough of an overview atm on the overall port usage situation on the library - so which service is using what port for what reason and I also don't have the time atm to go deeper with my thinking into this so that I wouldn't be able to do a review for now (everyone else of course invited to do so alternatively).

Is it generally ok if in doubt we just let this open and you might need to recreate the PR on the monorepo? Not that much LoC touched so it shouldn't be that much work.

@jochem-brouwer
Copy link
Member Author

Yep it's no problem to recreate it. And indeed it would be nice to get a general overview of the ports which are used.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants