Send update_add_htlc messages after HTLC removal messages#4086
Send update_add_htlc messages after HTLC removal messages#4086TheBlueMatt merged 1 commit intolightningdevkit:mainfrom
update_add_htlc messages after HTLC removal messages#4086Conversation
While nodes are generally supposed to validate commitment transactions after the `commitent_signed` and not while HTLCs are being added/removed, we don't. This can make a commitment update where we use HTLC balance claimed with a fulfill to send new HTLCs, which is perfectly valid, being rejected. While we shouldn't currently generate any such commitments, we might want to in the future, and on the off-chance that we do, or where such a commitment would result in a dust threshold overrun, its always safter to add new HTLCs to a commitment only after we've removed any HTLCs we're going to remove, which we do here.
|
I've assigned @tankyleo as a reviewer! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4086 +/- ##
=======================================
Coverage 88.60% 88.61%
=======================================
Files 176 176
Lines 132086 132070 -16
Branches 132086 132070 -16
=======================================
- Hits 117041 117032 -9
+ Misses 12376 12368 -8
- Partials 2669 2670 +1
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:
|
| self.enqueue_message(&mut *peer, msg); | ||
| } | ||
| for msg in update_add_htlcs { | ||
| self.enqueue_message(&mut *peer, msg); |
There was a problem hiding this comment.
I haven't verified this, but for this to work I assume we would have to consider the balance of those already-claimed-but-not-yet-committed HTLCs as part of our sendable balance when we attempt to send the new HTLC (via send_htlc) in the first place, no?
There was a problem hiding this comment.
For us to take advantage of it, yes. But there's also a possibility we hit some non-spec limit a peer has, eg a dust exposure limit, which this marginally reduces the chance of as well.
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
Just gonna land this, its painfully trivial |
4 participants
While nodes are generally supposed to validate commitment transactions after the
commitent_signedand not while HTLCs are being added/removed, we don't. This can make a commitment update where we use HTLC balance claimed with a fulfill to send new HTLCs, which is perfectly valid, being rejected. While we shouldn't currently generate any such commitments, we might want to in the future, and on the off-chance that we do, or where such a commitment would result in a dust threshold overrun, its always safter to add new HTLCs to a commitment only after we've removed any HTLCs we're going to remove, which we do here.