Skip to content

Commit fc880f2

Browse files
wvanlintwvanlint
committed
Review comments
1 parent 693b7d5 commit fc880f2

File tree

5 files changed

+383
-47
lines changed

5 files changed

+383
-47
lines changed

‎lightning/src/ln/channel.rs

+10-6
Original file line numberDiff line numberDiff line change
@@ -1191,7 +1191,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
11911191

11921192
// Checks whether we should emit a `ChannelPending` event.
11931193
pub(crate) fn should_emit_channel_pending_event(&mut self) -> bool {
1194-
self.is_funding_initiated() && !self.channel_pending_event_emitted
1194+
self.is_funding_broadcast() && !self.channel_pending_event_emitted
11951195
}
11961196

11971197
// Returns whether we already emitted a `ChannelPending` event.
@@ -1252,7 +1252,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
12521252

12531253
/// Returns true if funding_signed was sent/received and the
12541254
/// funding transaction has been broadcast if necessary.
1255-
pub fn is_funding_initiated(&self) -> bool {
1255+
pub fn is_funding_broadcast(&self) -> bool {
12561256
self.channel_state & !STATE_FLAGS >= ChannelState::FundingSent as u32 &&
12571257
self.channel_state & ChannelState::WaitingForBatch as u32 == 0
12581258
}
@@ -2672,8 +2672,12 @@ impl<SP: Deref> Channel<SP> where
26722672

26732673
let non_shutdown_state = self.context.channel_state & (!MULTI_STATE_FLAGS);
26742674

2675-
// If the WaitingForBatch flag is set, we can receive their channel_ready, but our
2676-
// channel_ready shouldn't have been sent and we shouldn't move to ChannelReady.
2675+
// Our channel_ready shouldn't have been sent if we are waiting for other channels in the
2676+
// batch, but we can receive channel_ready messages.
2677+
debug_assert!(
2678+
non_shutdown_state & ChannelState::OurChannelReady as u32 == 0 ||
2679+
non_shutdown_state & ChannelState::WaitingForBatch as u32 == 0
2680+
);
26772681
if non_shutdown_state & !(ChannelState::WaitingForBatch as u32) == ChannelState::FundingSent as u32 {
26782682
self.context.channel_state |= ChannelState::TheirChannelReady as u32;
26792683
} else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurChannelReady as u32) {
@@ -9105,7 +9109,7 @@ mod tests {
91059109

91069110
// Create a channel from node a to node b that will be part of batch funding.
91079111
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
9108-
let mut node_a_chan = OutboundV1Channel::<EnforcingSigner>::new(
9112+
let mut node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(
91099113
&feeest,
91109114
&&keys_provider,
91119115
&&keys_provider,
@@ -9121,7 +9125,7 @@ mod tests {
91219125

91229126
let open_channel_msg = node_a_chan.get_open_channel(genesis_block(network).header.block_hash());
91239127
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
9124-
let mut node_b_chan = InboundV1Channel::<EnforcingSigner>::new(
9128+
let mut node_b_chan = InboundV1Channel::<&TestKeysInterface>::new(
91259129
&feeest,
91269130
&&keys_provider,
91279131
&&keys_provider,

‎lightning/src/ln/channelmanager.rs

+37-33
Original file line numberDiff line numberDiff line change
@@ -1193,15 +1193,15 @@ where
11931193
/// `PersistenceNotifierGuard::notify_on_drop(..)` and pass the lock to it, to ensure the
11941194
/// Notifier the lock contains sends out a notification when the lock is released.
11951195
total_consistency_lock: RwLock<()>,
1196-
/// Tracks the progress of channels going through batch v1 channel establishment by whether
1197-
/// funding_signed was received and the monitor has been persisted.
1196+
/// Tracks the progress of channels going through batch funding by whether funding_signed was
1197+
/// received and the monitor has been persisted.
11981198
///
11991199
/// This information does not need to be persisted as funding nodes can forget
12001200
/// unfunded channels upon disconnection.
1201-
v1_funding_batch_states: FairRwLock<HashMap<Txid, Mutex<HashMap<([u8;32], PublicKey), bool>>>>,
1201+
funding_batch_states: FairRwLock<HashMap<Txid, Mutex<HashMap<([u8;32], PublicKey), bool>>>>,
12021202
/// Remaining channels in a funding batch need to be closed when one channel closes.
12031203
/// These batches are maintained here to be periodically processed to simplify locking behavior.
1204-
v1_funding_batches_to_be_closed: Mutex<Vec<Txid>>,
1204+
funding_batches_to_be_closed: Mutex<Vec<Txid>>,
12051205

12061206
background_events_processed_since_startup: AtomicBool,
12071207

@@ -1813,10 +1813,10 @@ macro_rules! update_maps_on_chan_removal {
18131813
short_to_chan_info.remove(&$channel_context.outbound_scid_alias());
18141814
// If the channel was part of a batch funding transaction, all channels in that
18151815
// batch are affected.
1816-
let v1_funding_batch_states = $self.v1_funding_batch_states.read().unwrap();
1816+
let funding_batch_states = $self.funding_batch_states.read().unwrap();
18171817
$channel_context.unbroadcasted_funding_txid().map(|txid| {
1818-
if v1_funding_batch_states.contains_key(&txid) {
1819-
$self.v1_funding_batches_to_be_closed.lock().unwrap().push(txid);
1818+
if funding_batch_states.contains_key(&txid) {
1819+
$self.funding_batches_to_be_closed.lock().unwrap().push(txid);
18201820
}
18211821
})
18221822
}}
@@ -1967,9 +1967,9 @@ macro_rules! handle_monitor_update_completion {
19671967
// should be updated as we have received funding_signed and persisted the monitor.
19681968
let mut completed_batch = None;
19691969
{
1970-
let v1_funding_batch_states = $self.v1_funding_batch_states.read().unwrap();
1970+
let funding_batch_states = $self.funding_batch_states.read().unwrap();
19711971
let batch_state_key_value = $chan.context.unbroadcasted_funding_txid()
1972-
.and_then(|txid| v1_funding_batch_states.get_key_value(&txid));
1972+
.and_then(|txid| funding_batch_states.get_key_value(&txid));
19731973
if let Some((txid, batch_state)) = batch_state_key_value {
19741974
let mut batch_state = batch_state.lock().unwrap();
19751975
batch_state.insert(
@@ -2019,7 +2019,7 @@ macro_rules! handle_monitor_update_completion {
20192019
// When all channels in a batched funding transaction have become ready, it is not necessary
20202020
// to track the progress of the batch anymore and the state of the channels can be updated.
20212021
if let Some(txid) = completed_batch {
2022-
let other_channel_ids = $self.v1_funding_batch_states.write().unwrap()
2022+
let other_channel_ids = $self.funding_batch_states.write().unwrap()
20232023
.remove(&txid)
20242024
.map(|batch_state| batch_state.into_inner().unwrap().into_iter().map(|(k, _)| k))
20252025
.into_iter().flatten()
@@ -2254,8 +2254,8 @@ where
22542254
total_consistency_lock: RwLock::new(()),
22552255
background_events_processed_since_startup: AtomicBool::new(false),
22562256
persistence_notifier: Notifier::new(),
2257-
v1_funding_batch_states: FairRwLock::new(HashMap::new()),
2258-
v1_funding_batches_to_be_closed: Mutex::new(Vec::new()),
2257+
funding_batch_states: FairRwLock::new(HashMap::new()),
2258+
funding_batches_to_be_closed: Mutex::new(Vec::new()),
22592259

22602260
entropy_source,
22612261
node_signer,
@@ -3659,7 +3659,7 @@ where
36593659
}
36603660

36613661
let is_batch_funding = temporary_channels.len() > 1;
3662-
let v1_funding_batch_state = RefCell::new(HashMap::new());
3662+
let funding_batch_state = RefCell::new(HashMap::new());
36633663
for (temporary_channel_id, counterparty_node_id) in temporary_channels {
36643664
result = result.and_then(|_| self.funding_transaction_generated_intern(
36653665
temporary_channel_id,
@@ -3684,7 +3684,7 @@ where
36843684
});
36853685
}
36863686
let outpoint = OutPoint { txid: tx.txid(), index: output_index.unwrap() };
3687-
v1_funding_batch_state.borrow_mut().insert((outpoint.to_channel_id(), (*counterparty_node_id).clone()), false);
3687+
funding_batch_state.borrow_mut().insert((outpoint.to_channel_id(), (*counterparty_node_id).clone()), false);
36883688
Ok(outpoint)
36893689
})
36903690
);
@@ -3702,7 +3702,7 @@ where
37023702
self.issue_channel_close_events(&chan.context, ClosureReason::ProcessingError { err: e.clone() });
37033703
});
37043704
}
3705-
for (channel_id, counterparty_node_id) in v1_funding_batch_state.borrow().keys() {
3705+
for (channel_id, counterparty_node_id) in funding_batch_state.borrow().keys() {
37063706
per_peer_state.get(counterparty_node_id)
37073707
.map(|peer_state_mutex| peer_state_mutex.lock().unwrap())
37083708
.and_then(|mut peer_state| peer_state.channel_by_id.remove(channel_id))
@@ -3713,9 +3713,9 @@ where
37133713
}
37143714
} else if is_batch_funding {
37153715
// Initialize the state of the batch.
3716-
self.v1_funding_batch_states.write().unwrap().insert(
3716+
self.funding_batch_states.write().unwrap().insert(
37173717
funding_transaction.txid(),
3718-
Mutex::new(v1_funding_batch_state.into_inner()),
3718+
Mutex::new(funding_batch_state.into_inner()),
37193719
);
37203720
}
37213721
result
@@ -4779,9 +4779,9 @@ where
47794779
// Close remaining channels in funding batches when one channel closes.
47804780
let mut affected_channels = Vec::new();
47814781
{
4782-
let mut v1_funding_batch_states = self.v1_funding_batch_states.write().unwrap();
4783-
for txid in self.v1_funding_batches_to_be_closed.lock().unwrap().drain(..) {
4784-
affected_channels.extend(v1_funding_batch_states
4782+
let mut funding_batch_states = self.funding_batch_states.write().unwrap();
4783+
for txid in self.funding_batches_to_be_closed.lock().unwrap().drain(..) {
4784+
affected_channels.extend(funding_batch_states
47854785
.remove(&txid)
47864786
.map(|state| state.into_inner().unwrap().into_iter().map(|(k, _)| k))
47874787
.into_iter().flatten()
@@ -5845,7 +5845,7 @@ where
58455845
match peer_state.channel_by_id.entry(msg.channel_id) {
58465846
hash_map::Entry::Occupied(mut chan) => {
58475847
let is_batch_funding = chan.get().context.unbroadcasted_funding_txid()
5848-
.map(|txid| self.v1_funding_batch_states.read().unwrap().contains_key(&txid))
5848+
.map(|txid| self.funding_batch_states.read().unwrap().contains_key(&txid))
58495849
.unwrap_or(false);
58505850
let monitor = try_chan_entry!(self,
58515851
chan.get_mut().funding_signed(&msg, best_block, &self.signer_provider, is_batch_funding, &self.logger), chan);
@@ -7547,20 +7547,24 @@ where
75477547
let peer_state = &mut *peer_state_lock;
75487548

75497549
peer_state.channel_by_id.retain(|_, chan| {
7550-
if !chan.context.is_funding_initiated() {
7550+
if !chan.context.is_funding_broadcast() {
7551+
update_maps_on_chan_removal!(self, &chan.context);
7552+
self.issue_channel_close_events(&chan.context, ClosureReason::DisconnectedPeer);
75517553
// It is possible to have persisted the monitor upon funding_signed
75527554
// but not have broadcast the transaction, especially for batch funding.
75537555
// The monitor should be moved to the correct state.
75547556
self.finish_force_close_channel(chan.context.force_shutdown(false));
7557+
false
75557558
} else {
75567559
chan.remove_uncommitted_htlcs_and_mark_paused(&self.logger);
7560+
if chan.is_shutdown() {
7561+
update_maps_on_chan_removal!(self, &chan.context);
7562+
self.issue_channel_close_events(&chan.context, ClosureReason::DisconnectedPeer);
7563+
false
7564+
} else {
7565+
true
7566+
}
75577567
}
7558-
if chan.is_shutdown() {
7559-
update_maps_on_chan_removal!(self, &chan.context);
7560-
self.issue_channel_close_events(&chan.context, ClosureReason::DisconnectedPeer);
7561-
return false;
7562-
}
7563-
true
75647568
});
75657569
peer_state.inbound_v1_channel_by_id.retain(|_, chan| {
75667570
update_maps_on_chan_removal!(self, &chan.context);
@@ -8365,7 +8369,7 @@ where
83658369
}
83668370
number_of_channels += peer_state.channel_by_id.len();
83678371
for (_, channel) in peer_state.channel_by_id.iter() {
8368-
if !channel.context.is_funding_initiated() {
8372+
if !channel.context.is_funding_broadcast() {
83698373
unfunded_channels += 1;
83708374
}
83718375
}
@@ -8377,7 +8381,7 @@ where
83778381
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
83788382
let peer_state = &mut *peer_state_lock;
83798383
for (_, channel) in peer_state.channel_by_id.iter() {
8380-
if channel.context.is_funding_initiated() {
8384+
if channel.context.is_funding_broadcast() {
83818385
channel.write(writer)?;
83828386
}
83838387
}
@@ -8824,7 +8828,7 @@ where
88248828
if let Some(short_channel_id) = channel.context.get_short_channel_id() {
88258829
short_to_chan_info.insert(short_channel_id, (channel.context.get_counterparty_node_id(), channel.context.channel_id()));
88268830
}
8827-
if channel.context.is_funding_initiated() {
8831+
if channel.context.is_funding_broadcast() {
88288832
id_to_peer.insert(channel.context.channel_id(), channel.context.get_counterparty_node_id());
88298833
}
88308834
match peer_channels.entry(channel.context.get_counterparty_node_id()) {
@@ -9521,8 +9525,8 @@ where
95219525
total_consistency_lock: RwLock::new(()),
95229526
background_events_processed_since_startup: AtomicBool::new(false),
95239527
persistence_notifier: Notifier::new(),
9524-
v1_funding_batch_states: FairRwLock::new(HashMap::new()),
9525-
v1_funding_batches_to_be_closed: Mutex::new(Vec::new()),
9528+
funding_batch_states: FairRwLock::new(HashMap::new()),
9529+
funding_batches_to_be_closed: Mutex::new(Vec::new()),
95269530

95279531
entropy_source: args.entropy_source,
95289532
node_signer: args.node_signer,

‎lightning/src/ln/functional_test_utils.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -1447,12 +1447,12 @@ pub fn check_closed_event(node: &Node, events_count: usize, expected_reason: Clo
14471447
let events = node.node.get_and_clear_pending_events();
14481448
assert_eq!(events.len(), events_count, "{:?}", events);
14491449
let mut issues_discard_funding = false;
1450-
for (idx, event) in events.into_iter().enumerate() {
1450+
for event in events {
14511451
match event {
1452-
Event::ChannelClosed { ref reason, counterparty_node_id,
1452+
Event::ChannelClosed { ref reason, counterparty_node_id,
14531453
channel_capacity_sats, .. } => {
14541454
assert_eq!(*reason, expected_reason);
1455-
assert_eq!(counterparty_node_id.unwrap(), expected_counterparty_node_ids[idx]);
1455+
assert!(expected_counterparty_node_ids.iter().any(|id| id == &counterparty_node_id.unwrap()));
14561456
assert_eq!(channel_capacity_sats.unwrap(), expected_channel_capacity);
14571457
},
14581458
Event::DiscardFunding { .. } => {
@@ -1473,7 +1473,7 @@ macro_rules! check_closed_event {
14731473
check_closed_event!($node, $events, $reason, false, $counterparty_node_ids, $channel_capacity);
14741474
};
14751475
($node: expr, $events: expr, $reason: expr, $is_check_discard_funding: expr, $counterparty_node_ids: expr, $channel_capacity: expr) => {
1476-
$crate::ln::functional_test_utils::check_closed_event(&$node, $events, $reason,
1476+
$crate::ln::functional_test_utils::check_closed_event(&$node, $events, $reason,
14771477
$is_check_discard_funding, &$counterparty_node_ids, $channel_capacity);
14781478
}
14791479
}

0 commit comments

Comments
 (0)