Skip to content

Commit 42e8844

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 fa61005 commit 42e8844

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
@@ -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,8 @@ macro_rules! break_channel_entry {
32933296
match $res {
32943297
Ok(res) => res,
32953298
Err(e) => {
3296-
let key = *$entry.key();
32973299
let (drop, res) =
3298-
convert_channel_err!($self, $peer_state, e, $entry.get_mut(), &key);
3300+
convert_channel_err!($self, $peer_state, e, $entry.get_mut());
32993301
if drop {
33003302
$entry.remove_entry();
33013303
}
@@ -3310,9 +3312,8 @@ macro_rules! try_channel_entry {
33103312
match $res {
33113313
Ok(res) => res,
33123314
Err(e) => {
3313-
let key = *$entry.key();
33143315
let (drop, res) =
3315-
convert_channel_err!($self, $peer_state, e, $entry.get_mut(), &key);
3316+
convert_channel_err!($self, $peer_state, e, $entry.get_mut());
33163317
if drop {
33173318
$entry.remove_entry();
33183319
}
@@ -4154,7 +4155,7 @@ where
41544155
let reason = ClosureReason::LocallyCoopClosedUnfundedChannel;
41554156
let err = ChannelError::Close((reason.to_string(), reason));
41564157
let mut chan = chan_entry.remove();
4157-
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, chan_id);
4158+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
41584159
e.dont_send_error_message();
41594160
shutdown_result = Err(e);
41604161
}
@@ -4351,7 +4352,7 @@ where
43514352
if let Some(mut chan) = peer_state.channel_by_id.remove(&channel_id) {
43524353
let reason = ClosureReason::FundingBatchClosure;
43534354
let err = ChannelError::Close((reason.to_string(), reason));
4354-
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
4355+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
43554356
shutdown_results.push((Err(e), counterparty_node_id));
43564357
}
43574358
}
@@ -4415,7 +4416,7 @@ where
44154416
if let Some(mut chan) = peer_state.channel_by_id.remove(channel_id) {
44164417
log_error!(logger, "Force-closing channel {}", channel_id);
44174418
let err = ChannelError::Close((message, reason));
4418-
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, channel_id);
4419+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
44194420
mem::drop(peer_state_lock);
44204421
mem::drop(per_peer_state);
44214422
if is_from_counterparty {
@@ -5894,7 +5895,7 @@ where
58945895
let reason = ClosureReason::ProcessingError { err: e.clone() };
58955896
let err = ChannelError::Close((e.clone(), reason));
58965897
let (_, e) =
5897-
convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
5898+
convert_channel_err!(self, peer_state, err, &mut chan);
58985899
shutdown_results.push((Err(e), counterparty_node_id));
58995900
});
59005901
}
@@ -7062,7 +7063,7 @@ where
70627063
if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
70637064

70647065
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);
7066+
let (needs_close, err) = convert_channel_err!(self, peer_state, e, funded_chan, FUNDED_CHANNEL);
70667067
handle_errors.push((Err(err), counterparty_node_id));
70677068
if needs_close { return false; }
70687069
}
@@ -7140,7 +7141,7 @@ where
71407141
let reason = ClosureReason::FundingTimedOut;
71417142
let msg = "Force-closing pending channel due to timeout awaiting establishment handshake".to_owned();
71427143
let err = ChannelError::Close((msg, reason));
7143-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
7144+
let (_, e) = convert_channel_err!(self, peer_state, err, chan);
71447145
handle_errors.push((Err(e), counterparty_node_id));
71457146
false
71467147
} else {
@@ -8726,18 +8727,15 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
87268727
// above so at this point we just need to clean up any lingering entries
87278728
// concerning this channel as it is safe to do so.
87288729
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.
87328730
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);
8731+
return Err(convert_channel_err!(self, peer_state, err, &mut chan).1);
87348732
},
87358733
}
87368734
},
87378735
Some(Err(mut chan)) => {
87388736
let err_msg = format!("Got an unexpected funding_created message from peer with counterparty_node_id {}", counterparty_node_id);
87398737
let err = ChannelError::close(err_msg);
8740-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id).1);
8738+
return Err(convert_channel_err!(self, peer_state, err, &mut chan).1);
87418739
},
87428740
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))
87438741
};
@@ -8753,7 +8751,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
87538751
let err = ChannelError::close($err.to_owned());
87548752
chan.unset_funding_info();
87558753
let mut chan = Channel::from(chan);
8756-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &funded_channel_id, UNFUNDED_CHANNEL).1);
8754+
return Err(convert_channel_err!(self, peer_state, err, &mut chan, UNFUNDED_CHANNEL).1);
87578755
} } }
87588756

87598757
match peer_state.channel_by_id.entry(funded_channel_id) {
@@ -9294,7 +9292,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
92949292
let err = ChannelError::Close((reason.to_string(), reason));
92959293
let mut chan = chan_entry.remove();
92969294
let (_, mut e) =
9297-
convert_channel_err!(self, peer_state, err, &mut chan, &msg.channel_id);
9295+
convert_channel_err!(self, peer_state, err, &mut chan);
92989296
e.dont_send_error_message();
92999297
return Err(e);
93009298
},
@@ -9353,7 +9351,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
93539351
// also implies there are no pending HTLCs left on the channel, so we can
93549352
// fully delete it from tracking (the channel monitor is still around to
93559353
// watch for old state broadcasts)!
9356-
let (_, err) = convert_channel_err!(self, peer_state, close_res, chan, &msg.channel_id, COOP_CLOSED);
9354+
let (_, err) = convert_channel_err!(self, peer_state, close_res, chan, COOP_CLOSED);
93579355
chan_entry.remove();
93589356
Some((tx, Err(err)))
93599357
} else {
@@ -10316,7 +10314,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1031610314
};
1031710315
let err = ChannelError::Close((reason.to_string(), reason));
1031810316
let mut chan = chan_entry.remove();
10319-
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
10317+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
1032010318
failed_channels.push((Err(e), counterparty_node_id));
1032110319
}
1032210320
}
@@ -10504,12 +10502,12 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1050410502
let chan_id = context.channel_id();
1050510503
log_trace!(logger, "Removing channel {} now that the signer is unblocked", chan_id);
1050610504
let (remove, err) = if let Some(funded_channel) = chan.as_funded_mut() {
10507-
convert_channel_err!(self, peer_state, shutdown_res, funded_channel, &chan_id, COOP_CLOSED)
10505+
convert_channel_err!(self, peer_state, shutdown_res, funded_channel, COOP_CLOSED)
1050810506
} else {
1050910507
debug_assert!(false);
1051010508
let reason = shutdown_res.closure_reason.clone();
1051110509
let err = ChannelError::Close((reason.to_string(), reason));
10512-
convert_channel_err!(self, peer_state, err, chan, &chan_id, UNFUNDED_CHANNEL)
10510+
convert_channel_err!(self, peer_state, err, chan, UNFUNDED_CHANNEL)
1051310511
};
1051410512
debug_assert!(remove);
1051510513
shutdown_results.push((Err(err), *cp_id));
@@ -10539,7 +10537,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1053910537
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1054010538
let peer_state = &mut *peer_state_lock;
1054110539
let pending_msg_events = &mut peer_state.pending_msg_events;
10542-
peer_state.channel_by_id.retain(|channel_id, chan| {
10540+
peer_state.channel_by_id.retain(|_, chan| {
1054310541
match chan.as_funded_mut() {
1054410542
Some(funded_chan) => {
1054510543
let logger = WithChannelContext::from(&self.logger, &funded_chan.context, None);
@@ -10555,7 +10553,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1055510553
if let Some((tx, shutdown_res)) = tx_shutdown_result_opt {
1055610554
// We're done with this channel. We got a closing_signed and sent back
1055710555
// a closing_signed with a closing transaction to broadcast.
10558-
let (_, err) = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, channel_id, COOP_CLOSED);
10556+
let (_, err) = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, COOP_CLOSED);
1055910557
handle_errors.push((*cp_id, Err(err)));
1056010558

1056110559
log_info!(logger, "Broadcasting {}", log_tx!(tx));
@@ -10565,7 +10563,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1056510563
},
1056610564
Err(e) => {
1056710565
has_update = true;
10568-
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, channel_id, FUNDED_CHANNEL);
10566+
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, FUNDED_CHANNEL);
1056910567
handle_errors.push((funded_chan.context.get_counterparty_node_id(), Err(res)));
1057010568
!close_channel
1057110569
}
@@ -11819,15 +11817,15 @@ where
1181911817
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1182011818
let peer_state = &mut *peer_state_lock;
1182111819
let pending_msg_events = &mut peer_state.pending_msg_events;
11822-
peer_state.channel_by_id.retain(|chan_id, chan| {
11820+
peer_state.channel_by_id.retain(|_, chan| {
1182311821
let logger = WithChannelContext::from(&self.logger, &chan.context(), None);
1182411822
if chan.peer_disconnected_is_resumable(&&logger) {
1182511823
return true;
1182611824
}
1182711825
// Clean up for removal.
1182811826
let reason = ClosureReason::DisconnectedPeer;
1182911827
let err = ChannelError::Close((reason.to_string(), reason));
11830-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
11828+
let (_, e) = convert_channel_err!(self, peer_state, err, chan);
1183111829
failed_channels.push((Err(e), counterparty_node_id));
1183211830
false
1183311831
});
@@ -12380,7 +12378,7 @@ where
1238012378
let peer_state = &mut *peer_state_lock;
1238112379
let pending_msg_events = &mut peer_state.pending_msg_events;
1238212380

12383-
peer_state.channel_by_id.retain(|chan_id, chan| {
12381+
peer_state.channel_by_id.retain(|channel_id, chan| {
1238412382
match chan.as_funded_mut() {
1238512383
// Retain unfunded channels.
1238612384
None => true,
@@ -12391,22 +12389,22 @@ where
1239112389
let reason = LocalHTLCFailureReason::CLTVExpiryTooSoon;
1239212390
let data = self.get_htlc_inbound_temp_fail_data(reason);
1239312391
timed_out_htlcs.push((source, payment_hash, HTLCFailReason::reason(reason, data),
12394-
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: funded_channel.context.channel_id() }));
12392+
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: *channel_id }));
1239512393
}
1239612394
let logger = WithChannelContext::from(&self.logger, &funded_channel.context, None);
1239712395
match funding_confirmed_opt {
1239812396
Some(FundingConfirmedMessage::Establishment(channel_ready)) => {
1239912397
send_channel_ready!(self, pending_msg_events, funded_channel, channel_ready);
1240012398
if funded_channel.context.is_usable() {
12401-
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", funded_channel.context.channel_id());
12399+
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", channel_id);
1240212400
if let Ok(msg) = self.get_channel_update_for_unicast(funded_channel) {
1240312401
pending_msg_events.push(MessageSendEvent::SendChannelUpdate {
1240412402
node_id: funded_channel.context.get_counterparty_node_id(),
1240512403
msg,
1240612404
});
1240712405
}
1240812406
} else {
12409-
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", funded_channel.context.channel_id());
12407+
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", channel_id);
1241012408
}
1241112409
},
1241212410
#[cfg(splicing)]
@@ -12417,7 +12415,7 @@ where
1241712415

1241812416
let mut pending_events = self.pending_events.lock().unwrap();
1241912417
pending_events.push_back((events::Event::ChannelReady {
12420-
channel_id: funded_channel.context.channel_id(),
12418+
channel_id,
1242112419
user_channel_id: funded_channel.context.get_user_id(),
1242212420
counterparty_node_id: funded_channel.context.get_counterparty_node_id(),
1242312421
funding_txo: funding_txo.map(|outpoint| outpoint.into_bitcoin_outpoint()),
@@ -12489,8 +12487,8 @@ where
1248912487
// un-confirmed we force-close the channel, ensuring short_to_chan_info
1249012488
// is always consistent.
1249112489
let mut short_to_chan_info = self.short_to_chan_info.write().unwrap();
12492-
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()));
12493-
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()),
12490+
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), *channel_id));
12491+
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), *channel_id),
1249412492
"SCIDs should never collide - ensure you weren't behind by a full {} blocks when creating channels",
1249512493
fake_scid::MAX_SCID_BLOCKS_FROM_NOW);
1249612494
}
@@ -12504,7 +12502,6 @@ where
1250412502
peer_state,
1250512503
err,
1250612504
funded_channel,
12507-
chan_id,
1250812505
FUNDED_CHANNEL
1250912506
);
1251012507
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)