Incl tx fee when calcing inbound+outbound HTLC limits on channels

[valentinewallace]

When we receive an inbound HTLC from a peer on an inbound channel,
the peer needs to be able to pay for the additional commitment tx
fees as well.

When we're sending an outbound HTLC on an outbound channel, we need
to be able to pay for the additional commitment tx fees as well.

Closes#548.

TODOs:

• add more comments to channel_reserve_test and added helper functions
• add impl + test Avoid stuck channels after fee increase with additional reserve lightning/bolts#740
• add a test that hits this case + update these docs https://github.com/rust-bitcoin/rust-lightning/blob/master/lightning/src/ln/channel.rs#L2043
• update the chanmon_consistency fuzz target to be able to hit this case

Oops, travis found a bug in your update for chanmon_consistency.rs. This fixes it:

diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index 4659d0afc..e31fea516 100644
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -641,12 +641,12 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
});
for event in events.drain(..) {
match event {
- events::Event::PaymentReceived { payment_hash, payment_secret, .. } => {
+ events::Event::PaymentReceived { payment_hash, payment_secret, amt } => {
if claim_set.insert(payment_hash.0) {
if $fail {
assert!(nodes[$node].fail_htlc_backwards(&payment_hash, &payment_secret));
} else {
- assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, 5_000_000));
+ assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, amt));
}
}
},

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

no reason, thx!

@TheBlueMatt@jkczyz I updated the logic in the latest commit but haven't updated the tests to match yet. Could you check my logic and then I'll update the tests?

Codecov Report

Merging #577 into master will increase coverage by 0.01%.
The diff coverage is 95.48%.

@@ Coverage Diff @@## master #577 +/- ##
==========================================
+ Coverage 91.26% 91.27% +0.01% 
==========================================
Files 35 35 Lines 20918 21122 +204 ==========================================
+ Hits 19090 19279 +189 - Misses 1828 1843 +15 

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	85.45% <90.00%> (-0.01%)	⬇️
lightning/src/ln/channel.rs	86.72% <91.89%> (+0.09%)	⬆️
lightning/src/ln/functional_tests.rs	97.11% <97.67%> (+<0.01%)	⬆️
lightning/src/ln/peer_handler.rs	55.00% <0.00%> (+0.85%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f08d610...17ccab4. Read the comment docs.

TODO: is the reason this added HTLC's amount can't count toward's the receiver's

I think that's preventive to avoid initiator getting under reserve if HTLC is cancelled at same time another HTLC is added.

Still wrapping my head around the fee spike buffer and how we're deviating from the spec. But wanted to leave some comments awhile.

I was thinking more on #577 (comment), my current thinking is that we might be able to do the following:

* Generalize the closure to handle the `!chan.get().is_usable()` case, taking the error code.
* Move that check into `Channel::update_add_htcl`
* Make the closure produce an entire `PendingHTLCStatus`, not just the `OnionErrorPacket`

This may also address #577 (comment).

I'm not positive this would work as there may be some subtly that I'm missing. But that's exactly why this could use some refactoring, IMHO. I think the advantage is that it keeps channel checks and updating pending_forward_info inside Channel rather than split between Channel and ChannelManager.

@TheBlueMatt Would welcome your insight on this.

Added in b36589c. It seems cleaner to me :)

This is redundant with the checks over the next few lines (and we shouldnt be failing the HTLC in this case, we should be failing the channel because our counterparty is misbehaving).

Oops @jkczyz I accidentally deleted your comment, lol. It said : "FWIW, I think the behavior here is the same as it had been before. Though maybe that wasn't desirable."

@TheBlueMatt It's not redundant because self.is_usable will return false if either party has sent a shutdown. If only we have sent a shutdown, we do indeed just want to fail the HTLC (which is why the htlc_fail_async_shutdown test exists.) Then the next check checks whether the remote has sent a shutdown, in which case we do want to fail the channel because that's a spec violation.

So, basically saying I think the logic is right here fcf5072. Though it might be more clear if we changed the is_usable check to more specifically check if only we have sent a shutdown.

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

Need to take a closer look at the test changes but wanted to publishes these draft comments that I had written earlier.

This comment is out-of-date now that create_pending_htlc_status no longer returns an Option.

In my previous suggestion that removed Option, I mentioned it may not be as intuitive to update pending_forward_state in this manner, though. A more intuitive approach could be to check against pending_forward_state for PendingHTLCStatus::Forward at each call site.

The disadvantage of this alternative is some code duplication. The advantage is it would be more explicit by clarifying when pending_forward_state is changed. So that may be a better approach as I think the comment would also no longer be necessary.

That said, you may have to make other fields public. And, there may be some borrow issues that make it less clean. So, up to you if you want to try it that way.

I guess I'm in favor of not increasing reasons that the Channel needs to know about the PendingHTLCStatus struct and I think it'd be better to work towards that struct being private to the ChannelManager to make this code more easily testable. I'm also pretty OK with the level of intuitiveness of the current code, plus increased conciseness is nice.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Cool! Was just looking through the tests and thinking if there were any other ways to go about it. Let me know if you want to run anything by me.

Looks great. A few nits and would be nice to get a bit more test coverage, but looks good and I'm still a big fan of the new fuzz coverage.

nit: the "as above" note here is a copy-paste bug (and why move this out of the else block where it makes the most sense?)

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

@ariard Hm, since it's a spec void, I'd think we'd default to giving them the benefit of the doubt & not closing?

Might be worth noting that, eventually, when we remove support for fee updates and switch to anchor-type fees, we can drop the 2x but we should keep the +1.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

@TheBlueMatt are you fine to remove the anchor outputs reference here (per Antoine's comment above ^)? I'm not too familiar with the proposal so don't have an opinion.

Can you explicit exactly where the spec deviation is ?

Clarified in the comment

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

If you set this to 0 the tests all still pass.

Hm, when I set this to 0 test_chan_reserve_violation_inbound_htlc_outbound_channel fails (as is the intention of the test). Would you mind double checking?

Commenting this line out and the tests still pass.

Sigh, yeah. Any advice on how to test this one? I'm thinking maybe I can add a function send_payment_test_only to channelmanager that sends payments w/o doing the normal checks? Can't figure out any way through the current public API atm.

Ongoing review, generally document exactly where we enforce each new spec point and deviation would be great for future reads

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

This is the spec deviation right ? Please at least add a comment for it and modify commit message to say we fail channel. Anyway that may not conjugate well with anchor output, where once implemented a initiator-sender consider it's always good to send a HTLC if above reserve because you can always bump ?

Sorry, don't believe this is a deviation. Curious why you think it's a deviation?
From the BOLT 2:

update_add_htlc
A sending node:
if it is responsible for paying the Bitcoin fee:
MUST NOT offer amount_msat if, after adding that HTLC to its commitment transaction, it cannot pay the fee for either the local or remote commitment transaction at the current feerate_per_kw while maintaining its channel reserve

AFAICT, this is a requirement on the sender but here we're at HTLC reception. It makes sense as that way you avoid to close channel even if sender violates spec. I think channel can still be unstuck if this HTLC is failed and sender-initiator balance is increased again without waiting feerate decrease.

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Ah but there's no fee spike buffer check here?

Here's the deviating fee spike buffer check: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1811-R1818

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Right, this check is intended to purely perform the check that you quote above here ^

After IRC discussion, this is right, I confused myself with what this check was enforcing, but still you should have point out the receiver spec-side :p

That's fair, lol

Saw the IRC discussion -- added clarifying comment https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1796

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

@TheBlueMatt

I don't get the rational between the difference in the way we account inbound HTLCs and outbound HTLCs. For any inbound HTLCs we evaluate as part of our balance independently of its state. We presume its resolution is favorable to us, even if it maybe not the case, which is reasonable without more tracking complexity.

Instead for outbound HTLC we account them in counterparty balance only after its removal announcement, independently of a success or failure. Why we don't also presume outbound HTLCs resolution are always favorable to them ? I.e accounting in their balance once we announce outbound HTLCs

I dont really get your question. We should not be assuming any kind of HTLC resolution here, nor do I think we do - we only ever consider HTLCs which have begun the removal process.

I know we talked about this offline but after reading conv again it was a still a misunderstood. I think we're making a different assumption for outbound, namely we don't consider that a committed outbound will resolve favorably for them.

Why we don't account OutboundHTLCState::Committed in part of their balance, removed_outbound_total_msat ?

I'm still a bit confused. Note that the "None" here means "there is no HTLCFailReason and the claim has resolved in our favor"

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

That's a nice improvement, this PR's pretty big already though, so I'd prefer to leave it as a follow-up?

@valentinewallace Sure will open documentation as a follow-up!

Why assign product of channel state comparison and then test this assignment instead of testing channel state directly ?

Just to make it readable and clear what's being checked. The variable makes it explicit that we're trying to check whether the local has sent a shutdown msg

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

Why we don't check that sender-initiator can also afford this new HTLC with regards to our local commitment transaction fee ?

That's what this is supposed to be doing: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1830. Though it also takes into account the channel reserve, which we discuss further below.

Code Review ACK d2ed6c1

Balances decrease may trigger a channel reserve or fee buffer spikes violation. There is 2 sources of balance decreases a) offering an HTLC and b) paying fees for the added weight of this HTLC. With regards to sender/receiver, a) is always static to evaluate as offers are unilateral in a single commit update, b) depends on channel funder. At HTLC sending, we're only concerned with checking b) as a potential counterparty violation (for security) and checking a) and b) as a violation of our own. At HTLC reception, we should only check a) and b) on counterparty. We deviate spec by 1) checking that we don't bypass our reserve, 2) checking fee buffer spikes integrity if peer is funder.

That said, I think we should move forward, and document behavior in follow-up. Likely spec should be also updated for clarification.

Great work Val !

[valentinewallace]

When we receive an inbound HTLC from a peer on an inbound channel,
the peer needs to be able to pay for the additional commitment tx
fees as well.

When we're sending an outbound HTLC on an outbound channel, we need
to be able to pay for the additional commitment tx fees as well.

Closes#548.

TODOs:

• add more comments to channel_reserve_test and added helper functions
• add impl + test Avoid stuck channels after fee increase with additional reserve lightning/bolts#740
• add a test that hits this case + update these docs https://github.com/rust-bitcoin/rust-lightning/blob/master/lightning/src/ln/channel.rs#L2043
• update the chanmon_consistency fuzz target to be able to hit this case

Oops, travis found a bug in your update for chanmon_consistency.rs. This fixes it:

diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index 4659d0afc..e31fea516 100644
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -641,12 +641,12 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
});
for event in events.drain(..) {
match event {
- events::Event::PaymentReceived { payment_hash, payment_secret, .. } => {
+ events::Event::PaymentReceived { payment_hash, payment_secret, amt } => {
if claim_set.insert(payment_hash.0) {
if $fail {
assert!(nodes[$node].fail_htlc_backwards(&payment_hash, &payment_secret));
} else {
- assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, 5_000_000));
+ assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, amt));
}
}
},

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

no reason, thx!

@TheBlueMatt@jkczyz I updated the logic in the latest commit but haven't updated the tests to match yet. Could you check my logic and then I'll update the tests?

Codecov Report

Merging #577 into master will increase coverage by 0.01%.
The diff coverage is 95.48%.

@@ Coverage Diff @@## master #577 +/- ##
==========================================
+ Coverage 91.26% 91.27% +0.01% 
==========================================
Files 35 35 Lines 20918 21122 +204 ==========================================
+ Hits 19090 19279 +189 - Misses 1828 1843 +15 

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	85.45% <90.00%> (-0.01%)	⬇️
lightning/src/ln/channel.rs	86.72% <91.89%> (+0.09%)	⬆️
lightning/src/ln/functional_tests.rs	97.11% <97.67%> (+<0.01%)	⬆️
lightning/src/ln/peer_handler.rs	55.00% <0.00%> (+0.85%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f08d610...17ccab4. Read the comment docs.

TODO: is the reason this added HTLC's amount can't count toward's the receiver's

I think that's preventive to avoid initiator getting under reserve if HTLC is cancelled at same time another HTLC is added.

Still wrapping my head around the fee spike buffer and how we're deviating from the spec. But wanted to leave some comments awhile.

I was thinking more on #577 (comment), my current thinking is that we might be able to do the following:

* Generalize the closure to handle the `!chan.get().is_usable()` case, taking the error code.
* Move that check into `Channel::update_add_htcl`
* Make the closure produce an entire `PendingHTLCStatus`, not just the `OnionErrorPacket`

This may also address #577 (comment).

I'm not positive this would work as there may be some subtly that I'm missing. But that's exactly why this could use some refactoring, IMHO. I think the advantage is that it keeps channel checks and updating pending_forward_info inside Channel rather than split between Channel and ChannelManager.

@TheBlueMatt Would welcome your insight on this.

Added in b36589c. It seems cleaner to me :)

This is redundant with the checks over the next few lines (and we shouldnt be failing the HTLC in this case, we should be failing the channel because our counterparty is misbehaving).

Oops @jkczyz I accidentally deleted your comment, lol. It said : "FWIW, I think the behavior here is the same as it had been before. Though maybe that wasn't desirable."

@TheBlueMatt It's not redundant because self.is_usable will return false if either party has sent a shutdown. If only we have sent a shutdown, we do indeed just want to fail the HTLC (which is why the htlc_fail_async_shutdown test exists.) Then the next check checks whether the remote has sent a shutdown, in which case we do want to fail the channel because that's a spec violation.

So, basically saying I think the logic is right here fcf5072. Though it might be more clear if we changed the is_usable check to more specifically check if only we have sent a shutdown.

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

Need to take a closer look at the test changes but wanted to publishes these draft comments that I had written earlier.

This comment is out-of-date now that create_pending_htlc_status no longer returns an Option.

In my previous suggestion that removed Option, I mentioned it may not be as intuitive to update pending_forward_state in this manner, though. A more intuitive approach could be to check against pending_forward_state for PendingHTLCStatus::Forward at each call site.

The disadvantage of this alternative is some code duplication. The advantage is it would be more explicit by clarifying when pending_forward_state is changed. So that may be a better approach as I think the comment would also no longer be necessary.

That said, you may have to make other fields public. And, there may be some borrow issues that make it less clean. So, up to you if you want to try it that way.

I guess I'm in favor of not increasing reasons that the Channel needs to know about the PendingHTLCStatus struct and I think it'd be better to work towards that struct being private to the ChannelManager to make this code more easily testable. I'm also pretty OK with the level of intuitiveness of the current code, plus increased conciseness is nice.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Cool! Was just looking through the tests and thinking if there were any other ways to go about it. Let me know if you want to run anything by me.

Looks great. A few nits and would be nice to get a bit more test coverage, but looks good and I'm still a big fan of the new fuzz coverage.

nit: the "as above" note here is a copy-paste bug (and why move this out of the else block where it makes the most sense?)

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

@ariard Hm, since it's a spec void, I'd think we'd default to giving them the benefit of the doubt & not closing?

Might be worth noting that, eventually, when we remove support for fee updates and switch to anchor-type fees, we can drop the 2x but we should keep the +1.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

@TheBlueMatt are you fine to remove the anchor outputs reference here (per Antoine's comment above ^)? I'm not too familiar with the proposal so don't have an opinion.

Can you explicit exactly where the spec deviation is ?

Clarified in the comment

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

If you set this to 0 the tests all still pass.

Hm, when I set this to 0 test_chan_reserve_violation_inbound_htlc_outbound_channel fails (as is the intention of the test). Would you mind double checking?

Commenting this line out and the tests still pass.

Sigh, yeah. Any advice on how to test this one? I'm thinking maybe I can add a function send_payment_test_only to channelmanager that sends payments w/o doing the normal checks? Can't figure out any way through the current public API atm.

Ongoing review, generally document exactly where we enforce each new spec point and deviation would be great for future reads

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

This is the spec deviation right ? Please at least add a comment for it and modify commit message to say we fail channel. Anyway that may not conjugate well with anchor output, where once implemented a initiator-sender consider it's always good to send a HTLC if above reserve because you can always bump ?

Sorry, don't believe this is a deviation. Curious why you think it's a deviation?
From the BOLT 2:

update_add_htlc
A sending node:
if it is responsible for paying the Bitcoin fee:
MUST NOT offer amount_msat if, after adding that HTLC to its commitment transaction, it cannot pay the fee for either the local or remote commitment transaction at the current feerate_per_kw while maintaining its channel reserve

AFAICT, this is a requirement on the sender but here we're at HTLC reception. It makes sense as that way you avoid to close channel even if sender violates spec. I think channel can still be unstuck if this HTLC is failed and sender-initiator balance is increased again without waiting feerate decrease.

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Ah but there's no fee spike buffer check here?

Here's the deviating fee spike buffer check: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1811-R1818

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Right, this check is intended to purely perform the check that you quote above here ^

After IRC discussion, this is right, I confused myself with what this check was enforcing, but still you should have point out the receiver spec-side :p

That's fair, lol

Saw the IRC discussion -- added clarifying comment https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1796

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

@TheBlueMatt

I don't get the rational between the difference in the way we account inbound HTLCs and outbound HTLCs. For any inbound HTLCs we evaluate as part of our balance independently of its state. We presume its resolution is favorable to us, even if it maybe not the case, which is reasonable without more tracking complexity.

Instead for outbound HTLC we account them in counterparty balance only after its removal announcement, independently of a success or failure. Why we don't also presume outbound HTLCs resolution are always favorable to them ? I.e accounting in their balance once we announce outbound HTLCs

I dont really get your question. We should not be assuming any kind of HTLC resolution here, nor do I think we do - we only ever consider HTLCs which have begun the removal process.

I know we talked about this offline but after reading conv again it was a still a misunderstood. I think we're making a different assumption for outbound, namely we don't consider that a committed outbound will resolve favorably for them.

Why we don't account OutboundHTLCState::Committed in part of their balance, removed_outbound_total_msat ?

I'm still a bit confused. Note that the "None" here means "there is no HTLCFailReason and the claim has resolved in our favor"

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

That's a nice improvement, this PR's pretty big already though, so I'd prefer to leave it as a follow-up?

@valentinewallace Sure will open documentation as a follow-up!

Why assign product of channel state comparison and then test this assignment instead of testing channel state directly ?

Just to make it readable and clear what's being checked. The variable makes it explicit that we're trying to check whether the local has sent a shutdown msg

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

Why we don't check that sender-initiator can also afford this new HTLC with regards to our local commitment transaction fee ?

That's what this is supposed to be doing: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1830. Though it also takes into account the channel reserve, which we discuss further below.

Code Review ACK d2ed6c1

Balances decrease may trigger a channel reserve or fee buffer spikes violation. There is 2 sources of balance decreases a) offering an HTLC and b) paying fees for the added weight of this HTLC. With regards to sender/receiver, a) is always static to evaluate as offers are unilateral in a single commit update, b) depends on channel funder. At HTLC sending, we're only concerned with checking b) as a potential counterparty violation (for security) and checking a) and b) as a violation of our own. At HTLC reception, we should only check a) and b) on counterparty. We deviate spec by 1) checking that we don't bypass our reserve, 2) checking fee buffer spikes integrity if peer is funder.

That said, I think we should move forward, and document behavior in follow-up. Likely spec should be also updated for clarification.

Great work Val !

[valentinewallace]

When we receive an inbound HTLC from a peer on an inbound channel,
the peer needs to be able to pay for the additional commitment tx
fees as well.

When we're sending an outbound HTLC on an outbound channel, we need
to be able to pay for the additional commitment tx fees as well.

Closes#548.

TODOs:

• add more comments to channel_reserve_test and added helper functions
• add impl + test Avoid stuck channels after fee increase with additional reserve lightning/bolts#740
• add a test that hits this case + update these docs https://github.com/rust-bitcoin/rust-lightning/blob/master/lightning/src/ln/channel.rs#L2043
• update the chanmon_consistency fuzz target to be able to hit this case

Oops, travis found a bug in your update for chanmon_consistency.rs. This fixes it:

diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index 4659d0afc..e31fea516 100644
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -641,12 +641,12 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
});
for event in events.drain(..) {
match event {
- events::Event::PaymentReceived { payment_hash, payment_secret, .. } => {
+ events::Event::PaymentReceived { payment_hash, payment_secret, amt } => {
if claim_set.insert(payment_hash.0) {
if $fail {
assert!(nodes[$node].fail_htlc_backwards(&payment_hash, &payment_secret));
} else {
- assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, 5_000_000));
+ assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, amt));
}
}
},

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

no reason, thx!

@TheBlueMatt@jkczyz I updated the logic in the latest commit but haven't updated the tests to match yet. Could you check my logic and then I'll update the tests?

Codecov Report

Merging #577 into master will increase coverage by 0.01%.
The diff coverage is 95.48%.

@@ Coverage Diff @@## master #577 +/- ##
==========================================
+ Coverage 91.26% 91.27% +0.01% 
==========================================
Files 35 35 Lines 20918 21122 +204 ==========================================
+ Hits 19090 19279 +189 - Misses 1828 1843 +15 

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	85.45% <90.00%> (-0.01%)	⬇️
lightning/src/ln/channel.rs	86.72% <91.89%> (+0.09%)	⬆️
lightning/src/ln/functional_tests.rs	97.11% <97.67%> (+<0.01%)	⬆️
lightning/src/ln/peer_handler.rs	55.00% <0.00%> (+0.85%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f08d610...17ccab4. Read the comment docs.

TODO: is the reason this added HTLC's amount can't count toward's the receiver's

I think that's preventive to avoid initiator getting under reserve if HTLC is cancelled at same time another HTLC is added.

Still wrapping my head around the fee spike buffer and how we're deviating from the spec. But wanted to leave some comments awhile.

I was thinking more on #577 (comment), my current thinking is that we might be able to do the following:

* Generalize the closure to handle the `!chan.get().is_usable()` case, taking the error code.
* Move that check into `Channel::update_add_htcl`
* Make the closure produce an entire `PendingHTLCStatus`, not just the `OnionErrorPacket`

This may also address #577 (comment).

I'm not positive this would work as there may be some subtly that I'm missing. But that's exactly why this could use some refactoring, IMHO. I think the advantage is that it keeps channel checks and updating pending_forward_info inside Channel rather than split between Channel and ChannelManager.

@TheBlueMatt Would welcome your insight on this.

Added in b36589c. It seems cleaner to me :)

This is redundant with the checks over the next few lines (and we shouldnt be failing the HTLC in this case, we should be failing the channel because our counterparty is misbehaving).

Oops @jkczyz I accidentally deleted your comment, lol. It said : "FWIW, I think the behavior here is the same as it had been before. Though maybe that wasn't desirable."

@TheBlueMatt It's not redundant because self.is_usable will return false if either party has sent a shutdown. If only we have sent a shutdown, we do indeed just want to fail the HTLC (which is why the htlc_fail_async_shutdown test exists.) Then the next check checks whether the remote has sent a shutdown, in which case we do want to fail the channel because that's a spec violation.

So, basically saying I think the logic is right here fcf5072. Though it might be more clear if we changed the is_usable check to more specifically check if only we have sent a shutdown.

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

Need to take a closer look at the test changes but wanted to publishes these draft comments that I had written earlier.

This comment is out-of-date now that create_pending_htlc_status no longer returns an Option.

In my previous suggestion that removed Option, I mentioned it may not be as intuitive to update pending_forward_state in this manner, though. A more intuitive approach could be to check against pending_forward_state for PendingHTLCStatus::Forward at each call site.

The disadvantage of this alternative is some code duplication. The advantage is it would be more explicit by clarifying when pending_forward_state is changed. So that may be a better approach as I think the comment would also no longer be necessary.

That said, you may have to make other fields public. And, there may be some borrow issues that make it less clean. So, up to you if you want to try it that way.

I guess I'm in favor of not increasing reasons that the Channel needs to know about the PendingHTLCStatus struct and I think it'd be better to work towards that struct being private to the ChannelManager to make this code more easily testable. I'm also pretty OK with the level of intuitiveness of the current code, plus increased conciseness is nice.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Cool! Was just looking through the tests and thinking if there were any other ways to go about it. Let me know if you want to run anything by me.

Looks great. A few nits and would be nice to get a bit more test coverage, but looks good and I'm still a big fan of the new fuzz coverage.

nit: the "as above" note here is a copy-paste bug (and why move this out of the else block where it makes the most sense?)

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

@ariard Hm, since it's a spec void, I'd think we'd default to giving them the benefit of the doubt & not closing?

Might be worth noting that, eventually, when we remove support for fee updates and switch to anchor-type fees, we can drop the 2x but we should keep the +1.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

@TheBlueMatt are you fine to remove the anchor outputs reference here (per Antoine's comment above ^)? I'm not too familiar with the proposal so don't have an opinion.

Can you explicit exactly where the spec deviation is ?

Clarified in the comment

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

If you set this to 0 the tests all still pass.

Hm, when I set this to 0 test_chan_reserve_violation_inbound_htlc_outbound_channel fails (as is the intention of the test). Would you mind double checking?

Commenting this line out and the tests still pass.

Sigh, yeah. Any advice on how to test this one? I'm thinking maybe I can add a function send_payment_test_only to channelmanager that sends payments w/o doing the normal checks? Can't figure out any way through the current public API atm.

Ongoing review, generally document exactly where we enforce each new spec point and deviation would be great for future reads

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

This is the spec deviation right ? Please at least add a comment for it and modify commit message to say we fail channel. Anyway that may not conjugate well with anchor output, where once implemented a initiator-sender consider it's always good to send a HTLC if above reserve because you can always bump ?

Sorry, don't believe this is a deviation. Curious why you think it's a deviation?
From the BOLT 2:

update_add_htlc
A sending node:
if it is responsible for paying the Bitcoin fee:
MUST NOT offer amount_msat if, after adding that HTLC to its commitment transaction, it cannot pay the fee for either the local or remote commitment transaction at the current feerate_per_kw while maintaining its channel reserve

AFAICT, this is a requirement on the sender but here we're at HTLC reception. It makes sense as that way you avoid to close channel even if sender violates spec. I think channel can still be unstuck if this HTLC is failed and sender-initiator balance is increased again without waiting feerate decrease.

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Ah but there's no fee spike buffer check here?

Here's the deviating fee spike buffer check: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1811-R1818

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Right, this check is intended to purely perform the check that you quote above here ^

After IRC discussion, this is right, I confused myself with what this check was enforcing, but still you should have point out the receiver spec-side :p

That's fair, lol

Saw the IRC discussion -- added clarifying comment https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1796

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

@TheBlueMatt

I don't get the rational between the difference in the way we account inbound HTLCs and outbound HTLCs. For any inbound HTLCs we evaluate as part of our balance independently of its state. We presume its resolution is favorable to us, even if it maybe not the case, which is reasonable without more tracking complexity.

Instead for outbound HTLC we account them in counterparty balance only after its removal announcement, independently of a success or failure. Why we don't also presume outbound HTLCs resolution are always favorable to them ? I.e accounting in their balance once we announce outbound HTLCs

I dont really get your question. We should not be assuming any kind of HTLC resolution here, nor do I think we do - we only ever consider HTLCs which have begun the removal process.

I know we talked about this offline but after reading conv again it was a still a misunderstood. I think we're making a different assumption for outbound, namely we don't consider that a committed outbound will resolve favorably for them.

Why we don't account OutboundHTLCState::Committed in part of their balance, removed_outbound_total_msat ?

I'm still a bit confused. Note that the "None" here means "there is no HTLCFailReason and the claim has resolved in our favor"

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

That's a nice improvement, this PR's pretty big already though, so I'd prefer to leave it as a follow-up?

@valentinewallace Sure will open documentation as a follow-up!

Why assign product of channel state comparison and then test this assignment instead of testing channel state directly ?

Just to make it readable and clear what's being checked. The variable makes it explicit that we're trying to check whether the local has sent a shutdown msg

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

Why we don't check that sender-initiator can also afford this new HTLC with regards to our local commitment transaction fee ?

That's what this is supposed to be doing: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1830. Though it also takes into account the channel reserve, which we discuss further below.

Code Review ACK d2ed6c1

Balances decrease may trigger a channel reserve or fee buffer spikes violation. There is 2 sources of balance decreases a) offering an HTLC and b) paying fees for the added weight of this HTLC. With regards to sender/receiver, a) is always static to evaluate as offers are unilateral in a single commit update, b) depends on channel funder. At HTLC sending, we're only concerned with checking b) as a potential counterparty violation (for security) and checking a) and b) as a violation of our own. At HTLC reception, we should only check a) and b) on counterparty. We deviate spec by 1) checking that we don't bypass our reserve, 2) checking fee buffer spikes integrity if peer is funder.

That said, I think we should move forward, and document behavior in follow-up. Likely spec should be also updated for clarification.

Great work Val !

[valentinewallace]

When we receive an inbound HTLC from a peer on an inbound channel,
the peer needs to be able to pay for the additional commitment tx
fees as well.

When we're sending an outbound HTLC on an outbound channel, we need
to be able to pay for the additional commitment tx fees as well.

Closes#548.

TODOs:

• add more comments to channel_reserve_test and added helper functions
• add impl + test Avoid stuck channels after fee increase with additional reserve lightning/bolts#740
• add a test that hits this case + update these docs https://github.com/rust-bitcoin/rust-lightning/blob/master/lightning/src/ln/channel.rs#L2043
• update the chanmon_consistency fuzz target to be able to hit this case

Oops, travis found a bug in your update for chanmon_consistency.rs. This fixes it:

diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index 4659d0afc..e31fea516 100644
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -641,12 +641,12 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
});
for event in events.drain(..) {
match event {
- events::Event::PaymentReceived { payment_hash, payment_secret, .. } => {
+ events::Event::PaymentReceived { payment_hash, payment_secret, amt } => {
if claim_set.insert(payment_hash.0) {
if $fail {
assert!(nodes[$node].fail_htlc_backwards(&payment_hash, &payment_secret));
} else {
- assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, 5_000_000));
+ assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, amt));
}
}
},

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

no reason, thx!

@TheBlueMatt@jkczyz I updated the logic in the latest commit but haven't updated the tests to match yet. Could you check my logic and then I'll update the tests?

Codecov Report

Merging #577 into master will increase coverage by 0.01%.
The diff coverage is 95.48%.

@@ Coverage Diff @@## master #577 +/- ##
==========================================
+ Coverage 91.26% 91.27% +0.01% 
==========================================
Files 35 35 Lines 20918 21122 +204 ==========================================
+ Hits 19090 19279 +189 - Misses 1828 1843 +15 

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	85.45% <90.00%> (-0.01%)	⬇️
lightning/src/ln/channel.rs	86.72% <91.89%> (+0.09%)	⬆️
lightning/src/ln/functional_tests.rs	97.11% <97.67%> (+<0.01%)	⬆️
lightning/src/ln/peer_handler.rs	55.00% <0.00%> (+0.85%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f08d610...17ccab4. Read the comment docs.

TODO: is the reason this added HTLC's amount can't count toward's the receiver's

I think that's preventive to avoid initiator getting under reserve if HTLC is cancelled at same time another HTLC is added.

Still wrapping my head around the fee spike buffer and how we're deviating from the spec. But wanted to leave some comments awhile.

I was thinking more on #577 (comment), my current thinking is that we might be able to do the following:

* Generalize the closure to handle the `!chan.get().is_usable()` case, taking the error code.
* Move that check into `Channel::update_add_htcl`
* Make the closure produce an entire `PendingHTLCStatus`, not just the `OnionErrorPacket`

This may also address #577 (comment).

I'm not positive this would work as there may be some subtly that I'm missing. But that's exactly why this could use some refactoring, IMHO. I think the advantage is that it keeps channel checks and updating pending_forward_info inside Channel rather than split between Channel and ChannelManager.

@TheBlueMatt Would welcome your insight on this.

Added in b36589c. It seems cleaner to me :)

This is redundant with the checks over the next few lines (and we shouldnt be failing the HTLC in this case, we should be failing the channel because our counterparty is misbehaving).

Oops @jkczyz I accidentally deleted your comment, lol. It said : "FWIW, I think the behavior here is the same as it had been before. Though maybe that wasn't desirable."

@TheBlueMatt It's not redundant because self.is_usable will return false if either party has sent a shutdown. If only we have sent a shutdown, we do indeed just want to fail the HTLC (which is why the htlc_fail_async_shutdown test exists.) Then the next check checks whether the remote has sent a shutdown, in which case we do want to fail the channel because that's a spec violation.

So, basically saying I think the logic is right here fcf5072. Though it might be more clear if we changed the is_usable check to more specifically check if only we have sent a shutdown.

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

Need to take a closer look at the test changes but wanted to publishes these draft comments that I had written earlier.

This comment is out-of-date now that create_pending_htlc_status no longer returns an Option.

In my previous suggestion that removed Option, I mentioned it may not be as intuitive to update pending_forward_state in this manner, though. A more intuitive approach could be to check against pending_forward_state for PendingHTLCStatus::Forward at each call site.

The disadvantage of this alternative is some code duplication. The advantage is it would be more explicit by clarifying when pending_forward_state is changed. So that may be a better approach as I think the comment would also no longer be necessary.

That said, you may have to make other fields public. And, there may be some borrow issues that make it less clean. So, up to you if you want to try it that way.

I guess I'm in favor of not increasing reasons that the Channel needs to know about the PendingHTLCStatus struct and I think it'd be better to work towards that struct being private to the ChannelManager to make this code more easily testable. I'm also pretty OK with the level of intuitiveness of the current code, plus increased conciseness is nice.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Cool! Was just looking through the tests and thinking if there were any other ways to go about it. Let me know if you want to run anything by me.

Looks great. A few nits and would be nice to get a bit more test coverage, but looks good and I'm still a big fan of the new fuzz coverage.

nit: the "as above" note here is a copy-paste bug (and why move this out of the else block where it makes the most sense?)

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

@ariard Hm, since it's a spec void, I'd think we'd default to giving them the benefit of the doubt & not closing?

Might be worth noting that, eventually, when we remove support for fee updates and switch to anchor-type fees, we can drop the 2x but we should keep the +1.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

@TheBlueMatt are you fine to remove the anchor outputs reference here (per Antoine's comment above ^)? I'm not too familiar with the proposal so don't have an opinion.

Can you explicit exactly where the spec deviation is ?

Clarified in the comment

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

If you set this to 0 the tests all still pass.

Hm, when I set this to 0 test_chan_reserve_violation_inbound_htlc_outbound_channel fails (as is the intention of the test). Would you mind double checking?

Commenting this line out and the tests still pass.

Sigh, yeah. Any advice on how to test this one? I'm thinking maybe I can add a function send_payment_test_only to channelmanager that sends payments w/o doing the normal checks? Can't figure out any way through the current public API atm.

Ongoing review, generally document exactly where we enforce each new spec point and deviation would be great for future reads

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

This is the spec deviation right ? Please at least add a comment for it and modify commit message to say we fail channel. Anyway that may not conjugate well with anchor output, where once implemented a initiator-sender consider it's always good to send a HTLC if above reserve because you can always bump ?

Sorry, don't believe this is a deviation. Curious why you think it's a deviation?
From the BOLT 2:

update_add_htlc
A sending node:
if it is responsible for paying the Bitcoin fee:
MUST NOT offer amount_msat if, after adding that HTLC to its commitment transaction, it cannot pay the fee for either the local or remote commitment transaction at the current feerate_per_kw while maintaining its channel reserve

AFAICT, this is a requirement on the sender but here we're at HTLC reception. It makes sense as that way you avoid to close channel even if sender violates spec. I think channel can still be unstuck if this HTLC is failed and sender-initiator balance is increased again without waiting feerate decrease.

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Ah but there's no fee spike buffer check here?

Here's the deviating fee spike buffer check: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1811-R1818

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Right, this check is intended to purely perform the check that you quote above here ^

After IRC discussion, this is right, I confused myself with what this check was enforcing, but still you should have point out the receiver spec-side :p

That's fair, lol

Saw the IRC discussion -- added clarifying comment https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1796

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

@TheBlueMatt

I don't get the rational between the difference in the way we account inbound HTLCs and outbound HTLCs. For any inbound HTLCs we evaluate as part of our balance independently of its state. We presume its resolution is favorable to us, even if it maybe not the case, which is reasonable without more tracking complexity.

Instead for outbound HTLC we account them in counterparty balance only after its removal announcement, independently of a success or failure. Why we don't also presume outbound HTLCs resolution are always favorable to them ? I.e accounting in their balance once we announce outbound HTLCs

I dont really get your question. We should not be assuming any kind of HTLC resolution here, nor do I think we do - we only ever consider HTLCs which have begun the removal process.

I know we talked about this offline but after reading conv again it was a still a misunderstood. I think we're making a different assumption for outbound, namely we don't consider that a committed outbound will resolve favorably for them.

Why we don't account OutboundHTLCState::Committed in part of their balance, removed_outbound_total_msat ?

I'm still a bit confused. Note that the "None" here means "there is no HTLCFailReason and the claim has resolved in our favor"

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

That's a nice improvement, this PR's pretty big already though, so I'd prefer to leave it as a follow-up?

@valentinewallace Sure will open documentation as a follow-up!

Why assign product of channel state comparison and then test this assignment instead of testing channel state directly ?

Just to make it readable and clear what's being checked. The variable makes it explicit that we're trying to check whether the local has sent a shutdown msg

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

Why we don't check that sender-initiator can also afford this new HTLC with regards to our local commitment transaction fee ?

That's what this is supposed to be doing: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1830. Though it also takes into account the channel reserve, which we discuss further below.

Code Review ACK d2ed6c1

Balances decrease may trigger a channel reserve or fee buffer spikes violation. There is 2 sources of balance decreases a) offering an HTLC and b) paying fees for the added weight of this HTLC. With regards to sender/receiver, a) is always static to evaluate as offers are unilateral in a single commit update, b) depends on channel funder. At HTLC sending, we're only concerned with checking b) as a potential counterparty violation (for security) and checking a) and b) as a violation of our own. At HTLC reception, we should only check a) and b) on counterparty. We deviate spec by 1) checking that we don't bypass our reserve, 2) checking fee buffer spikes integrity if peer is funder.

That said, I think we should move forward, and document behavior in follow-up. Likely spec should be also updated for clarification.

Great work Val !

[valentinewallace]

When we receive an inbound HTLC from a peer on an inbound channel,
the peer needs to be able to pay for the additional commitment tx
fees as well.

When we're sending an outbound HTLC on an outbound channel, we need
to be able to pay for the additional commitment tx fees as well.

Closes#548.

TODOs:

• add more comments to channel_reserve_test and added helper functions
• add impl + test Avoid stuck channels after fee increase with additional reserve lightning/bolts#740
• add a test that hits this case + update these docs https://github.com/rust-bitcoin/rust-lightning/blob/master/lightning/src/ln/channel.rs#L2043
• update the chanmon_consistency fuzz target to be able to hit this case

Oops, travis found a bug in your update for chanmon_consistency.rs. This fixes it:

diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index 4659d0afc..e31fea516 100644
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -641,12 +641,12 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
});
for event in events.drain(..) {
match event {
- events::Event::PaymentReceived { payment_hash, payment_secret, .. } => {
+ events::Event::PaymentReceived { payment_hash, payment_secret, amt } => {
if claim_set.insert(payment_hash.0) {
if $fail {
assert!(nodes[$node].fail_htlc_backwards(&payment_hash, &payment_secret));
} else {
- assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, 5_000_000));
+ assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, amt));
}
}
},

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

no reason, thx!

@TheBlueMatt@jkczyz I updated the logic in the latest commit but haven't updated the tests to match yet. Could you check my logic and then I'll update the tests?

Codecov Report

Merging #577 into master will increase coverage by 0.01%.
The diff coverage is 95.48%.

@@ Coverage Diff @@## master #577 +/- ##
==========================================
+ Coverage 91.26% 91.27% +0.01% 
==========================================
Files 35 35 Lines 20918 21122 +204 ==========================================
+ Hits 19090 19279 +189 - Misses 1828 1843 +15 

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	85.45% <90.00%> (-0.01%)	⬇️
lightning/src/ln/channel.rs	86.72% <91.89%> (+0.09%)	⬆️
lightning/src/ln/functional_tests.rs	97.11% <97.67%> (+<0.01%)	⬆️
lightning/src/ln/peer_handler.rs	55.00% <0.00%> (+0.85%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f08d610...17ccab4. Read the comment docs.

TODO: is the reason this added HTLC's amount can't count toward's the receiver's

I think that's preventive to avoid initiator getting under reserve if HTLC is cancelled at same time another HTLC is added.

Still wrapping my head around the fee spike buffer and how we're deviating from the spec. But wanted to leave some comments awhile.

I was thinking more on #577 (comment), my current thinking is that we might be able to do the following:

* Generalize the closure to handle the `!chan.get().is_usable()` case, taking the error code.
* Move that check into `Channel::update_add_htcl`
* Make the closure produce an entire `PendingHTLCStatus`, not just the `OnionErrorPacket`

This may also address #577 (comment).

I'm not positive this would work as there may be some subtly that I'm missing. But that's exactly why this could use some refactoring, IMHO. I think the advantage is that it keeps channel checks and updating pending_forward_info inside Channel rather than split between Channel and ChannelManager.

@TheBlueMatt Would welcome your insight on this.

Added in b36589c. It seems cleaner to me :)

This is redundant with the checks over the next few lines (and we shouldnt be failing the HTLC in this case, we should be failing the channel because our counterparty is misbehaving).

Oops @jkczyz I accidentally deleted your comment, lol. It said : "FWIW, I think the behavior here is the same as it had been before. Though maybe that wasn't desirable."

@TheBlueMatt It's not redundant because self.is_usable will return false if either party has sent a shutdown. If only we have sent a shutdown, we do indeed just want to fail the HTLC (which is why the htlc_fail_async_shutdown test exists.) Then the next check checks whether the remote has sent a shutdown, in which case we do want to fail the channel because that's a spec violation.

So, basically saying I think the logic is right here fcf5072. Though it might be more clear if we changed the is_usable check to more specifically check if only we have sent a shutdown.

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

Need to take a closer look at the test changes but wanted to publishes these draft comments that I had written earlier.

This comment is out-of-date now that create_pending_htlc_status no longer returns an Option.

In my previous suggestion that removed Option, I mentioned it may not be as intuitive to update pending_forward_state in this manner, though. A more intuitive approach could be to check against pending_forward_state for PendingHTLCStatus::Forward at each call site.

The disadvantage of this alternative is some code duplication. The advantage is it would be more explicit by clarifying when pending_forward_state is changed. So that may be a better approach as I think the comment would also no longer be necessary.

That said, you may have to make other fields public. And, there may be some borrow issues that make it less clean. So, up to you if you want to try it that way.

I guess I'm in favor of not increasing reasons that the Channel needs to know about the PendingHTLCStatus struct and I think it'd be better to work towards that struct being private to the ChannelManager to make this code more easily testable. I'm also pretty OK with the level of intuitiveness of the current code, plus increased conciseness is nice.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Cool! Was just looking through the tests and thinking if there were any other ways to go about it. Let me know if you want to run anything by me.

Looks great. A few nits and would be nice to get a bit more test coverage, but looks good and I'm still a big fan of the new fuzz coverage.

nit: the "as above" note here is a copy-paste bug (and why move this out of the else block where it makes the most sense?)

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

@ariard Hm, since it's a spec void, I'd think we'd default to giving them the benefit of the doubt & not closing?

Might be worth noting that, eventually, when we remove support for fee updates and switch to anchor-type fees, we can drop the 2x but we should keep the +1.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

@TheBlueMatt are you fine to remove the anchor outputs reference here (per Antoine's comment above ^)? I'm not too familiar with the proposal so don't have an opinion.

Can you explicit exactly where the spec deviation is ?

Clarified in the comment

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

If you set this to 0 the tests all still pass.

Hm, when I set this to 0 test_chan_reserve_violation_inbound_htlc_outbound_channel fails (as is the intention of the test). Would you mind double checking?

Commenting this line out and the tests still pass.

Sigh, yeah. Any advice on how to test this one? I'm thinking maybe I can add a function send_payment_test_only to channelmanager that sends payments w/o doing the normal checks? Can't figure out any way through the current public API atm.

Ongoing review, generally document exactly where we enforce each new spec point and deviation would be great for future reads

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

This is the spec deviation right ? Please at least add a comment for it and modify commit message to say we fail channel. Anyway that may not conjugate well with anchor output, where once implemented a initiator-sender consider it's always good to send a HTLC if above reserve because you can always bump ?

Sorry, don't believe this is a deviation. Curious why you think it's a deviation?
From the BOLT 2:

update_add_htlc
A sending node:
if it is responsible for paying the Bitcoin fee:
MUST NOT offer amount_msat if, after adding that HTLC to its commitment transaction, it cannot pay the fee for either the local or remote commitment transaction at the current feerate_per_kw while maintaining its channel reserve

AFAICT, this is a requirement on the sender but here we're at HTLC reception. It makes sense as that way you avoid to close channel even if sender violates spec. I think channel can still be unstuck if this HTLC is failed and sender-initiator balance is increased again without waiting feerate decrease.

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Ah but there's no fee spike buffer check here?

Here's the deviating fee spike buffer check: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1811-R1818

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Right, this check is intended to purely perform the check that you quote above here ^

After IRC discussion, this is right, I confused myself with what this check was enforcing, but still you should have point out the receiver spec-side :p

That's fair, lol

Saw the IRC discussion -- added clarifying comment https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1796

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

@TheBlueMatt

I don't get the rational between the difference in the way we account inbound HTLCs and outbound HTLCs. For any inbound HTLCs we evaluate as part of our balance independently of its state. We presume its resolution is favorable to us, even if it maybe not the case, which is reasonable without more tracking complexity.

Instead for outbound HTLC we account them in counterparty balance only after its removal announcement, independently of a success or failure. Why we don't also presume outbound HTLCs resolution are always favorable to them ? I.e accounting in their balance once we announce outbound HTLCs

I dont really get your question. We should not be assuming any kind of HTLC resolution here, nor do I think we do - we only ever consider HTLCs which have begun the removal process.

I know we talked about this offline but after reading conv again it was a still a misunderstood. I think we're making a different assumption for outbound, namely we don't consider that a committed outbound will resolve favorably for them.

Why we don't account OutboundHTLCState::Committed in part of their balance, removed_outbound_total_msat ?

I'm still a bit confused. Note that the "None" here means "there is no HTLCFailReason and the claim has resolved in our favor"

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

That's a nice improvement, this PR's pretty big already though, so I'd prefer to leave it as a follow-up?

@valentinewallace Sure will open documentation as a follow-up!

Why assign product of channel state comparison and then test this assignment instead of testing channel state directly ?

Just to make it readable and clear what's being checked. The variable makes it explicit that we're trying to check whether the local has sent a shutdown msg

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

Why we don't check that sender-initiator can also afford this new HTLC with regards to our local commitment transaction fee ?

That's what this is supposed to be doing: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1830. Though it also takes into account the channel reserve, which we discuss further below.

Code Review ACK d2ed6c1

Balances decrease may trigger a channel reserve or fee buffer spikes violation. There is 2 sources of balance decreases a) offering an HTLC and b) paying fees for the added weight of this HTLC. With regards to sender/receiver, a) is always static to evaluate as offers are unilateral in a single commit update, b) depends on channel funder. At HTLC sending, we're only concerned with checking b) as a potential counterparty violation (for security) and checking a) and b) as a violation of our own. At HTLC reception, we should only check a) and b) on counterparty. We deviate spec by 1) checking that we don't bypass our reserve, 2) checking fee buffer spikes integrity if peer is funder.

That said, I think we should move forward, and document behavior in follow-up. Likely spec should be also updated for clarification.

Great work Val !

[valentinewallace]

When we receive an inbound HTLC from a peer on an inbound channel,
the peer needs to be able to pay for the additional commitment tx
fees as well.

When we're sending an outbound HTLC on an outbound channel, we need
to be able to pay for the additional commitment tx fees as well.

Closes#548.

TODOs:

• add more comments to channel_reserve_test and added helper functions
• add impl + test Avoid stuck channels after fee increase with additional reserve lightning/bolts#740
• add a test that hits this case + update these docs https://github.com/rust-bitcoin/rust-lightning/blob/master/lightning/src/ln/channel.rs#L2043
• update the chanmon_consistency fuzz target to be able to hit this case

Oops, travis found a bug in your update for chanmon_consistency.rs. This fixes it:

diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index 4659d0afc..e31fea516 100644
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -641,12 +641,12 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
});
for event in events.drain(..) {
match event {
- events::Event::PaymentReceived { payment_hash, payment_secret, .. } => {
+ events::Event::PaymentReceived { payment_hash, payment_secret, amt } => {
if claim_set.insert(payment_hash.0) {
if $fail {
assert!(nodes[$node].fail_htlc_backwards(&payment_hash, &payment_secret));
} else {
- assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, 5_000_000));
+ assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, amt));
}
}
},

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

no reason, thx!

@TheBlueMatt@jkczyz I updated the logic in the latest commit but haven't updated the tests to match yet. Could you check my logic and then I'll update the tests?

Codecov Report

Merging #577 into master will increase coverage by 0.01%.
The diff coverage is 95.48%.

@@ Coverage Diff @@## master #577 +/- ##
==========================================
+ Coverage 91.26% 91.27% +0.01% 
==========================================
Files 35 35 Lines 20918 21122 +204 ==========================================
+ Hits 19090 19279 +189 - Misses 1828 1843 +15 

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	85.45% <90.00%> (-0.01%)	⬇️
lightning/src/ln/channel.rs	86.72% <91.89%> (+0.09%)	⬆️
lightning/src/ln/functional_tests.rs	97.11% <97.67%> (+<0.01%)	⬆️
lightning/src/ln/peer_handler.rs	55.00% <0.00%> (+0.85%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f08d610...17ccab4. Read the comment docs.

TODO: is the reason this added HTLC's amount can't count toward's the receiver's

I think that's preventive to avoid initiator getting under reserve if HTLC is cancelled at same time another HTLC is added.

Still wrapping my head around the fee spike buffer and how we're deviating from the spec. But wanted to leave some comments awhile.

I was thinking more on #577 (comment), my current thinking is that we might be able to do the following:

* Generalize the closure to handle the `!chan.get().is_usable()` case, taking the error code.
* Move that check into `Channel::update_add_htcl`
* Make the closure produce an entire `PendingHTLCStatus`, not just the `OnionErrorPacket`

This may also address #577 (comment).

I'm not positive this would work as there may be some subtly that I'm missing. But that's exactly why this could use some refactoring, IMHO. I think the advantage is that it keeps channel checks and updating pending_forward_info inside Channel rather than split between Channel and ChannelManager.

@TheBlueMatt Would welcome your insight on this.

Added in b36589c. It seems cleaner to me :)

This is redundant with the checks over the next few lines (and we shouldnt be failing the HTLC in this case, we should be failing the channel because our counterparty is misbehaving).

Oops @jkczyz I accidentally deleted your comment, lol. It said : "FWIW, I think the behavior here is the same as it had been before. Though maybe that wasn't desirable."

@TheBlueMatt It's not redundant because self.is_usable will return false if either party has sent a shutdown. If only we have sent a shutdown, we do indeed just want to fail the HTLC (which is why the htlc_fail_async_shutdown test exists.) Then the next check checks whether the remote has sent a shutdown, in which case we do want to fail the channel because that's a spec violation.

So, basically saying I think the logic is right here fcf5072. Though it might be more clear if we changed the is_usable check to more specifically check if only we have sent a shutdown.

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

Need to take a closer look at the test changes but wanted to publishes these draft comments that I had written earlier.

This comment is out-of-date now that create_pending_htlc_status no longer returns an Option.

In my previous suggestion that removed Option, I mentioned it may not be as intuitive to update pending_forward_state in this manner, though. A more intuitive approach could be to check against pending_forward_state for PendingHTLCStatus::Forward at each call site.

The disadvantage of this alternative is some code duplication. The advantage is it would be more explicit by clarifying when pending_forward_state is changed. So that may be a better approach as I think the comment would also no longer be necessary.

That said, you may have to make other fields public. And, there may be some borrow issues that make it less clean. So, up to you if you want to try it that way.

I guess I'm in favor of not increasing reasons that the Channel needs to know about the PendingHTLCStatus struct and I think it'd be better to work towards that struct being private to the ChannelManager to make this code more easily testable. I'm also pretty OK with the level of intuitiveness of the current code, plus increased conciseness is nice.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Cool! Was just looking through the tests and thinking if there were any other ways to go about it. Let me know if you want to run anything by me.

Looks great. A few nits and would be nice to get a bit more test coverage, but looks good and I'm still a big fan of the new fuzz coverage.

nit: the "as above" note here is a copy-paste bug (and why move this out of the else block where it makes the most sense?)

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

@ariard Hm, since it's a spec void, I'd think we'd default to giving them the benefit of the doubt & not closing?

Might be worth noting that, eventually, when we remove support for fee updates and switch to anchor-type fees, we can drop the 2x but we should keep the +1.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

@TheBlueMatt are you fine to remove the anchor outputs reference here (per Antoine's comment above ^)? I'm not too familiar with the proposal so don't have an opinion.

Can you explicit exactly where the spec deviation is ?

Clarified in the comment

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

If you set this to 0 the tests all still pass.

Hm, when I set this to 0 test_chan_reserve_violation_inbound_htlc_outbound_channel fails (as is the intention of the test). Would you mind double checking?

Commenting this line out and the tests still pass.

Sigh, yeah. Any advice on how to test this one? I'm thinking maybe I can add a function send_payment_test_only to channelmanager that sends payments w/o doing the normal checks? Can't figure out any way through the current public API atm.

Ongoing review, generally document exactly where we enforce each new spec point and deviation would be great for future reads

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

This is the spec deviation right ? Please at least add a comment for it and modify commit message to say we fail channel. Anyway that may not conjugate well with anchor output, where once implemented a initiator-sender consider it's always good to send a HTLC if above reserve because you can always bump ?

Sorry, don't believe this is a deviation. Curious why you think it's a deviation?
From the BOLT 2:

update_add_htlc
A sending node:
if it is responsible for paying the Bitcoin fee:
MUST NOT offer amount_msat if, after adding that HTLC to its commitment transaction, it cannot pay the fee for either the local or remote commitment transaction at the current feerate_per_kw while maintaining its channel reserve

AFAICT, this is a requirement on the sender but here we're at HTLC reception. It makes sense as that way you avoid to close channel even if sender violates spec. I think channel can still be unstuck if this HTLC is failed and sender-initiator balance is increased again without waiting feerate decrease.

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Ah but there's no fee spike buffer check here?

Here's the deviating fee spike buffer check: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1811-R1818

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Right, this check is intended to purely perform the check that you quote above here ^

After IRC discussion, this is right, I confused myself with what this check was enforcing, but still you should have point out the receiver spec-side :p

That's fair, lol

Saw the IRC discussion -- added clarifying comment https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1796

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

@TheBlueMatt

I don't get the rational between the difference in the way we account inbound HTLCs and outbound HTLCs. For any inbound HTLCs we evaluate as part of our balance independently of its state. We presume its resolution is favorable to us, even if it maybe not the case, which is reasonable without more tracking complexity.

Instead for outbound HTLC we account them in counterparty balance only after its removal announcement, independently of a success or failure. Why we don't also presume outbound HTLCs resolution are always favorable to them ? I.e accounting in their balance once we announce outbound HTLCs

I dont really get your question. We should not be assuming any kind of HTLC resolution here, nor do I think we do - we only ever consider HTLCs which have begun the removal process.

I know we talked about this offline but after reading conv again it was a still a misunderstood. I think we're making a different assumption for outbound, namely we don't consider that a committed outbound will resolve favorably for them.

Why we don't account OutboundHTLCState::Committed in part of their balance, removed_outbound_total_msat ?

I'm still a bit confused. Note that the "None" here means "there is no HTLCFailReason and the claim has resolved in our favor"

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

That's a nice improvement, this PR's pretty big already though, so I'd prefer to leave it as a follow-up?

@valentinewallace Sure will open documentation as a follow-up!

Why assign product of channel state comparison and then test this assignment instead of testing channel state directly ?

Just to make it readable and clear what's being checked. The variable makes it explicit that we're trying to check whether the local has sent a shutdown msg

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

Why we don't check that sender-initiator can also afford this new HTLC with regards to our local commitment transaction fee ?

That's what this is supposed to be doing: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1830. Though it also takes into account the channel reserve, which we discuss further below.

Code Review ACK d2ed6c1

Balances decrease may trigger a channel reserve or fee buffer spikes violation. There is 2 sources of balance decreases a) offering an HTLC and b) paying fees for the added weight of this HTLC. With regards to sender/receiver, a) is always static to evaluate as offers are unilateral in a single commit update, b) depends on channel funder. At HTLC sending, we're only concerned with checking b) as a potential counterparty violation (for security) and checking a) and b) as a violation of our own. At HTLC reception, we should only check a) and b) on counterparty. We deviate spec by 1) checking that we don't bypass our reserve, 2) checking fee buffer spikes integrity if peer is funder.

That said, I think we should move forward, and document behavior in follow-up. Likely spec should be also updated for clarification.

Great work Val !

[valentinewallace]

When we receive an inbound HTLC from a peer on an inbound channel,
the peer needs to be able to pay for the additional commitment tx
fees as well.

When we're sending an outbound HTLC on an outbound channel, we need
to be able to pay for the additional commitment tx fees as well.

Closes#548.

TODOs:

• add more comments to channel_reserve_test and added helper functions
• add impl + test Avoid stuck channels after fee increase with additional reserve lightning/bolts#740
• add a test that hits this case + update these docs https://github.com/rust-bitcoin/rust-lightning/blob/master/lightning/src/ln/channel.rs#L2043
• update the chanmon_consistency fuzz target to be able to hit this case

Oops, travis found a bug in your update for chanmon_consistency.rs. This fixes it:

diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index 4659d0afc..e31fea516 100644
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -641,12 +641,12 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
});
for event in events.drain(..) {
match event {
- events::Event::PaymentReceived { payment_hash, payment_secret, .. } => {
+ events::Event::PaymentReceived { payment_hash, payment_secret, amt } => {
if claim_set.insert(payment_hash.0) {
if $fail {
assert!(nodes[$node].fail_htlc_backwards(&payment_hash, &payment_secret));
} else {
- assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, 5_000_000));
+ assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, amt));
}
}
},

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

no reason, thx!

@TheBlueMatt@jkczyz I updated the logic in the latest commit but haven't updated the tests to match yet. Could you check my logic and then I'll update the tests?

Codecov Report

Merging #577 into master will increase coverage by 0.01%.
The diff coverage is 95.48%.

@@ Coverage Diff @@## master #577 +/- ##
==========================================
+ Coverage 91.26% 91.27% +0.01% 
==========================================
Files 35 35 Lines 20918 21122 +204 ==========================================
+ Hits 19090 19279 +189 - Misses 1828 1843 +15 

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	85.45% <90.00%> (-0.01%)	⬇️
lightning/src/ln/channel.rs	86.72% <91.89%> (+0.09%)	⬆️
lightning/src/ln/functional_tests.rs	97.11% <97.67%> (+<0.01%)	⬆️
lightning/src/ln/peer_handler.rs	55.00% <0.00%> (+0.85%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f08d610...17ccab4. Read the comment docs.

TODO: is the reason this added HTLC's amount can't count toward's the receiver's

I think that's preventive to avoid initiator getting under reserve if HTLC is cancelled at same time another HTLC is added.

Still wrapping my head around the fee spike buffer and how we're deviating from the spec. But wanted to leave some comments awhile.

I was thinking more on #577 (comment), my current thinking is that we might be able to do the following:

* Generalize the closure to handle the `!chan.get().is_usable()` case, taking the error code.
* Move that check into `Channel::update_add_htcl`
* Make the closure produce an entire `PendingHTLCStatus`, not just the `OnionErrorPacket`

This may also address #577 (comment).

I'm not positive this would work as there may be some subtly that I'm missing. But that's exactly why this could use some refactoring, IMHO. I think the advantage is that it keeps channel checks and updating pending_forward_info inside Channel rather than split between Channel and ChannelManager.

@TheBlueMatt Would welcome your insight on this.

Added in b36589c. It seems cleaner to me :)

This is redundant with the checks over the next few lines (and we shouldnt be failing the HTLC in this case, we should be failing the channel because our counterparty is misbehaving).

Oops @jkczyz I accidentally deleted your comment, lol. It said : "FWIW, I think the behavior here is the same as it had been before. Though maybe that wasn't desirable."

@TheBlueMatt It's not redundant because self.is_usable will return false if either party has sent a shutdown. If only we have sent a shutdown, we do indeed just want to fail the HTLC (which is why the htlc_fail_async_shutdown test exists.) Then the next check checks whether the remote has sent a shutdown, in which case we do want to fail the channel because that's a spec violation.

So, basically saying I think the logic is right here fcf5072. Though it might be more clear if we changed the is_usable check to more specifically check if only we have sent a shutdown.

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

Need to take a closer look at the test changes but wanted to publishes these draft comments that I had written earlier.

This comment is out-of-date now that create_pending_htlc_status no longer returns an Option.

In my previous suggestion that removed Option, I mentioned it may not be as intuitive to update pending_forward_state in this manner, though. A more intuitive approach could be to check against pending_forward_state for PendingHTLCStatus::Forward at each call site.

The disadvantage of this alternative is some code duplication. The advantage is it would be more explicit by clarifying when pending_forward_state is changed. So that may be a better approach as I think the comment would also no longer be necessary.

That said, you may have to make other fields public. And, there may be some borrow issues that make it less clean. So, up to you if you want to try it that way.

I guess I'm in favor of not increasing reasons that the Channel needs to know about the PendingHTLCStatus struct and I think it'd be better to work towards that struct being private to the ChannelManager to make this code more easily testable. I'm also pretty OK with the level of intuitiveness of the current code, plus increased conciseness is nice.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Cool! Was just looking through the tests and thinking if there were any other ways to go about it. Let me know if you want to run anything by me.

Looks great. A few nits and would be nice to get a bit more test coverage, but looks good and I'm still a big fan of the new fuzz coverage.

nit: the "as above" note here is a copy-paste bug (and why move this out of the else block where it makes the most sense?)

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

@ariard Hm, since it's a spec void, I'd think we'd default to giving them the benefit of the doubt & not closing?

Might be worth noting that, eventually, when we remove support for fee updates and switch to anchor-type fees, we can drop the 2x but we should keep the +1.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

@TheBlueMatt are you fine to remove the anchor outputs reference here (per Antoine's comment above ^)? I'm not too familiar with the proposal so don't have an opinion.

Can you explicit exactly where the spec deviation is ?

Clarified in the comment

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

If you set this to 0 the tests all still pass.

Hm, when I set this to 0 test_chan_reserve_violation_inbound_htlc_outbound_channel fails (as is the intention of the test). Would you mind double checking?

Commenting this line out and the tests still pass.

Sigh, yeah. Any advice on how to test this one? I'm thinking maybe I can add a function send_payment_test_only to channelmanager that sends payments w/o doing the normal checks? Can't figure out any way through the current public API atm.

Ongoing review, generally document exactly where we enforce each new spec point and deviation would be great for future reads

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

This is the spec deviation right ? Please at least add a comment for it and modify commit message to say we fail channel. Anyway that may not conjugate well with anchor output, where once implemented a initiator-sender consider it's always good to send a HTLC if above reserve because you can always bump ?

Sorry, don't believe this is a deviation. Curious why you think it's a deviation?
From the BOLT 2:

update_add_htlc
A sending node:
if it is responsible for paying the Bitcoin fee:
MUST NOT offer amount_msat if, after adding that HTLC to its commitment transaction, it cannot pay the fee for either the local or remote commitment transaction at the current feerate_per_kw while maintaining its channel reserve

AFAICT, this is a requirement on the sender but here we're at HTLC reception. It makes sense as that way you avoid to close channel even if sender violates spec. I think channel can still be unstuck if this HTLC is failed and sender-initiator balance is increased again without waiting feerate decrease.

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Ah but there's no fee spike buffer check here?

Here's the deviating fee spike buffer check: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1811-R1818

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Right, this check is intended to purely perform the check that you quote above here ^

After IRC discussion, this is right, I confused myself with what this check was enforcing, but still you should have point out the receiver spec-side :p

That's fair, lol

Saw the IRC discussion -- added clarifying comment https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1796

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

@TheBlueMatt

I don't get the rational between the difference in the way we account inbound HTLCs and outbound HTLCs. For any inbound HTLCs we evaluate as part of our balance independently of its state. We presume its resolution is favorable to us, even if it maybe not the case, which is reasonable without more tracking complexity.

Instead for outbound HTLC we account them in counterparty balance only after its removal announcement, independently of a success or failure. Why we don't also presume outbound HTLCs resolution are always favorable to them ? I.e accounting in their balance once we announce outbound HTLCs

I dont really get your question. We should not be assuming any kind of HTLC resolution here, nor do I think we do - we only ever consider HTLCs which have begun the removal process.

I know we talked about this offline but after reading conv again it was a still a misunderstood. I think we're making a different assumption for outbound, namely we don't consider that a committed outbound will resolve favorably for them.

Why we don't account OutboundHTLCState::Committed in part of their balance, removed_outbound_total_msat ?

I'm still a bit confused. Note that the "None" here means "there is no HTLCFailReason and the claim has resolved in our favor"

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

That's a nice improvement, this PR's pretty big already though, so I'd prefer to leave it as a follow-up?

@valentinewallace Sure will open documentation as a follow-up!

Why assign product of channel state comparison and then test this assignment instead of testing channel state directly ?

Just to make it readable and clear what's being checked. The variable makes it explicit that we're trying to check whether the local has sent a shutdown msg

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

Why we don't check that sender-initiator can also afford this new HTLC with regards to our local commitment transaction fee ?

That's what this is supposed to be doing: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1830. Though it also takes into account the channel reserve, which we discuss further below.

Code Review ACK d2ed6c1

Balances decrease may trigger a channel reserve or fee buffer spikes violation. There is 2 sources of balance decreases a) offering an HTLC and b) paying fees for the added weight of this HTLC. With regards to sender/receiver, a) is always static to evaluate as offers are unilateral in a single commit update, b) depends on channel funder. At HTLC sending, we're only concerned with checking b) as a potential counterparty violation (for security) and checking a) and b) as a violation of our own. At HTLC reception, we should only check a) and b) on counterparty. We deviate spec by 1) checking that we don't bypass our reserve, 2) checking fee buffer spikes integrity if peer is funder.

That said, I think we should move forward, and document behavior in follow-up. Likely spec should be also updated for clarification.

Great work Val !

[valentinewallace]

When we receive an inbound HTLC from a peer on an inbound channel,
the peer needs to be able to pay for the additional commitment tx
fees as well.

When we're sending an outbound HTLC on an outbound channel, we need
to be able to pay for the additional commitment tx fees as well.

Closes#548.

TODOs:

• add more comments to channel_reserve_test and added helper functions
• add impl + test Avoid stuck channels after fee increase with additional reserve lightning/bolts#740
• add a test that hits this case + update these docs https://github.com/rust-bitcoin/rust-lightning/blob/master/lightning/src/ln/channel.rs#L2043
• update the chanmon_consistency fuzz target to be able to hit this case

Oops, travis found a bug in your update for chanmon_consistency.rs. This fixes it:

diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index 4659d0afc..e31fea516 100644
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -641,12 +641,12 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
});
for event in events.drain(..) {
match event {
- events::Event::PaymentReceived { payment_hash, payment_secret, .. } => {
+ events::Event::PaymentReceived { payment_hash, payment_secret, amt } => {
if claim_set.insert(payment_hash.0) {
if $fail {
assert!(nodes[$node].fail_htlc_backwards(&payment_hash, &payment_secret));
} else {
- assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, 5_000_000));
+ assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, amt));
}
}
},

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

Haven't had a chance to review the tests, but this looks good to me. Is there a reason its still a draft?

no reason, thx!

@TheBlueMatt@jkczyz I updated the logic in the latest commit but haven't updated the tests to match yet. Could you check my logic and then I'll update the tests?

Codecov Report

Merging #577 into master will increase coverage by 0.01%.
The diff coverage is 95.48%.

@@ Coverage Diff @@## master #577 +/- ##
==========================================
+ Coverage 91.26% 91.27% +0.01% 
==========================================
Files 35 35 Lines 20918 21122 +204 ==========================================
+ Hits 19090 19279 +189 - Misses 1828 1843 +15 

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	85.45% <90.00%> (-0.01%)	⬇️
lightning/src/ln/channel.rs	86.72% <91.89%> (+0.09%)	⬆️
lightning/src/ln/functional_tests.rs	97.11% <97.67%> (+<0.01%)	⬆️
lightning/src/ln/peer_handler.rs	55.00% <0.00%> (+0.85%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f08d610...17ccab4. Read the comment docs.

TODO: is the reason this added HTLC's amount can't count toward's the receiver's

I think that's preventive to avoid initiator getting under reserve if HTLC is cancelled at same time another HTLC is added.

Still wrapping my head around the fee spike buffer and how we're deviating from the spec. But wanted to leave some comments awhile.

I was thinking more on #577 (comment), my current thinking is that we might be able to do the following:

* Generalize the closure to handle the `!chan.get().is_usable()` case, taking the error code.
* Move that check into `Channel::update_add_htcl`
* Make the closure produce an entire `PendingHTLCStatus`, not just the `OnionErrorPacket`

This may also address #577 (comment).

I'm not positive this would work as there may be some subtly that I'm missing. But that's exactly why this could use some refactoring, IMHO. I think the advantage is that it keeps channel checks and updating pending_forward_info inside Channel rather than split between Channel and ChannelManager.

@TheBlueMatt Would welcome your insight on this.

Added in b36589c. It seems cleaner to me :)

This is redundant with the checks over the next few lines (and we shouldnt be failing the HTLC in this case, we should be failing the channel because our counterparty is misbehaving).

Oops @jkczyz I accidentally deleted your comment, lol. It said : "FWIW, I think the behavior here is the same as it had been before. Though maybe that wasn't desirable."

@TheBlueMatt It's not redundant because self.is_usable will return false if either party has sent a shutdown. If only we have sent a shutdown, we do indeed just want to fail the HTLC (which is why the htlc_fail_async_shutdown test exists.) Then the next check checks whether the remote has sent a shutdown, in which case we do want to fail the channel because that's a spec violation.

So, basically saying I think the logic is right here fcf5072. Though it might be more clear if we changed the is_usable check to more specifically check if only we have sent a shutdown.

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

Need to take a closer look at the test changes but wanted to publishes these draft comments that I had written earlier.

This comment is out-of-date now that create_pending_htlc_status no longer returns an Option.

In my previous suggestion that removed Option, I mentioned it may not be as intuitive to update pending_forward_state in this manner, though. A more intuitive approach could be to check against pending_forward_state for PendingHTLCStatus::Forward at each call site.

The disadvantage of this alternative is some code duplication. The advantage is it would be more explicit by clarifying when pending_forward_state is changed. So that may be a better approach as I think the comment would also no longer be necessary.

That said, you may have to make other fields public. And, there may be some borrow issues that make it less clean. So, up to you if you want to try it that way.

I guess I'm in favor of not increasing reasons that the Channel needs to know about the PendingHTLCStatus struct and I think it'd be better to work towards that struct being private to the ChannelManager to make this code more easily testable. I'm also pretty OK with the level of intuitiveness of the current code, plus increased conciseness is nice.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Found a better way of testing so may want to hold off on further testing review til it's updated.

Cool! Was just looking through the tests and thinking if there were any other ways to go about it. Let me know if you want to run anything by me.

Looks great. A few nits and would be nice to get a bit more test coverage, but looks good and I'm still a big fan of the new fuzz coverage.

nit: the "as above" note here is a copy-paste bug (and why move this out of the else block where it makes the most sense?)

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

@ariard Hm, since it's a spec void, I'd think we'd default to giving them the benefit of the doubt & not closing?

Might be worth noting that, eventually, when we remove support for fee updates and switch to anchor-type fees, we can drop the 2x but we should keep the +1.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

@TheBlueMatt are you fine to remove the anchor outputs reference here (per Antoine's comment above ^)? I'm not too familiar with the proposal so don't have an opinion.

Can you explicit exactly where the spec deviation is ?

Clarified in the comment

Yea, a comment would be great here, at least, or just swap it for the explicit checks.

If you set this to 0 the tests all still pass.

Hm, when I set this to 0 test_chan_reserve_violation_inbound_htlc_outbound_channel fails (as is the intention of the test). Would you mind double checking?

Commenting this line out and the tests still pass.

Sigh, yeah. Any advice on how to test this one? I'm thinking maybe I can add a function send_payment_test_only to channelmanager that sends payments w/o doing the normal checks? Can't figure out any way through the current public API atm.

Ongoing review, generally document exactly where we enforce each new spec point and deviation would be great for future reads

We return a IgnoreError in get_channel_update for this but instead we should fail the channel as it's likely a hint of a buggy peer. Note: AFAICT, it's currently a spec void

This is the spec deviation right ? Please at least add a comment for it and modify commit message to say we fail channel. Anyway that may not conjugate well with anchor output, where once implemented a initiator-sender consider it's always good to send a HTLC if above reserve because you can always bump ?

Sorry, don't believe this is a deviation. Curious why you think it's a deviation?
From the BOLT 2:

update_add_htlc
A sending node:
if it is responsible for paying the Bitcoin fee:
MUST NOT offer amount_msat if, after adding that HTLC to its commitment transaction, it cannot pay the fee for either the local or remote commitment transaction at the current feerate_per_kw while maintaining its channel reserve

AFAICT, this is a requirement on the sender but here we're at HTLC reception. It makes sense as that way you avoid to close channel even if sender violates spec. I think channel can still be unstuck if this HTLC is failed and sender-initiator balance is increased again without waiting feerate decrease.

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Ah but there's no fee spike buffer check here?

Here's the deviating fee spike buffer check: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1811-R1818

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Right, this check is intended to purely perform the check that you quote above here ^

After IRC discussion, this is right, I confused myself with what this check was enforcing, but still you should have point out the receiver spec-side :p

That's fair, lol

Saw the IRC discussion -- added clarifying comment https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1796

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

@TheBlueMatt

I don't get the rational between the difference in the way we account inbound HTLCs and outbound HTLCs. For any inbound HTLCs we evaluate as part of our balance independently of its state. We presume its resolution is favorable to us, even if it maybe not the case, which is reasonable without more tracking complexity.

Instead for outbound HTLC we account them in counterparty balance only after its removal announcement, independently of a success or failure. Why we don't also presume outbound HTLCs resolution are always favorable to them ? I.e accounting in their balance once we announce outbound HTLCs

I dont really get your question. We should not be assuming any kind of HTLC resolution here, nor do I think we do - we only ever consider HTLCs which have begun the removal process.

I know we talked about this offline but after reading conv again it was a still a misunderstood. I think we're making a different assumption for outbound, namely we don't consider that a committed outbound will resolve favorably for them.

Why we don't account OutboundHTLCState::Committed in part of their balance, removed_outbound_total_msat ?

I'm still a bit confused. Note that the "None" here means "there is no HTLCFailReason and the claim has resolved in our favor"

@valentinewallace see ariard@9697578, this is a WIP for documenting better our HTLC acceptance policy. I think it's currently correct but that's quite a sensible part, in case of bug, a third-party may freely close our channels.

That's a nice improvement, this PR's pretty big already though, so I'd prefer to leave it as a follow-up?

@valentinewallace Sure will open documentation as a follow-up!

Why assign product of channel state comparison and then test this assignment instead of testing channel state directly ?

Just to make it readable and clear what's being checked. The variable makes it explicit that we're trying to check whether the local has sent a shutdown msg

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Can you explicit exactly where the spec deviation is ? Because on the feerate scope of the check, I think it's right, we add one HTLC, one more for the fee spike buffer and check feerate of the whole commitment tx per c-lightning interpretation.

However, and that's where our spec deviation is to me, this policy requirement doesn't exist on the receiver, it's only on the sender.

Also, reference to anchor output should be dropped. In fact if you can CPFP, feerate of the child may cover the HTLC feerate one so this check may be removed.

Note: that's not what the anchor output is recommending now but its scope is still unclear...

Why we don't check that sender-initiator can also afford this new HTLC with regards to our local commitment transaction fee ?

That's what this is supposed to be doing: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1830. Though it also takes into account the channel reserve, which we discuss further below.

Code Review ACK d2ed6c1

Balances decrease may trigger a channel reserve or fee buffer spikes violation. There is 2 sources of balance decreases a) offering an HTLC and b) paying fees for the added weight of this HTLC. With regards to sender/receiver, a) is always static to evaluate as offers are unilateral in a single commit update, b) depends on channel funder. At HTLC sending, we're only concerned with checking b) as a potential counterparty violation (for security) and checking a) and b) as a violation of our own. At HTLC reception, we should only check a) and b) on counterparty. We deviate spec by 1) checking that we don't bypass our reserve, 2) checking fee buffer spikes integrity if peer is funder.

That said, I think we should move forward, and document behavior in follow-up. Likely spec should be also updated for clarification.

Great work Val !