Repository navigation
Probing service follow-ups #975
Description
Activity
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.
Noted, work in progress.
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
RecentPaymentDetailswhich are probes can be not enough to get the locked in-flight probes amount? Was your concern connected with #981?I didn't mention a fix there, just brought up the problem. Using
RecentPaymentDetailsis only correct once state has been settled.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_paymentreadspending_outbound_paymentsfromChannelManager, 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.
- Reacted by Alexander Shevtsov
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)
Yeah, I had PR already ready for all the rest, just wanted to clarify that point before opening it.
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?
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.
In #815 we pointed out some minor nits/comments before merging: #815 (review)
We should address them in a follow-up.
(cc @randomlogin)