Use the default peering port for daemon start up #3662
Conversation
nano/node/node.cpp
Outdated
| @@ -702,6 +705,7 @@ void nano::node::keepalive_preconfigured (std::vector<std::string> const & peers | |||
| { | |||
| for (auto i (peers_a.begin ()), n (peers_a.end ()); i != n; ++i) | |||
| { | |||
| // Question: why isn't keepalive being called with `network.port`? looks like it should. Can we make this change? | |||
There was a problem hiding this comment.
I want to push another commit to either fix this or at least remove the comment after we've discussed it.
@clemahieu @dsiganos -- does this look ok to you? Why would the default value be used for these keepalives?
There was a problem hiding this comment.
The default value is used for the preconfigured keepalives because the preconfigured peers config only takes an address and not address and port pair as it probably should.
I would like to change the preconfigured peers config to take address and port info to make it more consistent and to make it easier to setup private tests networks.
There was a problem hiding this comment.
Ohh okay so we can't use our port because they use the default one anyways...if they haven't been set up with another explicit one which they mustn't in order for this to work... Ok I see. Well, yeah I agree with you there's some room for more flexibility in there, but it's clear for me now so I'll just take out the question and leave a small comment in there.
There was a problem hiding this comment.
Alright, updated the comment.
|
Is there a way to test this using a unit test? |
I was thinking about this. I don't think we have any daemon level unit tests. |
We could test argument parsing by putting all that functionality in to a common function which we can call from unit tests. If we're trying to test int main () functionality, we can move everything inside int main () to a new function with the same signature that does all the command line parsing. int main () would only call this new function and we can make a test that calls this signature with unit test argv/argc arguments to test parsing. For testing success, we could connect a directly created node to the node created through the entry-point and connect it on the expected port. |
theohax
force-pushed
the
default-port-daemon-start
branch
from
8751e71 to
e5c29c1
Compare
Changes related to removing json config files. # Conflicts: # nano/node/nodeconfig.cpp # nano/node/nodeconfig.hpp
|
Merging this -- with a ticket in the backlog for creating a test for it. |
This PR fixes a small regression in that after #3554 (which correctly considers input port numbers with a value of 0 mean 'let the OS decide'), the default peering port was not used anymore by the node not only in unit tests (which is correct), but also by the daemon itself at start up.
Now the daemon checks the value that comes from the configuration and if there isn't a specific port value in there, it will automatically set the default peering port value for the given network type before attempting to create and start the node.