<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">Thanks a lot Cory for following through the test case and producing a patch.</div><div class=""><br class=""></div><div class="">I confirm that libconsensus is now running stable within the Bits of Proof stack, </div><div class="">in-line with test cases we use to verify the java implementation of the script engine,</div><div class="">that are BTW borrowed from Bitcoin Core.</div><div class=""><br class=""></div><div class="">The performance of libconsensus is surprisingly close to the java one. </div><div class="">Validating a 2-of-2 a multi-sig transaction runs at 1021 ops/sec with java and 1135 ops/sec </div><div class="">in libconsensus. This is on a 2.2GH i7 laptop (4 hyper threading cores used by 8 threads).</div><div class="">Another nice demonstration why one should not trade in advances</div><div class="">of languages for the last decades for a marginal gain of performance with C/C++,</div><div class="">I assume thereby that Bouncy Castle’ EC lib s not superior to OpenSSL's.</div><div class=""><br class=""></div><div class="">I disagree that the problem was rare in the real-world, it should affect any modern </div><div class="">implementation that validates transactions parallel in multiple threads.</div><div class=""><br class=""></div><div class="">Aborting also does not make the problem less severe in my opinion. </div><div class="">Therefore hope the pull will be included into Core with next release.</div><div class=""><br class=""></div><div class="">I can’t assign a timeline to “near future" secp256k1 integration. Can you?</div><div class=""><br class=""></div><div apple-content-edited="true" class="">
<div style="color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">Tamas Blummer</div><div style="color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""></div></div>
<br class=""><div><blockquote type="cite" class=""><div class="">On Aug 18, 2015, at 07:03, Cory Fields <<a href="mailto:lists@coryfields.com" class="">lists@coryfields.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">Back to the list (from github) in case anyone finds this via Google.<br class=""><br class="">The patch that I posted here a few days ago did not fix the issue for Tamas.<br class=""><br class="">I spent some time tracking down this edge-case because<br class="">libbitcoinconsensus needs to be as bullet-proof as possible. Thanks to<br class="">Tamas for creating a bare-bones test case after some discussion.<br class=""><br class="">I finally managed to reproduce the issue on OSX. It's subtle and<br class="">likely rare in the real-world, though obviously not impossible given<br class="">the report here. For posterity, here's a rundown (braindump) of the<br class="">issue.<br class=""><br class="">When calling EC_KEY_new_by_curve_name(), openssl internally checks to<br class="">see how to setup the curve's EC_METHOD (simple, montgomery, or nist).<br class=""><br class="">Unfortunately, in all released OpenSSL versions (as far as I can tell<br class="">master is the only branch that has fixed this issue), it's tested like<br class="">so:<br class=""><br class="">- Try a method. If it fails, set a global error and return.<br class="">- If the global error is set, try a different method.<br class=""><br class="">Prior to OpenSSL 1.0.0, these were tested in the order:<br class="">EC_GFp_nist_method -> EC_GFp_mont_method. The secp256k1 curve fails<br class="">the ec_GFp_nist_group_set_curve test and sets the global error. That<br class="">error is then checked for failure, and EC_GFp_mont_method is tried<br class="">(and succeeds).<br class=""><br class="">Obviously that global error usage is dangerous, especially since it<br class="">happens for _each_ transaction verification in libbitcoinconsensus. In<br class="">a multi-threaded environment, a crash is guaranteed within a few<br class="">seconds.<br class=""><br class="">However, OpenSSL 1.0.1 reversed the order, trying EC_GFp_mont_method<br class="">first, so that the global error doesn't end up being used:<br class=""><a href="https://github.com/openssl/openssl/commit/17674bfdf75bffa4e225f8328b9d42cb74504005" class="">https://github.com/openssl/openssl/commit/17674bfdf75bffa4e225f8328b9d42cb74504005</a><br class=""><br class="">This was backported from master back to 1.0.1, but not to 1.0.0 or 0.9.8.<br class=""><br class="">So that change (accidentally) "solved" the problem. As you can see,<br class="">it's still possible to hit the reversed order in the<br class="">!defined(OPENSSL_BN_ASM_MONT) case. That's easily tested by building<br class="">OpenSSL with the -no-asm config option. It's probably also the case<br class="">for obscure architectures and OSs, but I haven't looked deeply into<br class="">that. In that case, it's reasonable to assume that this crash would<br class="">likely occur on such platforms.<br class=""><br class="">Also, OSX, even the latest version (10.10 as of now), still ships with<br class="">OpenSSL 0.9.8. Which is how Tamas ran into it.<br class=""><br class="">Since Bitcoin Core and libbitcoinconsensus are switching away from<br class="">OpenSSL for verification in the near future, I don't think this is<br class="">much of an issue. Especially since the problem manifests as a<br class="">controlled assertion failure/abort. However, I've prepared a patch for<br class="">anyone who may run into the issue in the short-term:<br class="">https://github.com/theuni/bitcoin/commit/adf0a691ee1c2f02e26828f976cfe5b78896b507<br class=""><br class="">I'll open a pull-request for Bitcoin Core to discuss whether it's<br class="">worth merging or not.<br class=""><br class="">Regards,<br class="">Cory<br class=""><br class="">On Fri, Aug 14, 2015 at 5:10 PM, Cory Fields <lists@coryfields.com> wrote:<br class=""><blockquote type="cite" class="">Ugh, what an unfortunate oversight!<br class=""><br class="">The good news is that this issue should be solved in future versions<br class="">when we switch to the new libsecp256k1 lib for validation.<br class=""><br class="">For now, I've thrown together a quick hack to allow a user-specifiable<br class="">callback for libbitcoinconsensus. I think it's not worth messing with<br class="">the official API since it will be fixed soon, but rather hacked in as<br class="">a temporary work-around as needed. It _should_ be documented as an<br class="">issue with the current version, though.<br class=""><br class="">Please see here for a work-around to try:<br class="">https://github.com/theuni/bitcoin/commits/openssl-consensus-threads<br class="">Unfortunately it's not pretty, but it works fine here. Note that you<br class="">should give this some _serious_ testing before deploying in any real<br class="">way. It should mimic the way we do it in Core, though.<br class=""><br class="">That's on top of current master, but it should be trivial to apply to<br class="">release tags.<br class=""><br class="">Please let me know how it works out.<br class=""><br class="">Regards,<br class="">Cory<br class=""><br class="">On Fri, Aug 14, 2015 at 12:37 PM, Tamas Blummer via bitcoin-dev<br class=""><bitcoin-dev@lists.linuxfoundation.org> wrote:<br class=""><blockquote type="cite" class="">We integrated libconsensus into bits of proof. It works well, in-line for all test cases with our Java engine and is about 50% faster on a single thread.<br class=""><br class="">The performance advantage unfortunatelly reverses if libconsensus is executed on several threads simultaneously as we do with the Java engine, since an error:<br class=""><br class=""> Assertion failed: (pkey != NULL), function CECKey, file ecwrapper.cpp, line 96.<br class=""><br class="">arises under that stress.<br class=""><br class="">I guess that the cause is that thread callbacks as advised for OpenSSL on https://www.openssl.org/docs/crypto/threads.html are not registered.<br class="">Registering those however would require access to OpenSSL functions, not exported from the lib.<br class=""><br class="">I’d be thankful for a pointer to a workaround.<br class=""><br class="">Tamas Blummer<br class=""><br class="">_______________________________________________<br class="">bitcoin-dev mailing list<br class="">bitcoin-dev@lists.linuxfoundation.org<br class="">https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev<br class=""><br class=""></blockquote></blockquote><br class=""></div></blockquote></div><br class=""></body></html>