More flexible mutual close fees

[t-bast]

This PR adds support for lightning/bolts#847
The fees negotiation has been refactored and more tests have been added.
With legacy nodes, we will keep the existing behavior (slowly converge on a fee).
But for nodes that support closing fee_ranges, mutual close will take much less round-trips.

It's possible to chose your desired feerates through new fields of the close API.
Note however that they will be ignored if you're fundee, as you won't be the one initiating fee negotiation.
It's ok as you're not paying that fee and can rely on CPFP if you want to speed up confirmation (or even simply spend your unconfirmed output directly when you need it).

This PR fixes #1742 and #1583

[codecov-commenter]

Codecov Report

Merging #1768 (4ce2dde) into master (daace53) will decrease coverage by 0.03%.
The diff coverage is 2.56%.

@@            Coverage Diff            @@
##           master   #1768      +/-   ##
=========================================
- Coverage    8.99%   8.96%   -0.04%     
=========================================
  Files         158     158              
  Lines       12273   12330      +57     
  Branches      531     530       -1     
=========================================
+ Hits         1104    1105       +1     
- Misses      11169   11225      +56     

Impacted Files	Coverage Δ
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala	0.00% <0.00%> (ø)
...c/main/scala/fr/acinq/eclair/channel/Channel.scala	0.78% <0.00%> (-0.03%)	⬇️
...in/scala/fr/acinq/eclair/channel/ChannelData.scala	0.00% <0.00%> (ø)
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala	0.60% <0.00%> (-0.01%)	⬇️
...ire/internal/channel/version0/ChannelCodecs0.scala	0.00% <0.00%> (ø)
...ire/internal/channel/version1/ChannelCodecs1.scala	0.00% <0.00%> (ø)
...ire/internal/channel/version2/ChannelCodecs2.scala	0.00% <0.00%> (ø)
...ire/internal/channel/version3/ChannelCodecs3.scala	0.00% <0.00%> (ø)
...q/eclair/wire/protocol/LightningMessageTypes.scala	25.80% <0.00%> (-0.87%)	⬇️
...ala/fr/acinq/eclair/wire/protocol/ChannelTlv.scala	85.00% <100.00%> (+1.66%)	⬆️
... and 1 more

[pm47]

Have you considered type-ifying the comparison of local_opt/remote closing fees? I'm not sure this would make the negotiation code simpler, but at least we would factor the state transitions instead of having multiple calls to handleMutualClose etc.

def Closing.compare(localClosingSigned_opt, remoteClosingSigned): NegotiationResult = ???

sealed trait NegotiationResult 
object NegotiationResult {
  sealed trait Success extends NegotiationResult
  case class TheyAcceptOurFee(...) extends Success
  case class WeAcceptTheirFee(...) extends Success
  sealed trait Failure extends NegotiationResult
  
}

Closing.checkClosingSignature(..) match {
  case Right(...) =>
    Closing.compare(localClosingSigned_opt, remoteClosingSigned) =>
      case Success(...) =>
        handleMutualClose(...)
      case Failure(...) =>
        stay()
  case Left(...) =>
    handleLocalError(...)
}

If it doesn't feel very clear, it's because it isn't 😅 .

Or we could also wait until we deprecate the iterative negotiation process and simplify the whole thing.

[pm47]

Side note: we should probably factorize this tlv codec boiler plate.

[t-bast]

Have you considered type-ifying the comparison of local_opt/remote closing fees?

I'll give it a try and we'll see if it's worth doing now or when we remove the legacy negotiation.

[t-bast]

Have you considered type-ifying the comparison of local_opt/remote closing fees?

I tried it but got lost in it, it quickly becomes a bit complex and increases the risk we introduce a regression...
I think we should revisit that when we remove the old negotiation and simplify that code.

[t-bast]

The corresponding spec PR has been merged, so we can (finally) merge this 🎉

[pm47]

LGTM

[pm47]

I would wait for cross-testing before merging, WDYT

[t-bast]

I would wait for cross-testing before merging, WDYT

I've tested cross-compatibility with c-lightning based on ElementsProject/lightning#4599 a while ago and everything looked good, so I think we can merge now. I'll test compatibility again once they merge this PR on their master branch, and we can adjust if needed.

I wanted to also test against rust-lightning, but couldn't get the LDK running against the right branch where they implemented this quick close, I'll give it another try.

[t-bast]

I wanted to also test against rust-lightning, but couldn't get the LDK running against the right branch where they implemented this quick close, I'll give it another try.

I've been able to test against the latest LDK and it works, there's one small bug on their side but apart from that it looks good.
I'll open an issue on their repo for the bug I found.