Skip to content

Plumb Features through into Routes#433

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.

Sign up for GitHub

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

Merged
TheBlueMatt merged 4 commits into from
Jan 22, 2020
Merged

Plumb Features through into Routes #433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d2ba7ca
Pass peer's Init message through to ChannelManager
TheBlueMatt Dec 28, 2019
a19d71d
Keep track of the Init Features for every connected/channel'd peer
TheBlueMatt Dec 29, 2019
912f877
Pass node features through to RouteHops
TheBlueMatt Dec 28, 2019
617a680
DRY-up list_channels by having a common lister that takes a filter
TheBlueMatt Jan 17, 2020
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
18 changes: 12 additions & 6 deletions fuzz/src/chanmon_consistency.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -29,8 +29,8 @@ use lightning::ln::channelmonitor;
use lightning::ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateErr, HTLCUpdate};
use lightning::ln::channelmanager::{ChannelManager, PaymentHash, PaymentPreimage, ChannelManagerReadArgs};
use lightning::ln::router::{Route, RouteHop};
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC};
use lightning::ln::features::{ChannelFeatures, InitFeatures, NodeFeatures};
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC, Init};
use lightning::util::enforcing_trait_impls::EnforcingChannelKeys;
use lightning::util::events;
use lightning::util::logger::Logger;
Expand DownExpand Up@@ -414,7 +414,9 @@ pub fn do_test(data: &[u8]) {
if let Err(_) = $source.send_payment(Route {
hops: vec![RouteHop {
pubkey: $dest.0.get_our_node_id(),
node_features: NodeFeatures::empty(),
short_channel_id: $dest.1,
channel_features: ChannelFeatures::empty(),
fee_msat: 5000000,
cltv_expiry_delta: 200,
}],
Expand All@@ -429,12 +431,16 @@ pub fn do_test(data: &[u8]) {
if let Err(_) = $source.send_payment(Route {
hops: vec![RouteHop {
pubkey: $middle.0.get_our_node_id(),
node_features: NodeFeatures::empty(),
short_channel_id: $middle.1,
channel_features: ChannelFeatures::empty(),
fee_msat: 50000,
cltv_expiry_delta: 100,
},RouteHop {
pubkey: $dest.0.get_our_node_id(),
node_features: NodeFeatures::empty(),
short_channel_id: $dest.1,
channel_features: ChannelFeatures::empty(),
fee_msat: 5000000,
cltv_expiry_delta: 200,
}],
Expand DownExpand Up@@ -650,15 +656,15 @@ pub fn do_test(data: &[u8]) {
},
0x11 => {
if chan_a_disconnected {
nodes[0].peer_connected(&nodes[1].get_our_node_id());
nodes[1].peer_connected(&nodes[0].get_our_node_id());
nodes[0].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
nodes[1].peer_connected(&nodes[0].get_our_node_id(), &Init { features: InitFeatures::empty() });
chan_a_disconnected = false;
}
},
0x12 => {
if chan_b_disconnected {
nodes[1].peer_connected(&nodes[2].get_our_node_id());
nodes[2].peer_connected(&nodes[1].get_our_node_id());
nodes[1].peer_connected(&nodes[2].get_our_node_id(), &Init { features: InitFeatures::empty() });
nodes[2].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
chan_b_disconnected = false;
}
},
Expand Down
4 changes: 3 additions & 1 deletion fuzz/src/router.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -5,8 +5,9 @@ use bitcoin::blockdata::transaction::Transaction;

use lightning::chain::chaininterface::{ChainError,ChainWatchInterface};
use lightning::ln::channelmanager::ChannelDetails;
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs;
use lightning::ln::msgs::{RoutingMessageHandler};
use lightning::ln::msgs::RoutingMessageHandler;
use lightning::ln::router::{Router, RouteHint};
use lightning::util::logger::Logger;
use lightning::util::ser::Readable;
Expand DownExpand Up@@ -198,6 +199,7 @@ pub fn do_test(data: &[u8]) {
channel_id: [0; 32],
short_channel_id: Some(slice_to_be64(get_slice!(8))),
remote_network_id: get_pubkey!(),
counterparty_features: InitFeatures::empty(),
channel_value_satoshis: slice_to_be64(get_slice!(8)),
user_id: 0,
inbound_capacity_msat: 0,
Expand Down
24 changes: 12 additions & 12 deletions lightning/src/ln/chanmon_update_fail_tests.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -215,10 +215,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
assert_eq!(reestablish_1.len(), 1);
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
assert_eq!(reestablish_2.len(), 1);

Expand All@@ -237,10 +237,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());

nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
assert_eq!(reestablish_1.len(), 1);
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
assert_eq!(reestablish_2.len(), 1);

Expand DownExpand Up@@ -938,8 +938,8 @@ fn test_monitor_update_fail_reestablish() {
commitment_signed_dance!(nodes[1], nodes[2], updates.commitment_signed, false);

*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

let as_reestablish = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
let bs_reestablish = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand All@@ -954,8 +954,8 @@ fn test_monitor_update_fail_reestablish() {
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);

nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

assert!(as_reestablish == get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id()));
assert!(bs_reestablish == get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id()));
Expand DownExpand Up@@ -1118,8 +1118,8 @@ fn claim_while_disconnected_monitor_update_fail() {
assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
check_added_monitors!(nodes[1], 1);

nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand DownExpand Up@@ -1246,8 +1246,8 @@ fn monitor_failed_no_reestablish_response() {
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand Down
131 changes: 95 additions & 36 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -275,6 +275,12 @@ pub(super) struct ChannelHolder<ChanSigner: ChannelKeys> {
pub(super) pending_msg_events: Vec<events::MessageSendEvent>,
}

/// State we hold per-peer. In the future we should put channels in here, but for now we only hold
/// the latest Init features we heard from the peer.
struct PeerState {
latest_features: InitFeatures,

@jkczyzjkczyzJan 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

High-level question: Given this is duplicated in PeerManager's Peer structs, has there been any though around designing the API in terms of Peer and Channel abstractions?

@TheBlueMattTheBlueMattJan 16, 2020

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

I'm not sure what you mean - you mean something like replacing the their_node_id argument with some struct that holds more information (eg the init message)? We could, I suppose, though note that latest_features here is intended to be available even while disconnected (though maybe it doesn't actually need to be, just nice to have).

@jkczyzjkczyzJan 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm mostly thinking whether it would be suitable to design the overall API (not just this module) in terms of Peer and Channel abstractions and use them throughout to build higher-level abstractions like ChannelManager. Admittedly, this is outside the scope of this PR.

@TheBlueMattTheBlueMattJan 16, 2020

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Hmm. Maybe. It would certainly be tricky because a Peer "owns" a channel, but also the Peer can disconnect and reconnect and get back the same Channel. We can explore more in the future.

}

#[cfg(not(any(target_pointer_width = "32", target_pointer_width = "64")))]
const ERR: () = "You need at least 32 bit pointers (well, usize, but we'll assume they're the same) for ChannelManager::latest_block_height";

Expand DownExpand Up@@ -328,6 +334,14 @@ pub struct ChannelManager<ChanSigner: ChannelKeys> {
channel_state: Mutex<ChannelHolder<ChanSigner>>,
our_network_key: SecretKey,

/// The bulk of our storage will eventually be here (channels and message queues and the like).
/// If we are connected to a peer we always at least have an entry here, even if no channels
/// are currently open with that peer.
/// Because adding or removing an entry is rare, we usually take an outer read lock and then
/// operate on the inner value freely. Sadly, this prevents parallel operation when opening a
/// new channel.
Comment on lines +340 to +342

@jkczyzjkczyzJan 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure that I follow what is meant by "usually" here. Can it be removed?

@TheBlueMattTheBlueMattJan 16, 2020

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

As in, if you add or remove a connection you'll need the outer write lock, but usually we dont so take the read lock.

per_peer_state: RwLock<HashMap<PublicKey, Mutex<PeerState>>>,

pending_events: Mutex<Vec<events::Event>>,
/// Used when we have to take a BIG lock to make sure everything is self-consistent.
/// Essentially just when we're serializing ourselves out.
Expand DownExpand Up@@ -390,6 +404,10 @@ pub struct ChannelDetails {
pub short_channel_id: Option<u64>,
/// 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 user_id passed in to create_channel, or 0 if the channel was inbound.
Expand DownExpand Up@@ -610,6 +628,8 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
}),
our_network_key: keys_manager.get_node_secret(),

per_peer_state: RwLock::new(HashMap::new()),

pending_events: Mutex::new(Vec::new()),
total_consistency_lock: RwLock::new(()),

Expand DownExpand Up@@ -660,56 +680,53 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
Ok(())
}

/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
/// more information.
pub fn list_channels(&self) -> Vec<ChannelDetails> {
let channel_state = self.channel_state.lock().unwrap();
let mut res = Vec::with_capacity(channel_state.by_id.len());
for (channel_id, channel) in channel_state.by_id.iter() {
let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
res.push(ChannelDetails {
channel_id: (*channel_id).clone(),
short_channel_id: channel.get_short_channel_id(),
remote_network_id: channel.get_their_node_id(),
channel_value_satoshis: channel.get_value_satoshis(),
inbound_capacity_msat,
outbound_capacity_msat,
user_id: channel.get_user_id(),
is_live: channel.is_live(),
});
}
res
}

/// Gets the list of usable channels, in random order. Useful as an argument to
/// Router::get_route to ensure non-announced channels are used.
///
/// These are guaranteed to have their is_live value set to true, see the documentation for
/// ChannelDetails::is_live for more info on exactly what the criteria are.
pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
let channel_state = self.channel_state.lock().unwrap();
let mut res = Vec::with_capacity(channel_state.by_id.len());
for (channel_id, channel) in channel_state.by_id.iter() {
// Note we use is_live here instead of usable which leads to somewhat confused
// internal/external nomenclature, but that's ok cause that's probably what the user
// really wanted anyway.
if channel.is_live() {
fn list_channels_with_filter<F: FnMut(&(&[u8; 32], &Channel<ChanSigner>)) -> bool>(&self, f: F) -> Vec<ChannelDetails> {
let mut res = Vec::new();
{
let channel_state = self.channel_state.lock().unwrap();
res.reserve(channel_state.by_id.len());
for (channel_id, channel) in channel_state.by_id.iter().filter(f) {
let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
res.push(ChannelDetails {
channel_id: (*channel_id).clone(),
short_channel_id: channel.get_short_channel_id(),
remote_network_id: channel.get_their_node_id(),
counterparty_features: InitFeatures::empty(),
channel_value_satoshis: channel.get_value_satoshis(),
inbound_capacity_msat,
outbound_capacity_msat,
user_id: channel.get_user_id(),
is_live: true,
is_live: channel.is_live(),
});
}
}
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();
}
}
res
}

/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
/// more information.
pub fn list_channels(&self) -> Vec<ChannelDetails> {
self.list_channels_with_filter(|_| true)
}

/// Gets the list of usable channels, in random order. Useful as an argument to
/// Router::get_route to ensure non-announced channels are used.
///
/// These are guaranteed to have their is_live value set to true, see the documentation for
/// ChannelDetails::is_live for more info on exactly what the criteria are.
pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
// Note we use is_live here instead of usable which leads to somewhat confused
// internal/external nomenclature, but that's ok cause that's probably what the user
// really wanted anyway.
self.list_channels_with_filter(|&(_, ref channel)| channel.is_live())
}

/// Begins the process of closing a channel. After this call (plus some timeout), no new HTLCs
/// will be accepted on the given channel, and after additional timeout/the closing of all
/// pending HTLCs, the channel will be closed on chain.
Expand DownExpand Up@@ -2780,6 +2797,7 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
let _ = self.total_consistency_lock.read().unwrap();
let mut failed_channels = Vec::new();
let mut failed_payments = Vec::new();
let mut no_channels_remain = true;
{
let mut channel_state_lock = self.channel_state.lock().unwrap();
let channel_state = &mut *channel_state_lock;
Expand DownExpand Up@@ -2818,6 +2836,8 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
short_to_id.remove(&short_id);
}
return false;
} else {
no_channels_remain = false;
}
}
true
Expand All@@ -2843,6 +2863,10 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
}
});
}
if no_channels_remain {
self.per_peer_state.write().unwrap().remove(their_node_id);
}

for failure in failed_channels.drain(..) {
self.finish_force_close_channel(failure);
}
Expand All@@ -2853,10 +2877,25 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
}
}

fn peer_connected(&self, their_node_id: &PublicKey) {
fn peer_connected(&self, their_node_id: &PublicKey, init_msg: &msgs::Init) {
log_debug!(self, "Generating channel_reestablish events for {}", log_pubkey!(their_node_id));

let _ = self.total_consistency_lock.read().unwrap();

{
let mut peer_state_lock = self.per_peer_state.write().unwrap();
match peer_state_lock.entry(their_node_id.clone()) {
hash_map::Entry::Vacant(e) => {
e.insert(Mutex::new(PeerState {
latest_features: init_msg.features.clone(),
}));
},
hash_map::Entry::Occupied(e) => {

@ariardariardJan 7, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is there any risk of peer collision ? An attacker can usurp pubkey available on the net but without privkey it won't get through the noise phase..and if a peer leaks its privkey we can't guarantee consistency of our structs (and we may have bigger troubles)

@TheBlueMattTheBlueMattJan 7, 2020

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Right. If you can impersonate a peer, that peer is screwed. Not a lot we can do then.

@jkczyzjkczyzJan 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When would this occur? Won't PeerManager disconnect when it sees a duplicate id?

https://github.com/rust-bitcoin/rust-lightning/blob/a252f81477c25bf1248fd9ea5269a6620187c487/lightning/src/ln/peer_handler.rs#L540-L554

@TheBlueMattTheBlueMattJan 16, 2020

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

If you're using our PeerManager, yes, this should not be possible, though in theory these functions are public and the user may run their own PeerManager-like thing that may not properly check.

@jkczyzjkczyzJan 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What sort of semantics do we want to provide to users in this case and where do we plan on documenting it? I can see three possibilities:

  1. Return an error / panic
  2. Overwrite the peer state with a new entry
  3. Update the peer state with new data

For (1), my assumption is (that if used properly) there should already have been a peer_disconnected event even if this is a reconnect. So the entry would not exist in that case. Is my understanding accurate?

Currently (2) and (3) are equivalent but may not be in the future. You're implementing this as (3) though the code would be simplified if (2) is the desired behavior (i.e., you could blindly insert into the map instead of matching on the entry).

@TheBlueMattTheBlueMattJan 16, 2020

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Good question. I think in general if you don't follow the API, there are no guarantees period. It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key, but if a counterparty's private key leaks, we won't panic, it just may result in the counterparty losing their funds/channels.

@ariardariardJan 17, 2020•
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Currently (2) and (3) are equivalent but may not be in the future.

Hmmm not sure I would change, it's state relative to the connection it shouldn't leak to the ChannelManager layer, if it's concerning channel+routing, assuming LN messages are ordered newer ones should take precedence.

It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key

Transport is assumed to be authenticated and encrypted so verification than message came from something which owns the relevant private key is done I think. That's said identity of the public key responder, how do you establish this knowledge is beyond specs. Keys leakage means your funds are loss, just send a HTLC emptying your balance to some exit node, it doesn't enter in the scope of option_upfront_shutdown_script.

e.get().lock().unwrap().latest_features = init_msg.features.clone();
},
}
}

let mut channel_state_lock = self.channel_state.lock().unwrap();
let channel_state = &mut *channel_state_lock;
let pending_msg_events = &mut channel_state.pending_msg_events;
Expand DownExpand Up@@ -3123,6 +3162,14 @@ impl<ChanSigner: ChannelKeys + Writeable> Writeable for ChannelManager<ChanSigne
}
}

let per_peer_state = self.per_peer_state.write().unwrap();
(per_peer_state.len() as u64).write(writer)?;
for (peer_pubkey, peer_state_mutex) in per_peer_state.iter() {
peer_pubkey.write(writer)?;
let peer_state = peer_state_mutex.lock().unwrap();
peer_state.latest_features.write(writer)?;
}

Ok(())
}
}
Expand DownExpand Up@@ -3256,6 +3303,16 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
claimable_htlcs.insert(payment_hash, previous_hops);
}

let peer_count: u64 = Readable::read(reader)?;
let mut per_peer_state = HashMap::with_capacity(cmp::min(peer_count as usize, 128));
for _ in 0..peer_count {
let peer_pubkey = Readable::read(reader)?;
let peer_state = PeerState {
latest_features: Readable::read(reader)?,
};
per_peer_state.insert(peer_pubkey, Mutex::new(peer_state));
}

let channel_manager = ChannelManager {
genesis_hash,
fee_estimator: args.fee_estimator,
Expand All@@ -3275,6 +3332,8 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
}),
our_network_key: args.keys_manager.get_node_secret(),

per_peer_state: RwLock::new(per_peer_state),

pending_events: Mutex::new(Vec::new()),
total_consistency_lock: RwLock::new(()),
keys_manager: args.keys_manager,
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Plumb Features through into Routes by TheBlueMatt · Pull Request #433 · lightningdevkit/rust-lightning · GitHub
Skip to content
Plumb Features through into Routes#433

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.

 Sign up for GitHub

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

 Merged

TheBlueMatt
merged 4 commits into

from

Jan 22, 2020
 Merged


Plumb Features through into Routes
#433
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


4 commits


Select commit
Hold shift + click to select a range
d2ba7ca

Pass peer's Init message through to ChannelManager


TheBlueMatt Dec 28, 2019
a19d71d

Keep track of the Init Features for every connected/channel'd peer


TheBlueMatt Dec 29, 2019
912f877

Pass node features through to RouteHops


TheBlueMatt Dec 28, 2019
617a680

DRY-up list_channels by having a common lister that takes a filter


TheBlueMatt Jan 17, 2020

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


18 changes: 12 additions & 6 deletions
fuzz/src/chanmon_consistency.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -29,8 +29,8 @@ use lightning::ln::channelmonitor;
use lightning::ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateErr, HTLCUpdate};
use lightning::ln::channelmanager::{ChannelManager, PaymentHash, PaymentPreimage, ChannelManagerReadArgs};
use lightning::ln::router::{Route, RouteHop};
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC};
use lightning::ln::features::{ChannelFeatures, InitFeatures, NodeFeatures};
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC, Init};
use lightning::util::enforcing_trait_impls::EnforcingChannelKeys;
use lightning::util::events;
use lightning::util::logger::Logger;
Expand DownExpand Up@@ -414,7 +414,9 @@ pub fn do_test(data: &[u8]) {
 if let Err(_) = $source.send_payment(Route {
 hops: vec![RouteHop {
 pubkey: $dest.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $dest.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 5000000,
 cltv_expiry_delta: 200,
 }],
Expand All@@ -429,12 +431,16 @@ pub fn do_test(data: &[u8]) {
 if let Err(_) = $source.send_payment(Route {
 hops: vec![RouteHop {
 pubkey: $middle.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $middle.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 50000,
 cltv_expiry_delta: 100,
 },RouteHop {
 pubkey: $dest.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $dest.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 5000000,
 cltv_expiry_delta: 200,
 }],
Expand DownExpand Up@@ -650,15 +656,15 @@ pub fn do_test(data: &[u8]) {
 },
 0x11 => {
 if chan_a_disconnected {
 nodes[0].peer_connected(&nodes[1].get_our_node_id());
 nodes[1].peer_connected(&nodes[0].get_our_node_id());
 nodes[0].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
 nodes[1].peer_connected(&nodes[0].get_our_node_id(), &Init { features: InitFeatures::empty() });
 chan_a_disconnected = false;
 }
 },
 0x12 => {
 if chan_b_disconnected {
 nodes[1].peer_connected(&nodes[2].get_our_node_id());
 nodes[2].peer_connected(&nodes[1].get_our_node_id());
 nodes[1].peer_connected(&nodes[2].get_our_node_id(), &Init { features: InitFeatures::empty() });
 nodes[2].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
 chan_b_disconnected = false;
 }
 },
Expand Down

4 changes: 3 additions & 1 deletion
fuzz/src/router.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -5,8 +5,9 @@ use bitcoin::blockdata::transaction::Transaction;

use lightning::chain::chaininterface::{ChainError,ChainWatchInterface};
use lightning::ln::channelmanager::ChannelDetails;
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs;
use lightning::ln::msgs::{RoutingMessageHandler};
use lightning::ln::msgs::RoutingMessageHandler;
use lightning::ln::router::{Router, RouteHint};
use lightning::util::logger::Logger;
use lightning::util::ser::Readable;
Expand DownExpand Up@@ -198,6 +199,7 @@ pub fn do_test(data: &[u8]) {
 channel_id: [0; 32],
 short_channel_id: Some(slice_to_be64(get_slice!(8))),
 remote_network_id: get_pubkey!(),
 counterparty_features: InitFeatures::empty(),
 channel_value_satoshis: slice_to_be64(get_slice!(8)),
 user_id: 0,
 inbound_capacity_msat: 0,
Expand Down

24 changes: 12 additions & 12 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -215,10 +215,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
 nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
 assert_eq!(reestablish_1.len(), 1);
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
 assert_eq!(reestablish_2.len(), 1);

Expand All@@ -237,10 +237,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
 assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());

 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
 assert_eq!(reestablish_1.len(), 1);
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
 assert_eq!(reestablish_2.len(), 1);

Expand DownExpand Up@@ -938,8 +938,8 @@ fn test_monitor_update_fail_reestablish() {
	commitment_signed_dance!(nodes[1], nodes[2], updates.commitment_signed, false);

	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reestablish = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reestablish = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand All@@ -954,8 +954,8 @@ fn test_monitor_update_fail_reestablish() {
	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	assert!(as_reestablish == get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id()));
	assert!(bs_reestablish == get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id()));
Expand DownExpand Up@@ -1118,8 +1118,8 @@ fn claim_while_disconnected_monitor_update_fail() {
	assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
	check_added_monitors!(nodes[1], 1);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand DownExpand Up@@ -1246,8 +1246,8 @@ fn monitor_failed_no_reestablish_response() {
	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand Down

131 changes: 95 additions & 36 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -275,6 +275,12 @@ pub(super) struct ChannelHolder<ChanSigner: ChannelKeys> {
	pub(super) pending_msg_events: Vec<events::MessageSendEvent>,
}

/// State we hold per-peer. In the future we should put channels in here, but for now we only hold
/// the latest Init features we heard from the peer.
struct PeerState {
	latest_features: InitFeatures,
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

High-level question: Given this is duplicated in PeerManager's Peer structs, has there been any though around designing the API in terms of Peer and Channel abstractions?
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not sure what you mean - you mean something like replacing the their_node_id argument with some struct that holds more information (eg the init message)? We could, I suppose, though note that latest_features here is intended to be available even while disconnected (though maybe it doesn't actually need to be, just nice to have).
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm mostly thinking whether it would be suitable to design the overall API (not just this module) in terms of Peer and Channel abstractions and use them throughout to build higher-level abstractions like ChannelManager. Admittedly, this is outside the scope of this PR.
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Hmm. Maybe. It would certainly be tricky because a Peer "owns" a channel, but also the Peer can disconnect and reconnect and get back the same Channel. We can explore more in the future.
}

#[cfg(not(any(target_pointer_width = "32", target_pointer_width = "64")))]
const ERR: () = "You need at least 32 bit pointers (well, usize, but we'll assume they're the same) for ChannelManager::latest_block_height";

Expand DownExpand Up@@ -328,6 +334,14 @@ pub struct ChannelManager<ChanSigner: ChannelKeys> {
	channel_state: Mutex<ChannelHolder<ChanSigner>>,
	our_network_key: SecretKey,

	/// The bulk of our storage will eventually be here (channels and message queues and the like).
	/// If we are connected to a peer we always at least have an entry here, even if no channels
	/// are currently open with that peer.
	/// Because adding or removing an entry is rare, we usually take an outer read lock and then
	/// operate on the inner value freely. Sadly, this prevents parallel operation when opening a
	/// new channel.

Comment on lines

+340
 to 
+342

@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Not sure that I follow what is meant by "usually" here. Can it be removed?
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

As in, if you add or remove a connection you'll need the outer write lock, but usually we dont so take the read lock.
	per_peer_state: RwLock<HashMap<PublicKey, Mutex<PeerState>>>,

	pending_events: Mutex<Vec<events::Event>>,
	/// Used when we have to take a BIG lock to make sure everything is self-consistent.
	/// Essentially just when we're serializing ourselves out.
Expand DownExpand Up@@ -390,6 +404,10 @@ pub struct ChannelDetails {
	pub short_channel_id: Option<u64>,
	/// 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 user_id passed in to create_channel, or 0 if the channel was inbound.
Expand DownExpand Up@@ -610,6 +628,8 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
 }),
 our_network_key: keys_manager.get_node_secret(),

 per_peer_state: RwLock::new(HashMap::new()),

 pending_events: Mutex::new(Vec::new()),
 total_consistency_lock: RwLock::new(()),

Expand DownExpand Up@@ -660,56 +680,53 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
 Ok(())
	}

	/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
	/// more information.
	pub fn list_channels(&self) -> Vec<ChannelDetails> {
 let channel_state = self.channel_state.lock().unwrap();
 let mut res = Vec::with_capacity(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter() {
 let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
 res.push(ChannelDetails {
 channel_id: (*channel_id).clone(),
 short_channel_id: channel.get_short_channel_id(),
 remote_network_id: channel.get_their_node_id(),
 channel_value_satoshis: channel.get_value_satoshis(),
 inbound_capacity_msat,
 outbound_capacity_msat,
 user_id: channel.get_user_id(),
 is_live: channel.is_live(),
 });
 }
 res
	}

	/// Gets the list of usable channels, in random order. Useful as an argument to
	/// Router::get_route to ensure non-announced channels are used.
	///
	/// These are guaranteed to have their is_live value set to true, see the documentation for
	/// ChannelDetails::is_live for more info on exactly what the criteria are.
	pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
 let channel_state = self.channel_state.lock().unwrap();
 let mut res = Vec::with_capacity(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter() {
 // Note we use is_live here instead of usable which leads to somewhat confused
 // internal/external nomenclature, but that's ok cause that's probably what the user
 // really wanted anyway.
 if channel.is_live() {
	fn list_channels_with_filter<F: FnMut(&(&[u8; 32], &Channel<ChanSigner>)) -> bool>(&self, f: F) -> Vec<ChannelDetails> {
 let mut res = Vec::new();
 {
 let channel_state = self.channel_state.lock().unwrap();
 res.reserve(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter().filter(f) {
 let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
 res.push(ChannelDetails {
 channel_id: (*channel_id).clone(),
 short_channel_id: channel.get_short_channel_id(),
 remote_network_id: channel.get_their_node_id(),
 counterparty_features: InitFeatures::empty(),
 channel_value_satoshis: channel.get_value_satoshis(),
 inbound_capacity_msat,
 outbound_capacity_msat,
 user_id: channel.get_user_id(),
 is_live: true,
 is_live: channel.is_live(),
 });
 }
 }
 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();
 }
 }
 res
	}

	/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
	/// more information.
	pub fn list_channels(&self) -> Vec<ChannelDetails> {
 self.list_channels_with_filter(|_| true)
	}

	/// Gets the list of usable channels, in random order. Useful as an argument to
	/// Router::get_route to ensure non-announced channels are used.
	///
	/// These are guaranteed to have their is_live value set to true, see the documentation for
	/// ChannelDetails::is_live for more info on exactly what the criteria are.
	pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
 // Note we use is_live here instead of usable which leads to somewhat confused
 // internal/external nomenclature, but that's ok cause that's probably what the user
 // really wanted anyway.
 self.list_channels_with_filter(|&(_, ref channel)| channel.is_live())
	}

	/// Begins the process of closing a channel. After this call (plus some timeout), no new HTLCs
	/// will be accepted on the given channel, and after additional timeout/the closing of all
	/// pending HTLCs, the channel will be closed on chain.
Expand DownExpand Up@@ -2780,6 +2797,7 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 let _ = self.total_consistency_lock.read().unwrap();
 let mut failed_channels = Vec::new();
 let mut failed_payments = Vec::new();
 let mut no_channels_remain = true;
 {
 let mut channel_state_lock = self.channel_state.lock().unwrap();
 let channel_state = &mut *channel_state_lock;
Expand DownExpand Up@@ -2818,6 +2836,8 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 short_to_id.remove(&short_id);
 }
 return false;
 } else {
 no_channels_remain = false;
 }
 }
 true
Expand All@@ -2843,6 +2863,10 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 }
 });
 }
 if no_channels_remain {
 self.per_peer_state.write().unwrap().remove(their_node_id);
 }

 for failure in failed_channels.drain(..) {
 self.finish_force_close_channel(failure);
 }
Expand All@@ -2853,10 +2877,25 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 }
	}

	fn peer_connected(&self, their_node_id: &PublicKey) {
	fn peer_connected(&self, their_node_id: &PublicKey, init_msg: &msgs::Init) {
 log_debug!(self, "Generating channel_reestablish events for {}", log_pubkey!(their_node_id));

 let _ = self.total_consistency_lock.read().unwrap();

 {
 let mut peer_state_lock = self.per_peer_state.write().unwrap();
 match peer_state_lock.entry(their_node_id.clone()) {
 hash_map::Entry::Vacant(e) => {
 e.insert(Mutex::new(PeerState {
 latest_features: init_msg.features.clone(),
 }));
 },
 hash_map::Entry::Occupied(e) => {
@ariardariardJan 7, 2020

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Is there any risk of peer collision ? An attacker can usurp pubkey available on the net but without privkey it won't get through the noise phase..and if a peer leaks its privkey we can't guarantee consistency of our structs (and we may have bigger troubles)
@TheBlueMattTheBlueMattJan 7, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Right. If you can impersonate a peer, that peer is screwed. Not a lot we can do then.
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

When would this occur? Won't PeerManager disconnect when it sees a duplicate id?
https://github.com/rust-bitcoin/rust-lightning/blob/a252f81477c25bf1248fd9ea5269a6620187c487/lightning/src/ln/peer_handler.rs#L540-L554
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

If you're using our PeerManager, yes, this should not be possible, though in theory these functions are public and the user may run their own PeerManager-like thing that may not properly check.
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What sort of semantics do we want to provide to users in this case and where do we plan on documenting it? I can see three possibilities:
  1. Return an error / panic
  2. Overwrite the peer state with a new entry
  3. Update the peer state with new data
For (1), my assumption is (that if used properly) there should already have been a peer_disconnected event even if this is a reconnect. So the entry would not exist in that case. Is my understanding accurate?
Currently (2) and (3) are equivalent but may not be in the future. You're implementing this as (3) though the code would be simplified if (2) is the desired behavior (i.e., you could blindly insert into the map instead of matching on the entry).
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Good question. I think in general if you don't follow the API, there are no guarantees period. It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key, but if a counterparty's private key leaks, we won't panic, it just may result in the counterparty losing their funds/channels.
@ariardariardJan 17, 2020•

edited

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Currently (2) and (3) are equivalent but may not be in the future.
Hmmm not sure I would change, it's state relative to the connection it shouldn't leak to the ChannelManager layer, if it's concerning channel+routing, assuming LN messages are ordered newer ones should take precedence.
It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key
Transport is assumed to be authenticated and encrypted so verification than message came from something which owns the relevant private key is done I think. That's said identity of the public key responder, how do you establish this knowledge is beyond specs. Keys leakage means your funds are loss, just send a HTLC emptying your balance to some exit node, it doesn't enter in the scope of option_upfront_shutdown_script.
 e.get().lock().unwrap().latest_features = init_msg.features.clone();
 },
 }
 }

 let mut channel_state_lock = self.channel_state.lock().unwrap();
 let channel_state = &mut *channel_state_lock;
 let pending_msg_events = &mut channel_state.pending_msg_events;
Expand DownExpand Up@@ -3123,6 +3162,14 @@ impl<ChanSigner: ChannelKeys + Writeable> Writeable for ChannelManager<ChanSigne
 }
 }

 let per_peer_state = self.per_peer_state.write().unwrap();
 (per_peer_state.len() as u64).write(writer)?;
 for (peer_pubkey, peer_state_mutex) in per_peer_state.iter() {
 peer_pubkey.write(writer)?;
 let peer_state = peer_state_mutex.lock().unwrap();
 peer_state.latest_features.write(writer)?;
 }

 Ok(())
	}
}
Expand DownExpand Up@@ -3256,6 +3303,16 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
 claimable_htlcs.insert(payment_hash, previous_hops);
 }

 let peer_count: u64 = Readable::read(reader)?;
 let mut per_peer_state = HashMap::with_capacity(cmp::min(peer_count as usize, 128));
 for _ in 0..peer_count {
 let peer_pubkey = Readable::read(reader)?;
 let peer_state = PeerState {
 latest_features: Readable::read(reader)?,
 };
 per_peer_state.insert(peer_pubkey, Mutex::new(peer_state));
 }

 let channel_manager = ChannelManager {
 genesis_hash,
 fee_estimator: args.fee_estimator,
Expand All@@ -3275,6 +3332,8 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
 }),
 our_network_key: args.keys_manager.get_node_secret(),

 per_peer_state: RwLock::new(per_peer_state),

 pending_events: Mutex::new(Vec::new()),
 total_consistency_lock: RwLock::new(()),
 keys_manager: args.keys_manager,
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Plumb Features through into Routes by TheBlueMatt · Pull Request #433 · lightningdevkit/rust-lightning · GitHub
Skip to content
Plumb Features through into Routes#433

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.

 Sign up for GitHub

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

 Merged

TheBlueMatt
merged 4 commits into

from

Jan 22, 2020
 Merged


Plumb Features through into Routes
#433
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


4 commits


Select commit
Hold shift + click to select a range
d2ba7ca

Pass peer's Init message through to ChannelManager


TheBlueMatt Dec 28, 2019
a19d71d

Keep track of the Init Features for every connected/channel'd peer


TheBlueMatt Dec 29, 2019
912f877

Pass node features through to RouteHops


TheBlueMatt Dec 28, 2019
617a680

DRY-up list_channels by having a common lister that takes a filter


TheBlueMatt Jan 17, 2020

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


18 changes: 12 additions & 6 deletions
fuzz/src/chanmon_consistency.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -29,8 +29,8 @@ use lightning::ln::channelmonitor;
use lightning::ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateErr, HTLCUpdate};
use lightning::ln::channelmanager::{ChannelManager, PaymentHash, PaymentPreimage, ChannelManagerReadArgs};
use lightning::ln::router::{Route, RouteHop};
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC};
use lightning::ln::features::{ChannelFeatures, InitFeatures, NodeFeatures};
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC, Init};
use lightning::util::enforcing_trait_impls::EnforcingChannelKeys;
use lightning::util::events;
use lightning::util::logger::Logger;
Expand DownExpand Up@@ -414,7 +414,9 @@ pub fn do_test(data: &[u8]) {
 if let Err(_) = $source.send_payment(Route {
 hops: vec![RouteHop {
 pubkey: $dest.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $dest.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 5000000,
 cltv_expiry_delta: 200,
 }],
Expand All@@ -429,12 +431,16 @@ pub fn do_test(data: &[u8]) {
 if let Err(_) = $source.send_payment(Route {
 hops: vec![RouteHop {
 pubkey: $middle.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $middle.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 50000,
 cltv_expiry_delta: 100,
 },RouteHop {
 pubkey: $dest.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $dest.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 5000000,
 cltv_expiry_delta: 200,
 }],
Expand DownExpand Up@@ -650,15 +656,15 @@ pub fn do_test(data: &[u8]) {
 },
 0x11 => {
 if chan_a_disconnected {
 nodes[0].peer_connected(&nodes[1].get_our_node_id());
 nodes[1].peer_connected(&nodes[0].get_our_node_id());
 nodes[0].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
 nodes[1].peer_connected(&nodes[0].get_our_node_id(), &Init { features: InitFeatures::empty() });
 chan_a_disconnected = false;
 }
 },
 0x12 => {
 if chan_b_disconnected {
 nodes[1].peer_connected(&nodes[2].get_our_node_id());
 nodes[2].peer_connected(&nodes[1].get_our_node_id());
 nodes[1].peer_connected(&nodes[2].get_our_node_id(), &Init { features: InitFeatures::empty() });
 nodes[2].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
 chan_b_disconnected = false;
 }
 },
Expand Down

4 changes: 3 additions & 1 deletion
fuzz/src/router.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -5,8 +5,9 @@ use bitcoin::blockdata::transaction::Transaction;

use lightning::chain::chaininterface::{ChainError,ChainWatchInterface};
use lightning::ln::channelmanager::ChannelDetails;
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs;
use lightning::ln::msgs::{RoutingMessageHandler};
use lightning::ln::msgs::RoutingMessageHandler;
use lightning::ln::router::{Router, RouteHint};
use lightning::util::logger::Logger;
use lightning::util::ser::Readable;
Expand DownExpand Up@@ -198,6 +199,7 @@ pub fn do_test(data: &[u8]) {
 channel_id: [0; 32],
 short_channel_id: Some(slice_to_be64(get_slice!(8))),
 remote_network_id: get_pubkey!(),
 counterparty_features: InitFeatures::empty(),
 channel_value_satoshis: slice_to_be64(get_slice!(8)),
 user_id: 0,
 inbound_capacity_msat: 0,
Expand Down

24 changes: 12 additions & 12 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -215,10 +215,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
 nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
 assert_eq!(reestablish_1.len(), 1);
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
 assert_eq!(reestablish_2.len(), 1);

Expand All@@ -237,10 +237,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
 assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());

 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
 assert_eq!(reestablish_1.len(), 1);
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
 assert_eq!(reestablish_2.len(), 1);

Expand DownExpand Up@@ -938,8 +938,8 @@ fn test_monitor_update_fail_reestablish() {
	commitment_signed_dance!(nodes[1], nodes[2], updates.commitment_signed, false);

	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reestablish = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reestablish = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand All@@ -954,8 +954,8 @@ fn test_monitor_update_fail_reestablish() {
	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	assert!(as_reestablish == get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id()));
	assert!(bs_reestablish == get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id()));
Expand DownExpand Up@@ -1118,8 +1118,8 @@ fn claim_while_disconnected_monitor_update_fail() {
	assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
	check_added_monitors!(nodes[1], 1);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand DownExpand Up@@ -1246,8 +1246,8 @@ fn monitor_failed_no_reestablish_response() {
	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand Down

131 changes: 95 additions & 36 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -275,6 +275,12 @@ pub(super) struct ChannelHolder<ChanSigner: ChannelKeys> {
	pub(super) pending_msg_events: Vec<events::MessageSendEvent>,
}

/// State we hold per-peer. In the future we should put channels in here, but for now we only hold
/// the latest Init features we heard from the peer.
struct PeerState {
	latest_features: InitFeatures,
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

High-level question: Given this is duplicated in PeerManager's Peer structs, has there been any though around designing the API in terms of Peer and Channel abstractions?
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not sure what you mean - you mean something like replacing the their_node_id argument with some struct that holds more information (eg the init message)? We could, I suppose, though note that latest_features here is intended to be available even while disconnected (though maybe it doesn't actually need to be, just nice to have).
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm mostly thinking whether it would be suitable to design the overall API (not just this module) in terms of Peer and Channel abstractions and use them throughout to build higher-level abstractions like ChannelManager. Admittedly, this is outside the scope of this PR.
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Hmm. Maybe. It would certainly be tricky because a Peer "owns" a channel, but also the Peer can disconnect and reconnect and get back the same Channel. We can explore more in the future.
}

#[cfg(not(any(target_pointer_width = "32", target_pointer_width = "64")))]
const ERR: () = "You need at least 32 bit pointers (well, usize, but we'll assume they're the same) for ChannelManager::latest_block_height";

Expand DownExpand Up@@ -328,6 +334,14 @@ pub struct ChannelManager<ChanSigner: ChannelKeys> {
	channel_state: Mutex<ChannelHolder<ChanSigner>>,
	our_network_key: SecretKey,

	/// The bulk of our storage will eventually be here (channels and message queues and the like).
	/// If we are connected to a peer we always at least have an entry here, even if no channels
	/// are currently open with that peer.
	/// Because adding or removing an entry is rare, we usually take an outer read lock and then
	/// operate on the inner value freely. Sadly, this prevents parallel operation when opening a
	/// new channel.

Comment on lines

+340
 to 
+342

@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Not sure that I follow what is meant by "usually" here. Can it be removed?
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

As in, if you add or remove a connection you'll need the outer write lock, but usually we dont so take the read lock.
	per_peer_state: RwLock<HashMap<PublicKey, Mutex<PeerState>>>,

	pending_events: Mutex<Vec<events::Event>>,
	/// Used when we have to take a BIG lock to make sure everything is self-consistent.
	/// Essentially just when we're serializing ourselves out.
Expand DownExpand Up@@ -390,6 +404,10 @@ pub struct ChannelDetails {
	pub short_channel_id: Option<u64>,
	/// 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 user_id passed in to create_channel, or 0 if the channel was inbound.
Expand DownExpand Up@@ -610,6 +628,8 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
 }),
 our_network_key: keys_manager.get_node_secret(),

 per_peer_state: RwLock::new(HashMap::new()),

 pending_events: Mutex::new(Vec::new()),
 total_consistency_lock: RwLock::new(()),

Expand DownExpand Up@@ -660,56 +680,53 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
 Ok(())
	}

	/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
	/// more information.
	pub fn list_channels(&self) -> Vec<ChannelDetails> {
 let channel_state = self.channel_state.lock().unwrap();
 let mut res = Vec::with_capacity(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter() {
 let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
 res.push(ChannelDetails {
 channel_id: (*channel_id).clone(),
 short_channel_id: channel.get_short_channel_id(),
 remote_network_id: channel.get_their_node_id(),
 channel_value_satoshis: channel.get_value_satoshis(),
 inbound_capacity_msat,
 outbound_capacity_msat,
 user_id: channel.get_user_id(),
 is_live: channel.is_live(),
 });
 }
 res
	}

	/// Gets the list of usable channels, in random order. Useful as an argument to
	/// Router::get_route to ensure non-announced channels are used.
	///
	/// These are guaranteed to have their is_live value set to true, see the documentation for
	/// ChannelDetails::is_live for more info on exactly what the criteria are.
	pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
 let channel_state = self.channel_state.lock().unwrap();
 let mut res = Vec::with_capacity(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter() {
 // Note we use is_live here instead of usable which leads to somewhat confused
 // internal/external nomenclature, but that's ok cause that's probably what the user
 // really wanted anyway.
 if channel.is_live() {
	fn list_channels_with_filter<F: FnMut(&(&[u8; 32], &Channel<ChanSigner>)) -> bool>(&self, f: F) -> Vec<ChannelDetails> {
 let mut res = Vec::new();
 {
 let channel_state = self.channel_state.lock().unwrap();
 res.reserve(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter().filter(f) {
 let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
 res.push(ChannelDetails {
 channel_id: (*channel_id).clone(),
 short_channel_id: channel.get_short_channel_id(),
 remote_network_id: channel.get_their_node_id(),
 counterparty_features: InitFeatures::empty(),
 channel_value_satoshis: channel.get_value_satoshis(),
 inbound_capacity_msat,
 outbound_capacity_msat,
 user_id: channel.get_user_id(),
 is_live: true,
 is_live: channel.is_live(),
 });
 }
 }
 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();
 }
 }
 res
	}

	/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
	/// more information.
	pub fn list_channels(&self) -> Vec<ChannelDetails> {
 self.list_channels_with_filter(|_| true)
	}

	/// Gets the list of usable channels, in random order. Useful as an argument to
	/// Router::get_route to ensure non-announced channels are used.
	///
	/// These are guaranteed to have their is_live value set to true, see the documentation for
	/// ChannelDetails::is_live for more info on exactly what the criteria are.
	pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
 // Note we use is_live here instead of usable which leads to somewhat confused
 // internal/external nomenclature, but that's ok cause that's probably what the user
 // really wanted anyway.
 self.list_channels_with_filter(|&(_, ref channel)| channel.is_live())
	}

	/// Begins the process of closing a channel. After this call (plus some timeout), no new HTLCs
	/// will be accepted on the given channel, and after additional timeout/the closing of all
	/// pending HTLCs, the channel will be closed on chain.
Expand DownExpand Up@@ -2780,6 +2797,7 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 let _ = self.total_consistency_lock.read().unwrap();
 let mut failed_channels = Vec::new();
 let mut failed_payments = Vec::new();
 let mut no_channels_remain = true;
 {
 let mut channel_state_lock = self.channel_state.lock().unwrap();
 let channel_state = &mut *channel_state_lock;
Expand DownExpand Up@@ -2818,6 +2836,8 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 short_to_id.remove(&short_id);
 }
 return false;
 } else {
 no_channels_remain = false;
 }
 }
 true
Expand All@@ -2843,6 +2863,10 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 }
 });
 }
 if no_channels_remain {
 self.per_peer_state.write().unwrap().remove(their_node_id);
 }

 for failure in failed_channels.drain(..) {
 self.finish_force_close_channel(failure);
 }
Expand All@@ -2853,10 +2877,25 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 }
	}

	fn peer_connected(&self, their_node_id: &PublicKey) {
	fn peer_connected(&self, their_node_id: &PublicKey, init_msg: &msgs::Init) {
 log_debug!(self, "Generating channel_reestablish events for {}", log_pubkey!(their_node_id));

 let _ = self.total_consistency_lock.read().unwrap();

 {
 let mut peer_state_lock = self.per_peer_state.write().unwrap();
 match peer_state_lock.entry(their_node_id.clone()) {
 hash_map::Entry::Vacant(e) => {
 e.insert(Mutex::new(PeerState {
 latest_features: init_msg.features.clone(),
 }));
 },
 hash_map::Entry::Occupied(e) => {
@ariardariardJan 7, 2020

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Is there any risk of peer collision ? An attacker can usurp pubkey available on the net but without privkey it won't get through the noise phase..and if a peer leaks its privkey we can't guarantee consistency of our structs (and we may have bigger troubles)
@TheBlueMattTheBlueMattJan 7, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Right. If you can impersonate a peer, that peer is screwed. Not a lot we can do then.
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

When would this occur? Won't PeerManager disconnect when it sees a duplicate id?
https://github.com/rust-bitcoin/rust-lightning/blob/a252f81477c25bf1248fd9ea5269a6620187c487/lightning/src/ln/peer_handler.rs#L540-L554
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

If you're using our PeerManager, yes, this should not be possible, though in theory these functions are public and the user may run their own PeerManager-like thing that may not properly check.
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What sort of semantics do we want to provide to users in this case and where do we plan on documenting it? I can see three possibilities:
  1. Return an error / panic
  2. Overwrite the peer state with a new entry
  3. Update the peer state with new data
For (1), my assumption is (that if used properly) there should already have been a peer_disconnected event even if this is a reconnect. So the entry would not exist in that case. Is my understanding accurate?
Currently (2) and (3) are equivalent but may not be in the future. You're implementing this as (3) though the code would be simplified if (2) is the desired behavior (i.e., you could blindly insert into the map instead of matching on the entry).
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Good question. I think in general if you don't follow the API, there are no guarantees period. It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key, but if a counterparty's private key leaks, we won't panic, it just may result in the counterparty losing their funds/channels.
@ariardariardJan 17, 2020•

edited

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Currently (2) and (3) are equivalent but may not be in the future.
Hmmm not sure I would change, it's state relative to the connection it shouldn't leak to the ChannelManager layer, if it's concerning channel+routing, assuming LN messages are ordered newer ones should take precedence.
It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key
Transport is assumed to be authenticated and encrypted so verification than message came from something which owns the relevant private key is done I think. That's said identity of the public key responder, how do you establish this knowledge is beyond specs. Keys leakage means your funds are loss, just send a HTLC emptying your balance to some exit node, it doesn't enter in the scope of option_upfront_shutdown_script.
 e.get().lock().unwrap().latest_features = init_msg.features.clone();
 },
 }
 }

 let mut channel_state_lock = self.channel_state.lock().unwrap();
 let channel_state = &mut *channel_state_lock;
 let pending_msg_events = &mut channel_state.pending_msg_events;
Expand DownExpand Up@@ -3123,6 +3162,14 @@ impl<ChanSigner: ChannelKeys + Writeable> Writeable for ChannelManager<ChanSigne
 }
 }

 let per_peer_state = self.per_peer_state.write().unwrap();
 (per_peer_state.len() as u64).write(writer)?;
 for (peer_pubkey, peer_state_mutex) in per_peer_state.iter() {
 peer_pubkey.write(writer)?;
 let peer_state = peer_state_mutex.lock().unwrap();
 peer_state.latest_features.write(writer)?;
 }

 Ok(())
	}
}
Expand DownExpand Up@@ -3256,6 +3303,16 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
 claimable_htlcs.insert(payment_hash, previous_hops);
 }

 let peer_count: u64 = Readable::read(reader)?;
 let mut per_peer_state = HashMap::with_capacity(cmp::min(peer_count as usize, 128));
 for _ in 0..peer_count {
 let peer_pubkey = Readable::read(reader)?;
 let peer_state = PeerState {
 latest_features: Readable::read(reader)?,
 };
 per_peer_state.insert(peer_pubkey, Mutex::new(peer_state));
 }

 let channel_manager = ChannelManager {
 genesis_hash,
 fee_estimator: args.fee_estimator,
Expand All@@ -3275,6 +3332,8 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
 }),
 our_network_key: args.keys_manager.get_node_secret(),

 per_peer_state: RwLock::new(per_peer_state),

 pending_events: Mutex::new(Vec::new()),
 total_consistency_lock: RwLock::new(()),
 keys_manager: args.keys_manager,
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Plumb Features through into Routes by TheBlueMatt · Pull Request #433 · lightningdevkit/rust-lightning · GitHub
Skip to content
Plumb Features through into Routes#433

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.

 Sign up for GitHub

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

 Merged

TheBlueMatt
merged 4 commits into

from

Jan 22, 2020
 Merged


Plumb Features through into Routes
#433
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


4 commits


Select commit
Hold shift + click to select a range
d2ba7ca

Pass peer's Init message through to ChannelManager


TheBlueMatt Dec 28, 2019
a19d71d

Keep track of the Init Features for every connected/channel'd peer


TheBlueMatt Dec 29, 2019
912f877

Pass node features through to RouteHops


TheBlueMatt Dec 28, 2019
617a680

DRY-up list_channels by having a common lister that takes a filter


TheBlueMatt Jan 17, 2020

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


18 changes: 12 additions & 6 deletions
fuzz/src/chanmon_consistency.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -29,8 +29,8 @@ use lightning::ln::channelmonitor;
use lightning::ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateErr, HTLCUpdate};
use lightning::ln::channelmanager::{ChannelManager, PaymentHash, PaymentPreimage, ChannelManagerReadArgs};
use lightning::ln::router::{Route, RouteHop};
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC};
use lightning::ln::features::{ChannelFeatures, InitFeatures, NodeFeatures};
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC, Init};
use lightning::util::enforcing_trait_impls::EnforcingChannelKeys;
use lightning::util::events;
use lightning::util::logger::Logger;
Expand DownExpand Up@@ -414,7 +414,9 @@ pub fn do_test(data: &[u8]) {
 if let Err(_) = $source.send_payment(Route {
 hops: vec![RouteHop {
 pubkey: $dest.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $dest.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 5000000,
 cltv_expiry_delta: 200,
 }],
Expand All@@ -429,12 +431,16 @@ pub fn do_test(data: &[u8]) {
 if let Err(_) = $source.send_payment(Route {
 hops: vec![RouteHop {
 pubkey: $middle.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $middle.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 50000,
 cltv_expiry_delta: 100,
 },RouteHop {
 pubkey: $dest.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $dest.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 5000000,
 cltv_expiry_delta: 200,
 }],
Expand DownExpand Up@@ -650,15 +656,15 @@ pub fn do_test(data: &[u8]) {
 },
 0x11 => {
 if chan_a_disconnected {
 nodes[0].peer_connected(&nodes[1].get_our_node_id());
 nodes[1].peer_connected(&nodes[0].get_our_node_id());
 nodes[0].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
 nodes[1].peer_connected(&nodes[0].get_our_node_id(), &Init { features: InitFeatures::empty() });
 chan_a_disconnected = false;
 }
 },
 0x12 => {
 if chan_b_disconnected {
 nodes[1].peer_connected(&nodes[2].get_our_node_id());
 nodes[2].peer_connected(&nodes[1].get_our_node_id());
 nodes[1].peer_connected(&nodes[2].get_our_node_id(), &Init { features: InitFeatures::empty() });
 nodes[2].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
 chan_b_disconnected = false;
 }
 },
Expand Down

4 changes: 3 additions & 1 deletion
fuzz/src/router.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -5,8 +5,9 @@ use bitcoin::blockdata::transaction::Transaction;

use lightning::chain::chaininterface::{ChainError,ChainWatchInterface};
use lightning::ln::channelmanager::ChannelDetails;
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs;
use lightning::ln::msgs::{RoutingMessageHandler};
use lightning::ln::msgs::RoutingMessageHandler;
use lightning::ln::router::{Router, RouteHint};
use lightning::util::logger::Logger;
use lightning::util::ser::Readable;
Expand DownExpand Up@@ -198,6 +199,7 @@ pub fn do_test(data: &[u8]) {
 channel_id: [0; 32],
 short_channel_id: Some(slice_to_be64(get_slice!(8))),
 remote_network_id: get_pubkey!(),
 counterparty_features: InitFeatures::empty(),
 channel_value_satoshis: slice_to_be64(get_slice!(8)),
 user_id: 0,
 inbound_capacity_msat: 0,
Expand Down

24 changes: 12 additions & 12 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -215,10 +215,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
 nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
 assert_eq!(reestablish_1.len(), 1);
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
 assert_eq!(reestablish_2.len(), 1);

Expand All@@ -237,10 +237,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
 assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());

 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
 assert_eq!(reestablish_1.len(), 1);
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
 assert_eq!(reestablish_2.len(), 1);

Expand DownExpand Up@@ -938,8 +938,8 @@ fn test_monitor_update_fail_reestablish() {
	commitment_signed_dance!(nodes[1], nodes[2], updates.commitment_signed, false);

	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reestablish = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reestablish = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand All@@ -954,8 +954,8 @@ fn test_monitor_update_fail_reestablish() {
	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	assert!(as_reestablish == get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id()));
	assert!(bs_reestablish == get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id()));
Expand DownExpand Up@@ -1118,8 +1118,8 @@ fn claim_while_disconnected_monitor_update_fail() {
	assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
	check_added_monitors!(nodes[1], 1);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand DownExpand Up@@ -1246,8 +1246,8 @@ fn monitor_failed_no_reestablish_response() {
	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand Down

131 changes: 95 additions & 36 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -275,6 +275,12 @@ pub(super) struct ChannelHolder<ChanSigner: ChannelKeys> {
	pub(super) pending_msg_events: Vec<events::MessageSendEvent>,
}

/// State we hold per-peer. In the future we should put channels in here, but for now we only hold
/// the latest Init features we heard from the peer.
struct PeerState {
	latest_features: InitFeatures,
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

High-level question: Given this is duplicated in PeerManager's Peer structs, has there been any though around designing the API in terms of Peer and Channel abstractions?
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not sure what you mean - you mean something like replacing the their_node_id argument with some struct that holds more information (eg the init message)? We could, I suppose, though note that latest_features here is intended to be available even while disconnected (though maybe it doesn't actually need to be, just nice to have).
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm mostly thinking whether it would be suitable to design the overall API (not just this module) in terms of Peer and Channel abstractions and use them throughout to build higher-level abstractions like ChannelManager. Admittedly, this is outside the scope of this PR.
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Hmm. Maybe. It would certainly be tricky because a Peer "owns" a channel, but also the Peer can disconnect and reconnect and get back the same Channel. We can explore more in the future.
}

#[cfg(not(any(target_pointer_width = "32", target_pointer_width = "64")))]
const ERR: () = "You need at least 32 bit pointers (well, usize, but we'll assume they're the same) for ChannelManager::latest_block_height";

Expand DownExpand Up@@ -328,6 +334,14 @@ pub struct ChannelManager<ChanSigner: ChannelKeys> {
	channel_state: Mutex<ChannelHolder<ChanSigner>>,
	our_network_key: SecretKey,

	/// The bulk of our storage will eventually be here (channels and message queues and the like).
	/// If we are connected to a peer we always at least have an entry here, even if no channels
	/// are currently open with that peer.
	/// Because adding or removing an entry is rare, we usually take an outer read lock and then
	/// operate on the inner value freely. Sadly, this prevents parallel operation when opening a
	/// new channel.

Comment on lines

+340
 to 
+342

@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Not sure that I follow what is meant by "usually" here. Can it be removed?
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

As in, if you add or remove a connection you'll need the outer write lock, but usually we dont so take the read lock.
	per_peer_state: RwLock<HashMap<PublicKey, Mutex<PeerState>>>,

	pending_events: Mutex<Vec<events::Event>>,
	/// Used when we have to take a BIG lock to make sure everything is self-consistent.
	/// Essentially just when we're serializing ourselves out.
Expand DownExpand Up@@ -390,6 +404,10 @@ pub struct ChannelDetails {
	pub short_channel_id: Option<u64>,
	/// 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 user_id passed in to create_channel, or 0 if the channel was inbound.
Expand DownExpand Up@@ -610,6 +628,8 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
 }),
 our_network_key: keys_manager.get_node_secret(),

 per_peer_state: RwLock::new(HashMap::new()),

 pending_events: Mutex::new(Vec::new()),
 total_consistency_lock: RwLock::new(()),

Expand DownExpand Up@@ -660,56 +680,53 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
 Ok(())
	}

	/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
	/// more information.
	pub fn list_channels(&self) -> Vec<ChannelDetails> {
 let channel_state = self.channel_state.lock().unwrap();
 let mut res = Vec::with_capacity(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter() {
 let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
 res.push(ChannelDetails {
 channel_id: (*channel_id).clone(),
 short_channel_id: channel.get_short_channel_id(),
 remote_network_id: channel.get_their_node_id(),
 channel_value_satoshis: channel.get_value_satoshis(),
 inbound_capacity_msat,
 outbound_capacity_msat,
 user_id: channel.get_user_id(),
 is_live: channel.is_live(),
 });
 }
 res
	}

	/// Gets the list of usable channels, in random order. Useful as an argument to
	/// Router::get_route to ensure non-announced channels are used.
	///
	/// These are guaranteed to have their is_live value set to true, see the documentation for
	/// ChannelDetails::is_live for more info on exactly what the criteria are.
	pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
 let channel_state = self.channel_state.lock().unwrap();
 let mut res = Vec::with_capacity(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter() {
 // Note we use is_live here instead of usable which leads to somewhat confused
 // internal/external nomenclature, but that's ok cause that's probably what the user
 // really wanted anyway.
 if channel.is_live() {
	fn list_channels_with_filter<F: FnMut(&(&[u8; 32], &Channel<ChanSigner>)) -> bool>(&self, f: F) -> Vec<ChannelDetails> {
 let mut res = Vec::new();
 {
 let channel_state = self.channel_state.lock().unwrap();
 res.reserve(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter().filter(f) {
 let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
 res.push(ChannelDetails {
 channel_id: (*channel_id).clone(),
 short_channel_id: channel.get_short_channel_id(),
 remote_network_id: channel.get_their_node_id(),
 counterparty_features: InitFeatures::empty(),
 channel_value_satoshis: channel.get_value_satoshis(),
 inbound_capacity_msat,
 outbound_capacity_msat,
 user_id: channel.get_user_id(),
 is_live: true,
 is_live: channel.is_live(),
 });
 }
 }
 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();
 }
 }
 res
	}

	/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
	/// more information.
	pub fn list_channels(&self) -> Vec<ChannelDetails> {
 self.list_channels_with_filter(|_| true)
	}

	/// Gets the list of usable channels, in random order. Useful as an argument to
	/// Router::get_route to ensure non-announced channels are used.
	///
	/// These are guaranteed to have their is_live value set to true, see the documentation for
	/// ChannelDetails::is_live for more info on exactly what the criteria are.
	pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
 // Note we use is_live here instead of usable which leads to somewhat confused
 // internal/external nomenclature, but that's ok cause that's probably what the user
 // really wanted anyway.
 self.list_channels_with_filter(|&(_, ref channel)| channel.is_live())
	}

	/// Begins the process of closing a channel. After this call (plus some timeout), no new HTLCs
	/// will be accepted on the given channel, and after additional timeout/the closing of all
	/// pending HTLCs, the channel will be closed on chain.
Expand DownExpand Up@@ -2780,6 +2797,7 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 let _ = self.total_consistency_lock.read().unwrap();
 let mut failed_channels = Vec::new();
 let mut failed_payments = Vec::new();
 let mut no_channels_remain = true;
 {
 let mut channel_state_lock = self.channel_state.lock().unwrap();
 let channel_state = &mut *channel_state_lock;
Expand DownExpand Up@@ -2818,6 +2836,8 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 short_to_id.remove(&short_id);
 }
 return false;
 } else {
 no_channels_remain = false;
 }
 }
 true
Expand All@@ -2843,6 +2863,10 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 }
 });
 }
 if no_channels_remain {
 self.per_peer_state.write().unwrap().remove(their_node_id);
 }

 for failure in failed_channels.drain(..) {
 self.finish_force_close_channel(failure);
 }
Expand All@@ -2853,10 +2877,25 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 }
	}

	fn peer_connected(&self, their_node_id: &PublicKey) {
	fn peer_connected(&self, their_node_id: &PublicKey, init_msg: &msgs::Init) {
 log_debug!(self, "Generating channel_reestablish events for {}", log_pubkey!(their_node_id));

 let _ = self.total_consistency_lock.read().unwrap();

 {
 let mut peer_state_lock = self.per_peer_state.write().unwrap();
 match peer_state_lock.entry(their_node_id.clone()) {
 hash_map::Entry::Vacant(e) => {
 e.insert(Mutex::new(PeerState {
 latest_features: init_msg.features.clone(),
 }));
 },
 hash_map::Entry::Occupied(e) => {
@ariardariardJan 7, 2020

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Is there any risk of peer collision ? An attacker can usurp pubkey available on the net but without privkey it won't get through the noise phase..and if a peer leaks its privkey we can't guarantee consistency of our structs (and we may have bigger troubles)
@TheBlueMattTheBlueMattJan 7, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Right. If you can impersonate a peer, that peer is screwed. Not a lot we can do then.
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

When would this occur? Won't PeerManager disconnect when it sees a duplicate id?
https://github.com/rust-bitcoin/rust-lightning/blob/a252f81477c25bf1248fd9ea5269a6620187c487/lightning/src/ln/peer_handler.rs#L540-L554
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

If you're using our PeerManager, yes, this should not be possible, though in theory these functions are public and the user may run their own PeerManager-like thing that may not properly check.
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What sort of semantics do we want to provide to users in this case and where do we plan on documenting it? I can see three possibilities:
  1. Return an error / panic
  2. Overwrite the peer state with a new entry
  3. Update the peer state with new data
For (1), my assumption is (that if used properly) there should already have been a peer_disconnected event even if this is a reconnect. So the entry would not exist in that case. Is my understanding accurate?
Currently (2) and (3) are equivalent but may not be in the future. You're implementing this as (3) though the code would be simplified if (2) is the desired behavior (i.e., you could blindly insert into the map instead of matching on the entry).
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Good question. I think in general if you don't follow the API, there are no guarantees period. It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key, but if a counterparty's private key leaks, we won't panic, it just may result in the counterparty losing their funds/channels.
@ariardariardJan 17, 2020•

edited

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Currently (2) and (3) are equivalent but may not be in the future.
Hmmm not sure I would change, it's state relative to the connection it shouldn't leak to the ChannelManager layer, if it's concerning channel+routing, assuming LN messages are ordered newer ones should take precedence.
It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key
Transport is assumed to be authenticated and encrypted so verification than message came from something which owns the relevant private key is done I think. That's said identity of the public key responder, how do you establish this knowledge is beyond specs. Keys leakage means your funds are loss, just send a HTLC emptying your balance to some exit node, it doesn't enter in the scope of option_upfront_shutdown_script.
 e.get().lock().unwrap().latest_features = init_msg.features.clone();
 },
 }
 }

 let mut channel_state_lock = self.channel_state.lock().unwrap();
 let channel_state = &mut *channel_state_lock;
 let pending_msg_events = &mut channel_state.pending_msg_events;
Expand DownExpand Up@@ -3123,6 +3162,14 @@ impl<ChanSigner: ChannelKeys + Writeable> Writeable for ChannelManager<ChanSigne
 }
 }

 let per_peer_state = self.per_peer_state.write().unwrap();
 (per_peer_state.len() as u64).write(writer)?;
 for (peer_pubkey, peer_state_mutex) in per_peer_state.iter() {
 peer_pubkey.write(writer)?;
 let peer_state = peer_state_mutex.lock().unwrap();
 peer_state.latest_features.write(writer)?;
 }

 Ok(())
	}
}
Expand DownExpand Up@@ -3256,6 +3303,16 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
 claimable_htlcs.insert(payment_hash, previous_hops);
 }

 let peer_count: u64 = Readable::read(reader)?;
 let mut per_peer_state = HashMap::with_capacity(cmp::min(peer_count as usize, 128));
 for _ in 0..peer_count {
 let peer_pubkey = Readable::read(reader)?;
 let peer_state = PeerState {
 latest_features: Readable::read(reader)?,
 };
 per_peer_state.insert(peer_pubkey, Mutex::new(peer_state));
 }

 let channel_manager = ChannelManager {
 genesis_hash,
 fee_estimator: args.fee_estimator,
Expand All@@ -3275,6 +3332,8 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
 }),
 our_network_key: args.keys_manager.get_node_secret(),

 per_peer_state: RwLock::new(per_peer_state),

 pending_events: Mutex::new(Vec::new()),
 total_consistency_lock: RwLock::new(()),
 keys_manager: args.keys_manager,
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Plumb Features through into Routes by TheBlueMatt · Pull Request #433 · lightningdevkit/rust-lightning · GitHub
Skip to content
Plumb Features through into Routes#433

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.

 Sign up for GitHub

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

 Merged

TheBlueMatt
merged 4 commits into

from

Jan 22, 2020
 Merged


Plumb Features through into Routes
#433
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


4 commits


Select commit
Hold shift + click to select a range
d2ba7ca

Pass peer's Init message through to ChannelManager


TheBlueMatt Dec 28, 2019
a19d71d

Keep track of the Init Features for every connected/channel'd peer


TheBlueMatt Dec 29, 2019
912f877

Pass node features through to RouteHops


TheBlueMatt Dec 28, 2019
617a680

DRY-up list_channels by having a common lister that takes a filter


TheBlueMatt Jan 17, 2020

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


18 changes: 12 additions & 6 deletions
fuzz/src/chanmon_consistency.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -29,8 +29,8 @@ use lightning::ln::channelmonitor;
use lightning::ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateErr, HTLCUpdate};
use lightning::ln::channelmanager::{ChannelManager, PaymentHash, PaymentPreimage, ChannelManagerReadArgs};
use lightning::ln::router::{Route, RouteHop};
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC};
use lightning::ln::features::{ChannelFeatures, InitFeatures, NodeFeatures};
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC, Init};
use lightning::util::enforcing_trait_impls::EnforcingChannelKeys;
use lightning::util::events;
use lightning::util::logger::Logger;
Expand DownExpand Up@@ -414,7 +414,9 @@ pub fn do_test(data: &[u8]) {
 if let Err(_) = $source.send_payment(Route {
 hops: vec![RouteHop {
 pubkey: $dest.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $dest.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 5000000,
 cltv_expiry_delta: 200,
 }],
Expand All@@ -429,12 +431,16 @@ pub fn do_test(data: &[u8]) {
 if let Err(_) = $source.send_payment(Route {
 hops: vec![RouteHop {
 pubkey: $middle.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $middle.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 50000,
 cltv_expiry_delta: 100,
 },RouteHop {
 pubkey: $dest.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $dest.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 5000000,
 cltv_expiry_delta: 200,
 }],
Expand DownExpand Up@@ -650,15 +656,15 @@ pub fn do_test(data: &[u8]) {
 },
 0x11 => {
 if chan_a_disconnected {
 nodes[0].peer_connected(&nodes[1].get_our_node_id());
 nodes[1].peer_connected(&nodes[0].get_our_node_id());
 nodes[0].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
 nodes[1].peer_connected(&nodes[0].get_our_node_id(), &Init { features: InitFeatures::empty() });
 chan_a_disconnected = false;
 }
 },
 0x12 => {
 if chan_b_disconnected {
 nodes[1].peer_connected(&nodes[2].get_our_node_id());
 nodes[2].peer_connected(&nodes[1].get_our_node_id());
 nodes[1].peer_connected(&nodes[2].get_our_node_id(), &Init { features: InitFeatures::empty() });
 nodes[2].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
 chan_b_disconnected = false;
 }
 },
Expand Down

4 changes: 3 additions & 1 deletion
fuzz/src/router.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -5,8 +5,9 @@ use bitcoin::blockdata::transaction::Transaction;

use lightning::chain::chaininterface::{ChainError,ChainWatchInterface};
use lightning::ln::channelmanager::ChannelDetails;
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs;
use lightning::ln::msgs::{RoutingMessageHandler};
use lightning::ln::msgs::RoutingMessageHandler;
use lightning::ln::router::{Router, RouteHint};
use lightning::util::logger::Logger;
use lightning::util::ser::Readable;
Expand DownExpand Up@@ -198,6 +199,7 @@ pub fn do_test(data: &[u8]) {
 channel_id: [0; 32],
 short_channel_id: Some(slice_to_be64(get_slice!(8))),
 remote_network_id: get_pubkey!(),
 counterparty_features: InitFeatures::empty(),
 channel_value_satoshis: slice_to_be64(get_slice!(8)),
 user_id: 0,
 inbound_capacity_msat: 0,
Expand Down

24 changes: 12 additions & 12 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -215,10 +215,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
 nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
 assert_eq!(reestablish_1.len(), 1);
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
 assert_eq!(reestablish_2.len(), 1);

Expand All@@ -237,10 +237,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
 assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());

 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
 assert_eq!(reestablish_1.len(), 1);
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
 assert_eq!(reestablish_2.len(), 1);

Expand DownExpand Up@@ -938,8 +938,8 @@ fn test_monitor_update_fail_reestablish() {
	commitment_signed_dance!(nodes[1], nodes[2], updates.commitment_signed, false);

	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reestablish = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reestablish = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand All@@ -954,8 +954,8 @@ fn test_monitor_update_fail_reestablish() {
	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	assert!(as_reestablish == get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id()));
	assert!(bs_reestablish == get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id()));
Expand DownExpand Up@@ -1118,8 +1118,8 @@ fn claim_while_disconnected_monitor_update_fail() {
	assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
	check_added_monitors!(nodes[1], 1);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand DownExpand Up@@ -1246,8 +1246,8 @@ fn monitor_failed_no_reestablish_response() {
	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand Down

131 changes: 95 additions & 36 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -275,6 +275,12 @@ pub(super) struct ChannelHolder<ChanSigner: ChannelKeys> {
	pub(super) pending_msg_events: Vec<events::MessageSendEvent>,
}

/// State we hold per-peer. In the future we should put channels in here, but for now we only hold
/// the latest Init features we heard from the peer.
struct PeerState {
	latest_features: InitFeatures,
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

High-level question: Given this is duplicated in PeerManager's Peer structs, has there been any though around designing the API in terms of Peer and Channel abstractions?
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not sure what you mean - you mean something like replacing the their_node_id argument with some struct that holds more information (eg the init message)? We could, I suppose, though note that latest_features here is intended to be available even while disconnected (though maybe it doesn't actually need to be, just nice to have).
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm mostly thinking whether it would be suitable to design the overall API (not just this module) in terms of Peer and Channel abstractions and use them throughout to build higher-level abstractions like ChannelManager. Admittedly, this is outside the scope of this PR.
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Hmm. Maybe. It would certainly be tricky because a Peer "owns" a channel, but also the Peer can disconnect and reconnect and get back the same Channel. We can explore more in the future.
}

#[cfg(not(any(target_pointer_width = "32", target_pointer_width = "64")))]
const ERR: () = "You need at least 32 bit pointers (well, usize, but we'll assume they're the same) for ChannelManager::latest_block_height";

Expand DownExpand Up@@ -328,6 +334,14 @@ pub struct ChannelManager<ChanSigner: ChannelKeys> {
	channel_state: Mutex<ChannelHolder<ChanSigner>>,
	our_network_key: SecretKey,

	/// The bulk of our storage will eventually be here (channels and message queues and the like).
	/// If we are connected to a peer we always at least have an entry here, even if no channels
	/// are currently open with that peer.
	/// Because adding or removing an entry is rare, we usually take an outer read lock and then
	/// operate on the inner value freely. Sadly, this prevents parallel operation when opening a
	/// new channel.

Comment on lines

+340
 to 
+342

@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Not sure that I follow what is meant by "usually" here. Can it be removed?
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

As in, if you add or remove a connection you'll need the outer write lock, but usually we dont so take the read lock.
	per_peer_state: RwLock<HashMap<PublicKey, Mutex<PeerState>>>,

	pending_events: Mutex<Vec<events::Event>>,
	/// Used when we have to take a BIG lock to make sure everything is self-consistent.
	/// Essentially just when we're serializing ourselves out.
Expand DownExpand Up@@ -390,6 +404,10 @@ pub struct ChannelDetails {
	pub short_channel_id: Option<u64>,
	/// 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 user_id passed in to create_channel, or 0 if the channel was inbound.
Expand DownExpand Up@@ -610,6 +628,8 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
 }),
 our_network_key: keys_manager.get_node_secret(),

 per_peer_state: RwLock::new(HashMap::new()),

 pending_events: Mutex::new(Vec::new()),
 total_consistency_lock: RwLock::new(()),

Expand DownExpand Up@@ -660,56 +680,53 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
 Ok(())
	}

	/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
	/// more information.
	pub fn list_channels(&self) -> Vec<ChannelDetails> {
 let channel_state = self.channel_state.lock().unwrap();
 let mut res = Vec::with_capacity(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter() {
 let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
 res.push(ChannelDetails {
 channel_id: (*channel_id).clone(),
 short_channel_id: channel.get_short_channel_id(),
 remote_network_id: channel.get_their_node_id(),
 channel_value_satoshis: channel.get_value_satoshis(),
 inbound_capacity_msat,
 outbound_capacity_msat,
 user_id: channel.get_user_id(),
 is_live: channel.is_live(),
 });
 }
 res
	}

	/// Gets the list of usable channels, in random order. Useful as an argument to
	/// Router::get_route to ensure non-announced channels are used.
	///
	/// These are guaranteed to have their is_live value set to true, see the documentation for
	/// ChannelDetails::is_live for more info on exactly what the criteria are.
	pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
 let channel_state = self.channel_state.lock().unwrap();
 let mut res = Vec::with_capacity(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter() {
 // Note we use is_live here instead of usable which leads to somewhat confused
 // internal/external nomenclature, but that's ok cause that's probably what the user
 // really wanted anyway.
 if channel.is_live() {
	fn list_channels_with_filter<F: FnMut(&(&[u8; 32], &Channel<ChanSigner>)) -> bool>(&self, f: F) -> Vec<ChannelDetails> {
 let mut res = Vec::new();
 {
 let channel_state = self.channel_state.lock().unwrap();
 res.reserve(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter().filter(f) {
 let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
 res.push(ChannelDetails {
 channel_id: (*channel_id).clone(),
 short_channel_id: channel.get_short_channel_id(),
 remote_network_id: channel.get_their_node_id(),
 counterparty_features: InitFeatures::empty(),
 channel_value_satoshis: channel.get_value_satoshis(),
 inbound_capacity_msat,
 outbound_capacity_msat,
 user_id: channel.get_user_id(),
 is_live: true,
 is_live: channel.is_live(),
 });
 }
 }
 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();
 }
 }
 res
	}

	/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
	/// more information.
	pub fn list_channels(&self) -> Vec<ChannelDetails> {
 self.list_channels_with_filter(|_| true)
	}

	/// Gets the list of usable channels, in random order. Useful as an argument to
	/// Router::get_route to ensure non-announced channels are used.
	///
	/// These are guaranteed to have their is_live value set to true, see the documentation for
	/// ChannelDetails::is_live for more info on exactly what the criteria are.
	pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
 // Note we use is_live here instead of usable which leads to somewhat confused
 // internal/external nomenclature, but that's ok cause that's probably what the user
 // really wanted anyway.
 self.list_channels_with_filter(|&(_, ref channel)| channel.is_live())
	}

	/// Begins the process of closing a channel. After this call (plus some timeout), no new HTLCs
	/// will be accepted on the given channel, and after additional timeout/the closing of all
	/// pending HTLCs, the channel will be closed on chain.
Expand DownExpand Up@@ -2780,6 +2797,7 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 let _ = self.total_consistency_lock.read().unwrap();
 let mut failed_channels = Vec::new();
 let mut failed_payments = Vec::new();
 let mut no_channels_remain = true;
 {
 let mut channel_state_lock = self.channel_state.lock().unwrap();
 let channel_state = &mut *channel_state_lock;
Expand DownExpand Up@@ -2818,6 +2836,8 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 short_to_id.remove(&short_id);
 }
 return false;
 } else {
 no_channels_remain = false;
 }
 }
 true
Expand All@@ -2843,6 +2863,10 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 }
 });
 }
 if no_channels_remain {
 self.per_peer_state.write().unwrap().remove(their_node_id);
 }

 for failure in failed_channels.drain(..) {
 self.finish_force_close_channel(failure);
 }
Expand All@@ -2853,10 +2877,25 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 }
	}

	fn peer_connected(&self, their_node_id: &PublicKey) {
	fn peer_connected(&self, their_node_id: &PublicKey, init_msg: &msgs::Init) {
 log_debug!(self, "Generating channel_reestablish events for {}", log_pubkey!(their_node_id));

 let _ = self.total_consistency_lock.read().unwrap();

 {
 let mut peer_state_lock = self.per_peer_state.write().unwrap();
 match peer_state_lock.entry(their_node_id.clone()) {
 hash_map::Entry::Vacant(e) => {
 e.insert(Mutex::new(PeerState {
 latest_features: init_msg.features.clone(),
 }));
 },
 hash_map::Entry::Occupied(e) => {
@ariardariardJan 7, 2020

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Is there any risk of peer collision ? An attacker can usurp pubkey available on the net but without privkey it won't get through the noise phase..and if a peer leaks its privkey we can't guarantee consistency of our structs (and we may have bigger troubles)
@TheBlueMattTheBlueMattJan 7, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Right. If you can impersonate a peer, that peer is screwed. Not a lot we can do then.
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

When would this occur? Won't PeerManager disconnect when it sees a duplicate id?
https://github.com/rust-bitcoin/rust-lightning/blob/a252f81477c25bf1248fd9ea5269a6620187c487/lightning/src/ln/peer_handler.rs#L540-L554
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

If you're using our PeerManager, yes, this should not be possible, though in theory these functions are public and the user may run their own PeerManager-like thing that may not properly check.
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What sort of semantics do we want to provide to users in this case and where do we plan on documenting it? I can see three possibilities:
  1. Return an error / panic
  2. Overwrite the peer state with a new entry
  3. Update the peer state with new data
For (1), my assumption is (that if used properly) there should already have been a peer_disconnected event even if this is a reconnect. So the entry would not exist in that case. Is my understanding accurate?
Currently (2) and (3) are equivalent but may not be in the future. You're implementing this as (3) though the code would be simplified if (2) is the desired behavior (i.e., you could blindly insert into the map instead of matching on the entry).
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Good question. I think in general if you don't follow the API, there are no guarantees period. It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key, but if a counterparty's private key leaks, we won't panic, it just may result in the counterparty losing their funds/channels.
@ariardariardJan 17, 2020•

edited

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Currently (2) and (3) are equivalent but may not be in the future.
Hmmm not sure I would change, it's state relative to the connection it shouldn't leak to the ChannelManager layer, if it's concerning channel+routing, assuming LN messages are ordered newer ones should take precedence.
It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key
Transport is assumed to be authenticated and encrypted so verification than message came from something which owns the relevant private key is done I think. That's said identity of the public key responder, how do you establish this knowledge is beyond specs. Keys leakage means your funds are loss, just send a HTLC emptying your balance to some exit node, it doesn't enter in the scope of option_upfront_shutdown_script.
 e.get().lock().unwrap().latest_features = init_msg.features.clone();
 },
 }
 }

 let mut channel_state_lock = self.channel_state.lock().unwrap();
 let channel_state = &mut *channel_state_lock;
 let pending_msg_events = &mut channel_state.pending_msg_events;
Expand DownExpand Up@@ -3123,6 +3162,14 @@ impl<ChanSigner: ChannelKeys + Writeable> Writeable for ChannelManager<ChanSigne
 }
 }

 let per_peer_state = self.per_peer_state.write().unwrap();
 (per_peer_state.len() as u64).write(writer)?;
 for (peer_pubkey, peer_state_mutex) in per_peer_state.iter() {
 peer_pubkey.write(writer)?;
 let peer_state = peer_state_mutex.lock().unwrap();
 peer_state.latest_features.write(writer)?;
 }

 Ok(())
	}
}
Expand DownExpand Up@@ -3256,6 +3303,16 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
 claimable_htlcs.insert(payment_hash, previous_hops);
 }

 let peer_count: u64 = Readable::read(reader)?;
 let mut per_peer_state = HashMap::with_capacity(cmp::min(peer_count as usize, 128));
 for _ in 0..peer_count {
 let peer_pubkey = Readable::read(reader)?;
 let peer_state = PeerState {
 latest_features: Readable::read(reader)?,
 };
 per_peer_state.insert(peer_pubkey, Mutex::new(peer_state));
 }

 let channel_manager = ChannelManager {
 genesis_hash,
 fee_estimator: args.fee_estimator,
Expand All@@ -3275,6 +3332,8 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
 }),
 our_network_key: args.keys_manager.get_node_secret(),

 per_peer_state: RwLock::new(per_peer_state),

 pending_events: Mutex::new(Vec::new()),
 total_consistency_lock: RwLock::new(()),
 keys_manager: args.keys_manager,
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Plumb Features through into Routes by TheBlueMatt · Pull Request #433 · lightningdevkit/rust-lightning · GitHub
Skip to content
Plumb Features through into Routes#433

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.

 Sign up for GitHub

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

 Merged

TheBlueMatt
merged 4 commits into

from

Jan 22, 2020
 Merged


Plumb Features through into Routes
#433
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


4 commits


Select commit
Hold shift + click to select a range
d2ba7ca

Pass peer's Init message through to ChannelManager


TheBlueMatt Dec 28, 2019
a19d71d

Keep track of the Init Features for every connected/channel'd peer


TheBlueMatt Dec 29, 2019
912f877

Pass node features through to RouteHops


TheBlueMatt Dec 28, 2019
617a680

DRY-up list_channels by having a common lister that takes a filter


TheBlueMatt Jan 17, 2020

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


18 changes: 12 additions & 6 deletions
fuzz/src/chanmon_consistency.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -29,8 +29,8 @@ use lightning::ln::channelmonitor;
use lightning::ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateErr, HTLCUpdate};
use lightning::ln::channelmanager::{ChannelManager, PaymentHash, PaymentPreimage, ChannelManagerReadArgs};
use lightning::ln::router::{Route, RouteHop};
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC};
use lightning::ln::features::{ChannelFeatures, InitFeatures, NodeFeatures};
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC, Init};
use lightning::util::enforcing_trait_impls::EnforcingChannelKeys;
use lightning::util::events;
use lightning::util::logger::Logger;
Expand DownExpand Up@@ -414,7 +414,9 @@ pub fn do_test(data: &[u8]) {
 if let Err(_) = $source.send_payment(Route {
 hops: vec![RouteHop {
 pubkey: $dest.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $dest.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 5000000,
 cltv_expiry_delta: 200,
 }],
Expand All@@ -429,12 +431,16 @@ pub fn do_test(data: &[u8]) {
 if let Err(_) = $source.send_payment(Route {
 hops: vec![RouteHop {
 pubkey: $middle.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $middle.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 50000,
 cltv_expiry_delta: 100,
 },RouteHop {
 pubkey: $dest.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $dest.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 5000000,
 cltv_expiry_delta: 200,
 }],
Expand DownExpand Up@@ -650,15 +656,15 @@ pub fn do_test(data: &[u8]) {
 },
 0x11 => {
 if chan_a_disconnected {
 nodes[0].peer_connected(&nodes[1].get_our_node_id());
 nodes[1].peer_connected(&nodes[0].get_our_node_id());
 nodes[0].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
 nodes[1].peer_connected(&nodes[0].get_our_node_id(), &Init { features: InitFeatures::empty() });
 chan_a_disconnected = false;
 }
 },
 0x12 => {
 if chan_b_disconnected {
 nodes[1].peer_connected(&nodes[2].get_our_node_id());
 nodes[2].peer_connected(&nodes[1].get_our_node_id());
 nodes[1].peer_connected(&nodes[2].get_our_node_id(), &Init { features: InitFeatures::empty() });
 nodes[2].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
 chan_b_disconnected = false;
 }
 },
Expand Down

4 changes: 3 additions & 1 deletion
fuzz/src/router.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -5,8 +5,9 @@ use bitcoin::blockdata::transaction::Transaction;

use lightning::chain::chaininterface::{ChainError,ChainWatchInterface};
use lightning::ln::channelmanager::ChannelDetails;
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs;
use lightning::ln::msgs::{RoutingMessageHandler};
use lightning::ln::msgs::RoutingMessageHandler;
use lightning::ln::router::{Router, RouteHint};
use lightning::util::logger::Logger;
use lightning::util::ser::Readable;
Expand DownExpand Up@@ -198,6 +199,7 @@ pub fn do_test(data: &[u8]) {
 channel_id: [0; 32],
 short_channel_id: Some(slice_to_be64(get_slice!(8))),
 remote_network_id: get_pubkey!(),
 counterparty_features: InitFeatures::empty(),
 channel_value_satoshis: slice_to_be64(get_slice!(8)),
 user_id: 0,
 inbound_capacity_msat: 0,
Expand Down

24 changes: 12 additions & 12 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -215,10 +215,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
 nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
 assert_eq!(reestablish_1.len(), 1);
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
 assert_eq!(reestablish_2.len(), 1);

Expand All@@ -237,10 +237,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
 assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());

 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
 assert_eq!(reestablish_1.len(), 1);
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
 assert_eq!(reestablish_2.len(), 1);

Expand DownExpand Up@@ -938,8 +938,8 @@ fn test_monitor_update_fail_reestablish() {
	commitment_signed_dance!(nodes[1], nodes[2], updates.commitment_signed, false);

	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reestablish = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reestablish = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand All@@ -954,8 +954,8 @@ fn test_monitor_update_fail_reestablish() {
	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	assert!(as_reestablish == get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id()));
	assert!(bs_reestablish == get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id()));
Expand DownExpand Up@@ -1118,8 +1118,8 @@ fn claim_while_disconnected_monitor_update_fail() {
	assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
	check_added_monitors!(nodes[1], 1);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand DownExpand Up@@ -1246,8 +1246,8 @@ fn monitor_failed_no_reestablish_response() {
	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand Down

131 changes: 95 additions & 36 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -275,6 +275,12 @@ pub(super) struct ChannelHolder<ChanSigner: ChannelKeys> {
	pub(super) pending_msg_events: Vec<events::MessageSendEvent>,
}

/// State we hold per-peer. In the future we should put channels in here, but for now we only hold
/// the latest Init features we heard from the peer.
struct PeerState {
	latest_features: InitFeatures,
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

High-level question: Given this is duplicated in PeerManager's Peer structs, has there been any though around designing the API in terms of Peer and Channel abstractions?
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not sure what you mean - you mean something like replacing the their_node_id argument with some struct that holds more information (eg the init message)? We could, I suppose, though note that latest_features here is intended to be available even while disconnected (though maybe it doesn't actually need to be, just nice to have).
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm mostly thinking whether it would be suitable to design the overall API (not just this module) in terms of Peer and Channel abstractions and use them throughout to build higher-level abstractions like ChannelManager. Admittedly, this is outside the scope of this PR.
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Hmm. Maybe. It would certainly be tricky because a Peer "owns" a channel, but also the Peer can disconnect and reconnect and get back the same Channel. We can explore more in the future.
}

#[cfg(not(any(target_pointer_width = "32", target_pointer_width = "64")))]
const ERR: () = "You need at least 32 bit pointers (well, usize, but we'll assume they're the same) for ChannelManager::latest_block_height";

Expand DownExpand Up@@ -328,6 +334,14 @@ pub struct ChannelManager<ChanSigner: ChannelKeys> {
	channel_state: Mutex<ChannelHolder<ChanSigner>>,
	our_network_key: SecretKey,

	/// The bulk of our storage will eventually be here (channels and message queues and the like).
	/// If we are connected to a peer we always at least have an entry here, even if no channels
	/// are currently open with that peer.
	/// Because adding or removing an entry is rare, we usually take an outer read lock and then
	/// operate on the inner value freely. Sadly, this prevents parallel operation when opening a
	/// new channel.

Comment on lines

+340
 to 
+342

@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Not sure that I follow what is meant by "usually" here. Can it be removed?
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

As in, if you add or remove a connection you'll need the outer write lock, but usually we dont so take the read lock.
	per_peer_state: RwLock<HashMap<PublicKey, Mutex<PeerState>>>,

	pending_events: Mutex<Vec<events::Event>>,
	/// Used when we have to take a BIG lock to make sure everything is self-consistent.
	/// Essentially just when we're serializing ourselves out.
Expand DownExpand Up@@ -390,6 +404,10 @@ pub struct ChannelDetails {
	pub short_channel_id: Option<u64>,
	/// 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 user_id passed in to create_channel, or 0 if the channel was inbound.
Expand DownExpand Up@@ -610,6 +628,8 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
 }),
 our_network_key: keys_manager.get_node_secret(),

 per_peer_state: RwLock::new(HashMap::new()),

 pending_events: Mutex::new(Vec::new()),
 total_consistency_lock: RwLock::new(()),

Expand DownExpand Up@@ -660,56 +680,53 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
 Ok(())
	}

	/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
	/// more information.
	pub fn list_channels(&self) -> Vec<ChannelDetails> {
 let channel_state = self.channel_state.lock().unwrap();
 let mut res = Vec::with_capacity(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter() {
 let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
 res.push(ChannelDetails {
 channel_id: (*channel_id).clone(),
 short_channel_id: channel.get_short_channel_id(),
 remote_network_id: channel.get_their_node_id(),
 channel_value_satoshis: channel.get_value_satoshis(),
 inbound_capacity_msat,
 outbound_capacity_msat,
 user_id: channel.get_user_id(),
 is_live: channel.is_live(),
 });
 }
 res
	}

	/// Gets the list of usable channels, in random order. Useful as an argument to
	/// Router::get_route to ensure non-announced channels are used.
	///
	/// These are guaranteed to have their is_live value set to true, see the documentation for
	/// ChannelDetails::is_live for more info on exactly what the criteria are.
	pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
 let channel_state = self.channel_state.lock().unwrap();
 let mut res = Vec::with_capacity(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter() {
 // Note we use is_live here instead of usable which leads to somewhat confused
 // internal/external nomenclature, but that's ok cause that's probably what the user
 // really wanted anyway.
 if channel.is_live() {
	fn list_channels_with_filter<F: FnMut(&(&[u8; 32], &Channel<ChanSigner>)) -> bool>(&self, f: F) -> Vec<ChannelDetails> {
 let mut res = Vec::new();
 {
 let channel_state = self.channel_state.lock().unwrap();
 res.reserve(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter().filter(f) {
 let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
 res.push(ChannelDetails {
 channel_id: (*channel_id).clone(),
 short_channel_id: channel.get_short_channel_id(),
 remote_network_id: channel.get_their_node_id(),
 counterparty_features: InitFeatures::empty(),
 channel_value_satoshis: channel.get_value_satoshis(),
 inbound_capacity_msat,
 outbound_capacity_msat,
 user_id: channel.get_user_id(),
 is_live: true,
 is_live: channel.is_live(),
 });
 }
 }
 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();
 }
 }
 res
	}

	/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
	/// more information.
	pub fn list_channels(&self) -> Vec<ChannelDetails> {
 self.list_channels_with_filter(|_| true)
	}

	/// Gets the list of usable channels, in random order. Useful as an argument to
	/// Router::get_route to ensure non-announced channels are used.
	///
	/// These are guaranteed to have their is_live value set to true, see the documentation for
	/// ChannelDetails::is_live for more info on exactly what the criteria are.
	pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
 // Note we use is_live here instead of usable which leads to somewhat confused
 // internal/external nomenclature, but that's ok cause that's probably what the user
 // really wanted anyway.
 self.list_channels_with_filter(|&(_, ref channel)| channel.is_live())
	}

	/// Begins the process of closing a channel. After this call (plus some timeout), no new HTLCs
	/// will be accepted on the given channel, and after additional timeout/the closing of all
	/// pending HTLCs, the channel will be closed on chain.
Expand DownExpand Up@@ -2780,6 +2797,7 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 let _ = self.total_consistency_lock.read().unwrap();
 let mut failed_channels = Vec::new();
 let mut failed_payments = Vec::new();
 let mut no_channels_remain = true;
 {
 let mut channel_state_lock = self.channel_state.lock().unwrap();
 let channel_state = &mut *channel_state_lock;
Expand DownExpand Up@@ -2818,6 +2836,8 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 short_to_id.remove(&short_id);
 }
 return false;
 } else {
 no_channels_remain = false;
 }
 }
 true
Expand All@@ -2843,6 +2863,10 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 }
 });
 }
 if no_channels_remain {
 self.per_peer_state.write().unwrap().remove(their_node_id);
 }

 for failure in failed_channels.drain(..) {
 self.finish_force_close_channel(failure);
 }
Expand All@@ -2853,10 +2877,25 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 }
	}

	fn peer_connected(&self, their_node_id: &PublicKey) {
	fn peer_connected(&self, their_node_id: &PublicKey, init_msg: &msgs::Init) {
 log_debug!(self, "Generating channel_reestablish events for {}", log_pubkey!(their_node_id));

 let _ = self.total_consistency_lock.read().unwrap();

 {
 let mut peer_state_lock = self.per_peer_state.write().unwrap();
 match peer_state_lock.entry(their_node_id.clone()) {
 hash_map::Entry::Vacant(e) => {
 e.insert(Mutex::new(PeerState {
 latest_features: init_msg.features.clone(),
 }));
 },
 hash_map::Entry::Occupied(e) => {
@ariardariardJan 7, 2020

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Is there any risk of peer collision ? An attacker can usurp pubkey available on the net but without privkey it won't get through the noise phase..and if a peer leaks its privkey we can't guarantee consistency of our structs (and we may have bigger troubles)
@TheBlueMattTheBlueMattJan 7, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Right. If you can impersonate a peer, that peer is screwed. Not a lot we can do then.
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

When would this occur? Won't PeerManager disconnect when it sees a duplicate id?
https://github.com/rust-bitcoin/rust-lightning/blob/a252f81477c25bf1248fd9ea5269a6620187c487/lightning/src/ln/peer_handler.rs#L540-L554
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

If you're using our PeerManager, yes, this should not be possible, though in theory these functions are public and the user may run their own PeerManager-like thing that may not properly check.
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What sort of semantics do we want to provide to users in this case and where do we plan on documenting it? I can see three possibilities:
  1. Return an error / panic
  2. Overwrite the peer state with a new entry
  3. Update the peer state with new data
For (1), my assumption is (that if used properly) there should already have been a peer_disconnected event even if this is a reconnect. So the entry would not exist in that case. Is my understanding accurate?
Currently (2) and (3) are equivalent but may not be in the future. You're implementing this as (3) though the code would be simplified if (2) is the desired behavior (i.e., you could blindly insert into the map instead of matching on the entry).
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Good question. I think in general if you don't follow the API, there are no guarantees period. It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key, but if a counterparty's private key leaks, we won't panic, it just may result in the counterparty losing their funds/channels.
@ariardariardJan 17, 2020•

edited

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Currently (2) and (3) are equivalent but may not be in the future.
Hmmm not sure I would change, it's state relative to the connection it shouldn't leak to the ChannelManager layer, if it's concerning channel+routing, assuming LN messages are ordered newer ones should take precedence.
It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key
Transport is assumed to be authenticated and encrypted so verification than message came from something which owns the relevant private key is done I think. That's said identity of the public key responder, how do you establish this knowledge is beyond specs. Keys leakage means your funds are loss, just send a HTLC emptying your balance to some exit node, it doesn't enter in the scope of option_upfront_shutdown_script.
 e.get().lock().unwrap().latest_features = init_msg.features.clone();
 },
 }
 }

 let mut channel_state_lock = self.channel_state.lock().unwrap();
 let channel_state = &mut *channel_state_lock;
 let pending_msg_events = &mut channel_state.pending_msg_events;
Expand DownExpand Up@@ -3123,6 +3162,14 @@ impl<ChanSigner: ChannelKeys + Writeable> Writeable for ChannelManager<ChanSigne
 }
 }

 let per_peer_state = self.per_peer_state.write().unwrap();
 (per_peer_state.len() as u64).write(writer)?;
 for (peer_pubkey, peer_state_mutex) in per_peer_state.iter() {
 peer_pubkey.write(writer)?;
 let peer_state = peer_state_mutex.lock().unwrap();
 peer_state.latest_features.write(writer)?;
 }

 Ok(())
	}
}
Expand DownExpand Up@@ -3256,6 +3303,16 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
 claimable_htlcs.insert(payment_hash, previous_hops);
 }

 let peer_count: u64 = Readable::read(reader)?;
 let mut per_peer_state = HashMap::with_capacity(cmp::min(peer_count as usize, 128));
 for _ in 0..peer_count {
 let peer_pubkey = Readable::read(reader)?;
 let peer_state = PeerState {
 latest_features: Readable::read(reader)?,
 };
 per_peer_state.insert(peer_pubkey, Mutex::new(peer_state));
 }

 let channel_manager = ChannelManager {
 genesis_hash,
 fee_estimator: args.fee_estimator,
Expand All@@ -3275,6 +3332,8 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
 }),
 our_network_key: args.keys_manager.get_node_secret(),

 per_peer_state: RwLock::new(per_peer_state),

 pending_events: Mutex::new(Vec::new()),
 total_consistency_lock: RwLock::new(()),
 keys_manager: args.keys_manager,
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Plumb Features through into Routes by TheBlueMatt · Pull Request #433 · lightningdevkit/rust-lightning · GitHub
Skip to content
Plumb Features through into Routes#433

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.

 Sign up for GitHub

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

 Merged

TheBlueMatt
merged 4 commits into

from

Jan 22, 2020
 Merged


Plumb Features through into Routes
#433
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


4 commits


Select commit
Hold shift + click to select a range
d2ba7ca

Pass peer's Init message through to ChannelManager


TheBlueMatt Dec 28, 2019
a19d71d

Keep track of the Init Features for every connected/channel'd peer


TheBlueMatt Dec 29, 2019
912f877

Pass node features through to RouteHops


TheBlueMatt Dec 28, 2019
617a680

DRY-up list_channels by having a common lister that takes a filter


TheBlueMatt Jan 17, 2020

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


18 changes: 12 additions & 6 deletions
fuzz/src/chanmon_consistency.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -29,8 +29,8 @@ use lightning::ln::channelmonitor;
use lightning::ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateErr, HTLCUpdate};
use lightning::ln::channelmanager::{ChannelManager, PaymentHash, PaymentPreimage, ChannelManagerReadArgs};
use lightning::ln::router::{Route, RouteHop};
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC};
use lightning::ln::features::{ChannelFeatures, InitFeatures, NodeFeatures};
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC, Init};
use lightning::util::enforcing_trait_impls::EnforcingChannelKeys;
use lightning::util::events;
use lightning::util::logger::Logger;
Expand DownExpand Up@@ -414,7 +414,9 @@ pub fn do_test(data: &[u8]) {
 if let Err(_) = $source.send_payment(Route {
 hops: vec![RouteHop {
 pubkey: $dest.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $dest.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 5000000,
 cltv_expiry_delta: 200,
 }],
Expand All@@ -429,12 +431,16 @@ pub fn do_test(data: &[u8]) {
 if let Err(_) = $source.send_payment(Route {
 hops: vec![RouteHop {
 pubkey: $middle.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $middle.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 50000,
 cltv_expiry_delta: 100,
 },RouteHop {
 pubkey: $dest.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $dest.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 5000000,
 cltv_expiry_delta: 200,
 }],
Expand DownExpand Up@@ -650,15 +656,15 @@ pub fn do_test(data: &[u8]) {
 },
 0x11 => {
 if chan_a_disconnected {
 nodes[0].peer_connected(&nodes[1].get_our_node_id());
 nodes[1].peer_connected(&nodes[0].get_our_node_id());
 nodes[0].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
 nodes[1].peer_connected(&nodes[0].get_our_node_id(), &Init { features: InitFeatures::empty() });
 chan_a_disconnected = false;
 }
 },
 0x12 => {
 if chan_b_disconnected {
 nodes[1].peer_connected(&nodes[2].get_our_node_id());
 nodes[2].peer_connected(&nodes[1].get_our_node_id());
 nodes[1].peer_connected(&nodes[2].get_our_node_id(), &Init { features: InitFeatures::empty() });
 nodes[2].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
 chan_b_disconnected = false;
 }
 },
Expand Down

4 changes: 3 additions & 1 deletion
fuzz/src/router.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -5,8 +5,9 @@ use bitcoin::blockdata::transaction::Transaction;

use lightning::chain::chaininterface::{ChainError,ChainWatchInterface};
use lightning::ln::channelmanager::ChannelDetails;
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs;
use lightning::ln::msgs::{RoutingMessageHandler};
use lightning::ln::msgs::RoutingMessageHandler;
use lightning::ln::router::{Router, RouteHint};
use lightning::util::logger::Logger;
use lightning::util::ser::Readable;
Expand DownExpand Up@@ -198,6 +199,7 @@ pub fn do_test(data: &[u8]) {
 channel_id: [0; 32],
 short_channel_id: Some(slice_to_be64(get_slice!(8))),
 remote_network_id: get_pubkey!(),
 counterparty_features: InitFeatures::empty(),
 channel_value_satoshis: slice_to_be64(get_slice!(8)),
 user_id: 0,
 inbound_capacity_msat: 0,
Expand Down

24 changes: 12 additions & 12 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -215,10 +215,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
 nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
 assert_eq!(reestablish_1.len(), 1);
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
 assert_eq!(reestablish_2.len(), 1);

Expand All@@ -237,10 +237,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
 assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());

 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
 assert_eq!(reestablish_1.len(), 1);
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
 assert_eq!(reestablish_2.len(), 1);

Expand DownExpand Up@@ -938,8 +938,8 @@ fn test_monitor_update_fail_reestablish() {
	commitment_signed_dance!(nodes[1], nodes[2], updates.commitment_signed, false);

	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reestablish = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reestablish = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand All@@ -954,8 +954,8 @@ fn test_monitor_update_fail_reestablish() {
	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	assert!(as_reestablish == get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id()));
	assert!(bs_reestablish == get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id()));
Expand DownExpand Up@@ -1118,8 +1118,8 @@ fn claim_while_disconnected_monitor_update_fail() {
	assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
	check_added_monitors!(nodes[1], 1);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand DownExpand Up@@ -1246,8 +1246,8 @@ fn monitor_failed_no_reestablish_response() {
	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand Down

131 changes: 95 additions & 36 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -275,6 +275,12 @@ pub(super) struct ChannelHolder<ChanSigner: ChannelKeys> {
	pub(super) pending_msg_events: Vec<events::MessageSendEvent>,
}

/// State we hold per-peer. In the future we should put channels in here, but for now we only hold
/// the latest Init features we heard from the peer.
struct PeerState {
	latest_features: InitFeatures,
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

High-level question: Given this is duplicated in PeerManager's Peer structs, has there been any though around designing the API in terms of Peer and Channel abstractions?
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not sure what you mean - you mean something like replacing the their_node_id argument with some struct that holds more information (eg the init message)? We could, I suppose, though note that latest_features here is intended to be available even while disconnected (though maybe it doesn't actually need to be, just nice to have).
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm mostly thinking whether it would be suitable to design the overall API (not just this module) in terms of Peer and Channel abstractions and use them throughout to build higher-level abstractions like ChannelManager. Admittedly, this is outside the scope of this PR.
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Hmm. Maybe. It would certainly be tricky because a Peer "owns" a channel, but also the Peer can disconnect and reconnect and get back the same Channel. We can explore more in the future.
}

#[cfg(not(any(target_pointer_width = "32", target_pointer_width = "64")))]
const ERR: () = "You need at least 32 bit pointers (well, usize, but we'll assume they're the same) for ChannelManager::latest_block_height";

Expand DownExpand Up@@ -328,6 +334,14 @@ pub struct ChannelManager<ChanSigner: ChannelKeys> {
	channel_state: Mutex<ChannelHolder<ChanSigner>>,
	our_network_key: SecretKey,

	/// The bulk of our storage will eventually be here (channels and message queues and the like).
	/// If we are connected to a peer we always at least have an entry here, even if no channels
	/// are currently open with that peer.
	/// Because adding or removing an entry is rare, we usually take an outer read lock and then
	/// operate on the inner value freely. Sadly, this prevents parallel operation when opening a
	/// new channel.

Comment on lines

+340
 to 
+342

@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Not sure that I follow what is meant by "usually" here. Can it be removed?
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

As in, if you add or remove a connection you'll need the outer write lock, but usually we dont so take the read lock.
	per_peer_state: RwLock<HashMap<PublicKey, Mutex<PeerState>>>,

	pending_events: Mutex<Vec<events::Event>>,
	/// Used when we have to take a BIG lock to make sure everything is self-consistent.
	/// Essentially just when we're serializing ourselves out.
Expand DownExpand Up@@ -390,6 +404,10 @@ pub struct ChannelDetails {
	pub short_channel_id: Option<u64>,
	/// 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 user_id passed in to create_channel, or 0 if the channel was inbound.
Expand DownExpand Up@@ -610,6 +628,8 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
 }),
 our_network_key: keys_manager.get_node_secret(),

 per_peer_state: RwLock::new(HashMap::new()),

 pending_events: Mutex::new(Vec::new()),
 total_consistency_lock: RwLock::new(()),

Expand DownExpand Up@@ -660,56 +680,53 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
 Ok(())
	}

	/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
	/// more information.
	pub fn list_channels(&self) -> Vec<ChannelDetails> {
 let channel_state = self.channel_state.lock().unwrap();
 let mut res = Vec::with_capacity(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter() {
 let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
 res.push(ChannelDetails {
 channel_id: (*channel_id).clone(),
 short_channel_id: channel.get_short_channel_id(),
 remote_network_id: channel.get_their_node_id(),
 channel_value_satoshis: channel.get_value_satoshis(),
 inbound_capacity_msat,
 outbound_capacity_msat,
 user_id: channel.get_user_id(),
 is_live: channel.is_live(),
 });
 }
 res
	}

	/// Gets the list of usable channels, in random order. Useful as an argument to
	/// Router::get_route to ensure non-announced channels are used.
	///
	/// These are guaranteed to have their is_live value set to true, see the documentation for
	/// ChannelDetails::is_live for more info on exactly what the criteria are.
	pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
 let channel_state = self.channel_state.lock().unwrap();
 let mut res = Vec::with_capacity(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter() {
 // Note we use is_live here instead of usable which leads to somewhat confused
 // internal/external nomenclature, but that's ok cause that's probably what the user
 // really wanted anyway.
 if channel.is_live() {
	fn list_channels_with_filter<F: FnMut(&(&[u8; 32], &Channel<ChanSigner>)) -> bool>(&self, f: F) -> Vec<ChannelDetails> {
 let mut res = Vec::new();
 {
 let channel_state = self.channel_state.lock().unwrap();
 res.reserve(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter().filter(f) {
 let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
 res.push(ChannelDetails {
 channel_id: (*channel_id).clone(),
 short_channel_id: channel.get_short_channel_id(),
 remote_network_id: channel.get_their_node_id(),
 counterparty_features: InitFeatures::empty(),
 channel_value_satoshis: channel.get_value_satoshis(),
 inbound_capacity_msat,
 outbound_capacity_msat,
 user_id: channel.get_user_id(),
 is_live: true,
 is_live: channel.is_live(),
 });
 }
 }
 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();
 }
 }
 res
	}

	/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
	/// more information.
	pub fn list_channels(&self) -> Vec<ChannelDetails> {
 self.list_channels_with_filter(|_| true)
	}

	/// Gets the list of usable channels, in random order. Useful as an argument to
	/// Router::get_route to ensure non-announced channels are used.
	///
	/// These are guaranteed to have their is_live value set to true, see the documentation for
	/// ChannelDetails::is_live for more info on exactly what the criteria are.
	pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
 // Note we use is_live here instead of usable which leads to somewhat confused
 // internal/external nomenclature, but that's ok cause that's probably what the user
 // really wanted anyway.
 self.list_channels_with_filter(|&(_, ref channel)| channel.is_live())
	}

	/// Begins the process of closing a channel. After this call (plus some timeout), no new HTLCs
	/// will be accepted on the given channel, and after additional timeout/the closing of all
	/// pending HTLCs, the channel will be closed on chain.
Expand DownExpand Up@@ -2780,6 +2797,7 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 let _ = self.total_consistency_lock.read().unwrap();
 let mut failed_channels = Vec::new();
 let mut failed_payments = Vec::new();
 let mut no_channels_remain = true;
 {
 let mut channel_state_lock = self.channel_state.lock().unwrap();
 let channel_state = &mut *channel_state_lock;
Expand DownExpand Up@@ -2818,6 +2836,8 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 short_to_id.remove(&short_id);
 }
 return false;
 } else {
 no_channels_remain = false;
 }
 }
 true
Expand All@@ -2843,6 +2863,10 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 }
 });
 }
 if no_channels_remain {
 self.per_peer_state.write().unwrap().remove(their_node_id);
 }

 for failure in failed_channels.drain(..) {
 self.finish_force_close_channel(failure);
 }
Expand All@@ -2853,10 +2877,25 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 }
	}

	fn peer_connected(&self, their_node_id: &PublicKey) {
	fn peer_connected(&self, their_node_id: &PublicKey, init_msg: &msgs::Init) {
 log_debug!(self, "Generating channel_reestablish events for {}", log_pubkey!(their_node_id));

 let _ = self.total_consistency_lock.read().unwrap();

 {
 let mut peer_state_lock = self.per_peer_state.write().unwrap();
 match peer_state_lock.entry(their_node_id.clone()) {
 hash_map::Entry::Vacant(e) => {
 e.insert(Mutex::new(PeerState {
 latest_features: init_msg.features.clone(),
 }));
 },
 hash_map::Entry::Occupied(e) => {
@ariardariardJan 7, 2020

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Is there any risk of peer collision ? An attacker can usurp pubkey available on the net but without privkey it won't get through the noise phase..and if a peer leaks its privkey we can't guarantee consistency of our structs (and we may have bigger troubles)
@TheBlueMattTheBlueMattJan 7, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Right. If you can impersonate a peer, that peer is screwed. Not a lot we can do then.
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

When would this occur? Won't PeerManager disconnect when it sees a duplicate id?
https://github.com/rust-bitcoin/rust-lightning/blob/a252f81477c25bf1248fd9ea5269a6620187c487/lightning/src/ln/peer_handler.rs#L540-L554
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

If you're using our PeerManager, yes, this should not be possible, though in theory these functions are public and the user may run their own PeerManager-like thing that may not properly check.
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What sort of semantics do we want to provide to users in this case and where do we plan on documenting it? I can see three possibilities:
  1. Return an error / panic
  2. Overwrite the peer state with a new entry
  3. Update the peer state with new data
For (1), my assumption is (that if used properly) there should already have been a peer_disconnected event even if this is a reconnect. So the entry would not exist in that case. Is my understanding accurate?
Currently (2) and (3) are equivalent but may not be in the future. You're implementing this as (3) though the code would be simplified if (2) is the desired behavior (i.e., you could blindly insert into the map instead of matching on the entry).
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Good question. I think in general if you don't follow the API, there are no guarantees period. It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key, but if a counterparty's private key leaks, we won't panic, it just may result in the counterparty losing their funds/channels.
@ariardariardJan 17, 2020•

edited

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Currently (2) and (3) are equivalent but may not be in the future.
Hmmm not sure I would change, it's state relative to the connection it shouldn't leak to the ChannelManager layer, if it's concerning channel+routing, assuming LN messages are ordered newer ones should take precedence.
It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key
Transport is assumed to be authenticated and encrypted so verification than message came from something which owns the relevant private key is done I think. That's said identity of the public key responder, how do you establish this knowledge is beyond specs. Keys leakage means your funds are loss, just send a HTLC emptying your balance to some exit node, it doesn't enter in the scope of option_upfront_shutdown_script.
 e.get().lock().unwrap().latest_features = init_msg.features.clone();
 },
 }
 }

 let mut channel_state_lock = self.channel_state.lock().unwrap();
 let channel_state = &mut *channel_state_lock;
 let pending_msg_events = &mut channel_state.pending_msg_events;
Expand DownExpand Up@@ -3123,6 +3162,14 @@ impl<ChanSigner: ChannelKeys + Writeable> Writeable for ChannelManager<ChanSigne
 }
 }

 let per_peer_state = self.per_peer_state.write().unwrap();
 (per_peer_state.len() as u64).write(writer)?;
 for (peer_pubkey, peer_state_mutex) in per_peer_state.iter() {
 peer_pubkey.write(writer)?;
 let peer_state = peer_state_mutex.lock().unwrap();
 peer_state.latest_features.write(writer)?;
 }

 Ok(())
	}
}
Expand DownExpand Up@@ -3256,6 +3303,16 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
 claimable_htlcs.insert(payment_hash, previous_hops);
 }

 let peer_count: u64 = Readable::read(reader)?;
 let mut per_peer_state = HashMap::with_capacity(cmp::min(peer_count as usize, 128));
 for _ in 0..peer_count {
 let peer_pubkey = Readable::read(reader)?;
 let peer_state = PeerState {
 latest_features: Readable::read(reader)?,
 };
 per_peer_state.insert(peer_pubkey, Mutex::new(peer_state));
 }

 let channel_manager = ChannelManager {
 genesis_hash,
 fee_estimator: args.fee_estimator,
Expand All@@ -3275,6 +3332,8 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
 }),
 our_network_key: args.keys_manager.get_node_secret(),

 per_peer_state: RwLock::new(per_peer_state),

 pending_events: Mutex::new(Vec::new()),
 total_consistency_lock: RwLock::new(()),
 keys_manager: args.keys_manager,
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
Plumb Features through into Routes by TheBlueMatt · Pull Request #433 · lightningdevkit/rust-lightning · GitHub
Skip to content
Plumb Features through into Routes#433

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.

 Sign up for GitHub

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

 Merged

TheBlueMatt
merged 4 commits into

from

Jan 22, 2020
 Merged


Plumb Features through into Routes
#433
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


4 commits


Select commit
Hold shift + click to select a range
d2ba7ca

Pass peer's Init message through to ChannelManager


TheBlueMatt Dec 28, 2019
a19d71d

Keep track of the Init Features for every connected/channel'd peer


TheBlueMatt Dec 29, 2019
912f877

Pass node features through to RouteHops


TheBlueMatt Dec 28, 2019
617a680

DRY-up list_channels by having a common lister that takes a filter


TheBlueMatt Jan 17, 2020

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


18 changes: 12 additions & 6 deletions
fuzz/src/chanmon_consistency.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -29,8 +29,8 @@ use lightning::ln::channelmonitor;
use lightning::ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateErr, HTLCUpdate};
use lightning::ln::channelmanager::{ChannelManager, PaymentHash, PaymentPreimage, ChannelManagerReadArgs};
use lightning::ln::router::{Route, RouteHop};
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC};
use lightning::ln::features::{ChannelFeatures, InitFeatures, NodeFeatures};
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, UpdateAddHTLC, Init};
use lightning::util::enforcing_trait_impls::EnforcingChannelKeys;
use lightning::util::events;
use lightning::util::logger::Logger;
Expand DownExpand Up@@ -414,7 +414,9 @@ pub fn do_test(data: &[u8]) {
 if let Err(_) = $source.send_payment(Route {
 hops: vec![RouteHop {
 pubkey: $dest.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $dest.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 5000000,
 cltv_expiry_delta: 200,
 }],
Expand All@@ -429,12 +431,16 @@ pub fn do_test(data: &[u8]) {
 if let Err(_) = $source.send_payment(Route {
 hops: vec![RouteHop {
 pubkey: $middle.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $middle.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 50000,
 cltv_expiry_delta: 100,
 },RouteHop {
 pubkey: $dest.0.get_our_node_id(),
 node_features: NodeFeatures::empty(),
 short_channel_id: $dest.1,
 channel_features: ChannelFeatures::empty(),
 fee_msat: 5000000,
 cltv_expiry_delta: 200,
 }],
Expand DownExpand Up@@ -650,15 +656,15 @@ pub fn do_test(data: &[u8]) {
 },
 0x11 => {
 if chan_a_disconnected {
 nodes[0].peer_connected(&nodes[1].get_our_node_id());
 nodes[1].peer_connected(&nodes[0].get_our_node_id());
 nodes[0].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
 nodes[1].peer_connected(&nodes[0].get_our_node_id(), &Init { features: InitFeatures::empty() });
 chan_a_disconnected = false;
 }
 },
 0x12 => {
 if chan_b_disconnected {
 nodes[1].peer_connected(&nodes[2].get_our_node_id());
 nodes[2].peer_connected(&nodes[1].get_our_node_id());
 nodes[1].peer_connected(&nodes[2].get_our_node_id(), &Init { features: InitFeatures::empty() });
 nodes[2].peer_connected(&nodes[1].get_our_node_id(), &Init { features: InitFeatures::empty() });
 chan_b_disconnected = false;
 }
 },
Expand Down

4 changes: 3 additions & 1 deletion
fuzz/src/router.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -5,8 +5,9 @@ use bitcoin::blockdata::transaction::Transaction;

use lightning::chain::chaininterface::{ChainError,ChainWatchInterface};
use lightning::ln::channelmanager::ChannelDetails;
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs;
use lightning::ln::msgs::{RoutingMessageHandler};
use lightning::ln::msgs::RoutingMessageHandler;
use lightning::ln::router::{Router, RouteHint};
use lightning::util::logger::Logger;
use lightning::util::ser::Readable;
Expand DownExpand Up@@ -198,6 +199,7 @@ pub fn do_test(data: &[u8]) {
 channel_id: [0; 32],
 short_channel_id: Some(slice_to_be64(get_slice!(8))),
 remote_network_id: get_pubkey!(),
 counterparty_features: InitFeatures::empty(),
 channel_value_satoshis: slice_to_be64(get_slice!(8)),
 user_id: 0,
 inbound_capacity_msat: 0,
Expand Down

24 changes: 12 additions & 12 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -215,10 +215,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
 nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
 assert_eq!(reestablish_1.len(), 1);
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
 assert_eq!(reestablish_2.len(), 1);

Expand All@@ -237,10 +237,10 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
 assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());

 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
 nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
 assert_eq!(reestablish_1.len(), 1);
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
 nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
 let reestablish_2 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
 assert_eq!(reestablish_2.len(), 1);

Expand DownExpand Up@@ -938,8 +938,8 @@ fn test_monitor_update_fail_reestablish() {
	commitment_signed_dance!(nodes[1], nodes[2], updates.commitment_signed, false);

	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reestablish = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reestablish = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand All@@ -954,8 +954,8 @@ fn test_monitor_update_fail_reestablish() {
	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	assert!(as_reestablish == get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id()));
	assert!(bs_reestablish == get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id()));
Expand DownExpand Up@@ -1118,8 +1118,8 @@ fn claim_while_disconnected_monitor_update_fail() {
	assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
	check_added_monitors!(nodes[1], 1);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand DownExpand Up@@ -1246,8 +1246,8 @@ fn monitor_failed_no_reestablish_response() {
	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);

	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });

	let as_reconnect = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
Expand Down

131 changes: 95 additions & 36 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -275,6 +275,12 @@ pub(super) struct ChannelHolder<ChanSigner: ChannelKeys> {
	pub(super) pending_msg_events: Vec<events::MessageSendEvent>,
}

/// State we hold per-peer. In the future we should put channels in here, but for now we only hold
/// the latest Init features we heard from the peer.
struct PeerState {
	latest_features: InitFeatures,
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

High-level question: Given this is duplicated in PeerManager's Peer structs, has there been any though around designing the API in terms of Peer and Channel abstractions?
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not sure what you mean - you mean something like replacing the their_node_id argument with some struct that holds more information (eg the init message)? We could, I suppose, though note that latest_features here is intended to be available even while disconnected (though maybe it doesn't actually need to be, just nice to have).
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm mostly thinking whether it would be suitable to design the overall API (not just this module) in terms of Peer and Channel abstractions and use them throughout to build higher-level abstractions like ChannelManager. Admittedly, this is outside the scope of this PR.
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Hmm. Maybe. It would certainly be tricky because a Peer "owns" a channel, but also the Peer can disconnect and reconnect and get back the same Channel. We can explore more in the future.
}

#[cfg(not(any(target_pointer_width = "32", target_pointer_width = "64")))]
const ERR: () = "You need at least 32 bit pointers (well, usize, but we'll assume they're the same) for ChannelManager::latest_block_height";

Expand DownExpand Up@@ -328,6 +334,14 @@ pub struct ChannelManager<ChanSigner: ChannelKeys> {
	channel_state: Mutex<ChannelHolder<ChanSigner>>,
	our_network_key: SecretKey,

	/// The bulk of our storage will eventually be here (channels and message queues and the like).
	/// If we are connected to a peer we always at least have an entry here, even if no channels
	/// are currently open with that peer.
	/// Because adding or removing an entry is rare, we usually take an outer read lock and then
	/// operate on the inner value freely. Sadly, this prevents parallel operation when opening a
	/// new channel.

Comment on lines

+340
 to 
+342

@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Not sure that I follow what is meant by "usually" here. Can it be removed?
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

As in, if you add or remove a connection you'll need the outer write lock, but usually we dont so take the read lock.
	per_peer_state: RwLock<HashMap<PublicKey, Mutex<PeerState>>>,

	pending_events: Mutex<Vec<events::Event>>,
	/// Used when we have to take a BIG lock to make sure everything is self-consistent.
	/// Essentially just when we're serializing ourselves out.
Expand DownExpand Up@@ -390,6 +404,10 @@ pub struct ChannelDetails {
	pub short_channel_id: Option<u64>,
	/// 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 user_id passed in to create_channel, or 0 if the channel was inbound.
Expand DownExpand Up@@ -610,6 +628,8 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
 }),
 our_network_key: keys_manager.get_node_secret(),

 per_peer_state: RwLock::new(HashMap::new()),

 pending_events: Mutex::new(Vec::new()),
 total_consistency_lock: RwLock::new(()),

Expand DownExpand Up@@ -660,56 +680,53 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
 Ok(())
	}

	/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
	/// more information.
	pub fn list_channels(&self) -> Vec<ChannelDetails> {
 let channel_state = self.channel_state.lock().unwrap();
 let mut res = Vec::with_capacity(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter() {
 let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
 res.push(ChannelDetails {
 channel_id: (*channel_id).clone(),
 short_channel_id: channel.get_short_channel_id(),
 remote_network_id: channel.get_their_node_id(),
 channel_value_satoshis: channel.get_value_satoshis(),
 inbound_capacity_msat,
 outbound_capacity_msat,
 user_id: channel.get_user_id(),
 is_live: channel.is_live(),
 });
 }
 res
	}

	/// Gets the list of usable channels, in random order. Useful as an argument to
	/// Router::get_route to ensure non-announced channels are used.
	///
	/// These are guaranteed to have their is_live value set to true, see the documentation for
	/// ChannelDetails::is_live for more info on exactly what the criteria are.
	pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
 let channel_state = self.channel_state.lock().unwrap();
 let mut res = Vec::with_capacity(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter() {
 // Note we use is_live here instead of usable which leads to somewhat confused
 // internal/external nomenclature, but that's ok cause that's probably what the user
 // really wanted anyway.
 if channel.is_live() {
	fn list_channels_with_filter<F: FnMut(&(&[u8; 32], &Channel<ChanSigner>)) -> bool>(&self, f: F) -> Vec<ChannelDetails> {
 let mut res = Vec::new();
 {
 let channel_state = self.channel_state.lock().unwrap();
 res.reserve(channel_state.by_id.len());
 for (channel_id, channel) in channel_state.by_id.iter().filter(f) {
 let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
 res.push(ChannelDetails {
 channel_id: (*channel_id).clone(),
 short_channel_id: channel.get_short_channel_id(),
 remote_network_id: channel.get_their_node_id(),
 counterparty_features: InitFeatures::empty(),
 channel_value_satoshis: channel.get_value_satoshis(),
 inbound_capacity_msat,
 outbound_capacity_msat,
 user_id: channel.get_user_id(),
 is_live: true,
 is_live: channel.is_live(),
 });
 }
 }
 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();
 }
 }
 res
	}

	/// Gets the list of open channels, in random order. See ChannelDetail field documentation for
	/// more information.
	pub fn list_channels(&self) -> Vec<ChannelDetails> {
 self.list_channels_with_filter(|_| true)
	}

	/// Gets the list of usable channels, in random order. Useful as an argument to
	/// Router::get_route to ensure non-announced channels are used.
	///
	/// These are guaranteed to have their is_live value set to true, see the documentation for
	/// ChannelDetails::is_live for more info on exactly what the criteria are.
	pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
 // Note we use is_live here instead of usable which leads to somewhat confused
 // internal/external nomenclature, but that's ok cause that's probably what the user
 // really wanted anyway.
 self.list_channels_with_filter(|&(_, ref channel)| channel.is_live())
	}

	/// Begins the process of closing a channel. After this call (plus some timeout), no new HTLCs
	/// will be accepted on the given channel, and after additional timeout/the closing of all
	/// pending HTLCs, the channel will be closed on chain.
Expand DownExpand Up@@ -2780,6 +2797,7 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 let _ = self.total_consistency_lock.read().unwrap();
 let mut failed_channels = Vec::new();
 let mut failed_payments = Vec::new();
 let mut no_channels_remain = true;
 {
 let mut channel_state_lock = self.channel_state.lock().unwrap();
 let channel_state = &mut *channel_state_lock;
Expand DownExpand Up@@ -2818,6 +2836,8 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 short_to_id.remove(&short_id);
 }
 return false;
 } else {
 no_channels_remain = false;
 }
 }
 true
Expand All@@ -2843,6 +2863,10 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 }
 });
 }
 if no_channels_remain {
 self.per_peer_state.write().unwrap().remove(their_node_id);
 }

 for failure in failed_channels.drain(..) {
 self.finish_force_close_channel(failure);
 }
Expand All@@ -2853,10 +2877,25 @@ impl<ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<ChanSigne
 }
	}

	fn peer_connected(&self, their_node_id: &PublicKey) {
	fn peer_connected(&self, their_node_id: &PublicKey, init_msg: &msgs::Init) {
 log_debug!(self, "Generating channel_reestablish events for {}", log_pubkey!(their_node_id));

 let _ = self.total_consistency_lock.read().unwrap();

 {
 let mut peer_state_lock = self.per_peer_state.write().unwrap();
 match peer_state_lock.entry(their_node_id.clone()) {
 hash_map::Entry::Vacant(e) => {
 e.insert(Mutex::new(PeerState {
 latest_features: init_msg.features.clone(),
 }));
 },
 hash_map::Entry::Occupied(e) => {
@ariardariardJan 7, 2020

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Is there any risk of peer collision ? An attacker can usurp pubkey available on the net but without privkey it won't get through the noise phase..and if a peer leaks its privkey we can't guarantee consistency of our structs (and we may have bigger troubles)
@TheBlueMattTheBlueMattJan 7, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Right. If you can impersonate a peer, that peer is screwed. Not a lot we can do then.
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

When would this occur? Won't PeerManager disconnect when it sees a duplicate id?
https://github.com/rust-bitcoin/rust-lightning/blob/a252f81477c25bf1248fd9ea5269a6620187c487/lightning/src/ln/peer_handler.rs#L540-L554
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

If you're using our PeerManager, yes, this should not be possible, though in theory these functions are public and the user may run their own PeerManager-like thing that may not properly check.
@jkczyzjkczyzJan 16, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What sort of semantics do we want to provide to users in this case and where do we plan on documenting it? I can see three possibilities:
  1. Return an error / panic
  2. Overwrite the peer state with a new entry
  3. Update the peer state with new data
For (1), my assumption is (that if used properly) there should already have been a peer_disconnected event even if this is a reconnect. So the entry would not exist in that case. Is my understanding accurate?
Currently (2) and (3) are equivalent but may not be in the future. You're implementing this as (3) though the code would be simplified if (2) is the desired behavior (i.e., you could blindly insert into the map instead of matching on the entry).
@TheBlueMattTheBlueMattJan 16, 2020

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Good question. I think in general if you don't follow the API, there are no guarantees period. It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key, but if a counterparty's private key leaks, we won't panic, it just may result in the counterparty losing their funds/channels.
@ariardariardJan 17, 2020•

edited

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Currently (2) and (3) are equivalent but may not be in the future.
Hmmm not sure I would change, it's state relative to the connection it shouldn't leak to the ChannelManager layer, if it's concerning channel+routing, assuming LN messages are ordered newer ones should take precedence.
It maybe could be further documented that you must somehow verify that a given message came from something that has proven knowledge of the relevant private key
Transport is assumed to be authenticated and encrypted so verification than message came from something which owns the relevant private key is done I think. That's said identity of the public key responder, how do you establish this knowledge is beyond specs. Keys leakage means your funds are loss, just send a HTLC emptying your balance to some exit node, it doesn't enter in the scope of option_upfront_shutdown_script.
 e.get().lock().unwrap().latest_features = init_msg.features.clone();
 },
 }
 }

 let mut channel_state_lock = self.channel_state.lock().unwrap();
 let channel_state = &mut *channel_state_lock;
 let pending_msg_events = &mut channel_state.pending_msg_events;
Expand DownExpand Up@@ -3123,6 +3162,14 @@ impl<ChanSigner: ChannelKeys + Writeable> Writeable for ChannelManager<ChanSigne
 }
 }

 let per_peer_state = self.per_peer_state.write().unwrap();
 (per_peer_state.len() as u64).write(writer)?;
 for (peer_pubkey, peer_state_mutex) in per_peer_state.iter() {
 peer_pubkey.write(writer)?;
 let peer_state = peer_state_mutex.lock().unwrap();
 peer_state.latest_features.write(writer)?;
 }

 Ok(())
	}
}
Expand DownExpand Up@@ -3256,6 +3303,16 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
 claimable_htlcs.insert(payment_hash, previous_hops);
 }

 let peer_count: u64 = Readable::read(reader)?;
 let mut per_peer_state = HashMap::with_capacity(cmp::min(peer_count as usize, 128));
 for _ in 0..peer_count {
 let peer_pubkey = Readable::read(reader)?;
 let peer_state = PeerState {
 latest_features: Readable::read(reader)?,
 };
 per_peer_state.insert(peer_pubkey, Mutex::new(peer_state));
 }

 let channel_manager = ChannelManager {
 genesis_hash,
 fee_estimator: args.fee_estimator,
Expand All@@ -3275,6 +3332,8 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArg
 }),
 our_network_key: args.keys_manager.get_node_secret(),

 per_peer_state: RwLock::new(per_peer_state),

 pending_events: Mutex::new(Vec::new()),
 total_consistency_lock: RwLock::new(()),
 keys_manager: args.keys_manager,
Expand Down
Loading