Introduce support for binding sockets to ports picked by the OS#3554
Introduce support for binding sockets to ports picked by the OS#3554
Conversation
6a89397 to
a7d0895
Compare
|
It would be nice to separate (in different commits) the stylistic changes from the necessary changes, to make it easier to know what are the important changes. For example, in this PR, I cannot tell which changes are stylistic and which are crucial. |
Yeah I tried to make as good as a separation as possible (even have a couple PRs that I haven't opened yet due to exactly this reason of not mixing things up and avoiding conflicts), but some I couldn't or was too tedious to separate. I'll try to keep it in mind to do better. |
| else | ||
| { | ||
| port = listening_port; | ||
| node.network.port = listening_port; |
There was a problem hiding this comment.
Is setting node.network.port a good idea? It seems to be breaking encapsulation.
It does not seem like a good idea for bootstrap_listener to be changing a global setting.
What if we had 2 bootstrap_listeners? Then this would break.
There was a problem hiding this comment.
I think that node.network.port should remain as is. So that in the case of zero, it continues to signify that we want automatic binding. If we now create a bootstrap_listener and then destroy it and create another one, then we lose the setting that binding should be automatic.
There was a problem hiding this comment.
node.network.port is important to be up to date with the actual value resulted after automatic binding
There was a problem hiding this comment.
I feel that this is not an improvement and that we are introducing traps for the future.
Let's discuss it tomorrow at the dev meeting.
Problems that I see:
- you cannot have multiple bootstrap_listeners
- you cannot create, destroy and re-create a bootstrap_listener whilst preserving settings
There was a problem hiding this comment.
I agree that the propagating of the port picked inside bootstrap_listener towards network.port could be done in a better place, perhaps the node itself can do this, but the core concept of propagating this setting needs to stay there.
I have a series of other PRs based on this one so unless you feel this impediment must be addressed right away I'd rather address it in a future PR and yes of course we can discuss tomorrow how exactly it could work, e.g. like I suggested keep the node in charge of the propagation or maybe a different approach.
About creating multiple bootstrap_listeners, I am not sure that is even a use case, because network.endpoint() won't be a thing anymore, which endpoint exactly?
But yeah please keep reviewing and let me know about any other things I should address so that I make up a list with them.
@clemahieu keeping you in the loop here, any thoughts about this?
There was a problem hiding this comment.
@dsiganos just addressed this one -- certainly isn't a lot much better but at least now only the node deals with setting stuff that belongs to network, instead of the bootstrap_listener doing that via node, which was an even worse encapsulation breakage.
42686a6 to
618076c
Compare
|
Just rebased this on top of latest Also, @reviewers, since #3555 got merged into this already, I'd like to finally merge everything into |
This PR allows the builder of a
nano::nodenot to specify a peering port (specify it as0), or a websocket one, and let the OS pick an available port when the socket is bound. He would be able to later retrieve that port by callingnode.network.endpoint ().port(), amongst other options.The PR also adds a bunch of comments about how the mechanism works and where to pay attention if stuff gets changed.
Additionally, a couple unrelated changes made their way here and are strictly related to fixing clang-tidy warnings (like shadowed variable declarations, useless copies, etc).