Assign default port to [ips_fixed] and [ips] if unspecified #2931
Conversation
Replaces previous behaviour where error was thrown
|
Thank you for your submission. It will be reviewed soon and submitted for processing in CI. |
1 similar comment
|
Thank you for your submission. It will be reviewed soon and submitted for processing in CI. |
| @@ -534,11 +534,12 @@ OverlayImpl::onPrepare() | |||
| { | |||
| if (addr.port () == 0) | |||
| { | |||
| Throw<std::runtime_error> ("Port not specified for " | |||
| "address:" + addr.to_string ()); | |||
| ips.push_back (to_string (addr.at_port(51235))); | |||
There was a problem hiding this comment.
Please don't make a port in the dynamic/private ports range the default.
There was a problem hiding this comment.
@MarkusTeufelberger 51235 is already the default port (see existing instances a few lines above). This patch is just to automatically set it on ips & fixed ips when not specified in the config.
There was a problem hiding this comment.
There currently is no default port in rippled, this is just the one these instances use.
You are suggesting to make a port from a port range that is supposed to be dynamic/private to be the default port of rippled.
There was a problem hiding this comment.
OK I see what you're saying, though I would argue that 51235 is currently the default port in all but name, afterall it is the default port set in the config & used by most instances (that I know of).
What do you suggest as far as default port assignment, or are you saying that #2861 is a non-issue?
There was a problem hiding this comment.
I'd recommend just grabbing an unassigned port from the proper range and maybe in parallel also asking officially for an assigned port - it is actually not that difficult or impossible to get that: https://tools.ietf.org/html/rfc6335#page-16
There was a problem hiding this comment.
I am supportive of registering a port. Give me a day, I have a plan 😈
There was a problem hiding this comment.
Update on the plan I hinted at above:
There already is a port (2459) that @JoelKatz has registered; we could repurpose it for this, and make it the new default for rippled. We'll follow-up with IANA about this.
@movitto, it may be a good idea to hold off on this PR so that we don't add an "implicit default" that people end up relying on only to then change it. Let's wait and see whether we'll go with 2459 or register something else.
There was a problem hiding this comment.
@movitto, it may be a good idea to hold off on this PR so that we don't add an "implicit default" that people end up relying on only to then change it. Let's wait and see whether we'll go with 2459 or register something else.
Sounds good @nbougalis, let us know how the IANA follow-up goes and I can update this PR from there.
Jenkins Build SummaryBuilt from this commit Built at 20190813 - 16:25:54 Test Results
|
Codecov Report
@@ Coverage Diff @@
## develop #2931 +/- ##
===========================================
- Coverage 69.38% 69.37% -0.01%
===========================================
Files 708 708
Lines 56426 56425 -1
===========================================
- Hits 39152 39147 -5
- Misses 17274 17278 +4
Continue to review full report at Codecov.
|
|
Thank you for your submission. It will be reviewed soon and submitted for processing in CI. |
|
Thank you for your submission. It will be reviewed soon and submitted for processing in CI. |
|
@nbougalis @MarkusTeufelberger @alloyxrp @mDuo13 updated the default port to reflect #3037. This patch just affects the port assigned to ips and ips_fixed entries if not specified, it does not change the default ports listened on in rippled.cfg. After this any configurations w/ unspecified ports may result in connection errors if peers are still listening on the legacy port 51235 |
|
Thank you for your submission. It will be reviewed soon and submitted for processing in CI. |
|
This has been superseded by #3235. |
Replaces previous behaviour where error was thrown.
Fixes issue #2861