-
Notifications
You must be signed in to change notification settings - Fork 138
Add probing service #815
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add probing service #815
Changes from all commits
0b0cd67
1a8f945
2431f88
36fdff3
a162ab9
5bd0bd1
935bd84
8acd55d
5e77f88
c68b8e3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ use std::convert::TryInto; | |
| use std::default::Default; | ||
| use std::net::ToSocketAddrs; | ||
| use std::path::PathBuf; | ||
| use std::sync::atomic::AtomicU64; | ||
| use std::sync::{Arc, Mutex, Once, RwLock}; | ||
| use std::time::SystemTime; | ||
| use std::{fmt, fs}; | ||
|
|
@@ -51,6 +52,7 @@ use crate::config::{ | |
| default_user_config, may_announce_channel, AnnounceError, AsyncPaymentsRole, | ||
| BitcoindRestClientConfig, Config, ElectrumSyncConfig, EsploraSyncConfig, HRNResolverConfig, | ||
| TorConfig, DEFAULT_ESPLORA_SERVER_URL, DEFAULT_LOG_FILENAME, DEFAULT_LOG_LEVEL, | ||
| DEFAULT_MAX_PROBE_AMOUNT_MSAT, DEFAULT_MIN_PROBE_AMOUNT_MSAT, | ||
| }; | ||
| use crate::connection::ConnectionManager; | ||
| use crate::entropy::NodeEntropy; | ||
|
|
@@ -77,6 +79,9 @@ use crate::logger::{log_error, LdkLogger, LogLevel, LogWriter, Logger}; | |
| use crate::message_handler::NodeCustomMessageHandler; | ||
| use crate::payment::asynchronous::om_mailbox::OnionMessageMailbox; | ||
| use crate::peer_store::PeerStore; | ||
| use crate::probing::{ | ||
| HighDegreeStrategy, Prober, ProbingConfig, ProbingStrategy, ProbingStrategyKind, RandomStrategy, | ||
| }; | ||
| use crate::runtime::{Runtime, RuntimeSpawner}; | ||
| use crate::tx_broadcaster::TransactionBroadcaster; | ||
| use crate::types::{ | ||
|
|
@@ -293,6 +298,7 @@ pub struct NodeBuilder { | |
| runtime_handle: Option<tokio::runtime::Handle>, | ||
| pathfinding_scores_sync_config: Option<PathfindingScoresSyncConfig>, | ||
| recovery_mode: bool, | ||
| probing_config: Option<ProbingConfig>, | ||
| } | ||
|
|
||
| impl NodeBuilder { | ||
|
|
@@ -311,16 +317,19 @@ impl NodeBuilder { | |
| let runtime_handle = None; | ||
| let pathfinding_scores_sync_config = None; | ||
| let recovery_mode = false; | ||
| let async_payments_role = None; | ||
| let probing_config = None; | ||
| Self { | ||
| config, | ||
| chain_data_source_config, | ||
| gossip_source_config, | ||
| liquidity_source_config, | ||
| log_writer_config, | ||
| runtime_handle, | ||
| async_payments_role: None, | ||
| async_payments_role, | ||
| pathfinding_scores_sync_config, | ||
| recovery_mode, | ||
| probing_config, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -626,6 +635,31 @@ impl NodeBuilder { | |
| self | ||
| } | ||
|
|
||
| /// Configures background probing. | ||
| /// | ||
| /// Use [`ProbingConfigBuilder`] to build the configuration: | ||
| /// ```no_run | ||
| /// # #[cfg(not(feature = "uniffi"))] | ||
| /// # { | ||
| /// use std::time::Duration; | ||
| /// use ldk_node::Builder; | ||
| /// use ldk_node::probing::ProbingConfigBuilder; | ||
| /// | ||
| /// let mut builder = Builder::new(); | ||
| /// builder.set_probing_config( | ||
| /// ProbingConfigBuilder::high_degree(100) | ||
| /// .interval(Duration::from_secs(30)) | ||
| /// .build() | ||
| /// ); | ||
| /// # } | ||
| /// ``` | ||
| /// | ||
| /// [`ProbingConfigBuilder`]: crate::probing::ProbingConfigBuilder | ||
| pub fn set_probing_config(&mut self, config: ProbingConfig) -> &mut Self { | ||
| self.probing_config = Some(config); | ||
| self | ||
| } | ||
|
|
||
| /// Builds a [`Node`] instance with a [`SqliteStore`] backend and according to the options | ||
| /// previously configured. | ||
| pub fn build(&self, node_entropy: NodeEntropy) -> Result<Node, BuildError> { | ||
|
|
@@ -797,6 +831,7 @@ impl NodeBuilder { | |
| self.gossip_source_config.as_ref(), | ||
| self.liquidity_source_config.as_ref(), | ||
| self.pathfinding_scores_sync_config.as_ref(), | ||
| self.probing_config.as_ref(), | ||
| self.async_payments_role, | ||
| self.recovery_mode, | ||
| seed_bytes, | ||
|
|
@@ -1097,6 +1132,13 @@ impl ArcedNodeBuilder { | |
| self.inner.write().expect("lock").set_wallet_recovery_mode(); | ||
| } | ||
|
|
||
| /// Configures background probing. | ||
| /// | ||
| /// See [`ProbingConfig`] for details. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please make sure the docs on the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it does so already. The builder methods do have (short) docs on them for both normal and arced builders. |
||
| pub fn set_probing_config(&self, config: Arc<ProbingConfig>) { | ||
| self.inner.write().expect("lock").set_probing_config((*config).clone()); | ||
| } | ||
|
|
||
| /// Builds a [`Node`] instance with a [`SqliteStore`] backend and according to the options | ||
| /// previously configured. | ||
| pub fn build(&self, node_entropy: Arc<NodeEntropy>) -> Result<Arc<Node>, BuildError> { | ||
|
|
@@ -1240,8 +1282,9 @@ fn build_with_store_internal( | |
| gossip_source_config: Option<&GossipSourceConfig>, | ||
| liquidity_source_config: Option<&LiquiditySourceConfig>, | ||
| pathfinding_scores_sync_config: Option<&PathfindingScoresSyncConfig>, | ||
| async_payments_role: Option<AsyncPaymentsRole>, recovery_mode: bool, seed_bytes: [u8; 64], | ||
| runtime: Arc<Runtime>, logger: Arc<Logger>, kv_store: Arc<DynStore>, | ||
| probing_config: Option<&ProbingConfig>, async_payments_role: Option<AsyncPaymentsRole>, | ||
| recovery_mode: bool, seed_bytes: [u8; 64], runtime: Arc<Runtime>, logger: Arc<Logger>, | ||
| kv_store: Arc<DynStore>, | ||
| ) -> Result<Node, BuildError> { | ||
| optionally_install_rustls_cryptoprovider(); | ||
|
|
||
|
|
@@ -1639,7 +1682,10 @@ fn build_with_store_internal( | |
| }, | ||
| } | ||
|
|
||
| let scoring_fee_params = ProbabilisticScoringFeeParameters::default(); | ||
| let mut scoring_fee_params = ProbabilisticScoringFeeParameters::default(); | ||
|
randomlogin marked this conversation as resolved.
|
||
| if let Some(penalty) = probing_config.and_then(|c| c.diversity_penalty_msat) { | ||
| scoring_fee_params.probing_diversity_penalty_msat = penalty; | ||
| } | ||
| let router = Arc::new(DefaultRouter::new( | ||
| Arc::clone(&network_graph), | ||
| Arc::clone(&logger), | ||
|
|
@@ -2019,6 +2065,40 @@ fn build_with_store_internal( | |
| _leak_checker.0.push(Arc::downgrade(&wallet) as Weak<dyn Any + Send + Sync>); | ||
| } | ||
|
|
||
| let prober = probing_config.map(|probing_cfg| { | ||
| let strategy: Arc<dyn ProbingStrategy> = match &probing_cfg.kind { | ||
| ProbingStrategyKind::HighDegree { top_node_count } => { | ||
| Arc::new(HighDegreeStrategy::new( | ||
| Arc::clone(&network_graph), | ||
| Arc::clone(&channel_manager), | ||
| Arc::clone(&router), | ||
| *top_node_count, | ||
| DEFAULT_MIN_PROBE_AMOUNT_MSAT, | ||
| DEFAULT_MAX_PROBE_AMOUNT_MSAT, | ||
| probing_cfg.cooldown, | ||
| config.probing_liquidity_limit_multiplier, | ||
| )) | ||
| }, | ||
| ProbingStrategyKind::Random { max_hops } => Arc::new(RandomStrategy::new( | ||
| Arc::clone(&network_graph), | ||
| Arc::clone(&channel_manager), | ||
| *max_hops, | ||
| DEFAULT_MIN_PROBE_AMOUNT_MSAT, | ||
| DEFAULT_MAX_PROBE_AMOUNT_MSAT, | ||
| )), | ||
| ProbingStrategyKind::Custom(s) => Arc::clone(s), | ||
| }; | ||
| Arc::new(Prober { | ||
| channel_manager: Arc::clone(&channel_manager), | ||
| logger: Arc::clone(&logger), | ||
| strategy, | ||
| interval: probing_cfg.interval, | ||
| max_locked_msat: probing_cfg.max_locked_msat, | ||
| locked_msat: Arc::new(AtomicU64::new(0)), | ||
| inflight_probes: Mutex::new(HashMap::new()), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, it seems accounting will still be off if we send out probes and then restart, as we'll re-init with an empty map that forgot about the previously-sent probes. Do we think that's acceptable or do we need to somehow persist this map? Seems like persistence would add some considerable complication on top? Thoughts?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Initially I wanted probing to be without persistence for the sake of simplicity, but now it seems it is really important, because if the node really goes out of the budget then likely this will occur after a restart and eventually it will drain the whole balance. The corner case is the situation when a probing node:
In that case the budget would be falsely shown as near-zero, whereas in fact there some probes sent and unresolved. If we restart node more rarely than the time needed for locked inflight probes to release, we're more or less are fine. So if a node restarts more rarely than once in two hours it should be okay (it would not drain the balance completely, but there could be time periods where the probing budget is in reality around twice as bigger as it should be). There is an option to add to the public API this: https://github.com/lightningdevkit/rust-lightning/blob/03ab73da95af34f3ae3d28d7e83383cfa0b04fc1/lightning/src/ln/channelmanager.rs#L6143 Also some calculations when when the budget limit hits: assume probing is done every 500ms, or equivalently it has frequency of That would mean during time All in all, I think it's worth adding some way of persistence, because for 'ordinary' nodes the cap seems to be easily reached if the settings (frequency, amount) is not tuned properly. |
||
| }) | ||
| }); | ||
|
|
||
| Ok(Node { | ||
| runtime, | ||
| stop_sender, | ||
|
|
@@ -2052,6 +2132,7 @@ fn build_with_store_internal( | |
| om_mailbox, | ||
| async_payments_role, | ||
| hrn_resolver, | ||
| prober, | ||
| #[cfg(cycle_tests)] | ||
| _leak_checker, | ||
| }) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,7 @@ use crate::payment::asynchronous::static_invoice_store::StaticInvoiceStore; | |
| use crate::payment::store::{ | ||
| PaymentDetails, PaymentDetailsUpdate, PaymentDirection, PaymentKind, PaymentStatus, | ||
| }; | ||
| use crate::probing::Prober; | ||
| use crate::runtime::Runtime; | ||
| use crate::types::{ | ||
| CustomTlvRecord, DynStore, KeysManager, OnionMessenger, PaymentStore, Sweeper, Wallet, | ||
|
|
@@ -509,12 +510,13 @@ where | |
| payment_store: Arc<PaymentStore>, | ||
| peer_store: Arc<PeerStore<L>>, | ||
| keys_manager: Arc<KeysManager>, | ||
| runtime: Arc<Runtime>, | ||
| logger: L, | ||
| config: Arc<Config>, | ||
| static_invoice_store: Option<StaticInvoiceStore>, | ||
| onion_messenger: Arc<OnionMessenger>, | ||
| om_mailbox: Option<Arc<OnionMessageMailbox>>, | ||
| prober: Option<Arc<Prober>>, | ||
| runtime: Arc<Runtime>, | ||
| logger: L, | ||
| config: Arc<Config>, | ||
| } | ||
|
|
||
| impl<L: Deref + Clone + Sync + Send + 'static> EventHandler<L> | ||
|
|
@@ -530,7 +532,7 @@ where | |
| payment_store: Arc<PaymentStore>, peer_store: Arc<PeerStore<L>>, | ||
| keys_manager: Arc<KeysManager>, static_invoice_store: Option<StaticInvoiceStore>, | ||
| onion_messenger: Arc<OnionMessenger>, om_mailbox: Option<Arc<OnionMessageMailbox>>, | ||
| runtime: Arc<Runtime>, logger: L, config: Arc<Config>, | ||
| prober: Option<Arc<Prober>>, runtime: Arc<Runtime>, logger: L, config: Arc<Config>, | ||
| ) -> Self { | ||
| Self { | ||
| event_queue, | ||
|
|
@@ -544,12 +546,13 @@ where | |
| payment_store, | ||
| peer_store, | ||
| keys_manager, | ||
| logger, | ||
| runtime, | ||
| config, | ||
| static_invoice_store, | ||
| onion_messenger, | ||
| om_mailbox, | ||
| prober, | ||
| runtime, | ||
| logger, | ||
| config, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1158,8 +1161,24 @@ where | |
|
|
||
| LdkEvent::PaymentPathSuccessful { .. } => {}, | ||
| LdkEvent::PaymentPathFailed { .. } => {}, | ||
| LdkEvent::ProbeSuccessful { .. } => {}, | ||
| LdkEvent::ProbeFailed { .. } => {}, | ||
| LdkEvent::ProbeSuccessful { path, payment_id, .. } => { | ||
| if let Some(prober) = &self.prober { | ||
| if let Some(amount) = | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe just move this check into
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved |
||
| prober.inflight_probes.lock().expect("lock").remove(&payment_id) | ||
| { | ||
| prober.handle_background_probe_successful(&path, amount); | ||
| } | ||
| } | ||
| }, | ||
| LdkEvent::ProbeFailed { path, payment_id, .. } => { | ||
| if let Some(prober) = &self.prober { | ||
| if let Some(amount) = | ||
| prober.inflight_probes.lock().expect("lock").remove(&payment_id) | ||
| { | ||
| prober.handle_background_probe_failed(&path, amount); | ||
| } | ||
| } | ||
| }, | ||
| LdkEvent::HTLCHandlingFailed { failure_type, .. } => { | ||
| if let Some(liquidity_source) = self.liquidity_source.as_ref() { | ||
| liquidity_source.handle_htlc_handling_failed(failure_type).await; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.