Hold times for successful payments #3801

joostjager · 2025-05-26T11:55:36Z

Apply the hold time reporting mechanism as implemented in #2256 to the success case. Spec in lightning/bolts#1044

ldk-reviews-bot · 2025-05-26T11:55:39Z

👋 Thanks for assigning @valentinewallace as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-05-27T12:02:51Z

Codecov Report

Attention: Patch coverage is 85.88710% with 210 lines in your changes missing coverage. Please review.

Project coverage is 88.82%. Comparing base (257ebad) to head (5519b4c).

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	84.72%	116 Missing and 7 partials ⚠️
lightning/src/ln/channel.rs	84.44%	75 Missing and 11 partials ⚠️
lightning/src/ln/payment_tests.rs	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3801      +/-   ##
==========================================
- Coverage   88.82%   88.82%   -0.01%     
==========================================
  Files         165      165              
  Lines      119075   119955     +880     
  Branches   119075   119955     +880     
==========================================
+ Hits       105769   106550     +781     
- Misses      10986    11101     +115     
+ Partials     2320     2304      -16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ldk-reviews-bot · 2025-07-07T00:00:40Z

🔔 1st Reminder

Hey @valentinewallace @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-07-07T00:00:43Z

🔔 1st Reminder

Hey @valentinewallace @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

valentinewallace

Have an open question on persistence/restarts but otherwise LGTM pending a second reviewer. check_commits / fuzz need fixing in CI.

valentinewallace · 2025-07-08T19:37:34Z

lightning/src/ln/payment_tests.rs

+	#[cfg(feature = "std")]
+	thread::sleep(Duration::from_millis(200));


Could use a comment. Tbh, this part is pretty unrelated to the test that it's currently in.

I can create a new single path test for it, would that be better?

valentinewallace · 2025-07-08T19:52:42Z

lightning/src/ln/onion_utils.rs

+				attribution_data.shift_left();
+			},
+			Err(()) => {
+				log_debug!(


Can you comment that we will hit this as soon as we hit a node that doesn't support the attr failures feature?

valentinewallace · 2025-07-08T20:13:20Z

lightning/src/ln/channelmanager.rs

+				let attribution_data = process_fulfill_attribution_data(
+					None,
+					&htlc.prev_hop.incoming_packet_shared_secret,
+					0,


Can we check for this behavior in the existing test, i.e. the recipient will always set the final hold time to 0?

Added to the test_fulfill_hold_times

valentinewallace · 2025-07-08T20:17:15Z

lightning/src/ln/onion_utils.rs

@@ -2817,6 +2817,27 @@ fn process_failure_packet(
 	update_attribution_data(onion_error, shared_secret, hold_time);
 }

+pub(crate) fn process_fulfill_attribution_data(


Can we add a comment for this method, including that it's for intermediate/final nodes?

valentinewallace · 2025-07-08T20:33:19Z

lightning/src/ln/onion_utils.rs

+			// Shift attribution data to prepare for processing the next hop.
+			attribution_data.shift_left();
+		} else {
+			log_debug!(


Discussed offline, not sure that's adding up for me because we currently persist a bunch of stuff in Channel, which we shouldn't be doing if we expect attribution data to be None after restart anyway.

ldk-reviews-bot · 2025-07-09T00:01:33Z

🔔 2nd Reminder

Hey @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

joostjager · 2025-07-11T13:55:28Z

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -2901,6 +2901,8 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim, second_f
 		);
 		check_added_monitors!(nodes[2], 1);
 		get_htlc_update_msgs!(nodes[2], node_b_id);
+	// Note that we don't populate fulfill_msg.attribution_data here, which will lead to hold times being


Rustfmt 🤷‍♂️

joostjager · 2025-07-11T15:23:05Z

Comments addressed. Pre-rebase diff: https://github.com/lightningdevkit/rust-lightning/compare/cd289da..bc46f299a7ad61fe9e4eabdd614085c7bb2fca5f

and a few after-rebase touch ups: https://github.com/lightningdevkit/rust-lightning/compare/bff580f..23f52732a4969d8323b8998717736b6dec3ae1da

Preparation for reuse of the logic.

Need to store AttributionData as part of the inbound HTLC removal reason so that it can be used in the upstream UpdateFulfillHTLC message.

Necessary to preserve attribution data when the HTLC is in the holding cell.

Adds hold time reporting for the final and intermediate nodes.

joostjager · 2025-07-11T15:55:38Z

lightning/src/ln/channelmanager.rs

+				let session_priv = if path.has_trampoline_hops() {
+					let session_priv_hash =
+						Sha256::hash(&session_priv.secret_bytes()).to_byte_array();
+					derived_key = SecretKey::from_slice(&session_priv_hash[..]).unwrap();


Directly taking a ref to this only seems to work in newer rust versions.

AttributionData is needed as part of the outbound HTLC outcome when revoke_and_ack has happened and the AttributionData is decoded to get the hold times for inclusion in the PaymentPathSuccessful event.

Prepare for inspecting hold times in PaymentPathSuccessful.

Hold times are surfaced via the PaymentPathSuccessful event.

ldk-reviews-bot · 2025-07-12T00:00:36Z

🔔 3rd Reminder

Hey @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

joostjager changed the title ~~Successful payment hold times~~ Hold times for successful payments May 26, 2025

joostjager force-pushed the fulfill-hold-times branch 7 times, most recently from badc803 to e913312 Compare May 27, 2025 09:52

joostjager mentioned this pull request May 27, 2025

Simplify UpdateFulfillFetch::NewClaim #3803

Merged

joostjager force-pushed the fulfill-hold-times branch from e913312 to 69a99b4 Compare May 27, 2025 11:53

joostjager force-pushed the fulfill-hold-times branch from 69a99b4 to 1f56e04 Compare May 27, 2025 12:24

joostjager mentioned this pull request May 27, 2025

Success hold times lightning/bolts#1261

Closed

joostjager mentioned this pull request Jun 5, 2025

Attribution data (feature 36/37) lightning/bolts#1044

Open

joostjager force-pushed the fulfill-hold-times branch 8 times, most recently from bace463 to 1c64a50 Compare June 26, 2025 13:52

joostjager marked this pull request as ready for review June 26, 2025 14:16

joostjager marked this pull request as draft June 26, 2025 14:17

joostjager added this to Weekly Goals Jun 26, 2025

joostjager self-assigned this Jun 26, 2025

joostjager force-pushed the fulfill-hold-times branch 2 times, most recently from 233cbfc to f5668f5 Compare June 30, 2025 09:36

joostjager force-pushed the fulfill-hold-times branch from 3aed96e to b05cc65 Compare July 4, 2025 08:11

joostjager requested a review from valentinewallace July 4, 2025 08:12

joostjager force-pushed the fulfill-hold-times branch from b05cc65 to cd289da Compare July 8, 2025 18:05

valentinewallace reviewed Jul 8, 2025

View reviewed changes

joostjager force-pushed the fulfill-hold-times branch from cd289da to fd4740c Compare July 11, 2025 13:54

joostjager commented Jul 11, 2025

View reviewed changes

joostjager force-pushed the fulfill-hold-times branch from fd4740c to bc46f29 Compare July 11, 2025 15:16

joostjager added 2 commits July 11, 2025 17:17

Rustfmt parts of channel.rs and channelmanager.rs

1ed3121

Rustfmt touch ups

c6d6561

joostjager force-pushed the fulfill-hold-times branch from bc46f29 to bff580f Compare July 11, 2025 15:22

joostjager added 7 commits July 11, 2025 17:24

Small refactors in onion_utils

483d97b

Extract hold time calculation

b19f911

Preparation for reuse of the logic.

Add AttributionData to UpdateFulfillHTLC message

7947c0d

Add AttributionData to InboundHTLCRemovalReason::Fulfill

55e9449

Need to store AttributionData as part of the inbound HTLC removal reason so that it can be used in the upstream UpdateFulfillHTLC message.

Add AttributionData to HTLCUpdateAwaitingACK::ClaimHTLC

d8ae3d0

Necessary to preserve attribution data when the HTLC is in the holding cell.

Hold time reporting

4861747

Adds hold time reporting for the final and intermediate nodes.

Remove code duplication in FundedChannel::read

526e9aa

joostjager force-pushed the fulfill-hold-times branch 2 times, most recently from c440aed to 664b471 Compare July 11, 2025 15:54

joostjager commented Jul 11, 2025

View reviewed changes

joostjager force-pushed the fulfill-hold-times branch from 664b471 to a69eaef Compare July 11, 2025 15:56

joostjager added 3 commits July 11, 2025 17:59

Add AttributionData to OutboundHTLCOutcome::Success

1da5cd2

AttributionData is needed as part of the outbound HTLC outcome when revoke_and_ack has happened and the AttributionData is decoded to get the hold times for inclusion in the PaymentPathSuccessful event.

Return path events from expect_payment_sent

2b18bf6

Prepare for inspecting hold times in PaymentPathSuccessful.

Decode hold times as a sender

23f5273

Hold times are surfaced via the PaymentPathSuccessful event.

joostjager force-pushed the fulfill-hold-times branch from a69eaef to 23f5273 Compare July 11, 2025 16:00

		#[cfg(feature = "std")]
		thread::sleep(Duration::from_millis(200));

Hold times for successful payments #3801

Are you sure you want to change the base?

Hold times for successful payments #3801

Uh oh!

Conversation

joostjager commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ldk-reviews-bot commented Jul 7, 2025

Uh oh!

ldk-reviews-bot commented Jul 7, 2025

Uh oh!

valentinewallace left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Jul 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Jul 12, 2025

Uh oh!

Uh oh!

joostjager commented May 26, 2025 •

edited

Loading

ldk-reviews-bot commented May 26, 2025 •

edited

Loading

codecov bot commented May 27, 2025 •

edited

Loading

valentinewallace left a comment •

edited

Loading

joostjager commented Jul 11, 2025 •

edited

Loading