Skip to content

Conversation

@Tapanito
Copy link
Collaborator

@Tapanito Tapanito commented Feb 21, 2025

High Level Overview of Change

Context of Change

Adds a new config stanza allow_private_endpoints. The new config option allows servers to accept and advertise open slots for servers with private IP addresses.

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)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

@codecov
Copy link

codecov bot commented Feb 21, 2025

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.1%. Comparing base (c17676a) to head (b4fb904).
⚠️ Report is 111 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/peerfinder/detail/PeerfinderConfig.cpp 66.7% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #5309   +/-   ##
=======================================
  Coverage     78.1%   78.1%           
=======================================
  Files          790     790           
  Lines        67907   67916    +9     
  Branches      8225    8218    -7     
=======================================
+ Hits         53032   53056   +24     
+ Misses       14875   14860   -15     
Files with missing lines Coverage Δ
src/xrpld/core/Config.h 85.7% <ø> (ø)
src/xrpld/core/ConfigSections.h 100.0% <ø> (ø)
src/xrpld/core/detail/Config.cpp 75.3% <100.0%> (+0.1%) ⬆️
src/xrpld/peerfinder/PeerfinderManager.h 50.0% <ø> (ø)
src/xrpld/peerfinder/detail/Logic.h 55.1% <100.0%> (+5.1%) ⬆️
src/xrpld/peerfinder/detail/PeerfinderConfig.cpp 82.8% <66.7%> (-0.9%) ⬇️

... and 2 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vlntb vlntb self-requested a review February 28, 2025 15:04
@vlntb
Copy link
Collaborator

vlntb commented Mar 12, 2025

Some commits are not signed. You need to rebase and sign every commit.

run("new in 100/out 10", {}, 100, 10, 4000, 10, 100, 6);
run("new in 0/out 10", {}, 0, 10, 4000, 10, 0, 1);
run("new in 100/out 10, private", {}, 100, 10, 0, 10, 0, 6);
auto testcases = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good job re-writing that!
The code is much easier to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!

@vlntb vlntb requested a review from gregtatcam March 12, 2025 16:58
if (!is_valid_address(ep.address))
// Discard invalid addresses and if we don't support private IPs in
// Endpoint message
if (!is_valid_address(ep.address) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit]: Consider breaking this into two checks to have two separate debug messages: one for invalid address and the other for !config_.allowPrivateEndpoints && !is_public(ep.address) case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

Copy link
Collaborator

@vlntb vlntb left a comment

Choose a reason for hiding this comment

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

LGTM.
Minor suggestions.

adds allow_private_endpoints configuration

formatting issues

clang formatting

one more attempt to please clang
@Tapanito Tapanito force-pushed the feature/private-network branch from 88120e7 to e061b43 Compare March 13, 2025 14:44
Copy link
Collaborator

@vlntb vlntb left a comment

Choose a reason for hiding this comment

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

LGTM

@kennyzlei kennyzlei removed the request for review from gregtatcam May 27, 2025 18:32
@Tapanito Tapanito changed the title New allow_private_endpoints stanza draft: New allow_private_endpoints stanza May 28, 2025
@Tapanito Tapanito closed this Jul 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants