Skip to content

Commit 5dcae5c

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 fefb7b1 commit 5dcae5c

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
@@ -3169,32 +3169,33 @@ macro_rules! locked_close_channel {
31693169
/// Returns (boolean indicating if we should remove the Channel object from memory, a mapped error)
31703170
#[rustfmt::skip]
31713171
macro_rules! convert_channel_err {
3172-
($self: ident, $peer_state: expr, $err: expr, $chan: expr, $close: expr, $locked_close: expr, $channel_id: expr, _internal) => {
3172+
($self: ident, $peer_state: expr, $err: expr, $chan: expr, $close: expr, $locked_close: expr, $channel_id: expr, _internal) => { {
31733173
match $err {
31743174
ChannelError::Warn(msg) => {
3175-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), *$channel_id))
3175+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), $channel_id))
31763176
},
31773177
ChannelError::WarnAndDisconnect(msg) => {
3178-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::WarnAndDisconnect(msg), *$channel_id))
3178+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::WarnAndDisconnect(msg), $channel_id))
31793179
},
31803180
ChannelError::Ignore(msg) => {
3181-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), *$channel_id))
3181+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), $channel_id))
31823182
},
31833183
ChannelError::Close((msg, reason)) => {
31843184
let (mut shutdown_res, chan_update) = $close(reason);
31853185
let logger = WithChannelContext::from(&$self.logger, &$chan.context(), None);
31863186
log_error!(logger, "Closed channel {} due to close-required error: {}", $channel_id, msg);
31873187
$locked_close(&mut shutdown_res, $chan);
31883188
let err =
3189-
MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, chan_update);
3189+
MsgHandleErrInternal::from_finish_shutdown(msg, $channel_id, shutdown_res, chan_update);
31903190
(true, err)
31913191
},
31923192
ChannelError::SendError(msg) => {
3193-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::SendError(msg), *$channel_id))
3193+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::SendError(msg), $channel_id))
31943194
},
31953195
}
3196-
};
3197-
($self: ident, $peer_state: expr, $shutdown_result: expr, $funded_channel: expr, $channel_id: expr, COOP_CLOSED) => { {
3196+
} };
3197+
($self: ident, $peer_state: expr, $shutdown_result: expr, $funded_channel: expr, COOP_CLOSED) => { {
3198+
let chan_id = $funded_channel.context.channel_id();
31983199
let reason = ChannelError::Close(("Coop Closed".to_owned(), $shutdown_result.closure_reason.clone()));
31993200
let do_close = |_| {
32003201
(
@@ -3206,12 +3207,13 @@ macro_rules! convert_channel_err {
32063207
locked_close_channel!($self, $peer_state, funded_channel, shutdown_res_mut, FUNDED);
32073208
};
32083209
let (close, mut err) =
3209-
convert_channel_err!($self, $peer_state, reason, $funded_channel, do_close, locked_close, $channel_id, _internal);
3210+
convert_channel_err!($self, $peer_state, reason, $funded_channel, do_close, locked_close, chan_id, _internal);
32103211
err.dont_send_error_message();
32113212
debug_assert!(close);
32123213
err
32133214
} };
3214-
($self: ident, $peer_state: expr, $err: expr, $funded_channel: expr, $channel_id: expr, FUNDED_CHANNEL) => { {
3215+
($self: ident, $peer_state: expr, $err: expr, $funded_channel: expr, FUNDED_CHANNEL) => { {
3216+
let chan_id = $funded_channel.context.channel_id();
32153217
let mut do_close = |reason| {
32163218
(
32173219
$funded_channel.force_shutdown(reason),
@@ -3221,20 +3223,21 @@ macro_rules! convert_channel_err {
32213223
let mut locked_close = |shutdown_res_mut: &mut ShutdownResult, funded_channel: &mut FundedChannel<_>| {
32223224
locked_close_channel!($self, $peer_state, funded_channel, shutdown_res_mut, FUNDED);
32233225
};
3224-
convert_channel_err!($self, $peer_state, $err, $funded_channel, do_close, locked_close, $channel_id, _internal)
3226+
convert_channel_err!($self, $peer_state, $err, $funded_channel, do_close, locked_close, chan_id, _internal)
32253227
} };
3226-
($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr, UNFUNDED_CHANNEL) => { {
3228+
($self: ident, $peer_state: expr, $err: expr, $channel: expr, UNFUNDED_CHANNEL) => { {
3229+
let chan_id = $channel.context().channel_id();
32273230
let mut do_close = |reason| { ($channel.force_shutdown(reason), None) };
32283231
let locked_close = |_, chan: &mut Channel<_>| { locked_close_channel!($self, chan.context(), UNFUNDED); };
3229-
convert_channel_err!($self, $peer_state, $err, $channel, do_close, locked_close, $channel_id, _internal)
3232+
convert_channel_err!($self, $peer_state, $err, $channel, do_close, locked_close, chan_id, _internal)
32303233
} };
3231-
($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr) => {
3234+
($self: ident, $peer_state: expr, $err: expr, $channel: expr) => {
32323235
match $channel.as_funded_mut() {
32333236
Some(funded_channel) => {
3234-
convert_channel_err!($self, $peer_state, $err, funded_channel, $channel_id, FUNDED_CHANNEL)
3237+
convert_channel_err!($self, $peer_state, $err, funded_channel, FUNDED_CHANNEL)
32353238
},
32363239
None => {
3237-
convert_channel_err!($self, $peer_state, $err, $channel, $channel_id, UNFUNDED_CHANNEL)
3240+
convert_channel_err!($self, $peer_state, $err, $channel, UNFUNDED_CHANNEL)
32383241
},
32393242
}
32403243
};
@@ -3245,9 +3248,7 @@ macro_rules! break_channel_entry {
32453248
match $res {
32463249
Ok(res) => res,
32473250
Err(e) => {
3248-
let key = *$entry.key();
3249-
let (drop, res) =
3250-
convert_channel_err!($self, $peer_state, e, $entry.get_mut(), &key);
3251+
let (drop, res) = convert_channel_err!($self, $peer_state, e, $entry.get_mut());
32513252
if drop {
32523253
$entry.remove_entry();
32533254
}
@@ -3262,9 +3263,7 @@ macro_rules! try_channel_entry {
32623263
match $res {
32633264
Ok(res) => res,
32643265
Err(e) => {
3265-
let key = *$entry.key();
3266-
let (drop, res) =
3267-
convert_channel_err!($self, $peer_state, e, $entry.get_mut(), &key);
3266+
let (drop, res) = convert_channel_err!($self, $peer_state, e, $entry.get_mut());
32683267
if drop {
32693268
$entry.remove_entry();
32703269
}
@@ -4106,7 +4105,7 @@ where
41064105
let reason = ClosureReason::LocallyCoopClosedUnfundedChannel;
41074106
let err = ChannelError::Close((reason.to_string(), reason));
41084107
let mut chan = chan_entry.remove();
4109-
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, chan_id);
4108+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
41104109
e.dont_send_error_message();
41114110
shutdown_result = Err(e);
41124111
}
@@ -4303,7 +4302,7 @@ where
43034302
if let Some(mut chan) = peer_state.channel_by_id.remove(&channel_id) {
43044303
let reason = ClosureReason::FundingBatchClosure;
43054304
let err = ChannelError::Close((reason.to_string(), reason));
4306-
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
4305+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
43074306
shutdown_results.push((Err(e), counterparty_node_id));
43084307
}
43094308
}
@@ -4367,7 +4366,7 @@ where
43674366
if let Some(mut chan) = peer_state.channel_by_id.remove(channel_id) {
43684367
log_error!(logger, "Force-closing channel {}", channel_id);
43694368
let err = ChannelError::Close((message, reason));
4370-
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, channel_id);
4369+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
43714370
mem::drop(peer_state_lock);
43724371
mem::drop(per_peer_state);
43734372
if is_from_counterparty {
@@ -5843,7 +5842,7 @@ where
58435842
let reason = ClosureReason::ProcessingError { err: e.clone() };
58445843
let err = ChannelError::Close((e.clone(), reason));
58455844
let (_, e) =
5846-
convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
5845+
convert_channel_err!(self, peer_state, err, &mut chan);
58475846
shutdown_results.push((Err(e), counterparty_node_id));
58485847
});
58495848
}
@@ -7260,7 +7259,7 @@ where
72607259
if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
72617260

72627261
if let Err(e) = funded_chan.timer_check_closing_negotiation_progress() {
7263-
let (needs_close, err) = convert_channel_err!(self, peer_state, e, funded_chan, chan_id, FUNDED_CHANNEL);
7262+
let (needs_close, err) = convert_channel_err!(self, peer_state, e, funded_chan, FUNDED_CHANNEL);
72647263
handle_errors.push((Err(err), counterparty_node_id));
72657264
if needs_close { return false; }
72667265
}
@@ -7338,7 +7337,7 @@ where
73387337
let reason = ClosureReason::FundingTimedOut;
73397338
let msg = "Force-closing pending channel due to timeout awaiting establishment handshake".to_owned();
73407339
let err = ChannelError::Close((msg, reason));
7341-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
7340+
let (_, e) = convert_channel_err!(self, peer_state, err, chan);
73427341
handle_errors.push((Err(e), counterparty_node_id));
73437342
false
73447343
} else {
@@ -9173,18 +9172,15 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
91739172
// above so at this point we just need to clean up any lingering entries
91749173
// concerning this channel as it is safe to do so.
91759174
debug_assert!(matches!(err, ChannelError::Close(_)));
9176-
// Really we should be returning the channel_id the peer expects based
9177-
// on their funding info here, but they're horribly confused anyway, so
9178-
// there's not a lot we can do to save them.
91799175
let mut chan = Channel::from(inbound_chan);
9180-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id, UNFUNDED_CHANNEL).1);
9176+
return Err(convert_channel_err!(self, peer_state, err, &mut chan).1);
91819177
},
91829178
}
91839179
},
91849180
Some(Err(mut chan)) => {
91859181
let err_msg = format!("Got an unexpected funding_created message from peer with counterparty_node_id {}", counterparty_node_id);
91869182
let err = ChannelError::close(err_msg);
9187-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id).1);
9183+
return Err(convert_channel_err!(self, peer_state, err, &mut chan).1);
91889184
},
91899185
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))
91909186
};
@@ -9200,7 +9196,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
92009196
let err = ChannelError::close($err.to_owned());
92019197
chan.unset_funding_info();
92029198
let mut chan = Channel::from(chan);
9203-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &funded_channel_id, UNFUNDED_CHANNEL).1);
9199+
return Err(convert_channel_err!(self, peer_state, err, &mut chan, UNFUNDED_CHANNEL).1);
92049200
} } }
92059201

92069202
match peer_state.channel_by_id.entry(funded_channel_id) {
@@ -9740,8 +9736,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
97409736
let reason = ClosureReason::CounterpartyCoopClosedUnfundedChannel;
97419737
let err = ChannelError::Close((reason.to_string(), reason));
97429738
let mut chan = chan_entry.remove();
9743-
let (_, mut e) =
9744-
convert_channel_err!(self, peer_state, err, &mut chan, &msg.channel_id);
9739+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
97459740
e.dont_send_error_message();
97469741
return Err(e);
97479742
},
@@ -9800,7 +9795,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
98009795
// also implies there are no pending HTLCs left on the channel, so we can
98019796
// fully delete it from tracking (the channel monitor is still around to
98029797
// watch for old state broadcasts)!
9803-
let err = convert_channel_err!(self, peer_state, close_res, chan, &msg.channel_id, COOP_CLOSED);
9798+
let err = convert_channel_err!(self, peer_state, close_res, chan, COOP_CLOSED);
98049799
chan_entry.remove();
98059800
Some((tx, Err(err)))
98069801
} else {
@@ -10887,13 +10882,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1088710882
};
1088810883
let err = ChannelError::Close((reason.to_string(), reason));
1088910884
let mut chan = chan_entry.remove();
10890-
let (_, e) = convert_channel_err!(
10891-
self,
10892-
peer_state,
10893-
err,
10894-
&mut chan,
10895-
&channel_id
10896-
);
10885+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
1089710886
failed_channels.push((Err(e), counterparty_node_id));
1089810887
}
1089910888
}
@@ -11098,13 +11087,14 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1109811087
let chan_id = context.channel_id();
1109911088
log_trace!(logger, "Removing channel {} now that the signer is unblocked", chan_id);
1110011089
let (remove, err) = if let Some(funded) = chan.as_funded_mut() {
11101-
let err = convert_channel_err!(self, peer_state, shutdown, funded, &chan_id, COOP_CLOSED);
11090+
let err =
11091+
convert_channel_err!(self, peer_state, shutdown, funded, COOP_CLOSED);
1110211092
(true, err)
1110311093
} else {
1110411094
debug_assert!(false);
1110511095
let reason = shutdown.closure_reason.clone();
1110611096
let err = ChannelError::Close((reason.to_string(), reason));
11107-
convert_channel_err!(self, peer_state, err, chan, &chan_id, UNFUNDED_CHANNEL)
11097+
convert_channel_err!(self, peer_state, err, chan, UNFUNDED_CHANNEL)
1110811098
};
1110911099
debug_assert!(remove);
1111011100
shutdown_results.push((Err(err), *cp_id));
@@ -11134,7 +11124,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1113411124
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1113511125
let peer_state = &mut *peer_state_lock;
1113611126
let pending_msg_events = &mut peer_state.pending_msg_events;
11137-
peer_state.channel_by_id.retain(|channel_id, chan| {
11127+
peer_state.channel_by_id.retain(|_, chan| {
1113811128
match chan.as_funded_mut() {
1113911129
Some(funded_chan) => {
1114011130
let logger = WithChannelContext::from(&self.logger, &funded_chan.context, None);
@@ -11150,7 +11140,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1115011140
if let Some((tx, shutdown_res)) = tx_shutdown_result_opt {
1115111141
// We're done with this channel. We got a closing_signed and sent back
1115211142
// a closing_signed with a closing transaction to broadcast.
11153-
let err = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, channel_id, COOP_CLOSED);
11143+
let err = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, COOP_CLOSED);
1115411144
handle_errors.push((*cp_id, Err(err)));
1115511145

1115611146
log_info!(logger, "Broadcasting {}", log_tx!(tx));
@@ -11160,7 +11150,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1116011150
},
1116111151
Err(e) => {
1116211152
has_update = true;
11163-
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, channel_id, FUNDED_CHANNEL);
11153+
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, FUNDED_CHANNEL);
1116411154
handle_errors.push((funded_chan.context.get_counterparty_node_id(), Err(res)));
1116511155
!close_channel
1116611156
}
@@ -12414,15 +12404,15 @@ where
1241412404
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1241512405
let peer_state = &mut *peer_state_lock;
1241612406
let pending_msg_events = &mut peer_state.pending_msg_events;
12417-
peer_state.channel_by_id.retain(|chan_id, chan| {
12407+
peer_state.channel_by_id.retain(|_, chan| {
1241812408
let logger = WithChannelContext::from(&self.logger, &chan.context(), None);
1241912409
if chan.peer_disconnected_is_resumable(&&logger) {
1242012410
return true;
1242112411
}
1242212412
// Clean up for removal.
1242312413
let reason = ClosureReason::DisconnectedPeer;
1242412414
let err = ChannelError::Close((reason.to_string(), reason));
12425-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
12415+
let (_, e) = convert_channel_err!(self, peer_state, err, chan);
1242612416
failed_channels.push((Err(e), counterparty_node_id));
1242712417
false
1242812418
});
@@ -12975,7 +12965,7 @@ where
1297512965
let peer_state = &mut *peer_state_lock;
1297612966
let pending_msg_events = &mut peer_state.pending_msg_events;
1297712967

12978-
peer_state.channel_by_id.retain(|chan_id, chan| {
12968+
peer_state.channel_by_id.retain(|channel_id, chan| {
1297912969
match chan.as_funded_mut() {
1298012970
// Retain unfunded channels.
1298112971
None => true,
@@ -12986,22 +12976,22 @@ where
1298612976
let reason = LocalHTLCFailureReason::CLTVExpiryTooSoon;
1298712977
let data = self.get_htlc_inbound_temp_fail_data(reason);
1298812978
timed_out_htlcs.push((source, payment_hash, HTLCFailReason::reason(reason, data),
12989-
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: funded_channel.context.channel_id() }));
12979+
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: *channel_id }));
1299012980
}
1299112981
let logger = WithChannelContext::from(&self.logger, &funded_channel.context, None);
1299212982
match funding_confirmed_opt {
1299312983
Some(FundingConfirmedMessage::Establishment(channel_ready)) => {
1299412984
send_channel_ready!(self, pending_msg_events, funded_channel, channel_ready);
1299512985
if funded_channel.context.is_usable() {
12996-
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", funded_channel.context.channel_id());
12986+
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", channel_id);
1299712987
if let Ok(msg) = self.get_channel_update_for_unicast(funded_channel) {
1299812988
pending_msg_events.push(MessageSendEvent::SendChannelUpdate {
1299912989
node_id: funded_channel.context.get_counterparty_node_id(),
1300012990
msg,
1300112991
});
1300212992
}
1300312993
} else {
13004-
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", funded_channel.context.channel_id());
12994+
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", channel_id);
1300512995
}
1300612996
},
1300712997
#[cfg(splicing)]
@@ -13012,7 +13002,7 @@ where
1301213002

1301313003
let mut pending_events = self.pending_events.lock().unwrap();
1301413004
pending_events.push_back((events::Event::ChannelReady {
13015-
channel_id: funded_channel.context.channel_id(),
13005+
channel_id: *channel_id,
1301613006
user_channel_id: funded_channel.context.get_user_id(),
1301713007
counterparty_node_id: funded_channel.context.get_counterparty_node_id(),
1301813008
funding_txo: funding_txo.map(|outpoint| outpoint.into_bitcoin_outpoint()),
@@ -13084,8 +13074,8 @@ where
1308413074
// un-confirmed we force-close the channel, ensuring short_to_chan_info
1308513075
// is always consistent.
1308613076
let mut short_to_chan_info = self.short_to_chan_info.write().unwrap();
13087-
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()));
13088-
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()),
13077+
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), *channel_id));
13078+
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), *channel_id),
1308913079
"SCIDs should never collide - ensure you weren't behind by a full {} blocks when creating channels",
1309013080
fake_scid::MAX_SCID_BLOCKS_FROM_NOW);
1309113081
}
@@ -13099,7 +13089,6 @@ where
1309913089
peer_state,
1310013090
err,
1310113091
funded_channel,
13102-
chan_id,
1310313092
FUNDED_CHANNEL
1310413093
);
1310513094
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
@@ -9042,7 +9042,7 @@ pub fn test_duplicate_chan_id() {
90429042
} => {
90439043
// Technically, at this point, nodes[1] would be justified in thinking both
90449044
// channels are closed, but currently we do not, so we just move forward with it.
9045-
assert_eq!(msg.channel_id, channel_id);
9045+
assert_eq!(msg.channel_id, funding_created.temporary_channel_id);
90469046
assert_eq!(node_id, node_a_id);
90479047
},
90489048
_ => panic!("Unexpected event"),

0 commit comments

Comments
 (0)