Uh oh!
There was an error while loading. Please reload this page.
Hold times for successful payments - #3801
Conversation
👋 Thanks for assigning @tnull as a reviewer! |
badc803 to
e913312Comparee913312 to
69a99b4CompareCodecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #3801 +/- ##
==========================================
+ Coverage 88.82% 89.13% +0.31%
==========================================
Files 166 166 Lines 119487 123678 +4191 Branches 119487 123678 +4191 ==========================================
+ Hits 106136 110246 +4110 - Misses 11021 11104 +83 + Partials 2330 2328 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
69a99b4 to
1f56e04Comparebace463 to
1c64a50Compare233cbfc to
f5668f5Compareldk-reviews-bot
commented
Jul 12, 2025
🔔 3rd Reminder Hey @carlaKC! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Jul 14, 2025
🔔 4th Reminder Hey @carlaKC! This PR has been waiting for your review. |
23f5273 to
1994550Compare1994550 to
741ee1eComparejoostjager
commented
Jul 15, 2025
Rebased |
ldk-reviews-bot
commented
Jul 16, 2025
🔔 5th Reminder Hey @valentinewallace@carlaKC! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Jul 16, 2025
🔔 1st Reminder Hey @valentinewallace@carlaKC! This PR has been waiting for your review. |
TheBlueMatt
left a comment
There was a problem hiding this comment.
Not super happy with how much this ended up touching but the hold times parts LGTM. The rustfmt part needs rebase, though, and I'm kinda skeptical of starting a PR with 1423 insertions(+), 570 deletions(-) when the rest of the PR is only 625 insertions(+), 196 deletions(-), seems like the rustfmt part should be separated out if you want to do it first.
| let attribution_data = process_fulfill_attribution_data( | ||
| attribution_data.as_ref(), | ||
| &htlc.prev_hop.incoming_packet_shared_secret, | ||
| 0, |
There was a problem hiding this comment.
Ha, saved by the rounding :). Without the rounding this will clearly give away that we're a phantom node. As-is it instead can also be a peer that is within 100ms of us :)
| /// | ||
| /// May contain a closed channel if the HTLC sent along the path was fulfilled on chain. | ||
| path: Path, | ||
| /// The hold times as reported by each hop. |
There was a problem hiding this comment.
If we're going to expose this we really need to document in more detail what the values represent (100ms chunks where previous hops should encompass later hops' hold times) and how to use it (for deciding which node is slow in scoring, noting that our current scorer does not do this). Also, we should probably include it in PaymentPathFailed too?
There was a problem hiding this comment.
Docs added and hold_times exposed outside of tests in PaymentPathFailed.
741ee1e to
bca896aComparePreparation 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.
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.
a6c8603 to
4831900CompareComments addressed diff |
Hold times are surfaced via the PaymentPathSuccessful event.
Now that fulfill hold times are surfaced in the event always, this commit follow ups with the failure hold times.
TheBlueMatt
left a comment
There was a problem hiding this comment.
LGTM. A few nits but I'm happy to address them post-merge, there's probably a few other things I want to tweak post-merge (at least making the fulfill message passed in owned instead of reference to remove some clones).
| } | ||
| fn duration_since_epoch() -> Option<Duration> { | ||
| pub(crate) fn duration_since_epoch() -> Option<Duration> { |
There was a problem hiding this comment.
Its a bit confusing to use this in channelmanager.rs - we already have a ChannelManager::duration_since_epoch that uses the block time if we're built without time access, but also every call to hold_time takes the result of this call as the second argument, so it seems like this should just be inlined into hold_time (dunno if you'd want to rename it after doing so).
| (58, self.interactive_tx_signing_session, option), // Added in 0.2 | ||
| (59, self.funding.minimum_depth_override, option), // Added in 0.2 | ||
| (60, self.context.historical_scids, optional_vec), // Added in 0.2 | ||
| (61, fulfill_attribution_data, optional_vec), |
| (58, interactive_tx_signing_session, option), // Added in 0.2 | ||
| (59, minimum_depth_override, option), // Added in 0.2 | ||
| (60, historical_scids, optional_vec), // Added in 0.2 | ||
| (61, fulfill_attribution_data, optional_vec), |
| (55, removed_htlc_attribution_data, optional_vec), | ||
| (57, holding_cell_attribution_data, optional_vec), |
There was a problem hiding this comment.
These could get the added-in tag that exists on the write side too, while we're here.
valentinewallace
left a comment
There was a problem hiding this comment.
Mainly reviewed the diffs since my last LGTM. Nothing blocking 👍
| ); | ||
| 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 |
There was a problem hiding this comment.
I think it's putting the comment on the else, we could move it up a line?
| /// milliseconds. So a hop reporting 2 is a hold time that corresponds to roughly 200 milliseconds. As earlier | ||
| /// hops hold on to an HTLC for longer, the hold times in the list are expected to decrease. When our peer | ||
| /// didn't provide attribution data, the list is empty. The same applies to HTLCs that were resolved onchain. | ||
| /// Because of unavailability of hold times, the list may be shorter than the number of hops in the path. |
There was a problem hiding this comment.
I think it might be useful to clarify that the hold time at idx 0 corresponds to the hop at path.hops[0], etc
Apply the hold time reporting mechanism as implemented in #2256 to the success case. Spec in lightning/bolts#1044