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

signer: create workaround for SignOutputRaw quirk #203

Merged
merged 1 commit into from
Nov 26, 2024
+113 −40
Merged

signer: create workaround for SignOutputRaw quirk #203

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
35 changes: 18 additions & 17 deletions macaroon_recipes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,24 @@ var (
// implemented in lndclient and the value is the original name of the
// RPC method defined in the proto.
renames = map[string]string{
"ChannelBackup": "ExportChannelBackup",
"ChannelBackups": "ExportAllChannelBackups",
"ConfirmedWalletBalance": "WalletBalance",
"Connect": "ConnectPeer",
"DecodePaymentRequest": "DecodePayReq",
"ListTransactions": "GetTransactions",
"PayInvoice": "SendPaymentSync",
"UpdateChanPolicy": "UpdateChannelPolicy",
"NetworkInfo": "GetNetworkInfo",
"SubscribeGraph": "SubscribeChannelGraph",
"InterceptHtlcs": "HtlcInterceptor",
"ImportMissionControl": "XImportMissionControl",
"EstimateFeeRate": "EstimateFee",
"EstimateFeeToP2WSH": "EstimateFee",
"OpenChannelStream": "OpenChannel",
"ListSweepsVerbose": "ListSweeps",
"MinRelayFee": "EstimateFee",
"ChannelBackup": "ExportChannelBackup",
"ChannelBackups": "ExportAllChannelBackups",
"ConfirmedWalletBalance": "WalletBalance",
"Connect": "ConnectPeer",
"DecodePaymentRequest": "DecodePayReq",
"ListTransactions": "GetTransactions",
"PayInvoice": "SendPaymentSync",
"UpdateChanPolicy": "UpdateChannelPolicy",
"NetworkInfo": "GetNetworkInfo",
"SubscribeGraph": "SubscribeChannelGraph",
"InterceptHtlcs": "HtlcInterceptor",
"ImportMissionControl": "XImportMissionControl",
"EstimateFeeRate": "EstimateFee",
"EstimateFeeToP2WSH": "EstimateFee",
"OpenChannelStream": "OpenChannel",
"ListSweepsVerbose": "ListSweeps",
"MinRelayFee": "EstimateFee",
"SignOutputRawKeyLocator": "SignOutputRaw",
}

// ignores is a list of method names on the client implementations that
Expand Down
118 changes: 95 additions & 23 deletions signer_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ type SignerClient interface {
signDescriptors []*SignDescriptor,
prevOutputs []*wire.TxOut) ([][]byte, error)

// SignOutputRawKeyLocator is a copy of the SignOutputRaw that fixes a
// specific issue around how the key locator is populated in the sign
// descriptor. We copy this method instead of fixing the original to
// make sure we don't break any existing applications that have already
// adjusted themselves to use the specific behavior of the original
// SignOutputRaw method.
SignOutputRawKeyLocator(ctx context.Context, tx *wire.MsgTx,
signDescriptors []*SignDescriptor,
prevOutputs []*wire.TxOut) ([][]byte, error)

// ComputeInputScript generates the proper input script for P2WPKH
// output and NP2WPKH outputs. This method only requires that the
// `Output`, `HashType`, `SigHashes` and `InputIndex` fields are
Expand Down Expand Up @@ -215,26 +225,70 @@ func (s *signerClient) RawClientWithMacAuth(
return s.signerMac.WithMacaroonAuth(parentCtx), s.timeout, s.client
}

func marshallSignDescriptors(
signDescriptors []*SignDescriptor) []*signrpc.SignDescriptor {
func marshallSignDescriptors(signDescriptors []*SignDescriptor,
fullDescriptors bool) []*signrpc.SignDescriptor {

rpcSignDescs := make([]*signrpc.SignDescriptor, len(signDescriptors))
for i, signDesc := range signDescriptors {
var keyBytes []byte
var keyLocator *signrpc.KeyLocator
if signDesc.KeyDesc.PubKey != nil {
keyBytes = signDesc.KeyDesc.PubKey.SerializeCompressed()
// partialDescriptor is a helper method that creates a partially
// populated sign descriptor that is backward compatible with the way
// some applications like Loop expect the call to lnd to be made. This
// function only populates _either_ the public key or the key locator in
// the descriptor, but not both.
partialDescriptor := func(
d keychain.KeyDescriptor) *signrpc.KeyDescriptor {

keyDesc := &signrpc.KeyDescriptor{}
if d.PubKey != nil {
keyDesc.RawKeyBytes = d.PubKey.SerializeCompressed()
} else {
keyLocator = &signrpc.KeyLocator{
KeyFamily: int32(
signDesc.KeyDesc.KeyLocator.Family,
),
KeyIndex: int32(
signDesc.KeyDesc.KeyLocator.Index,
),
keyDesc.KeyLoc = &signrpc.KeyLocator{
KeyFamily: int32(d.KeyLocator.Family),
KeyIndex: int32(d.KeyLocator.Index),
}
}

return keyDesc
}

// fullDescriptor is a helper method that creates a fully populated sign
// descriptor that includes both the public key and the key locator (if
// available). For the locator we explicitly check that both the family
// _and_ the index is non-zero. In some applications it's possible that
// the family is always set (because only a specific family is used),
// but the index might be zero because it's the first key, or because it
// isn't known at that particular moment.
// We aim to be compatible with this method in lnd's wallet:
// https://github.com/lightningnetwork/lnd/blob/master/lnwallet/btcwallet/signer.go#L286
// Because we know all custom families (0 to 255) are derived at wallet
// creation, and the very first index of each family/account is always
// derived, we know that only using the public key for that very first
// index will work. But for a freshly initialized wallet (e.g. restored
// from seed), we won't know any indexes greater than 0, so we _need_ to
// also specify the key locator and not just the public key.
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 where I'm confused. If we only specify the public key, then we'll derive all private keys from 0 to 10k for that key family, checking the public key each time.

With the way the logic works, there're 3 main paths:

  1. You specify no public key, it checks the cache: https://github.com/lightningnetwork/lnd/blob/94f7ed46deab346cc19ff1e6aa00cecdfe3eb775/keychain/btcwallet.go#L266-L280
  2. Index is set, or no public key, it tries to derive from scratch: https://github.com/lightningnetwork/lnd/blob/94f7ed46deab346cc19ff1e6aa00cecdfe3eb775/keychain/btcwallet.go#L300-L322
  3. Only pubkey, it scans: https://github.com/lightningnetwork/lnd/blob/94f7ed46deab346cc19ff1e6aa00cecdfe3eb775/keychain/btcwallet.go#L324-L363

When we hit disk it eventually calls into deriveKeyFromPath: https://github.com/btcsuite/btcwallet/blob/66a3aeef6e78751e644a3d03fd856e982e0db4ac/waddrmgr/scoped_manager.go#L742-L769. DIsk wise, this only need the account to exist, as then it'll decrypt the master key and derive the addr as normal: https://github.com/btcsuite/btcwallet/blob/66a3aeef6e78751e644a3d03fd856e982e0db4ac/waddrmgr/scoped_manager.go#L385-L416

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't land in any of the code paths you linked... The problem is that the signrpc.SignOutputRaw RPC uses input.Signer.SignOutputRaw which is implemented by BtcWallet.SignOutputRaw which then calls into BtcWallet.fetchPrivKey which has a completely different logic.

fullDescriptor := func(
Copy link
Member

Choose a reason for hiding this comment

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

nit: instead of these two closures, we could just have one: descriptor(d keychain.KeyDescriptor, partial bool) and check for partial when filling the keydescriptor.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to make this as clear and obvious as possible, with as much comment on why we're doing it that way, so I went with two closures on purpose. Just so nobody else spends debug hours on this (because this is already the second or third time I ran into this, once in Loop and once in Pool).

Copy link
Member

Choose a reason for hiding this comment

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

What is resolved by setting both? From my reading of the code, only one or the other is read.

d keychain.KeyDescriptor) *signrpc.KeyDescriptor {

keyDesc := &signrpc.KeyDescriptor{}
if d.PubKey != nil {
keyDesc.RawKeyBytes = d.PubKey.SerializeCompressed()
}

if d.KeyLocator.Family != 0 && d.KeyLocator.Index != 0 {
keyDesc.KeyLoc = &signrpc.KeyLocator{
KeyFamily: int32(d.KeyLocator.Family),
KeyIndex: int32(d.KeyLocator.Index),
}
}

return keyDesc
}

rpcSignDescs := make([]*signrpc.SignDescriptor, len(signDescriptors))
for i, signDesc := range signDescriptors {
keyDesc := partialDescriptor(signDesc.KeyDesc)
if fullDescriptors {
keyDesc = fullDescriptor(signDesc.KeyDesc)
}

var doubleTweak []byte
if signDesc.DoubleTweak != nil {
doubleTweak = signDesc.DoubleTweak.Serialize()
Expand All @@ -247,12 +301,9 @@ func marshallSignDescriptors(
PkScript: signDesc.Output.PkScript,
Value: signDesc.Output.Value,
},
Sighash: uint32(signDesc.HashType),
InputIndex: int32(signDesc.InputIndex),
KeyDesc: &signrpc.KeyDescriptor{
RawKeyBytes: keyBytes,
KeyLoc: keyLocator,
},
Sighash: uint32(signDesc.HashType),
InputIndex: int32(signDesc.InputIndex),
KeyDesc: keyDesc,
SingleTweak: signDesc.SingleTweak,
DoubleTweak: doubleTweak,
TapTweak: signDesc.TapTweak,
Expand Down Expand Up @@ -283,11 +334,32 @@ func (s *signerClient) SignOutputRaw(ctx context.Context, tx *wire.MsgTx,
signDescriptors []*SignDescriptor, prevOutputs []*wire.TxOut) ([][]byte,
error) {

return s.signOutputRaw(ctx, tx, signDescriptors, prevOutputs, false)
}

// SignOutputRawKeyLocator is a copy of the SignOutputRaw that fixes a specific
// issue around how the key locator is populated in the sign descriptor. We copy
// this method instead of fixing the original to make sure we don't break any
// existing applications that have already adjusted themselves to use the
// specific behavior of the original SignOutputRaw method.
func (s *signerClient) SignOutputRawKeyLocator(ctx context.Context,
tx *wire.MsgTx, signDescriptors []*SignDescriptor,
prevOutputs []*wire.TxOut) ([][]byte, error) {

return s.signOutputRaw(ctx, tx, signDescriptors, prevOutputs, true)
}

// signOutputRaw is a helper method that performs the actual signing of the
// transaction.
func (s *signerClient) signOutputRaw(ctx context.Context, tx *wire.MsgTx,
signDescriptors []*SignDescriptor, prevOutputs []*wire.TxOut,
fullDescriptor bool) ([][]byte, error) {

txRaw, err := encodeTx(tx)
if err != nil {
return nil, err
}
rpcSignDescs := marshallSignDescriptors(signDescriptors)
rpcSignDescs := marshallSignDescriptors(signDescriptors, fullDescriptor)
rpcPrevOutputs := marshallTxOut(prevOutputs)

rpcCtx, cancel := context.WithTimeout(ctx, s.timeout)
Expand Down Expand Up @@ -321,7 +393,7 @@ func (s *signerClient) ComputeInputScript(ctx context.Context, tx *wire.MsgTx,
if err != nil {
return nil, err
}
rpcSignDescs := marshallSignDescriptors(signDescriptors)
rpcSignDescs := marshallSignDescriptors(signDescriptors, false)
rpcPrevOutputs := marshallTxOut(prevOutputs)

rpcCtx, cancel := context.WithTimeout(ctx, s.timeout)
Expand Down
Loading