feat: expose channel_reserve_satoshis via ChannelParameters
#3910
Conversation
|
👋 I see @wpaulino was un-assigned. |
Prabhat1308
force-pushed
the
probot/expose_csr
branch
from
2933e6a to
8332a7a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3910 +/- ##
=======================================
Coverage 88.94% 88.94%
=======================================
Files 174 174
Lines 124201 124179 -22
Branches 124201 124179 -22
=======================================
- Hits 110472 110453 -19
- Misses 11251 11252 +1
+ Partials 2478 2474 -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:
|
lightning/src/ln/msgs.rs
Outdated
| @@ -3072,6 +3078,7 @@ impl LengthReadable for OpenChannelV2 { | |||
| let dust_limit_satoshis: u64 = Readable::read(r)?; | |||
| let max_htlc_value_in_flight_msat: u64 = Readable::read(r)?; | |||
| let htlc_minimum_msat: u64 = Readable::read(r)?; | |||
| let channel_reserve_satoshis: u64 = Readable::read(r)?; | |||
There was a problem hiding this comment.
We can't do this, as OpenChannelV2 doesn't have that field, see https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#the-open_channel2-message
It's also not a 'common' field per se as per spec:
Note that open_channel's channel_reserve_satoshi has been omitted. Instead, the channel reserve is fixed at 1% of the total channel balance (open_channel2.funding_satoshis + accept_channel2.funding_satoshis) rounded down to the nearest whole satoshi or the dust_limit_satoshis, whichever is greater.
There was a problem hiding this comment.
The best bet would be to have a new top-level field in OpenChannelRequest for channel_reserve_satoshis and then set that in channelmanager like:
channel_reserve_satoshis: match msg {
OpenChannelMessageRef::V1(msg) => msg.channel_reserve_satoshis,
OpenChannelMessageRef::V2(msg) => get_v2_channel_reserve_satoshis(
channel_value_satoshis, msg.common_fields.dust_limit_satoshis
);
},The get_v2_channel_reserve_satoshis exists in the channel module. You'd need to make that pub(super).
This would be a breaking change and also would need docs and shouldn't be backported.
There was a problem hiding this comment.
The best bet would be to have a new top-level field in
OpenChannelRequest
Hmm, while that would be pretty easy, I'm not super happy about exposing such a detail at the top level API, especially if we have sub-structs in-place. can't we reuse ChannelParameters still, but apply above rules for v2 channels? Or, maybe it would be the right time to refactor OpenChannelRequest (or ChannelParameters) to discern between V1 and V2 channels?
There was a problem hiding this comment.
Yeah you could reuse ChannelParameters and just have the field there, update the doc comment for it as it currently mentions being a subset of the common fields. It will just need to be initialised as 0 in CommonOpenChannelFields::channel_parameters and then set like above when creating the OpenChannelRequest event. I'm not sure an Option is worth the effort as it'll always end up being Some() when accessed by the user.
Refactoring is fine, but we'd want to still have a single OpenChannelRequest event.
There was a problem hiding this comment.
I think still at the current state putting the field in ChannelParameters would be a violation of the spec ? Given that if we include it in ChannelParameters it would still be under CommonOpenChannelFields and hence in both OpenChannel v1 and v2 even if we put a dummy value like 0 in the beginning . I think we might be looking at a refactor here if we want to use ChannelParameters .
There was a problem hiding this comment.
I'm confused, why can't we just include it in ChannelParameters and move the CommonOpenChannelFields::channel_parameters implementation to OpenChannelMessageRef or even OpenChannelV1/V2, so that we can use the different values based on the type? Also, users can already tell from the OpenChannelRequest if it's dual-funded or not via channel_negotiation_type.
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
Prabhat1308
force-pushed
the
probot/expose_csr
branch
from
8332a7a to
d52cb88
Compare
| .commitment_feerate_sat_per_1000_weight, | ||
| to_self_delay: self.common_fields.to_self_delay, | ||
| max_accepted_htlcs: self.common_fields.max_accepted_htlcs, | ||
| channel_reserve_satoshis: None, // V2 doesn't have this field |
There was a problem hiding this comment.
Hmm, I think this is confusing, as there will be a channel reserve, albeit a hard-coded one. I think we'd need to calculate that 1% value here, no?
There was a problem hiding this comment.
Note that this would only be calculable once we know the acceptor's funding_satoshis as it is based off the combined initiator and acceptor funding_satoshis.
There was a problem hiding this comment.
I think I might be missing something . I have a few questions on this.
- initiator sends OpenChannelV2 and then Acceptor sends AcceptChannelv2. If this is the case , we would never know the acceptor's
funding_satoshisand hence this will always be a None (meaning we dont know yet instead of it does not exist for OpenChannelV2). - both the handle functions for the channels (open/accept) says that dual-funding is not supported. Even if I get the value of acceptor's
funding_satoshis, I am not getting a expected response but an error ?
exposes CSR in ChannelParameters and moves CommonOpenChannelFields::channel_parameters() to OpenChannelV1/V2::channel_parameters()
Prabhat1308
force-pushed
the
probot/expose_csr
branch
from
d52cb88 to
a2e79a1
Compare
fixes #3909
exposes
channel_reserve_satoshisby adding it toChannelParametersandCommonAcceptChannelFieldsfor the user to ascertain the value set by the counterparty