Skip to content

Commit bbf200e

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 0574bf8 commit bbf200e

File tree

2 files changed

+48
-54
lines changed

2 files changed

+48
-54
lines changed

‎lightning/src/ln/channelmanager.rs

Lines changed: 47 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -3217,32 +3217,33 @@ macro_rules! locked_close_channel {
32173217
/// Returns (boolean indicating if we should remove the Channel object from memory, a mapped error)
32183218
#[rustfmt::skip]
32193219
macro_rules! convert_channel_err {
3220-
($self: ident, $peer_state: expr, $err: expr, $chan: expr, $close: expr, $locked_close: expr, $channel_id: expr, _internal) => {
3220+
($self: ident, $peer_state: expr, $err: expr, $chan: expr, $close: expr, $locked_close: expr, $channel_id: expr, _internal) => { {
32213221
match $err {
32223222
ChannelError::Warn(msg) => {
3223-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), *$channel_id))
3223+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), $channel_id))
32243224
},
32253225
ChannelError::WarnAndDisconnect(msg) => {
3226-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::WarnAndDisconnect(msg), *$channel_id))
3226+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::WarnAndDisconnect(msg), $channel_id))
32273227
},
32283228
ChannelError::Ignore(msg) => {
3229-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), *$channel_id))
3229+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), $channel_id))
32303230
},
32313231
ChannelError::Close((msg, reason)) => {
32323232
let (mut shutdown_res, chan_update) = $close(reason);
32333233
let logger = WithChannelContext::from(&$self.logger, &$chan.context(), None);
32343234
log_error!(logger, "Closed channel {} due to close-required error: {}", $channel_id, msg);
32353235
$locked_close(&mut shutdown_res, $chan);
32363236
let err =
3237-
MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, chan_update);
3237+
MsgHandleErrInternal::from_finish_shutdown(msg, $channel_id, shutdown_res, chan_update);
32383238
(true, err)
32393239
},
32403240
ChannelError::SendError(msg) => {
3241-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::SendError(msg), *$channel_id))
3241+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::SendError(msg), $channel_id))
32423242
},
32433243
}
3244-
};
3245-
($self: ident, $peer_state: expr, $shutdown_result: expr, $funded_channel: expr, $channel_id: expr, COOP_CLOSED) => { {
3244+
} };
3245+
($self: ident, $peer_state: expr, $shutdown_result: expr, $funded_channel: expr, COOP_CLOSED) => { {
3246+
let chan_id = $funded_channel.context.channel_id();
32463247
let reason = ChannelError::Close(("Coop Closed".to_owned(), $shutdown_result.closure_reason.clone()));
32473248
let do_close = |_| {
32483249
(
@@ -3254,12 +3255,13 @@ macro_rules! convert_channel_err {
32543255
locked_close_channel!($self, $peer_state, funded_channel, shutdown_res_mut, FUNDED);
32553256
};
32563257
let (close, mut err) =
3257-
convert_channel_err!($self, $peer_state, reason, $funded_channel, do_close, locked_close, $channel_id, _internal);
3258+
convert_channel_err!($self, $peer_state, reason, $funded_channel, do_close, locked_close, chan_id, _internal);
32583259
err.dont_send_error_message();
32593260
debug_assert!(close);
32603261
(close, err)
32613262
} };
3262-
($self: ident, $peer_state: expr, $err: expr, $funded_channel: expr, $channel_id: expr, FUNDED_CHANNEL) => { {
3263+
($self: ident, $peer_state: expr, $err: expr, $funded_channel: expr, FUNDED_CHANNEL) => { {
3264+
let chan_id = $funded_channel.context.channel_id();
32633265
let mut do_close = |reason| {
32643266
(
32653267
$funded_channel.force_shutdown(reason),
@@ -3269,20 +3271,21 @@ macro_rules! convert_channel_err {
32693271
let mut locked_close = |shutdown_res_mut: &mut ShutdownResult, funded_channel: &mut FundedChannel<_>| {
32703272
locked_close_channel!($self, $peer_state, funded_channel, shutdown_res_mut, FUNDED);
32713273
};
3272-
convert_channel_err!($self, $peer_state, $err, $funded_channel, do_close, locked_close, $channel_id, _internal)
3274+
convert_channel_err!($self, $peer_state, $err, $funded_channel, do_close, locked_close, chan_id, _internal)
32733275
} };
3274-
($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr, UNFUNDED_CHANNEL) => { {
3276+
($self: ident, $peer_state: expr, $err: expr, $channel: expr, UNFUNDED_CHANNEL) => { {
3277+
let chan_id = $channel.context().channel_id();
32753278
let mut do_close = |reason| { ($channel.force_shutdown(reason), None) };
32763279
let locked_close = |_, chan: &mut Channel<_>| { locked_close_channel!($self, chan.context(), UNFUNDED); };
3277-
convert_channel_err!($self, $peer_state, $err, $channel, do_close, locked_close, $channel_id, _internal)
3280+
convert_channel_err!($self, $peer_state, $err, $channel, do_close, locked_close, chan_id, _internal)
32783281
} };
3279-
($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr) => {
3282+
($self: ident, $peer_state: expr, $err: expr, $channel: expr) => {
32803283
match $channel.as_funded_mut() {
32813284
Some(funded_channel) => {
3282-
convert_channel_err!($self, $peer_state, $err, funded_channel, $channel_id, FUNDED_CHANNEL)
3285+
convert_channel_err!($self, $peer_state, $err, funded_channel, FUNDED_CHANNEL)
32833286
},
32843287
None => {
3285-
convert_channel_err!($self, $peer_state, $err, $channel, $channel_id, UNFUNDED_CHANNEL)
3288+
convert_channel_err!($self, $peer_state, $err, $channel, UNFUNDED_CHANNEL)
32863289
},
32873290
}
32883291
};
@@ -3293,9 +3296,7 @@ macro_rules! break_channel_entry {
32933296
match $res {
32943297
Ok(res) => res,
32953298
Err(e) => {
3296-
let key = *$entry.key();
3297-
let (drop, res) =
3298-
convert_channel_err!($self, $peer_state, e, $entry.get_mut(), &key);
3299+
let (drop, res) = convert_channel_err!($self, $peer_state, e, $entry.get_mut());
32993300
if drop {
33003301
$entry.remove_entry();
33013302
}
@@ -3310,9 +3311,7 @@ macro_rules! try_channel_entry {
33103311
match $res {
33113312
Ok(res) => res,
33123313
Err(e) => {
3313-
let key = *$entry.key();
3314-
let (drop, res) =
3315-
convert_channel_err!($self, $peer_state, e, $entry.get_mut(), &key);
3314+
let (drop, res) = convert_channel_err!($self, $peer_state, e, $entry.get_mut());
33163315
if drop {
33173316
$entry.remove_entry();
33183317
}
@@ -4154,7 +4153,7 @@ where
41544153
let reason = ClosureReason::LocallyCoopClosedUnfundedChannel;
41554154
let err = ChannelError::Close((reason.to_string(), reason));
41564155
let mut chan = chan_entry.remove();
4157-
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);
41584157
e.dont_send_error_message();
41594158
shutdown_result = Err(e);
41604159
}
@@ -4351,7 +4350,7 @@ where
43514350
if let Some(mut chan) = peer_state.channel_by_id.remove(&channel_id) {
43524351
let reason = ClosureReason::FundingBatchClosure;
43534352
let err = ChannelError::Close((reason.to_string(), reason));
4354-
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
4353+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
43554354
shutdown_results.push((Err(e), counterparty_node_id));
43564355
}
43574356
}
@@ -4415,7 +4414,7 @@ where
44154414
if let Some(mut chan) = peer_state.channel_by_id.remove(channel_id) {
44164415
log_error!(logger, "Force-closing channel {}", channel_id);
44174416
let err = ChannelError::Close((message, reason));
4418-
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, channel_id);
4417+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
44194418
mem::drop(peer_state_lock);
44204419
mem::drop(per_peer_state);
44214420
if is_from_counterparty {
@@ -5894,7 +5893,7 @@ where
58945893
let reason = ClosureReason::ProcessingError { err: e.clone() };
58955894
let err = ChannelError::Close((e.clone(), reason));
58965895
let (_, e) =
5897-
convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
5896+
convert_channel_err!(self, peer_state, err, &mut chan);
58985897
shutdown_results.push((Err(e), counterparty_node_id));
58995898
});
59005899
}
@@ -7062,7 +7061,7 @@ where
70627061
if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
70637062

70647063
if let Err(e) = funded_chan.timer_check_closing_negotiation_progress() {
7065-
let (needs_close, err) = convert_channel_err!(self, peer_state, e, funded_chan, chan_id, FUNDED_CHANNEL);
7064+
let (needs_close, err) = convert_channel_err!(self, peer_state, e, funded_chan, FUNDED_CHANNEL);
70667065
handle_errors.push((Err(err), counterparty_node_id));
70677066
if needs_close { return false; }
70687067
}
@@ -7140,7 +7139,7 @@ where
71407139
let reason = ClosureReason::FundingTimedOut;
71417140
let msg = "Force-closing pending channel due to timeout awaiting establishment handshake".to_owned();
71427141
let err = ChannelError::Close((msg, reason));
7143-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
7142+
let (_, e) = convert_channel_err!(self, peer_state, err, chan);
71447143
handle_errors.push((Err(e), counterparty_node_id));
71457144
false
71467145
} else {
@@ -8726,18 +8725,15 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
87268725
// above so at this point we just need to clean up any lingering entries
87278726
// concerning this channel as it is safe to do so.
87288727
debug_assert!(matches!(err, ChannelError::Close(_)));
8729-
// Really we should be returning the channel_id the peer expects based
8730-
// on their funding info here, but they're horribly confused anyway, so
8731-
// there's not a lot we can do to save them.
87328728
let mut chan = Channel::from(inbound_chan);
8733-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id, UNFUNDED_CHANNEL).1);
8729+
return Err(convert_channel_err!(self, peer_state, err, &mut chan).1);
87348730
},
87358731
}
87368732
},
87378733
Some(Err(mut chan)) => {
87388734
let err_msg = format!("Got an unexpected funding_created message from peer with counterparty_node_id {}", counterparty_node_id);
87398735
let err = ChannelError::close(err_msg);
8740-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id).1);
8736+
return Err(convert_channel_err!(self, peer_state, err, &mut chan).1);
87418737
},
87428738
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))
87438739
};
@@ -8753,7 +8749,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
87538749
let err = ChannelError::close($err.to_owned());
87548750
chan.unset_funding_info();
87558751
let mut chan = Channel::from(chan);
8756-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &funded_channel_id, UNFUNDED_CHANNEL).1);
8752+
return Err(convert_channel_err!(self, peer_state, err, &mut chan, UNFUNDED_CHANNEL).1);
87578753
} } }
87588754

87598755
match peer_state.channel_by_id.entry(funded_channel_id) {
@@ -9293,8 +9289,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
92939289
let reason = ClosureReason::CounterpartyCoopClosedUnfundedChannel;
92949290
let err = ChannelError::Close((reason.to_string(), reason));
92959291
let mut chan = chan_entry.remove();
9296-
let (_, mut e) =
9297-
convert_channel_err!(self, peer_state, err, &mut chan, &msg.channel_id);
9292+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
92989293
e.dont_send_error_message();
92999294
return Err(e);
93009295
},
@@ -9353,7 +9348,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
93539348
// also implies there are no pending HTLCs left on the channel, so we can
93549349
// fully delete it from tracking (the channel monitor is still around to
93559350
// watch for old state broadcasts)!
9356-
let (_, err) = convert_channel_err!(self, peer_state, close_res, chan, &msg.channel_id, COOP_CLOSED);
9351+
let (_, err) = convert_channel_err!(self, peer_state, close_res, chan, COOP_CLOSED);
93579352
chan_entry.remove();
93589353
Some((tx, Err(err)))
93599354
} else {
@@ -10312,7 +10307,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1031210307
};
1031310308
let err = ChannelError::Close((reason.to_string(), reason));
1031410309
let mut chan = chan_entry.remove();
10315-
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
10310+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
1031610311
failed_channels.push((Err(e), counterparty_node_id));
1031710312
}
1031810313
}
@@ -10500,12 +10495,12 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1050010495
let chan_id = context.channel_id();
1050110496
log_trace!(logger, "Removing channel {} now that the signer is unblocked", chan_id);
1050210497
let (remove, err) = if let Some(funded_channel) = chan.as_funded_mut() {
10503-
convert_channel_err!(self, peer_state, shutdown_res, funded_channel, &chan_id, COOP_CLOSED)
10498+
convert_channel_err!(self, peer_state, shutdown_res, funded_channel, COOP_CLOSED)
1050410499
} else {
1050510500
debug_assert!(false);
1050610501
let reason = shutdown_res.closure_reason.clone();
1050710502
let err = ChannelError::Close((reason.to_string(), reason));
10508-
convert_channel_err!(self, peer_state, err, chan, &chan_id, UNFUNDED_CHANNEL)
10503+
convert_channel_err!(self, peer_state, err, chan, UNFUNDED_CHANNEL)
1050910504
};
1051010505
debug_assert!(remove);
1051110506
shutdown_results.push((Err(err), *cp_id));
@@ -10535,7 +10530,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1053510530
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1053610531
let peer_state = &mut *peer_state_lock;
1053710532
let pending_msg_events = &mut peer_state.pending_msg_events;
10538-
peer_state.channel_by_id.retain(|channel_id, chan| {
10533+
peer_state.channel_by_id.retain(|_, chan| {
1053910534
match chan.as_funded_mut() {
1054010535
Some(funded_chan) => {
1054110536
let logger = WithChannelContext::from(&self.logger, &funded_chan.context, None);
@@ -10551,7 +10546,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1055110546
if let Some((tx, shutdown_res)) = tx_shutdown_result_opt {
1055210547
// We're done with this channel. We got a closing_signed and sent back
1055310548
// a closing_signed with a closing transaction to broadcast.
10554-
let (_, err) = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, channel_id, COOP_CLOSED);
10549+
let (_, err) = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, COOP_CLOSED);
1055510550
handle_errors.push((*cp_id, Err(err)));
1055610551

1055710552
log_info!(logger, "Broadcasting {}", log_tx!(tx));
@@ -10561,7 +10556,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1056110556
},
1056210557
Err(e) => {
1056310558
has_update = true;
10564-
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, channel_id, FUNDED_CHANNEL);
10559+
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, FUNDED_CHANNEL);
1056510560
handle_errors.push((funded_chan.context.get_counterparty_node_id(), Err(res)));
1056610561
!close_channel
1056710562
}
@@ -11815,15 +11810,15 @@ where
1181511810
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1181611811
let peer_state = &mut *peer_state_lock;
1181711812
let pending_msg_events = &mut peer_state.pending_msg_events;
11818-
peer_state.channel_by_id.retain(|chan_id, chan| {
11813+
peer_state.channel_by_id.retain(|_, chan| {
1181911814
let logger = WithChannelContext::from(&self.logger, &chan.context(), None);
1182011815
if chan.peer_disconnected_is_resumable(&&logger) {
1182111816
return true;
1182211817
}
1182311818
// Clean up for removal.
1182411819
let reason = ClosureReason::DisconnectedPeer;
1182511820
let err = ChannelError::Close((reason.to_string(), reason));
11826-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
11821+
let (_, e) = convert_channel_err!(self, peer_state, err, chan);
1182711822
failed_channels.push((Err(e), counterparty_node_id));
1182811823
false
1182911824
});
@@ -12376,7 +12371,7 @@ where
1237612371
let peer_state = &mut *peer_state_lock;
1237712372
let pending_msg_events = &mut peer_state.pending_msg_events;
1237812373

12379-
peer_state.channel_by_id.retain(|chan_id, chan| {
12374+
peer_state.channel_by_id.retain(|channel_id, chan| {
1238012375
match chan.as_funded_mut() {
1238112376
// Retain unfunded channels.
1238212377
None => true,
@@ -12387,22 +12382,22 @@ where
1238712382
let reason = LocalHTLCFailureReason::CLTVExpiryTooSoon;
1238812383
let data = self.get_htlc_inbound_temp_fail_data(reason);
1238912384
timed_out_htlcs.push((source, payment_hash, HTLCFailReason::reason(reason, data),
12390-
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: funded_channel.context.channel_id() }));
12385+
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: *channel_id }));
1239112386
}
1239212387
let logger = WithChannelContext::from(&self.logger, &funded_channel.context, None);
1239312388
match funding_confirmed_opt {
1239412389
Some(FundingConfirmedMessage::Establishment(channel_ready)) => {
1239512390
send_channel_ready!(self, pending_msg_events, funded_channel, channel_ready);
1239612391
if funded_channel.context.is_usable() {
12397-
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", funded_channel.context.channel_id());
12392+
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", channel_id);
1239812393
if let Ok(msg) = self.get_channel_update_for_unicast(funded_channel) {
1239912394
pending_msg_events.push(MessageSendEvent::SendChannelUpdate {
1240012395
node_id: funded_channel.context.get_counterparty_node_id(),
1240112396
msg,
1240212397
});
1240312398
}
1240412399
} else {
12405-
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", funded_channel.context.channel_id());
12400+
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", channel_id);
1240612401
}
1240712402
},
1240812403
#[cfg(splicing)]
@@ -12413,7 +12408,7 @@ where
1241312408

1241412409
let mut pending_events = self.pending_events.lock().unwrap();
1241512410
pending_events.push_back((events::Event::ChannelReady {
12416-
channel_id: funded_channel.context.channel_id(),
12411+
channel_id,
1241712412
user_channel_id: funded_channel.context.get_user_id(),
1241812413
counterparty_node_id: funded_channel.context.get_counterparty_node_id(),
1241912414
funding_txo: funding_txo.map(|outpoint| outpoint.into_bitcoin_outpoint()),
@@ -12485,8 +12480,8 @@ where
1248512480
// un-confirmed we force-close the channel, ensuring short_to_chan_info
1248612481
// is always consistent.
1248712482
let mut short_to_chan_info = self.short_to_chan_info.write().unwrap();
12488-
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()));
12489-
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()),
12483+
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), *channel_id));
12484+
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), *channel_id),
1249012485
"SCIDs should never collide - ensure you weren't behind by a full {} blocks when creating channels",
1249112486
fake_scid::MAX_SCID_BLOCKS_FROM_NOW);
1249212487
}
@@ -12500,7 +12495,6 @@ where
1250012495
peer_state,
1250112496
err,
1250212497
funded_channel,
12503-
chan_id,
1250412498
FUNDED_CHANNEL
1250512499
);
1250612500
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)