Unlock transaction inputs if tx cannot be published (#1404)

* Unlock transaction inputs if tx cannot be published

In some cases, funding a tx will work but publishing may fail (because mempool fees are not met for example).
In that case we need to make sure that the tx inputs are unlocked.

eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreWallet.scala

@@ -26,6 +26,7 @@ import org.json4s.JsonAST._
 import scodec.bits.ByteVector
 
 import scala.concurrent.{ExecutionContext, Future}
+import scala.util.{Failure, Success}
 
 /**
  * Created by PM on 06/07/2017.
@@ -63,7 +64,30 @@ class BitcoinCoreWallet(rpcClient: BitcoinJsonRPCClient)(implicit ec: ExecutionC
 
   def publishTransaction(tx: Transaction)(implicit ec: ExecutionContext): Future[String] = bitcoinClient.publishTransaction(tx)
 
-  def unlockOutpoints(outPoints: Seq[OutPoint])(implicit ec: ExecutionContext): Future[Boolean] = rpcClient.invoke("lockunspent", true, outPoints.toList.map(outPoint => Utxo(outPoint.txid, outPoint.index))) collect { case JBool(result) => result }
+  /**
+   *
+   * @param outPoints outpoints to unlock
+   * @return true if all outpoints were successfully unlocked, false otherwise
+   */
+  def unlockOutpoints(outPoints: Seq[OutPoint])(implicit ec: ExecutionContext): Future[Boolean] = {
+    // we unlock utxos one by one and not as a list as it would fail at the first utxo that is not actually lock and the rest would not be processed
+    val futures = outPoints
+      .map(outPoint => Utxo(outPoint.txid, outPoint.index))
+      .map(utxo => rpcClient
+        .invoke("lockunspent", true, List(utxo))
+        .mapTo[JBool]
+        .transformWith {
+          case Success(JBool(result)) => Future.successful(result)
+          case Failure(JsonRPCError(error)) if error.message.contains("expected locked output") =>
+            Future.successful(true) // we consider that the outpoint was successfully unlocked (since it was not locked to begin with)
+          case Failure(t) =>
+            logger.warn(s"Cannot unlock utxo=$utxo", t)
+            Future.successful(false)
+        })
+    val future = Future.sequence(futures)
+    // return true if all outpoints were unlocked false otherwise
+    future.map(_.forall(b => b))
+  }
 
   override def getBalance: Future[Satoshi] = rpcClient.invoke("getbalance") collect { case JDecimal(balance) => Satoshi(balance.bigDecimal.scaleByPowerOfTen(8).longValue) }
 
@@ -103,13 +127,15 @@ class BitcoinCoreWallet(rpcClient: BitcoinJsonRPCClient)(implicit ec: ExecutionC
     } yield MakeFundingTxResponse(fundingTx, outputIndex, fee)
   }
 
-  override def commit(tx: Transaction): Future[Boolean] = publishTransaction(tx)
-    .map(_ => true) // if bitcoind says OK, then we consider the tx successfully published
-    .recoverWith { case JsonRPCError(e) =>
+  override def commit(tx: Transaction): Future[Boolean] = publishTransaction(tx).transformWith {
+    case Success(_) => Future.successful(true)
+    case Failure(e) =>
       logger.warn(s"txid=${tx.txid} error=$e")
-      bitcoinClient.getTransaction(tx.txid).map(_ => true).recover { case _ => false } // if we get a parseable error from bitcoind AND the tx is NOT in the mempool/blockchain, then we consider that the tx was not published
-    }
-    .recover { case _ => true } // in all other cases we consider that the tx has been published
+      bitcoinClient.getTransaction(tx.txid).transformWith {
+        case Success(_) => Future.successful(true) // tx is in the mempool, we consider that it was published
+        case Failure(_) => rollback(tx).transform { case _ => Success(false) } // we use transform here because we want to return false in all cases even if rollback fails
+      }
+  }
 
   override def rollback(tx: Transaction): Future[Boolean] = unlockOutpoints(tx.txIn.map(_.outPoint)) // we unlock all utxos used by the tx
 

eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreWalletSpec.scala

@@ -20,9 +20,9 @@ import akka.actor.Status.Failure
 import akka.pattern.pipe
 import akka.testkit.TestProbe
 import com.typesafe.config.ConfigFactory
-import fr.acinq.bitcoin.{Block, ByteVector32, MilliBtc, OutPoint, Satoshi, Script, Transaction, TxIn, TxOut}
+import fr.acinq.bitcoin.{Block, Btc, ByteVector32, MilliBtc, OutPoint, Satoshi, Script, Transaction, TxIn, TxOut}
 import fr.acinq.eclair.blockchain._
-import fr.acinq.eclair.blockchain.bitcoind.BitcoinCoreWallet.FundTransactionResponse
+import fr.acinq.eclair.blockchain.bitcoind.BitcoinCoreWallet.{FundTransactionResponse, SignTransactionResponse}
 import fr.acinq.eclair.blockchain.bitcoind.BitcoindService.BitcoinReq
 import fr.acinq.eclair.blockchain.bitcoind.rpc.{BasicBitcoinJsonRPCClient, JsonRPCError}
 import fr.acinq.eclair.transactions.Scripts
@@ -68,6 +68,105 @@ class BitcoinCoreWalletSpec extends TestKitBaseClass with BitcoindService with A
     waitForBitcoindReady()
   }
 
+  def getLocks(sender: TestProbe = TestProbe()) : Set[OutPoint] = {
+    implicit val formats = DefaultFormats
+    sender.send(bitcoincli, BitcoinReq("listlockunspent"))
+    val JArray(locks) = sender.expectMsgType[JValue]
+    val txids = locks.map { item =>
+      val JString(txid) = item \ "txid"
+      val JInt(vout) = item \ "vout"
+      OutPoint(ByteVector32.fromValidHex(txid).reverse, vout.toInt)
+    }
+    txids.toSet
+  }
+
+  test("unlock transaction inputs if publishing fails") {
+    val bitcoinClient = new BasicBitcoinJsonRPCClient(
+      user = config.getString("bitcoind.rpcuser"),
+      password = config.getString("bitcoind.rpcpassword"),
+      host = config.getString("bitcoind.host"),
+      port = config.getInt("bitcoind.rpcport"))
+    val wallet = new BitcoinCoreWallet(bitcoinClient)
+
+    val sender = TestProbe()
+    val pubkeyScript = Script.write(Script.pay2wsh(Scripts.multiSig2of2(randomKey.publicKey, randomKey.publicKey)))
+
+    // create a huge tx so we make sure it has > 1 inputs
+    wallet.makeFundingTx(pubkeyScript, Btc(250), 1000).pipeTo(sender.ref)
+    val MakeFundingTxResponse(fundingTx, outputIndex, _) = sender.expectMsgType[MakeFundingTxResponse]
+
+    // spend the first 2 inputs
+    val tx1 = fundingTx.copy(
+      txIn = fundingTx.txIn.take(2),
+      txOut = fundingTx.txOut.updated(outputIndex, fundingTx.txOut(outputIndex).copy(amount = Btc(50)))
+    )
+    wallet.signTransaction(tx1).pipeTo(sender.ref)
+    val SignTransactionResponse(tx2, true) = sender.expectMsgType[SignTransactionResponse]
+
+    wallet.commit(tx2).pipeTo(sender.ref)
+    assert(sender.expectMsgType[Boolean])
+
+    // fundingTx inputs are still locked except for the first 2 that were just spent
+    val expectedLocks = fundingTx.txIn.drop(2).map(_.outPoint).toSet
+    awaitCond({
+      val locks = getLocks(sender)
+      expectedLocks -- locks isEmpty
+    }, max = 10 seconds, interval = 1 second)
+
+    // publishing fundingTx will fail as its first 2 inputs are already spent by tx above in the mempool
+    wallet.commit(fundingTx).pipeTo(sender.ref)
+    val result = sender.expectMsgType[Boolean]
+    assert(!result)
+
+    // and all locked inputs should now be unlocked
+    awaitCond({
+      val locks = getLocks(sender)
+      locks isEmpty
+    }, max = 10 seconds, interval = 1 second)
+  }
+
+  test("unlock outpoints correcly") {
+    val bitcoinClient = new BasicBitcoinJsonRPCClient(
+      user = config.getString("bitcoind.rpcuser"),
+      password = config.getString("bitcoind.rpcpassword"),
+      host = config.getString("bitcoind.host"),
+      port = config.getInt("bitcoind.rpcport"))
+
+    val wallet = new BitcoinCoreWallet(bitcoinClient)
+
+    val sender = TestProbe()
+    val pubkeyScript = Script.write(Script.pay2wsh(Scripts.multiSig2of2(randomKey.publicKey, randomKey.publicKey)))
+
+    {
+      // test #1: unlock outpoints that are actually locked
+      // create a huge tx so we make sure it has > 1 inputs
+      wallet.makeFundingTx(pubkeyScript, Btc(250), 1000).pipeTo(sender.ref)
+      val MakeFundingTxResponse(fundingTx, outputIndex, _) = sender.expectMsgType[MakeFundingTxResponse]
+      assert(fundingTx.txIn.size > 2)
+      assert(getLocks(sender) == fundingTx.txIn.map(_.outPoint).toSet)
+      wallet.rollback(fundingTx).pipeTo(sender.ref)
+      assert(sender.expectMsgType[Boolean])
+    }
+    {
+      // test #2: some outpoints are locked, some are unlocked
+      wallet.makeFundingTx(pubkeyScript, Btc(250), 1000).pipeTo(sender.ref)
+      val MakeFundingTxResponse(fundingTx, outputIndex, _) = sender.expectMsgType[MakeFundingTxResponse]
+      assert(fundingTx.txIn.size > 2)
+      assert(getLocks(sender) == fundingTx.txIn.map(_.outPoint).toSet)
+
+      // unlock the first 2 outpoints
+      val tx1 = fundingTx.copy(txIn = fundingTx.txIn.take(2))
+      wallet.rollback(tx1).pipeTo(sender.ref)
+      assert(sender.expectMsgType[Boolean])
+      assert(getLocks(sender) == fundingTx.txIn.drop(2).map(_.outPoint).toSet)
+
+      // and try to unlock all outpoints: it should work too
+      wallet.rollback(fundingTx).pipeTo(sender.ref)
+      assert(sender.expectMsgType[Boolean])
+      assert(getLocks(sender) isEmpty)
+    }
+  }
+
   test("absence of rounding") {
     val hexIn = "020000000001404b4c0000000000220020822eb4234126c5fc84910e51a161a9b7af94eb67a2344f7031db247e0ecc2f9200000000"
     val hexOut = "02000000013361e994f6bd5cbe9dc9e8cb3acdc12bc1510a3596469d9fc03cfddd71b223720000000000feffffff02c821354a00000000160014b6aa25d6f2a692517f2cf1ad55f243a5ba672cac404b4c0000000000220020822eb4234126c5fc84910e51a161a9b7af94eb67a2344f7031db247e0ecc2f9200000000"