From 3d353a9879481606fce9a85716499f9163ad230b Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 5 Jan 2022 11:04:26 +0100 Subject: [PATCH] config_builder+rpcwallet: simplify RPC signing With the remote signing instance now not needing to know anything about addresses or current derivation indices, we don't need to forward any such calls to that instance and can simplify the RPCKeyRing considerably. --- config_builder.go | 2 +- lntest/itest/lnd_remote_signer_test.go | 4 - lnwallet/rpcwallet/rpcwallet.go | 734 ++++++++++++------------- 3 files changed, 351 insertions(+), 389 deletions(-) diff --git a/config_builder.go b/config_builder.go index f1a3f1c40..b32077aaa 100644 --- a/config_builder.go +++ b/config_builder.go @@ -687,7 +687,7 @@ func (d *RPCSignerWalletImpl) BuildChainControl( rpcKeyRing, err := rpcwallet.NewRPCKeyRing( baseKeyRing, walletController, - d.DefaultWalletImpl.cfg.RemoteSigner, + d.DefaultWalletImpl.cfg.RemoteSigner, walletConfig.CoinType, rpcwallet.DefaultRPCTimeout, ) if err != nil { diff --git a/lntest/itest/lnd_remote_signer_test.go b/lntest/itest/lnd_remote_signer_test.go index 16c677949..3ec897547 100644 --- a/lntest/itest/lnd_remote_signer_test.go +++ b/lntest/itest/lnd_remote_signer_test.go @@ -238,10 +238,6 @@ func testRemoteSigner(net *lntest.NetworkHarness, t *harnessTest) { if subTest.sendCoins { net.SendCoins(t.t, btcutil.SatoshiPerBitcoin, watchOnly) - assertAccountBalance( - t.t, signer, "default", - btcutil.SatoshiPerBitcoin, 0, - ) assertAccountBalance( t.t, watchOnly, "default", btcutil.SatoshiPerBitcoin, 0, diff --git a/lnwallet/rpcwallet/rpcwallet.go b/lnwallet/rpcwallet/rpcwallet.go index b31881d15..4d39613db 100644 --- a/lnwallet/rpcwallet/rpcwallet.go +++ b/lnwallet/rpcwallet/rpcwallet.go @@ -4,7 +4,6 @@ import ( "bytes" "context" "crypto/x509" - "encoding/binary" "errors" "fmt" "io/ioutil" @@ -16,16 +15,15 @@ import ( "github.com/btcsuite/btcutil" "github.com/btcsuite/btcutil/hdkeychain" "github.com/btcsuite/btcutil/psbt" - "github.com/btcsuite/btcwallet/waddrmgr" - btcwallet "github.com/btcsuite/btcwallet/wallet" + basewallet "github.com/btcsuite/btcwallet/wallet" "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/keychain" "github.com/lightningnetwork/lnd/lncfg" "github.com/lightningnetwork/lnd/lnrpc/signrpc" "github.com/lightningnetwork/lnd/lnrpc/walletrpc" "github.com/lightningnetwork/lnd/lnwallet" + "github.com/lightningnetwork/lnd/lnwallet/btcwallet" "github.com/lightningnetwork/lnd/lnwallet/chainfee" - "github.com/lightningnetwork/lnd/lnwallet/chanfunding" "github.com/lightningnetwork/lnd/lnwire" "github.com/lightningnetwork/lnd/macaroons" "google.golang.org/grpc" @@ -58,6 +56,8 @@ type RPCKeyRing struct { watchOnlyKeyRing keychain.SecretKeyRing + coinType uint32 + rpcTimeout time.Duration signerClient signrpc.SignerClient @@ -74,7 +74,7 @@ var _ lnwallet.WalletController = (*RPCKeyRing)(nil) // delegates any signing or ECDH operations to the remove signer through RPC. func NewRPCKeyRing(watchOnlyKeyRing keychain.SecretKeyRing, watchOnlyWalletController lnwallet.WalletController, - remoteSigner *lncfg.RemoteSigner, + remoteSigner *lncfg.RemoteSigner, coinType uint32, rpcTimeout time.Duration) (*RPCKeyRing, error) { rpcConn, err := connectRPC( @@ -89,6 +89,7 @@ func NewRPCKeyRing(watchOnlyKeyRing keychain.SecretKeyRing, return &RPCKeyRing{ WalletController: watchOnlyWalletController, watchOnlyKeyRing: watchOnlyKeyRing, + coinType: coinType, rpcTimeout: rpcTimeout, signerClient: signrpc.NewSignerClient(rpcConn), walletClient: walletrpc.NewWalletKitClient(rpcConn), @@ -105,189 +106,7 @@ func NewRPCKeyRing(watchOnlyKeyRing keychain.SecretKeyRing, func (r *RPCKeyRing) NewAddress(addrType lnwallet.AddressType, change bool, account string) (btcutil.Address, error) { - ctxt, cancel := context.WithTimeout(context.Background(), r.rpcTimeout) - defer cancel() - - rpcAddrType := walletrpc.AddressType_WITNESS_PUBKEY_HASH - if addrType == lnwallet.NestedWitnessPubKey { - rpcAddrType = walletrpc.AddressType_NESTED_WITNESS_PUBKEY_HASH - } - - remoteAddr, err := r.walletClient.NextAddr(ctxt, &walletrpc.AddrRequest{ - Account: account, - Type: rpcAddrType, - Change: change, - }) - if err != nil { - return nil, fmt.Errorf("error deriving address on remote "+ - "signer instance: %v", err) - } - - localAddr, err := r.WalletController.NewAddress( - addrType, change, account, - ) - if err != nil { - return nil, fmt.Errorf("error deriving address on local "+ - "wallet instance: %v", err) - } - - // We need to make sure we've derived the same address on the remote - // signing machine, otherwise we don't know whether we're at the same - // address index (and therefore the same wallet state in general). - if localAddr.String() != remoteAddr.Addr { - return nil, fmt.Errorf("error deriving address on remote "+ - "signing instance, got different address (%s) than "+ - "on local wallet instance (%s)", remoteAddr.Addr, - localAddr.String()) - } - - return localAddr, nil -} - -// LastUnusedAddress returns the last *unused* address known by the wallet. An -// address is unused if it hasn't received any payments. This can be useful in -// UIs in order to continually show the "freshest" address without having to -// worry about "address inflation" caused by continual refreshing. Similar to -// NewAddress it can derive a specified address type, and also optionally a -// change address. The account parameter must be non-empty as it determines -// which account the address should be generated from. -func (r *RPCKeyRing) LastUnusedAddress(lnwallet.AddressType, - string) (btcutil.Address, error) { - - // Because the underlying wallet will create a new address if the last - // derived address has been used in the meantime, we would need to proxy - // that call as well. But since that's deep within the btcwallet code, - // we cannot easily proxy it without more refactoring. Since this is an - // address type that is probably not widely used we can probably get - // away with not supporting it. - return nil, fmt.Errorf("unused address types are not supported when " + - "remote signing is enabled") -} - -// ImportAccount imports an account backed by an account extended public key. -// The master key fingerprint denotes the fingerprint of the root key -// corresponding to the account public key (also known as the key with -// derivation path m/). This may be required by some hardware wallets for proper -// identification and signing. -// -// The address type can usually be inferred from the key's version, but may be -// required for certain keys to map them into the proper scope. -// -// For BIP-0044 keys, an address type must be specified as we intend to not -// support importing BIP-0044 keys into the wallet using the legacy -// pay-to-pubkey-hash (P2PKH) scheme. A nested witness address type will force -// the standard BIP-0049 derivation scheme, while a witness address type will -// force the standard BIP-0084 derivation scheme. -// -// For BIP-0049 keys, an address type must also be specified to make a -// distinction between the standard BIP-0049 address schema (nested witness -// pubkeys everywhere) and our own BIP-0049Plus address schema (nested pubkeys -// externally, witness pubkeys internally). -func (r *RPCKeyRing) ImportAccount(name string, - accountPubKey *hdkeychain.ExtendedKey, masterKeyFingerprint uint32, - addrType *waddrmgr.AddressType, - dryRun bool) (*waddrmgr.AccountProperties, []btcutil.Address, - []btcutil.Address, error) { - - ctxt, cancel := context.WithTimeout(context.Background(), r.rpcTimeout) - defer cancel() - - var masterKeyFingerprintBytes [4]byte - binary.BigEndian.PutUint32( - masterKeyFingerprintBytes[:], masterKeyFingerprint, - ) - - rpcAddrType, err := toRPCAddrType(addrType) - if err != nil { - return nil, nil, nil, fmt.Errorf("error converting address "+ - "type: %v", err) - } - - remoteAcct, err := r.walletClient.ImportAccount( - ctxt, &walletrpc.ImportAccountRequest{ - Name: name, - ExtendedPublicKey: accountPubKey.String(), - MasterKeyFingerprint: masterKeyFingerprintBytes[:], - AddressType: rpcAddrType, - DryRun: dryRun, - }, - ) - if err != nil { - return nil, nil, nil, fmt.Errorf("error importing account on "+ - "remote signer instance: %v", err) - } - - props, extAddrs, intAddrs, err := r.WalletController.ImportAccount( - name, accountPubKey, masterKeyFingerprint, addrType, dryRun, - ) - if err != nil { - return nil, nil, nil, fmt.Errorf("error importing account on "+ - "local wallet instance: %v", err) - } - - mismatchErr := fmt.Errorf("error importing account on remote signing "+ - "instance, got different external addresses (%v) than on "+ - "local wallet instance (%s)", remoteAcct.DryRunExternalAddrs, - extAddrs) - if len(remoteAcct.DryRunExternalAddrs) != len(extAddrs) { - return nil, nil, nil, mismatchErr - } - for idx, remoteExtAddr := range remoteAcct.DryRunExternalAddrs { - if extAddrs[idx].String() != remoteExtAddr { - return nil, nil, nil, mismatchErr - } - } - - mismatchErr = fmt.Errorf("error importing account on remote signing "+ - "instance, got different internal addresses (%v) than on "+ - "local wallet instance (%s)", remoteAcct.DryRunInternalAddrs, - intAddrs) - if len(remoteAcct.DryRunInternalAddrs) != len(intAddrs) { - return nil, nil, nil, mismatchErr - } - for idx, remoteIntAddr := range remoteAcct.DryRunInternalAddrs { - if intAddrs[idx].String() != remoteIntAddr { - return nil, nil, nil, mismatchErr - } - } - - return props, extAddrs, intAddrs, nil -} - -// ImportPublicKey imports a single derived public key into the wallet. The -// address type can usually be inferred from the key's version, but in the case -// of legacy versions (xpub, tpub), an address type must be specified as we -// intend to not support importing BIP-44 keys into the wallet using the legacy -// pay-to-pubkey-hash (P2PKH) scheme. -func (r *RPCKeyRing) ImportPublicKey(pubKey *btcec.PublicKey, - addrType waddrmgr.AddressType) error { - - ctxt, cancel := context.WithTimeout(context.Background(), r.rpcTimeout) - defer cancel() - - rpcAddrType, err := toRPCAddrType(&addrType) - if err != nil { - return fmt.Errorf("error converting address type: %v", err) - } - - _, err = r.walletClient.ImportPublicKey( - ctxt, &walletrpc.ImportPublicKeyRequest{ - PublicKey: pubKey.SerializeCompressed(), - AddressType: rpcAddrType, - }, - ) - if err != nil { - return fmt.Errorf("error importing pubkey on remote signer "+ - "instance: %v", err) - } - - err = r.WalletController.ImportPublicKey(pubKey, addrType) - if err != nil { - return fmt.Errorf("error importing pubkey on local signer "+ - "instance: %v", err) - } - - return nil + return r.WalletController.NewAddress(addrType, change, account) } // SendOutputs funds, signs, and broadcasts a Bitcoin transaction paying out to @@ -296,7 +115,9 @@ func (r *RPCKeyRing) ImportPublicKey(pubKey *btcec.PublicKey, // // NOTE: This method requires the global coin selection lock to be held. // -// This is a part of the WalletController interface. +// NOTE: This is a part of the WalletController interface. +// +// NOTE: This method only signs with BIP49/84 keys. func (r *RPCKeyRing) SendOutputs(outputs []*wire.TxOut, feeRate chainfee.SatPerKWeight, minConfs int32, label string) (*wire.MsgTx, error) { @@ -304,7 +125,7 @@ func (r *RPCKeyRing) SendOutputs(outputs []*wire.TxOut, tx, err := r.WalletController.SendOutputs( outputs, feeRate, minConfs, label, ) - if err != nil && err != btcwallet.ErrTxUnsigned { + if err != nil && err != basewallet.ErrTxUnsigned { return nil, err } if err == nil { @@ -324,7 +145,9 @@ func (r *RPCKeyRing) SendOutputs(outputs []*wire.TxOut, // watch-only wallet if it can map this outpoint into a coin we // own. If not, then we can't continue because our wallet state // is out of sync. - info, err := r.coinFromOutPoint(txIn.PreviousOutPoint) + info, err := r.WalletController.FetchInputInfo( + &txIn.PreviousOutPoint, + ) if err != nil { return nil, fmt.Errorf("error looking up utxo: %v", err) } @@ -332,7 +155,7 @@ func (r *RPCKeyRing) SendOutputs(outputs []*wire.TxOut, // Now that we know the input is ours, we'll populate the // signDesc with the per input unique information. signDesc.Output = &wire.TxOut{ - Value: info.Value, + Value: int64(info.Value), PkScript: info.PkScript, } signDesc.InputIndex = i @@ -351,19 +174,17 @@ func (r *RPCKeyRing) SendOutputs(outputs []*wire.TxOut, return tx, r.WalletController.PublishTransaction(tx, label) } -// FinalizePsbt expects a partial transaction with all inputs and outputs fully -// declared and tries to sign all inputs that belong to the specified account. -// Lnd must be the last signer of the transaction. That means, if there are any -// unsigned non-witness inputs or inputs without UTXO information attached or -// inputs without witness data that do not belong to lnd's wallet, this method -// will fail. If no error is returned, the PSBT is ready to be extracted and the -// final TX within to be broadcast. +// SignPsbt expects a partial transaction with all inputs and outputs fully +// declared and tries to sign all unsigned inputs that have all required fields +// (UTXO information, BIP32 derivation information, witness or sig scripts) set. +// If no error is returned, the PSBT is ready to be given to the next signer or +// to be finalized if lnd was the last signer. // -// NOTE: This method does NOT publish the transaction after it's been -// finalized successfully. -// -// This is a part of the WalletController interface. -func (r *RPCKeyRing) FinalizePsbt(packet *psbt.Packet, accountName string) error { +// NOTE: This RPC only signs inputs (and only those it can sign), it does not +// perform any other tasks (such as coin selection, UTXO locking or +// input/output/fee value validation, PSBT finalization). Any input that is +// incomplete will be skipped. +func (r *RPCKeyRing) SignPsbt(packet *psbt.Packet) error { ctxt, cancel := context.WithTimeout(context.Background(), r.rpcTimeout) defer cancel() @@ -372,14 +193,11 @@ func (r *RPCKeyRing) FinalizePsbt(packet *psbt.Packet, accountName string) error return fmt.Errorf("error serializing PSBT: %v", err) } - resp, err := r.walletClient.FinalizePsbt( - ctxt, &walletrpc.FinalizePsbtRequest{ - FundedPsbt: buf.Bytes(), - Account: accountName, - }, - ) + resp, err := r.walletClient.SignPsbt(ctxt, &walletrpc.SignPsbtRequest{ + FundedPsbt: buf.Bytes(), + }) if err != nil { - return fmt.Errorf("error finalizing PSBT in remote signer "+ + return fmt.Errorf("error signing PSBT in remote signer "+ "instance: %v", err) } @@ -401,6 +219,137 @@ func (r *RPCKeyRing) FinalizePsbt(packet *psbt.Packet, accountName string) error return nil } +// FinalizePsbt expects a partial transaction with all inputs and outputs fully +// declared and tries to sign all inputs that belong to the specified account. +// Lnd must be the last signer of the transaction. That means, if there are any +// unsigned non-witness inputs or inputs without UTXO information attached or +// inputs without witness data that do not belong to lnd's wallet, this method +// will fail. If no error is returned, the PSBT is ready to be extracted and the +// final TX within to be broadcast. +// +// NOTE: This method does NOT publish the transaction after it's been +// finalized successfully. +// +// NOTE: This is a part of the WalletController interface. +// +// NOTE: We need to overwrite this method because we need to redirect the call +// to ComputeInputScript to the RPC key ring's implementation. If we forward +// the call to the default WalletController implementation, we get an error +// since that wallet is watch-only. If we forward the call to the remote signer, +// we get an error because the signer doesn't know the UTXO information required +// in ComputeInputScript. +// +// TODO(guggero): Refactor btcwallet to accept ComputeInputScript as a function +// parameter in FinalizePsbt so we can get rid of this code duplication. +func (r *RPCKeyRing) FinalizePsbt(packet *psbt.Packet, _ string) error { + // Let's check that this is actually something we can and want to sign. + // We need at least one input and one output. + err := psbt.VerifyInputOutputLen(packet, true, true) + if err != nil { + return err + } + + // Go through each input that doesn't have final witness data attached + // to it already and try to sign it. We do expect that we're the last + // ones to sign. If there is any input without witness data that we + // cannot sign because it's not our UTXO, this will be a hard failure. + tx := packet.UnsignedTx + sigHashes := txscript.NewTxSigHashes(tx) + for idx, txIn := range tx.TxIn { + in := packet.Inputs[idx] + + // We can only sign if we have UTXO information available. We + // can just continue here as a later step will fail with a more + // precise error message. + if in.WitnessUtxo == nil && in.NonWitnessUtxo == nil { + continue + } + + // Skip this input if it's got final witness data attached. + if len(in.FinalScriptWitness) > 0 { + continue + } + + // We can only sign this input if it's ours, so we try to map it + // to a coin we own. If we can't, then we'll continue as it + // isn't our input. + utxo, err := r.FetchInputInfo(&txIn.PreviousOutPoint) + if err != nil { + continue + } + fullTx := utxo.PrevTx + signDesc := &input.SignDescriptor{ + KeyDesc: keychain.KeyDescriptor{}, + Output: &wire.TxOut{ + Value: int64(utxo.Value), + PkScript: utxo.PkScript, + }, + HashType: in.SighashType, + SigHashes: sigHashes, + InputIndex: idx, + } + + // Find out what UTXO we are signing. Wallets _should_ always + // provide the full non-witness UTXO for segwit v0. + var signOutput *wire.TxOut + if in.NonWitnessUtxo != nil { + prevIndex := txIn.PreviousOutPoint.Index + signOutput = in.NonWitnessUtxo.TxOut[prevIndex] + + if !psbt.TxOutsEqual(signDesc.Output, signOutput) { + return fmt.Errorf("found UTXO %#v but it "+ + "doesn't match PSBT's input %v", + signDesc.Output, signOutput) + } + + if fullTx.TxHash() != txIn.PreviousOutPoint.Hash { + return fmt.Errorf("found UTXO tx %v but it "+ + "doesn't match PSBT's input %v", + fullTx.TxHash(), + txIn.PreviousOutPoint.Hash) + } + } + + // Fall back to witness UTXO only for older wallets. + if in.WitnessUtxo != nil { + signOutput = in.WitnessUtxo + + if !psbt.TxOutsEqual(signDesc.Output, signOutput) { + return fmt.Errorf("found UTXO %#v but it "+ + "doesn't match PSBT's input %v", + signDesc.Output, signOutput) + } + } + + // Do the actual signing in ComputeInputScript which in turn + // will invoke the remote signer. + script, err := r.ComputeInputScript(tx, signDesc) + if err != nil { + return fmt.Errorf("error computing input script for "+ + "input %d: %v", idx, err) + } + + // Serialize the witness format from the stack representation to + // the wire representation. + var witnessBytes bytes.Buffer + err = psbt.WriteTxWitness(&witnessBytes, script.Witness) + if err != nil { + return fmt.Errorf("error serializing witness: %v", err) + } + packet.Inputs[idx].FinalScriptWitness = witnessBytes.Bytes() + packet.Inputs[idx].FinalScriptSig = script.SigScript + } + + // Make sure the PSBT itself thinks it's finalized and ready to be + // broadcast. + err = psbt.MaybeFinalizeAll(packet) + if err != nil { + return fmt.Errorf("error finalizing PSBT: %v", err) + } + + return nil +} + // DeriveNextKey attempts to derive the *next* key within the key family // (account in BIP43) specified. This method should return the next external // child within this branch. @@ -409,40 +358,7 @@ func (r *RPCKeyRing) FinalizePsbt(packet *psbt.Packet, accountName string) error func (r *RPCKeyRing) DeriveNextKey( keyFam keychain.KeyFamily) (keychain.KeyDescriptor, error) { - ctxt, cancel := context.WithTimeout(context.Background(), r.rpcTimeout) - defer cancel() - - // We need to keep the local and remote wallet in sync. That's why we - // first attempt to also derive the next key on the remote wallet. - remoteDesc, err := r.walletClient.DeriveNextKey(ctxt, &walletrpc.KeyReq{ - KeyFamily: int32(keyFam), - }) - if err != nil { - return keychain.KeyDescriptor{}, fmt.Errorf("error deriving "+ - "key on remote signer instance: %v", err) - } - - localDesc, err := r.watchOnlyKeyRing.DeriveNextKey(keyFam) - if err != nil { - return keychain.KeyDescriptor{}, fmt.Errorf("error deriving "+ - "key on local wallet instance: %v", err) - } - - // We never know if the administrator of the remote signing wallet does - // manual calls to next address or whatever. So we cannot be certain - // that we're always fully in sync. But as long as our local index is - // lower or equal to the remote index we know the remote wallet should - // have all keys we have locally. Only if the remote wallet falls behind - // the local we might have problems that the remote wallet won't know - // outputs we're giving it to sign. - if uint32(remoteDesc.KeyLoc.KeyIndex) < localDesc.Index { - return keychain.KeyDescriptor{}, fmt.Errorf("error deriving "+ - "key on remote signer instance, derived index %d was "+ - "lower than local index %d", remoteDesc.KeyLoc.KeyIndex, - localDesc.Index) - } - - return localDesc, nil + return r.watchOnlyKeyRing.DeriveNextKey(keyFam) } // DeriveKey attempts to derive an arbitrary key specified by the passed @@ -453,41 +369,7 @@ func (r *RPCKeyRing) DeriveNextKey( func (r *RPCKeyRing) DeriveKey( keyLoc keychain.KeyLocator) (keychain.KeyDescriptor, error) { - ctxt, cancel := context.WithTimeout(context.Background(), r.rpcTimeout) - defer cancel() - - // We need to keep the local and remote wallet in sync. That's why we - // first attempt to also derive the same key on the remote wallet. - remoteDesc, err := r.walletClient.DeriveKey(ctxt, &signrpc.KeyLocator{ - KeyFamily: int32(keyLoc.Family), - KeyIndex: int32(keyLoc.Index), - }) - if err != nil { - return keychain.KeyDescriptor{}, fmt.Errorf("error deriving "+ - "key on remote signer instance: %v", err) - } - - localDesc, err := r.watchOnlyKeyRing.DeriveKey(keyLoc) - if err != nil { - return keychain.KeyDescriptor{}, fmt.Errorf("error deriving "+ - "key on local wallet instance: %v", err) - } - - // We never know if the administrator of the remote signing wallet does - // manual calls to next address or whatever. So we cannot be certain - // that we're always fully in sync. But as long as our local index is - // lower or equal to the remote index we know the remote wallet should - // have all keys we have locally. Only if the remote wallet falls behind - // the local we might have problems that the remote wallet won't know - // outputs we're giving it to sign. - if uint32(remoteDesc.KeyLoc.KeyIndex) < localDesc.Index { - return keychain.KeyDescriptor{}, fmt.Errorf("error deriving "+ - "key on remote signer instance, derived index %d was "+ - "lower than local index %d", remoteDesc.KeyLoc.KeyIndex, - localDesc.Index) - } - - return localDesc, nil + return r.watchOnlyKeyRing.DeriveKey(keyLoc) } // ECDH performs a scalar multiplication (ECDH-like operation) between the @@ -618,23 +500,15 @@ func (r *RPCKeyRing) DerivePrivKey(_ keychain.KeyDescriptor) (*btcec.PrivateKey, // NOTE: The resulting signature should be void of a sighash byte. // // NOTE: This method is part of the input.Signer interface. +// +// NOTE: This method only signs with BIP1017 (internal) keys! func (r *RPCKeyRing) SignOutputRaw(tx *wire.MsgTx, signDesc *input.SignDescriptor) (input.Signature, error) { - ctxt, cancel := context.WithTimeout(context.Background(), r.rpcTimeout) - defer cancel() - - rpcSignReq, err := toRPCSignReq(tx, signDesc) - if err != nil { - return nil, err - } - - resp, err := r.signerClient.SignOutputRaw(ctxt, rpcSignReq) - if err != nil { - return nil, err - } - - return btcec.ParseDERSignature(resp.RawSigs[0], btcec.S256()) + // Forward the call to the remote signing instance. This call is only + // ever called for signing witness (p2pkh or p2wsh) inputs and never + // nested witness inputs, so the sigScript is always nil. + return r.remoteSign(tx, signDesc, nil) } // ComputeInputScript generates a complete InputIndex for the passed @@ -651,109 +525,162 @@ func (r *RPCKeyRing) SignOutputRaw(tx *wire.MsgTx, func (r *RPCKeyRing) ComputeInputScript(tx *wire.MsgTx, signDesc *input.SignDescriptor) (*input.Script, error) { + addr, witnessProgram, sigScript, err := r.WalletController.ScriptForOutput( + signDesc.Output, + ) + if err != nil { + return nil, err + } + signDesc.WitnessScript = witnessProgram + + // Let's give the TX to the remote instance now, so it can sign the + // input. + sig, err := r.remoteSign(tx, signDesc, sigScript) + if err != nil { + return nil, fmt.Errorf("error signing with remote instance: %v", + err) + } + + // ComputeInputScript currently is only used for P2WKH and NP2WKH + // addresses. So the last item on the stack is always the compressed + // public key. + return &input.Script{ + Witness: wire.TxWitness{ + append(sig.Serialize(), byte(signDesc.HashType)), + addr.PubKey().SerializeCompressed(), + }, + SigScript: sigScript, + }, nil +} + +// remoteSign signs the input specified in signDesc of the given transaction tx +// using the remote signing instance. +func (r *RPCKeyRing) remoteSign(tx *wire.MsgTx, signDesc *input.SignDescriptor, + sigScript []byte) (input.Signature, error) { + + packet, err := packetFromTx(tx) + if err != nil { + return nil, fmt.Errorf("error converting TX into PSBT: %v", err) + } + + // Catch incorrect signing input index, just in case. + if signDesc.InputIndex < 0 || signDesc.InputIndex >= len(packet.Inputs) { + return nil, fmt.Errorf("invalid input index in sign descriptor") + } + in := &packet.Inputs[signDesc.InputIndex] + txIn := tx.TxIn[signDesc.InputIndex] + + // Make sure we actually know about the input. We either have been + // watching the UTXO on-chain or we have been given all the required + // info in the sign descriptor. + info, err := r.WalletController.FetchInputInfo(&txIn.PreviousOutPoint) + switch { + // No error, we do have the full UTXO and derivation info available. + case err == nil: + in.WitnessUtxo = &wire.TxOut{ + Value: int64(info.Value), + PkScript: info.PkScript, + } + in.NonWitnessUtxo = info.PrevTx + in.Bip32Derivation = []*psbt.Bip32Derivation{info.Derivation} + + // The wallet doesn't know about this UTXO, so it's probably a TX that + // we haven't published yet (e.g. a channel funding TX). So we need to + // assemble everything from the sign descriptor. We won't be able to + // supply a non-witness UTXO (=full TX of the input being spent) in this + // case. That is no problem if the signing instance is another lnd + // instance since we don't require it for pure witness inputs. But a + // hardware wallet might require it for security reasons. + case signDesc.KeyDesc.PubKey != nil && signDesc.Output != nil: + in.WitnessUtxo = signDesc.Output + in.Bip32Derivation = []*psbt.Bip32Derivation{{ + Bip32Path: []uint32{ + keychain.BIP0043Purpose + + hdkeychain.HardenedKeyStart, + r.coinType + hdkeychain.HardenedKeyStart, + uint32(signDesc.KeyDesc.Family) + + hdkeychain.HardenedKeyStart, + 0, + signDesc.KeyDesc.Index, + }, + PubKey: signDesc.KeyDesc.PubKey.SerializeCompressed(), + }} + + default: + return nil, fmt.Errorf("error assembling UTXO information, "+ + "wallet returned err='%v' and sign descriptor is "+ + "incomplete", err) + } + + // Assemble all other information about the input we have. + in.RedeemScript = sigScript + in.SighashType = signDesc.HashType + in.WitnessScript = signDesc.WitnessScript + + if len(signDesc.SingleTweak) > 0 { + in.Unknowns = append(in.Unknowns, &psbt.Unknown{ + Key: btcwallet.PsbtKeyTypeInputSignatureTweakSingle, + Value: signDesc.SingleTweak, + }) + } + if signDesc.DoubleTweak != nil { + in.Unknowns = append(in.Unknowns, &psbt.Unknown{ + Key: btcwallet.PsbtKeyTypeInputSignatureTweakDouble, + Value: signDesc.DoubleTweak.Serialize(), + }) + } + + // Okay, let's sign the input by the remote signer now. ctxt, cancel := context.WithTimeout(context.Background(), r.rpcTimeout) defer cancel() - rpcSignReq, err := toRPCSignReq(tx, signDesc) - if err != nil { - return nil, err - } - - resp, err := r.signerClient.ComputeInputScript(ctxt, rpcSignReq) - if err != nil { - return nil, err - } - - return &input.Script{ - Witness: resp.InputScripts[0].Witness, - SigScript: resp.InputScripts[0].SigScript, - }, nil -} - -// coinFromOutPoint attempts to locate details pertaining to a coin based on -// its outpoint. If the coin isn't under the control of the backing watch-only -// wallet, then an error is returned. -func (r *RPCKeyRing) coinFromOutPoint(op wire.OutPoint) (*chanfunding.Coin, - error) { - - inputInfo, err := r.WalletController.FetchInputInfo(&op) - if err != nil { - return nil, err - } - - return &chanfunding.Coin{ - TxOut: wire.TxOut{ - Value: int64(inputInfo.Value), - PkScript: inputInfo.PkScript, - }, - OutPoint: inputInfo.OutPoint, - }, nil -} - -// toRPCSignReq converts the given raw transaction and sign descriptors into -// their corresponding RPC counterparts. -func toRPCSignReq(tx *wire.MsgTx, - signDesc *input.SignDescriptor) (*signrpc.SignReq, error) { - - if signDesc.Output == nil { - return nil, fmt.Errorf("need output to sign") - } - var buf bytes.Buffer - if err := tx.Serialize(&buf); err != nil { - return nil, err + if err := packet.Serialize(&buf); err != nil { + return nil, fmt.Errorf("error serializing PSBT: %v", err) } - rpcSignDesc := &signrpc.SignDescriptor{ - KeyDesc: &signrpc.KeyDescriptor{ - KeyLoc: &signrpc.KeyLocator{ - KeyFamily: int32(signDesc.KeyDesc.Family), - KeyIndex: int32(signDesc.KeyDesc.Index), - }, - }, - SingleTweak: signDesc.SingleTweak, - WitnessScript: signDesc.WitnessScript, - Output: &signrpc.TxOut{ - Value: signDesc.Output.Value, - PkScript: signDesc.Output.PkScript, - }, - Sighash: uint32(signDesc.HashType), - InputIndex: int32(signDesc.InputIndex), + resp, err := r.walletClient.SignPsbt( + ctxt, &walletrpc.SignPsbtRequest{FundedPsbt: buf.Bytes()}, + ) + if err != nil { + return nil, fmt.Errorf("error signing PSBT in remote signer "+ + "instance: %v", err) } - if signDesc.KeyDesc.PubKey != nil { - rpcSignDesc.KeyDesc.RawKeyBytes = - signDesc.KeyDesc.PubKey.SerializeCompressed() - } - if signDesc.DoubleTweak != nil { - rpcSignDesc.DoubleTweak = signDesc.DoubleTweak.Serialize() + signedPacket, err := psbt.NewFromRawBytes( + bytes.NewReader(resp.SignedPsbt), false, + ) + if err != nil { + return nil, fmt.Errorf("error parsing signed PSBT: %v", err) } - return &signrpc.SignReq{ - RawTxBytes: buf.Bytes(), - SignDescs: []*signrpc.SignDescriptor{rpcSignDesc}, - }, nil -} - -// toRPCAddrType converts the given address type to its RPC counterpart. -func toRPCAddrType(addrType *waddrmgr.AddressType) (walletrpc.AddressType, - error) { - - if addrType == nil { - return walletrpc.AddressType_UNKNOWN, nil + // We expect a signature in the input now. + if signDesc.InputIndex >= len(signedPacket.Inputs) { + return nil, fmt.Errorf("remote signer returned invalid PSBT") + } + in = &signedPacket.Inputs[signDesc.InputIndex] + if len(in.PartialSigs) != 1 { + return nil, fmt.Errorf("remote signer returned invalid "+ + "partial signature, wanted 1, got %d", + len(in.PartialSigs)) + } + sigWithSigHash := in.PartialSigs[0] + if sigWithSigHash == nil { + return nil, fmt.Errorf("remote signer returned nil signature") } - switch *addrType { - case waddrmgr.WitnessPubKey: - return walletrpc.AddressType_WITNESS_PUBKEY_HASH, nil - - case waddrmgr.NestedWitnessPubKey: - return walletrpc.AddressType_HYBRID_NESTED_WITNESS_PUBKEY_HASH, - nil - - default: - return 0, fmt.Errorf("unhandled address type %v", *addrType) + // The remote signer always adds the sighash type, so we need to account + // for that. + if len(sigWithSigHash.Signature) < btcec.MinSigLen+1 { + return nil, fmt.Errorf("remote signer returned invalid "+ + "partial signature: signature too short with %d bytes", + len(sigWithSigHash.Signature)) } + + // Parse the signature, but chop off the last byte which is the sighash + // type. + sig := sigWithSigHash.Signature[0 : len(sigWithSigHash.Signature)-1] + return btcec.ParseDERSignature(sig, btcec.S256()) } // connectRPC tries to establish an RPC connection to the given host:port with @@ -802,3 +729,42 @@ func connectRPC(hostPort, tlsCertPath, macaroonPath string) (*grpc.ClientConn, return conn, nil } + +// packetFromTx creates a PSBT from a tx that potentially already contains +// signed inputs. +func packetFromTx(original *wire.MsgTx) (*psbt.Packet, error) { + // The psbt.NewFromUnsignedTx function complains if there are any + // scripts or witness content on a TX. So we create a copy of the TX and + // nil out all the offending data, but also keep a backup around that we + // add to the PSBT afterwards. + noSigs := original.Copy() + for idx := range noSigs.TxIn { + noSigs.TxIn[idx].SignatureScript = nil + noSigs.TxIn[idx].Witness = nil + } + + // With all the data that is seen as "signed", we can now create the + // empty packet. + packet, err := psbt.NewFromUnsignedTx(noSigs) + if err != nil { + return nil, err + } + + var buf bytes.Buffer + for idx, txIn := range original.TxIn { + if len(txIn.SignatureScript) > 0 { + packet.Inputs[idx].FinalScriptSig = txIn.SignatureScript + } + + if len(txIn.Witness) > 0 { + buf.Reset() + err = psbt.WriteTxWitness(&buf, txIn.Witness) + if err != nil { + return nil, err + } + packet.Inputs[idx].FinalScriptWitness = buf.Bytes() + } + } + + return packet, nil +}