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

Allow responding asynchronously to OnionMessage #2996

Merged
merged 5 commits into from
May 31, 2024

Conversation

shaavan
Copy link
Member

@shaavan shaavan commented Apr 15, 2024

This PR addresses second part of #2882 and builds on #2907

This PR converts handle_onion_message_response into the public function which can be used for asynchronous response to a separately created ResponseInstruction.
Also, this introduces a new variant of ResponseInstruction. This allows creating a reply_path for the receiver while responding to their message.

@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2024

Codecov Report

Attention: Patch coverage is 96.74419% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 89.87%. Comparing base (df01208) to head (d8adbb0).

Files Patch % Lines
lightning/src/onion_message/messenger.rs 92.72% 3 Missing and 1 partial ⚠️
lightning/src/onion_message/functional_tests.rs 98.12% 1 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2996      +/-   ##
==========================================
- Coverage   89.90%   89.87%   -0.03%     
==========================================
  Files         117      117              
  Lines       97105    97257     +152     
  Branches    97105    97257     +152     
==========================================
+ Hits        87300    87412     +112     
- Misses       7245     7275      +30     
- Partials     2560     2570      +10     

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

@shaavan
Copy link
Member Author

shaavan commented Apr 23, 2024

Updated from pr2996.01 to pr2996.02 (diff):

Update:

  1. Synced with base PR (Introduce ResponseInstructions for OnionMessage Handling #2907) to solve merge conflicts.

@shaavan
Copy link
Member Author

shaavan commented Apr 24, 2024

cc @jkczyz, @TheBlueMatt

@shaavan
Copy link
Member Author

shaavan commented Apr 26, 2024

Updated from pr2996.02 to pr2996.03 (diff):
Addressed @jkczyz comments:

Updates:

  1. Address nit comments.
  2. Expanded return type of handle_onion_message_response. This change allows access to the success/failure status of sending the response to the user which they can handle appropriately.

@shaavan
Copy link
Member Author

shaavan commented May 9, 2024

Updated from pr2996.03 to pr2996.04 (diff):

Updates:

  1. Rebase on main, to resolve conflicts post Introduce ResponseInstructions for OnionMessage Handling #2907 merge.

@shaavan
Copy link
Member Author

shaavan commented May 10, 2024

Updated from pr2996.04 to pr2996.05 (diff):
Addressed @TheBlueMatt, @jkczyz comments

Updates:

  1. Expanded respond(), and respond_with_reply_path() documentation.
  2. Updated test to check if we got the expected variant of SendSuccess, when calling handle_onion_message_response.
  3. Refactor the code to improve brevity.
  4. Update handle_onion_message response to fail in case we fail to create a blinded_path.
  5. Fix the Fuzz test.

@shaavan
Copy link
Member Author

shaavan commented May 11, 2024

Updated from pr2996.05 to pr2996.06 (diff):
Addressed @jkczyz comments

Updates:

  1. Simplify the respond, and respond_with_reply_path docs to make them concise.
  2. Simplify the result check in the added test.
  3. Introduced logging in handle_onion_message_response if create_blinded_path fails to create reply_path.

@shaavan
Copy link
Member Author

shaavan commented May 13, 2024

Updated from pr2996.06 to pr2996.07 (diff):

Updates:

  1. Rebase on main, to fix ci failures.

@shaavan
Copy link
Member Author

shaavan commented May 14, 2024

Updated from pr2996.07 to pr2996.08 (diff):
Addressed @TheBlueMatt and @jkczyz comments

Changes:

  1. Expanded docs for respond and respond_with_reply_path so that there purpose is more clearly explained.
  2. Expand log message to also contain the error itself.
  3. nit blinded_path -> reply_path.
  4. Simplified logger -> self.logger, since there are no context to add with the error message within the function.
  5. Improve the code style to be more idiomatic.
  6. Simplified format_args! -> format! for log_trace!

@shaavan
Copy link
Member Author

shaavan commented May 15, 2024

Updated from pr2996.08 to pr2996.09 (diff):
Addressed @jkczyz comment

Update:

  1. Fix grammatical error.
  2. Update logging so that a string will not be created in case it is not being logged.

@shaavan
Copy link
Member Author

shaavan commented May 19, 2024

Updated from pr2996.09 to pr2996.10 (diff):
Addressed @jkczyz comments

Changes:

  1. Corrected a nit, reply_path -> reply path
  2. Expanded SendError::PathNotFound documentation as it can also be triggered when we fail to create a reply_path.
  3. Introduce two new variants in TestCustomMessage, to allow an arbitrary number of back-and-forth communication, allowing test response functionality properly.
  4. Introduce a new test to test the reply_path creation success, creation failure, and usage functionality.

@shaavan
Copy link
Member Author

shaavan commented May 22, 2024

Updated from pr2996.12 to pr2996.13 (diff):
Addressed @jkczyz comment

Changes:

  1. Removed redundant diff to keep commits atomic.
  2. Improved the SendError::PathNotFound docs.
  3. Removed the redundant granular comments from the added test.

@shaavan
Copy link
Member Author

shaavan commented May 23, 2024

Updated from pr2996.13 to pr2996.14 (diff):
Addressed @jkczyz comment

Changes:

  1. Renamed Request and Response to Ping and Pong to better suit their new roles of being each other's response.
  2. Removed the now redundant variants ResponseA and ResponseB.
  3. Introduced a new variable in TestCustomMessageHandler that signals to handle_custom_message which ResponseInstruction it should create.
  4. Updated the introduced test to utilize this new setup.

@shaavan
Copy link
Member Author

shaavan commented May 23, 2024

Updated from pr2996.14 to pr2996.15 (diff):
Addressed @jkczyz comment

Changes:

  1. Introduce the add_reply_path boolean as part of expectations. This allows saving up on a mutex variable and keeps things consistent with the current test suite.
  2. Introduce a new struct Expectation to keep the current (and future) expectations neatly ordered.

@shaavan
Copy link
Member Author

shaavan commented May 24, 2024

Updated from pr2996.15 to pr2996.16 (diff):
Addressed @jkczyz comments

  1. Refactor the function and improve naming structs for TestCustomMessageHandler.
  2. Improve the test readability by properly structuring and separating success and failure cases in separate tests.

@shaavan
Copy link
Member Author

shaavan commented May 28, 2024

Updated from pr2996.16 to pr2996.17 (diff):

Changes:

  1. Rebase on main

@shaavan
Copy link
Member Author

shaavan commented May 28, 2024

@jkczyz, @TheBlueMatt
A gentle ping! :)

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

LGTM other than expanding the docs a bit.

@shaavan
Copy link
Member Author

shaavan commented May 29, 2024

Updated from pr2996.17 to pr2996.18 (diff):
Addressed @jkczyz comment

Update:

  1. Rebase on main.
  2. Update the create_blinded_path function and BlindedPath::new_for_message to match the API change.
  3. Expanded handle_onion_message_response docs.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

BTW, please avoid rebasing mid-review as it makes it difficult for reviewers to tell what has changed.

shaavan added 5 commits May 30, 2024 12:36
…st coverage

This commit modifies handle_onion_message_response to be accessible
publicly as handle_onion_message, enabling users to respond
asynchronously. Additionally, a new test is introduced to validate this
functionality.
And expand handle_onion_message_response return Type

1. Introduce a new function in OnionMessenger to create blinded paths.
2. Use it in handle_onion_message_response to create a reply_path for
   the right variant and use it in onion_message.
3. Expand the return type of handle_onion_message_response to handle three cases:
	1. Ok(None) in case of no response to be sent.
	2. Ok(Some(SendSuccess) and Err(SendError) in case of successful and
   	unsuccessful queueing up of response messages respectively.

This allows the user to get access to the Success/Failure status of the sending
of response and handle it accordingly.
1. These two variants will be modified in an upcoming commit
   to be each other's response.
2. The names are updated to better fit their new roles.
- Introduce a new struct for keeping expectations organized.
- Add a boolean field to track whether a response is expected,
  and hence whether a `reply_path` should be included with the response.
- Update Ping and Pong roles for bidirectional communication.
- Introduce panic for when there is no responder and we were expecting
  to include a `reply_path`.
- Refactor `handle_custom_message` code.
- The new tests check both successful and unsuccessful `reply_path`
  creation conditions.
@shaavan
Copy link
Member Author

shaavan commented May 30, 2024

Updated from pr2996.18 to pr2996.19 (diff):
Addressed @jkczyz's comment:

Changes:

  1. Removed the redundant commented code.
  2. Applied nit fixes to docs.

@jkczyz:

BTW, please avoid rebasing mid-review as it makes it difficult for reviewers to tell what has changed.

Understood! I tried to keep the changes with rebase separate, but for this one, I took a shortcut since the suggestion was a small one. I can see how that would have made reviewing difficult. I’ll make sure to keep the rebase and changes separate in the future. Thanks!


let response = match msg {
TestCustomMessage::Ping => TestCustomMessage::Pong,
TestCustomMessage::Pong => TestCustomMessage::Ping,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we responding to a pong with a ping? Shouldn't we fail if we are expected to reply to a Pong instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

The new test for this PR required multiple back-and-forth communication between the nodes. To allow doing so we settled on making Ping and Pong each other's response.

Here's a discussion thread related to it!

Thanks!

@TheBlueMatt TheBlueMatt merged commit bd16a1e into lightningdevkit:main May 31, 2024
15 of 16 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