From 0f47b52ebcfdb1b8e232d203783e3455ac90d82a Mon Sep 17 00:00:00 2001 From: sstone Date: Wed, 20 Sep 2023 13:28:37 +0200 Subject: [PATCH] Address review comments Rename `wallet` field to `walletName` Improve comments related to signature workflow and verification of wallet inputs/outputs Remove `descriptorChecksum` method: it's provided by bitcoin-kmp and does not need to be exposed --- .../main/scala/fr/acinq/eclair/Setup.scala | 9 ++++---- .../eclair/blockchain/OnChainWallet.scala | 8 ++++++- .../bitcoind/rpc/BitcoinCoreClient.scala | 16 +++++++++++++- .../channel/fund/InteractiveTxBuilder.scala | 3 ++- .../keymanager/LocalOnChainKeyManager.scala | 21 ++++++++++++------- .../crypto/keymanager/OnChainKeyManager.scala | 2 +- .../LocalOnChainKeyManagerSpec.scala | 17 --------------- 7 files changed, 42 insertions(+), 34 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/Setup.scala b/eclair-core/src/main/scala/fr/acinq/eclair/Setup.scala index f1653bf615..fcf3548d32 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/Setup.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/Setup.scala @@ -150,10 +150,10 @@ class Setup(val datadir: File, case JArray(values) => values.map(value => value.extract[String]) } eclairBackedWalletOk <- onChainKeyManager_opt match { - case Some(keyManager) if !wallets.contains(keyManager.wallet) => keyManager.createWallet(bitcoinClient) + case Some(keyManager) if !wallets.contains(keyManager.walletName) => keyManager.createWallet(bitcoinClient) case _ => Future.successful(true) } - _ = assert(eclairBackedWalletOk || onChainKeyManager_opt.map(_.wallet) != wallet, s"cannot create eclair-backed wallet=${onChainKeyManager_opt.map(_.wallet)}, check logs for details") + _ = assert(eclairBackedWalletOk || onChainKeyManager_opt.map(_.walletName) != wallet, s"cannot create eclair-backed wallet=${onChainKeyManager_opt.map(_.walletName)}, check logs for details") progress = (json \ "verificationprogress").extract[Double] ibd = (json \ "initialblockdownload").extract[Boolean] blocks = (json \ "blocks").extract[Long] @@ -252,7 +252,7 @@ class Setup(val datadir: File, finalPubkey = new AtomicReference[PublicKey](null) pubkeyRefreshDelay = FiniteDuration(config.getDuration("bitcoind.final-pubkey-refresh-delay").getSeconds, TimeUnit.SECONDS) - bitcoinClient = new BitcoinCoreClient(bitcoin, if (bitcoin.wallet == onChainKeyManager_opt.map(_.wallet)) onChainKeyManager_opt else None) with OnchainPubkeyCache { + bitcoinClient = new BitcoinCoreClient(bitcoin, if (bitcoin.wallet == onChainKeyManager_opt.map(_.walletName)) onChainKeyManager_opt else None) with OnchainPubkeyCache { val refresher: typed.ActorRef[OnchainPubkeyRefresher.Command] = system.spawn(Behaviors.supervise(OnchainPubkeyRefresher(this, finalPubkey, pubkeyRefreshDelay)).onFailure(typed.SupervisorStrategy.restart), name = "onchain-address-manager") override def getP2wpkhPubkey(renew: Boolean): PublicKey = { @@ -314,8 +314,7 @@ class Setup(val datadir: File, routerTimeout = after(FiniteDuration(config.getDuration("router.init-timeout").getSeconds, TimeUnit.SECONDS), using = system.scheduler)(Future.failed(new RuntimeException("Router initialization timed out"))) _ <- Future.firstCompletedOf(routerInitialized.future :: routerTimeout :: Nil) - _ = bitcoinClient.getReceiveAddress().map(address => logger.info(s"initial address=$address for bitcoin wallet=${bitcoinClient.rpcClient.wallet.getOrElse("")}")) - + _ = bitcoinClient.getReceiveAddress().map(address => logger.info(s"initial address=$address for bitcoin wallet=${bitcoinClient.rpcClient.wallet.getOrElse("(default)")}")) channelsListener = system.spawn(ChannelsListener(channelsListenerReady), name = "channels-listener") _ <- channelsListenerReady.future diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/OnChainWallet.scala b/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/OnChainWallet.scala index 0138f857af..bc2ac328b5 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/OnChainWallet.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/OnChainWallet.scala @@ -39,7 +39,13 @@ trait OnChainChannelFunder { */ def fundTransaction(tx: Transaction, feeRate: FeeratePerKw, replaceable: Boolean, externalInputsWeight: Map[OutPoint, Long] = Map.empty)(implicit ec: ExecutionContext): Future[FundTransactionResponse] - /** Sign a PSBT. Result may be partially signed: only inputs known to our bitcoin wallet will be signed. */ + /** + * Sign a PSBT. Result may be partially signed: only inputs known to our bitcoin wallet will be signed. * + * + * @param psbt PSBT to sign + * @param ourInputs our wallet inputs. If Eclair is managing Bitcoin Core wallet keys, only these inputs will be signed. + * @param ourOutputs our wallet outputs. If Eclair is managing Bitcoin Core wallet keys, it will check that it can actually spend them (i.e re-compute private keys for them) + */ def signPsbt(psbt: Psbt, ourInputs: Seq[Int], ourOutputs: Seq[Int])(implicit ec: ExecutionContext): Future[ProcessPsbtResponse] /** diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala b/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala index 7ae6e641c2..f8315e960b 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala @@ -57,7 +57,7 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onChainKeyManag implicit val formats: Formats = org.json4s.DefaultFormats onChainKeyManager_opt.foreach { keyManager => - require(rpcClient.wallet.contains(keyManager.wallet), s"eclair-backed bitcoin wallet mismatch: eclair-signer.conf uses wallet=${keyManager.wallet}, but eclair.conf uses wallet=${rpcClient.wallet.getOrElse("")}") + require(rpcClient.wallet.contains(keyManager.walletName), s"eclair-backed bitcoin wallet mismatch: eclair-signer.conf uses wallet=${keyManager.walletName}, but eclair.conf uses wallet=${rpcClient.wallet.getOrElse("")}") } val useEclairSigner = onChainKeyManager_opt.nonEmpty @@ -753,7 +753,21 @@ object BitcoinCoreClient { def toSatoshi(btcAmount: BigDecimal): Satoshi = Satoshi(btcAmount.bigDecimal.scaleByPowerOfTen(8).longValue) + /** + * Bitcoin Core descriptor, as used in Bitcoin Core's RPC API. + * + * @param desc Descriptor string representation + * @param internal True if this descriptor is used to generate change addresses. False if this descriptor is used to generate receiving addresses; defined only for active descriptors + * @param timestamp The creation time of the descriptor + * @param active Whether this descriptor is currently used to generate new addresses + */ case class Descriptor(desc: String, internal: Boolean = false, timestamp: Long, active: Boolean = true) + /** + * Descriptors for a specific Bitcoin wallet. + * + * @param wallet_name wallet name + * @param descriptors list of wallet descriptors + */ case class Descriptors(wallet_name: String, descriptors: Seq[Descriptor]) } \ No newline at end of file diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala index 998f8e5d11..dc358fbbda 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala @@ -773,7 +773,6 @@ private class InteractiveTxBuilder(replyTo: ActorRef[InteractiveTxBuilder.Respon if (unsignedTx.localInputs.isEmpty) { context.self ! SignTransactionResult(PartiallySignedSharedTransaction(unsignedTx, TxSignatures(fundingParams.channelId, tx, Nil, sharedSig_opt))) } 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 == i.outPoint)) val ourWalletOutputs = unsignedTx.localOutputs.flatMap { case Output.Local.Change(_, amount, pubkeyScript) => Some(tx.txOut.indexWhere(output => output.amount == amount && output.publicKeyScript == pubkeyScript)) @@ -782,6 +781,8 @@ private class InteractiveTxBuilder(replyTo: ActorRef[InteractiveTxBuilder.Respon // We trust that non-change 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.NonChange => None } + // We track our wallet inputs and outputs, so we can verify them when we sign the transaction: if Eclair is managing bitcoin core wallet keys, it will + // only sign our wallet inputs, and check that it can re-compute private keys for our wallet outputs context.pipeToSelf(wallet.signPsbt(new Psbt(tx), ourWalletInputs, ourWalletOutputs).map { response => val localOutpoints = unsignedTx.localInputs.map(_.outPoint).toSet diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnChainKeyManager.scala b/eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnChainKeyManager.scala index 35cdeeed07..56cf4d86c0 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnChainKeyManager.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnChainKeyManager.scala @@ -34,7 +34,6 @@ import scala.jdk.CollectionConverters.{CollectionHasAsScala, MapHasAsScala} import scala.util.{Failure, Success, Try} object LocalOnChainKeyManager extends Logging { - def descriptorChecksum(span: String): String = fr.acinq.bitcoin.Descriptor.checksum(span) /** * Load a configuration file and create an on-chain key manager @@ -62,7 +61,13 @@ object LocalOnChainKeyManager extends Logging { } } -class LocalOnChainKeyManager(override val wallet: String, seed: ByteVector, override val walletTimestamp: TimestampSecond, chainHash: ByteVector32) extends OnChainKeyManager with Logging { +/** + * A manager for on-chain keys used by Eclair, to be used in combination with a watch-only descriptor-based wallet managed by Bitcoin Core. + * In this setup, Bitcoin Core handles all non-sensitive wallet tasks (including watching the blockchain and building transactions), while + * Eclair is in charge of signing transactions. + * This is an advanced feature particularly suited when Eclair runs in a secure runtime. + */ +class LocalOnChainKeyManager(override val walletName: String, seed: ByteVector, override val walletTimestamp: TimestampSecond, chainHash: ByteVector32) extends OnChainKeyManager with Logging { import LocalOnChainKeyManager._ @@ -100,11 +105,11 @@ class LocalOnChainKeyManager(override val wallet: String, seed: ByteVector, over override def createWallet(rpcClient: BitcoinJsonRPCClient)(implicit ec: ExecutionContext): Future[Boolean] = { if (walletTimestamp < (TimestampSecond.now() - 2.hours)) { - logger.warn(s"eclair-backed wallet descriptors for wallet=$wallet are too old to be automatically imported into bitcoin core, you will need to manually import them and select how far back to rescan") + logger.warn(s"eclair-backed wallet descriptors for wallet=$walletName are too old to be automatically imported into bitcoin core, you will need to manually import them and select how far back to rescan") Future.successful(false) } else { - logger.info(s"creating a new on-chain eclair-backed wallet in bitcoind: $wallet") - rpcClient.invoke("createwallet", wallet, /* disable_private_keys */ true, /* blank */ true, /* passphrase */ "", /* avoid_reuse */ false, /* descriptors */ true, /* load_on_startup */ true).flatMap(_ => { + logger.info(s"creating a new on-chain eclair-backed wallet in bitcoind: $walletName") + rpcClient.invoke("createwallet", walletName, /* disable_private_keys */ true, /* blank */ true, /* passphrase */ "", /* avoid_reuse */ false, /* descriptors */ true, /* load_on_startup */ true).flatMap(_ => { logger.info(s"importing new descriptors ${descriptors(0).descriptors}") rpcClient.invoke("importdescriptors", descriptors(0).descriptors).collect { case JArray(results) => results.forall(item => { @@ -132,9 +137,9 @@ class LocalOnChainKeyManager(override val wallet: String, seed: ByteVector, over // 84'/{0'/1'}/0'/1/* for change addresses val receiveDesc = s"wpkh([$fingerPrintHex/$keyPath]${encode(accountPub, prefix)}/0/*)" val changeDesc = s"wpkh([$fingerPrintHex/$keyPath]${encode(accountPub, prefix)}/1/*)" - Descriptors(wallet_name = wallet, descriptors = List( - Descriptor(desc = s"$receiveDesc#${descriptorChecksum(receiveDesc)}", internal = false, active = true, timestamp = walletTimestamp.toLong), - Descriptor(desc = s"$changeDesc#${descriptorChecksum(changeDesc)}", internal = true, active = true, timestamp = walletTimestamp.toLong), + Descriptors(wallet_name = walletName, descriptors = List( + Descriptor(desc = s"$receiveDesc#${fr.acinq.bitcoin.Descriptor.checksum(receiveDesc)}", internal = false, active = true, timestamp = walletTimestamp.toLong), + Descriptor(desc = s"$changeDesc#${fr.acinq.bitcoin.Descriptor.checksum(changeDesc)}", internal = true, active = true, timestamp = walletTimestamp.toLong), )) } diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/OnChainKeyManager.scala b/eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/OnChainKeyManager.scala index 867289b4bd..969253a803 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/OnChainKeyManager.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/OnChainKeyManager.scala @@ -27,7 +27,7 @@ import scala.concurrent.{ExecutionContext, Future} import scala.util.Try trait OnChainKeyManager { - def wallet: String + def walletName: String /** * @return the creation time of the wallet managed by this key manager diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/crypto/keymanager/LocalOnChainKeyManagerSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/crypto/keymanager/LocalOnChainKeyManagerSpec.scala index 353d34c3da..b550e452ea 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/crypto/keymanager/LocalOnChainKeyManagerSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/crypto/keymanager/LocalOnChainKeyManagerSpec.scala @@ -114,21 +114,4 @@ class LocalOnChainKeyManagerSpec extends AnyFunSuite { assert(error.getMessage.contains("input sighash must be SIGHASH_ALL")) } } - - test("compute descriptor checksums") { - val data = Seq( - "pkh([6ded4eb8/44h/0h/0h]xpub6C6N5WVF5zmurBR52MZZj8Jxm6eDiKyM4wFCm7xTYBEsAvJPqBKp2u2K7RTsZaYDN8duBWq4acrD4vrwjaKHTYuntGjL334nVHtLNuaj5Mu/0/*)#5mzpq0w6", - "wpkh([6ded4eb8/84h/0h/0h]xpub6CDeom4xT3Wg7BuyXU2Sd9XerTKttyfxRwJE36mi5HxFYpYdtdwM76Zx8swPnc6zxuArMYJgjNy91fJ13YtGPHgf49YqA8KdXg6D69tzNFh/0/*)#refya6f0", - "sh(wpkh([6ded4eb8/49h/0h/0h]xpub6Cb8jR9kYsfC6kj9CsE18SyudWjW2V3FnBFkT2oqq6n7NWWvJrjhFin3sAYg8X7ApX8iPophBa98mo4nMvSxnqrXvpnwaRopecQz859Ai1s/0/*))#xrhyhtvl", - "tr([6ded4eb8/86h/0h/0h]xpub6CDp1iw76taes3pkqfiJ6PYhwURkaYksJ62CrrdTVr6ow9wR9mKAtUGoZQqb8pRDiq2F8k31tYrrJjVGTRSLYGQ7nYpmewH94ThsAgDxJ4h/0/*)#2nm7drky", - "pkh([6ded4eb8/44h/0h/0h]xpub6C6N5WVF5zmurBR52MZZj8Jxm6eDiKyM4wFCm7xTYBEsAvJPqBKp2u2K7RTsZaYDN8duBWq4acrD4vrwjaKHTYuntGjL334nVHtLNuaj5Mu/1/*)#908qa67z", - "wpkh([6ded4eb8/84h/0h/0h]xpub6CDeom4xT3Wg7BuyXU2Sd9XerTKttyfxRwJE36mi5HxFYpYdtdwM76Zx8swPnc6zxuArMYJgjNy91fJ13YtGPHgf49YqA8KdXg6D69tzNFh/1/*)#jdv9q0eh", - "sh(wpkh([6ded4eb8/49h/0h/0h]xpub6Cb8jR9kYsfC6kj9CsE18SyudWjW2V3FnBFkT2oqq6n7NWWvJrjhFin3sAYg8X7ApX8iPophBa98mo4nMvSxnqrXvpnwaRopecQz859Ai1s/1/*))#nzej05eq", - "tr([6ded4eb8/86h/0h/0h]xpub6CDp1iw76taes3pkqfiJ6PYhwURkaYksJ62CrrdTVr6ow9wR9mKAtUGoZQqb8pRDiq2F8k31tYrrJjVGTRSLYGQ7nYpmewH94ThsAgDxJ4h/1/*)#m87lskxu" - ) - data.foreach(dnc => { - val Array(desc, checksum) = dnc.split('#') - assert(checksum == LocalOnChainKeyManager.descriptorChecksum(desc)) - }) - } }