From e1cee96c120c83839e0776a3421edf73120c43ba Mon Sep 17 00:00:00 2001 From: Bastien Teinturier <31281497+t-bast@users.noreply.github.com> Date: Thu, 23 Mar 2023 18:50:35 +0100 Subject: [PATCH] Fix opportunistic zero-conf (#2616) If we didn't plan on using zero-conf, but our peer sends us an early channel_ready, we can opportunistically switch to zero-conf. But we can only do that if we're sure that our peer cannot double-spend the funding transaction. We previously checked their contribution to the funding output, but that's not enough: they may add inputs to the funding transaction even if they don't contribute to the funding output. We were also setting duplicate `WatchPublished` in case we were already using zero-conf, which is now fixed. When our peer sends us channel_ready while we're still waiting for confirmations, we may opportunistically switch to zero-conf, in which case we have both a WatchPublished and a WatchConfirmed pending. But it may not actually be a real switch to zero-conf: maybe the transaction is confirmed, and they simply received the block slightly before us. In that case, the WatchConfirmed may trigger first, and it would be inefficient to let the WatchPublished override our funding status: it will make us set a new WatchConfirmed that will instantly trigger and rewrite the funding status again. --- .../fr/acinq/eclair/channel/Helpers.scala | 2 +- .../fr/acinq/eclair/channel/fsm/Channel.scala | 61 +++++++++++-------- .../channel/fsm/ChannelOpenDualFunded.scala | 11 +--- .../channel/fsm/ChannelOpenSingleFunded.scala | 13 +++- .../channel/fsm/DualFundingHandlers.scala | 18 +++++- ...WaitForDualFundingConfirmedStateSpec.scala | 30 +++++++++ 6 files changed, 96 insertions(+), 39 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala index c3ec0c52d..3c2596faf 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala @@ -379,7 +379,7 @@ object Helpers { /** * When using dual funding, we wait for multiple confirmations even if we're the initiator because: - * - our peer may also contribute to the funding transaction + * - our peer may also contribute to the funding transaction, even if they don't contribute to the channel funding amount * - even if they don't, we may RBF the transaction and don't want to handle reorgs */ def minDepthDualFunding(channelConf: ChannelConf, localFeatures: Features[InitFeature], isInitiator: Boolean, localAmount: Satoshi, remoteAmount: Satoshi): Option[Long] = { diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala index fd1a65a43..3a7e95ec9 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala @@ -1563,31 +1563,42 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with case Event(TickChannelOpenTimeout, _) => stay() case Event(w: WatchPublishedTriggered, d: PersistentChannelData) => - val fundingStatus = LocalFundingStatus.ZeroconfPublishedFundingTx(w.tx) - d.commitments.updateLocalFundingStatus(w.tx.txid, fundingStatus) match { - case Right((commitments1, _)) => - log.info(s"zero-conf funding txid=${w.tx.txid} has been published") - watchFundingConfirmed(w.tx.txid, Some(nodeParams.channelConf.minDepthBlocks)) - val d1 = d match { - // NB: we discard remote's stashed channel_ready, they will send it back at reconnection - case d: DATA_WAIT_FOR_FUNDING_CONFIRMED => - val realScidStatus = RealScidStatus.Unknown - val shortIds = createShortIds(d.channelId, realScidStatus) - DATA_WAIT_FOR_CHANNEL_READY(commitments1, shortIds) - case d: DATA_WAIT_FOR_DUAL_FUNDING_CONFIRMED => - val realScidStatus = RealScidStatus.Unknown - val shortIds = createShortIds(d.channelId, realScidStatus) - DATA_WAIT_FOR_DUAL_FUNDING_READY(commitments1, shortIds) - case d: DATA_WAIT_FOR_CHANNEL_READY => d.copy(commitments = commitments1) - case d: DATA_WAIT_FOR_DUAL_FUNDING_READY => d.copy(commitments = commitments1) - case d: DATA_NORMAL => d.copy(commitments = commitments1) - case d: DATA_SHUTDOWN => d.copy(commitments = commitments1) - case d: DATA_NEGOTIATING => d.copy(commitments = commitments1) - case d: DATA_WAIT_FOR_REMOTE_PUBLISH_FUTURE_COMMITMENT => d.copy(commitments = commitments1) - case d: DATA_CLOSING => d.copy(commitments = commitments1) - } - stay() using d1 storing() - case Left(_) => stay() + // When our peer sends us channel_ready while we're still waiting for confirmations, we may opportunistically + // switch to zero-conf, in which case we have both a WatchPublished and a WatchConfirmed pending. But it may not + // actually be a real switch to zero-conf: maybe the transaction is confirmed, and they simply received the block + // slightly before us. In that case, the WatchConfirmed may trigger first, and it would be inefficient to let the + // WatchPublished override our funding status: it will make us set a new WatchConfirmed that will instantly + // trigger and rewrite the funding status again. + val alreadyConfirmed = d.commitments.active.map(_.localFundingStatus).collect { case LocalFundingStatus.ConfirmedFundingTx(tx) => tx }.exists(_.txid == w.tx.txid) + if (alreadyConfirmed) { + stay() + } else { + val fundingStatus = LocalFundingStatus.ZeroconfPublishedFundingTx(w.tx) + d.commitments.updateLocalFundingStatus(w.tx.txid, fundingStatus) match { + case Right((commitments1, _)) => + log.info(s"zero-conf funding txid=${w.tx.txid} has been published") + watchFundingConfirmed(w.tx.txid, Some(nodeParams.channelConf.minDepthBlocks)) + val d1 = d match { + // NB: we discard remote's stashed channel_ready, they will send it back at reconnection + case d: DATA_WAIT_FOR_FUNDING_CONFIRMED => + val realScidStatus = RealScidStatus.Unknown + val shortIds = createShortIds(d.channelId, realScidStatus) + DATA_WAIT_FOR_CHANNEL_READY(commitments1, shortIds) + case d: DATA_WAIT_FOR_DUAL_FUNDING_CONFIRMED => + val realScidStatus = RealScidStatus.Unknown + val shortIds = createShortIds(d.channelId, realScidStatus) + DATA_WAIT_FOR_DUAL_FUNDING_READY(commitments1, shortIds) + case d: DATA_WAIT_FOR_CHANNEL_READY => d.copy(commitments = commitments1) + case d: DATA_WAIT_FOR_DUAL_FUNDING_READY => d.copy(commitments = commitments1) + case d: DATA_NORMAL => d.copy(commitments = commitments1) + case d: DATA_SHUTDOWN => d.copy(commitments = commitments1) + case d: DATA_NEGOTIATING => d.copy(commitments = commitments1) + case d: DATA_WAIT_FOR_REMOTE_PUBLISH_FUTURE_COMMITMENT => d.copy(commitments = commitments1) + case d: DATA_CLOSING => d.copy(commitments = commitments1) + } + stay() using d1 storing() + case Left(_) => stay() + } } case Event(w: WatchFundingConfirmedTriggered, d: PersistentChannelData) => diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenDualFunded.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenDualFunded.scala index cad5fcf03..a8030fbdf 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenDualFunded.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenDualFunded.scala @@ -30,7 +30,7 @@ import fr.acinq.eclair.channel.publish.TxPublisher.SetChannelId import fr.acinq.eclair.crypto.ShaChain import fr.acinq.eclair.transactions.Scripts import fr.acinq.eclair.wire.protocol._ -import fr.acinq.eclair.{Features, RealShortChannelId, ToMilliSatoshiConversion, UInt64} +import fr.acinq.eclair.{RealShortChannelId, ToMilliSatoshiConversion, UInt64} /** * Created by t-bast on 19/04/2022. @@ -618,15 +618,8 @@ trait ChannelOpenDualFunded extends DualFundingHandlers with ErrorHandlers { case Event(e: BITCOIN_FUNDING_DOUBLE_SPENT, d: DATA_WAIT_FOR_DUAL_FUNDING_CONFIRMED) => handleDualFundingDoubleSpent(e, d) case Event(remoteChannelReady: ChannelReady, d: DATA_WAIT_FOR_DUAL_FUNDING_CONFIRMED) => - // We can skip waiting for confirmations if: - // - there is a single version of the funding tx (otherwise we don't know which one to use) - // - they didn't contribute to the funding output or we trust them to not double-spend - val canUseZeroConf = remoteChannelReady.alias_opt.isDefined && - d.commitments.active.size == 1 && - (d.latestFundingTx.fundingParams.remoteAmount == 0.sat || d.commitments.params.localParams.initFeatures.hasFeature(Features.ZeroConf)) - if (canUseZeroConf) { + if (switchToZeroConf(remoteChannelReady, d)) { log.info("this channel isn't zero-conf, but they sent an early channel_ready with an alias: no need to wait for confirmations") - // NB: we will receive a WatchFundingConfirmedTriggered later that will simply be ignored. blockchain ! WatchPublished(self, d.commitments.latest.fundingTxId) } log.debug("received their channel_ready, deferring message") diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenSingleFunded.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenSingleFunded.scala index 8e3390eb9..440101a17 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenSingleFunded.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenSingleFunded.scala @@ -31,7 +31,7 @@ import fr.acinq.eclair.crypto.ShaChain import fr.acinq.eclair.transactions.Transactions.TxOwner import fr.acinq.eclair.transactions.{Scripts, Transactions} import fr.acinq.eclair.wire.protocol.{AcceptChannel, AnnouncementSignatures, ChannelReady, ChannelTlv, Error, FundingCreated, FundingSigned, OpenChannel, TlvStream} -import fr.acinq.eclair.{MilliSatoshiLong, RealShortChannelId, UInt64, randomKey, toLongId} +import fr.acinq.eclair.{Features, MilliSatoshiLong, RealShortChannelId, UInt64, randomKey, toLongId} import scodec.bits.ByteVector import scala.util.{Failure, Success} @@ -372,9 +372,16 @@ trait ChannelOpenSingleFunded extends SingleFundingHandlers with ErrorHandlers { when(WAIT_FOR_FUNDING_CONFIRMED)(handleExceptions { case Event(remoteChannelReady: ChannelReady, d: DATA_WAIT_FOR_FUNDING_CONFIRMED) => - if (remoteChannelReady.alias_opt.isDefined && d.commitments.params.localParams.isInitiator) { + // We are here if: + // - we're using zero-conf, but our peer was very fast and we received their channel_ready before our watcher + // notification that the funding tx has been successfully published: in that case we don't put a duplicate watch + // - we're not using zero-conf, but our peer decided to trust us anyway, in which case we can skip waiting for + // confirmations if we're the initiator (no risk of double-spend) and they provided a channel alias + val switchToZeroConf = d.commitments.params.localParams.isInitiator && + remoteChannelReady.alias_opt.isDefined && + !d.commitments.params.localParams.initFeatures.hasFeature(Features.ZeroConf) + if (switchToZeroConf) { log.info("this channel isn't zero-conf, but we are funder and they sent an early channel_ready with an alias: no need to wait for confirmations") - // NB: we will receive a WatchFundingConfirmedTriggered later that will simply be ignored blockchain ! WatchPublished(self, d.commitments.latest.fundingTxId) } log.debug("received their channel_ready, deferring message") diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/DualFundingHandlers.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/DualFundingHandlers.scala index a47087bb6..8fc798b8c 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/DualFundingHandlers.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/DualFundingHandlers.scala @@ -25,7 +25,7 @@ import fr.acinq.eclair.channel.LocalFundingStatus.DualFundedUnconfirmedFundingTx import fr.acinq.eclair.channel._ import fr.acinq.eclair.channel.fsm.Channel.BITCOIN_FUNDING_DOUBLE_SPENT import fr.acinq.eclair.channel.fund.InteractiveTxBuilder._ -import fr.acinq.eclair.wire.protocol.Error +import fr.acinq.eclair.wire.protocol.{ChannelReady, Error} import scala.concurrent.Future import scala.util.{Failure, Success} @@ -59,6 +59,22 @@ trait DualFundingHandlers extends CommonFundingHandlers { } } + /** Return true if we should stop waiting for confirmations when receiving our peer's channel_ready. */ + def switchToZeroConf(remoteChannelReady: ChannelReady, d: DATA_WAIT_FOR_DUAL_FUNDING_CONFIRMED): Boolean = d.latestFundingTx.fundingParams.minDepth_opt match { + case Some(_) => + // We're not using zero-conf, but our peer decided to trust us anyway. We can skip waiting for confirmations if: + // - they provided a channel alias + // - there is a single version of the funding tx (otherwise we don't know which one to use) + // - they didn't contribute to the funding transaction (and thus cannot double-spend it) + remoteChannelReady.alias_opt.isDefined && + d.commitments.active.size == 1 && + d.latestFundingTx.sharedTx.tx.remoteInputs.isEmpty + case None => + // We're already using zero-conf, but our peer was very fast and we received their channel_ready before our + // watcher notification that the funding tx has been successfully published. + false + } + def handleNewBlockDualFundingUnconfirmed(c: CurrentBlockHeight, d: DATA_WAIT_FOR_DUAL_FUNDING_CONFIRMED) = { // We regularly notify the node operator that they may want to RBF this channel. val blocksSinceOpen = c.blockHeight - d.waitingSince diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/c/WaitForDualFundingConfirmedStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/c/WaitForDualFundingConfirmedStateSpec.scala index 29d85f4e5..0c5121e9e 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/c/WaitForDualFundingConfirmedStateSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/c/WaitForDualFundingConfirmedStateSpec.scala @@ -19,6 +19,7 @@ package fr.acinq.eclair.channel.states.c import akka.actor.Status import akka.actor.typed.scaladsl.adapter.actorRefAdapter import akka.testkit.{TestFSMRef, TestProbe} +import com.softwaremill.quicklens.{ModifyPimp, QuicklensAt} import fr.acinq.bitcoin.scalacompat.{ByteVector32, SatoshiLong, Transaction} import fr.acinq.eclair.blockchain.bitcoind.ZmqWatcher._ import fr.acinq.eclair.blockchain.{CurrentBlockHeight, SingleKeyOnChainWallet} @@ -521,6 +522,7 @@ class WaitForDualFundingConfirmedStateSpec extends TestKitBaseClass with Fixture assert(channelReady.alias_opt.isDefined) bob2alice.forward(alice) alice2bob.expectNoMessage(100 millis) + alice2blockchain.expectNoMessage(100 millis) awaitCond(alice.stateData.asInstanceOf[DATA_WAIT_FOR_DUAL_FUNDING_CONFIRMED].deferred.contains(channelReady)) awaitCond(alice.stateName == WAIT_FOR_DUAL_FUNDING_CONFIRMED) awaitCond(bob.stateName == WAIT_FOR_DUAL_FUNDING_READY) @@ -544,6 +546,33 @@ class WaitForDualFundingConfirmedStateSpec extends TestKitBaseClass with Fixture awaitCond(alice.stateName == NORMAL) } + test("recv ChannelReady (initiator, no remote contribution, with remote inputs)", Tag(ChannelStateTestsTags.DualFunding)) { f => + import f._ + // We test the following scenario: + // - Bob doesn't contribute to the channel funding output + // - But Bob adds inputs to the interactive-tx transaction + // - And sends an early channel_ready to try to fool us into using zero-conf + // We don't have code to contribute to an interactive-tx without contributing to the funding output, so we tweak + // internal channel data to simulate it. + val aliceData = alice.stateData.asInstanceOf[DATA_WAIT_FOR_DUAL_FUNDING_CONFIRMED] + val fundingTx1 = aliceData.latestFundingTx.copy(fundingParams = aliceData.latestFundingTx.fundingParams.copy(remoteAmount = 0 sat)) + val aliceData1 = aliceData + .modify(_.commitments.active.at(0).localFundingStatus) + .setTo(fundingTx1) + alice.setState(alice.stateName, aliceData1) + assert(alice.stateData.asInstanceOf[DATA_WAIT_FOR_DUAL_FUNDING_CONFIRMED].latestFundingTx.fundingParams.minDepth_opt.nonEmpty) + assert(alice.stateData.asInstanceOf[DATA_WAIT_FOR_DUAL_FUNDING_CONFIRMED].latestFundingTx.fundingParams.remoteAmount == 0.sat) + assert(alice.stateData.asInstanceOf[DATA_WAIT_FOR_DUAL_FUNDING_CONFIRMED].latestFundingTx.sharedTx.tx.remoteInputs.nonEmpty) + bob ! WatchFundingConfirmedTriggered(BlockHeight(42000), 42, fundingTx1.sharedTx.asInstanceOf[FullySignedSharedTransaction].signedTx) + val channelReady = bob2alice.expectMsgType[ChannelReady] + assert(channelReady.alias_opt.isDefined) + bob2alice.forward(alice) + alice2bob.expectNoMessage(100 millis) + alice2blockchain.expectNoMessage(100 millis) + awaitCond(alice.stateData.asInstanceOf[DATA_WAIT_FOR_DUAL_FUNDING_CONFIRMED].deferred.contains(channelReady)) + awaitCond(alice.stateName == WAIT_FOR_DUAL_FUNDING_CONFIRMED) + } + test("recv ChannelReady (non-initiator)", Tag(ChannelStateTestsTags.DualFunding)) { f => import f._ val fundingTx = alice.stateData.asInstanceOf[DATA_WAIT_FOR_DUAL_FUNDING_CONFIRMED].latestFundingTx.sharedTx.asInstanceOf[FullySignedSharedTransaction].signedTx @@ -552,6 +581,7 @@ class WaitForDualFundingConfirmedStateSpec extends TestKitBaseClass with Fixture assert(channelReady.alias_opt.isDefined) alice2bob.forward(bob) bob2alice.expectNoMessage(100 millis) + bob2blockchain.expectNoMessage(100 millis) awaitCond(bob.stateData.asInstanceOf[DATA_WAIT_FOR_DUAL_FUNDING_CONFIRMED].deferred.contains(channelReady)) awaitCond(bob.stateName == WAIT_FOR_DUAL_FUNDING_CONFIRMED) awaitCond(alice.stateName == WAIT_FOR_DUAL_FUNDING_READY)