What the developer wrote
Authored by Bastien Teinturier
Multiple bug fixes found by AI scanning (#3346)
* Fix 0-fee commitments fee bumping with remote-main
When using 0-fee commitments, if our peer publishes their commitment
and our main output is large enough, we don't need to use a wallet
input to pay RBF fees, we can directly do it using our main output.
However, we didn't take into account the whole package weight when
doing so: this means that there was a balance range where we would
not correctly bump the commit tx to the target feerate.
* Retry transaction publishing on unclassified failures
We classify bitcoind publish failures by substring-matching the RPC
error message. Any rejection that doesn't match any of the RPC error
messages that we know would previously not be retried.
This was dangerous, as it could cause us to abandon a time-sensitive
transaction, which could cause loss of funds. We now eagerly retry
when we cannot classify the RPC publish failure.
* Fix `channel_ready` using invalid musig2 nonce if RBF
We must use `commitments1` instead of `d.commitments` when creating our
`channel_ready` message to ensure that we use the correct musig2 nonce
for taproot channels. Otherwise, if we have multiple RBF attempts and
the ony confirming is *not* the latest one, we will use the wrong nonce.
* Fix out-of-bound array access in payment FSM
Remote failures can be manipulated to trigger an out-of-bound array
access which crashes the payment lifecycle actor.
What changed, and why it matters
This commit fixes four separate bugs in the Eclair Lightning node, all reported as found by AI scanning. Two of the bugs can cause loss of funds: one miscalculates transaction fees when bumping a channel close, and another could abandon time-sensitive transactions on unrecognized Bitcoin RPC errors. A third bug could use the wrong cryptographic nonce after a channel funding RBF, and a fourth could crash the payment actor via a maliciously crafted remote failure message. The commit is a straightforward set of fixes, but it is partial in the sense that it does not include broader hardening beyond the specific bugs identified.
Apply the patch promptly. Nodes running with zero-fee commitments or taproot channels should prioritize this update because of the direct fund-loss scenarios. Operators should monitor transaction publishing logs for repeated retries after unknown bitcoind errors to ensure they do not mask a different operational problem.
Security signals we found
Loss-of-funds risk from incorrect RBF fee calculation in zero-fee commitment handling
Loss-of-funds risk from abandoning time-sensitive transactions on unrecognized bitcoind RPC errors
Invalid MuSig2 nonce usage in channel_ready after RBF for taproot channels
Out-of-bounds array access crash in payment FSM from manipulated remote failure index
Fixes introduced under title 'Multiple bug fixes found by AI scanning'
Evidence from the diff
The commit patches four issues: (1) ZeroFeeCommitmentFormat fee bumping now accounts for the full package weight (commit tx + main tx + anchor input weight) and dust limit when deciding whether the main output can cover RBF fees; (2) TxPublisher now retries on UnknownTxFailure instead of giving up, preventing abandonment of time-sensitive transactions when bitcoind returns unrecognized RPC errors; (3) ChannelOpenDualFunded now uses the post-RBF commitments1 when constructing channel_ready, ensuring the correct MuSig2 nonce is used for taproot channels when a non-latest RBF attempt confirms; (4) PaymentLifecycle now validates the index in DecryptedFailurePacket before indexing route.hops, preventing an out-of-bounds array access that could crash the payment FSM on a manipulated remote failure.
Changed components
eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scalaeclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenDualFunded.scalaeclair-core/src/main/scala/fr/acinq/eclair/channel/publish/TxPublisher.scalaeclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scalaInspect captured patch +38 / −27
### eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala
@@ -1209,9 +1209,13 @@ object Helpers {
// In that case, we don't need to create a dedicated anchor transaction which avoids using wallet inputs.
val useMainTxForAnchor = commitment.commitmentFormat match {
case _: AnchorOutputsCommitmentFormat | _: SimpleTaprootChannelCommitmentFormat => false
- case ZeroFeeCommitmentFormat =>
- val commitFee = Transactions.weight2fee(feerates.fastest, commitTx.weight())
- mainTx_opt.exists(_.tx.txOut.map(_.amount).sum > commitFee)
+ case ZeroFeeCommitmentFormat => mainTx_opt match {
+ case Some(mainTx) =>
+ val packageWeight = commitTx.weight() + mainTx.expectedWeight + commitment.commitmentFormat.anchorInputWeight
+ val packageFee = Transactions.weight2fee(feerates.fastest, packageWeight)
+ mainTx.tx.txOut.map(_.amount).sum > packageFee + commitment.localCommitParams.dustLimit
+ case None => false
+ }
}
val spendAnchor = incomingHtlcs.nonEmpty || outgoingHtlcs.nonEmpty || spendAnchorWithoutHtlcs
val anchorTx_opt = if (spendAnchor && !useMainTxForAnchor) {
### eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenDualFunded.scala
@@ -759,7 +759,7 @@ trait ChannelOpenDualFunded extends DualFundingHandlers with ErrorHandlers {
// We still watch the funding tx for confirmation even if we can use the zero-conf channel right away.
watchFundingConfirmed(w.tx.txid, Some(nodeParams.channelConf.minDepth), delay_opt = None)
val shortIds = createShortIdAliases(d.channelId)
- val channelReady = createChannelReady(shortIds, d.commitments)
+ val channelReady = createChannelReady(shortIds, commitments1)
d.deferred.foreach(self ! _)
invalidTxSigsReceived = Map.empty
goto(WAIT_FOR_DUAL_FUNDING_READY) using DATA_WAIT_FOR_DUAL_FUNDING_READY(commitments1, shortIds) storing() sending channelReady
@@ -770,7 +770,7 @@ trait ChannelOpenDualFunded extends DualFundingHandlers with ErrorHandlers {
acceptFundingTxConfirmed(w, d) match {
case Right((commitments1, _)) =>
val shortIds = createShortIdAliases(d.channelId)
- val channelReady = createChannelReady(shortIds, d.commitments)
+ val channelReady = createChannelReady(shortIds, commitments1)
reportRbfFailure(d.status, InvalidRbfTxConfirmed(d.channelId))
val toSend = d.status match {
case DualFundingStatus.WaitingForConfirmations | DualFundingStatus.RbfAborted => Seq(channelReady)
### eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/TxPublisher.scala
@@ -310,8 +310,9 @@ private class TxPublisher(nodeParams: NodeParams, factory: TxPublisher.ChildFact
// sense to retry.
run(pending2, retryNextBlock, channelContext)
case TxRejectedReason.UnknownTxFailure =>
- // We don't automatically retry unknown failures, they should be investigated manually.
- run(pending2, retryNextBlock, channelContext)
+ // We automatically retry unknown failures to ensure that we don't skip time-sensitive transactions if
+ // bitcoind adds a new failure message that isn't caught by our parser.
+ run(pending2, retryNextBlock ++ rejectedAttempts.map(_.cmd), channelContext)
}
}
### eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala
@@ -203,24 +203,29 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
Metrics.PaymentError.withTag(Tags.Failure, Tags.FailureType(UnreadableRemoteFailure(request.amount, Nil, e, startedAt = d.sentAt, failedAt = now, htlcFailure.holdTimes))).increment()
failure
}) match {
- case res@Right(Sphinx.DecryptedFailurePacket(_, index, failureMessage)) =>
- // We have discovered some liquidity information with this payment: we update the router accordingly.
- val stoppedRoute = route.stopAt(index)
- if (stoppedRoute.hops.length > 1) {
- router ! Router.RouteCouldRelay(stoppedRoute)
- }
- failureMessage match {
- case TemporaryChannelFailure(update_opt, _) =>
- val failingHop = route.hops(index)
- val isLiquidityIssue = update_opt match {
- // If the relay parameters have changed, it's not necessarily a liquidity issue.
- case Some(update) => HopRelayParams.areSame(failingHop.params, HopRelayParams.FromAnnouncement(update), ignoreHtlcSize = true)
- case None => true
- }
- if (isLiquidityIssue) {
- router ! Router.ChannelCouldNotRelay(stoppedRoute.amount, failingHop)
- }
- case _ => // other errors should not be used for liquidity issues
+ case res@Right(Sphinx.DecryptedFailurePacket(nodeId, index, failureMessage)) =>
+ val isRecipient = nodeId == recipient.nodeId || route.finalHop_opt.collect { case h: NodeHop => h.nodeId }.contains(nodeId)
+ if (!isRecipient && index >= 1 && index < route.hops.length) {
+ // We have discovered some liquidity information with this payment: we update the router accordingly.
+ val stoppedRoute = route.stopAt(index)
+ if (stoppedRoute.hops.length > 1) {
+ router ! Router.RouteCouldRelay(stoppedRoute)
+ }
+ failureMessage match {
+ case TemporaryChannelFailure(update_opt, _) =>
+ val failingHop = route.hops(index)
+ val isLiquidityIssue = update_opt match {
+ // If the relay parameters have changed, it's not necessarily a liquidity issue.
+ case Some(update) => HopRelayParams.areSame(failingHop.params, HopRelayParams.FromAnnouncement(update), ignoreHtlcSize = true)
+ case None => true
+ }
+ if (isLiquidityIssue) {
+ router ! Router.ChannelCouldNotRelay(stoppedRoute.amount, failingHop)
+ }
+ case _ => // other errors should not be used for liquidity issues
+ }
+ } else if (!isRecipient) {
+ log.warning("ignoring failure with invalid route position: node={} index={} hops={}", nodeId, index, route.hops.length)
}
res
case res => res
### eclair-core/src/test/scala/fr/acinq/eclair/channel/publish/TxPublisherSpec.scala
@@ -33,7 +33,6 @@ import fr.acinq.eclair.transactions.Transactions._
import fr.acinq.eclair.{BlockHeight, CltvExpiry, NodeParams, TestConstants, TestKitBaseClass, randomBytes32, randomBytes64, randomKey}
import org.scalatest.Outcome
import org.scalatest.funsuite.FixtureAnyFunSuiteLike
-import scodec.bits.ByteVector
import java.util.UUID
import scala.concurrent.duration.DurationInt
@@ -359,8 +358,10 @@ class TxPublisherSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike {
txPublisher ! TxRejected(attempt.id, cmd, UnknownTxFailure)
attempt.actor.expectMsg(FinalTxPublisher.Stop)
- // We don't retry, even after a new block has been found:
+ // We retry after a new block has been found:
system.eventStream.publish(CurrentBlockHeight(BlockHeight(8200)))
+ val attempt2 = factory.expectMsgType[FinalTxPublisherSpawned]
+ assert(attempt2.actor.expectMsgType[FinalTxPublisher.Publish].cmd == cmd)
factory.expectNoMessage(100 millis)
}
Why this scored 74/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.