[bug]: returned (wallet) account is not deterministic if there are various scopes for the same account name

[Torakushi]

Background

If we have two accounts with the same name (not "default") but with different scopes (saying p2wkh and p2tr), the account returned by b.wallet.LookupAccount(accountName) is not deterministic.

The easiest way to check this is to call lncli wallet accounts list --name accountName various times (without providing address_type). Sometimes it will return the account with the taproot scope and sometimes the other one. This is not deterministic because in LookupAccount, we browse a map.

More problematic, the problem should happen when calling FundPsbt with a walletrpc.FundPsbtRequest that contains an account name in the payload (an account with various scopes, not the "default" one) see here

Your environment

• lnd v0.15.5
• MacOS 12.6
• bitcoind v.24.0
• To reproduce easily I used Polar v1.4.0 (see steps bellow)

Steps to reproduce

To reproduce easily this non deterministic behaviour you can:

• have 2 regtest lnd nodes (Alice and Bob)

• import in lnd-bob the Alice p2tr default account and p2wkh default account (using the same name) (lncli wallet accounts import key --address_type p2tr/p2wkh alice)

• then call lncli wallet account list --name alice various times in lnd-bob and see that it will sometimes return the p2tr scope and sometimes the p2wkh scope

Expected behaviour

Deterministic response

Actual behaviour

Non-deterministic response

[ellemouton]

Hi @Torakushi - I think the intended behaviour is to only ever have 1 scope per account name.

So perhaps to prevent this from happening in future, all we need is to prevent users from doing this in the first place

[Torakushi]

Hi @ellemouton

Indeed it looks reasonable if this is the intended behaviour (otherwise it might be confusing)

But maybe it would be nice to be able to select a specific scope in FundPsbtRequest for an account name with various scopes (aka default at the moment) for two reasons:

1) It solves the breaking change induced by using taproot address as the default change output

2) If we ever want to allow an account name to have different scopes, it will be useful.

[Torakushi]

Regarding this issue, I can create a PR to prohibit users to create a custom account with various key scopes.

As well, this PR will make the list accounts (+ fundpsbt) deterministic for users who have already created custom accounts with various key scopes.

The question is: do we want to solve this behaviour directly in btcsuite or only in lnd (in this case I will just add some some security checks in LND. It is not as clean but if we need to..)

[guggero]

Accounts are a bit wonky in btcwallet the way they are currently implemented. I think cleaning up the way they work would require a lot of refactoring. So I'd suggest to instead add the checks to lnd in order for users not to get into non-deterministic states.

[Torakushi]

Indeed, I didn't realise how tricky it would be in btcsuite 😄

Makes sense! I will create a PR in LND to solve this.

Thanks!