Add support for bech32m bitcoin wallets #2873
Conversation
sstone
force-pushed
the
use-bech32m-wallet
branch
from
9f9774f to
d603e95
Compare
sstone
force-pushed
the
use-bech32m-wallet
branch
from
d603e95 to
2353b24
Compare
sstone
force-pushed
the
use-bech32m-wallet
branch
2 times, most recently
from
cd2dac0 to
02c9ab2
Compare
sstone
force-pushed
the
use-bech32m-wallet
branch
2 times, most recently
from
6f067ff to
eb4b6c7
Compare
These changes allow eclair to be used with a bitcoin core wallet configured to generate bech32m (p2tr) addresses and change addresses. The wallet still needs to be able to generate bech32 (p2wpkh) addresses in some cases (support for static_remote_key for non anchor channels for example).
sstone
force-pushed
the
use-bech32m-wallet
branch
from
eb4b6c7 to
39e2842
Compare
| @@ -28,6 +28,18 @@ import scala.concurrent.{ExecutionContext, Future} | |||
| * Created by PM on 06/07/2017. | |||
| */ | |||
|
|
|||
| sealed trait AddressType | |||
There was a problem hiding this comment.
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| @@ -784,7 +785,7 @@ class EclairImpl(appKit: Kit) extends Eclair with Logging { | |||
| } | |||
|
|
|||
| override def getOnChainMasterPubKey(account: Long): String = appKit.nodeParams.onChainKeyManager_opt match { | |||
There was a problem hiding this comment.
Shoudn't this now return an object containing the master pubkey for p2wpkh addresses AND the master pubkey for p2tr addresses?
| } yield address | ||
| private def verifyAddress(address: String)(implicit ec: ExecutionContext): Future[String] = { | ||
| for { | ||
| addressInfo <- rpcClient.invoke("getaddressinfo", address) |
There was a problem hiding this comment.
We shouldn't make this call if onChainKeyManager_opt is empty, this makes unnecessary calls to bitcoind for which we ignore the result.
| @@ -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 | |||
There was a problem hiding this comment.
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): PublicKeyIt'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.
| 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) { |
There was a problem hiding this comment.
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)?
These changes allow eclair to be used with a bitcoin core wallet configured to generate
bech32m(p2tr) addresses and change addresses, which is not possible currently because there are a few place wherep2wpkhis "hardcoded" (i.e. we implicitly assume that wallet addresses arep2wpkhaddresses). We also handle the special case where eclair manages bitcoin core's private keys.The wallet still needs to be able to generate
bech32(p2wpkh) addresses in some cases (support forstatic_remote_keyfor non anchor channels for example).