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.

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

Introduce MessageContext and use it to allow abandon failed payments #3085

Merged
merged 4 commits into from
Jul 9, 2024

Conversation

shaavan
Copy link
Member

@shaavan shaavan commented May 28, 2024

Resolves #2837

Resolves #3124

Summary

This PR introduces a new field, MessageContext, in the ReceiveTlvs struct. The key changes are as follows:

  1. Introduction of MessageContext:
    • This struct allows users to append additional data to the created reply_path, which they expect to receive back from the counterparty along with their response.
  2. Introduction of OffersContext in MessageContext:
    • OffersContext enum introduces a variant containing the Outbound Payment's PaymentId field that can be utilized by OffersMessage.
    • This feature enables users to abandon outbound payments that fail for any reason.
  3. Integration with create_blinded_path flow:
    • The PR integrates MessageContext into the create_blinded_path process.
    • This enhancement is utilized in the pay_for_offer and create_refund_builder functions to pass PaymentId along with the generated reply_path and path respectively.

These improvements aim to allow passing additional information with the create reply_path and managing outbound payment failures more robustly.

Note:
This PR builds on #2996

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 83.75000% with 26 lines in your changes missing coverage. Please review.

Project coverage is 90.27%. Comparing base (40283e7) to head (42c096c).
Report is 6 commits behind head on main.

Files Patch % Lines
lightning/src/onion_message/messenger.rs 75.51% 12 Missing ⚠️
lightning/src/util/test_utils.rs 50.00% 6 Missing ⚠️
lightning/src/ln/channelmanager.rs 90.90% 3 Missing ⚠️
lightning/src/ln/peer_handler.rs 0.00% 2 Missing ⚠️
lightning/src/ln/max_payment_path_len_tests.rs 85.71% 1 Missing ⚠️
lightning/src/ln/offers_tests.rs 75.00% 1 Missing ⚠️
lightning/src/onion_message/functional_tests.rs 97.43% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3085      +/-   ##
==========================================
+ Coverage   89.80%   90.27%   +0.47%     
==========================================
  Files         121      121              
  Lines       99532   102381    +2849     
  Branches    99532   102381    +2849     
==========================================
+ Hits        89382    92429    +3047     
+ Misses       7498     7305     -193     
+ Partials     2652     2647       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tnull
Copy link
Contributor

tnull commented May 29, 2024

Unfortunately this seems to need a rebase already.

@shaavan
Copy link
Member Author

shaavan commented May 29, 2024

Updated from pr3085.01 to pr3085.02 (diff):
Addressed @tnull comment

Changes:

  1. Rebase on main to resolve merge conflicts.

@shaavan
Copy link
Member Author

shaavan commented May 30, 2024

Updated from pr3085.02 to pr3085.03 (diff):

Changes:

  1. Fix Fuzz failure

@jkczyz jkczyz self-requested a review May 30, 2024 15:14
@shaavan
Copy link
Member Author

shaavan commented May 31, 2024

There’s another variation of this PR that might help us achieve our goal using enums. You can check it out here: link to branch (check out the top three commits).

This approach will help keep the RecipientData for Offers Messages and Custom Messages separate, making the use of RecipientData more focused and intentional.

Let me know which of the two approaches seems better.

@jkczyz
Copy link
Contributor

jkczyz commented May 31, 2024

There’s another variation of this PR that might help us achieve our goal using enums. You can check it out here: link to branch (check out the top three commits).

This approach will help keep the RecipientData for Offers Messages and Custom Messages separate, making the use of RecipientData more focused and intentional.

Let me know which of the two approaches seems better.

@valentinewallace @TheBlueMatt Would you mind checking out the two alternatives for including RecipientData in blinded paths? This PR uses a single struct which is passed to each message handler. The alternative approach in the linked branch uses an enum such that there is a different variant for each handler. Each variant wraps another type, which is passed to the handler instead of RecipientData.

@TheBlueMatt
Copy link
Collaborator

Hmm, yea, we need to figure out what we want to do with ReceiveTlvs as a general matter. We currently don't use it much, and hid the path_id from LDK devs because we weren't sure if/how we wanted to expose it to users to set their own. I guess if we took ReceiveTlvs and defined some "LDK internal range" of types we could use an enum (or private struct) for those and then have a custom_other_tlvs field that we make pub? Do we currently use the path_id at all internally to LDK?

@jkczyz
Copy link
Contributor

jkczyz commented Jun 3, 2024

Hmm, yea, we need to figure out what we want to do with ReceiveTlvs as a general matter. We currently don't use it much, and hid the path_id from LDK devs because we weren't sure if/how we wanted to expose it to users to set their own. I guess if we took ReceiveTlvs and defined some "LDK internal range" of types we could use an enum (or private struct) for those and then have a custom_other_tlvs field that we make pub?

Presumably, the other TLVs would be Vec<(u64, Vec<u8>)>? Could be another variant of that enum. Not sure if a private struct would be much better since we eventually need to pass the data to the handler.

Do we currently use the path_id at all internally to LDK?

No, I believe we don't set it at all. Last we spoke about it, the thought was to reserve it for internal use.

@TheBlueMatt
Copy link
Collaborator

Presumably, the other TLVs would be Vec<(u64, Vec<u8>)>? Could be another variant of that enum.

Yea, the "user stuffed data" variant would presumably have this?

No, I believe we don't set it at all. Last we spoke about it, the thought was to reserve it for internal use.

I wonder if we should bother? Like, there's nothing special about the TLV the spec says should be the path_id, we can ignore it and no one will ever know. That would simplify the types here a bit.

@jkczyz
Copy link
Contributor

jkczyz commented Jun 3, 2024

I wonder if we should bother? Like, there's nothing special about the TLV the spec says should be the path_id, we can ignore it and no one will ever know. That would simplify the types here a bit.

Probably fine to ignore it or just encode the message::ReceiveTlvs enum (assuming this replaces RecipientData) as the path_id. Then we'd essentially have our own "namespace" for internal/custom types. Though this would diverge from what we do for payment::ReceiveTlvs.

@jkczyz
Copy link
Contributor

jkczyz commented Jun 6, 2024

Checking my understanding regarding path_id. The spec (BOLT4) doesn't distinguish whether it's used for messages or payments, but it seems to imply either.

When a recipient creates a blinded path to itself:

  • MAY store private data in encrypted_data_tlv[r].path_id to verify that the route is used in the right context and was created by them

The final recipient (payment or message not specified, but message is then implied):

  • MUST ignore the message if the path_id does not match the blinded route it created

But then in the rational, the spec uses a payment example:

The final recipient must verify that the blinded route is used in the right
context (e.g. for a specific payment) and was created by them. Otherwise a
malicious sender could create different blinded routes to all the nodes that
they suspect could be the real recipient and try them until one accepts the
message. The recipient can protect against that by storing E_r and the
context (e.g. a payment_hash), and verifying that they match when receiving
the onion. Otherwise, to avoid additional storage cost, it can put some private
context information in the path_id field (e.g. the payment_preimage) and
verify that when receiving the onion. Note that it's important to use private
information in that case, that senders cannot have access to.

Later on in the BOLT under the "Onion Messages" section, the writer of onionmsg_tlv:

  • For the final node's onionmsg_tlv:
    • if the final node is permitted to reply:
      • For every reply_path path:
        • MAY use path_id to contain a secret so it can recognize use of this reply_path.

And then the reader:

  • otherwise (it is the final node):
    • if path_id is set and corresponds to a path the reader has previously published in a reply_path:
      • if the onion message is not a reply to that previous onion:
        • MUST ignore the onion message
    • otherwise (unknown or unset path_id):
      • if the onion message is a reply to an onion message which contained a path_id:
        • MUST respond (or not respond) exactly as if it did not send the initial onion message.

Then in the rational:

Care must be taken that replies are only accepted using the exact
reply_path given, otherwise probing is possible. That means checking
both ways: non-replies don't use the reply path, and replies always
use the reply path.

Note how that section talks about reply paths.

In LDK, we don't use path_id currently.

  • payment::ReceiveTlvs doesn't have a path_id, but instead has payment_secret and payment_context fields encoded in custom TLVs.
  • message::ReceiveTlvs does have a path_id field, but it is an Option and is never set.

IIUC, we shouldn't include a path_id in Offer::paths or Refund::paths as it would increase the QR code size. But for reply paths and (possibly) payment paths, we should. If we encoded the RecipientData in the path_id (or any TLV really), we could use its presence and variant to determine if it is being used by the appropriate handler.

I think that (plus any additional handler checks) would be sufficient to prevent de-anonymization discussed in the quoted rationale sections. @TheBlueMatt @valentinewallace Does that seem to check out?

If so, we could drop the path_id field as discussed in my previous comment and still use the path_id TLV for encoding RecipientData. I'm not sure if we want to replace RecipientData with ReceiveTlvs though. This would prevent us from adding custom TLVs in the future. Or at very least would require refactoring the code back to how it currently is in this PR. Thoughts?

@TheBlueMatt
Copy link
Collaborator

Is the attack there basically that someone can take an offer we generated, then request the invoice_request from every node in the network and see who responds (or equivalent for payments)?

@jkczyz
Copy link
Contributor

jkczyz commented Jun 10, 2024

Is the attack there basically that someone can take an offer we generated, then request the invoice_request from every node in the network and see who responds (or equivalent for payments)?

Really to any node within N hops of the introduction node in one of the offer paths. Though using the path_id in an offer path would increase the QR code size. The attack seems only applicable when the recipient is an announced node. Otherwise, the attacker can't create a blinded path without a suspected node id to try for the recipient.

@TheBlueMatt
Copy link
Collaborator

Really to any node within N hops of the introduction node in one of the offer paths.

Hmm, okay, I think we're talking about different things, then. I think my above-described attack works though, right? Currently anyone can probe all public nodes and figure out if it is the node that issued an LDK offer, I think?

Though using the path_id in an offer path would increase the QR code size. The attack seems only applicable when the recipient is an announced node. Otherwise, the attacker can't create a blinded path without a suspected node id to try for the recipient.

Right, though it seems like if you know the node's pubkey you could do it too? Seems like something we should fix in both cases.

@jkczyz
Copy link
Contributor

jkczyz commented Jun 10, 2024

Hmm, okay, I think we're talking about different things, then. I think my above-described attack works though, right? Currently anyone can probe all public nodes and figure out if it is the node that issued an LDK offer, I think?

Yeah, I was just saying you could limit the search.

Right, though it seems like if you know the node's pubkey you could do it too? Seems like something we should fix in both cases.

Correct. IIUC, the fix would result in an increased offer QR code size.

@TheBlueMatt
Copy link
Collaborator

Correct. IIUC, the fix would result in an increased offer QR code size.

Yep, that's my understanding. Sadly we kinda have to cause this is a big issue :).

@TheBlueMatt
Copy link
Collaborator

In any case, as it relates to this PR, ISTM we don't care too much about users using path_id in their own blinded paths for their own use, so we might as well just have a top-level enum which users can put whatever they want in and then have an LDK internal variant that has private fields that only LDK can use.

@jkczyz
Copy link
Contributor

jkczyz commented Jun 11, 2024

In any case, as it relates to this PR, ISTM we don't care too much about users using path_id in their own blinded paths for their own use, so we might as well just have a top-level enum which users can put whatever they want in and then have an LDK internal variant that has private fields that only LDK can use.

Not sure I understand the suggestion here. We want different variants for each LDK usage so we can tell if the path is used correctly. Having a single variant with a bunch of Option fields makes this less intuitive. Or are you saying it would be a two-level enum? Feels like we should just have one enum with all the LDK internal variants plus one variant for user-specific usage (i.e., to use with CustomOnionMessageHandler) and never actually expose the enum publicly.

@TheBlueMatt
Copy link
Collaborator

Right, we're on the same page, my only point is that it seems like we should have a single-level enum, whereas the previous suggestion was a two-level enum.

@jkczyz
Copy link
Contributor

jkczyz commented Jun 11, 2024

Right, we're on the same page, my only point is that it seems like we should have a single-level enum, whereas the previous suggestion was a two-level enum.

I think the previous suggestion was a struct (ReceiveTlvs) holding an enum (RecipientData). 😄 IIUC, that is what we would need to hide the LDK-specific variants from the user. Only instead of the BlindedPath creation methods taking RecipientData, they would take ReceiveTlvs (just like the payment variations only with non-pub field). ReceiveTlvs would need a constructor to create one holding a custom variant, while LDK code could freely construct ReceiveTlvs holding the other RecipientData variants as needed.

@TheBlueMatt
Copy link
Collaborator

I think the previous suggestion was a struct (ReceiveTlvs) holding an enum (RecipientData).

Ah, right, I was thinking we'd just go to a single top-level enum.

IIUC, that is what we would need to hide the LDK-specific variants from the user.

Or we could go the other way - have a top-level enum where the LDK-hidden variants contain a struct, that way the user-visible API is simpler.

@jkczyz
Copy link
Contributor

jkczyz commented Jun 12, 2024

Or we could go the other way - have a top-level enum where the LDK-hidden variants contain a struct, that way the user-visible API is simpler.

SGTM. Each variant will likely need to hold a struct in order have something to pass to each handler. But, yeah, those structs can be pub only not creatable externally, IIUC. So the user would only be able to create the variant for custom data.

@jkczyz
Copy link
Contributor

jkczyz commented Jun 14, 2024

@TheBlueMatt FWIW, it might be simpler in the serialization code if we keep ReceiveTlvs as a struct with an optional enum. Otherwise, it's difficult to support the unblinded case.

payloads.push((Payload::Receive {
control_tlvs: ReceiveControlTlvs::Unblinded(ReceiveTlvs { path_id: None, }),
reply_path: reply_path.take(),
message,
}, prev_control_tlvs_ss.unwrap()));

@shaavan
Copy link
Member Author

shaavan commented Jun 15, 2024

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

Addressed comments from @TheBlueMatt and @jkczyz

Changes:

  1. Rebase on main.
  2. Updated ReceiveTlvs to include the optional RecipientData enum.
  3. This new approach allows for the serialization of the entire ReceiveTlvs under type 6.
  4. Using an enum allows us to ensure the response message received over the appended reply_path is of the correct type before invoking the handler.

@shaavan shaavan force-pushed the issue2837 branch 2 times, most recently from 64496c4 to 3be2801 Compare July 5, 2024 13:32
@shaavan
Copy link
Member Author

shaavan commented Jul 5, 2024

Updated from pr3085.15 to pr3085.16 (diff):

Changes:

  1. Fix fuzz test.

@TheBlueMatt TheBlueMatt added this to the 0.0.124 milestone Jul 8, 2024
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Pretty much LGTM other than a few last comments.

lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved
fuzz/src/invoice_request_deser.rs Outdated Show resolved Hide resolved
fuzz/src/refund_deser.rs Outdated Show resolved Hide resolved
lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/message.rs Show resolved Hide resolved
lightning/src/blinded_path/message.rs Show resolved Hide resolved
lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved
let secp_ctx = &self.secp_ctx;
let expanded_key = &self.inbound_payment_key;

let abandon_if_payment = |context| {
match context {
OffersContext::OutboundPayment { payment_id } => self.abandon_payment(payment_id),
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have to verify in InvoiceError - users may set a payment_id containing public(ish) data, and we can't let some third party fail all our outbound payments by providing InvoiceErrors.

lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
valentinewallace added a commit to valentinewallace/rust-lightning that referenced this pull request Jul 8, 2024
valentinewallace added a commit to valentinewallace/rust-lightning that referenced this pull request Jul 8, 2024
@shaavan
Copy link
Member Author

shaavan commented Jul 8, 2024

Updated from pr3085.16 to pr3085.17 (diff):
Addressed @jkczyz, @TheBlueMatt comments

  1. Update the fuzz test to take in the right MessageContext.
  2. Remove redundant Readable for ReceiveTlvs implementation.
  3. Update handle_onion_message_response to check based on context instead of boolean.
  4. Update documentation.

lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved
lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
@shaavan
Copy link
Member Author

shaavan commented Jul 8, 2024

Updated from pr3085.17 to pr3085.18 (diff):
Addressed @jkczyz comments

Update:

  1. Fix documentation
  2. Fix ci.

@jkczyz
Copy link
Contributor

jkczyz commented Jul 8, 2024

CI is sad because ln::max_payment_path_len_tests::bolt12_invoice_too_large_blinded_paths is now failing. @shaavan Would you mind rebasing and updating that test accordingly now that a PaymentFailed event is generated?

shaavan added 2 commits July 9, 2024 01:44
1. The path_id will be removed from the codebase in the following
   commits.
1. New Enum for Enhanced Data Handling:
   - Introduced the `MessageContext` enum, which allows the node to include
     additional context data along with the `reply_path` sent to the counterparty.
   - The node anticipates receiving this data back for further processing.

2. Variants in MessageContext:
   - The `MessageContext` enum includes two variants: "Offers" and
     "Context"
   - One of the variants, `Offers`, holds the `payment_id`
     of the associated Outbound BOLT12 Payment.

3. Future Usage:
   - This enum will be utilized in a subsequent commit to abandon outbound
     payments that have failed to complete for various reasons.
@shaavan
Copy link
Member Author

shaavan commented Jul 8, 2024

Updated from pr3085.18 to pr3085.19 (diff):
Addressed @jkczyz comments

  1. Rebase on main, and fix the bolt12_invoice_too_large_blinded_paths test, to make CI happy :)

Thanks for the pointer, @jkczyz!

shaavan added 2 commits July 9, 2024 17:27
1. Handling Offers Data:
   - Updated `handle_message` to accept `OffersContext` data as an input field.
   - If it is present, it will be utilized by the handler to
     abandon outbound payments that have failed for any reason.

2. Consistency in Custom Message Handling:
   - Updated `handle_custom_message` to accept optional custom data.
     for consistency.
   - Note: `custom_data` will remain unused in this PR.
…t field

- Enabled `create_blinded_paths` to accept `MessageContext` TLVs as
  an input field.
- `MessageContext` is intended to be sent along with the `reply_path`
   to the counterparty.
- Added `MessageContext` in the `create_blinded_paths` flow, optionally
  appending it within the `reply_path`.
- Updated tests to verify the new feature.
@shaavan
Copy link
Member Author

shaavan commented Jul 9, 2024

Updated from pr3085.19 to pr3085.20 (diff):
Addressed @TheBlueMatt and @jkczyz comment

  1. Update handle_custom_message input field to take in Option<Vec> instead of <Vec>.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Please update the PR title and summary to reflect name changes.

/// This data is encrypted by the recipient and remains invisible to anyone else.
/// It is included in the [`BlindedPath`], making it accessible again to the recipient
/// whenever the [`BlindedPath`] is used.
/// The recipient can authenticate the message and utilize it for further processing
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit

Suggested change
/// The recipient can authenticate the message and utilize it for further processing
/// The recipient can use this data to authenticate the message or for further processing

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll update this in #3139 since I'm touching this.

let secp_ctx = &self.secp_ctx;
let expanded_key = &self.inbound_payment_key;

let abandon_if_payment = |context| {
match context {
OffersContext::OutboundPayment { payment_id } => self.abandon_payment(payment_id),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the intended spec would be that we have some ability to verify the blinded path itself, which implies maybe a MAC in the blinded path :(

let secp_ctx = &self.secp_ctx;
let expanded_key = &self.inbound_payment_key;

let abandon_if_payment = |context| {
match context {
OffersContext::OutboundPayment { payment_id } => self.abandon_payment(payment_id),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems weird that this will call through and use PaymentFailureReason::UserAbandoned. ISTM we could just call pending_outbound_payments.abandon_payment manually with a better failure reason. We can do that in a followup/#3139 though.

@TheBlueMatt TheBlueMatt merged commit 6035c83 into lightningdevkit:main Jul 9, 2024
11 of 17 checks passed
@shaavan shaavan changed the title Introduce RecipientData and use it to allow abandon failed payments Introduce MessageContext and use it to allow abandon failed payments Jul 10, 2024
@shaavan shaavan deleted the issue2837 branch July 10, 2024 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider removing path_id from Responder Update payment and generate Event from OffersMessageHandler
5 participants