Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small Offers Fixes #2881

Merged
merged 3 commits into from
Mar 13, 2024
Merged

Small Offers Fixes #2881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
98340dc
Use `{}`, not `{:?}` for `PublicKey` logging
TheBlueMatt Feb 6, 2024
b290336
Add `PersistenceNotifierGuard` take to offer/refund payments
TheBlueMatt Feb 6, 2024
29984a7
Use structured logging where appropriate in `OnionMessenger`
TheBlueMatt Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7755,6 +7755,8 @@ macro_rules! create_refund_builder { ($self: ident, $builder: ty) => {
.absolute_expiry(absolute_expiry)
.path(path);

let _persistence_guard = PersistenceNotifierGuard::notify_on_drop($self);

let expiration = StaleExpiration::AbsoluteTimeout(absolute_expiry);
$self.pending_outbound_payments
.add_new_awaiting_invoice(
Expand Down Expand Up @@ -7870,6 +7872,8 @@ where
let invoice_request = builder.build_and_sign()?;
let reply_path = self.create_blinded_path().map_err(|_| Bolt12SemanticError::MissingPaths)?;

let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
TheBlueMatt marked this conversation as resolved.
Show resolved Hide resolved

let expiration = StaleExpiration::TimerTicks(1);
self.pending_outbound_payments
.add_new_awaiting_invoice(
Expand Down Expand Up @@ -7937,6 +7941,8 @@ where
return Err(Bolt12SemanticError::UnsupportedChain);
}

let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

@jkczyz jkczyz Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

  1. Generate it in ChannelManager before it is actually sent (thus needing persistence here)
  2. Generate it in OnionMessenger, which we avoided for ConnectionNeeded (but event not persisted then)
  3. Generate in PeerManager by pairing the OnionMessage with the (cloned) Bolt12Invoice payload (same, not persisted)

Seems like the options aren't that great. Any other alternatives?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, not sure it matters much and it reads simpler here, less chance we add something and forget to move the persistence lock.


match self.create_inbound_payment(Some(amount_msats), relative_expiry, None) {
Ok((payment_hash, payment_secret)) => {
let payment_paths = self.create_blinded_payment_paths(amount_msats, payment_secret)
Expand Down
41 changes: 27 additions & 14 deletions lightning/src/onion_message/messenger.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use super::packet::OnionMessageContents;
use super::packet::ParsedOnionMessageContents;
use super::offers::OffersMessageHandler;
use super::packet::{BIG_PACKET_HOP_DATA_LEN, ForwardControlTlvs, Packet, Payload, ReceiveControlTlvs, SMALL_PACKET_HOP_DATA_LEN};
use crate::util::logger::Logger;
use crate::util::logger::{Logger, WithContext};
use crate::util::ser::Writeable;

use core::fmt;
Expand Down Expand Up @@ -748,25 +748,31 @@ where
&self, contents: T, destination: Destination, reply_path: Option<BlindedPath>,
log_suffix: fmt::Arguments
) -> Result<SendSuccess, SendError> {
let mut logger = WithContext::from(&self.logger, None, None);
let result = self.find_path(destination)
.and_then(|path| self.enqueue_onion_message(path, contents, reply_path, log_suffix));
.and_then(|path| {
let first_hop = path.intermediate_nodes.get(0).map(|p| *p);
logger = WithContext::from(&self.logger, first_hop, None);
self.enqueue_onion_message(path, contents, reply_path, log_suffix)
});

match result.as_ref() {
Err(SendError::GetNodeIdFailed) => {
log_warn!(self.logger, "Unable to retrieve node id {}", log_suffix);
log_warn!(logger, "Unable to retrieve node id {}", log_suffix);
},
Err(SendError::PathNotFound) => {
log_trace!(self.logger, "Failed to find path {}", log_suffix);
log_trace!(logger, "Failed to find path {}", log_suffix);
},
Err(e) => {
log_trace!(self.logger, "Failed sending onion message {}: {:?}", log_suffix, e);
log_trace!(logger, "Failed sending onion message {}: {:?}", log_suffix, e);
},
Ok(SendSuccess::Buffered) => {
log_trace!(self.logger, "Buffered onion message {}", log_suffix);
log_trace!(logger, "Buffered onion message {}", log_suffix);
},
Ok(SendSuccess::BufferedAwaitingConnection(node_id)) => {
log_trace!(
self.logger, "Buffered onion message waiting on peer connection {}: {:?}",
logger,
"Buffered onion message waiting on peer connection {}: {}",
log_suffix, node_id
);
},
Expand Down Expand Up @@ -925,12 +931,13 @@ where
OMH::Target: OffersMessageHandler,
CMH::Target: CustomOnionMessageHandler,
{
fn handle_onion_message(&self, _peer_node_id: &PublicKey, msg: &OnionMessage) {
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) {
Ok(PeeledOnion::Receive(message, path_id, reply_path)) => {
log_trace!(
self.logger,
"Received an onion message with path_id {:02x?} and {} reply_path: {:?}",
logger,
"Received an onion message with path_id {:02x?} and {} reply_path: {:?}",
path_id, if reply_path.is_some() { "a" } else { "no" }, message);

match message {
Expand All @@ -957,7 +964,10 @@ where
Ok(PeeledOnion::Forward(next_node_id, onion_message)) => {
let mut message_recipients = self.message_recipients.lock().unwrap();
if outbound_buffer_full(&next_node_id, &message_recipients) {
log_trace!(self.logger, "Dropping forwarded onion message to peer {:?}: outbound buffer full", next_node_id);
log_trace!(
logger,
"Dropping forwarded onion message to peer {}: outbound buffer full",
next_node_id);
return
}

Expand All @@ -971,16 +981,19 @@ where
e.get(), OnionMessageRecipient::ConnectedPeer(..)
) => {
e.get_mut().enqueue_message(onion_message);
log_trace!(self.logger, "Forwarding an onion message to peer {}", next_node_id);
log_trace!(logger, "Forwarding an onion message to peer {}", next_node_id);
},
_ => {
log_trace!(self.logger, "Dropping forwarded onion message to disconnected peer {:?}", next_node_id);
log_trace!(
logger,
"Dropping forwarded onion message to disconnected peer {}",
next_node_id);
return
},
}
},
Err(e) => {
log_error!(self.logger, "Failed to process onion message {:?}", e);
log_error!(logger, "Failed to process onion message {:?}", e);
}
}
}
Expand Down
Loading