Skip to content

Commit 4a4cdc1

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #286 from TheBlueMatt/2019-01-monitor-update-fixes
Fix a few monitor-update failure bugs found by new fuzzer
2 parents 4cc7d5d + be8213b commit 4a4cdc1

File tree

3 files changed

+162
-49
lines changed

3 files changed

+162
-49
lines changed

‎src/ln/channel.rs

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2187,23 +2187,15 @@ impl Channel {
21872187
/// commitment update or a revoke_and_ack generation). The messages which were generated from
21882188
/// that original call must *not* have been sent to the remote end, and must instead have been
21892189
/// dropped. They will be regenerated when monitor_updating_restored is called.
2190-
pub fn monitor_update_failed(&mut self, order: RAACommitmentOrder, mut pending_forwards: Vec<(PendingForwardHTLCInfo, u64)>, mut pending_fails: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, raa_first_dropped_cs: bool) {
2190+
pub fn monitor_update_failed(&mut self, order: RAACommitmentOrder, resend_raa: bool, resend_commitment: bool, mut pending_forwards: Vec<(PendingForwardHTLCInfo, u64)>, mut pending_fails: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>) {
21912191
assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, 0);
2192-
match order {
2193-
RAACommitmentOrder::CommitmentFirst => {
2194-
self.monitor_pending_revoke_and_ack = false;
2195-
self.monitor_pending_commitment_signed = true;
2196-
},
2197-
RAACommitmentOrder::RevokeAndACKFirst => {
2198-
self.monitor_pending_revoke_and_ack = true;
2199-
self.monitor_pending_commitment_signed = raa_first_dropped_cs;
2200-
},
2201-
}
2192+
self.monitor_pending_revoke_and_ack = resend_raa;
2193+
self.monitor_pending_commitment_signed = resend_commitment;
2194+
self.monitor_pending_order = Some(order);
22022195
assert!(self.monitor_pending_forwards.is_empty());
22032196
mem::swap(&mut pending_forwards, &mut self.monitor_pending_forwards);
22042197
assert!(self.monitor_pending_failures.is_empty());
22052198
mem::swap(&mut pending_fails, &mut self.monitor_pending_failures);
2206-
self.monitor_pending_order = Some(order);
22072199
self.channel_state |= ChannelState::MonitorUpdateFailed as u32;
22082200
}
22092201

‎src/ln/channelmanager.rs

Lines changed: 46 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,16 @@ impl MsgHandleErrInternal {
161161
}
162162
}
163163
#[inline]
164+
fn ignore_no_close(err: &'static str) -> Self {
165+
Self {
166+
err: HandleError {
167+
err,
168+
action: Some(msgs::ErrorAction::IgnoreError),
169+
},
170+
shutdown_finish: None,
171+
}
172+
}
173+
#[inline]
164174
fn from_no_close(err: msgs::HandleError) -> Self {
165175
Self { err, shutdown_finish: None }
166176
}
@@ -381,7 +391,7 @@ pub struct ChannelDetails {
381391
}
382392

383393
macro_rules! handle_error {
384-
($self: ident, $internal: expr, $their_node_id: expr) => {
394+
($self: ident, $internal: expr) => {
385395
match $internal {
386396
Ok(msg) => Ok(msg),
387397
Err(MsgHandleErrInternal { err, shutdown_finish }) => {
@@ -439,17 +449,10 @@ macro_rules! try_chan_entry {
439449
}
440450

441451
macro_rules! return_monitor_err {
442-
($self: expr, $err: expr, $channel_state: expr, $entry: expr, $action_type: path) => {
443-
return_monitor_err!($self, $err, $channel_state, $entry, $action_type, Vec::new(), Vec::new())
444-
};
445-
($self: expr, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $raa_first_dropped_cs: expr) => {
446-
if $action_type != RAACommitmentOrder::RevokeAndACKFirst { panic!("Bad return_monitor_err call!"); }
447-
return_monitor_err!($self, $err, $channel_state, $entry, $action_type, Vec::new(), Vec::new(), $raa_first_dropped_cs)
452+
($self: ident, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $resend_raa: expr, $resend_commitment: expr) => {
453+
return_monitor_err!($self, $err, $channel_state, $entry, $action_type, $resend_raa, $resend_commitment, Vec::new(), Vec::new())
448454
};
449-
($self: expr, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $failed_forwards: expr, $failed_fails: expr) => {
450-
return_monitor_err!($self, $err, $channel_state, $entry, $action_type, $failed_forwards, $failed_fails, false)
451-
};
452-
($self: expr, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $failed_forwards: expr, $failed_fails: expr, $raa_first_dropped_cs: expr) => {
455+
($self: ident, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $resend_raa: expr, $resend_commitment: expr, $failed_forwards: expr, $failed_fails: expr) => {
453456
match $err {
454457
ChannelMonitorUpdateErr::PermanentFailure => {
455458
let (channel_id, mut chan) = $entry.remove_entry();
@@ -468,7 +471,7 @@ macro_rules! return_monitor_err {
468471
return Err(MsgHandleErrInternal::from_finish_shutdown("ChannelMonitor storage failure", channel_id, chan.force_shutdown(), $self.get_channel_update(&chan).ok()))
469472
},
470473
ChannelMonitorUpdateErr::TemporaryFailure => {
471-
$entry.get_mut().monitor_update_failed($action_type, $failed_forwards, $failed_fails, $raa_first_dropped_cs);
474+
$entry.get_mut().monitor_update_failed($action_type, $resend_raa, $resend_commitment, $failed_forwards, $failed_fails);
472475
return Err(MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore("Failed to update ChannelMonitor"), *$entry.key()));
473476
},
474477
}
@@ -477,7 +480,7 @@ macro_rules! return_monitor_err {
477480

478481
// Does not break in case of TemporaryFailure!
479482
macro_rules! maybe_break_monitor_err {
480-
($self: expr, $err: expr, $channel_state: expr, $entry: expr, $action_type: path) => {
483+
($self: ident, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $resend_raa: expr, $resend_commitment: expr) => {
481484
match $err {
482485
ChannelMonitorUpdateErr::PermanentFailure => {
483486
let (channel_id, mut chan) = $entry.remove_entry();
@@ -487,7 +490,7 @@ macro_rules! maybe_break_monitor_err {
487490
break Err(MsgHandleErrInternal::from_finish_shutdown("ChannelMonitor storage failure", channel_id, chan.force_shutdown(), $self.get_channel_update(&chan).ok()))
488491
},
489492
ChannelMonitorUpdateErr::TemporaryFailure => {
490-
$entry.get_mut().monitor_update_failed($action_type, Vec::new(), Vec::new(), false);
493+
$entry.get_mut().monitor_update_failed($action_type, $resend_raa, $resend_commitment, Vec::new(), Vec::new());
491494
},
492495
}
493496
}
@@ -1018,7 +1021,7 @@ impl ChannelManager {
10181021
} {
10191022
Some((update_add, commitment_signed, chan_monitor)) => {
10201023
if let Err(e) = self.monitor.add_update_monitor(chan_monitor.get_funding_txo().unwrap(), chan_monitor) {
1021-
maybe_break_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::CommitmentFirst);
1024+
maybe_break_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::CommitmentFirst, false, true);
10221025
// Note that MonitorUpdateFailed here indicates (per function docs)
10231026
// that we will resent the commitment update once we unfree monitor
10241027
// updating, so we have to take special care that we don't return
@@ -1044,7 +1047,7 @@ impl ChannelManager {
10441047
return Ok(());
10451048
};
10461049

1047-
match handle_error!(self, err, route.hops.first().unwrap().pubkey) {
1050+
match handle_error!(self, err) {
10481051
Ok(_) => unreachable!(),
10491052
Err(e) => {
10501053
if let Some(msgs::ErrorAction::IgnoreError) = e.action {
@@ -1087,7 +1090,7 @@ impl ChannelManager {
10871090
None => return
10881091
}
10891092
};
1090-
match handle_error!(self, res, chan.get_their_node_id()) {
1093+
match handle_error!(self, res) {
10911094
Ok(funding_msg) => {
10921095
(chan, funding_msg.0, funding_msg.1)
10931096
},
@@ -1962,7 +1965,7 @@ impl ChannelManager {
19621965
let (revoke_and_ack, commitment_signed, closing_signed, chan_monitor) =
19631966
try_chan_entry!(self, chan.get_mut().commitment_signed(&msg, &*self.fee_estimator), channel_state, chan);
19641967
if let Err(e) = self.monitor.add_update_monitor(chan_monitor.get_funding_txo().unwrap(), chan_monitor) {
1965-
return_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::RevokeAndACKFirst, commitment_signed.is_some());
1968+
return_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::RevokeAndACKFirst, true, commitment_signed.is_some());
19661969
//TODO: Rebroadcast closing_signed if present on monitor update restoration
19671970
}
19681971
channel_state.pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK {
@@ -2037,10 +2040,16 @@ impl ChannelManager {
20372040
//TODO: here and below MsgHandleErrInternal, #153 case
20382041
return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!", msg.channel_id));
20392042
}
2043+
let was_frozen_for_monitor = chan.get().is_awaiting_monitor_update();
20402044
let (commitment_update, pending_forwards, pending_failures, closing_signed, chan_monitor) =
20412045
try_chan_entry!(self, chan.get_mut().revoke_and_ack(&msg, &*self.fee_estimator), channel_state, chan);
20422046
if let Err(e) = self.monitor.add_update_monitor(chan_monitor.get_funding_txo().unwrap(), chan_monitor) {
2043-
return_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::CommitmentFirst, pending_forwards, pending_failures);
2047+
if was_frozen_for_monitor {
2048+
assert!(commitment_update.is_none() && closing_signed.is_none() && pending_forwards.is_empty() && pending_failures.is_empty());
2049+
return Err(MsgHandleErrInternal::ignore_no_close("Previous monitor update failure prevented responses to RAA"));
2050+
} else {
2051+
return_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::CommitmentFirst, false, commitment_update.is_some(), pending_forwards, pending_failures);
2052+
}
20442053
}
20452054
if let Some(updates) = commitment_update {
20462055
channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
@@ -2147,7 +2156,7 @@ impl ChannelManager {
21472156
if commitment_update.is_none() {
21482157
order = RAACommitmentOrder::RevokeAndACKFirst;
21492158
}
2150-
return_monitor_err!(self, e, channel_state, chan, order);
2159+
return_monitor_err!(self, e, channel_state, chan, order, revoke_and_ack.is_some(), commitment_update.is_some());
21512160
//TODO: Resend the funding_locked if needed once we get the monitor running again
21522161
}
21532162
}
@@ -2243,7 +2252,7 @@ impl ChannelManager {
22432252
return Ok(())
22442253
};
22452254

2246-
match handle_error!(self, err, their_node_id) {
2255+
match handle_error!(self, err) {
22472256
Ok(_) => unreachable!(),
22482257
Err(e) => {
22492258
if let Some(msgs::ErrorAction::IgnoreError) = e.action {
@@ -2429,82 +2438,82 @@ impl ChannelMessageHandler for ChannelManager {
24292438
//TODO: Handle errors and close channel (or so)
24302439
fn handle_open_channel(&self, their_node_id: &PublicKey, msg: &msgs::OpenChannel) -> Result<(), HandleError> {
24312440
let _ = self.total_consistency_lock.read().unwrap();
2432-
handle_error!(self, self.internal_open_channel(their_node_id, msg), their_node_id)
2441+
handle_error!(self, self.internal_open_channel(their_node_id, msg))
24332442
}
24342443

24352444
fn handle_accept_channel(&self, their_node_id: &PublicKey, msg: &msgs::AcceptChannel) -> Result<(), HandleError> {
24362445
let _ = self.total_consistency_lock.read().unwrap();
2437-
handle_error!(self, self.internal_accept_channel(their_node_id, msg), their_node_id)
2446+
handle_error!(self, self.internal_accept_channel(their_node_id, msg))
24382447
}
24392448

24402449
fn handle_funding_created(&self, their_node_id: &PublicKey, msg: &msgs::FundingCreated) -> Result<(), HandleError> {
24412450
let _ = self.total_consistency_lock.read().unwrap();
2442-
handle_error!(self, self.internal_funding_created(their_node_id, msg), their_node_id)
2451+
handle_error!(self, self.internal_funding_created(their_node_id, msg))
24432452
}
24442453

24452454
fn handle_funding_signed(&self, their_node_id: &PublicKey, msg: &msgs::FundingSigned) -> Result<(), HandleError> {
24462455
let _ = self.total_consistency_lock.read().unwrap();
2447-
handle_error!(self, self.internal_funding_signed(their_node_id, msg), their_node_id)
2456+
handle_error!(self, self.internal_funding_signed(their_node_id, msg))
24482457
}
24492458

24502459
fn handle_funding_locked(&self, their_node_id: &PublicKey, msg: &msgs::FundingLocked) -> Result<(), HandleError> {
24512460
let _ = self.total_consistency_lock.read().unwrap();
2452-
handle_error!(self, self.internal_funding_locked(their_node_id, msg), their_node_id)
2461+
handle_error!(self, self.internal_funding_locked(their_node_id, msg))
24532462
}
24542463

24552464
fn handle_shutdown(&self, their_node_id: &PublicKey, msg: &msgs::Shutdown) -> Result<(), HandleError> {
24562465
let _ = self.total_consistency_lock.read().unwrap();
2457-
handle_error!(self, self.internal_shutdown(their_node_id, msg), their_node_id)
2466+
handle_error!(self, self.internal_shutdown(their_node_id, msg))
24582467
}
24592468

24602469
fn handle_closing_signed(&self, their_node_id: &PublicKey, msg: &msgs::ClosingSigned) -> Result<(), HandleError> {
24612470
let _ = self.total_consistency_lock.read().unwrap();
2462-
handle_error!(self, self.internal_closing_signed(their_node_id, msg), their_node_id)
2471+
handle_error!(self, self.internal_closing_signed(their_node_id, msg))
24632472
}
24642473

24652474
fn handle_update_add_htlc(&self, their_node_id: &PublicKey, msg: &msgs::UpdateAddHTLC) -> Result<(), msgs::HandleError> {
24662475
let _ = self.total_consistency_lock.read().unwrap();
2467-
handle_error!(self, self.internal_update_add_htlc(their_node_id, msg), their_node_id)
2476+
handle_error!(self, self.internal_update_add_htlc(their_node_id, msg))
24682477
}
24692478

24702479
fn handle_update_fulfill_htlc(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFulfillHTLC) -> Result<(), HandleError> {
24712480
let _ = self.total_consistency_lock.read().unwrap();
2472-
handle_error!(self, self.internal_update_fulfill_htlc(their_node_id, msg), their_node_id)
2481+
handle_error!(self, self.internal_update_fulfill_htlc(their_node_id, msg))
24732482
}
24742483

24752484
fn handle_update_fail_htlc(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFailHTLC) -> Result<(), HandleError> {
24762485
let _ = self.total_consistency_lock.read().unwrap();
2477-
handle_error!(self, self.internal_update_fail_htlc(their_node_id, msg), their_node_id)
2486+
handle_error!(self, self.internal_update_fail_htlc(their_node_id, msg))
24782487
}
24792488

24802489
fn handle_update_fail_malformed_htlc(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFailMalformedHTLC) -> Result<(), HandleError> {
24812490
let _ = self.total_consistency_lock.read().unwrap();
2482-
handle_error!(self, self.internal_update_fail_malformed_htlc(their_node_id, msg), their_node_id)
2491+
handle_error!(self, self.internal_update_fail_malformed_htlc(their_node_id, msg))
24832492
}
24842493

24852494
fn handle_commitment_signed(&self, their_node_id: &PublicKey, msg: &msgs::CommitmentSigned) -> Result<(), HandleError> {
24862495
let _ = self.total_consistency_lock.read().unwrap();
2487-
handle_error!(self, self.internal_commitment_signed(their_node_id, msg), their_node_id)
2496+
handle_error!(self, self.internal_commitment_signed(their_node_id, msg))
24882497
}
24892498

24902499
fn handle_revoke_and_ack(&self, their_node_id: &PublicKey, msg: &msgs::RevokeAndACK) -> Result<(), HandleError> {
24912500
let _ = self.total_consistency_lock.read().unwrap();
2492-
handle_error!(self, self.internal_revoke_and_ack(their_node_id, msg), their_node_id)
2501+
handle_error!(self, self.internal_revoke_and_ack(their_node_id, msg))
24932502
}
24942503

24952504
fn handle_update_fee(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFee) -> Result<(), HandleError> {
24962505
let _ = self.total_consistency_lock.read().unwrap();
2497-
handle_error!(self, self.internal_update_fee(their_node_id, msg), their_node_id)
2506+
handle_error!(self, self.internal_update_fee(their_node_id, msg))
24982507
}
24992508

25002509
fn handle_announcement_signatures(&self, their_node_id: &PublicKey, msg: &msgs::AnnouncementSignatures) -> Result<(), HandleError> {
25012510
let _ = self.total_consistency_lock.read().unwrap();
2502-
handle_error!(self, self.internal_announcement_signatures(their_node_id, msg), their_node_id)
2511+
handle_error!(self, self.internal_announcement_signatures(their_node_id, msg))
25032512
}
25042513

25052514
fn handle_channel_reestablish(&self, their_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(), HandleError> {
25062515
let _ = self.total_consistency_lock.read().unwrap();
2507-
handle_error!(self, self.internal_channel_reestablish(their_node_id, msg), their_node_id)
2516+
handle_error!(self, self.internal_channel_reestablish(their_node_id, msg))
25082517
}
25092518

25102519
fn peer_disconnected(&self, their_node_id: &PublicKey, no_connection_possible: bool) {

0 commit comments

Comments
 (0)