Wallet and key import and export

[sipa]

Introduces four new RPC calls:

• dumpprivkey: retrieve the private key corresponding to an address
• importprivkey: add a private key to your wallet
• removeprivkey: remove a private key from your wallet
• dumpwallet: export the contents of your wallet in various ways
• importwallet: import/merge a dumped wallet into your own.

The private key format is analoguous to the address format. It is
a 51-character base58-encoded string, that includes a version number
and a checksum.

The wallet format is JSON-based, and includes:

• addresses (informational, optional)
• private keys
• amounts per balance (informational, optional)
• blocks where addresses were first used (optional)
• ids of transactions in which addresses were credited (optional)
• labels (optional)

It specifically does not contain:

• sender address book
• settings
• account information
• unconfirmed wallet transactions

Note that playing around with import/export and moving addresses around between wallets may result in double-spends, which may result in corrupted wallets. See #195 for a solution

[gavinandresen]

Nice! I'll have to find some time to try to break it...
rpc.cpp is getting way too big; maybe this would be a good time to start breaking it up into separate .cpp files with related functions (most of this code could be rpcimport.cpp, maybe....)

[rnavarro]

Another feature that would be really really handy would be the ability to specify an account for the importprivkey command

[sipa]

Discussion is here: http://forum.bitcoin.org/index.php?topic=8091.0

[jgarzik]

I think this is useful for wallet security (print out a private key, stick in safety deposit box), so prioritizing a rebase for this would be appreciated.

[sipa]

Rebased against master, a few bugs fixed, and split off into separate source file.

TODO: known wrong balance cached when importing keys

[EricJ2190]

Since this uses the ifstream class, it needs to include fstream in headers.h to build in MSVC.

[sipa]

I've rebased the code against Matt's newenc, plus some other refactorings. I'll close the pull request until those are merged.

[sipa]

Ok, reopened because github doesn't seem to track updates to the branch while the pullreq is closed, but this still depends on #403. Also, i'm not convinced how stable the wallet dump format is yet, so I've split the last commit in two, allowing merging of only key import/export.

[lachesis]

Didn't build for me:
wallet.cpp:273:65: error: ‘PubKeyToAddress’ was not declared in this scope

[sipa]

@lachesis: should be fixed

[gmaxwell]

I noticed some weird behavior with transactions heard over the network on imported keys:

{
    "account" : "",
    "address" : "1Wh4bh",
    "category" : "send",
    "amount" : -0.49500000,
    "fee" : -0.00500000,
    "confirmations" : 0,
    "txid" : "275d51ed81ae359cffe3728f6c7de8246b2b3c147b9dce316a5e952db5d76991",
    "time" : 1310993846
}

Kind of a short address there… :) (from a checkout this morning)

[gmaxwell]

It's really slow to import addresses one at a time— giving a rescan for each. If someone feels adventurous, deferred lazy rescan in a thread, and/or an import that call that takes many would be nice.

[sipa]

Rebased against master, and now includes a feature-complete 'removeprivkey' call (should remove the private key, the address book entry, redundant transactions, and account information). I don't have time to test this the coming weeks, but if people want to try it, be my guest.

[sipa]

rebased against 0.4.0rc1, not tested after rebase

[gmaxwell]

I think the existence of http://www.bitaddress.org/ is a strong argument against this functionality— the feature isn't done yet and people are already creating websites which compromise bitcoin user's security.

Official import/export functionality will increase the apparent legitimacy of attacks exploiting the functionality and bitcoin will suffer the bad PR when users hurt themselves using it. :-/

[casascius]

@gmaxwell: What part of this functionality constitutes an exploit? The idea that one should be able to have their own private keys on paper, or on bitbills, or on physical bitcoins, etc. should be something that enhances security, not detracts from it. Please explain how security is compromised.

[casascius]

I would like to request an addition here to support the Mini Private Key format as described in the Wiki. Basically, importing using this format would be exactly the same as the existing (51-character) string, but a 22-character mini private key could be provided instead. This would probably fit right here as a different SetSecret that took the string, and ought to take no more than a dozen lines of code. These codes are being used today in Casascius physical bitcoins. The 22-character code need not be persisted in any way - it ought to simply be converted to a normal instance of CBitcoinSecret immediately upon entry, so, these would be displayed in the normal 51-character format if they were exported or dumped.

The mini private key is simple. The Bitcoin private key is computed as the SHA256 hash of the given string. No more, no less. A shorter private key is universally desirable both in QR codes and in situations where the private key must be manually typed.

The mini private key format offers a simple well-formedness check and also a hash-based check for typo detection, which is designed to be briefly computationally intensive (requires multiple rounds of SHA256) so as to not detract from the limited entropy that can fit in 22 characters in a brute-force attack scenario. It is described in the Wiki article. Only 22-character strings that meet the well-formedness and typo check criteria should need to be considered for evaluation as importable private keys.

[enmaku]

@sipa: Nice mod!
@gavinandresen re: testing: Works beautifully on Debian squeeze i686. All added functionality tested and working properly.
@casascius: +1 for adding Mini Private Key format. It should be pretty easy to implement. Also, I believe the problem @gmaxwell was addressing is that scammers can generate "secure" keypairs for their victims and then never relinquish control of the private key, thus allowing them to steal funds from those accounts later

[casascius]

@enmaku,@gmaxwell: I wasn't sure if keeping the private key was the concern here, because bitaddress.org appears to have been created with that concern in mind. The generator at bitaddress.org is actually quite novel from a security perspective: the generation is entirely client-side and implemented in javascript in easy-to-read code in a single self-contained html file that contains no references to any network resources. The QR codes are generated from scratch, client-side, as well. This is exactly how a personal address generator should be. One can save the single .html file to disk and use it to safely generate bitcoin addresses straight to their printer while disconnected from the internet. To me, it's a shining example of a good idea rather than an exploit.

[enmaku]

@casascius: I think the concern is that while you and I might check the source, 99% of folks won't and eventually one will crop up that phones home and makes a big database of privkeys ripe for exploitation. Realistically though this isn't our problem any more than any other product or service whose userbase is regularly phished. We can put the info out there, but people want this feature and if it's not in the main client it'll be in external tools like pywallet. We can't stop those who want to from importing addresses, merging wallets, etc. The best we can do is put up big scary popups in the GUI implementation informing folks that messing with the wallet is dangerous and if they do stupid things they'll get bad results.

[casascius]

@enmaku: I share the concern, and am appreciative of the fact that the author of this website makes no attempt to explain what his website does to newbies. You have to know why you're there and what the number means, otherwise it is just a gibberish generator. And the part I appreciate the most is simply that he has released functional code that can be clipped and put into other environments where the source is ostensibly trusted. (e.g. on MtGox, or on the LinuxCoin ISO). Free bonus is it's platform independent.

I think the best thing that can be done to the GUI implementation would be to include this functionality where the user can get at it, and seek to be less user unfriendly, then websites like this wouldn't have a reason to exist.

[casascius]

Here is some code I wrote - AND TESTED =) - that enables full support for mini private key imports, including proper rejection of codes failing the typo check. Regular 51-character private keys still work like normal. Simply replace, removing 5 lines starting from "string strLabel = "";" thru "bool fGood = vchSecret..." and inserting the following. Sorry I am new to developing with git, learning fast, but am unfamiliar with whether there is a way I can submit a "pull on a pull", and sorry if there is a much better way to submit this.

Edit: updated it to make sure removeprivkey benefited from the change as well, as the initial post had the new code right inside importprivkey and only benefited that command.

Add the following function to rpcdump.cpp:

// Sets the private key of a CBitcoinSecret, detecting whether it's a mini or full private key
bool SetSecretDetect(string &strSecret, CBitcoinSecret &secret) {
    int nSecretLength = strSecret.size();
    if (nSecretLength == 22 || nSecretLength == 26)
    {
        if (strSecret[0] == 'S')
        {
            int i;
            bool fMini = false;
            for (i = 0; i < nSecretLength; i++)
            {
                char c = strSecret[i];
                if (c < '1' || c > 'z') break;
                if (c > '9' && c < 'A') break;
                if (c > 'Z' && c < 'a') break;
                if (c == 'I' || c == 'l' || c == 'O') break;
            }
            if (i==nSecretLength)
            {
                string strKeycheck(strSecret);
                strKeycheck += "?";
                uint256 hash;
                SHA256((unsigned char*)strKeycheck.c_str(), strKeycheck.size(), (unsigned char*)&hash);
                if (*(hash.begin()) == 0)
                    fMini=true;
                else
                {
                    uint256 hash2;
                    for (i=0; i<358; i++) // 358*2=716 +1=717
                    {
                        SHA256((unsigned char*)BEGIN(hash), sizeof(hash), (unsigned char*)&hash2);
                        SHA256((unsigned char*)BEGIN(hash2), sizeof(hash2), (unsigned char*)&hash);
                    }
                    if (*(hash.begin()) == 0) fMini = true;
                }
                if (fMini)
                {
                    uint256 hash;
                    SHA256((unsigned char*)strSecret.c_str(), nSecretLength, (unsigned char*)&hash);
                    secret.SetHash256(hash);
                    return true;
                }
            }
        }
    }
    return secret.SetString(strSecret);
}

..... then in importprivkey() and removeprivkey() find the following line: .....

bool fGood = vchSecret.SetString(strSecret);

..... and replace it with .....

bool fGood = SetSecretDetect(strSecret, vchSecret);

..... and then in base58.h in class CBitcoinSecret (public), I added, just after SetSecret(): ......

void SetHash256(const uint256& hash256)
{
    SetData(fTestNet ? 239 : 128, &hash256, 32);
}

[sipa]

I'm not opposed to adding other private key formats if they are useful, but I don't think just any format should be added to the main client (as opposed to eg. shipping a small script to do the conversion). That said, the mini private key format sounds useful. What do other people think about supporting this format?

Second, importprivkey is the wrong place to implement it, as CBitcoinSecrets are used in other places as well that could benefit from supporting it. I think either inside the constructor of CBitcoinSecret, or as a separate utility function that does the conversion.

[casascius]

I had originally penciled it in to base58.h but it threw a fit seemingly because I had created a dependency on "string" within the header file that wasn't resolved at compile time with the order it was included, and since this was my first venture into the code (and I haven't used the STL much recently, I'll bet vector<char> might have gotten me further), I was not quite sure how to make it work without messing with other files. If you have a recommendation on how you think it should be implemented, I'd be happy to make it that way and test it.

[sipa]

The std namespace is not included in header files, to prevent namespace pollution. Use std::string and std::vector there instead of string and vector.

[casascius]

Excellent suggestion, I will do that. (Sorry, C++ semantics rusty for me, otherwise that's something that should otherwise be clear).

My intent is to just make sure that the CBitcoinSecret has a SetString override that can handle either kind of key. (I believe the base class already has a SetString). I can do it in the constructor, but as far as I can tell, that will require a new member (a flag) or an exception thrown back to report back to the caller whether the string was any good. Suggest to me which you'd prefer, and I will do it that way and test it.

[casascius]

One more thing... the bigger the block chain gets, the longer importprivkey takes to run, mainly because of the calls to rescan the wallet. I want this in the main client badly because I use it all the time and believe that keys on paper will help solve the black eye Bitcoin has received over security incidents, but it occurs to me that there is no way that this would be taken seriously as acceptable in the main client if it needs to pause for 90+ seconds (and growing) to perform an import. The client itself needs badly to maintain an index for the purpose of doing that rescanning (if it doesn't do so already), among other useful purposes I can think of.

[sipa]

Keeping a database of all addresses and all outputs to them would be a massive cost that isn't justified for just somewhat faster importing of addresses. Take into account that when more complex scripts are supported, this becomes even harder, as there could be outputs with several possible addresses or even combinations thereof.

The solution is using importwallet instead of importprivkey. It supports annotating keys with information about which transactions or starting in which block they are used, and only doing selective rescanning.

[casascius]

With respect to the massive database of addresses... I could see myself trying to implement that down the road as my familiarity with the code gets better, but possibly in the form of a patch or an option that is turned off by most people because it's only useful to a specific audience: server operators.

The specific real-world everyday application it would support would be so that websites (e.g. MtGox, or marketplaces that accept bitcoin deposits) could accept a "deposit by typed private key", and the private key wouldn't necessarily be imported, but rather, bitcoind would do one of two things:

either, one - immediately create and broadcast a one-off transaction (without adding the key to the wallet like importprivkey does) that sent the entire balance at that address to a specific one given as a parameter, at which point it would be treated like a deposit that came in externally (with the option of sending only a specific amount, the rest, if any, going back to the original address or to an optionally provided but specific "change" address)...

• or - two, offer an RPC query feature that simply coughed up all of the unspent transactions associated with a given address, so that some sort of RPC client could implement the creation of that transaction (a feature I believe is useful in itself for other purposes that also wouldn't apply to everybody)

Such a feature would depend on the ability to quickly find all transactions (standard ones, at least) given an address, and would promote the usability of paper bitcoin wallets and physical bearer items, which in turn is a viable way for joe schmoe and/or Grandpa to securely manage his bitcoins without needing to learn about wallet encryption and keyloggers. If bitcoind is tied up for a minute and a half to import an address, this will be impossible in a MtGox-like server scenario.

[casascius]

OK, here are the goods. Revised mini private key support that consists of adding two SetSecret functions to CBitcoinSecret to override the ones in its base class and no modifications anywhere else. I suppose this is a lot cleaner, and it requires only modifying base58.h and not rpcdump.cpp.

bool SetString(const std::string& strAddress) 
{
    return SetString(strAddress.c_str());
}


bool SetString(const char *psz) 
{
    int nSecretLength = strlen(psz);
    if (nSecretLength == 22 || nSecretLength == 26)
    {
        if (psz[0] == 'S')
        {
            int i;
            bool fMini = false;
            for (i = 0; i < nSecretLength; i++)
            {
                char c = psz[i];
                if (c < '1' || c > 'z') break;
                if (c > '9' && c < 'A') break;
                if (c > 'Z' && c < 'a') break;
                if (c == 'I' || c == 'l' || c == 'O') break;
            }
            if (i==nSecretLength)
            {
                std::string strKeycheck(psz);
                strKeycheck += "?";
                uint256 hash;
                SHA256((unsigned char*)strKeycheck.c_str(), strKeycheck.size(), (unsigned char*)&hash);
                if (*(hash.begin()) == 0)
                    fMini=true;
                else
                {
                    uint256 hash2;
                    for (i=0; i<358; i++) // 358*2=716 +1=717
                    {
                        SHA256((unsigned char*)BEGIN(hash), sizeof(hash), (unsigned char*)&hash2);
                        SHA256((unsigned char*)BEGIN(hash2), sizeof(hash2), (unsigned char*)&hash);
                    }
                    if (*(hash.begin()) == 0) fMini = true;
                }
                if (fMini)
                {
                    uint256 hash;
                    SHA256((unsigned char*)psz, nSecretLength, (unsigned char*)&hash);
                    SetData(fTestNet ? 239 : 128, &hash, 32);

                    return true;
                }
            }
        }
    }
    return CBase58Data::SetString(psz);

}

[sipa]

What do other people think about support for @casascius' mini private keys?

[enmaku]

I think it could be a useful standard for short keys, though as a contributor on casascius' deterministic wallet generator I may be a bit biased ;)

[gmaxwell]

The ec multiply is just slow on our existing cpu implementations. Gate count wise it's much faster than sha256. (of course, you'll still also need 1x sha256+ripemd160 to get an address)

I think you've missed the point on the birthday paradox issue: I'm not proposing that this somehow makes it easier to find the private key from the address or anything like that. There are N=58^21 possible keys (minus the ones lost to checking), there are M being used. These are the only figures you need to know to figure out the probability of two identical keys coming into existence by chance. When M=sqrt(N) it's 50%. The interior operations are as irrelevant as the 86400 seconds in a day are to the traditional birthday paradox example. :)

For your use this level of security may be adequate. But if this is put in the software it will be used for other things, and I don't believe it is adequate for very many things. (If you'd reached 2^160 I'd have nothing to complain about).

I don't follow why you did the either/or check (I'd correctly used the 7 bit number initially, but sipa corrected me on IRC doh, not that I'm also not guilty of misreading the code). Why isn't this sha256^n(key)[0]&127==0 ? The early check reduces the security of the strengthening (and makes analysis harder) without providing a substantial speedup.

Generating these keys in javascript is a bad idea. The current browsers JS does not provide an appropriate environment for security-important operations. The security of the official bitcoin software and its standard feature set should not be compromised by performance considerations related to poor performance in things that are a bad idea to begin with.

[casascius]

@sipa: If you calculated half of all the minikeys and ignored all the ones that needed 717 rounds to generate, you'd only victimize those who elected to use the ones that passed on the first round. Those who follow the best practice of only using only the ones that pass on round 717, and not on round 1, where able, are not going to be affected. On the other hand, if you're a javascript and you need to quickly generate an address to collect a payment that's going to get swept to another wallet right away anyway, the option for a 1 round key comes in handy.

Supporting the import of these private keys in the client doesn't in and of itself encourage their unnecessary use. The average joe user isn't going to be making the decision as to which kind of key to use, these decisions are presumably going to be made by developers who would have the intellectual means to weigh whether generating minikeys versus full keys in their app is a good design decision. I think it would be irresponsible to put a "generate minikey" button in the client. But I don't think it serves any useful purpose to not accept them as imports and instead demand the user use some other utility to get the job he already needs done, done. (Having him find and download the other utility exposes him to a new class of risks by itself, far more frightening than a brute force attack on the key space).

[casascius]

@gmaxwell: I used the "either or" check so that there would be a means to generate a crappy key if you only need it for a short time for a crappy purpose. In other words, if you have the means to select a key that passes the 717 check and not the 1st check, then by all means do that. That means, to generate a "better" key, explicitly discard all the candidates that pass on the 1st check.

I agree with you that javascript is a poor environment to be generating keys, and I mention it only for ease of illustration. Minikeys could be needed in the context of an interpreted language on a server, or on a microcontroller. Imagine a check cashing shop that sells BTC and uses a VeriFone point-of-sale credit card machine to generate and issue a minicode on a piece of receipt tape - a very plausible application. That little box with its 400 MHz underclocked ARM and 2MB memory might not be able to do 180000 SHA256's in any short order, and 95% says the dude buying it is probably going to redeem it that same day, so the window of opportunity for a thief is vanishingly short.

On the other hand, let's kick around as granted the conclusion that this is a bad idea. What do we want Joe Sixpack to do instead when he acquires a private key given to him on paper? We could make him type 51 characters instead of 22, sure. We could make him type 26 or 30 as well. We could make him scan a QR code, but it's likely he either has nothing to scan it with, or is almost certainly going to have to hand-key it off his cell phone and into his computer, making the QR code of no benefit and nothing more than an extra burdensome step. I suppose it's fair to say that a minimum length keycode is universally desirable, and it's really just where to draw the line of being reckless with security.

Let me also propose an alternate solution. The key space for my proposal is all of those who pass the SHA256 check with the magic byte "00". What if, in addition to this, we define another key space, and that's all of those 22-character base58 codes who fail my check but pass with magic byte "01". And then we utilize a far more computationally intensive key derivation function to compute the 32-bytes in place of SHA256, that dwarfs both SHA256 and EC in resource complexity, and define that as the best practice for generating such short keycodes. And then still leave 717 and even 1-round SHA256 as a weak option so that microcontrollers can still stay in the game where needed.

[casascius]

Example: use PBKDF2 whenever SHA256(code + '?')[0]==0x01, and further, use the next byte (sha256()[1]) to encode the number of iterations needed to derive the key, exponentially (2^n), so that its difficulty is easily scaled (or 2^(n/4) to increase the useful range of choices). So a code whose sha256(code+'?') starts with 0x01 0x28 would derive the key with 1024 (2^(40/4)) rounds, and one whose check hash starts with 0x01 0x60 would use 16777216 rounds (2^(96/4)). An upper bound would have to be hardcoded to prevent CPU DoS attacks on servers that accept redemptions, which would be increased by consensus as computing power advanced.

[casascius]

OK, there it is. Proof of concept with PBKDF2, at least, at casascius/bitcoin. For testing purposes, I have commented out the wallet scanning, so all the delay experienced can be attributed to PBKDF2 and not the wallet scanning.

I also got rid of the 717-round stuff. I haven't used any such codes on physical bitcoins so it can be scrapped right now.

So my submission either accepts a 1-round SHA256 if the test hash starts with 0x00 (as used on physical coins), and uses PBKDF2 if the test hash starts with 0x0100 thru 0x0150 (rejecting all else).

Using 0x0150 does PBKDF2 with 1048576 iterations, and it takes my machine about six seconds to derive the key at that rate. Using 0x0128 uses 1024 iterations, and the delay is imperceptible (to me as the user). If accepted, the hardcoded limit should probably be somewhat lower to avoid DoS's.

Here are some test keys.
Sksec8N4Pb9x3EmKChBq9c - ?=0x0128 - calls for 1024 iterations
SmbM4uRBu2mQymCsuMKkiW - ?=0x0130 - calls for 4096 iterations
Sou6jrw7YW8oeQjXeS8J5u - ?=0x0138 - calls for 16384 iterations
SU6ppdqUTS3KWcnZuFxebf - ?=0x0140 - calls for 65536 iterations
STk4hKyfJdXdZ2cZG3VByy - ?=0x0148 - calls for 262144 iterations
SmEVr24rSdaUgnmX8LwpeM - ?=0x0150 - calls for 1048576 iterations
S172quV4MT1ESiS5REfFQx - ?=0x0151 - will get rejected due to being past current hardcoded limit.

EDIT: I previously had a brainfart and put 16777216 instead of 1048576 iterations. 1048576 is the correct number.

[T-X]

@sipa: "What do other people think about support for @casascius' mini private keys?"

Sounds like an extra feature which will need extra attention. Maybe it should be added as an additional commit on top later and should be further discussed under a separate pull request after the "straightforward" 51-character base58-encoded import/export version got added?

[runeksvendsen]

EDIT: I got rid of the below error by adding an include in bitcoinrpc.cpp for boost/filesystem.hpp

I get the following error when I try to build the showwallet branch with gcc 4.6:

g++ -c -pthread -Wno-invalid-offsetof -Wformat -g -DNOPCH  -DUSE_SSL -fno-stack-protector -fstack-protector-all -Wstack-protector -Wl,-z,relro -Wl,-z,now -D_FORTIFY_SOURCE=2 -O2 -MMD -o obj/nogui/bitcoinrpc.o bitcoinrpc.cpp
bitcoinrpc.cpp: In function 'void ThreadRPCServer2(void*)':
bitcoinrpc.cpp:2213:9: error: 'filesystem' has not been declared
bitcoinrpc.cpp:2213:26: error: expected ';' before 'certfile'
bitcoinrpc.cpp:2214:14: error: 'certfile' was not declared in this scope
bitcoinrpc.cpp:2214:49: error: 'filesystem' has not been declared
bitcoinrpc.cpp:2215:13: error: 'filesystem' has not been declared
bitcoinrpc.cpp:2215:32: error: 'certfile' was not declared in this scope
bitcoinrpc.cpp:2217:9: error: 'filesystem' has not been declared
bitcoinrpc.cpp:2217:26: error: expected ';' before 'pkfile'
bitcoinrpc.cpp:2218:14: error: 'pkfile' was not declared in this scope
bitcoinrpc.cpp:2218:45: error: 'filesystem' has not been declared
bitcoinrpc.cpp:2219:13: error: 'filesystem' has not been declared
bitcoinrpc.cpp:2219:32: error: 'pkfile' was not declared in this scope
make: *** [obj/nogui/bitcoinrpc.o] Error 1

I'm using gcc 4.6.1, libc6 2.13 and libboost 1.46 from Debian sid (the Linuxcoin distribution). I have tried with gcc 4.5 as well, same error.
The regular bitcoin branch builds without issues.

[sipa]

Thanks for reporting - an include was missing. It should be fixed now.

[runeksvendsen]

Thank you for adding this feature. I just tried importing a private key using this version of bitcoind on an offline computer, running Linuxcoin from a USB stick. I am now back in my everyday OS, Ubuntu, and everything seems to have worked fine - and I received no errors when importing the key, although it took a while.
So consider it a thumbs up for the importprivkey command from here!

[gavinandresen]

I just pulled the other dumpprivkey; is this one now obsolete?

[sipa]

No, this one still contains removeprivkey and dumpwallet/importwallet. I've rebased them against current master now, but I'll close the request until it's more clear what dumpwallet/importwallet should do (there's talk of including addresses, for example).

[sipa]

closing