Skip to content

Commit 77fabef

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 942942f commit 77fabef

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
}
@@ -4348,7 +4349,7 @@ where
43484349
if let Some(mut chan) = peer_state.channel_by_id.remove(&channel_id) {
43494350
let reason = ClosureReason::FundingBatchClosure;
43504351
let err = ChannelError::Close((reason.to_string(), reason));
4351-
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
4352+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
43524353
shutdown_results.push((Err(e), counterparty_node_id));
43534354
}
43544355
}
@@ -4412,7 +4413,7 @@ where
44124413
if let Some(mut chan) = peer_state.channel_by_id.remove(channel_id) {
44134414
log_error!(logger, "Force-closing channel {}", channel_id);
44144415
let err = ChannelError::Close((message, reason));
4415-
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, channel_id);
4416+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
44164417
mem::drop(peer_state_lock);
44174418
mem::drop(per_peer_state);
44184419
if is_from_counterparty {
@@ -5871,7 +5872,7 @@ where
58715872
let reason = ClosureReason::ProcessingError { err: e.clone() };
58725873
let err = ChannelError::Close((e.clone(), reason));
58735874
let (_, e) =
5874-
convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
5875+
convert_channel_err!(self, peer_state, err, &mut chan);
58755876
shutdown_results.push((Err(e), counterparty_node_id));
58765877
});
58775878
}
@@ -7037,7 +7038,7 @@ where
70377038
if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
70387039

70397040
if let Err(e) = funded_chan.timer_check_closing_negotiation_progress() {
7040-
let (needs_close, err) = convert_channel_err!(self, peer_state, e, funded_chan, chan_id, FUNDED_CHANNEL);
7041+
let (needs_close, err) = convert_channel_err!(self, peer_state, e, funded_chan, FUNDED_CHANNEL);
70417042
handle_errors.push((Err(err), counterparty_node_id));
70427043
if needs_close { return false; }
70437044
}
@@ -7115,7 +7116,7 @@ where
71157116
let reason = ClosureReason::FundingTimedOut;
71167117
let msg = "Force-closing pending channel due to timeout awaiting establishment handshake".to_owned();
71177118
let err = ChannelError::Close((msg, reason));
7118-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
7119+
let (_, e) = convert_channel_err!(self, peer_state, err, chan);
71197120
handle_errors.push((Err(e), counterparty_node_id));
71207121
false
71217122
} else {
@@ -8697,18 +8698,15 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
86978698
// above so at this point we just need to clean up any lingering entries
86988699
// concerning this channel as it is safe to do so.
86998700
debug_assert!(matches!(err, ChannelError::Close(_)));
8700-
// Really we should be returning the channel_id the peer expects based
8701-
// on their funding info here, but they're horribly confused anyway, so
8702-
// there's not a lot we can do to save them.
87038701
let mut chan = Channel::from(inbound_chan);
8704-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id, UNFUNDED_CHANNEL).1);
8702+
return Err(convert_channel_err!(self, peer_state, err, &mut chan).1);
87058703
},
87068704
}
87078705
},
87088706
Some(Err(mut chan)) => {
87098707
let err_msg = format!("Got an unexpected funding_created message from peer with counterparty_node_id {}", counterparty_node_id);
87108708
let err = ChannelError::close(err_msg);
8711-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id).1);
8709+
return Err(convert_channel_err!(self, peer_state, err, &mut chan).1);
87128710
},
87138711
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))
87148712
};
@@ -8724,7 +8722,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
87248722
let err = ChannelError::close($err.to_owned());
87258723
chan.unset_funding_info();
87268724
let mut chan = Channel::from(chan);
8727-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &funded_channel_id, UNFUNDED_CHANNEL).1);
8725+
return Err(convert_channel_err!(self, peer_state, err, &mut chan, UNFUNDED_CHANNEL).1);
87288726
} } }
87298727

87308728
match peer_state.channel_by_id.entry(funded_channel_id) {
@@ -9265,7 +9263,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
92659263
let err = ChannelError::Close((reason.to_string(), reason));
92669264
let mut chan = chan_entry.remove();
92679265
let (_, mut e) =
9268-
convert_channel_err!(self, peer_state, err, &mut chan, &msg.channel_id);
9266+
convert_channel_err!(self, peer_state, err, &mut chan);
92699267
e.dont_send_error_message();
92709268
return Err(e);
92719269
},
@@ -9324,7 +9322,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
93249322
// also implies there are no pending HTLCs left on the channel, so we can
93259323
// fully delete it from tracking (the channel monitor is still around to
93269324
// watch for old state broadcasts)!
9327-
let (_, err) = convert_channel_err!(self, peer_state, close_res, chan, &msg.channel_id, COOP_CLOSED);
9325+
let (_, err) = convert_channel_err!(self, peer_state, close_res, chan, COOP_CLOSED);
93289326
chan_entry.remove();
93299327
Some((tx, Err(err)))
93309328
} else {
@@ -10287,7 +10285,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1028710285
};
1028810286
let err = ChannelError::Close((reason.to_string(), reason));
1028910287
let mut chan = chan_entry.remove();
10290-
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
10288+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
1029110289
failed_channels.push((Err(e), counterparty_node_id));
1029210290
}
1029310291
}
@@ -10475,12 +10473,12 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1047510473
let chan_id = context.channel_id();
1047610474
log_trace!(logger, "Removing channel {} now that the signer is unblocked", chan_id);
1047710475
let (remove, err) = if let Some(funded_channel) = chan.as_funded_mut() {
10478-
convert_channel_err!(self, peer_state, shutdown_res, funded_channel, &chan_id, COOP_CLOSED)
10476+
convert_channel_err!(self, peer_state, shutdown_res, funded_channel, COOP_CLOSED)
1047910477
} else {
1048010478
debug_assert!(false);
1048110479
let reason = shutdown_res.closure_reason.clone();
1048210480
let err = ChannelError::Close((reason.to_string(), reason));
10483-
convert_channel_err!(self, peer_state, err, chan, &chan_id, UNFUNDED_CHANNEL)
10481+
convert_channel_err!(self, peer_state, err, chan, UNFUNDED_CHANNEL)
1048410482
};
1048510483
debug_assert!(remove);
1048610484
shutdown_results.push((Err(err), *cp_id));
@@ -10510,7 +10508,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1051010508
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1051110509
let peer_state = &mut *peer_state_lock;
1051210510
let pending_msg_events = &mut peer_state.pending_msg_events;
10513-
peer_state.channel_by_id.retain(|channel_id, chan| {
10511+
peer_state.channel_by_id.retain(|_, chan| {
1051410512
match chan.as_funded_mut() {
1051510513
Some(funded_chan) => {
1051610514
let logger = WithChannelContext::from(&self.logger, &funded_chan.context, None);
@@ -10526,7 +10524,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1052610524
if let Some((tx, shutdown_res)) = tx_shutdown_result_opt {
1052710525
// We're done with this channel. We got a closing_signed and sent back
1052810526
// a closing_signed with a closing transaction to broadcast.
10529-
let (_, err) = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, channel_id, COOP_CLOSED);
10527+
let (_, err) = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, COOP_CLOSED);
1053010528
handle_errors.push((*cp_id, Err(err)));
1053110529

1053210530
log_info!(logger, "Broadcasting {}", log_tx!(tx));
@@ -10536,7 +10534,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1053610534
},
1053710535
Err(e) => {
1053810536
has_update = true;
10539-
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, channel_id, FUNDED_CHANNEL);
10537+
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, FUNDED_CHANNEL);
1054010538
handle_errors.push((funded_chan.context.get_counterparty_node_id(), Err(res)));
1054110539
!close_channel
1054210540
}
@@ -11761,15 +11759,15 @@ where
1176111759
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1176211760
let peer_state = &mut *peer_state_lock;
1176311761
let pending_msg_events = &mut peer_state.pending_msg_events;
11764-
peer_state.channel_by_id.retain(|chan_id, chan| {
11762+
peer_state.channel_by_id.retain(|_, chan| {
1176511763
let logger = WithChannelContext::from(&self.logger, &chan.context(), None);
1176611764
if chan.peer_disconnected_is_resumable(&&logger) {
1176711765
return true;
1176811766
}
1176911767
// Clean up for removal.
1177011768
let reason = ClosureReason::DisconnectedPeer;
1177111769
let err = ChannelError::Close((reason.to_string(), reason));
11772-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
11770+
let (_, e) = convert_channel_err!(self, peer_state, err, chan);
1177311771
failed_channels.push((Err(e), counterparty_node_id));
1177411772
false
1177511773
});
@@ -12322,7 +12320,7 @@ where
1232212320
let peer_state = &mut *peer_state_lock;
1232312321
let pending_msg_events = &mut peer_state.pending_msg_events;
1232412322

12325-
peer_state.channel_by_id.retain(|chan_id, chan| {
12323+
peer_state.channel_by_id.retain(|channel_id, chan| {
1232612324
match chan.as_funded_mut() {
1232712325
// Retain unfunded channels.
1232812326
None => true,
@@ -12333,22 +12331,22 @@ where
1233312331
let reason = LocalHTLCFailureReason::CLTVExpiryTooSoon;
1233412332
let data = self.get_htlc_inbound_temp_fail_data(reason);
1233512333
timed_out_htlcs.push((source, payment_hash, HTLCFailReason::reason(reason, data),
12336-
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: funded_channel.context.channel_id() }));
12334+
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: *channel_id }));
1233712335
}
1233812336
let logger = WithChannelContext::from(&self.logger, &funded_channel.context, None);
1233912337
match funding_confirmed_opt {
1234012338
Some(FundingConfirmedMessage::Establishment(channel_ready)) => {
1234112339
send_channel_ready!(self, pending_msg_events, funded_channel, channel_ready);
1234212340
if funded_channel.context.is_usable() {
12343-
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", funded_channel.context.channel_id());
12341+
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", channel_id);
1234412342
if let Ok(msg) = self.get_channel_update_for_unicast(funded_channel) {
1234512343
pending_msg_events.push(MessageSendEvent::SendChannelUpdate {
1234612344
node_id: funded_channel.context.get_counterparty_node_id(),
1234712345
msg,
1234812346
});
1234912347
}
1235012348
} else {
12351-
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", funded_channel.context.channel_id());
12349+
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", channel_id);
1235212350
}
1235312351
},
1235412352
#[cfg(splicing)]
@@ -12359,7 +12357,7 @@ where
1235912357

1236012358
let mut pending_events = self.pending_events.lock().unwrap();
1236112359
pending_events.push_back((events::Event::ChannelReady {
12362-
channel_id: funded_channel.context.channel_id(),
12360+
channel_id,
1236312361
user_channel_id: funded_channel.context.get_user_id(),
1236412362
counterparty_node_id: funded_channel.context.get_counterparty_node_id(),
1236512363
funding_txo: funding_txo.map(|outpoint| outpoint.into_bitcoin_outpoint()),
@@ -12431,8 +12429,8 @@ where
1243112429
// un-confirmed we force-close the channel, ensuring short_to_chan_info
1243212430
// is always consistent.
1243312431
let mut short_to_chan_info = self.short_to_chan_info.write().unwrap();
12434-
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()));
12435-
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()),
12432+
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), *channel_id));
12433+
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), *channel_id),
1243612434
"SCIDs should never collide - ensure you weren't behind by a full {} blocks when creating channels",
1243712435
fake_scid::MAX_SCID_BLOCKS_FROM_NOW);
1243812436
}
@@ -12446,7 +12444,6 @@ where
1244612444
peer_state,
1244712445
err,
1244812446
funded_channel,
12449-
chan_id,
1245012447
FUNDED_CHANNEL
1245112448
);
1245212449
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)