[LSPS5 FollowUp] Simplify LSPS5/client peer state by martinsaposnic · Pull Request #3969 · lightningdevkit/rust-lightning

martinsaposnic · 2025-07-28T18:32:02Z

Addressing some LSPS5 follow ups, specifically comments #3662 (comment) and #3662 (comment)

This PR introduces changes so the LSPS5/client drops the TimeProvider, and now instead of just expiring pending requests after a certain time, it just keeps the 5 most recent requests of each type (the 5 here can be changed, not married to the value, I put it just to put something. Thoughts @tnull @TheBlueMatt ?)

My preference for implementing this was:

Keep the ergonomics of the hashmap's O(1) get function
Have something like VecDeque with_capacity to be able to truncate / evict the last value when a new value comes in
LSPS5/client was recently reviewed so I wanted to minimize changes for the reviewers happiness

Some alternatives for making this change were

Add a new dependency like linked-hash-map (just saying it out loud makes me nervous lol)
Have a Vec or VecDeque with capacity to keep track of the requests, truncate when limit is reached, and then do vec.iter().position(|v, _, _| v == r_id) for retrieval (which is kind of ugly and not O(1) like in hashmaps)
Define a new type BoundedMap, which is a fixed size map with a defined capacity and FIFO eviction

I went with the last option, but I'm open to alternatives

Also, the last commit introduces a change where the peerstate is cleaned if there are no pending requests with that peer

ldk-reviews-bot · 2025-07-28T18:32:05Z

👋 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 · 2025-07-28T18:46:09Z

Codecov Report

❌ Patch coverage is 96.29630% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.99%. Comparing base (61e5819) to head (cf23e91).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning-liquidity/src/lsps5/client.rs	96.25%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3969 +/- ##
==========================================
+ Coverage 88.94% 88.99% +0.04% 
==========================================
Files 174 174 Lines 124201 124199 -2 Branches 124201 124199 -2 ==========================================
+ Hits 110472 110525 +53 + Misses 11251 11200 -51 + Partials 2478 2474 -4

Flag	Coverage Δ
fuzzing	`22.64% <0.00%> (+<0.01%)`	⬆️
tests	`88.81% <96.29%> (+0.04%)`	⬆️

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

Did a first pass, mostly looks good.

tnull · 2025-07-29T12:29:37Z

+ }
+
+ self.map.insert(key.clone(), value);
+ self.order.push_back(key.clone());


I think this clone is unnecessary?

tnull · 2025-07-29T12:31:55Z

+ if let Some(oldest) = self.order.pop_front() {
+ self.map.remove(&oldest);
+ }
+ }


Can we sprinkle in some debug_asserts ensuring that neither map nor order ever exceed cap at the end of the methods taking &mut?

tnull · 2025-07-29T12:35:45Z

+ assert!(m.get(&"key").is_none());
+	}
+
+	#[test]


These tests would be a prime example were proptests would be easily applicable and would extend test coverage.

tnull · 2025-07-29T12:36:49Z

 	}
 }

+pub(crate) mod bounded_map;


Huh, why are these mod definitions at the bottom by now? Let's move them to the top please, and let's keep pub and pub(crate) definitions in separate groups.

tnull · 2025-07-29T12:38:35Z

 };
 self.with_peer_state(*counterparty_node_id, handle_response);
+
+ let mut outer_state_lock = self.per_peer_state.write().unwrap();


Mind moving this out to a dedicated method?

ldk-reviews-bot · 2025-07-29T12:40:52Z

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

martinsaposnic · 2025-07-30T18:57:28Z

thanks for the review @tnull ! all comments are addressed in fixup commits

tnull

Feel free to squash fixups!

martinsaposnic · 2025-07-31T12:24:55Z

done! @tnull

tnull

LGTM

ldk-reviews-bot · 2025-08-01T13:28:44Z

✅ Added second reviewer: @valentinewallace

TheBlueMatt

Have a Vec or VecDeque with capacity to keep track of the requests, truncate when limit is reached, and then do vec.iter().position(|v, _, _| v == r_id) for retrieval (which is kind of ugly and not O(1) like in hashmaps)

Sure, but for N=5 its probably faster than doing the SipHash-1-3 required to do that HashMap O(1), not to mention it avoids a second allocated datastructure. Let's definitely not overengineer this - walking a Vec is totally fine for N=5.

Client will no longer use a time provider to expire pending requests. Instead, it will just keep the X most recent requests (coming in next commit).

martinsaposnic · 2025-08-04T14:49:26Z

Have a Vec or VecDeque with capacity to keep track of the requests, truncate when limit is reached, and then do vec.iter().position(|v, _, _| v == r_id) for retrieval (which is kind of ugly and not O(1) like in hashmaps)
Sure, but for N=5 its probably faster than doing the SipHash-1-3 required to do that HashMap O(1), not to mention it avoids a second allocated datastructure. Let's definitely not overengineer this - walking a Vec is totally fine for N=5.

thanks for reviewing @TheBlueMatt

just pushed fixup commit ec6a9f0 where I drop the bounded_map and change everything so it uses vecs for keeping track of the client's pending requests. let me know what you think

TheBlueMatt

I think this LGTM, feel free to squash if @tnull is fine with dropping the fancy map.

tnull

Fixup LGTM, please squash so we can get this landed.

martinsaposnic · 2025-08-05T11:01:40Z

thanks for taking a look @tnull. fixup squashed!

offtopic question about process:

I'm not sure if there's an easy way to verify that the squash was actually just a squash and nothing more. for example, I just split the fixup into the last two commits and reworded the second one. I did not change anything else, but there's no easy way for you to quickly confirm that I didn't accidentally change something else

tnull · 2025-08-05T12:44:29Z

I'm not sure if there's an easy way to verify that the squash was actually just a squash and nothing more. for example, I just split the fixup into the last two commits and reworded the second one. I did not change anything else, but there's no easy way for you to quickly confirm that I didn't accidentally change something else

For one you can hit the 'Compare' button above to see the diff between pushes. I also always check out the working branch locally to review, so can simply do a diff to see if/what changed. As a bonus you can use range-diff to review the diff even after a rebase.

tnull

Going ahead landing this given that the current state had two 'LGTM' before

ldk-reviews-bot requested a review from valentinewallace July 28, 2025 18:42

martinsaposnic force-pushed the simplify-lsps5-client-peer-state branch from c8e9948 to fba3056Compare July 28, 2025 19:36

tnull requested review from tnull and removed request for valentinewallace July 29, 2025 08:49

tnull reviewed Jul 29, 2025
View reviewed changes

martinsaposnic mentioned this pull request Jul 30, 2025
LSPS5 Follow ups #3944
Closed
18 tasks

martinsaposnic requested a review from tnull July 30, 2025 18:57

graphite-appBot reviewed Jul 30, 2025
View reviewed changes

Comment threadlightning-liquidity/src/lsps5/client.rs

graphite-appBot reviewed Jul 30, 2025
View reviewed changes

Comment threadlightning-liquidity/src/lsps5/client.rs

martinsaposnic force-pushed the simplify-lsps5-client-peer-state branch from 1332159 to 91c7904Compare July 30, 2025 19:35

tnull reviewed Jul 31, 2025
View reviewed changes

martinsaposnic force-pushed the simplify-lsps5-client-peer-state branch from 91c7904 to 5d25ab1Compare July 31, 2025 12:14

martinsaposnic requested a review from tnull July 31, 2025 12:24

martinsaposnic force-pushed the simplify-lsps5-client-peer-state branch from 5d25ab1 to 993c366Compare July 31, 2025 16:01

tnull previously approved these changes Aug 1, 2025
View reviewed changes

ldk-reviews-bot requested a review from valentinewallace August 1, 2025 13:28

tnull requested review from TheBlueMatt and removed request for valentinewallace August 1, 2025 13:28

TheBlueMatt reviewed Aug 1, 2025
View reviewed changes

Get rid of TimeProvider on LSPS5/client.
d2c7c66
Client will no longer use a time provider to expire pending requests. Instead, it will just keep the X most recent requests (coming in next commit).

martinsaposnic dismissed tnull’s stale review via ec6a9f0August 4, 2025 14:45

martinsaposnic force-pushed the simplify-lsps5-client-peer-state branch from 993c366 to ec6a9f0Compare August 4, 2025 14:45

martinsaposnic requested a review from TheBlueMatt August 4, 2025 14:49

martinsaposnic mentioned this pull request Aug 4, 2025
LSPS5 -> More follow ups #3987
Merged

martinsaposnic requested a review from tnull August 4, 2025 15:27

TheBlueMatt reviewed Aug 4, 2025
View reviewed changes

tnull reviewed Aug 5, 2025
View reviewed changes

martinsaposnic added 2 commits August 5, 2025 07:53

Use VecDeque on LSPS5/Client to limit amount of pending requests
9ab2d27

Remove peerState if there are no pending requests with that peer
cf23e91

martinsaposnic force-pushed the simplify-lsps5-client-peer-state branch from ec6a9f0 to cf23e91Compare August 5, 2025 10:54

martinsaposnic requested a review from tnull August 5, 2025 11:34

tnull approved these changes Aug 5, 2025
View reviewed changes

tnull merged commit d704eb8 into lightningdevkit:mainAug 5, 2025
23 of 24 checks passed

Uh oh!

Conversation

martinsaposnic commented Jul 28, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Jul 28, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecovBot commented Jul 28, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnullJul 29, 2025

Choose a reason for hiding this comment

Uh oh!

tnullJul 29, 2025

Choose a reason for hiding this comment

Uh oh!

tnullJul 29, 2025

Choose a reason for hiding this comment

Uh oh!

tnullJul 29, 2025

Choose a reason for hiding this comment

Uh oh!

tnullJul 29, 2025

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Jul 29, 2025

Uh oh!

martinsaposnic commented Jul 30, 2025

Uh oh!

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

martinsaposnic commented Jul 31, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Aug 1, 2025

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

martinsaposnic commented Aug 4, 2025

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

martinsaposnic commented Aug 5, 2025

Uh oh!

tnull commented Aug 5, 2025

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

martinsaposnic commented Jul 28, 2025•
edited
Loading

ldk-reviews-bot commented Jul 28, 2025•
edited
Loading

codecovBot commented Jul 28, 2025•
edited
Loading

tnull left a comment •
edited
Loading