Add probing service#815
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tnull! This PR has been waiting for your review. |
There was a problem hiding this comment.
Hi @randomlogin, thanks for the work on this! I've reviewed the first two commits:
I've left a bunch of inline comments addressing configuration and public API, commit hygiene, testing infrastructure, and test flakiness.
In summary:
- A couple of items are exposed publicly that seem like they should be scoped to probing or gated for tests only (see
scoring_fee_paramsinConfigandscorer_channel_liquidityonNode). - The probing tests duplicate existing test helpers (
setup_node,MockLogFacadeLogger). Reusing and extending what's already intests/common/would reduce duplication and keep the test file focused on the tests themselves. test_probe_budget_blocks_when_node_offlinehas a race condition where the prober dispatches probes before the baseline capacity is measured, causing the assertion between the baseline and stuck capacities to fail. Details in the inline comment.- A few nits about commit hygiene, import structure, and suggestions for renaming stuff.
Also needs to be rebased.
|
🔔 7th Reminder Hey @tnull! This PR has been waiting for your review. |
|
@enigbe, thanks for a review, the updates are incoming soon. |
436e4a3 to
07dfde4
Compare
ff741c2 to
c31f1ce
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks for taking this on and excuse the delay here!
Did a first review pass and this already looks great! Here are some relatively minor comments, mostly concerning the API design.
|
🔔 4th Reminder Hey @enigbe! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Seems tests are failing right now:
thread 'exhausted_probe_budget_blocks_new_probes' (167312) panicked at tests/probing_tests.rs:381:5:
no probe dispatched within 15 s
failures:
exhausted_probe_budget_blocks_new_probes
probe_budget_increments_and_decrements
f99786b to
1e73e6e
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Looks good I think, but this needs a small rebase by now. We'll probably also want to wait until the backport of the upstream PR for accurate accounting landed.
In the meantime please fix the commit history, many fixups seem to have been squashed into the wrong commit. Maybe having two commits (add probing, add probing tests) would be easiest.
| log_debug!(prober.logger, "Skipping probe: locked-msat budget exceeded."); | ||
| continue; | ||
| } | ||
| match prober.channel_manager.send_probe(path.clone()) { |
There was a problem hiding this comment.
Codex:
- [P2] Skip one-hop paths before probing — /home/tnull/workspace/ldk-node/src/probing.rs:807-807
When a strategy returns a non-blinded path with fewer than two hops—for example, high-degree routing to a directly connected LSP or a random walk that dead-ends at the first hop—ChannelManager::send_probe always rejects it as unnecessary. In those configurations the background loop just
wakes up and logs send failures instead of dispatching probes, so filter these paths or have the strategies choose another target before calling send_probe.
There was a problem hiding this comment.
I'd leave current setup: as there can be genuine situations when strategy returns 1-hop path (for example we're connected to a dead end). On the current tick we have no choice but to do nothing, it is a legitimate failure.
On the level of strategy we can try to make this situations rare (added checks to both strategies, now they return None).
About logging/handling: we could move it to run_prober but it is essentially the same (perhaps without a logging):
tokio::select! {
biased;
_ = stop_rx.changed() => {
log_debug!(prober.logger, "Stopping background probing.");
return;
}
_ = ticker.tick() => {
let path = match prober.strategy.next_probe() {
Some(p) => p,
None => continue,
};
...
//something like this
if path.hops.len() < 2 {
continue;
}
...
| .nodes() | ||
| .unordered_iter() | ||
| .filter_map(|(id, info)| { | ||
| PublicKey::try_from(*id).ok().map(|pubkey| (pubkey, info.channels.len())) |
There was a problem hiding this comment.
Might be better to convert things to NodeId vs converting the entire graph's NodeIds to pubkeys here. The pubkey conversion stuff isn't free as it validates the points are on the curve.
bda5449 to
ceab99f
Compare
|
This needs a rebase, preferably after #971 landed which will include the pending LDK changes. |
|
#971 landed, so should be good to rebase. |
4cabf7a to
41aaba9
Compare
|
Rebased. Also recreated |
41aaba9 to
fdc63df
Compare
Introduce a background probing service that periodically sends payment probes to discover liquidity along Lightning routes. Probes update the local scorer with channel liquidity information, improving pathfinding for subsequent real payments. The service supports three strategies: - HighDegreeStrategy: probes nodes with the most channels in the network graph - RandomWalkStrategy: walks random paths from the local node - Custom: user-supplied strategy via the ProbingStrategy trait A dedicated ProbingConfigBuilder exposes amount bounds, locked-msat caps, probing intervals, and per-node cooldowns, with sensible defaults. The service runs as a cancellable background task driven by the existing Runtime, and budget accounting tracks both in-flight and locked amounts to bound outbound liquidity exposure. UniFFI bindings expose the probing service to the Swift, Kotlin, and Python language bindings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add integration tests that verify the probing service fires probes on the configured interval and respects the locked-msat budget cap. Shared helpers in tests/common are extended with probing-aware setup. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fdc63df to
f5fbf42
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Looks good, mod some minor nits/comments that can happen in a follow-up.
Will land this once CI passes (a bit more). Kicked it once more for now, unfortunately it's a bit flaky right now, so the failures so far seem unrelated.
| _leak_checker.0.push(Arc::downgrade(&wallet) as Weak<dyn Any + Send + Sync>); | ||
| } | ||
|
|
||
| let prober = probing_config.map(|probing_cfg| { |
There was a problem hiding this comment.
nit: Would be better to move this up before the leak checker setup.
| Some(p) => p, | ||
| None => continue, | ||
| }; | ||
| let amount: u64 = path.hops.iter().map(|h| h.fee_msat).sum(); |
There was a problem hiding this comment.
Codex:
Medium: /home/tnull/worktrees/ldk-node/pr-815/src/probing.rs:807 undercounts custom blinded probe paths. sum(hop.fee_msat) only equals total locked value for unblinded paths. A custom ProbingStrategy can return a blinded Path, where the blinded final value is excluded, so max_locked_msat
can be exceeded before locked_msat() catches up.
| break; | ||
| } | ||
|
|
||
| route_least_htlc_upper_bound = |
There was a problem hiding this comment.
Codex:
- Medium: /home/tnull/worktrees/ldk-node/pr-815/src/probing.rs:609 caps the random-walk delivered amount against each channel’s htlc_maximum_msat, but later adds downstream fees at /home/tnull/worktrees/ldk-node/pr-815/src/probing.rs:691 and only rechecks the first hop at /home/tnull/
worktrees/ldk-node/pr-815/src/probing.rs:714. Intermediate HTLCs can exceed advertised maxima; the fee math also uses unchecked u64 arithmetic on gossip-sourced fee fields.
|
|
||
| /// Verifies that `locked_msat` increases when a probe is dispatched and returns | ||
| /// to zero once the probe resolves (succeeds or fails). | ||
| #[tokio::test(flavor = "multi_thread")] |
There was a problem hiding this comment.
Codex:
- Test reliability: /home/tnull/worktrees/ldk-node/pr-815/tests/probing_tests.rs:124 is bind-sensitive. cargo test --test probing_tests failed twice with InvalidSocketAddress from /home/tnull/worktrees/ldk-node/pr-815/tests/common/mod.rs:739. The failed test passed when run alone, so this
looks like fixed-port test setup contention, but the added test target is not reliably green as-is.
Replace the hand-rolled run_prober loop with the probing service that landed on ldk-node master (lightningdevkit/ldk-node#815), backported onto the fedimint-0.7.0 fork base (joschisan/ldk-node branch probing-0.7.0, consumed as a pinned git dep until an upstream release ships it). The built-in Prober adds what the external loop could not: an in-flight locked-liquidity cap, per-node cooldowns, and a scorer diversity penalty that spreads successive probes across routes. The high-degree strategy probes the network's most-connected nodes instead of the curated destination list; probe.rs stays as the candidate target set for a future custom strategy. Defaults: 10s interval, top 100 hubs, 1k-100k sat amounts, 500k sats locked at most, 1h cooldown, 250 msat diversity penalty — tunable via FM_PROBE_TOP_NODE_COUNT, FM_PROBE_DIVERSITY_PENALTY_MSAT, FM_PROBE_MAX_LOCKED_MSAT and FM_PROBE_MIN/MAX_AMOUNT_MSAT, replacing FM_PROBE_INTERVAL_SECS and FM_PROBE_MAX_AMOUNT_SAT. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the hand-rolled run_prober loop with the probing service that landed on ldk-node master (lightningdevkit/ldk-node#815), backported onto the fedimint-0.7.0 fork base (joschisan/ldk-node branch probing-0.7.0, consumed as a pinned git dep until an upstream release ships it). The built-in Prober adds what the external loop could not: an in-flight locked-liquidity cap, per-node cooldowns, and a scorer diversity penalty that spreads successive probes across routes. The high-degree strategy probes the network's most-connected nodes instead of the curated destination list; probe.rs stays as the candidate target set for a future custom strategy. Defaults: 10s interval, top 100 hubs, 1k-100k sat amounts, 500k sats locked at most, 1h cooldown, 250 msat diversity penalty — tunable via FM_PROBE_TOP_NODE_COUNT, FM_PROBE_DIVERSITY_PENALTY_MSAT, FM_PROBE_MAX_LOCKED_MSAT and FM_PROBE_MIN/MAX_AMOUNT_MSAT, replacing FM_PROBE_INTERVAL_SECS and FM_PROBE_MAX_AMOUNT_SAT. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Everything the gatewaydv2 daemon needs from existing crates, kept additive so v1 is untouched at runtime: - fedimint-eventlog/client: per-operation event-log index and replay+tail subscription, bridged so the entry encoding is unchanged - fedimint-gwv2-client: daemon-driven send/receive primitives (start_receive, finalize_send with failure reason, per-operation event emission), richer payment events (preimage, realized ln fee, forfeit signature, destination node) and an optional daemon callback so the v2 daemon can orchestrate payments without the v1 state machines - fedimint-lightning: migrate the LDK client to ldk-node 0.7, pinned to a fork backporting upstream's probing service (lightningdevkit/ldk-node#815) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Everything the gatewaydv2 daemon needs from existing crates, kept additive so v1 is untouched at runtime: - fedimint-eventlog/client: per-operation event-log index and replay+tail subscription, bridged so the entry encoding is unchanged - fedimint-gwv2-client: daemon-driven send/receive primitives (start_receive, finalize_send with failure reason, per-operation event emission), richer payment events (preimage, realized ln fee, forfeit signature, destination node) and an optional daemon callback so the v2 daemon can orchestrate payments without the v1 state machines - fedimint-lightning: migrate the LDK client to ldk-node 0.7, pinned to a fork backporting upstream's probing service (lightningdevkit/ldk-node#815) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6 participants
Added a probing service which is used to send probes to estimate channels' capacities.
Related issue: #765.
Probing is intended to be used in two ways:
For probing a new abstraction
Proberis defined and is (optionally) created during node building.Prober periodically sends probes to feed the data to the scorer.
Prober sends probes using a ProbingStrategy.
ProbingStrategy trait has only one method:
fn next_probe(&self) -> Option<Probe>; every tick it generates a probe, whereProberepresents how to send a probe.To accommodate two different ways the probing is used, we either construct a probing route manually (
Probe::PrebuiltRoute) or rely on the router/scorer (Probe::Destination).Prober tracks how much liquidity is locked in-flight in probes, prevents the new probes from firing if the cap is reached.
There are two probing strategies implemented:
Random probing strategy, it picks a random route from the current node, the route is probed via
send_probe, thus ignores scoring parameters (what hops to pick), it also ignoresliquidity_limit_multiplierwhich prohibits taking a hop if its capacity is too small. It is a true random route.High degree probing strategy, it examines the graph and finds the nodes with the biggest number of (public) channels and probes routes to them using
send_spontaneous_preflight_probeswhich uses the current router/scorer.The former is meant to be used on payment nodes, while the latter on probing nodes. For the HighDegreeStrategy to work it is recommended to set
probing_diversity_penalty_msatto some nonzero value to prevent routes reuse, however it may fail to find any available routes.There are three tests added:
Example output (runs for ~1 minute, needs
--nocaptureflag):For performance testing I had to expose the scoring data (
scorer_channel_liquidity).Also exposed
scoring_fee_params: ProbabilisticScoringFeeParameterstoConfig.TODOs: