Uh oh!
There was an error while loading. Please reload this page.
Add LSPS5 DOS protections. - #3993
Conversation
👋 Thanks for assigning @tnull as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #3993 +/- ##
==========================================
+ Coverage 88.73% 88.75% +0.02%
==========================================
Files 176 176 Lines 129042 129094 +52 Branches 129042 129094 +52 ==========================================
+ Hits 114501 114583 +82 + Misses 11939 11911 -28 + Partials 2602 2600 -2
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:
|
ldk-reviews-bot
commented
Aug 11, 2025
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
There was a problem hiding this comment.
Thanks for looking into this!
| use lightning::sign::NodeSigner; | ||
| /// A trait for implementing Denial-of-Service (DoS) protection mechanisms for LSP services. | ||
| pub trait DosProtectionEnforcer { |
There was a problem hiding this comment.
IMO this is over-engineering. We can just have a single method on LSPS5ServiceHandler that checks the necessary bools. I also don't think this warrants to be in a separate module right now.
| peer_state | ||
| .outbound_channels_by_intercept_scid | ||
| .values() | ||
| .any(|c| c.is_pending_channel_open()) |
There was a problem hiding this comment.
Hmm, we'll also want to allow LSPS5 for any activity beyond this point, no? I think rather than adding helpers for each individual state, maybe we finally should expose the list of OutboundJITChannelStates at least pub(crate) and also implement Ord for OutboundJITChannelState? This would allow us to use comparison operators on the channel states.
Also, can we somehow enforce that for a pending payment, we'd always first process it and advance the LSPS2 state before evaluating whether we can notifiy the LSPS5 client?
There was a problem hiding this comment.
what I thought here (and I didn't clarify, sorry) is that it should only check for the pending_channel_open. after that state, the channel will be opened and the LSPS5 service has_active_channels check will kick in, so there is no point on checking the other LSPS2 states. does that make sense?
| if !self.peer_is_engaged(sender_node_id) { | ||
| return Err(LightningError { | ||
| err: format!( | ||
| "Rejecting LSPS5 request from {:?} without existing engagement", |
There was a problem hiding this comment.
"existing engagement" is pretty unclear terminology, would be good to find more intuitive wording.
| "Rejecting LSPS5 request from {:?} without existing engagement", | ||
| sender_node_id | ||
| ), | ||
| action: ErrorAction::IgnoreAndLog(Level::Info), |
There was a problem hiding this comment.
Please don't log on on Info here.
| /// Maximum number of webhooks allowed per client. | ||
| pub max_webhooks_per_client: u32, | ||
| /// Require an existing channel or active LSPS1/LSPS2 flow before accepting requests. | ||
| pub enforce_dos_protections: bool, |
There was a problem hiding this comment.
Let's at least start out with enabling this by default, i.e., don't make it configurable.
ldk-reviews-bot
commented
Aug 11, 2025
👋 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. |
38c8436 to
ffe25e6Comparemartinsaposnic
commented
Aug 11, 2025
@tnull thanks for the review! I think this is a much better I pushed a fixup commit addressing all the comments ffe25e6 I still have my doubts regarding this:
I think that maybe that's not necessary and I posted a question related to that: #3993 (comment) thanks! |
tnull
left a comment
There was a problem hiding this comment.
Thanks, much cleaner already! Some comments.
Uh oh!
There was an error while loading. Please reload this page.
| /// The different states a requested JIT channel can be in. | ||
| #[derive(Debug)] | ||
| enum OutboundJITChannelState { | ||
| #[derive(Clone, Debug, PartialEq, Eq)] |
There was a problem hiding this comment.
Why does this need to be Clone suddenly?
| PaymentForwarded { channel_id: ChannelId }, | ||
| } | ||
| impl OutboundJITChannelState { |
There was a problem hiding this comment.
Do we need this explicit implementation? Wouldn't derive(PartialOrd) do the ~the same thing?
There was a problem hiding this comment.
this would need putting also the derive(Ord) and derive(PartialOrd) on PaymentQueue and InterceptId
happy to do it but wanted to minimize changes
There was a problem hiding this comment.
seems like I can't define Ord but derive PartialOrd
from the linter:
error: you are implementing `Ord` explicitly but have derived `PartialOrd`
--> lightning-liquidity/src/lsps2/service.rs:164:1
|
164 | / impl Ord for OutboundJITChannelState {
165 | | fn cmp(&self, other: &Self) -> core::cmp::Ordering {
166 | | self.stage().cmp(&other.stage())
167 | | }
168 | | }
| |_^
There was a problem hiding this comment.
Ah, sorry. Then I have to eat my words. I'd actually prefer the way you had vs. introducing the redundant ..Stage object. Sorry, mind reverting to explicitly implementing it?
| let outer_state_lock = self.per_peer_state.read().unwrap(); | ||
| if let Some(inner_state_lock) = outer_state_lock.get(counterparty_node_id) { | ||
| let peer_state = inner_state_lock.lock().unwrap(); | ||
| peer_state.outbound_channels_by_intercept_scid.values().map(|c| c.state.clone()).max() |
There was a problem hiding this comment.
I don't buy we need to clone here.
There was a problem hiding this comment.
error[E0507]: cannot move out of `c.state` which is behind a shared reference
--> lightning-liquidity/src/lsps2/service.rs:599:68
|
599 | peer_state.outbound_channels_by_intercept_scid.values().map(|c| c.state).max()
| ^^^^^^^ move occurs because `c.state` has type `OutboundJITChannelState`, which does not implement the `Copy` trait
|
help: consider cloning the value if the performance cost is acceptable
|
599 | peer_state.outbound_channels_by_intercept_scid.values().map(|c| c.state.clone()).max()
| ++++++++
There was a problem hiding this comment.
Let me rephrase: we definitely can't clone the entire state, with all the HTLC data, etc each time we want to check whether a client reached a certain state. I still think handling all the state by reference should be doable, but it's probably easiest to mirror the fn has_active_requests(&self, counterparty_node_id: &PublicKey) -> bool above.
(While eventually we want to give users insight into the held state, it doesn't need to happen in this PR, so for now it's probably easiest to revert to having OutboundJITChannelState private, and just check the bool returned by the helper mentioned above)
| ) -> bool { | ||
| self.client_has_open_channel(client_id) | ||
| || lsps1_has_activity | ||
| || lsps2_max_state.map_or(false, |s| { |
There was a problem hiding this comment.
Hmm, why not just use >= PendingChannelOpen now that we can?
There was a problem hiding this comment.
this could not be done directly because I would need to create some dummy properties for the PendingChannelOpen struct for it to be able to compare:
|s| s >= OutboundJITChannelState::PendingChannelOpen { payment_queue: PaymentQueue::new(), opening_fee_msat: 0 })
I prefer not to do this so went with something different:
- dropped the
OutboundJITChannelState::ord_index() - instead, I created a new enum
OutboundJITStagethat matches one to one withOutboundJITChannelStatebut has no properties. this newOutboundJITStagehasDerive(ord, partialOrd)so it just grabs the declaration order for its ordering
with this, now we can do |s| s.stage() >= OutboundJITStage::PendingChannelOpen
There was a problem hiding this comment.
Ah, see #3993 (comment)
I'd prefer not to introduce a redundant object here. Sorry for the noise.
8eaea29 to
9a59755Comparemartinsaposnic
commented
Aug 12, 2025
I squashed the last fixup commit and created a new fixup commit addressing the latest comments also responded to some of the comments that may need a follow up thanks @tnull ! |
martinsaposnic
commented
Aug 13, 2025
just pushed a fixup commit addressing the latest comments a74c585
let me know what you think @tnull , thanks!! |
142842b to
109de16Compare| peer_state.outbound_channels_by_intercept_scid.values().any(|chan| { | ||
| matches!( | ||
| chan.state, | ||
| OutboundJITChannelState::PendingChannelOpen { .. } |
There was a problem hiding this comment.
Wait, how do we imagine this to work for the initial LSPS2 receive? If we only allow clients to even register for notifications once we already have a (pending) channel open, how would a first-time user's phone get notified to wake up and accept the channel when they receive the very first payment?
I think we'd need to extend this to also accept LSPS5 requests for PendingInitialPayment?
There was a problem hiding this comment.
yeah, that makes sense. I will update this
Uh oh!
There was an error while loading. Please reload this page.
5148642 to
e9e2addCompare
TheBlueMatt
left a comment
There was a problem hiding this comment.
actually I'm thinking that probably we need a service event for communicating that the request is being rejected? right now it silently fails. the client would not have a clue that it failed because of DoS protection reasons. thoughts?
Yea, ISTM we should respond with SetWebhookError.
ldk-reviews-bot
commented
Aug 25, 2025
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
martinsaposnic
commented
Aug 25, 2025
last fixup commit adds logic to respond with an error instead of silently failing 6041e71 this should be ready for review again @TheBlueMatt , thanks!! |
ldk-reviews-bot
commented
Aug 27, 2025
🔔 2nd Reminder Hey @tnull@TheBlueMatt! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Aug 27, 2025
🔔 1st Reminder Hey @tnull@TheBlueMatt! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Aug 30, 2025
🔔 3rd Reminder Hey @tnull@TheBlueMatt! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Aug 30, 2025
🔔 2nd Reminder Hey @tnull@TheBlueMatt! This PR has been waiting for your review. |
TheBlueMatt
left a comment
There was a problem hiding this comment.
Didn't take a super close look at the tests but this LGTM, feel free to squash.
ldk-reviews-bot
commented
Sep 1, 2025
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
6041e71 to
be0f7e8Comparemartinsaposnic
commented
Sep 1, 2025
just squashed all fixup commits there were no conflicts with main, but I rebased because it's been a while since the last one. the force-push compare is messed up because of that, but I haven't introduced any additional changes. sorry 🙃 |
| } | ||
| } | ||
| macro_rules! assert_lsps5_reject { |
There was a problem hiding this comment.
nit: Using methods instead of macros would be preferable, as it improves debugging if something inside of the assert fails. But feel free to leave as is for now.
| ); | ||
| } | ||
| fn establish_lsps2_prior_interaction(lsps_nodes: &LSPSNodes) { |
There was a problem hiding this comment.
Rather than just adding this at the end, can we move this up to the other helper methods at the start of the file?
martinsaposnic
commented
Sep 1, 2025
just pushed a fixup commit addressing the last 2 comments. thanks @tnull ! |
When handling an incoming LSPS5 request, the manager will check if the counterparty is 'engaged' in some way before responding. `Engaged` meaning = active channel | LSPS2 active operation | LSPS1 active operation. Logic: `If not engaged then reject request;` A single test is added only checking for the active channel condition, because it's not super easy to get LSPS1-2 on the correct state to check this (yet). Other tangential work is happening that will make this easier and more tests will come in the near future
7d1934d to
4370cffComparemartinsaposnic
commented
Sep 2, 2025
all fixups are squashed now! |
Uh oh!
There was an error while loading. Please reload this page.
When handling an incoming LSPS5 request, the manager will check
if the counterparty is 'engaged' in some way before responding.
Engagedmeaning = active channel | LSPS2 active operation | LSPS1 active operation.Logic:
If not engaged then reject request;A single test is added only checking for the active channel condition,
because it's not super easy to get LSPS1-2 on the correct state to check this (yet).
Other tangential work is happening that will make this easier and more tests will come in the near future.
A few decisions that could be changed:- the DOS protections are optional (default true). maybe this should not be configurable?- I made thedos_protection_enforcergeneric enough so it would be possible to add more behavior in the future. also some logic could be moved here like theignored_peerslogic from the manager, which could make sense to move to the dos enforcerthoughts @tnull ?