Add MPP receive timeout handling #1353

dunxen · 2022-03-09T13:17:28Z

Closes #1050.

dunxen · 2022-03-09T13:22:30Z

lightning/src/ln/channelmanager.rs

@@ -3234,6 +3238,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 										phantom_shared_secret,
 									},
 									value: amt_to_forward,
+									ticks: 0,


I should probably call this timer_ticks.

Yep, probably should :)

TheBlueMatt

Looks basically good to me!

TheBlueMatt · 2022-03-09T19:29:17Z

lightning/src/ln/channelmanager.rs

@@ -3234,6 +3238,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 										phantom_shared_secret,
 									},
 									value: amt_to_forward,
+									ticks: 0,


Yep, probably should :)

TheBlueMatt · 2022-03-09T19:36:21Z

lightning/src/ln/channelmanager.rs

+					}
+				}
+
+				for payment_hash in &mut timed_out_payment_hashes {


You should be able to use claimable_htlcs.retain() instead of iterating and removing here.

lightning/src/ln/functional_tests.rs

codecov-commenter · 2022-03-11T17:38:06Z

Codecov Report

Merging #1353 (daa89dd) into main (996d3d8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head daa89dd differs from pull request most recent head 4086ce8. Consider uploading reports for the commit 4086ce8 to get more accurate results

@@            Coverage Diff             @@
##             main    #1353      +/-   ##
==========================================
+ Coverage   90.75%   90.77%   +0.01%     
==========================================
  Files          73       73              
  Lines       40979    41045      +66     
  Branches    40979    41045      +66     
==========================================
+ Hits        37191    37258      +67     
+ Misses       3788     3787       -1

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	`84.90% <100.00%> (+0.09%)`	⬆️
lightning/src/ln/payment_tests.rs	`99.23% <100.00%> (+0.07%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 996d3d8...4086ce8. Read the comment docs.

TheBlueMatt · 2022-03-11T21:58:12Z

lightning/src/ln/channelmanager.rs

+				channel_state.claimable_htlcs.retain(|payment_hash, htlcs| {
+					if htlcs.into_iter().all(|htlc| {
+						if let OnionPayload::Invoice(ref final_hop_data) = htlc.onion_payload {
+							if htlc.value < final_hop_data.total_msat {


I feel like we should handle all-parts-received MPP payments and non-MPP payments the same - ie instead of just checking if this is an MPP payment here, we should check if we've actually received all the parts we need (by summing up the HTLCs for each given payment hash) before we timeout-fail parts.

Wait, scratch my comment. I understand now. 🤦

TheBlueMatt

LGTM, if you want to expand the test to also test that we dont time out if we're still waiting on parts (and that you can then receive the payment after the other parts come in) that'd be cool too.

dunxen · 2022-03-16T03:14:23Z

LGTM, if you want to expand the test to also test that we dont time out if we're still waiting on parts (and that you can then receive the payment after the other parts come in) that'd be cool too.

Yeah, I definitely want to do that!

valentinewallace

A few optional nits. I'm ACK after the new test changes :)

valentinewallace · 2022-03-16T20:29:34Z

lightning/src/ln/channelmanager.rs

+						} else if !htlcs.into_iter().all(|htlc| {
+							htlc.timer_ticks += 1;
+							return htlc.timer_ticks < MPP_TIMEOUT_TICKS


nit: IMO avoiding double negatives is more readable
so s/!htlcs/htlcs, s/</>=

Yeah, I agree. Looks like I'd also need to change the .all() to a .any() :)

valentinewallace · 2022-03-16T20:33:04Z

lightning/src/ln/functional_tests.rs

@@ -55,6 +55,8 @@ use sync::{Arc, Mutex};
 use ln::functional_test_utils::*;
 use ln::chan_utils::CommitmentTransaction;

+use crate::ln::channelmanager::MPP_TIMEOUT_TICKS;


nit: s/crate:://

dunxen · 2022-03-22T13:55:06Z

Cleaned up a little more and expanded the test to cover the success case too (also checking that timeouts don't affect when we have all parts) and rather moved the tests to payment_tests.rs to be with the other MPP stuff.

TheBlueMatt · 2022-03-22T16:41:45Z

lightning/src/ln/channelmanager.rs

@@ -3668,6 +3674,28 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana

 					true
 				});
+
+				channel_state.claimable_htlcs.retain(|payment_hash, htlcs| {
+					if htlcs.is_empty() { return false }


nit: should be unreachable, maybe add a debug_assert!(false); here before the return.

dunxen commented Mar 9, 2022

View reviewed changes

TheBlueMatt reviewed Mar 9, 2022

View reviewed changes

dunxen force-pushed the 2022-03-mpp-receive-timeout branch from 37afbe4 to 5403dfb Compare March 11, 2022 17:14

dunxen marked this pull request as ready for review March 11, 2022 17:23

valentinewallace self-requested a review March 11, 2022 19:57

valentinewallace previously approved these changes Mar 11, 2022

View reviewed changes

TheBlueMatt reviewed Mar 11, 2022

View reviewed changes

dunxen dismissed valentinewallace’s stale review via b0b3ad1 March 12, 2022 16:19

dunxen force-pushed the 2022-03-mpp-receive-timeout branch 2 times, most recently from b0b3ad1 to d0369db Compare March 12, 2022 16:57

TheBlueMatt previously approved these changes Mar 15, 2022

View reviewed changes

valentinewallace reviewed Mar 16, 2022

View reviewed changes

dunxen dismissed TheBlueMatt’s stale review via daa89dd March 22, 2022 13:50

dunxen force-pushed the 2022-03-mpp-receive-timeout branch from d0369db to daa89dd Compare March 22, 2022 13:50

TheBlueMatt previously approved these changes Mar 22, 2022

View reviewed changes

TheBlueMatt assigned valentinewallace Mar 22, 2022

Add MPP receive timeout handling

4086ce8

dunxen dismissed TheBlueMatt’s stale review via 4086ce8 March 22, 2022 18:57

dunxen force-pushed the 2022-03-mpp-receive-timeout branch from daa89dd to 4086ce8 Compare March 22, 2022 18:57

valentinewallace self-requested a review March 22, 2022 19:15

TheBlueMatt approved these changes Mar 22, 2022

View reviewed changes

valentinewallace approved these changes Mar 22, 2022

View reviewed changes

valentinewallace merged commit 74b9c1a into lightningdevkit:main Mar 22, 2022

dunxen deleted the 2022-03-mpp-receive-timeout branch March 23, 2022 06:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MPP receive timeout handling #1353

Add MPP receive timeout handling #1353

dunxen commented Mar 9, 2022 •

edited

Loading

dunxen Mar 9, 2022

TheBlueMatt Mar 9, 2022

TheBlueMatt left a comment

TheBlueMatt Mar 9, 2022

TheBlueMatt Mar 9, 2022

codecov-commenter commented Mar 11, 2022 •

edited

Loading

TheBlueMatt Mar 11, 2022

This comment was marked as outdated.

dunxen Mar 12, 2022

TheBlueMatt left a comment

dunxen commented Mar 16, 2022

valentinewallace left a comment

valentinewallace Mar 16, 2022

dunxen Mar 17, 2022

valentinewallace Mar 16, 2022

dunxen commented Mar 22, 2022

TheBlueMatt Mar 22, 2022

Add MPP receive timeout handling #1353

Add MPP receive timeout handling #1353

Conversation

dunxen commented Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 11, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

dunxen commented Mar 16, 2022

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunxen commented Mar 22, 2022

Choose a reason for hiding this comment

dunxen commented Mar 9, 2022 •

edited

Loading

codecov-commenter commented Mar 11, 2022 •

edited

Loading