From 2ef5ea97425f44d4bf1c8e5d67f80586898e440f Mon Sep 17 00:00:00 2001 From: Torakushi Date: Mon, 27 Feb 2023 17:34:11 +0100 Subject: [PATCH 1/4] btcwallet: fix non-deterministic behaviour in ListAccounts In theory, it should be only one custom account with a given name. However, due to a lack of check, users could have created custom accounts with various key scopes. In that case, ListAccounts has to list all these accounts. --- lnwallet/btcwallet/btcwallet.go | 49 +++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 9 deletions(-) diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go index 8305dbb55..c5ff1c8c8 100644 --- a/lnwallet/btcwallet/btcwallet.go +++ b/lnwallet/btcwallet/btcwallet.go @@ -636,17 +636,36 @@ func (b *BtcWallet) ListAccounts(name string, break } - // Otherwise, we'll retrieve the single account that's mapped by - // the given name. - scope, acctNum, err := b.wallet.LookupAccount(name) - if err != nil { - return nil, err + // In theory, there should be only one custom account for the + // given name. However, due to a lack of check, users could + // create custom accounts with various key scopes. This + // behaviour has been fixed but, we return all potential custom + // accounts with the given name. + for _, scope := range waddrmgr.DefaultKeyScopes { + a, err := b.wallet.AccountPropertiesByName( + scope, name, + ) + switch { + case waddrmgr.IsError(err, waddrmgr.ErrAccountNotFound): + continue + + // In the specific case of a wallet initialized only by + // importing account xpubs (watch only wallets), it is + // possible that some keyscopes will be 'unknown' by the + // wallet (depending on the xpubs given to initialize + // it). If the keyscope is not found, just skip it. + case waddrmgr.IsError(err, waddrmgr.ErrScopeNotFound): + continue + + case err != nil: + return nil, err + } + + res = append(res, a) } - account, err := b.wallet.AccountProperties(scope, acctNum) - if err != nil { - return nil, err + if len(res) == 0 { + return nil, newAccountNotFoundError(name) } - res = append(res, account) // Only the key scope filter was provided, so we'll return all accounts // matching it. @@ -690,6 +709,18 @@ func (b *BtcWallet) ListAccounts(name string, return res, nil } +// newAccountNotFoundError returns an error indicating that the manager didn't +// find the specific account. This error is used to be compatible with the old +// 'LookupAccount' behaviour previously used. +func newAccountNotFoundError(name string) error { + str := fmt.Sprintf("account name '%s' not found", name) + + return waddrmgr.ManagerError{ + ErrorCode: waddrmgr.ErrAccountNotFound, + Description: str, + } +} + // RequiredReserve returns the minimum amount of satoshis that should be // kept in the wallet in order to fee bump anchor channels if necessary. // The value scales with the number of public anchor channels but is From dd5ed71669e36bac4f877803a318c7711424b639 Mon Sep 17 00:00:00 2001 From: Torakushi Date: Mon, 6 Mar 2023 18:19:18 +0100 Subject: [PATCH 2/4] btcwallet: remove LookupAccount because of non-deterministic result In theory, it should be only one custom account with a given name. However, users could have created custom accounts with various key scopes. In that case, 'LookupAccount' has a non deterministic behaviour. To fix that, we browse through all key scopes (deterministically) and return the first account we found. --- lnwallet/btcwallet/btcwallet.go | 17 ++++----------- lnwallet/btcwallet/psbt.go | 37 +++++++++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go index c5ff1c8c8..11f76a222 100644 --- a/lnwallet/btcwallet/btcwallet.go +++ b/lnwallet/btcwallet/btcwallet.go @@ -82,11 +82,6 @@ var ( // requested for the default imported account within the wallet. errNoImportedAddrGen = errors.New("addresses cannot be generated for " + "the default imported account") - - // errIncompatibleAccountAddr is an error returned when the type of a - // new address being requested is incompatible with the account. - errIncompatibleAccountAddr = errors.New("incompatible address type " + - "for account") ) // BtcWallet is an implementation of the lnwallet.WalletController interface @@ -516,18 +511,14 @@ func (b *BtcWallet) keyScopeForAccountAddr(accountName string, return addrKeyScope, defaultAccount, nil } - // Otherwise, look up the account's key scope and check that it supports - // the requested address type. - keyScope, account, err := b.wallet.LookupAccount(accountName) + // Otherwise, look up the custom account and if it supports the given + // key scope. + accountNumber, err := b.wallet.AccountNumber(addrKeyScope, accountName) if err != nil { return waddrmgr.KeyScope{}, 0, err } - if keyScope != addrKeyScope { - return waddrmgr.KeyScope{}, 0, errIncompatibleAccountAddr - } - - return keyScope, account, nil + return addrKeyScope, accountNumber, nil } // NewAddress returns the next external or internal address for the wallet diff --git a/lnwallet/btcwallet/psbt.go b/lnwallet/btcwallet/psbt.go index 395c5667c..b3e836946 100644 --- a/lnwallet/btcwallet/psbt.go +++ b/lnwallet/btcwallet/psbt.go @@ -97,7 +97,7 @@ func (b *BtcWallet) FundPsbt(packet *psbt.Packet, minConfs int32, "custom change type for custom accounts") } - scope, account, err := b.wallet.LookupAccount(accountName) + scope, account, err := b.lookupFirstCustomAccount(accountName) if err != nil { return 0, err } @@ -512,7 +512,7 @@ func (b *BtcWallet) FinalizePsbt(packet *psbt.Packet, accountName string) error // number to determine if the inputs belonging to this account should be // signed. default: - scope, account, err := b.wallet.LookupAccount(accountName) + scope, account, err := b.lookupFirstCustomAccount(accountName) if err != nil { return err } @@ -522,3 +522,36 @@ func (b *BtcWallet) FinalizePsbt(packet *psbt.Packet, accountName string) error return b.wallet.FinalizePsbt(keyScope, accountNum, packet) } + +// lookupFirstCustomAccount returns the first custom account found. In theory, +// there should be only one custom account for the given name. However, due to a +// lack of check, users could have created custom accounts with various key +// scopes. This behaviour has been fixed but, we still need to handle this +// specific case to avoid non-deterministic behaviour implied by LookupAccount. +func (b *BtcWallet) lookupFirstCustomAccount( + name string) (waddrmgr.KeyScope, uint32, error) { + + var ( + account *waddrmgr.AccountProperties + keyScope waddrmgr.KeyScope + ) + for _, scope := range waddrmgr.DefaultKeyScopes { + var err error + account, err = b.wallet.AccountPropertiesByName(scope, name) + if waddrmgr.IsError(err, waddrmgr.ErrAccountNotFound) { + continue + } + if err != nil { + return keyScope, 0, err + } + + keyScope = scope + + break + } + if account == nil { + return waddrmgr.KeyScope{}, 0, newAccountNotFoundError(name) + } + + return keyScope, account.AccountNumber, nil +} From 77d3f00c3b9002d0ab7990d29141c5a5206514d7 Mon Sep 17 00:00:00 2001 From: Torakushi Date: Fri, 17 Feb 2023 18:06:19 +0100 Subject: [PATCH 3/4] walletrpc+btcwallet: no custom account with various key scopes Currently, a user can create a custom account with various key scopes. This is not a desired behaviour. --- cmd/lncli/walletrpc_active.go | 3 +++ itest/lnd_wallet_import_test.go | 33 +++++++++++++++++++++++++++++++++ lntest/rpc/wallet_kit.go | 14 ++++++++++++++ lnwallet/btcwallet/btcwallet.go | 20 ++++++++++++++++++++ 4 files changed, 70 insertions(+) diff --git a/cmd/lncli/walletrpc_active.go b/cmd/lncli/walletrpc_active.go index 7bae32fbf..577121928 100644 --- a/cmd/lncli/walletrpc_active.go +++ b/cmd/lncli/walletrpc_active.go @@ -1447,6 +1447,9 @@ var importAccountCommand = cli.Command{ 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. + If an account with the same name already exists (even with a different + key scope), an error will be returned. + 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 diff --git a/itest/lnd_wallet_import_test.go b/itest/lnd_wallet_import_test.go index afd96a352..c0012213d 100644 --- a/itest/lnd_wallet_import_test.go +++ b/itest/lnd_wallet_import_test.go @@ -2,6 +2,7 @@ package itest import ( "bytes" + "fmt" "testing" "time" @@ -468,6 +469,10 @@ func testWalletImportAccount(ht *lntest.HarnessTest) { name string addrType walletrpc.AddressType }{ + { + name: "standard BIP-0044", + addrType: walletrpc.AddressType_WITNESS_PUBKEY_HASH, + }, { name: "standard BIP-0049", addrType: walletrpc. @@ -550,6 +555,34 @@ func runWalletImportAccountScenario(ht *lntest.HarnessTest, } dave.RPC.ImportAccount(importReq) + // Try to import an account with the same name but with a different + // key scope. It should return an error. + otherAddrType := walletrpc.AddressType_TAPROOT_PUBKEY + if addrType == walletrpc.AddressType_TAPROOT_PUBKEY { + otherAddrType-- + } + + listReq = &walletrpc.ListAccountsRequest{ + Name: "default", + AddressType: otherAddrType, + } + listResp = carol.RPC.ListAccounts(listReq) + require.Len(ht, listResp.Accounts, 1) + + carolAccountOtherAddrType := listResp.Accounts[0] + + errAccountExists := fmt.Sprintf( + "account '%s' already exists", importedAccount, + ) + + importReq = &walletrpc.ImportAccountRequest{ + Name: importedAccount, + ExtendedPublicKey: carolAccountOtherAddrType.ExtendedPublicKey, + AddressType: otherAddrType, + } + err := dave.RPC.ImportAccountAssertErr(importReq) + require.ErrorContains(ht, err, errAccountExists) + // We'll generate an address for Carol from Dave's node to receive some // funds. externalAddr := newExternalAddr( diff --git a/lntest/rpc/wallet_kit.go b/lntest/rpc/wallet_kit.go index 95af5f92d..f28eedf6a 100644 --- a/lntest/rpc/wallet_kit.go +++ b/lntest/rpc/wallet_kit.go @@ -235,6 +235,20 @@ func (h *HarnessRPC) ImportAccount( return resp } +// ImportAccountAssertErr makes the ImportAccount RPC call and asserts an error +// is returned. It then returns the error. +func (h *HarnessRPC) ImportAccountAssertErr( + req *walletrpc.ImportAccountRequest) error { + + ctxt, cancel := context.WithTimeout(h.runCtx, DefaultTimeout) + defer cancel() + + _, err := h.WalletKit.ImportAccount(ctxt, req) + require.Error(h, err) + + return err +} + // ImportPublicKey makes a RPC call to the node's WalletKitClient and asserts. // //nolint:lll diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go index 11f76a222..3050be03a 100644 --- a/lnwallet/btcwallet/btcwallet.go +++ b/lnwallet/btcwallet/btcwallet.go @@ -843,6 +843,10 @@ func (b *BtcWallet) ListAddresses(name string, // 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 custom accounts, we will first check if there is no account with the same +// name (even with a different key scope). No custom account should have various +// key scopes as it will result in non-deterministic behaviour. +// // 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 @@ -860,6 +864,22 @@ func (b *BtcWallet) ImportAccount(name string, accountPubKey *hdkeychain.Extende dryRun bool) (*waddrmgr.AccountProperties, []btcutil.Address, []btcutil.Address, error) { + // For custom accounts, we first check if there is no existing account + // with the same name. + if name != lnwallet.DefaultAccountName && + name != waddrmgr.ImportedAddrAccountName { + + _, err := b.ListAccounts(name, nil) + if err == nil { + return nil, nil, nil, + fmt.Errorf("account '%s' already exists", + name) + } + if !waddrmgr.IsError(err, waddrmgr.ErrAccountNotFound) { + return nil, nil, nil, err + } + } + if !dryRun { accountProps, err := b.wallet.ImportAccount( name, accountPubKey, masterKeyFingerprint, addrType, From be4dae0ec587fedb02ad5168ac1f42334eb4b67f Mon Sep 17 00:00:00 2001 From: Torakushi Date: Thu, 20 Apr 2023 14:32:11 +0200 Subject: [PATCH 4/4] update release-notes-0.17 --- docs/release-notes/release-notes-0.17.0.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/release-notes/release-notes-0.17.0.md b/docs/release-notes/release-notes-0.17.0.md index b1e50c113..981e331ad 100644 --- a/docs/release-notes/release-notes-0.17.0.md +++ b/docs/release-notes/release-notes-0.17.0.md @@ -80,6 +80,13 @@ package](https://github.com/lightningnetwork/lnd/pull/7356) * [Fixed a bug where we didn't check for correct networks when submitting onchain transactions](https://github.com/lightningnetwork/lnd/pull/6448). +* [Fix non-deterministic behaviour in RPC calls for + custom accounts](https://github.com/lightningnetwork/lnd/pull/7565). + In theory, it should be only one custom account with a given name. However, + due to a lack of check, users could have created custom accounts with various + key scopes. The non-deterministic behaviours linked to this case are fixed, + and users can no longer create two custom accounts with the same name. + ## Misc * [Generate default macaroons @@ -157,6 +164,7 @@ unlock or create. * Michael Street * MG-ng * Oliver Gugger +* Pierre Beugnet * Satarupa Deb * Shaurya Arora * Torkel Rogstad