Significantly Improve Wallet Load times #2733

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

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

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

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.

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.

Then roll a quick RAII wrapper (don't add more dependence on boost). Here's one for you http://codepad.org/gr8M2LE6

Look at how easy it was to introduce a memory leak. I know that you guys know better than this!

~90 LOC just to avoid a goto?

I think that speaks for itself :)

90 LOC to have the right tool. Note, you could use boost::auto_ptr or whatever RAII wrapper they have around dynamically created objects. I prefer not to create additional dependencies on boost. That Bitcoin source code is sloppy and breaks numerous "best practices" endorsed by luminaries of C++ should not even be up for debate - I know that you guys know better! The attitude needs to change or else it will remain a difficult to maintain mess, becoming progressively harder to implement changes without breaking things and tons of extra work.

NAK that logic

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.

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.

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.

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

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.

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