-
Notifications
You must be signed in to change notification settings - Fork 411
Don't generate a commitment if we cannot afford a holding cell feerate #3828
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
184efad
to
413e895
Compare
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:
|
e4f2142
to
cc03f48
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Duh, yea, okay, can you update the commit message to clarify that? The way I read the commit message it implied that we would be sending an update with HTLCs, just not a fee update. Otherwise LGTM
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.
f8b74a7
to
d693047
Compare
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, sorry, thought it was the same node, was reading too fast :)
While an
update_fee
is 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.