[Custom Transactions] Add TxBuilder::get_available_balances#4026
[Custom Transactions] Add TxBuilder::get_available_balances#4026TheBlueMatt merged 14 commits intolightningdevkit:mainfrom
TxBuilder::get_available_balances#4026Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
@TheBlueMatt would appreciate a high-level design pass on this one thank you. I largely just move |
lightning/src/sign/tx_builder.rs
Outdated
| fn get_available_balances( | ||
| &self, is_outbound_from_holder: bool, channel_value_satoshis: u64, | ||
| value_to_holder_msat: u64, pending_htlcs: &[HTLCAmountDirection], feerate_per_kw: u32, | ||
| dust_exposure_limiting_feerate: Option<u32>, max_dust_htlc_exposure_msat: u64, | ||
| holder_channel_constraints: ChannelConstraints, | ||
| counterparty_channel_constraints: ChannelConstraints, channel_type: &ChannelTypeFeatures, | ||
| ) -> crate::ln::channel::AvailableBalances; | ||
| fn get_next_commitment_stats( | ||
| &self, local: bool, is_outbound_from_holder: bool, channel_value_satoshis: u64, | ||
| value_to_holder_msat: u64, next_commitment_htlcs: &[HTLCAmountDirection], | ||
| addl_nondust_htlc_count: usize, feerate_per_kw: u32, | ||
| dust_exposure_limiting_feerate: Option<u32>, broadcaster_dust_limit_satoshis: u64, | ||
| channel_type: &ChannelTypeFeatures, | ||
| ) -> NextCommitmentStats; |
There was a problem hiding this comment.
I'm thinking we'll want to merge these two into one :)
There was a problem hiding this comment.
I feel like we could, yea?
There was a problem hiding this comment.
Current blocker for me is that get_available_balances is a function of both local and remote commitments, but will dig further thank you :)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4026 +/- ##
==========================================
+ Coverage 85.86% 85.94% +0.08%
==========================================
Files 159 159
Lines 104302 104644 +342
Branches 104302 104644 +342
==========================================
+ Hits 89558 89939 +381
+ Misses 12246 12197 -49
- Partials 2498 2508 +10
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:
|
TheBlueMatt
left a comment
There was a problem hiding this comment.
I'm confused, I was under the impression we were going to pause the custom tx builder work for a month or two after landing #3921?
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
Agreed yes this doesn't have to land anytime soon - I still wanted to get this PR out of my head and written before I stop |
TheBlueMatt
left a comment
There was a problem hiding this comment.
API looks good to me, but, indeed, next_commitment_stats and available_balances feel like two things that are just close enough that they might as well be merged, no?
lightning/src/sign/tx_builder.rs
Outdated
| holder_channel_constraints.dust_limit_satoshis, | ||
| channel_type, | ||
| ) | ||
| // TODO: should `get_available_balances` be fallible ? |
There was a problem hiding this comment.
Hmmm, could definitely go either way. I do like the idea of merging get_next_commitment_stats and get_available_balances, at which point we'd want the joint thing to be fallible, though.
7d0a143 to
26ea1e0
Compare
lightning/src/ln/channel.rs
Outdated
| self.get_holder_channel_constraints(funding), | ||
| self.get_counterparty_channel_constraints(funding), | ||
| funding.get_channel_type(), | ||
| ).unwrap().available_balances |
There was a problem hiding this comment.
If this direction is good I'll bubble any errors here upstream
There was a problem hiding this comment.
Yea, I think this makes sense. Since its intended as a public trait, I wonder if we shouldn't, in debug_assertions follow this immediately with a commit tx build step with a single additional HTLC paying the max stated value and assert that it is accepted.
There was a problem hiding this comment.
Since its intended as a public trait, I wonder if we shouldn't, in debug_assertions follow this immediately with a commit tx build step with a single additional HTLC paying the max stated value and assert that it is accepted.
done below thanks
Yea, I think this makes sense.
take a look at the last commit below: c0e97e3
TLDR:
Instead of making this call fallible and potentially force closing on all of our public list_channels calls,
I would rather make a convincing argument for why this call will never error, and in case we do error in prod, return balances at 0.
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
TheBlueMatt
left a comment
There was a problem hiding this comment.
Didn't carefully review the code changes, but the API basically LGTM.
lightning/src/ln/channel.rs
Outdated
| } | ||
|
|
||
| let to_remote_satoshis = remote_balance_before_fee_msat / 1000 - commit_tx_fee_sat; | ||
| let holder_channel_constraints = ChannelConstraints { |
There was a problem hiding this comment.
Rather than having all this logic explicitly written out here (and in the outbound constructor), can we actually build the Channel object first and then try calling get_next_local_commitment_stats (or whatever) on it and fail if it fails rather than returning the build Channel?
There was a problem hiding this comment.
addressed below thank you
lightning/src/ln/channel.rs
Outdated
| include_counterparty_unknown_htlcs, | ||
| ); | ||
| let inbound_htlcs_count = | ||
| next_remote_commitment_htlcs.iter().filter(|htlc| !htlc.outbound).count(); |
There was a problem hiding this comment.
If we're gonna stop returning the HTLC count in NextCommitmentStats, should we start returning the HTLC set from get_next_commitment_htlcs so we don't have to fetch it twice?
lightning/src/ln/channel.rs
Outdated
| self.get_holder_channel_constraints(funding), | ||
| self.get_counterparty_channel_constraints(funding), | ||
| funding.get_channel_type(), | ||
| ).unwrap().available_balances |
There was a problem hiding this comment.
Yea, I think this makes sense. Since its intended as a public trait, I wonder if we shouldn't, in debug_assertions follow this immediately with a commit tx build step with a single additional HTLC paying the max stated value and assert that it is accepted.
TheBlueMatt
left a comment
There was a problem hiding this comment.
Didn't carefully review the code changes, but the API basically LGTM.
This commit has no functional changes.
This commit moves the previous `TxBuilder::get_next_commitment_stats` method to a private function, and then calls this function in `TxBuilder::get_channel_stats`. Similar to the previous `TxBuilder::get_next_commitment_stats` method, `TxBuilder::get_channel_stats` fails if any party cannot afford the HTLCs outbound from said party, and the anchors if they are the funder. Aside from the API changes on `TxBuilder`, there are no functional changes in this commit.
Move calls to `TxBuilder::commit_tx_fee_sat` in
`new_for_inbound_channel` and `new_for_outbound_channel` to
`ChannelContext::get_next_{*}_commitment_stats`, and set the parameters
such that the exact same behavior is maintained.
We also replace calls to `TxBuilder::commit_tx_fee_sat` in
`get_pending_htlc_stats`, `next_local_commit_tx_fee_msat`, and
`next_remote_commit_tx_fee_msat` with `chan_utils::commit_tx_fee_sat`.
All three functions get deleted in an upcoming commit, so we accept
this temporary use of the `chan_utils::commit_tx_fee_sat` function.
We make temporary use of the raw `tx_builder::saturating_sub_anchor_outputs` function in `get_available_balances_for_scope`. This ok because we move most of the `get_available_balances_for_scope` function to the `TxBuilder::get_channel_stats` call in an upcoming commit. Again, no functional change is introduced in this commit.
1298bf7 to
da9a1c1
Compare
|
I pushed some tags on the fork repo to help track the fixup-squash-rebase progression: Then da9a1c1 / HEAD here is a tiny diff from |
|
Thanks @TheBlueMatt I consolidated into a single |
lightning/src/ln/channel.rs
Outdated
| .expect("At least one FundingScope is always provided") | ||
| } | ||
|
|
||
| #[rustfmt::skip] |
There was a problem hiding this comment.
nit: don't put rustfmt::skip on new code?
lightning/src/ln/channel.rs
Outdated
| } | ||
|
|
||
| #[rustfmt::skip] | ||
| pub(super) fn get_available_balances<F: FeeEstimator>( |
There was a problem hiding this comment.
We've got 4x methods across two types for getting balances, and the respective get_available_balances calls are each only used once in the codebase. Perhaps we could remove them and just allow the callers to handle the unwrap, since it would be around the same amount of code?
lightning/src/ln/channel.rs
Outdated
| holder_channel_reserve_satoshis: funding | ||
| .counterparty_selected_channel_reserve_satoshis | ||
| .unwrap_or(0), | ||
| counterparty_dust_limit_satoshis: self.counterparty_dust_limit_satoshis, | ||
| counterparty_channel_reserve_satoshis: funding.holder_selected_channel_reserve_satoshis, |
There was a problem hiding this comment.
nit: I do quite like the _selected pattern that's common in the codebase because I personally find it a bit less confusing.
non-blocking because this has seen a few rounds!
TheBlueMatt
left a comment
There was a problem hiding this comment.
LGTM, feel free to squash the new fixup with @carlaKC's suggestions.
In an upcoming commit, we move `get_available_balances_for_scope` behind `TxBuilder::get_channel_stats`, and pass channel parameters relevant to balance calculations in `TxBuilder::get_channel_stats` via `ChannelConstraints`. There are no functional changes in this commit.
This snippet is currently used in `tx_builder::get_next_commitent_stats`, and will be used in an upcoming commit in `get_available_balances_for_scope`. There are no functional changes in this commit, as the `extra_accepted_htlc_dust_exposure` member of `NextCommitmentStats` was not used.
We no longer make use of `get_pending_htlc_stats`, `get_dust_buffer_feerate`, `next_local_commit_tx_fee_msat`, and `next_remote_commit_tx_fee_msat` in the `channel` module, and instead make use of tooling from the `tx_builder` module. `HTLCStats::pending_outbound_htlcs` and `HTLCStats::pending_outbound_htlcs_value_msat` are now calculated in `get_available_balances_for_scope`, and do not include outbound HTLCs in states `AwaitingRemoteRevokeToRemove` and `AwaitingRemovedRemoteRevoke`. `HTLCStats::pending_inbound_htlcs_value_msat` is now calculated in `get_available_balances_for_scope`, and does not include inbound HTLCs in state `LocalRemoved`. To determine whether a HTLC is dust for the purpose of calculating total dust exposure, we now refer only to `ChannelContext::feerate_per_kw`, and ignore any upcoming fee updates stored in `pending_update_fee`. The same applies for dust exposure due to excess fees; we ignore any fee updates in `ChannelContext::pending_update_fee`, and only refer to `ChannelContext::feerate_per_kw`. For outbound feerate updates, this is ok because all such updates first get placed in the holding cell. We validate dust exposure again upon freeing the feerate update from the holding cell, and immediately generate the corresponding commitment. For inbound feerate updates, it is possible that the peer sends us a feerate update that is in excess of our dust exposure limiting feerate, at the same time that we send non-dust HTLCs that exhaust the max dust exposure at the new feerate. This leads to a channel force-close when the peer sends us their commitment signed including the HTLCs and the new feerate. Similar to the `HTLCStats` members above, when calculating dust exposure on both holder and counterparty transactions in `get_available_balances_for_scope`, we now do not include inbound HTLCs in states `LocalRemoved`, and outbound HTLCs in states `AwaitingRemoteRevokeToRemove` and `AwaitingRemovedRemoteRevoke`. In the case where `is_outbound_from_holder` is true, `max_reserved_commit_tx_fee_msat` and `min_reserved_commit_tx_fee_msat` now do not include pending inbound HTLCs in state `LocalRemoved`. In the case where `is_outbound_from_holder` is false, `max_reserved_commit_tx_fee_msat` now also includes outbound HTLCs in the holding cell, and does not include inbound HTLCs in state `LocalRemoved`. These fee values are also the result of the feerate getting multiplied by the fee spike buffer increase multiple, instead of the final commitment transaction fee getting multiplied by that multiple. This results in higher values, as we multiply before the rounding down to the nearest satoshi. This reduces the set of HTLC additions we would send. Finally, these values also account for any non-dust HTLCs that transition to dust at the higher feerate, resulting in lower values. This increases the set of HTLC additions we would send, and previous versions of LDK will fail only the single HTLC and not the channel in case we breach their buffer.
This is a direct code move to `tx_builder::get_available_balances`.
We choose to multiply `FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE` by the feerate when checking the fee spike buffer in `can_accept_incoming_htlc` instead of multiplying the multiple by the commitment transaction fee. This allows us to delete `NextCommitmentStats::commit_tx_fee_sat`, and return balances including the commitment transaction fee in `TxBuilder::get_channel_stats`. This unblocks a good amount of cleanup. Note that this means LDK now rejects HTLCs that previous versions of LDK would have accepted. We made the mirroring change in `get_available_balances_for_scope` a few commits earlier. We also now account for non-dust HTLCs turning to dust at the multiplied feerate, decreasing the overall weight of the transaction. We also remove other fields in `NextCommitmentStats` which can be easily calculated in `channel` only. `TxBuilder::get_channel_stats` could also check the reserve requirements, given that it gets the reserves in `ChannelConstraints`. I leave this to follow-up work.
Note that `AvailableBalances` will always refer to the holder's balances, even when `local` is set to `false`, when calling `TxBuilder::get_channel_stats`.
`get_available_balances_for_scope` only errors if some party in the channel cannot afford the HTLCs outbound from said party, and the anchors and transaction fee if they are the funder. We do not account for the channel reserve here, so this error should be exceedingly rare, but could nonetheless happen due to concurrent updates on the channel's state. The upcoming zero-reserve channel type could also make this case more reachable. `send_htlc` maps such an error to its own error type since it proposes an update to the channel's state. The other callers only read the channel's state, so it would not be a good fit to have them return an error too. Hence, we choose to let these callers panic in debug mode, and return saturated values in release mode. Note that we now handle the if-we-removed-it-already-but-haven't -fully-resolved-they-can-still-send-an-inbound-HTLC case, as `LocalRemoved` HTLCs are considered resolved when calculating `AvailableBalances`. We update the documentation accordingly.
TheBlueMatt
left a comment
There was a problem hiding this comment.
Thanks! Just gonna land this since the only changes since @carlaKC's ACK were addressing her feedback in trivial changes.
|
Post merge ACK - thanks for the last few changes! |
Depends on #3921