Add `lightning-liquidity` crate to the workspace #3436

tnull · 2024-12-03T10:21:32Z

We upstream the lightning-liquidity crate into the rust-lightning
workspace. First, all source files are copied over as per current main of the lightning-liquidity repository (commit c80eb75f5a31bea5c2b73e41c50ca382ec0020f8).

Then, fixup commits adjust the crate to the current state of rust-lightning, and add it to our local CI script.

Once this is merged I'll transfer any relevant open issues from https://github.com/lightningdevkit/lightning-liquidity and archive the repo.

tnull · 2024-12-03T12:40:23Z

AFAICT, remaining CI failures should be unrelated.

tnull · 2024-12-03T18:05:31Z

Rebased on main to re-run CI with fixes.

arik-so · 2024-12-04T22:39:16Z

check_commits and linting are still failing

TheBlueMatt

Ran out of time to finish really digging into this, but at least have some doc requests and such.

lightning-liquidity/Cargo.toml

lightning-liquidity/src/events.rs

lightning-liquidity/src/manager.rs

lightning-liquidity/src/sync/nostd_sync.rs

lightning-liquidity/src/utils.rs

lightning-liquidity/src/lsps2/service.rs

tnull · 2024-12-05T08:29:30Z

check_commits and linting are still failing

Yup, both are expected. check_commits should pass once we squash, and the linting failures are pre-existing, I think.

tnull · 2024-12-05T09:12:52Z

Ran out of time to finish really digging into this, but at least have some doc requests and such.

Sure, no worries! I'm generally happy to address any comments that come up during review, but would lean towards more or less only addressing anything that's needed to move the crate in this PR, and address anything that might be more substantial in follow-ups.

codecov · 2024-12-05T13:15:24Z

Codecov Report

Attention: Patch coverage is 59.92574% with 1403 lines in your changes missing coverage. Please review.

Project coverage is 89.44%. Comparing base (797993c) to head (71212ad).
Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning-liquidity/src/lsps2/service.rs	57.76%	494 Missing and 23 partials ⚠️
lightning-liquidity/src/lsps1/client.rs	0.00%	333 Missing ⚠️
lightning-liquidity/src/lsps0/ser.rs	44.75%	171 Missing and 45 partials ⚠️
lightning-liquidity/src/manager.rs	47.03%	154 Missing and 7 partials ⚠️
lightning-liquidity/src/lsps2/client.rs	61.69%	76 Missing and 1 partial ⚠️
lightning-liquidity/src/lsps0/client.rs	57.89%	32 Missing ⚠️
lightning-liquidity/src/lsps1/msgs.rs	90.50%	11 Missing and 6 partials ⚠️
lightning/src/sync/debug_sync.rs	0.00%	14 Missing ⚠️
lightning-liquidity/src/lsps0/msgs.rs	89.60%	13 Missing ⚠️
lightning-liquidity/src/events.rs	95.03%	5 Missing and 3 partials ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3436      +/-   ##
==========================================
- Coverage   89.69%   89.44%   -0.26%     
==========================================
  Files         130      149      +19     
  Lines      107335   116152    +8817     
  Branches   107335   116152    +8817     
==========================================
+ Hits        96277   103894    +7617     
- Misses       8658     9793    +1135     
- Partials     2400     2465      +65

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

tnull · 2024-12-09T15:29:53Z

Addressed all the pending feedback.

lightning-liquidity/src/events.rs

lightning-liquidity/src/lsps2/service.rs

TheBlueMatt

Most of the new comments we can freely ignore and are more things to address later, but some of them probably are quite important.

lightning-liquidity/README.md

lightning-liquidity/src/lsps2/client.rs

lightning-liquidity/src/lsps2/service.rs

tnull · 2024-12-11T13:01:03Z

Responded to all pending comments and addressed most of them.

tnull · 2024-12-11T13:11:20Z

@TheBlueMatt Let me know if I can squash fixups.

tnull · 2024-12-11T13:51:14Z

Btw now moved all open issues from the lightning-liqudity repo to here and added a label of the same name.

TheBlueMatt

Feel free to squash. I think I'm okay with landing this, AFAIU there's no remaining trivial "anyone can run us out of memory" vulnerabilities, but would like @johncantrell97 to take a glance at the set of changes and confirm that matches his understanding as well.

lightning-liquidity/src/lsps2/service.rs

We upstream the `lightning-liquidity` into the `rust-lightning` workspace. Files are copied over as per commit c80eb75f5a31bea5c2b73e41c50ca382ec0020f8.

.. to which end we also need to make some additions to `debug_sync` and `nostd_sync`.

We add a size limit on the event queue, after which we'll just start dropping events to ensure we could never OOM. Additionally, we document the requirement that users need to handle generated events ASAP.

We introduce a new `MAX_PENDING_REQUESTS_PER_PEER` limit for the number of pending (get_info and buy) requests per peer.

.. which is a prefactor to also start checking the total number of pending requests in the next commit.

To this end we introduce a new counter keeping track of overall requests pending and reject inbound requests if they would put us over the limit.

We clean up any `get_info` request state when peers disconnect.

.. we clean up any pending buy requests that hit their `valid_until` time when the counterparty disconnects.

We're now also pruning any expired `OutboundJITChannels` if we haven't seen any related HTLCs.

In addition to pruning expired requests on peer disconnection we also regularly prune for all peers on block connection, and also remove the entire `PeerState` if it's empty after pruning (i.e., has no pending requsts or in-flight channels left).

tnull · 2024-12-11T16:02:43Z

Feel free to squash.

Squashed fixups without further changes.

wvanlint · 2024-12-13T00:21:50Z

Exciting for this to be upstreamed! @johncantrell97 is out currently, I went over the changes briefly.

With regard to OOM vulnerabilities, it seems only limits on pending requests have been introduced i.e. requests that need to be handled by the server. Could memory not still increase with a constant throughput of handling requests?

Could a server not repeatedly fulfill BuyRequests with invoice_parameters_generated, which removes the pending request but allocates an OutboundJITChannel?
Similarly, handling GetInfoRequest inserts a PeerState for any new peer, and the pending request will be removed in opening_fee_params_generated. Empty PeerStates get cleared after disconnection though in the changes.

Basically, we might need to limit the number of PeerStates and OutboundJITChannels per peer, in addition to a total pending requests limit.

I also wonder if silently dropping events from EventQueue::enqueue could result in the flow getting stuck, what would happen if LSPS2ServiceEvent::OpenChannel gets dropped for example? Wouldn't the held HTLCs get stuck as they're waiting for the channel to be opened? Rate-limiting at the message handling level should limit the event queue growth as well.

tnull requested a review from TheBlueMatt December 3, 2024 10:21

tnull added this to the 0.1 milestone Dec 3, 2024

tnull force-pushed the 2024-12-add-lightning-liquidity-crate branch from 1cd0e69 to 510ae5d Compare December 3, 2024 12:39

tnull force-pushed the 2024-12-add-lightning-liquidity-crate branch from 510ae5d to d408ac6 Compare December 3, 2024 18:05

TheBlueMatt reviewed Dec 5, 2024

View reviewed changes

tnull force-pushed the 2024-12-add-lightning-liquidity-crate branch 4 times, most recently from 1608102 to dca99f4 Compare December 5, 2024 13:06

tnull force-pushed the 2024-12-add-lightning-liquidity-crate branch 4 times, most recently from fe85637 to 8b9c6e0 Compare December 9, 2024 15:29

tnull force-pushed the 2024-12-add-lightning-liquidity-crate branch from 8b9c6e0 to 4d20463 Compare December 9, 2024 16:02

TheBlueMatt reviewed Dec 9, 2024

View reviewed changes

lightning-liquidity/src/events.rs Show resolved Hide resolved

lightning-liquidity/src/lsps2/service.rs Outdated Show resolved Hide resolved

lightning-liquidity/src/lsps2/service.rs Show resolved Hide resolved

tnull force-pushed the 2024-12-add-lightning-liquidity-crate branch from 3a318f8 to 71212ad Compare December 10, 2024 09:42

TheBlueMatt reviewed Dec 10, 2024

View reviewed changes

tnull force-pushed the 2024-12-add-lightning-liquidity-crate branch from 71212ad to 6f5875c Compare December 11, 2024 12:51

tnull mentioned this pull request Dec 11, 2024

LSPS2: Expiring of requests lightningdevkit/lightning-liquidity#27

Closed

TheBlueMatt reviewed Dec 11, 2024

View reviewed changes

lightning-liquidity/src/lsps2/service.rs Show resolved Hide resolved

tnull added 13 commits December 11, 2024 17:01

Add lightning-liquidity crate to the workspace

b8b8071

We upstream the `lightning-liquidity` into the `rust-lightning` workspace. Files are copied over as per commit c80eb75f5a31bea5c2b73e41c50ca382ec0020f8.

Avoid allocating during LSPSMethod serialization

94c1e37

Use split_off instead of collecting in get_pending_events()

d8ba98b

Use LDK-internal Hash{Map,Set} types in lightning-liquidity

3a8e1bf

Use lightning::sync via symlink in lightning-liquidity

9c9b4a8

.. to which end we also need to make some additions to `debug_sync` and `nostd_sync`.

Upper-bound the event queue size

7e42b36

We add a size limit on the event queue, after which we'll just start dropping events to ensure we could never OOM. Additionally, we document the requirement that users need to handle generated events ASAP.

LSPS2: Introduce MAX_PENDING_REQUESTS_PER_PEER service limit

3f2e232

We introduce a new `MAX_PENDING_REQUESTS_PER_PEER` limit for the number of pending (get_info and buy) requests per peer.

LSPS2: DRY up pending request insertion/removal

1f13c80

.. which is a prefactor to also start checking the total number of pending requests in the next commit.

LSPS2: Enforce a limit on total pending requests

f75f124

To this end we introduce a new counter keeping track of overall requests pending and reject inbound requests if they would put us over the limit.

LSPS2: Clean pending get_info request state on disconnection

6451a43

We clean up any `get_info` request state when peers disconnect.

LSPS2: Prune expired buy requests on disconnection

806fffb

.. we clean up any pending buy requests that hit their `valid_until` time when the counterparty disconnects.

LSPS2: Also prune expired OutboundJITChannels pending initial payments

bdea03e

We're now also pruning any expired `OutboundJITChannels` if we haven't seen any related HTLCs.

LSPS2: Prune empty PeerStates

08fd499

In addition to pruning expired requests on peer disconnection we also regularly prune for all peers on block connection, and also remove the entire `PeerState` if it's empty after pruning (i.e., has no pending requsts or in-flight channels left).

tnull force-pushed the 2024-12-add-lightning-liquidity-crate branch from 91c6f62 to 08fd499 Compare December 11, 2024 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `lightning-liquidity` crate to the workspace #3436

Add `lightning-liquidity` crate to the workspace #3436

tnull commented Dec 3, 2024 •

edited

Loading

tnull commented Dec 3, 2024

tnull commented Dec 3, 2024

arik-so commented Dec 4, 2024

TheBlueMatt left a comment

tnull commented Dec 5, 2024

tnull commented Dec 5, 2024

codecov bot commented Dec 5, 2024 •

edited

Loading

tnull commented Dec 9, 2024

TheBlueMatt left a comment

tnull commented Dec 11, 2024

tnull commented Dec 11, 2024

tnull commented Dec 11, 2024

TheBlueMatt left a comment

tnull commented Dec 11, 2024

wvanlint commented Dec 13, 2024

Add lightning-liquidity crate to the workspace #3436

Are you sure you want to change the base?

Add lightning-liquidity crate to the workspace #3436

Conversation

tnull commented Dec 3, 2024 • edited Loading

tnull commented Dec 3, 2024

tnull commented Dec 3, 2024

arik-so commented Dec 4, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

tnull commented Dec 5, 2024

tnull commented Dec 5, 2024

codecov bot commented Dec 5, 2024 • edited Loading

Codecov Report

tnull commented Dec 9, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

tnull commented Dec 11, 2024

tnull commented Dec 11, 2024

tnull commented Dec 11, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

tnull commented Dec 11, 2024

wvanlint commented Dec 13, 2024

Add `lightning-liquidity` crate to the workspace #3436

Add `lightning-liquidity` crate to the workspace #3436

tnull commented Dec 3, 2024 •

edited

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading