[Custom Transactions] Add TxBuilder::get_next_commitment_stats
#3921
Conversation
|
👋 Thanks for assigning @TheBlueMatt 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.70% -0.24%
==========================================
Files 174 174
Lines 123876 124502 +626
Branches 123876 124502 +626
==========================================
+ Hits 110173 110434 +261
- Misses 11250 11586 +336
- Partials 2453 2482 +29
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.
|
@TheBlueMatt did you have a chance to look at the latest commit here ? It has the API cleanup, thank you ! |
There was a problem hiding this comment.
Discussed it offline, but I'm somewhat skeptical of a design that adds too many more methods on TxBuilder. We should consider exploring options that keep TxBuilder much smaller with more logic in larger methods (that we're willing to call and throw away some of the result of, eg relying more on something like that looks like build_commitment_stats to figure out dust exposures and whether an htlc is acceptable).
|
@TheBlueMatt would appreciate a look here :)
|
tankyleo
changed the title
TxBuilder set the HTLC dust limit
lightning/src/sign/tx_builder.rs
Outdated
| htlcs | ||
| .iter() | ||
| .filter_map(|htlc| { | ||
| htlc.is_dust(true, dust_buffer_feerate, holder_dust_limit_satoshis, channel_type) |
There was a problem hiding this comment.
Shouldn't this only count sent HTLCs? If our counterparty adds a dust HTLC it doesnt impact how much of our money might be burned to dust
There was a problem hiding this comment.
hmmm inbound HTLCs are your money once the preimage is revealed, so you do care how many of them go to dust ?
There was a problem hiding this comment.
Yea, I guess it depends on the context, if we're deciding if we should accept the HTLC (vs fail it back) it is dust (cause it might become our money), if we're deciding if the commitment transaction is safe to accept its not dust (cause we'll fail the HTLC). Kinda annoying to capture...
There was a problem hiding this comment.
let me see the only two times we currently look at on_holder_tx_dust_exposure_msat when validating counterparty updates is in validate_update_fee (called when receiving update_fee), and fn can_accept_incoming_htlc (called when the HTLC is already Committed).
if we're deciding if we should accept the HTLC (vs fail it back) it is dust (cause it might become our money),
Do I understand correct this is fn can_accept_incoming_htlc ?
if we're deciding if the commitment transaction is safe to accept its not dust (cause we'll fail the HTLC).
Were you referring to the fn validate_commitment_signed function ? Currently we don't validate dust exposure there
lightning/src/ln/channel.rs
Outdated
| use OutboundHTLCState::*; | ||
| let pending_outbound_htlcs = self.pending_outbound_htlcs.iter().filter_map( | ||
| |OutboundHTLCOutput { ref state, amount_msat, .. }| { | ||
| matches!(state, LocalAnnounced { .. } | Committed | RemoteRemoved { .. }) |
There was a problem hiding this comment.
This needs to depend on who generated the commitment we're talking about - if we're being called in, eg, validate_update_add_htlc, we're looking at a commitment the counterparty generated, which shouldn't include any LocalAnnounced or RemoteRemoved HTLCs, whereas if we're being called from can_send_update_fee we can't assume we'll wait until the counterparty sends a CS, so we need those.
There was a problem hiding this comment.
For LocalAnnounced HTLCs, we could do
(1) -> update_add_htlc `LocalAnnounced`
(2) <- update_add_htlc `validate_update_add`
-> commitment_signed
<- revoke_and_ack
(3) <- commitment_signed
-> revoke_and_ack
-> commitment_signed
<- revoke_and_ack
At (3), (1) is present on the local commitment transaction (generated by the counterparty), so seems reasonable to count it on the commitments at (2) ?
As for RemoteRemoved HTLCs, these for sure won't be on the next local commitment transaction, but might still be present on the next remote commitment transaction depending on the ordering of the CS...
So far, I have attempted to imitate fn next_local_commit_tx_fee_msat and fn next_remote_commit_tx_fee_msat as much as possible, will review all this tomorrow thank you.
There was a problem hiding this comment.
Right, but an alternative situation is
(1) -> update_add_htlc `LocalAnnounced`
(2) <- update_add_htlc `validate_update_add`
-> commitment_signed
(3) <- commitment_signed
<- revoke_and_ack
-> revoke_and_ack
-> commitment_signed
<- revoke_and_ack
Now at (3), because its before the received RAA, the HTLC is not present in the commitment transactions. Indeed, it will be included in some counterparty commitment transaction in the future eventually, but it definitely isn't in this one, and it seems weird to consider it because that will cause us to think a commitment tx is "invalid" (as in protocol-invalid-they-overdrew-reserves) when its not.
There was a problem hiding this comment.
Right so at (2), you would rather err on the side of considering HTLCs not present ? So far I've erred on the side of assuming they will be present (which is the current behavior I believe)
lightning/src/ln/channel.rs
Outdated
| self.pending_outbound_htlcs | ||
| .iter() | ||
| .filter_map(|htlc| { | ||
| matches!( |
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.
While we previously took into account HTLCs in the holding cell when
validating channel updates, we now choose to ignore them; our
counterparty certainly does not know about them, and we will re-validate
their addition to the channel upon freeing the holding cell.
tankyleo
changed the title
TxBuilder::get_next_commitment_stats
| #[cfg(any(test, fuzzing))] | ||
| #[derive(Clone, PartialEq, PartialOrd, Eq, Ord)] |
There was a problem hiding this comment.
| #[cfg(any(test, fuzzing))] | |
| #[derive(Clone, PartialEq, PartialOrd, Eq, Ord)] | |
| #[cfg_attr(any(test, fuzzing), derive(Clone, PartialEq, PartialOrd, Eq, Ord))] |
lightning/src/sign/tx_builder.rs
Outdated
| htlcs | ||
| .iter() | ||
| .filter_map(|htlc| { | ||
| htlc.is_dust(true, dust_buffer_feerate, holder_dust_limit_satoshis, channel_type) |
There was a problem hiding this comment.
Yea, I guess it depends on the context, if we're deciding if we should accept the HTLC (vs fail it back) it is dust (cause it might become our money), if we're deciding if the commitment transaction is safe to accept its not dust (cause we'll fail the HTLC). Kinda annoying to capture...
lightning/src/ln/channel.rs
Outdated
| use OutboundHTLCState::*; | ||
| let pending_outbound_htlcs = self.pending_outbound_htlcs.iter().filter_map( | ||
| |OutboundHTLCOutput { ref state, amount_msat, .. }| { | ||
| matches!(state, LocalAnnounced { .. } | Committed | RemoteRemoved { .. }) |
There was a problem hiding this comment.
Right, but an alternative situation is
(1) -> update_add_htlc `LocalAnnounced`
(2) <- update_add_htlc `validate_update_add`
-> commitment_signed
(3) <- commitment_signed
<- revoke_and_ack
-> revoke_and_ack
-> commitment_signed
<- revoke_and_ack
Now at (3), because its before the received RAA, the HTLC is not present in the commitment transactions. Indeed, it will be included in some counterparty commitment transaction in the future eventually, but it definitely isn't in this one, and it seems weird to consider it because that will cause us to think a commitment tx is "invalid" (as in protocol-invalid-they-overdrew-reserves) when its not.
| }) | ||
| .map(|&OutboundHTLCOutput { amount_msat, .. }| HTLCAmountDirection { outbound: true, amount_msat }); | ||
|
|
||
| // We do not include holding cell HTLCs, we will validate them upon freeing the holding cell... |
There was a problem hiding this comment.
Presumably we need the option to do this, though, since we want to check a theoretical "next commitment if we included the entire holding cell" before we accept something new into the holding cell, to make sure we aren't just accepting it to fail it later.
and