Skip to content

Implement the last three (relevant) unimplemented()s in ChannelManager #364

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 29, 2019
Merged

Implement the last three (relevant) unimplemented()s in ChannelManager #364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1e0ca4
Handle monitor update failures during funding on the funder side
TheBlueMatt Jul 26, 2019
b27f788
Allow a counterparty to resend their funding_locked before ours
TheBlueMatt Jul 29, 2019
8ba3529
Handle monitor update failure during funding on the fundee side
TheBlueMatt Jul 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 130 additions & 1 deletion src/ln/chanmon_update_fail_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
use ln::channelmanager::{RAACommitmentOrder, PaymentPreimage, PaymentHash};
use ln::channelmonitor::ChannelMonitorUpdateErr;
use ln::msgs;
use ln::msgs::{ChannelMessageHandler, LocalFeatures};
use ln::msgs::{ChannelMessageHandler, LocalFeatures, RoutingMessageHandler};
use util::events::{Event, EventsProvider, MessageSendEvent, MessageSendEventsProvider};
use util::errors::APIError;

Expand Down Expand Up @@ -1553,3 +1553,132 @@ fn monitor_update_claim_fail_no_response() {

claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
}

// Note that restore_between_fails with !fail_on_generate is useless
// Also note that !fail_on_generate && !fail_on_signed is useless
// Finally, note that !fail_on_signed is not possible with fail_on_generate && !restore_between_fails
// confirm_a_first and restore_b_before_conf are wholly unrelated to earlier bools and
// restore_b_before_conf has no meaning if !confirm_a_first
fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails: bool, fail_on_signed: bool, confirm_a_first: bool, restore_b_before_conf: bool) {
// Test that if the monitor update generated by funding_transaction_generated fails we continue
// the channel setup happily after the update is restored.
let mut nodes = create_network(2, &[None, None]);

nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 43).unwrap();
nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), LocalFeatures::new(), &get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id())).unwrap();
nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), LocalFeatures::new(), &get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id())).unwrap();

let (temporary_channel_id, funding_tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 43);

if fail_on_generate {
*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
}
nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output);
check_added_monitors!(nodes[0], 1);

*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id())).unwrap();
check_added_monitors!(nodes[1], 1);

if restore_between_fails {
assert!(fail_on_generate);
*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
nodes[0].node.test_restore_channel_monitor();
check_added_monitors!(nodes[0], 1);
assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
}

if fail_on_signed {
*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
} else {
assert!(restore_between_fails || !fail_on_generate); // We can't switch to good now (there's no monitor update)
assert!(fail_on_generate); // Somebody has to fail
}
let funding_signed_res = nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()));
if fail_on_signed || !restore_between_fails {
if let msgs::HandleError { err, action: Some(msgs::ErrorAction::IgnoreError) } = funding_signed_res.unwrap_err() {
if fail_on_generate && !restore_between_fails {
assert_eq!(err, "Previous monitor update failure prevented funding_signed from allowing funding broadcast");
check_added_monitors!(nodes[0], 0);
} else {
assert_eq!(err, "Failed to update ChannelMonitor");
check_added_monitors!(nodes[0], 1);
}
} else { panic!(); }

assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
nodes[0].node.test_restore_channel_monitor();
} else {
funding_signed_res.unwrap();
}

check_added_monitors!(nodes[0], 1);

let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::FundingBroadcastSafe { ref funding_txo, user_channel_id } => {
assert_eq!(user_channel_id, 43);
assert_eq!(*funding_txo, funding_output);
},
_ => panic!("Unexpected event"),
};

if confirm_a_first {
confirm_transaction(&nodes[0].chain_monitor, &funding_tx, funding_tx.version);
nodes[1].node.handle_funding_locked(&nodes[0].node.get_our_node_id(), &get_event_msg!(nodes[0], MessageSendEvent::SendFundingLocked, nodes[1].node.get_our_node_id())).unwrap();
} else {
assert!(!restore_b_before_conf);
confirm_transaction(&nodes[1].chain_monitor, &funding_tx, funding_tx.version);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
}

// Make sure nodes[1] isn't stupid enough to re-send the FundingLocked on reconnect
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
reconnect_nodes(&nodes[0], &nodes[1], (false, confirm_a_first), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

if !restore_b_before_conf {
confirm_transaction(&nodes[1].chain_monitor, &funding_tx, funding_tx.version);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
}

*nodes[1].chan_monitor.update_ret.lock().unwrap() = Ok(());
nodes[1].node.test_restore_channel_monitor();
check_added_monitors!(nodes[1], 1);

let (channel_id, (announcement, as_update, bs_update)) = if !confirm_a_first {
nodes[0].node.handle_funding_locked(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingLocked, nodes[0].node.get_our_node_id())).unwrap();

confirm_transaction(&nodes[0].chain_monitor, &funding_tx, funding_tx.version);
let (funding_locked, channel_id) = create_chan_between_nodes_with_value_confirm_second(&nodes[1], &nodes[0]);
(channel_id, create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &funding_locked))
} else {
if restore_b_before_conf {
confirm_transaction(&nodes[1].chain_monitor, &funding_tx, funding_tx.version);
}
let (funding_locked, channel_id) = create_chan_between_nodes_with_value_confirm_second(&nodes[0], &nodes[1]);
(channel_id, create_chan_between_nodes_with_value_b(&nodes[1], &nodes[0], &funding_locked))
};
for node in nodes.iter() {
assert!(node.router.handle_channel_announcement(&announcement).unwrap());
node.router.handle_channel_update(&as_update).unwrap();
node.router.handle_channel_update(&bs_update).unwrap();
}

send_payment(&nodes[0], &[&nodes[1]], 8000000);
close_channel(&nodes[0], &nodes[1], &channel_id, funding_tx, true);
}

#[test]
fn during_funding_monitor_fail() {
do_during_funding_monitor_fail(false, false, true, true, true);
do_during_funding_monitor_fail(true, false, true, false, false);
do_during_funding_monitor_fail(true, true, true, true, false);
do_during_funding_monitor_fail(true, true, false, false, false);
}
82 changes: 61 additions & 21 deletions src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,9 +181,9 @@ enum ChannelState {
/// "disconnected" and no updates are allowed until after we've done a channel_reestablish
/// dance.
PeerDisconnected = (1 << 7),
/// Flag which is set on ChannelFunded and FundingSent indicating the user has told us they
/// failed to update our ChannelMonitor somewhere and we should pause sending any outbound
/// messages until they've managed to do so.
/// Flag which is set on ChannelFunded, FundingCreated, and FundingSent indicating the user has
/// told us they failed to update our ChannelMonitor somewhere and we should pause sending any
/// outbound messages until they've managed to do so.
MonitorUpdateFailed = (1 << 8),
/// Flag which implies that we have sent a commitment_signed but are awaiting the responding
/// revoke_and_ack message. During this time period, we can't generate new commitment_signed
Expand Down Expand Up @@ -248,6 +248,7 @@ pub(super) struct Channel {
/// send it first.
resend_order: RAACommitmentOrder,

monitor_pending_funding_locked: bool,
monitor_pending_revoke_and_ack: bool,
monitor_pending_commitment_signed: bool,
monitor_pending_forwards: Vec<(PendingForwardHTLCInfo, u64)>,
Expand Down Expand Up @@ -457,6 +458,7 @@ impl Channel {

resend_order: RAACommitmentOrder::CommitmentFirst,

monitor_pending_funding_locked: false,
monitor_pending_revoke_and_ack: false,
monitor_pending_commitment_signed: false,
monitor_pending_forwards: Vec::new(),
Expand Down Expand Up @@ -672,6 +674,7 @@ impl Channel {

resend_order: RAACommitmentOrder::CommitmentFirst,

monitor_pending_funding_locked: false,
monitor_pending_revoke_and_ack: false,
monitor_pending_commitment_signed: false,
monitor_pending_forwards: Vec::new(),
Expand Down Expand Up @@ -1540,7 +1543,7 @@ impl Channel {
if !self.channel_outbound {
return Err(ChannelError::Close("Received funding_signed for an inbound channel?"));
}
if self.channel_state != ChannelState::FundingCreated as u32 {
if self.channel_state & !(ChannelState::MonitorUpdateFailed as u32) != ChannelState::FundingCreated as u32 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO MonitorUpdateFailed and PeerDisconnected shouldn't pollute ChannelState as they are private state to the node and not public state we expect in synchronization with the othe peer state machine. MonitorUpdateFailed forerun any other states and we stop protocol execution when we get one. It would make easier to reason on if at some point we refactor it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was kind of the original discussion when monitor update failure was first implemented - its either done as a ChannelManager thing where the Channel isn't touched and the ChannelManager is responsible for message retransmission, or its done at the Channel level (which is way lighter-weight, though much ore complicated) and we have to handle it everywhere. While you may have a point with PeerDisconnected, handling it on the Channel level makes it much easier to avoid DoS issues where buffers in ChannelManager for message retransmission aren't bounded. Ultimately this means a blowup in state complexity, but it does afford us a few really nice features wrt being able to take some steps forward even though a channel is frozen.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point even if I think you can bound per channel_id at the ChannelManager level to avoid DoS issues. Okay, another advantage I see tracking in Channel is you may have a per channel monitor failure, if you decide to send your updates filtered by the funding_outpoint to different set of monitors. Being able to take some steps forward even though a channel is frozen, let me think that in fact you only want to commit updates for CS/RAA, for the funding_signed case not being able to update immediately is this a real risk ? First commitment transaction doesn't have any time-sensitive output and remote peer can't broadcast a lower state

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think its ever much of a real risk, more of a nice-to-have. On the update_fulfill_htlc side its really nice to have, though, as it allows us to mark an HTLC fulfilled/claim upstream even if the monitor updating has failed.

return Err(ChannelError::Close("Received funding_signed in strange state!"));
}
if self.channel_monitor.get_min_seen_secret() != (1 << 48) ||
Expand All @@ -1561,10 +1564,14 @@ impl Channel {
self.sign_commitment_transaction(&mut local_initial_commitment_tx, &msg.signature);
self.channel_monitor.provide_latest_local_commitment_tx_info(local_initial_commitment_tx.clone(), local_keys, self.feerate_per_kw, Vec::new());
self.last_local_commitment_txn = vec![local_initial_commitment_tx];
self.channel_state = ChannelState::FundingSent as u32;
self.channel_state = ChannelState::FundingSent as u32 | (self.channel_state & (ChannelState::MonitorUpdateFailed as u32));
self.cur_local_commitment_transaction_number -= 1;

Ok(self.channel_monitor.clone())
if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
Ok(self.channel_monitor.clone())
} else {
Err(ChannelError::Ignore("Previous monitor update failure prevented funding_signed from allowing funding broadcast"))
}
}

pub fn funding_locked(&mut self, msg: &msgs::FundingLocked) -> Result<(), ChannelError> {
Expand All @@ -1579,10 +1586,13 @@ impl Channel {
} else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurFundingLocked as u32) {
self.channel_state = ChannelState::ChannelFunded as u32 | (self.channel_state & MULTI_STATE_FLAGS);
self.channel_update_count += 1;
} else if self.channel_state & (ChannelState::ChannelFunded as u32) != 0 &&
// Note that funding_signed/funding_created will have decremented both by 1!
self.cur_local_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1 &&
self.cur_remote_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1 {
} else if (self.channel_state & (ChannelState::ChannelFunded as u32) != 0 &&
// Note that funding_signed/funding_created will have decremented both by 1!
self.cur_local_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1 &&
self.cur_remote_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1) ||
// If we reconnected before sending our funding locked they may still resend theirs:
(self.channel_state & (ChannelState::FundingSent as u32 | ChannelState::TheirFundingLocked as u32) ==
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit : maybe this should go in its own commit given that's not monitor update related

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm is this covered by new tests ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new tests do, indeed, fail in this case. But I went ahead and pulled it into its own commit.

(ChannelState::FundingSent as u32 | ChannelState::TheirFundingLocked as u32)) {
if self.their_cur_commitment_point != Some(msg.next_per_commitment_point) {
return Err(ChannelError::Close("Peer sent a reconnect funding_locked with a different point"));
}
Expand Down Expand Up @@ -2345,10 +2355,29 @@ impl Channel {
/// Indicates that the latest ChannelMonitor update has been committed by the client
/// successfully and we should restore normal operation. Returns messages which should be sent
/// to the remote side.
pub fn monitor_updating_restored(&mut self) -> (Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, RAACommitmentOrder, Vec<(PendingForwardHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>) {
pub fn monitor_updating_restored(&mut self) -> (Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, RAACommitmentOrder, Vec<(PendingForwardHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, bool, Option<msgs::FundingLocked>) {
assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, ChannelState::MonitorUpdateFailed as u32);
self.channel_state &= !(ChannelState::MonitorUpdateFailed as u32);

let needs_broadcast_safe = self.channel_state & (ChannelState::FundingSent as u32) != 0 && self.channel_outbound;

// Because we will never generate a FundingBroadcastSafe event when we're in
// MonitorUpdateFailed, if we assume the user only broadcast the funding transaction when
// they received the FundingBroadcastSafe event, we can only ever hit
// monitor_pending_funding_locked when we're an inbound channel which failed to persist the
// monitor on funding_created, and we even got the funding transaction confirmed before the
// monitor was persisted.
let funding_locked = if self.monitor_pending_funding_locked {
assert!(!self.channel_outbound, "Funding transaction broadcast without FundingBroadcastSafe!");
self.monitor_pending_funding_locked = false;
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
Some(msgs::FundingLocked {
channel_id: self.channel_id(),
next_per_commitment_point: next_per_commitment_point,
})
} else { None };

let mut forwards = Vec::new();
mem::swap(&mut forwards, &mut self.monitor_pending_forwards);
let mut failures = Vec::new();
Expand All @@ -2357,7 +2386,7 @@ impl Channel {
if self.channel_state & (ChannelState::PeerDisconnected as u32) != 0 {
self.monitor_pending_revoke_and_ack = false;
self.monitor_pending_commitment_signed = false;
return (None, None, RAACommitmentOrder::RevokeAndACKFirst, forwards, failures);
return (None, None, RAACommitmentOrder::RevokeAndACKFirst, forwards, failures, needs_broadcast_safe, funding_locked);
}

let raa = if self.monitor_pending_revoke_and_ack {
Expand All @@ -2370,11 +2399,12 @@ impl Channel {
self.monitor_pending_revoke_and_ack = false;
self.monitor_pending_commitment_signed = false;
let order = self.resend_order.clone();
log_trace!(self, "Restored monitor updating resulting in {} commitment update and {} RAA, with {} first",
log_trace!(self, "Restored monitor updating resulting in {}{} commitment update and {} RAA, with {} first",
if needs_broadcast_safe { "a funding broadcast safe, " } else { "" },
if commitment_update.is_some() { "a" } else { "no" },
if raa.is_some() { "an" } else { "no" },
match order { RAACommitmentOrder::CommitmentFirst => "commitment", RAACommitmentOrder::RevokeAndACKFirst => "RAA"});
(raa, commitment_update, order, forwards, failures)
(raa, commitment_update, order, forwards, failures, needs_broadcast_safe, funding_locked)
}

pub fn update_fee(&mut self, fee_estimator: &FeeEstimator, msg: &msgs::UpdateFee) -> Result<(), ChannelError> {
Expand Down Expand Up @@ -2484,7 +2514,9 @@ impl Channel {
} else { None };

if self.channel_state & (ChannelState::FundingSent as u32) == ChannelState::FundingSent as u32 {
if self.channel_state & ChannelState::OurFundingLocked as u32 == 0 {
// If we're waiting on a monitor update, we shouldn't re-send any funding_locked's.
if self.channel_state & (ChannelState::OurFundingLocked as u32) == 0 ||
self.channel_state & (ChannelState::MonitorUpdateFailed as u32) != 0 {
if msg.next_remote_commitment_number != 0 {
return Err(ChannelError::Close("Peer claimed they saw a revoke_and_ack but we haven't sent funding_locked yet"));
}
Expand Down Expand Up @@ -2975,12 +3007,17 @@ impl Channel {
//they can by sending two revoke_and_acks back-to-back, but not really). This appears to be
//a protocol oversight, but I assume I'm just missing something.
if need_commitment_update {
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
return Ok(Some(msgs::FundingLocked {
channel_id: self.channel_id,
next_per_commitment_point: next_per_commitment_point,
}));
if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
return Ok(Some(msgs::FundingLocked {
channel_id: self.channel_id,
next_per_commitment_point: next_per_commitment_point,
}));
} else {
self.monitor_pending_funding_locked = true;
return Ok(None);
}
}
}
}
Expand Down Expand Up @@ -3699,6 +3736,7 @@ impl Writeable for Channel {
RAACommitmentOrder::RevokeAndACKFirst => 1u8.write(writer)?,
}

self.monitor_pending_funding_locked.write(writer)?;
self.monitor_pending_revoke_and_ack.write(writer)?;
self.monitor_pending_commitment_signed.write(writer)?;

Expand Down Expand Up @@ -3866,6 +3904,7 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
_ => return Err(DecodeError::InvalidValue),
};

let monitor_pending_funding_locked = Readable::read(reader)?;
let monitor_pending_revoke_and_ack = Readable::read(reader)?;
let monitor_pending_commitment_signed = Readable::read(reader)?;

Expand Down Expand Up @@ -3962,6 +4001,7 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {

resend_order,

monitor_pending_funding_locked,
monitor_pending_revoke_and_ack,
monitor_pending_commitment_signed,
monitor_pending_forwards,
Expand Down
Loading