Skip to content

Commit 0dac107

Browse files
TheBlueMattTheBlueMatt
committed
Drop the channel_id argument from convert_channel_err
The `channel_id` argument to `convert_channel_err` allows us to set a different channel ID in the error message around the funding process. However, its not exactly critical to make sure we get it right, and yet another macro argument is annoying to deal with, so here we simply drop it and use the `Channel` value. This means that when force-closing we sometimes send an `error` with the `temporary_channel_id` rather than the funded `channel_id`, but its not that critical to get right (and we don't in all cases anyway), the peer will eventually figure it out when we reconnect or they try to send more messages about the channel.
1 parent 27ce012 commit 0dac107

File tree

2 files changed

+48
-51
lines changed

2 files changed

+48
-51
lines changed

‎lightning/src/ln/channelmanager.rs‎

Lines changed: 47 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -3215,32 +3215,33 @@ macro_rules! locked_close_channel {
32153215
/// Returns (boolean indicating if we should remove the Channel object from memory, a mapped error)
32163216
#[rustfmt::skip]
32173217
macro_rules! convert_channel_err {
3218-
($self: ident, $peer_state: expr, $err: expr, $chan: expr, $close: expr, $locked_close: expr, $channel_id: expr, _internal) => {
3218+
($self: ident, $peer_state: expr, $err: expr, $chan: expr, $close: expr, $locked_close: expr, $channel_id: expr, _internal) => { {
32193219
match $err {
32203220
ChannelError::Warn(msg) => {
3221-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), *$channel_id))
3221+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), $channel_id))
32223222
},
32233223
ChannelError::WarnAndDisconnect(msg) => {
3224-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::WarnAndDisconnect(msg), *$channel_id))
3224+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::WarnAndDisconnect(msg), $channel_id))
32253225
},
32263226
ChannelError::Ignore(msg) => {
3227-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), *$channel_id))
3227+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), $channel_id))
32283228
},
32293229
ChannelError::Close((msg, reason)) => {
32303230
let (mut shutdown_res, chan_update) = $close(reason);
32313231
let logger = WithChannelContext::from(&$self.logger, &$chan.context(), None);
32323232
log_error!(logger, "Closed channel {} due to close-required error: {}", $channel_id, msg);
32333233
$locked_close(&mut shutdown_res, $chan);
32343234
let err =
3235-
MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, chan_update);
3235+
MsgHandleErrInternal::from_finish_shutdown(msg, $channel_id, shutdown_res, chan_update);
32363236
(true, err)
32373237
},
32383238
ChannelError::SendError(msg) => {
3239-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::SendError(msg), *$channel_id))
3239+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::SendError(msg), $channel_id))
32403240
},
32413241
}
3242-
};
3243-
($self: ident, $peer_state: expr, $shutdown_result: expr, $funded_channel: expr, $channel_id: expr, COOP_CLOSED) => { {
3242+
} };
3243+
($self: ident, $peer_state: expr, $shutdown_result: expr, $funded_channel: expr, COOP_CLOSED) => { {
3244+
let chan_id = $funded_channel.context.channel_id();
32443245
let reason = ChannelError::Close(("Coop Closed".to_owned(), $shutdown_result.closure_reason.clone()));
32453246
let do_close = |_| {
32463247
(
@@ -3252,12 +3253,13 @@ macro_rules! convert_channel_err {
32523253
locked_close_channel!($self, $peer_state, funded_channel, shutdown_res_mut, FUNDED);
32533254
};
32543255
let (close, mut err) =
3255-
convert_channel_err!($self, $peer_state, reason, $funded_channel, do_close, locked_close, $channel_id, _internal);
3256+
convert_channel_err!($self, $peer_state, reason, $funded_channel, do_close, locked_close, chan_id, _internal);
32563257
err.dont_send_error_message();
32573258
debug_assert!(close);
32583259
(close, err)
32593260
} };
3260-
($self: ident, $peer_state: expr, $err: expr, $funded_channel: expr, $channel_id: expr, FUNDED_CHANNEL) => { {
3261+
($self: ident, $peer_state: expr, $err: expr, $funded_channel: expr, FUNDED_CHANNEL) => { {
3262+
let chan_id = $funded_channel.context.channel_id();
32613263
let mut do_close = |reason| {
32623264
(
32633265
$funded_channel.force_shutdown(reason),
@@ -3267,20 +3269,21 @@ macro_rules! convert_channel_err {
32673269
let mut locked_close = |shutdown_res_mut: &mut ShutdownResult, funded_channel: &mut FundedChannel<_>| {
32683270
locked_close_channel!($self, $peer_state, funded_channel, shutdown_res_mut, FUNDED);
32693271
};
3270-
convert_channel_err!($self, $peer_state, $err, $funded_channel, do_close, locked_close, $channel_id, _internal)
3272+
convert_channel_err!($self, $peer_state, $err, $funded_channel, do_close, locked_close, chan_id, _internal)
32713273
} };
3272-
($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr, UNFUNDED_CHANNEL) => { {
3274+
($self: ident, $peer_state: expr, $err: expr, $channel: expr, UNFUNDED_CHANNEL) => { {
3275+
let chan_id = $channel.context().channel_id();
32733276
let mut do_close = |reason| { ($channel.force_shutdown(reason), None) };
32743277
let locked_close = |_, chan: &mut Channel<_>| { locked_close_channel!($self, chan.context(), UNFUNDED); };
3275-
convert_channel_err!($self, $peer_state, $err, $channel, do_close, locked_close, $channel_id, _internal)
3278+
convert_channel_err!($self, $peer_state, $err, $channel, do_close, locked_close, chan_id, _internal)
32763279
} };
3277-
($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr) => {
3280+
($self: ident, $peer_state: expr, $err: expr, $channel: expr) => {
32783281
match $channel.as_funded_mut() {
32793282
Some(funded_channel) => {
3280-
convert_channel_err!($self, $peer_state, $err, funded_channel, $channel_id, FUNDED_CHANNEL)
3283+
convert_channel_err!($self, $peer_state, $err, funded_channel, FUNDED_CHANNEL)
32813284
},
32823285
None => {
3283-
convert_channel_err!($self, $peer_state, $err, $channel, $channel_id, UNFUNDED_CHANNEL)
3286+
convert_channel_err!($self, $peer_state, $err, $channel, UNFUNDED_CHANNEL)
32843287
},
32853288
}
32863289
};
@@ -3291,9 +3294,8 @@ macro_rules! break_channel_entry {
32913294
match $res {
32923295
Ok(res) => res,
32933296
Err(e) => {
3294-
let key = *$entry.key();
32953297
let (drop, res) =
3296-
convert_channel_err!($self, $peer_state, e, $entry.get_mut(), &key);
3298+
convert_channel_err!($self, $peer_state, e, $entry.get_mut());
32973299
if drop {
32983300
$entry.remove_entry();
32993301
}
@@ -3308,9 +3310,8 @@ macro_rules! try_channel_entry {
33083310
match $res {
33093311
Ok(res) => res,
33103312
Err(e) => {
3311-
let key = *$entry.key();
33123313
let (drop, res) =
3313-
convert_channel_err!($self, $peer_state, e, $entry.get_mut(), &key);
3314+
convert_channel_err!($self, $peer_state, e, $entry.get_mut());
33143315
if drop {
33153316
$entry.remove_entry();
33163317
}
@@ -4152,7 +4153,7 @@ where
41524153
let reason = ClosureReason::LocallyCoopClosedUnfundedChannel;
41534154
let err = ChannelError::Close((reason.to_string(), reason));
41544155
let mut chan = chan_entry.remove();
4155-
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, chan_id);
4156+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
41564157
e.dont_send_error_message();
41574158
shutdown_result = Err(e);
41584159
}
@@ -4346,7 +4347,7 @@ where
43464347
if let Some(mut chan) = peer_state.channel_by_id.remove(&channel_id) {
43474348
let reason = ClosureReason::FundingBatchClosure;
43484349
let err = ChannelError::Close((reason.to_string(), reason));
4349-
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
4350+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
43504351
shutdown_results.push((Err(e), counterparty_node_id));
43514352
}
43524353
}
@@ -4410,7 +4411,7 @@ where
44104411
if let Some(mut chan) = peer_state.channel_by_id.remove(channel_id) {
44114412
log_error!(logger, "Force-closing channel {}", channel_id);
44124413
let err = ChannelError::Close((message, reason));
4413-
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, channel_id);
4414+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
44144415
mem::drop(peer_state_lock);
44154416
mem::drop(per_peer_state);
44164417
if is_from_counterparty {
@@ -5845,7 +5846,7 @@ where
58455846
let reason = ClosureReason::ProcessingError { err: e.clone() };
58465847
let err = ChannelError::Close((e.clone(), reason));
58475848
let (_, e) =
5848-
convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
5849+
convert_channel_err!(self, peer_state, err, &mut chan);
58495850
shutdown_results.push((Err(e), counterparty_node_id));
58505851
});
58515852
}
@@ -7011,7 +7012,7 @@ where
70117012
if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
70127013

70137014
if let Err(e) = funded_chan.timer_check_closing_negotiation_progress() {
7014-
let (needs_close, err) = convert_channel_err!(self, peer_state, e, funded_chan, chan_id, FUNDED_CHANNEL);
7015+
let (needs_close, err) = convert_channel_err!(self, peer_state, e, funded_chan, FUNDED_CHANNEL);
70157016
handle_errors.push((Err(err), counterparty_node_id));
70167017
if needs_close { return false; }
70177018
}
@@ -7089,7 +7090,7 @@ where
70897090
let reason = ClosureReason::FundingTimedOut;
70907091
let msg = "Force-closing pending channel due to timeout awaiting establishment handshake".to_owned();
70917092
let err = ChannelError::Close((msg, reason));
7092-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
7093+
let (_, e) = convert_channel_err!(self, peer_state, err, chan);
70937094
handle_errors.push((Err(e), counterparty_node_id));
70947095
false
70957096
} else {
@@ -8668,18 +8669,15 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
86688669
// above so at this point we just need to clean up any lingering entries
86698670
// concerning this channel as it is safe to do so.
86708671
debug_assert!(matches!(err, ChannelError::Close(_)));
8671-
// Really we should be returning the channel_id the peer expects based
8672-
// on their funding info here, but they're horribly confused anyway, so
8673-
// there's not a lot we can do to save them.
86748672
let mut chan = Channel::from(inbound_chan);
8675-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id, UNFUNDED_CHANNEL).1);
8673+
return Err(convert_channel_err!(self, peer_state, err, &mut chan).1);
86768674
},
86778675
}
86788676
},
86798677
Some(Err(mut chan)) => {
86808678
let err_msg = format!("Got an unexpected funding_created message from peer with counterparty_node_id {}", counterparty_node_id);
86818679
let err = ChannelError::close(err_msg);
8682-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id).1);
8680+
return Err(convert_channel_err!(self, peer_state, err, &mut chan).1);
86838681
},
86848682
None => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.temporary_channel_id))
86858683
};
@@ -8695,7 +8693,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
86958693
let err = ChannelError::close($err.to_owned());
86968694
chan.unset_funding_info();
86978695
let mut chan = Channel::from(chan);
8698-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &funded_channel_id, UNFUNDED_CHANNEL).1);
8696+
return Err(convert_channel_err!(self, peer_state, err, &mut chan, UNFUNDED_CHANNEL).1);
86998697
} } }
87008698

87018699
match peer_state.channel_by_id.entry(funded_channel_id) {
@@ -9236,7 +9234,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
92369234
let err = ChannelError::Close((reason.to_string(), reason));
92379235
let mut chan = chan_entry.remove();
92389236
let (_, mut e) =
9239-
convert_channel_err!(self, peer_state, err, &mut chan, &msg.channel_id);
9237+
convert_channel_err!(self, peer_state, err, &mut chan);
92409238
e.dont_send_error_message();
92419239
return Err(e);
92429240
},
@@ -9295,7 +9293,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
92959293
// also implies there are no pending HTLCs left on the channel, so we can
92969294
// fully delete it from tracking (the channel monitor is still around to
92979295
// watch for old state broadcasts)!
9298-
let (_, err) = convert_channel_err!(self, peer_state, close_res, chan, &msg.channel_id, COOP_CLOSED);
9296+
let (_, err) = convert_channel_err!(self, peer_state, close_res, chan, COOP_CLOSED);
92999297
chan_entry.remove();
93009298
Some((tx, Err(err)))
93019299
} else {
@@ -10258,7 +10256,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1025810256
};
1025910257
let err = ChannelError::Close((reason.to_string(), reason));
1026010258
let mut chan = chan_entry.remove();
10261-
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
10259+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
1026210260
failed_channels.push((Err(e), counterparty_node_id));
1026310261
}
1026410262
}
@@ -10446,12 +10444,12 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1044610444
let chan_id = context.channel_id();
1044710445
log_trace!(logger, "Removing channel {} now that the signer is unblocked", chan_id);
1044810446
let (remove, err) = if let Some(funded_channel) = chan.as_funded_mut() {
10449-
convert_channel_err!(self, peer_state, shutdown_res, funded_channel, &chan_id, COOP_CLOSED)
10447+
convert_channel_err!(self, peer_state, shutdown_res, funded_channel, COOP_CLOSED)
1045010448
} else {
1045110449
debug_assert!(false);
1045210450
let reason = shutdown_res.closure_reason.clone();
1045310451
let err = ChannelError::Close((reason.to_string(), reason));
10454-
convert_channel_err!(self, peer_state, err, chan, &chan_id, UNFUNDED_CHANNEL)
10452+
convert_channel_err!(self, peer_state, err, chan, UNFUNDED_CHANNEL)
1045510453
};
1045610454
debug_assert!(remove);
1045710455
shutdown_results.push((Err(err), *cp_id));
@@ -10481,7 +10479,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1048110479
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1048210480
let peer_state = &mut *peer_state_lock;
1048310481
let pending_msg_events = &mut peer_state.pending_msg_events;
10484-
peer_state.channel_by_id.retain(|channel_id, chan| {
10482+
peer_state.channel_by_id.retain(|_, chan| {
1048510483
match chan.as_funded_mut() {
1048610484
Some(funded_chan) => {
1048710485
let logger = WithChannelContext::from(&self.logger, &funded_chan.context, None);
@@ -10497,7 +10495,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1049710495
if let Some((tx, shutdown_res)) = tx_shutdown_result_opt {
1049810496
// We're done with this channel. We got a closing_signed and sent back
1049910497
// a closing_signed with a closing transaction to broadcast.
10500-
let (_, err) = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, channel_id, COOP_CLOSED);
10498+
let (_, err) = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, COOP_CLOSED);
1050110499
handle_errors.push((*cp_id, Err(err)));
1050210500

1050310501
log_info!(logger, "Broadcasting {}", log_tx!(tx));
@@ -10507,7 +10505,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1050710505
},
1050810506
Err(e) => {
1050910507
has_update = true;
10510-
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, channel_id, FUNDED_CHANNEL);
10508+
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, FUNDED_CHANNEL);
1051110509
handle_errors.push((funded_chan.context.get_counterparty_node_id(), Err(res)));
1051210510
!close_channel
1051310511
}
@@ -11692,15 +11690,15 @@ where
1169211690
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1169311691
let peer_state = &mut *peer_state_lock;
1169411692
let pending_msg_events = &mut peer_state.pending_msg_events;
11695-
peer_state.channel_by_id.retain(|chan_id, chan| {
11693+
peer_state.channel_by_id.retain(|_, chan| {
1169611694
let logger = WithChannelContext::from(&self.logger, &chan.context(), None);
1169711695
if chan.peer_disconnected_is_resumable(&&logger) {
1169811696
return true;
1169911697
}
1170011698
// Clean up for removal.
1170111699
let reason = ClosureReason::DisconnectedPeer;
1170211700
let err = ChannelError::Close((reason.to_string(), reason));
11703-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
11701+
let (_, e) = convert_channel_err!(self, peer_state, err, chan);
1170411702
failed_channels.push((Err(e), counterparty_node_id));
1170511703
false
1170611704
});
@@ -12246,7 +12244,7 @@ where
1224612244
let peer_state = &mut *peer_state_lock;
1224712245
let pending_msg_events = &mut peer_state.pending_msg_events;
1224812246

12249-
peer_state.channel_by_id.retain(|chan_id, chan| {
12247+
peer_state.channel_by_id.retain(|channel_id, chan| {
1225012248
match chan.as_funded_mut() {
1225112249
// Retain unfunded channels.
1225212250
None => true,
@@ -12257,22 +12255,22 @@ where
1225712255
let reason = LocalHTLCFailureReason::CLTVExpiryTooSoon;
1225812256
let data = self.get_htlc_inbound_temp_fail_data(reason);
1225912257
timed_out_htlcs.push((source, payment_hash, HTLCFailReason::reason(reason, data),
12260-
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: funded_channel.context.channel_id() }));
12258+
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: *channel_id }));
1226112259
}
1226212260
let logger = WithChannelContext::from(&self.logger, &funded_channel.context, None);
1226312261
match funding_confirmed_opt {
1226412262
Some(FundingConfirmedMessage::Establishment(channel_ready)) => {
1226512263
send_channel_ready!(self, pending_msg_events, funded_channel, channel_ready);
1226612264
if funded_channel.context.is_usable() {
12267-
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", funded_channel.context.channel_id());
12265+
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", channel_id);
1226812266
if let Ok(msg) = self.get_channel_update_for_unicast(funded_channel) {
1226912267
pending_msg_events.push(MessageSendEvent::SendChannelUpdate {
1227012268
node_id: funded_channel.context.get_counterparty_node_id(),
1227112269
msg,
1227212270
});
1227312271
}
1227412272
} else {
12275-
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", funded_channel.context.channel_id());
12273+
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", channel_id);
1227612274
}
1227712275
},
1227812276
#[cfg(splicing)]
@@ -12283,7 +12281,7 @@ where
1228312281

1228412282
let mut pending_events = self.pending_events.lock().unwrap();
1228512283
pending_events.push_back((events::Event::ChannelReady {
12286-
channel_id: funded_channel.context.channel_id(),
12284+
channel_id,
1228712285
user_channel_id: funded_channel.context.get_user_id(),
1228812286
counterparty_node_id: funded_channel.context.get_counterparty_node_id(),
1228912287
funding_txo: funding_txo.map(|outpoint| outpoint.into_bitcoin_outpoint()),
@@ -12355,8 +12353,8 @@ where
1235512353
// un-confirmed we force-close the channel, ensuring short_to_chan_info
1235612354
// is always consistent.
1235712355
let mut short_to_chan_info = self.short_to_chan_info.write().unwrap();
12358-
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()));
12359-
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()),
12356+
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), *channel_id));
12357+
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), *channel_id),
1236012358
"SCIDs should never collide - ensure you weren't behind by a full {} blocks when creating channels",
1236112359
fake_scid::MAX_SCID_BLOCKS_FROM_NOW);
1236212360
}
@@ -12370,7 +12368,6 @@ where
1237012368
peer_state,
1237112369
err,
1237212370
funded_channel,
12373-
chan_id,
1237412371
FUNDED_CHANNEL
1237512372
);
1237612373
failed_channels.push((Err(e), *counterparty_node_id));

‎lightning/src/ln/functional_tests.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9067,7 +9067,7 @@ pub fn test_duplicate_chan_id() {
90679067
} => {
90689068
// Technically, at this point, nodes[1] would be justified in thinking both
90699069
// channels are closed, but currently we do not, so we just move forward with it.
9070-
assert_eq!(msg.channel_id, channel_id);
9070+
assert_eq!(msg.channel_id, funding_created.temporary_channel_id);
90719071
assert_eq!(node_id, node_a_id);
90729072
},
90739073
_ => panic!("Unexpected event"),

0 commit comments

Comments
 (0)