Abandon evicted replaceable txs

This ensures the bitcoind wallet won't keep the transaction around and avoid using its inputs for other transactions. Fixes #1898
ACINQ · Aug 16, 2021 · 7a1ce33 · 7a1ce33
1 parent 6ee75e5
commit 7a1ce33
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 4 deletions.
diff --git a/...r-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/ExtendedBitcoinClient.scala b/...r-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/ExtendedBitcoinClient.scala
@@ -152,6 +152,15 @@ class ExtendedBitcoinClient(val rpcClient: BitcoinJsonRPCClient) extends Logging
         getRawTransaction(tx.txid).map(_ => tx.txid).recoverWith { case _ => Future.failed(e) }
     }
 
+  /**
+   * Mark a transaction as abandoned, which will allow for its wallet inputs to be re-spent.
+   * This method can be used to replace "stuck" or evicted transactions.
+   * It only works on transactions which are not included in a block and are not currently in the mempool.
+   */
+  def abandonTransaction(txId: ByteVector32)(implicit ec: ExecutionContext): Future[Boolean] = {
+    rpcClient.invoke("abandontransaction", txId).map(_ => true).recover(_ => false)
+  }
+
   /**
    * @param outPoints outpoints to unlock.
    * @return true if all outpoints were successfully unlocked, false otherwise.
@@ -282,7 +291,7 @@ class ExtendedBitcoinClient(val rpcClient: BitcoinJsonRPCClient) extends Logging
       val JBool(safe) = utxo \ "safe"
       val JDecimal(amount) = utxo \ "amount"
       val JString(txid) = utxo \ "txid"
-      val label =  utxo \ "label" match {
+      val label = utxo \ "label" match {
         case JString(label) => Some(label)
         case _ => None
       }

diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisher.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisher.scala
@@ -24,7 +24,7 @@ import fr.acinq.eclair.blockchain.bitcoind.ZmqWatcher
 import fr.acinq.eclair.blockchain.bitcoind.rpc.ExtendedBitcoinClient
 import fr.acinq.eclair.blockchain.bitcoind.rpc.ExtendedBitcoinClient.FundTransactionOptions
 import fr.acinq.eclair.blockchain.fee.FeeratePerKw
-import fr.acinq.eclair.channel.publish.TxPublisher.TxPublishLogContext
+import fr.acinq.eclair.channel.publish.TxPublisher.{TxPublishLogContext, TxRejectedReason}
 import fr.acinq.eclair.channel.publish.TxTimeLocksMonitor.CheckTx
 import fr.acinq.eclair.channel.{Commitments, HtlcTxAndRemoteSig}
 import fr.acinq.eclair.transactions.Transactions
@@ -329,6 +329,14 @@ private class ReplaceableTxPublisher(nodeParams: NodeParams,
     Behaviors.receiveMessagePartial {
       case WrappedTxResult(MempoolTxMonitor.TxConfirmed) => sendResult(replyTo, TxPublisher.TxConfirmed(cmd, tx))
       case WrappedTxResult(MempoolTxMonitor.TxRejected(reason)) =>
+        reason match {
+          case TxRejectedReason.WalletInputGone =>
+            // The transaction now has an unknown input from bitcoind's point of view, so it will keep it in the wallet in
+            // case that input appears later in the mempool or the blockchain. In our case, we know it won't happen so we
+            // abandon that transaction and will retry with a different set of inputs (if it still makes sense to publish).
+            bitcoinClient.abandonTransaction(tx.txid)
+          case _ => // nothing to do
+        }
         replyTo ! TxPublisher.TxRejected(loggingInfo.id, cmd, reason)
         // We wait for our parent to stop us: when that happens we will unlock utxos.
         Behaviors.same

diff --git a/...r-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/ExtendedBitcoinClientSpec.scala b/...r-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/ExtendedBitcoinClientSpec.scala
@@ -361,6 +361,44 @@ class ExtendedBitcoinClientSpec extends TestKitBaseClass with BitcoindService wi
     assert(mempoolTx3.ancestorFees === mempoolTx1.fees + 12500.sat)
   }
 
+  test("abandon transaction") {
+    val sender = TestProbe()
+    val bitcoinClient = new ExtendedBitcoinClient(bitcoinrpcclient)
+
+    // Broadcast a wallet transaction.
+    val opts = FundTransactionOptions(TestConstants.feeratePerKw, changePosition = Some(1))
+    bitcoinClient.fundTransaction(Transaction(2, Nil, Seq(TxOut(250000 sat, Script.pay2wpkh(randomKey().publicKey))), 0), opts).pipeTo(sender.ref)
+    val fundedTx1 = sender.expectMsgType[FundTransactionResponse].tx
+    bitcoinClient.signTransaction(fundedTx1, Nil).pipeTo(sender.ref)
+    val signedTx1 = sender.expectMsgType[SignTransactionResponse].tx
+    bitcoinClient.publishTransaction(signedTx1).pipeTo(sender.ref)
+    sender.expectMsg(signedTx1.txid)
+
+    // Double-spend that transaction.
+    val fundedTx2 = fundedTx1.copy(txOut = TxOut(200000 sat, Script.pay2wpkh(randomKey().publicKey)) +: fundedTx1.txOut.tail)
+    bitcoinClient.signTransaction(fundedTx2, Nil).pipeTo(sender.ref)
+    val signedTx2 = sender.expectMsgType[SignTransactionResponse].tx
+    assert(signedTx2.txid != signedTx1.txid)
+    bitcoinClient.publishTransaction(signedTx2).pipeTo(sender.ref)
+    sender.expectMsg(signedTx2.txid)
+
+    // Abandon the first wallet transaction.
+    bitcoinClient.abandonTransaction(signedTx1.txid).pipeTo(sender.ref)
+    sender.expectMsg(true)
+
+    // Abandoning an already-abandoned transaction is a no-op.
+    bitcoinClient.abandonTransaction(signedTx1.txid).pipeTo(sender.ref)
+    sender.expectMsg(true)
+
+    // We can't abandon the second transaction (it's in the mempool).
+    bitcoinClient.abandonTransaction(signedTx2.txid).pipeTo(sender.ref)
+    sender.expectMsg(false)
+
+    // We can't abandon a confirmed transaction.
+    bitcoinClient.abandonTransaction(signedTx2.txIn.head.outPoint.txid).pipeTo(sender.ref)
+    sender.expectMsg(false)
+  }
+
   test("detect if tx has been double-spent") {
     val sender = TestProbe()
     val bitcoinClient = new ExtendedBitcoinClient(bitcoinrpcclient)

diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisherSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisherSpec.scala
@@ -29,7 +29,7 @@ import fr.acinq.eclair.blockchain.bitcoind.{BitcoinCoreWallet, BitcoindService}
 import fr.acinq.eclair.blockchain.fee.FeeratePerKw
 import fr.acinq.eclair.channel._
 import fr.acinq.eclair.channel.publish.ReplaceableTxPublisher.{Publish, Stop}
-import fr.acinq.eclair.channel.publish.TxPublisher.TxRejectedReason.{ConflictingTxUnconfirmed, CouldNotFund, TxSkipped}
+import fr.acinq.eclair.channel.publish.TxPublisher.TxRejectedReason.{ConflictingTxUnconfirmed, CouldNotFund, TxSkipped, WalletInputGone}
 import fr.acinq.eclair.channel.publish.TxPublisher._
 import fr.acinq.eclair.channel.states.{ChannelStateTestsHelperMethods, ChannelStateTestsTags}
 import fr.acinq.eclair.transactions.Transactions.{ClaimLocalAnchorOutputTx, HtlcSuccessTx, HtlcTimeoutTx}
@@ -224,6 +224,33 @@ class ReplaceableTxPublisherSpec extends TestKitBaseClass with AnyFunSuiteLike w
     })
   }
 
+  test("remote commit tx replaces local commit tx, not spending anchor output") {
+    withFixture(Seq(500 millibtc), f => {
+      import f._
+
+      val remoteCommit = bob.stateData.asInstanceOf[DATA_NORMAL].commitments.fullySignedLocalCommitTx(bob.underlyingActor.nodeParams.channelKeyManager)
+      assert(bob.stateData.asInstanceOf[DATA_NORMAL].commitments.localCommit.spec.feeratePerKw === FeeratePerKw(2500 sat))
+
+      // We lower the feerate to make it easy to replace our commit tx by theirs in the mempool.
+      val lowFeerate = FeeratePerKw(500 sat)
+      updateFee(lowFeerate, alice, bob, alice2bob, bob2alice)
+      val (localCommit, anchorTx) = closeChannelWithoutHtlcs(f)
+      publisher ! Publish(probe.ref, anchorTx, FeeratePerKw(600 sat))
+      val mempoolTxs = getMempoolTxs(2)
+      assert(mempoolTxs.map(_.txid).contains(localCommit.tx.txid))
+
+      // Our commit tx is replaced by theirs.
+      walletClient.publishTransaction(remoteCommit.tx).pipeTo(probe.ref)
+      probe.expectMsg(remoteCommit.tx.txid)
+      generateBlocks(1)
+      system.eventStream.publish(CurrentBlockCount(currentBlockHeight(probe)))
+
+      val result = probe.expectMsgType[TxRejected]
+      assert(result.cmd === anchorTx)
+      assert(result.reason === WalletInputGone)
+    })
+  }
+
   test("not enough funds to increase commit tx feerate") {
     withFixture(Seq(10.5 millibtc), f => {
       import f._

diff --git a/...r-core/src/test/scala/fr/acinq/eclair/channel/states/ChannelStateTestsHelperMethods.scala b/...r-core/src/test/scala/fr/acinq/eclair/channel/states/ChannelStateTestsHelperMethods.scala
@@ -25,7 +25,7 @@ import fr.acinq.bitcoin.{ByteVector32, Crypto, SatoshiLong, ScriptFlags, Transac
 import fr.acinq.eclair.TestConstants.{Alice, Bob, TestFeeEstimator}
 import fr.acinq.eclair._
 import fr.acinq.eclair.blockchain.bitcoind.ZmqWatcher._
-import fr.acinq.eclair.blockchain.fee.FeeTargets
+import fr.acinq.eclair.blockchain.fee.{FeeTargets, FeeratePerKw}
 import fr.acinq.eclair.blockchain.{EclairWallet, TestWallet}
 import fr.acinq.eclair.channel._
 import fr.acinq.eclair.channel.publish.TxPublisher
@@ -270,6 +270,21 @@ trait ChannelStateTestsHelperMethods extends TestKitBase {
     }
   }
 
+  def updateFee(feerate: FeeratePerKw, s: TestFSMRef[ChannelState, ChannelData, Channel], r: TestFSMRef[ChannelState, ChannelData, Channel], s2r: TestProbe, r2s: TestProbe): Unit = {
+    s ! CMD_UPDATE_FEE(feerate, commit = true)
+    s2r.expectMsgType[UpdateFee]
+    s2r.forward(r)
+    s2r.expectMsgType[CommitSig]
+    s2r.forward(r)
+    r2s.expectMsgType[RevokeAndAck]
+    r2s.forward(s)
+    r2s.expectMsgType[CommitSig]
+    r2s.forward(s)
+    s2r.expectMsgType[RevokeAndAck]
+    s2r.forward(r)
+    awaitCond(s.stateData.asInstanceOf[HasCommitments].commitments.localCommit.spec.feeratePerKw == feerate)
+  }
+
   def mutualClose(s: TestFSMRef[ChannelState, ChannelData, Channel], r: TestFSMRef[ChannelState, ChannelData, Channel], s2r: TestProbe, r2s: TestProbe, s2blockchain: TestProbe, r2blockchain: TestProbe): Unit = {
     val sender = TestProbe()
     // s initiates a closing