Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Significantly Improve Wallet Load times #2733

Closed
wants to merge 2 commits into from
Closed

Significantly Improve Wallet Load times #2733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5ba77b
improve wallet load time by removing duplicated checks in EC_KEY_chec…
pstratem Jun 3, 2013
ab139c9
Merge branch 'master' of git://github.com/bitcoin/bitcoin into keyfix
pstratem Jun 3, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 68 additions & 3 deletions src/key.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,13 @@
#include <openssl/ecdsa.h>
#include <openssl/rand.h>
#include <openssl/obj_mac.h>
#include <openssl/err.h>
#include <openssl/asn1t.h>
#include <openssl/objects.h>
#include <openssl/evp.h>

#include "key.h"

#include "util.h"

// anonymous namespace with local implementation code (OpenSSL interaction)
namespace {
Expand Down Expand Up @@ -164,13 +168,56 @@ class CECKey {
assert(nSize == nSize2);
}

bool SetPrivKey(const CPrivKey &privkey) {
bool SetPrivKey(const CPrivKey &privkey, bool fSkipCheck=false) {
const unsigned char* pbegin = &privkey[0];
if (d2i_ECPrivateKey(&pkey, &pbegin, privkey.size())) {
// d2i_ECPrivateKey returns true if parsing succeeds.
// This doesn't necessarily mean the key is valid.
if (EC_KEY_check_key(pkey))

if (fSkipCheck)
return true;

const EC_GROUP *group = EC_KEY_get0_group(pkey);
const EC_POINT *pub_key = EC_KEY_get0_public_key(pkey);
const BIGNUM *priv_key = EC_KEY_get0_private_key(pkey);

BN_CTX *ctx = NULL;
BIGNUM *order = NULL;
EC_POINT *point = NULL;

if (!pkey || !group || !pub_key || !priv_key)
goto err;

order = BN_new();

if ((ctx = BN_CTX_new()) == NULL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like ctx and point are memory leaked in the return true case...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I need my glasses or is that a "goto" statement?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"goto" use is just fine. It is efficient for both the compiler and human eyes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, goto is not okay.
http://stackoverflow.com/questions/46586/goto-still-considered-harmful

It's true that there are exceedingly rare cases where a goto is useful but this is not one of them. Especially this case, error handling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is code that I assume was copied from the OpenSSL sources, which uses such error handling extensively. Without RAII-like features (as it was copied from C), I also know of no less contrived way of dealing with this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Goto is fine in C code (in many cases the only sane way to do error
handling with proper cleanup), but in C++ we can indeed do better with RAII.
Using boost would be preferable to rolling our own RAII wrappers.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a c++ guy, but I believe there are RuntimeExceptions, right? And maybe they allows to pass description of error. It would be much better than to use goto.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than creating 90 Lines of RAII wrappers, You could probably just rewrite it like this:

bool SetPrivKeyCleanup(BN_CTX *ctx,EC_POINT *point)
{
 if (ctx   != NULL)
  BN_CTX_free(ctx);
 if (point != NULL)
  EC_POINT_free(point);
 return false;
}

Then just ammend:

            if (!pkey || !group || !pub_key || !priv_key)
                return SetPrivKeyCleanup(ctx,point);

            order = BN_new();

            if ((ctx = BN_CTX_new()) == NULL)
                return SetPrivKeyCleanup(ctx,point);

            if (!EC_GROUP_get_order(group, order, ctx))
                return SetPrivKeyCleanup(ctx,point);

            // check if generator * priv_key == pub_key 
            if (BN_cmp(priv_key, order) >= 0)
                return SetPrivKeyCleanup(ctx,point);

            if ((point = EC_POINT_new(group)) == NULL)
                return SetPrivKeyCleanup(ctx,point);

            if (!EC_POINT_mul(group, point, priv_key, NULL, NULL, ctx))
                return SetPrivKeyCleanup(ctx,point);

            if (EC_POINT_cmp(group, point, pub_key, ctx) != 0)
                return SetPrivKeyCleanup(ctx,point);

            return true;

Since I didn't test this there is probably a good reason why it wouldn't work, but you get the basic idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still prefer RAII. Make the compiler do the work of making it leak proof.
Safer and more convenient.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed making an EC_POINT class that implements EC_POINT_new in the constructor and EC_POINT_free in the destructor, as well as operator overloads for EC_POINT_mul and EC_POINT_cmp, is much more in the spirit of C++. I was really just addressing the comment:

Do I need my glasses or is that a "goto" statement?

Its not really an issue worth making a fuss about really. Perhaps I should have kept quiet.

goto err;

if (!EC_GROUP_get_order(group, order, ctx))
goto err;

// check if generator * priv_key == pub_key
if (BN_cmp(priv_key, order) >= 0)
goto err;

if ((point = EC_POINT_new(group)) == NULL)
goto err;

if (!EC_POINT_mul(group, point, priv_key, NULL, NULL, ctx))
goto err;

if (EC_POINT_cmp(group, point, pub_key, ctx) != 0)
goto err;

return true;

err:
if (ctx != NULL)
BN_CTX_free(ctx);
if (point != NULL)
EC_POINT_free(point);
return false;

}
return false;
}
Expand Down Expand Up @@ -393,3 +440,21 @@ bool CPubKey::Decompress() {
key.GetPubKey(*this, false);
return true;
}

bool CKey::Load(CPrivKey &privkey, CPubKey &vchPubKey, bool fSkipCheck=false) {
CECKey key;
if (!key.SetPrivKey(privkey, fSkipCheck))
return false;

key.GetSecretBytes(vch);
fCompressed = vchPubKey.IsCompressed();
fValid = true;

// TODO maybe this can be skipped also
CPubKey pubkey;
key.GetPubKey(pubkey, fCompressed);
if (pubkey != vchPubKey)
return false;

return true;
}
3 changes: 3 additions & 0 deletions src/key.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,9 @@ class CKey {
// 0x1D = second key with even y, 0x1E = second key with odd y,
// add 0x04 for compressed keys.
bool SignCompact(const uint256 &hash, std::vector<unsigned char>& vchSig) const;

// Load private key and check that public key matches.
bool Load(CPrivKey &privkey, CPubKey &vchPubKey, bool fSkipCheck);
};

#endif
30 changes: 25 additions & 5 deletions src/walletdb.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include "walletdb.h"
#include "wallet.h"
#include "hash.h"
#include <boost/filesystem.hpp>

using namespace std;
Expand Down Expand Up @@ -191,6 +192,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
// Taking advantage of the fact that pair serialization
// is just the two items serialized one after the other
ssKey >> strType;

if (strType == "name")
{
string strAddress;
Expand Down Expand Up @@ -269,23 +271,41 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
strErr = "Error reading wallet database: CPubKey corrupt";
return false;
}

CKey key;
CPrivKey pkey;
uint256 hash = 0;

if (strType == "key")
ssValue >> pkey;
else {
CWalletKey wkey;
ssValue >> wkey;
pkey = wkey.vchPrivKey;
}
if (!key.SetPrivKey(pkey, vchPubKey.IsCompressed()))

try
{
strErr = "Error reading wallet database: CPrivKey corrupt";
return false;
ssValue >> hash;
}
catch(...){}

bool fSkipCheck = false;

if (hash != 0)
{
if (Hash(pkey.begin(), pkey.end()) != hash)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hash should be calculated over both private and public key, as the reason for the test is guaranteeing that we have matching keypairs.

{
strErr = "Error reading wallet database: CPrivKey corrupt";
return false;
}

fSkipCheck = true;
}
if (key.GetPubKey() != vchPubKey)

if (!key.Load(pkey, vchPubKey, fSkipCheck))
{
strErr = "Error reading wallet database: CPrivKey pubkey inconsistency";
strErr = "Error reading wallet database: CPrivKey corrupt";
return false;
}
if (!pwallet->LoadKey(key, vchPubKey))
Expand Down
2 changes: 1 addition & 1 deletion src/walletdb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class CWalletDB : public CDB
bool WriteKey(const CPubKey& vchPubKey, const CPrivKey& vchPrivKey)
{
nWalletDBUpdated++;
return Write(std::make_pair(std::string("key"), vchPubKey), vchPrivKey, false);
return Write(std::make_pair(std::string("key"), vchPubKey), std::make_pair(vchPrivKey, Hash(vchPrivKey.begin(), vchPrivKey.end())), false);
}

bool WriteCryptedKey(const CPubKey& vchPubKey, const std::vector<unsigned char>& vchCryptedSecret, bool fEraseUnencryptedKey = true)
Expand Down