Electrum: improve coin selection (fixes #1146)

eclair-core/src/main/scala/fr/acinq/eclair/blockchain/electrum/ElectrumWallet.scala

@@ -928,32 +928,6 @@ object ElectrumWallet {
         TxIn(utxo.outPoint, signatureScript = sigScript, sequence = TxIn.SEQUENCE_FINAL, witness = witness)
       })
 
-    /**
-     *
-     * @param amount                amount we want to pay
-     * @param allowSpendUnconfirmed if true, use unconfirmed utxos
-     * @return a set of utxos with a total value that is greater than amount
-     */
-    def chooseUtxos(amount: Satoshi, allowSpendUnconfirmed: Boolean): Seq[Utxo] = {
-      @tailrec
-      def select(chooseFrom: Seq[Utxo], selected: Set[Utxo]): Set[Utxo] = {
-        if (totalAmount(selected) >= amount) selected
-        else if (chooseFrom.isEmpty) throw new IllegalArgumentException("insufficient funds")
-        else select(chooseFrom.tail, selected + chooseFrom.head)
-      }
-
-      // select utxos that are not locked by pending txs
-      val lockedOutputs = locks.map(_.txIn.map(_.outPoint)).flatten
-      val unlocked = utxos.filterNot(utxo => lockedOutputs.contains(utxo.outPoint))
-      val unlocked1 = if (allowSpendUnconfirmed) unlocked else unlocked.filter(_.item.height > 0)
-
-      // sort utxos by amount, in increasing order
-      // this way we minimize the number of utxos in the wallet, and so we minimize the fees we'll pay for them
-      val unlocked2 = unlocked1.sortBy(_.item.value)
-      val selected = select(unlocked2, Set())
-      selected.toSeq
-    }
-
     /**
      *
      * @param tx           input tx that has no inputs
@@ -971,35 +945,62 @@ object ElectrumWallet {
       val amount = tx.txOut.map(_.amount).sum
       require(amount > dustLimit, "amount to send is below dust limit")
 
-      // start with a hefty fee estimate
-      val utxos = chooseUtxos(amount + Transactions.weight2fee(feeRatePerKw, 1000), allowSpendUnconfirmed)
-      val spent = totalAmount(utxos)
-
-      // add utxos, and sign with dummy sigs
-      val tx1 = addUtxosWithDummySig(tx, utxos)
+      val unlocked = {
+        // select utxos that are not locked by pending txs
+        val lockedOutputs = locks.flatMap(_.txIn.map(_.outPoint))
+        val unlocked1 = utxos.filterNot(utxo => lockedOutputs.contains(utxo.outPoint))
+        val unlocked2 = if (allowSpendUnconfirmed) unlocked1 else unlocked1.filter(_.item.height > 0)
+        // sort utxos by amount, in increasing order
+        // this way we minimize the number of utxos in the wallet, and so we minimize the fees we'll pay for them
+        unlocked2.sortBy(_.item.value)
+      }
 
-      // compute the actual fee that we should pay
-      val fee1 = {
-        // add a dummy change output, which will be needed most of the time
-        val tx2 = tx1.addOutput(TxOut(amount, computePublicKeyScript(currentChangeKey.publicKey)))
+      // computes the fee what we would have to pay for our tx with our candidate utxos and an optional change output
+      def computeFee(candidates: Seq[Utxo], change: Option[TxOut]): Satoshi = {
+        val tx1 = addUtxosWithDummySig(tx, candidates)
+        val tx2 = change.map(o => tx1.addOutput(o)).getOrElse(tx1)
         Transactions.weight2fee(feeRatePerKw, tx2.weight())
       }
 
-      // add change output only if non-dust, otherwise change is added to the fee
-      val (tx2, fee2, pos) = (spent - amount - fee1) match {
-        case dustChange if dustChange < dustLimit => (tx1, fee1 + dustChange, -1) // if change is below dust we add it to fees
-        case change => (tx1.addOutput(TxOut(change, computePublicKeyScript(currentChangeKey.publicKey))), fee1, 1) // change output index is always 1
+      val dummyChange = TxOut(Satoshi(0), computePublicKeyScript(currentChangeKey.publicKey))
+
+      @tailrec
+      def loop(current: Seq[Utxo], remaining: Seq[Utxo]): (Seq[Utxo], Option[TxOut]) = {
+        totalAmount(current) match {
+          case total if total - computeFee(current, None) < amount && remaining.isEmpty =>
+            // not enough funds to send amount and pay fees even without a change output
+            throw new IllegalArgumentException("insufficient funds")
+          case total if total - computeFee(current, None) < amount =>
+            // not enough funds, try with an additional input
+            loop(remaining.head +: current, remaining.tail)
+          case total if total - computeFee(current, None) <= amount + dustLimit =>
+            // change output would be below dust, we don't add one and just overpay fees
+            (current, None)
+          case total if total - computeFee(current, Some(dummyChange)) <= amount + dustLimit && remaining.isEmpty =>
+            // change output is above dust limit but cannot pay for it's own fee, and we have no more utxos => we overpay a bit
+            (current, None)
+          case total if total - computeFee(current, Some(dummyChange)) <= amount + dustLimit =>
+            // try with an additional input
+            loop(remaining.head +: current, remaining.tail)
+          case total =>
+            val fee = computeFee(current, Some(dummyChange))
+            val change = dummyChange.copy(amount = total - amount - fee)
+            (current, Some(change))
+        }
       }
 
+      val (selected, change_opt) = loop(Seq.empty[Utxo], unlocked)
+
       // sign our tx
+      val tx1 = addUtxosWithDummySig(tx, selected)
+      val tx2 = change_opt.map(out => tx1.addOutput(out)).getOrElse(tx1)
       val tx3 = signTransaction(tx2)
-      //Transaction.correctlySpends(tx3, utxos.map(utxo => utxo.outPoint -> TxOut(Satoshi(utxo.item.value), computePublicKeyScript(utxo.key.publicKey))).toMap, ScriptFlags.STANDARD_SCRIPT_VERIFY_FLAGS)
 
-      // and add the completed tx to the lokcs
+      // and add the completed tx to the locks
       val data1 = this.copy(locks = this.locks + tx3)
-      val fee3 = spent - tx3.txOut.map(_.amount).sum
+      val fee = selected.map(s => Satoshi(s.item.value)).sum - tx3.txOut.map(_.amount).sum
 
-      (data1, tx3, fee3)
+      (data1, tx3, fee)
     }
 
     def signTransaction(tx: Transaction): Transaction = {

eclair-core/src/test/scala/fr/acinq/eclair/blockchain/electrum/ElectrumWalletBasicSpec.scala

@@ -22,7 +22,7 @@ import fr.acinq.bitcoin.Crypto.PrivateKey
 import fr.acinq.bitcoin.DeterministicWallet.{ExtendedPrivateKey, derivePrivateKey}
 import fr.acinq.bitcoin._
 import fr.acinq.eclair.blockchain.electrum.db.sqlite.SqliteWalletDb
-import fr.acinq.eclair.transactions.Transactions
+import fr.acinq.eclair.transactions.{Scripts, Transactions}
 import grizzled.slf4j.Logging
 import org.scalatest.FunSuite
 import scodec.bits.ByteVector
@@ -184,6 +184,23 @@ class ElectrumWalletBasicSpec extends FunSuite with Logging {
     assert(tx.txOut.map(_.amount).sum + fee == state3.balance._1 + state3.balance._2)
   }
 
+  test("check that issue #1146 is fixed") {
+    val state3 = addFunds(state, state.changeKeys(0), 0.5 btc)
+
+    val pub1 = state.accountKeys(0).publicKey
+    val pub2 = state.accountKeys(1).publicKey
+    val redeemScript = Scripts.multiSig2of2(pub1, pub2)
+    val pubkeyScript = Script.pay2wsh(redeemScript)
+    val (tx, fee) = state3.spendAll(pubkeyScript, feeRatePerKw = 750)
+    val Some((received, sent, Some(fee1))) = state3.computeTransactionDelta(tx)
+    assert(received === 0.sat)
+    assert(fee == fee1)
+    assert(tx.txOut.map(_.amount).sum + fee == state3.balance._1 + state3.balance._2)
+
+    val tx1 = Transaction(version = 2, txIn = Nil, txOut = TxOut(tx.txOut.map(_.amount).sum, pubkeyScript) :: Nil, lockTime = 0)
+    assert(Try(state3.completeTransaction(tx1, 750, 0 sat, dustLimit, true)).isSuccess)
+  }
+
   test("fuzzy test") {
     val random = new Random()
     (0 to 10) foreach { _ =>
@@ -197,7 +214,8 @@ class ElectrumWalletBasicSpec extends FunSuite with Logging {
         val amount = dustLimit + random.nextInt(10000000).sat
         val tx = Transaction(version = 2, txIn = Nil, txOut = TxOut(amount, Script.pay2pkh(state1.accountKeys(0).publicKey)) :: Nil, lockTime = 0)
         Try(state1.completeTransaction(tx, feeRatePerKw, minimumFee, dustLimit, true)) match {
-          case Success((state2, tx1, fee1)) => ()
+          case Success((state2, tx1, fee1)) =>
+            tx1.txOut.foreach(o => require(o.amount >= dustLimit, "output is below dust limit"))
           case Failure(cause) if cause.getMessage != null && cause.getMessage.contains("insufficient funds") => ()
           case Failure(cause) => logger.error(s"unexpected $cause")
         }

eclair-core/src/test/scala/fr/acinq/eclair/blockchain/electrum/ElectrumWalletSpec.scala

@@ -23,13 +23,15 @@ import java.util.concurrent.atomic.AtomicLong
 import akka.actor.{ActorRef, ActorSystem, Props}
 import akka.testkit.{TestKit, TestProbe}
 import com.whisk.docker.DockerReadyChecker
-import fr.acinq.bitcoin.{Block, Btc, ByteVector32, DeterministicWallet, MnemonicCode, Transaction, TxOut}
+import fr.acinq.bitcoin.{Block, Btc, ByteVector32, DeterministicWallet, MnemonicCode, OutPoint, Satoshi, Script, ScriptFlags, ScriptWitness, SigVersion, Transaction, TxIn, TxOut}
 import fr.acinq.eclair.LongToBtcAmount
 import fr.acinq.eclair.blockchain.bitcoind.BitcoinCoreWallet.{FundTransactionResponse, SignTransactionResponse}
 import fr.acinq.eclair.blockchain.bitcoind.{BitcoinCoreWallet, BitcoindService}
 import fr.acinq.eclair.blockchain.electrum.ElectrumClient.{BroadcastTransaction, BroadcastTransactionResponse, SSL}
 import fr.acinq.eclair.blockchain.electrum.ElectrumClientPool.ElectrumServerAddress
 import fr.acinq.eclair.blockchain.electrum.db.sqlite.SqliteWalletDb
+import fr.acinq.eclair.transactions.{Scripts, Transactions}
+import fr.acinq.{bitcoin, eclair}
 import grizzled.slf4j.Logging
 import org.json4s.JsonAST.{JDecimal, JString, JValue}
 import org.scalatest.{BeforeAndAfterAll, FunSuiteLike}
@@ -341,4 +343,65 @@ class ElectrumWalletSpec extends TestKit(ActorSystem("test")) with FunSuiteLike
     probe.send(wallet, IsDoubleSpent(tx2))
     probe.expectMsg(IsDoubleSpentResponse(tx2, true))
   }
+
+  test("use all available balance") {
+    val probe = TestProbe()
+
+    // send all our funds to ourself, so we have only one utxo which is the worse case here
+    val GetCurrentReceiveAddressResponse(address) = getCurrentAddress(probe)
+    probe.send(wallet, SendAll(Script.write(eclair.addressToPublicKeyScript(address, Block.RegtestGenesisBlock.hash)), 750))
+    val SendAllResponse(tx, _) = probe.expectMsgType[SendAllResponse]
+    probe.send(wallet, BroadcastTransaction(tx))
+    val BroadcastTransactionResponse(`tx`, None) = probe.expectMsgType[BroadcastTransactionResponse]
+
+    probe.send(bitcoincli, BitcoinReq("generate", 1))
+    probe.expectMsgType[JValue]
+
+    awaitCond({
+      probe.send(wallet, GetData)
+      val data = probe.expectMsgType[GetDataResponse].state
+      data.utxos.length == 1 && data.utxos(0).outPoint.txid == tx.txid
+    }, max = 30 seconds, interval = 1 second)
+
+
+    // send everything to a multisig 2-of-2, with the smallest possible fee rate
+    val priv = eclair.randomKey
+    val script = Script.pay2wsh(Scripts.multiSig2of2(priv.publicKey, priv.publicKey))
+    probe.send(wallet, SendAll(Script.write(script), eclair.MinimumFeeratePerKw))
+    val SendAllResponse(tx1, _) = probe.expectMsgType[SendAllResponse]
+    probe.send(wallet, BroadcastTransaction(tx1))
+    val BroadcastTransactionResponse(`tx1`, None) = probe.expectMsgType[BroadcastTransactionResponse]
+
+    probe.send(bitcoincli, BitcoinReq("generate", 1))
+    probe.expectMsgType[JValue]
+
+    awaitCond({
+      probe.send(wallet, GetData)
+      val data = probe.expectMsgType[GetDataResponse].state
+      data.utxos.isEmpty
+    }, max = 30 seconds, interval = 1 second)
+
+    // send everything back to ourselves again
+    val tx2 = Transaction(version = 2,
+      txIn = TxIn(OutPoint(tx1, 0),  signatureScript = Nil, sequence = TxIn.SEQUENCE_FINAL) :: Nil,
+      txOut = TxOut(Satoshi(0), eclair.addressToPublicKeyScript(address, Block.RegtestGenesisBlock.hash)) :: Nil,
+      lockTime = 0)
+
+    val sig = Transaction.signInput(tx2, 0, Scripts.multiSig2of2(priv.publicKey, priv.publicKey), bitcoin.SIGHASH_ALL, tx1.txOut(0).amount, SigVersion.SIGVERSION_WITNESS_V0, priv)
+    val tx3 = tx2.updateWitness(0, ScriptWitness(Seq(ByteVector.empty, sig, sig, Script.write(Scripts.multiSig2of2(priv.publicKey, priv.publicKey)))))
+    Transaction.correctlySpends(tx3, Seq(tx1), ScriptFlags.STANDARD_SCRIPT_VERIFY_FLAGS)
+    val fee = Transactions.weight2fee(tx3.weight(), 253)
+    val tx4 = tx3.copy(txOut = tx3.txOut(0).copy(amount = tx1.txOut(0).amount - fee) :: Nil)
+    val sig1 = Transaction.signInput(tx4, 0, Scripts.multiSig2of2(priv.publicKey, priv.publicKey), bitcoin.SIGHASH_ALL, tx1.txOut(0).amount, SigVersion.SIGVERSION_WITNESS_V0, priv)
+    val tx5 = tx4.updateWitness(0, ScriptWitness(Seq(ByteVector.empty, sig1, sig1, Script.write(Scripts.multiSig2of2(priv.publicKey, priv.publicKey)))))
+
+    probe.send(wallet, BroadcastTransaction(tx5))
+    val BroadcastTransactionResponse(_, None) = probe.expectMsgType[BroadcastTransactionResponse]
+
+    awaitCond({
+      probe.send(wallet, GetData)
+      val data = probe.expectMsgType[GetDataResponse].state
+      data.utxos.length == 1 && data.utxos(0).outPoint.txid == tx5.txid
+    }, max = 30 seconds, interval = 1 second)
+  }
 }