Merge pull request #1388 from lightning-signer/2022-03-grind
authorArik Sosman <arik-so@users.noreply.github.com>
Fri, 25 Mar 2022 23:35:21 +0000 (16:35 -0700)
committerGitHub <noreply@github.com>
Fri, 25 Mar 2022 23:35:21 +0000 (16:35 -0700)
lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_tests.rs
lightning/src/routing/network_graph.rs
lightning/src/routing/scoring.rs
lightning/src/util/events.rs

index 0b0e092468d008c8c245a2c90198b2858ee7eebd..171a41dfa26f02478aaa3c94f6225863cc4491de 100644 (file)
@@ -4745,7 +4745,7 @@ impl<Signer: Sign> Channel<Signer> {
        /// should be sent back to the counterparty node.
        ///
        /// [`msgs::AcceptChannel`]: crate::ln::msgs::AcceptChannel
-       pub fn accept_inbound_channel(&mut self) -> msgs::AcceptChannel {
+       pub fn accept_inbound_channel(&mut self, user_id: u64) -> msgs::AcceptChannel {
                if self.is_outbound() {
                        panic!("Tried to send accept_channel for an outbound channel?");
                }
@@ -4759,6 +4759,7 @@ impl<Signer: Sign> Channel<Signer> {
                        panic!("The inbound channel has already been accepted");
                }
 
+               self.user_id = user_id;
                self.inbound_awaiting_accept = false;
 
                self.generate_accept_channel_message()
@@ -6420,7 +6421,7 @@ mod tests {
                let mut node_b_chan = Channel::<EnforcingSigner>::new_from_req(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config, 0, &&logger, 42).unwrap();
 
                // Node B --> Node A: accept channel, explicitly setting B's dust limit.
-               let mut accept_channel_msg = node_b_chan.accept_inbound_channel();
+               let mut accept_channel_msg = node_b_chan.accept_inbound_channel(0);
                accept_channel_msg.dust_limit_satoshis = 546;
                node_a_chan.accept_channel(&accept_channel_msg, &config.peer_channel_config_limits, &InitFeatures::known()).unwrap();
                node_a_chan.holder_dust_limit_satoshis = 1560;
@@ -6538,7 +6539,7 @@ mod tests {
                let mut node_b_chan = Channel::<EnforcingSigner>::new_from_req(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config, 0, &&logger, 42).unwrap();
 
                // Node B --> Node A: accept channel
-               let accept_channel_msg = node_b_chan.accept_inbound_channel();
+               let accept_channel_msg = node_b_chan.accept_inbound_channel(0);
                node_a_chan.accept_channel(&accept_channel_msg, &config.peer_channel_config_limits, &InitFeatures::known()).unwrap();
 
                // Node A --> Node B: funding created
index 185a06df937f5252304cf283afa39cc2cfcdfd11..e68ef27a72343078b24fb5b733d85aecd9d83e93 100644 (file)
@@ -4288,8 +4288,13 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        ///
        /// The `temporary_channel_id` parameter indicates which inbound channel should be accepted.
        ///
-       /// [`Event::OpenChannelRequest`]: crate::util::events::Event::OpenChannelRequest
-       pub fn accept_inbound_channel(&self, temporary_channel_id: &[u8; 32]) -> Result<(), APIError> {
+       /// For inbound channels, the `user_channel_id` parameter will be provided back in
+       /// [`Event::ChannelClosed::user_channel_id`] to allow tracking of which events correspond
+       /// with which `accept_inbound_channel` call.
+       ///
+       /// [`Event::OpenChannelRequest`]: events::Event::OpenChannelRequest
+       /// [`Event::ChannelClosed::user_channel_id`]: events::Event::ChannelClosed::user_channel_id
+       pub fn accept_inbound_channel(&self, temporary_channel_id: &[u8; 32], user_channel_id: u64) -> Result<(), APIError> {
                let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
 
                let mut channel_state_lock = self.channel_state.lock().unwrap();
@@ -4301,7 +4306,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                }
                                channel_state.pending_msg_events.push(events::MessageSendEvent::SendAcceptChannel {
                                        node_id: channel.get().get_counterparty_node_id(),
-                                       msg: channel.get_mut().accept_inbound_channel(),
+                                       msg: channel.get_mut().accept_inbound_channel(user_channel_id),
                                });
                        }
                        hash_map::Entry::Vacant(_) => {
@@ -4342,7 +4347,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                if !self.default_configuration.manually_accept_inbound_channels {
                                        channel_state.pending_msg_events.push(events::MessageSendEvent::SendAcceptChannel {
                                                node_id: counterparty_node_id.clone(),
-                                               msg: channel.accept_inbound_channel(),
+                                               msg: channel.accept_inbound_channel(0),
                                        });
                                } else {
                                        let mut pending_events = self.pending_events.lock().unwrap();
index 05d42fba28bbd5980a92c398dd12254f60dec122..525b4f33c438f0c01fda291ae90fbdab4f2c7867 100644 (file)
@@ -8110,7 +8110,7 @@ fn test_manually_accept_inbound_channel_request() {
        let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, Some(manually_accept_conf.clone())]);
        let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
-       nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, Some(manually_accept_conf)).unwrap();
+       let temp_channel_id = nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, Some(manually_accept_conf)).unwrap();
        let res = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
 
        nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &res);
@@ -8122,7 +8122,7 @@ fn test_manually_accept_inbound_channel_request() {
        let events = nodes[1].node.get_and_clear_pending_events();
        match events[0] {
                Event::OpenChannelRequest { temporary_channel_id, .. } => {
-                       nodes[1].node.accept_inbound_channel(&temporary_channel_id).unwrap();
+                       nodes[1].node.accept_inbound_channel(&temporary_channel_id, 23).unwrap();
                }
                _ => panic!("Unexpected event"),
        }
@@ -8136,6 +8136,19 @@ fn test_manually_accept_inbound_channel_request() {
                }
                _ => panic!("Unexpected event"),
        }
+
+       nodes[1].node.force_close_channel(&temp_channel_id).unwrap();
+
+       let close_msg_ev = nodes[1].node.get_and_clear_pending_msg_events();
+       assert_eq!(close_msg_ev.len(), 1);
+
+       let events = nodes[1].node.get_and_clear_pending_events();
+       match events[0] {
+               Event::ChannelClosed { user_channel_id, .. } => {
+                       assert_eq!(user_channel_id, 23);
+               }
+               _ => panic!("Unexpected event"),
+       }
 }
 
 #[test]
@@ -8259,8 +8272,8 @@ fn test_can_not_accept_inbound_channel_twice() {
        let events = nodes[1].node.get_and_clear_pending_events();
        match events[0] {
                Event::OpenChannelRequest { temporary_channel_id, .. } => {
-                       nodes[1].node.accept_inbound_channel(&temporary_channel_id).unwrap();
-                       let api_res = nodes[1].node.accept_inbound_channel(&temporary_channel_id);
+                       nodes[1].node.accept_inbound_channel(&temporary_channel_id, 0).unwrap();
+                       let api_res = nodes[1].node.accept_inbound_channel(&temporary_channel_id, 0);
                        match api_res {
                                Err(APIError::APIMisuseError { err }) => {
                                        assert_eq!(err, "The channel isn't currently awaiting to be accepted.");
@@ -8292,7 +8305,7 @@ fn test_can_not_accept_unknown_inbound_channel() {
        let node = create_network(1, &node_cfg, &node_chanmgr)[0].node;
 
        let unknown_channel_id = [0; 32];
-       let api_res = node.accept_inbound_channel(&unknown_channel_id);
+       let api_res = node.accept_inbound_channel(&unknown_channel_id, 0);
        match api_res {
                Err(APIError::ChannelUnavailable { err }) => {
                        assert_eq!(err, "Can't accept a channel that doesn't exist");
index 13e6beedd4da247c777b37aea628550d9473ce63..282c87051dd48f2c3b3a82db778cc20a78f7e2c5 100644 (file)
@@ -401,82 +401,91 @@ where C::Target: chain::Access, L::Target: Logger
                        return ();
                }
 
-               // Send a gossip_timestamp_filter to enable gossip message receipt. Note that we have to
-               // use a "all timestamps" filter as sending the current timestamp would result in missing
-               // gossip messages that are simply sent late. We could calculate the intended filter time
-               // by looking at the current time and subtracting two weeks (before which we'll reject
-               // messages), but there's not a lot of reason to bother - our peers should be discarding
-               // the same messages.
+               // The lightning network's gossip sync system is completely broken in numerous ways.
+               //
+               // Given no broadly-available set-reconciliation protocol, the only reasonable approach is
+               // to do a full sync from the first few peers we connect to, and then receive gossip
+               // updates from all our peers normally.
+               //
+               // Originally, we could simply tell a peer to dump us the entire gossip table on startup,
+               // wasting lots of bandwidth but ensuring we have the full network graph. After the initial
+               // dump peers would always send gossip and we'd stay up-to-date with whatever our peer has
+               // seen.
+               //
+               // In order to reduce the bandwidth waste, "gossip queries" were introduced, allowing you
+               // to ask for the SCIDs of all channels in your peer's routing graph, and then only request
+               // channel data which you are missing. Except there was no way at all to identify which
+               // `channel_update`s you were missing, so you still had to request everything, just in a
+               // very complicated way with some queries instead of just getting the dump.
+               //
+               // Later, an option was added to fetch the latest timestamps of the `channel_update`s to
+               // make efficient sync possible, however it has yet to be implemented in lnd, which makes
+               // relying on it useless.
+               //
+               // After gossip queries were introduced, support for receiving a full gossip table dump on
+               // connection was removed from several nodes, making it impossible to get a full sync
+               // without using the "gossip queries" messages.
+               //
+               // Once you opt into "gossip queries" the only way to receive any gossip updates that a
+               // peer receives after you connect, you must send a `gossip_timestamp_filter` message. This
+               // message, as the name implies, tells the peer to not forward any gossip messages with a
+               // timestamp older than a given value (not the time the peer received the filter, but the
+               // timestamp in the update message, which is often hours behind when the peer received the
+               // message).
+               //
+               // Obnoxiously, `gossip_timestamp_filter` isn't *just* a filter, but its also a request for
+               // your peer to send you the full routing graph (subject to the filter). Thus, in order to
+               // tell a peer to send you any updates as it sees them, you have to also ask for the full
+               // routing graph to be synced. If you set a timestamp filter near the current time, peers
+               // will simply not forward any new updates they see to you which were generated some time
+               // ago (which is not uncommon). If you instead set a timestamp filter near 0 (or two weeks
+               // ago), you will always get the full routing graph from all your peers.
+               //
+               // Most lightning nodes today opt to simply turn off receiving gossip data which only
+               // propagated some time after it was generated, and, worse, often disable gossiping with
+               // several peers after their first connection. The second behavior can cause gossip to not
+               // propagate fully if there are cuts in the gossiping subgraph.
+               //
+               // In an attempt to cut a middle ground between always fetching the full graph from all of
+               // our peers and never receiving gossip from peers at all, we send all of our peers a
+               // `gossip_timestamp_filter`, with the filter time set either two weeks ago or an hour ago.
+               //
+               // For no-std builds, we bury our head in the sand and do a full sync on each connection.
+               let should_request_full_sync = self.should_request_full_sync(&their_node_id);
+               #[allow(unused_mut, unused_assignments)]
+               let mut gossip_start_time = 0;
+               #[cfg(feature = "std")]
+               {
+                       gossip_start_time = SystemTime::now().duration_since(UNIX_EPOCH).expect("Time must be > 1970").as_secs();
+                       if should_request_full_sync {
+                               gossip_start_time -= 60 * 60 * 24 * 7 * 2; // 2 weeks ago
+                       } else {
+                               gossip_start_time -= 60 * 60; // an hour ago
+                       }
+               }
+
                let mut pending_events = self.pending_events.lock().unwrap();
                pending_events.push(MessageSendEvent::SendGossipTimestampFilter {
                        node_id: their_node_id.clone(),
                        msg: GossipTimestampFilter {
                                chain_hash: self.network_graph.genesis_hash,
-                               first_timestamp: 0,
+                               first_timestamp: gossip_start_time as u32, // 2106 issue!
                                timestamp_range: u32::max_value(),
                        },
                });
-
-               // Check if we need to perform a full synchronization with this peer
-               if !self.should_request_full_sync(&their_node_id) {
-                       return ();
-               }
-
-               let first_blocknum = 0;
-               let number_of_blocks = 0xffffffff;
-               log_debug!(self.logger, "Sending query_channel_range peer={}, first_blocknum={}, number_of_blocks={}", log_pubkey!(their_node_id), first_blocknum, number_of_blocks);
-               pending_events.push(MessageSendEvent::SendChannelRangeQuery {
-                       node_id: their_node_id.clone(),
-                       msg: QueryChannelRange {
-                               chain_hash: self.network_graph.genesis_hash,
-                               first_blocknum,
-                               number_of_blocks,
-                       },
-               });
        }
 
-       /// Statelessly processes a reply to a channel range query by immediately
-       /// sending an SCID query with SCIDs in the reply. To keep this handler
-       /// stateless, it does not validate the sequencing of replies for multi-
-       /// reply ranges. It does not validate whether the reply(ies) cover the
-       /// queried range. It also does not filter SCIDs to only those in the
-       /// original query range. We also do not validate that the chain_hash
-       /// matches the chain_hash of the NetworkGraph. Any chan_ann message that
-       /// does not match our chain_hash will be rejected when the announcement is
-       /// processed.
-       fn handle_reply_channel_range(&self, their_node_id: &PublicKey, msg: ReplyChannelRange) -> Result<(), LightningError> {
-               log_debug!(self.logger, "Handling reply_channel_range peer={}, first_blocknum={}, number_of_blocks={}, sync_complete={}, scids={}", log_pubkey!(their_node_id), msg.first_blocknum, msg.number_of_blocks, msg.sync_complete, msg.short_channel_ids.len(),);
-
-               log_debug!(self.logger, "Sending query_short_channel_ids peer={}, batch_size={}", log_pubkey!(their_node_id), msg.short_channel_ids.len());
-               let mut pending_events = self.pending_events.lock().unwrap();
-               pending_events.push(MessageSendEvent::SendShortIdsQuery {
-                       node_id: their_node_id.clone(),
-                       msg: QueryShortChannelIds {
-                               chain_hash: msg.chain_hash,
-                               short_channel_ids: msg.short_channel_ids,
-                       }
-               });
-
+       fn handle_reply_channel_range(&self, _their_node_id: &PublicKey, _msg: ReplyChannelRange) -> Result<(), LightningError> {
+               // We don't make queries, so should never receive replies. If, in the future, the set
+               // reconciliation extensions to gossip queries become broadly supported, we should revert
+               // this code to its state pre-0.0.106.
                Ok(())
        }
 
-       /// When an SCID query is initiated the remote peer will begin streaming
-       /// gossip messages. In the event of a failure, we may have received
-       /// some channel information. Before trying with another peer, the
-       /// caller should update its set of SCIDs that need to be queried.
-       fn handle_reply_short_channel_ids_end(&self, their_node_id: &PublicKey, msg: ReplyShortChannelIdsEnd) -> Result<(), LightningError> {
-               log_debug!(self.logger, "Handling reply_short_channel_ids_end peer={}, full_information={}", log_pubkey!(their_node_id), msg.full_information);
-
-               // If the remote node does not have up-to-date information for the
-               // chain_hash they will set full_information=false. We can fail
-               // the result and try again with a different peer.
-               if !msg.full_information {
-                       return Err(LightningError {
-                               err: String::from("Received reply_short_channel_ids_end with no information"),
-                               action: ErrorAction::IgnoreError
-                       });
-               }
-
+       fn handle_reply_short_channel_ids_end(&self, _their_node_id: &PublicKey, _msg: ReplyShortChannelIdsEnd) -> Result<(), LightningError> {
+               // We don't make queries, so should never receive replies. If, in the future, the set
+               // reconciliation extensions to gossip queries become broadly supported, we should revert
+               // this code to its state pre-0.0.106.
                Ok(())
        }
 
@@ -1541,7 +1550,7 @@ mod tests {
        use routing::network_graph::{NetGraphMsgHandler, NetworkGraph, NetworkUpdate, MAX_EXCESS_BYTES_FOR_RELAY};
        use ln::msgs::{Init, OptionalField, RoutingMessageHandler, UnsignedNodeAnnouncement, NodeAnnouncement,
                UnsignedChannelAnnouncement, ChannelAnnouncement, UnsignedChannelUpdate, ChannelUpdate, 
-               ReplyChannelRange, ReplyShortChannelIdsEnd, QueryChannelRange, QueryShortChannelIds, MAX_VALUE_MSAT};
+               ReplyChannelRange, QueryChannelRange, QueryShortChannelIds, MAX_VALUE_MSAT};
        use util::test_utils;
        use util::logger::Logger;
        use util::ser::{Readable, Writeable};
@@ -2278,15 +2287,16 @@ mod tests {
        }
 
        #[test]
+       #[cfg(feature = "std")]
        fn calling_sync_routing_table() {
+               use std::time::{SystemTime, UNIX_EPOCH};
+
                let network_graph = create_network_graph();
                let (secp_ctx, net_graph_msg_handler) = create_net_graph_msg_handler(&network_graph);
                let node_privkey_1 = &SecretKey::from_slice(&[42; 32]).unwrap();
                let node_id_1 = PublicKey::from_secret_key(&secp_ctx, node_privkey_1);
 
                let chain_hash = genesis_block(Network::Testnet).header.block_hash();
-               let first_blocknum = 0;
-               let number_of_blocks = 0xffff_ffff;
 
                // It should ignore if gossip_queries feature is not enabled
                {
@@ -2296,132 +2306,23 @@ mod tests {
                        assert_eq!(events.len(), 0);
                }
 
-               // It should send a query_channel_message with the correct information
+               // It should send a gossip_timestamp_filter with the correct information
                {
                        let init_msg = Init { features: InitFeatures::known(), remote_network_address: None };
                        net_graph_msg_handler.peer_connected(&node_id_1, &init_msg);
                        let events = net_graph_msg_handler.get_and_clear_pending_msg_events();
-                       assert_eq!(events.len(), 2);
+                       assert_eq!(events.len(), 1);
                        match &events[0] {
                                MessageSendEvent::SendGossipTimestampFilter{ node_id, msg } => {
                                        assert_eq!(node_id, &node_id_1);
                                        assert_eq!(msg.chain_hash, chain_hash);
-                                       assert_eq!(msg.first_timestamp, 0);
+                                       let expected_timestamp = SystemTime::now().duration_since(UNIX_EPOCH).expect("Time must be > 1970").as_secs();
+                                       assert!((msg.first_timestamp as u64) >= expected_timestamp - 60*60*24*7*2);
+                                       assert!((msg.first_timestamp as u64) < expected_timestamp - 60*60*24*7*2 + 10);
                                        assert_eq!(msg.timestamp_range, u32::max_value());
                                },
                                _ => panic!("Expected MessageSendEvent::SendChannelRangeQuery")
                        };
-                       match &events[1] {
-                               MessageSendEvent::SendChannelRangeQuery{ node_id, msg } => {
-                                       assert_eq!(node_id, &node_id_1);
-                                       assert_eq!(msg.chain_hash, chain_hash);
-                                       assert_eq!(msg.first_blocknum, first_blocknum);
-                                       assert_eq!(msg.number_of_blocks, number_of_blocks);
-                               },
-                               _ => panic!("Expected MessageSendEvent::SendChannelRangeQuery")
-                       };
-               }
-
-               // It should not enqueue a query when should_request_full_sync return false.
-               // The initial implementation allows syncing with the first 5 peers after
-               // which should_request_full_sync will return false
-               {
-                       let network_graph = create_network_graph();
-                       let (secp_ctx, net_graph_msg_handler) = create_net_graph_msg_handler(&network_graph);
-                       let init_msg = Init { features: InitFeatures::known(), remote_network_address: None };
-                       for n in 1..7 {
-                               let node_privkey = &SecretKey::from_slice(&[n; 32]).unwrap();
-                               let node_id = PublicKey::from_secret_key(&secp_ctx, node_privkey);
-                               net_graph_msg_handler.peer_connected(&node_id, &init_msg);
-                               let events = net_graph_msg_handler.get_and_clear_pending_msg_events();
-                               if n <= 5 {
-                                       assert_eq!(events.len(), 2);
-                               } else {
-                                       // Even after the we stop sending the explicit query, we should still send a
-                                       // gossip_timestamp_filter on each new connection.
-                                       assert_eq!(events.len(), 1);
-                               }
-
-                       }
-               }
-       }
-
-       #[test]
-       fn handling_reply_channel_range() {
-               let network_graph = create_network_graph();
-               let (secp_ctx, net_graph_msg_handler) = create_net_graph_msg_handler(&network_graph);
-               let node_privkey_1 = &SecretKey::from_slice(&[42; 32]).unwrap();
-               let node_id_1 = PublicKey::from_secret_key(&secp_ctx, node_privkey_1);
-
-               let chain_hash = genesis_block(Network::Testnet).header.block_hash();
-
-               // Test receipt of a single reply that should enqueue an SCID query
-               // matching the SCIDs in the reply
-               {
-                       let result = net_graph_msg_handler.handle_reply_channel_range(&node_id_1, ReplyChannelRange {
-                               chain_hash,
-                               sync_complete: true,
-                               first_blocknum: 0,
-                               number_of_blocks: 2000,
-                               short_channel_ids: vec![
-                                       0x0003e0_000000_0000, // 992x0x0
-                                       0x0003e8_000000_0000, // 1000x0x0
-                                       0x0003e9_000000_0000, // 1001x0x0
-                                       0x0003f0_000000_0000, // 1008x0x0
-                                       0x00044c_000000_0000, // 1100x0x0
-                                       0x0006e0_000000_0000, // 1760x0x0
-                               ],
-                       });
-                       assert!(result.is_ok());
-
-                       // We expect to emit a query_short_channel_ids message with the received scids
-                       let events = net_graph_msg_handler.get_and_clear_pending_msg_events();
-                       assert_eq!(events.len(), 1);
-                       match &events[0] {
-                               MessageSendEvent::SendShortIdsQuery { node_id, msg } => {
-                                       assert_eq!(node_id, &node_id_1);
-                                       assert_eq!(msg.chain_hash, chain_hash);
-                                       assert_eq!(msg.short_channel_ids, vec![
-                                               0x0003e0_000000_0000, // 992x0x0
-                                               0x0003e8_000000_0000, // 1000x0x0
-                                               0x0003e9_000000_0000, // 1001x0x0
-                                               0x0003f0_000000_0000, // 1008x0x0
-                                               0x00044c_000000_0000, // 1100x0x0
-                                               0x0006e0_000000_0000, // 1760x0x0
-                                       ]);
-                               },
-                               _ => panic!("expected MessageSendEvent::SendShortIdsQuery"),
-                       }
-               }
-       }
-
-       #[test]
-       fn handling_reply_short_channel_ids() {
-               let network_graph = create_network_graph();
-               let (secp_ctx, net_graph_msg_handler) = create_net_graph_msg_handler(&network_graph);
-               let node_privkey = &SecretKey::from_slice(&[41; 32]).unwrap();
-               let node_id = PublicKey::from_secret_key(&secp_ctx, node_privkey);
-
-               let chain_hash = genesis_block(Network::Testnet).header.block_hash();
-
-               // Test receipt of a successful reply
-               {
-                       let result = net_graph_msg_handler.handle_reply_short_channel_ids_end(&node_id, ReplyShortChannelIdsEnd {
-                               chain_hash,
-                               full_information: true,
-                       });
-                       assert!(result.is_ok());
-               }
-
-               // Test receipt of a reply that indicates the peer does not maintain up-to-date information
-               // for the chain_hash requested in the query.
-               {
-                       let result = net_graph_msg_handler.handle_reply_short_channel_ids_end(&node_id, ReplyShortChannelIdsEnd {
-                               chain_hash,
-                               full_information: false,
-                       });
-                       assert!(result.is_err());
-                       assert_eq!(result.err().unwrap().err, "Received reply_short_channel_ids_end with no information");
                }
        }
 
index d8b1740c19ec2f1ec6098fb5f941c5e387845865..459303f7d87f0c988293a2360798665570607955 100644 (file)
@@ -197,10 +197,6 @@ pub struct FixedPenaltyScorer {
        penalty_msat: u64,
 }
 
-impl_writeable_tlv_based!(FixedPenaltyScorer, {
-       (0, penalty_msat, required),
-});
-
 impl FixedPenaltyScorer {
        /// Creates a new scorer using `penalty_msat`.
        pub fn with_penalty(penalty_msat: u64) -> Self {
@@ -218,6 +214,22 @@ impl Score for FixedPenaltyScorer {
        fn payment_path_successful(&mut self, _path: &[&RouteHop]) {}
 }
 
+impl Writeable for FixedPenaltyScorer {
+       #[inline]
+       fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
+               write_tlv_fields!(w, {});
+               Ok(())
+       }
+}
+
+impl ReadableArgs<u64> for FixedPenaltyScorer {
+       #[inline]
+       fn read<R: Read>(r: &mut R, penalty_msat: u64) -> Result<Self, DecodeError> {
+               read_tlv_fields!(r, {});
+               Ok(Self { penalty_msat })
+       }
+}
+
 /// [`Score`] implementation that provides reasonable default behavior.
 ///
 /// Used to apply a fixed penalty to each channel, thus avoiding long paths when shorter paths with
@@ -504,19 +516,24 @@ pub struct ProbabilisticScorerUsingTime<G: Deref<Target = NetworkGraph>, T: Time
 }
 
 /// Parameters for configuring [`ProbabilisticScorer`].
+///
+/// Used to configure a base penalty and a liquidity penalty, the sum of which is the channel
+/// penalty (i.e., the amount in msats willing to be paid to avoid routing through the channel).
 #[derive(Clone, Copy)]
 pub struct ProbabilisticScoringParameters {
-       /// A multiplier used to determine the amount in msats willing to be paid to avoid routing
-       /// through a channel, as per multiplying by the negative `log10` of the channel's success
-       /// probability for a payment.
+       /// A fixed penalty in msats to apply to each channel.
        ///
-       /// The success probability is determined by the effective channel capacity, the payment amount,
-       /// and knowledge learned from prior successful and unsuccessful payments. The lower bound of
-       /// the success probability is 0.01, effectively limiting the penalty to the range
-       /// `0..=2*liquidity_penalty_multiplier_msat`. The knowledge learned is decayed over time based
-       /// on [`liquidity_offset_half_life`].
+       /// Default value: 500 msat
+       pub base_penalty_msat: u64,
+
+       /// A multiplier used in conjunction with the negative `log10` of the channel's success
+       /// probability for a payment to determine the liquidity penalty.
+       ///
+       /// The penalty is based in part by the knowledge learned from prior successful and unsuccessful
+       /// payments. This knowledge is decayed over time based on [`liquidity_offset_half_life`]. The
+       /// penalty is effectively limited to `2 * liquidity_penalty_multiplier_msat`.
        ///
-       /// Default value: 10,000 msat
+       /// Default value: 40,000 msat
        ///
        /// [`liquidity_offset_half_life`]: Self::liquidity_offset_half_life
        pub liquidity_penalty_multiplier_msat: u64,
@@ -537,11 +554,6 @@ pub struct ProbabilisticScoringParameters {
        pub liquidity_offset_half_life: Duration,
 }
 
-impl_writeable_tlv_based!(ProbabilisticScoringParameters, {
-       (0, liquidity_penalty_multiplier_msat, required),
-       (2, liquidity_offset_half_life, required),
-});
-
 /// Accounting for channel liquidity balance uncertainty.
 ///
 /// Direction is defined in terms of [`NodeId`] partial ordering, where the source node is the
@@ -590,7 +602,8 @@ impl<G: Deref<Target = NetworkGraph>, T: Time> ProbabilisticScorerUsingTime<G, T
 impl Default for ProbabilisticScoringParameters {
        fn default() -> Self {
                Self {
-                       liquidity_penalty_multiplier_msat: 10_000,
+                       base_penalty_msat: 500,
+                       liquidity_penalty_multiplier_msat: 40_000,
                        liquidity_offset_half_life: Duration::from_secs(3600),
                }
        }
@@ -653,20 +666,21 @@ impl<L: Deref<Target = u64>, T: Time, U: Deref<Target = T>> DirectedChannelLiqui
        /// Returns a penalty for routing the given HTLC `amount_msat` through the channel in this
        /// direction.
        fn penalty_msat(&self, amount_msat: u64, liquidity_penalty_multiplier_msat: u64) -> u64 {
+               let max_penalty_msat = liquidity_penalty_multiplier_msat.saturating_mul(2);
                let max_liquidity_msat = self.max_liquidity_msat();
                let min_liquidity_msat = core::cmp::min(self.min_liquidity_msat(), max_liquidity_msat);
                if amount_msat > max_liquidity_msat {
-                       u64::max_value()
+                       max_penalty_msat
                } else if amount_msat <= min_liquidity_msat {
                        0
                } else {
-                       let numerator = max_liquidity_msat + 1 - amount_msat;
-                       let denominator = max_liquidity_msat + 1 - min_liquidity_msat;
-                       approx::negative_log10_times_1024(numerator, denominator)
-                               .saturating_mul(liquidity_penalty_multiplier_msat) / 1024
+                       let numerator = (max_liquidity_msat - amount_msat).saturating_add(1);
+                       let denominator = (max_liquidity_msat - min_liquidity_msat).saturating_add(1);
+                       let penalty_msat = approx::negative_log10_times_1024(numerator, denominator)
+                               .saturating_mul(liquidity_penalty_multiplier_msat) / 1024;
+                       // Upper bound the penalty to ensure some channel is selected.
+                       penalty_msat.min(max_penalty_msat)
                }
-               // Upper bound the penalty to ensure some channel is selected.
-               .min(2 * liquidity_penalty_multiplier_msat)
        }
 
        /// Returns the lower bound of the channel liquidity balance in this direction.
@@ -745,6 +759,7 @@ impl<G: Deref<Target = NetworkGraph>, T: Time> Score for ProbabilisticScorerUsin
                        .unwrap_or(&ChannelLiquidity::new())
                        .as_directed(source, target, capacity_msat, liquidity_offset_half_life)
                        .penalty_msat(amount_msat, liquidity_penalty_multiplier_msat)
+                       .saturating_add(self.params.base_penalty_msat)
        }
 
        fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
@@ -1722,7 +1737,7 @@ mod tests {
        fn increased_penalty_nearing_liquidity_upper_bound() {
                let network_graph = network_graph();
                let params = ProbabilisticScoringParameters {
-                       liquidity_penalty_multiplier_msat: 1_000, ..Default::default()
+                       base_penalty_msat: 0, liquidity_penalty_multiplier_msat: 1_000, ..Default::default()
                };
                let scorer = ProbabilisticScorer::new(params, &network_graph);
                let source = source_node_id();
@@ -1747,7 +1762,7 @@ mod tests {
                let last_updated = SinceEpoch::now();
                let network_graph = network_graph();
                let params = ProbabilisticScoringParameters {
-                       liquidity_penalty_multiplier_msat: 1_000, ..Default::default()
+                       base_penalty_msat: 0, liquidity_penalty_multiplier_msat: 1_000, ..Default::default()
                };
                let scorer = ProbabilisticScorer::new(params, &network_graph)
                        .with_channel(42,
@@ -1767,7 +1782,7 @@ mod tests {
        fn does_not_further_penalize_own_channel() {
                let network_graph = network_graph();
                let params = ProbabilisticScoringParameters {
-                       liquidity_penalty_multiplier_msat: 1_000, ..Default::default()
+                       base_penalty_msat: 0, liquidity_penalty_multiplier_msat: 1_000, ..Default::default()
                };
                let mut scorer = ProbabilisticScorer::new(params, &network_graph);
                let sender = sender_node_id();
@@ -1788,7 +1803,7 @@ mod tests {
        fn sets_liquidity_lower_bound_on_downstream_failure() {
                let network_graph = network_graph();
                let params = ProbabilisticScoringParameters {
-                       liquidity_penalty_multiplier_msat: 1_000, ..Default::default()
+                       base_penalty_msat: 0, liquidity_penalty_multiplier_msat: 1_000, ..Default::default()
                };
                let mut scorer = ProbabilisticScorer::new(params, &network_graph);
                let source = source_node_id();
@@ -1810,7 +1825,7 @@ mod tests {
        fn sets_liquidity_upper_bound_on_failure() {
                let network_graph = network_graph();
                let params = ProbabilisticScoringParameters {
-                       liquidity_penalty_multiplier_msat: 1_000, ..Default::default()
+                       base_penalty_msat: 0, liquidity_penalty_multiplier_msat: 1_000, ..Default::default()
                };
                let mut scorer = ProbabilisticScorer::new(params, &network_graph);
                let source = source_node_id();
@@ -1832,7 +1847,7 @@ mod tests {
        fn reduces_liquidity_upper_bound_along_path_on_success() {
                let network_graph = network_graph();
                let params = ProbabilisticScoringParameters {
-                       liquidity_penalty_multiplier_msat: 1_000, ..Default::default()
+                       base_penalty_msat: 0, liquidity_penalty_multiplier_msat: 1_000, ..Default::default()
                };
                let mut scorer = ProbabilisticScorer::new(params, &network_graph);
                let sender = sender_node_id();
@@ -1856,6 +1871,7 @@ mod tests {
        fn decays_liquidity_bounds_over_time() {
                let network_graph = network_graph();
                let params = ProbabilisticScoringParameters {
+                       base_penalty_msat: 0,
                        liquidity_penalty_multiplier_msat: 1_000,
                        liquidity_offset_half_life: Duration::from_secs(10),
                };
@@ -1907,6 +1923,7 @@ mod tests {
        fn decays_liquidity_bounds_without_shift_overflow() {
                let network_graph = network_graph();
                let params = ProbabilisticScoringParameters {
+                       base_penalty_msat: 0,
                        liquidity_penalty_multiplier_msat: 1_000,
                        liquidity_offset_half_life: Duration::from_secs(10),
                };
@@ -1931,6 +1948,7 @@ mod tests {
        fn restricts_liquidity_bounds_after_decay() {
                let network_graph = network_graph();
                let params = ProbabilisticScoringParameters {
+                       base_penalty_msat: 0,
                        liquidity_penalty_multiplier_msat: 1_000,
                        liquidity_offset_half_life: Duration::from_secs(10),
                };
@@ -1968,6 +1986,7 @@ mod tests {
        fn restores_persisted_liquidity_bounds() {
                let network_graph = network_graph();
                let params = ProbabilisticScoringParameters {
+                       base_penalty_msat: 0,
                        liquidity_penalty_multiplier_msat: 1_000,
                        liquidity_offset_half_life: Duration::from_secs(10),
                };
@@ -1997,6 +2016,7 @@ mod tests {
        fn decays_persisted_liquidity_bounds() {
                let network_graph = network_graph();
                let params = ProbabilisticScoringParameters {
+                       base_penalty_msat: 0,
                        liquidity_penalty_multiplier_msat: 1_000,
                        liquidity_offset_half_life: Duration::from_secs(10),
                };
@@ -2023,4 +2043,39 @@ mod tests {
                SinceEpoch::advance(Duration::from_secs(10));
                assert_eq!(deserialized_scorer.channel_penalty_msat(42, 500, 1_000, &source, &target), 371);
        }
+
+       #[test]
+       fn adds_base_penalty_to_liquidity_penalty() {
+               let network_graph = network_graph();
+               let source = source_node_id();
+               let target = target_node_id();
+
+               let params = ProbabilisticScoringParameters {
+                       base_penalty_msat: 0, liquidity_penalty_multiplier_msat: 1_000, ..Default::default()
+               };
+               let scorer = ProbabilisticScorer::new(params, &network_graph);
+               assert_eq!(scorer.channel_penalty_msat(42, 128, 1_024, &source, &target), 58);
+
+               let params = ProbabilisticScoringParameters {
+                       base_penalty_msat: 500, liquidity_penalty_multiplier_msat: 1_000, ..Default::default()
+               };
+               let scorer = ProbabilisticScorer::new(params, &network_graph);
+               assert_eq!(scorer.channel_penalty_msat(42, 128, 1_024, &source, &target), 558);
+       }
+
+       #[test]
+       fn calculates_log10_without_overflowing_u64_max_value() {
+               let network_graph = network_graph();
+               let source = source_node_id();
+               let target = target_node_id();
+
+               let params = ProbabilisticScoringParameters {
+                       base_penalty_msat: 0, ..Default::default()
+               };
+               let scorer = ProbabilisticScorer::new(params, &network_graph);
+               assert_eq!(
+                       scorer.channel_penalty_msat(42, u64::max_value(), u64::max_value(), &source, &target),
+                       80_000,
+               );
+       }
 }
index 623b2ea01acb9cf623bc2a4f7bd73e8f32a404ce..c86df5dff693f540baca838ca77fafc05e379b99 100644 (file)
@@ -365,11 +365,15 @@ pub enum Event {
                /// The channel_id of the channel which has been closed. Note that on-chain transactions
                /// resolving the channel are likely still awaiting confirmation.
                channel_id: [u8; 32],
-               /// The `user_channel_id` value passed in to [`ChannelManager::create_channel`], or 0 for
-               /// an inbound channel. This will always be zero for objects serialized with LDK versions
-               /// prior to 0.0.102.
+               /// The `user_channel_id` value passed in to [`ChannelManager::create_channel`] for outbound
+               /// channels, or to [`ChannelManager::accept_inbound_channel`] for inbound channels if
+               /// [`UserConfig::manually_accept_inbound_channels`] config flag is set to true. Otherwise
+               /// `user_channel_id` will be 0 for an inbound channel.
+               /// This will always be zero for objects serialized with LDK versions prior to 0.0.102.
                ///
                /// [`ChannelManager::create_channel`]: crate::ln::channelmanager::ChannelManager::create_channel
+               /// [`ChannelManager::accept_inbound_channel`]: crate::ln::channelmanager::ChannelManager::accept_inbound_channel
+               /// [`UserConfig::manually_accept_inbound_channels`]: crate::util::config::UserConfig::manually_accept_inbound_channels
                user_channel_id: u64,
                /// The reason the channel was closed.
                reason: ClosureReason