Merge pull request #7565 from Torakushi/accounts

Fix non-deterministic behaviour for custom accounts

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

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

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(

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

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
@@ -636,17 +627,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 +700,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
@@ -821,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
@@ -838,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,

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
+}