Skip to content

Commit e1847f0

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 3c25995 commit e1847f0

File tree

2 files changed

+48
-60
lines changed

2 files changed

+48
-60
lines changed

‎lightning/src/ln/channelmanager.rs‎

Lines changed: 47 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
(close, 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,12 +11048,12 @@ 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_channel) = chan.as_funded_mut() {
11062-
convert_channel_err!(self, peer_state, shutdown_res, funded_channel, &chan_id, COOP_CLOSED)
11051+
convert_channel_err!(self, peer_state, shutdown_res, funded_channel, COOP_CLOSED)
1106311052
} else {
1106411053
debug_assert!(false);
1106511054
let reason = shutdown_res.closure_reason.clone();
1106611055
let err = ChannelError::Close((reason.to_string(), reason));
11067-
convert_channel_err!(self, peer_state, err, chan, &chan_id, UNFUNDED_CHANNEL)
11056+
convert_channel_err!(self, peer_state, err, chan, UNFUNDED_CHANNEL)
1106811057
};
1106911058
debug_assert!(remove);
1107011059
shutdown_results.push((Err(err), *cp_id));
@@ -11094,7 +11083,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1109411083
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1109511084
let peer_state = &mut *peer_state_lock;
1109611085
let pending_msg_events = &mut peer_state.pending_msg_events;
11097-
peer_state.channel_by_id.retain(|channel_id, chan| {
11086+
peer_state.channel_by_id.retain(|_, chan| {
1109811087
match chan.as_funded_mut() {
1109911088
Some(funded_chan) => {
1110011089
let logger = WithChannelContext::from(&self.logger, &funded_chan.context, None);
@@ -11110,7 +11099,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1111011099
if let Some((tx, shutdown_res)) = tx_shutdown_result_opt {
1111111100
// We're done with this channel. We got a closing_signed and sent back
1111211101
// a closing_signed with a closing transaction to broadcast.
11113-
let (_, err) = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, channel_id, COOP_CLOSED);
11102+
let (_, err) = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, COOP_CLOSED);
1111411103
handle_errors.push((*cp_id, Err(err)));
1111511104

1111611105
log_info!(logger, "Broadcasting {}", log_tx!(tx));
@@ -11120,7 +11109,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1112011109
},
1112111110
Err(e) => {
1112211111
has_update = true;
11123-
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, channel_id, FUNDED_CHANNEL);
11112+
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, FUNDED_CHANNEL);
1112411113
handle_errors.push((funded_chan.context.get_counterparty_node_id(), Err(res)));
1112511114
!close_channel
1112611115
}
@@ -12374,15 +12363,15 @@ where
1237412363
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1237512364
let peer_state = &mut *peer_state_lock;
1237612365
let pending_msg_events = &mut peer_state.pending_msg_events;
12377-
peer_state.channel_by_id.retain(|chan_id, chan| {
12366+
peer_state.channel_by_id.retain(|_, chan| {
1237812367
let logger = WithChannelContext::from(&self.logger, &chan.context(), None);
1237912368
if chan.peer_disconnected_is_resumable(&&logger) {
1238012369
return true;
1238112370
}
1238212371
// Clean up for removal.
1238312372
let reason = ClosureReason::DisconnectedPeer;
1238412373
let err = ChannelError::Close((reason.to_string(), reason));
12385-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
12374+
let (_, e) = convert_channel_err!(self, peer_state, err, chan);
1238612375
failed_channels.push((Err(e), counterparty_node_id));
1238712376
false
1238812377
});
@@ -12935,7 +12924,7 @@ where
1293512924
let peer_state = &mut *peer_state_lock;
1293612925
let pending_msg_events = &mut peer_state.pending_msg_events;
1293712926

12938-
peer_state.channel_by_id.retain(|chan_id, chan| {
12927+
peer_state.channel_by_id.retain(|channel_id, chan| {
1293912928
match chan.as_funded_mut() {
1294012929
// Retain unfunded channels.
1294112930
None => true,
@@ -12946,22 +12935,22 @@ where
1294612935
let reason = LocalHTLCFailureReason::CLTVExpiryTooSoon;
1294712936
let data = self.get_htlc_inbound_temp_fail_data(reason);
1294812937
timed_out_htlcs.push((source, payment_hash, HTLCFailReason::reason(reason, data),
12949-
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: funded_channel.context.channel_id() }));
12938+
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: *channel_id }));
1295012939
}
1295112940
let logger = WithChannelContext::from(&self.logger, &funded_channel.context, None);
1295212941
match funding_confirmed_opt {
1295312942
Some(FundingConfirmedMessage::Establishment(channel_ready)) => {
1295412943
send_channel_ready!(self, pending_msg_events, funded_channel, channel_ready);
1295512944
if funded_channel.context.is_usable() {
12956-
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", funded_channel.context.channel_id());
12945+
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", channel_id);
1295712946
if let Ok(msg) = self.get_channel_update_for_unicast(funded_channel) {
1295812947
pending_msg_events.push(MessageSendEvent::SendChannelUpdate {
1295912948
node_id: funded_channel.context.get_counterparty_node_id(),
1296012949
msg,
1296112950
});
1296212951
}
1296312952
} else {
12964-
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", funded_channel.context.channel_id());
12953+
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", channel_id);
1296512954
}
1296612955
},
1296712956
#[cfg(splicing)]
@@ -12972,7 +12961,7 @@ where
1297212961

1297312962
let mut pending_events = self.pending_events.lock().unwrap();
1297412963
pending_events.push_back((events::Event::ChannelReady {
12975-
channel_id: funded_channel.context.channel_id(),
12964+
channel_id: *channel_id,
1297612965
user_channel_id: funded_channel.context.get_user_id(),
1297712966
counterparty_node_id: funded_channel.context.get_counterparty_node_id(),
1297812967
funding_txo: funding_txo.map(|outpoint| outpoint.into_bitcoin_outpoint()),
@@ -13044,8 +13033,8 @@ where
1304413033
// un-confirmed we force-close the channel, ensuring short_to_chan_info
1304513034
// is always consistent.
1304613035
let mut short_to_chan_info = self.short_to_chan_info.write().unwrap();
13047-
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()));
13048-
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()),
13036+
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), *channel_id));
13037+
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), *channel_id),
1304913038
"SCIDs should never collide - ensure you weren't behind by a full {} blocks when creating channels",
1305013039
fake_scid::MAX_SCID_BLOCKS_FROM_NOW);
1305113040
}
@@ -13059,7 +13048,6 @@ where
1305913048
peer_state,
1306013049
err,
1306113050
funded_channel,
13062-
chan_id,
1306313051
FUNDED_CHANNEL
1306413052
);
1306513053
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)