Potential coin loss due to counterintuitive importwallet RPC behavior #12399

TL;DR: importwallet ignores the reserve=1 field, thus the keypool of the imported wallet is lgnored. Whenever the user requests a new address in the UI it won't be part of their previous backups of the imported wallet.

The dumpwallet RPC call produces a human-readable wallet dump which contains all private keys and for each key its birth date and a field change=1 or reserve=1 or label=what the user entered in the UI.
I assume reserve=1 means that a private key was part of the keypool, i.e. a pool of keys which are stored before the user even requests a new address in the UI to ensure backups are valid for quite some time in the future.

Now I would expect the importwallet RPC call to be precisely the inverse operation of the dumpwallet RPC call just by looking at their names, "dump" and "wallet" sounds like it includes everything. And the fact that the file is human readable and its content includes information about what is part of the keypool makes it look even more like the keypool is preserved.

However the current code of master, and also the actual behavior of v0.15.1 which I've tested, shows that in fact importwallet ignores the reserve=1 field: https://github.com/bitcoin/bitcoin/blob/89005ddad1c4a9732ecae47c7de34b4de157f48f/src/wallet/rpcdump.cpp#L570-L581

This means that the imported keys are not considered as part of the keypool, and not returned to the user when they request a new address. As the dump contains the birth date of the keys they might assume that importing a dump which is much older than the wallet they are importing to would result in the UI returning the dump's keypool addresses first. But they will get addresses of the keypool of the wallet to which they imported the dump, of which they might not have a backup as they consider the dump as their backup. Thus all backups they have created from their walletdump are invalid.

I don't see how this causes coin loss. The keys are imported to the wallet and their transactions are still scanned for regardless of whether they were in the keypool or not. So what if they are not returned to the user? Transactions to those keys are still scanned for; the backups are not invalidated as the keys are all there.

@achow101

I don't see how this causes coin loss. The keys are imported to the wallet and their transactions are still scanned for regardless of whether they were in the keypool or not. So what if they are not returned to the user? Transactions to those keys are still scanned for; the backups are not invalidated as the keys are all there.

I was not referring to preexisting stored coins on the imported keys or coins incoming to the imported keys. You lose coins if:

• you request a new address in the UI.
• you have someone send coins to that address.
• afterwards your disk dies and you try to restore from a backup of your wallet dump. The requested address won't be included in it as it was from the keypool of the wallet you imported to, not from the keypool of the imported wallet of which your dump was from.

(Of course the user should assume that if there are 2 keypools involved then both can be used, so also the one of the newer wallet which imported the older one - but the dumpwallet output also contains the birth date of all keys, and the keypool normally should be used in order of key age. So if the imported dump is much older than the importing wallet then I would assume the keypool of the imported dump is used up first, but it is not used at all.)

The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.

Closing due to lack of interest. Pull requests with improvements are always welcome.

@MarcoFalke

The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.

Closing due to lack of interest. Pull requests with improvements are always welcome.

Uh, sorry but this is not a feature request, it is a bug report, and it can cause users to lose money!

Thus requesting more people to be interested in this seems inappropriate because someone being "interested" is equal to them having lost money due to the bug, and you wouldn't want people to lose money I suppose :)

I cannot offer a PR because I'm not qualified to write C++ code, sorry.

Sorry, I just couldn't follow how this leads to loss of fund, so I interpreted the issue as a wish-list item where you want imported keypool keys be queued up in the keypool again. I don't think anyone is ever going to work on that idea.

However, if you think this leads to loss of coins, then please include exact steps to reproduce and I am happy to take another look.

I haven't tested, but I think @xor-freenet is imagining this sequence of steps, which I have also worried about:

1. Import backed up wallet to a new computer; call the computer C.
2. With C, perform a transaction.
3. C's hard disk crashes.

If importing a wallet also imported the keypool, you'd be okay: your original wallet backup (the one you used in step 1) would work.

But if not, I assume that means you've lost your outputs from 2.

This could be mitigated by making a new backup each time you import a wallet, but some users may reasonably assume there's no point in backing up a wallet when you've just restored from a backup.

This comment is based on assumptions and no actual tests; sorry if I'm mistaken about all this.

@MarcoFalke : @falsifian has correctly described the exact steps you requested which lead to loss of funds.
Sorry for forgetting to reply with them myself, and thanks to falsifian for doing so.

falsifian also brought up a very good additional point:

but some users may reasonably assume there's no point in backing up a wallet when you've just restored from a backup

This is amplified by the fact that professional users might even before they send any funds to a wallet dump it first, import the dump to a fresh wallet, and then work off the imported dump. That's because only working upon a working-copy which is the result of a restored backup ensures that both:

• you are forced to test your restoring mechanism so you know it works.
• and it also prevents you from ever forgetting to backup anything because your working copy is based upon a backup.

With the current state of Bitcoin Core's importwallet however, it means they lose their money even though they intended to do a professional failsafe procedure!

Further, notice that it is not unlikely for people to prefer dumpwallet + importwallet over just backing up the wallet.dat:

• dumpwallet produces a human readable file which can thus be much more easily validated to be non-corrupted than a binary blob such as wallet.dat.
• Also if someone wants to conduct cold storage they might prefer dumpwallet because you can just print the output on a sheet of paper (albeit that is not encrypted with a password, please be careful!).

Thus please re-open this ticket, it is not a feature request, it is a bug :(

@MarcoFalke @falsifian In case you had read my reply off an email notification please notice that I had to edit a word in the "This is amplified by..." sentence to make it more easy to understand, and GitHub won't notice you by email about that, so here it is: "This is amplified by the fact that professional users might even before they send (<- edited, was 'import') any funds to a wallet dump it first, import the dump to a fresh wallet, and then work off the imported dump."