Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for bech32m bitcoin wallets #2873

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add support for bech32m bitcoin wallets #2873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import fr.acinq.bitcoin.scalacompat.{BlockHash, ByteVector32, ByteVector64, Cryp
import fr.acinq.eclair.ApiTypes.ChannelNotFound
import fr.acinq.eclair.balance.CheckBalance.GlobalBalance
import fr.acinq.eclair.balance.{BalanceActor, ChannelsListener}
import fr.acinq.eclair.blockchain.AddressType
import fr.acinq.eclair.blockchain.OnChainWallet.OnChainBalance
import fr.acinq.eclair.blockchain.bitcoind.rpc.BitcoinCoreClient
import fr.acinq.eclair.blockchain.bitcoind.rpc.BitcoinCoreClient.{Descriptors, WalletTx}
Expand Down Expand Up @@ -784,7 +785,7 @@ class EclairImpl(appKit: Kit) extends Eclair with Logging {
}

override def getOnChainMasterPubKey(account: Long): String = appKit.nodeParams.onChainKeyManager_opt match {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shoudn't this now return an object containing the master pubkey for p2wpkh addresses AND the master pubkey for p2tr addresses?

case Some(keyManager) => keyManager.masterPubKey(account)
case Some(keyManager) => keyManager.masterPubKey(account, AddressType.Bech32)
case _ => throw new RuntimeException("on-chain seed is not configured")
}

Expand Down
16 changes: 14 additions & 2 deletions eclair-core/src/main/scala/fr/acinq/eclair/Setup.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ class Setup(val datadir: File,
_ <- feeratesRetrieved.future

finalPubkey = new AtomicReference[PublicKey](null)
finalPubkeyScript = new AtomicReference[ByteVector](null)
pubkeyRefreshDelay = FiniteDuration(config.getDuration("bitcoind.final-pubkey-refresh-delay").getSeconds, TimeUnit.SECONDS)
// there are 3 possibilities regarding onchain key management:
// 1) there is no `eclair-signer.conf` file in Eclair's data directory, Eclair will not manage Bitcoin core keys, and Eclair's API will not return bitcoin core descriptors. This is the default mode.
Expand All @@ -275,18 +276,29 @@ class Setup(val datadir: File,
// 3) there is an `eclair-signer.conf` file in Eclair's data directory, and the name of the wallet set in `eclair-signer.conf` matches the `eclair.bitcoind.wallet` setting in `eclair.conf`.
// Eclair will assume that this is a watch-only bitcoin wallet that has been created from descriptors generated by Eclair, and will manage its private keys, and here we pass the onchain key manager to our bitcoin client.
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")

val refresher: typed.ActorRef[OnchainPubkeyRefresher.Command] = system.spawn(Behaviors.supervise(OnchainPubkeyRefresher(bitcoinChainHash, this, finalPubkey, finalPubkeyScript, pubkeyRefreshDelay)).onFailure(typed.SupervisorStrategy.restart), name = "onchain-address-manager")

override def getP2wpkhPubkey(renew: Boolean): PublicKey = {
val key = finalPubkey.get()
if (renew) refresher ! OnchainPubkeyRefresher.Renew
if (renew) refresher ! OnchainPubkeyRefresher.RenewPubkey
key
}

override def getPubkeyScript(renew: Boolean): ByteVector = {
val script = finalPubkeyScript.get()
if (renew) refresher ! OnchainPubkeyRefresher.RenewPubkeyScript
script
}
}
_ = if (bitcoinClient.useEclairSigner) logger.info("using eclair to sign bitcoin core transactions")
initialPubkey <- bitcoinClient.getP2wpkhPubkey()
_ = finalPubkey.set(initialPubkey)

initialAddress <- bitcoinClient.getReceiveAddress()
Right(initialPubkeyScript) = addressToPublicKeyScript(bitcoinChainHash, initialAddress)
_ = finalPubkeyScript.set(Script.write(initialPubkeyScript))

// If we started funding a transaction and restarted before signing it, we may have utxos that stay locked forever.
// We want to do something about it: we can unlock them automatically, or let the node operator decide what to do.
//
Expand Down
18 changes: 16 additions & 2 deletions eclair-core/src/main/scala/fr/acinq/eclair/blockchain/OnChainWallet.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ package fr.acinq.eclair.blockchain

import fr.acinq.bitcoin.psbt.Psbt
import fr.acinq.bitcoin.scalacompat.Crypto.PublicKey
import fr.acinq.bitcoin.scalacompat.{OutPoint, Satoshi, Transaction, TxId}
import fr.acinq.bitcoin.scalacompat.{BlockHash, OutPoint, Satoshi, Script, Transaction, TxId, addressToPublicKeyScript}
import fr.acinq.eclair.blockchain.fee.FeeratePerKw
import scodec.bits.ByteVector

Expand All @@ -28,6 +28,18 @@ import scala.concurrent.{ExecutionContext, Future}
* Created by PM on 06/07/2017.
*/

sealed trait AddressType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is confusing, this isn't really what we're interested in (bech32 vs bech32m is just the encoding, not really the type of address). What we want to do here is distinguish between segwit v0 p2wpkh addresses and segwit v1 p2tr (with the standard BIP86 NoScriptTweak), right? Those are the only type of on-chain addresses we will generate. So I think we should more explicitly do:

// @formatter:off
sealed trait AddressType
object AddressType {
  /** A segwit v0 p2wpkh address. */
  case object P2WPKH extends AddressType { override def toString: String = "p2wpkh" }
  /** A segwit v1 p2tr address that doesn't have any script path, as defined in BIP86. */
  case object P2TR extends AddressType { override def toString: String = "p2tr" }
}
// @formatter:on


object AddressType {
case object Bech32 extends AddressType {
override def toString: String = "bech32"
}

case object Bech32m extends AddressType {
override def toString: String = "bech32m"
}
}

/** This trait lets users fund lightning channels. */
trait OnChainChannelFunder {

Expand Down Expand Up @@ -119,7 +131,7 @@ trait OnChainAddressGenerator {
/**
* @param label used if implemented with bitcoin core, can be ignored by implementation
*/
def getReceiveAddress(label: String = "")(implicit ec: ExecutionContext): Future[String]
def getReceiveAddress(label: String = "", addressType_opt: Option[AddressType] = None)(implicit ec: ExecutionContext): Future[String]

/** Generate a p2wpkh wallet address and return the corresponding public key. */
def getP2wpkhPubkey()(implicit ec: ExecutionContext): Future[PublicKey]
Expand All @@ -132,6 +144,8 @@ trait OnchainPubkeyCache {
* @param renew applies after requesting the current pubkey, and is asynchronous
*/
def getP2wpkhPubkey(renew: Boolean = true): PublicKey

def getPubkeyScript(renew: Boolean = true): ByteVector
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is way too vague and confusing since in bitcoin terms a "scriptPubKey" can refer to a lot of things...and this trait is supposed to manage public keys directly. I think we should instead have:

def getP2wpkhPubkey(renew: Boolean = true): PublicKey
def getP2trPubkey(renew: Boolean = true): PublicKey

It's not the responsibility of this OnchainPubkeyCache to convert it to an actual script, this can be done from the public key afterwards. This should simplify the OnchainPubkeyRefresher because it should still simply refresh a pubkey (either segwit v0 or segwit v1), and can then also return the corresponding script if needed.

}

/** This trait lets users check the wallet's on-chain balance. */
Expand Down
52 changes: 40 additions & 12 deletions eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/OnchainPubkeyRefresher.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,61 +3,89 @@ package fr.acinq.eclair.blockchain.bitcoind

import akka.actor.typed.Behavior
import akka.actor.typed.scaladsl.{ActorContext, Behaviors, TimerScheduler}
import fr.acinq.bitcoin.scalacompat.{BlockHash, Script, addressToPublicKeyScript}
import fr.acinq.bitcoin.scalacompat.Crypto.PublicKey
import fr.acinq.eclair.blockchain.OnChainAddressGenerator
import scodec.bits.ByteVector

import java.util.concurrent.atomic.AtomicReference
import scala.concurrent.ExecutionContext.Implicits.global
import scala.concurrent.duration.FiniteDuration
import scala.util.{Failure, Success}

/**
* Handles the renewal of public keys generated by bitcoin core and used to send onchain funds to when channels get closed
* Handles the renewal of public keys and public key scripts generated by bitcoin core and used to send onchain funds to when channels get closed
*/
object OnchainPubkeyRefresher {

// @formatter:off
sealed trait Command
case object Renew extends Command
private case class Set(pubkey: PublicKey) extends Command
case object RenewPubkey extends Command
private case class SetPubkey(pubkey: PublicKey) extends Command
case object RenewPubkeyScript extends Command
private case class SetPubkeyScript(pubkeyScript: ByteVector) extends Command
private case class Error(reason: Throwable) extends Command
private case object Done extends Command
// @formatter:on

def apply(generator: OnChainAddressGenerator, finalPubkey: AtomicReference[PublicKey], delay: FiniteDuration): Behavior[Command] = {
def apply(chainHash: BlockHash, generator: OnChainAddressGenerator, finalPubkey: AtomicReference[PublicKey], finalPubkeyScript: AtomicReference[ByteVector], delay: FiniteDuration): Behavior[Command] = {
Behaviors.setup { context =>
Behaviors.withTimers { timers =>
new OnchainPubkeyRefresher(generator, finalPubkey, context, timers, delay).idle()
new OnchainPubkeyRefresher(chainHash, generator, finalPubkey, finalPubkeyScript, context, timers, delay).idle()
}
}
}
}

private class OnchainPubkeyRefresher(generator: OnChainAddressGenerator, finalPubkey: AtomicReference[PublicKey], context: ActorContext[OnchainPubkeyRefresher.Command], timers: TimerScheduler[OnchainPubkeyRefresher.Command], delay: FiniteDuration) {
private class OnchainPubkeyRefresher(chainHash: BlockHash, generator: OnChainAddressGenerator, finalPubkey: AtomicReference[PublicKey], finalPubkeyScript: AtomicReference[ByteVector], context: ActorContext[OnchainPubkeyRefresher.Command], timers: TimerScheduler[OnchainPubkeyRefresher.Command], delay: FiniteDuration) {

import OnchainPubkeyRefresher._

def idle(): Behavior[Command] = Behaviors.receiveMessagePartial {
case Renew =>
context.log.debug(s"received Renew current script is ${finalPubkey.get()}")
case RenewPubkey =>
context.log.debug(s"received RenewPubkey current pubkey is ${finalPubkey.get()}")
context.pipeToSelf(generator.getP2wpkhPubkey()) {
case Success(pubkey) => Set(pubkey)
case Success(pubkey) => SetPubkey(pubkey)
case Failure(reason) => Error(reason)
}
Behaviors.receiveMessagePartial {
case Set(script) =>
case SetPubkey(script) =>
timers.startSingleTimer(Done, delay) // wait a bit to avoid generating too many addresses in case of mass channel force-close
waiting(script)
case Error(reason) =>
context.log.error("cannot generate new onchain address", reason)
Behaviors.same
}
case RenewPubkeyScript =>
context.log.debug(s"received Renew current script is ${finalPubkeyScript.get()}")
context.pipeToSelf(generator.getReceiveAddress("")) {
case Success(address) => addressToPublicKeyScript(chainHash, address) match {
case Right(script) => SetPubkeyScript(Script.write(script))
case Left(error) => Error(error.getCause)
}
case Failure(reason) => Error(reason)
}
Behaviors.receiveMessagePartial {
case SetPubkeyScript(script) =>
timers.startSingleTimer(Done, delay) // wait a bit to avoid generating too many addresses in case of mass channel force-close
waiting(script)
case Error(reason) =>
context.log.error("cannot generate new onchain address", reason)
Behaviors.same
}
}

def waiting(pubkey: PublicKey): Behavior[Command] = Behaviors.receiveMessagePartial {
case Done =>
context.log.info(s"setting final onchain public key to $pubkey")
finalPubkey.set(pubkey)
idle()
}

def waiting(script: PublicKey): Behavior[Command] = Behaviors.receiveMessagePartial {
def waiting(script: ByteVector): Behavior[Command] = Behaviors.receiveMessagePartial {
case Done =>
context.log.info(s"setting final onchain script to $script")
finalPubkey.set(script)
finalPubkeyScript.set(script)
idle()
}
}
30 changes: 24 additions & 6 deletions eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,11 @@ import fr.acinq.bitcoin.scalacompat.Crypto.PublicKey
import fr.acinq.bitcoin.scalacompat._
import fr.acinq.bitcoin.{Bech32, Block, SigHash}
import fr.acinq.eclair.ShortChannelId.coordinates
import fr.acinq.eclair.blockchain.OnChainWallet
import fr.acinq.eclair.blockchain.OnChainWallet.{FundTransactionResponse, MakeFundingTxResponse, OnChainBalance, ProcessPsbtResponse}
import fr.acinq.eclair.blockchain.bitcoind.ZmqWatcher.{GetTxWithMetaResponse, UtxoStatus, ValidateResult}
import fr.acinq.eclair.blockchain.fee.{FeeratePerKB, FeeratePerKw}
import fr.acinq.eclair.blockchain.{AddressType, OnChainWallet}
import fr.acinq.eclair.crypto.keymanager.OnChainKeyManager
import fr.acinq.eclair.json.SatoshiSerializer
import fr.acinq.eclair.transactions.Transactions
import fr.acinq.eclair.wire.protocol.ChannelAnnouncement
import fr.acinq.eclair.{BlockHeight, TimestampSecond, TxCoordinates}
Expand Down Expand Up @@ -550,10 +549,29 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onChainKeyManag
}
}

def getReceiveAddress(label: String)(implicit ec: ExecutionContext): Future[String] = for {
JString(address) <- rpcClient.invoke("getnewaddress", label)
_ <- extractPublicKey(address)
} yield address
private def verifyAddress(address: String)(implicit ec: ExecutionContext): Future[String] = {
for {
addressInfo <- rpcClient.invoke("getaddressinfo", address)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't make this call if onChainKeyManager_opt is empty, this makes unnecessary calls to bitcoind for which we ignore the result.

JString(keyPath) = addressInfo \ "hdkeypath"
} yield {
// check that when we manage private keys we can re-compute the address we got from bitcoin core
onChainKeyManager_opt match {
case Some(keyManager) =>
val (_, computed) = keyManager.derivePublicKey(DeterministicWallet.KeyPath(keyPath))
if (computed != address) return Future.failed(new RuntimeException("cannot recompute address generated by bitcoin core"))
case None => ()
}
address
}
}

def getReceiveAddress(label: String, addressType_opt: Option[AddressType] = None)(implicit ec: ExecutionContext): Future[String] = {
val params = List(label) ++ addressType_opt.map(_.toString).toList
for {
JString(address) <- rpcClient.invoke("getnewaddress", params: _*)
_ <- verifyAddress(address)
} yield address
}

def getP2wpkhPubkey()(implicit ec: ExecutionContext): Future[Crypto.PublicKey] = for {
JString(address) <- rpcClient.invoke("getnewaddress", "", "bech32")
Expand Down
18 changes: 11 additions & 7 deletions eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/CommonHandlers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@

package fr.acinq.eclair.channel.fsm

import akka.actor.{ActorRef, FSM, Status}
import akka.actor.FSM
import fr.acinq.bitcoin.scalacompat.{ByteVector32, Script}
import fr.acinq.eclair.Features
import fr.acinq.eclair.channel._
import fr.acinq.eclair.db.PendingCommandsDb
import fr.acinq.eclair.io.Peer
import fr.acinq.eclair.wire.protocol.{HtlcSettlementMessage, LightningMessage, UpdateMessage}
import fr.acinq.eclair.{Features, InitFeature}
import scodec.bits.ByteVector

import scala.concurrent.duration.DurationInt
Expand Down Expand Up @@ -115,17 +115,21 @@ trait CommonHandlers {
upfrontShutdownScript
} else {
log.info("ignoring pre-generated shutdown script, because option_upfront_shutdown_script is disabled")
generateFinalScriptPubKey()
generateFinalScriptPubKey(data.commitments.params.localParams.initFeatures, data.commitments.params.remoteParams.initFeatures)
}
case None =>
// normal case: we don't pre-generate shutdown scripts
generateFinalScriptPubKey()
generateFinalScriptPubKey(data.commitments.params.localParams.initFeatures, data.commitments.params.remoteParams.initFeatures)
}
}

private def generateFinalScriptPubKey(): ByteVector = {
val finalPubKey = wallet.getP2wpkhPubkey()
val finalScriptPubKey = Script.write(Script.pay2wpkh(finalPubKey))
private def generateFinalScriptPubKey(localFeatures: Features[InitFeature], remoteFeatures: Features[InitFeature]): ByteVector = {
val allowAnySegwit = Features.canUseFeature(localFeatures, remoteFeatures, Features.ShutdownAnySegwit)
val finalScriptPubKey = if (allowAnySegwit) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's confusing to only gate it on allowAnySegwit: it looks like this will use p2tr, but actually not, it's instead using the default address type configured on the bitcoin node. This is mixing configuration options from eclair and from bitcoind...shouldn't we instead always use p2tr when the channel is a taproot channel and always use p2wpkh otherwise? Or instead always use p2tr when allowAnySegwit is set (which allows deprecating support for p2wpkh at some point in the future)?

wallet.getPubkeyScript()
} else {
Script.write(Script.pay2wpkh(wallet.getP2wpkhPubkey()))
}
log.info(s"using finalScriptPubkey=$finalScriptPubKey")
finalScriptPubKey
}
Expand Down
Loading
Loading