search

ethereumjs / ethereumjs-devp2p

Project is in active development and has been moved to the EthereumJS VM monorepo.
https://github.com/ethereumjs/ethereumjs-vm/tree/master/packages/devp2p
141 stars 62 forks source link

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

Open jochem-brouwer opened 4 years ago

jochem-brouwer commented 4 years ago

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 commented 4 years ago

Coverage Status

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

holgerd77 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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.