Batch forwarding followup#3955
Conversation
We limit the tail latency of our batch forwarding delay.
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3955 +/- ##
==========================================
+ Coverage 88.91% 88.93% +0.01%
==========================================
Files 173 174 +1
Lines 123393 123874 +481
Branches 123393 123874 +481
==========================================
+ Hits 109717 110162 +445
- Misses 11216 11253 +37
+ Partials 2460 2459 -1
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:
|
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
#3891 (comment) really seems trivial to address.
|
👋 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. |
tnull
force-pushed
the
2025-07-batch-fwd-followup
branch
4 times, most recently
from
def4a4d to
e6eb5a2
Compare
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Feel free to squash and we can land this IMO
|
|
||
| // On startup, we wait a bit before attempting the initial forward, which gives the user some time | ||
| // to re-connect peers. | ||
| const INIT_FWD_DELAY: Duration = Duration::from_secs(2); |
There was a problem hiding this comment.
Were you intending to do something fancier here where we look at peer connection counts compared to channel counts? Certainly don't have to, its not a new thing, but we'd discussed it so wasn't sure if you wanted to.
There was a problem hiding this comment.
Were you intending to do something fancier here where we look at peer connection counts compared to channel counts? Certainly don't have to, its not a new thing, but we'd discussed it so wasn't sure if you wanted to.
Hmm, I didn't intend to for now, see #3891 (comment)
I think the only reliable thing we could add is to shortcut (i.e., avoid the 2 second delay) if we already have all peers connected. But apart from that, I'm not sure we could lean on much more?
There was a problem hiding this comment.
Yea, I agree that's probably all we can realistically do. Seems like a nice optimization tho, especially for mobiles with one or two channel(s)/LSP(s).
We previously introduced an additional `loop_exit_check` in background processor. Since all reviewers seemed to be skeptical about its usefulness, we drop it again here.
.. as requested on the original PR, we combine the two `sleeper` futures into a single one.
In addition to randomizing the millisecond cound, we randomize at the microsecond resolution by drawing from a uniform distribution over the interval [0; 1000).
On startup, we wait a bit before attempting the initial forward, which gives the user some time to re-connect peers.
tnull
force-pushed
the
2025-07-batch-fwd-followup
branch
from
e6eb5a2 to
5b583a9
Compare
Squashed the fixup |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Trivial, despite the large diff size in constants, landing.
3 participants
Just a quick follow-up to #3891, addressing the outstanding comments there.