Small Offers Fixes #2881
Small Offers Fixes #2881
Conversation
WalkthroughThe recent updates bring improvements to both the channel manager and onion message handling. The channel manager now utilizes Changes
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: 5
Configuration used: CodeRabbit UI
Files selected for processing (2)
- lightning/src/ln/channelmanager.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (3 hunks)
Additional comments: 3
lightning/src/onion_message/messenger.rs (1)
- 756-756: Verify the logic for incrementing
ticksintimer_tick_occurredto ensure it aligns with the intended behavior for handling pending connections.lightning/src/ln/channelmanager.rs (2)
- 7775-7776: The introduction of
_persistence_guardusingPersistenceNotifierGuard::notify_on_drop(self)is correctly implemented to ensure that persistence notifications are triggered upon dropping. This change enhances the reliability of outbound payment handling by ensuring that necessary notifications are not missed.- 7838-7839: Similarly, the addition of
_persistence_guardin the context of inbound payments is correctly implemented. By usingPersistenceNotifierGuard::notify_on_drop(self), this change ensures that persistence notifications are triggered upon dropping, enhancing the reliability of inbound payment handling.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2881 +/- ##
==========================================
- Coverage 89.23% 89.19% -0.04%
==========================================
Files 117 117
Lines 95496 95513 +17
Branches 95496 95513 +17
==========================================
- Hits 85216 85195 -21
- Misses 7801 7835 +34
- Partials 2479 2483 +4 ☔ View full report in Codecov by Sentry. |
TheBlueMatt
force-pushed
the
2024-02-offers-tweak
branch
2 times, most recently
from
c3b5188
to
b047c4c
Compare
|
Went ahead and squash-pushed because this is super trivial: $ git diff-tree -U1 37d6c233 b047c4c88
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index ffe518fbf..eb3e1bb3a 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -7684,2 +7684,4 @@ where
+ let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
+
let expiration = StaleExpiration::AbsoluteTimeout(absolute_expiry);
diff --git a/lightning/src/onion_message/messenger.rs b/lightning/src/onion_message/messenger.rs
index fe0761ff9..7d4dca375 100644
--- a/lightning/src/onion_message/messenger.rs
+++ b/lightning/src/onion_message/messenger.rs
@@ -916,2 +916,3 @@ where
fn handle_onion_message(&self, peer_node_id: &PublicKey, msg: &OnionMessage) {
+ let logger = WithContext::from(&self.logger, Some(*peer_node_id), None);
match self.peel_onion_message(msg) {
@@ -919,3 +920,3 @@ where
log_trace!(
- WithContext::from(&self.logger, Some(*peer_node_id), None),
+ logger,
"Received an onion message with path_id {:02x?} and {} reply_path: {:?}",
@@ -948,3 +949,3 @@ where
log_trace!(
- WithContext::from(&self.logger, Some(*peer_node_id), None),
+ logger,
"Dropping forwarded onion message to peer {}: outbound buffer full",
@@ -964,6 +965,3 @@ where
e.get_mut().enqueue_message(onion_message);
- log_trace!(
- WithContext::from(&self.logger, Some(*peer_node_id), None),
- "Forwarding an onion message to peer {}",
- next_node_id);
+ log_trace!(logger, "Forwarding an onion message to peer {}", next_node_id);
},
@@ -971,3 +969,3 @@ where
log_trace!(
- WithContext::from(&self.logger, Some(*peer_node_id), None),
+ logger,
"Dropping forwarded onion message to disconnected peer {}",
@@ -979,6 +977,3 @@ where
Err(e) => {
- log_error!(
- WithContext::from(&self.logger, Some(*peer_node_id), None),
- "Failed to process onion message {:?}",
- e);
+ log_error!(logger, "Failed to process onion message {:?}", e);
} |
| @@ -7833,6 +7837,8 @@ where | |||
| let amount_msats = refund.amount_msats(); | |||
| let relative_expiry = DEFAULT_RELATIVE_EXPIRY.as_secs() as u32; | |||
|
|
|||
| let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); | |||
There was a problem hiding this comment.
Perhaps this just needs more clarification in the commit message, but why isn't this required in the OffersMessageHandler implementation? I might just not have a full grasp of the issue.
There was a problem hiding this comment.
Hmm, I think we're okay. There's two cases where we care about the persistence-guard - (a) when we changed the ChannelManager and we need to write a fresh copy to disk, (b) when we have a new message that we need to send a peer, but only if its not a response (in which case the socket handler will prod the PeerManager for us).
There was a problem hiding this comment.
Somewhat related: I was chatting with @tnull about adding an InvoiceSent event to help with LDK Node managing inbound payments. We create the invoice in the OffersMessageHandler implementation, but we don't send it until it goes through the OnionMessenger. By the time the PeerManager sends it, the invoice is no longer accessible as it's inside the OnionMessage. Wondering what the best strategy would be for generating such an event?
- Generate it in
ChannelManagerbefore it is actually sent (thus needing persistence here) - Generate it in
OnionMessenger, which we avoided forConnectionNeeded(but event not persisted then) - Generate in
PeerManagerby pairing theOnionMessagewith the (cloned)Bolt12Invoicepayload (same, not persisted)
Seems like the options aren't that great. Any other alternatives?
There was a problem hiding this comment.
Generate it in ChannelManager before it is actually sent (thus needing persistence here)
I think probably this? We can wake without persistence now, which IIRC will also see events.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- lightning/src/ln/channelmanager.rs (3 hunks)
- lightning/src/onion_message/messenger.rs (6 hunks)
Additional comments: 12
lightning/src/onion_message/messenger.rs (9)
- 738-738: The introduction of
intro_node_loggerusingWithContext::from(&self.logger, Some(path.first_node()), None);is a significant improvement for contextual logging. This change allows for more detailed and context-aware logging, especially useful in a complex system like LDK where operations can span multiple nodes and paths. This enhancement aligns with best practices for logging in distributed systems, providing clearer insights into the flow of onion messages through the network.- 753-753: Using the
intro_node_loggerfor logging buffered onion messages enhances the clarity of log messages by including the introduction node's context. This is a valuable addition for debugging and monitoring the onion message routing process.- 757-758: Similarly, logging buffered onion messages awaiting peer connection with the introduction node's context is a thoughtful improvement. It provides clear visibility into the state of onion messages that are pending due to connection issues, which is crucial for diagnosing and resolving routing delays or failures.
- 917-918: The instantiation of
loggerwithWithContext::from(&self.logger, Some(*peer_node_id), None);for logging within thehandle_onion_messagefunction is another excellent use of contextual logging. It ensures that log messages related to handling onion messages are associated with the peer node ID, improving the traceability of message handling events.- 922-923: The detailed logging for received onion messages, including path ID and reply path information, is beneficial for understanding the flow and processing of onion messages. This level of detail in logs aids in debugging and monitoring the handling of different types of onion messages.
- 950-953: Logging the dropping of forwarded onion messages due to a full outbound buffer with the peer node ID context is crucial for identifying potential bottlenecks or issues in message forwarding. This log statement helps in pinpointing problems related to message congestion or capacity limits.
- 970-973: Similarly, logging the dropping of forwarded onion messages to disconnected peers with the peer node ID context is important for diagnosing routing issues. It highlights scenarios where messages cannot be forwarded due to connectivity problems, aiding in the identification and resolution of network connectivity issues.
- 979-979: The use of
log_error!for logging failures in processing onion messages is appropriate. It ensures that such critical errors are prominently logged, facilitating quick identification and troubleshooting of issues in onion message processing.- 31-31: The import of
WithContextfromcrate::util::loggeris crucial for the logging improvements introduced in this PR. It ensures that the new contextual logging capabilities are available for use throughout the file. This change is a foundational part of enhancing logging detail and clarity in the handling of onion messages.lightning/src/ln/channelmanager.rs (3)
- 7685-7686: The introduction of
_persistence_guardusingPersistenceNotifierGuard::notify_on_drop(self)ensures that persistence notifications are managed upon dropping objects. This is a good practice for enhancing reliability in payment processes. However, it's crucial to ensure that thePersistenceNotifierGuardis tested thoroughly to handle all edge cases, especially in asynchronous environments where race conditions might occur.
Ensure that the implementation of
PersistenceNotifierGuard::notify_on_drop(self)is robust against potential race conditions by reviewing its implementation and adding necessary tests if they are missing.
- 7777-7778: Adding
_persistence_guardin the context of handling invoice requests is appropriate for ensuring that persistence notifications are triggered upon object drops. This aligns with the PR's objective to enhance the reliability of payment processing. However, based on the previous comment byjkczyz, it's important to consider whether similar mechanisms are needed in other areas, such ascreate_refund_builder, to maintain consistency and reliability across the codebase.
Verification successful
The verification process revealed multiple instances of
PersistenceNotifierGuard::notify_on_dropbeing used across various functions withinchannelmanager.rs, indicating a consistent implementation of the persistence guard mechanism to enhance reliability in payment processing and channel management. However, there was no specific mention ofcreate_refund_builderusing this mechanism, which could imply that either the function does not exist or does not employ the persistence guard in the same manner. Given the widespread and consistent use of this mechanism in other areas, the implementation strategy aligns with the PR's objective, even ifcreate_refund_builderspecifically does not use or does not exist with this mechanism.
* 7840-7841: The addition of `_persistence_guard` here is consistent with the PR's goal to improve persistence notification handling. The discussion between `jkczyz` and `TheBlueMatt` raises an important point about the necessity of such mechanisms in different contexts, specifically in the `OffersMessageHandler` implementation. It's crucial to ensure that the use of `_persistence_guard` is consistent and justified across different parts of the code, considering the scenarios outlined by `TheBlueMatt`.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if similar persistence guard mechanisms are implemented in `create_refund_builder` and other relevant areas. grep -C3 "PersistenceNotifierGuard::notify_on_drop" lightning/src/ln/channelmanager.rsLength of output: 13647
|
Seems CI is unhappy |
TheBlueMatt
force-pushed
the
2024-02-offers-tweak
branch
from
61dfc61
to
8646a8c
Compare
|
Sorry, fixed. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- lightning/src/ln/channelmanager.rs (3 hunks)
- lightning/src/onion_message/messenger.rs (6 hunks)
Files skipped from review as they are similar to previous changes (2)
- lightning/src/ln/channelmanager.rs
- lightning/src/onion_message/messenger.rs
| let intro_node_logger = | ||
| WithContext::from(&self.logger, Some(destination.first_node()), None); |
There was a problem hiding this comment.
Hmmm... it's not clear to me we want to use destination.first_node(). With DefaultMessageRouter it will either be our peer or soon-to-be peer, which I believe is what we want. But a different MessageRouter may return a partial path back from destination, in which case the peer (or soon-to-be peer) in question would not be the introduction node.
There was a problem hiding this comment.
Oops, duh, sorry.
The `Debug` serialization of `PublicKey`s includes both the X and Y coordinate, which isn't something most of our users deal with. Instead, logging using `Display` gives users the keys they're used to.
TheBlueMatt
force-pushed
the
2024-02-offers-tweak
branch
from
4cc2065
to
9a5a04e
Compare
|
Rebased and squashed after addressing the last comment. |
Will need to update now to use |
This resolves an issue where offer and refund payments get delayed while we wait for the `invoice_request`/`invoice` onion messages to get sent. It further ensures we're likely to have the `ChannelManager` persisted with the new payment info after initiating the send/receive.
TheBlueMatt
force-pushed
the
2024-02-offers-tweak
branch
from
9a5a04e
to
29984a7
Compare
|
Oops, apologies, fixed now. |
| @@ -7937,6 +7941,8 @@ where | |||
| return Err(Bolt12SemanticError::UnsupportedChain); | |||
| } | |||
|
|
|||
| let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); | |||
There was a problem hiding this comment.
Might be a bit better to put this after all the potential error paths/right before we push into self.pending_offers_messages, but that's a very minor optimization.
There was a problem hiding this comment.
Yea, not sure it matters much and it reads simpler here, less chance we add something and forget to move the persistence lock.
Went to go try to pay an offer with ldk-sample (lightningdevkit/ldk-sample#129) and found some minor things worth fixing. Sadly, it seems CLN can't respond to my invoice_request, not sure why but I imagine its not doing response path introduction_point direct connecting, which is causing it to fail.