[Bitcoin-development] Encrypted Wallet Backward Compatibility

[Bitcoin-development] Encrypted Wallet Backward Compatibility

[Matt Corallo]

[-- Attachment #1: Type: text/plain, Size: 2395 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Bitcoin-development] Encrypted Wallet Backward Compatibility

[Luke-Jr]

On Monday, July 04, 2011 1:52:53 PM Matt Corallo wrote:
> 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: [Bitcoin-development] Encrypted Wallet Backward Compatibility

[Gavin Andresen]

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.

-- 
--
Gavin Andresen

Re: [Bitcoin-development] Encrypted Wallet Backward Compatibility

[Matt Corallo]

[-- Attachment #1: Type: text/plain, Size: 2456 bytes --]

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?

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Bitcoin-development] Encrypted Wallet Backward Compatibility

[Gregory Maxwell]

On Mon, Jul 4, 2011 at 2:23 PM, Gavin Andresen <gavinandresen@gmail.com> wrote:
> 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.

Re: [Bitcoin-development] Encrypted Wallet Backward Compatibility

[Douglas Huff]

[-- Attachment #1: Type: text/plain, Size: 377 bytes --]


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.

-- 
Douglas Huff



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 881 bytes --]

Re: [Bitcoin-development] Encrypted Wallet Backward Compatibility

[Gregory Maxwell]

On Mon, Jul 4, 2011 at 5:18 PM, Douglas Huff <dhuff@jrbobdobbs.org> wrote:
>
> 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?

Re: [Bitcoin-development] Encrypted Wallet Backward Compatibility

[Matt Corallo]

[-- Attachment #1: Type: text/plain, Size: 3469 bytes --]

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:
> 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?

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Bitcoin-development] Encrypted Wallet Backward Compatibility

[Gavin Andresen]

> 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.

> 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.

-- 
--
Gavin Andresen

Re: [Bitcoin-development] Encrypted Wallet Backward Compatibility

[Jeff Garzik]

On Mon, Jul 4, 2011 at 10:26 PM, Gavin Andresen <gavinandresen@gmail.com> wrote:
>> 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.

> 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.

-- 
Jeff Garzik
exMULTI, Inc.
jgarzik@exmulti.com

Re: [Bitcoin-development] Encrypted Wallet Backward Compatibility

[Matt Corallo]

[-- Attachment #1: Type: text/plain, Size: 346 bytes --]

On Mon, 2011-07-04 at 22:26 -0400, Gavin Andresen wrote:
> 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.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Bitcoin-development] Encrypted Wallet Backward Compatibility

[Matt Corallo]

[-- Attachment #1: Type: text/plain, Size: 953 bytes --]


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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Bitcoin-development] Encrypted Wallet Backward Compatibility

[Pieter Wuille]

On Sun, Jul 10, 2011 at 04:21:17PM +0200, Matt Corallo wrote:
> 
> 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.

> 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.

-- 
Pieter