Probing service follow-ups #975

Activity

1. tnull

   added this to the 0.8 milestone on Jul 9, 2026
2. joostjager commented on Jul 16, 2026

   

   joostjager

   on Jul 16, 2026

   Contributor

   More actions

   One additional probing follow-up came up in this review comment on #660: the restart fallback is brittle, can undercount partially restored probes, and otherwise counts unrelated outbound HTLCs.
3. randomlogin commented on Jul 16, 2026

   

   randomlogin

   on Jul 16, 2026

   Contributor

   More actions

   Noted, work in progress.
4. randomlogin commented on Jul 21, 2026

   

   randomlogin

   on Jul 21, 2026

   Contributor

   More actions

   One additional probing follow-up came up in this review comment on #660: the restart fallback is brittle, can undercount partially restored probes, and otherwise counts unrelated outbound HTLCs.

   @joostjager could you please elaborate your fix? Why just summing all RecentPaymentDetails which are probes can be not enough to get the locked in-flight probes amount? Was your concern connected with #981?
5. joostjager commented on Jul 22, 2026

   

   joostjager

   on Jul 22, 2026

   Contributor

   More actions

   I didn't mention a fix there, just brought up the problem. Using RecentPaymentDetails is only correct once state has been settled.
6. randomlogin commented on Jul 22, 2026

   

   randomlogin

   on Jul 22, 2026

   Contributor

   More actions

   I'm trying to understand the problem.
   Currently we have this for the locked probing amount:

   	pub fn locked_msat(&self) -> u64 {
   		return self
   			.channel_manager
   			.list_recent_payments()
   			.into_iter()
   			.filter_map(|p| match p {
   				RecentPaymentDetails::Pending {
   					is_probe: true,
   					total_msat,
   					pending_fee_msat,
   					..
   				} => Some(total_msat + pending_fee_msat.unwrap_or(0)),
   				_ => None,
   			})
   			.sum();
   	}

   During normal operation list_recent_payment reads pending_outbound_payments from ChannelManager, it happily lives in the memory with complete data which is populated once (actually even before) the payment is sent.

   The only problem is persistence: channel manager persists asynchronously and in case of crash we might not have written state to disk. On the contrary, channel monitor's persistence is synchronous and we can rely on it upon restart.

   When we restart, we call ChannelManager::read() which checks whether channel monitor's state differs from channel manager's one. If so (we lost something in the channel manager), we are going to force close channel, the probe will be immediately resolved and then the channel is closed. And so we don't need to make any changes?

   Correct me please if I overlooked something.
7. joostjager commented on Jul 22, 2026

   

   joostjager

   on Jul 22, 2026

   Contributor

   More actions

   Maybe no change is needed then. It seemed the original fallback that was later removed from #660 was there for a reason, @tnull mentioned a fix possibly needed, but I can't retrieve that revision of the PR anymore. Perhaps the original author @tankyleo can comment on it?

   👍React with 👍1Reacted by Alexander Shevtsov
8. tnull commented on Jul 24, 2026

   

   tnull

   on Jul 24, 2026

   CollaboratorAuthor

   More actions

   Yes, might very well be that we don't need any additional fixes beyond what #981 already did.

   Would be good to still tackle the remaining comments on #815 (review) though (cc @randomlogin)
9. randomlogin commented on Jul 24, 2026

   

   randomlogin

   on Jul 24, 2026

   Last edited by randomlogin

   Contributor

   More actions

   Yeah, I had PR already ready for all the rest, just wanted to clarify that point before opening it.
10. randomlogin commented on Jul 24, 2026

    

    randomlogin

    on Jul 24, 2026

    Last edited by randomlogin

    Contributor

    More actions

    Another thing left there was:

    Codex:
    Test reliability: /home/tnull/worktrees/ldk-node/pr-815/tests/probing_tests.rs:124 is bind-sensitive. cargo test --test probing_tests failed twice with InvalidSocketAddress from /home/tnull/worktrees/ldk-node/pr-815/tests/common/mod.rs:739.
    The failed test passed when run alone, so thislooks like fixed-port test setup contention, but the added test target is not reliably green as-is.

    I couldn't reproduce, but can it be that that test run gave the same socket address for different tests?
11. tnull commented on Jul 24, 2026

    

    tnull

    on Jul 24, 2026

    CollaboratorAuthor

    More actions

    Another thing left there was:

    Codex:
    Test reliability: /home/tnull/worktrees/ldk-node/pr-815/tests/probing_tests.rs:124 is bind-sensitive. cargo test --test probing_tests failed twice with InvalidSocketAddress from /home/tnull/worktrees/ldk-node/pr-815/tests/common/mod.rs:739.
    The failed test passed when run alone, so thislooks like fixed-port test setup contention, but the added test target is not reliably green as-is.

    I couldn't reproduce, but can it be that that test run gave the same socket address for different tests?

    Yeah, that is one well-known source of test flakiness that we haven't fully fixed yet. Feel free to ignore for your PR.
12. tnull

    closed this as completedin #1004on Jul 27, 2026