From: Matt Corallo Date: Tue, 6 Jul 2021 23:41:27 +0000 (+0000) Subject: Improve ChannelDetails readability significantly. X-Git-Tag: v0.0.99~2^2 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=refs%2Fheads%2F2021-07-chan-details-usability;p=rust-lightning Improve ChannelDetails readability significantly. After the merge of #984, Jeff pointed out that `ChannelDetails` has become a bit of a "bag of variables", and that a few of the variable names in #984 were more confusing than necessary in context. This addresses several issues by: * Splitting counterparty parameters into a separate `ChannelCounterpartyParameters` struct, * using the name `unspendable_punishment_reserve` for both outbound and inbound channel reserves, differentiating them based on their position in the counterparty parameters struct or not, * Using the name `force_close_spend_delay` instead of `spend_csv_on_our_commitment_funds` to better communicate what is occurring. --- diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs index fabad11ac..baa32312e 100644 --- a/fuzz/src/router.rs +++ b/fuzz/src/router.rs @@ -13,7 +13,7 @@ use bitcoin::hash_types::BlockHash; use lightning::chain; use lightning::chain::transaction::OutPoint; -use lightning::ln::channelmanager::ChannelDetails; +use lightning::ln::channelmanager::{ChannelDetails, ChannelCounterparty}; use lightning::ln::features::InitFeatures; use lightning::ln::msgs; use lightning::routing::router::{get_route, RouteHint, RouteHintHop}; @@ -207,20 +207,22 @@ pub fn do_test(data: &[u8], out: Out) { let rnid = node_pks.iter().skip(slice_to_be16(get_slice!(2))as usize % node_pks.len()).next().unwrap(); first_hops_vec.push(ChannelDetails { channel_id: [0; 32], + counterparty: ChannelCounterparty { + node_id: *rnid, + features: InitFeatures::known(), + unspendable_punishment_reserve: 0, + forwarding_info: None, + }, funding_txo: Some(OutPoint { txid: bitcoin::Txid::from_slice(&[0; 32]).unwrap(), index: 0 }), short_channel_id: Some(scid), - remote_network_id: *rnid, - counterparty_features: InitFeatures::known(), channel_value_satoshis: slice_to_be64(get_slice!(8)), user_id: 0, inbound_capacity_msat: 0, - to_self_reserve_satoshis: None, - to_remote_reserve_satoshis: 0, + unspendable_punishment_reserve: None, confirmations_required: None, - spend_csv_on_our_commitment_funds: None, + force_close_spend_delay: None, is_outbound: true, is_funding_locked: true, is_usable: true, is_public: true, outbound_capacity_msat: 0, - counterparty_forwarding_info: None, }); } Some(&first_hops_vec[..]) diff --git a/lightning-invoice/src/utils.rs b/lightning-invoice/src/utils.rs index 9d41b928d..f419f5f7f 100644 --- a/lightning-invoice/src/utils.rs +++ b/lightning-invoice/src/utils.rs @@ -36,12 +36,12 @@ where Some(id) => id, None => continue, }; - let forwarding_info = match channel.counterparty_forwarding_info { + let forwarding_info = match channel.counterparty.forwarding_info { Some(info) => info, None => continue, }; route_hints.push(RouteHint(vec![RouteHintHop { - src_node_id: channel.remote_network_id, + src_node_id: channel.counterparty.node_id, short_channel_id, fees: RoutingFees { base_msat: forwarding_info.fee_base_msat, diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 21d13eaef..e38b232e9 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -602,6 +602,29 @@ const CHECK_CLTV_EXPIRY_SANITY: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRA #[allow(dead_code)] const CHECK_CLTV_EXPIRY_SANITY_2: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - 2*CLTV_CLAIM_BUFFER; +/// Channel parameters which apply to our counterparty. These are split out from [`ChannelDetails`] +/// to better separate parameters. +#[derive(Clone, Debug, PartialEq)] +pub struct ChannelCounterparty { + /// The node_id of our counterparty + pub node_id: PublicKey, + /// The Features the channel counterparty provided upon last connection. + /// Useful for routing as it is the most up-to-date copy of the counterparty's features and + /// many routing-relevant features are present in the init context. + pub features: InitFeatures, + /// The value, in satoshis, that must always be held in the channel for our counterparty. This + /// value ensures that if our counterparty broadcasts a revoked state, we can punish them by + /// claiming at least this value on chain. + /// + /// This value is not included in [`inbound_capacity_msat`] as it can never be spent. + /// + /// [`inbound_capacity_msat`]: ChannelDetails::inbound_capacity_msat + pub unspendable_punishment_reserve: u64, + /// Information on the fees and requirements that the counterparty requires when forwarding + /// payments to us through this channel. + pub forwarding_info: Option, +} + /// Details of a channel, as returned by ChannelManager::list_channels and ChannelManager::list_usable_channels #[derive(Clone, Debug, PartialEq)] pub struct ChannelDetails { @@ -610,6 +633,8 @@ pub struct ChannelDetails { /// Note that this means this value is *not* persistent - it can change once during the /// lifetime of the channel. pub channel_id: [u8; 32], + /// Parameters which apply to our counterparty. See individual fields for more information. + pub counterparty: ChannelCounterparty, /// The Channel's funding transaction output, if we've negotiated the funding transaction with /// our counterparty already. /// @@ -619,12 +644,6 @@ pub struct ChannelDetails { /// The position of the funding transaction in the chain. None if the funding transaction has /// not yet been confirmed and the channel fully opened. pub short_channel_id: Option, - /// The node_id of our counterparty - pub remote_network_id: PublicKey, - /// The Features the channel counterparty provided upon last connection. - /// Useful for routing as it is the most up-to-date copy of the counterparty's features and - /// many routing-relevant features are present in the init context. - pub counterparty_features: InitFeatures, /// The value, in satoshis, of this channel as appears in the funding output pub channel_value_satoshis: u64, /// The value, in satoshis, that must always be held in the channel for us. This value ensures @@ -636,15 +655,7 @@ pub struct ChannelDetails { /// This value will be `None` for outbound channels until the counterparty accepts the channel. /// /// [`outbound_capacity_msat`]: ChannelDetails::outbound_capacity_msat - pub to_self_reserve_satoshis: Option, - /// The value, in satoshis, that must always be held in the channel for our counterparty. This - /// value ensures that if our counterparty broadcasts a revoked state, we can punish them by - /// claiming at least this value on chain. - /// - /// This value is not included in [`inbound_capacity_msat`] as it can never be spent. - /// - /// [`inbound_capacity_msat`]: ChannelDetails::inbound_capacity_msat - pub to_remote_reserve_satoshis: u64, + pub unspendable_punishment_reserve: Option, /// The user_id passed in to create_channel, or 0 if the channel was inbound. pub user_id: u64, /// The available outbound capacity for sending HTLCs to the remote peer. This does not include @@ -685,7 +696,7 @@ pub struct ChannelDetails { /// time to claim our non-HTLC-encumbered funds. /// /// This value will be `None` for outbound channels until the counterparty accepts the channel. - pub spend_csv_on_our_commitment_funds: Option, + pub force_close_spend_delay: Option, /// True if the channel was initiated (and thus funded) by us. pub is_outbound: bool, /// True if the channel is confirmed, funding_locked messages have been exchanged, and the @@ -703,9 +714,6 @@ pub struct ChannelDetails { pub is_usable: bool, /// True if this channel is (or will be) publicly-announced. pub is_public: bool, - /// Information on the fees and requirements that the counterparty requires when forwarding - /// payments to us through this channel. - pub counterparty_forwarding_info: Option, } /// If a payment fails to send, it can be in one of several states. This enum is returned as the @@ -1170,30 +1178,32 @@ impl ChannelMana channel.get_holder_counterparty_selected_channel_reserve_satoshis(); res.push(ChannelDetails { channel_id: (*channel_id).clone(), + counterparty: ChannelCounterparty { + node_id: channel.get_counterparty_node_id(), + features: InitFeatures::empty(), + unspendable_punishment_reserve: to_remote_reserve_satoshis, + forwarding_info: channel.counterparty_forwarding_info(), + }, funding_txo: channel.get_funding_txo(), short_channel_id: channel.get_short_channel_id(), - remote_network_id: channel.get_counterparty_node_id(), - counterparty_features: InitFeatures::empty(), channel_value_satoshis: channel.get_value_satoshis(), - to_self_reserve_satoshis, - to_remote_reserve_satoshis, + unspendable_punishment_reserve: to_self_reserve_satoshis, inbound_capacity_msat, outbound_capacity_msat, user_id: channel.get_user_id(), confirmations_required: channel.minimum_depth(), - spend_csv_on_our_commitment_funds: channel.get_counterparty_selected_contest_delay(), + force_close_spend_delay: channel.get_counterparty_selected_contest_delay(), is_outbound: channel.is_outbound(), is_funding_locked: channel.is_usable(), is_usable: channel.is_live(), is_public: channel.should_announce(), - counterparty_forwarding_info: channel.counterparty_forwarding_info(), }); } } let per_peer_state = self.per_peer_state.read().unwrap(); for chan in res.iter_mut() { - if let Some(peer_state) = per_peer_state.get(&chan.remote_network_id) { - chan.counterparty_features = peer_state.lock().unwrap().latest_features.clone(); + if let Some(peer_state) = per_peer_state.get(&chan.counterparty.node_id) { + chan.counterparty.features = peer_state.lock().unwrap().latest_features.clone(); } } res @@ -4286,7 +4296,7 @@ impl if msg.channel_id == [0; 32] { for chan in self.list_channels() { - if chan.remote_network_id == *counterparty_node_id { + if chan.counterparty.node_id == *counterparty_node_id { // Untrusted messages from peer, we throw away the error if id points to a non-existent channel let _ = self.force_close_channel_with_peer(&chan.channel_id, Some(counterparty_node_id)); } diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 9da2d981e..6c14ebebd 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -459,10 +459,10 @@ pub fn get_route(our_node_id: &PublicKey, network: &NetworkGraph, paye if let Some(hops) = first_hops { for chan in hops { let short_channel_id = chan.short_channel_id.expect("first_hops should be filled in with usable channels, not pending ones"); - if chan.remote_network_id == *our_node_id { + if chan.counterparty.node_id == *our_node_id { return Err(LightningError{err: "First hop cannot have our_node_id as a destination.".to_owned(), action: ErrorAction::IgnoreError}); } - first_hop_targets.insert(chan.remote_network_id, (short_channel_id, chan.counterparty_features.to_context(), chan.outbound_capacity_msat, chan.counterparty_features.to_context())); + first_hop_targets.insert(chan.counterparty.node_id, (short_channel_id, chan.counterparty.features.to_context(), chan.outbound_capacity_msat, chan.counterparty.features.to_context())); } if first_hop_targets.is_empty() { return Err(LightningError{err: "Cannot route when there are no outbound routes away from us".to_owned(), action: ErrorAction::IgnoreError}); @@ -1200,6 +1200,30 @@ mod tests { use prelude::*; use std::sync::Arc; + fn get_channel_details(short_channel_id: Option, node_id: PublicKey, + features: InitFeatures, outbound_capacity_msat: u64) -> channelmanager::ChannelDetails { + channelmanager::ChannelDetails { + channel_id: [0; 32], + counterparty: channelmanager::ChannelCounterparty { + features, + node_id, + unspendable_punishment_reserve: 0, + forwarding_info: None, + }, + funding_txo: Some(OutPoint { txid: bitcoin::Txid::from_slice(&[0; 32]).unwrap(), index: 0 }), + short_channel_id, + channel_value_satoshis: 0, + user_id: 0, + outbound_capacity_msat, + inbound_capacity_msat: 42, + unspendable_punishment_reserve: None, + confirmations_required: None, + force_close_spend_delay: None, + is_outbound: true, is_funding_locked: true, + is_usable: true, is_public: true, + } + } + // Using the same keys for LN and BTC ids fn add_channel(net_graph_msg_handler: &NetGraphMsgHandler, Arc>, secp_ctx: &Secp256k1, node_1_privkey: &SecretKey, node_2_privkey: &SecretKey, features: ChannelFeatures, short_channel_id: u64) { @@ -1636,24 +1660,7 @@ mod tests { // Simple route to 2 via 1 - let our_chans = vec![channelmanager::ChannelDetails { - channel_id: [0; 32], - funding_txo: Some(OutPoint { txid: bitcoin::Txid::from_slice(&[0; 32]).unwrap(), index: 0 }), - short_channel_id: Some(2), - remote_network_id: our_id, - counterparty_features: InitFeatures::from_le_bytes(vec![0b11]), - channel_value_satoshis: 100000, - user_id: 0, - outbound_capacity_msat: 100000, - inbound_capacity_msat: 100000, - to_self_reserve_satoshis: None, - to_remote_reserve_satoshis: 0, - confirmations_required: None, - spend_csv_on_our_commitment_funds: None, - is_outbound: true, is_funding_locked: true, - is_usable: true, is_public: true, - counterparty_forwarding_info: None, - }]; + let our_chans = vec![get_channel_details(Some(2), our_id, InitFeatures::from_le_bytes(vec![0b11]), 100000)]; if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2], None, Some(&our_chans.iter().collect::>()), &Vec::new(), 100, 42, Arc::clone(&logger)) { assert_eq!(err, "First hop cannot have our_node_id as a destination."); @@ -1960,24 +1967,7 @@ mod tests { } else { panic!(); } // If we specify a channel to node7, that overrides our local channel view and that gets used - let our_chans = vec![channelmanager::ChannelDetails { - channel_id: [0; 32], - funding_txo: Some(OutPoint { txid: bitcoin::Txid::from_slice(&[0; 32]).unwrap(), index: 0 }), - short_channel_id: Some(42), - remote_network_id: nodes[7].clone(), - counterparty_features: InitFeatures::from_le_bytes(vec![0b11]), - channel_value_satoshis: 0, - user_id: 0, - outbound_capacity_msat: 250_000_000, - inbound_capacity_msat: 0, - to_self_reserve_satoshis: None, - to_remote_reserve_satoshis: 0, - confirmations_required: None, - spend_csv_on_our_commitment_funds: None, - is_outbound: true, is_funding_locked: true, - is_usable: true, is_public: true, - counterparty_forwarding_info: None, - }]; + let our_chans = vec![get_channel_details(Some(42), nodes[7].clone(), InitFeatures::from_le_bytes(vec![0b11]), 250_000_000)]; let route = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2], None, Some(&our_chans.iter().collect::>()), &Vec::new(), 100, 42, Arc::clone(&logger)).unwrap(); assert_eq!(route.paths[0].len(), 2); @@ -2014,24 +2004,7 @@ mod tests { } else { panic!(); } // If we specify a channel to node7, that overrides our local channel view and that gets used - let our_chans = vec![channelmanager::ChannelDetails { - channel_id: [0; 32], - funding_txo: Some(OutPoint { txid: bitcoin::Txid::from_slice(&[0; 32]).unwrap(), index: 0 }), - short_channel_id: Some(42), - remote_network_id: nodes[7].clone(), - counterparty_features: InitFeatures::from_le_bytes(vec![0b11]), - channel_value_satoshis: 0, - user_id: 0, - outbound_capacity_msat: 250_000_000, - inbound_capacity_msat: 0, - to_self_reserve_satoshis: None, - to_remote_reserve_satoshis: 0, - confirmations_required: None, - spend_csv_on_our_commitment_funds: None, - is_outbound: true, is_funding_locked: true, - is_usable: true, is_public: true, - counterparty_forwarding_info: None, - }]; + let our_chans = vec![get_channel_details(Some(42), nodes[7].clone(), InitFeatures::from_le_bytes(vec![0b11]), 250_000_000)]; let route = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2], None, Some(&our_chans.iter().collect::>()), &Vec::new(), 100, 42, Arc::clone(&logger)).unwrap(); assert_eq!(route.paths[0].len(), 2); @@ -2085,24 +2058,7 @@ mod tests { assert_eq!(route.paths[0][2].channel_features.le_flags(), &id_to_feature_flags(3)); // If we specify a channel to node7, that overrides our local channel view and that gets used - let our_chans = vec![channelmanager::ChannelDetails { - channel_id: [0; 32], - funding_txo: Some(OutPoint { txid: bitcoin::Txid::from_slice(&[0; 32]).unwrap(), index: 0 }), - short_channel_id: Some(42), - remote_network_id: nodes[7].clone(), - counterparty_features: InitFeatures::from_le_bytes(vec![0b11]), - channel_value_satoshis: 0, - user_id: 0, - outbound_capacity_msat: 250_000_000, - inbound_capacity_msat: 0, - to_self_reserve_satoshis: None, - to_remote_reserve_satoshis: 0, - confirmations_required: None, - spend_csv_on_our_commitment_funds: None, - is_outbound: true, is_funding_locked: true, - is_usable: true, is_public: true, - counterparty_forwarding_info: None, - }]; + let our_chans = vec![get_channel_details(Some(42), nodes[7].clone(), InitFeatures::from_le_bytes(vec![0b11]), 250_000_000)]; let route = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2], None, Some(&our_chans.iter().collect::>()), &Vec::new(), 100, 42, Arc::clone(&logger)).unwrap(); assert_eq!(route.paths[0].len(), 2); @@ -2228,24 +2184,7 @@ mod tests { let (_, our_id, _, nodes) = get_nodes(&secp_ctx); // Simple test with outbound channel to 4 to test that last_hops and first_hops connect - let our_chans = vec![channelmanager::ChannelDetails { - channel_id: [0; 32], - funding_txo: Some(OutPoint { txid: bitcoin::Txid::from_slice(&[0; 32]).unwrap(), index: 0 }), - short_channel_id: Some(42), - remote_network_id: nodes[3].clone(), - counterparty_features: InitFeatures::from_le_bytes(vec![0b11]), - channel_value_satoshis: 0, - user_id: 0, - outbound_capacity_msat: 250_000_000, - inbound_capacity_msat: 0, - to_self_reserve_satoshis: None, - to_remote_reserve_satoshis: 0, - confirmations_required: None, - spend_csv_on_our_commitment_funds: None, - is_outbound: true, is_funding_locked: true, - is_usable: true, is_public: true, - counterparty_forwarding_info: None, - }]; + let our_chans = vec![get_channel_details(Some(42), nodes[3].clone(), InitFeatures::from_le_bytes(vec![0b11]), 250_000_000)]; let mut last_hops = last_hops(&nodes); let route = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[6], None, Some(&our_chans.iter().collect::>()), &last_hops.iter().collect::>(), 100, 42, Arc::clone(&logger)).unwrap(); assert_eq!(route.paths[0].len(), 2); @@ -2359,24 +2298,7 @@ mod tests { htlc_minimum_msat: None, htlc_maximum_msat: last_hop_htlc_max, }]); - let our_chans = vec![channelmanager::ChannelDetails { - channel_id: [0; 32], - funding_txo: Some(OutPoint { txid: bitcoin::Txid::from_slice(&[0; 32]).unwrap(), index: 0 }), - short_channel_id: Some(42), - remote_network_id: middle_node_id, - counterparty_features: InitFeatures::from_le_bytes(vec![0b11]), - channel_value_satoshis: 100000, - user_id: 0, - outbound_capacity_msat: outbound_capacity_msat, - inbound_capacity_msat: 100000, - to_self_reserve_satoshis: None, - to_remote_reserve_satoshis: 0, - confirmations_required: None, - spend_csv_on_our_commitment_funds: None, - is_outbound: true, is_funding_locked: true, - is_usable: true, is_public: true, - counterparty_forwarding_info: None, - }]; + let our_chans = vec![get_channel_details(Some(42), middle_node_id, InitFeatures::from_le_bytes(vec![0b11]), outbound_capacity_msat)]; get_route(&source_node_id, &NetworkGraph::new(genesis_block(Network::Testnet).header.block_hash()), &target_node_id, None, Some(&our_chans.iter().collect::>()), &vec![&last_hops], route_val, 42, Arc::new(test_utils::TestLogger::new())) } @@ -2525,24 +2447,7 @@ mod tests { }); // Now, limit the first_hop by the outbound_capacity_msat of 200_000 sats. - let our_chans = vec![channelmanager::ChannelDetails { - channel_id: [0; 32], - funding_txo: Some(OutPoint { txid: bitcoin::Txid::from_slice(&[0; 32]).unwrap(), index: 0 }), - short_channel_id: Some(42), - remote_network_id: nodes[0].clone(), - counterparty_features: InitFeatures::from_le_bytes(vec![0b11]), - channel_value_satoshis: 0, - user_id: 0, - outbound_capacity_msat: 200_000_000, - inbound_capacity_msat: 0, - to_self_reserve_satoshis: None, - to_remote_reserve_satoshis: 0, - confirmations_required: None, - spend_csv_on_our_commitment_funds: None, - is_outbound: true, is_funding_locked: true, - is_usable: true, is_public: true, - counterparty_forwarding_info: None, - }]; + let our_chans = vec![get_channel_details(Some(42), nodes[0].clone(), InitFeatures::from_le_bytes(vec![0b11]), 200_000_000)]; { // Attempt to route more than available results in a failure.