expose `feerate_per_kw` in `ChannelDetails` #2094

johncantrell97 · 2023-03-09T22:04:21Z

Wanted to be able to monitor this for channels with disconnected peers. Closes #2093

codecov-commenter · 2023-03-09T22:42:02Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.11 🎉

Comparison is base (36834b3) 91.14% compared to head (d76b43f) 91.25%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2094      +/-   ##
==========================================
+ Coverage   91.14%   91.25%   +0.11%     
==========================================
  Files         101      101              
  Lines       48887    51613    +2726     
  Branches    48887    51613    +2726     
==========================================
+ Hits        44557    47101    +2544     
- Misses       4330     4512     +182

Impacted Files	Coverage Δ
lightning/src/ln/functional_test_utils.rs	`93.79% <ø> (+1.28%)`	⬆️
lightning/src/routing/router.rs	`94.82% <ø> (+0.08%)`	⬆️
lightning/src/ln/channel.rs	`89.83% <100.00%> (+0.47%)`	⬆️
lightning/src/ln/channelmanager.rs	`91.19% <100.00%> (+2.32%)`	⬆️

... and 9 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

tnull

Looks good! Would suggest to give the full unit in the field name, see below.

tnull · 2023-03-10T12:31:57Z

lightning/src/ln/channelmanager.rs

@@ -1118,6 +1118,10 @@ pub struct ChannelDetails {
 	/// inbound. This may be zero for inbound channels serialized with LDK versions prior to
 	/// 0.0.113.
 	pub user_channel_id: u128,
+	/// The currently negotiated feerate per 1000-weight-unit applied to commitment and htlc txs


Suggested change

/// The currently negotiated feerate per 1000-weight-unit applied to commitment and htlc txs

/// The currently negotiated fee rate denominated in satoshi per 1000 weight units, which is applied to commitment and HTLC transactions.

tnull · 2023-03-10T12:38:00Z

lightning/src/ln/channelmanager.rs

+	/// The currently negotiated feerate per 1000-weight-unit applied to commitment and htlc txs
+	///
+	/// This value will be `None` for objects serialized with LDK versions prior to 0.0.115.
+	pub feerate_per_kw: Option<u32>,


To avoid any uncertainty and to align this a bit more with the FeeEstimator interface, could we name this feerate_sat_per_kw or feerate_sat_per_1000_weight?

Yeah, I prefer the latter. I find kw a little confusing if it's been a while since I've looked at fee rate units.

Hm, did just notice that feerate_per_kw is used for HolderSignedTx, CommitmentTransaction, and in a few other function calls. It's not really related to this change but perhaps a global replacement makes sense.

Mh, I'm not sure if this warrants a lot of git noise, so we may just want to change it here and in related parts of the public API to feerate_sat_per_kw or similar.

tnull · 2023-03-10T12:40:40Z

lightning/src/ln/channelmanager.rs

@@ -1260,6 +1264,7 @@ impl ChannelDetails {
 			outbound_scid_alias: if channel.is_usable() { Some(channel.outbound_scid_alias()) } else { None },
 			inbound_scid_alias: channel.latest_inbound_scid_alias(),
 			channel_value_satoshis: channel.get_value_satoshis(),
+			feerate_per_kw: Some(channel.get_feerate()),


Maybe it would be to include a tiny refactor while you're here and rename Channel::get_feerate to get_feerate_sat_per_kw?

dunxen · 2023-03-10T14:39:24Z

lightning/src/routing/router.rs

@@ -2208,6 +2208,7 @@ mod tests {
 			inbound_htlc_minimum_msat: None,
 			inbound_htlc_maximum_msat: None,
 			config: None,
+			feerate_sat_per_1000_weight: None


nit: Maybe very nit picky, but should it not be "sats"?

I don't think so, as it's FeeEstimator::get_est_sat_per_1000_weight already and the unit symbol is sat (cf. https://bitcoin.design/guide/designing-products/units-and-symbols/).

Right for unit symbol makes sense, also didn't remember existing usage

@johncantrell97 On that note, as you're already touching ChannelManager, you could also rename the target_feerate_sats_per_1000_weight argument of ChannelManager::close_channel_with_target_feerate to target_feerate_sat_per_1000_weight .

I'm confused, it's plural, the design guide linked says explicitly plural is "sats"? Not like it matters tho.

Oh my, you're actually right. I read the guide and seem to have switched it around in my head. I still think units should be singular (you wouldn't use "USDs" or "BTCs"), but will now shut up and eat my words on the basis of the guide I linked to myself. 🤦‍♂️

i have no hard opinion on this to be honest ¯_(ツ)_/¯

I prefer sats but am now seeing that it's indeed already mixed usage in the code base so maybe it's a future cleanup that is needed?

dunxen · 2023-03-13T21:56:19Z

The field name just needs an update

error[E0560]: struct `ChannelDetails` has no field named `feerate_per_kw`
    --> lightning/src/routing/router.rs:5741:4
     |
5741 |             feerate_per_kw: None,
     |             ^^^^^^^^^^^^^^ `ChannelDetails` does not have this field

renames Channel::get_feerate to get_feerate_sat_per_1000_weight

johncantrell97 force-pushed the expose-fee branch 2 times, most recently from f5f774c to 63d3be8 Compare March 9, 2023 22:34

tnull reviewed Mar 10, 2023

View reviewed changes

johncantrell97 force-pushed the expose-fee branch from 63d3be8 to e26dc51 Compare March 10, 2023 14:29

dunxen previously approved these changes Mar 10, 2023

View reviewed changes

expose feerate_sat_per_1000_weight in ChannelDetails

d76b43f

renames Channel::get_feerate to get_feerate_sat_per_1000_weight

johncantrell97 dismissed dunxen’s stale review via d76b43f March 15, 2023 17:36

johncantrell97 force-pushed the expose-fee branch from e26dc51 to d76b43f Compare March 15, 2023 17:36

TheBlueMatt approved these changes Mar 15, 2023

View reviewed changes

TheBlueMatt assigned tnull and dunxen Mar 15, 2023

dunxen approved these changes Mar 15, 2023

View reviewed changes

TheBlueMatt merged commit 12d799e into lightningdevkit:main Mar 15, 2023
14 checks passed

johncantrell97 deleted the expose-fee branch March 15, 2023 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expose `feerate_per_kw` in `ChannelDetails` #2094

expose `feerate_per_kw` in `ChannelDetails` #2094

johncantrell97 commented Mar 9, 2023

codecov-commenter commented Mar 9, 2023 •

edited

tnull left a comment

tnull Mar 10, 2023 •

edited

tnull Mar 10, 2023

johncantrell97 Mar 10, 2023

johncantrell97 Mar 10, 2023 •

edited

tnull Mar 10, 2023

tnull Mar 10, 2023

dunxen Mar 10, 2023

tnull Mar 10, 2023

dunxen Mar 10, 2023 •

edited

tnull Mar 11, 2023

TheBlueMatt Mar 13, 2023

tnull Mar 13, 2023 •

edited

dunxen Mar 13, 2023

johncantrell97 Mar 15, 2023

dunxen commented Mar 13, 2023

	/// The currently negotiated feerate per 1000-weight-unit applied to commitment and htlc txs
	/// The currently negotiated fee rate denominated in satoshi per 1000 weight units, which is applied to commitment and HTLC transactions.

expose feerate_per_kw in ChannelDetails #2094

expose feerate_per_kw in ChannelDetails #2094

Conversation

johncantrell97 commented Mar 9, 2023

codecov-commenter commented Mar 9, 2023 • edited

Codecov Report

tnull left a comment

Choose a reason for hiding this comment

tnull Mar 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johncantrell97 Mar 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunxen Mar 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Mar 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunxen commented Mar 13, 2023

expose `feerate_per_kw` in `ChannelDetails` #2094

expose `feerate_per_kw` in `ChannelDetails` #2094

codecov-commenter commented Mar 9, 2023 •

edited

tnull Mar 10, 2023 •

edited

johncantrell97 Mar 10, 2023 •

edited

dunxen Mar 10, 2023 •

edited

tnull Mar 13, 2023 •

edited