-
Notifications
You must be signed in to change notification settings - Fork 358
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
Expose withheld amount for underpaying HTLCs in PaymentForwarded
#2858
Expose withheld amount for underpaying HTLCs in PaymentForwarded
#2858
Conversation
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughThe recent updates introduce a new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (10)
- lightning/src/events/mod.rs (5 hunks)
- lightning/src/ln/chanmon_update_fail_tests.rs (8 hunks)
- lightning/src/ln/channel.rs (1 hunks)
- lightning/src/ln/channelmanager.rs (6 hunks)
- lightning/src/ln/functional_test_utils.rs (3 hunks)
- lightning/src/ln/functional_tests.rs (5 hunks)
- lightning/src/ln/payment_tests.rs (3 hunks)
- lightning/src/ln/reload_tests.rs (2 hunks)
- lightning/src/ln/reorg_tests.rs (1 hunks)
- lightning/src/ln/shutdown_tests.rs (3 hunks)
Files not reviewed due to errors (2)
- lightning/src/ln/functional_test_utils.rs (Error: unable to parse review)
- lightning/src/ln/payment_tests.rs (Error: unable to parse review)
Files skipped from review due to trivial changes (2)
- lightning/src/ln/chanmon_update_fail_tests.rs
- lightning/src/ln/shutdown_tests.rs
Additional comments: 14
lightning/src/ln/reorg_tests.rs (1)
- 128-128: The
expect_payment_forwarded
call now includes aNone
parameter forskimmed_fee_msat
, aligning with the PR's objective to handle underpaid HTLCs. This change is consistent with the PR's goal to enhance visibility and traceability of withheld amounts in HTLC forwarding scenarios.lightning/src/ln/reload_tests.rs (2)
- 931-931: The line
expect_payment_forwarded!(nodes[2], nodes[0], nodes[3], Some(1000), None, false, false);
has been replaced withNone
as the third argument in theexpect_payment_forwarded!
macro call. This change seems to align with the PR's objective to enhance the handling of underpaid HTLCs by introducing more explicit control over the fee skimming behavior. However, it's crucial to ensure that this change does not inadvertently affect the logic for forwarding payments, especially in scenarios where underpayment is not a factor.Ensure that the updated macro call with
None
correctly handles all scenarios, including those not involving underpaid HTLCs. It's important to verify that this change does not introduce any regressions in payment forwarding behavior.
- 1081-1081: The line
expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], Some(1000), None, false, true);
has been replaced withNone
as the third argument in theexpect_payment_forwarded!
macro call. Similar to the previous comment, this modification is presumably aimed at refining the handling of fees for underpaid HTLCs. It's essential to validate that this adjustment does not negatively impact the payment forwarding process, particularly in standard payment scenarios.Confirm that the change to the
expect_payment_forwarded!
macro call withNone
accurately processes all relevant cases, including those not related to underpayment. This verification is crucial to prevent any unintended consequences on the payment forwarding mechanism.lightning/src/events/mod.rs (1)
- 797-812: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [786-809]
The addition of
skimmed_fee_msat
to thePaymentForwarded
event is correctly implemented. This field provides forwarding nodes with detailed information about the fees they withhold when processing underpaid HTLCs, aligning with the PR's objectives. The optional nature ofskimmed_fee_msat
ensures compatibility with scenarios where an intercepted HTLC is forwarded with the expected amount, making the field unnecessary. The implementation also correctly handles the potential for duplicatePaymentForwarded
events whenfee_earned_msat
isNone
, indicating that the final fee calculation is pending due to on-chain transactions. This change enhances transparency and decision-making for nodes in the forwarding and settlement of HTLCs.lightning/src/ln/functional_tests.rs (2)
- 5100-5100: The change in the value for
outbound_amount_forwarded_msat
fromSome(196)
toNone
in theexpect_payment_forwarded!
macro call is a significant alteration. This change likely reflects a specific test scenario adjustment. It's important to ensure that this change accurately represents the intended test conditions and outcomes, especially in the context of underpaid HTLCs. The PR description does not explicitly mention this adjustment, so it's crucial to verify its alignment with the overall objectives.- 8822-8822: This modification in the
expect_payment_forwarded!
macro call, specifically the conditional value foroutbound_amount_forwarded_msat
, demonstrates flexibility in testing different scenarios of HTLC forwarding and settlement. It's a good practice to cover various cases, including on-chain and off-chain settlements. Ensuring that these test modifications align with the intended scenarios described in the PR objectives is important for the accuracy and comprehensiveness of the test suite.lightning/src/ln/channel.rs (1)
- 3307-3315: The modification to
update_fulfill_htlc
's return type to includeOption<u64>
as the third element in the tuple is a significant change. This addition likely represents theskimmed_fee_msat
and is a crucial part of enhancing transparency for forwarding nodes regarding withheld fees in underpaid HTLCs. Ensure that all calls to this function across the codebase have been updated to handle the new tuple structure. Additionally, consider adding documentation comments to explain the significance of each element in the returned tuple, especially theOption<u64>
element, to improve code readability and maintainability.lightning/src/ln/channelmanager.rs (7)
- 5671-5673: The addition of
skimmed_fee_msat
as a parameter toclaim_funds_internal
is consistent with the PR's objective to enhance visibility of withheld amounts in underpaid HTLCs. Ensure that all calls to this function have been updated to include this new parameter.- 5771-5772: The
debug_assert!
ensures thatskimmed_fee_msat
is always less than or equal tofee_earned_msat
, which is a logical check to maintain data integrity. This is a good practice for catching potential logic errors during development.- 5771-5780: The inclusion of
skimmed_fee_msat
in thePaymentForwarded
event aligns with the PR's goal to provide detailed information about fees withheld from underpaid HTLCs. This change enhances transparency for forwarding nodes.- 6719-6719: The tuple unpacking for
htlc_source
,forwarded_htlc_value
, andskimmed_fee_msat
withininternal_update_fulfill_htlc
introduces the handling of the newskimmed_fee_msat
parameter. This change is necessary for processing the updated event structure.- 6757-6760: The call to
claim_funds_internal
withininternal_update_fulfill_htlc
has been correctly updated to include theskimmed_fee_msat
parameter. This ensures that the function aligns with the updated signature and supports the new functionality.- 7257-7257: In the context of claiming HTLCs from the monitor,
skimmed_fee_msat
is passed asNone
since this scenario does not involve skimming fees from underpaid HTLCs. This usage is appropriate given the context and maintains the flexibility of theclaim_funds_internal
function.- 11130-11130: The call to
claim_funds_internal
from within the channel monitor's handling logic correctly passesNone
forskimmed_fee_msat
when claiming funds. This is consistent with the expected behavior when the source of the preimage is not directly related to an underpaid HTLC forwarding event.
9901a4b
to
d0085fd
Compare
@coderabbitai pause |
PaymentForwarded
PaymentForwarded
d0085fd
to
3a0c65a
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2858 +/- ##
==========================================
+ Coverage 89.14% 89.35% +0.20%
==========================================
Files 116 116
Lines 93205 95035 +1830
Branches 93205 95035 +1830
==========================================
+ Hits 83089 84914 +1825
- Misses 7583 7643 +60
+ Partials 2533 2478 -55 ☔ View full report in Codecov by Sentry. |
3a0c65a
to
da670f7
Compare
Lays groundwork to make claim_payment* test utils easier to adapt without changing a million callsites.
787c6c4
to
c5bb00a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Pretty trivially exposes a new event field with test coverage, so I don't think a 2nd reviewer is needed, but I'll give it a day or so before merging just in case.
@@ -281,7 +281,7 @@ fn mpp_retry_overpay() { | |||
let extra_fees = vec![0, total_overpaid_amount]; | |||
let expected_route = &[&[&nodes[1], &nodes[3]][..], &[&nodes[2], &nodes[3]][..]]; | |||
let args = ClaimAlongRouteArgs::new(&nodes[0], &expected_route[..], payment_preimage) | |||
.with_expected_extra_fees(extra_fees); | |||
.with_expected_min_htlc_overpay(extra_fees); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that one weird thing about this is that min HTLC overpayment may occur for any hop, not just the penultimate hop (unlike skimmed fees). Pre-existing though.
We generally allow routing nodes to forward less than the expected HTLC amount, if the receiver knowingly accepts this and claims the underpaying HTLC (see `ChannelConfig::accept_underpaying_htlcs`). This use case is in particular useful for the LSPS2/JIT channel setting where the intial underpaying HTLC pays for the channel open. While we previously exposed the withheld amount as `PaymentClaimable::counterparty_skimmed_fee_msat` on the receiver side, we did not individually provide it on the forwarding node's side. Here, we therefore expose this additionally withheld amount via `PaymentForwarded::skimmed_fee_msat`.
c5bb00a
to
9644588
Compare
Amended the last commit to include the following changes: > git diff-tree -U2 c5bb00a0a 96445880f
diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs
index 15bf220cf..801e8695c 100644
--- a/lightning/src/events/mod.rs
+++ b/lightning/src/events/mod.rs
@@ -804,5 +804,7 @@ pub enum Event {
/// expected amount. This means our counterparty accepted to receive less than the invoice
/// amount, e.g., by claiming the payment featuring a corresponding
- /// [`PaymentClaimable::counterparty_skimmed_fee_msat`]
+ /// [`PaymentClaimable::counterparty_skimmed_fee_msat`].
+ ///
+ /// Will also always be `None` for events serialized with LDK prior to version 0.0.122.
///
/// The caveat described above the `total_fee_earned_msat` field applies here as well.
diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs
index a8154a621..5f7c72be1 100644
--- a/lightning/src/ln/chanmon_update_fail_tests.rs
+++ b/lightning/src/ln/chanmon_update_fail_tests.rs
@@ -14,5 +14,6 @@
use bitcoin::blockdata::constants::genesis_block;
-use bitcoin::hash_types::BlockHash; use bitcoin::network::constants::Network;
+use bitcoin::hash_types::BlockHash;
+use bitcoin::network::constants::Network;
use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ChannelMonitor};
use crate::chain::transaction::OutPoint;
diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
index b91d255fc..e53fd6b0c 100644
--- a/lightning/src/ln/functional_test_utils.rs
+++ b/lightning/src/ln/functional_test_utils.rs
@@ -2237,7 +2237,8 @@ macro_rules! expect_payment_forwarded {
let mut events = $node.node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
- $crate::ln::functional_test_utils::expect_payment_forwarded( events.pop().unwrap(), &$node,
- &$prev_node, &$next_node, $expected_fee, None, $upstream_force_closed,
- $downstream_force_closed);
+ $crate::ln::functional_test_utils::expect_payment_forwarded(
+ events.pop().unwrap(), &$node, &$prev_node, &$next_node, $expected_fee, None,
+ $upstream_force_closed, $downstream_force_closed
+ );
}
} |
We generally allow routing nodes to forward less than the expected HTLC amount, if the receiver knowingly accepts this and claims the underpaying HTLC (see
ChannelConfig::accept_underpaying_htlcs
). This is in particular useful for the LSPS2/JIT use case where the intial underpaying HTLC pays for the channel open (see lightning/blips#25).While we previously exposed the withheld amount as
PaymentClaimable::counterparty_skimmed_fee_msat
on the receiver side, we did not individually provide it on the forwarding node's side. Here, we therefore expose this additionally withheld amount viaPaymentForwarded::skimmed_fee_msat
.(cc @johncantrell97 @valentinewallace)