Open and accept zero reserve channels#4428
Draft
tankyleo wants to merge 22 commits intolightningdevkit:mainfrom
Draft
Open and accept zero reserve channels#4428tankyleo wants to merge 22 commits intolightningdevkit:mainfrom
tankyleo wants to merge 22 commits intolightningdevkit:mainfrom
Conversation
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_onchain_stats`. Similar to the previous `TxBuilder::get_next_commitment_stats` method, `TxBuilder::get_onchain_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_onchain_stats` call in an upcoming commit. Again, no functional change is introduced in this commit.
In an upcoming commit, we move `get_available_balances_for_scope` behind `TxBuilder::get_onchain_stats`, and pass channel parameters relevant to balance calculations in `TxBuilder::get_onchain_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_onchain_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_onchain_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_onchain_stats`.
We can unwrap `get_available_balances` when not counting counterparty unknown HTLCs because all balance changes in channel are validated with the same `get_next_remote_commitment_stats` call before getting applied to the channel's state. Now calls to `get_available_balances` external to `ChannelContext` will only be updated to include HTLCs in the holding cell, and `LocalAnnounced` HTLCs once we receive the ack from the counterparty for these updates. `FundedChannel::send_htlc` includes these HTLCs when calculating available balances, so it will always have the latest view of the channel's state and can fail graciously in case some balance has been overdrawn.
|
👋 Hi! I see this is a draft PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1801
Based on top of #4026, starting commit is cc9de95
Public API changes: 1104cb2 and 1789a29