Pull request submitted: https://github.com/bitcoin/bitcoin/pull/6571
Regards, Cory On Tue, Aug 18, 2015 at 1:25 PM, Cory Fields <li...@coryfields.com> wrote: > See responses inline. > > On Tue, Aug 18, 2015 at 6:31 AM, Tamas Blummer <ta...@bitsofproof.com> wrote: >> Thanks a lot Cory for following through the test case and producing a patch. >> >> I confirm that libconsensus is now running stable within the Bits of Proof >> stack, >> in-line with test cases we use to verify the java implementation of the >> script engine, >> that are BTW borrowed from Bitcoin Core. >> >> The performance of libconsensus is surprisingly close to the java one. >> Validating a 2-of-2 a multi-sig transaction runs at 1021 ops/sec with java >> and 1135 ops/sec >> in libconsensus. This is on a 2.2GH i7 laptop (4 hyper threading cores used >> by 8 threads). >> Another nice demonstration why one should not trade in advances >> of languages for the last decades for a marginal gain of performance with >> C/C++, >> I assume thereby that Bouncy Castle’ EC lib s not superior to OpenSSL's. > > A few points there. First, Core is switching to libsecp256k1 for > several reasons, and one of them is speed. I seem to recall it being > up to 8x faster than OpenSSL. > > Also, it can depend heavily on compiler switches and optimization > levels. For example, In playing with my test-case for hitting the > OpenSSL race issue, I managed to get a ~100% speedup by simply using > -O3 and lto. > >> >> I disagree that the problem was rare in the real-world, it should affect any >> modern >> implementation that validates transactions parallel in multiple threads. >> > > Well I'd say you're a bit biased in this case ;) > > It's only those using ancient (0.98 or 1.00) versions of OpenSSL who > are affected, or those with OPENSSL_BN_ASM_MONT support disabled or > missing. Note that official releases of libbitcoinconsensus are > compiled against a much newer version and shouldn't have any issues. > > The earlier patches for locking callbacks should be unnecessary. > >> Aborting also does not make the problem less severe in my opinion. > > Well it's not a good thing by any means, but it's certainly better > than incorrect results! In any undefined/error condition for the > consensus library, aborting is the right thing to do. If we can't > explain how we've reached a certain "unreachable" condition as is the > case here, the only reasonable recourse is to shut down. Otherwise we > risk network forks, DOS, etc. > >> Therefore hope the pull will be included into Core with next release. >> > > It will likely be unnecessary for the next release, but I do think > it's worth backporting to the 0.10 and 0.9 series. > >> I can’t assign a timeline to “near future" secp256k1 integration. Can you? > > I believe the libsecp256k1 guys are generally happy with the lib these > days, but I'll avoid guessing at a timeline. We can discuss that on > the PR for this fix, which I'll do today. > > Regards, > Cory _______________________________________________ bitcoin-dev mailing list bitcoin-dev@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev
