Allow responding asynchronously to OnionMessage

[shaavan]

trafficstars

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]

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 :warning:
lightning/src/onion_message/functional_tests.rs	98.12%	1 Missing and 2 partials :warning:

:exclamation: 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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[shaavan]

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

Update:

1. Synced with base PR (#2907) to solve merge conflicts.

[shaavan]

cc @jkczyz, @TheBlueMatt

[shaavan]

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]

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

Updates:

1. Rebase on main, to resolve conflicts post #2907 merge.

[shaavan]

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]

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]

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

Updates:

1. Rebase on main, to fix ci failures.

[shaavan]

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]

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]

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]

Updated from pr2996.10 to pr2996.11 (diff): Addressed @TheBlueMatt comment

Changes:

1. Reverted handle_custom_message to default to avoid API changes.
2. Introduced a new test_handle_custom_message handler for the ResponseInstruction tests.

[shaavan]

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

1. Removed the test-specific handle_custom_message, and instead utilized the newly introduced TestCustomMessage variants to facilitate the respond_with_reply_path test.
2. Expanded the TestCustomMessage docs, to properly explain each variant's role.

[shaavan]

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]

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]

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]

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]

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

Changes:

1. Rebase on main

[shaavan]

@jkczyz, @TheBlueMatt A gentle ping! :)

[shaavan]

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.

[shaavan]

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!