Conversation
|
👋 Thanks for assigning @kaloudis 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.
| pub struct HighDegreeStrategy { | ||
| network_graph: Arc<Graph>, | ||
| /// How many of the highest-degree nodes to cycle through. | ||
| pub top_n: usize, |
There was a problem hiding this comment.
Could top_n be renamed to num_top_nodes? The latter reads less generic to me but up to you to modify or not.
There was a problem hiding this comment.
I'd leave it as is (maybe top_k, as somehow it is more common in algorithms to describe the number of samplings).
What about top_node_count?
Personally I don't like 'num' as a short for 'number'
|
🔔 7th Reminder Hey @tnull! This PR has been waiting for your review. |
|
@enigbe, thanks for a review, the updates are incoming soon. |
randomlogin
force-pushed
the
add-probing-service
branch
3 times, most recently
from
436e4a3 to
07dfde4
Compare
Introduce a background probing service that periodically dispatches probes to improve the scorer's liquidity estimates. Includes two built-in strategies.
randomlogin
force-pushed
the
add-probing-service
branch
from
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.
| } | ||
|
|
||
| let scoring_fee_params = ProbabilisticScoringFeeParameters::default(); | ||
| let mut scoring_fee_params = ProbabilisticScoringFeeParameters::default(); |
There was a problem hiding this comment.
I do wonder if we should allow the user to set the entire ProbabilisticScoringFeeParameters and ProbabilisticScoringDecayParameters via the ProbingConfigBuilder mentioned above? Do you see any reason where that would conflict with other API design decisions?
There was a problem hiding this comment.
I think we should expose these settings (in NodeBuilder, not probing builder).
However these are for fine tuning and should be used only by advanced users. Also I would expose UserConfig, as for example user cannot decide what features do advertise.
I can add builder methods for scoring parameters, though maybe it should be in another PR aimed on exposing settings for an advanced user?
There was a problem hiding this comment.
Also I would expose UserConfig, as for example user cannot decide what features do advertise.
Yes, this is very intentional, as we want to provide a sane/safe API. For example letting user freely choose to set certain parameters if we don't implement them properly will just lead to a lot of footguns, in some circumstances even with the potential for money loss.
There was a problem hiding this comment.
In that case I think we don't need to expose ProbabilisticScoringFeeParameters and ProbabilisticScoringFeeParameters by the very reason you mentioned.
|
🔔 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.
Have yet to do another full pass. However, please start looking into how we can expose the adding probing APIs as part of the uniffi bindings in the meantime. We try to keep feature parity between Rust and bindings APIs as far as possible.
| /// **Warning:** This is expensive — O(scorer size) per call. It works by serializing the | ||
| /// entire `CombinedScorer` and deserializing it as a plain `ProbabilisticScorer` to access | ||
| /// `estimated_channel_liquidity_range`. Intended for testing and debugging, not hot paths. | ||
| pub fn scorer_channel_liquidity(&self, scid: u64, target: PublicKey) -> Option<(u64, u64)> { |
There was a problem hiding this comment.
I don't think we'll want to expose this here. If we think we need this we'd need to work out an API that is more reasonable than returning plain (u64, u64) (which are rather cryptic to begin with) and would also work in bindings. I also don't think the re-serializing of the scorer here is acceptable - if we need access to estimated_channel_liquidity_range, we probably need to work out how we can expose that in a reasonable way on CombinedScorer upstream
There was a problem hiding this comment.
Removed it from this PR, instead created a PR upstream: lightningdevkit/rust-lightning#4568
After some considerations I think it's indeed needed to be exposed, as it is needed to gather data about probing strategies to polish them.
Even despite the fact we cannot get the real liquidity data on live network to have 2 figures to compare (real vs estimate), it can be used in for example measuring the number of failed payments against the liquidity estimates we had on the path. Or to have a bunch of our own routing nodes with exact channel liquidity data (on signet) to compare with the estimations.
Also removed the test that naively compared strategy performances (and used this method).
There was a problem hiding this comment.
That PR now landed. You might need to wait for other PRs that fix pre-existing API breakages before you can use the API here. Until then, feel free to change the dependency to a branch of yours that depends on the same LDK base as our current main (but that includes the fix from LDK#4568), but note you'll also have to update bitcoin-payment-instructions to depend on the same revision accordingly.
There was a problem hiding this comment.
The test which I removed was more an diagnostic, not a correctness, whether the scorer gets its estimations, it didn't assert anything, so I'm not sure if I need to return it to the PR, should not be a blocker for now.
(still of course the upstream change is needed)
|
🔔 9th Reminder Hey @enigbe! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @enigbe! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @enigbe! This PR has been waiting for your review. |
randomlogin
force-pushed
the
add-probing-service
branch
from
8360b6e to
56b5b0f
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks for the update. It seems this is blocked now until we can bump the LDK dependency to include the upstream PR?
Strategy constructors (high_degree/random_walk/custom) moved from ProbingConfig to ProbingConfigBuilder, so they live on the builder rather than on the thing being built. ProbingConfigBuilder setters switched from consuming `self -> Self` to `&mut self -> &mut Self`, matching NodeBuilder. `build` now takes `&self`. Existing fluent call sites still compile unchanged. Removed the flat new_high_degree/new_random_walk UniFFI constructors on ProbingConfig that replicated the builder wiring. Bindings now go through ArcedProbingConfigBuilder (exposed as ProbingConfigBuilder via UDL), which wraps ProbingConfigBuilder in an RwLock for the Arc semantics UniFFI requires — mirroring ArcedNodeBuilder. AI-assisted (Claude Code).
randomlogin
force-pushed
the
add-probing-service
branch
from
18ff1db to
bd34ddf
Compare
randomlogin
force-pushed
the
add-probing-service
branch
from
bd34ddf to
a4c4928
Compare
Previously we always queried gossip data to construct probing route, which would fail for unannounced channels. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
5 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: