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

Allow port numbers be be specified using a either a colon or a space #4328

Merged
merged 4 commits into from
Mar 15, 2023
Merged

Allow port numbers be be specified using a either a colon or a space #4328

merged 4 commits into from
Mar 15, 2023

Conversation

RichardAH
Copy link
Collaborator

High Level Overview of Change

An annoying gotcha for node operators is accidentally specifying IP:PORT combinations using a colon. Rippled expects a space, not a colon, and for whatever reason does not provide good feedback when this operator error is made.

This small PR simply allows this mistake (colon) to be replaced inline with a space, preserving the intention of the operator.

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Test Plan

  • testColons() has been added to the config unit tests. This tests the functionality when specifying IP:PORT in rippled.cfg.
  • The RPCCall test regime is not specific enough to test this functionality, but I have tested it by hand (and you should too).

nbougalis
nbougalis suggested changes Oct 25, 2022
View reviewed changes
src/ripple/core/impl/Config.cpp Outdated Show resolved Hide resolved
src/ripple/net/impl/RPCCall.cpp Outdated Show resolved Hide resolved
@RichardAH
Copy link
Collaborator Author

Thanks for the review @nbougalis. After some consideration I decided simply detecting the presence of more than one colon should be sufficient to filter out IPv6 cases. Since IPv6 can be ambiguous if a colon is used for port (in particular if square braces are also not used), IPv6 ports must remain specified via space character.

nbougalis
nbougalis approved these changes Oct 26, 2022
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.

Looks good with the change to detect IPv6 addresses.

I have an idea for a more comprehensive fix (improve Boost's parsers, and use those 😆) but this is fine for now.

ximinez
ximinez requested changes Nov 23, 2022
View reviewed changes
src/ripple/core/impl/Config.cpp Outdated Show resolved Hide resolved
src/ripple/rpc/handlers/Connect.cpp Outdated Show resolved Hide resolved
@intelliot
Copy link
Collaborator

@RichardAH when you have a chance, please respond to ximinez's comments above. Thanks!

@intelliot intelliot requested a review from ximinez December 20, 2022 00:20
@intelliot intelliot assigned ximinez and unassigned RichardAH Feb 17, 2023
ximinez
ximinez approved these changes Feb 17, 2023
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.

I wouldn't mind seeing develop merged in to the branch just so CI can get a shot at it, but I don't see a problem if not. Sorry for the delay!

@intelliot intelliot changed the base branch from develop to next February 23, 2023 05:37
@intelliot intelliot changed the base branch from next to develop February 23, 2023 05:37
@intelliot intelliot assigned RichardAH and unassigned ximinez Feb 23, 2023
@intelliot
Copy link
Collaborator

@RichardAH I merged develop into the branch. If you feel this PR is ready to merge, please put the Passed label on it.

(do not merge it yet)

@RichardAH RichardAH added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Feb 27, 2023
@intelliot intelliot added this to the 1.11.0 milestone Mar 1, 2023
@intelliot intelliot modified the milestones: 1.11.0, 1.10.1 Mar 15, 2023
@intelliot intelliot merged commit cb08f2b into XRPLF:develop Mar 15, 2023
ximinez added a commit to ximinez/rippled that referenced this pull request Mar 15, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into github-ci
4a9b075 
* upstream/develop:
  Rectify the import paths of boost/iterator: (XRPLF#4293)
  Allow port numbers be be specified with a colon: (XRPLF#4328)
  Use <=> operator for base_uint, Issue, and Book: (XRPLF#4411)
  Reporting Mode: Do not attempt to acquire missing data from peer network (XRPLF#4458)
ximinez added a commit to ximinez/rippled that referenced this pull request Mar 15, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into neverfeature
4d120b4 
* upstream/develop:
  Rectify the import paths of boost/iterator: (XRPLF#4293)
  Allow port numbers be be specified with a colon: (XRPLF#4328)
  Use <=> operator for base_uint, Issue, and Book: (XRPLF#4411)
  Reporting Mode: Do not attempt to acquire missing data from peer network (XRPLF#4458)
ximinez added a commit to ximinez/rippled that referenced this pull request Mar 15, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into ledgerinit
c913a56 
* upstream/develop:
  Rectify the import paths of boost/iterator: (XRPLF#4293)
  Allow port numbers be be specified with a colon: (XRPLF#4328)
  Use <=> operator for base_uint, Issue, and Book: (XRPLF#4411)
  Reporting Mode: Do not attempt to acquire missing data from peer network (XRPLF#4458)
ximinez added a commit to ximinez/rippled that referenced this pull request Mar 15, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into badmarker
d6883e6 
* upstream/develop:
  Rectify the import paths of boost/iterator: (XRPLF#4293)
  Allow port numbers be be specified with a colon: (XRPLF#4328)
  Use <=> operator for base_uint, Issue, and Book: (XRPLF#4411)
  Reporting Mode: Do not attempt to acquire missing data from peer network (XRPLF#4458)
ximinez added a commit to ximinez/rippled that referenced this pull request Mar 15, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into site-timeout
5df53c8 
* upstream/develop:
  Rectify the import paths of boost/iterator: (XRPLF#4293)
  Allow port numbers be be specified with a colon: (XRPLF#4328)
  Use <=> operator for base_uint, Issue, and Book: (XRPLF#4411)
  Reporting Mode: Do not attempt to acquire missing data from peer network (XRPLF#4458)
ximinez added a commit to ximinez/rippled that referenced this pull request Mar 15, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into gt--amm-core-fun…
2b511cf 
…ctionality

* upstream/develop:
  Rectify the import paths of boost/iterator: (XRPLF#4293)
  Allow port numbers be be specified with a colon: (XRPLF#4328)
  Use <=> operator for base_uint, Issue, and Book: (XRPLF#4411)
  Reporting Mode: Do not attempt to acquire missing data from peer network (XRPLF#4458)
ximinez added a commit to ximinez/rippled that referenced this pull request Mar 15, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into badmarker_rh--nf…
60db213 
…tpage

* upstream/develop:
  Rectify the import paths of boost/iterator: (XRPLF#4293)
  Allow port numbers be be specified with a colon: (XRPLF#4328)
  Use <=> operator for base_uint, Issue, and Book: (XRPLF#4411)
  Reporting Mode: Do not attempt to acquire missing data from peer network (XRPLF#4458)
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 nbougalis approved these changes

Assignees

@RichardAH RichardAH

Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Testable Will Need Documentation
Projects
Core Ledger
Archived in project
Milestone
Post-1.10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@RichardAH @intelliot @nbougalis @HowardHinnant @ximinez