From 0ad8fde0d6350d2ab3133c515bc09d39d242fa1a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 24 Dec 2019 13:08:33 -0500 Subject: [PATCH] Relay/store channel/node announces w/ unknown req'd feature bits This change was made in the flat features BOLT PR, as if a channel requires some unknown feature bits we should still rumor it, we just shouldn't route through it. --- lightning/src/ln/msgs.rs | 28 +++++--- lightning/src/ln/router.rs | 136 ++++++++++++++++++++++++++++++------- 2 files changed, 130 insertions(+), 34 deletions(-) diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index e4808010..cccbf83a 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -192,6 +192,23 @@ impl Features { pub(crate) fn byte_count(&self) -> usize { self.flags.len() } + + #[cfg(test)] + pub(crate) fn set_require_unknown_bits(&mut self) { + let newlen = cmp::max(2, self.flags.len()); + self.flags.resize(newlen, 0u8); + self.flags[1] |= 0x40; + } + + #[cfg(test)] + pub(crate) fn clear_require_unknown_bits(&mut self) { + let newlen = cmp::max(2, self.flags.len()); + self.flags.resize(newlen, 0u8); + self.flags[1] &= !0x40; + if self.flags.len() == 2 && self.flags[1] == 0 { + self.flags.resize(1, 0u8); + } + } } impl Features { @@ -1272,13 +1289,7 @@ impl Writeable for UnsignedChannelAnnouncement { impl Readable for UnsignedChannelAnnouncement { fn read(r: &mut R) -> Result { Ok(Self { - features: { - let f: ChannelFeatures = Readable::read(r)?; - if f.requires_unknown_bits() { - return Err(DecodeError::UnknownRequiredFeature); - } - f - }, + features: Readable::read(r)?, chain_hash: Readable::read(r)?, short_channel_id: Readable::read(r)?, node_id_1: Readable::read(r)?, @@ -1406,9 +1417,6 @@ impl Writeable for UnsignedNodeAnnouncement { impl Readable for UnsignedNodeAnnouncement { fn read(r: &mut R) -> Result { let features: NodeFeatures = Readable::read(r)?; - if features.requires_unknown_bits() { - return Err(DecodeError::UnknownRequiredFeature); - } let timestamp: u32 = Readable::read(r)?; let node_id: PublicKey = Readable::read(r)?; let mut rgb = [0; 3]; diff --git a/lightning/src/ln/router.rs b/lightning/src/ln/router.rs index 1579f805..5a21055c 100644 --- a/lightning/src/ln/router.rs +++ b/lightning/src/ln/router.rs @@ -414,10 +414,6 @@ impl RoutingMessageHandler for Router { let msg_hash = hash_to_message!(&Sha256dHash::hash(&msg.contents.encode()[..])[..]); secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.signature, &msg.contents.node_id); - if msg.contents.features.requires_unknown_bits() { - panic!("Unknown-required-features NodeAnnouncements should never deserialize!"); - } - let mut network = self.network_map.write().unwrap(); match network.nodes.get_mut(&msg.contents.node_id) { None => Err(LightningError{err: "No existing channels for node_announcement", action: ErrorAction::IgnoreError}), @@ -432,7 +428,7 @@ impl RoutingMessageHandler for Router { node.alias = msg.contents.alias; node.addresses = msg.contents.addresses.clone(); - let should_relay = msg.contents.excess_data.is_empty() && msg.contents.excess_address_data.is_empty() && !msg.contents.features.supports_unknown_bits(); + let should_relay = msg.contents.excess_data.is_empty() && msg.contents.excess_address_data.is_empty(); node.announcement_message = if should_relay { Some(msg.clone()) } else { None }; Ok(should_relay) } @@ -450,10 +446,6 @@ impl RoutingMessageHandler for Router { secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.bitcoin_signature_1, &msg.contents.bitcoin_key_1); secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.bitcoin_signature_2, &msg.contents.bitcoin_key_2); - if msg.contents.features.requires_unknown_bits() { - panic!("Unknown-required-features ChannelAnnouncements should never deserialize!"); - } - let checked_utxo = match self.chain_monitor.get_chain_utxo(msg.contents.chain_hash, msg.contents.short_channel_id) { Ok((script_pubkey, _value)) => { let expected_script = Builder::new().push_opcode(opcodes::all::OP_PUSHNUM_2) @@ -483,7 +475,7 @@ impl RoutingMessageHandler for Router { let mut network_lock = self.network_map.write().unwrap(); let network = network_lock.borrow_parts(); - let should_relay = msg.contents.excess_data.is_empty() && !msg.contents.features.supports_unknown_bits(); + let should_relay = msg.contents.excess_data.is_empty(); let chan_info = ChannelInfo { features: msg.contents.features.clone(), @@ -939,19 +931,23 @@ impl Router { } } - for chan_id in $node.channels.iter() { - let chan = network.channels.get(chan_id).unwrap(); - if chan.one_to_two.src_node_id == *$node_id { - // ie $node is one, ie next hop in A* is two, via the two_to_one channel - if first_hops.is_none() || chan.two_to_one.src_node_id != network.our_node_id { - if chan.two_to_one.enabled { - add_entry!(chan_id, chan.one_to_two.src_node_id, chan.two_to_one, $fee_to_target_msat); - } - } - } else { - if first_hops.is_none() || chan.one_to_two.src_node_id != network.our_node_id { - if chan.one_to_two.enabled { - add_entry!(chan_id, chan.two_to_one.src_node_id, chan.one_to_two, $fee_to_target_msat); + if !$node.features.requires_unknown_bits() { + for chan_id in $node.channels.iter() { + let chan = network.channels.get(chan_id).unwrap(); + if !chan.features.requires_unknown_bits() { + if chan.one_to_two.src_node_id == *$node_id { + // ie $node is one, ie next hop in A* is two, via the two_to_one channel + if first_hops.is_none() || chan.two_to_one.src_node_id != network.our_node_id { + if chan.two_to_one.enabled { + add_entry!(chan_id, chan.one_to_two.src_node_id, chan.two_to_one, $fee_to_target_msat); + } + } + } else { + if first_hops.is_none() || chan.one_to_two.src_node_id != network.our_node_id { + if chan.one_to_two.enabled { + add_entry!(chan_id, chan.two_to_one.src_node_id, chan.one_to_two, $fee_to_target_msat); + } + } } } } @@ -1015,7 +1011,7 @@ mod tests { use chain::chaininterface; use ln::channelmanager; use ln::router::{Router,NodeInfo,NetworkMap,ChannelInfo,DirectionalChannelInfo,RouteHint}; - use ln::msgs::{ChannelFeatures, NodeFeatures}; + use ln::msgs::{ChannelFeatures, NodeFeatures, LightningError, ErrorAction}; use util::test_utils; use util::test_utils::TestVecWriter; use util::logger::Logger; @@ -1441,6 +1437,98 @@ mod tests { assert_eq!(route.hops[1].cltv_expiry_delta, 42); } + { // Disable channels 4 and 12 by requiring unknown feature bits + let mut network = router.network_map.write().unwrap(); + network.channels.get_mut(&NetworkMap::get_key(4, zero_hash.clone())).unwrap().features.set_require_unknown_bits(); + network.channels.get_mut(&NetworkMap::get_key(12, zero_hash.clone())).unwrap().features.set_require_unknown_bits(); + } + + { // If all the channels require some features we don't understand, route should fail + if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = router.get_route(&node3, None, &Vec::new(), 100, 42) { + assert_eq!(err, "Failed to find a path to the given destination"); + } else { panic!(); } + } + + { // If we specify a channel to node8, that overrides our local channel view and that gets used + let our_chans = vec![channelmanager::ChannelDetails { + channel_id: [0; 32], + short_channel_id: Some(42), + remote_network_id: node8.clone(), + channel_value_satoshis: 0, + user_id: 0, + outbound_capacity_msat: 0, + inbound_capacity_msat: 0, + is_live: true, + }]; + let route = router.get_route(&node3, Some(&our_chans), &Vec::new(), 100, 42).unwrap(); + assert_eq!(route.hops.len(), 2); + + assert_eq!(route.hops[0].pubkey, node8); + assert_eq!(route.hops[0].short_channel_id, 42); + assert_eq!(route.hops[0].fee_msat, 200); + assert_eq!(route.hops[0].cltv_expiry_delta, (13 << 8) | 1); + + assert_eq!(route.hops[1].pubkey, node3); + assert_eq!(route.hops[1].short_channel_id, 13); + assert_eq!(route.hops[1].fee_msat, 100); + assert_eq!(route.hops[1].cltv_expiry_delta, 42); + } + + { // Re-enable channels 4 and 12 by wiping the unknown feature bits + let mut network = router.network_map.write().unwrap(); + network.channels.get_mut(&NetworkMap::get_key(4, zero_hash.clone())).unwrap().features.clear_require_unknown_bits(); + network.channels.get_mut(&NetworkMap::get_key(12, zero_hash.clone())).unwrap().features.clear_require_unknown_bits(); + } + + { // Disable nodes 1, 2, and 8 by requiring unknown feature bits + let mut network = router.network_map.write().unwrap(); + network.nodes.get_mut(&node1).unwrap().features.set_require_unknown_bits(); + network.nodes.get_mut(&node2).unwrap().features.set_require_unknown_bits(); + network.nodes.get_mut(&node8).unwrap().features.set_require_unknown_bits(); + } + + { // If all nodes require some features we don't understand, route should fail + if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = router.get_route(&node3, None, &Vec::new(), 100, 42) { + assert_eq!(err, "Failed to find a path to the given destination"); + } else { panic!(); } + } + + { // If we specify a channel to node8, that overrides our local channel view and that gets used + let our_chans = vec![channelmanager::ChannelDetails { + channel_id: [0; 32], + short_channel_id: Some(42), + remote_network_id: node8.clone(), + channel_value_satoshis: 0, + user_id: 0, + outbound_capacity_msat: 0, + inbound_capacity_msat: 0, + is_live: true, + }]; + let route = router.get_route(&node3, Some(&our_chans), &Vec::new(), 100, 42).unwrap(); + assert_eq!(route.hops.len(), 2); + + assert_eq!(route.hops[0].pubkey, node8); + assert_eq!(route.hops[0].short_channel_id, 42); + assert_eq!(route.hops[0].fee_msat, 200); + assert_eq!(route.hops[0].cltv_expiry_delta, (13 << 8) | 1); + + assert_eq!(route.hops[1].pubkey, node3); + assert_eq!(route.hops[1].short_channel_id, 13); + assert_eq!(route.hops[1].fee_msat, 100); + assert_eq!(route.hops[1].cltv_expiry_delta, 42); + } + + { // Re-enable nodes 1, 2, and 8 + let mut network = router.network_map.write().unwrap(); + network.nodes.get_mut(&node1).unwrap().features.clear_require_unknown_bits(); + network.nodes.get_mut(&node2).unwrap().features.clear_require_unknown_bits(); + network.nodes.get_mut(&node8).unwrap().features.clear_require_unknown_bits(); + } + + // Note that we don't test disabling node 3 and failing to route to it, as we (somewhat + // naively) assume that the user checked the feature bits on the invoice, which override + // the node_announcement. + { // Route to 1 via 2 and 3 because our channel to 1 is disabled let route = router.get_route(&node1, None, &Vec::new(), 100, 42).unwrap(); assert_eq!(route.hops.len(), 3); -- 2.30.2