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

Allow forwarding less than the amount in the onion #2319

Merged

Conversation

valentinewallace
Copy link
Contributor

@valentinewallace valentinewallace commented May 25, 2023

Support skimming an additional fee off of intercepted HTLCs, per lightning/blips#25.

V1 of addressing #1999
Based on #2305

@valentinewallace valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from ad259dd to 2a07438 Compare May 25, 2023 18:49
@codecov-commenter
Copy link

codecov-commenter commented May 25, 2023

Codecov Report

Patch coverage: 90.61% and project coverage change: +0.15 🎉

Comparison is base (ae9e96e) 90.35% compared to head (2127eb8) 90.50%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2319      +/-   ##
==========================================
+ Coverage   90.35%   90.50%   +0.15%     
==========================================
  Files         106      106              
  Lines       54347    59105    +4758     
  Branches    54347    59105    +4758     
==========================================
+ Hits        49105    53494    +4389     
- Misses       5242     5611     +369     
Impacted Files Coverage Δ
lightning/src/ln/functional_tests.rs 98.34% <ø> (+0.11%) ⬆️
lightning/src/ln/channel.rs 89.49% <57.89%> (-0.33%) ⬇️
lightning/src/events/mod.rs 43.58% <90.00%> (+2.17%) ⬆️
lightning/src/ln/channelmanager.rs 87.79% <94.07%> (+1.21%) ⬆️
lightning/src/ln/payment_tests.rs 97.66% <98.79%> (+0.04%) ⬆️
lightning/src/ln/functional_test_utils.rs 87.63% <100.00%> (+0.34%) ⬆️
lightning/src/ln/msgs.rs 84.59% <100.00%> (+0.01%) ⬆️
lightning/src/util/config.rs 57.47% <100.00%> (-0.18%) ⬇️

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@wpaulino wpaulino added this to the 0.0.116 milestone May 25, 2023
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

At first glance this basically looks good I think.

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@dunxen dunxen self-requested a review June 4, 2023 20:24
@valentinewallace valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from 2a07438 to a1cbf67 Compare June 8, 2023 14:18
@valentinewallace valentinewallace marked this pull request as ready for review June 8, 2023 14:21
@valentinewallace
Copy link
Contributor Author

valentinewallace commented Jun 8, 2023

I removed phantom support for now. Also noticed that this breaks compat for current users of UserConfig::accept_intercept_htlcs. Not sure if a release note is sufficient so thoughts welcome there. (edit: we now only break compat if the forwarder actually skims a fee)

lightning/src/events/mod.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
@valentinewallace valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from a1cbf67 to db93d1b Compare June 9, 2023 14:28
@tnull tnull self-requested a review June 9, 2023 21:39
Copy link
Contributor

@alecchendev alecchendev left a comment

Choose a reason for hiding this comment

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

Nice!

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/msgs.rs Show resolved Hide resolved
@@ -3760,6 +3812,7 @@ where
payment_hash,
purpose: purpose(),
amount_msat,
counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),
Copy link
Contributor

Choose a reason for hiding this comment

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

In the case where a counterparty overshoots the amount in the onion, I think this would report the skimmed fee inaccurately? E.g. the counterparty_skimmed_fee_msat would be offset by amount_msat - total_value. It probably wouldn't happen very often and the offset probably wouldn't be much, but figured I'd still make note of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think this field will be inaccurate if some non-penultimate intermediate node took took less fee than intended by the sender (if I understand you correctly), but I'm not sure if we're able to detect that? IIUC we only have sender_intended_total and actual_received_total to work off of here, though might be missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be possible by adding the skimmed fee to PendingHTLCRouting::Receive/ReceiveKeysend (which seems possible since the skimmed fee gets sent through construct_recv_pending_htlc_info), and then probably keeping this on ClaimableHTLC and using those when finally calculating? Not sure if it's worth trying to fit it in here, could maybe be followup

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea, I think this would be nice, otherwise an intermediary node can cause a payment to fail by not taking enough fee? Its not itself a super big deal, but I think you could use it to figure out if the destination node is an LSP Client and if so a recent one - eg if you have a guess that the ultimate node is one further than some large LSP you could short yourself 1 msat and see if the payment fails to see if its an LSP client or just a routing node peer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! Fixed. I didn't want to just trust whatever the counterparty set as the skimmed fee since they could technically lie about that, so it's now a mix of the previous and new solution, PTAL.

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Looks good to me after a quick initial pass.

I wonder if it would be nice to also expose the counterparty_skimmed_fee_msat field via PaymentClaimed?

lightning/src/util/config.rs Outdated Show resolved Hide resolved
lightning/src/util/config.rs Outdated Show resolved Hide resolved
lightning/src/util/config.rs Outdated Show resolved Hide resolved
@valentinewallace
Copy link
Contributor Author

I wonder if it would be nice to also expose the counterparty_skimmed_fee_msat field via PaymentClaimed?

Added a TODO for this to #1999

@TheBlueMatt
Copy link
Collaborator

Feel free to squash IMO.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Will need a small rebase after #2077

lightning/src/events/mod.rs Show resolved Hide resolved
@@ -3760,6 +3812,7 @@ where
payment_hash,
purpose: purpose(),
amount_msat,
counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea, I think this would be nice, otherwise an intermediary node can cause a payment to fail by not taking enough fee? Its not itself a super big deal, but I think you could use it to figure out if the destination node is an LSP Client and if so a recent one - eg if you have a guess that the ultimate node is one further than some large LSP you could short yourself 1 msat and see if the payment fails to see if its an LSP client or just a routing node peer?

onion_packet,
short_channel_id: next_hop_scid,
skimmed_fee_msat:
// The minuend here must match the expected forward amount generated for the
Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL what minuend means.

skimmed_fee_msat:
// The minuend here must match the expected forward amount generated for the
// HTLCIntercepted event.
Some(payment.forward_info.outgoing_amt_msat.saturating_sub(amt_to_forward_msat)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be None if we're not subtracting anything. Also do we want to fail if the resulting amount is zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to None. I don't think we should fail if we forward more than expected, since the docs already say we allow this?

@valentinewallace
Copy link
Contributor Author

Rebased on #2077 to get a head start on rebase conflicts.

@valentinewallace valentinewallace force-pushed the 2023-05-forward-less-than-onion branch 2 times, most recently from 45a7180 to 703b03a Compare June 16, 2023 15:26
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/util/config.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@valentinewallace valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from 703b03a to 1c63f20 Compare June 20, 2023 16:29
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

LGTM, one real comment and a nit, feel free to squash.

lightning/src/ln/channelmanager.rs Show resolved Hide resolved
lightning/src/ln/channel.rs Show resolved Hide resolved
@TheBlueMatt TheBlueMatt modified the milestones: 0.0.116, 0.0.116alpha Jun 20, 2023
Copy link
Contributor

@alecchendev alecchendev left a comment

Choose a reason for hiding this comment

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

LGTM I think!

lightning/src/util/config.rs Outdated Show resolved Hide resolved
Comment on lines +412 to +416
/// # Note
/// It's important for payee wallet software to verify that [`PaymentClaimable::amount_msat`] is
/// as-expected if this feature is activated, otherwise they may lose money!
/// [`PaymentClaimable::counterparty_skimmed_fee_msat`] provides the fee taken by the
/// counterparty.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's worth mentioning here or is maybe more fit for an LSP client spec, but I think the attack described in #2319 (comment) is still possible if the payee fails a payment based on the skimmed fee being too high instead of just checking amount_msat and that the sum of amount_msat + counterparty_skimmed_fee_msat add up to at least what they expected. The docs here already only say to check amount_msat so I think it's probably fine as is but figured I'd still make note of it :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think so? If the fee skimmed is too high that means the immediately-prior node took too much, if a node prior to that in the path took too much it should result in the second-to-last hop taking too little (if they're taking a percentage fee).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh my bad, for some reason I was thinking intermediate nodes could still affect the final skimmed fee but that was fixed, oops

@valentinewallace valentinewallace force-pushed the 2023-05-forward-less-than-onion branch 2 times, most recently from 646a030 to 134e631 Compare June 20, 2023 21:57
We need the channel lock for constructing a pending HTLC's status because we
need to know if the channel accepts underpaying HTLCs in upcoming commits.
See ChannelConfig::accept_underpaying_htlcs
Receivers need to use this value to verify incoming payments if
ChannelConfig::accept_underpaying_htlcs is set.
Used to get an accurate skimmed fee in the resulting PaymentClaimable event.
Useful for penultimate hops in routes to take an extra fee, if for example they
opened a JIT channel to the payee and want them to help bear the channel open
cost.
So the receiver can verify it and approve underpaying HTLCs (see
ChannelConfig::accept_underpaying_htlcs).
Make sure the penultimate hop took the amount of fee that they claimed to take.
Without checking this TLV, we're heavily relying on the receiving wallet code
to correctly implement logic to calculate that that the fee is as expected.
@valentinewallace valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from 134e631 to 2127eb8 Compare June 20, 2023 21:57
@valentinewallace
Copy link
Contributor Author

Squashed.

@@ -2984,7 +3032,7 @@ where
session_priv: session_priv.clone(),
first_hop_htlc_msat: htlc_msat,
payment_id,
}, onion_packet, &self.logger);
}, onion_packet, None, &self.logger);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably need, as a followup, to support setting this - if you're an LSP and want to be the first payment to a given client you'll need it to take a fee on the first hop. Ideally we'd have something to handle that, but right now I don't think we will let you "just" send a payment to an intercept SCID, which we may consider.

@tnull tnull merged commit 15b1c9b into lightningdevkit:main Jun 21, 2023
@TheBlueMatt TheBlueMatt modified the milestones: 0.0.116alpha, 0.0.116 Jun 24, 2023
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.

6 participants