Fix remote reserve check with inbound claims-in-flight #316
Conversation
ariard
left a comment
ariard
left a comment
There was a problem hiding this comment.
utACK, take some time to get channel_reserve_in_flight_removes, but I think it's right
src/ln/channel.rs
Outdated
| // channel reserve is to ensure we can punish *them* if they misbehave, so we ignore any | ||
| // outbound HTLCs which will not be present in the next commitment transaction we send | ||
| // them (at least for fulfilled ones, failed ones won't modify value_to_self). | ||
| // Note that we will send HTLCs which we'd think violate the reserve value if we do not do |
There was a problem hiding this comment.
This is a bit confusingly worded, I'll rephrase it to "Note that we will send HTLCs which another instance of rust-lightning would think violate the reserve value..."
There was a problem hiding this comment.
Speaking of rust-lightning instance means this point isn't cleary specified in specs right ? Maybe think to open an issue on this.
src/ln/channel.rs
Outdated
| // something if we punish them for broadcasting an old state). | ||
| if htlc_inbound_value_msat + msg.amount_msat + self.value_to_self_msat > (self.channel_value_satoshis - Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis)) * 1000 { | ||
| // Note that we don't really care about our commitment transactions, as the purpose of the | ||
| // channel reserve is to ensure we can punish *them* if they misbehave, so we ignore any |
src/ln/channel.rs
Outdated
| // the reserve_satoshis we told them to always have as direct payment so that they lose | ||
| // something if we punish them for broadcasting an old state). | ||
| if htlc_inbound_value_msat + msg.amount_msat + self.value_to_self_msat > (self.channel_value_satoshis - Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis)) * 1000 { | ||
| // Note that we don't really care about our commitment transactions, as the purpose of the |
There was a problem hiding this comment.
Okay, by "we don't really care about our commitment transactions" , you mean we can't rely on htlc outputs in commitment tx because what matters there is their fulfilled or not state ?
There was a problem hiding this comment.
Hmm, I'll rephrase, but what I really meant is that we dont care about lack of to_remote output in our local commitment txn as we werent gonna use it to punish them anyway.
src/ln/functional_tests.rs
Outdated
| let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1); | ||
|
|
||
| let b_chan_values = get_channel_value_stat!(nodes[1], chan_1.2); | ||
| // Route the first two HTLCs. Note that the test is to check that, when you consider the values |
There was a problem hiding this comment.
Well IMO this note is simply explaining the test better than the long description above, maybe it should be in top-level comment
Found by chanmon_fail_consistency fuzzer.
Found by chanmon_fail_consistency fuzz test.
TheBlueMatt
force-pushed
the
2019-03-removed-reserve-check
branch
from
1915540 to
054530c
Compare
|
Can you confirm the new comments are clearer-enough? |
|
I didn't actually double-check the spec (should do to make sure it's clear) but this is the only interpretation that makes sense, I think.
… On Mar 22, 2019, at 21:49, ariard ***@***.***> wrote:
@ariard commented on this pull request.
In src/ln/channel.rs:
> @@ -1594,7 +1594,22 @@ impl Channel {
// Check our_channel_reserve_satoshis (we're getting paid, so they have to at least meet
// the reserve_satoshis we told them to always have as direct payment so that they lose
// something if we punish them for broadcasting an old state).
- if htlc_inbound_value_msat + msg.amount_msat + self.value_to_self_msat > (self.channel_value_satoshis - Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis)) * 1000 {
+ // Note that we don't really care about our commitment transactions, as the purpose of the
+ // channel reserve is to ensure we can punish *them* if they misbehave, so we ignore any
+ // outbound HTLCs which will not be present in the next commitment transaction we send
+ // them (at least for fulfilled ones, failed ones won't modify value_to_self).
+ // Note that we will send HTLCs which we'd think violate the reserve value if we do not do
Speaking of rust-lightning instance means this point isn't cleary specified in specs right ? Maybe think to open an issue on this.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Good, read again every litigious comment, get them better |
2 participants
Based on #314, this fixes a bug found by chanmon_fail_consistency (and another bug that was introduced in the first fix). See comments/commits for more info.