Fix holding cell freeing in case we fail to add some HTLC #295
Conversation
TheBlueMatt
force-pushed
the
2019-01-holding-cell-limits
branch
2 times, most recently
from
0616f46 to
81142ac
Compare
TheBlueMatt
force-pushed
the
2019-01-holding-cell-limits
branch
2 times, most recently
from
b377d06 to
d0275f8
Compare
|
Rebased. |
| @@ -1863,6 +1872,11 @@ impl Channel { | |||
| } | |||
| if err.is_some() { | |||
| self.holding_cell_htlc_updates.push(htlc_update); | |||
| if let Some(ChannelError::Ignore(_)) = err { | |||
There was a problem hiding this comment.
Your test don't check that right ? Because commenting out this part, it's still a success. So new requirement "if we went to free the holding cell HTLC updates, and adding one failed as we hit our outbound HTLC limit (or in-flight value limit), we would not send a commitment_signed" is not covered by test ?
There was a problem hiding this comment.
Yea, its not covered by the test, I'm not actually sure how to hit it after the other fixes, but if you dont have it and you do get a failure you will see the chanmon_fail_consistency test fail due to broken consistency (also if you only include this fix and comment out the "Cannot push more than their max accepted HTLCs" check, the test fails).
There was a problem hiding this comment.
Yes try to hit it but seems hard to me because you will ever hit fix in get_outbound_pending_htlc_states while waiting for remote RAA and at handling this one you will flush the whole in a strait. You weren't able to backport the failure from chanmon_fail_consistency ? (try to test but hit failure assert from rust-secp on 1.29.2). Wonder if the fail come from an AddHTLC and not other cases..
There was a problem hiding this comment.
The test case here is (essentially) the backport from the fuzz test, but it won't actually hit because of the changes to considering holding cell when adding. Note that if you comment those changes out and a few bits of the test you can test this.
There was a problem hiding this comment.
Yes but because fix in get_outbound_pending_htlc_stats, are we sure that the one in free_holding_cell_htlcs is still relevant? As you said you don't know how to hit it and me too
There was a problem hiding this comment.
No, I'm not, but it cant hurt :p (and I know its possible to hit with update_fee stuff, but hopefully that goes away before we have to care).
TheBlueMatt
force-pushed
the
2019-01-holding-cell-limits
branch
from
d0275f8 to
9e31403
Compare
|
Also see #295 (review), seems to me some errors msgs in send_htlc are wrong. |
|
Oh and maybe confusing "And then refuse to add things to our holding cell once we reach our limits considering the holding cell" isn't this TODO L3186 in channel.rs ? |
|
As for the error messages being confused, who's reserve value things are kinda goes both ways. Its our reserve value in that its the amount we have to keep to ourselves (as reserve), but its set by them, and to keep variable names consistent, the variable is their_reserve. |
|
Nah don't talking about reserve value, but the one above, we check against their_max_htlc_value_in_flight_msat but we send back as error msg "Cannot send value that would put us over our max HTLC value in flight"? |
TheBlueMatt
force-pushed
the
2019-01-holding-cell-limits
branch
from
9e31403 to
1fa1545
Compare
|
After IRC discussion removed the "our" to reduce confusion. |
TheBlueMatt
force-pushed
the
2019-01-holding-cell-limits
branch
4 times, most recently
from
1a718e5 to
0b76979
Compare
Previously, if we went to free the holding cell HTLC updates, and adding one failed as we hit our outbound HTLC limit (or in-flight value limit), we would not send a commitment_signed, leaving us in an invalid state. We first fix that bug, and then refuse to add things to our holding cell once we reach our limits considering the holding cell, as we shouldn't have multiple commitment dance rounds worth of HTLCs in the holding cell anyway.
TheBlueMatt
force-pushed
the
2019-01-holding-cell-limits
branch
from
0b76979 to
bf26056
Compare
Based on #294.
Previously, if we went to free the holding cell HTLC updates, and
adding one failed as we hit our outbound HTLC limit (or in-flight
value limit), we would not send a commitment_signed, leaving us in
an invalid state. We first fix that bug, and then refuse to add
things to our holding cell once we reach our limits considering the
holding cell, as we shouldn't have multiple commitment dance rounds
worth of HTLCs in the holding cell anyway.