[Custom Transactions] Add TxBuilder::get_next_commitment_stats
#3921
Conversation
|
👋 Thanks for assigning @carlaKC as a reviewer! |
|
We discussed earlier passing the entire list of HTLCs to So we prefer to add a single method that surfaces this limit to channel, and then let channel sort HTLCs depending on whether their value sits above or below that amount. As a result, builders of custom transactions will only have a say on where the dust limit sits, and won't be able to choose arbitrary subsets for dust and non-dust HTLCs. |
Is the main motivation for this to get all of the commitment-related logic out of
The key questions we're looking to answer seems to be "can I afford a commitment with this theoretical dust/nondust htlc"? This does seem to be something we could move into Sadly here's no getting around needing to know the dust limit if we want to clamp our capacity to that value, so we'd still need to surface
Tempting to suggest just adding an so tl;dr: I'd be interested in seeing what trying to pull more of the dust logic out into Meta note: This has got a lot of overlap with 3bb0586, so I think we should either:
|
I would say we focus on the latter for this PR, and let the former be the overarching goal :)
Let me know what you think of this new direction here. Still have some clunkiness to resolve, but that's what it's looking like right now.
|
Definitely let me rebase on top of your PR, you've rebased once already :) |
tankyleo
force-pushed
the
dust-limit
branch
2 times, most recently
from
12fd6c3 to
067bfa0
Compare
|
🔔 1st Reminder Hey @carlaKC! This PR has been waiting for your review. |
I like this approach! Definitely prefer being able to move a lot of the dust / second stage tx reasoning into the builder 👍 |
|
🔔 2nd Reminder Hey @carlaKC! This PR has been waiting for your review. |
tankyleo
changed the title
|
I'm aware I still owe a follow-up PR from the previous PR in this project, will push that soon in a separate PR :) |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
lightning/src/sign/tx_builder.rs
Outdated
| fn on_holder_tx_dust_exposure_msat( | ||
| &self, dust_buffer_feerate: u32, holder_dust_limit_satoshis: u64, | ||
| channel_type: &ChannelTypeFeatures, htlcs: &[HTLCAmountDirection], | ||
| ) -> u64; | ||
| fn on_counterparty_tx_dust_exposure_msat( |
There was a problem hiding this comment.
Not really sure I understand the point of these two methods if we have htlc_success_timeout_dust_limit as a method - if the API includes a method to say "hey, for an HTLC of type X what is the threshold where its dust", why bother with a method to say "given these HTLCs, how many are dust?", it seems the second can be calculated from the first, no? The same applies for passing the HTLC list to commit_tx_fee_sat.
Alternatively, we could drop the htlc_success_timeout_dust_limits method (if we want to not require there be some strict threshold to indicate when an HTLC is dust, though I think its a fine assumption to bake into the API, I dunno why someone would want to have some HTLCs be dust and others at the same value not be) and have a more generic "is HTLC dust on the next counterparty/local commitment transaction" call, but even there it seems like we don't need all of these methods.
There was a problem hiding this comment.
Sounds good yes I initially had a version of the API that matches what you describe. We wanted to see what it would look like if we moved more logic out of channel into TxBuilder, but I agree we only really need the dust limits.
I will clean up those methods, and add TxBuilder methods for 1) the dust limit 2) the htlc endogenous fees (to calculate counterparty dust exposure).
There was a problem hiding this comment.
I mean don't get me wrong, I'd love to move more logic out of channel.rs into TxBuilder, that would be great, but we also don't want to have an overlapping API where we have two ways to do things over the API. Also, we want to eventually make TxBuilder public, so it would be nice to avoid adding too much complexity (in the form of a ton of different methods).
There was a problem hiding this comment.
FWIW the real blocker on the more ambitious / complex API was the get_available_balances method - at the moment this method requires an exact dust limit above which all HTLCs are non-dust.
I have some code written that moves most of get_available_balances behind TxBuilder, but the complexity is not worth it, especially if we can make the assumption you describe above about the strict dust-vs-nondust threshold.
|
👋 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. |
tankyleo
changed the title
tankyleo
changed the title
TxBuilder set the HTLC dust limit
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3921 +/- ##
==========================================
+ Coverage 88.93% 88.95% +0.01%
==========================================
Files 174 174
Lines 123876 124763 +887
Branches 123876 124763 +887
==========================================
+ Hits 110173 110982 +809
- Misses 11250 11279 +29
- Partials 2453 2502 +49
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:
|
There was a problem hiding this comment.
for what it can be worth it I have reviewed 1eb1c9e and it looks correct to me.
As for my understanding on the reasoning for the change, motivation seems to be to move usage of second_stage_tx_fees_sat and htlc_tx_fees_sat to TxBuilder in order to abstract away usage of weight of htlc transactions away from channel.rs.
|
@carlaKC thank you for the review ! I addressed the feedback so far, see above. I also added commits so you have further context about how the API will fit with the rest of channel, but we can split these into a separate PR depending on preference. Big todo remaining is addressing Matt's feedback further above on which HTLC states we include when. |
|
Rebase: diff
|
Given a snapshot of the lightning state machine, `TxBuilder::get_next_commitment_stats` calculates the transaction fees, the dust exposure, and the holder and counterparty balances (the balances themselves do *not* account for the transaction fee).
In upcoming commits, these methods will serve as proxies to
`SpecTxBuilder::get_next_commitment_stats` in all validation of channel
updates in `ChannelContext`.
Eventually, these methods will completely replace
`get_pending_htlc_stats`, and
`get_next_{local, remote}_commit_tx_fee_msat`.
When predicting the HTLCs on next commitment, we take the conservative
approach and only assume that a HTLC will not be in the next commitment
when it is guaranteed that it won't be.
`ChannelContext::get_pending_htlc_stats` predicts that the set of HTLCs
on the next commitment will be all the HTLCs in
`ChannelContext.pending_inbound_htlcs`, and
`ChannelContext.pending_outbound_htlcs`, as well as all the outbound
HTLC adds in the holding cell.
This is an overestimate:
* Outbound HTLC removals which have been ACK'ed by the counterparty will
certainly not be present in any *next* commitment, even though they
remain in `pending_outbound_htlcs`.
* Outbound HTLCs in the `RemoteRemoved` state, will not be present in
the next *local* commitment.
* Outbound HTLCs in the `LocalAnnounced` state have no guarantee that
they were received by the counterparty before she sent the
`update_fee`.
* Outbound `update_add_htlc`'s in the holding cell are certainly not
known by the counterparty, and we will reevaluate their addition to
the channel when freeing the holding cell.
* Inbound HTLCs in the `LocalRemoved` state will not be present in the
next *remote* commitment.
`ChannelContext::next_local_commit_tx_fee_msat` over-counts outbound
HTLCs in the `LocalAnnounced` and `RemoteRemoved` states, as well as
outbound `update_add_htlc`'s in the holding cell.
`ChannelContext::next_remote_commit_tx_fee_msat` over-counts inbound
HTLCs in the `LocalRemoved` state, as well as outbound HTLCs in the
`LocalAnnounced` state.
This commit stops using these functions in favor of the newly added
`ChannelContext::get_next_{local, remote}_commitment_stats` methods, and
fixes the issues described above.
As a side-effect, this commit makes consistent the set of HTLCs used to
calculate dust exposure, transaction fees, and balances in
`validate_update_add_htlc`.
`ChannelContext::get_pending_htlc_stats` predicts that the set of HTLCs
on the next commitment will be all the HTLCs in
`ChannelContext.pending_inbound_htlcs`, and
`ChannelContext.pending_outbound_htlcs`, as well as all the outbound
HTLC adds in the holding cell.
This is an overestimate:
* Outbound HTLC removals which have been ACK'ed by the counterparty will
certainly not be present in any *next* commitment, even though they
remain in `pending_outbound_htlcs`.
* Outbound HTLCs in the `RemoteRemoved` state, will not be present in
the next *local* commitment.
* Outbound HTLCs in the `LocalAnnounced` state have no guarantee that
they were received by the counterparty before she sent the
`update_fee`.
* Outbound `update_add_htlc`'s in the holding cell are certainly not
known by the counterparty, and we will reevaluate their addition to
the channel when freeing the holding cell.
* Inbound HTLCs in the `LocalRemoved` state will not be present in the
next *remote* commitment.
This commit stops using `get_pending_htlc_stats` in favor of the newly
added `ChannelContext::get_next_{local, remote}_commitment_stats`
methods, and fixes the issues described above.
`ChannelContext::get_pending_htlc_stats` predicts that the set of HTLCs
on the next commitment will be all the HTLCs in
`ChannelContext.pending_inbound_htlcs`, and
`ChannelContext.pending_outbound_htlcs`, as well as all the outbound
HTLC adds in the holding cell.
This is an overestimate:
* Outbound HTLC removals which have been ACK'ed by the counterparty will
certainly not be present in any *next* commitment, even though they
remain in `pending_outbound_htlcs` (I refer to states
`AwaitingRemoteRevokeToRemove` and `AwaitingRemovedRemoteRevoke`).
* Outbound HTLCs in the `RemoteRemoved` state, will not be present in
the next *local* commitment.
* Inbound HTLCs in the `LocalRemoved` state will not be present in the
next *remote* commitment.
`ChannelContext::build_commitment_stats(funding, true, true, ..)` makes
these errors when predicting the HTLC count on the remote commitment:
* Inbound HTLCs in the state `RemoteAnnounced` are not included, but
they will be in the next remote commitment transaction if the local
ACK's the addition before producing the next remote commitment.
* Inbound HTLCs in the state `AwaitingRemoteRevokeToAnnounce` are not
included, even though the local has ACK'ed the addition.
* Outbound HTLCs in the state `AwaitingRemoteRevokeToRemove` are
counted, even though the local party has ACK'ed the removal.
This commit replaces these functions in favor of the newly added
`ChannelContext::get_next_{local, remote}_commitment_stats` methods,
and fixes the issues described above.
This commit also includes outbound HTLC additions in the holding cell in
the fees on the next remote commitment transaction; they were
previously not included.
`ChannelContext::get_pending_htlc_stats` predicts that the set of HTLCs
on the next commitment will be all the HTLCs in
`ChannelContext.pending_inbound_htlcs`, and
`ChannelContext.pending_outbound_htlcs`, as well as all the outbound
HTLC adds in the holding cell.
This is an overestimate:
* Outbound HTLC removals which have been ACK'ed by the counterparty will
certainly not be present in any *next* commitment, even though they
remain in `pending_outbound_htlcs`.
* Outbound HTLCs in the `RemoteRemoved` state, will not be present in
the next *local* commitment.
* Outbound HTLCs in the `LocalAnnounced` state have no guarantee that
they were yet received by the counterparty.
* Outbound `update_add_htlc`'s in the holding cell are certainly not
known by the counterparty, and we will reevaluate their addition to
the channel when freeing the holding cell.
* Inbound HTLCs in the `LocalRemoved` state will not be present in the
next *remote* commitment.
This commit stops using `get_pending_htlc_stats` in favor of the newly
added `ChannelContext::get_next_{local, remote}_commitment_stats`
methods, and fixes the issues described above.
The value of `remote_balance_before_fee_msat` remains *exactly*
the same.
`ChannelContext::next_remote_commit_tx_fee_msat` counts inbound HTLCs in
the `LocalRemoved` state, as well as outbound HTLCs in the
`LocalAnnounced` state. We now do not count them for the same reasons
described above.
Finally, we now always check holder dust exposure, whereas we previously
would only do it if the incoming HTLC was dust on our own commitment
transaction.
Anytime we build a (feerate, HTLC-set, fee) pair, cache it, and check that the fee matches if the feerate and HTLC-set match when building a commitment transaction. The fee in the old caches are never actually checked in the current test suite; both of the new cached fees are checked in ~200 tests.
The cached fee is never checked in the current test suite.
|
Rebase: full diff
|
|
@carlaKC I believe the PR is ready for a deeper look :) Can't quite figure out why CI fails to build, it does pass on my machine. I thought we could focus on the first two commits for now, and we push the validation / prediction commits to the next PR in this series. Let me know what you'd prefer. I've pushed the validation / prediction commits here too so you have the context. Also see their commit messages for further description of the changes. |
The predicted fee is only a function of the predicted feerate, and the predicted number of nondust HTLCs. If the feerate and the number of nondust HTLCs match, the fee should match too. This significantly increases the hit rate of the assertion that the predicted fee was correct.
panic if value_to_holder_msat is greater than channel value satoshis
derive copy on `PredictedNextFee`
trim unnecessary if else expression
So far we have always panicked if the total value of HTLCs is greater than the payer's balance; we continue to do so here. Also adjust `subtract_addl_outputs` for clarity
|
@carlaKC A correction: In the b16779a commit message, I say "This commit also includes outbound HTLC additions in the holding cell in the fees on the next remote commitment transaction; they were previously not included." This is wrong, they are already included via the additional HTLC parameter of |
|
🔔 1st Reminder Hey @carlaKC! This PR has been waiting for your review. |
|
I also plan to expand the commit messages to describe in detail how we now read the feerates we use to calculate the next commitment stats. |
There was a problem hiding this comment.
Reviewed first two commits - API looks good!
I don't have strong feelings on how to split this up - happy to separate API/usage or do both in one go. @tankyleo lmk your preference and I'll review accordingly.
I'm ready for this to get a rebase/squash so it can pick up some minor relevant V3 channel changes.
| let excess_feerate_opt = | ||
| feerate_per_kw.checked_sub(dust_exposure_limiting_feerate.unwrap_or(0)); |
There was a problem hiding this comment.
Do we need to consider pending_update_fee here?
There was a problem hiding this comment.
Here's the current plan: if there is a pending_update_fee, that feerate gets set to the feerate_per_kw parameter of get_next_commitment_stats. I assume if there is a pending_update_fee, we will want to know commitment transaction fees with pending_update_fee as the feerate, not self.feerate_per_kw. Similar for dust exposure, we will want to increase pending_update_fee by some factor to get the dust_buffer_feerate.
| /// We take the conservative approach and only assume that a HTLC will | ||
| /// not be in the next commitment when it is guaranteed that it won't be. | ||
| #[allow(dead_code)] | ||
| #[rustfmt::skip] |
There was a problem hiding this comment.
nit: don't skip formatting on new code?
There was a problem hiding this comment.
On my machine rustfmt is not consistent when formatting get_next_commitment_htlcs and get_next_commitment_value_to_self_msat, and also makes a big mess of get_next_commitment_value_to_self_msat :)
| &self, local: bool, htlc_candidate: Option<HTLCAmountDirection>, include_counterparty_unknown_htlcs: bool, | ||
| ) -> Vec<HTLCAmountDirection> { | ||
| let mut commitment_htlcs = Vec::with_capacity( | ||
| 1 + self.pending_inbound_htlcs.len() |
There was a problem hiding this comment.
Is the 1 here for htlc_candidate?
Maybe htlc_candidate.is_some() as usize instead if so?
There was a problem hiding this comment.
Yes ! The idea here is to allocate the max number of HTLCs that can be written to commitment_htlcs - for an Option that's 1, no need for a runtime is_some check ?
| // `LocalRemoved` HTLCs will certainly not be present on any future remote | ||
| // commitments, but they could be in a future local commitment as the remote has | ||
| // not yet acknowledged the removal. | ||
| let pending_inbound_htlcs = self |
There was a problem hiding this comment.
Directly extend commitment_htlcs here and below? Allows us to simplify the include_counterparty_unknown_htlcs if statement to just push holding_cell_htlcs when we need them.
There was a problem hiding this comment.
Since this will be a very hot path, I want to pay particular attention to performance / optimization opportunities - based on intuition a single call to extend with a chained iterator is more optimal than repeated calls to extend with the constituent iterators. WDYT ?
Cleanup `HTLCAmountDirection::is_dust`
Return a single `NextCommitmentStats`
Rename variable
and