fix: Expand the pathing tables and lower weight on sabbxd, also return up to 6 paths - #6507
fix: Expand the pathing tables and lower weight on sabbxd, also return up to 6 paths#6507shortthefomo wants to merge 32 commits into
Conversation
…P tends to limit it and it is missing many viable options
shortthefomo
changed the title
…hefomo/rippled into pathing-fix-nonXRP-to-XRP
…th which can be appended
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6507 +/- ##
=======================================
Coverage 79.8% 79.9%
=======================================
Files 861 878 +17
Lines 67857 67889 +32
Branches 7553 7537 -16
=======================================
+ Hits 54169 54223 +54
+ Misses 13688 13666 -22
... and 55 files with indirect coverage changes 🚀 New features to boost your workflow:
|
shortthefomo
changed the title
There was a problem hiding this comment.
Pull request overview
This PR aims to improve pathfinding results by (1) expanding the nonXRP→XRP path template table used by Pathfinder::initPathTable() and (2) increasing the maximum number of paths returned in ripple_path_find responses (aligned with Payment’s accepted path limit).
Changes:
- Extends
pt_nonXRP_to_XRPtemplates to explore deeper book-chains. - Increases
PathRequest’s path cap used for ranking/selection to allow up to 6 paths in responses when the optionalfullLiquidityPathis appended.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/xrpld/app/paths/Pathfinder.cpp |
Modifies the nonXRP→XRP path template list and associated search-level costs. |
src/xrpld/app/paths/PathRequest.h |
Adjusts the internal max-paths cap used to compute/select paths for pathfind responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
…to produce any usable results
shortthefomo
changed the title
|
/ai-review |
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
…th which can be appended
…to produce any usable results
c76994e to
630fa6b
Compare
4ee8bca to
936a694
Compare
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
We recently merged a refactor to One-time setupIf you don't already have clang-tidy working in your env, on macOS: brew install llvm@21
# Follow brew's hint to put $(brew --prefix llvm@21)/bin on PATH so run-clang-tidy is found.Workflow on your branch (before merging develop)1. Grab the new git remote -v # should show 'upstream' among others; if not:
# git remote set-url upstream git@github.com:XRPLF/rippled.git
git fetch upstream
git checkout upstream/develop -- .clang-tidy2. Reconfigure conan/cmake so 3. Apply renames for the files modified in your PR: git diff --name-only $(git merge-base HEAD upstream/develop) HEAD \
| grep -E '\.(cpp|h|hpp|ipp)$' \
| xargs run-clang-tidy -p build -fix -allow-no-checks
# or -p .build, or whatever your build dir is called4. Build + test, then commit as a single dedicated commit: cmake --build build -j8
git commit -am "refactor: Align identifier naming with develop"5. Now merge develop: git merge upstream/developExtraRun clang-tidy once more after the merge to catch any stragglers introduced from develop's side: run-clang-tidy -p build -fix -allow-no-checks src tests
# or -p .build, or whatever your build dir is called |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
4c0b0a0 to
1c52832
Compare
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
|
no longer needed with #7392 |
Expanded the pt_nonXRP_to_XRP template set by adding 1 new route pattern:
sabbxd (cost 8)
Adjust sabbxd to a lower weight as it is needed for valid result set past X -> XRP
sabxd (cost 5)
High Level Overview of Change
This change updates
Pathfinder::initPathTable()to expand nonXRP→XRP path exploration by adding three additional non-default template patterns inpt_nonXRP_to_XRP:sabxdsabbxdThese templates add deeper order-book traversal options for nonXRP→XRP pathfinding when shorter templates do not produce fillable routes.
PRE PATCH result
POST PATCH result
As well max_paths_ ceiling was increased to 5 so that a maximum of 6 paths can be returned as that's the limit payments accept. There is little to no performance change for this.
Context of Change
We observed asymmetric route discovery between forward and inverse directions in some markets, where nonXRP→XRP requests could miss viable liquidity that existed behind deeper book chains.
The existing table-driven architecture in
initPathTable()is cost-gated by search depth, so the least invasive fix was to add targeted path templates rather than changing core ranking or liquidity evaluation logic. This keeps behavior predictable while broadening candidate generation.Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Before / After
Before:
sxd,saxd,saaxd,sbxd,sabxd,sabaxd).After:
{5, "sabxd"}{8, "sabbxd"}