Consider channels "live" even if they are awaiting a monitor update#954

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
TheBlueMatt merged 1 commit into from
Jul 1, 2021
Merged

Consider channels "live" even if they are awaiting a monitor update #954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
208 changes: 120 additions & 88 deletions lightning/src/ln/chanmon_update_fail_tests.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -906,8 +906,8 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
check_added_monitors!(nodes[1], 1);

// Attempt to forward a third payment but fail due to the second channel being unavailable
// for forwarding.
// Forward a third payment which will also be added to the holding cell, despite the channel
// being paused waiting a monitor update.
let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[2]);
{
let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
Expand All@@ -922,39 +922,11 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
commitment_signed_dance!(nodes[1], nodes[0], send_event.commitment_msg, false, true);
check_added_monitors!(nodes[1], 0);

let mut events_2 = nodes[1].node.get_and_clear_pending_msg_events();

@jkczyzjkczyzJun 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a TL;DR to summarize the changes to this test to help in review?

@TheBlueMattTheBlueMattJun 30, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Because it is no longer the case that receiving an HTLC while we're pending a monitor update restoration causes the HTLC to fail, code which relied on that behavior broke. eg the first code hunk which was removed explicitly delivered an HTLC and expected it to fail, which no longer happens.

The big if-block diff hunk is because the two cases have converged somewhat - we now always have a payment in node[1]'s outbound holding cell.

assert_eq!(events_2.len(), 1);
match events_2.remove(0) {
MessageSendEvent::UpdateHTLCs { node_id, updates } => {
assert_eq!(node_id, nodes[0].node.get_our_node_id());
assert!(updates.update_fulfill_htlcs.is_empty());
assert_eq!(updates.update_fail_htlcs.len(), 1);
assert!(updates.update_fail_malformed_htlcs.is_empty());
assert!(updates.update_add_htlcs.is_empty());
assert!(updates.update_fee.is_none());

nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
commitment_signed_dance!(nodes[0], nodes[1], updates.commitment_signed, false, true);

let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
assert_eq!(msg_events.len(), 1);
match msg_events[0] {
MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
assert_eq!(msg.contents.short_channel_id, chan_2.0.contents.short_channel_id);
assert_eq!(msg.contents.flags & 2, 2); // temp disabled
},
_ => panic!("Unexpected event"),
}

let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
assert_eq!(payment_hash, payment_hash_3);
assert!(!rejected_by_dest);
} else { panic!("Unexpected event!"); }
},
_ => panic!("Unexpected event type!"),
};
// Call forward_pending_htlcs and check that the new HTLC was simply added to the holding cell
// and not forwarded.
expect_pending_htlcs_forwardable!(nodes[1]);
check_added_monitors!(nodes[1], 0);
assert!(nodes[1].node.get_and_clear_pending_events().is_empty());

let (payment_preimage_4, payment_hash_4) = if test_ignore_second_cs {
// Try to route another payment backwards from 2 to make sure 1 holds off on responding
Expand All@@ -971,7 +943,6 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
(Some(payment_preimage_4), Some(payment_hash_4))
} else { (None, None) };

Expand DownExpand Up@@ -1021,14 +992,10 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &messages_a.0);
commitment_signed_dance!(nodes[0], nodes[1], messages_a.1, false);
let events_4 = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events_4.len(), 1);
if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events_4[0] {
assert_eq!(payment_hash, payment_hash_1);
assert!(rejected_by_dest);
} else { panic!("Unexpected event!"); }
expect_payment_failed!(nodes[0], payment_hash_1, true);

nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &send_event_b.msgs[0]);
let as_cs;
if test_ignore_second_cs {
nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
check_added_monitors!(nodes[2], 1);
Expand All@@ -1044,40 +1011,83 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_revoke_and_ack);
check_added_monitors!(nodes[1], 1);
let as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
assert!(as_cs.update_add_htlcs.is_empty());
assert!(as_cs.update_fail_htlcs.is_empty());
assert!(as_cs.update_fail_malformed_htlcs.is_empty());
assert!(as_cs.update_fulfill_htlcs.is_empty());
assert!(as_cs.update_fee.is_none());
as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_cs.commitment_signed);
check_added_monitors!(nodes[1], 1);
let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
} else {
nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
check_added_monitors!(nodes[2], 1);
let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
check_added_monitors!(nodes[2], 1);
assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());
let bs_revoke_and_commit = nodes[2].node.get_and_clear_pending_msg_events();
assert_eq!(bs_revoke_and_commit.len(), 2);
match bs_revoke_and_commit[0] {
MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {
assert_eq!(*node_id, nodes[1].node.get_our_node_id());
nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &msg);
check_added_monitors!(nodes[1], 1);
},
_ => panic!("Unexpected event"),
}

nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
check_added_monitors!(nodes[1], 1);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
} else {
commitment_signed_dance!(nodes[2], nodes[1], send_event_b.commitment_msg, false);
as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

match bs_revoke_and_commit[1] {
MessageSendEvent::UpdateHTLCs { ref node_id, ref updates } => {
assert_eq!(*node_id, nodes[1].node.get_our_node_id());
assert!(updates.update_add_htlcs.is_empty());
assert!(updates.update_fail_htlcs.is_empty());
assert!(updates.update_fail_malformed_htlcs.is_empty());
assert!(updates.update_fulfill_htlcs.is_empty());
assert!(updates.update_fee.is_none());
nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &updates.commitment_signed);
check_added_monitors!(nodes[1], 1);
},
_ => panic!("Unexpected event"),
}
}

assert_eq!(as_cs.update_add_htlcs.len(), 1);
assert!(as_cs.update_fail_htlcs.is_empty());
assert!(as_cs.update_fail_malformed_htlcs.is_empty());
assert!(as_cs.update_fulfill_htlcs.is_empty());
assert!(as_cs.update_fee.is_none());
let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());


nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &as_cs.update_add_htlcs[0]);
nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
check_added_monitors!(nodes[2], 1);
let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
check_added_monitors!(nodes[2], 1);
let bs_second_cs = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());

nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
check_added_monitors!(nodes[1], 1);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_second_cs.commitment_signed);
check_added_monitors!(nodes[1], 1);
let as_second_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_second_raa);
check_added_monitors!(nodes[2], 1);
assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());

expect_pending_htlcs_forwardable!(nodes[2]);

let events_6 = nodes[2].node.get_and_clear_pending_events();
assert_eq!(events_6.len(), 1);
assert_eq!(events_6.len(), 2);
match events_6[0] {
Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_2); },
_ => panic!("Unexpected event"),
};
match events_6[1] {
Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_3); },
_ => panic!("Unexpected event"),
};

if test_ignore_second_cs {
expect_pending_htlcs_forwardable!(nodes[1]);
Expand DownExpand Up@@ -1612,9 +1622,9 @@ fn first_message_on_recv_ordering() {
fn test_monitor_update_fail_claim() {
// Basic test for monitor update failures when processing claim_funds calls.
// We set up a simple 3-node network, sending a payment from A to B and failing B's monitor
// update to claim the payment. We then send a payment C->B->A, making the forward of this
// payment from B to A fail due to the paused channel. Finally, we restore the channel monitor
// updating and claim the payment on B.
// update to claim the payment. We then send two payments C->B->A, which are held at B.
// Finally, we restore the channel monitor updating and claim the payment on B, forwarding
// the payments from C onwards to A.
let chanmon_cfgs = create_chanmon_cfgs(3);
let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
Expand All@@ -1630,12 +1640,19 @@ fn test_monitor_update_fail_claim() {

*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::TemporaryFailure));
assert!(nodes[1].node.claim_funds(payment_preimage_1));
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);

@jkczyzjkczyzJun 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there any way to assert the state of the channel rather than examining the log?

@TheBlueMattTheBlueMattJun 30, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

This line was only moved, not created. the log assert is nice IMO as it checks the specific error, not just that one happened, though the observable state is still checked in that no new message was generated.

check_added_monitors!(nodes[1], 1);

// Note that at this point there is a pending commitment transaction update for A being held by
// B. Even when we go to send the payment from C through B to A, B will not update this
// already-signed commitment transaction and will instead wait for it to resolve before
// forwarding the payment onwards.

let (_, payment_hash_2, payment_secret_2) = get_payment_preimage_hash!(nodes[0]);
let route;
{
let net_graph_msg_handler = &nodes[2].net_graph_msg_handler;
let route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1000000, TEST_FINAL_CLTV, &logger).unwrap();
route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1_000_000, TEST_FINAL_CLTV, &logger).unwrap();
nodes[2].node.send_payment(&route, payment_hash_2, &Some(payment_secret_2)).unwrap();
check_added_monitors!(nodes[2], 1);
}
Expand All@@ -1650,29 +1667,19 @@ fn test_monitor_update_fail_claim() {
nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
let events = nodes[1].node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 0);
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

let bs_fail_update = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
nodes[2].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_update.update_fail_htlcs[0]);
commitment_signed_dance!(nodes[2], nodes[1], bs_fail_update.commitment_signed, false, true);

let msg_events = nodes[2].node.get_and_clear_pending_msg_events();
assert_eq!(msg_events.len(), 1);
match msg_events[0] {
MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
assert_eq!(msg.contents.short_channel_id, chan_1.0.contents.short_channel_id);
assert_eq!(msg.contents.flags & 2, 2); // temp disabled
},
_ => panic!("Unexpected event"),
}
let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[0]);
nodes[2].node.send_payment(&route, payment_hash_3, &Some(payment_secret_3)).unwrap();
check_added_monitors!(nodes[2], 1);

let events = nodes[2].node.get_and_clear_pending_events();
let mut events = nodes[2].node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 1);
if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
assert_eq!(payment_hash, payment_hash_2);
assert!(!rejected_by_dest);
} else { panic!("Unexpected event!"); }
let payment_event = SendEvent::from_event(events.pop().unwrap());
nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
let events = nodes[1].node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 0);
commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

// Now restore monitor updating on the 0<->1 channel and claim the funds on B.
let (outpoint, latest_update) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_1.2).unwrap().clone();
Expand All@@ -1682,12 +1689,37 @@ fn test_monitor_update_fail_claim() {
let bs_fulfill_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_fulfill_update.update_fulfill_htlcs[0]);
commitment_signed_dance!(nodes[0], nodes[1], bs_fulfill_update.commitment_signed, false);
expect_payment_sent!(nodes[0], payment_preimage_1);

// Get the payment forwards, note that they were batched into one commitment update.
expect_pending_htlcs_forwardable!(nodes[1]);
check_added_monitors!(nodes[1], 1);
let bs_forward_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[0]);
nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[1]);
commitment_signed_dance!(nodes[0], nodes[1], bs_forward_update.commitment_signed, false);
expect_pending_htlcs_forwardable!(nodes[0]);

let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
if let Event::PaymentSent { payment_preimage, .. } = events[0] {
assert_eq!(payment_preimage, payment_preimage_1);
} else { panic!("Unexpected event!"); }
assert_eq!(events.len(), 2);
match events[0] {
Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
assert_eq!(payment_hash_2, *payment_hash);
assert!(payment_preimage.is_none());
assert_eq!(payment_secret_2, *payment_secret);
assert_eq!(1_000_000, amt);
},
_ => panic!("Unexpected event"),
}
match events[1] {
Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
assert_eq!(payment_hash_3, *payment_hash);
assert!(payment_preimage.is_none());
assert_eq!(payment_secret_3, *payment_secret);
assert_eq!(1_000_000, amt);
},
_ => panic!("Unexpected event"),
}
}

#[test]
Expand Down
24 changes: 16 additions & 8 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -3460,7 +3460,7 @@ impl<Signer: Sign> Channel<Signer> {
/// is_usable() and considers things like the channel being temporarily disabled.
/// Allowed in any state (including after shutdown)
pub fn is_live(&self) -> bool {
self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32) == 0)
self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32) == 0)
}

/// Returns true if this channel has been marked as awaiting a monitor update to move forward.
Expand DownExpand Up@@ -3974,10 +3974,18 @@ impl<Signer: Sign> Channel<Signer> {

/// Adds a pending outbound HTLC to this channel, note that you probably want
/// send_htlc_and_commit instead cause you'll want both messages at once.
/// This returns an option instead of a pure UpdateAddHTLC as we may be in a state where we are
/// waiting on the remote peer to send us a revoke_and_ack during which time we cannot add new
/// HTLCs on the wire or we wouldn't be able to determine what they actually ACK'ed.
/// You MUST call send_commitment prior to any other calls on this Channel
///
/// This returns an optional UpdateAddHTLC as we may be in a state where we cannot add HTLCs on
/// the wire:
/// * In cases where we're waiting on the remote peer to send us a revoke_and_ack, we
/// wouldn't be able to determine what they actually ACK'ed if we have two sets of updates
/// awaiting ACK.
/// * In cases where we're marked MonitorUpdateFailed, we cannot commit to a new state as we
/// may not yet have sent the previous commitment update messages and will need to regenerate
/// them.
///
/// You MUST call send_commitment prior to calling any other methods on this Channel!
///
/// If an Err is returned, it's a ChannelError::Ignore!
pub fn send_htlc(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError> {
if (self.channel_state & (ChannelState::ChannelFunded as u32 | BOTH_SIDES_SHUTDOWN_MASK)) != (ChannelState::ChannelFunded as u32) {
Expand All@@ -3996,14 +4004,14 @@ impl<Signer: Sign> Channel<Signer> {
return Err(ChannelError::Ignore(format!("Cannot send less than their minimum HTLC value ({})", self.counterparty_htlc_minimum_msat)));
}

if (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
if (self.channel_state & (ChannelState::PeerDisconnected as u32)) != 0 {
// Note that this should never really happen, if we're !is_live() on receipt of an
// incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
// the user to send directly into a !is_live() channel. However, if we

@ariardariardJun 24, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This comment sounds weird, I still think we should be able to receive a HTLC even if the forwarding channel is knock-out, though we don't currently in decode_update_add_htlc_onion. Maybe possible after #680.

@TheBlueMattTheBlueMattJun 25, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Right, I suppose its reasonable to accept an HTLC for forwarding to a channel which is disconnected currently if you're a special type of node that can make the channel become connected soon (of course for a general purpose node we should definitely fail). There isn't a lot of harm in waiting to fail until the payment gets to the send_htlc call (instead of doing it immediately upon receipt), so I think I agree we should just drop it (but keep this return).

All that to say, I don't think anything needs to change in this PR, correct? The comment is still true today, though I agree we could change it in a followup and should change it after #680.

@ariardariardJun 25, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah I just believe this forwarding policy makes really sense if the next hop is owned by a mobile client, with a clear agreement "I'm going to be offline 99% of my time, please cache my HTLC" ? Or at least it would be worthy as an experimentation.

Right, it's more a good-place-to-clean-once-we-have-680.

// disconnected during the time the previous hop was doing the commitment dance we may
// end up getting here after the forwarding delay. In any case, returning an
// IgnoreError will get ChannelManager to do the right thing and fail backwards now.
return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected/frozen for channel monitor update".to_owned()));
return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
}

let (outbound_htlc_count, htlc_outbound_value_msat) = self.get_outbound_pending_htlc_stats();
Expand DownExpand Up@@ -4048,7 +4056,7 @@ impl<Signer: Sign> Channel<Signer> {
}

// Now update local state:
if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
Comment thread
TheBlueMatt marked this conversation as resolved.
self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
amount_msat,
payment_hash,
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Consider channels "live" even if they are awaiting a monitor update#954

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

TheBlueMatt
merged 1 commit into

from

Jul 1, 2021
 Merged


Consider channels "live" even if they are awaiting a monitor update
#954
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


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


208 changes: 120 additions & 88 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -906,8 +906,8 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
	check_added_monitors!(nodes[1], 1);

	// Attempt to forward a third payment but fail due to the second channel being unavailable
	// for forwarding.
	// Forward a third payment which will also be added to the holding cell, despite the channel
	// being paused waiting a monitor update.
	let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[2]);
	{
 let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
Expand All@@ -922,39 +922,11 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
	commitment_signed_dance!(nodes[1], nodes[0], send_event.commitment_msg, false, true);
	check_added_monitors!(nodes[1], 0);

	let mut events_2 = nodes[1].node.get_and_clear_pending_msg_events();
@jkczyzjkczyzJun 30, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is there a TL;DR to summarize the changes to this test to help in review?
@TheBlueMattTheBlueMattJun 30, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Because it is no longer the case that receiving an HTLC while we're pending a monitor update restoration causes the HTLC to fail, code which relied on that behavior broke. eg the first code hunk which was removed explicitly delivered an HTLC and expected it to fail, which no longer happens.
The big if-block diff hunk is because the two cases have converged somewhat - we now always have a payment in node[1]'s outbound holding cell.
	assert_eq!(events_2.len(), 1);
	match events_2.remove(0) {
 MessageSendEvent::UpdateHTLCs { node_id, updates } => {
 assert_eq!(node_id, nodes[0].node.get_our_node_id());
 assert!(updates.update_fulfill_htlcs.is_empty());
 assert_eq!(updates.update_fail_htlcs.len(), 1);
 assert!(updates.update_fail_malformed_htlcs.is_empty());
 assert!(updates.update_add_htlcs.is_empty());
 assert!(updates.update_fee.is_none());

 nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
 commitment_signed_dance!(nodes[0], nodes[1], updates.commitment_signed, false, true);

 let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
 assert_eq!(msg_events.len(), 1);
 match msg_events[0] {
 MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
 assert_eq!(msg.contents.short_channel_id, chan_2.0.contents.short_channel_id);
 assert_eq!(msg.contents.flags & 2, 2); // temp disabled
 },
 _ => panic!("Unexpected event"),
 }

 let events = nodes[0].node.get_and_clear_pending_events();
 assert_eq!(events.len(), 1);
 if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
 assert_eq!(payment_hash, payment_hash_3);
 assert!(!rejected_by_dest);
 } else { panic!("Unexpected event!"); }
 },
 _ => panic!("Unexpected event type!"),
	};
	// Call forward_pending_htlcs and check that the new HTLC was simply added to the holding cell
	// and not forwarded.
	expect_pending_htlcs_forwardable!(nodes[1]);
	check_added_monitors!(nodes[1], 0);
	assert!(nodes[1].node.get_and_clear_pending_events().is_empty());

	let (payment_preimage_4, payment_hash_4) = if test_ignore_second_cs {
 // Try to route another payment backwards from 2 to make sure 1 holds off on responding
Expand All@@ -971,7 +943,6 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1);
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 (Some(payment_preimage_4), Some(payment_hash_4))
	} else { (None, None) };

Expand DownExpand Up@@ -1021,14 +992,10 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &messages_a.0);
	commitment_signed_dance!(nodes[0], nodes[1], messages_a.1, false);
	let events_4 = nodes[0].node.get_and_clear_pending_events();
	assert_eq!(events_4.len(), 1);
	if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events_4[0] {
 assert_eq!(payment_hash, payment_hash_1);
 assert!(rejected_by_dest);
	} else { panic!("Unexpected event!"); }
	expect_payment_failed!(nodes[0], payment_hash_1, true);

	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &send_event_b.msgs[0]);
	let as_cs;
	if test_ignore_second_cs {
 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
 check_added_monitors!(nodes[2], 1);
Expand All@@ -1044,40 +1011,83 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_revoke_and_ack);
 check_added_monitors!(nodes[1], 1);
 let as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
 assert!(as_cs.update_add_htlcs.is_empty());
 assert!(as_cs.update_fail_htlcs.is_empty());
 assert!(as_cs.update_fail_malformed_htlcs.is_empty());
 assert!(as_cs.update_fulfill_htlcs.is_empty());
 assert!(as_cs.update_fee.is_none());
 as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

 nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_cs.commitment_signed);
 check_added_monitors!(nodes[1], 1);
 let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
	} else {
 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
 check_added_monitors!(nodes[2], 1);
 let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

 nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
 check_added_monitors!(nodes[2], 1);
 assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());
 let bs_revoke_and_commit = nodes[2].node.get_and_clear_pending_msg_events();
 assert_eq!(bs_revoke_and_commit.len(), 2);
 match bs_revoke_and_commit[0] {
 MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {
 assert_eq!(*node_id, nodes[1].node.get_our_node_id());
 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &msg);
 check_added_monitors!(nodes[1], 1);
 },
 _ => panic!("Unexpected event"),
 }

 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
 check_added_monitors!(nodes[1], 1);
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
	} else {
 commitment_signed_dance!(nodes[2], nodes[1], send_event_b.commitment_msg, false);
 as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

 match bs_revoke_and_commit[1] {
 MessageSendEvent::UpdateHTLCs { ref node_id, ref updates } => {
 assert_eq!(*node_id, nodes[1].node.get_our_node_id());
 assert!(updates.update_add_htlcs.is_empty());
 assert!(updates.update_fail_htlcs.is_empty());
 assert!(updates.update_fail_malformed_htlcs.is_empty());
 assert!(updates.update_fulfill_htlcs.is_empty());
 assert!(updates.update_fee.is_none());
 nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &updates.commitment_signed);
 check_added_monitors!(nodes[1], 1);
 },
 _ => panic!("Unexpected event"),
 }
	}

	assert_eq!(as_cs.update_add_htlcs.len(), 1);
	assert!(as_cs.update_fail_htlcs.is_empty());
	assert!(as_cs.update_fail_malformed_htlcs.is_empty());
	assert!(as_cs.update_fulfill_htlcs.is_empty());
	assert!(as_cs.update_fee.is_none());
	let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());


	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &as_cs.update_add_htlcs[0]);
	nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
	check_added_monitors!(nodes[2], 1);
	let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
	check_added_monitors!(nodes[2], 1);
	let bs_second_cs = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());

	nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
	check_added_monitors!(nodes[1], 1);
	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

	nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_second_cs.commitment_signed);
	check_added_monitors!(nodes[1], 1);
	let as_second_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_second_raa);
	check_added_monitors!(nodes[2], 1);
	assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());

	expect_pending_htlcs_forwardable!(nodes[2]);

	let events_6 = nodes[2].node.get_and_clear_pending_events();
	assert_eq!(events_6.len(), 1);
	assert_eq!(events_6.len(), 2);
	match events_6[0] {
 Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_2); },
 _ => panic!("Unexpected event"),
	};
	match events_6[1] {
 Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_3); },
 _ => panic!("Unexpected event"),
	};

	if test_ignore_second_cs {
 expect_pending_htlcs_forwardable!(nodes[1]);
Expand DownExpand Up@@ -1612,9 +1622,9 @@ fn first_message_on_recv_ordering() {
fn test_monitor_update_fail_claim() {
	// Basic test for monitor update failures when processing claim_funds calls.
	// We set up a simple 3-node network, sending a payment from A to B and failing B's monitor
	// update to claim the payment. We then send a payment C->B->A, making the forward of this
	// payment from B to A fail due to the paused channel. Finally, we restore the channel monitor
	// updating and claim the payment on B.
	// update to claim the payment. We then send two payments C->B->A, which are held at B.
	// Finally, we restore the channel monitor updating and claim the payment on B, forwarding
	// the payments from C onwards to A.
	let chanmon_cfgs = create_chanmon_cfgs(3);
	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
Expand All@@ -1630,12 +1640,19 @@ fn test_monitor_update_fail_claim() {

	*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::TemporaryFailure));
	assert!(nodes[1].node.claim_funds(payment_preimage_1));
	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
@jkczyzjkczyzJun 30, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is there any way to assert the state of the channel rather than examining the log?
@TheBlueMattTheBlueMattJun 30, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

This line was only moved, not created. the log assert is nice IMO as it checks the specific error, not just that one happened, though the observable state is still checked in that no new message was generated.
	check_added_monitors!(nodes[1], 1);

	// Note that at this point there is a pending commitment transaction update for A being held by
	// B. Even when we go to send the payment from C through B to A, B will not update this
	// already-signed commitment transaction and will instead wait for it to resolve before
	// forwarding the payment onwards.

	let (_, payment_hash_2, payment_secret_2) = get_payment_preimage_hash!(nodes[0]);
	let route;
	{
 let net_graph_msg_handler = &nodes[2].net_graph_msg_handler;
let route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1000000, TEST_FINAL_CLTV, &logger).unwrap();
 route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1_000_000, TEST_FINAL_CLTV, &logger).unwrap();
 nodes[2].node.send_payment(&route, payment_hash_2, &Some(payment_secret_2)).unwrap();
 check_added_monitors!(nodes[2], 1);
	}
Expand All@@ -1650,29 +1667,19 @@ fn test_monitor_update_fail_claim() {
	nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
	let events = nodes[1].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 0);
	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
	commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

	let bs_fail_update = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
	nodes[2].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_update.update_fail_htlcs[0]);
	commitment_signed_dance!(nodes[2], nodes[1], bs_fail_update.commitment_signed, false, true);

	let msg_events = nodes[2].node.get_and_clear_pending_msg_events();
	assert_eq!(msg_events.len(), 1);
	match msg_events[0] {
 MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
 assert_eq!(msg.contents.short_channel_id, chan_1.0.contents.short_channel_id);
 assert_eq!(msg.contents.flags & 2, 2); // temp disabled
 },
 _ => panic!("Unexpected event"),
	}
	let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[0]);
	nodes[2].node.send_payment(&route, payment_hash_3, &Some(payment_secret_3)).unwrap();
	check_added_monitors!(nodes[2], 1);

	let events = nodes[2].node.get_and_clear_pending_events();
	let mut events = nodes[2].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 1);
	if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
 assert_eq!(payment_hash, payment_hash_2);
 assert!(!rejected_by_dest);
	} else { panic!("Unexpected event!"); }
	let payment_event = SendEvent::from_event(events.pop().unwrap());
	nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
	let events = nodes[1].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 0);
	commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

	// Now restore monitor updating on the 0<->1 channel and claim the funds on B.
	let (outpoint, latest_update) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_1.2).unwrap().clone();
Expand All@@ -1682,12 +1689,37 @@ fn test_monitor_update_fail_claim() {
	let bs_fulfill_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
	nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_fulfill_update.update_fulfill_htlcs[0]);
	commitment_signed_dance!(nodes[0], nodes[1], bs_fulfill_update.commitment_signed, false);
	expect_payment_sent!(nodes[0], payment_preimage_1);

	// Get the payment forwards, note that they were batched into one commitment update.
	expect_pending_htlcs_forwardable!(nodes[1]);
	check_added_monitors!(nodes[1], 1);
	let bs_forward_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
	nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[0]);
	nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[1]);
	commitment_signed_dance!(nodes[0], nodes[1], bs_forward_update.commitment_signed, false);
	expect_pending_htlcs_forwardable!(nodes[0]);

	let events = nodes[0].node.get_and_clear_pending_events();
	assert_eq!(events.len(), 1);
	if let Event::PaymentSent { payment_preimage, .. } = events[0] {
 assert_eq!(payment_preimage, payment_preimage_1);
	} else { panic!("Unexpected event!"); }
	assert_eq!(events.len(), 2);
	match events[0] {
 Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
 assert_eq!(payment_hash_2, *payment_hash);
 assert!(payment_preimage.is_none());
 assert_eq!(payment_secret_2, *payment_secret);
 assert_eq!(1_000_000, amt);
 },
 _ => panic!("Unexpected event"),
	}
	match events[1] {
 Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
 assert_eq!(payment_hash_3, *payment_hash);
 assert!(payment_preimage.is_none());
 assert_eq!(payment_secret_3, *payment_secret);
 assert_eq!(1_000_000, amt);
 },
 _ => panic!("Unexpected event"),
	}
}

#[test]
Expand Down

24 changes: 16 additions & 8 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3460,7 +3460,7 @@ impl<Signer: Sign> Channel<Signer> {
	/// is_usable() and considers things like the channel being temporarily disabled.
	/// Allowed in any state (including after shutdown)
	pub fn is_live(&self) -> bool {
 self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32) == 0)
 self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32) == 0)
	}

	/// Returns true if this channel has been marked as awaiting a monitor update to move forward.
Expand DownExpand Up@@ -3974,10 +3974,18 @@ impl<Signer: Sign> Channel<Signer> {

	/// Adds a pending outbound HTLC to this channel, note that you probably want
	/// send_htlc_and_commit instead cause you'll want both messages at once.
	/// This returns an option instead of a pure UpdateAddHTLC as we may be in a state where we are
	/// waiting on the remote peer to send us a revoke_and_ack during which time we cannot add new
	/// HTLCs on the wire or we wouldn't be able to determine what they actually ACK'ed.
	/// You MUST call send_commitment prior to any other calls on this Channel
	///
	/// This returns an optional UpdateAddHTLC as we may be in a state where we cannot add HTLCs on
	/// the wire:
	/// * In cases where we're waiting on the remote peer to send us a revoke_and_ack, we
	/// wouldn't be able to determine what they actually ACK'ed if we have two sets of updates
	/// awaiting ACK.
	/// * In cases where we're marked MonitorUpdateFailed, we cannot commit to a new state as we
	/// may not yet have sent the previous commitment update messages and will need to regenerate
	/// them.
	///
	/// You MUST call send_commitment prior to calling any other methods on this Channel!
	///
	/// If an Err is returned, it's a ChannelError::Ignore!
	pub fn send_htlc(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError> {
 if (self.channel_state & (ChannelState::ChannelFunded as u32 | BOTH_SIDES_SHUTDOWN_MASK)) != (ChannelState::ChannelFunded as u32) {
Expand All@@ -3996,14 +4004,14 @@ impl<Signer: Sign> Channel<Signer> {
 return Err(ChannelError::Ignore(format!("Cannot send less than their minimum HTLC value ({})", self.counterparty_htlc_minimum_msat)));
 }

 if (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
 if (self.channel_state & (ChannelState::PeerDisconnected as u32)) != 0 {
 // Note that this should never really happen, if we're !is_live() on receipt of an
 // incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
 // the user to send directly into a !is_live() channel. However, if we
@ariardariardJun 24, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

This comment sounds weird, I still think we should be able to receive a HTLC even if the forwarding channel is knock-out, though we don't currently in decode_update_add_htlc_onion. Maybe possible after #680.
@TheBlueMattTheBlueMattJun 25, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Right, I suppose its reasonable to accept an HTLC for forwarding to a channel which is disconnected currently if you're a special type of node that can make the channel become connected soon (of course for a general purpose node we should definitely fail). There isn't a lot of harm in waiting to fail until the payment gets to the send_htlc call (instead of doing it immediately upon receipt), so I think I agree we should just drop it (but keep this return).
All that to say, I don't think anything needs to change in this PR, correct? The comment is still true today, though I agree we could change it in a followup and should change it after #680.
@ariardariardJun 25, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Yeah I just believe this forwarding policy makes really sense if the next hop is owned by a mobile client, with a clear agreement "I'm going to be offline 99% of my time, please cache my HTLC" ? Or at least it would be worthy as an experimentation.
Right, it's more a good-place-to-clean-once-we-have-680.
 // disconnected during the time the previous hop was doing the commitment dance we may
 // end up getting here after the forwarding delay. In any case, returning an
 // IgnoreError will get ChannelManager to do the right thing and fail backwards now.
 return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected/frozen for channel monitor update".to_owned()));
 return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
 }

 let (outbound_htlc_count, htlc_outbound_value_msat) = self.get_outbound_pending_htlc_stats();
Expand DownExpand Up@@ -4048,7 +4056,7 @@ impl<Signer: Sign> Channel<Signer> {
 }

 // Now update local state:
 if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
 if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
Comment thread
TheBlueMatt marked this conversation as resolved.

 self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
 amount_msat,
 payment_hash,
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Consider channels "live" even if they are awaiting a monitor update#954

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

TheBlueMatt
merged 1 commit into

from

Jul 1, 2021
 Merged


Consider channels "live" even if they are awaiting a monitor update
#954
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


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


208 changes: 120 additions & 88 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -906,8 +906,8 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
	check_added_monitors!(nodes[1], 1);

	// Attempt to forward a third payment but fail due to the second channel being unavailable
	// for forwarding.
	// Forward a third payment which will also be added to the holding cell, despite the channel
	// being paused waiting a monitor update.
	let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[2]);
	{
 let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
Expand All@@ -922,39 +922,11 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
	commitment_signed_dance!(nodes[1], nodes[0], send_event.commitment_msg, false, true);
	check_added_monitors!(nodes[1], 0);

	let mut events_2 = nodes[1].node.get_and_clear_pending_msg_events();
@jkczyzjkczyzJun 30, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is there a TL;DR to summarize the changes to this test to help in review?
@TheBlueMattTheBlueMattJun 30, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Because it is no longer the case that receiving an HTLC while we're pending a monitor update restoration causes the HTLC to fail, code which relied on that behavior broke. eg the first code hunk which was removed explicitly delivered an HTLC and expected it to fail, which no longer happens.
The big if-block diff hunk is because the two cases have converged somewhat - we now always have a payment in node[1]'s outbound holding cell.
	assert_eq!(events_2.len(), 1);
	match events_2.remove(0) {
 MessageSendEvent::UpdateHTLCs { node_id, updates } => {
 assert_eq!(node_id, nodes[0].node.get_our_node_id());
 assert!(updates.update_fulfill_htlcs.is_empty());
 assert_eq!(updates.update_fail_htlcs.len(), 1);
 assert!(updates.update_fail_malformed_htlcs.is_empty());
 assert!(updates.update_add_htlcs.is_empty());
 assert!(updates.update_fee.is_none());

 nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
 commitment_signed_dance!(nodes[0], nodes[1], updates.commitment_signed, false, true);

 let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
 assert_eq!(msg_events.len(), 1);
 match msg_events[0] {
 MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
 assert_eq!(msg.contents.short_channel_id, chan_2.0.contents.short_channel_id);
 assert_eq!(msg.contents.flags & 2, 2); // temp disabled
 },
 _ => panic!("Unexpected event"),
 }

 let events = nodes[0].node.get_and_clear_pending_events();
 assert_eq!(events.len(), 1);
 if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
 assert_eq!(payment_hash, payment_hash_3);
 assert!(!rejected_by_dest);
 } else { panic!("Unexpected event!"); }
 },
 _ => panic!("Unexpected event type!"),
	};
	// Call forward_pending_htlcs and check that the new HTLC was simply added to the holding cell
	// and not forwarded.
	expect_pending_htlcs_forwardable!(nodes[1]);
	check_added_monitors!(nodes[1], 0);
	assert!(nodes[1].node.get_and_clear_pending_events().is_empty());

	let (payment_preimage_4, payment_hash_4) = if test_ignore_second_cs {
 // Try to route another payment backwards from 2 to make sure 1 holds off on responding
Expand All@@ -971,7 +943,6 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1);
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 (Some(payment_preimage_4), Some(payment_hash_4))
	} else { (None, None) };

Expand DownExpand Up@@ -1021,14 +992,10 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &messages_a.0);
	commitment_signed_dance!(nodes[0], nodes[1], messages_a.1, false);
	let events_4 = nodes[0].node.get_and_clear_pending_events();
	assert_eq!(events_4.len(), 1);
	if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events_4[0] {
 assert_eq!(payment_hash, payment_hash_1);
 assert!(rejected_by_dest);
	} else { panic!("Unexpected event!"); }
	expect_payment_failed!(nodes[0], payment_hash_1, true);

	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &send_event_b.msgs[0]);
	let as_cs;
	if test_ignore_second_cs {
 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
 check_added_monitors!(nodes[2], 1);
Expand All@@ -1044,40 +1011,83 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_revoke_and_ack);
 check_added_monitors!(nodes[1], 1);
 let as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
 assert!(as_cs.update_add_htlcs.is_empty());
 assert!(as_cs.update_fail_htlcs.is_empty());
 assert!(as_cs.update_fail_malformed_htlcs.is_empty());
 assert!(as_cs.update_fulfill_htlcs.is_empty());
 assert!(as_cs.update_fee.is_none());
 as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

 nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_cs.commitment_signed);
 check_added_monitors!(nodes[1], 1);
 let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
	} else {
 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
 check_added_monitors!(nodes[2], 1);
 let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

 nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
 check_added_monitors!(nodes[2], 1);
 assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());
 let bs_revoke_and_commit = nodes[2].node.get_and_clear_pending_msg_events();
 assert_eq!(bs_revoke_and_commit.len(), 2);
 match bs_revoke_and_commit[0] {
 MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {
 assert_eq!(*node_id, nodes[1].node.get_our_node_id());
 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &msg);
 check_added_monitors!(nodes[1], 1);
 },
 _ => panic!("Unexpected event"),
 }

 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
 check_added_monitors!(nodes[1], 1);
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
	} else {
 commitment_signed_dance!(nodes[2], nodes[1], send_event_b.commitment_msg, false);
 as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

 match bs_revoke_and_commit[1] {
 MessageSendEvent::UpdateHTLCs { ref node_id, ref updates } => {
 assert_eq!(*node_id, nodes[1].node.get_our_node_id());
 assert!(updates.update_add_htlcs.is_empty());
 assert!(updates.update_fail_htlcs.is_empty());
 assert!(updates.update_fail_malformed_htlcs.is_empty());
 assert!(updates.update_fulfill_htlcs.is_empty());
 assert!(updates.update_fee.is_none());
 nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &updates.commitment_signed);
 check_added_monitors!(nodes[1], 1);
 },
 _ => panic!("Unexpected event"),
 }
	}

	assert_eq!(as_cs.update_add_htlcs.len(), 1);
	assert!(as_cs.update_fail_htlcs.is_empty());
	assert!(as_cs.update_fail_malformed_htlcs.is_empty());
	assert!(as_cs.update_fulfill_htlcs.is_empty());
	assert!(as_cs.update_fee.is_none());
	let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());


	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &as_cs.update_add_htlcs[0]);
	nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
	check_added_monitors!(nodes[2], 1);
	let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
	check_added_monitors!(nodes[2], 1);
	let bs_second_cs = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());

	nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
	check_added_monitors!(nodes[1], 1);
	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

	nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_second_cs.commitment_signed);
	check_added_monitors!(nodes[1], 1);
	let as_second_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_second_raa);
	check_added_monitors!(nodes[2], 1);
	assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());

	expect_pending_htlcs_forwardable!(nodes[2]);

	let events_6 = nodes[2].node.get_and_clear_pending_events();
	assert_eq!(events_6.len(), 1);
	assert_eq!(events_6.len(), 2);
	match events_6[0] {
 Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_2); },
 _ => panic!("Unexpected event"),
	};
	match events_6[1] {
 Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_3); },
 _ => panic!("Unexpected event"),
	};

	if test_ignore_second_cs {
 expect_pending_htlcs_forwardable!(nodes[1]);
Expand DownExpand Up@@ -1612,9 +1622,9 @@ fn first_message_on_recv_ordering() {
fn test_monitor_update_fail_claim() {
	// Basic test for monitor update failures when processing claim_funds calls.
	// We set up a simple 3-node network, sending a payment from A to B and failing B's monitor
	// update to claim the payment. We then send a payment C->B->A, making the forward of this
	// payment from B to A fail due to the paused channel. Finally, we restore the channel monitor
	// updating and claim the payment on B.
	// update to claim the payment. We then send two payments C->B->A, which are held at B.
	// Finally, we restore the channel monitor updating and claim the payment on B, forwarding
	// the payments from C onwards to A.
	let chanmon_cfgs = create_chanmon_cfgs(3);
	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
Expand All@@ -1630,12 +1640,19 @@ fn test_monitor_update_fail_claim() {

	*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::TemporaryFailure));
	assert!(nodes[1].node.claim_funds(payment_preimage_1));
	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
@jkczyzjkczyzJun 30, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is there any way to assert the state of the channel rather than examining the log?
@TheBlueMattTheBlueMattJun 30, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

This line was only moved, not created. the log assert is nice IMO as it checks the specific error, not just that one happened, though the observable state is still checked in that no new message was generated.
	check_added_monitors!(nodes[1], 1);

	// Note that at this point there is a pending commitment transaction update for A being held by
	// B. Even when we go to send the payment from C through B to A, B will not update this
	// already-signed commitment transaction and will instead wait for it to resolve before
	// forwarding the payment onwards.

	let (_, payment_hash_2, payment_secret_2) = get_payment_preimage_hash!(nodes[0]);
	let route;
	{
 let net_graph_msg_handler = &nodes[2].net_graph_msg_handler;
let route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1000000, TEST_FINAL_CLTV, &logger).unwrap();
 route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1_000_000, TEST_FINAL_CLTV, &logger).unwrap();
 nodes[2].node.send_payment(&route, payment_hash_2, &Some(payment_secret_2)).unwrap();
 check_added_monitors!(nodes[2], 1);
	}
Expand All@@ -1650,29 +1667,19 @@ fn test_monitor_update_fail_claim() {
	nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
	let events = nodes[1].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 0);
	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
	commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

	let bs_fail_update = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
	nodes[2].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_update.update_fail_htlcs[0]);
	commitment_signed_dance!(nodes[2], nodes[1], bs_fail_update.commitment_signed, false, true);

	let msg_events = nodes[2].node.get_and_clear_pending_msg_events();
	assert_eq!(msg_events.len(), 1);
	match msg_events[0] {
 MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
 assert_eq!(msg.contents.short_channel_id, chan_1.0.contents.short_channel_id);
 assert_eq!(msg.contents.flags & 2, 2); // temp disabled
 },
 _ => panic!("Unexpected event"),
	}
	let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[0]);
	nodes[2].node.send_payment(&route, payment_hash_3, &Some(payment_secret_3)).unwrap();
	check_added_monitors!(nodes[2], 1);

	let events = nodes[2].node.get_and_clear_pending_events();
	let mut events = nodes[2].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 1);
	if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
 assert_eq!(payment_hash, payment_hash_2);
 assert!(!rejected_by_dest);
	} else { panic!("Unexpected event!"); }
	let payment_event = SendEvent::from_event(events.pop().unwrap());
	nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
	let events = nodes[1].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 0);
	commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

	// Now restore monitor updating on the 0<->1 channel and claim the funds on B.
	let (outpoint, latest_update) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_1.2).unwrap().clone();
Expand All@@ -1682,12 +1689,37 @@ fn test_monitor_update_fail_claim() {
	let bs_fulfill_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
	nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_fulfill_update.update_fulfill_htlcs[0]);
	commitment_signed_dance!(nodes[0], nodes[1], bs_fulfill_update.commitment_signed, false);
	expect_payment_sent!(nodes[0], payment_preimage_1);

	// Get the payment forwards, note that they were batched into one commitment update.
	expect_pending_htlcs_forwardable!(nodes[1]);
	check_added_monitors!(nodes[1], 1);
	let bs_forward_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
	nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[0]);
	nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[1]);
	commitment_signed_dance!(nodes[0], nodes[1], bs_forward_update.commitment_signed, false);
	expect_pending_htlcs_forwardable!(nodes[0]);

	let events = nodes[0].node.get_and_clear_pending_events();
	assert_eq!(events.len(), 1);
	if let Event::PaymentSent { payment_preimage, .. } = events[0] {
 assert_eq!(payment_preimage, payment_preimage_1);
	} else { panic!("Unexpected event!"); }
	assert_eq!(events.len(), 2);
	match events[0] {
 Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
 assert_eq!(payment_hash_2, *payment_hash);
 assert!(payment_preimage.is_none());
 assert_eq!(payment_secret_2, *payment_secret);
 assert_eq!(1_000_000, amt);
 },
 _ => panic!("Unexpected event"),
	}
	match events[1] {
 Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
 assert_eq!(payment_hash_3, *payment_hash);
 assert!(payment_preimage.is_none());
 assert_eq!(payment_secret_3, *payment_secret);
 assert_eq!(1_000_000, amt);
 },
 _ => panic!("Unexpected event"),
	}
}

#[test]
Expand Down

24 changes: 16 additions & 8 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3460,7 +3460,7 @@ impl<Signer: Sign> Channel<Signer> {
	/// is_usable() and considers things like the channel being temporarily disabled.
	/// Allowed in any state (including after shutdown)
	pub fn is_live(&self) -> bool {
 self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32) == 0)
 self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32) == 0)
	}

	/// Returns true if this channel has been marked as awaiting a monitor update to move forward.
Expand DownExpand Up@@ -3974,10 +3974,18 @@ impl<Signer: Sign> Channel<Signer> {

	/// Adds a pending outbound HTLC to this channel, note that you probably want
	/// send_htlc_and_commit instead cause you'll want both messages at once.
	/// This returns an option instead of a pure UpdateAddHTLC as we may be in a state where we are
	/// waiting on the remote peer to send us a revoke_and_ack during which time we cannot add new
	/// HTLCs on the wire or we wouldn't be able to determine what they actually ACK'ed.
	/// You MUST call send_commitment prior to any other calls on this Channel
	///
	/// This returns an optional UpdateAddHTLC as we may be in a state where we cannot add HTLCs on
	/// the wire:
	/// * In cases where we're waiting on the remote peer to send us a revoke_and_ack, we
	/// wouldn't be able to determine what they actually ACK'ed if we have two sets of updates
	/// awaiting ACK.
	/// * In cases where we're marked MonitorUpdateFailed, we cannot commit to a new state as we
	/// may not yet have sent the previous commitment update messages and will need to regenerate
	/// them.
	///
	/// You MUST call send_commitment prior to calling any other methods on this Channel!
	///
	/// If an Err is returned, it's a ChannelError::Ignore!
	pub fn send_htlc(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError> {
 if (self.channel_state & (ChannelState::ChannelFunded as u32 | BOTH_SIDES_SHUTDOWN_MASK)) != (ChannelState::ChannelFunded as u32) {
Expand All@@ -3996,14 +4004,14 @@ impl<Signer: Sign> Channel<Signer> {
 return Err(ChannelError::Ignore(format!("Cannot send less than their minimum HTLC value ({})", self.counterparty_htlc_minimum_msat)));
 }

 if (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
 if (self.channel_state & (ChannelState::PeerDisconnected as u32)) != 0 {
 // Note that this should never really happen, if we're !is_live() on receipt of an
 // incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
 // the user to send directly into a !is_live() channel. However, if we
@ariardariardJun 24, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

This comment sounds weird, I still think we should be able to receive a HTLC even if the forwarding channel is knock-out, though we don't currently in decode_update_add_htlc_onion. Maybe possible after #680.
@TheBlueMattTheBlueMattJun 25, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Right, I suppose its reasonable to accept an HTLC for forwarding to a channel which is disconnected currently if you're a special type of node that can make the channel become connected soon (of course for a general purpose node we should definitely fail). There isn't a lot of harm in waiting to fail until the payment gets to the send_htlc call (instead of doing it immediately upon receipt), so I think I agree we should just drop it (but keep this return).
All that to say, I don't think anything needs to change in this PR, correct? The comment is still true today, though I agree we could change it in a followup and should change it after #680.
@ariardariardJun 25, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Yeah I just believe this forwarding policy makes really sense if the next hop is owned by a mobile client, with a clear agreement "I'm going to be offline 99% of my time, please cache my HTLC" ? Or at least it would be worthy as an experimentation.
Right, it's more a good-place-to-clean-once-we-have-680.
 // disconnected during the time the previous hop was doing the commitment dance we may
 // end up getting here after the forwarding delay. In any case, returning an
 // IgnoreError will get ChannelManager to do the right thing and fail backwards now.
 return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected/frozen for channel monitor update".to_owned()));
 return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
 }

 let (outbound_htlc_count, htlc_outbound_value_msat) = self.get_outbound_pending_htlc_stats();
Expand DownExpand Up@@ -4048,7 +4056,7 @@ impl<Signer: Sign> Channel<Signer> {
 }

 // Now update local state:
 if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
 if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
Comment thread
TheBlueMatt marked this conversation as resolved.

 self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
 amount_msat,
 payment_hash,
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Consider channels "live" even if they are awaiting a monitor update#954

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

TheBlueMatt
merged 1 commit into

from

Jul 1, 2021
 Merged


Consider channels "live" even if they are awaiting a monitor update
#954
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


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


208 changes: 120 additions & 88 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -906,8 +906,8 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
	check_added_monitors!(nodes[1], 1);

	// Attempt to forward a third payment but fail due to the second channel being unavailable
	// for forwarding.
	// Forward a third payment which will also be added to the holding cell, despite the channel
	// being paused waiting a monitor update.
	let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[2]);
	{
 let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
Expand All@@ -922,39 +922,11 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
	commitment_signed_dance!(nodes[1], nodes[0], send_event.commitment_msg, false, true);
	check_added_monitors!(nodes[1], 0);

	let mut events_2 = nodes[1].node.get_and_clear_pending_msg_events();
@jkczyzjkczyzJun 30, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is there a TL;DR to summarize the changes to this test to help in review?
@TheBlueMattTheBlueMattJun 30, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Because it is no longer the case that receiving an HTLC while we're pending a monitor update restoration causes the HTLC to fail, code which relied on that behavior broke. eg the first code hunk which was removed explicitly delivered an HTLC and expected it to fail, which no longer happens.
The big if-block diff hunk is because the two cases have converged somewhat - we now always have a payment in node[1]'s outbound holding cell.
	assert_eq!(events_2.len(), 1);
	match events_2.remove(0) {
 MessageSendEvent::UpdateHTLCs { node_id, updates } => {
 assert_eq!(node_id, nodes[0].node.get_our_node_id());
 assert!(updates.update_fulfill_htlcs.is_empty());
 assert_eq!(updates.update_fail_htlcs.len(), 1);
 assert!(updates.update_fail_malformed_htlcs.is_empty());
 assert!(updates.update_add_htlcs.is_empty());
 assert!(updates.update_fee.is_none());

 nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
 commitment_signed_dance!(nodes[0], nodes[1], updates.commitment_signed, false, true);

 let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
 assert_eq!(msg_events.len(), 1);
 match msg_events[0] {
 MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
 assert_eq!(msg.contents.short_channel_id, chan_2.0.contents.short_channel_id);
 assert_eq!(msg.contents.flags & 2, 2); // temp disabled
 },
 _ => panic!("Unexpected event"),
 }

 let events = nodes[0].node.get_and_clear_pending_events();
 assert_eq!(events.len(), 1);
 if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
 assert_eq!(payment_hash, payment_hash_3);
 assert!(!rejected_by_dest);
 } else { panic!("Unexpected event!"); }
 },
 _ => panic!("Unexpected event type!"),
	};
	// Call forward_pending_htlcs and check that the new HTLC was simply added to the holding cell
	// and not forwarded.
	expect_pending_htlcs_forwardable!(nodes[1]);
	check_added_monitors!(nodes[1], 0);
	assert!(nodes[1].node.get_and_clear_pending_events().is_empty());

	let (payment_preimage_4, payment_hash_4) = if test_ignore_second_cs {
 // Try to route another payment backwards from 2 to make sure 1 holds off on responding
Expand All@@ -971,7 +943,6 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1);
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 (Some(payment_preimage_4), Some(payment_hash_4))
	} else { (None, None) };

Expand DownExpand Up@@ -1021,14 +992,10 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &messages_a.0);
	commitment_signed_dance!(nodes[0], nodes[1], messages_a.1, false);
	let events_4 = nodes[0].node.get_and_clear_pending_events();
	assert_eq!(events_4.len(), 1);
	if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events_4[0] {
 assert_eq!(payment_hash, payment_hash_1);
 assert!(rejected_by_dest);
	} else { panic!("Unexpected event!"); }
	expect_payment_failed!(nodes[0], payment_hash_1, true);

	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &send_event_b.msgs[0]);
	let as_cs;
	if test_ignore_second_cs {
 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
 check_added_monitors!(nodes[2], 1);
Expand All@@ -1044,40 +1011,83 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_revoke_and_ack);
 check_added_monitors!(nodes[1], 1);
 let as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
 assert!(as_cs.update_add_htlcs.is_empty());
 assert!(as_cs.update_fail_htlcs.is_empty());
 assert!(as_cs.update_fail_malformed_htlcs.is_empty());
 assert!(as_cs.update_fulfill_htlcs.is_empty());
 assert!(as_cs.update_fee.is_none());
 as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

 nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_cs.commitment_signed);
 check_added_monitors!(nodes[1], 1);
 let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
	} else {
 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
 check_added_monitors!(nodes[2], 1);
 let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

 nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
 check_added_monitors!(nodes[2], 1);
 assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());
 let bs_revoke_and_commit = nodes[2].node.get_and_clear_pending_msg_events();
 assert_eq!(bs_revoke_and_commit.len(), 2);
 match bs_revoke_and_commit[0] {
 MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {
 assert_eq!(*node_id, nodes[1].node.get_our_node_id());
 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &msg);
 check_added_monitors!(nodes[1], 1);
 },
 _ => panic!("Unexpected event"),
 }

 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
 check_added_monitors!(nodes[1], 1);
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
	} else {
 commitment_signed_dance!(nodes[2], nodes[1], send_event_b.commitment_msg, false);
 as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

 match bs_revoke_and_commit[1] {
 MessageSendEvent::UpdateHTLCs { ref node_id, ref updates } => {
 assert_eq!(*node_id, nodes[1].node.get_our_node_id());
 assert!(updates.update_add_htlcs.is_empty());
 assert!(updates.update_fail_htlcs.is_empty());
 assert!(updates.update_fail_malformed_htlcs.is_empty());
 assert!(updates.update_fulfill_htlcs.is_empty());
 assert!(updates.update_fee.is_none());
 nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &updates.commitment_signed);
 check_added_monitors!(nodes[1], 1);
 },
 _ => panic!("Unexpected event"),
 }
	}

	assert_eq!(as_cs.update_add_htlcs.len(), 1);
	assert!(as_cs.update_fail_htlcs.is_empty());
	assert!(as_cs.update_fail_malformed_htlcs.is_empty());
	assert!(as_cs.update_fulfill_htlcs.is_empty());
	assert!(as_cs.update_fee.is_none());
	let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());


	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &as_cs.update_add_htlcs[0]);
	nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
	check_added_monitors!(nodes[2], 1);
	let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
	check_added_monitors!(nodes[2], 1);
	let bs_second_cs = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());

	nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
	check_added_monitors!(nodes[1], 1);
	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

	nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_second_cs.commitment_signed);
	check_added_monitors!(nodes[1], 1);
	let as_second_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_second_raa);
	check_added_monitors!(nodes[2], 1);
	assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());

	expect_pending_htlcs_forwardable!(nodes[2]);

	let events_6 = nodes[2].node.get_and_clear_pending_events();
	assert_eq!(events_6.len(), 1);
	assert_eq!(events_6.len(), 2);
	match events_6[0] {
 Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_2); },
 _ => panic!("Unexpected event"),
	};
	match events_6[1] {
 Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_3); },
 _ => panic!("Unexpected event"),
	};

	if test_ignore_second_cs {
 expect_pending_htlcs_forwardable!(nodes[1]);
Expand DownExpand Up@@ -1612,9 +1622,9 @@ fn first_message_on_recv_ordering() {
fn test_monitor_update_fail_claim() {
	// Basic test for monitor update failures when processing claim_funds calls.
	// We set up a simple 3-node network, sending a payment from A to B and failing B's monitor
	// update to claim the payment. We then send a payment C->B->A, making the forward of this
	// payment from B to A fail due to the paused channel. Finally, we restore the channel monitor
	// updating and claim the payment on B.
	// update to claim the payment. We then send two payments C->B->A, which are held at B.
	// Finally, we restore the channel monitor updating and claim the payment on B, forwarding
	// the payments from C onwards to A.
	let chanmon_cfgs = create_chanmon_cfgs(3);
	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
Expand All@@ -1630,12 +1640,19 @@ fn test_monitor_update_fail_claim() {

	*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::TemporaryFailure));
	assert!(nodes[1].node.claim_funds(payment_preimage_1));
	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
@jkczyzjkczyzJun 30, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is there any way to assert the state of the channel rather than examining the log?
@TheBlueMattTheBlueMattJun 30, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

This line was only moved, not created. the log assert is nice IMO as it checks the specific error, not just that one happened, though the observable state is still checked in that no new message was generated.
	check_added_monitors!(nodes[1], 1);

	// Note that at this point there is a pending commitment transaction update for A being held by
	// B. Even when we go to send the payment from C through B to A, B will not update this
	// already-signed commitment transaction and will instead wait for it to resolve before
	// forwarding the payment onwards.

	let (_, payment_hash_2, payment_secret_2) = get_payment_preimage_hash!(nodes[0]);
	let route;
	{
 let net_graph_msg_handler = &nodes[2].net_graph_msg_handler;
let route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1000000, TEST_FINAL_CLTV, &logger).unwrap();
 route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1_000_000, TEST_FINAL_CLTV, &logger).unwrap();
 nodes[2].node.send_payment(&route, payment_hash_2, &Some(payment_secret_2)).unwrap();
 check_added_monitors!(nodes[2], 1);
	}
Expand All@@ -1650,29 +1667,19 @@ fn test_monitor_update_fail_claim() {
	nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
	let events = nodes[1].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 0);
	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
	commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

	let bs_fail_update = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
	nodes[2].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_update.update_fail_htlcs[0]);
	commitment_signed_dance!(nodes[2], nodes[1], bs_fail_update.commitment_signed, false, true);

	let msg_events = nodes[2].node.get_and_clear_pending_msg_events();
	assert_eq!(msg_events.len(), 1);
	match msg_events[0] {
 MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
 assert_eq!(msg.contents.short_channel_id, chan_1.0.contents.short_channel_id);
 assert_eq!(msg.contents.flags & 2, 2); // temp disabled
 },
 _ => panic!("Unexpected event"),
	}
	let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[0]);
	nodes[2].node.send_payment(&route, payment_hash_3, &Some(payment_secret_3)).unwrap();
	check_added_monitors!(nodes[2], 1);

	let events = nodes[2].node.get_and_clear_pending_events();
	let mut events = nodes[2].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 1);
	if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
 assert_eq!(payment_hash, payment_hash_2);
 assert!(!rejected_by_dest);
	} else { panic!("Unexpected event!"); }
	let payment_event = SendEvent::from_event(events.pop().unwrap());
	nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
	let events = nodes[1].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 0);
	commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

	// Now restore monitor updating on the 0<->1 channel and claim the funds on B.
	let (outpoint, latest_update) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_1.2).unwrap().clone();
Expand All@@ -1682,12 +1689,37 @@ fn test_monitor_update_fail_claim() {
	let bs_fulfill_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
	nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_fulfill_update.update_fulfill_htlcs[0]);
	commitment_signed_dance!(nodes[0], nodes[1], bs_fulfill_update.commitment_signed, false);
	expect_payment_sent!(nodes[0], payment_preimage_1);

	// Get the payment forwards, note that they were batched into one commitment update.
	expect_pending_htlcs_forwardable!(nodes[1]);
	check_added_monitors!(nodes[1], 1);
	let bs_forward_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
	nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[0]);
	nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[1]);
	commitment_signed_dance!(nodes[0], nodes[1], bs_forward_update.commitment_signed, false);
	expect_pending_htlcs_forwardable!(nodes[0]);

	let events = nodes[0].node.get_and_clear_pending_events();
	assert_eq!(events.len(), 1);
	if let Event::PaymentSent { payment_preimage, .. } = events[0] {
 assert_eq!(payment_preimage, payment_preimage_1);
	} else { panic!("Unexpected event!"); }
	assert_eq!(events.len(), 2);
	match events[0] {
 Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
 assert_eq!(payment_hash_2, *payment_hash);
 assert!(payment_preimage.is_none());
 assert_eq!(payment_secret_2, *payment_secret);
 assert_eq!(1_000_000, amt);
 },
 _ => panic!("Unexpected event"),
	}
	match events[1] {
 Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
 assert_eq!(payment_hash_3, *payment_hash);
 assert!(payment_preimage.is_none());
 assert_eq!(payment_secret_3, *payment_secret);
 assert_eq!(1_000_000, amt);
 },
 _ => panic!("Unexpected event"),
	}
}

#[test]
Expand Down

24 changes: 16 additions & 8 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3460,7 +3460,7 @@ impl<Signer: Sign> Channel<Signer> {
	/// is_usable() and considers things like the channel being temporarily disabled.
	/// Allowed in any state (including after shutdown)
	pub fn is_live(&self) -> bool {
 self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32) == 0)
 self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32) == 0)
	}

	/// Returns true if this channel has been marked as awaiting a monitor update to move forward.
Expand DownExpand Up@@ -3974,10 +3974,18 @@ impl<Signer: Sign> Channel<Signer> {

	/// Adds a pending outbound HTLC to this channel, note that you probably want
	/// send_htlc_and_commit instead cause you'll want both messages at once.
	/// This returns an option instead of a pure UpdateAddHTLC as we may be in a state where we are
	/// waiting on the remote peer to send us a revoke_and_ack during which time we cannot add new
	/// HTLCs on the wire or we wouldn't be able to determine what they actually ACK'ed.
	/// You MUST call send_commitment prior to any other calls on this Channel
	///
	/// This returns an optional UpdateAddHTLC as we may be in a state where we cannot add HTLCs on
	/// the wire:
	/// * In cases where we're waiting on the remote peer to send us a revoke_and_ack, we
	/// wouldn't be able to determine what they actually ACK'ed if we have two sets of updates
	/// awaiting ACK.
	/// * In cases where we're marked MonitorUpdateFailed, we cannot commit to a new state as we
	/// may not yet have sent the previous commitment update messages and will need to regenerate
	/// them.
	///
	/// You MUST call send_commitment prior to calling any other methods on this Channel!
	///
	/// If an Err is returned, it's a ChannelError::Ignore!
	pub fn send_htlc(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError> {
 if (self.channel_state & (ChannelState::ChannelFunded as u32 | BOTH_SIDES_SHUTDOWN_MASK)) != (ChannelState::ChannelFunded as u32) {
Expand All@@ -3996,14 +4004,14 @@ impl<Signer: Sign> Channel<Signer> {
 return Err(ChannelError::Ignore(format!("Cannot send less than their minimum HTLC value ({})", self.counterparty_htlc_minimum_msat)));
 }

 if (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
 if (self.channel_state & (ChannelState::PeerDisconnected as u32)) != 0 {
 // Note that this should never really happen, if we're !is_live() on receipt of an
 // incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
 // the user to send directly into a !is_live() channel. However, if we
@ariardariardJun 24, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

This comment sounds weird, I still think we should be able to receive a HTLC even if the forwarding channel is knock-out, though we don't currently in decode_update_add_htlc_onion. Maybe possible after #680.
@TheBlueMattTheBlueMattJun 25, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Right, I suppose its reasonable to accept an HTLC for forwarding to a channel which is disconnected currently if you're a special type of node that can make the channel become connected soon (of course for a general purpose node we should definitely fail). There isn't a lot of harm in waiting to fail until the payment gets to the send_htlc call (instead of doing it immediately upon receipt), so I think I agree we should just drop it (but keep this return).
All that to say, I don't think anything needs to change in this PR, correct? The comment is still true today, though I agree we could change it in a followup and should change it after #680.
@ariardariardJun 25, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Yeah I just believe this forwarding policy makes really sense if the next hop is owned by a mobile client, with a clear agreement "I'm going to be offline 99% of my time, please cache my HTLC" ? Or at least it would be worthy as an experimentation.
Right, it's more a good-place-to-clean-once-we-have-680.
 // disconnected during the time the previous hop was doing the commitment dance we may
 // end up getting here after the forwarding delay. In any case, returning an
 // IgnoreError will get ChannelManager to do the right thing and fail backwards now.
 return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected/frozen for channel monitor update".to_owned()));
 return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
 }

 let (outbound_htlc_count, htlc_outbound_value_msat) = self.get_outbound_pending_htlc_stats();
Expand DownExpand Up@@ -4048,7 +4056,7 @@ impl<Signer: Sign> Channel<Signer> {
 }

 // Now update local state:
 if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
 if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
Comment thread
TheBlueMatt marked this conversation as resolved.

 self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
 amount_msat,
 payment_hash,
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Consider channels "live" even if they are awaiting a monitor update#954

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

TheBlueMatt
merged 1 commit into

from

Jul 1, 2021
 Merged


Consider channels "live" even if they are awaiting a monitor update
#954
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


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


208 changes: 120 additions & 88 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -906,8 +906,8 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
	check_added_monitors!(nodes[1], 1);

	// Attempt to forward a third payment but fail due to the second channel being unavailable
	// for forwarding.
	// Forward a third payment which will also be added to the holding cell, despite the channel
	// being paused waiting a monitor update.
	let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[2]);
	{
 let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
Expand All@@ -922,39 +922,11 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
	commitment_signed_dance!(nodes[1], nodes[0], send_event.commitment_msg, false, true);
	check_added_monitors!(nodes[1], 0);

	let mut events_2 = nodes[1].node.get_and_clear_pending_msg_events();
@jkczyzjkczyzJun 30, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is there a TL;DR to summarize the changes to this test to help in review?
@TheBlueMattTheBlueMattJun 30, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Because it is no longer the case that receiving an HTLC while we're pending a monitor update restoration causes the HTLC to fail, code which relied on that behavior broke. eg the first code hunk which was removed explicitly delivered an HTLC and expected it to fail, which no longer happens.
The big if-block diff hunk is because the two cases have converged somewhat - we now always have a payment in node[1]'s outbound holding cell.
	assert_eq!(events_2.len(), 1);
	match events_2.remove(0) {
 MessageSendEvent::UpdateHTLCs { node_id, updates } => {
 assert_eq!(node_id, nodes[0].node.get_our_node_id());
 assert!(updates.update_fulfill_htlcs.is_empty());
 assert_eq!(updates.update_fail_htlcs.len(), 1);
 assert!(updates.update_fail_malformed_htlcs.is_empty());
 assert!(updates.update_add_htlcs.is_empty());
 assert!(updates.update_fee.is_none());

 nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
 commitment_signed_dance!(nodes[0], nodes[1], updates.commitment_signed, false, true);

 let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
 assert_eq!(msg_events.len(), 1);
 match msg_events[0] {
 MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
 assert_eq!(msg.contents.short_channel_id, chan_2.0.contents.short_channel_id);
 assert_eq!(msg.contents.flags & 2, 2); // temp disabled
 },
 _ => panic!("Unexpected event"),
 }

 let events = nodes[0].node.get_and_clear_pending_events();
 assert_eq!(events.len(), 1);
 if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
 assert_eq!(payment_hash, payment_hash_3);
 assert!(!rejected_by_dest);
 } else { panic!("Unexpected event!"); }
 },
 _ => panic!("Unexpected event type!"),
	};
	// Call forward_pending_htlcs and check that the new HTLC was simply added to the holding cell
	// and not forwarded.
	expect_pending_htlcs_forwardable!(nodes[1]);
	check_added_monitors!(nodes[1], 0);
	assert!(nodes[1].node.get_and_clear_pending_events().is_empty());

	let (payment_preimage_4, payment_hash_4) = if test_ignore_second_cs {
 // Try to route another payment backwards from 2 to make sure 1 holds off on responding
Expand All@@ -971,7 +943,6 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1);
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 (Some(payment_preimage_4), Some(payment_hash_4))
	} else { (None, None) };

Expand DownExpand Up@@ -1021,14 +992,10 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &messages_a.0);
	commitment_signed_dance!(nodes[0], nodes[1], messages_a.1, false);
	let events_4 = nodes[0].node.get_and_clear_pending_events();
	assert_eq!(events_4.len(), 1);
	if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events_4[0] {
 assert_eq!(payment_hash, payment_hash_1);
 assert!(rejected_by_dest);
	} else { panic!("Unexpected event!"); }
	expect_payment_failed!(nodes[0], payment_hash_1, true);

	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &send_event_b.msgs[0]);
	let as_cs;
	if test_ignore_second_cs {
 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
 check_added_monitors!(nodes[2], 1);
Expand All@@ -1044,40 +1011,83 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_revoke_and_ack);
 check_added_monitors!(nodes[1], 1);
 let as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
 assert!(as_cs.update_add_htlcs.is_empty());
 assert!(as_cs.update_fail_htlcs.is_empty());
 assert!(as_cs.update_fail_malformed_htlcs.is_empty());
 assert!(as_cs.update_fulfill_htlcs.is_empty());
 assert!(as_cs.update_fee.is_none());
 as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

 nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_cs.commitment_signed);
 check_added_monitors!(nodes[1], 1);
 let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
	} else {
 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
 check_added_monitors!(nodes[2], 1);
 let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

 nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
 check_added_monitors!(nodes[2], 1);
 assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());
 let bs_revoke_and_commit = nodes[2].node.get_and_clear_pending_msg_events();
 assert_eq!(bs_revoke_and_commit.len(), 2);
 match bs_revoke_and_commit[0] {
 MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {
 assert_eq!(*node_id, nodes[1].node.get_our_node_id());
 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &msg);
 check_added_monitors!(nodes[1], 1);
 },
 _ => panic!("Unexpected event"),
 }

 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
 check_added_monitors!(nodes[1], 1);
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
	} else {
 commitment_signed_dance!(nodes[2], nodes[1], send_event_b.commitment_msg, false);
 as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

 match bs_revoke_and_commit[1] {
 MessageSendEvent::UpdateHTLCs { ref node_id, ref updates } => {
 assert_eq!(*node_id, nodes[1].node.get_our_node_id());
 assert!(updates.update_add_htlcs.is_empty());
 assert!(updates.update_fail_htlcs.is_empty());
 assert!(updates.update_fail_malformed_htlcs.is_empty());
 assert!(updates.update_fulfill_htlcs.is_empty());
 assert!(updates.update_fee.is_none());
 nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &updates.commitment_signed);
 check_added_monitors!(nodes[1], 1);
 },
 _ => panic!("Unexpected event"),
 }
	}

	assert_eq!(as_cs.update_add_htlcs.len(), 1);
	assert!(as_cs.update_fail_htlcs.is_empty());
	assert!(as_cs.update_fail_malformed_htlcs.is_empty());
	assert!(as_cs.update_fulfill_htlcs.is_empty());
	assert!(as_cs.update_fee.is_none());
	let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());


	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &as_cs.update_add_htlcs[0]);
	nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
	check_added_monitors!(nodes[2], 1);
	let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
	check_added_monitors!(nodes[2], 1);
	let bs_second_cs = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());

	nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
	check_added_monitors!(nodes[1], 1);
	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

	nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_second_cs.commitment_signed);
	check_added_monitors!(nodes[1], 1);
	let as_second_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_second_raa);
	check_added_monitors!(nodes[2], 1);
	assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());

	expect_pending_htlcs_forwardable!(nodes[2]);

	let events_6 = nodes[2].node.get_and_clear_pending_events();
	assert_eq!(events_6.len(), 1);
	assert_eq!(events_6.len(), 2);
	match events_6[0] {
 Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_2); },
 _ => panic!("Unexpected event"),
	};
	match events_6[1] {
 Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_3); },
 _ => panic!("Unexpected event"),
	};

	if test_ignore_second_cs {
 expect_pending_htlcs_forwardable!(nodes[1]);
Expand DownExpand Up@@ -1612,9 +1622,9 @@ fn first_message_on_recv_ordering() {
fn test_monitor_update_fail_claim() {
	// Basic test for monitor update failures when processing claim_funds calls.
	// We set up a simple 3-node network, sending a payment from A to B and failing B's monitor
	// update to claim the payment. We then send a payment C->B->A, making the forward of this
	// payment from B to A fail due to the paused channel. Finally, we restore the channel monitor
	// updating and claim the payment on B.
	// update to claim the payment. We then send two payments C->B->A, which are held at B.
	// Finally, we restore the channel monitor updating and claim the payment on B, forwarding
	// the payments from C onwards to A.
	let chanmon_cfgs = create_chanmon_cfgs(3);
	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
Expand All@@ -1630,12 +1640,19 @@ fn test_monitor_update_fail_claim() {

	*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::TemporaryFailure));
	assert!(nodes[1].node.claim_funds(payment_preimage_1));
	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
@jkczyzjkczyzJun 30, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is there any way to assert the state of the channel rather than examining the log?
@TheBlueMattTheBlueMattJun 30, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

This line was only moved, not created. the log assert is nice IMO as it checks the specific error, not just that one happened, though the observable state is still checked in that no new message was generated.
	check_added_monitors!(nodes[1], 1);

	// Note that at this point there is a pending commitment transaction update for A being held by
	// B. Even when we go to send the payment from C through B to A, B will not update this
	// already-signed commitment transaction and will instead wait for it to resolve before
	// forwarding the payment onwards.

	let (_, payment_hash_2, payment_secret_2) = get_payment_preimage_hash!(nodes[0]);
	let route;
	{
 let net_graph_msg_handler = &nodes[2].net_graph_msg_handler;
let route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1000000, TEST_FINAL_CLTV, &logger).unwrap();
 route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1_000_000, TEST_FINAL_CLTV, &logger).unwrap();
 nodes[2].node.send_payment(&route, payment_hash_2, &Some(payment_secret_2)).unwrap();
 check_added_monitors!(nodes[2], 1);
	}
Expand All@@ -1650,29 +1667,19 @@ fn test_monitor_update_fail_claim() {
	nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
	let events = nodes[1].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 0);
	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
	commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

	let bs_fail_update = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
	nodes[2].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_update.update_fail_htlcs[0]);
	commitment_signed_dance!(nodes[2], nodes[1], bs_fail_update.commitment_signed, false, true);

	let msg_events = nodes[2].node.get_and_clear_pending_msg_events();
	assert_eq!(msg_events.len(), 1);
	match msg_events[0] {
 MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
 assert_eq!(msg.contents.short_channel_id, chan_1.0.contents.short_channel_id);
 assert_eq!(msg.contents.flags & 2, 2); // temp disabled
 },
 _ => panic!("Unexpected event"),
	}
	let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[0]);
	nodes[2].node.send_payment(&route, payment_hash_3, &Some(payment_secret_3)).unwrap();
	check_added_monitors!(nodes[2], 1);

	let events = nodes[2].node.get_and_clear_pending_events();
	let mut events = nodes[2].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 1);
	if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
 assert_eq!(payment_hash, payment_hash_2);
 assert!(!rejected_by_dest);
	} else { panic!("Unexpected event!"); }
	let payment_event = SendEvent::from_event(events.pop().unwrap());
	nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
	let events = nodes[1].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 0);
	commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

	// Now restore monitor updating on the 0<->1 channel and claim the funds on B.
	let (outpoint, latest_update) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_1.2).unwrap().clone();
Expand All@@ -1682,12 +1689,37 @@ fn test_monitor_update_fail_claim() {
	let bs_fulfill_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
	nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_fulfill_update.update_fulfill_htlcs[0]);
	commitment_signed_dance!(nodes[0], nodes[1], bs_fulfill_update.commitment_signed, false);
	expect_payment_sent!(nodes[0], payment_preimage_1);

	// Get the payment forwards, note that they were batched into one commitment update.
	expect_pending_htlcs_forwardable!(nodes[1]);
	check_added_monitors!(nodes[1], 1);
	let bs_forward_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
	nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[0]);
	nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[1]);
	commitment_signed_dance!(nodes[0], nodes[1], bs_forward_update.commitment_signed, false);
	expect_pending_htlcs_forwardable!(nodes[0]);

	let events = nodes[0].node.get_and_clear_pending_events();
	assert_eq!(events.len(), 1);
	if let Event::PaymentSent { payment_preimage, .. } = events[0] {
 assert_eq!(payment_preimage, payment_preimage_1);
	} else { panic!("Unexpected event!"); }
	assert_eq!(events.len(), 2);
	match events[0] {
 Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
 assert_eq!(payment_hash_2, *payment_hash);
 assert!(payment_preimage.is_none());
 assert_eq!(payment_secret_2, *payment_secret);
 assert_eq!(1_000_000, amt);
 },
 _ => panic!("Unexpected event"),
	}
	match events[1] {
 Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
 assert_eq!(payment_hash_3, *payment_hash);
 assert!(payment_preimage.is_none());
 assert_eq!(payment_secret_3, *payment_secret);
 assert_eq!(1_000_000, amt);
 },
 _ => panic!("Unexpected event"),
	}
}

#[test]
Expand Down

24 changes: 16 additions & 8 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3460,7 +3460,7 @@ impl<Signer: Sign> Channel<Signer> {
	/// is_usable() and considers things like the channel being temporarily disabled.
	/// Allowed in any state (including after shutdown)
	pub fn is_live(&self) -> bool {
 self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32) == 0)
 self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32) == 0)
	}

	/// Returns true if this channel has been marked as awaiting a monitor update to move forward.
Expand DownExpand Up@@ -3974,10 +3974,18 @@ impl<Signer: Sign> Channel<Signer> {

	/// Adds a pending outbound HTLC to this channel, note that you probably want
	/// send_htlc_and_commit instead cause you'll want both messages at once.
	/// This returns an option instead of a pure UpdateAddHTLC as we may be in a state where we are
	/// waiting on the remote peer to send us a revoke_and_ack during which time we cannot add new
	/// HTLCs on the wire or we wouldn't be able to determine what they actually ACK'ed.
	/// You MUST call send_commitment prior to any other calls on this Channel
	///
	/// This returns an optional UpdateAddHTLC as we may be in a state where we cannot add HTLCs on
	/// the wire:
	/// * In cases where we're waiting on the remote peer to send us a revoke_and_ack, we
	/// wouldn't be able to determine what they actually ACK'ed if we have two sets of updates
	/// awaiting ACK.
	/// * In cases where we're marked MonitorUpdateFailed, we cannot commit to a new state as we
	/// may not yet have sent the previous commitment update messages and will need to regenerate
	/// them.
	///
	/// You MUST call send_commitment prior to calling any other methods on this Channel!
	///
	/// If an Err is returned, it's a ChannelError::Ignore!
	pub fn send_htlc(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError> {
 if (self.channel_state & (ChannelState::ChannelFunded as u32 | BOTH_SIDES_SHUTDOWN_MASK)) != (ChannelState::ChannelFunded as u32) {
Expand All@@ -3996,14 +4004,14 @@ impl<Signer: Sign> Channel<Signer> {
 return Err(ChannelError::Ignore(format!("Cannot send less than their minimum HTLC value ({})", self.counterparty_htlc_minimum_msat)));
 }

 if (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
 if (self.channel_state & (ChannelState::PeerDisconnected as u32)) != 0 {
 // Note that this should never really happen, if we're !is_live() on receipt of an
 // incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
 // the user to send directly into a !is_live() channel. However, if we
@ariardariardJun 24, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

This comment sounds weird, I still think we should be able to receive a HTLC even if the forwarding channel is knock-out, though we don't currently in decode_update_add_htlc_onion. Maybe possible after #680.
@TheBlueMattTheBlueMattJun 25, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Right, I suppose its reasonable to accept an HTLC for forwarding to a channel which is disconnected currently if you're a special type of node that can make the channel become connected soon (of course for a general purpose node we should definitely fail). There isn't a lot of harm in waiting to fail until the payment gets to the send_htlc call (instead of doing it immediately upon receipt), so I think I agree we should just drop it (but keep this return).
All that to say, I don't think anything needs to change in this PR, correct? The comment is still true today, though I agree we could change it in a followup and should change it after #680.
@ariardariardJun 25, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Yeah I just believe this forwarding policy makes really sense if the next hop is owned by a mobile client, with a clear agreement "I'm going to be offline 99% of my time, please cache my HTLC" ? Or at least it would be worthy as an experimentation.
Right, it's more a good-place-to-clean-once-we-have-680.
 // disconnected during the time the previous hop was doing the commitment dance we may
 // end up getting here after the forwarding delay. In any case, returning an
 // IgnoreError will get ChannelManager to do the right thing and fail backwards now.
 return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected/frozen for channel monitor update".to_owned()));
 return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
 }

 let (outbound_htlc_count, htlc_outbound_value_msat) = self.get_outbound_pending_htlc_stats();
Expand DownExpand Up@@ -4048,7 +4056,7 @@ impl<Signer: Sign> Channel<Signer> {
 }

 // Now update local state:
 if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
 if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
Comment thread
TheBlueMatt marked this conversation as resolved.

 self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
 amount_msat,
 payment_hash,
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Consider channels "live" even if they are awaiting a monitor update#954

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

TheBlueMatt
merged 1 commit into

from

Jul 1, 2021
 Merged


Consider channels "live" even if they are awaiting a monitor update
#954
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


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


208 changes: 120 additions & 88 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -906,8 +906,8 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
	check_added_monitors!(nodes[1], 1);

	// Attempt to forward a third payment but fail due to the second channel being unavailable
	// for forwarding.
	// Forward a third payment which will also be added to the holding cell, despite the channel
	// being paused waiting a monitor update.
	let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[2]);
	{
 let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
Expand All@@ -922,39 +922,11 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
	commitment_signed_dance!(nodes[1], nodes[0], send_event.commitment_msg, false, true);
	check_added_monitors!(nodes[1], 0);

	let mut events_2 = nodes[1].node.get_and_clear_pending_msg_events();
@jkczyzjkczyzJun 30, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is there a TL;DR to summarize the changes to this test to help in review?
@TheBlueMattTheBlueMattJun 30, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Because it is no longer the case that receiving an HTLC while we're pending a monitor update restoration causes the HTLC to fail, code which relied on that behavior broke. eg the first code hunk which was removed explicitly delivered an HTLC and expected it to fail, which no longer happens.
The big if-block diff hunk is because the two cases have converged somewhat - we now always have a payment in node[1]'s outbound holding cell.
	assert_eq!(events_2.len(), 1);
	match events_2.remove(0) {
 MessageSendEvent::UpdateHTLCs { node_id, updates } => {
 assert_eq!(node_id, nodes[0].node.get_our_node_id());
 assert!(updates.update_fulfill_htlcs.is_empty());
 assert_eq!(updates.update_fail_htlcs.len(), 1);
 assert!(updates.update_fail_malformed_htlcs.is_empty());
 assert!(updates.update_add_htlcs.is_empty());
 assert!(updates.update_fee.is_none());

 nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
 commitment_signed_dance!(nodes[0], nodes[1], updates.commitment_signed, false, true);

 let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
 assert_eq!(msg_events.len(), 1);
 match msg_events[0] {
 MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
 assert_eq!(msg.contents.short_channel_id, chan_2.0.contents.short_channel_id);
 assert_eq!(msg.contents.flags & 2, 2); // temp disabled
 },
 _ => panic!("Unexpected event"),
 }

 let events = nodes[0].node.get_and_clear_pending_events();
 assert_eq!(events.len(), 1);
 if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
 assert_eq!(payment_hash, payment_hash_3);
 assert!(!rejected_by_dest);
 } else { panic!("Unexpected event!"); }
 },
 _ => panic!("Unexpected event type!"),
	};
	// Call forward_pending_htlcs and check that the new HTLC was simply added to the holding cell
	// and not forwarded.
	expect_pending_htlcs_forwardable!(nodes[1]);
	check_added_monitors!(nodes[1], 0);
	assert!(nodes[1].node.get_and_clear_pending_events().is_empty());

	let (payment_preimage_4, payment_hash_4) = if test_ignore_second_cs {
 // Try to route another payment backwards from 2 to make sure 1 holds off on responding
Expand All@@ -971,7 +943,6 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1);
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 (Some(payment_preimage_4), Some(payment_hash_4))
	} else { (None, None) };

Expand DownExpand Up@@ -1021,14 +992,10 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &messages_a.0);
	commitment_signed_dance!(nodes[0], nodes[1], messages_a.1, false);
	let events_4 = nodes[0].node.get_and_clear_pending_events();
	assert_eq!(events_4.len(), 1);
	if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events_4[0] {
 assert_eq!(payment_hash, payment_hash_1);
 assert!(rejected_by_dest);
	} else { panic!("Unexpected event!"); }
	expect_payment_failed!(nodes[0], payment_hash_1, true);

	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &send_event_b.msgs[0]);
	let as_cs;
	if test_ignore_second_cs {
 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
 check_added_monitors!(nodes[2], 1);
Expand All@@ -1044,40 +1011,83 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_revoke_and_ack);
 check_added_monitors!(nodes[1], 1);
 let as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
 assert!(as_cs.update_add_htlcs.is_empty());
 assert!(as_cs.update_fail_htlcs.is_empty());
 assert!(as_cs.update_fail_malformed_htlcs.is_empty());
 assert!(as_cs.update_fulfill_htlcs.is_empty());
 assert!(as_cs.update_fee.is_none());
 as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

 nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_cs.commitment_signed);
 check_added_monitors!(nodes[1], 1);
 let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
	} else {
 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
 check_added_monitors!(nodes[2], 1);
 let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

 nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
 check_added_monitors!(nodes[2], 1);
 assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());
 let bs_revoke_and_commit = nodes[2].node.get_and_clear_pending_msg_events();
 assert_eq!(bs_revoke_and_commit.len(), 2);
 match bs_revoke_and_commit[0] {
 MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {
 assert_eq!(*node_id, nodes[1].node.get_our_node_id());
 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &msg);
 check_added_monitors!(nodes[1], 1);
 },
 _ => panic!("Unexpected event"),
 }

 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
 check_added_monitors!(nodes[1], 1);
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
	} else {
 commitment_signed_dance!(nodes[2], nodes[1], send_event_b.commitment_msg, false);
 as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

 match bs_revoke_and_commit[1] {
 MessageSendEvent::UpdateHTLCs { ref node_id, ref updates } => {
 assert_eq!(*node_id, nodes[1].node.get_our_node_id());
 assert!(updates.update_add_htlcs.is_empty());
 assert!(updates.update_fail_htlcs.is_empty());
 assert!(updates.update_fail_malformed_htlcs.is_empty());
 assert!(updates.update_fulfill_htlcs.is_empty());
 assert!(updates.update_fee.is_none());
 nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &updates.commitment_signed);
 check_added_monitors!(nodes[1], 1);
 },
 _ => panic!("Unexpected event"),
 }
	}

	assert_eq!(as_cs.update_add_htlcs.len(), 1);
	assert!(as_cs.update_fail_htlcs.is_empty());
	assert!(as_cs.update_fail_malformed_htlcs.is_empty());
	assert!(as_cs.update_fulfill_htlcs.is_empty());
	assert!(as_cs.update_fee.is_none());
	let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());


	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &as_cs.update_add_htlcs[0]);
	nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
	check_added_monitors!(nodes[2], 1);
	let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
	check_added_monitors!(nodes[2], 1);
	let bs_second_cs = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());

	nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
	check_added_monitors!(nodes[1], 1);
	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

	nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_second_cs.commitment_signed);
	check_added_monitors!(nodes[1], 1);
	let as_second_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_second_raa);
	check_added_monitors!(nodes[2], 1);
	assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());

	expect_pending_htlcs_forwardable!(nodes[2]);

	let events_6 = nodes[2].node.get_and_clear_pending_events();
	assert_eq!(events_6.len(), 1);
	assert_eq!(events_6.len(), 2);
	match events_6[0] {
 Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_2); },
 _ => panic!("Unexpected event"),
	};
	match events_6[1] {
 Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_3); },
 _ => panic!("Unexpected event"),
	};

	if test_ignore_second_cs {
 expect_pending_htlcs_forwardable!(nodes[1]);
Expand DownExpand Up@@ -1612,9 +1622,9 @@ fn first_message_on_recv_ordering() {
fn test_monitor_update_fail_claim() {
	// Basic test for monitor update failures when processing claim_funds calls.
	// We set up a simple 3-node network, sending a payment from A to B and failing B's monitor
	// update to claim the payment. We then send a payment C->B->A, making the forward of this
	// payment from B to A fail due to the paused channel. Finally, we restore the channel monitor
	// updating and claim the payment on B.
	// update to claim the payment. We then send two payments C->B->A, which are held at B.
	// Finally, we restore the channel monitor updating and claim the payment on B, forwarding
	// the payments from C onwards to A.
	let chanmon_cfgs = create_chanmon_cfgs(3);
	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
Expand All@@ -1630,12 +1640,19 @@ fn test_monitor_update_fail_claim() {

	*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::TemporaryFailure));
	assert!(nodes[1].node.claim_funds(payment_preimage_1));
	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
@jkczyzjkczyzJun 30, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is there any way to assert the state of the channel rather than examining the log?
@TheBlueMattTheBlueMattJun 30, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

This line was only moved, not created. the log assert is nice IMO as it checks the specific error, not just that one happened, though the observable state is still checked in that no new message was generated.
	check_added_monitors!(nodes[1], 1);

	// Note that at this point there is a pending commitment transaction update for A being held by
	// B. Even when we go to send the payment from C through B to A, B will not update this
	// already-signed commitment transaction and will instead wait for it to resolve before
	// forwarding the payment onwards.

	let (_, payment_hash_2, payment_secret_2) = get_payment_preimage_hash!(nodes[0]);
	let route;
	{
 let net_graph_msg_handler = &nodes[2].net_graph_msg_handler;
let route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1000000, TEST_FINAL_CLTV, &logger).unwrap();
 route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1_000_000, TEST_FINAL_CLTV, &logger).unwrap();
 nodes[2].node.send_payment(&route, payment_hash_2, &Some(payment_secret_2)).unwrap();
 check_added_monitors!(nodes[2], 1);
	}
Expand All@@ -1650,29 +1667,19 @@ fn test_monitor_update_fail_claim() {
	nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
	let events = nodes[1].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 0);
	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
	commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

	let bs_fail_update = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
	nodes[2].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_update.update_fail_htlcs[0]);
	commitment_signed_dance!(nodes[2], nodes[1], bs_fail_update.commitment_signed, false, true);

	let msg_events = nodes[2].node.get_and_clear_pending_msg_events();
	assert_eq!(msg_events.len(), 1);
	match msg_events[0] {
 MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
 assert_eq!(msg.contents.short_channel_id, chan_1.0.contents.short_channel_id);
 assert_eq!(msg.contents.flags & 2, 2); // temp disabled
 },
 _ => panic!("Unexpected event"),
	}
	let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[0]);
	nodes[2].node.send_payment(&route, payment_hash_3, &Some(payment_secret_3)).unwrap();
	check_added_monitors!(nodes[2], 1);

	let events = nodes[2].node.get_and_clear_pending_events();
	let mut events = nodes[2].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 1);
	if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
 assert_eq!(payment_hash, payment_hash_2);
 assert!(!rejected_by_dest);
	} else { panic!("Unexpected event!"); }
	let payment_event = SendEvent::from_event(events.pop().unwrap());
	nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
	let events = nodes[1].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 0);
	commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

	// Now restore monitor updating on the 0<->1 channel and claim the funds on B.
	let (outpoint, latest_update) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_1.2).unwrap().clone();
Expand All@@ -1682,12 +1689,37 @@ fn test_monitor_update_fail_claim() {
	let bs_fulfill_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
	nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_fulfill_update.update_fulfill_htlcs[0]);
	commitment_signed_dance!(nodes[0], nodes[1], bs_fulfill_update.commitment_signed, false);
	expect_payment_sent!(nodes[0], payment_preimage_1);

	// Get the payment forwards, note that they were batched into one commitment update.
	expect_pending_htlcs_forwardable!(nodes[1]);
	check_added_monitors!(nodes[1], 1);
	let bs_forward_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
	nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[0]);
	nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[1]);
	commitment_signed_dance!(nodes[0], nodes[1], bs_forward_update.commitment_signed, false);
	expect_pending_htlcs_forwardable!(nodes[0]);

	let events = nodes[0].node.get_and_clear_pending_events();
	assert_eq!(events.len(), 1);
	if let Event::PaymentSent { payment_preimage, .. } = events[0] {
 assert_eq!(payment_preimage, payment_preimage_1);
	} else { panic!("Unexpected event!"); }
	assert_eq!(events.len(), 2);
	match events[0] {
 Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
 assert_eq!(payment_hash_2, *payment_hash);
 assert!(payment_preimage.is_none());
 assert_eq!(payment_secret_2, *payment_secret);
 assert_eq!(1_000_000, amt);
 },
 _ => panic!("Unexpected event"),
	}
	match events[1] {
 Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
 assert_eq!(payment_hash_3, *payment_hash);
 assert!(payment_preimage.is_none());
 assert_eq!(payment_secret_3, *payment_secret);
 assert_eq!(1_000_000, amt);
 },
 _ => panic!("Unexpected event"),
	}
}

#[test]
Expand Down

24 changes: 16 additions & 8 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3460,7 +3460,7 @@ impl<Signer: Sign> Channel<Signer> {
	/// is_usable() and considers things like the channel being temporarily disabled.
	/// Allowed in any state (including after shutdown)
	pub fn is_live(&self) -> bool {
 self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32) == 0)
 self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32) == 0)
	}

	/// Returns true if this channel has been marked as awaiting a monitor update to move forward.
Expand DownExpand Up@@ -3974,10 +3974,18 @@ impl<Signer: Sign> Channel<Signer> {

	/// Adds a pending outbound HTLC to this channel, note that you probably want
	/// send_htlc_and_commit instead cause you'll want both messages at once.
	/// This returns an option instead of a pure UpdateAddHTLC as we may be in a state where we are
	/// waiting on the remote peer to send us a revoke_and_ack during which time we cannot add new
	/// HTLCs on the wire or we wouldn't be able to determine what they actually ACK'ed.
	/// You MUST call send_commitment prior to any other calls on this Channel
	///
	/// This returns an optional UpdateAddHTLC as we may be in a state where we cannot add HTLCs on
	/// the wire:
	/// * In cases where we're waiting on the remote peer to send us a revoke_and_ack, we
	/// wouldn't be able to determine what they actually ACK'ed if we have two sets of updates
	/// awaiting ACK.
	/// * In cases where we're marked MonitorUpdateFailed, we cannot commit to a new state as we
	/// may not yet have sent the previous commitment update messages and will need to regenerate
	/// them.
	///
	/// You MUST call send_commitment prior to calling any other methods on this Channel!
	///
	/// If an Err is returned, it's a ChannelError::Ignore!
	pub fn send_htlc(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError> {
 if (self.channel_state & (ChannelState::ChannelFunded as u32 | BOTH_SIDES_SHUTDOWN_MASK)) != (ChannelState::ChannelFunded as u32) {
Expand All@@ -3996,14 +4004,14 @@ impl<Signer: Sign> Channel<Signer> {
 return Err(ChannelError::Ignore(format!("Cannot send less than their minimum HTLC value ({})", self.counterparty_htlc_minimum_msat)));
 }

 if (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
 if (self.channel_state & (ChannelState::PeerDisconnected as u32)) != 0 {
 // Note that this should never really happen, if we're !is_live() on receipt of an
 // incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
 // the user to send directly into a !is_live() channel. However, if we
@ariardariardJun 24, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

This comment sounds weird, I still think we should be able to receive a HTLC even if the forwarding channel is knock-out, though we don't currently in decode_update_add_htlc_onion. Maybe possible after #680.
@TheBlueMattTheBlueMattJun 25, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Right, I suppose its reasonable to accept an HTLC for forwarding to a channel which is disconnected currently if you're a special type of node that can make the channel become connected soon (of course for a general purpose node we should definitely fail). There isn't a lot of harm in waiting to fail until the payment gets to the send_htlc call (instead of doing it immediately upon receipt), so I think I agree we should just drop it (but keep this return).
All that to say, I don't think anything needs to change in this PR, correct? The comment is still true today, though I agree we could change it in a followup and should change it after #680.
@ariardariardJun 25, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Yeah I just believe this forwarding policy makes really sense if the next hop is owned by a mobile client, with a clear agreement "I'm going to be offline 99% of my time, please cache my HTLC" ? Or at least it would be worthy as an experimentation.
Right, it's more a good-place-to-clean-once-we-have-680.
 // disconnected during the time the previous hop was doing the commitment dance we may
 // end up getting here after the forwarding delay. In any case, returning an
 // IgnoreError will get ChannelManager to do the right thing and fail backwards now.
 return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected/frozen for channel monitor update".to_owned()));
 return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
 }

 let (outbound_htlc_count, htlc_outbound_value_msat) = self.get_outbound_pending_htlc_stats();
Expand DownExpand Up@@ -4048,7 +4056,7 @@ impl<Signer: Sign> Channel<Signer> {
 }

 // Now update local state:
 if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
 if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
Comment thread
TheBlueMatt marked this conversation as resolved.

 self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
 amount_msat,
 payment_hash,
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Consider channels "live" even if they are awaiting a monitor update#954

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

TheBlueMatt
merged 1 commit into

from

Jul 1, 2021
 Merged


Consider channels "live" even if they are awaiting a monitor update
#954
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


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


208 changes: 120 additions & 88 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -906,8 +906,8 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
	check_added_monitors!(nodes[1], 1);

	// Attempt to forward a third payment but fail due to the second channel being unavailable
	// for forwarding.
	// Forward a third payment which will also be added to the holding cell, despite the channel
	// being paused waiting a monitor update.
	let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[2]);
	{
 let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
Expand All@@ -922,39 +922,11 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
	commitment_signed_dance!(nodes[1], nodes[0], send_event.commitment_msg, false, true);
	check_added_monitors!(nodes[1], 0);

	let mut events_2 = nodes[1].node.get_and_clear_pending_msg_events();
@jkczyzjkczyzJun 30, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is there a TL;DR to summarize the changes to this test to help in review?
@TheBlueMattTheBlueMattJun 30, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Because it is no longer the case that receiving an HTLC while we're pending a monitor update restoration causes the HTLC to fail, code which relied on that behavior broke. eg the first code hunk which was removed explicitly delivered an HTLC and expected it to fail, which no longer happens.
The big if-block diff hunk is because the two cases have converged somewhat - we now always have a payment in node[1]'s outbound holding cell.
	assert_eq!(events_2.len(), 1);
	match events_2.remove(0) {
 MessageSendEvent::UpdateHTLCs { node_id, updates } => {
 assert_eq!(node_id, nodes[0].node.get_our_node_id());
 assert!(updates.update_fulfill_htlcs.is_empty());
 assert_eq!(updates.update_fail_htlcs.len(), 1);
 assert!(updates.update_fail_malformed_htlcs.is_empty());
 assert!(updates.update_add_htlcs.is_empty());
 assert!(updates.update_fee.is_none());

 nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
 commitment_signed_dance!(nodes[0], nodes[1], updates.commitment_signed, false, true);

 let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
 assert_eq!(msg_events.len(), 1);
 match msg_events[0] {
 MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
 assert_eq!(msg.contents.short_channel_id, chan_2.0.contents.short_channel_id);
 assert_eq!(msg.contents.flags & 2, 2); // temp disabled
 },
 _ => panic!("Unexpected event"),
 }

 let events = nodes[0].node.get_and_clear_pending_events();
 assert_eq!(events.len(), 1);
 if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
 assert_eq!(payment_hash, payment_hash_3);
 assert!(!rejected_by_dest);
 } else { panic!("Unexpected event!"); }
 },
 _ => panic!("Unexpected event type!"),
	};
	// Call forward_pending_htlcs and check that the new HTLC was simply added to the holding cell
	// and not forwarded.
	expect_pending_htlcs_forwardable!(nodes[1]);
	check_added_monitors!(nodes[1], 0);
	assert!(nodes[1].node.get_and_clear_pending_events().is_empty());

	let (payment_preimage_4, payment_hash_4) = if test_ignore_second_cs {
 // Try to route another payment backwards from 2 to make sure 1 holds off on responding
Expand All@@ -971,7 +943,6 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1);
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 (Some(payment_preimage_4), Some(payment_hash_4))
	} else { (None, None) };

Expand DownExpand Up@@ -1021,14 +992,10 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &messages_a.0);
	commitment_signed_dance!(nodes[0], nodes[1], messages_a.1, false);
	let events_4 = nodes[0].node.get_and_clear_pending_events();
	assert_eq!(events_4.len(), 1);
	if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events_4[0] {
 assert_eq!(payment_hash, payment_hash_1);
 assert!(rejected_by_dest);
	} else { panic!("Unexpected event!"); }
	expect_payment_failed!(nodes[0], payment_hash_1, true);

	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &send_event_b.msgs[0]);
	let as_cs;
	if test_ignore_second_cs {
 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
 check_added_monitors!(nodes[2], 1);
Expand All@@ -1044,40 +1011,83 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_revoke_and_ack);
 check_added_monitors!(nodes[1], 1);
 let as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
 assert!(as_cs.update_add_htlcs.is_empty());
 assert!(as_cs.update_fail_htlcs.is_empty());
 assert!(as_cs.update_fail_malformed_htlcs.is_empty());
 assert!(as_cs.update_fulfill_htlcs.is_empty());
 assert!(as_cs.update_fee.is_none());
 as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

 nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_cs.commitment_signed);
 check_added_monitors!(nodes[1], 1);
 let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
	} else {
 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
 check_added_monitors!(nodes[2], 1);
 let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

 nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
 check_added_monitors!(nodes[2], 1);
 assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());
 let bs_revoke_and_commit = nodes[2].node.get_and_clear_pending_msg_events();
 assert_eq!(bs_revoke_and_commit.len(), 2);
 match bs_revoke_and_commit[0] {
 MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {
 assert_eq!(*node_id, nodes[1].node.get_our_node_id());
 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &msg);
 check_added_monitors!(nodes[1], 1);
 },
 _ => panic!("Unexpected event"),
 }

 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
 check_added_monitors!(nodes[1], 1);
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
	} else {
 commitment_signed_dance!(nodes[2], nodes[1], send_event_b.commitment_msg, false);
 as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

 match bs_revoke_and_commit[1] {
 MessageSendEvent::UpdateHTLCs { ref node_id, ref updates } => {
 assert_eq!(*node_id, nodes[1].node.get_our_node_id());
 assert!(updates.update_add_htlcs.is_empty());
 assert!(updates.update_fail_htlcs.is_empty());
 assert!(updates.update_fail_malformed_htlcs.is_empty());
 assert!(updates.update_fulfill_htlcs.is_empty());
 assert!(updates.update_fee.is_none());
 nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &updates.commitment_signed);
 check_added_monitors!(nodes[1], 1);
 },
 _ => panic!("Unexpected event"),
 }
	}

	assert_eq!(as_cs.update_add_htlcs.len(), 1);
	assert!(as_cs.update_fail_htlcs.is_empty());
	assert!(as_cs.update_fail_malformed_htlcs.is_empty());
	assert!(as_cs.update_fulfill_htlcs.is_empty());
	assert!(as_cs.update_fee.is_none());
	let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());


	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &as_cs.update_add_htlcs[0]);
	nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
	check_added_monitors!(nodes[2], 1);
	let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
	check_added_monitors!(nodes[2], 1);
	let bs_second_cs = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());

	nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
	check_added_monitors!(nodes[1], 1);
	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

	nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_second_cs.commitment_signed);
	check_added_monitors!(nodes[1], 1);
	let as_second_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_second_raa);
	check_added_monitors!(nodes[2], 1);
	assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());

	expect_pending_htlcs_forwardable!(nodes[2]);

	let events_6 = nodes[2].node.get_and_clear_pending_events();
	assert_eq!(events_6.len(), 1);
	assert_eq!(events_6.len(), 2);
	match events_6[0] {
 Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_2); },
 _ => panic!("Unexpected event"),
	};
	match events_6[1] {
 Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_3); },
 _ => panic!("Unexpected event"),
	};

	if test_ignore_second_cs {
 expect_pending_htlcs_forwardable!(nodes[1]);
Expand DownExpand Up@@ -1612,9 +1622,9 @@ fn first_message_on_recv_ordering() {
fn test_monitor_update_fail_claim() {
	// Basic test for monitor update failures when processing claim_funds calls.
	// We set up a simple 3-node network, sending a payment from A to B and failing B's monitor
	// update to claim the payment. We then send a payment C->B->A, making the forward of this
	// payment from B to A fail due to the paused channel. Finally, we restore the channel monitor
	// updating and claim the payment on B.
	// update to claim the payment. We then send two payments C->B->A, which are held at B.
	// Finally, we restore the channel monitor updating and claim the payment on B, forwarding
	// the payments from C onwards to A.
	let chanmon_cfgs = create_chanmon_cfgs(3);
	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
Expand All@@ -1630,12 +1640,19 @@ fn test_monitor_update_fail_claim() {

	*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::TemporaryFailure));
	assert!(nodes[1].node.claim_funds(payment_preimage_1));
	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
@jkczyzjkczyzJun 30, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is there any way to assert the state of the channel rather than examining the log?
@TheBlueMattTheBlueMattJun 30, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

This line was only moved, not created. the log assert is nice IMO as it checks the specific error, not just that one happened, though the observable state is still checked in that no new message was generated.
	check_added_monitors!(nodes[1], 1);

	// Note that at this point there is a pending commitment transaction update for A being held by
	// B. Even when we go to send the payment from C through B to A, B will not update this
	// already-signed commitment transaction and will instead wait for it to resolve before
	// forwarding the payment onwards.

	let (_, payment_hash_2, payment_secret_2) = get_payment_preimage_hash!(nodes[0]);
	let route;
	{
 let net_graph_msg_handler = &nodes[2].net_graph_msg_handler;
let route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1000000, TEST_FINAL_CLTV, &logger).unwrap();
 route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1_000_000, TEST_FINAL_CLTV, &logger).unwrap();
 nodes[2].node.send_payment(&route, payment_hash_2, &Some(payment_secret_2)).unwrap();
 check_added_monitors!(nodes[2], 1);
	}
Expand All@@ -1650,29 +1667,19 @@ fn test_monitor_update_fail_claim() {
	nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
	let events = nodes[1].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 0);
	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
	commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

	let bs_fail_update = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
	nodes[2].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_update.update_fail_htlcs[0]);
	commitment_signed_dance!(nodes[2], nodes[1], bs_fail_update.commitment_signed, false, true);

	let msg_events = nodes[2].node.get_and_clear_pending_msg_events();
	assert_eq!(msg_events.len(), 1);
	match msg_events[0] {
 MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
 assert_eq!(msg.contents.short_channel_id, chan_1.0.contents.short_channel_id);
 assert_eq!(msg.contents.flags & 2, 2); // temp disabled
 },
 _ => panic!("Unexpected event"),
	}
	let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[0]);
	nodes[2].node.send_payment(&route, payment_hash_3, &Some(payment_secret_3)).unwrap();
	check_added_monitors!(nodes[2], 1);

	let events = nodes[2].node.get_and_clear_pending_events();
	let mut events = nodes[2].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 1);
	if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
 assert_eq!(payment_hash, payment_hash_2);
 assert!(!rejected_by_dest);
	} else { panic!("Unexpected event!"); }
	let payment_event = SendEvent::from_event(events.pop().unwrap());
	nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
	let events = nodes[1].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 0);
	commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

	// Now restore monitor updating on the 0<->1 channel and claim the funds on B.
	let (outpoint, latest_update) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_1.2).unwrap().clone();
Expand All@@ -1682,12 +1689,37 @@ fn test_monitor_update_fail_claim() {
	let bs_fulfill_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
	nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_fulfill_update.update_fulfill_htlcs[0]);
	commitment_signed_dance!(nodes[0], nodes[1], bs_fulfill_update.commitment_signed, false);
	expect_payment_sent!(nodes[0], payment_preimage_1);

	// Get the payment forwards, note that they were batched into one commitment update.
	expect_pending_htlcs_forwardable!(nodes[1]);
	check_added_monitors!(nodes[1], 1);
	let bs_forward_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
	nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[0]);
	nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[1]);
	commitment_signed_dance!(nodes[0], nodes[1], bs_forward_update.commitment_signed, false);
	expect_pending_htlcs_forwardable!(nodes[0]);

	let events = nodes[0].node.get_and_clear_pending_events();
	assert_eq!(events.len(), 1);
	if let Event::PaymentSent { payment_preimage, .. } = events[0] {
 assert_eq!(payment_preimage, payment_preimage_1);
	} else { panic!("Unexpected event!"); }
	assert_eq!(events.len(), 2);
	match events[0] {
 Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
 assert_eq!(payment_hash_2, *payment_hash);
 assert!(payment_preimage.is_none());
 assert_eq!(payment_secret_2, *payment_secret);
 assert_eq!(1_000_000, amt);
 },
 _ => panic!("Unexpected event"),
	}
	match events[1] {
 Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
 assert_eq!(payment_hash_3, *payment_hash);
 assert!(payment_preimage.is_none());
 assert_eq!(payment_secret_3, *payment_secret);
 assert_eq!(1_000_000, amt);
 },
 _ => panic!("Unexpected event"),
	}
}

#[test]
Expand Down

24 changes: 16 additions & 8 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3460,7 +3460,7 @@ impl<Signer: Sign> Channel<Signer> {
	/// is_usable() and considers things like the channel being temporarily disabled.
	/// Allowed in any state (including after shutdown)
	pub fn is_live(&self) -> bool {
 self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32) == 0)
 self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32) == 0)
	}

	/// Returns true if this channel has been marked as awaiting a monitor update to move forward.
Expand DownExpand Up@@ -3974,10 +3974,18 @@ impl<Signer: Sign> Channel<Signer> {

	/// Adds a pending outbound HTLC to this channel, note that you probably want
	/// send_htlc_and_commit instead cause you'll want both messages at once.
	/// This returns an option instead of a pure UpdateAddHTLC as we may be in a state where we are
	/// waiting on the remote peer to send us a revoke_and_ack during which time we cannot add new
	/// HTLCs on the wire or we wouldn't be able to determine what they actually ACK'ed.
	/// You MUST call send_commitment prior to any other calls on this Channel
	///
	/// This returns an optional UpdateAddHTLC as we may be in a state where we cannot add HTLCs on
	/// the wire:
	/// * In cases where we're waiting on the remote peer to send us a revoke_and_ack, we
	/// wouldn't be able to determine what they actually ACK'ed if we have two sets of updates
	/// awaiting ACK.
	/// * In cases where we're marked MonitorUpdateFailed, we cannot commit to a new state as we
	/// may not yet have sent the previous commitment update messages and will need to regenerate
	/// them.
	///
	/// You MUST call send_commitment prior to calling any other methods on this Channel!
	///
	/// If an Err is returned, it's a ChannelError::Ignore!
	pub fn send_htlc(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError> {
 if (self.channel_state & (ChannelState::ChannelFunded as u32 | BOTH_SIDES_SHUTDOWN_MASK)) != (ChannelState::ChannelFunded as u32) {
Expand All@@ -3996,14 +4004,14 @@ impl<Signer: Sign> Channel<Signer> {
 return Err(ChannelError::Ignore(format!("Cannot send less than their minimum HTLC value ({})", self.counterparty_htlc_minimum_msat)));
 }

 if (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
 if (self.channel_state & (ChannelState::PeerDisconnected as u32)) != 0 {
 // Note that this should never really happen, if we're !is_live() on receipt of an
 // incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
 // the user to send directly into a !is_live() channel. However, if we
@ariardariardJun 24, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

This comment sounds weird, I still think we should be able to receive a HTLC even if the forwarding channel is knock-out, though we don't currently in decode_update_add_htlc_onion. Maybe possible after #680.
@TheBlueMattTheBlueMattJun 25, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Right, I suppose its reasonable to accept an HTLC for forwarding to a channel which is disconnected currently if you're a special type of node that can make the channel become connected soon (of course for a general purpose node we should definitely fail). There isn't a lot of harm in waiting to fail until the payment gets to the send_htlc call (instead of doing it immediately upon receipt), so I think I agree we should just drop it (but keep this return).
All that to say, I don't think anything needs to change in this PR, correct? The comment is still true today, though I agree we could change it in a followup and should change it after #680.
@ariardariardJun 25, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Yeah I just believe this forwarding policy makes really sense if the next hop is owned by a mobile client, with a clear agreement "I'm going to be offline 99% of my time, please cache my HTLC" ? Or at least it would be worthy as an experimentation.
Right, it's more a good-place-to-clean-once-we-have-680.
 // disconnected during the time the previous hop was doing the commitment dance we may
 // end up getting here after the forwarding delay. In any case, returning an
 // IgnoreError will get ChannelManager to do the right thing and fail backwards now.
 return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected/frozen for channel monitor update".to_owned()));
 return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
 }

 let (outbound_htlc_count, htlc_outbound_value_msat) = self.get_outbound_pending_htlc_stats();
Expand DownExpand Up@@ -4048,7 +4056,7 @@ impl<Signer: Sign> Channel<Signer> {
 }

 // Now update local state:
 if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
 if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
Comment thread
TheBlueMatt marked this conversation as resolved.

 self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
 amount_msat,
 payment_hash,
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Consider channels "live" even if they are awaiting a monitor update#954

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

TheBlueMatt
merged 1 commit into

from

Jul 1, 2021
 Merged


Consider channels "live" even if they are awaiting a monitor update
#954
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


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


208 changes: 120 additions & 88 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -906,8 +906,8 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
	check_added_monitors!(nodes[1], 1);

	// Attempt to forward a third payment but fail due to the second channel being unavailable
	// for forwarding.
	// Forward a third payment which will also be added to the holding cell, despite the channel
	// being paused waiting a monitor update.
	let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[2]);
	{
 let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
Expand All@@ -922,39 +922,11 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
	commitment_signed_dance!(nodes[1], nodes[0], send_event.commitment_msg, false, true);
	check_added_monitors!(nodes[1], 0);

	let mut events_2 = nodes[1].node.get_and_clear_pending_msg_events();
@jkczyzjkczyzJun 30, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is there a TL;DR to summarize the changes to this test to help in review?
@TheBlueMattTheBlueMattJun 30, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Because it is no longer the case that receiving an HTLC while we're pending a monitor update restoration causes the HTLC to fail, code which relied on that behavior broke. eg the first code hunk which was removed explicitly delivered an HTLC and expected it to fail, which no longer happens.
The big if-block diff hunk is because the two cases have converged somewhat - we now always have a payment in node[1]'s outbound holding cell.
	assert_eq!(events_2.len(), 1);
	match events_2.remove(0) {
 MessageSendEvent::UpdateHTLCs { node_id, updates } => {
 assert_eq!(node_id, nodes[0].node.get_our_node_id());
 assert!(updates.update_fulfill_htlcs.is_empty());
 assert_eq!(updates.update_fail_htlcs.len(), 1);
 assert!(updates.update_fail_malformed_htlcs.is_empty());
 assert!(updates.update_add_htlcs.is_empty());
 assert!(updates.update_fee.is_none());

 nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
 commitment_signed_dance!(nodes[0], nodes[1], updates.commitment_signed, false, true);

 let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
 assert_eq!(msg_events.len(), 1);
 match msg_events[0] {
 MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
 assert_eq!(msg.contents.short_channel_id, chan_2.0.contents.short_channel_id);
 assert_eq!(msg.contents.flags & 2, 2); // temp disabled
 },
 _ => panic!("Unexpected event"),
 }

 let events = nodes[0].node.get_and_clear_pending_events();
 assert_eq!(events.len(), 1);
 if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
 assert_eq!(payment_hash, payment_hash_3);
 assert!(!rejected_by_dest);
 } else { panic!("Unexpected event!"); }
 },
 _ => panic!("Unexpected event type!"),
	};
	// Call forward_pending_htlcs and check that the new HTLC was simply added to the holding cell
	// and not forwarded.
	expect_pending_htlcs_forwardable!(nodes[1]);
	check_added_monitors!(nodes[1], 0);
	assert!(nodes[1].node.get_and_clear_pending_events().is_empty());

	let (payment_preimage_4, payment_hash_4) = if test_ignore_second_cs {
 // Try to route another payment backwards from 2 to make sure 1 holds off on responding
Expand All@@ -971,7 +943,6 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1);
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 (Some(payment_preimage_4), Some(payment_hash_4))
	} else { (None, None) };

Expand DownExpand Up@@ -1021,14 +992,10 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &messages_a.0);
	commitment_signed_dance!(nodes[0], nodes[1], messages_a.1, false);
	let events_4 = nodes[0].node.get_and_clear_pending_events();
	assert_eq!(events_4.len(), 1);
	if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events_4[0] {
 assert_eq!(payment_hash, payment_hash_1);
 assert!(rejected_by_dest);
	} else { panic!("Unexpected event!"); }
	expect_payment_failed!(nodes[0], payment_hash_1, true);

	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &send_event_b.msgs[0]);
	let as_cs;
	if test_ignore_second_cs {
 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
 check_added_monitors!(nodes[2], 1);
Expand All@@ -1044,40 +1011,83 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {

 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_revoke_and_ack);
 check_added_monitors!(nodes[1], 1);
 let as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
 assert!(as_cs.update_add_htlcs.is_empty());
 assert!(as_cs.update_fail_htlcs.is_empty());
 assert!(as_cs.update_fail_malformed_htlcs.is_empty());
 assert!(as_cs.update_fulfill_htlcs.is_empty());
 assert!(as_cs.update_fee.is_none());
 as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

 nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_cs.commitment_signed);
 check_added_monitors!(nodes[1], 1);
 let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
	} else {
 nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_b.commitment_msg);
 check_added_monitors!(nodes[2], 1);
 let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

 nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
 check_added_monitors!(nodes[2], 1);
 assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());
 let bs_revoke_and_commit = nodes[2].node.get_and_clear_pending_msg_events();
 assert_eq!(bs_revoke_and_commit.len(), 2);
 match bs_revoke_and_commit[0] {
 MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {
 assert_eq!(*node_id, nodes[1].node.get_our_node_id());
 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &msg);
 check_added_monitors!(nodes[1], 1);
 },
 _ => panic!("Unexpected event"),
 }

 nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
 check_added_monitors!(nodes[1], 1);
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
	} else {
 commitment_signed_dance!(nodes[2], nodes[1], send_event_b.commitment_msg, false);
 as_cs = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());

 match bs_revoke_and_commit[1] {
 MessageSendEvent::UpdateHTLCs { ref node_id, ref updates } => {
 assert_eq!(*node_id, nodes[1].node.get_our_node_id());
 assert!(updates.update_add_htlcs.is_empty());
 assert!(updates.update_fail_htlcs.is_empty());
 assert!(updates.update_fail_malformed_htlcs.is_empty());
 assert!(updates.update_fulfill_htlcs.is_empty());
 assert!(updates.update_fee.is_none());
 nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &updates.commitment_signed);
 check_added_monitors!(nodes[1], 1);
 },
 _ => panic!("Unexpected event"),
 }
	}

	assert_eq!(as_cs.update_add_htlcs.len(), 1);
	assert!(as_cs.update_fail_htlcs.is_empty());
	assert!(as_cs.update_fail_malformed_htlcs.is_empty());
	assert!(as_cs.update_fulfill_htlcs.is_empty());
	assert!(as_cs.update_fee.is_none());
	let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());


	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &as_cs.update_add_htlcs[0]);
	nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_cs.commitment_signed);
	check_added_monitors!(nodes[2], 1);
	let bs_second_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());

	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa);
	check_added_monitors!(nodes[2], 1);
	let bs_second_cs = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());

	nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_second_raa);
	check_added_monitors!(nodes[1], 1);
	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

	nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_second_cs.commitment_signed);
	check_added_monitors!(nodes[1], 1);
	let as_second_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());

	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_second_raa);
	check_added_monitors!(nodes[2], 1);
	assert!(nodes[2].node.get_and_clear_pending_msg_events().is_empty());

	expect_pending_htlcs_forwardable!(nodes[2]);

	let events_6 = nodes[2].node.get_and_clear_pending_events();
	assert_eq!(events_6.len(), 1);
	assert_eq!(events_6.len(), 2);
	match events_6[0] {
 Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_2); },
 _ => panic!("Unexpected event"),
	};
	match events_6[1] {
 Event::PaymentReceived { payment_hash, .. } => { assert_eq!(payment_hash, payment_hash_3); },
 _ => panic!("Unexpected event"),
	};

	if test_ignore_second_cs {
 expect_pending_htlcs_forwardable!(nodes[1]);
Expand DownExpand Up@@ -1612,9 +1622,9 @@ fn first_message_on_recv_ordering() {
fn test_monitor_update_fail_claim() {
	// Basic test for monitor update failures when processing claim_funds calls.
	// We set up a simple 3-node network, sending a payment from A to B and failing B's monitor
	// update to claim the payment. We then send a payment C->B->A, making the forward of this
	// payment from B to A fail due to the paused channel. Finally, we restore the channel monitor
	// updating and claim the payment on B.
	// update to claim the payment. We then send two payments C->B->A, which are held at B.
	// Finally, we restore the channel monitor updating and claim the payment on B, forwarding
	// the payments from C onwards to A.
	let chanmon_cfgs = create_chanmon_cfgs(3);
	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
Expand All@@ -1630,12 +1640,19 @@ fn test_monitor_update_fail_claim() {

	*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::TemporaryFailure));
	assert!(nodes[1].node.claim_funds(payment_preimage_1));
	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
@jkczyzjkczyzJun 30, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is there any way to assert the state of the channel rather than examining the log?
@TheBlueMattTheBlueMattJun 30, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

This line was only moved, not created. the log assert is nice IMO as it checks the specific error, not just that one happened, though the observable state is still checked in that no new message was generated.
	check_added_monitors!(nodes[1], 1);

	// Note that at this point there is a pending commitment transaction update for A being held by
	// B. Even when we go to send the payment from C through B to A, B will not update this
	// already-signed commitment transaction and will instead wait for it to resolve before
	// forwarding the payment onwards.

	let (_, payment_hash_2, payment_secret_2) = get_payment_preimage_hash!(nodes[0]);
	let route;
	{
 let net_graph_msg_handler = &nodes[2].net_graph_msg_handler;
let route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1000000, TEST_FINAL_CLTV, &logger).unwrap();
 route = get_route(&nodes[2].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1_000_000, TEST_FINAL_CLTV, &logger).unwrap();
 nodes[2].node.send_payment(&route, payment_hash_2, &Some(payment_secret_2)).unwrap();
 check_added_monitors!(nodes[2], 1);
	}
Expand All@@ -1650,29 +1667,19 @@ fn test_monitor_update_fail_claim() {
	nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
	let events = nodes[1].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 0);
	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
	commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

	let bs_fail_update = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
	nodes[2].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_update.update_fail_htlcs[0]);
	commitment_signed_dance!(nodes[2], nodes[1], bs_fail_update.commitment_signed, false, true);

	let msg_events = nodes[2].node.get_and_clear_pending_msg_events();
	assert_eq!(msg_events.len(), 1);
	match msg_events[0] {
 MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
 assert_eq!(msg.contents.short_channel_id, chan_1.0.contents.short_channel_id);
 assert_eq!(msg.contents.flags & 2, 2); // temp disabled
 },
 _ => panic!("Unexpected event"),
	}
	let (_, payment_hash_3, payment_secret_3) = get_payment_preimage_hash!(nodes[0]);
	nodes[2].node.send_payment(&route, payment_hash_3, &Some(payment_secret_3)).unwrap();
	check_added_monitors!(nodes[2], 1);

	let events = nodes[2].node.get_and_clear_pending_events();
	let mut events = nodes[2].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 1);
	if let Event::PaymentFailed { payment_hash, rejected_by_dest, .. } = events[0] {
 assert_eq!(payment_hash, payment_hash_2);
 assert!(!rejected_by_dest);
	} else { panic!("Unexpected event!"); }
	let payment_event = SendEvent::from_event(events.pop().unwrap());
	nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
	let events = nodes[1].node.get_and_clear_pending_msg_events();
	assert_eq!(events.len(), 0);
	commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false, true);

	// Now restore monitor updating on the 0<->1 channel and claim the funds on B.
	let (outpoint, latest_update) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_1.2).unwrap().clone();
Expand All@@ -1682,12 +1689,37 @@ fn test_monitor_update_fail_claim() {
	let bs_fulfill_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
	nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_fulfill_update.update_fulfill_htlcs[0]);
	commitment_signed_dance!(nodes[0], nodes[1], bs_fulfill_update.commitment_signed, false);
	expect_payment_sent!(nodes[0], payment_preimage_1);

	// Get the payment forwards, note that they were batched into one commitment update.
	expect_pending_htlcs_forwardable!(nodes[1]);
	check_added_monitors!(nodes[1], 1);
	let bs_forward_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
	nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[0]);
	nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[1]);
	commitment_signed_dance!(nodes[0], nodes[1], bs_forward_update.commitment_signed, false);
	expect_pending_htlcs_forwardable!(nodes[0]);

	let events = nodes[0].node.get_and_clear_pending_events();
	assert_eq!(events.len(), 1);
	if let Event::PaymentSent { payment_preimage, .. } = events[0] {
 assert_eq!(payment_preimage, payment_preimage_1);
	} else { panic!("Unexpected event!"); }
	assert_eq!(events.len(), 2);
	match events[0] {
 Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
 assert_eq!(payment_hash_2, *payment_hash);
 assert!(payment_preimage.is_none());
 assert_eq!(payment_secret_2, *payment_secret);
 assert_eq!(1_000_000, amt);
 },
 _ => panic!("Unexpected event"),
	}
	match events[1] {
 Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
 assert_eq!(payment_hash_3, *payment_hash);
 assert!(payment_preimage.is_none());
 assert_eq!(payment_secret_3, *payment_secret);
 assert_eq!(1_000_000, amt);
 },
 _ => panic!("Unexpected event"),
	}
}

#[test]
Expand Down

24 changes: 16 additions & 8 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3460,7 +3460,7 @@ impl<Signer: Sign> Channel<Signer> {
	/// is_usable() and considers things like the channel being temporarily disabled.
	/// Allowed in any state (including after shutdown)
	pub fn is_live(&self) -> bool {
 self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32) == 0)
 self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32) == 0)
	}

	/// Returns true if this channel has been marked as awaiting a monitor update to move forward.
Expand DownExpand Up@@ -3974,10 +3974,18 @@ impl<Signer: Sign> Channel<Signer> {

	/// Adds a pending outbound HTLC to this channel, note that you probably want
	/// send_htlc_and_commit instead cause you'll want both messages at once.
	/// This returns an option instead of a pure UpdateAddHTLC as we may be in a state where we are
	/// waiting on the remote peer to send us a revoke_and_ack during which time we cannot add new
	/// HTLCs on the wire or we wouldn't be able to determine what they actually ACK'ed.
	/// You MUST call send_commitment prior to any other calls on this Channel
	///
	/// This returns an optional UpdateAddHTLC as we may be in a state where we cannot add HTLCs on
	/// the wire:
	/// * In cases where we're waiting on the remote peer to send us a revoke_and_ack, we
	/// wouldn't be able to determine what they actually ACK'ed if we have two sets of updates
	/// awaiting ACK.
	/// * In cases where we're marked MonitorUpdateFailed, we cannot commit to a new state as we
	/// may not yet have sent the previous commitment update messages and will need to regenerate
	/// them.
	///
	/// You MUST call send_commitment prior to calling any other methods on this Channel!
	///
	/// If an Err is returned, it's a ChannelError::Ignore!
	pub fn send_htlc(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError> {
 if (self.channel_state & (ChannelState::ChannelFunded as u32 | BOTH_SIDES_SHUTDOWN_MASK)) != (ChannelState::ChannelFunded as u32) {
Expand All@@ -3996,14 +4004,14 @@ impl<Signer: Sign> Channel<Signer> {
 return Err(ChannelError::Ignore(format!("Cannot send less than their minimum HTLC value ({})", self.counterparty_htlc_minimum_msat)));
 }

 if (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
 if (self.channel_state & (ChannelState::PeerDisconnected as u32)) != 0 {
 // Note that this should never really happen, if we're !is_live() on receipt of an
 // incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
 // the user to send directly into a !is_live() channel. However, if we
@ariardariardJun 24, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

This comment sounds weird, I still think we should be able to receive a HTLC even if the forwarding channel is knock-out, though we don't currently in decode_update_add_htlc_onion. Maybe possible after #680.
@TheBlueMattTheBlueMattJun 25, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Right, I suppose its reasonable to accept an HTLC for forwarding to a channel which is disconnected currently if you're a special type of node that can make the channel become connected soon (of course for a general purpose node we should definitely fail). There isn't a lot of harm in waiting to fail until the payment gets to the send_htlc call (instead of doing it immediately upon receipt), so I think I agree we should just drop it (but keep this return).
All that to say, I don't think anything needs to change in this PR, correct? The comment is still true today, though I agree we could change it in a followup and should change it after #680.
@ariardariardJun 25, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Yeah I just believe this forwarding policy makes really sense if the next hop is owned by a mobile client, with a clear agreement "I'm going to be offline 99% of my time, please cache my HTLC" ? Or at least it would be worthy as an experimentation.
Right, it's more a good-place-to-clean-once-we-have-680.
 // disconnected during the time the previous hop was doing the commitment dance we may
 // end up getting here after the forwarding delay. In any case, returning an
 // IgnoreError will get ChannelManager to do the right thing and fail backwards now.
 return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected/frozen for channel monitor update".to_owned()));
 return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
 }

 let (outbound_htlc_count, htlc_outbound_value_msat) = self.get_outbound_pending_htlc_stats();
Expand DownExpand Up@@ -4048,7 +4056,7 @@ impl<Signer: Sign> Channel<Signer> {
 }

 // Now update local state:
 if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
 if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
Comment thread
TheBlueMatt marked this conversation as resolved.

 self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
 amount_msat,
 payment_hash,
Expand Down
Loading