Skip to content

Commit 4217ec3

Browse files
committed
Use the new monitor persistence flow for funding_created handling
Building on the previous commits, this finishes our transition to doing all message-sending in the monitor update completion pipeline, unifying our immediate- and async- `ChannelMonitor` update and persistence flows.
1 parent c9b53c5 commit 4217ec3

File tree

3 files changed

+41
-49
lines changed

3 files changed

+41
-49
lines changed

lightning/src/ln/channel.rs

+8-5
Original file line numberDiff line numberDiff line change
@@ -2234,9 +2234,9 @@ impl<Signer: Sign> Channel<Signer> {
22342234

22352235
pub fn funding_created<SP: Deref, L: Deref>(
22362236
&mut self, msg: &msgs::FundingCreated, best_block: BestBlock, signer_provider: &SP, logger: &L
2237-
) -> Result<(msgs::FundingSigned, ChannelMonitor<<SP::Target as SignerProvider>::Signer>, Option<msgs::ChannelReady>), ChannelError>
2237+
) -> Result<(msgs::FundingSigned, ChannelMonitor<Signer>), ChannelError>
22382238
where
2239-
SP::Target: SignerProvider,
2239+
SP::Target: SignerProvider<Signer=Signer>,
22402240
L::Target: Logger
22412241
{
22422242
if self.is_outbound() {
@@ -2312,10 +2312,13 @@ impl<Signer: Sign> Channel<Signer> {
23122312

23132313
log_info!(logger, "Generated funding_signed for peer for channel {}", log_bytes!(self.channel_id()));
23142314

2315+
let need_channel_ready = self.check_get_channel_ready(0).is_some();
2316+
self.monitor_updating_paused(false, false, need_channel_ready, Vec::new(), Vec::new(), Vec::new());
2317+
23152318
Ok((msgs::FundingSigned {
23162319
channel_id: self.channel_id,
23172320
signature
2318-
}, channel_monitor, self.check_get_channel_ready(0)))
2321+
}, channel_monitor))
23192322
}
23202323

23212324
/// Handles a funding_signed message from the remote end.
@@ -3707,7 +3710,7 @@ impl<Signer: Sign> Channel<Signer> {
37073710
///
37083711
/// [`chain::Watch`]: crate::chain::Watch
37093712
/// [`ChannelMonitorUpdateStatus::InProgress`]: crate::chain::ChannelMonitorUpdateStatus::InProgress
3710-
pub fn monitor_updating_paused(&mut self, resend_raa: bool, resend_commitment: bool,
3713+
fn monitor_updating_paused(&mut self, resend_raa: bool, resend_commitment: bool,
37113714
resend_channel_ready: bool, mut pending_forwards: Vec<(PendingHTLCInfo, u64)>,
37123715
mut pending_fails: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>,
37133716
mut pending_finalized_claimed_htlcs: Vec<HTLCSource>
@@ -7115,7 +7118,7 @@ mod tests {
71157118
}]};
71167119
let funding_outpoint = OutPoint{ txid: tx.txid(), index: 0 };
71177120
let funding_created_msg = node_a_chan.get_outbound_funding_created(tx.clone(), funding_outpoint, &&logger).unwrap();
7118-
let (funding_signed_msg, _, _) = node_b_chan.funding_created(&funding_created_msg, best_block, &&keys_provider, &&logger).unwrap();
7121+
let (funding_signed_msg, _) = node_b_chan.funding_created(&funding_created_msg, best_block, &&keys_provider, &&logger).unwrap();
71197122

71207123
// Node B --> Node A: funding signed
71217124
let _ = node_a_chan.funding_signed(&funding_signed_msg, best_block, &&keys_provider, &&logger);

lightning/src/ln/channelmanager.rs

+30-41
Original file line numberDiff line numberDiff line change
@@ -4241,55 +4241,27 @@ where
42414241
}
42424242

42434243
fn internal_funding_created(&self, counterparty_node_id: &PublicKey, msg: &msgs::FundingCreated) -> Result<(), MsgHandleErrInternal> {
4244+
let best_block = *self.best_block.read().unwrap();
4245+
42444246
let per_peer_state = self.per_peer_state.read().unwrap();
42454247
let peer_state_mutex_opt = per_peer_state.get(counterparty_node_id);
42464248
if let None = peer_state_mutex_opt {
42474249
return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id), msg.temporary_channel_id))
42484250
}
4249-
let ((funding_msg, monitor, mut channel_ready), mut chan) = {
4250-
let best_block = *self.best_block.read().unwrap();
4251-
let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap();
4252-
let peer_state = &mut *peer_state_lock;
4251+
4252+
let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap();
4253+
let peer_state = &mut *peer_state_lock;
4254+
let ((funding_msg, monitor), chan) =
42534255
match peer_state.channel_by_id.entry(msg.temporary_channel_id) {
42544256
hash_map::Entry::Occupied(mut chan) => {
42554257
(try_chan_entry!(self, chan.get_mut().funding_created(msg, best_block, &self.signer_provider, &self.logger), chan), chan.remove())
42564258
},
42574259
hash_map::Entry::Vacant(_) => 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))
4258-
}
4259-
};
4260-
// Because we have exclusive ownership of the channel here we can release the peer_state
4261-
// lock before watch_channel
4262-
match self.chain_monitor.watch_channel(monitor.get_funding_txo().0, monitor) {
4263-
ChannelMonitorUpdateStatus::Completed => {},
4264-
ChannelMonitorUpdateStatus::PermanentFailure => {
4265-
// Note that we reply with the new channel_id in error messages if we gave up on the
4266-
// channel, not the temporary_channel_id. This is compatible with ourselves, but the
4267-
// spec is somewhat ambiguous here. Not a huge deal since we'll send error messages for
4268-
// any messages referencing a previously-closed channel anyway.
4269-
// We do not propagate the monitor update to the user as it would be for a monitor
4270-
// that we didn't manage to store (and that we don't care about - we don't respond
4271-
// with the funding_signed so the channel can never go on chain).
4272-
let (_monitor_update, failed_htlcs) = chan.force_shutdown(false);
4273-
assert!(failed_htlcs.is_empty());
4274-
return Err(MsgHandleErrInternal::send_err_msg_no_close("ChannelMonitor storage failure".to_owned(), funding_msg.channel_id));
4275-
},
4276-
ChannelMonitorUpdateStatus::InProgress => {
4277-
// There's no problem signing a counterparty's funding transaction if our monitor
4278-
// hasn't persisted to disk yet - we can't lose money on a transaction that we haven't
4279-
// accepted payment from yet. We do, however, need to wait to send our channel_ready
4280-
// until we have persisted our monitor.
4281-
chan.monitor_updating_paused(false, false, channel_ready.is_some(), Vec::new(), Vec::new(), Vec::new());
4282-
channel_ready = None; // Don't send the channel_ready now
4283-
},
4284-
}
4285-
// It's safe to unwrap as we've held the `per_peer_state` read lock since checking that the
4286-
// peer exists, despite the inner PeerState potentially having no channels after removing
4287-
// the channel above.
4288-
let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap();
4289-
let peer_state = &mut *peer_state_lock;
4260+
};
4261+
42904262
match peer_state.channel_by_id.entry(funding_msg.channel_id) {
42914263
hash_map::Entry::Occupied(_) => {
4292-
return Err(MsgHandleErrInternal::send_err_msg_no_close("Already had channel with the new channel_id".to_owned(), funding_msg.channel_id))
4264+
Err(MsgHandleErrInternal::send_err_msg_no_close("Already had channel with the new channel_id".to_owned(), funding_msg.channel_id))
42934265
},
42944266
hash_map::Entry::Vacant(e) => {
42954267
let mut id_to_peer = self.id_to_peer.lock().unwrap();
@@ -4303,17 +4275,34 @@ where
43034275
i_e.insert(chan.get_counterparty_node_id());
43044276
}
43054277
}
4278+
4279+
// There's no problem signing a counterparty's funding transaction if our monitor
4280+
// hasn't persisted to disk yet - we can't lose money on a transaction that we haven't
4281+
// accepted payment from yet. We do, however, need to wait to send our channel_ready
4282+
// until we have persisted our monitor.
43064283
peer_state.pending_msg_events.push(events::MessageSendEvent::SendFundingSigned {
43074284
node_id: counterparty_node_id.clone(),
43084285
msg: funding_msg,
43094286
});
4310-
if let Some(msg) = channel_ready {
4311-
send_channel_ready!(self, peer_state.pending_msg_events, chan, msg);
4287+
4288+
let monitor_res = self.chain_monitor.watch_channel(monitor.get_funding_txo().0, monitor);
4289+
4290+
let chan = e.insert(chan);
4291+
let mut res = handle_new_monitor_update!(self, monitor_res, 0, peer_state_lock, peer_state, chan, MANUALLY_REMOVING, 42);
4292+
4293+
// Note that we reply with the new channel_id in error messages if we gave up on the
4294+
// channel, not the temporary_channel_id. This is compatible with ourselves, but the
4295+
// spec is somewhat ambiguous here. Not a huge deal since we'll send error messages for
4296+
// any messages referencing a previously-closed channel anyway.
4297+
// We do not propagate the monitor update to the user as it would be for a monitor
4298+
// that we didn't manage to store (and that we don't care about - we don't respond
4299+
// with the funding_signed so the channel can never go on chain).
4300+
if let Err(MsgHandleErrInternal { shutdown_finish: Some((res, _)), .. }) = &mut res {
4301+
res.0 = None;
43124302
}
4313-
e.insert(chan);
4303+
res
43144304
}
43154305
}
4316-
Ok(())
43174306
}
43184307

43194308
fn internal_funding_signed(&self, counterparty_node_id: &PublicKey, msg: &msgs::FundingSigned) -> Result<(), MsgHandleErrInternal> {

lightning/src/ln/functional_tests.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -8684,9 +8684,9 @@ fn test_duplicate_chan_id() {
86848684
};
86858685
check_added_monitors!(nodes[0], 0);
86868686
nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created);
8687-
// At this point we'll try to add a duplicate channel monitor, which will be rejected, but
8688-
// still needs to be cleared here.
8689-
check_added_monitors!(nodes[1], 1);
8687+
// At this point we'll look up if the channel_id is present and immediately fail the channel
8688+
// without trying to persist the `ChannelMonitor.
8689+
check_added_monitors!(nodes[1], 0);
86908690

86918691
// ...still, nodes[1] will reject the duplicate channel.
86928692
{

0 commit comments

Comments
 (0)