Remaining follow-ups from 3921 #4032
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
tankyleo
force-pushed
the
2025-08-fallible-stats
branch
from
3142f12 to
ff95850
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4032 +/- ##
==========================================
- Coverage 88.76% 88.73% -0.04%
==========================================
Files 176 176
Lines 129518 129492 -26
Branches 129518 129492 -26
==========================================
- Hits 114968 114902 -66
- Misses 11945 11973 +28
- Partials 2605 2617 +12
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:
|
lightning/src/ln/channel.rs
Outdated
| msg.feerate_per_kw, | ||
| dust_exposure_limiting_feerate, | ||
| ) | ||
| .expect("Updating the fee should never exhaust the balance after HTLCs and anchors"); |
There was a problem hiding this comment.
IMO the tx builder should fail these - if the commitment transaction cannot be built because its nonsense, it shouldn't return stats, it should fail. Also, generally, we shouldn't be expecting these calls at all IMO - if a different tx builder has different logic for deciding when something is bogus, we should let it decide that and fail rather than declaring that it must always accept.
There was a problem hiding this comment.
IMO the tx builder should fail these - if the commitment transaction cannot be built because its nonsense, it shouldn't return stats, it should fail.
Not sure I understand, isn't that what's happening already ?
Also, generally, we shouldn't be expecting these calls at all IMO - if a different tx builder has different logic for deciding when something is bogus, we should let it decide that and fail rather than declaring that it must always accept.
Done, see commit below
There was a problem hiding this comment.
IMO the tx builder should fail these - if the commitment transaction cannot be built because its nonsense, it shouldn't return stats, it should fail.
Not sure I understand, isn't that what's happening already ?
Ah do you mean that it should also fail if the tx fee exhausts the balance ? Right now we fail such cases on commitment_signed; we would now fail these on receiving update_fee.
We can leave this for after 0.2 I'm thinking ?
There was a problem hiding this comment.
Ah do you mean that it should also fail if the tx fee exhausts the balance ? Right now we fail such cases on commitment_signed; we would now fail these on receiving update_fee.
Hmm, good point. Can you remind me why we validate anything at all in validate_update_fee? When we actually get a commitment_signed we'll validate what we need there (well, we should, not sure if we do), and I'm not entirely sure why we started validating it in the update_fee handler. I recall something about fee updates causing us to send counterparty commitment sigs that overflowed things, but that shouldn't matter - we won't include the new fee in counterparty commitment transactions until the peer sends their cs anyway.
We can leave this for after 0.2 I'm thinking ?
We could, but it seems like an easy change that would further reduce the code complexity in channel.rs.
There was a problem hiding this comment.
@TheBlueMatt Some guessing below, will be digging further:
- Right now we validate dust exposure in
validate_update_feeand "can you afford the new fee" invalidate_commitment_signed - Previously only the
build_commitment_transactioncall would allow us to determine if counterparty can afford the fee. - We now have these
commitment_statsmethods, so we could totally move this "can you afford proposed fee" check fromvalidate_commitment_signedtovalidate_update_fee. validate_update_feedoes validate dust exposure because dust exposure stats are easily provided byget_pending_htlc_statswithout building a full commitment transaction.
I will attempt to clean all this up now thank you.
There was a problem hiding this comment.
Right, so I think we should remove validate_update_fee entirely and just validate in validate_commitment_signed. The spec says we have to allow an update_fee that overdraws the balance as long as they claim an HTLC before they call commitment_signed, and I don't see a reason why we shouldn't just do it all there.
There was a problem hiding this comment.
@TheBlueMatt one more thing: the spec says we should check dust exposure on receiving update_fee not on receiving commitment_signed:
Should we move dust exposure checks to validate_commitment_signed too ?
|
👋 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. |
There was a problem hiding this comment.
Responded to the pending comment but ultimately this is gonna need rebase on #4011
|
Needs rebase now, then should be good to go 🎉 |
We now do not require that `channel_value_satoshis * 1000` is greater than or equal to `value_to_holder_msat`.
While these HTLCs may currently be unknown to our counterparty, they can end up in commitments soon. Moreover, we are considering failing a single HTLC here, not the entire channel, so we opt to be conservative in what we accept to forward.
While these fee updates may currently be unknown to our counterparty, they can end up in commitments soon. Moreover, we are considering failing a single HTLC here, not the entire channel, so we opt to be conservative in what we accept to forward.
We sometimes do not have easy access to the `dust_exposure_limiting_feerate`, yet we are still interested in basic stats on commitments like balances and transaction fees. So we relax the requirement that the `dust_exposure_limiting_feerate` is always set when `feerate_per_kw` is not 0.
tankyleo
force-pushed
the
2025-08-fallible-stats
branch
from
5db254a to
0f6234c
Compare
|
@TheBlueMatt take a look at that last commit. Since we have to validate dust exposure on receiving |
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
lightning/src/ln/channel.rs
Outdated
| @@ -4336,7 +4302,7 @@ where | |||
| dust_exposure_limiting_feerate, | |||
| self.holder_dust_limit_satoshis, | |||
| funding.get_channel_type(), | |||
| ); | |||
| )?; | |||
There was a problem hiding this comment.
Hmm, it does feel very weird that we can fail in a test context but not in a prod context. Can we either unwrap here or handle the error "gracefully"?
| @@ -4553,6 +4516,18 @@ where | |||
| "Balance after HTLCs and anchors exhausted on local commitment", | |||
| )) | |||
| })?; | |||
|
|
|||
| next_local_commitment_stats | |||
There was a problem hiding this comment.
I'm confused by this commit, I thought we discussed going the other way - removing validate_update_fee entirely and only validating in commitment_signed not doing any validation when we receive an update_fee? The spec mandates (IIRC) this, basically - a node is allowed to send an update_fee that overdraws its remaining funds as long as it also then claims an HTLC in an update_fulfill_htlc message before it sends a commitment_signed, so rejecting early would reject perfectly valid fee updates.
There was a problem hiding this comment.
@TheBlueMatt What about dust exposure ? Seems spec says we should validate this on update_fee, not on commitment_signed ?
There was a problem hiding this comment.
I think that's wrong? Like, we don't actually care whether the new feerate would suffice until it's actually applied to a commitment transaction, and in fact we always care about it when applied to a new commitment transaction.
The spec may be saying that with an eye towards our ability to disconnect to "reject" an update fee by not seeing the CS, but that never works in practice because peers will send their CS and can't build a new one. In practice this just means a reconnect loop instead of a channel closure.
There was a problem hiding this comment.
Sounds good thank you i'll delete validate_update_fee and move all checks to commitment_signed
We allow a peer to send an `update_fee` message that pushes the dust exposure over our max as long as they send HTLC updates that bring the dust exposure back down under the limit before they send `commitment_signed`.
tankyleo
force-pushed
the
2025-08-fallible-stats
branch
from
0f6234c to
ff85846
Compare
tankyleo
force-pushed
the
2025-08-fallible-stats
branch
from
ff85846 to
6c01026
Compare
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
See commit messages