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

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.
 
@@ -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 {
@@ -70,7 +70,7 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onchainKeyManag
     }
   }
 
-  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
@@ -80,6 +80,8 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onchainKeyManag
       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)

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

@@ -1437,7 +1437,7 @@ class BitcoinCoreClientWithEclairSignerSpec extends BitcoinCoreClientSpec {
     val onchainKeyManager = new LocalOnchainKeyManager(s"eclair_$hex", seed, TimestampSecond.now(), Block.RegtestGenesisBlock.hash)
     val jsonRpcClient = new BasicBitcoinJsonRPCClient(rpcAuthMethod = bitcoinrpcauthmethod, host = "localhost", port = bitcoindRpcPort, wallet = Some(s"eclair_$hex"))
     val wallet1 = new BitcoinCoreClient(jsonRpcClient, Some(onchainKeyManager))
-    wallet1.createDescriptorWallet().pipeTo(sender.ref)
+    wallet1.createEclairBackedWallet().pipeTo(sender.ref)
     sender.expectMsg(true)
 
     // this account xpub can be used to create a watch-only wallet
@@ -1477,7 +1477,7 @@ class BitcoinCoreClientWithEclairSignerSpec extends BitcoinCoreClientSpec {
       val onchainKeyManager = new LocalOnchainKeyManager(s"eclair_$hex", seed, TimestampSecond.now(), Block.RegtestGenesisBlock.hash)
       val jsonRpcClient = new BasicBitcoinJsonRPCClient(rpcAuthMethod = bitcoinrpcauthmethod, host = "localhost", port = bitcoindRpcPort, wallet = Some(s"eclair_$hex"))
       val wallet1 = new BitcoinCoreClient(jsonRpcClient, Some(onchainKeyManager))
-      wallet1.createDescriptorWallet().pipeTo(sender.ref)
+      wallet1.createEclairBackedWallet().pipeTo(sender.ref)
       sender.expectMsg(true)
       wallet1.getReceiveAddress().pipeTo(sender.ref)
       val address = sender.expectMsgType[String]

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

@@ -174,7 +174,7 @@ trait BitcoindService extends Logging {
     val sender = TestProbe()
     waitForBitcoindUp(sender)
     if (useEclairSigner) {
-      makeBitcoinCoreClient.createDescriptorWallet().pipeTo(sender.ref)
+      makeBitcoinCoreClient.createEclairBackedWallet().pipeTo(sender.ref)
       sender.expectMsg(true)
     } else {
       sender.send(bitcoincli, BitcoinReq("createwallet", defaultWallet))

eclair-core/src/test/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisherSpec.scala

@@ -1665,7 +1665,7 @@ class ReplaceableTxPublisherWithEclairSignerSpec extends ReplaceableTxPublisherS
 
       override def getP2wpkhPubkey(renew: Boolean): PublicKey = pubkey
     }
-    walletClient.createDescriptorWallet().pipeTo(probe.ref)
+    walletClient.createEclairBackedWallet().pipeTo(probe.ref)
     probe.expectMsg(true)
 
     (walletRpcClient, walletClient)

eclair-core/src/test/scala/fr/acinq/eclair/integration/ChannelIntegrationSpec.scala

@@ -37,7 +37,7 @@ import fr.acinq.eclair.router.Router
 import fr.acinq.eclair.transactions.Transactions.{AnchorOutputsCommitmentFormat, TxOwner}
 import fr.acinq.eclair.transactions.{OutgoingHtlc, Scripts, Transactions}
 import fr.acinq.eclair.wire.protocol._
-import fr.acinq.eclair.{MilliSatoshi, MilliSatoshiLong, TestUtils, randomBytes32}
+import fr.acinq.eclair.{MilliSatoshi, MilliSatoshiLong, randomBytes32}
 import org.json4s.JsonAST.{JString, JValue}
 
 import java.util.UUID
@@ -455,12 +455,9 @@ abstract class ChannelIntegrationSpec extends IntegrationSpec {
 class StandardChannelIntegrationSpec extends ChannelIntegrationSpec {
 
   test("start eclair nodes") {
-    val mapA = Map("eclair.node-alias" -> "A", "eclair.channel.expiry-delta-blocks" -> 40, "eclair.channel.fulfill-safety-before-timeout-blocks" -> 12, "eclair.server.port" -> TestUtils.availablePort, "eclair.api.port" -> TestUtils.availablePort)
-    val mapB = Map("eclair.node-alias" -> "C", "eclair.channel.expiry-delta-blocks" -> 40, "eclair.channel.fulfill-safety-before-timeout-blocks" -> 12, "eclair.server.port" -> TestUtils.availablePort, "eclair.api.port" -> TestUtils.availablePort)
-    val mapC = Map("eclair.node-alias" -> "F", "eclair.channel.expiry-delta-blocks" -> 40, "eclair.channel.fulfill-safety-before-timeout-blocks" -> 12, "eclair.server.port" -> TestUtils.availablePort, "eclair.api.port" -> TestUtils.availablePort)
-    instantiateEclairNode("A", ConfigFactory.parseMap(mapA.asJava).withFallback(withDefaultCommitment).withFallback(commonConfig))
-    instantiateEclairNode("C", ConfigFactory.parseMap(mapB.asJava).withFallback(withAnchorOutputs).withFallback(commonConfig))
-    instantiateEclairNode("F", ConfigFactory.parseMap(mapC.asJava).withFallback(withDefaultCommitment).withFallback(commonConfig))
+    instantiateEclairNode("A", ConfigFactory.parseMap(Map("eclair.node-alias" -> "A", "eclair.channel.expiry-delta-blocks" -> 40, "eclair.channel.fulfill-safety-before-timeout-blocks" -> 12, "eclair.server.port" -> (if (useEclairSigner) 29840 else 29740), "eclair.api.port" -> (if (useEclairSigner) 28190 else 28090)).asJava).withFallback(withDefaultCommitment).withFallback(commonConfig))
+    instantiateEclairNode("C", ConfigFactory.parseMap(Map("eclair.node-alias" -> "C", "eclair.channel.expiry-delta-blocks" -> 40, "eclair.channel.fulfill-safety-before-timeout-blocks" -> 12, "eclair.server.port" -> (if (useEclairSigner) 29841 else 29741), "eclair.api.port" -> (if (useEclairSigner) 28191 else 28091)).asJava).withFallback(withAnchorOutputs).withFallback(commonConfig))
+    instantiateEclairNode("F", ConfigFactory.parseMap(Map("eclair.node-alias" -> "F", "eclair.channel.expiry-delta-blocks" -> 40, "eclair.channel.fulfill-safety-before-timeout-blocks" -> 12, "eclair.server.port" -> (if (useEclairSigner) 29842 else 29742), "eclair.api.port" -> (if (useEclairSigner) 28192 else 28092)).asJava).withFallback(withDefaultCommitment).withFallback(commonConfig))
   }
 
   test("connect nodes") {

eclair-core/src/test/scala/fr/acinq/eclair/integration/IntegrationSpec.scala

@@ -151,8 +151,7 @@ abstract class IntegrationSpec extends TestKitBaseClass with BitcoindService wit
       Files.writeString(datadir.toPath.resolve("eclair-signer.conf"), eclairSignerConf)
     }
     implicit val system: ActorSystem = ActorSystem(s"system-$name", config)
-
-    val setup = new Setup(datadir, pluginParams = Seq.empty, seeds_opt = Some(Setup.Seeds(randomBytes32(), randomBytes32())))
+    val setup = new Setup(datadir, pluginParams = Seq.empty)
     val kit = Await.result(setup.bootstrap, 10 seconds)
     nodes = nodes + (name -> kit)
   }

eclair-core/src/test/scala/fr/acinq/eclair/integration/PaymentIntegrationSpec.scala

@@ -840,7 +840,7 @@ class PaymentIntegrationSpec extends IntegrationSpec {
       if (i % 10 == 0) {
         generateBlocks(1, Some(address))
       }
-      AnnouncementsBatchValidationSpec.simulateChannel(bitcoinClient, onchainKeyManager)
+      AnnouncementsBatchValidationSpec.simulateChannel(bitcoinClient)
     }
     generateBlocks(1, Some(address))
     logger.info(s"simulated ${channels.size} channels")

eclair-core/src/test/scala/fr/acinq/eclair/router/AnnouncementsBatchValidationSpec.scala

@@ -19,19 +19,18 @@ package fr.acinq.eclair.router
 import akka.actor.ActorSystem
 import akka.pattern.pipe
 import akka.testkit.TestProbe
+import sttp.client3.okhttp.OkHttpFutureBackend
 import fr.acinq.bitcoin.scalacompat.Crypto.PrivateKey
 import fr.acinq.bitcoin.scalacompat.{Block, Satoshi, SatoshiLong, Script, Transaction}
 import fr.acinq.eclair.blockchain.bitcoind.ZmqWatcher.ValidateResult
 import fr.acinq.eclair.blockchain.bitcoind.rpc.BitcoinJsonRPCAuthMethod.UserPassword
 import fr.acinq.eclair.blockchain.bitcoind.rpc.{BasicBitcoinJsonRPCClient, BitcoinCoreClient}
 import fr.acinq.eclair.blockchain.fee.FeeratePerKw
-import fr.acinq.eclair.crypto.keymanager.OnchainKeyManager
 import fr.acinq.eclair.transactions.Scripts
 import fr.acinq.eclair.wire.protocol.{ChannelAnnouncement, ChannelUpdate}
 import fr.acinq.eclair.{BlockHeight, CltvExpiryDelta, Features, MilliSatoshiLong, RealShortChannelId, ShortChannelId, randomKey}
 import org.json4s.JsonAST.JString
 import org.scalatest.funsuite.AnyFunSuite
-import sttp.client3.okhttp.OkHttpFutureBackend
 
 import scala.concurrent.duration._
 import scala.concurrent.{Await, ExecutionContext}
@@ -55,7 +54,7 @@ class AnnouncementsBatchValidationSpec extends AnyFunSuite {
     val channels = for (i <- 0 until 50) yield {
       // let's generate a block every 10 txs so that we can compute short ids
       if (i % 10 == 0) generateBlocks(1, bitcoinClient)
-      simulateChannel(bitcoinClient, null)
+      simulateChannel(bitcoinClient)
     }
     generateBlocks(6, bitcoinClient)
     val announcements = channels.map(c => makeChannelAnnouncement(c, bitcoinClient))
@@ -86,7 +85,7 @@ object AnnouncementsBatchValidationSpec {
     Await.result(generatedF, 10 seconds)
   }
 
-  def simulateChannel(bitcoinClient: BitcoinCoreClient, onchainKeyManager: OnchainKeyManager)(implicit ec: ExecutionContext): SimulatedChannel = {
+  def simulateChannel(bitcoinClient: BitcoinCoreClient)(implicit ec: ExecutionContext): SimulatedChannel = {
     val node1Key = randomKey()
     val node2Key = randomKey()
     val node1BitcoinKey = randomKey()