Encrypted Wallet Backward Compatibility
13 messages, 4 to 10 Jul 2011. Started by Matt Corallo.
Matt Corallo (5), Gavin Andresen (2), Greg Maxwell (2), Luke Dashjr, Douglas Huff, Jeff Garzik, Pieter Wuille
joric rightly points out that there are currently backward-compatibility issues with Wallet encryption. As it stands now: In version 0.3.23, Bitcoin dies with "ReserveKeyFromKeyPool() : unknown key in key pool" after writing one unencrypted private key to the (otherwise) encrypted wallet. In version 0.3.22 (and I'd assume prior versions as well), Bitcoin opens fine and displays transactions, however shows a total balance of what is help only in unencrypted keys (of which it also writes a minimum of one before opening), and each transaction shows only confirmation count, date, no description, and a debit/credit of 0.00. When you try to perform any action which attempts to read keypool, you get the "ReserveKeyFromKeyPool() : unknown key in key pool" error.
So, the question is how best to work around Bitcoin's overwillingness to load wallets with keys that it has no clue about.
There were several suggestions of renaming wallet.dat for encrypted wallets. Obviously this has many advantages and disadvantages. It breaks backup scripts, old clients will now create a new wallet instead of using the old one, potentially causing users to (wrongfully) assume their wallet is encrypted if they accidentally start opening an old version. Im not a huge fan of this one, mostly because if a user opens an old version, they will get a blank transactionless wallet which IMO is worse than an odd error message. "My wallet is gone, Ive lost everything, wtf???" vs "My wallet got corrupted, crap need see what I can recover from it, I hope I dont lose much"
Another option is to simply do nothing, and let old clients get mad. If a user goes back to an old client, it cant spend coins using the encrypted keys no matter what is done. If the new client handles multiple key types gracefully, however, it can simply say "Hey, I see you have a mix of key types here, can I have your password to encrypt the unencrypted ones?" and move on with no harm done. IMO, I would much prefer old users see error messages and be unable to use their wallet, then accidentally create multiple wallets, and give them a screen making them think their coins are all gone. Comments?
PS. to prevent this in the future, Bitcoin really shouldn't continue on as if nothing had happened when faced with unknown keys: https://github.com/bitcoin/bitcoin/pull/378
Matt
On Monday, July 04, 2011 1:52:53 PM Matt Corallo wrote:
Quoted text (3 lines)
> There were several suggestions of renaming wallet.dat for encrypted > wallets. Obviously this has many advantages and disadvantages. It > breaks backup scripts,
It shouldn't. Backup scripts should make a copy with the JSON-RPC call.
What about changing the format of wallet.dat to something that triggers an error in the old clients? ie, maybe a dummy crafted-to-make-old-versions- complain file that simply means "use ewallet.dat"?
RE: "You have some unencrypted keys, should I encrypt them for you?"
That re-opens an "attacker packs the keypool with keypairs that they know about" (if I can read/write wallet.dat, then I can delete encrypted keypool keys and insert a bunch of unencrypted keypool keys that I know how to spend, and rely on the user to click "OK" because users are trained to just click "OK").
RE: breaking backup scripts: if they use the backupwallet RPC command, then they will Just Work.
0.4 and later could, on wallet encryption, create a wallet_e.dat (encrypted wallet). Then truncate wallet.dat and set its file-permissions to 000, so if old versions of bitcoin OR any dumb wallet backup scripts try to read it they fail.
RE: future-proofing: wallet.dat contains nFileVersion (version of bitcoin that last wrote the wallet). Adding a nMinVersion that specifies "you must be at least THIS version to read this file" seems like a good idea so if you have version 0.4 or later future wallet upgrades give you a reasonable message if you try to downgrade after an incompatible change.
For some reason my mail client let me respond off-list here, didnt mean to...
On Mon, 2011-07-04 at 14:23 -0400, Gavin Andresen wrote:
Quoted text (7 lines)
> RE: "You have some unencrypted keys, should I encrypt them for you?" > > That re-opens an "attacker packs the keypool with keypairs that they > know about" (if I can read/write wallet.dat, then I can delete > encrypted keypool keys and insert a bunch of unencrypted keypool keys > that I know how to spend, and rely on the user to click "OK" because > users are trained to just click "OK").
Not strictly true, if the keys are loaded, but not added to mapAddressBook or setKeyPool, they wont be used for any new transactions, or shown to the user, but the user is still able to receive Bitcoins to those keys.
Quoted text (2 lines)
> RE: breaking backup scripts: if they use the backupwallet RPC > command, then they will Just Work.
Not really, most backupwallet-based scripts will backup wallet.dat, encrypt wallet.dat, upload wallet.dat. Now it backups up wallet.dat and the encrypt part fails because there is no wallet.dat, only wallet_e.dat. If we rename to wallet.dat on output, now the user's restore might not work...
Quoted text (5 lines)
> > 0.4 and later could, on wallet encryption, create a wallet_e.dat > (encrypted wallet). Then truncate wallet.dat and set its > file-permissions to 000, so if old versions of bitcoin OR any dumb > wallet backup scripts try to read it they fail.
True, but that is only a solution for Linux and Mac and then you are back to unreadable error on Windows load and other unforeseeable errors for odd scripts.
I suppose I just really dont like the idea of renaming wallet.dat, everything knows the filename and is used to it.
Quoted text (7 lines)
> > RE: future-proofing: wallet.dat contains nFileVersion (version of > bitcoin that last wrote the wallet). Adding a nMinVersion that > specifies "you must be at least THIS version to read this file" seems > like a good idea so if you have version 0.4 or later future wallet > upgrades give you a reasonable message if you try to downgrade after > an incompatible change.
Yep, just something simple that says, no reading this to old versions is needed, IMO the older version should freak out if it sees keys that it doesn't know about (as it could also indicate wallet corruption in some rare cases), but nMinVersion works just as well, in any case this should only very rarely be a problem...how often will we change the wallet format?
On Mon, Jul 4, 2011 at 2:23 PM, Gavin Andresen <gavinandresen@gmail•com> wrote:
Quoted text (4 lines)
> 0.4 and later could, on wallet encryption, create a wallet_e.dat > (encrypted wallet). Then truncate wallet.dat and set its > file-permissions to 000, so if old versions of bitcoin OR any dumb > wallet backup scripts try to read it they fail.
[snip]
Rewriting the old one before erasing it and replacing it with a placeholder might increase the chances that the old unencrypted keying material was not left on disk.
On Jul 4, 2011, at 1:23 PM, Gavin Andresen wrote:
Quoted text (2 lines)
> RE: breaking backup scripts: if they use the backupwallet RPC > command, then they will Just Work.
I'd go a step further and say that, since import/export is planned to get merged at about the same time, intentionally breaking unsafe/badly designed backup mechanisms is actually desirable.
On Mon, Jul 4, 2011 at 5:18 PM, Douglas Huff <dhuff@jrbobdobbs•org> wrote:
Quoted text (6 lines)
> > On Jul 4, 2011, at 1:23 PM, Gavin Andresen wrote: >> RE: breaking backup scripts: if they use the backupwallet RPC >> command, then they will Just Work. > > I'd go a step further and say that, since import/export is planned to get merged at about the same time, intentionally breaking unsafe/badly designed backup mechanisms is actually desirable.
Silently breaking them, not so much.
Or do you think people are going to notice that they've started backing up a zero byte file?
Despite this being quick, I really want to get 0.3.24 out and rolling so that we have us much lead time on 0.4 as possible so that any solution to this problem that is better in 0.3.24 can be in as many hands as possible, and so that the network issues can be resolved.
All that needs to happen for that is to agree on either https://github.com/bitcoin/bitcoin/pull/378 or https://github.com/bitcoin/bitcoin/pull/381 thus, I would ask that we get acks on those and then continue this discussion. Frankly, I prefer 378 as it is simpler and means that you dont have to keep track of what features you have or have not used in a wallet, but instead just write and let the keys take care of themselves. However, I'm game for either, I just want to get 0.3.24 out the door ASAP (ie preferably rc2 tomorrow and release by the end of the week).
Matt
On Mon, 2011-07-04 at 22:39 +0200, Matt Corallo wrote:
Quoted text (46 lines)
> For some reason my mail client let me respond off-list here, didnt mean > to... > > On Mon, 2011-07-04 at 14:23 -0400, Gavin Andresen wrote: > > RE: "You have some unencrypted keys, should I encrypt them for you?" > > > > That re-opens an "attacker packs the keypool with keypairs that they > > know about" (if I can read/write wallet.dat, then I can delete > > encrypted keypool keys and insert a bunch of unencrypted keypool keys > > that I know how to spend, and rely on the user to click "OK" because > > users are trained to just click "OK"). > Not strictly true, if the keys are loaded, but not added to > mapAddressBook or setKeyPool, they wont be used for any new > transactions, or shown to the user, but the user is still able to > receive Bitcoins to those keys. > > RE: breaking backup scripts: if they use the backupwallet RPC > > command, then they will Just Work. > Not really, most backupwallet-based scripts will backup wallet.dat, > encrypt wallet.dat, upload wallet.dat. Now it backups up wallet.dat and > the encrypt part fails because there is no wallet.dat, only > wallet_e.dat. If we rename to wallet.dat on output, now the user's > restore might not work... > > > > 0.4 and later could, on wallet encryption, create a wallet_e.dat > > (encrypted wallet). Then truncate wallet.dat and set its > > file-permissions to 000, so if old versions of bitcoin OR any dumb > > wallet backup scripts try to read it they fail. > True, but that is only a solution for Linux and Mac and then you are > back to unreadable error on Windows load and other unforeseeable errors > for odd scripts. > > I suppose I just really dont like the idea of renaming wallet.dat, > everything knows the filename and is used to it. > > > > RE: future-proofing: wallet.dat contains nFileVersion (version of > > bitcoin that last wrote the wallet). Adding a nMinVersion that > > specifies "you must be at least THIS version to read this file" seems > > like a good idea so if you have version 0.4 or later future wallet > > upgrades give you a reasonable message if you try to downgrade after > > an incompatible change. > Yep, just something simple that says, no reading this to old versions is > needed, IMO the older version should freak out if it sees keys that it > doesn't know about (as it could also indicate wallet corruption in some > rare cases), but nMinVersion works just as well, in any case this should > only very rarely be a problem...how often will we change the wallet > format?
Quoted text (2 lines)
> Despite this being quick, I really want to get 0.3.24 out and rolling so > that we have us much lead time on 0.4 as possible
Agreed.
Quoted text (3 lines)
> All that needs to happen for that is to agree on either > https://github.com/bitcoin/bitcoin/pull/378 or > https://github.com/bitcoin/bitcoin/pull/381'
I don't think 0.3.24 "needs" either of those pulls. Fixing downgrade-to-0.3.24 is low on the priority list, because downgrade-to-something-before-0.3.24 is just about as likely, and that has to do something mostly reasonable.
I just pulled https://github.com/bitcoin/bitcoin/pull/379 "Do not use comma as thousands separator", and pulled a block-chain lock-in at block 13444. Those were the only issues I think really need to be in 0.3.24.
On Mon, Jul 4, 2011 at 10:26 PM, Gavin Andresen <gavinandresen@gmail•com> wrote:
Quoted text (8 lines)
>> All that needs to happen for that is to agree on either >> https://github.com/bitcoin/bitcoin/pull/378 or >> https://github.com/bitcoin/bitcoin/pull/381' > > I don't think 0.3.24 "needs" either of those pulls. Fixing > downgrade-to-0.3.24 is low on the priority list, because > downgrade-to-something-before-0.3.24 is just about as likely, and that > has to do something mostly reasonable.
Yeah, same thoughts here.
Quoted text (4 lines)
> I just pulled https://github.com/bitcoin/bitcoin/pull/379 "Do not use > comma as thousands separator", and pulled a block-chain lock-in at > block 13444. Those were the only issues I think really need to be in > 0.3.24.
Tagged -tip as -rc2.
Bug reports are mostly quiet, so maybe we can even get the release before you leave.
On Mon, 2011-07-04 at 22:26 -0400, Gavin Andresen wrote:
Quoted text (4 lines)
> I don't think 0.3.24 "needs" either of those pulls. Fixing > downgrade-to-0.3.24 is low on the priority list, because > downgrade-to-something-before-0.3.24 is just about as likely, and that > has to do something mostly reasonable.
Really, well I disagree but OK, 0.3.24 it is.
At Luke's suggestion, I did a bit more digging and was able to find a data structure in wallet settings that should cause all versions (well all versions since Bitcoin was in github, and probably before then) to crash on load instead of making a new wallet or opening in some bizarre half-state. I just put an empty object in addrIncoming (nfc what it was used for, but it will get the desire effect and it isnt used anywhere in the code aside from its definition). You can see the commit at https://github.com/TheBlueMatt/bitcoin/commit/2e8383469d7e12a495b3a1dbd41a8d211ff34fe8 Does anyone disagree and think a different solution would work better?
This resolves all known issues and suggestions that I know of on newenc except for the invalid mlock calculations, which I will go fix right now. So...aside from that bug does anyone have any remaining suggestions/blockers on newenc and, if not, can we get final ACKs on it?
Matt
On Sun, Jul 10, 2011 at 04:21:17PM +0200, Matt Corallo wrote:
Quoted text (11 lines)
> > At Luke's suggestion, I did a bit more digging and was able to find a > data structure in wallet settings that should cause all versions (well > all versions since Bitcoin was in github, and probably before then) to > crash on load instead of making a new wallet or opening in some bizarre > half-state. I just put an empty object in addrIncoming (nfc what it was > used for, but it will get the desire effect and it isnt used anywhere in > the code aside from its definition). > You can see the commit at > https://github.com/TheBlueMatt/bitcoin/commit/2e8383469d7e12a495b3a1dbd41a8d211ff34fe8 > Does anyone disagree and think a different solution would work better?
Though giving an mostly incomprehensible/unrelated error is never nice to the user, i believe it's better than creating an empty wallet and letting the user wonder where his wallet went. This way, we fail soon and don't ever get a corrupt wallet.
Quoted text (4 lines)
> This resolves all known issues and suggestions that I know of on newenc > except for the invalid mlock calculations, which I will go fix right > now. So...aside from that bug does anyone have any remaining > suggestions/blockers on newenc and, if not, can we get final ACKs on it?
ACK on newenc, and thanks for all the work you put in it already.