Introduce ResponseInstructions for OnionMessage Handling #2907
Conversation
WalkthroughThe changes focus on enhancing the handling of onion messages by introducing the ability to manage reply paths for asynchronous operations. Updates include modifications to function signatures to support Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (8)
- fuzz/src/onion_message.rs (1 hunks)
- lightning/src/ln/channelmanager.rs (3 hunks)
- lightning/src/ln/msgs.rs (2 hunks)
- lightning/src/ln/peer_handler.rs (5 hunks)
- lightning/src/onion_message/functional_tests.rs (5 hunks)
- lightning/src/onion_message/messenger.rs (5 hunks)
- lightning/src/onion_message/offers.rs (4 hunks)
- lightning/src/onion_message/packet.rs (2 hunks)
Additional comments: 32
lightning/src/onion_message/offers.rs (3)
- 15-15: The import of
OnionMessageHandleraligns with the PR's objective to refactor and simplify the codebase, particularly by moving thehandle_onion_message_responsedeclaration to theOnionMessageHandlertrait. This change is consistent and necessary for the new functionality.- 33-33: The introduction of
ReceivedOnionMessageis a key part of the PR's objective to enhance message handling capabilities by exposingreply_pathandpath_id. This change is crucial for enabling more complex operations within message handlers.- 123-125: The implementation of the
msg_typemethod forOffersMessageis a new addition that supports the changes introduced by theReceivedOnionMessageenum. This method enhances the system's ability to handle and respond to onion messages efficiently by providing a straightforward way to identify the message type. This is a good practice for maintainability and clarity.lightning/src/onion_message/packet.rs (2)
- 160-161: The addition of the
msg_typemethod to theOnionMessageContentstrait is a strategic enhancement that aligns with the PR's objectives. It provides a standardized way to retrieve the message type, which can be particularly useful for logging, debugging, and handling messages based on their type. This change promotes better code organization and readability.- 138-143: The implementation of the
msg_typemethod forParsedOnionMessageContentsis correctly done, ensuring that each variant returns the appropriate message type. This implementation is essential for the functionality introduced by the PR and follows the best practices of ensuring that traits are properly implemented for enums that represent different message types.fuzz/src/onion_message.rs (1)
- 126-129: The implementation of
handle_custom_messagecorrectly matches the PR's objective by accepting a reference toReceivedOnionMessageand handling it appropriately. However, it's important to ensure that theresponder.respondmethod is capable of handling theTestCustomMessagewithout any issues, especially in terms of type compatibility and expected behavior.Ensure that the
responder.respondmethod is properly implemented and tested for handling instances ofTestCustomMessage. This might involve verifying the method's implementation in other parts of the codebase or adding unit tests if they are not already present.lightning/src/onion_message/functional_tests.rs (3)
- 73-73: The implementation of
handle_messageinTestOffersMessageHandleris empty. While this might be intentional for a test stub, it's important to ensure that this aligns with the testing objectives. If the method is supposed to simulate or test specific behavior, its current implementation won't achieve that.- 94-96: The
msg_typemethod inTestCustomMessagereturns a static string"Custom". This implementation is straightforward and seems correct for testing purposes, assuming that the differentiation betweenRequestandResponseis not needed at this level.- 442-444: The
msg_typemethod inInvalidCustomMessagereturns a static string"Invalid". This is consistent with the intended use of this struct to represent an invalid message type for testing. The implementation is straightforward and correct for its purpose.lightning/src/onion_message/messenger.rs (5)
- 246-250: The
Responderstruct introduces a way to encapsulate a response mechanism within the onion messaging system. It holds a reference to theOnionMessageHandler, the message to respond to, the reply path, and an optional path ID. This design effectively encapsulates the response logic, making it easier to manage responses in a structured way. However, it's important to ensure that the lifetime'aassociated with themessengerreference does not lead to dangling references or lifetime management issues in practice.- 265-267: The
MessageWithIdstruct is a simple yet effective way to associate a message with an optional path ID. This struct will be particularly useful for handling messages without a reply path, simplifying the tracking and management of such messages. The use of generics for the message type (T: OnionMessageContents) ensures flexibility and reusability of this struct across different message types.- 270-273: The introduction of the
ReceivedOnionMessageenum is a significant improvement in handling onion messages, as it clearly differentiates between messages with and without a reply path. This differentiation is crucial for implementing asynchronous responses and other complex operations within message handlers. The use of generics (OMH: OnionMessageHandler, T: OnionMessageContents) and the inclusion of theResponderandMessageWithIdstructs as variants enhance the modularity and flexibility of message handling.- 551-551: The modification to the
handle_custom_messagefunction in theCustomOnionMessageHandlertrait to accept a reference toReceivedOnionMessageinstead ofSelf::CustomMessageis a logical change that aligns with the introduction of theReceivedOnionMessageenum. This change allows custom message handlers to leverage the new message handling capabilities introduced by the enum, such as distinguishing between messages based on the availability of a reply path. It's important to ensure that all implementations of this trait are updated accordingly to handle the new parameter type.- 956-961: The usage of the
ReceivedOnionMessageenum within thehandle_onion_messagemethod of theOnionMessengerstruct demonstrates the practical application of the enum in differentiating message handling based on the presence of a reply path. Creating instances ofReceivedOnionMessagewith eitherWithReplyPathorWithoutReplyPathvariants based on the actual message context is a clear and concise way to handle the varied nature of onion messages. This approach enhances the clarity and maintainability of the message handling logic.lightning/src/ln/peer_handler.rs (15)
- 18-24: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [21-32]
The import of
ReceivedOnionMessageis correctly added to support the new enum handling in message processing.
- 127-127: The addition of the
handle_onion_message_responsemethod in theOnionMessageHandlerimplementation forIgnoringMessageHandleraligns with the PR's objective to enhance message handling capabilities by introducing theReceivedOnionMessageenum.- 139-139: The implementation of
handle_messageforOffersMessageHandlerwith the generic typeOMHand parameterreceived_onion_messageis consistent with the PR's goal to refactor message handling to utilize the newReceivedOnionMessageenum.- 143-143: The addition of
handle_custom_messagein theCustomOnionMessageHandlerimplementation forIgnoringMessageHandleris a necessary update to accommodate the newReceivedOnionMessageenum. This change ensures that custom onion messages can be processed correctly.- 157-157: The implementation of
msg_typeforInfalliblein theOnionMessageContentstrait is unreachable, which is appropriate sinceInfalliblecannot be instantiated. This serves as a placeholder to satisfy the trait requirements.- 18-24: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [21-32]
Overall, the changes to the
peer_handler.rsfile are consistent with the PR's objectives to introduce theReceivedOnionMessageenum and refactor message handling. The modifications are logically structured and align with the described enhancements in message processing capabilities. The implementation details, such as the addition of new methods and the update to trait implementations, are correctly addressed to support the new enum handling.Also applies to: 127-127, 139-139, 143-143, 157-157
- 18-24: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [216-216]
The
fuzz_threaded_connectionstest is a valuable addition that simulates a real-world scenario where peers rapidly connect and disconnect. This test can help uncover potential issues related to race conditions and map consistency.
- 18-24: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [217-217]
The
test_feature_incompatible_peersandtest_chain_incompatible_peerstests correctly address the scenarios where peers are incompatible due to feature flags or chain hashes. These tests ensure that the peer manager properly handles such incompatibilities by disconnecting the peers.
- 18-24: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [218-218]
The
test_disconnect_peerandtest_disconnect_all_peertests effectively verify the functionality for disconnecting a single peer or all peers, respectively. These tests are crucial for ensuring that the peer manager can manage peer connections as expected.
- 18-24: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [219-219]
The
test_timer_tick_occurredtest checks the behavior of the peer manager when the timer tick occurs, specifically testing the ping/pong mechanism and the disconnection of peers that do not respond. This test is important for validating the keep-alive mechanism of the peer connections.
- 18-24: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [220-220]
The
test_do_attempt_write_datatest ensures that the peer manager can handle situations where there are more channel messages available than can fit into a peer's buffer. This test is crucial for verifying the peer manager's ability to manage message queues and prevent infinite loops.
- 18-24: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [221-221]
The
test_handshake_timeouttest validates the peer manager's behavior when a peer does not complete the handshake within the expected timeframe. This test ensures that the peer manager can detect and disconnect peers that fail to complete the handshake, maintaining the integrity of the network.
- 18-24: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [222-222]
The
test_send_simple_msgtest checks the basic functionality of sending a message from one peer to another. This test is fundamental for ensuring that the peer manager can facilitate communication between peers.
- 18-24: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [223-223]
The
test_non_init_first_msgtest ensures that the peer manager correctly handles the scenario where the first message received is not anInitmessage. This test is important for maintaining the protocol's requirement that theInitmessage must be the first message exchanged.
- 18-24: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [224-224]
The
test_process_events_multithreadedtest is a thorough examination of theprocess_eventsmethod's behavior when called from multiple threads. This test is crucial for ensuring that the peer manager can handle concurrent events without entering into excessive loop iterations or encountering race conditions.lightning/src/ln/msgs.rs (1)
- 1595-1596: The method
handle_onion_message_responseis added but not used within the provided context. It's unclear how this method integrates with the rest of the system or its intended use case. Without additional context or usage examples, it's difficult to assess its correctness, potential side effects, or how it interacts with other components.Verification successful
The method
handle_onion_message_responseis indeed used within the codebase, as indicated by its reference inlightning/src/onion_message/messenger.rsand its implementation in bothlightning/src/onion_message/messenger.rsandlightning/src/ln/peer_handler.rs. This demonstrates that the method is integrated and has a specific use case within the system, particularly in the context of handling onion message responses. Therefore, the initial concern about the method's integration and intended use case is addressed by the new context provided.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of `handle_onion_message_response` method within the codebase. rg "handle_onion_message_response"Length of output: 559
lightning/src/ln/channelmanager.rs (2)
- 67-67: The import of
ReceivedOnionMessagefromcrate::onion_message::messengeraligns with the PR's objective to enhance message handling capabilities by exposingreply_pathandpath_id. This change is crucial for enabling more complex operations within message handlers.- 78-79: The import of
OnionMessageHandlerfromsuper::msgsis a preparatory step for the changes in message handling logic. It's important to ensure that all necessary traits and interfaces are available for the new logic introduced in the PR.
lightning/src/ln/channelmanager.rs
Outdated
| Err(error) => Some(OffersMessage::InvoiceError(error.into())), | ||
| } | ||
| } else { | ||
| #[cfg(feature = "std")] | ||
| let builder = invoice_request.respond_with(payment_paths, payment_hash); | ||
| #[cfg(not(feature = "std"))] | ||
| let builder = invoice_request.respond_with_no_std( | ||
| payment_paths, payment_hash, created_at | ||
| ); | ||
| let response = builder.and_then(|builder| builder.allow_mpp().build()) | ||
| .map_err(|e| OffersMessage::InvoiceError(e.into())) | ||
| .and_then(|invoice| | ||
| match invoice.sign(|invoice| self.node_signer.sign_bolt12_invoice(invoice)) { | ||
| Ok(invoice) => Ok(OffersMessage::Invoice(invoice)), | ||
| Err(SignError::Signing(())) => Err(OffersMessage::InvoiceError( | ||
| InvoiceError::from_string("Failed signing invoice".to_string()) | ||
| )), | ||
| Err(SignError::Verification(_)) => Err(OffersMessage::InvoiceError( | ||
| InvoiceError::from_string("Failed invoice signature verification".to_string()) | ||
| )), | ||
| }); | ||
| match response { | ||
| Ok(invoice) => Some(invoice), | ||
| Err(error) => Some(error), | ||
| } | ||
| } | ||
| } else { | ||
| #[cfg(feature = "std")] | ||
| let builder = invoice_request.respond_with(payment_paths, payment_hash); | ||
| #[cfg(not(feature = "std"))] | ||
| let builder = invoice_request.respond_with_no_std( | ||
| payment_paths, payment_hash, created_at | ||
| ); | ||
| let response = builder.and_then(|builder| builder.allow_mpp().build()) | ||
| .map_err(|e| OffersMessage::InvoiceError(e.into())) | ||
| .and_then(|invoice| | ||
| match invoice.sign(|invoice| self.node_signer.sign_bolt12_invoice(invoice)) { | ||
| Ok(invoice) => Ok(OffersMessage::Invoice(invoice)), | ||
| Err(SignError::Signing(())) => Err(OffersMessage::InvoiceError( | ||
| InvoiceError::from_string("Failed signing invoice".to_string()) | ||
| )), | ||
| Err(SignError::Verification(_)) => Err(OffersMessage::InvoiceError( | ||
| InvoiceError::from_string("Failed invoice signature verification".to_string()) | ||
| )), | ||
| }); | ||
| match response { | ||
| Ok(invoice) => Some(invoice), | ||
| Err(error) => Some(error), | ||
| }, | ||
| OffersMessage::Invoice(invoice) => { | ||
| match invoice.verify(expanded_key, secp_ctx) { | ||
| Err(()) => { | ||
| Some(OffersMessage::InvoiceError(InvoiceError::from_string("Unrecognized invoice".to_owned()))) | ||
| }, | ||
| Ok(_) if invoice.invoice_features().requires_unknown_bits_from(&self.bolt12_invoice_features()) => { | ||
| Some(OffersMessage::InvoiceError(Bolt12SemanticError::UnknownRequiredFeatures.into())) | ||
| }, | ||
| Ok(payment_id) => { | ||
| if let Err(e) = self.send_payment_for_bolt12_invoice(&invoice, payment_id) { | ||
| log_trace!(self.logger, "Failed paying invoice: {:?}", e); | ||
| Some(OffersMessage::InvoiceError(InvoiceError::from_string(format!("{:?}", e)))) | ||
| } else { | ||
| None | ||
| } | ||
| }, | ||
| } | ||
| } | ||
| }, | ||
| OffersMessage::Invoice(invoice) => { | ||
| match invoice.verify(expanded_key, secp_ctx) { | ||
| Err(()) => { | ||
| Some(OffersMessage::InvoiceError(InvoiceError::from_string("Unrecognized invoice".to_owned()))) | ||
| }, | ||
| Ok(_) if invoice.invoice_features().requires_unknown_bits_from(&self.bolt12_invoice_features()) => { | ||
| Some(OffersMessage::InvoiceError(Bolt12SemanticError::UnknownRequiredFeatures.into())) | ||
| }, | ||
| Ok(payment_id) => { | ||
| if let Err(e) = self.send_payment_for_bolt12_invoice(&invoice, payment_id) { | ||
| log_trace!(self.logger, "Failed paying invoice: {:?}", e); | ||
| Some(OffersMessage::InvoiceError(InvoiceError::from_string(format!("{:?}", e)))) | ||
| } else { | ||
| None | ||
| } | ||
| }, | ||
| } | ||
| }, | ||
| OffersMessage::InvoiceError(invoice_error) => { | ||
| log_trace!(self.logger, "Received invoice_error: {}", invoice_error); | ||
| None | ||
| }, | ||
| }, | ||
| OffersMessage::InvoiceError(invoice_error) => { | ||
| log_trace!(self.logger, "Received invoice_error: {}", invoice_error); | ||
| None | ||
| }, | ||
| }; | ||
|
|
||
| if let Some(response) = response_option { | ||
| responder.respond(response); | ||
| } |
There was a problem hiding this comment.
The implementation of OffersMessageHandler for ChannelManager has been significantly updated to handle different types of messages using the ReceivedOnionMessage enum. This change is central to the PR's objectives. However, there are several areas that require attention:
-
Complexity and Length: The
handle_messagemethod is quite lengthy and handles multiple scenarios, which could impact maintainability. Consider breaking down this method into smaller, more focused methods, each handling a specific type ofOffersMessage. -
Error Handling: The method uses
return responder.respond(OffersMessage::InvoiceError(error.into())),in multiple places to handle errors. This pattern is consistent and correctly uses theresponderto send back error messages. However, ensure that all possible error scenarios are covered and that the error messages are informative and user-friendly. -
Conditional Compilation: The use of
#[cfg(feature = "std")]and#[cfg(not(feature = "std"))]to differentiate between standard and no_std environments is necessary but adds complexity. Ensure that both paths are thoroughly tested to prevent regressions in either environment. -
Performance Considerations: The method performs several operations that could be expensive, such as cloning
invoice_requestand creating payment paths. While necessary for the functionality, be mindful of the potential impact on performance, especially in high-throughput scenarios. Consider optimizations or caching strategies where applicable. -
Best Practices: The method demonstrates good practices in terms of using enums to handle different message types and leveraging traits for modularity. Continue to follow these patterns to maintain code quality.
Overall, the changes are in line with the PR's objectives but could benefit from further refinement to enhance maintainability and performance.
Consider breaking down the handle_message method into smaller methods to improve maintainability. Additionally, review performance-critical sections for potential optimizations.
| fn handle_custom_message<OMH: OnionMessageHandler>(&self, received_onion_message: &ReceivedOnionMessage<OMH, Self::CustomMessage>) { | ||
| if let ReceivedOnionMessage::WithReplyPath(responder) = received_onion_message { | ||
| match self.expected_messages.lock().unwrap().pop_front() { | ||
| Some(expected_msg) => assert_eq!(expected_msg, responder.message), | ||
| None => panic!("Unexpected message: {:?}", responder.message), | ||
| } | ||
|
|
||
| match msg { | ||
| TestCustomMessage::Request => Some(TestCustomMessage::Response), | ||
| TestCustomMessage::Response => None, | ||
| let response_option = match responder.message { | ||
| TestCustomMessage::Request => Some(TestCustomMessage::Response), | ||
| TestCustomMessage::Response => None, | ||
| }; | ||
|
|
||
| if let Some(response) = response_option { | ||
| responder.respond(response) | ||
| } |
There was a problem hiding this comment.
The handle_custom_message method in TestCustomMessageHandler correctly handles the ReceivedOnionMessage enum, particularly the WithReplyPath variant. This implementation demonstrates proper pattern matching and conditional logic based on the message type. However, ensure that the panic in the None case of the expected_messages deque is acceptable for your testing strategy. In a production-like test environment, you might want to handle this case more gracefully.
Consider adding a more descriptive error message or handling the unexpected message case without panicking to improve test robustness and readability.
| @@ -40,7 +42,7 @@ pub trait OffersMessageHandler { | |||
| /// The returned [`OffersMessage`], if any, is enqueued to be sent by [`OnionMessenger`]. | |||
| /// | |||
| /// [`OnionMessenger`]: crate::onion_message::messenger::OnionMessenger | |||
| fn handle_message(&self, message: OffersMessage) -> Option<OffersMessage>; | |||
| fn handle_message<OMH: OnionMessageHandler>(&self, received_onion_message: &ReceivedOnionMessage<OMH, OffersMessage>); | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process has confirmed that two out of the three identified implementations of OffersMessageHandler (functional_tests.rs and peer_handler.rs) have been updated to match the new handle_message function signature, including generic types and the ReceivedOnionMessage enum. However, the implementation in fuzz/src/onion_message.rs has not been updated to reflect these changes, indicating an inconsistency in the codebase. Therefore, the review comment is validated in its concern, but an issue has been identified with one implementation not adhering to the updated signature.
Analysis chain
The modification of the handle_message function signature to include generic types (OMH: OnionMessageHandler) and the new ReceivedOnionMessage enum is a significant change. It allows the message handlers to process messages more effectively by utilizing the information provided by the new enum. However, it's important to ensure that all implementations of OffersMessageHandler across the codebase are updated to match this new signature.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations of OffersMessageHandler to ensure they've been updated.
rg --type rust 'impl OffersMessageHandler'
Length of output: 320
Script:
#!/bin/bash
# Check the handle_message method implementation in lightning/src/onion_message/functional_tests.rs for TestOffersMessageHandler
rg 'fn handle_message' lightning/src/onion_message/functional_tests.rs
# Check the handle_message method implementation in lightning/src/ln/peer_handler.rs for IgnoringMessageHandler
rg 'fn handle_message' lightning/src/ln/peer_handler.rs
# Check the handle_message method implementation in fuzz/src/onion_message.rs for TestOffersMessageHandler
rg 'fn handle_message' fuzz/src/onion_message.rs
Length of output: 525
| @@ -243,6 +243,54 @@ impl OnionMessageRecipient { | |||
| } | |||
| } | |||
|
|
|||
| pub struct Responder<'a, OMH: OnionMessageHandler, T: OnionMessageContents> { | |||
| messenger: &'a OMH, | |||
| pub message: T, | |||
There was a problem hiding this comment.
Let's leave the message out of responder and put it directly in ReceivedOnionMessage::WithReplyPath. You can add a &'static str field for the message type.
There was a problem hiding this comment.
Sounds reasonable! Addressed in 8526afc
Also instead of introducing message type as a separate variable, I used the fact that we enforce response and message to be of the same type at compile time.
Let me know if we should rather have a separate message type variable, and I would love to make that change.
| pub path_id: Option<[u8; 32]>, | ||
| } | ||
|
|
||
| pub enum ReceivedOnionMessage<'a, OMH: OnionMessageHandler, T: OnionMessageContents> { |
There was a problem hiding this comment.
Addressed in 8526afc
Currently added very rudimentary docs. Let me know if you have some recommendations to expand to docs, and I would love to incorporate them!
|
Updated from pr2907.01 to pr2907.02 (diff): Changes:
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (6)
- fuzz/src/onion_message.rs (2 hunks)
- lightning/src/ln/channelmanager.rs (3 hunks)
- lightning/src/ln/peer_handler.rs (5 hunks)
- lightning/src/onion_message/functional_tests.rs (5 hunks)
- lightning/src/onion_message/messenger.rs (5 hunks)
- lightning/src/onion_message/offers.rs (4 hunks)
Files skipped from review as they are similar to previous changes (5)
- fuzz/src/onion_message.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/onion_message/functional_tests.rs
- lightning/src/onion_message/offers.rs
Additional comments: 3
lightning/src/onion_message/messenger.rs (3)
- 246-269: The
Responderstruct and itsrespondmethod are well-implemented, providing a clear and type-safe way for message handlers to send responses. However, it's important to ensure that thehandle_onion_message_responsemethod called onself.messengerproperly handles errors or unexpected behavior, especially since the response sending process involves network operations that can fail.- 563-563: The modification to the
CustomOnionMessageHandlertrait to useReceivedOnionMessageinstead of a generic message type is a significant improvement. It allows custom message handlers to uniformly handle messages regardless of whether they include a reply path. This change enhances the flexibility and usability of custom onion message handlers. Ensure that all implementations of this trait are updated accordingly to accommodate this change.Verification successful
The verification process has confirmed that all implementations of
CustomOnionMessageHandleracross the codebase have been updated to include thehandle_custom_messagemethod, in line with the modifications mentioned in the review comment. This ensures uniform handling of messages by custom message handlers, enhancing flexibility and usability as noted.* 968-973: The usage of `ReceivedOnionMessage::new` within the `handle_onion_message` method for both offers and custom messages is correct and demonstrates the utility of the `ReceivedOnionMessage` enum in simplifying the message handling process. This approach ensures that message handlers can easily access the necessary information, such as the reply path, to process the message appropriately. Good job on this refactor.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all implementations of CustomOnionMessageHandler have been updated. rg "impl CustomOnionMessageHandler" --type rust | xargs -I {} bash -c 'grep -q "fn handle_custom_message" {} || echo "Missing implementation in {}"'Length of output: 1307
Script:
#!/bin/bash # Corrected script to verify that all implementations of CustomOnionMessageHandler have been updated. rg "impl CustomOnionMessageHandler" --type rust -l | while read -r file; do if ! grep -q "fn handle_custom_message" "$file"; then echo "Missing implementation in $file" fi doneLength of output: 330
| /// A enum to handle received [`OnionMessage`] | ||
| pub enum ReceivedOnionMessage<'a, OMH: OnionMessageHandler, T: OnionMessageContents> { | ||
| WithReplyPath { | ||
| message: T, | ||
| responder: Responder<'a, OMH, T>, | ||
| }, | ||
| WithoutReplyPath { | ||
| message: T, | ||
| path_id: Option<[u8; 32]>, | ||
| }, | ||
| } | ||
|
|
||
| impl<'a, OMH: OnionMessageHandler, T: OnionMessageContents> ReceivedOnionMessage<'a, OMH, T> { | ||
| fn new(messenger: &'a OMH, message: T, reply_path_option: Option<BlindedPath>, path_id: Option<[u8; 32]> ) -> Self { | ||
| match reply_path_option { | ||
| Some(reply_path) => { | ||
| let responder = Responder { | ||
| messenger, | ||
| reply_path, | ||
| path_id, | ||
| _phantom: std::marker::PhantomData | ||
| }; | ||
| ReceivedOnionMessage::WithReplyPath { | ||
| message, | ||
| responder | ||
| } | ||
| } | ||
| None => ReceivedOnionMessage::WithoutReplyPath { | ||
| message, | ||
| path_id | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The ReceivedOnionMessage enum effectively encapsulates the two possible states of a received onion message: one with a reply path and one without. This design choice simplifies the handling logic in downstream code. However, consider adding documentation to explain the purpose of the path_id field and how it should be used, as it's a critical part of the onion message protocol that might not be immediately clear to future maintainers or users of the code.
Would you like me to draft the additional documentation for the path_id field?
There was a problem hiding this comment.
@shaavan: I've drafted the additional documentation for the path_id field in the ReceivedOnionMessage enum. Here it is:
/// The `path_id` field is an optional identifier associated with the routing path of an onion message.
/// It serves several purposes:
/// - **Routing Responses**: In cases where a `reply_path` is provided, `path_id` helps in routing the response
/// back through the same path the original message traversed. This is crucial for maintaining the anonymity
/// properties of the onion message protocol.
/// - **Debugging and Logging**: `path_id` can also be useful for debugging purposes, allowing developers and
/// network operators to trace the flow of messages through the network and identify potential routing issues.
/// - **Message Correlation**: It enables the correlation of messages and responses, ensuring that responders
/// can match incoming messages with their corresponding replies.
///
/// It's important to note that `path_id` is optional and may not be present in all onion messages. Its usage
/// is determined by the specifics of the onion message's routing requirements and the implementations of the
/// onion message handlers.Please let me know if this aligns with your expectations or if there are any adjustments you'd like to make.
| @@ -243,6 +243,66 @@ impl OnionMessageRecipient { | |||
| } | |||
| } | |||
|
|
|||
| /// A struct handling response to an [`OnionMessage`] | |||
| pub struct Responder<'a, OMH: OnionMessageHandler, T: OnionMessageContents> { | |||
| messenger: &'a OMH, | |||
There was a problem hiding this comment.
Instead of expanding OnionMessageHandler's definition, could we use a Fn or similar trait?
| pub path_id: Option<[u8; 32]>, | ||
| // This phantom Data is used to ensure that we use T in the struct definition | ||
| // This allow us to ensure at compile time that the received message type and response type will be same | ||
| _phantom: std::marker::PhantomData<T> |
There was a problem hiding this comment.
Please use core instead of std.
| response, self.reply_path.clone(), format_args!( | ||
| "when responding to {} onion message with path_id {:02x?}", | ||
| message_type, | ||
| self.path_id.clone() |
| // Utilising the fact that we ensure at compile time that | ||
| // received message type, and response type will be same | ||
| let message_type = T::msg_type(&response); | ||
| self.messenger.handle_onion_message_response( |
There was a problem hiding this comment.
Use tabs instead of spaces.
| pub struct Responder<'a, OMH: OnionMessageHandler, T: OnionMessageContents> { | ||
| messenger: &'a OMH, | ||
| pub reply_path: BlindedPath, | ||
| pub path_id: Option<[u8; 32]>, |
There was a problem hiding this comment.
I'm not entirely sure if it is possible, but it would be nice to have some symmetry in the enum variants by having the path_id directly in both. Perhaps by storing FormatArgs? But you may run into some lifetime issues. Copying the path_id wouldn't be horrible, though.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 11
Configuration used: CodeRabbit UI
Files selected for processing (6)
- fuzz/src/onion_message.rs (2 hunks)
- lightning/src/ln/channelmanager.rs (3 hunks)
- lightning/src/ln/peer_handler.rs (5 hunks)
- lightning/src/onion_message/functional_tests.rs (5 hunks)
- lightning/src/onion_message/messenger.rs (6 hunks)
- lightning/src/onion_message/offers.rs (4 hunks)
Additional comments: 20
lightning/src/onion_message/offers.rs (4)
- 14-14: The addition of the
BlindedPathimport suggests that this type is now being used within the file, likely due to the changes in how messages are handled. Ensure thatBlindedPathis indeed utilized in the updated code, particularly in the context of thehandle_messagefunction signature change.- 16-16: The import of
OnionMessageHandlerindicates an extension or modification of the onion message handling capabilities. Given the PR's objectives, this aligns with the expected enhancements to message handling, including the ability to respond to messages with or without a reply path.- 34-34: The addition of the
ReceivedOnionMessageimport is crucial for the implementation of the new enum and its usage within thehandle_messagefunction. This change is central to the PR's objectives, enabling the handling of messages with or without a reply path. It's important to verify thatReceivedOnionMessageis used appropriately in the function signature and implementation.- 124-126: The addition of the
msg_typefunction implementation provides a way to identify the type of message being handled. This could be useful for logging, metrics, or conditional logic based on the message type. Ensure that this implementation is consistent with the rest of the codebase and that the returned string accurately reflects the message type.lightning/src/onion_message/functional_tests.rs (4)
- 19-19: The addition of
ReceivedOnionMessagein theusestatement aligns with the PR's objective to enhance message handling capabilities. This change is necessary for the subsequent modifications in the file to compile successfully.- 73-73: The
handle_messagemethod inTestOffersMessageHandlerhas been modified to accept a reference toReceivedOnionMessage. However, the method body is empty, which might be intentional for testing purposes, but it's important to ensure that this aligns with the intended test scenarios. If the method is expected to perform actions based on the received message, its implementation should be completed.- 94-96: The addition of the
msg_typemethod in theTestCustomMessageimplementation is a good practice for providing more descriptive information about the message type. This can be useful for debugging and logging purposes. The method correctly returns a static string reference.- 135-149: The
handle_custom_messagemethod inTestCustomMessageHandlerhas been significantly modified to handle theReceivedOnionMessageenum, specifically theWithReplyPathvariant. This change allows the handler to process messages with or without a reply path, enhancing the flexibility of message handling. The method correctly matches on the message variant and processes the expected message types (RequestandResponse). The use ofresponder.respond(response)for sending a response when applicable is appropriate. However, it's important to ensure that theresponderobject is correctly implemented and tested elsewhere to handle the response sending logic effectively.lightning/src/onion_message/messenger.rs (1)
- 587-587: The
handle_custom_messagemethod in theCustomOnionMessageHandlertrait now accepts aReceivedOnionMessageinstead of a raw message. This change aligns with the new handling strategy for onion messages. Ensure that all implementations of this trait are updated accordingly and that they properly handle bothWithReplyPathandWithoutReplyPathvariants to maintain the integrity of message processing.Verification successful
Given the provided output, it's clear that the
handle_custom_messagemethod has been updated in some implementations to accept aReceivedOnionMessageparameter. However, the output also shows multiple signatures for this method inpeer_handler.rs, some of which do not align with the expected update. To accurately verify the handling ofWithReplyPathandWithoutReplyPathvariants and ensure all implementations have been correctly updated, further inspection of the method implementations in the mentioned files is necessary. At this point, it's not possible to definitively conclude whether all requirements have been met based on the provided output alone.
Based on the new context provided from the script execution, it's clear that there are multiple implementations of
CustomOnionMessageHandleracross different files in the codebase. Specifically, the methodhandle_custom_messageis implemented in bothfuzz/src/onion_message.rsandlightning/src/onion_message/functional_tests.rswith the expected signature that accepts aReceivedOnionMessage. Additionally, there are implementations inlightning/src/ln/peer_handler.rsthat match the expected method signature, including handling of theInfallibletype, which suggests a correct handling of custom messages according to the new strategy.Given this information, it can be concluded that the implementations of
CustomOnionMessageHandlerhave been updated accordingly to handleReceivedOnionMessageand properly process bothWithReplyPathandWithoutReplyPathvariants, maintaining the integrity of message processing as mentioned in the review comment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify all implementations of CustomOnionMessageHandler handle ReceivedOnionMessage correctly. rg "impl CustomOnionMessageHandler for" --files-with-matches | xargs rg "fn handle_custom_message"Length of output: 1036
Script:
#!/bin/bash # Inspect the implementations of handle_custom_message in the mentioned files to verify correct handling of ReceivedOnionMessage. fd "onion_message.rs" fuzz/src/ | xargs cat fd "functional_tests.rs" lightning/src/onion_message/ | xargs cat fd "peer_handler.rs" lightning/src/ln/ | xargs catLength of output: 209934
Script:
#!/bin/bash # Verify all implementations of CustomOnionMessageHandler handle ReceivedOnionMessage correctly. rg "impl CustomOnionMessageHandler for" --files-with-matches | xargs rg "fn handle_custom_message"Length of output: 1036
lightning/src/ln/peer_handler.rs (9)
- 18-24: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [21-32]
The import of
ReceivedOnionMessageand related types is correctly added to support the new message handling functionality. This change aligns with the PR's objective to enhance onion message handling capabilities.
- 139-139: The implementation of the
handle_messagemethod in theOffersMessageHandlertrait forIgnoringMessageHandleris a necessary addition to support the handling of offer messages. This change is consistent with the PR's goal to improve message processing efficiency and clarity.- 143-143: The implementation of the
handle_custom_messagemethod in theCustomOnionMessageHandlertrait forIgnoringMessageHandleris crucial for handling custom onion messages. This addition aligns with the PR's objectives to enhance onion message handling capabilities.- 157-157: The implementation of the
tlv_typeandmsg_typemethods forInfallibleunderOnionMessageContentsis necessary for the trait's completeness. However, the use ofunreachable!()indicates these methods should never be called forInfallible. This approach is acceptable given the context, but it's important to ensure that the logic preventing these calls is robust to avoid potential runtime panics.- 32-32: The addition of imports related to onion messaging (
CustomOnionMessageHandler,PendingOnionMessage,ReceivedOnionMessage) is consistent with the PR's objectives to enhance onion message handling. This change is necessary for the new functionality introduced in the PR.- 127-127: Adding the
handle_onion_message_responsemethod to theOnionMessageHandlertrait implementation forIgnoringMessageHandleris crucial for handling onion message responses. This change aligns with the PR's goal to improve the handling of onion messages.- 139-139: Implementing the
handle_messagemethod forOffersMessageHandlerinIgnoringMessageHandleris essential for processing offer messages. This addition supports the PR's objectives to enhance message processing capabilities.- 143-143: The implementation of
handle_custom_messageforCustomOnionMessageHandlerinIgnoringMessageHandleris necessary for handling custom onion messages. This change is in line with the PR's objectives to improve onion message handling.- 157-157: Implementing the
tlv_typeandmsg_typemethods forInfallibleunderOnionMessageContentsusingunreachable!()is an acceptable approach given the context. However, it's important to ensure that these methods are indeed never called to avoid runtime panics.lightning/src/ln/channelmanager.rs (2)
- 67-67: The reordering of imports improves readability and maintains consistency with Rust's conventional import ordering (std, external crates, then internal modules). This change is straightforward and enhances code maintainability.
- 78-78: This conditional import (
#[cfg(not(c_bindings))]) is a good practice for feature-specific code, ensuring that unnecessary dependencies are not compiled into the binary when thec_bindingsfeature is not enabled. This keeps the binary size optimized and the compile time shorter for those configurations.
| @@ -40,7 +43,7 @@ pub trait OffersMessageHandler { | |||
| /// The returned [`OffersMessage`], if any, is enqueued to be sent by [`OnionMessenger`]. | |||
| /// | |||
| /// [`OnionMessenger`]: crate::onion_message::messenger::OnionMessenger | |||
| fn handle_message(&self, message: OffersMessage) -> Option<OffersMessage>; | |||
| fn handle_message<F: Fn(OffersMessage, BlindedPath)>(&self, message: &ReceivedOnionMessage<F, OffersMessage>); | |||
There was a problem hiding this comment.
The modification of the handle_message function signature to include a closure is a significant change. This adjustment likely facilitates more flexible handling of messages, possibly allowing for asynchronous operations or more complex logic when dealing with received messages. It's essential to ensure that this change integrates well with the existing architecture and does not introduce unnecessary complexity or potential issues with closure capture and execution.
Consider simplifying the function signature if the closure's flexibility is not fully utilized or if it introduces complexity that could be avoided with a more straightforward approach.
fuzz/src/onion_message.rs
Outdated
| fn handle_message(&self, _message: OffersMessage) -> Option<OffersMessage> { | ||
| None | ||
| } | ||
| fn handle_message<F: Fn(OffersMessage, BlindedPath)>(&self, _message: &ReceivedOnionMessage<OMH, OffersMessage>) {} |
There was a problem hiding this comment.
The handle_message method in TestOffersMessageHandler has been modified to accept a ReceivedOnionMessage<OMH, OffersMessage> parameter, but the method body is empty. This might be intentional for a test stub, but it's important to ensure that this aligns with the intended testing strategy. If the method is supposed to perform actions based on the received message, this would be the place to implement those actions or at least leave a TODO comment for future implementation.
fuzz/src/onion_message.rs
Outdated
| fn handle_custom_message<F: Fn(Self::CustomMessage, BlindedPath), T: OnionMessageContents>(&self, message: &ReceivedOnionMessage<F, T>) { | ||
| if let ReceivedOnionMessage::WithReplyPath({_, responder}) = message { | ||
| responder.respond(TestCustomMessage {}) | ||
| } |
There was a problem hiding this comment.
The handle_custom_message method in TestCustomMessageHandler has been updated to handle a ReceivedOnionMessage and respond based on the message type. This implementation only handles the WithReplyPath variant and ignores the WithoutReplyPath variant. It's crucial to ensure that this behavior is intentional and aligns with the testing objectives. If handling of the WithoutReplyPath variant is required, consider adding an else branch to handle it appropriately.
lightning/src/ln/channelmanager.rs
Outdated
| Err(error) => Some(OffersMessage::InvoiceError(error.into())), | ||
| } | ||
| } else { | ||
| #[cfg(feature = "std")] | ||
| let builder = invoice_request.respond_with(payment_paths, payment_hash); | ||
| #[cfg(not(feature = "std"))] | ||
| let builder = invoice_request.respond_with_no_std( | ||
| payment_paths, payment_hash, created_at | ||
| ); | ||
| let response = builder.and_then(|builder| builder.allow_mpp().build()) | ||
| .map_err(|e| OffersMessage::InvoiceError(e.into())) | ||
| .and_then(|invoice| | ||
| match invoice.sign(|invoice| self.node_signer.sign_bolt12_invoice(invoice)) { | ||
| Ok(invoice) => Ok(OffersMessage::Invoice(invoice)), | ||
| Err(SignError::Signing(())) => Err(OffersMessage::InvoiceError( | ||
| InvoiceError::from_string("Failed signing invoice".to_string()) | ||
| )), | ||
| Err(SignError::Verification(_)) => Err(OffersMessage::InvoiceError( | ||
| InvoiceError::from_string("Failed invoice signature verification".to_string()) | ||
| )), | ||
| }); | ||
| match response { | ||
| Ok(invoice) => Some(invoice), | ||
| Err(error) => Some(error), | ||
| } | ||
| } | ||
| } else { | ||
| #[cfg(feature = "std")] | ||
| let builder = invoice_request.respond_with(payment_paths, payment_hash); | ||
| #[cfg(not(feature = "std"))] | ||
| let builder = invoice_request.respond_with_no_std( | ||
| payment_paths, payment_hash, created_at | ||
| ); | ||
| let response = builder.and_then(|builder| builder.allow_mpp().build()) | ||
| .map_err(|e| OffersMessage::InvoiceError(e.into())) | ||
| .and_then(|invoice| | ||
| match invoice.sign(|invoice| self.node_signer.sign_bolt12_invoice(invoice)) { | ||
| Ok(invoice) => Ok(OffersMessage::Invoice(invoice)), | ||
| Err(SignError::Signing(())) => Err(OffersMessage::InvoiceError( | ||
| InvoiceError::from_string("Failed signing invoice".to_string()) | ||
| )), | ||
| Err(SignError::Verification(_)) => Err(OffersMessage::InvoiceError( | ||
| InvoiceError::from_string("Failed invoice signature verification".to_string()) | ||
| )), | ||
| }); | ||
| match response { | ||
| Ok(invoice) => Some(invoice), | ||
| Err(error) => Some(error), | ||
| }, | ||
| OffersMessage::Invoice(invoice) => { | ||
| match invoice.verify(expanded_key, secp_ctx) { | ||
| Err(()) => { | ||
| Some(OffersMessage::InvoiceError(InvoiceError::from_string("Unrecognized invoice".to_owned()))) | ||
| }, | ||
| Ok(_) if invoice.invoice_features().requires_unknown_bits_from(&self.bolt12_invoice_features()) => { | ||
| Some(OffersMessage::InvoiceError(Bolt12SemanticError::UnknownRequiredFeatures.into())) | ||
| }, | ||
| Ok(payment_id) => { | ||
| if let Err(e) = self.send_payment_for_bolt12_invoice(&invoice, payment_id) { | ||
| log_trace!(self.logger, "Failed paying invoice: {:?}", e); | ||
| Some(OffersMessage::InvoiceError(InvoiceError::from_string(format!("{:?}", e)))) | ||
| } else { | ||
| None | ||
| } | ||
| }, | ||
| } | ||
| } | ||
| }, | ||
| OffersMessage::Invoice(invoice) => { | ||
| match invoice.verify(expanded_key, secp_ctx) { | ||
| Err(()) => { | ||
| Some(OffersMessage::InvoiceError(InvoiceError::from_string("Unrecognized invoice".to_owned()))) | ||
| }, | ||
| Ok(_) if invoice.invoice_features().requires_unknown_bits_from(&self.bolt12_invoice_features()) => { | ||
| Some(OffersMessage::InvoiceError(Bolt12SemanticError::UnknownRequiredFeatures.into())) | ||
| }, | ||
| Ok(payment_id) => { | ||
| if let Err(e) = self.send_payment_for_bolt12_invoice(&invoice, payment_id) { | ||
| log_trace!(self.logger, "Failed paying invoice: {:?}", e); | ||
| Some(OffersMessage::InvoiceError(InvoiceError::from_string(format!("{:?}", e)))) | ||
| } else { | ||
| None | ||
| } | ||
| }, | ||
| } | ||
| }, | ||
| OffersMessage::InvoiceError(invoice_error) => { | ||
| log_trace!(self.logger, "Received invoice_error: {}", invoice_error); | ||
| None | ||
| }, | ||
| }, | ||
| OffersMessage::InvoiceError(invoice_error) => { | ||
| log_trace!(self.logger, "Received invoice_error: {}", invoice_error); | ||
| None | ||
| }, | ||
| }; | ||
|
|
||
| if let Some(response) = response_option { | ||
| responder.respond(response); | ||
| } |
There was a problem hiding this comment.
The refactoring of the handle_message function to accept a ReceivedOnionMessage instead of an OffersMessage and the subsequent handling of its variants (WithReplyPath and WithoutReplyPath) is a significant improvement. This change aligns with the PR's objective to enhance message handling capabilities by exposing reply_path for asynchronous responses. However, there are several areas within this implementation that require attention:
-
Error Handling and Logging: The use of
log_trace!for logging errors (lines 9372, 9381) is appropriate for debugging purposes. However, ensure that the logging level is consistent with the severity of the errors being logged. For critical errors, consider using a higher severity level. -
Complexity and Maintainability: The nested match statements and conditional logic within this function increase its complexity. Consider refactoring to separate concerns and improve readability. For example, handling each
OffersMessagevariant could be extracted into separate functions. -
Feature Flags: The use of feature flags (
#[cfg(feature = "std")]and#[cfg(not(feature = "std"))]) to differentiate between standard and no-std environments (lines 9324-9355) is necessary for cross-platform compatibility. Ensure that all conditional compilations are tested in both environments to prevent runtime issues. -
Correctness and Logic: The logic for handling different
OffersMessagevariants appears to be correct based on the provided context. However, it's crucial to ensure that all edge cases are considered, especially in error handling scenarios. For instance, the handling ofInvoiceError(lines 9380-9383) simply logs the error without taking further action. Consider if any additional steps should be taken in response to anInvoiceError. -
Performance Considerations: The cloning of
invoice_request(line 9290) might have performance implications, especially ifinvoice_requestis a large structure. Evaluate if there's a way to avoid cloning or if the performance impact is negligible.
Overall, the changes to the handle_message function are in line with the PR's objectives and improve the codebase's functionality. However, consider the above points to further refine the implementation.
Consider refactoring the handle_message function to improve readability and maintainability. Specifically, extracting the handling of each OffersMessage variant into separate functions could reduce complexity. Additionally, review error handling and logging practices to ensure they are appropriate for the severity of the errors.
lightning/src/ln/peer_handler.rs
Outdated
|
|
||
| impl OnionMessageHandler for IgnoringMessageHandler { | ||
| fn handle_onion_message(&self, _their_node_id: &PublicKey, _msg: &msgs::OnionMessage) {} | ||
| fn handle_onion_message_response<T: OnionMessageContents>(&self, _response: T, _reply_path: BlindedPath, _log_suffix: fmt::Arguments) {} |
There was a problem hiding this comment.
The addition of the handle_onion_message_response method to the OnionMessageHandler trait is a significant change. It's essential to ensure that all implementations of this trait are updated to include this new method. This change is crucial for handling onion message responses more effectively.
fn handle_onion_message_response<T: OnionMessageContents>(&self, _response: T, _reply_path: BlindedPath, _log_suffix: fmt::Arguments) {}| /// A struct handling response to an [`OnionMessage`] | ||
| pub struct Responder<R, T> | ||
| where | ||
| R: RespondFunction<T>, | ||
| T: OnionMessageContents | ||
| { | ||
| messenger_function: R, | ||
| pub reply_path: BlindedPath, | ||
| // This phantom Data is used to ensure that we use T in the struct definition | ||
| // This allow us to ensure at compile time that the received message type and response type will be same | ||
| _phantom: PhantomData<T> | ||
| } |
There was a problem hiding this comment.
The Responder struct introduces a generic way to handle responses to onion messages, leveraging the RespondFunction trait. It's important to ensure that the reply_path is always valid when respond is called, as an invalid path could lead to message delivery failures. Consider adding validation or error handling to address potential issues with the reply_path.
| /// A enum to handle received [`OnionMessage`] | ||
| pub enum ReceivedOnionMessage<R, T> | ||
| where | ||
| R: RespondFunction<T>, | ||
| T: OnionMessageContents | ||
| { | ||
| WithReplyPath { | ||
| responder: Responder<R, T>, | ||
| message: T, | ||
| path_id: Option<[u8; 32]> | ||
| }, | ||
| WithoutReplyPath { | ||
| message: T, | ||
| path_id: Option<[u8; 32]>, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The ReceivedOnionMessage enum is a key addition, distinguishing between messages with and without a reply path. This distinction is crucial for handling responses correctly. However, it's essential to ensure that all parts of the code that consume this enum handle both variants appropriately, especially in error handling and message forwarding logic, to prevent any loss of messages or incorrect handling.
| fn new(messenger_function: R, message: T, reply_path_option: Option<BlindedPath>, path_id: Option<[u8; 32]> ) -> Self { | ||
| match reply_path_option { | ||
| Some(reply_path) => { | ||
| let responder = Responder { | ||
| messenger_function, | ||
| reply_path, | ||
| _phantom: PhantomData | ||
| }; | ||
| ReceivedOnionMessage::WithReplyPath { | ||
| responder, | ||
| message, | ||
| path_id | ||
| } | ||
| } | ||
| None => ReceivedOnionMessage::WithoutReplyPath { | ||
| message, | ||
| path_id | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new function for ReceivedOnionMessage elegantly handles the creation of both variants based on the presence of a reply_path. This approach simplifies the construction of received messages. However, consider adding error handling or logging if the reply_path is expected but not provided in scenarios where it's crucial for the message processing logic.
| // message_type is defined outside of closure to avoid burrowing | ||
| // issues when using msg for creating ReceivedOnionMessage::New() | ||
| let message_type = msg.msg_type(); | ||
| let closure = |response, reply_path: BlindedPath| { | ||
| self.handle_onion_message_response( | ||
| response, reply_path, format_args!( | ||
| "when responding to {} onion message with path_id {:02x?}", | ||
| message_type, | ||
| path_id | ||
| ) | ||
| ); | ||
| }; | ||
| let message = ReceivedOnionMessage::new(closure, msg, reply_path, path_id); | ||
| self.offers_handler.handle_message(&message); | ||
| }, | ||
| ParsedOnionMessageContents::Custom(msg) => { | ||
| let response = self.custom_handler.handle_custom_message(msg); | ||
| self.handle_onion_message_response( | ||
| response, reply_path, format_args!( | ||
| "when responding to Custom onion message with path_id {:02x?}", | ||
| path_id | ||
| ) | ||
| ); | ||
| // message_type is defined outside of closure to avoid burrowing | ||
| // issues when using msg for creating ReceivedOnionMessage::New() | ||
| let message_type = msg.msg_type(); | ||
| let closure = |response, reply_path: BlindedPath| { | ||
| self.handle_onion_message_response( | ||
| response, reply_path, format_args!( | ||
| "when responding to {} onion message with path_id {:02x?}", | ||
| message_type, | ||
| path_id | ||
| ) | ||
| ); | ||
| }; | ||
| let message = ReceivedOnionMessage::new(closure, msg, reply_path, path_id); | ||
| self.custom_handler.handle_custom_message(&message); |
There was a problem hiding this comment.
The handling of ParsedOnionMessageContents::Offers and ParsedOnionMessageContents::Custom within OnionMessenger::handle_onion_message uses closures to encapsulate the response logic. This is a neat way to keep the response handling code concise. However, ensure that the closures capture all necessary context (e.g., logging context, additional metadata) and that error handling within these closures is robust, especially considering the asynchronous nature of onion message responses.
| fn handle_onion_message_response<T: OnionMessageContents>( | ||
| &self, response: T, reply_path: BlindedPath, log_suffix: fmt::Arguments | ||
| ) { | ||
| let _ = self.find_path_and_enqueue_onion_message( | ||
| response, Destination::BlindedPath(reply_path), None, log_suffix | ||
| ); | ||
| } |
There was a problem hiding this comment.
The handle_onion_message_response method in OnionMessenger provides a mechanism to handle responses to onion messages. It's crucial that this method correctly finds the path for the reply and handles any errors that may occur during this process, such as path not found or issues with the reply path. Consider adding more detailed error handling and logging to improve debuggability and resilience.
| @@ -250,19 +250,19 @@ pub struct Responder<'a, OMH: OnionMessageHandler, T: OnionMessageContents> { | |||
| pub path_id: Option<[u8; 32]>, | |||
| // This phantom Data is used to ensure that we use T in the struct definition | |||
| // This allow us to ensure at compile time that the received message type and response type will be same | |||
| _phantom: std::marker::PhantomData<T> | |||
| _phantom: core::marker::PhantomData<T> | |||
| } | |||
|
|
|||
| impl<'a, OMH: OnionMessageHandler, T: OnionMessageContents> Responder<'a, OMH, T> { | |||
| pub fn respond(&self, response: T) { | |||
There was a problem hiding this comment.
Need a tab here and the closing parenthesis, too.
There was a problem hiding this comment.
I think this suggestion belongs to an earlier version of PR, so skipped this one for now.
Let me know if it needs addressing in the latest version too, and I would love to incorporate it!
| R: RespondFunction<T>, | ||
| T: OnionMessageContents | ||
| { | ||
| fn new(messenger_function: R, message: T, reply_path_option: Option<BlindedPath>, path_id: Option<[u8; 32]> ) -> Self { |
There was a problem hiding this comment.
No need to use option in the name here, IMO.
There was a problem hiding this comment.
I get your point!
I still think having a slightly different name could be handy, especially since we're using reply_path: in the function.
I've simplified the name a bit to make it less wordy but still meaningful.
Let me know if we want to simplify it further, and I would love to make the change!
| let closure = |response, reply_path: BlindedPath| { | ||
| self.handle_onion_message_response( | ||
| response, reply_path, format_args!( | ||
| "when responding to {} onion message with path_id {:02x?}", | ||
| message_type, | ||
| path_id | ||
| ) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
I think we can DRY this up by moving the closure definition outside the match.
There was a problem hiding this comment.
I gave this a shot, but since the type of response depends on the type of message, creating and using the same function for both handle_message and handle_custom_message, became tricky.
So I have hit a roadblock on this idea for now!
Let me know if there's some simpler way to achieve DRY code here, and I would love to make the change!
Thanks!
There was a problem hiding this comment.
Good point! We'll have to define the closure using a local macro definition instead.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- fuzz/src/onion_message.rs (2 hunks)
- lightning/src/ln/channelmanager.rs (3 hunks)
- lightning/src/ln/msgs.rs (1 hunks)
- lightning/src/ln/peer_handler.rs (5 hunks)
- lightning/src/onion_message/functional_tests.rs (5 hunks)
- lightning/src/onion_message/messenger.rs (5 hunks)
- lightning/src/onion_message/offers.rs (4 hunks)
Files skipped from review as they are similar to previous changes (5)
- fuzz/src/onion_message.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/onion_message/functional_tests.rs
- lightning/src/onion_message/offers.rs
Additional comments: 7
lightning/src/onion_message/messenger.rs (5)
- 247-288: The introduction of the
ReceivedOnionMessageenum with variantsWithReplyPathandWithoutReplyPathis a good design choice for handling received onion messages based on whether they include a reply path. This design enhances the clarity and maintainability of the code by clearly distinguishing between these two cases. However, ensure that thepath_idfield, which is optional in both variants, aligns with the expected behavior in all use cases where this enum is utilized.- 290-302: The
RespondFunctiontrait provides a clean and straightforward interface for responding to onion messages. By defining a single methodrespondthat accepts the response message and aBlindedPath, it allows for flexible implementation of response handling logic. This design supports the overall goal of enhancing onion message handling capabilities.- 304-325: The
Responderstruct is well-designed, encapsulating the necessary information and functionality to respond to an onion message. The use ofPhantomDatato enforce type safety between the received message and the response message is a good practice, ensuring that the types align correctly at compile time. This design contributes to the robustness and maintainability of the onion message handling system.- 913-917: The modification to the
handle_onion_message_responsemethod to utilize theReceivedOnionMessageenum and theResponderstruct aligns with the PR's objectives of enhancing onion message handling. This change streamlines the response handling process, making it more efficient and maintainable. Ensure that all calls to this method are updated accordingly to pass the correct parameters.- 995-1021: The modifications to the
handle_onion_messagemethod to differentiate between offers and custom messages and to utilize theReceivedOnionMessageenum for processing are well-implemented. The use of closures to encapsulate the response handling logic for each message type is a clever design choice that enhances the modularity and readability of the code. However, ensure that the closures correctly capture all necessary context and that thehandle_messageandhandle_custom_messagemethods of the respective handlers are prepared to handle these closures as expected.lightning/src/ln/msgs.rs (2)
- 35-35: The import of
BlindedPathis added.- 40-40: The import of
OnionMessageContentsis added.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- fuzz/src/onion_message.rs (2 hunks)
- lightning/src/ln/channelmanager.rs (3 hunks)
- lightning/src/ln/peer_handler.rs (4 hunks)
- lightning/src/onion_message/functional_tests.rs (5 hunks)
- lightning/src/onion_message/messenger.rs (5 hunks)
- lightning/src/onion_message/offers.rs (3 hunks)
Files skipped from review as they are similar to previous changes (5)
- fuzz/src/onion_message.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/onion_message/functional_tests.rs
- lightning/src/onion_message/offers.rs
Additional comments: 4
lightning/src/onion_message/messenger.rs (4)
- 247-262: The introduction of the
ReceivedOnionMessageenum withWithReplyPathandWithoutReplyPathvariants is a well-structured approach to handle receivedOnionMessagewith flexibility for messages with or without a reply path. The use of generics and optionalpath_idenhances its utility in various contexts.- 290-302: The
RespondFunctiontrait provides a clear and flexible contract for handling responses to received onion messages. Its design promotes a clean separation of concerns and integrates well with theReceivedOnionMessageenum andResponderstruct.- 304-325: The
Responderstruct is well-designed, encapsulating the necessary components for responding to messages with a clean integration with theRespondFunctiontrait. The use ofPhantomDatais appropriate, and therespondmethod's implementation aligns with the intended functionality.- 913-917: The
handle_onion_message_responsemethod is logically structured to handle responses to received onion messages by enqueuing them for sending. Its effectiveness depends on thefind_path_and_enqueue_onion_messagemethod's implementation, which is not visible in the provided code segment.
lightning/src/ln/channelmanager.rs
Outdated
| impl<M: Deref, T: Deref, ES: Deref, NS: Deref, SP: Deref, F: Deref, R: Deref, L: Deref> | ||
| OffersMessageHandler for ChannelManager<M, T, ES, NS, SP, F, R, L> | ||
| impl<M: Deref, BI: Deref, ES: Deref, NS: Deref, SP: Deref, FE: Deref, R: Deref, L: Deref> | ||
| OffersMessageHandler for ChannelManager<M, BI, ES, NS, SP, FE, R, L> | ||
| where | ||
| M::Target: chain::Watch<<SP::Target as SignerProvider>::EcdsaSigner>, | ||
| T::Target: BroadcasterInterface, | ||
| BI::Target: BroadcasterInterface, | ||
| ES::Target: EntropySource, | ||
| NS::Target: NodeSigner, | ||
| SP::Target: SignerProvider, | ||
| F::Target: FeeEstimator, | ||
| FE::Target: FeeEstimator, | ||
| R::Target: Router, | ||
| L::Target: Logger, |
There was a problem hiding this comment.
Let's keep these as is for consistency with the other impl blocks.
lightning/src/ln/channelmanager.rs
Outdated
| return Some(OffersMessage::InvoiceError(error.into())); | ||
| }, | ||
| }; | ||
| if let ReceivedOnionMessage::WithReplyPath{responder, message, path_id: _} = message { |
There was a problem hiding this comment.
Given that we currently don't send a reply path with a Bolt12Invoice, we'll still want to pay one even if there isn't a reply path. So we should move this outside the match.
| WithReplyPath { | ||
| message: T, | ||
| path_id: Option<[u8; 32]>, | ||
| responder: Responder<R, T>, | ||
| }, | ||
| WithoutReplyPath { | ||
| message: T, | ||
| path_id: Option<[u8; 32]>, | ||
| }, |
There was a problem hiding this comment.
Variants will need docs, too.
| pub trait RespondFunction<T: OnionMessageContents>{ | ||
| fn respond(self, response: T, reply_path: BlindedPath); | ||
| } |
| } | ||
| } | ||
|
|
||
| pub trait RespondFunction<T: OnionMessageContents>{ |
There was a problem hiding this comment.
To reduce verbosity, I wonder if we should standardize on naming such traits as RespondFn.
| impl<R, T> ReceivedOnionMessage<R, T> | ||
| where | ||
| R: RespondFunction<T>, | ||
| T: OnionMessageContents |
There was a problem hiding this comment.
Could we switch the order of the type parameters? Seems like T is more important.
| let closure = |response, reply_path: BlindedPath| { | ||
| self.handle_onion_message_response( | ||
| response, reply_path, format_args!( | ||
| "when responding to {} onion message with path_id {:02x?}", | ||
| message_type, | ||
| path_id | ||
| ) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Good point! We'll have to define the closure using a local macro definition instead.
| WithReplyPath { | ||
| message: T, | ||
| path_id: Option<[u8; 32]>, | ||
| responder: Responder<R, T>, |
There was a problem hiding this comment.
Rather than passing in a trait/lambda here, can we have a method on the enum that returns an OnionMessageReply struct that holds the message and BlindedPath? It seems like that would get us to the same place, but would avoid the trait indirection and would allow us to have a nice OnionMessageReply struct that the user can async pass back to the OnionMessenger via a new dedicated API as well.
There was a problem hiding this comment.
Hmmm... it's not really a reply but rather a message that can be replied to. Regardless of naming, I'm not sure I see how such an interface would be any better or work for that matter. The handler would need a reference to OnionMessenger, which would result in a circular reference since OnionMessenger already has a reference to the handler. Am I misunderstanding what you're proposing?
There was a problem hiding this comment.
@shaavan Discussed further offline with @TheBlueMatt. Seems we'll run into the circular reference and some other lifetime issues in the async case. We'd soon see this when writing a test where the handler tries to store the ReceivedOnionMessage for later processing.
So we'll have to drop the ResponseFunction unfortunately. Instead, we'll need to do something like the following:
- Have
Responder::respondreturn some sort ofResponseInstructions<T>(exact name TBD... maybeOnionMessageResponseinstead?) which would include the response message and (in a follow-up) a way to indicated that a reply path should be built to include with the response - Change the handlers back to have a return value only this time using type
ResponseInstructions<T>whereTis the concrete message type for the handler. - Instead of using an
Optionin the handler's return type you could possibly makeResponseInstructionsan enum with aNoResponsevariant, FWIW. - When OnionMessenger gets the returned
ResponseInstructionsit would respond appropriately.
That way a handler can take ownership of ReceivedOnionMessage and produce a ResponseInstructions asynchronously. In another follow-up PR we would add a method to OnionMessenger that takes a ResponseInstructions and responds just as it does in the sync case.
|
Updated from pr2907.05 to pr2907.06 (diff): Changes:
|
|
Updated from pr2907.07 to pr2907.08 (diff):
|
|
Updated from pr2907.08 to pr2907.09 (diff): Updates:
|
shaavan
changed the title
lightning/src/ln/channelmanager.rs
Outdated
| let responder = match responder { | ||
| Some(responder) => responder, | ||
| None => return ResponseInstruction::NoResponse, | ||
| }; |
There was a problem hiding this comment.
This needs to go inside the OffersMessage::InvoiceRequest arm. Otherwise, we'll never reach the OffersMessage::Invoice arm for invoices sent by LDK since we currently don't have a way to include a reply path. Thus, we'll never pay the invoice.
This is only a problem for the offer flow. For the refund flow, we do so in a different manner:
rust-lightning/lightning/src/ln/channelmanager.rs
Lines 7993 to 7998 in b8b1ef3
There was a problem hiding this comment.
That makes sense!
I tinkered with your suggestion and I realized I needed to clone the responder to allow it to be used later down the code.
So to avoid this, I have experimented with using a closure approach in 51730b4
This solves the problem of not correctly going through the flow, while also avoiding cloning the responder down the line.
Let me know if the approach seems sound!
Thanks a lot, for the suggestions and pointers!
There was a problem hiding this comment.
Right, good point! Maybe it would be better to explicitly respond inline throughout instead of having a catch-all at the end of the handler. We already are doing that for early returns. Then you don't need the closure and can use the above code.
For the OffersMessage::Invoice arm, you'll need to store the response in variable like we do at the end of the OffersMessage::InvoiceRequest arm instead of having the match evaluate to the response. Both arms would respond there.
There was a problem hiding this comment.
Implemented explicit return inline in 5b05294
I needed to move
let responder = match responder {
Some(responder) => responder,
None => return ResponseInstruction::NoResponse,
};
under the InvoiceRequest arm, as you mentioned in your earlier comment. So I have match operations to return inline of Invoice branch too. Let me know if you have any suggestions on how I can improve the code further!
Thanks!
|
Updated from pr2907.22 to pr2907.23 (diff): Updates:
|
|
Okay, looks like this needs a minor rebase and then otherwise is ready to go. Sadly it needs to wait on the next release, however. |
No problemo! :) Maybe we might also get the follow-up (#2996) in before the next release! |
|
Lets wait to land this until after #3017, which will probably imply a rebase. |
- Currently, handle_message (or handle_custom_message) only exposes the generated response for an OnionMessage, lacking the necessary reply_path for asynchronous responses. - This commit introduces a new flow for OnionMessage handling. - Instead of solely taking the message as input, handle_message now accepts an Optional `responder`, returning a `ResponseInstruction` containing both the response and the reply_path. - `ResponseInstruction` utilizes different enum variants to indicate how `handle_onion_message_response` should handle the response. - This enhancement enables exposing the reply_path alongside the response and allows for more complex response mechanisms, such as responding with an added reply_path. - The commit introduces the foundational framework (structs & enums) for this new flow.
|
Updated from pr2907.24 to pr2907.25 (diff): Changes:
@TheBlueMatt, a gentle ping! :) |
| @@ -135,6 +135,9 @@ impl<T: OnionMessageContents> OnionMessageContents for ParsedOnionMessageContent | |||
| &ParsedOnionMessageContents::Custom(ref msg) => msg.tlv_type(), | |||
| } | |||
| } | |||
| fn msg_type() -> &'static str { | |||
There was a problem hiding this comment.
This really shouldn't be static (ie should take &self) - we intend custom messages to be an enum across multiple message types and this doesn't really allow it.
There was a problem hiding this comment.
Hmm... this was requested to avoid having users pass a string to handle_onion_message_response in the follow-up. See #2907 (comment). We really only care about the class of message we are responding to. In handle_onion_message_response, we have the response (not the original request), so calling this on the response message passed in wouldn't give a correct logging. That is, for example, we currently log "when responding to Offers onion message" rather than "when responding to InvoiceRequest onion message". And by making this take &self it would log "when responding to Invoice onion message", which is not correct.
There was a problem hiding this comment.
This assumes the OffersMessage implementation changed. Though for custom messages, I can see how a handler may implement more than one class of messages. So I'm not sure how to best handle this. Either in the async case the user passes in the request message's string or we rephrase the logging maybe?
There was a problem hiding this comment.
I guess we could hold the request string in Responder and move it to the ResponseInstructions.That way the user doesn't need to pass it. @shaavan Let's handle this in the follow-up PR.
There was a problem hiding this comment.
Hmm? handle_onion_message_response has a specific message its considering, it doesn't need to access the message type via the generic T but can simply say response.msg_type() and log that.
There was a problem hiding this comment.
As mentioned above, the log is stating which message the response is in response to, not the response itself.
There was a problem hiding this comment.
Ah, apologies, I misunderstood that for some reason, but I don't really see why we can't just change the log to say "when sending a responding X"?
There was a problem hiding this comment.
The rest of the message says "with path_id {:02x?}", which refers to the message being respond to. Anyhow, you're right that we should change it to &self. Doing so would allow implementations to return the specific message type.
There was a problem hiding this comment.
I think the suggestion sounds reasonable!
Reverted the msg_type function to take self as a reference in 32a8615
Thanks for the pointers!
|
Updated from pr2907.25 to pr2907.26 (diff): Updates:
|
|
Updated from pr2907.26 to pr2907.27 (diff): Updates:
|
This commit integrates the newly introduced ResponseInstruction structs and enums into the codebase for handling OnionMessage responses.
resolves #2882
Introduce ResponseInstructions for OnionMessage Handling
responder, returning aResponseInstructioncontaining both the response and the reply_path.ResponseInstructionutilizes different enum variants to indicate howhandle_onion_message_responseshould handle the response.