Skip to content

Commit 05e8010

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 5ec54dc commit 05e8010

File tree

2 files changed

+49
-60
lines changed

2 files changed

+49
-60
lines changed

‎lightning/src/ln/channelmanager.rs

Lines changed: 48 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -3220,32 +3220,33 @@ macro_rules! locked_close_channel {
32203220
/// Returns (boolean indicating if we should remove the Channel object from memory, a mapped error)
32213221
#[rustfmt::skip]
32223222
macro_rules! convert_channel_err {
3223-
($self: ident, $peer_state: expr, $err: expr, $chan: expr, $close: expr, $locked_close: expr, $channel_id: expr, _internal) => {
3223+
($self: ident, $peer_state: expr, $err: expr, $chan: expr, $close: expr, $locked_close: expr, $channel_id: expr, _internal) => { {
32243224
match $err {
32253225
ChannelError::Warn(msg) => {
3226-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), *$channel_id))
3226+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), $channel_id))
32273227
},
32283228
ChannelError::WarnAndDisconnect(msg) => {
3229-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::WarnAndDisconnect(msg), *$channel_id))
3229+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::WarnAndDisconnect(msg), $channel_id))
32303230
},
32313231
ChannelError::Ignore(msg) => {
3232-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), *$channel_id))
3232+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), $channel_id))
32333233
},
32343234
ChannelError::Close((msg, reason)) => {
32353235
let (mut shutdown_res, chan_update) = $close(reason);
32363236
let logger = WithChannelContext::from(&$self.logger, &$chan.context(), None);
32373237
log_error!(logger, "Closed channel {} due to close-required error: {}", $channel_id, msg);
32383238
$locked_close(&mut shutdown_res, $chan);
32393239
let err =
3240-
MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, chan_update);
3240+
MsgHandleErrInternal::from_finish_shutdown(msg, $channel_id, shutdown_res, chan_update);
32413241
(true, err)
32423242
},
32433243
ChannelError::SendError(msg) => {
3244-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::SendError(msg), *$channel_id))
3244+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::SendError(msg), $channel_id))
32453245
},
32463246
}
3247-
};
3248-
($self: ident, $peer_state: expr, $shutdown_result: expr, $funded_channel: expr, $channel_id: expr, COOP_CLOSED) => { {
3247+
} };
3248+
($self: ident, $peer_state: expr, $shutdown_result: expr, $funded_channel: expr, COOP_CLOSED) => { {
3249+
let chan_id = $funded_channel.context.channel_id();
32493250
let reason = ChannelError::Close(("Coop Closed".to_owned(), $shutdown_result.closure_reason.clone()));
32503251
let do_close = |_| {
32513252
(
@@ -3257,12 +3258,13 @@ macro_rules! convert_channel_err {
32573258
locked_close_channel!($self, $peer_state, funded_channel, shutdown_res_mut, FUNDED);
32583259
};
32593260
let (close, mut err) =
3260-
convert_channel_err!($self, $peer_state, reason, $funded_channel, do_close, locked_close, $channel_id, _internal);
3261+
convert_channel_err!($self, $peer_state, reason, $funded_channel, do_close, locked_close, chan_id, _internal);
32613262
err.dont_send_error_message();
32623263
debug_assert!(close);
32633264
err
32643265
} };
3265-
($self: ident, $peer_state: expr, $err: expr, $funded_channel: expr, $channel_id: expr, FUNDED_CHANNEL) => { {
3266+
($self: ident, $peer_state: expr, $err: expr, $funded_channel: expr, FUNDED_CHANNEL) => { {
3267+
let chan_id = $funded_channel.context.channel_id();
32663268
let mut do_close = |reason| {
32673269
(
32683270
$funded_channel.force_shutdown(reason),
@@ -3272,20 +3274,21 @@ macro_rules! convert_channel_err {
32723274
let mut locked_close = |shutdown_res_mut: &mut ShutdownResult, funded_channel: &mut FundedChannel<_>| {
32733275
locked_close_channel!($self, $peer_state, funded_channel, shutdown_res_mut, FUNDED);
32743276
};
3275-
convert_channel_err!($self, $peer_state, $err, $funded_channel, do_close, locked_close, $channel_id, _internal)
3277+
convert_channel_err!($self, $peer_state, $err, $funded_channel, do_close, locked_close, chan_id, _internal)
32763278
} };
3277-
($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr, UNFUNDED_CHANNEL) => { {
3279+
($self: ident, $peer_state: expr, $err: expr, $channel: expr, UNFUNDED_CHANNEL) => { {
3280+
let chan_id = $channel.context().channel_id();
32783281
let mut do_close = |reason| { ($channel.force_shutdown(reason), None) };
32793282
let locked_close = |_, chan: &mut Channel<_>| { locked_close_channel!($self, chan.context(), UNFUNDED); };
3280-
convert_channel_err!($self, $peer_state, $err, $channel, do_close, locked_close, $channel_id, _internal)
3283+
convert_channel_err!($self, $peer_state, $err, $channel, do_close, locked_close, chan_id, _internal)
32813284
} };
3282-
($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr) => {
3285+
($self: ident, $peer_state: expr, $err: expr, $channel: expr) => {
32833286
match $channel.as_funded_mut() {
32843287
Some(funded_channel) => {
3285-
convert_channel_err!($self, $peer_state, $err, funded_channel, $channel_id, FUNDED_CHANNEL)
3288+
convert_channel_err!($self, $peer_state, $err, funded_channel, FUNDED_CHANNEL)
32863289
},
32873290
None => {
3288-
convert_channel_err!($self, $peer_state, $err, $channel, $channel_id, UNFUNDED_CHANNEL)
3291+
convert_channel_err!($self, $peer_state, $err, $channel, UNFUNDED_CHANNEL)
32893292
},
32903293
}
32913294
};
@@ -3296,9 +3299,7 @@ macro_rules! break_channel_entry {
32963299
match $res {
32973300
Ok(res) => res,
32983301
Err(e) => {
3299-
let key = *$entry.key();
3300-
let (drop, res) =
3301-
convert_channel_err!($self, $peer_state, e, $entry.get_mut(), &key);
3302+
let (drop, res) = convert_channel_err!($self, $peer_state, e, $entry.get_mut());
33023303
if drop {
33033304
$entry.remove_entry();
33043305
}
@@ -3313,9 +3314,7 @@ macro_rules! try_channel_entry {
33133314
match $res {
33143315
Ok(res) => res,
33153316
Err(e) => {
3316-
let key = *$entry.key();
3317-
let (drop, res) =
3318-
convert_channel_err!($self, $peer_state, e, $entry.get_mut(), &key);
3317+
let (drop, res) = convert_channel_err!($self, $peer_state, e, $entry.get_mut());
33193318
if drop {
33203319
$entry.remove_entry();
33213320
}
@@ -4157,7 +4156,7 @@ where
41574156
let reason = ClosureReason::LocallyCoopClosedUnfundedChannel;
41584157
let err = ChannelError::Close((reason.to_string(), reason));
41594158
let mut chan = chan_entry.remove();
4160-
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, chan_id);
4159+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
41614160
e.dont_send_error_message();
41624161
shutdown_result = Err(e);
41634162
}
@@ -4354,7 +4353,7 @@ where
43544353
if let Some(mut chan) = peer_state.channel_by_id.remove(&channel_id) {
43554354
let reason = ClosureReason::FundingBatchClosure;
43564355
let err = ChannelError::Close((reason.to_string(), reason));
4357-
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
4356+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
43584357
shutdown_results.push((Err(e), counterparty_node_id));
43594358
}
43604359
}
@@ -4418,7 +4417,7 @@ where
44184417
if let Some(mut chan) = peer_state.channel_by_id.remove(channel_id) {
44194418
log_error!(logger, "Force-closing channel {}", channel_id);
44204419
let err = ChannelError::Close((message, reason));
4421-
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, channel_id);
4420+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
44224421
mem::drop(peer_state_lock);
44234422
mem::drop(per_peer_state);
44244423
if is_from_counterparty {
@@ -5897,7 +5896,7 @@ where
58975896
let reason = ClosureReason::ProcessingError { err: e.clone() };
58985897
let err = ChannelError::Close((e.clone(), reason));
58995898
let (_, e) =
5900-
convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
5899+
convert_channel_err!(self, peer_state, err, &mut chan);
59015900
shutdown_results.push((Err(e), counterparty_node_id));
59025901
});
59035902
}
@@ -7302,7 +7301,7 @@ where
73027301
if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
73037302

73047303
if let Err(e) = funded_chan.timer_check_closing_negotiation_progress() {
7305-
let (needs_close, err) = convert_channel_err!(self, peer_state, e, funded_chan, chan_id, FUNDED_CHANNEL);
7304+
let (needs_close, err) = convert_channel_err!(self, peer_state, e, funded_chan, FUNDED_CHANNEL);
73067305
handle_errors.push((Err(err), counterparty_node_id));
73077306
if needs_close { return false; }
73087307
}
@@ -7380,7 +7379,7 @@ where
73807379
let reason = ClosureReason::FundingTimedOut;
73817380
let msg = "Force-closing pending channel due to timeout awaiting establishment handshake".to_owned();
73827381
let err = ChannelError::Close((msg, reason));
7383-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
7382+
let (_, e) = convert_channel_err!(self, peer_state, err, chan);
73847383
handle_errors.push((Err(e), counterparty_node_id));
73857384
false
73867385
} else {
@@ -9140,18 +9139,15 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
91409139
// above so at this point we just need to clean up any lingering entries
91419140
// concerning this channel as it is safe to do so.
91429141
debug_assert!(matches!(err, ChannelError::Close(_)));
9143-
// Really we should be returning the channel_id the peer expects based
9144-
// on their funding info here, but they're horribly confused anyway, so
9145-
// there's not a lot we can do to save them.
91469142
let mut chan = Channel::from(inbound_chan);
9147-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id, UNFUNDED_CHANNEL).1);
9143+
return Err(convert_channel_err!(self, peer_state, err, &mut chan).1);
91489144
},
91499145
}
91509146
},
91519147
Some(Err(mut chan)) => {
91529148
let err_msg = format!("Got an unexpected funding_created message from peer with counterparty_node_id {}", counterparty_node_id);
91539149
let err = ChannelError::close(err_msg);
9154-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id).1);
9150+
return Err(convert_channel_err!(self, peer_state, err, &mut chan).1);
91559151
},
91569152
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))
91579153
};
@@ -9167,7 +9163,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
91679163
let err = ChannelError::close($err.to_owned());
91689164
chan.unset_funding_info();
91699165
let mut chan = Channel::from(chan);
9170-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &funded_channel_id, UNFUNDED_CHANNEL).1);
9166+
return Err(convert_channel_err!(self, peer_state, err, &mut chan, UNFUNDED_CHANNEL).1);
91719167
} } }
91729168

91739169
match peer_state.channel_by_id.entry(funded_channel_id) {
@@ -9707,8 +9703,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
97079703
let reason = ClosureReason::CounterpartyCoopClosedUnfundedChannel;
97089704
let err = ChannelError::Close((reason.to_string(), reason));
97099705
let mut chan = chan_entry.remove();
9710-
let (_, mut e) =
9711-
convert_channel_err!(self, peer_state, err, &mut chan, &msg.channel_id);
9706+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
97129707
e.dont_send_error_message();
97139708
return Err(e);
97149709
},
@@ -9767,7 +9762,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
97679762
// also implies there are no pending HTLCs left on the channel, so we can
97689763
// fully delete it from tracking (the channel monitor is still around to
97699764
// watch for old state broadcasts)!
9770-
let err = convert_channel_err!(self, peer_state, close_res, chan, &msg.channel_id, COOP_CLOSED);
9765+
let err = convert_channel_err!(self, peer_state, close_res, chan, COOP_CLOSED);
97719766
chan_entry.remove();
97729767
Some((tx, Err(err)))
97739768
} else {
@@ -10848,13 +10843,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1084810843
};
1084910844
let err = ChannelError::Close((reason.to_string(), reason));
1085010845
let mut chan = chan_entry.remove();
10851-
let (_, e) = convert_channel_err!(
10852-
self,
10853-
peer_state,
10854-
err,
10855-
&mut chan,
10856-
&channel_id
10857-
);
10846+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
1085810847
failed_channels.push((Err(e), counterparty_node_id));
1085910848
}
1086010849
}
@@ -11059,13 +11048,14 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1105911048
let chan_id = context.channel_id();
1106011049
log_trace!(logger, "Removing channel {} now that the signer is unblocked", chan_id);
1106111050
let (remove, err) = if let Some(funded) = chan.as_funded_mut() {
11062-
let err = convert_channel_err!(self, peer_state, shutdown, funded, &chan_id, COOP_CLOSED);
11051+
let err =
11052+
convert_channel_err!(self, peer_state, shutdown, funded, COOP_CLOSED);
1106311053
(true, err)
1106411054
} else {
1106511055
debug_assert!(false);
1106611056
let reason = shutdown.closure_reason.clone();
1106711057
let err = ChannelError::Close((reason.to_string(), reason));
11068-
convert_channel_err!(self, peer_state, err, chan, &chan_id, UNFUNDED_CHANNEL)
11058+
convert_channel_err!(self, peer_state, err, chan, UNFUNDED_CHANNEL)
1106911059
};
1107011060
debug_assert!(remove);
1107111061
shutdown_results.push((Err(err), *cp_id));
@@ -11095,7 +11085,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1109511085
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1109611086
let peer_state = &mut *peer_state_lock;
1109711087
let pending_msg_events = &mut peer_state.pending_msg_events;
11098-
peer_state.channel_by_id.retain(|channel_id, chan| {
11088+
peer_state.channel_by_id.retain(|_, chan| {
1109911089
match chan.as_funded_mut() {
1110011090
Some(funded_chan) => {
1110111091
let logger = WithChannelContext::from(&self.logger, &funded_chan.context, None);
@@ -11111,7 +11101,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1111111101
if let Some((tx, shutdown_res)) = tx_shutdown_result_opt {
1111211102
// We're done with this channel. We got a closing_signed and sent back
1111311103
// a closing_signed with a closing transaction to broadcast.
11114-
let err = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, channel_id, COOP_CLOSED);
11104+
let err = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, COOP_CLOSED);
1111511105
handle_errors.push((*cp_id, Err(err)));
1111611106

1111711107
log_info!(logger, "Broadcasting {}", log_tx!(tx));
@@ -11121,7 +11111,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1112111111
},
1112211112
Err(e) => {
1112311113
has_update = true;
11124-
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, channel_id, FUNDED_CHANNEL);
11114+
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, FUNDED_CHANNEL);
1112511115
handle_errors.push((funded_chan.context.get_counterparty_node_id(), Err(res)));
1112611116
!close_channel
1112711117
}
@@ -12375,15 +12365,15 @@ where
1237512365
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1237612366
let peer_state = &mut *peer_state_lock;
1237712367
let pending_msg_events = &mut peer_state.pending_msg_events;
12378-
peer_state.channel_by_id.retain(|chan_id, chan| {
12368+
peer_state.channel_by_id.retain(|_, chan| {
1237912369
let logger = WithChannelContext::from(&self.logger, &chan.context(), None);
1238012370
if chan.peer_disconnected_is_resumable(&&logger) {
1238112371
return true;
1238212372
}
1238312373
// Clean up for removal.
1238412374
let reason = ClosureReason::DisconnectedPeer;
1238512375
let err = ChannelError::Close((reason.to_string(), reason));
12386-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
12376+
let (_, e) = convert_channel_err!(self, peer_state, err, chan);
1238712377
failed_channels.push((Err(e), counterparty_node_id));
1238812378
false
1238912379
});
@@ -12936,7 +12926,7 @@ where
1293612926
let peer_state = &mut *peer_state_lock;
1293712927
let pending_msg_events = &mut peer_state.pending_msg_events;
1293812928

12939-
peer_state.channel_by_id.retain(|chan_id, chan| {
12929+
peer_state.channel_by_id.retain(|channel_id, chan| {
1294012930
match chan.as_funded_mut() {
1294112931
// Retain unfunded channels.
1294212932
None => true,
@@ -12947,22 +12937,22 @@ where
1294712937
let reason = LocalHTLCFailureReason::CLTVExpiryTooSoon;
1294812938
let data = self.get_htlc_inbound_temp_fail_data(reason);
1294912939
timed_out_htlcs.push((source, payment_hash, HTLCFailReason::reason(reason, data),
12950-
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: funded_channel.context.channel_id() }));
12940+
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: *channel_id }));
1295112941
}
1295212942
let logger = WithChannelContext::from(&self.logger, &funded_channel.context, None);
1295312943
match funding_confirmed_opt {
1295412944
Some(FundingConfirmedMessage::Establishment(channel_ready)) => {
1295512945
send_channel_ready!(self, pending_msg_events, funded_channel, channel_ready);
1295612946
if funded_channel.context.is_usable() {
12957-
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", funded_channel.context.channel_id());
12947+
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", channel_id);
1295812948
if let Ok(msg) = self.get_channel_update_for_unicast(funded_channel) {
1295912949
pending_msg_events.push(MessageSendEvent::SendChannelUpdate {
1296012950
node_id: funded_channel.context.get_counterparty_node_id(),
1296112951
msg,
1296212952
});
1296312953
}
1296412954
} else {
12965-
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", funded_channel.context.channel_id());
12955+
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", channel_id);
1296612956
}
1296712957
},
1296812958
#[cfg(splicing)]
@@ -12973,7 +12963,7 @@ where
1297312963

1297412964
let mut pending_events = self.pending_events.lock().unwrap();
1297512965
pending_events.push_back((events::Event::ChannelReady {
12976-
channel_id: funded_channel.context.channel_id(),
12966+
channel_id: *channel_id,
1297712967
user_channel_id: funded_channel.context.get_user_id(),
1297812968
counterparty_node_id: funded_channel.context.get_counterparty_node_id(),
1297912969
funding_txo: funding_txo.map(|outpoint| outpoint.into_bitcoin_outpoint()),
@@ -13045,8 +13035,8 @@ where
1304513035
// un-confirmed we force-close the channel, ensuring short_to_chan_info
1304613036
// is always consistent.
1304713037
let mut short_to_chan_info = self.short_to_chan_info.write().unwrap();
13048-
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()));
13049-
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()),
13038+
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), *channel_id));
13039+
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), *channel_id),
1305013040
"SCIDs should never collide - ensure you weren't behind by a full {} blocks when creating channels",
1305113041
fake_scid::MAX_SCID_BLOCKS_FROM_NOW);
1305213042
}
@@ -13060,7 +13050,6 @@ where
1306013050
peer_state,
1306113051
err,
1306213052
funded_channel,
13063-
chan_id,
1306413053
FUNDED_CHANNEL
1306513054
);
1306613055
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
@@ -9041,7 +9041,7 @@ pub fn test_duplicate_chan_id() {
90419041
} => {
90429042
// Technically, at this point, nodes[1] would be justified in thinking both
90439043
// channels are closed, but currently we do not, so we just move forward with it.
9044-
assert_eq!(msg.channel_id, channel_id);
9044+
assert_eq!(msg.channel_id, funding_created.temporary_channel_id);
90459045
assert_eq!(node_id, node_a_id);
90469046
},
90479047
_ => panic!("Unexpected event"),

0 commit comments

Comments
 (0)