Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove AvailableBalances::balance_msat #2476

Merged
merged 1 commit into from
Aug 15, 2023
Merged

Remove AvailableBalances::balance_msat #2476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion fuzz/src/router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,6 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
force_close_spend_delay: None,
is_outbound: true, is_channel_ready: true,
is_usable: true, is_public: true,
balance_msat: 0,
outbound_capacity_msat: capacity.saturating_mul(1000),
next_outbound_htlc_limit_msat: capacity.saturating_mul(1000),
next_outbound_htlc_minimum_msat: 0,
Expand Down
3 changes: 1 addition & 2 deletions lightning/src/chain/chainmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,8 +364,7 @@ where C::Target: chain::Filter,
/// claims which are awaiting confirmation.
///
/// Includes the balances from each [`ChannelMonitor`] *except* those included in
/// `ignored_channels`, allowing you to filter out balances from channels which are still open
/// (and whose balance should likely be pulled from the [`ChannelDetails`]).
/// `ignored_channels`.
///
/// See [`ChannelMonitor::get_claimable_balances`] for more details on the exact criteria for
/// inclusion in the return value.
Expand Down
11 changes: 0 additions & 11 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,6 @@ pub struct ChannelValueStat {
}

pub struct AvailableBalances {
/// The amount that would go to us if we close the channel, ignoring any on-chain fees.
pub balance_msat: u64,
Comment on lines -68 to -69
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove this, is there still a way to calculate our balance with msat precision since ChannelMonitor::get_claimable_balances will be rounded to sats?

Copy link
Contributor Author

@wvanlint wvanlint Aug 10, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not currently. With regard to AvailableBalances::balance_msat, its computation was very specific to a use case that is not applicable anymore and its value is likely not what is expected. For introspection at msat precision, perhaps Channel::value_to_self_msat and the pending HTLCs can be exposed? For the latter, see #2442.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps Channel::value_to_self_msat

Hm, perhaps. Will take a look at #2442, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, what would the use case be for getting a claimable balance in msat? on-chain doesn't have msats

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking just to get our channel balance in general, not necessarily on-chain

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay. It's nice to have a specific use case for a balance IMO, pretty vague term in lightning. Maybe something like that would be useful for accounting reasons, though 🤷‍♀️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea it's probably not a big deal to not have it for now

/// Total amount available for our counterparty to send to us.
pub inbound_capacity_msat: u64,
/// Total amount available for us to send to our counterparty.
Expand Down Expand Up @@ -1611,14 +1609,6 @@ impl<Signer: ChannelSigner> ChannelContext<Signer> {
let inbound_stats = context.get_inbound_pending_htlc_stats(None);
let outbound_stats = context.get_outbound_pending_htlc_stats(None);

let mut balance_msat = context.value_to_self_msat;
for ref htlc in context.pending_inbound_htlcs.iter() {
if let InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_)) = htlc.state {
balance_msat += htlc.amount_msat;
}
}
balance_msat -= outbound_stats.pending_htlcs_value_msat;

let outbound_capacity_msat = context.value_to_self_msat
.saturating_sub(outbound_stats.pending_htlcs_value_msat)
.saturating_sub(
Expand Down Expand Up @@ -1735,7 +1725,6 @@ impl<Signer: ChannelSigner> ChannelContext<Signer> {
outbound_capacity_msat,
next_outbound_htlc_limit_msat: available_capacity_msat,
next_outbound_htlc_minimum_msat,
balance_msat,
}
}

Expand Down
31 changes: 12 additions & 19 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1357,6 +1357,12 @@ pub struct ChannelCounterparty {
}

/// Details of a channel, as returned by [`ChannelManager::list_channels`] and [`ChannelManager::list_usable_channels`]
///
/// Balances of a channel are available through [`ChainMonitor::get_claimable_balances`] and
/// [`ChannelMonitor::get_claimable_balances`], calculated with respect to the corresponding on-chain
/// transactions.
///
/// [`ChainMonitor::get_claimable_balances`]: crate::chain::chainmonitor::ChainMonitor::get_claimable_balances
#[derive(Clone, Debug, PartialEq)]
pub struct ChannelDetails {
/// The channel's ID (prior to funding transaction generation, this is a random 32 bytes,
Expand Down Expand Up @@ -1432,24 +1438,11 @@ pub struct ChannelDetails {
///
/// This value will be `None` for objects serialized with LDK versions prior to 0.0.115.
pub feerate_sat_per_1000_weight: Option<u32>,
/// Our total balance. This is the amount we would get if we close the channel.
wvanlint marked this conversation as resolved.
Show resolved Hide resolved
/// This value is not exact. Due to various in-flight changes and feerate changes, exactly this
/// amount is not likely to be recoverable on close.
///
/// This does not include any pending HTLCs which are not yet fully resolved (and, thus, whose
/// balance is not available for inclusion in new outbound HTLCs). This further does not include
/// any pending outgoing HTLCs which are awaiting some other resolution to be sent.
/// This does not consider any on-chain fees.
///
/// See also [`ChannelDetails::outbound_capacity_msat`]
pub balance_msat: u64,
/// The available outbound capacity for sending HTLCs to the remote peer. This does not include
/// any pending HTLCs which are not yet fully resolved (and, thus, whose balance is not
/// available for inclusion in new outbound HTLCs). This further does not include any pending
/// outgoing HTLCs which are awaiting some other resolution to be sent.
///
/// See also [`ChannelDetails::balance_msat`]
///
/// This value is not exact. Due to various in-flight changes, feerate changes, and our
/// conflict-avoidance policy, exactly this amount is not likely to be spendable. However, we
/// should be able to spend nearly this amount.
Expand All @@ -1459,8 +1452,8 @@ pub struct ChannelDetails {
/// the current state and per-HTLC limit(s). This is intended for use when routing, allowing us
/// to use a limit as close as possible to the HTLC limit we can currently send.
///
/// See also [`ChannelDetails::next_outbound_htlc_minimum_msat`],
/// [`ChannelDetails::balance_msat`], and [`ChannelDetails::outbound_capacity_msat`].
/// See also [`ChannelDetails::next_outbound_htlc_minimum_msat`] and
/// [`ChannelDetails::outbound_capacity_msat`].
pub next_outbound_htlc_limit_msat: u64,
/// The minimum value for sending a single HTLC to the remote peer. This is the equivalent of
/// [`ChannelDetails::next_outbound_htlc_limit_msat`] but represents a lower-bound, rather than
Expand Down Expand Up @@ -1588,7 +1581,6 @@ impl ChannelDetails {
channel_value_satoshis: context.get_value_satoshis(),
feerate_sat_per_1000_weight: Some(context.get_feerate_sat_per_1000_weight()),
unspendable_punishment_reserve: to_self_reserve_satoshis,
balance_msat: balance.balance_msat,
inbound_capacity_msat: balance.inbound_capacity_msat,
outbound_capacity_msat: balance.outbound_capacity_msat,
next_outbound_htlc_limit_msat: balance.next_outbound_htlc_limit_msat,
Expand Down Expand Up @@ -7621,7 +7613,7 @@ impl Writeable for ChannelDetails {
(10, self.channel_value_satoshis, required),
(12, self.unspendable_punishment_reserve, option),
(14, user_channel_id_low, required),
(16, self.balance_msat, required),
(16, self.next_outbound_htlc_limit_msat, required), // Forwards compatibility for removed balance_msat field.
(18, self.outbound_capacity_msat, required),
(19, self.next_outbound_htlc_limit_msat, required),
(20, self.inbound_capacity_msat, required),
Expand Down Expand Up @@ -7657,7 +7649,7 @@ impl Readable for ChannelDetails {
(10, channel_value_satoshis, required),
(12, unspendable_punishment_reserve, option),
(14, user_channel_id_low, required),
(16, balance_msat, required),
(16, _balance_msat, option), // Backwards compatibility for removed balance_msat field.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

God rustc pisses me off for doing this - it just blindly picks a type (IIRC i64, so we're okay) and assumes that that's the type we want here. Its completely nonsense and results in code that does surprising things. Can you re-add the let _balance_msat: Option<u64> = _balance_msat below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks for catching this! Re-added the type hint.

(18, outbound_capacity_msat, required),
// Note that by the time we get past the required read above, outbound_capacity_msat will be
// filled in, so we can safely unwrap it here.
Expand All @@ -7683,6 +7675,8 @@ impl Readable for ChannelDetails {
let user_channel_id = user_channel_id_low as u128 +
((user_channel_id_high_opt.unwrap_or(0 as u64) as u128) << 64);

let _balance_msat: Option<u64> = _balance_msat;

Ok(Self {
inbound_scid_alias,
channel_id: channel_id.0.unwrap(),
Expand All @@ -7695,7 +7689,6 @@ impl Readable for ChannelDetails {
channel_value_satoshis: channel_value_satoshis.0.unwrap(),
unspendable_punishment_reserve,
user_channel_id,
balance_msat: balance_msat.0.unwrap(),
outbound_capacity_msat: outbound_capacity_msat.0.unwrap(),
next_outbound_htlc_limit_msat: next_outbound_htlc_limit_msat.0.unwrap(),
next_outbound_htlc_minimum_msat: next_outbound_htlc_minimum_msat.0.unwrap(),
Expand Down
2 changes: 0 additions & 2 deletions lightning/src/routing/router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2725,7 +2725,6 @@ mod tests {
inbound_scid_alias: None,
channel_value_satoshis: 0,
user_channel_id: 0,
balance_msat: 0,
outbound_capacity_msat,
next_outbound_htlc_limit_msat: outbound_capacity_msat,
next_outbound_htlc_minimum_msat: 0,
Expand Down Expand Up @@ -6794,7 +6793,6 @@ pub(crate) mod bench_utils {
outbound_scid_alias: None,
channel_value_satoshis: 10_000_000_000,
user_channel_id: 0,
balance_msat: 10_000_000_000,
outbound_capacity_msat: 10_000_000_000,
next_outbound_htlc_minimum_msat: 0,
next_outbound_htlc_limit_msat: 10_000_000_000,
Expand Down
1 change: 1 addition & 0 deletions pending_changelog/remove_balance_msat.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* The `AvailableBalances::balance_msat` field has been removed in favor of `ChannelMonitor::get_claimable_balances`. `ChannelDetails` serialized with versions of LDK >= 0.0.117 will have their `balance_msat` field set to `next_outbound_htlc_limit_msat` when read by versions of LDK prior to 0.0.117 (#2476).