Stricter requirements on input details for signing (#3052)

We verify that details about all inputs are provided to the `sign`
function. While this isn't mandatory for segwit v0, it ensures that
all of our existing tests exercise this codepath and reduces the risk
that we forget to provide some wallet inputs, which would result in an
invalid signature which would be hard to investigate.

With this change, some of the unit tests started failing, which showed
that we weren't correctly setting wallet inputs in the fee-bumping case
in `ReplaceableTxFunder`, which we've fixed.

We also add a test in `TransactionsSpec.scala` to verify that signing
fails when details about some inputs are missing.

Author: t-bast

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

@@ -305,7 +305,7 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val lockUtxos: Bool
     })
   }
 
-  def utxoUpdatePsbt(psbt: Psbt)(implicit ec: ExecutionContext): Future[Psbt] = {
+  private def utxoUpdatePsbt(psbt: Psbt)(implicit ec: ExecutionContext): Future[Psbt] = {
     val encoded = Base64.getEncoder.encodeToString(Psbt.write(psbt).toByteArray)
     rpcClient.invoke("utxoupdatepsbt", encoded).map(json => {
       val JString(base64) = json

eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala

@@ -105,18 +105,15 @@ object InteractiveTxBuilder {
     // @formatter:off
     def info: InputInfo
     def weight: Int
-    // we don't need to provide extra inputs here, as this method will only be called for multisig-2-of-2 inputs which only require the output that is spent for signing
-    // for taproot channels, we'll use a musig2 input that is not signed like this: instead, the signature will be the Musig2 aggregation if a local and remote partial signature
-    def sign(keyManager: ChannelKeyManager, params: ChannelParams, tx: Transaction): ByteVector64
     // @formatter:on
   }
 
   case class Multisig2of2Input(info: InputInfo, fundingTxIndex: Long, remoteFundingPubkey: PublicKey) extends SharedFundingInput {
     override val weight: Int = 388
 
-    override def sign(keyManager: ChannelKeyManager, params: ChannelParams, tx: Transaction): ByteVector64 = {
+    def sign(keyManager: ChannelKeyManager, params: ChannelParams, tx: Transaction, spentUtxos: Map[OutPoint, TxOut]): ByteVector64 = {
       val localFundingPubkey = keyManager.fundingPublicKey(params.localParams.fundingKeyPath, fundingTxIndex)
-      keyManager.sign(Transactions.SpliceTx(info, tx), localFundingPubkey, TxOwner.Local, params.commitmentFormat, Map.empty)
+      keyManager.sign(Transactions.SpliceTx(info, tx), localFundingPubkey, TxOwner.Local, params.commitmentFormat, spentUtxos)
     }
   }
 
@@ -339,6 +336,8 @@ object InteractiveTxBuilder {
     val remoteFees: MilliSatoshi = remoteAmountIn - remoteAmountOut
     // Note that the truncation is a no-op: sub-satoshi balances are carried over from inputs to outputs and cancel out.
     val fees: Satoshi = (localFees + remoteFees).truncateToSatoshi
+    // When signing transactions that include taproot inputs, we must provide details about all of the transaction's inputs.
+    val inputDetails: Map[OutPoint, TxOut] = (sharedInput_opt.toSeq.map(i => i.outPoint -> i.txOut) ++ localInputs.map(i => i.outPoint -> i.txOut) ++ remoteInputs.map(i => i.outPoint -> i.txOut)).toMap
 
     def localOnlyNonChangeOutputs: List[Output.Local.NonChange] = localOutputs.collect { case o: Local.NonChange => o }
 
@@ -914,7 +913,9 @@ private class InteractiveTxBuilder(replyTo: ActorRef[InteractiveTxBuilder.Respon
     import fr.acinq.bitcoin.scalacompat.KotlinUtils._
 
     val tx = unsignedTx.buildUnsignedTx()
-    val sharedSig_opt = fundingParams.sharedInput_opt.map(_.sign(keyManager, channelParams, tx))
+    val sharedSig_opt = fundingParams.sharedInput_opt.collect {
+      case i: Multisig2of2Input => i.sign(keyManager, channelParams, tx, unsignedTx.inputDetails)
+    }
     if (unsignedTx.localInputs.isEmpty) {
       context.self ! SignTransactionResult(PartiallySignedSharedTransaction(unsignedTx, TxSignatures(fundingParams.channelId, tx, Nil, sharedSig_opt)))
     } else {

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxFunder.scala

@@ -274,7 +274,7 @@ private class ReplaceableTxFunder(nodeParams: NodeParams,
         Behaviors.stopped
       case AdjustPreviousTxOutputResult.TxOutputAdjusted(updatedTx) =>
         log.debug("bumping {} fees without adding new inputs: txid={}", cmd.desc, updatedTx.txInfo.tx.txid)
-        sign(updatedTx, targetFeerate, previousTx.totalAmountIn, Map.empty)
+        sign(updatedTx, targetFeerate, previousTx.totalAmountIn, previousTx.walletInputs)
       case AdjustPreviousTxOutputResult.AddWalletInputs(tx) =>
         log.debug("bumping {} fees requires adding new inputs (feerate={})", cmd.desc, targetFeerate)
         // We restore the original transaction (remove previous attempt's wallet inputs).
@@ -426,32 +426,23 @@ private class ReplaceableTxFunder(nodeParams: NodeParams,
     }
   }
 
-  private def addInputs(tx: ReplaceableTxWithWalletInputs, targetFeerate: FeeratePerKw, commitment: FullCommitment): Future[(ReplaceableTxWithWalletInputs, Satoshi, Map[OutPoint, TxOut])] = {
-
-    def getWalletUtxos(txInfo: TransactionWithInputInfo): Future[Map[OutPoint, TxOut]] = {
-      val inputs = txInfo.tx.txIn.filterNot(_.outPoint == txInfo.input.outPoint)
-      val txids = inputs.map(_.outPoint.txid).toSet
-      for {
-        txs <- Future.sequence(txids.toSeq.map(bitcoinClient.getTransaction))
-        txMap = txs.map(tx => tx.txid -> tx).toMap
-      } yield {
-        inputs.map(_.outPoint).map(outPoint => {
-          require(txMap.contains(outPoint.txid) && txMap(outPoint.txid).txOut.size >= outPoint.index, s"missing wallet input $outPoint")
-          outPoint -> txMap(outPoint.txid).txOut(outPoint.index.toInt)
-        }).toMap
+  private def getWalletUtxos(txInfo: TransactionWithInputInfo): Future[Map[OutPoint, TxOut]] = {
+    Future.sequence(txInfo.tx.txIn.filter(_.outPoint != txInfo.input.outPoint).map(txIn => {
+      bitcoinClient.getTransaction(txIn.outPoint.txid).flatMap {
+        case inputTx if inputTx.txOut.size <= txIn.outPoint.index => Future.failed(new IllegalArgumentException(s"input ${inputTx.txid}:${txIn.outPoint.index} doesn't exist"))
+        case inputTx => Future.successful(txIn.outPoint -> inputTx.txOut(txIn.outPoint.index.toInt))
       }
-    }
+    })).map(_.toMap)
+  }
 
-    tx match {
-      case anchorTx: ClaimLocalAnchorWithWitnessData => for {
-        (fundedTx, amountIn) <- addInputs(anchorTx, targetFeerate, commitment)
-        spentUtxos <- getWalletUtxos(fundedTx.txInfo)
-      } yield (fundedTx, amountIn, spentUtxos)
-      case htlcTx: HtlcWithWitnessData => for {
-        (fundedTx, amountIn) <- addInputs(htlcTx, targetFeerate, commitment)
-        spentUtxos <- getWalletUtxos(fundedTx.txInfo)
-      } yield (fundedTx, amountIn, spentUtxos)
-    }
+  private def addInputs(tx: ReplaceableTxWithWalletInputs, targetFeerate: FeeratePerKw, commitment: FullCommitment): Future[(ReplaceableTxWithWalletInputs, Satoshi, Map[OutPoint, TxOut])] = {
+    for {
+      (fundedTx, amountIn) <- tx match {
+        case anchorTx: ClaimLocalAnchorWithWitnessData => addInputs(anchorTx, targetFeerate, commitment)
+        case htlcTx: HtlcWithWitnessData => addInputs(htlcTx, targetFeerate, commitment)
+      }
+      spentUtxos <- getWalletUtxos(fundedTx.txInfo)
+    } yield (fundedTx, amountIn, spentUtxos)
   }
 
   private def addInputs(anchorTx: ClaimLocalAnchorWithWitnessData, targetFeerate: FeeratePerKw, commitment: FullCommitment): Future[(ClaimLocalAnchorWithWitnessData, Satoshi)] = {

eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/ChannelKeyManager.scala

@@ -64,7 +64,7 @@ trait ChannelKeyManager {
    * @param publicKey        extended public key
    * @param txOwner          owner of the transaction (local/remote)
    * @param commitmentFormat format of the commitment tx
-   * @param extraUtxos       extra outputs spent by this transaction (in addition to our [[fr.acinq.eclair.transactions.Transactions.InputInfo]] output, which is assumed to always be the first spent output)
+   * @param extraUtxos       extra outputs spent by this transaction (in addition to [[fr.acinq.eclair.transactions.Transactions.InputInfo]])
    * @return a signature generated with the private key that matches the input extended public key
    */
   def sign(tx: TransactionWithInputInfo, publicKey: ExtendedPublicKey, txOwner: TxOwner, commitmentFormat: CommitmentFormat, extraUtxos: Map[OutPoint, TxOut]): ByteVector64
@@ -78,7 +78,7 @@ trait ChannelKeyManager {
    * @param remotePoint      remote point
    * @param txOwner          owner of the transaction (local/remote)
    * @param commitmentFormat format of the commitment tx
-   * @param extraUtxos       extra outputs spent by this transaction (in addition to our [[fr.acinq.eclair.transactions.Transactions.InputInfo]] output, which is assumed to always be the first spent output)
+   * @param extraUtxos       extra outputs spent by this transaction (in addition to [[fr.acinq.eclair.transactions.Transactions.InputInfo]])
    * @return a signature generated with a private key generated from the input key's matching private key and the remote point.
    */
   def sign(tx: TransactionWithInputInfo, publicKey: ExtendedPublicKey, remotePoint: PublicKey, txOwner: TxOwner, commitmentFormat: CommitmentFormat, extraUtxos: Map[OutPoint, TxOut]): ByteVector64
@@ -91,7 +91,7 @@ trait ChannelKeyManager {
    * @param remoteSecret     remote secret
    * @param txOwner          owner of the transaction (local/remote)
    * @param commitmentFormat format of the commitment tx
-   * @param extraUtxos       extra outputs spent by this transaction (in addition to our [[fr.acinq.eclair.transactions.Transactions.InputInfo]] output, which is assumed to always be the first spent output)
+   * @param extraUtxos       extra outputs spent by this transaction (in addition to [[fr.acinq.eclair.transactions.Transactions.InputInfo]])
    * @return a signature generated with a private key generated from the input key's matching private key and the remote secret.
    */
   def sign(tx: TransactionWithInputInfo, publicKey: ExtendedPublicKey, remoteSecret: PrivateKey, txOwner: TxOwner, commitmentFormat: CommitmentFormat, extraUtxos: Map[OutPoint, TxOut]): ByteVector64

eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalChannelKeyManager.scala

@@ -104,7 +104,7 @@ class LocalChannelKeyManager(seed: ByteVector, chainHash: BlockHash) extends Cha
    * @param publicKey        extended public key
    * @param txOwner          owner of the transaction (local/remote)
    * @param commitmentFormat format of the commitment tx
-   * @param extraUtxos       extra outputs spent by this transaction (in addition to our [[fr.acinq.eclair.transactions.Transactions.InputInfo]] output, which is assumed to always be the first spent output)
+   * @param extraUtxos       extra outputs spent by this transaction (in addition to [[fr.acinq.eclair.transactions.Transactions.InputInfo]])
    * @return a signature generated with the private key that matches the input extended public key
    */
   override def sign(tx: TransactionWithInputInfo, publicKey: ExtendedPublicKey, txOwner: TxOwner, commitmentFormat: CommitmentFormat, extraUtxos: Map[OutPoint, TxOut]): ByteVector64 = {
@@ -125,7 +125,7 @@ class LocalChannelKeyManager(seed: ByteVector, chainHash: BlockHash) extends Cha
    * @param remotePoint      remote point
    * @param txOwner          owner of the transaction (local/remote)
    * @param commitmentFormat format of the commitment tx
-   * @param extraUtxos       extra outputs spent by this transaction (in addition to our [[fr.acinq.eclair.transactions.Transactions.InputInfo]] output, which is assumed to always be the first spent output)
+   * @param extraUtxos       extra outputs spent by this transaction (in addition to [[fr.acinq.eclair.transactions.Transactions.InputInfo]])
    * @return a signature generated with a private key generated from the input key's matching private key and the remote point.
    */
   override def sign(tx: TransactionWithInputInfo, publicKey: ExtendedPublicKey, remotePoint: PublicKey, txOwner: TxOwner, commitmentFormat: CommitmentFormat, extraUtxos: Map[OutPoint, TxOut]): ByteVector64 = {
@@ -147,7 +147,7 @@ class LocalChannelKeyManager(seed: ByteVector, chainHash: BlockHash) extends Cha
    * @param remoteSecret     remote secret
    * @param txOwner          owner of the transaction (local/remote)
    * @param commitmentFormat format of the commitment tx
-   * @param extraUtxos       extra outputs spent by this transaction (in addition to our [[fr.acinq.eclair.transactions.Transactions.InputInfo]] output, which is assumed to always be the first spent output)
+   * @param extraUtxos       extra outputs spent by this transaction (in addition to [[fr.acinq.eclair.transactions.Transactions.InputInfo]])
    * @return a signature generated with a private key generated from the input key's matching private key and the remote secret.
    */
   override def sign(tx: TransactionWithInputInfo, publicKey: ExtendedPublicKey, remoteSecret: PrivateKey, txOwner: TxOwner, commitmentFormat: CommitmentFormat, extraUtxos: Map[OutPoint, TxOut]): ByteVector64 = {

eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala

@@ -131,18 +131,31 @@ object Transactions {
     /** Sighash flags to use when signing the transaction. */
     def sighash(txOwner: TxOwner, commitmentFormat: CommitmentFormat): Int = SIGHASH_ALL
 
+    /**
+     * @param extraUtxos extra outputs spent by this transaction (in addition to the main [[input]]).
+     */
     def sign(key: PrivateKey, txOwner: TxOwner, commitmentFormat: CommitmentFormat, extraUtxos: Map[OutPoint, TxOut]): ByteVector64 = {
       sign(key, sighash(txOwner, commitmentFormat), extraUtxos)
     }
 
-    def sign(key: PrivateKey, sighashType: Int, extraUtxos: Map[OutPoint, TxOut]): ByteVector64 = input match {
-      case _: InputInfo.TaprootInput => ByteVector64.Zeroes
-      case InputInfo.SegwitInput(outPoint, txOut, redeemScript) =>
-        // NB: the tx may have multiple inputs, we will only sign the one provided in txinfo.input. Bear in mind that the
-        // signature will be invalidated if other inputs are added *afterwards* and sighashType was SIGHASH_ALL.
-        val inputIndex = tx.txIn.indexWhere(_.outPoint == outPoint)
-        val sigDER = Transaction.signInput(tx, inputIndex, redeemScript, sighashType, txOut.amount, SIGVERSION_WITNESS_V0, key)
-        Crypto.der2compact(sigDER)
+    def sign(key: PrivateKey, sighashType: Int, extraUtxos: Map[OutPoint, TxOut]): ByteVector64 = {
+      val inputsMap = extraUtxos + (input.outPoint -> input.txOut)
+      tx.txIn.foreach(txIn => {
+        // Note that using a require here is dangerous, because callers don't except this function to throw.
+        // But we want to ensure that we're correctly providing input details, otherwise our signature will silently be
+        // invalid when using taproot. We verify this in all cases, even when using segwit v0, to ensure that we have as
+        // many tests as possible that exercise this codepath.
+        require(inputsMap.contains(txIn.outPoint), s"cannot sign $desc with txId=${tx.txid}: missing input details for ${txIn.outPoint}")
+      })
+      input match {
+        case InputInfo.SegwitInput(outPoint, txOut, redeemScript) =>
+          // NB: the tx may have multiple inputs, we will only sign the one provided in txinfo.input. Bear in mind that the
+          // signature will be invalidated if other inputs are added *afterwards* and sighashType was SIGHASH_ALL.
+          val inputIndex = tx.txIn.indexWhere(_.outPoint == outPoint)
+          val sigDER = Transaction.signInput(tx, inputIndex, redeemScript, sighashType, txOut.amount, SIGVERSION_WITNESS_V0, key)
+          Crypto.der2compact(sigDER)
+        case _: InputInfo.TaprootInput => ???
+      }
     }
 
     def checkSig(sig: ByteVector64, pubKey: PublicKey, txOwner: TxOwner, commitmentFormat: CommitmentFormat): Boolean = input match {