Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Append default port (51235) in [ips] and [ips_fixed] when no port is specified #3235

Closed
wants to merge 1 commit into from
Closed

Append default port (51235) in [ips] and [ips_fixed] when no port is specified #3235

wants to merge 1 commit into from

Conversation

gregtatcam
Copy link
Collaborator

FIXES: #2861

@MarkusTeufelberger
Copy link
Collaborator

Shouldn't this be the IANA registered port and no longer 51235?

nbougalis
nbougalis suggested changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@nbougalis nbougalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if I want to enshrine 51235 as the default any more than it already is, as I believe that we should try to migrate to 2459 (check out the IANA registration!).

src/ripple/overlay/impl/OverlayImpl.cpp Outdated Show resolved Hide resolved
src/ripple/overlay/impl/OverlayImpl.cpp Outdated Show resolved Hide resolved
src/ripple/overlay/impl/OverlayImpl.cpp Outdated Show resolved Hide resolved
@nbougalis
Copy link
Contributor

Shouldn't this be the IANA registered port and no longer 51235?

I guess for the fixed IPs that's true, since people had to manually specify the default, but I don't know of any servers that listen on 2459 atm, except for mine... so defaulting to it also seems wrong

We need to think of a transition plan. An option might be a config setting that opens 2459 and 51235 to support both new style and legacy connections.

@movitto
Copy link
Contributor

movitto commented Feb 6, 2020

We need to think of a transition plan. An option might be a config setting that opens 2459 and 51235 to support both new style and legacy connections.

Could work but seems redundant, listening on two ports for the same protocol / use case.

Related to the discussion of #2931, perhaps a good solution would be to try both ports on the client side and select the first one that works? As you mentioned there it's not ideal but would provide an interim solution until the majority of the network switched over to the new port at which point we could simplify / just use new standardized port.

@codecov-io
Copy link

codecov-io commented Feb 12, 2020 •
edited
Loading

Codecov Report

Merging #3235 into develop will decrease coverage by 0.00%.
The diff coverage is 9.09%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3235      +/-   ##
===========================================
- Coverage    70.22%   70.22%   -0.01%     
===========================================
  Files          683      683              
  Lines        54621    54625       +4     
===========================================
  Hits         38360    38360              
- Misses       16261    16265       +4
Impacted Files Coverage Δ
src/ripple/protocol/SystemParameters.h 100.00% <ø> (ø)
src/ripple/rpc/handlers/Connect.cpp 0.00% <0.00%> (ø)
src/ripple/overlay/impl/OverlayImpl.cpp 28.96% <10.00%> (-0.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 023f570...6d9f656. Read the comment docs.

HowardHinnant
HowardHinnant suggested changes Feb 19, 2020
View reviewed changes
Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left one comment regarding const correctness. If there's a good rationale for rejecting that comment, I'm ok.

@seelabs
Copy link
Collaborator

seelabs commented Mar 5, 2020

Ping @gregtatcam

@gregtatcam
Copy link
Collaborator Author

Yes, the main motivation for set_port was to reduce the copy but I see you point. Updated with no interface changes and some extra copying.

HowardHinnant
HowardHinnant previously approved these changes Mar 19, 2020
View reviewed changes
seelabs
seelabs previously approved these changes Mar 19, 2020
View reviewed changes
Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@gregtatcam gregtatcam dismissed stale reviews from seelabs and HowardHinnant via 62364d4 March 26, 2020 22:47
@gregtatcam gregtatcam force-pushed the fix-default-port branch 2 times, most recently from 62364d4 to a203307 Compare March 26, 2020 22:49
@carlhua carlhua requested review from HowardHinnant and seelabs April 3, 2020 13:31
HowardHinnant
HowardHinnant previously approved these changes Apr 3, 2020
View reviewed changes
ximinez
ximinez requested changes Apr 3, 2020
View reviewed changes
Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nbougalis @gregtatcam I had an idea for more simply handling migration to the IANA port. Is there any real harm to sending requests to two ports? If not, ximinez@53d98f4 adds both ports to the request list if the port is undefined in the config....

src/ripple/overlay/impl/OverlayImpl.cpp Outdated Show resolved Hide resolved
src/ripple/overlay/impl/OverlayImpl.cpp Outdated Show resolved Hide resolved
src/ripple/overlay/impl/OverlayImpl.cpp Outdated Show resolved Hide resolved
src/ripple/overlay/impl/OverlayImpl.cpp Outdated Show resolved Hide resolved
ximinez
ximinez previously approved these changes Apr 7, 2020
View reviewed changes
Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, pending local testing.

src/ripple/rpc/handlers/Connect.cpp Show resolved Hide resolved
@ximinez ximinez added the Documentation README changes, code comments, etc. label Apr 7, 2020
@carlhua carlhua requested a review from HowardHinnant April 17, 2020 14:02
HowardHinnant
HowardHinnant previously approved these changes Apr 22, 2020
View reviewed changes
@gregtatcam gregtatcam dismissed stale reviews from HowardHinnant and ximinez via 9c1328b April 27, 2020 18:55
@manojsdoshi manojsdoshi added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label May 19, 2020
@nbougalis nbougalis mentioned this pull request May 29, 2020
Closed
This was referenced May 29, 2020
Closed
Closed
Merged
@mDuo13 mDuo13 mentioned this pull request Jun 30, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ximinez ximinez ximinez approved these changes

@HowardHinnant HowardHinnant HowardHinnant approved these changes

@nbougalis nbougalis Awaiting requested review from nbougalis

@seelabs seelabs Awaiting requested review from seelabs

Assignees

@HowardHinnant HowardHinnant

@ximinez ximinez

Labels
Documentation README changes, code comments, etc. Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Append default port (51235) in [ips_fixed] when no port specified
9 participants
@gregtatcam @MarkusTeufelberger @nbougalis @movitto @codecov-io @seelabs @HowardHinnant @ximinez @manojsdoshi