Allow forwarding less than the amount in the onion by valentinewallace · Pull Request #2319 · lightningdevkit/rust-lightning

valentinewallace · 2023-05-25T17:02:59Z

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

V1 of addressing #1999
~~Based on #2305~~

codecov-commenter · 2023-05-25T20:16:24Z

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.

TheBlueMatt

At first glance this basically looks good I think.

valentinewallace · 2023-06-08T14:23:14Z

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)

alecchendev

Nice!

alecchendev · 2023-06-10T00:57:49Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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.

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.

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

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?

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.

tnull

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?

valentinewallace · 2023-06-12T21:12:15Z

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 · 2023-06-13T21:06:05Z

Feel free to squash IMO.

TheBlueMatt

Will need a small rebase after #2077

TheBlueMatt · 2023-06-13T22:39:24Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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?

TheBlueMatt · 2023-06-13T22:42:02Z

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


TIL what minuend means.

TheBlueMatt · 2023-06-13T22:46:25Z

+ 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)),


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

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 · 2023-06-15T20:29:40Z

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

TheBlueMatt

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

alecchendev

LGTM I think!

alecchendev · 2023-06-20T21:24:47Z

+	/// # 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.


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

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

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

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.

See its docs

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 · 2023-06-20T22:03:22Z

Squashed.

TheBlueMatt · 2023-06-21T01:08:26Z

 first_hop_htlc_msat: htlc_msat,
 payment_id,
- }, onion_packet, &self.logger);
+ }, onion_packet, None, &self.logger);


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.

valentinewallace · 2023-05-25T17:02:59Z

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

V1 of addressing #1999
~~Based on #2305~~

codecov-commenter · 2023-05-25T20:16:24Z

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.

TheBlueMatt

At first glance this basically looks good I think.

valentinewallace · 2023-06-08T14:23:14Z

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)

alecchendev

Nice!

alecchendev · 2023-06-10T00:57:49Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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.

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.

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

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?

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.

tnull

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?

valentinewallace · 2023-06-12T21:12:15Z

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 · 2023-06-13T21:06:05Z

Feel free to squash IMO.

TheBlueMatt

Will need a small rebase after #2077

TheBlueMatt · 2023-06-13T22:39:24Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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?

TheBlueMatt · 2023-06-13T22:42:02Z

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


TIL what minuend means.

TheBlueMatt · 2023-06-13T22:46:25Z

+ 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)),


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

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 · 2023-06-15T20:29:40Z

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

TheBlueMatt

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

alecchendev

LGTM I think!

alecchendev · 2023-06-20T21:24:47Z

+	/// # 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.


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

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

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

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.

See its docs

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 · 2023-06-20T22:03:22Z

Squashed.

TheBlueMatt · 2023-06-21T01:08:26Z

 first_hop_htlc_msat: htlc_msat,
 payment_id,
- }, onion_packet, &self.logger);
+ }, onion_packet, None, &self.logger);


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.

valentinewallace · 2023-05-25T17:02:59Z

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

V1 of addressing #1999
~~Based on #2305~~

codecov-commenter · 2023-05-25T20:16:24Z

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.

TheBlueMatt

At first glance this basically looks good I think.

valentinewallace · 2023-06-08T14:23:14Z

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)

alecchendev

Nice!

alecchendev · 2023-06-10T00:57:49Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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.

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.

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

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?

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.

tnull

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?

valentinewallace · 2023-06-12T21:12:15Z

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 · 2023-06-13T21:06:05Z

Feel free to squash IMO.

TheBlueMatt

Will need a small rebase after #2077

TheBlueMatt · 2023-06-13T22:39:24Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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?

TheBlueMatt · 2023-06-13T22:42:02Z

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


TIL what minuend means.

TheBlueMatt · 2023-06-13T22:46:25Z

+ 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)),


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

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 · 2023-06-15T20:29:40Z

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

TheBlueMatt

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

alecchendev

LGTM I think!

alecchendev · 2023-06-20T21:24:47Z

+	/// # 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.


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

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

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

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.

See its docs

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 · 2023-06-20T22:03:22Z

Squashed.

TheBlueMatt · 2023-06-21T01:08:26Z

 first_hop_htlc_msat: htlc_msat,
 payment_id,
- }, onion_packet, &self.logger);
+ }, onion_packet, None, &self.logger);


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.

valentinewallace · 2023-05-25T17:02:59Z

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

V1 of addressing #1999
~~Based on #2305~~

codecov-commenter · 2023-05-25T20:16:24Z

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.

TheBlueMatt

At first glance this basically looks good I think.

valentinewallace · 2023-06-08T14:23:14Z

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)

alecchendev

Nice!

alecchendev · 2023-06-10T00:57:49Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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.

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.

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

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?

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.

tnull

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?

valentinewallace · 2023-06-12T21:12:15Z

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 · 2023-06-13T21:06:05Z

Feel free to squash IMO.

TheBlueMatt

Will need a small rebase after #2077

TheBlueMatt · 2023-06-13T22:39:24Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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?

TheBlueMatt · 2023-06-13T22:42:02Z

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


TIL what minuend means.

TheBlueMatt · 2023-06-13T22:46:25Z

+ 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)),


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

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 · 2023-06-15T20:29:40Z

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

TheBlueMatt

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

alecchendev

LGTM I think!

alecchendev · 2023-06-20T21:24:47Z

+	/// # 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.


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

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

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

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.

See its docs

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 · 2023-06-20T22:03:22Z

Squashed.

TheBlueMatt · 2023-06-21T01:08:26Z

 first_hop_htlc_msat: htlc_msat,
 payment_id,
- }, onion_packet, &self.logger);
+ }, onion_packet, None, &self.logger);


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.

valentinewallace · 2023-05-25T17:02:59Z

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

V1 of addressing #1999
~~Based on #2305~~

codecov-commenter · 2023-05-25T20:16:24Z

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.

TheBlueMatt

At first glance this basically looks good I think.

valentinewallace · 2023-06-08T14:23:14Z

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)

alecchendev

Nice!

alecchendev · 2023-06-10T00:57:49Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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.

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.

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

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?

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.

tnull

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?

valentinewallace · 2023-06-12T21:12:15Z

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 · 2023-06-13T21:06:05Z

Feel free to squash IMO.

TheBlueMatt

Will need a small rebase after #2077

TheBlueMatt · 2023-06-13T22:39:24Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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?

TheBlueMatt · 2023-06-13T22:42:02Z

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


TIL what minuend means.

TheBlueMatt · 2023-06-13T22:46:25Z

+ 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)),


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

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 · 2023-06-15T20:29:40Z

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

TheBlueMatt

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

alecchendev

LGTM I think!

alecchendev · 2023-06-20T21:24:47Z

+	/// # 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.


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

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

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

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.

See its docs

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 · 2023-06-20T22:03:22Z

Squashed.

TheBlueMatt · 2023-06-21T01:08:26Z

 first_hop_htlc_msat: htlc_msat,
 payment_id,
- }, onion_packet, &self.logger);
+ }, onion_packet, None, &self.logger);


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.

valentinewallace · 2023-05-25T17:02:59Z

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

V1 of addressing #1999
~~Based on #2305~~

codecov-commenter · 2023-05-25T20:16:24Z

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.

TheBlueMatt

At first glance this basically looks good I think.

valentinewallace · 2023-06-08T14:23:14Z

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)

alecchendev

Nice!

alecchendev · 2023-06-10T00:57:49Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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.

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.

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

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?

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.

tnull

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?

valentinewallace · 2023-06-12T21:12:15Z

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 · 2023-06-13T21:06:05Z

Feel free to squash IMO.

TheBlueMatt

Will need a small rebase after #2077

TheBlueMatt · 2023-06-13T22:39:24Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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?

TheBlueMatt · 2023-06-13T22:42:02Z

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


TIL what minuend means.

TheBlueMatt · 2023-06-13T22:46:25Z

+ 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)),


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

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 · 2023-06-15T20:29:40Z

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

TheBlueMatt

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

alecchendev

LGTM I think!

alecchendev · 2023-06-20T21:24:47Z

+	/// # 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.


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

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

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

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.

See its docs

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 · 2023-06-20T22:03:22Z

Squashed.

TheBlueMatt · 2023-06-21T01:08:26Z

 first_hop_htlc_msat: htlc_msat,
 payment_id,
- }, onion_packet, &self.logger);
+ }, onion_packet, None, &self.logger);


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.

valentinewallace · 2023-05-25T17:02:59Z

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

V1 of addressing #1999
~~Based on #2305~~

codecov-commenter · 2023-05-25T20:16:24Z

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.

TheBlueMatt

At first glance this basically looks good I think.

valentinewallace · 2023-06-08T14:23:14Z

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)

alecchendev

Nice!

alecchendev · 2023-06-10T00:57:49Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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.

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.

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

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?

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.

tnull

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?

valentinewallace · 2023-06-12T21:12:15Z

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 · 2023-06-13T21:06:05Z

Feel free to squash IMO.

TheBlueMatt

Will need a small rebase after #2077

TheBlueMatt · 2023-06-13T22:39:24Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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?

TheBlueMatt · 2023-06-13T22:42:02Z

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


TIL what minuend means.

TheBlueMatt · 2023-06-13T22:46:25Z

+ 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)),


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

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 · 2023-06-15T20:29:40Z

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

TheBlueMatt

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

alecchendev

LGTM I think!

alecchendev · 2023-06-20T21:24:47Z

+	/// # 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.


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

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

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

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.

See its docs

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 · 2023-06-20T22:03:22Z

Squashed.

TheBlueMatt · 2023-06-21T01:08:26Z

 first_hop_htlc_msat: htlc_msat,
 payment_id,
- }, onion_packet, &self.logger);
+ }, onion_packet, None, &self.logger);


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.

valentinewallace · 2023-05-25T17:02:59Z

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

V1 of addressing #1999
~~Based on #2305~~

codecov-commenter · 2023-05-25T20:16:24Z

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.

TheBlueMatt

At first glance this basically looks good I think.

valentinewallace · 2023-06-08T14:23:14Z

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)

alecchendev

Nice!

alecchendev · 2023-06-10T00:57:49Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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.

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.

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

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?

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.

tnull

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?

valentinewallace · 2023-06-12T21:12:15Z

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 · 2023-06-13T21:06:05Z

Feel free to squash IMO.

TheBlueMatt

Will need a small rebase after #2077

TheBlueMatt · 2023-06-13T22:39:24Z

 payment_hash,
 purpose: purpose(),
 amount_msat,
+ counterparty_skimmed_fee_msat: total_value.saturating_sub(amount_msat),


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?

TheBlueMatt · 2023-06-13T22:42:02Z

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


TIL what minuend means.

TheBlueMatt · 2023-06-13T22:46:25Z

+ 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)),


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

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 · 2023-06-15T20:29:40Z

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

TheBlueMatt

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

alecchendev

LGTM I think!

alecchendev · 2023-06-20T21:24:47Z

+	/// # 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.


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

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

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

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.

See its docs

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 · 2023-06-20T22:03:22Z

Squashed.

TheBlueMatt · 2023-06-21T01:08:26Z

 first_hop_htlc_msat: htlc_msat,
 payment_id,
- }, onion_packet, &self.logger);
+ }, onion_packet, None, &self.logger);


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.

valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from ad259dd to 2a07438Compare May 25, 2023 18:49

wpaulino added this to the 0.0.116 milestone May 25, 2023

TheBlueMatt reviewed May 31, 2023
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated

dunxen self-requested a review June 4, 2023 20:24

valentinewallace added blocked on dependent pr and removed blocked on dependent pr labels Jun 5, 2023

valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from 2a07438 to a1cbf67Compare June 8, 2023 14:18

valentinewallace marked this pull request as ready for review June 8, 2023 14:21

TheBlueMatt reviewed Jun 8, 2023
View reviewed changes

Comment threadlightning/src/events/mod.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated

valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from a1cbf67 to db93d1bCompare June 9, 2023 14:28

tnull self-requested a review June 9, 2023 21:39

alecchendev reviewed Jun 10, 2023
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/msgs.rs

alecchendev reviewed Jun 10, 2023
View reviewed changes

tnull reviewed Jun 12, 2023
View reviewed changes

Comment threadlightning/src/util/config.rs Outdated
Comment threadlightning/src/util/config.rs Outdated
Comment threadlightning/src/util/config.rs Outdated

valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from db93d1b to ca293bdCompare June 12, 2023 21:06

valentinewallace mentioned this pull request Jun 12, 2023
Allow Accepting HTLCs with less value than the onion claims to pay #1999
Closed
3 tasks

TheBlueMatt reviewed Jun 13, 2023
View reviewed changes

TheBlueMatt added the blocked on dependent pr label Jun 13, 2023

valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from ca293bd to 8370444Compare June 15, 2023 20:27

valentinewallace force-pushed the 2023-05-forward-less-than-onion branch 2 times, most recently from 45a7180 to 703b03aCompare June 16, 2023 15:26

valentinewallace removed the blocked on dependent pr label Jun 16, 2023

alecchendev reviewed Jun 16, 2023
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated

tnull reviewed Jun 17, 2023
View reviewed changes

Comment threadlightning/src/util/config.rs Outdated
Comment threadpending_changelog/forward-underpaying-htlc.txt
Comment threadlightning/src/ln/channelmanager.rs Outdated

valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from 703b03a to 1c63f20Compare June 20, 2023 16:29

TheBlueMatt reviewed Jun 20, 2023
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs

TheBlueMatt modified the milestones: 0.0.116, 0.0.116alphaJun 20, 2023

alecchendev reviewed Jun 20, 2023
View reviewed changes

valentinewallace force-pushed the 2023-05-forward-less-than-onion branch 2 times, most recently from 646a030 to 134e631Compare June 20, 2023 21:57

valentinewallace added 12 commits June 20, 2023 17:57

Move PendingHTLCStatus construction inside channel lock
a2a7fef
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.

Move next hop packet pubkey calculation to outside channel lock
672d666

Add config knob for accepting underpaying HTLCs
85a5b31
See ChannelConfig::accept_underpaying_htlcs

Set extra skimmed fee on intercepted forward
52f2901
Receivers need to use this value to verify incoming payments if ChannelConfig::accept_underpaying_htlcs is set.

Persist counterparty skimmed fee in ClaimableHTLC
9485304
Used to get an accurate skimmed fee in the resulting PaymentClaimable event.

Add PaymentClaimable::counterparty_skimmed_fee_msat
27b99ac
See its docs

Allow receiving less than the onion claims to pay
9a3914d
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.

Track the sender's skimmed fee in UpdateAddHTLC
47567e4

Persist update_add sender skimmed fee in Channel
5a79d6c

Set UpdateAddHTLC::skimmed_fee_msat on forward
4cee622
So the receiver can verify it and approve underpaying HTLCs (see ChannelConfig::accept_underpaying_htlcs).

Check UpdateAddHTLC::skimmed_fee_msat on receive
0d94d9f
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.

Document on claim events that amount_msat may be > invoice amount
2127eb8

valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from 134e631 to 2127eb8Compare June 20, 2023 21:57

TheBlueMatt approved these changes Jun 21, 2023
View reviewed changes

tnull approved these changes Jun 21, 2023
View reviewed changes

tnull merged commit 15b1c9b into lightningdevkit:mainJun 21, 2023

TheBlueMatt modified the milestones: 0.0.116alpha, 0.0.116 Jun 24, 2023

wpaulino mentioned this pull request Aug 9, 2023
Provide the HTLCs that settled a payment. #2478
Merged