Skip to content

Commit cb923c6

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #2112 from TheBlueMatt/2023-02-sent-persist-order
Delay RAA-after-next processing until PaymentSent is are handled
2 parents d4ad826 + 31049ed commit cb923c6

13 files changed

+327
-106
lines changed

‎lightning-invoice/src/utils.rs

+1-16
Original file line numberDiff line numberDiff line change
@@ -1373,22 +1373,7 @@ mod test {
13731373
assert_eq!(other_events.borrow().len(), 1);
13741374
check_payment_claimable(&other_events.borrow()[0], payment_hash, payment_secret, payment_amt, payment_preimage_opt, invoice.recover_payee_pub_key());
13751375
do_claim_payment_along_route(&nodes[0], &[&vec!(&nodes[fwd_idx])[..]], false, payment_preimage);
1376-
let events = nodes[0].node.get_and_clear_pending_events();
1377-
assert_eq!(events.len(), 2);
1378-
match events[0] {
1379-
Event::PaymentSent { payment_preimage: ref ev_preimage, payment_hash: ref ev_hash, ref fee_paid_msat, .. } => {
1380-
assert_eq!(payment_preimage, *ev_preimage);
1381-
assert_eq!(payment_hash, *ev_hash);
1382-
assert_eq!(fee_paid_msat, &Some(0));
1383-
},
1384-
_ => panic!("Unexpected event")
1385-
}
1386-
match events[1] {
1387-
Event::PaymentPathSuccessful { payment_hash: hash, .. } => {
1388-
assert_eq!(hash, Some(payment_hash));
1389-
},
1390-
_ => panic!("Unexpected event")
1391-
}
1376+
expect_payment_sent(&nodes[0], payment_preimage, None, true, true);
13921377
}
13931378

13941379
#[test]

‎lightning/src/chain/chainmonitor.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -805,7 +805,7 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner, C: Deref, T: Deref, F: Deref, L
805805
#[cfg(test)]
806806
mod tests {
807807
use crate::{check_added_monitors, check_closed_broadcast, check_closed_event};
808-
use crate::{expect_payment_sent, expect_payment_claimed, expect_payment_sent_without_paths, expect_payment_path_successful, get_event_msg};
808+
use crate::{expect_payment_claimed, expect_payment_path_successful, get_event_msg};
809809
use crate::{get_htlc_update_msgs, get_local_commitment_txn, get_revoke_commit_msgs, get_route_and_payment_hash, unwrap_send_err};
810810
use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Watch};
811811
use crate::chain::channelmonitor::LATENCY_GRACE_PERIOD_BLOCKS;
@@ -888,7 +888,7 @@ mod tests {
888888

889889
let updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
890890
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fulfill_htlcs[0]);
891-
expect_payment_sent_without_paths!(nodes[0], payment_preimage_1);
891+
expect_payment_sent(&nodes[0], payment_preimage_1, None, false, false);
892892
nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &updates.commitment_signed);
893893
check_added_monitors!(nodes[0], 1);
894894
let (as_first_raa, as_first_update) = get_revoke_commit_msgs!(nodes[0], nodes[1].node.get_our_node_id());
@@ -901,7 +901,7 @@ mod tests {
901901
let bs_first_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id());
902902

903903
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_second_updates.update_fulfill_htlcs[0]);
904-
expect_payment_sent_without_paths!(nodes[0], payment_preimage_2);
904+
expect_payment_sent(&nodes[0], payment_preimage_2, None, false, false);
905905
nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_second_updates.commitment_signed);
906906
check_added_monitors!(nodes[0], 1);
907907
nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_first_raa);
@@ -985,7 +985,7 @@ mod tests {
985985
}
986986
}
987987

988-
expect_payment_sent!(nodes[0], payment_preimage);
988+
expect_payment_sent(&nodes[0], payment_preimage, None, true, false);
989989
}
990990

991991
#[test]

‎lightning/src/events/mod.rs

+5
Original file line numberDiff line numberDiff line change
@@ -507,6 +507,11 @@ pub enum Event {
507507
/// payment is no longer retryable, due either to the [`Retry`] provided or
508508
/// [`ChannelManager::abandon_payment`] having been called for the corresponding payment.
509509
///
510+
/// In exceedingly rare cases, it is possible that an [`Event::PaymentFailed`] is generated for
511+
/// a payment after an [`Event::PaymentSent`] event for this same payment has already been
512+
/// received and processed. In this case, the [`Event::PaymentFailed`] event MUST be ignored,
513+
/// and the payment MUST be treated as having succeeded.
514+
///
510515
/// [`Retry`]: crate::ln::channelmanager::Retry
511516
/// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment
512517
PaymentFailed {

‎lightning/src/ln/chanmon_update_fail_tests.rs

+72-7
Original file line numberDiff line numberDiff line change
@@ -1403,6 +1403,7 @@ fn claim_while_disconnected_monitor_update_fail() {
14031403
MessageSendEvent::UpdateHTLCs { ref node_id, ref updates } => {
14041404
assert_eq!(*node_id, nodes[0].node.get_our_node_id());
14051405
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fulfill_htlcs[0]);
1406+
expect_payment_sent(&nodes[0], payment_preimage_1, None, false, false);
14061407
nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &updates.commitment_signed);
14071408
check_added_monitors!(nodes[0], 1);
14081409

@@ -1440,7 +1441,7 @@ fn claim_while_disconnected_monitor_update_fail() {
14401441

14411442
nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_raa);
14421443
check_added_monitors!(nodes[0], 1);
1443-
expect_payment_sent!(nodes[0], payment_preimage_1);
1444+
expect_payment_path_successful!(nodes[0]);
14441445

14451446
claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
14461447
}
@@ -2196,7 +2197,7 @@ fn test_fail_htlc_on_broadcast_after_claim() {
21962197
expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], vec![HTLCDestination::NextHopChannel { node_id: Some(nodes[2].node.get_our_node_id()), channel_id: chan_id_2 }]);
21972198

21982199
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_updates.update_fulfill_htlcs[0]);
2199-
expect_payment_sent_without_paths!(nodes[0], payment_preimage);
2200+
expect_payment_sent(&nodes[0], payment_preimage, None, false, false);
22002201
commitment_signed_dance!(nodes[0], nodes[1], bs_updates.commitment_signed, true, true);
22012202
expect_payment_path_successful!(nodes[0]);
22022203
}
@@ -2449,7 +2450,7 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) {
24492450
assert!(updates.update_fee.is_none());
24502451
assert_eq!(updates.update_fulfill_htlcs.len(), 1);
24512452
nodes[1].node.handle_update_fulfill_htlc(&nodes[0].node.get_our_node_id(), &updates.update_fulfill_htlcs[0]);
2452-
expect_payment_sent_without_paths!(nodes[1], payment_preimage_0);
2453+
expect_payment_sent(&nodes[1], payment_preimage_0, None, false, false);
24532454
assert_eq!(updates.update_add_htlcs.len(), 1);
24542455
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &updates.update_add_htlcs[0]);
24552456
updates.commitment_signed
@@ -2466,7 +2467,7 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) {
24662467
expect_payment_claimable!(nodes[1], payment_hash_1, payment_secret_1, 100000);
24672468
check_added_monitors!(nodes[1], 1);
24682469

2469-
commitment_signed_dance!(nodes[1], nodes[0], (), false, true, false);
2470+
commitment_signed_dance!(nodes[1], nodes[0], (), false, true, false, false);
24702471

24712472
let events = nodes[1].node.get_and_clear_pending_events();
24722473
assert_eq!(events.len(), 2);
@@ -2567,7 +2568,7 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim, second_f
25672568
bs_updates = Some(get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()));
25682569
assert_eq!(bs_updates.as_ref().unwrap().update_fulfill_htlcs.len(), 1);
25692570
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_updates.as_ref().unwrap().update_fulfill_htlcs[0]);
2570-
expect_payment_sent_without_paths!(nodes[0], payment_preimage);
2571+
expect_payment_sent(&nodes[0], payment_preimage, None, false, false);
25712572
if htlc_status == HTLCStatusAtDupClaim::Cleared {
25722573
commitment_signed_dance!(nodes[0], nodes[1], &bs_updates.as_ref().unwrap().commitment_signed, false);
25732574
expect_payment_path_successful!(nodes[0]);
@@ -2598,7 +2599,7 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim, second_f
25982599
bs_updates = Some(get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()));
25992600
assert_eq!(bs_updates.as_ref().unwrap().update_fulfill_htlcs.len(), 1);
26002601
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_updates.as_ref().unwrap().update_fulfill_htlcs[0]);
2601-
expect_payment_sent_without_paths!(nodes[0], payment_preimage);
2602+
expect_payment_sent(&nodes[0], payment_preimage, None, false, false);
26022603
}
26032604
if htlc_status != HTLCStatusAtDupClaim::Cleared {
26042605
commitment_signed_dance!(nodes[0], nodes[1], &bs_updates.as_ref().unwrap().commitment_signed, false);
@@ -2797,7 +2798,7 @@ fn double_temp_error() {
27972798
assert_eq!(node_id, nodes[0].node.get_our_node_id());
27982799
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &update_fulfill_1);
27992800
check_added_monitors!(nodes[0], 0);
2800-
expect_payment_sent_without_paths!(nodes[0], payment_preimage_1);
2801+
expect_payment_sent(&nodes[0], payment_preimage_1, None, false, false);
28012802
nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &commitment_signed_b1);
28022803
check_added_monitors!(nodes[0], 1);
28032804
nodes[0].node.process_pending_htlc_forwards();
@@ -3024,3 +3025,67 @@ fn test_inbound_reload_without_init_mon() {
30243025
do_test_inbound_reload_without_init_mon(false, true);
30253026
do_test_inbound_reload_without_init_mon(false, false);
30263027
}
3028+
3029+
#[test]
3030+
fn test_blocked_chan_preimage_release() {
3031+
// Test that even if a channel's `ChannelMonitorUpdate` flow is blocked waiting on an event to
3032+
// be handled HTLC preimage `ChannelMonitorUpdate`s will still go out.
3033+
let chanmon_cfgs = create_chanmon_cfgs(3);
3034+
let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
3035+
let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
3036+
let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
3037+
3038+
create_announced_chan_between_nodes(&nodes, 0, 1).2;
3039+
create_announced_chan_between_nodes(&nodes, 1, 2).2;
3040+
3041+
send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 5_000_000);
3042+
3043+
// Tee up two payments in opposite directions across nodes[1], one it sent to generate a
3044+
// PaymentSent event and one it forwards.
3045+
let (payment_preimage_1, payment_hash_1, _) = route_payment(&nodes[1], &[&nodes[2]], 1_000_000);
3046+
let (payment_preimage_2, payment_hash_2, _) = route_payment(&nodes[2], &[&nodes[1], &nodes[0]], 1_000_000);
3047+
3048+
// Claim the first payment to get a `PaymentSent` event (but don't handle it yet).
3049+
nodes[2].node.claim_funds(payment_preimage_1);
3050+
check_added_monitors(&nodes[2], 1);
3051+
expect_payment_claimed!(nodes[2], payment_hash_1, 1_000_000);
3052+
3053+
let cs_htlc_fulfill_updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
3054+
nodes[1].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &cs_htlc_fulfill_updates.update_fulfill_htlcs[0]);
3055+
do_commitment_signed_dance(&nodes[1], &nodes[2], &cs_htlc_fulfill_updates.commitment_signed, false, false);
3056+
check_added_monitors(&nodes[1], 0);
3057+
3058+
// Now claim the second payment on nodes[0], which will ultimately result in nodes[1] trying to
3059+
// claim an HTLC on its channel with nodes[2], but that channel is blocked on the above
3060+
// `PaymentSent` event.
3061+
nodes[0].node.claim_funds(payment_preimage_2);
3062+
check_added_monitors(&nodes[0], 1);
3063+
expect_payment_claimed!(nodes[0], payment_hash_2, 1_000_000);
3064+
3065+
let as_htlc_fulfill_updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
3066+
nodes[1].node.handle_update_fulfill_htlc(&nodes[0].node.get_our_node_id(), &as_htlc_fulfill_updates.update_fulfill_htlcs[0]);
3067+
check_added_monitors(&nodes[1], 1); // We generate only a preimage monitor update
3068+
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
3069+
3070+
// Finish the CS dance between nodes[0] and nodes[1].
3071+
do_commitment_signed_dance(&nodes[1], &nodes[0], &as_htlc_fulfill_updates.commitment_signed, false, false);
3072+
check_added_monitors(&nodes[1], 0);
3073+
3074+
let events = nodes[1].node.get_and_clear_pending_events();
3075+
assert_eq!(events.len(), 3);
3076+
if let Event::PaymentSent { .. } = events[0] {} else { panic!(); }
3077+
if let Event::PaymentPathSuccessful { .. } = events[2] {} else { panic!(); }
3078+
if let Event::PaymentForwarded { .. } = events[1] {} else { panic!(); }
3079+
3080+
// The event processing should release the last RAA update.
3081+
check_added_monitors(&nodes[1], 1);
3082+
3083+
// When we fetch the next update the message getter will generate the next update for nodes[2],
3084+
// generating a further monitor update.
3085+
let bs_htlc_fulfill_updates = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
3086+
check_added_monitors(&nodes[1], 1);
3087+
3088+
nodes[2].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_htlc_fulfill_updates.update_fulfill_htlcs[0]);
3089+
do_commitment_signed_dance(&nodes[2], &nodes[1], &bs_htlc_fulfill_updates.commitment_signed, false, false);
3090+
expect_payment_sent(&nodes[2], payment_preimage_2, None, true, true);
3091+
}

‎lightning/src/ln/channel.rs

+32-8
Original file line numberDiff line numberDiff line change
@@ -3201,7 +3201,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
32013201
/// generating an appropriate error *after* the channel state has been updated based on the
32023202
/// revoke_and_ack message.
32033203
pub fn revoke_and_ack<F: Deref, L: Deref>(&mut self, msg: &msgs::RevokeAndACK,
3204-
fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
3204+
fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L, hold_mon_update: bool,
32053205
) -> Result<(Vec<(HTLCSource, PaymentHash)>, Option<ChannelMonitorUpdate>), ChannelError>
32063206
where F::Target: FeeEstimator, L::Target: Logger,
32073207
{
@@ -3382,6 +3382,22 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
33823382
}
33833383
}
33843384

3385+
let release_monitor = self.context.blocked_monitor_updates.is_empty() && !hold_mon_update;
3386+
let release_state_str =
3387+
if hold_mon_update { "Holding" } else if release_monitor { "Releasing" } else { "Blocked" };
3388+
macro_rules! return_with_htlcs_to_fail {
3389+
($htlcs_to_fail: expr) => {
3390+
if !release_monitor {
3391+
self.context.blocked_monitor_updates.push(PendingChannelMonitorUpdate {
3392+
update: monitor_update,
3393+
});
3394+
return Ok(($htlcs_to_fail, None));
3395+
} else {
3396+
return Ok(($htlcs_to_fail, Some(monitor_update)));
3397+
}
3398+
}
3399+
}
3400+
33853401
if (self.context.channel_state & ChannelState::MonitorUpdateInProgress as u32) == ChannelState::MonitorUpdateInProgress as u32 {
33863402
// We can't actually generate a new commitment transaction (incl by freeing holding
33873403
// cells) while we can't update the monitor, so we just return what we have.
@@ -3400,7 +3416,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
34003416
self.context.monitor_pending_failures.append(&mut revoked_htlcs);
34013417
self.context.monitor_pending_finalized_fulfills.append(&mut finalized_claimed_htlcs);
34023418
log_debug!(logger, "Received a valid revoke_and_ack for channel {} but awaiting a monitor update resolution to reply.", log_bytes!(self.context.channel_id()));
3403-
return Ok((Vec::new(), self.push_ret_blockable_mon_update(monitor_update)));
3419+
return_with_htlcs_to_fail!(Vec::new());
34043420
}
34053421

34063422
match self.free_holding_cell_htlcs(fee_estimator, logger) {
@@ -3410,8 +3426,11 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
34103426
self.context.latest_monitor_update_id = monitor_update.update_id;
34113427
monitor_update.updates.append(&mut additional_update.updates);
34123428

3429+
log_debug!(logger, "Received a valid revoke_and_ack for channel {} with holding cell HTLCs freed. {} monitor update.",
3430+
log_bytes!(self.context.channel_id()), release_state_str);
3431+
34133432
self.monitor_updating_paused(false, true, false, to_forward_infos, revoked_htlcs, finalized_claimed_htlcs);
3414-
Ok((htlcs_to_fail, self.push_ret_blockable_mon_update(monitor_update)))
3433+
return_with_htlcs_to_fail!(htlcs_to_fail);
34153434
},
34163435
(None, htlcs_to_fail) => {
34173436
if require_commitment {
@@ -3422,14 +3441,19 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
34223441
self.context.latest_monitor_update_id = monitor_update.update_id;
34233442
monitor_update.updates.append(&mut additional_update.updates);
34243443

3425-
log_debug!(logger, "Received a valid revoke_and_ack for channel {}. Responding with a commitment update with {} HTLCs failed.",
3426-
log_bytes!(self.context.channel_id()), update_fail_htlcs.len() + update_fail_malformed_htlcs.len());
3444+
log_debug!(logger, "Received a valid revoke_and_ack for channel {}. Responding with a commitment update with {} HTLCs failed. {} monitor update.",
3445+
log_bytes!(self.context.channel_id()),
3446+
update_fail_htlcs.len() + update_fail_malformed_htlcs.len(),
3447+
release_state_str);
3448+
34273449
self.monitor_updating_paused(false, true, false, to_forward_infos, revoked_htlcs, finalized_claimed_htlcs);
3428-
Ok((htlcs_to_fail, self.push_ret_blockable_mon_update(monitor_update)))
3450+
return_with_htlcs_to_fail!(htlcs_to_fail);
34293451
} else {
3430-
log_debug!(logger, "Received a valid revoke_and_ack for channel {} with no reply necessary.", log_bytes!(self.context.channel_id()));
3452+
log_debug!(logger, "Received a valid revoke_and_ack for channel {} with no reply necessary. {} monitor update.",
3453+
log_bytes!(self.context.channel_id()), release_state_str);
3454+
34313455
self.monitor_updating_paused(false, false, false, to_forward_infos, revoked_htlcs, finalized_claimed_htlcs);
3432-
Ok((htlcs_to_fail, self.push_ret_blockable_mon_update(monitor_update)))
3456+
return_with_htlcs_to_fail!(htlcs_to_fail);
34333457
}
34343458
}
34353459
}

0 commit comments

Comments
 (0)