Address review comments

* Fix typos, use more explicit method names, ...

* Use hardcoded ports in tests instead of "first available port"

* User-friendly error message when eclair-baked wallet creation fails at startup

* Simplify ReplaceableTxFunder

* Replace EitherKt with Scala's Either type

* Update signPsbt() to return Try

* Skip validation of local non-change outputs:

Local non-change outputs send to an external address, for splicing out funds for example.
We assume that these inputs are trusted i.e have been created by a trusted API call and our local
onchain key manager will not validate them during the signing process.

* Do not track our wallet inputs/outputs

It is currently easy to identify the wallet inputs/outputs that we added to fund/bump transactions, we don't need to explicitly track them.

* Document why we use a separate, specific file for the onchain key manager

Using a new signer section is eclair.conf would be simpler but "leaks" because it becomes available everywhere
in the code through the actor system's settings instead of being contained to where it is actually needed
and could potentially be exposed through a bug that "exports" the configuration (through logs, ....)
though this is highly unlikely.

docs/BitcoinCoreKeys.md

@@ -1,6 +1,6 @@
 # Using Eclair to manage your Bitcoin Core wallet's private keys
 
-You can configure Eclair to control (and never expose) the private keys of your Bitcoin Core wallet. This feature was designed to take advantage of deployements where your Eclair node runs in a
+You can configure Eclair to control (and never expose) the private keys of your Bitcoin Core wallet. This feature was designed to take advantage of deployment where your Eclair node runs in a
 "trusted" runtime environment, but is also very useful if your Bitcoin and Eclair nodes run on different machines for example, with a setup for the Bitcoin host that
 is less secure than for Eclair (because it is shared among several services for example).
 
@@ -43,7 +43,7 @@ This is an example of `eclair-signer.conf` configuration file:
 Set `eclair.bitcoind.wallet` to the name of the wallet in your `eclair-signer.conf` file (`eclair` in the example above) and restart Eclair.
 Eclair will automatically create a new, empty, descriptor-enabled, watch-only wallet in Bitcoin Core and import its descriptors.
 
-:warning: Eclair will not import descriptors if the timestamp set in your `eclair-signer.conf` is more than 2 hours old. If the mnmeonics and
+:warning: Eclair will not import descriptors if the timestamp set in your `eclair-signer.conf` is more than 2 hours old. If the mnemonics and
 passphrase that your are using are new, you can safely update this timestamp, but if they have been used before you will need to follow
 the steps described in the next section.
 
@@ -59,7 +59,7 @@ To send funds onchain you must use `eclair-cli sendonchain`.
 :warning: to backup the private keys of this wallet you must either backup your mnemonic code and passphrase, or backup the `eclair-signer.conf` file in your eclair
 directory (default is `~/.eclair`) along with your channels and node seed files.
 
-:warning: You can also initialise a backup onchain wallet with the same mnemonic code and passphrase (on a hardware wallet for example), but be warned that using them may interfere with your node's operations (for example you may end up
+:warning: You can also initialize a backup onchain wallet with the same mnemonic code and passphrase (on a hardware wallet for example), but be warned that using them may interfere with your node's operations (for example you may end up
 double-spending funding transactions generated by your node).
 
 ## Importing an existing Eclair-backed bitcoin core wallet
@@ -117,4 +117,4 @@ $ cat descriptors.json | xargs -0 bitcoin-cli -rpcwallet=eclair importdescriptor
 
 3) Configure Eclair to handle private keys for this wallet
 
-Set `eclair.bitcoind.wallet` to the name of the wallet in `eclair-signer.conf` restart Eclair.
+Set `eclair.bitcoind.wallet` to the name of the wallet in `eclair-signer.conf` and restart Eclair.

eclair-core/src/main/scala/fr/acinq/eclair/Setup.scala

@@ -142,12 +142,12 @@ class Setup(val datadir: File,
       port = config.getInt("bitcoind.rpcport"),
       wallet = wallet)
 
-    def createDescriptorWallet(wallets: List[String]): Future[Boolean] = {
+    def createEclairBackedWallet(wallets: List[String]): Future[Boolean] = {
       if (wallet.exists(name => wallets.contains(name))) {
         // wallet already exists
         Future.successful(true)
       } else {
-        new BitcoinCoreClient(bitcoinClient, onchainKeyManager_opt).createDescriptorWallet().recover { case e =>
+        new BitcoinCoreClient(bitcoinClient, onchainKeyManager_opt).createEclairBackedWallet().recover { case e =>
           logger.error(s"cannot create descriptor wallet", e)
           throw BitcoinWalletNotCreatedException(wallet.getOrElse(""))
         }
@@ -161,7 +161,8 @@ class Setup(val datadir: File,
         .collect {
           case JArray(values) => values.map(value => value.extract[String])
         }
-      true <- createDescriptorWallet(wallets)
+      walletCreated <- createEclairBackedWallet(wallets)
+      _ = assert(walletCreated, "Cannot create eclair-backed wallet, check logs for details")
       progress = (json \ "verificationprogress").extract[Double]
       ibd = (json \ "initialblockdownload").extract[Boolean]
       blocks = (json \ "blocks").extract[Long]

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

@@ -48,7 +48,7 @@ import scala.util.{Failure, Success, Try}
  * The Bitcoin Core client provides some high-level utility methods to interact with Bitcoin Core.
  *
  * @param rpcClient             bitcoin core JSON rpc client
- * @param onchainKeyManager_opt optional onchain key manager. If provided and its wallet name matcher our rpcClient wallet's name, it will be used to sign transactions (it is assumed that bitcoin
+ * @param onchainKeyManager_opt optional onchain key manager. If provided and its wallet name matches our rpcClient wallet's name, it will be used to sign transactions (it is assumed that bitcoin
  *                              core uses a watch-only wallet with descriptors generated by Eclair with this onchain key manager)
  */
 class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onchainKeyManager_opt: Option[OnchainKeyManager] = None) extends OnChainWallet with Logging {
@@ -64,22 +64,23 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onchainKeyManag
     rpcClient.invoke("importdescriptors", descriptors).collect {
       case JArray(results) => results.forall(item => {
         val JBool(success) = item \ "success"
-        val JArray(_) = item \ "warnings"
         success
       })
     }
   }
 
-  def createDescriptorWallet()(implicit ec: ExecutionContext): Future[Boolean] = {
+  def createEclairBackedWallet()(implicit ec: ExecutionContext): Future[Boolean] = {
     onchainKeyManager_opt match {
       case None => Future.successful(true) // no eclair-backed wallet is configured
       case Some(onchainKeyManager) if !rpcClient.wallet.contains(onchainKeyManager.wallet) => Future.successful(true) // configured wallet has a different name
-      case Some(onchainKeyManager) if !onchainKeyManager.getDescriptors(0).descriptors.forall(desc => TimestampSecond(desc.timestamp) >= TimestampSecond.now() - 2.hours) =>
+      case Some(onchainKeyManager) if onchainKeyManager.getWalletTimestamp() < (TimestampSecond.now() - 2.hours) =>
         logger.warn(s"descriptors are too old, you will need to manually import them and select how far back to rescan")
         Future.failed(new RuntimeException("Could not import descriptors, please check logs for details"))
       case Some(onchainKeyManager) =>
         logger.info(s"Creating a new on-chain eclair-backed wallet in bitcoind: ${onchainKeyManager.wallet}")
         for {
+          // arguments to createwallet: wallet_name, disable_private_keys=true, blank=true, passphrase="", avoid_reuse=false, descriptors=true, load_on_startup=true
+          // we create an empty watch-only wallet, and then import our public key descriptors
           _ <- rpcClient.invoke("createwallet", onchainKeyManager.wallet, true, true, "", false, true, true)
           _ = logger.info(s"importing new descriptors ${onchainKeyManager.getDescriptors(0).descriptors}")
           result <- importDescriptors(onchainKeyManager.getDescriptors(0).descriptors)
@@ -352,7 +353,7 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onchainKeyManag
         actualFees = kmp2scala(signed.psbt.computeFees())
         _ = require(actualFees == fees, s"actual funding fees $actualFees do not match bitcoin core fee $fees")
         fundingTx = kmp2scala(extracted.getRight)
-        actualFeerate = FeeratePerKw((actualFees * 1000) / fundingTx.weight())
+        actualFeerate = Transactions.fee2rate(actualFees, fundingTx.weight())
         maxFeerate = requestedFeeRate + requestedFeeRate / 2
         _ = require(actualFeerate < maxFeerate, s"actual feerate $actualFeerate is more than 50% above requested fee rate $targetFeerate")
         _ = logger.debug(s"created funding txid=${extracted.getRight.txid} outputIndex=$outputIndex fee=${fees}")
@@ -472,7 +473,7 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onchainKeyManag
 
     def sign(psbt: Psbt): Future[ProcessPsbtResponse] = if (useEclairSigner) {
       val onchainKeyManager = onchainKeyManager_opt.getOrElse(throw new RuntimeException("We are configured to use an eclair signer has not been loaded")) // this should not be possible
-      Try(onchainKeyManager.signPsbt(psbt, ourInputs, ourOutputs)) match {
+      onchainKeyManager.signPsbt(psbt, ourInputs, ourOutputs) match {
         case Success(signedPsbt) => Future.successful(ProcessPsbtResponse(signedPsbt, signedPsbt.extract().isRight))
         case Failure(error) => Future.failed(error)
       }
@@ -627,7 +628,7 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onchainKeyManag
       signedPsbt <- unlockIfFails(fundedTx.tx.txid, lockedOutputs)(signPsbt(new Psbt(fundedTx.tx), fundedTx.tx.txIn.indices, fundedTx.tx.txOut.indices.filterNot(_ == theirOutputPos)))
       signedTx = signedPsbt.finalTx
       actualFees = kmp2scala(signedPsbt.psbt.computeFees())
-      actualFeerate = FeeratePerKw((actualFees * 1000) / signedTx.weight())
+      actualFeerate = Transactions.fee2rate(actualFees, signedTx.weight())
       maxFeerate = feeratePerKw + feeratePerKw / 2
       _ = require(actualFeerate < maxFeerate, s"actual fee rate $actualFeerate is more than 50% above requested fee rate $feeratePerKw")
       txid <- publishTransaction(signedTx)

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

@@ -775,7 +775,14 @@ private class InteractiveTxBuilder(replyTo: ActorRef[InteractiveTxBuilder.Respon
     } else {
       // we only sign our wallet inputs, and check that we can spend our wallet outputs
       val ourWalletInputs = unsignedTx.localInputs.map(i => tx.txIn.indexWhere(_.outPoint == OutPoint(i.previousTx, i.previousTxOutput.toInt)))
-      val ourWalletOutputs = unsignedTx.localOutputs.map(o => tx.txOut.indexWhere(output => output.amount == o.amount && output.publicKeyScript == o.pubkeyScript))
+      val ourWalletOutputs = unsignedTx.localOutputs.collect {
+        // README!! there are 2 types of local outputs:
+        // Change, which go back into our wallet
+        // NonChange, which go to an external address (typically during a splice-out)
+        // Here we only keep outputs which are ours i.e go back into our wallet
+        // And we trust that NonChange outputs are valid. This only works if the entry point for creating such outputs is trusted (for example, a secure API call)
+        case Output.Local.Change(_, amount, pubkeyScript) => tx.txOut.indexWhere(output => output.amount == amount && output.publicKeyScript == pubkeyScript)
+      }
       context.pipeToSelf(wallet.signPsbt(new Psbt(tx), ourWalletInputs, ourWalletOutputs).map {
         response =>
           val localOutpoints = unsignedTx.localInputs.map(_.outPoint).toSet
@@ -786,7 +793,7 @@ private class InteractiveTxBuilder(replyTo: ActorRef[InteractiveTxBuilder.Respon
           val expectedLocalAmountIn = unsignedTx.localInputs.map(i => i.previousTx.txOut(i.previousTxOutput.toInt).amount).sum
           require(actualLocalAmountIn == expectedLocalAmountIn, s"local spent amount ${actualLocalAmountIn} does not match what we expect ($expectedLocalAmountIn")
           val actualLocalAmountOut = ourWalletOutputs.map(i => partiallySignedTx.txOut(i).amount).sum
-          val expectedLocalAmountOut = unsignedTx.localOutputs.map(_.amount).sum
+          val expectedLocalAmountOut = unsignedTx.localOutputs.collect { case c: Output.Local.Change => c.amount }.sum
           require(actualLocalAmountOut == expectedLocalAmountOut, s"local output amount ${actualLocalAmountOut} does not match what we expect ($expectedLocalAmountOut")
           val sigs = partiallySignedTx.txIn.filter(txIn => localOutpoints.contains(txIn.outPoint)).map(_.witness)
           PartiallySignedSharedTransaction(unsignedTx, TxSignatures(fundingParams.channelId, partiallySignedTx, sigs, sharedSig_opt))