Skip to content

Small Offers Fixes #2881

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

Merged
merged 3 commits into from
Mar 13, 2024
Merged

Small Offers Fixes #2881

Changes from 1 commit
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);

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