Uh oh!
There was an error while loading. Please reload this page.
Misc Followups - #3942
Conversation
👋 Thanks for assigning @joostjager as a reviewer! |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| /// Because of unavailability of hold times, the list may be shorter than the number of hops in the path. | ||
| /// The time that each hop indicated it held the HTLC. | ||
| /// | ||
| /// We expect that at each hop the hold time will be strictly greater than the hold time of |
There was a problem hiding this comment.
The unrounded hold times should be strictly greater, but clearly after rounding that may not be the case anymore. Not sure if that's fully clear now.
There was a problem hiding this comment.
I don't think that's true? As long as everyone rounds in the same way (truncating, rather than rounding) a >= condition will hold after rounding.
There was a problem hiding this comment.
I thought strictly greater means > not >=
There was a problem hiding this comment.
Oh lol, yea, I see why its confusing. Pushed a fixup:
$ git diff-tree -U2 517610c2c 15a70f1d9
diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs
index 1a6a18d928..e4fd7ad2ef 100644
--- a/lightning/src/events/mod.rs+++ b/lightning/src/events/mod.rs@@ -1105,11 +1105,12 @@ pub enum Event {
/// The time that each hop indicated it held the HTLC.
///
- /// We expect that at each hop the hold time will be strictly greater than the hold time of- /// the following hops, as a node along the path shouldn't have completed the HTLC until- /// the next node has completed it.- ///
/// The unit in which the hold times are expressed are 100's of milliseconds. So a hop
/// reporting 2 is a hold time that corresponds to between 200 and 299 milliseconds.
///
+ /// We expect that at each hop the actual hold time will be strictly greater than the hold+ /// time of the following hops, as a node along the path shouldn't have completed the HTLC+ /// until the next node has completed it. Note that because hold times are in 100's of ms,+ /// hold times as reported are likely to often be equal across hops.+ ///
/// If our peer didn't provide attribution data or the HTLC resolved on chain, the list
/// will be empty.
@@ -1172,11 +1173,12 @@ pub enum Event {
/// The time that each hop indicated it held the HTLC.
///
- /// We expect that at each hop the hold time will be strictly greater than the hold time of- /// the following hops, as a node along the path shouldn't have completed the HTLC until- /// the next node has completed it.- ///
/// The unit in which the hold times are expressed are 100's of milliseconds. So a hop
/// reporting 2 is a hold time that corresponds to between 200 and 299 milliseconds.
///
+ /// We expect that at each hop the actual hold time will be strictly greater than the hold+ /// time of the following hops, as a node along the path shouldn't have completed the HTLC+ /// until the next node has completed it. Note that because hold times are in 100's of ms,+ /// hold times as reported are likely to often be equal across hops.+ ///
/// If our peer didn't provide attribution data or the HTLC resolved on chain, the list
/// will be empty.ldk-reviews-bot
commented
Jul 18, 2025
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
Most of the message handlers in LDK pass messages to the handler by reference. This is unnecessarily inefficient and there's no real reason to do so. Because we just tweaked what `UpdateFulfillHTLC` messages look like, this is a good opportunity to make progress here, passing at least one message owned rather than by reference.
In the previous commit we switched to passing `UpdateFulfillHTLC` messages to handlers owned, rather than by reference. Here we update `ChanelManager`'s handling of fulfill attribution data to avoid unnecessary clones now that we own the object.
865519c to
517610cCompareTheBlueMatt
commented
Jul 20, 2025
Fixed a bug in the bench build: $ git diff-tree -U3 865519c6b 517610c2c
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index e9052209d4..76c972fe8a 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -18317,7 +18317,7 @@ pub mod bench {
expect_payment_claimed!(ANodeHolder { node: &$node_b }, payment_hash, 10_000);
match $node_b.get_and_clear_pending_msg_events().pop().unwrap() {
- MessageSendEvent::UpdateHTLCs { node_id, channel_id: _, updates } => {+ MessageSendEvent::UpdateHTLCs { node_id, mut updates, .. } => {
assert_eq!(node_id, $node_a.get_our_node_id());
let fulfill = updates.update_fulfill_htlcs.remove(0);
$node_a.handle_update_fulfill_htlc($node_b.get_our_node_id(), fulfill); |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #3942 +/- ##
=======================================
Coverage 89.05% 89.05% =======================================
Files 167 167 Lines 121800 121812 +12 Branches 121800 121812 +12 =======================================
+ Hits 108464 108482 +18 + Misses 10928 10927 -1 + Partials 2408 2403 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
517610c to
15a70f1CompareUh oh!
There was an error while loading. Please reload this page.
joostjager
commented
Jul 23, 2025
Merged with one ack because high confidence in Rust type safety |
Mostly post-#3801 things, not entirely.