Skip to content

Commit 6761bc6

Browse files
committed
Review comments
1 parent d60cfd0 commit 6761bc6

File tree

2 files changed

+39
-27
lines changed

2 files changed

+39
-27
lines changed

lightning/src/events/mod.rs

+8-6
Original file line numberDiff line numberDiff line change
@@ -784,17 +784,19 @@ pub enum Event {
784784
user_channel_id: u128,
785785
/// The reason the channel was closed.
786786
reason: ClosureReason,
787-
/// Counterparty in the closed channel.
788-
///
787+
/// Counterparty in the closed channel.
788+
///
789789
/// This field will be `None` for objects serialized prior to LDK 0.0.117.
790790
counterparty_node_id: Option<PublicKey>,
791-
/// Channel capacity of the closing channel (sats).
792-
///
791+
/// Channel capacity of the closing channel (sats).
792+
///
793793
/// This field will be `None` for objects serialized prior to LDK 0.0.117.
794794
channel_capacity_sats: Option<u64>,
795795
},
796796
/// Used to indicate to the user that they can abandon the funding transaction and recycle the
797797
/// inputs for another purpose.
798+
///
799+
/// This event is not guaranteed to be generated for channels that are closed due to a restart.
798800
DiscardFunding {
799801
/// The channel_id of the channel which has been closed.
800802
channel_id: [u8; 32],
@@ -993,8 +995,8 @@ impl Writeable for Event {
993995
(5, outbound_amount_forwarded_msat, option),
994996
});
995997
},
996-
&Event::ChannelClosed { ref channel_id, ref user_channel_id, ref reason,
997-
ref counterparty_node_id, ref channel_capacity_sats
998+
&Event::ChannelClosed { ref channel_id, ref user_channel_id, ref reason,
999+
ref counterparty_node_id, ref channel_capacity_sats
9981000
} => {
9991001
9u8.write(writer)?;
10001002
// `user_channel_id` used to be a single u64 value. In order to remain backwards

lightning/src/ln/channel.rs

+31-21
Original file line numberDiff line numberDiff line change
@@ -304,8 +304,19 @@ enum ChannelState {
304304
/// have received funding_signed and have their monitors persisted.
305305
WaitingForBatch = 1 << 13,
306306
}
307-
const BOTH_SIDES_SHUTDOWN_MASK: u32 = ChannelState::LocalShutdownSent as u32 | ChannelState::RemoteShutdownSent as u32;
308-
const MULTI_STATE_FLAGS: u32 = BOTH_SIDES_SHUTDOWN_MASK | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32;
307+
const BOTH_SIDES_SHUTDOWN_MASK: u32 =
308+
ChannelState::LocalShutdownSent as u32 |
309+
ChannelState::RemoteShutdownSent as u32;
310+
const MULTI_STATE_FLAGS: u32 =
311+
BOTH_SIDES_SHUTDOWN_MASK |
312+
ChannelState::PeerDisconnected as u32 |
313+
ChannelState::MonitorUpdateInProgress as u32;
314+
const STATE_FLAGS: u32 =
315+
MULTI_STATE_FLAGS |
316+
ChannelState::TheirChannelReady as u32 |
317+
ChannelState::OurChannelReady as u32 |
318+
ChannelState::AwaitingRemoteRevoke as u32 |
319+
ChannelState::WaitingForBatch as u32;
309320

310321
pub const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1;
311322

@@ -914,7 +925,7 @@ impl<Signer: ChannelSigner> ChannelContext<Signer> {
914925

915926
/// Returns true if we've ever received a message from the remote end for this Channel
916927
pub fn have_received_message(&self) -> bool {
917-
self.channel_state > (ChannelState::OurInitSent as u32)
928+
self.channel_state & !STATE_FLAGS > (ChannelState::OurInitSent as u32)
918929
}
919930

920931
/// Returns true if this channel is fully established and not known to be closing.
@@ -1192,7 +1203,7 @@ impl<Signer: ChannelSigner> ChannelContext<Signer> {
11921203
/// Returns true if funding_signed was sent/received and the
11931204
/// funding transaction has been broadcast if necessary.
11941205
pub fn is_funding_initiated(&self) -> bool {
1195-
self.channel_state >= ChannelState::FundingSent as u32 &&
1206+
self.channel_state & !STATE_FLAGS >= ChannelState::FundingSent as u32 &&
11961207
self.channel_state & ChannelState::WaitingForBatch as u32 == 0
11971208
}
11981209

@@ -2603,6 +2614,8 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
26032614

26042615
let non_shutdown_state = self.context.channel_state & (!MULTI_STATE_FLAGS);
26052616

2617+
// If the WaitingForBatch flag is set, we can receive their channel_ready, but our
2618+
// channel_ready shouldn't have been sent and we shouldn't move to ChannelReady.
26062619
if non_shutdown_state & !(ChannelState::WaitingForBatch as u32) == ChannelState::FundingSent as u32 {
26072620
self.context.channel_state |= ChannelState::TheirChannelReady as u32;
26082621
} else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurChannelReady as u32) {
@@ -3102,7 +3115,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
31023115
) -> (Option<ChannelMonitorUpdate>, Vec<(HTLCSource, PaymentHash)>)
31033116
where F::Target: FeeEstimator, L::Target: Logger
31043117
{
3105-
if self.context.channel_state >= ChannelState::ChannelReady as u32 &&
3118+
if self.context.channel_state & !STATE_FLAGS >= ChannelState::ChannelReady as u32 &&
31063119
(self.context.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32)) == 0 {
31073120
self.free_holding_cell_htlcs(fee_estimator, logger)
31083121
} else { (None, Vec::new()) }
@@ -3551,7 +3564,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
35513564
/// completed.
35523565
pub fn remove_uncommitted_htlcs_and_mark_paused<L: Deref>(&mut self, logger: &L) where L::Target: Logger {
35533566
assert_eq!(self.context.channel_state & ChannelState::ShutdownComplete as u32, 0);
3554-
if self.context.channel_state < ChannelState::FundingSent as u32 {
3567+
if self.context.channel_state & !STATE_FLAGS < ChannelState::FundingSent as u32 {
35553568
self.context.channel_state = ChannelState::ShutdownComplete as u32;
35563569
return;
35573570
}
@@ -3665,13 +3678,13 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
36653678
// (re-)broadcast the funding transaction as we may have declined to broadcast it when we
36663679
// first received the funding_signed.
36673680
let mut funding_broadcastable =
3668-
if self.context.is_outbound() && self.context.channel_state & !MULTI_STATE_FLAGS >= ChannelState::FundingSent as u32 && self.context.channel_state & ChannelState::WaitingForBatch as u32 == 0 {
3681+
if self.context.is_outbound() && self.context.channel_state & !STATE_FLAGS >= ChannelState::FundingSent as u32 && self.context.channel_state & ChannelState::WaitingForBatch as u32 == 0 {
36693682
self.context.funding_txid.take();
36703683
self.context.funding_transaction.take()
36713684
} else { None };
36723685
// That said, if the funding transaction is already confirmed (ie we're active with a
36733686
// minimum_depth over 0) don't bother re-broadcasting the confirmed funding tx.
3674-
if self.context.channel_state & !MULTI_STATE_FLAGS >= ChannelState::ChannelReady as u32 && self.context.minimum_depth != Some(0) {
3687+
if self.context.channel_state & !STATE_FLAGS >= ChannelState::ChannelReady as u32 && self.context.minimum_depth != Some(0) {
36753688
funding_broadcastable = None;
36763689
}
36773690

@@ -4166,7 +4179,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
41664179
if self.context.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 {
41674180
return Err(ChannelError::Close("Peer sent shutdown when we needed a channel_reestablish".to_owned()));
41684181
}
4169-
if self.context.channel_state < ChannelState::FundingSent as u32 {
4182+
if self.context.channel_state & !STATE_FLAGS < ChannelState::FundingSent as u32 {
41704183
// Spec says we should fail the connection, not the channel, but that's nonsense, there
41714184
// are plenty of reasons you may want to fail a channel pre-funding, and spec says you
41724185
// can do that via error message without getting a connection fail anyway...
@@ -4587,7 +4600,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
45874600

45884601
/// Returns true if our channel_ready has been sent
45894602
pub fn is_our_channel_ready(&self) -> bool {
4590-
(self.context.channel_state & ChannelState::OurChannelReady as u32) != 0 || self.context.channel_state >= ChannelState::ChannelReady as u32
4603+
(self.context.channel_state & ChannelState::OurChannelReady as u32) != 0 || self.context.channel_state & !STATE_FLAGS >= ChannelState::ChannelReady as u32
45914604
}
45924605

45934606
/// Returns true if our peer has either initiated or agreed to shut down the channel.
@@ -4650,7 +4663,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
46504663
// We got a reorg but not enough to trigger a force close, just ignore.
46514664
false
46524665
} else {
4653-
if self.context.funding_tx_confirmation_height != 0 && self.context.channel_state < ChannelState::ChannelReady as u32 {
4666+
if self.context.funding_tx_confirmation_height != 0 && self.context.channel_state & !STATE_FLAGS < ChannelState::ChannelReady as u32 {
46544667
// We should never see a funding transaction on-chain until we've received
46554668
// funding_signed (if we're an outbound channel), or seen funding_generated (if we're
46564669
// an inbound channel - before that we have no known funding TXID). The fuzzer,
@@ -4811,7 +4824,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
48114824
}
48124825

48134826
let non_shutdown_state = self.context.channel_state & (!MULTI_STATE_FLAGS);
4814-
if non_shutdown_state >= ChannelState::ChannelReady as u32 ||
4827+
if non_shutdown_state & !STATE_FLAGS >= ChannelState::ChannelReady as u32 ||
48154828
(non_shutdown_state & ChannelState::OurChannelReady as u32) == ChannelState::OurChannelReady as u32 {
48164829
let mut funding_tx_confirmations = height as i64 - self.context.funding_tx_confirmation_height as i64 + 1;
48174830
if self.context.funding_tx_confirmation_height == 0 {
@@ -4839,7 +4852,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
48394852
height >= self.context.channel_creation_height + FUNDING_CONF_DEADLINE_BLOCKS {
48404853
log_info!(logger, "Closing channel {} due to funding timeout", log_bytes!(self.context.channel_id));
48414854
// If funding_tx_confirmed_in is unset, the channel must not be active
4842-
assert!(non_shutdown_state <= ChannelState::ChannelReady as u32);
4855+
assert!(non_shutdown_state & !STATE_FLAGS <= ChannelState::ChannelReady as u32);
48434856
assert_eq!(non_shutdown_state & ChannelState::OurChannelReady as u32, 0);
48444857
return Err(ClosureReason::FundingTimedOut);
48454858
}
@@ -5437,7 +5450,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
54375450
// If we haven't funded the channel yet, we don't need to bother ensuring the shutdown
54385451
// script is set, we just force-close and call it a day.
54395452
let mut chan_closed = false;
5440-
if self.context.channel_state < ChannelState::FundingSent as u32 {
5453+
if self.context.channel_state & !STATE_FLAGS < ChannelState::FundingSent as u32 {
54415454
chan_closed = true;
54425455
}
54435456

@@ -5466,7 +5479,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
54665479

54675480
// From here on out, we may not fail!
54685481
self.context.target_closing_feerate_sats_per_kw = target_feerate_sats_per_kw;
5469-
if self.context.channel_state < ChannelState::FundingSent as u32 {
5482+
if self.context.channel_state & !STATE_FLAGS < ChannelState::FundingSent as u32 {
54705483
self.context.channel_state = ChannelState::ShutdownComplete as u32;
54715484
} else {
54725485
self.context.channel_state |= ChannelState::LocalShutdownSent as u32;
@@ -7252,7 +7265,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
72527265
// If we've gotten to the funding stage of the channel, populate the signer with its
72537266
// required channel parameters.
72547267
let non_shutdown_state = channel_state & (!MULTI_STATE_FLAGS);
7255-
if non_shutdown_state >= (ChannelState::FundingCreated as u32) {
7268+
if non_shutdown_state & !STATE_FLAGS >= (ChannelState::FundingCreated as u32) {
72567269
holder_signer.provide_channel_parameters(&channel_parameters);
72577270
}
72587271
(channel_keys_id, holder_signer)
@@ -8978,13 +8991,10 @@ mod tests {
89788991
&config,
89798992
0,
89808993
&&logger,
8981-
42,
8994+
true, // Allow node b to send a 0conf channel_ready.
89828995
).unwrap();
89838996

8984-
// Allow node b to send a 0conf channel_ready.
8985-
node_b_chan.set_0conf();
8986-
8987-
let accept_channel_msg = node_b_chan.accept_inbound_channel(0);
8997+
let accept_channel_msg = node_b_chan.accept_inbound_channel();
89888998
node_a_chan.accept_channel(
89898999
&accept_channel_msg,
89909000
&config.channel_handshake_limits,

0 commit comments

Comments
 (0)