Skip to content

Drop forwarded HTLCs which were still pending at persist-time #1915

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 13, 2022
Merged

Drop forwarded HTLCs which were still pending at persist-time #1915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
769f590
Remove unused fetch in `lightning_invoice` tests.
TheBlueMatt Dec 13, 2022
2d68183
Drop forwarded HTLCs which were still pending at persist-time
TheBlueMatt Dec 13, 2022
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
1 change: 0 additions & 1 deletion lightning-invoice/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,7 +716,6 @@ mod test {
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
let payment_hash = PaymentHash([0; 32]);
let payment_secret = &nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(10_000), 3600);
let invoice = crate::utils::create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash(
&nodes[1].node, nodes[1].keys_manager, nodes[1].logger, Currency::BitcoinTestnet,
Some(10_000), "test".to_string(), Duration::from_secs(1234567), 3600,
Expand Down
28 changes: 21 additions & 7 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7527,25 +7527,39 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
}
for (htlc_source, htlc) in monitor.get_all_current_outbound_htlcs() {
if let HTLCSource::PreviousHopData(prev_hop_data) = htlc_source {
let pending_forward_matches_htlc = |info: &PendingAddHTLCInfo| {
info.prev_funding_outpoint == prev_hop_data.outpoint &&
info.prev_htlc_id == prev_hop_data.htlc_id
};
// The ChannelMonitor is now responsible for this HTLC's
// failure/success and will let us know what its outcome is. If we
// still have an entry for this HTLC in `forward_htlcs`, we were
// apparently not persisted after the monitor was when forwarding
// the payment.
// still have an entry for this HTLC in `forward_htlcs` or
// `pending_intercepted_htlcs`, we were apparently not persisted after
// the monitor was when forwarding the payment.
forward_htlcs.retain(|_, forwards| {
forwards.retain(|forward| {
if let HTLCForwardInfo::AddHTLC(htlc_info) = forward {
if htlc_info.prev_short_channel_id == prev_hop_data.short_channel_id &&
htlc_info.prev_htlc_id == prev_hop_data.htlc_id
{
if pending_forward_matches_htlc(&htlc_info) {
log_info!(args.logger, "Removing pending to-forward HTLC with hash {} as it was forwarded to the closed channel {}",
log_bytes!(htlc.payment_hash.0), log_bytes!(monitor.get_funding_txo().0.to_channel_id()));
false
} else { true }
} else { true }
});
!forwards.is_empty()
})
});
pending_intercepted_htlcs.as_mut().unwrap().retain(|intercepted_id, htlc_info| {
if pending_forward_matches_htlc(&htlc_info) {
log_info!(args.logger, "Removing pending intercepted HTLC with hash {} as it was forwarded to the closed channel {}",
log_bytes!(htlc.payment_hash.0), log_bytes!(monitor.get_funding_txo().0.to_channel_id()));
pending_events_read.retain(|event| {
if let Event::HTLCIntercepted { intercept_id: ev_id, .. } = event {
intercepted_id != ev_id
} else { true }
});
false
} else { true }
});
}
}
}
Expand Down
59 changes: 53 additions & 6 deletions lightning/src/ln/reload_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use crate::ln::msgs;
use crate::ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, ErrorAction};
use crate::util::enforcing_trait_impls::EnforcingSigner;
use crate::util::test_utils;
use crate::util::errors::APIError;
use crate::util::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
use crate::util::ser::{Writeable, ReadableArgs};
use crate::util::config::UserConfig;
Expand Down Expand Up @@ -814,15 +815,17 @@ fn test_partial_claim_before_restart() {
do_test_partial_claim_before_restart(true);
}

fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_htlc: bool) {
fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_htlc: bool, use_intercept: bool) {
if !use_cs_commitment { assert!(!claim_htlc); }
// If we go to forward a payment, and the ChannelMonitor persistence completes, but the
// ChannelManager does not, we shouldn't try to forward the payment again, nor should we fail
// it back until the ChannelMonitor decides the fate of the HTLC.
// This was never an issue, but it may be easy to regress here going forward.
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]);
let mut intercept_forwards_config = test_default_channel_config();
intercept_forwards_config.accept_intercept_htlcs = true;
let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, Some(intercept_forwards_config), None]);

let persister;
let new_chain_monitor;
Expand All @@ -833,7 +836,13 @@ fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_ht
let chan_id_1 = create_announced_chan_between_nodes(&nodes, 0, 1, channelmanager::provided_init_features(), channelmanager::provided_init_features()).2;
let chan_id_2 = create_announced_chan_between_nodes(&nodes, 1, 2, channelmanager::provided_init_features(), channelmanager::provided_init_features()).2;

let (route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], 1_000_000);
let intercept_scid = nodes[1].node.get_intercept_scid();

let (mut route, payment_hash, payment_preimage, payment_secret) =
get_route_and_payment_hash!(nodes[0], nodes[2], 1_000_000);
if use_intercept {
route.paths[0][1].short_channel_id = intercept_scid;
}
let payment_id = PaymentId(nodes[0].keys_manager.backing.get_secure_random_bytes());
let htlc_expiry = nodes[0].best_block_info().1 + TEST_FINAL_CLTV;
nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret), payment_id).unwrap();
Expand All @@ -843,8 +852,27 @@ fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_ht
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false);

// Store the `ChannelManager` before handling the `PendingHTLCsForwardable`/`HTLCIntercepted`
// events, expecting either event (and the HTLC itself) to be missing on reload even though its
// present when we serialized.
let node_encoded = nodes[1].node.encode();

let mut intercept_id = None;
let mut expected_outbound_amount_msat = None;
if use_intercept {
let events = nodes[1].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::HTLCIntercepted { intercept_id: ev_id, expected_outbound_amount_msat: ev_amt, .. } => {
intercept_id = Some(ev_id);
expected_outbound_amount_msat = Some(ev_amt);
},
_ => panic!()
}
nodes[1].node.forward_intercepted_htlc(intercept_id.unwrap(), &chan_id_2,
nodes[2].node.get_our_node_id(), expected_outbound_amount_msat.unwrap()).unwrap();
}

expect_pending_htlcs_forwardable!(nodes[1]);

let payment_event = SendEvent::from_node(&nodes[1]);
Expand Down Expand Up @@ -872,8 +900,20 @@ fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_ht
let chan_1_monitor_serialized = get_monitor!(nodes[1], chan_id_2).encode();
reload_node!(nodes[1], node_encoded, &[&chan_0_monitor_serialized, &chan_1_monitor_serialized], persister, new_chain_monitor, nodes_1_deserialized);

// Note that this checks that this is the only event on nodes[1], implying the
// `HTLCIntercepted` event has been removed in the `use_intercept` case.
check_closed_event!(nodes[1], 1, ClosureReason::OutdatedChannelManager);

if use_intercept {
// Attempt to forward the HTLC back out over nodes[1]' still-open channel, ensuring we get
// a intercept-doesn't-exist error.
let forward_err = nodes[1].node.forward_intercepted_htlc(intercept_id.unwrap(), &chan_id_1,
nodes[0].node.get_our_node_id(), expected_outbound_amount_msat.unwrap()).unwrap_err();
assert_eq!(forward_err, APIError::APIMisuseError {
err: format!("Payment with intercept id {} not found", log_bytes!(intercept_id.unwrap().0))
});
}

let bs_commitment_tx = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
assert_eq!(bs_commitment_tx.len(), 1);

Expand Down Expand Up @@ -929,9 +969,16 @@ fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_ht

#[test]
fn forwarded_payment_no_manager_persistence() {
do_forwarded_payment_no_manager_persistence(true, true);
do_forwarded_payment_no_manager_persistence(true, false);
do_forwarded_payment_no_manager_persistence(false, false);
do_forwarded_payment_no_manager_persistence(true, true, false);
do_forwarded_payment_no_manager_persistence(true, false, false);
do_forwarded_payment_no_manager_persistence(false, false, false);
}

#[test]
fn intercepted_payment_no_manager_persistence() {
do_forwarded_payment_no_manager_persistence(true, true, true);
do_forwarded_payment_no_manager_persistence(true, false, true);
do_forwarded_payment_no_manager_persistence(false, false, true);
}

#[test]
Expand Down