What the developer wrote
Authored by Bastien Teinturier
Don't store duplicate settlement messages (#3336)
If our peer is buggy, or if we have a bug in our message queue, we may
receive duplicate settlement messages for pending HTLCs. It is fine to
handle them and relay the settlement downstream (instead of immediately
force-closing), but we shouldn't store the duplicate settlement in the
remote changes, otherwise it will trigger a force-close when exchanging
`commit_sig`.
What changed, and why it matters
This patch fixes a bug in Eclair's Lightning payment channel code. If a buggy peer (or internal message queue bug) sent the same HTLC settlement message twice, Eclair would store the duplicate in its list of pending remote changes. Later, when signing the next commitment, that duplicate would be treated as a conflicting update and trigger an unnecessary force-close of the channel. The fix ignores duplicate settlement messages instead of storing them again, while still allowing the first one to be relayed downstream. It also still force-closes if a conflicting (different) settlement arrives for the same HTLC.
Treat as a defensive hardening/bugfix with security relevance: merge and backport to supported release branches. Operators should upgrade to avoid unnecessary channel force-closes caused by duplicate settlement messages from peers. No immediate incident response is required unless unexplained force-closes have been observed.
Security signals we found
Avoids unilateral channel force-close due to duplicate settlement messages
Prevents duplicate remote proposal accumulation in commitment changes
Maintains relay of first valid settlement to downstream HTLC origin
Still permits force-close when conflicting settlement contents are received
Adds unit tests for duplicate UpdateFulfillHtlc and UpdateFailHtlc handling
Evidence from the diff
In Commitments.scala, receiveFulfill, receiveFail, and receiveFailMalformed now check CommitmentChanges.alreadyProposed(changes.remoteChanges.proposed, htlc.id) before adding a remote proposal. If the HTLC is already in the remote proposed changes, the method returns Right(this, origin, htlc) without mutating changes. This prevents duplicate UpdateFulfillHtlc/UpdateFailHtlc/UpdateFailMalformedHtlc messages from being appended to remoteChanges.proposed, which previously caused a force-close during commit_sig exchange. The patch preserves the original settlement relay behavior and still allows a force-close path when a conflicting (different) settlement is received, because the duplicate guard only skips adding the proposal; signature validation later detects inconsistent settlement contents.
Changed components
eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scalaCommitments.receiveFulfillCommitments.receiveFailCommitments.receiveFailMalformedLightning channel normal-state HTLC settlement flowInspect captured patch +51 / −0
diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala
index 6dd63a4..359f244 100644
--- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala
+++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala
@@ -987,6 +987,10 @@ case class Commitments(channelParams: ChannelParams,
def receiveFulfill(fulfill: UpdateFulfillHtlc): Either[ChannelException, (Commitments, Origin, UpdateAddHtlc)] =
getOutgoingHtlcCrossSigned(fulfill.id) match {
case Some(htlc) if htlc.paymentHash == Crypto.sha256(fulfill.paymentPreimage) => originChannels.get(fulfill.id) match {
+ case Some(origin) if CommitmentChanges.alreadyProposed(changes.remoteChanges.proposed, htlc.id) =>
+ // We've already received a fail/fulfill for this HTLC, so we don't need to add it again to the remote changes.
+ // If it differs from the previous settlement message, our peer is buggy and we will force-close when exchanging commit_sig.
+ Right(this, origin, htlc)
case Some(origin) =>
payment.Monitoring.Metrics.recordOutgoingPaymentDistribution(remoteNodeId, htlc.amountMsat)
Right(copy(changes = changes.addRemoteProposal(fulfill)), origin, htlc)
@@ -1027,6 +1031,7 @@ case class Commitments(channelParams: ChannelParams,
def receiveFail(fail: UpdateFailHtlc): Either[ChannelException, (Commitments, Origin, UpdateAddHtlc)] =
getOutgoingHtlcCrossSigned(fail.id) match {
case Some(htlc) => originChannels.get(fail.id) match {
+ case Some(origin) if CommitmentChanges.alreadyProposed(changes.remoteChanges.proposed, htlc.id) => Right(this, origin, htlc)
case Some(origin) => Right(copy(changes = changes.addRemoteProposal(fail)), origin, htlc)
case None => Left(UnknownHtlcId(channelId, fail.id))
}
@@ -1040,6 +1045,7 @@ case class Commitments(channelParams: ChannelParams,
} else {
getOutgoingHtlcCrossSigned(fail.id) match {
case Some(htlc) => originChannels.get(fail.id) match {
+ case Some(origin) if CommitmentChanges.alreadyProposed(changes.remoteChanges.proposed, htlc.id) => Right(this, origin, htlc)
case Some(origin) => Right(copy(changes = changes.addRemoteProposal(fail)), origin, htlc)
case None => Left(UnknownHtlcId(channelId, fail.id))
}
diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala
index 095c31c..a6becf4 100644
--- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala
+++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala
@@ -1853,6 +1853,29 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
alice2blockchain.expectWatchTxConfirmed(tx.txid)
}
+ test("recv UpdateFulfillHtlc (duplicate)") { f =>
+ import f._
+
+ val (r, htlc) = addHtlc(50_000_000 msat, alice, bob, alice2bob, bob2alice)
+ crossSign(alice, bob, alice2bob, bob2alice)
+ bob ! CMD_FULFILL_HTLC(htlc.id, r, None)
+ val fulfill = bob2alice.expectMsgType[UpdateFulfillHtlc]
+ bob2alice.forward(alice, fulfill)
+ awaitCond(alice.commitments.changes.remoteChanges.proposed.contains(fulfill))
+ alice2relayer.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.RemoteFulfill]]
+ val remoteChanges = alice.commitments.changes.remoteChanges
+
+ // We relay fulfill commands (which are ignored downstream since they are duplicates) and ignore failures.
+ // We don't store the duplicate/conflicting message.
+ bob2alice.forward(alice, fulfill)
+ alice2relayer.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.RemoteFulfill]]
+ assert(alice.commitments.changes.remoteChanges == remoteChanges)
+ val fail = UpdateFailHtlc(fulfill.channelId, fulfill.id, randomBytes(292))
+ bob2alice.forward(alice, fail)
+ alice2relayer.expectNoMessage(100 millis)
+ assert(alice.commitments.changes.remoteChanges == remoteChanges)
+ }
+
private def testCmdFailHtlc(f: FixtureParam, commitmentFormat: CommitmentFormat): Unit = {
import f._
@@ -2096,6 +2119,28 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
alice2blockchain.expectWatchTxConfirmed(tx.txid)
}
+ test("recv UpdateFailHtlc (duplicate)") { f =>
+ import f._
+
+ val (r, htlc) = addHtlc(27_000_000 msat, alice, bob, alice2bob, bob2alice)
+ crossSign(alice, bob, alice2bob, bob2alice)
+ bob ! CMD_FAIL_HTLC(htlc.id, FailureReason.LocalFailure(PermanentChannelFailure()), None)
+ val fail = bob2alice.expectMsgType[UpdateFailHtlc]
+ bob2alice.forward(alice, fail)
+ awaitCond(alice.commitments.changes.remoteChanges.proposed.contains(fail))
+ alice2relayer.expectNoMessage(100 millis)
+ val remoteChanges = alice.commitments.changes.remoteChanges
+
+ // We ignore duplicate failures, but if we receive a fulfill we relay it downstream.
+ bob2alice.forward(alice, fail)
+ alice2relayer.expectNoMessage(100 millis)
+ assert(alice.commitments.changes.remoteChanges == remoteChanges)
+ val fulfill = UpdateFulfillHtlc(fail.channelId, fail.id, r)
+ bob2alice.forward(alice, fulfill)
+ alice2relayer.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.RemoteFulfill]]
+ assert(alice.commitments.changes.remoteChanges == remoteChanges)
+ }
+
test("recv UpdateFailHtlc (onion error bigger than recommended value)") { f =>
import f._
val (_, htlc) = addHtlc(50000000 msat, alice, bob, alice2bob, bob2alice)
Why this scored 60/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.