Stricter requirements on input details for signing (fixup for #3047)

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 {