Expose withheld amount for underpaying HTLCs in PaymentForwarded#2858
Merged
valentinewallace merged 4 commits intoFeb 1, 2024
Merged
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 (
|
tnull
commented
Jan 29, 2024
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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_forwardedcall now includes aNoneparameter 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 withNoneas 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
Nonecorrectly 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 withNoneas 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 withNoneaccurately 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_msatto thePaymentForwardedevent 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_msatensures 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 duplicatePaymentForwardedevents whenfee_earned_msatisNone, 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_msatfromSome(196)toNonein 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_msatand 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_msatas a parameter toclaim_funds_internalis 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_msatis 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_msatin thePaymentForwardedevent 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_msatwithininternal_update_fulfill_htlcintroduces the handling of the newskimmed_fee_msatparameter. This change is necessary for processing the updated event structure.- 6757-6760: The call to
claim_funds_internalwithininternal_update_fulfill_htlchas been correctly updated to include theskimmed_fee_msatparameter. 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_msatis passed asNonesince this scenario does not involve skimming fees from underpaid HTLCs. This usage is appropriate given the context and maintains the flexibility of theclaim_funds_internalfunction.- 11130-11130: The call to
claim_funds_internalfrom within the channel monitor's handling logic correctly passesNoneforskimmed_fee_msatwhen 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.
tnull
force-pushed
the
2024-01-expose-skimmed-fee-msat
branch
from
9901a4b to
d0085fd
Compare
Contributor
Author
|
@coderabbitai pause |
tnull
changed the title
PaymentForwardedPaymentForwarded
tnull
force-pushed
the
2024-01-expose-skimmed-fee-msat
branch
from
d0085fd to
3a0c65a
Compare
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
tnull
force-pushed
the
2024-01-expose-skimmed-fee-msat
branch
from
3a0c65a to
da670f7
Compare
Lays groundwork to make claim_payment* test utils easier to adapt without changing a million callsites.
tnull
force-pushed
the
2024-01-expose-skimmed-fee-msat
branch
2 times, most recently
from
787c6c4 to
c5bb00a
Compare
valentinewallace
previously approved these changes
Jan 31, 2024
valentinewallace
left a comment
valentinewallace
left a comment
Contributor
There was a problem hiding this comment.
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.
| 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); |
Contributor
There was a problem hiding this comment.
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`.
tnull
force-pushed
the
2024-01-expose-skimmed-fee-msat
branch
from
c5bb00a to
9644588
Compare
Contributor
Author
|
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
+ );
}
} |
valentinewallace
approved these changes
Feb 1, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_msaton 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)