Skip to content

Implement local channel tracking by Router at channel_update #207

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions fuzz/fuzz_targets/router_target.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,10 @@ pub fn do_test(data: &[u8]) {
remote_network_id: get_pubkey!(),
channel_value_satoshis: slice_to_be64(get_slice!(8)),
user_id: 0,
their_cltv_expiry_delta: 0,
their_htlc_minimum_msat: 0,
their_fee_base_msat: 0,
their_fee_proportional_millionths: 0,
});
}
Some(&first_hops_vec[..])
Expand Down
44 changes: 44 additions & 0 deletions src/ln/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,9 @@ pub(super) struct Channel {
their_to_self_delay: u16,
//implied by BREAKDOWN_TIMEOUT: our_to_self_delay: u16,
their_max_accepted_htlcs: u16,
their_cltv_expiry_delta: u16,
their_fee_base_msat: u32,
their_fee_proportional_millionths: u32,
//implied by OUR_MAX_HTLCS: our_max_accepted_htlcs: u16,
minimum_depth: u32,

Expand Down Expand Up @@ -491,6 +494,9 @@ impl Channel {
our_htlc_minimum_msat: Channel::derive_our_htlc_minimum_msat(feerate),
their_to_self_delay: 0,
their_max_accepted_htlcs: 0,
their_cltv_expiry_delta: 0,
their_fee_base_msat: 0,
their_fee_proportional_millionths: 0,
minimum_depth: 0, // Filled in in accept_channel

their_funding_pubkey: None,
Expand Down Expand Up @@ -681,6 +687,9 @@ impl Channel {
our_htlc_minimum_msat: Channel::derive_our_htlc_minimum_msat(msg.feerate_per_kw as u64),
their_to_self_delay: msg.to_self_delay,
their_max_accepted_htlcs: msg.max_accepted_htlcs,
their_cltv_expiry_delta: 0,
their_fee_base_msat: 0,
their_fee_proportional_millionths: 0,
minimum_depth: Channel::derive_minimum_depth(msg.funding_satoshis*1000, msg.push_msat),

their_funding_pubkey: Some(msg.funding_pubkey),
Expand Down Expand Up @@ -2040,6 +2049,20 @@ impl Channel {

}

/// Set channel fields with remote peer forwarding parameters, used mainly by ChannelDetails to feed the Router
/// If remote peer set htlc_minimum_msat to full channel value, we reject the whole update and close channel
pub fn channel_update(&mut self, msg: &msgs::ChannelUpdate) -> Result<(), ChannelError> {
if msg.contents.htlc_minimum_msat >= (self.channel_value_satoshis - self.their_channel_reserve_satoshis) * 1000 {
return Err(ChannelError::Close("Minimum htlc value is full channel value"));
}
self.their_cltv_expiry_delta = msg.contents.cltv_expiry_delta;
self.their_htlc_minimum_msat = msg.contents.htlc_minimum_msat;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be sanity-checked against funding_satoshis (see new_from_req), and while ideally we'd sanity-check against the channel_limits we dont keep those around and I think its probably OK to let counterparties update such things if they need to temporarily disable a channel. This needs to be documented in Config, though, noting that a counterparty can increase this value once the channel is established beyond the minimum, but this is equivalent to them just rejecting all HTLCs so we can't enforce it anyway.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, added comments in Config and sanity-check against channel_value_satoshis (funding_satoshis) and if fail reject the whole update, but should we send back a ChannelError:Close here, or as you said interpret it as temporary disabling of channel?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if they send back something completely bogus, nothing wrong with a ChannelError::Close.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, but don't do comparison for others fields because I think we don't have reference bases

self.their_fee_base_msat = msg.contents.fee_base_msat;
self.their_fee_proportional_millionths = msg.contents.fee_proportional_millionths;

Ok(())
}

/// Adds a pending update to this channel. See the doc for send_htlc for
/// further details on the optionness of the return value.
/// You MUST call send_commitment prior to any other calls on this Channel
Expand Down Expand Up @@ -2641,6 +2664,18 @@ impl Channel {
self.our_htlc_minimum_msat
}

pub fn get_their_cltv_expiry_delta(&self) -> u16 {
self.their_cltv_expiry_delta
}

pub fn get_their_fee_base_msat(&self) -> u32 {
self.their_fee_base_msat
}

pub fn get_their_fee_proportional_millionths(&self) -> u32 {
self.their_fee_proportional_millionths
}

pub fn get_value_satoshis(&self) -> u64 {
self.channel_value_satoshis
}
Expand Down Expand Up @@ -3587,6 +3622,9 @@ impl Writeable for Channel {
self.our_htlc_minimum_msat.write(writer)?;
self.their_to_self_delay.write(writer)?;
self.their_max_accepted_htlcs.write(writer)?;
self.their_cltv_expiry_delta.write(writer)?;
self.their_fee_base_msat.write(writer)?;
self.their_fee_proportional_millionths.write(writer)?;
self.minimum_depth.write(writer)?;

write_option!(self.their_funding_pubkey);
Expand Down Expand Up @@ -3761,6 +3799,9 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
let our_htlc_minimum_msat = Readable::read(reader)?;
let their_to_self_delay = Readable::read(reader)?;
let their_max_accepted_htlcs = Readable::read(reader)?;
let their_cltv_expiry_delta = Readable::read(reader)?;
let their_fee_base_msat = Readable::read(reader)?;
let their_fee_proportional_millionths = Readable::read(reader)?;
let minimum_depth = Readable::read(reader)?;

let their_funding_pubkey = read_option!();
Expand Down Expand Up @@ -3838,6 +3879,9 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
our_htlc_minimum_msat,
their_to_self_delay,
their_max_accepted_htlcs,
their_cltv_expiry_delta,
their_fee_base_msat,
their_fee_proportional_millionths,
minimum_depth,

their_funding_pubkey,
Expand Down
44 changes: 44 additions & 0 deletions src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,14 @@ pub struct ChannelDetails {
pub channel_value_satoshis: u64,
/// The user_id passed in to create_channel, or 0 if the channel was inbound.
pub user_id: u64,
/// The CLTV delta subtracted by the remote peer as a forwarding node to any incoming HTLC's cltv_expiry
pub their_cltv_expiry_delta: u16,
/// The minimum HTLC amount for which the remote peer as a forwarding node is ready to relay
pub their_htlc_minimum_msat: u64,
/// The base fee the remote peer as a forwarding node will take to relay any HTLC.
pub their_fee_base_msat: u32,
/// The proportional fee on amount forwarded by a HTLC the remote peer will take to relay.
pub their_fee_proportional_millionths: u32,
}

macro_rules! handle_error {
Expand Down Expand Up @@ -539,6 +547,10 @@ impl ChannelManager {
remote_network_id: channel.get_their_node_id(),
channel_value_satoshis: channel.get_value_satoshis(),
user_id: channel.get_user_id(),
their_cltv_expiry_delta: channel.get_their_cltv_expiry_delta(),
their_htlc_minimum_msat: channel.get_their_htlc_minimum_msat(),
their_fee_base_msat: channel.get_their_fee_base_msat(),
their_fee_proportional_millionths: channel.get_their_fee_proportional_millionths(),
});
}
res
Expand All @@ -560,6 +572,10 @@ impl ChannelManager {
remote_network_id: channel.get_their_node_id(),
channel_value_satoshis: channel.get_value_satoshis(),
user_id: channel.get_user_id(),
their_cltv_expiry_delta: channel.get_their_cltv_expiry_delta(),
their_htlc_minimum_msat: channel.get_their_htlc_minimum_msat(),
their_fee_base_msat: channel.get_their_fee_base_msat(),
their_fee_proportional_millionths: channel.get_their_fee_proportional_millionths(),
});
}
}
Expand Down Expand Up @@ -2444,6 +2460,29 @@ impl ChannelManager {
Ok(())
}

fn internal_channel_update(&self, their_node_id: &PublicKey, msg: &msgs::ChannelUpdate) -> Result<(), MsgHandleErrInternal> {
let mut channel_state_lock = self.channel_state.lock().unwrap();
let channel_state = channel_state_lock.borrow_parts();
let chan_id = match channel_state.short_to_id.get(&msg.contents.short_channel_id) {
Some(chan_id) => chan_id.clone(),
None => {
// It's not a local channel
return Ok(())
}
};
match channel_state.by_id.entry(chan_id) {
hash_map::Entry::Occupied(mut chan) => {
if chan.get().get_their_node_id() != *their_node_id {
//TODO: see issue #153, need a consistent behavior on obnoxious behavior from random node
return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!", chan_id));
}
try_chan_entry!(self, chan.get_mut().channel_update(&msg), channel_state, chan);
},
hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel", chan_id))
}
Ok(())
}

fn internal_channel_reestablish(&self, their_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(), MsgHandleErrInternal> {
let mut channel_state_lock = self.channel_state.lock().unwrap();
let channel_state = channel_state_lock.borrow_parts();
Expand Down Expand Up @@ -2776,6 +2815,11 @@ impl ChannelMessageHandler for ChannelManager {
handle_error!(self, self.internal_announcement_signatures(their_node_id, msg), their_node_id)
}

fn handle_channel_update(&self, their_node_id: &PublicKey, msg: &msgs::ChannelUpdate) -> Result<(), HandleError> {
let _ = self.total_consistency_lock.read().unwrap();
handle_error!(self, self.internal_channel_update(their_node_id, msg), their_node_id)
}

fn handle_channel_reestablish(&self, their_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(), HandleError> {
let _ = self.total_consistency_lock.read().unwrap();
handle_error!(self, self.internal_channel_reestablish(their_node_id, msg), their_node_id)
Expand Down
2 changes: 2 additions & 0 deletions src/ln/msgs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,8 @@ pub trait ChannelMessageHandler : events::MessageSendEventsProvider + Send + Syn
// Channel-to-announce:
/// Handle an incoming announcement_signatures message from the given peer.
fn handle_announcement_signatures(&self, their_node_id: &PublicKey, msg: &AnnouncementSignatures) -> Result<(), HandleError>;
/// Handle an incoming channel_update message from the given peer.
fn handle_channel_update(&self, their_node_id: &PublicKey, msg: &ChannelUpdate) -> Result<(), HandleError>;

// Connection loss/reestablish:
/// Indicates a connection to the peer failed/an existing connection was lost. If no connection
Expand Down
1 change: 1 addition & 0 deletions src/ln/peer_handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,7 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
},
258 => {
let msg = try_potential_decodeerror!(msgs::ChannelUpdate::read(&mut reader));
try_potential_handleerror!(self.message_handler.chan_handler.handle_channel_update(&peer.their_node_id.unwrap(), &msg));
let should_forward = try_potential_handleerror!(self.message_handler.route_handler.handle_channel_update(&msg));

if should_forward {
Expand Down
8 changes: 8 additions & 0 deletions src/ln/router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1292,6 +1292,10 @@ mod tests {
remote_network_id: node8.clone(),
channel_value_satoshis: 0,
user_id: 0,
their_cltv_expiry_delta: 0,
their_htlc_minimum_msat: 0,
their_fee_base_msat: 0,
their_fee_proportional_millionths: 0,
}];
let route = router.get_route(&node3, Some(&our_chans), &Vec::new(), 100, 42).unwrap();
assert_eq!(route.hops.len(), 2);
Expand Down Expand Up @@ -1367,6 +1371,10 @@ mod tests {
remote_network_id: node4.clone(),
channel_value_satoshis: 0,
user_id: 0,
their_cltv_expiry_delta: 0,
their_htlc_minimum_msat: 0,
their_fee_base_msat: 0,
their_fee_proportional_millionths: 0,
}];
let route = router.get_route(&node7, Some(&our_chans), &last_hops, 100, 42).unwrap();
assert_eq!(route.hops.len(), 2);
Expand Down
4 changes: 3 additions & 1 deletion src/util/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ pub struct ChannelHandshakeLimits {
/// only applies to inbound channels.
pub min_funding_satoshis: u64,
/// The remote node sets a limit on the minimum size of HTLCs we can send to them. This allows
/// you to limit the maximum minimum-size they can require.
/// you to limit the maximum minimum-size they can require. Remote node may increase its
/// htlc_minimum_msat beyond limits with a channel_update msg, as it's equivalent to them
/// just rejecting all HTLCs we can't enforce it.
pub max_htlc_minimum_msat: u64,
/// The remote node sets a limit on the maximum value of pending HTLCs to them at any given
/// time to limit their funds exposure to HTLCs. This allows you to set a minimum such value.
Expand Down
3 changes: 3 additions & 0 deletions src/util/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,9 @@ impl msgs::ChannelMessageHandler for TestChannelMessageHandler {
fn handle_announcement_signatures(&self, _their_node_id: &PublicKey, _msg: &msgs::AnnouncementSignatures) -> Result<(), HandleError> {
Err(HandleError { err: "", action: None })
}
fn handle_channel_update(&self, _their_node_id: &PublicKey, _msg: &msgs::ChannelUpdate) -> Result<(), HandleError> {
Err(HandleError { err: "", action: None })
}
fn handle_channel_reestablish(&self, _their_node_id: &PublicKey, _msg: &msgs::ChannelReestablish) -> Result<(), HandleError> {
Err(HandleError { err: "", action: None })
}
Expand Down