During mutual close negotiation, Eclair might not send last closing_signed msg

[SomberNight]

During mutual close negotiation, Eclair sometimes does not send the last closing_signed msg.
Specifically I am talking about this part of BOLT-02:

https://github.com/lightningnetwork/lightning-rfc/blob/b201efe0546120c14bf154ce5f4e18da7243fe7a/02-peer-protocol.md#requirements-6

Closing Negotiation: closing_signed
Requirements
The receiving node:

• if the receiver agrees with the fee:
  • SHOULD reply with a closing_signed with the same fee_satoshis value.

Consider a channel between Alice and Bob, with Bob running Eclair.
If Alice sends a closing_signed with a fee_satoshis Bob agrees with,
I've found that

• sometimes Bob replies with a closing_signed with the same fee_satoshis value,
• other times Bob does not reply and just broadcasts the on-chain tx.

I am looking at this part of the code:

eclair/eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala

Lines 1223 to 1227 in c37eb1a

	if (lastLocalClosingFee.contains(nextClosingFee)) {
	// next computed fee is the same than the one we previously sent (probably because of rounding), let's close now
	handleMutualClose(signedClosingTx, Left(d.copy(bestUnpublishedClosingTx_opt = Some(signedClosingTx))))
	} else if (nextClosingFee == remoteClosingFee) {
	// we have converged!

eclair/eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala

Line 443 in c37eb1a

def nextClosingFee(localClosingFee: Satoshi, remoteClosingFee: Satoshi): Satoshi = ((localClosingFee + remoteClosingFee) / 4) * 2

---

Consider scenario1:

• Alice sends closing_signed with fee_satoshis = 504,
• Bob sends closing_signed with fee_satoshis = 500,
• Alice sends closing_signed with fee_satoshis = 503,
• Now I think Bob will calculate nextClosingFee to be (500 + 503) // 4 * 2 == 500,
  which matches what he sent last, and will not reply, just broadcast the tx (with fee_satoshis = 503).

Ideally, Bob should send a final closing_signed with fee_satoshis = 503,
as this is what BOLT-02 says he should do.

---

Consider scenario2:

• Alice sends closing_signed with fee_satoshis = 508,
• Bob sends closing_signed with fee_satoshis = 500,
• Alice sends closing_signed with fee_satoshis = 502,
• Now I think Bob will calculate nextClosingFee to be (500 + 504) // 4 * 2 == 502,
  so Bob enters the "we have converged!" branch,
  replies with a closing_signed with fee_satoshis = 502,
  and broadcast the tx (with fee_satoshis = 502)

(so scenario2 works out as expected)

---

So my issue is that - like the comment in the code suggests - due to rounding errors, to an outside observer Eclair behaves probabilistically: sometimes it sends the last closing_signed messages, sometimes it does not.

[t-bast]

Why is that a problem?

As the spec says, it's only a SHOULD, not a MUST, and the mutual close ends with an agreed upon tx on-chain.

[SomberNight]

Well, sure, it is not a critical issue. Still it is a SHOULD, so ideally it would behave like that.

Clearly whoever wrote the code agrees too. That is, if you believe there is no point for Bob to send a closing_signed if it agrees with the received fee_satoshis then the second branch is much more complicated than need be; indeed the branches could be merged.

eclair/eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala

Lines 1223 to 1231 in c37eb1a

	if (lastLocalClosingFee.contains(nextClosingFee)) {
	// next computed fee is the same than the one we previously sent (probably because of rounding), let's close now
	handleMutualClose(signedClosingTx, Left(d.copy(bestUnpublishedClosingTx_opt = Some(signedClosingTx))))
	} else if (nextClosingFee == remoteClosingFee) {
	// we have converged!
	val closingTxProposed1 = d.closingTxProposed match {
	case previousNegotiations :+ currentNegotiation => previousNegotiations :+ (currentNegotiation :+ ClosingTxProposed(closingTx, closingSigned))
	}
	handleMutualClose(signedClosingTx, Left(d.copy(closingTxProposed = closingTxProposed1, bestUnpublishedClosingTx_opt = Some(signedClosingTx)))) sending closingSigned

I've found this issue while using Electrum as there currently the GUI only gives positive feedback to the user that the mutual close worked when receiving the final closing_signed. Though clearly Electrum should not rely on behaviour that is just a SHOULD and not a MUST.

[t-bast]

I agree, it would be nice to fix this, but it's quite low priority.
If someone wants to take a stab at it though, contributions are welcome!