Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start publishing PaymentForwarded events. #404

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

G8XSU
Copy link
Contributor

@G8XSU G8XSU commented Nov 12, 2024

  • Start publishing PaymentForwarded events.
    Required for ldk-server implementation or rather any forwarding node. Since a mobile node shouldn't be forwarding payments, these shouldn't get generated for mobile nodes.

@G8XSU G8XSU requested a review from tnull November 12, 2024 22:29
@G8XSU
Copy link
Contributor Author

G8XSU commented Nov 12, 2024

404 Secured!

@G8XSU
Copy link
Contributor Author

G8XSU commented Nov 12, 2024

Pushed minor fix to fix CI.

Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

This is somewhat preexisting, but will be more important if we introduce this event: going forward we need to consider steps to mitigate the potential DoS issue that might arise if users don't handle events fast enough and our event queue and forwarding/payment stores grow unboundedly.

Maybe we should extend the docs on Node in general and Node::next_event to stress more explicitly that users really need to handle events as fast as possible, especially if they would ever open announced channels? Also, we might need to consider starting to bound the event queue as dropping events is preferable to risk running OOM?

So I think we should take some (initial) mitigation steps as part of this PR or as a follow-up.

src/event.rs Outdated Show resolved Hide resolved
@G8XSU G8XSU requested a review from tnull November 14, 2024 19:29
@G8XSU
Copy link
Contributor Author

G8XSU commented Nov 14, 2024

I don't think dropping events is a good idea, as users might miss that information. (discussed it offline in dev-sync, we need a way for ldk to somehow stop publishing events if event queue is beyond some large threshold.)

In Node::next_event we need to stress more explicitly that users really need to handle events as fast as possible.

I understand, will do it as separate PR.

@G8XSU
Copy link
Contributor Author

G8XSU commented Nov 14, 2024

Created #405 as separate PR.

Copy link
Contributor

@enigbe enigbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow-up to @tnull's comment, I have just a small question about event handling for older-versioned events in the event queue of upgraded nodes.

src/event.rs Outdated Show resolved Hide resolved
@G8XSU
Copy link
Contributor Author

G8XSU commented Nov 20, 2024

Addressed all comments, kept prev_channel_id & next_channel_id as non-optional since there was never an ldk-node release with LDK < 0.0.0115. (and those were introduced in 0.0.107)

src/event.rs Outdated Show resolved Hide resolved
src/event.rs Outdated Show resolved Hide resolved
src/event.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically LGTM (mod addressing/clarifying open comments).

Could you add some basic test coverage for this, e.g., by adding some

   expect_event!(nodes[X], PaymentForwarded);

checks for the intermediate node in the multi_hop_sending test?

@tnull
Copy link
Collaborator

tnull commented Nov 26, 2024

@G8XSU Let me know when you addressed the final round of comments/added some test coverage.

@G8XSU
Copy link
Contributor Author

G8XSU commented Nov 26, 2024

Addressed comments, Added tests and asserting over single payment path, waiting for CI and msrv check.

Do i need to send it over specific path and then assert?

@G8XSU G8XSU requested a review from tnull November 26, 2024 17:05
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, mod one comment and one nit.

Feel free to address them while squashing in the fixups to get this in a mergeable state.

src/event.rs Outdated Show resolved Hide resolved
src/event.rs Show resolved Hide resolved
Required for ldk-server implementation or rather any forwarding node.
Since a mobile node shouldn't be forwarding payments, these shouldn't
get generated for mobile nodes.
@G8XSU G8XSU force-pushed the payment-forwarded-event branch from 1d0aec2 to d8d9002 Compare November 27, 2024 19:13
@G8XSU
Copy link
Contributor Author

G8XSU commented Nov 27, 2024

Squashed with current fixups,

@tnull tnull merged commit c605781 into lightningdevkit:main Nov 28, 2024
4 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants