Don't generate a commitment if we cannot afford a holding cell feerate #3828
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
tankyleo
force-pushed
the
lonely-commitment
branch
from
184efad to
413e895
Compare
tankyleo
force-pushed
the
lonely-commitment
branch
3 times, most recently
from
d0a4441 to
b2a166c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3828 +/- ##
==========================================
+ Coverage 89.90% 90.36% +0.46%
==========================================
Files 160 160
Lines 129272 133275 +4003
Branches 129272 133275 +4003
==========================================
+ Hits 116218 120432 +4214
+ Misses 10360 10142 -218
- Partials 2694 2701 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tankyleo
force-pushed
the
lonely-commitment
branch
2 times, most recently
from
e4f2142 to
cc03f48
Compare
tankyleo
force-pushed
the
lonely-commitment
branch
from
cc03f48 to
d94822c
Compare
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
👋 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.
I'm confused, why does this "violate the specification"? Our peer has no idea we had this update_fee queued, we never sent it to them AFAIU. It makes sense to me that we should want to send the commitment even if we can't update the fee, we definitely shouldn't sit on HTLCs!
The problem is that currently the code makes it possible to send a commitment with zero updates, in the case we cannot afford the new feerate and there are no pending HTLC updates. |
tankyleo
force-pushed
the
lonely-commitment
branch
from
d94822c to
f8b74a7
Compare
Thanks I agree it was not clear, I gave it another shot here hope this works :) |
Upon releasing an `update_fee` from its holding cell, it can be dropped and not sent to the peer in case we no longer can afford the new feerate. When this happens, we previously still sent a commitment to the peer, which could break the spec if no other updates were sent to the peer. Now, when a fee update gets dropped from its holding cell, we also do not produce a commitment if no other updates are to be sent to the peer.
tankyleo
force-pushed
the
lonely-commitment
branch
from
f8b74a7 to
d693047
Compare
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
There was a problem hiding this comment.
Thanks. LGTM, No changes since @wpaulino ACK'd, so landing.
| get_route_and_payment_hash!(nodes[1], nodes[0], 5000 * 1000); | ||
| let onion = RecipientOnionFields::secret_only(payment_secret); | ||
| let id = PaymentId(payment_hash.0); | ||
| nodes[1].node.send_payment_with_route(route, payment_hash, onion, id).unwrap(); |
There was a problem hiding this comment.
Probably we should be rejecting this (ie considering holding-cell fee updates when we decide if we can afford a new outbound HTLC).
There was a problem hiding this comment.
You mean an inbound HTLC I think ? Is that a little too strict given that our peer had no idea we had a fee update in the holding cell when it sent the update_add_htlc ?
There was a problem hiding this comment.
Oops, sorry, thought it was the same node, was reading too fast :)
While an
update_feeis in the holding cell, it is possible for HTLCs to get added to the commitment transaction such that when we release the holding cell, we can no longer afford this new feerate.In that case, we previously would drop the fee update, but still send a commitment (at the old feerate), which is a break of the specification.
We now stop generating this lonely commitment when the fee update gets dropped upon release from the holding cell.