Forum thread: http://forum.bitcoin.org/index.php?topic=8728.0

Changed topupkeypool to keypoolrefill at tcatm's suggestion

A note for users of the previous version - Ill write a converter tomorrow that will try to convert the old encrypted wallets to this new format.

TODO List:
Anything marked with //TODO
s/password/passphrase/
write better text for first encryption ie "DONT LOSE THIS PASSWORD" and such
An option to decrypt the wallet? (Decided against this one as noone seemed to care if it got implemented, if others here disagree, please respond)
Figure out why walletphassphrase is lagging (top up keypool?)

Looks OK, I like the feature set, I haven't had a chance to compile and test it.

Looks OK at first glance. Will give it an in-depth look and test RSN.

One nit: mlock() can definitely fail based variable conditions such as ulimit. The code never checks for mlock failure, and seems to assume you may munlock even if mlock failed.

If it fails, it should continue, yes we are potentially not as secure, but we should stop just because we cant mlock(). Also, since munlock() is called implicitly when we deallocate, its unnecessary, I pulled it out.
Oh got Im stupid, sorry, readded that.

I spent a couple days beating the hell out of this implementation and Matt has quickly fixed all the bugs I've encountered.

In particular I've now sent thousands of transactions and thousands of getaddresses in an abusive testing rig which is constantly unlocking/locking/password changing/account moving, etc. and not lost any coin. I've basically run out of obvious automated testing at this point.

I am, however, unable to do GUI testing, so I hope and assume other people are beating on that.

To the best of my ability to determine this is now ready for mainline and testing by a broader audience.

I think that before release the wallettools probably need to be updated to support this: both as an additional validation of the implementation, but also so that there isn't a gap in recovery tools, but I don't see any reason that should block mainlining it.

Just tried to build 499334e on win32 using mingw, works fine, except a couple of things.

1. In crypter.cpp you have to move headers.h to the very top otherwise you'll get a whole bunch of "Cannot convert from 'const wchar_t *' to '_TCHAR *'" (more precisely, wx.h should be included before windows.h).
2. Cancel button in the encryption dialog does not work as expected (buggy wxGetPasswordFromUser).
   P.S. Also note encryption can't be undone yet, so don't try it on your wallets. )

Just figured out if you're trying to read encrypted wallet with official client (0.3.24rc1 in my case) wallet gets destroyed and not readable by anything. It's not very nice.

If you try to read it with the old client, the client won't (shouldn't) touch it and just complain that it is "Unable to read wallet.dat". However, if you reopen it with this client, it should open fine.
Also, thanks for the mingw heads-up, I suppose I must've changed the header order at some point after I tested on mingw...
Oh well, should be fixed now.

Well, not really. Official 0.3.23 says basically the same as 0.3.24rc1: EXCEPTION: St13runtime_error ReserveKeyFromKeyPool() : unknown key in key pool (...)bitcoin.exe in AppInit(), EXCEPTION: St13runtime_error ReserveKeyFromKeyPool() : unknown key in key pool (...)bitcoin.exe in CMyApp::OnUnhandledException(), Runtime error!

After that wallet.dat becomes completely inaccesible, 499334e says "Error loading wallet.dat". If someone is interested I've tried that only on a new wallet (with no transactions).

Ah, shit you are totally right, and here I was thinking the client was smarter than that...Ill make sure a fix goes into 0.3.24 so at least people can downgrade to that safely.
Oh, forgot to mention, I changed master key format last night, so wallet encrypted with this previously will not load.

See #378

would it be possible to encrypt the wallet with a null/default pass-phrase by default rather than leaving the wallet unencrypted?

null/default passphrase is 100% identical to no encryption, if an attacker gets the wallet, they can still just dump it in their .bitcoin folder and send coins, its better to be backward compatible if users dont encrypt than just break that too for nothing.

I am not sure the mlock() wrappers are correct. ((void *)(((size_t)(a)) & ((size_t)(((PAGESIZE)<<1)-1)))) seems to throw away the high bits of the address, instead of the low ones.

I suggest using:

mlock((void*)((size_t)a & ~(PAGESIZE - 1)), ((((size_t)a + b - 1) | (PAGESIZE - 1)) + 1) - ((size_t)a & ~(PAGESIZE - 1)));

Here is a test program:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <assert.h>

#define PAGESIZE 4096

int my_mlock(void *a, size_t b)
{
    void *start = (void*)((size_t)a & ~(PAGESIZE - 1));
    void *stop  = (void*)((((size_t)a + b - 1) | (PAGESIZE - 1)) + 1);
    size_t range = stop - start;
    assert(start <= a);
    assert(start+range >= (a+b));
    assert((((size_t)start) & (PAGESIZE-1)) == 0);
    assert((((size_t)start+range) & (PAGESIZE-1)) == 0);
    // return mlock(start, range);
    return 0;
}

int main(void)
{
    size_t as[] = {0x400000, 0x3FFFFF, 0x3FFF00, 0x400100};
    size_t bs[] = {0,1,2,3,4,7,15,127,128,129,255,256,257,4000,4095,4096,4097,4100,4200,8000,8190,8300};
    for (int na=0; na<sizeof(as)/sizeof(as[0]); na++) {
      for (int nb=0; nb<sizeof(bs)/sizeof(bs[0]); nb++) {
        my_mlock((void*)(as[na]),bs[nb]);
      }
    }
    return 0;
}

Apart from the above comment: ACK.

re gavin's comment
"This needs a comment explaining what it is for-- "This class writes an addrIncoming entry that causes pre-0.4 clients to crash on startup if reading a private-key-encrypted wallet.""
done.

ACK, looks ready-for-prime-time to me.

Unit tests?

Why the unencrypted master key is stored in memory? Why not unencrypted private key？