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

Updated docs on PeerManager::process_events. #3395

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mhrheaume
Copy link
Contributor

Try to make it a bit more clear that there are downsides to solely relying on lightning-net-tokio, and it's still recommended to occasionally call this function in a separate loop.

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

LGTM

/// or one of the other clients provided in our language bindings, as it will be called
/// automatically when a message is received from a peer. However, it can in some cases delay
/// messages from going out if peers are not sending messages, so it is still recommended to
/// call it routinely from [`lightning-background-processor`] or a similar background task.
Copy link
Collaborator

@TheBlueMatt TheBlueMatt Nov 5, 2024

Choose a reason for hiding this comment

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

The logic of when its called in lightning-background-processor is that that BP gets woken up ~every time there's a state change in the channel logic (implying we likely have a message to send). So its not really just "calling it routinely" but rather calling it when we have a message to send because we changed the channel state by sending/claiming/forwarding a payment. Would be nice to capture that here somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to capture that, but LMK if you have any better wording in mind!

/// automatically when a message is received from a peer. However, it can in some cases delay
/// messages from going out if peers are not sending messages, so it is still recommended to
/// call it when messages may need to be sent, e.g. after processing channel events that may
/// have resulted in an update to the channel state.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now it seems to imply that even if you're using the BP you have to call it manually? Maybe we should just flip this on its head and say that this needs to be called any time we may have messages to send, and that its automatically called after processing messages by net-tokio and by the BP when the channel state has changed, but for users not using both of those they may need to call it manually?

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.64%. Comparing base (948f179) to head (9d07106).
Report is 118 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3395      +/-   ##
==========================================
+ Coverage   89.56%   89.64%   +0.08%     
==========================================
  Files         127      128       +1     
  Lines      103547   104881    +1334     
  Branches   103547   104881    +1334     
==========================================
+ Hits        92744    94025    +1281     
- Misses       8094     8139      +45     
- Partials     2709     2717       +8     

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

Try to make it a bit more clear that there are downsides to solely
relying on `lightning-net-tokio`, and it's still recommended to
occasionally call this function in a separate loop.
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.

3 participants