Skip to content

Support for SCID Aliases #1311

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Mar 10, 2022
Merged
1 change: 1 addition & 0 deletions fuzz/src/router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
},
funding_txo: Some(OutPoint { txid: bitcoin::Txid::from_slice(&[0; 32]).unwrap(), index: 0 }),
short_channel_id: Some(scid),
inbound_scid_alias: None,
channel_value_satoshis: slice_to_be64(get_slice!(8)),
user_channel_id: 0, inbound_capacity_msat: 0,
unspendable_punishment_reserve: None,
Expand Down
11 changes: 9 additions & 2 deletions lightning-invoice/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ pub fn create_phantom_invoice<Signer: Sign, K: Deref>(

for hint in phantom_route_hints {
for channel in &hint.channels {
let short_channel_id = match channel.short_channel_id {
let short_channel_id = match channel.get_inbound_payment_scid() {
Some(id) => id,
None => continue,
};
Expand Down Expand Up @@ -162,7 +162,7 @@ where
let our_channels = channelmanager.list_usable_channels();
let mut route_hints = vec![];
for channel in our_channels {
let short_channel_id = match channel.short_channel_id {
let short_channel_id = match channel.get_inbound_payment_scid() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do something similar when we're sending a payment, i.e. when use the alias when getting first_hops for the route?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll leave that for the 0conf PR - we don't need to care right now cause first-hop SCID isn't communicated anywhere, its just an internal tracker for us, so its only needed after 0conf.

Some(id) => id,
None => continue,
};
Expand Down Expand Up @@ -313,6 +313,13 @@ mod test {
assert_eq!(invoice.min_final_cltv_expiry(), MIN_FINAL_CLTV_EXPIRY as u64);
assert_eq!(invoice.description(), InvoiceDescription::Direct(&Description("test".to_string())));

// Invoice SCIDs should always use inbound SCID aliases over the real channel ID, if one is
// available.
assert_eq!(invoice.route_hints().len(), 1);
assert_eq!(invoice.route_hints()[0].0.len(), 1);
assert_eq!(invoice.route_hints()[0].0[0].short_channel_id,
nodes[1].node.list_usable_channels()[0].inbound_scid_alias.unwrap());

let payment_params = PaymentParameters::from_node_id(invoice.recover_payee_pub_key())
.with_features(invoice.features().unwrap().clone())
.with_route_hints(invoice.route_hints());
Expand Down
114 changes: 93 additions & 21 deletions lightning/src/ln/channel.rs

Large diffs are not rendered by default.

263 changes: 188 additions & 75 deletions lightning/src/ln/channelmanager.rs

Large diffs are not rendered by default.

60 changes: 60 additions & 0 deletions lightning/src/ln/functional_test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,61 @@ pub fn create_announced_chan_between_nodes_with_value<'a, 'b, 'c, 'd>(nodes: &'a
(chan_announcement.1, chan_announcement.2, chan_announcement.3, chan_announcement.4)
}

pub fn create_unannounced_chan_between_nodes_with_value<'a, 'b, 'c, 'd>(nodes: &'a Vec<Node<'b, 'c, 'd>>, a: usize, b: usize, channel_value: u64, push_msat: u64, a_flags: InitFeatures, b_flags: InitFeatures) -> (msgs::FundingLocked, Transaction) {
let mut no_announce_cfg = test_default_channel_config();
no_announce_cfg.channel_options.announced_channel = false;
Comment on lines +696 to +698
Copy link
Contributor

Choose a reason for hiding this comment

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

Getting some deja vu from another PR. Is there a way to reuse some existing utility code such that the code that follows these lines isn't needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, all the existing utilities end up either calling create_channel itself or expecting to exchange AnnouncementSignatures at the end. I could rejigger the utilities above to separate those bits out, if you prefer? I don't really have a strong feeling between that vs having some code duplication in utilities right around each other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't feel too strongly. I think I already asked another contributor to do the same in another PR, heh.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, on second thought I'm kinda struggling to see how to do this - the top 7 lines could be split out into three lines and four lines, but that doesn't seem so useful. Then there's the funding generation, which almost can be combined, except that we rely on exchanging the funding_locked messages in order so that we only get one channel_update out at a time - we could swap for the create_chan_between_nodes_with_value_confirm_first helper, but then we're stick peeling apart multiple pending messages which takes more LoC than we save :(.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem with passing on this. My concern was more with code repetition resulting in needing to modify two places rather than reducing lines of code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, I agree, its unclear how to resolve it in this case, though, without digging through a number of other tests and changing their util method calls, which is yet more diff in this PR :/

nodes[a].node.create_channel(nodes[b].node.get_our_node_id(), channel_value, push_msat, 42, Some(no_announce_cfg)).unwrap();
let open_channel = get_event_msg!(nodes[a], MessageSendEvent::SendOpenChannel, nodes[b].node.get_our_node_id());
nodes[b].node.handle_open_channel(&nodes[a].node.get_our_node_id(), a_flags, &open_channel);
let accept_channel = get_event_msg!(nodes[b], MessageSendEvent::SendAcceptChannel, nodes[a].node.get_our_node_id());
nodes[a].node.handle_accept_channel(&nodes[b].node.get_our_node_id(), b_flags, &accept_channel);

let (temporary_channel_id, tx, _) = create_funding_transaction(&nodes[a], channel_value, 42);
nodes[a].node.funding_transaction_generated(&temporary_channel_id, tx.clone()).unwrap();
nodes[b].node.handle_funding_created(&nodes[a].node.get_our_node_id(), &get_event_msg!(nodes[a], MessageSendEvent::SendFundingCreated, nodes[b].node.get_our_node_id()));
check_added_monitors!(nodes[b], 1);

let cs_funding_signed = get_event_msg!(nodes[b], MessageSendEvent::SendFundingSigned, nodes[a].node.get_our_node_id());
nodes[a].node.handle_funding_signed(&nodes[b].node.get_our_node_id(), &cs_funding_signed);
check_added_monitors!(nodes[a], 1);

let conf_height = core::cmp::max(nodes[a].best_block_info().1 + 1, nodes[b].best_block_info().1 + 1);
confirm_transaction_at(&nodes[a], &tx, conf_height);
connect_blocks(&nodes[a], CHAN_CONFIRM_DEPTH - 1);
confirm_transaction_at(&nodes[b], &tx, conf_height);
connect_blocks(&nodes[b], CHAN_CONFIRM_DEPTH - 1);
let as_funding_locked = get_event_msg!(nodes[a], MessageSendEvent::SendFundingLocked, nodes[b].node.get_our_node_id());
nodes[a].node.handle_funding_locked(&nodes[b].node.get_our_node_id(), &get_event_msg!(nodes[b], MessageSendEvent::SendFundingLocked, nodes[a].node.get_our_node_id()));
let as_update = get_event_msg!(nodes[a], MessageSendEvent::SendChannelUpdate, nodes[b].node.get_our_node_id());
nodes[b].node.handle_funding_locked(&nodes[a].node.get_our_node_id(), &as_funding_locked);
let bs_update = get_event_msg!(nodes[b], MessageSendEvent::SendChannelUpdate, nodes[a].node.get_our_node_id());

nodes[a].node.handle_channel_update(&nodes[b].node.get_our_node_id(), &bs_update);
nodes[b].node.handle_channel_update(&nodes[a].node.get_our_node_id(), &as_update);

let mut found_a = false;
for chan in nodes[a].node.list_usable_channels() {
if chan.channel_id == as_funding_locked.channel_id {
assert!(!found_a);
found_a = true;
assert!(!chan.is_public);
}
}
assert!(found_a);

let mut found_b = false;
for chan in nodes[b].node.list_usable_channels() {
if chan.channel_id == as_funding_locked.channel_id {
assert!(!found_b);
found_b = true;
assert!(!chan.is_public);
}
}
assert!(found_b);

(as_funding_locked, tx)
}

pub fn update_nodes_with_chan_announce<'a, 'b, 'c, 'd>(nodes: &'a Vec<Node<'b, 'c, 'd>>, a: usize, b: usize, ann: &msgs::ChannelAnnouncement, upd_1: &msgs::ChannelUpdate, upd_2: &msgs::ChannelUpdate) {
nodes[a].node.broadcast_node_announcement([0, 0, 0], [0; 32], Vec::new());
let a_events = nodes[a].node.get_and_clear_pending_msg_events();
Expand Down Expand Up @@ -748,6 +803,11 @@ pub fn update_nodes_with_chan_announce<'a, 'b, 'c, 'd>(nodes: &'a Vec<Node<'b, '
node.net_graph_msg_handler.handle_channel_update(upd_2).unwrap();
node.net_graph_msg_handler.handle_node_announcement(&a_node_announcement).unwrap();
node.net_graph_msg_handler.handle_node_announcement(&b_node_announcement).unwrap();

// Note that channel_updates are also delivered to ChannelManagers to ensure we have
// forwarding info for local channels even if its not accepted in the network graph.
node.node.handle_channel_update(&nodes[a].node.get_our_node_id(), &upd_1);
node.node.handle_channel_update(&nodes[b].node.get_our_node_id(), &upd_2);
}
}

Expand Down
Loading