Conversation
|
👋 Thanks for assigning @enigbe 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.
src/probing.rs
Outdated
| 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.
| pathfinding_scores_sync_config: Option<PathfindingScoresSyncConfig>, | ||
| recovery_mode: bool, | ||
| probing_strategy: Option<ProbingStrategyConfig>, | ||
| probing_diversity_penalty_msat: Option<u64>, |
There was a problem hiding this comment.
Can we track this field as part of ProbingStrategyConfig?
| /// Configures background probing toward the highest-degree nodes in the network graph. | ||
| /// | ||
| /// `top_n` controls how many of the most-connected nodes are cycled through. | ||
| #[cfg_attr(feature = "uniffi", allow(dead_code))] |
There was a problem hiding this comment.
Rather than adding all these on NodeBuilder, it might be preferable to create a dedicated builder in src/probing.rs and then only have the output of that (ProbingStrategyConfig? Or something similar?) settable on Node.
| } | ||
|
|
||
| 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?
| /// | ||
| /// On each tick: | ||
| /// 1. Picks one of our confirmed, usable channels to start from. | ||
| /// 2. Performs a deterministic walk of a randomly chosen depth (up to |
There was a problem hiding this comment.
Doing a random walk is a great idea! Do you think it would be a worthwhile extension (or another strategy idea) to use some kind of weighted random walk, e.g. to favor channels of certain criteria (smaller/larger)?
Maybe a bounded breadth first search strategy could also make sense, which would allow the peer to explore it's closer vincinity?
|
|
||
| // Track the tightest HTLC limit across all hops to cap the probe amount. | ||
| // The first hop limit comes from our live channel state; subsequent hops use htlc_maximum_msat from the gossip channel update. | ||
| let mut route_least_htlc_upper_bound = first_hop.next_outbound_htlc_limit_msat; |
There was a problem hiding this comment.
Do we also need a lower bound?
| /// Like [`open_channel`] but skips the `wait_for_tx` electrum check so that | ||
| /// multiple channels can be opened back-to-back before any blocks are mined. | ||
| /// The caller is responsible for mining blocks and confirming the funding txs. | ||
| pub async fn open_channel_no_electrum_wait( |
There was a problem hiding this comment.
Hmm, if we want this, could we call this open_channel_no_wait or similar and then have open_channel call into open_channel_no_wait to DRY up the functionality. Note that this helper is also a bit risky to introduce as currently we do not have UTXO locking, i.e., if this is used to open multiple subsequent channels for the same node it will double-spend itself.
| } | ||
|
|
||
| // helpers | ||
| async fn wait_until(timeout: Duration, predicate: impl Fn() -> bool) -> bool { |
There was a problem hiding this comment.
I think we should look to use tokio::time::timeout instead?
4 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: