Avoid unusable channels after a large splice

[t-bast]

Splicing (and dual funding as well) introduce a new scenario that could not happen before, where the channel initiator (who pays the fees for the commit tx) can end up below the channel reserve, or right above it but any additional HTLC would make it go below the reserve.

In that case it means that most of the channels funds are on the non initiator side, so we should allow HTLCs from the non-initiator to the initiator to move funds towards the initiator (towards a more balanced channel where both sides meet the channel reserve requirements). We allow slightly dipping into the channel reserve in that case, for at most 5 pending HTLCs (to limit our exposure).

We limit this to only a few HTLCs to make sure that the initiator still has funds in the channel, that are close enough to their channel reserve. If they ended up with a commit tx where they have 0 sats, they would always have an incentive to broadcast this transaction once it's revoked, because they'd have nothing at stake in it.

This was proposed in the spec: https://github.com/lightning/bolts/pull/1083. But the other implementations didn't like it and preferred tracking the previous channel reserve (which was usually met before) until the new channel reserve is met. I don't like this because it's awkward to track (where would we put that state?) and not completely trivial to keep up-to-date. I believe that simply allowing a few HTLCs provides the same benefits (in practice that will stay within the bound of the previous channel reserve) with less complexity.

Note that on the receiver side, if the sender lets us dip into our channel reserve, we gladly accept it, because it's in our favor.

[codecov-commenter]

Codecov Report

Merging #2761 (3201d61) into master (9ca9227) will increase coverage by 0.01%. Report is 1 commits behind head on master. The diff coverage is 100.00%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master    #2761      +/-   ##
==========================================
+ Coverage   85.84%   85.85%   +0.01%     
==========================================
  Files         216      216              
  Lines       18102    18094       -8     
  Branches      762      768       +6     
==========================================
- Hits        15540    15535       -5     
+ Misses       2562     2559       -3     

Files	Coverage Δ
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala	93.39% <100.00%> (-0.03%)	:arrow_down:
...in/scala/fr/acinq/eclair/channel/Commitments.scala	96.82% <100.00%> (+<0.01%)	:arrow_up:
...cinq/eclair/remote/EclairInternalsSerializer.scala	97.82% <100.00%> (ø)
...scala/fr/acinq/eclair/router/BalanceEstimate.scala	98.93% <100.00%> (ø)
.../src/main/scala/fr/acinq/eclair/router/Graph.scala	97.25% <100.00%> (-0.09%)	:arrow_down:
...cala/fr/acinq/eclair/router/RouteCalculation.scala	94.56% <100.00%> (ø)
...src/main/scala/fr/acinq/eclair/router/Router.scala	94.78% <100.00%> (ø)
...main/scala/fr/acinq/eclair/router/Validation.scala	93.75% <100.00%> (-1.25%)	:arrow_down:

... and 3 files with indirect coverage changes

[t-bast]

Is there already a test that exercises the case where bob (sender/non-initiator) rejects an htlc because the commit fees would take alice (receiver/initiator) below the commit-tx fee limit?

Good point, we test that in the update_fee case (see recv UpdateFee (sender can't afford it)), but not in the HTLC case. I'll add a test case for that.

[t-bast]

Is there already a test that exercises the case where bob (sender/non-initiator) rejects an htlc because the commit fees would take alice (receiver/initiator) below the commit-tx fee limit?

Actually I was wrong, there are already tests for that (it would have been surprising that we missed that scenario):

• recv UpdateAddHtlc (insufficient funds w/ pending htlcs 1/2 verifies that if the initiator sends us an HTLC that makes them dip into their reserve, we force-close
• recv UpdateAddHtlc (insufficient funds w/ pending htlcs) (anchor outputs) verifies that if the initiator sends us an HTLC that would make their balance go exactly to 0, we force-close
• recv CMD_ADD_HTLC (insufficient funds, missing 1 msat) verifies that we don't send HTLCs that would make us go below our reserve if we're the initiator
• recv CMD_ADD_HTLC (HTLC dips into remote funder fee reserve) tests that if the non-initiator sends us an HTLC that makes us (initiator) go below our reserve, we accept it (because it's in our benefit)

[remyers]

Is there already a test that exercises the case where bob (sender/non-initiator) rejects an htlc because the commit fees would take alice (receiver/initiator) below the commit-tx fee limit?

Actually I was wrong, there are already tests for that (it would have been surprising that we missed that scenario):

While these tests do cover this, none of them specifically exercise the clause:

} else if (reduced.toLocal > fees && reduced.htlcs.size < 5) {

where reduced.toLocal <= fees and reduced.htlcs.size < 5. Only with dust and reserve limits set as low as possible (at an unrealistic 354 sats) could I trigger reduced.toLocal <= fees, but it required 8 htlcs. Basically the reudced.htlcs.size < 5 clause would have been sufficient.

My conclusion is that although we don't actually need a separate test for reduced.toLocal <= fees, it's still good to have that clause as a fail safe.