#4059 followups

[TheBlueMatt]

Mostly some issues in persistence, but also one doc fix.

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

❌ Patch coverage is 38.38384% with 61 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.53%. Comparing base (e929517) to head (a9ddf3f).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning-liquidity/src/lsps2/service.rs	40.74%	27 Missing and 5 partials ⚠️
lightning-liquidity/src/lsps5/service.rs	35.55%	24 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4118      +/-   ##
==========================================
+ Coverage   88.39%   88.53%   +0.13%     
==========================================
  Files         178      179       +1     
  Lines      134179   134413     +234     
  Branches   134179   134413     +234     
==========================================
+ Hits       118610   119002     +392     
+ Misses      12895    12653     -242     
- Partials     2674     2758      +84     

Flag	Coverage Δ
fuzzing	21.23% <0.00%> (?)
tests	88.37% <38.38%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Hmm, if we want to check again here, why bother with the two need_remove/need_persist lists in the first place? Wouldn't it be easier to keep the two steps then separate, ie. first just iterate all peers and repersist what needs persisting, then retain and remove any prunable peers in one go? Seems like keeping the two steps separate might be simpler?

[TheBlueMatt]

Yea, this did end up a bit messier than I was hoping. One thing I still want to avoid, though, is holding the top-level write lock while persisting. Its not clear to me how to accomplish that without two passes (unless we want to do a loop-until-we-loop-and-dont-find-peers-to-remove which I guess we could but feels a bit wasteful). Do you have any suggestions there?

LSPS5 doesn't have inner peer locks, so we ultimately take the write lock inside the second loop (and into a persist!) as well. There the three-pass thing is definitely a weaker argument, but I'm still not sure if a loop-until-we-loop-and-dont-find-peers-to-remove is better.

[tnull]

Use fetch_update to avoid the race here?

[TheBlueMatt]

Hmm? Not sure which race you're referring to. I copied this logic from PeerManager.

[tnull]

Isn't there a potential race between fetch_sub and store below? Although granted I don't believe it would have much consequence, doing both in one operation just seemed cleaner.

[TheBlueMatt]

Hmmm, I actually think its correct. If we do a fetch_sub and the number we get back is greater than 1, we still "own" the "mutex" - no other persist should be in the loop. We can thus write one to reset the counter such that we won't loop too many times. fetch_update isn't atomic, its actually a compare_and_swap loop which I think doens't quite work.

[ldk-reviews-bot]

👋 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.

[TheBlueMatt]

Rebased and addressed the issue @tnull identified.

[TheBlueMatt]

Squashed without further changes.

[tnull]

One comment, otherwise LGTM

[TheBlueMatt]

Made the removal check in LSPS2 consistent, as well as made the counterparty node id nomenclature consistent:

$ git diff-tree -U2 b099dd6976 a9ddf3f1c9
diff --git a/lightning-liquidity/src/lsps5/service.rs b/lightning-liquidity/src/lsps5/service.rs
index ca04ccf179..1111c682fb 100644
--- a/lightning-liquidity/src/lsps5/service.rs
+++ b/lightning-liquidity/src/lsps5/service.rs
@@ -275,10 +275,10 @@ where
 			}

-			for counterparty_node_id in need_persist.into_iter() {
-				debug_assert!(!need_remove.contains(&counterparty_node_id));
-				self.persist_peer_state(counterparty_node_id).await?;
+			for client_id in need_persist.into_iter() {
+				debug_assert!(!need_remove.contains(&client_id));
+				self.persist_peer_state(client_id).await?;
 			}

-			for counterparty_node_id in need_remove {
+			for client_id in need_remove {
 				let mut future_opt = None;
 				{
@@ -289,9 +289,9 @@ where
 					// itself.
 					let mut per_peer_state = self.per_peer_state.write().unwrap();
-					if let Entry::Occupied(mut entry) = per_peer_state.entry(counterparty_node_id) {
+					if let Entry::Occupied(mut entry) = per_peer_state.entry(client_id) {
 						let state = entry.get_mut();
-						if state.is_prunable() {
+						if state.is_prunable() && !self.client_has_open_channel(&client_id) {
 							entry.remove();
-							let key = counterparty_node_id.to_string();
+							let key = client_id.to_string();
 							future_opt = Some(self.kv_store.remove(
 								LIQUIDITY_MANAGER_PERSISTENCE_PRIMARY_NAMESPACE,
@@ -312,5 +312,5 @@ where
 					future.await?;
 				} else {
-					self.persist_peer_state(counterparty_node_id).await?;
+					self.persist_peer_state(client_id).await?;
 				}
 			}

[tnull]

Also double-checked this builds and tests on LDK Node.