Expose channel_reserve_satoshis via ChannelParameters
#4319
Conversation
|
👋 I see @valentinewallace was un-assigned. |
yashbhutwala
force-pushed
the
expose-channel-reserve-satoshis
branch
from
cd475ee to
2f585af
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4319 +/- ##
==========================================
+ Coverage 86.09% 86.25% +0.16%
==========================================
Files 156 156
Lines 102804 102809 +5
Branches 102804 102809 +5
==========================================
+ Hits 88508 88679 +171
+ Misses 11788 11618 -170
- Partials 2508 2512 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @valentinewallace @tankyleo! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @valentinewallace @tankyleo! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @valentinewallace @tankyleo! This PR has been waiting for your review. |
1 similar comment
|
🔔 2nd Reminder Hey @valentinewallace @tankyleo! This PR has been waiting for your review. |
|
@tnull @dunxen @TheBlueMatt please review 🙏 |
|
🔔 3rd Reminder Hey @valentinewallace @tankyleo! This PR has been waiting for your review. |
1 similar comment
|
🔔 3rd Reminder Hey @valentinewallace @tankyleo! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @valentinewallace @tankyleo! This PR has been waiting for your review. |
1 similar comment
|
🔔 4th Reminder Hey @valentinewallace @tankyleo! This PR has been waiting for your review. |
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
Basically looks good. Probably good if @tankyleo takes a look since he's working on channel reserve stuff
yashbhutwala
force-pushed
the
expose-channel-reserve-satoshis
branch
2 times, most recently
from
e8cabf3 to
48a70a8
Compare
tankyleo
left a comment
tankyleo
left a comment
There was a problem hiding this comment.
Thanks for the PR excuse the delay on my side ! I'll ping tnull as well since he is a consumer of this API and opened the issue in the first place
| pub(super) fn channel_parameters(&self) -> msgs::ChannelParameters { | ||
| let (common_fields, channel_reserve_satoshis) = match self { | ||
| Self::V1(msg) => (&msg.common_fields, Some(msg.channel_reserve_satoshis)), | ||
| Self::V2(msg) => (&msg.common_fields, None), |
There was a problem hiding this comment.
The test suite still passes if I set this to Some(0), let's add coverage for the V2 case.
There was a problem hiding this comment.
Done! Added test_open_channel_request_channel_reserve_satoshis_v2 which verifies that channel_reserve_satoshis is None for V2 (dual-funded) channels. This ensures the V2 code path is properly covered.
| // For V1 channels, channel_reserve_satoshis should be Some with the value from the message | ||
| assert_eq!( | ||
| params.channel_reserve_satoshis, | ||
| Some(expected_reserve), |
There was a problem hiding this comment.
As described elsewhere, let's also add coverage for the V2 case
There was a problem hiding this comment.
Done! The new V2 test is added alongside the V1 test.
lightning/src/ln/msgs.rs
Outdated
| /// The minimum value unencumbered by HTLCs for the counterparty to keep in the channel. | ||
| /// | ||
| /// For V1 channels (`open_channel`), this is the explicit `channel_reserve_satoshis` value | ||
| /// from the counterparty. |
There was a problem hiding this comment.
As described above, this is a subset of CommonOpenChannelFields, and the channel_reserve_satoshis of the OpenChannel message actually sets the minimum value that the non-channel-initiator must keep in the channel, and not the counterparty (as long as we define the counterparty here to be the sender of the OpenChannel message).
There was a problem hiding this comment.
Good catch! Fixed the documentation to clarify that:
- The reserve is for the "non-channel-initiator" to keep (not "counterparty")
- The value comes "from the channel initiator" (not "from the counterparty")
This makes it clear that from the OpenChannelRequest perspective (where we're the non-initiator receiving the message), the initiator is specifying what reserve we must maintain.
| /// the final reserve value cannot be determined at that point. | ||
| /// | ||
| /// [`Event::OpenChannelRequest`]: crate::events::Event::OpenChannelRequest | ||
| pub channel_reserve_satoshis: Option<u64>, |
There was a problem hiding this comment.
A question for @tnull : OpenChannelV2 drops both the channel_reserve_satoshis, and push_msat fields. For the push_msats field, we currently handle this transition in the OpenChannelRequest event with an InboundChannelFunds enum, with PushMsat(u64) as the V1 variant, and DualFunded as the V2 variant.
We also note for InboundChannelFunds that it "allows the differentiation between a request for a dual-funded and non-dual-funded channel."
Seems to me now we can also differentiate V1 vs V2 based on the channel_reserve_satohis field.
To keep things consistent, would it be worth moving push_msats to ChannelParameters too, and set it to None in the V2 case, like we are doing here for channel_reserve_satoshis ?
There was a problem hiding this comment.
This is a good point about consistency. I'll leave this design question for @tnull to weigh in on, as it would be a larger change to consolidate push_msats into ChannelParameters as well.
There was a problem hiding this comment.
I kinda feel like we should include a value here for v2 that is the "reserve if you don't contribute" value and call it like likely_channel_reserve_satoshis? Otherwise it just feels weird that we have a value here that we expect to eventually be useless?
There was a problem hiding this comment.
Thanks for the suggestion @TheBlueMatt! To clarify - would you prefer:
- Rename to
likely_channel_reserve_satoshisand populate it for V2 withmax(1% of initiator_funding, dust_limit), or - Keep a separate field/approach?
Also, does this address @tankyleo's earlier question about push_msats consistency, or should we still consider consolidating that into ChannelParameters as well?
|
🔔 1st Reminder Hey @tnull @valentinewallace! This PR has been waiting for your review. |
yashbhutwala
force-pushed
the
expose-channel-reserve-satoshis
branch
from
9423f93 to
31ebde8
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
Add `channel_reserve_satoshis` field to `ChannelParameters` struct to expose the channel reserve value in `OpenChannelRequest` events. This allows users handling inbound channel requests to see the reserve requirement. For V1 channels, this is the explicit value from the `open_channel` message. For V2 (dual-funded) channels, this is `None` since the reserve is calculated based on total channel value which isn't known until both parties contribute. Also implements `channel_parameters()` on `OpenChannelMessageRef` to DRY up the code, as suggested in review.
yashbhutwala
force-pushed
the
expose-channel-reserve-satoshis
branch
from
31ebde8 to
e137705
Compare
5 participants
Summary
channel_reserve_satoshis: Option<u64>field toChannelParametersstructSome(value)with the explicit reserve fromopen_channelmessageNonesince reserve is calculated from total channel value (initiator + acceptor funding), which isn't known atOpenChannelRequesttimechannel_parameters()fromCommonOpenChannelFieldsto individualOpenChannelandOpenChannelV2impls to handle V1/V2 difference correctlytest_open_channel_request_channel_reserve_satoshisto verify the implementationFixes #3909
Revives the closed PR #3910 with the approach suggested by reviewers:
Option<u64>instead of trying to read from wire for V2 (which doesn't have this field)channel_parameters()toOpenChannel/OpenChannelV2Test plan
cargo check -p lightningpassescargo test -p lightning --lib)test_open_channel_request_channel_reserve_satoshisvalidates V1 channels correctly populate the fieldchannel_open_testspass🤖 Generated with Claude Code