Expose channel_reserve_satoshis via InboundChannelFunds::PushMsat#4319
Expose channel_reserve_satoshis via InboundChannelFunds::PushMsat#4319yashbhutwala wants to merge 2 commits intolightningdevkit:mainfrom
channel_reserve_satoshis via InboundChannelFunds::PushMsat#4319Conversation
|
👋 I see @tnull 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.05% 86.20% +0.15%
==========================================
Files 156 156
Lines 103384 103392 +8
Branches 103384 103392 +8
==========================================
+ Hits 88964 89130 +166
+ Misses 11905 11745 -160
- Partials 2515 2517 +2
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
lightning/src/ln/channelmanager.rs
Outdated
| 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.
lightning/src/ln/msgs.rs
Outdated
| /// 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?
There was a problem hiding this comment.
A question for @tnull :
OpenChannelV2drops both thechannel_reserve_satoshis, andpush_msatfields. For thepush_msatsfield, we currently handle this transition in theOpenChannelRequestevent with anInboundChannelFundsenum, withPushMsat(u64)as the V1 variant, andDualFundedas the V2 variant.
Yeah, IMO makes sense to keep it consistent (and eventually drop both once we can deprecate v1^^).
There was a problem hiding this comment.
Yea dont see why it should be a separate field?
There was a problem hiding this comment.
Agreed — moved channel_reserve_satoshis out of ChannelParameters and into InboundChannelFunds::PushMsat as a struct variant:
pub enum InboundChannelFunds {
PushMsat {
push_msat: u64,
channel_reserve_satoshis: u64,
},
DualFunded,
}This keeps ChannelParameters free of V1-specific fields, and the V1/V2 distinction is handled entirely by the enum — consistent with how push_msat already differentiates. Also restored CommonOpenChannelFields::channel_parameters() and removed the OpenChannelMessageRef::channel_parameters() helper.
@TheBlueMatt @tnull would appreciate a re-review when you get a chance!
There was a problem hiding this comment.
Conceptually makes sense to me to have such an enum (without having had a closer look at the code), but it is a bit odd to have channel_reserve_satoshis under 'inbound channel funds' as the reserve is ofc not an 'inbound fund'. Not sure what a better naming scheme would be here though.
|
🔔 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. |
yashbhutwala
force-pushed
the
expose-channel-reserve-satoshis
branch
from
31ebde8 to
e137705
Compare
|
🔔 2nd 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
e137705 to
1ba60a2
Compare
…dChannelFunds` Per reviewer feedback, `channel_reserve_satoshis` is better placed in the `InboundChannelFunds` enum rather than as a separate `Option<u64>` on `ChannelParameters`. This is consistent with how V1 vs V2 channel differences are already handled via the enum. - Convert `InboundChannelFunds::PushMsat` from a tuple variant to a struct variant with `push_msat` and `channel_reserve_satoshis` fields - Remove `channel_reserve_satoshis` field from `ChannelParameters` - Restore `CommonOpenChannelFields::channel_parameters()` method - Remove `OpenChannelMessageRef::channel_parameters()` helper - Update tests to check reserve from the enum variant Co-Authored-By: Claude (claude.ai)
yashbhutwala
changed the title
channel_reserve_satoshis via ChannelParameterschannel_reserve_satoshis via InboundChannelFunds::PushMsat
yashbhutwala
requested review from
TheBlueMatt,
tankyleo,
tnull and
valentinewallace
|
🔔 1st Reminder Hey @TheBlueMatt @valentinewallace @tankyleo! This PR has been waiting for your review. |
2 similar comments
|
🔔 1st Reminder Hey @TheBlueMatt @valentinewallace @tankyleo! This PR has been waiting for your review. |
|
🔔 1st Reminder Hey @TheBlueMatt @valentinewallace @tankyleo! This PR has been waiting for your review. |
6 participants
Summary
Expose the
channel_reserve_satoshisvalue from V1open_channelmessages to users handlingEvent::OpenChannelRequest.InboundChannelFunds::PushMsatfrom a tuple variant to a struct variant carrying bothpush_msatandchannel_reserve_satoshisCommonOpenChannelFields::channel_parameters()(keepingChannelParametersfree of V1-specific fields)Fixes #3909
Revives the closed PR #3910 with the approach agreed upon by reviewers (@tnull, @TheBlueMatt): fold the reserve into
InboundChannelFundsrather than adding anOption<u64>toChannelParameters.Test plan
cargo check -p lightningpassescargo +1.75.0 test -p lightning --lib)test_open_channel_request_channel_reserve_satoshisvalidates V1 channels correctly populate the fieldtest_open_channel_request_channel_reserve_satoshis_v2validates V2 channels useDualFundedvariantchannel_open_testspass🤖 Generated with Claude Code