search

sparrowwallet / sparrow

Desktop Bitcoin Wallet focused on security and privacy. Free and open source.
https://sparrowwallet.com/
Apache License 2.0
1.36k stars 192 forks source link

account discovery not working when using New Wallet instead of Import Wallet #1368

Closed AndySchroder closed 6 months ago

AndySchroder commented 7 months ago

https://github.com/sparrowwallet/sparrow/commit/72cb69645165ffd8c3d752809be166d9dc9ebf19 adds BIP-0044 account discovery as a button. https://github.com/sparrowwallet/sparrow/commit/91d491f5eca4b967590f4cfa943d52f63b90530e adds automatic account discovery when using wallet import. I don't seem to see the discover button anymore that was introduced in https://github.com/sparrowwallet/sparrow/commit/72cb69645165ffd8c3d752809be166d9dc9ebf19 . I'm not sure if https://github.com/sparrowwallet/sparrow/commit/91d491f5eca4b967590f4cfa943d52f63b90530e or another commit removed it.

If doing the following to import an existing wallet (instead of File->Import Wallet):

  1. File->New Wallet
  2. Check "has existing transactions ..."
  3. select a day before the first transaction
  4. New or imported Software Wallet
  5. Use 24 words
  6. enter seed
  7. Create Keystore
  8. Import Keystore
  9. Apply

No accounts are discovered and clicking Add Account there is no discover button.

However, doing File->Import Wallet does discover the accounts correctly. However, when going to Add Account, the discover button also isn't there (but in this case isn't needed because automatic discovery on import worked).

I think any way you import a software wallet, it should automatically discover accounts. Also if you go to Advanced and change the birthday or gap limit it should re-try to discover accounts.

craigraw commented 7 months ago

I don't seem to see the discover button anymore that was introduced in https://github.com/sparrowwallet/sparrow/commit/72cb69645165ffd8c3d752809be166d9dc9ebf19 .

Since Bitcoin Core does not (in general) support checking arbitrary addresses for funds, the button is not added when Bitcoin Core is the connection type.

However, doing File->Import Wallet does discover the accounts correctly.

I suspect this only worked because you had previously scanned these wallet addresses, so Bitcoin Core was able to rely on it's wallet file. This won't work generally.

I think any way you import a software wallet, it should automatically discover accounts.

I see File > New Wallet as a more declarative way to create a wallet, while File > Import Wallet is a more explorative way. There are valid uses cases for both, and having both available is valuable.

Given your advanced use of Sparrow, consider running an Electrum server - it really is a better user experience.

AndySchroder commented 7 months ago

Okay, so I did encounter this with bitcoin core. Will have to re-test with a regular electrum server..

However, does it make sense to just add all the accounts to bitcoin core when adding a new or imported software wallet keystore (either by New Wallet or Import Wallet) and using it with single signature? It seems like the rescan process in bitcoin core is not impacted much at all by extra xpubs, so does it hurt much to just do that? If you haven't clicked "has existing transactions ...", this shouldn't take any more time at all, right? Also, if you later change the birthdate of the keystore, since it would already be tracking all accounts, it seems like all accounts would be automatically rescanned since I think bitcoin core re-scans all wallets whenever it rescans.

That would improve the UX if "Add Account" is clicked and a Discover button could exist if it is finished scanning. Or, you could make the accounts automatically appear if they have been discovered after the rescan completes.

It seems like New Wallet should follow whatever Import Wallet does and find other accounts if they have already been added to bitcoin core.

craigraw commented 7 months ago

It seems like the rescan process in bitcoin core is not impacted much at all by extra xpubs, so does it hurt much to just do that?

Are you sure about this? I don't much data on the impact of adding additional descriptors to Bitcoin Core in terms of RAM, CPU, rescan time etc.

If you haven't clicked "has existing transactions ...", this shouldn't take any more time at all, right?

Correct, but that doesn't account for future work Bitcoin Core may have to do.

It seems like New Wallet should follow whatever Import Wallet does and find other accounts if they have already been added to bitcoin core.

I disagree for the reason stated above. I personally have need of both approaches.

AndySchroder commented 7 months ago

Regarding the resources needed to monitor lots of extra wallets. Here is a test:

I have 37 wallets right now.

$ 
$ bitcoin-cli -rpcwallet=cormorant listdescriptors|grep desc|grep -v addr|wc -l
37
$

If I run

$ bitcoin-cli -rpcwallet=cormorant rescanblockchain

with blockfilterindex=1

I get

2024-04-11T06:38:56Z [cormorant] Rescan started from block 000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f... (fast variant using block filters)

...

2024-04-11T07:48:56Z [cormorant] Still rescanning. At block 710002. Progress=0.691436

...

2024-04-11T08:06:16Z [cormorant] Scanning current mempool transactions.
2024-04-11T08:06:17Z [cormorant] Rescan completed in         5241512ms
2024-04-11T08:07:04Z Saw new header hash=000000000000000000017ec181d09f0e92c1b0f62552f571f0e39a84754100b3 height=838728

That comes out to an average of 6ms/block over the whole chain and about 8ms/block for recent blocks.

bitcoind seems to have done this rescan in serial, so only 1 of my 8 CPU were in use. I would think the rescan could eventually be parallelized quite a bit, so I'd imagine the time will eventually be made quicker.

This is a full blockchain rescan, so it doesn't count the resources used to perpetually monitor all of these wallets, but I think it is the simplest test that I can do right now.

Now I'm not sure what bitcoind does as far as gap limits, but I'd imagine if we have a bunch of empty wallets that are tracked, will they use that much extra effort if there aren't any transactions in them? I don't know that these wallets had many transactions in them since they are mostly all test wallets, but that should be similar to scanning a bunch of BIP-0044 accounts that are empty.

AndySchroder commented 7 months ago

Also, here is some related performance data: https://github.com/bitcoin/bitcoin/pull/25957 . I don't think they specify whether the transactions it is tracking are all in the same wallet or multiple wallets, but they do present a large range of transactions.

craigraw commented 7 months ago

Thanks for the data. Unfortunately we still don't have a good idea of what additional resources are used for additional descriptors in a Bitcoin Core wallet. Sparrow currently allows up to 33 accounts to be added, which would mean 66 descriptors in total.

I would think the rescan could eventually be parallelized quite a bit, so I'd imagine the time will eventually be made quicker.

AFAIK the scanning currently requires the outputs of previous transactions (i.e. there is a serialization requirement), so I'm not sure this is possible.

Now I'm not sure what bitcoind does as far as gap limits

The keypool configuration setting determines the effective gap limit. See https://bitcointalk.org/index.php?topic=2940114.msg30228579#msg30228579. For the 66 descriptors, we would be scanning for 66000 addresses by default (as opposed to the current 2000 for an empty wallet).

AndySchroder commented 7 months ago

Why does each account require 2 descriptors?

AndySchroder commented 7 months ago

What are all the accounts that are added? I thought there was 0-9 + premix+postmix+badbank. That's 13, not 33. Are you including legacy, nested segwit, and native segwit?

For File->New Wallet (what I originally opened this issue for), you normally have to choose a derivation path specifically, so in that case I would only expect the descriptor for account 0 to be defined and then you would only add accounts that can be discovered by incrementing the derivation path from that descriptor (which should then only be 13 and not 33). This is the use case I am trying to deal with is only needing to record the seed and descriptor for account 0 and then all other accounts should be found automatically per BIP-0044.

craigraw commented 7 months ago

Receive and change chains (until we get multipath descriptors https://github.com/bitcoin/bitcoin/pull/22838).

craigraw commented 7 months ago

What are all the accounts that are added?

See https://github.com/sparrowwallet/sparrow/issues/1119

AndySchroder commented 7 months ago

The keypool configuration setting determines the effective gap limit. See https://bitcointalk.org/index.php?topic=2940114.msg30228579#msg30228579. For the 66 descriptors, we would be scanning for 66000 addresses by default (as opposed to the current 2000 for an empty wallet).

Okay, so sparrow has a much lower default gap limit of 20 and you are just hoping no one increases it beyond the default bitcoind gap limit?

craigraw commented 7 months ago

you are just hoping no one increases it beyond the default bitcoind gap limit

Not at all. Sparrow reads and manages the keypool for all descriptors it adds.

AndySchroder commented 7 months ago

Okay, that link seemed to be for the bitcoind command line option -keypool, but I guess you are using https://bitcoincore.org/en/doc/26.0.0/rpc/wallet/newkeypool/ or https://bitcoincore.org/en/doc/26.0.0/rpc/wallet/keypoolrefill/ then?

AndySchroder commented 7 months ago

What are all the accounts that are added?

See #1119

So does File->Import Wallet scan 99 accounts if it has to check through legacy, nested segwit, and native segwit?

craigraw commented 7 months ago

So does File->Import Wallet scan 99 accounts if it has to check through legacy, nested segwit, and native segwit?

I refreshed my memory on this - my earlier estimate was a little out. To to keep scanning time reasonable it currently scans the first 10 accounts. But as you remind me, it does scan for 3 script types on all accounts, plus a couple of extra derivations using by Electrum/Bisq. In total I count 35 derivations/script type pairs scanned, so 70 descriptors to match this.

AndySchroder commented 7 months ago

Wondering if there are by chance some kind of public test wallets that have lots of transactions and accounts that I can add and do the bitcoin-cli -rpcwallet=cormorant rescanblockchain command again on mainnet to get more appropriate data? Possibly there is someone that has an old wallet that they have emptied but shared with the community for testing since it now has zero balance. Or if that has not happened, maybe there are some test descriptors that I can add as watching only that, although may not be separate accounts, could get the number of descriptors up to a high enough value that we could collect some meaningful data.

AndySchroder commented 7 months ago

On a related note, is there any effort (like a new descriptor format) to include the birthday block height of a descriptor in the descriptor itself?

AndySchroder commented 7 months ago

Doing some tests with a real electrum server now.

I guess I'm wondering if when going to File->New Wallet it should also try to auto discover whenever a keystore is changed or a descriptor is changed when using a real electrum server.

When using bitcoin core, if we think that it will definitely be too slow to add all possible descriptors at the beginning, it might make sense to keep the Discover button when you click Add Account and you could just have a pop up that warns you it might take some time to auto discover because the blockchain needs to be re-scanned. Or, in addition to the "has existing transactions ..." when you create or import the wallet, ask the user if they want to try and auto discover all accounts.

craigraw commented 7 months ago

Seems like a coldcard would auto discover all accounts just as if importing with a seed.

A coldcard doesn't "discover" anything really. It does derive addresses to know what it can sign for, but otherwise I don't think it's comparable.

(I guess this is because there are no concerns about the wallet's birthday that you need to query the user about?).

Correct - wallet birthday is not used for an Electrum server.

I guess I'm wondering if when going to File->New Wallet it should also try to auto discover whenever a keystore is changed or a descriptor is changed when using a real electrum server.

As discussed I prefer a declarative model here.

it might make sense to keep the Discover button when you click Add Account

I have done so in 646b8b0e. This will just use whatever descriptors/wallet history is already present in Bitcoin Core, so useful in the case of restoring a Sparrow wallet where the Bitcoin Core wallet is still present with any accounts previously added there. This matches the behaviour in the Import Wallet discovery.

AndySchroder commented 7 months ago

A coldcard doesn't "discover" anything really. It does derive addresses to know what it can sign for, but otherwise I don't think it's comparable.

The way I understand it is you send the coldcard some derivation path and it provides the xpub, then you can derive keys from the xpub, then you query the electrum server for each address.

For a software wallet with a seed, you take the seed, then compute the xpub for a derivation, then derive the keys from the xpub, then you query the electrum server for each address.

The only difference I see here is that with a software wallet you compute the xpubs whereas with a coldcard you ask the coldcard for the xpubs. In both cases you are iterating over different derivations paths for the different accounts and then querying the electrum server to do the "discovery". The only difference seems to be where you get the xpub.

The Discover button works when clicking Add Account with the coldcard, so you seem to be doing this process I described above already. I'm just thinking that when going to File->Import wallet it should follow the same discovery process for the coldcard as when you click this button, just like if you use File->Import with a software wallet.

AndySchroder commented 7 months ago

I guess I'm wondering if when going to File->New Wallet it should also try to auto discover whenever a keystore is changed or a descriptor is changed when using a real electrum server.

As discussed I prefer a declarative model here

Thinking more about this, I agree with you now!

AndySchroder commented 7 months ago

it might make sense to keep the Discover button when you click Add Account

I have done so in 646b8b0. This will just use whatever descriptors/wallet history is already present in Bitcoin Core, so useful in the case of restoring a Sparrow wallet where the Bitcoin Core wallet is still present with any accounts previously added there. This matches the behaviour in the Import Wallet discovery.

This seems like a decent compromise for now. Possibly when https://github.com/bitcoin/bitcoin/issues/29183 gets finished it will be worth revisiting the topic and possibly there will be more data available at that time about how much work it is to track a bunch of extra descriptors.

Do you know if someone manually adds an Account #1 with bitcoin core (and it is not already tracked), then they add Account #2 (and that is also not already tracked) does it wait to complete the rescan for Account #1 to do another rescan for Account #2 or does it stop and start the rescan with Account #2 included?

craigraw commented 7 months ago

Thinking more about this, I agree with you now!

Great! - can we close this one off?

This seems like a decent compromise for now.

Something else I like about the current setup is that it's useful for developers. If you add lot of descriptors, it makes querying with bitcoin-cli a lot more cumbersome.

does it wait to complete the rescan

As far as I know, rescanning cannot happen concurrently. This is quite a serious limitation, so users with lots of changing wallets are better off with an Electrum server. The current scan will continue (unless it is explicitly stopped) until it is done.

AndySchroder commented 7 months ago

Wondering of your thoughts on https://github.com/sparrowwallet/sparrow/issues/1368#issuecomment-2057285672 ? This seems to be a new issue, do you agree?

craigraw commented 6 months ago

As discussed I prefer a declarative model for the File > New Wallet flow, not an auto discovery model. So no, I do not believe a new issue is useful, and I think this one can be closed off.

AndySchroder commented 6 months ago

https://github.com/sparrowwallet/sparrow/issues/1368#issuecomment-2057285672 applies to File->Import Wallet, that's why I was suggesting it might be a new issue, but if you don't even want to fix that then I won't bother.

craigraw commented 6 months ago

I may have misunderstood what you were referring to there. By "same discovery process for the coldcard" do you mean when connected via USB? It's not possible using the file or QR import, since only one account is exported at a time.

AndySchroder commented 6 months ago

Yes, if the Coldcard is connected via USB, it should be able to fetch all the xpubs for each account and then discover if there are any transactions in them, just like a software wallet with a seed checks the xpub for each account and then discovers if there are any transactions in each account.

craigraw commented 6 months ago

Yes, if the Coldcard is connected via USB, it should be able to fetch all the xpubs for each account

Unfortunately this is IMO too slow to be practical. It takes 90 seconds to scan 10 xpubs from a Coldcard Mk2 - this is too slow for the standard "import my coldcard" flow. For that reason, the discover accounts functionality for a USB connected device needs to remain in the Add Account dialog, where the user is more explicitly requesting it.

Closing this off as I think all issues have been addressed/considered.