-
Notifications
You must be signed in to change notification settings - Fork 339
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 {prev,next}_user_channel_id
fields in PaymentForwarded
#2924
Expose {prev,next}_user_channel_id
fields in PaymentForwarded
#2924
Conversation
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughThe recent updates enhance the handling of payment forwarding within the Lightning Network's codebase, specifically by refining the Changes
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 (
|
063595e
to
42a69f2
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- lightning/src/events/mod.rs (4 hunks)
- lightning/src/ln/channelmanager.rs (9 hunks)
- lightning/src/ln/functional_test_utils.rs (2 hunks)
Files not reviewed due to errors (1)
- (no review received)
Additional comments: 10
lightning/src/events/mod.rs (1)
- 800-817: The addition of
prev_user_channel_id
andnext_user_channel_id
to thePaymentForwarded
event is a significant enhancement for tracking payments and channels byuser_channel_id
. This change directly addresses the PR's objective to streamline the process of tracking payments and channels, thereby optimizing resource usage and simplifying the codebase for developers working on related functionalities.lightning/src/ln/channelmanager.rs (9)
- 5736-5736: The addition of
next_user_channel_id
to theclaim_funds_internal
function signature is aligned with the PR's objectives. Ensure that this new parameter is integrated correctly throughout the system and that its usage is consistent with the intended tracking enhancements.- 5755-5755: Introduction of
prev_user_channel_id
is consistent with the PR's objectives of enhancing payment tracking. Verify its effective usage and ensure it does not introduce unintended side effects.- 5816-5816: Ensure that the conditional check involving
prev_channel_id
is correctly implemented and necessary for the intended enhancements.- 5842-5843: Augmentation of the
PaymentForwarded
event withprev_user_channel_id
andnext_user_channel_id
aligns with the PR's objectives. Verify its impact on the event's usage across the system to ensure it contributes to the intended simplification and optimization.- 6821-6821: Introduction of
next_user_channel_id
in theinternal_update_fulfill_htlc
function is consistent with the PR's objectives. Verify its integration and usage within the function and related functionalities to ensure it does not disrupt existing processes.- 6851-6851: Assignment of
next_user_channel_id
within this conditional block is consistent with the PR's objectives. Verify its correct implementation and impact on related functionalities to ensure it does not introduce unintended side effects.- 6863-6863: The call to
claim_funds_internal
withnext_user_channel_id
as a parameter aligns with the PR's objectives. Verify its integration and impact on the system to ensure it contributes to the intended enhancements.- 7365-7365: The call to
claim_funds_internal
withoutnext_user_channel_id
in this context appears to be a specific case. Verify the justification for not includingnext_user_channel_id
to ensure it aligns with the PR's objectives and does not introduce inconsistencies.- 11325-11325: The call to
claim_funds_internal
withoutnext_user_channel_id
in this context appears to be another specific case. Verify the justification for not includingnext_user_channel_id
to ensure it aligns with the PR's objectives and does not introduce inconsistencies.
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 (3)
- lightning/src/events/mod.rs (4 hunks)
- lightning/src/ln/channelmanager.rs (9 hunks)
- lightning/src/ln/functional_test_utils.rs (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- lightning/src/events/mod.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
42a69f2
to
7b6a355
Compare
@coderabbitai pause, now! |
@tnull: Understood, pausing the automated reviews now. Actions performedReviews paused. |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2924 +/- ##
==========================================
+ Coverage 89.15% 90.03% +0.87%
==========================================
Files 117 117
Lines 94868 100403 +5535
Branches 94868 100403 +5535
==========================================
+ Hits 84584 90396 +5812
+ Misses 7808 7560 -248
+ Partials 2476 2447 -29 ☔ View full report in Codecov by Sentry. |
7b6a355
to
a062317
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.
Thanks.
@@ -7359,7 +7364,7 @@ where | |||
log_trace!(logger, "Claiming HTLC with preimage {} from our monitor", preimage); | |||
self.claim_funds_internal(htlc_update.source, preimage, | |||
htlc_update.htlc_value_satoshis.map(|v| v * 1000), None, true, | |||
false, counterparty_node_id, funding_outpoint, channel_id); | |||
false, counterparty_node_id, funding_outpoint, channel_id, None); |
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.
Would we consider including this field in MonitorEvent
s in the future so it can be available here?
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.
Thought about that too, but that might require starting to track it in the ChannelMonitor
s first, which might be a bit out-of-scope?
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.
We also currently don't have any concept of how to handle user channel ids after a channel closes - is recycling allowed at that point?
@@ -6857,7 +6862,7 @@ where | |||
}; | |||
self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), | |||
Some(forwarded_htlc_value), skimmed_fee_msat, false, false, Some(*counterparty_node_id), | |||
funding_txo, msg.channel_id | |||
funding_txo, msg.channel_id, Some(next_user_channel_id), |
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.
FWIW, tests pass when I revert this change.
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.
Ugh, good catch. That's due to the map_or
in the test which makes it too forgiving. I'll see to revisit that.
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.
Now pushed a fixup to the test: #2924 (comment)
It's not perfect, but I wanted to avoid cluttering the macro by introducing yet another expected_X
variable just for this slim use case. Let me know if you'd prefer it over the current approach though.
d909a4e
a062317
to
d909a4e
Compare
This is useful for users that track channels by `user_channel_id`. For example, in `lightning-liquidity` we currently keep a full `HashMap<ChanelId, u128>` around *just* to be able to associate `PaymentForwarded` events with the channels otherwise tracked by `user_channel_id`.
.. as it's the same as `prev_channel_id` defined a few lines above.
d909a4e
to
ab4b872
Compare
Now pushed a minor fixup to the test: > git diff-tree -U2 a062317 ab4b872
diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
index 7319a28a0..0c8909b56 100644
--- a/lightning/src/ln/functional_test_utils.rs
+++ b/lightning/src/ln/functional_test_utils.rs
@@ -2250,9 +2250,20 @@ pub fn expect_payment_forwarded<CM: AChannelManager, H: NodeHolder<CM=CM>>(
// node's channel list
if !downstream_force_closed {
- assert!(node.node().list_channels().iter().any(|x|
- x.counterparty.node_id == next_node.node().get_our_node_id() &&
- x.channel_id == next_channel_id.unwrap() &&
- next_user_channel_id.map_or(true, |user_id| x.user_channel_id == user_id)
- ));
+ // As documented, `next_user_channel_id` will only be `Some` if we didn't settle via an
+ // onchain transaction, just as the `total_fee_earned_msat` field. Rather than
+ // introducing yet another variable, we use the latter's state as a flag to detect
+ // this and only check if it's `Some`.
+ if total_fee_earned_msat.is_none() {
+ assert!(node.node().list_channels().iter().any(|x|
+ x.counterparty.node_id == next_node.node().get_our_node_id() &&
+ x.channel_id == next_channel_id.unwrap()
+ ));
+ } else {
+ assert!(node.node().list_channels().iter().any(|x|
+ x.counterparty.node_id == next_node.node().get_our_node_id() &&
+ x.channel_id == next_channel_id.unwrap() &&
+ x.user_channel_id == next_user_channel_id.unwrap()
+ ));
+ }
}
assert_eq!(claim_from_onchain_tx, downstream_force_closed); |
a36b529
into
lightningdevkit:main
This is useful for users that track channels by
user_channel_id
.For example, in
lightning-liquidity
we currently keep a fullHashMap<ChanelId, u128>
around just to be able to associatePaymentForwarded
events with the channels otherwise tracked byuser_channel_id
.(cc @wvanlint, @johncantrell97)