From 1c2b3c244e4273b6471b69e18d977298b4f60d44 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Mon, 1 Nov 2021 16:35:39 -0500 Subject: [PATCH 1/4] Clarify Scorer docs around penalizing channels --- lightning/src/routing/scorer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/routing/scorer.rs b/lightning/src/routing/scorer.rs index d2b167675e0..26fb05a72e9 100644 --- a/lightning/src/routing/scorer.rs +++ b/lightning/src/routing/scorer.rs @@ -60,7 +60,7 @@ use std::time::Instant; /// [`routing::Score`] implementation that provides reasonable default behavior. /// /// Used to apply a fixed penalty to each channel, thus avoiding long paths when shorter paths with -/// slightly higher fees are available. May also further penalize failed channels. +/// slightly higher fees are available. Will further penalize channels that fail to relay payments. /// /// See [module-level documentation] for usage. /// From 88cf9b33c3982376b22d34ddaa4a958bf0d11bfd Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Thu, 28 Oct 2021 23:23:45 -0500 Subject: [PATCH 2/4] Refactor channel failure penalty logic Move channel failure penalty logic into a ChannelFailure abstraction. This encapsulates the logic for accumulating penalties and decaying them over time. It also is responsible for the no-std behavior. This cleans up Scorer and will make it easier to serialize it. --- lightning/src/routing/scorer.rs | 104 ++++++++++++++++++-------------- 1 file changed, 58 insertions(+), 46 deletions(-) diff --git a/lightning/src/routing/scorer.rs b/lightning/src/routing/scorer.rs index 26fb05a72e9..9089a893200 100644 --- a/lightning/src/routing/scorer.rs +++ b/lightning/src/routing/scorer.rs @@ -52,7 +52,6 @@ use routing::network_graph::NodeId; use routing::router::RouteHop; use prelude::*; -#[cfg(not(feature = "no-std"))] use core::time::Duration; #[cfg(not(feature = "no-std"))] use std::time::Instant; @@ -67,10 +66,8 @@ use std::time::Instant; /// [module-level documentation]: crate::routing::scorer pub struct Scorer { params: ScoringParameters, - #[cfg(not(feature = "no-std"))] - channel_failures: HashMap, - #[cfg(feature = "no-std")] - channel_failures: HashMap, + // TODO: Remove entries of closed channels. + channel_failures: HashMap, } /// Parameters for configuring [`Scorer`]. @@ -78,18 +75,33 @@ pub struct ScoringParameters { /// A fixed penalty in msats to apply to each channel. pub base_penalty_msat: u64, - /// A penalty in msats to apply to a channel upon failure. + /// A penalty in msats to apply to a channel upon failing to relay a payment. /// - /// This may be reduced over time based on [`failure_penalty_half_life`]. + /// This accumulates for each failure but may be reduced over time based on + /// [`failure_penalty_half_life`]. /// /// [`failure_penalty_half_life`]: Self::failure_penalty_half_life pub failure_penalty_msat: u64, - /// The time needed before any accumulated channel failure penalties are cut in half. - #[cfg(not(feature = "no-std"))] + /// The time required to elapse before any accumulated [`failure_penalty_msat`] penalties are + /// cut in half. + /// + /// [`failure_penalty_msat`]: Self::failure_penalty_msat pub failure_penalty_half_life: Duration, } +/// Accounting for penalties against a channel for failing to relay any payments. +/// +/// Penalties decay over time, though accumulate as more failures occur. +struct ChannelFailure { + /// Accumulated penalty in msats for the channel as of `last_failed`. + undecayed_penalty_msat: u64, + + /// Last time the channel failed. Used to decay `undecayed_penalty_msat`. + #[cfg(not(feature = "no-std"))] + last_failed: Instant, +} + impl Scorer { /// Creates a new scorer using the given scoring parameters. pub fn new(params: ScoringParameters) -> Self { @@ -105,14 +117,41 @@ impl Scorer { Self::new(ScoringParameters { base_penalty_msat: penalty_msat, failure_penalty_msat: 0, - #[cfg(not(feature = "no-std"))] failure_penalty_half_life: Duration::from_secs(0), }) } +} - #[cfg(not(feature = "no-std"))] - fn decay_from(&self, penalty_msat: u64, last_failure: &Instant) -> u64 { - decay_from(penalty_msat, last_failure, self.params.failure_penalty_half_life) +impl ChannelFailure { + fn new(failure_penalty_msat: u64) -> Self { + Self { + undecayed_penalty_msat: failure_penalty_msat, + #[cfg(not(feature = "no-std"))] + last_failed: Instant::now(), + } + } + + fn add_penalty(&mut self, failure_penalty_msat: u64, half_life: Duration) { + self.undecayed_penalty_msat = self.decayed_penalty_msat(half_life) + failure_penalty_msat; + #[cfg(not(feature = "no-std"))] + { + self.last_failed = Instant::now(); + } + } + + fn decayed_penalty_msat(&self, half_life: Duration) -> u64 { + let decays = self.elapsed().as_secs().checked_div(half_life.as_secs()); + match decays { + Some(decays) => self.undecayed_penalty_msat >> decays, + None => 0, + } + } + + fn elapsed(&self) -> Duration { + #[cfg(not(feature = "no-std"))] + return self.last_failed.elapsed(); + #[cfg(feature = "no-std")] + return Duration::from_secs(0); } } @@ -127,7 +166,6 @@ impl Default for ScoringParameters { Self { base_penalty_msat: 500, failure_penalty_msat: 1024 * 1000, - #[cfg(not(feature = "no-std"))] failure_penalty_half_life: Duration::from_secs(3600), } } @@ -137,45 +175,19 @@ impl routing::Score for Scorer { fn channel_penalty_msat( &self, short_channel_id: u64, _source: &NodeId, _target: &NodeId ) -> u64 { - #[cfg(not(feature = "no-std"))] - let failure_penalty_msat = match self.channel_failures.get(&short_channel_id) { - Some((penalty_msat, last_failure)) => self.decay_from(*penalty_msat, last_failure), - None => 0, - }; - #[cfg(feature = "no-std")] - let failure_penalty_msat = - self.channel_failures.get(&short_channel_id).copied().unwrap_or(0); + let failure_penalty_msat = self.channel_failures + .get(&short_channel_id) + .map_or(0, |value| value.decayed_penalty_msat(self.params.failure_penalty_half_life)); self.params.base_penalty_msat + failure_penalty_msat } fn payment_path_failed(&mut self, _path: &Vec, short_channel_id: u64) { let failure_penalty_msat = self.params.failure_penalty_msat; - #[cfg(not(feature = "no-std"))] - { - let half_life = self.params.failure_penalty_half_life; - self.channel_failures - .entry(short_channel_id) - .and_modify(|(penalty_msat, last_failure)| { - let decayed_penalty = decay_from(*penalty_msat, last_failure, half_life); - *penalty_msat = decayed_penalty + failure_penalty_msat; - *last_failure = Instant::now(); - }) - .or_insert_with(|| (failure_penalty_msat, Instant::now())); - } - #[cfg(feature = "no-std")] + let half_life = self.params.failure_penalty_half_life; self.channel_failures .entry(short_channel_id) - .and_modify(|penalty_msat| *penalty_msat += failure_penalty_msat) - .or_insert(failure_penalty_msat); - } -} - -#[cfg(not(feature = "no-std"))] -fn decay_from(penalty_msat: u64, last_failure: &Instant, half_life: Duration) -> u64 { - let decays = last_failure.elapsed().as_secs().checked_div(half_life.as_secs()); - match decays { - Some(decays) => penalty_msat >> decays, - None => 0, + .and_modify(|failure| failure.add_penalty(failure_penalty_msat, half_life)) + .or_insert_with(|| ChannelFailure::new(failure_penalty_msat)); } } From a8d3b5aabf88487cb72dccd4553ba1894166fbb9 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 29 Oct 2021 08:52:27 -0500 Subject: [PATCH 3/4] Parameterize Scorer by a Time trait Scorer uses time to determine how much to penalize a channel after a failure occurs. Parameterizing it by time cleans up the code such that no-std support is in a single AlwaysPresent struct, which implements the Time trait. Time is implemented for std::time::Instant when std is available. This parameterization also allows for deterministic testing since a clock could be devised to advance forward as needed. --- lightning-background-processor/src/lib.rs | 3 +- lightning-invoice/src/utils.rs | 3 +- lightning/src/ln/channelmanager.rs | 8 +- lightning/src/ln/functional_test_utils.rs | 7 +- lightning/src/ln/functional_tests.rs | 9 +-- lightning/src/ln/shutdown_tests.rs | 3 +- lightning/src/routing/router.rs | 61 ++++++++-------- lightning/src/routing/scorer.rs | 89 ++++++++++++++++------- lightning/src/util/test_utils.rs | 4 + 9 files changed, 111 insertions(+), 76 deletions(-) diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs index 6a89f5e9968..50743774b7a 100644 --- a/lightning-background-processor/src/lib.rs +++ b/lightning-background-processor/src/lib.rs @@ -312,7 +312,6 @@ mod tests { use lightning::ln::features::InitFeatures; use lightning::ln::msgs::{ChannelMessageHandler, Init}; use lightning::ln::peer_handler::{PeerManager, MessageHandler, SocketDescriptor, IgnoringMessageHandler}; - use lightning::routing::scorer::Scorer; use lightning::routing::network_graph::{NetworkGraph, NetGraphMsgHandler}; use lightning::util::config::UserConfig; use lightning::util::events::{Event, MessageSendEventsProvider, MessageSendEvent}; @@ -635,7 +634,7 @@ mod tests { let data_dir = nodes[0].persister.get_data_dir(); let persister = move |node: &ChannelManager, Arc, Arc, Arc, Arc>| FilesystemPersister::persist_manager(data_dir.clone(), node); let router = DefaultRouter::new(Arc::clone(&nodes[0].network_graph), Arc::clone(&nodes[0].logger)); - let scorer = Arc::new(Mutex::new(Scorer::default())); + let scorer = Arc::new(Mutex::new(test_utils::TestScorer::default())); let invoice_payer = Arc::new(InvoicePayer::new(Arc::clone(&nodes[0].node), router, scorer, Arc::clone(&nodes[0].logger), |_: &_| {}, RetryAttempts(2))); let event_handler = Arc::clone(&invoice_payer); let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].net_graph_msg_handler.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone()); diff --git a/lightning-invoice/src/utils.rs b/lightning-invoice/src/utils.rs index d489cc69e3e..c7cd048bee0 100644 --- a/lightning-invoice/src/utils.rs +++ b/lightning-invoice/src/utils.rs @@ -157,7 +157,6 @@ mod test { use lightning::ln::features::InitFeatures; use lightning::ln::msgs::ChannelMessageHandler; use lightning::routing::router::{Payee, RouteParameters, find_route}; - use lightning::routing::scorer::Scorer; use lightning::util::events::MessageSendEventsProvider; use lightning::util::test_utils; #[test] @@ -183,7 +182,7 @@ mod test { let first_hops = nodes[0].node.list_usable_channels(); let network_graph = node_cfgs[0].network_graph; let logger = test_utils::TestLogger::new(); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let route = find_route( &nodes[0].node.get_our_node_id(), ¶ms, network_graph, Some(&first_hops.iter().collect::>()), &logger, &scorer, diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index f7441e0f1da..17a287c74c0 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -6087,9 +6087,9 @@ mod tests { use ln::msgs; use ln::msgs::ChannelMessageHandler; use routing::router::{Payee, RouteParameters, find_route}; - use routing::scorer::Scorer; use util::errors::APIError; use util::events::{Event, MessageSendEvent, MessageSendEventsProvider}; + use util::test_utils; #[cfg(feature = "std")] #[test] @@ -6325,7 +6325,7 @@ mod tests { let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); // To start (1), send a regular payment but don't claim it. let expected_route = [&nodes[1]]; @@ -6430,7 +6430,7 @@ mod tests { }; let network_graph = nodes[0].network_graph; let first_hops = nodes[0].node.list_usable_channels(); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let route = find_route( &payer_pubkey, ¶ms, network_graph, Some(&first_hops.iter().collect::>()), nodes[0].logger, &scorer @@ -6473,7 +6473,7 @@ mod tests { }; let network_graph = nodes[0].network_graph; let first_hops = nodes[0].node.list_usable_channels(); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let route = find_route( &payer_pubkey, ¶ms, network_graph, Some(&first_hops.iter().collect::>()), nodes[0].logger, &scorer diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index f605a63abd4..515fc79e8cf 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -17,7 +17,6 @@ use ln::{PaymentPreimage, PaymentHash, PaymentSecret}; use ln::channelmanager::{ChainParameters, ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailure, PaymentId}; use routing::network_graph::{NetGraphMsgHandler, NetworkGraph}; use routing::router::{Payee, Route, get_route}; -use routing::scorer::Scorer; use ln::features::{InitFeatures, InvoiceFeatures}; use ln::msgs; use ln::msgs::{ChannelMessageHandler,RoutingMessageHandler}; @@ -1016,7 +1015,7 @@ macro_rules! get_route_and_payment_hash { let payee = $crate::routing::router::Payee::new($recv_node.node.get_our_node_id()) .with_features($crate::ln::features::InvoiceFeatures::known()) .with_route_hints($last_hops); - let scorer = ::routing::scorer::Scorer::with_fixed_penalty(0); + let scorer = ::util::test_utils::TestScorer::with_fixed_penalty(0); let route = ::routing::router::get_route( &$send_node.node.get_our_node_id(), &payee, $send_node.network_graph, Some(&$send_node.node.list_usable_channels().iter().collect::>()), @@ -1353,7 +1352,7 @@ pub const TEST_FINAL_CLTV: u32 = 70; pub fn route_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) -> (PaymentPreimage, PaymentHash, PaymentSecret) { let payee = Payee::new(expected_route.last().unwrap().node.get_our_node_id()) .with_features(InvoiceFeatures::known()); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let route = get_route( &origin_node.node.get_our_node_id(), &payee, &origin_node.network_graph, Some(&origin_node.node.list_usable_channels().iter().collect::>()), @@ -1371,7 +1370,7 @@ pub fn route_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: pub fn route_over_limit<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) { let payee = Payee::new(expected_route.last().unwrap().node.get_our_node_id()) .with_features(InvoiceFeatures::known()); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let route = get_route(&origin_node.node.get_our_node_id(), &payee, origin_node.network_graph, None, recv_value, TEST_FINAL_CLTV, origin_node.logger, &scorer).unwrap(); assert_eq!(route.paths.len(), 1); assert_eq!(route.paths[0].len(), expected_route.len()); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 8c0870afaa6..1bf981680ba 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -25,7 +25,6 @@ use ln::{chan_utils, onion_utils}; use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT; use routing::network_graph::{NetworkUpdate, RoutingFees}; use routing::router::{Payee, Route, RouteHop, RouteHint, RouteHintHop, RouteParameters, find_route, get_route}; -use routing::scorer::Scorer; use ln::features::{ChannelFeatures, InitFeatures, InvoiceFeatures, NodeFeatures}; use ln::msgs; use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, ErrorAction}; @@ -7161,7 +7160,7 @@ fn test_check_htlc_underpaying() { // Create some initial channels create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let payee = Payee::new(nodes[1].node.get_our_node_id()).with_features(InvoiceFeatures::known()); let route = get_route(&nodes[0].node.get_our_node_id(), &payee, nodes[0].network_graph, None, 10_000, TEST_FINAL_CLTV, nodes[0].logger, &scorer).unwrap(); let (_, our_payment_hash, _) = get_payment_preimage_hash!(nodes[0]); @@ -7561,7 +7560,7 @@ fn test_bump_penalty_txn_on_revoked_htlcs() { let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1000000, 59000000, InitFeatures::known(), InitFeatures::known()); // Lock HTLC in both directions (using a slightly lower CLTV delay to provide timely RBF bumps) let payee = Payee::new(nodes[1].node.get_our_node_id()).with_features(InvoiceFeatures::known()); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let route = get_route(&nodes[0].node.get_our_node_id(), &payee, &nodes[0].network_graph, None, 3_000_000, 50, nodes[0].logger, &scorer).unwrap(); let payment_preimage = send_along_route(&nodes[0], route, &[&nodes[1]], 3_000_000).0; @@ -9061,7 +9060,7 @@ fn test_keysend_payments_to_public_node() { final_value_msat: 10000, final_cltv_expiry_delta: 40, }; - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let route = find_route(&payer_pubkey, ¶ms, network_graph, None, nodes[0].logger, &scorer).unwrap(); let test_preimage = PaymentPreimage([42; 32]); @@ -9095,7 +9094,7 @@ fn test_keysend_payments_to_private_node() { }; let network_graph = nodes[0].network_graph; let first_hops = nodes[0].node.list_usable_channels(); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let route = find_route( &payer_pubkey, ¶ms, network_graph, Some(&first_hops.iter().collect::>()), nodes[0].logger, &scorer diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs index b766cefd7b3..f72f7145980 100644 --- a/lightning/src/ln/shutdown_tests.rs +++ b/lightning/src/ln/shutdown_tests.rs @@ -15,7 +15,6 @@ use ln::{PaymentPreimage, PaymentHash}; use ln::channelmanager::PaymentSendFailure; use routing::router::{Payee, get_route}; use routing::network_graph::NetworkUpdate; -use routing::scorer::Scorer; use ln::features::{InitFeatures, InvoiceFeatures}; use ln::msgs; use ln::msgs::{ChannelMessageHandler, ErrorAction}; @@ -82,7 +81,7 @@ fn updates_shutdown_wait() { let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); let chan_2 = create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known()); let logger = test_utils::TestLogger::new(); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let (our_payment_preimage, our_payment_hash, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 100000); diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 93d3dc8f10a..633775bfe63 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1473,7 +1473,6 @@ mod tests { use routing; use routing::network_graph::{NetworkGraph, NetGraphMsgHandler, NodeId}; use routing::router::{get_route, Payee, Route, RouteHint, RouteHintHop, RouteHop, RoutingFees}; - use routing::scorer::Scorer; use chain::transaction::OutPoint; use ln::features::{ChannelFeatures, InitFeatures, InvoiceFeatures, NodeFeatures}; use ln::msgs::{ErrorAction, LightningError, OptionalField, UnsignedChannelAnnouncement, ChannelAnnouncement, RoutingMessageHandler, @@ -1943,7 +1942,7 @@ mod tests { let (secp_ctx, network_graph, _, _, logger) = build_graph(); let (_, our_id, _, nodes) = get_nodes(&secp_ctx); let payee = Payee::new(nodes[2]); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); // Simple route to 2 via 1 @@ -1974,7 +1973,7 @@ mod tests { let (secp_ctx, network_graph, _, _, logger) = build_graph(); let (_, our_id, _, nodes) = get_nodes(&secp_ctx); let payee = Payee::new(nodes[2]); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); // Simple route to 2 via 1 @@ -1993,7 +1992,7 @@ mod tests { let (secp_ctx, network_graph, net_graph_msg_handler, _, logger) = build_graph(); let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); let payee = Payee::new(nodes[2]); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); // Simple route to 2 via 1 @@ -2118,7 +2117,7 @@ mod tests { let (secp_ctx, network_graph, net_graph_msg_handler, _, logger) = build_graph(); let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); let payee = Payee::new(nodes[2]).with_features(InvoiceFeatures::known()); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); // A route to node#2 via two paths. // One path allows transferring 35-40 sats, another one also allows 35-40 sats. @@ -2254,7 +2253,7 @@ mod tests { let (secp_ctx, network_graph, net_graph_msg_handler, _, logger) = build_graph(); let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); let payee = Payee::new(nodes[2]); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); // // Disable channels 4 and 12 by flags=2 update_channel(&net_graph_msg_handler, &secp_ctx, &privkeys[1], UnsignedChannelUpdate { @@ -2312,7 +2311,7 @@ mod tests { let (secp_ctx, network_graph, net_graph_msg_handler, _, logger) = build_graph(); let (_, our_id, privkeys, nodes) = get_nodes(&secp_ctx); let payee = Payee::new(nodes[2]); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); // Disable nodes 1, 2, and 8 by requiring unknown feature bits let unknown_features = NodeFeatures::known().set_unknown_feature_required(); @@ -2353,7 +2352,7 @@ mod tests { fn our_chans_test() { let (secp_ctx, network_graph, _, _, logger) = build_graph(); let (_, our_id, _, nodes) = get_nodes(&secp_ctx); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); // Route to 1 via 2 and 3 because our channel to 1 is disabled let payee = Payee::new(nodes[0]); @@ -2482,7 +2481,7 @@ mod tests { fn partial_route_hint_test() { let (secp_ctx, network_graph, _, _, logger) = build_graph(); let (_, our_id, _, nodes) = get_nodes(&secp_ctx); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); // Simple test across 2, 3, 5, and 4 via a last_hop channel // Tests the behaviour when the RouteHint contains a suboptimal hop. @@ -2581,7 +2580,7 @@ mod tests { let (secp_ctx, network_graph, _, _, logger) = build_graph(); let (_, our_id, _, nodes) = get_nodes(&secp_ctx); let payee = Payee::new(nodes[6]).with_route_hints(empty_last_hop(&nodes)); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); // Test handling of an empty RouteHint passed in Invoice. @@ -2663,7 +2662,7 @@ mod tests { let (secp_ctx, network_graph, net_graph_msg_handler, _, logger) = build_graph(); let (_, our_id, privkeys, nodes) = get_nodes(&secp_ctx); let payee = Payee::new(nodes[6]).with_route_hints(multi_hint_last_hops(&nodes)); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); // Test through channels 2, 3, 5, 8. // Test shows that multiple hop hints are considered. @@ -2769,7 +2768,7 @@ mod tests { let (secp_ctx, network_graph, _, _, logger) = build_graph(); let (_, our_id, _, nodes) = get_nodes(&secp_ctx); let payee = Payee::new(nodes[6]).with_route_hints(last_hops_with_public_channel(&nodes)); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); // This test shows that public routes can be present in the invoice // which would be handled in the same manner. @@ -2818,7 +2817,7 @@ mod tests { fn our_chans_last_hop_connect_test() { let (secp_ctx, network_graph, _, _, logger) = build_graph(); let (_, our_id, _, nodes) = get_nodes(&secp_ctx); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); // Simple test with outbound channel to 4 to test that last_hops and first_hops connect let our_chans = vec![get_channel_details(Some(42), nodes[3].clone(), InitFeatures::from_le_bytes(vec![0b11]), 250_000_000)]; @@ -2939,7 +2938,7 @@ mod tests { }]); let payee = Payee::new(target_node_id).with_route_hints(vec![last_hops]); let our_chans = vec![get_channel_details(Some(42), middle_node_id, InitFeatures::from_le_bytes(vec![0b11]), outbound_capacity_msat)]; - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); get_route(&source_node_id, &payee, &NetworkGraph::new(genesis_block(Network::Testnet).header.block_hash()), Some(&our_chans.iter().collect::>()), route_val, 42, &test_utils::TestLogger::new(), &scorer) } @@ -2993,7 +2992,7 @@ mod tests { let (secp_ctx, network_graph, mut net_graph_msg_handler, chain_monitor, logger) = build_graph(); let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let payee = Payee::new(nodes[2]).with_features(InvoiceFeatures::known()); // We will use a simple single-path route from @@ -3265,7 +3264,7 @@ mod tests { // one of the latter hops is limited. let (secp_ctx, network_graph, net_graph_msg_handler, _, logger) = build_graph(); let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let payee = Payee::new(nodes[3]).with_features(InvoiceFeatures::known()); // Path via {node7, node2, node4} is channels {12, 13, 6, 11}. @@ -3388,7 +3387,7 @@ mod tests { fn ignore_fee_first_hop_test() { let (secp_ctx, network_graph, net_graph_msg_handler, _, logger) = build_graph(); let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let payee = Payee::new(nodes[2]); // Path via node0 is channels {1, 3}. Limit them to 100 and 50 sats (total limit 50). @@ -3434,7 +3433,7 @@ mod tests { fn simple_mpp_route_test() { let (secp_ctx, network_graph, net_graph_msg_handler, _, logger) = build_graph(); let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let payee = Payee::new(nodes[2]).with_features(InvoiceFeatures::known()); // We need a route consisting of 3 paths: @@ -3565,7 +3564,7 @@ mod tests { fn long_mpp_route_test() { let (secp_ctx, network_graph, net_graph_msg_handler, _, logger) = build_graph(); let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let payee = Payee::new(nodes[3]).with_features(InvoiceFeatures::known()); // We need a route consisting of 3 paths: @@ -3727,7 +3726,7 @@ mod tests { fn mpp_cheaper_route_test() { let (secp_ctx, network_graph, net_graph_msg_handler, _, logger) = build_graph(); let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let payee = Payee::new(nodes[3]).with_features(InvoiceFeatures::known()); // This test checks that if we have two cheaper paths and one more expensive path, @@ -3894,7 +3893,7 @@ mod tests { // if the fee is not properly accounted for, the behavior is different. let (secp_ctx, network_graph, net_graph_msg_handler, _, logger) = build_graph(); let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let payee = Payee::new(nodes[3]).with_features(InvoiceFeatures::known()); // We need a route consisting of 2 paths: @@ -4063,7 +4062,7 @@ mod tests { // path finding we realize that we found more capacity than we need. let (secp_ctx, network_graph, net_graph_msg_handler, _, logger) = build_graph(); let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let payee = Payee::new(nodes[2]).with_features(InvoiceFeatures::known()); // We need a route consisting of 3 paths: @@ -4220,7 +4219,7 @@ mod tests { let network_graph = Arc::new(NetworkGraph::new(genesis_block(Network::Testnet).header.block_hash())); let net_graph_msg_handler = NetGraphMsgHandler::new(Arc::clone(&network_graph), None, Arc::clone(&logger)); let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let payee = Payee::new(nodes[6]); add_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, &privkeys[1], ChannelFeatures::from_le_bytes(id_to_feature_flags(6)), 6); @@ -4349,7 +4348,7 @@ mod tests { // we calculated fees on a higher value, resulting in us ignoring such paths. let (secp_ctx, network_graph, net_graph_msg_handler, _, logger) = build_graph(); let (our_privkey, our_id, _, nodes) = get_nodes(&secp_ctx); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let payee = Payee::new(nodes[2]); // We modify the graph to set the htlc_maximum of channel 2 to below the value we wish to @@ -4411,7 +4410,7 @@ mod tests { // resulting in us thinking there is no possible path, even if other paths exist. let (secp_ctx, network_graph, net_graph_msg_handler, _, logger) = build_graph(); let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let payee = Payee::new(nodes[2]).with_features(InvoiceFeatures::known()); // We modify the graph to set the htlc_minimum of channel 2 and 4 as needed - channel 2 @@ -4478,7 +4477,7 @@ mod tests { let (_, our_id, _, nodes) = get_nodes(&secp_ctx); let logger = Arc::new(test_utils::TestLogger::new()); let network_graph = NetworkGraph::new(genesis_block(Network::Testnet).header.block_hash()); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let payee = Payee::new(nodes[0]).with_features(InvoiceFeatures::known()); { @@ -4519,7 +4518,7 @@ mod tests { let payee = Payee::new(nodes[6]).with_route_hints(last_hops(&nodes)); // Without penalizing each hop 100 msats, a longer path with lower fees is chosen. - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let route = get_route( &our_id, &payee, &network_graph, None, 100, 42, Arc::clone(&logger), &scorer @@ -4532,7 +4531,7 @@ mod tests { // Applying a 100 msat penalty to each hop results in taking channels 7 and 10 to nodes[6] // from nodes[2] rather than channel 6, 11, and 8, even though the longer path is cheaper. - let scorer = Scorer::with_fixed_penalty(100); + let scorer = test_utils::TestScorer::with_fixed_penalty(100); let route = get_route( &our_id, &payee, &network_graph, None, 100, 42, Arc::clone(&logger), &scorer @@ -4575,7 +4574,7 @@ mod tests { let payee = Payee::new(nodes[6]).with_route_hints(last_hops(&nodes)); // A path to nodes[6] exists when no penalties are applied to any channel. - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); let route = get_route( &our_id, &payee, &network_graph, None, 100, 42, Arc::clone(&logger), &scorer @@ -4704,7 +4703,7 @@ mod tests { }, }; let graph = NetworkGraph::read(&mut d).unwrap(); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); // First, get 100 (source, destination) pairs for which route-getting actually succeeds... let mut seed = random_init_seed() as usize; @@ -4735,7 +4734,7 @@ mod tests { }, }; let graph = NetworkGraph::read(&mut d).unwrap(); - let scorer = Scorer::with_fixed_penalty(0); + let scorer = test_utils::TestScorer::with_fixed_penalty(0); // First, get 100 (source, destination) pairs for which route-getting actually succeeds... let mut seed = random_init_seed() as usize; diff --git a/lightning/src/routing/scorer.rs b/lightning/src/routing/scorer.rs index 9089a893200..d9b8da2dce0 100644 --- a/lightning/src/routing/scorer.rs +++ b/lightning/src/routing/scorer.rs @@ -53,8 +53,6 @@ use routing::router::RouteHop; use prelude::*; use core::time::Duration; -#[cfg(not(feature = "no-std"))] -use std::time::Instant; /// [`routing::Score`] implementation that provides reasonable default behavior. /// @@ -64,10 +62,23 @@ use std::time::Instant; /// See [module-level documentation] for usage. /// /// [module-level documentation]: crate::routing::scorer -pub struct Scorer { +pub type Scorer = ScorerUsingTime::; + +/// Time used by [`Scorer`]. +#[cfg(not(feature = "no-std"))] +pub type DefaultTime = std::time::Instant; + +/// Time used by [`Scorer`]. +#[cfg(feature = "no-std")] +pub type DefaultTime = Eternity; + +/// [`routing::Score`] implementation parameterized by [`Time`]. +/// +/// See [`Scorer`] for details. +pub struct ScorerUsingTime { params: ScoringParameters, // TODO: Remove entries of closed channels. - channel_failures: HashMap, + channel_failures: HashMap>, } /// Parameters for configuring [`Scorer`]. @@ -86,6 +97,11 @@ pub struct ScoringParameters { /// The time required to elapse before any accumulated [`failure_penalty_msat`] penalties are /// cut in half. /// + /// # Note + /// + /// When time is an [`Eternity`], as is default when enabling feature `no-std`, it will never + /// elapse. Therefore, this penalty will never decay. + /// /// [`failure_penalty_msat`]: Self::failure_penalty_msat pub failure_penalty_half_life: Duration, } @@ -93,16 +109,24 @@ pub struct ScoringParameters { /// Accounting for penalties against a channel for failing to relay any payments. /// /// Penalties decay over time, though accumulate as more failures occur. -struct ChannelFailure { +struct ChannelFailure { /// Accumulated penalty in msats for the channel as of `last_failed`. undecayed_penalty_msat: u64, /// Last time the channel failed. Used to decay `undecayed_penalty_msat`. - #[cfg(not(feature = "no-std"))] - last_failed: Instant, + last_failed: T, } -impl Scorer { +/// A measurement of time. +pub trait Time { + /// Returns an instance corresponding to the current moment. + fn now() -> Self; + + /// Returns the amount of time elapsed since `self` was created. + fn elapsed(&self) -> Duration; +} + +impl ScorerUsingTime { /// Creates a new scorer using the given scoring parameters. pub fn new(params: ScoringParameters) -> Self { Self { @@ -122,42 +146,31 @@ impl Scorer { } } -impl ChannelFailure { +impl ChannelFailure { fn new(failure_penalty_msat: u64) -> Self { Self { undecayed_penalty_msat: failure_penalty_msat, - #[cfg(not(feature = "no-std"))] - last_failed: Instant::now(), + last_failed: T::now(), } } fn add_penalty(&mut self, failure_penalty_msat: u64, half_life: Duration) { self.undecayed_penalty_msat = self.decayed_penalty_msat(half_life) + failure_penalty_msat; - #[cfg(not(feature = "no-std"))] - { - self.last_failed = Instant::now(); - } + self.last_failed = T::now(); } fn decayed_penalty_msat(&self, half_life: Duration) -> u64 { - let decays = self.elapsed().as_secs().checked_div(half_life.as_secs()); + let decays = self.last_failed.elapsed().as_secs().checked_div(half_life.as_secs()); match decays { Some(decays) => self.undecayed_penalty_msat >> decays, None => 0, } } - - fn elapsed(&self) -> Duration { - #[cfg(not(feature = "no-std"))] - return self.last_failed.elapsed(); - #[cfg(feature = "no-std")] - return Duration::from_secs(0); - } } -impl Default for Scorer { +impl Default for ScorerUsingTime { fn default() -> Self { - Scorer::new(ScoringParameters::default()) + Self::new(ScoringParameters::default()) } } @@ -171,7 +184,7 @@ impl Default for ScoringParameters { } } -impl routing::Score for Scorer { +impl routing::Score for ScorerUsingTime { fn channel_penalty_msat( &self, short_channel_id: u64, _source: &NodeId, _target: &NodeId ) -> u64 { @@ -191,3 +204,27 @@ impl routing::Score for Scorer { .or_insert_with(|| ChannelFailure::new(failure_penalty_msat)); } } + +#[cfg(not(feature = "no-std"))] +impl Time for std::time::Instant { + fn now() -> Self { + std::time::Instant::now() + } + + fn elapsed(&self) -> Duration { + std::time::Instant::elapsed(self) + } +} + +/// A state in which time has no meaning. +pub struct Eternity; + +impl Time for Eternity { + fn now() -> Self { + Self + } + + fn elapsed(&self) -> Duration { + Duration::from_secs(0) + } +} diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 28f63e21ed9..4734a1cb459 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -21,6 +21,7 @@ use ln::features::{ChannelFeatures, InitFeatures}; use ln::msgs; use ln::msgs::OptionalField; use ln::script::ShutdownScript; +use routing::scorer::{Eternity, ScorerUsingTime}; use util::enforcing_trait_impls::{EnforcingSigner, EnforcementState}; use util::events; use util::logger::{Logger, Level, Record}; @@ -690,3 +691,6 @@ impl core::fmt::Debug for OnRegisterOutput { .finish() } } + +/// A scorer useful in testing, when the passage of time isn't a concern. +pub type TestScorer = ScorerUsingTime; From ae210e7d09758560b0e418b939222e7bc26ee009 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Thu, 28 Oct 2021 23:44:26 -0500 Subject: [PATCH 4/4] Implement (de)serialization for Scorer Scorer should be serialized to retain penalty data between restarts. Implement (de)serialization for Scorer by serializing last failure times as duration since the UNIX epoch. For no-std, the zero-Duration is used. --- lightning/src/routing/scorer.rs | 80 ++++++++++++++++++++++++++++++++- lightning/src/util/ser.rs | 17 +++++++ 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/lightning/src/routing/scorer.rs b/lightning/src/routing/scorer.rs index d9b8da2dce0..2aa0b4c145e 100644 --- a/lightning/src/routing/scorer.rs +++ b/lightning/src/routing/scorer.rs @@ -44,15 +44,25 @@ //! # } //! ``` //! +//! # Note +//! +//! If persisting [`Scorer`], it must be restored using the same [`Time`] parameterization. Using a +//! different type results in undefined behavior. Specifically, persisting when built with feature +//! `no-std` and restoring without it, or vice versa, uses different types and thus is undefined. +//! //! [`find_route`]: crate::routing::router::find_route use routing; +use ln::msgs::DecodeError; use routing::network_graph::NodeId; use routing::router::RouteHop; +use util::ser::{Readable, Writeable, Writer}; use prelude::*; +use core::ops::Sub; use core::time::Duration; +use io::{self, Read}; /// [`routing::Score`] implementation that provides reasonable default behavior. /// @@ -75,6 +85,10 @@ pub type DefaultTime = Eternity; /// [`routing::Score`] implementation parameterized by [`Time`]. /// /// See [`Scorer`] for details. +/// +/// # Note +/// +/// Mixing [`Time`] types between serialization and deserialization results in undefined behavior. pub struct ScorerUsingTime { params: ScoringParameters, // TODO: Remove entries of closed channels. @@ -106,6 +120,12 @@ pub struct ScoringParameters { pub failure_penalty_half_life: Duration, } +impl_writeable_tlv_based!(ScoringParameters, { + (0, base_penalty_msat, required), + (2, failure_penalty_msat, required), + (4, failure_penalty_half_life, required), +}); + /// Accounting for penalties against a channel for failing to relay any payments. /// /// Penalties decay over time, though accumulate as more failures occur. @@ -118,12 +138,17 @@ struct ChannelFailure { } /// A measurement of time. -pub trait Time { +pub trait Time: Sub where Self: Sized { /// Returns an instance corresponding to the current moment. fn now() -> Self; /// Returns the amount of time elapsed since `self` was created. fn elapsed(&self) -> Duration; + + /// Returns the amount of time passed since the beginning of [`Time`]. + /// + /// Used during (de-)serialization. + fn duration_since_epoch() -> Duration; } impl ScorerUsingTime { @@ -211,6 +236,11 @@ impl Time for std::time::Instant { std::time::Instant::now() } + fn duration_since_epoch() -> Duration { + use std::time::SystemTime; + SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap() + } + fn elapsed(&self) -> Duration { std::time::Instant::elapsed(self) } @@ -224,7 +254,55 @@ impl Time for Eternity { Self } + fn duration_since_epoch() -> Duration { + Duration::from_secs(0) + } + fn elapsed(&self) -> Duration { Duration::from_secs(0) } } + +impl Sub for Eternity { + type Output = Self; + + fn sub(self, _other: Duration) -> Self { + self + } +} + +impl Writeable for ScorerUsingTime { + #[inline] + fn write(&self, w: &mut W) -> Result<(), io::Error> { + self.params.write(w)?; + self.channel_failures.write(w) + } +} + +impl Readable for ScorerUsingTime { + #[inline] + fn read(r: &mut R) -> Result { + Ok(Self { + params: Readable::read(r)?, + channel_failures: Readable::read(r)?, + }) + } +} + +impl Writeable for ChannelFailure { + #[inline] + fn write(&self, w: &mut W) -> Result<(), io::Error> { + self.undecayed_penalty_msat.write(w)?; + (T::duration_since_epoch() - self.last_failed.elapsed()).write(w) + } +} + +impl Readable for ChannelFailure { + #[inline] + fn read(r: &mut R) -> Result { + Ok(Self { + undecayed_penalty_msat: Readable::read(r)?, + last_failed: T::now() - (T::duration_since_epoch() - Readable::read(r)?), + }) + } +} diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index ba8a9fd0d35..8fb72a8f9fe 100644 --- a/lightning/src/util/ser.rs +++ b/lightning/src/util/ser.rs @@ -27,6 +27,7 @@ use bitcoin::consensus::Encodable; use bitcoin::hashes::sha256d::Hash as Sha256dHash; use bitcoin::hash_types::{Txid, BlockHash}; use core::marker::Sized; +use core::time::Duration; use ln::msgs::DecodeError; use ln::{PaymentPreimage, PaymentHash, PaymentSecret}; @@ -911,3 +912,19 @@ impl Readable for String { Ok(ret) } } + +impl Writeable for Duration { + #[inline] + fn write(&self, w: &mut W) -> Result<(), io::Error> { + self.as_secs().write(w)?; + self.subsec_nanos().write(w) + } +} +impl Readable for Duration { + #[inline] + fn read(r: &mut R) -> Result { + let secs = Readable::read(r)?; + let nanos = Readable::read(r)?; + Ok(Duration::new(secs, nanos)) + } +}