Skip to content

Add probe path selection for background liquidity probing#4664

Open
151henry151 wants to merge 3 commits into
lightningdevkit:mainfrom
151henry151:probe-path-selection-2720
Open

Add probe path selection for background liquidity probing#4664
151henry151 wants to merge 3 commits into
lightningdevkit:mainfrom
151henry151:probe-path-selection-2720

Conversation

@151henry151

@151henry151 151henry151 commented Jun 6, 2026

Copy link
Copy Markdown

PR description updated (2026-06-08) to match review feedback in b17afa6b4.

Issue #2720 originally suggested a wrapper ScoreLookUp around ProbabilisticScorer. #3422 already added probing_diversity_penalty_msat for the same purpose; this PR wires that field into background probe path selection without a redundant wrapper or separate router API.

Background prober loops (random target/amount on a timer) remain userland / separate-crate per maintainer consensus.

This adds RouteParameters::from_probe_target (uncapped routing fees and a runtime-only background_probe flag), ProbabilisticScoringFeeParameters::for_probing(amount_msat) (sets a diversity penalty when none is configured), and ScoreLookUp::params_for_probe (ProbabilisticScorer calls for_probing; DefaultRouter::find_route calls it automatically when background_probe is set). Background probes route through the existing find_route path — there is no separate find_probe_route on Router. ChannelManager::send_probe_to_node is the simple find-and-send entry point. Manual callers using the free find_route should call scorer.params_for_probe(...) themselves. Preflight probe sending shares an internal trim_unannounced_probe_last_hops helper; behavior is unchanged. Router and functional tests verify probe path diversity across repeated probes and end-to-end probe sending.

For ongoing probing, use send_probe_to_node with a DefaultRouter — diversity scoring is applied per call via params_for_probe. Alternatively, set probing_diversity_penalty_msat on the router at construction for fixed probe amounts. This is intentionally different from send_spontaneous_preflight_probes, which probes all MPP paths with payment fee caps and tries to avoid saturating liquidity immediately before the payment is sent.

Fixes #2720

Tested with:
cargo +1.75.0 test -p lightning --lib probing — passed
cargo +1.75.0 test -p lightning --lib find_probe_route — passed
cargo +1.75.0 test -p lightning --lib for_probing — passed
cargo +1.75.0 test -p lightning --lib preflight_probe — passed
cargo +1.75.0 test -p lightning --lib router::tests — passed
cargo +1.75.0 doc -p lightning --no-deps — passed
RUSTFLAGS="--cfg=c_bindings" cargo +1.75.0 check -p lightning — passed
cargo +1.75.0 fmt --all — passed

Expose probe-tuned route finding via find_probe_route, Router::find_probe_route,
and ChannelManager::send_probe_to_node. Wire probing_diversity_penalty_msat through
ProbabilisticScoringFeeParameters::for_probing and DefaultRouter::find_probe_route_with_diversity.
@ldk-reviews-bot

ldk-reviews-bot commented Jun 6, 2026

Copy link
Copy Markdown

I've assigned @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Comment thread lightning/src/routing/router.rs Outdated
Comment thread lightning/src/routing/scoring.rs
Comment thread lightning/src/ln/probing_tests.rs Outdated
@ldk-reviews-bot ldk-reviews-bot requested a review from wpaulino June 7, 2026 00:07
@ldk-claude-review-bot

ldk-claude-review-bot commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

I've completed a full re-review of the refactored PR. The architecture changed significantly since my prior pass (now using ScoreLookUp::params_for_probe rather than the removed find_probe_route_with_diversity function), so I re-examined all the new code paths.

Review Summary

No new issues found.

Verification of changes:

  • lightning/src/ln/channelmanager.rstrim_unannounced_probe_last_hops is a behavior-preserving extraction of the prior inline trim logic (final-value accumulation order preserved). send_probe_to_node correctly routes, guards against empty paths, trims, and sends.
  • lightning/src/routing/router.rsDefaultRouter::find_route branch on background_probe is correct; the two read_lock() calls are sequential (the first guard is dropped at the end of the let statement before the second is acquired), so there is no deadlock or reentrancy hazard. get_route clearing background_probe before storing in the returned Route correctly keeps the runtime-only flag out of persistence, matching Readable.
  • lightning/src/routing/scoring.rsparams_for_probe delegation chain (ProbabilisticScorerfor_probing, CombinedScorer/ScorerAccountingForInFlightHtlcs/Deref/MultiThreadedScoreLockRead → forward) has no recursion. The divisor concern from my prior pass is resolved: base amount uses 1<<30 (= BASE_AMOUNT_PENALTY_DIVISOR, confirmed at scoring.rs:1756) and historical amount uses AMOUNT_PENALTY_DIVISOR (= 1<<20), both matching channel_penalty_msat.

Prior issues — all confirmed resolved:

  • Divisor/comment issue (scoring.rs) — fixed; uses AMOUNT_PENALTY_DIVISOR with accurate derivation comment.
  • Doc bug (find_probe_route_with_diversity) — obsolete; that function was removed in the refactor.
  • Shallow probing test — send_probe_to_node_happy_path now exercises the full lifecycle via send_probe_along_route + expect_probe_successful_events.

Minor cross-cutting observations (not blocking):

  • DefaultRouter's Router impl gained a new SP: Clone bound (router.rs:103). This is technically a minor API tightening for any downstream DefaultRouter instantiated with a non-Clone score-params type, though ProbabilisticScoringFeeParameters is Clone so the common case is unaffected.
  • for_probing's heuristic diversity penalty sums base_penalty_msat, historical_liquidity_penalty_multiplier_msat, and the two amount components, but omits the (default-dominant) liquidity_penalty_multiplier_msat. This is a judgment call rather than a bug; just flagging that the resulting penalty can be considerably smaller than the dominant per-channel routing penalty, which may limit diversification on some topologies.

Fix find_probe_route_with_diversity docs to recommend send_probe for
pre-computed routes rather than send_probe_to_node. Correct for_probing
historical amount divisor to AMOUNT_PENALTY_DIVISOR. Extend
send_probe_to_node_happy_path to complete the probe lifecycle.
@151henry151

151henry151 commented Jun 7, 2026

Copy link
Copy Markdown
Author

Addressed the bot feedback in cbd9c04: corrected the find_probe_route_with_diversity docs so pre-computed routes go through send_probe rather than send_probe_to_node, fixed the for_probing historical-amount divisor to AMOUNT_PENALTY_DIVISOR (the earlier 1 << 31 ignored the /2048 cancellation in combined_penalty_msat), and extended send_probe_to_node_happy_path to run the full probe lifecycle.

Is the norm here is to squash this kind of follow-up into one commit or keep the two-commit history?

Comment thread lightning/src/routing/router.rs Outdated
Comment thread lightning/src/ln/channelmanager.rs Outdated
Comment thread lightning/src/routing/router.rs Outdated
Move probe score-param tuning into ScoreLookUp::params_for_probe;
remove Router::find_probe_route, the free find_probe_route fn, and
DefaultRouter::find_probe_route_with_diversity. Background probes now
route through find_route with RouteParameters::from_probe_target;
DefaultRouter calls params_for_probe automatically when background_probe
is set. Remove new rustfmt::skips and fix "liquidity guard" wording.
@151henry151

151henry151 commented Jun 8, 2026

Copy link
Copy Markdown
Author

Addressed all three comments in the latest commit (b17afa6).

Moved probe score-param tuning to ScoreLookUp::params_for_probeProbabilisticScorer overrides it to call the existing for_probing; the Deref blanket impl and ScorerAccountingForInFlightHtlcs forward it; other scorers get the default (clone unchanged). Removed Router::find_probe_route, the free find_probe_route fn, and DefaultRouter::find_probe_route_with_diversity. send_probe_to_node now calls find_route with RouteParameters::from_probe_target (sets a runtime-only background_probe flag); DefaultRouter::find_route calls params_for_probe when that flag is set. Fixed “liquidity guard” wording and removed the new rustfmt::skips.

Squash commits, or keep history?

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

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.

add probing code

4 participants