path: root/e6/65de50583b53986ae48a555f528a81af4fb601

Return-Path: <tamas@bitsofproof.com>
Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org
	[172.17.192.35])
	by mail.linuxfoundation.org (Postfix) with ESMTPS id 4ACB2305
	for <bitcoin-dev@lists.linuxfoundation.org>;
	Tue, 18 Aug 2015 10:31:59 +0000 (UTC)
X-Greylist: from auto-whitelisted by SQLgrey-1.7.6
Received: from wp059.webpack.hosteurope.de (wp059.webpack.hosteurope.de
	[80.237.132.66])
	by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 9B8FE164
	for <bitcoin-dev@lists.linuxfoundation.org>;
	Tue, 18 Aug 2015 10:31:56 +0000 (UTC)
Received: from [81.0.112.130] (helo=[192.168.0.109]); authenticated
	by wp059.webpack.hosteurope.de running ExIM with esmtpsa
	(TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)
	id 1ZReBO-0007T7-63; Tue, 18 Aug 2015 12:31:54 +0200
Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2102\))
Content-Type: multipart/signed;
	boundary="Apple-Mail=_ABE2F079-7D85-4296-8776-AF2355709186";
	protocol="application/pgp-signature"; micalg=pgp-sha512
X-Pgp-Agent: GPGMail 2.5.1
From: Tamas Blummer <tamas@bitsofproof.com>
In-Reply-To: <CAApLimgAHB4Bgqp9WG1LiGegUPeKxuppTgizheN4-jwxiuAUag@mail.gmail.com>
Date: Tue, 18 Aug 2015 12:31:52 +0200
Message-Id: <68E206FF-4ABD-4547-BF73-8661A7C2F08B@bitsofproof.com>
References: <09C8843E-8379-404D-8357-05BDB1F749C1@me.com>
	<CADZB0_YvvDDq4XzfvQeeWJ2oZxPukP0oXYSrEeC3gy9_Fk0ZuA@mail.gmail.com>
	<D018B1B0-B613-4C05-84BB-02CE6E2FEA3E@me.com>
	<499C1F46-5EB8-4846-86B6-0B3F2E02D972@bitsofproof.com>
	<CAApLimiQFaUPgQTLiCKvEb+PQ7pOGg-JvfMfYGzq_X7SfmWLHQ@mail.gmail.com>
	<CAApLimgAHB4Bgqp9WG1LiGegUPeKxuppTgizheN4-jwxiuAUag@mail.gmail.com>
To: Cory Fields <lists@coryfields.com>
X-Mailer: Apple Mail (2.2102)
X-bounce-key: webpack.hosteurope.de; tamas@bitsofproof.com; 1439893916;
	24e43bee; 
X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,HTML_MESSAGE,
	RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1
X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on
	smtp1.linux-foundation.org
Cc: bitcoin-dev@lists.linuxfoundation.org
Subject: Re: [bitcoin-dev] libconsensus assertion fails if used in multiple
	threads
X-BeenThere: bitcoin-dev@lists.linuxfoundation.org
X-Mailman-Version: 2.1.12
Precedence: list
List-Id: Bitcoin Development Discussion <bitcoin-dev.lists.linuxfoundation.org>
List-Unsubscribe: <https://lists.linuxfoundation.org/mailman/options/bitcoin-dev>,
	<mailto:bitcoin-dev-request@lists.linuxfoundation.org?subject=unsubscribe>
List-Archive: <http://lists.linuxfoundation.org/pipermail/bitcoin-dev/>
List-Post: <mailto:bitcoin-dev@lists.linuxfoundation.org>
List-Help: <mailto:bitcoin-dev-request@lists.linuxfoundation.org?subject=help>
List-Subscribe: <https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev>,
	<mailto:bitcoin-dev-request@lists.linuxfoundation.org?subject=subscribe>
X-List-Received-Date: Tue, 18 Aug 2015 10:31:59 -0000


--Apple-Mail=_ABE2F079-7D85-4296-8776-AF2355709186
Content-Type: multipart/alternative;
	boundary="Apple-Mail=_02C4D528-0B90-446A-B445-C69F5D37CC35"


--Apple-Mail=_02C4D528-0B90-446A-B445-C69F5D37CC35
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8

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=E2=80=99 EC lib s not superior to =
OpenSSL's.

I disagree that the problem was rare in the real-world, it should affect =
any modern
implementation that validates transactions parallel in multiple threads.

Aborting also does not make the problem less severe in my opinion.
Therefore hope the pull will be included into Core with next release.

I can=E2=80=99t assign a timeline to =E2=80=9Cnear future" secp256k1 =
integration. Can you?

Tamas Blummer


> On Aug 18, 2015, at 07:03, Cory Fields <lists@coryfields.com> wrote:
>=20
> Back to the list (from github) in case anyone finds this via Google.
>=20
> The patch that I posted here a few days ago did not fix the issue for =
Tamas.
>=20
> I spent some time tracking down this edge-case because
> libbitcoinconsensus needs to be as bullet-proof as possible. Thanks to
> Tamas for creating a bare-bones test case after some discussion.
>=20
> I finally managed to reproduce the issue on OSX. It's subtle and
> likely rare in the real-world, though obviously not impossible given
> the report here. For posterity, here's a rundown (braindump) of the
> issue.
>=20
> When calling EC_KEY_new_by_curve_name(), openssl internally checks to
> see how to setup the curve's EC_METHOD (simple, montgomery, or nist).
>=20
> Unfortunately, in all released OpenSSL versions (as far as I can tell
> master is the only branch that has fixed this issue), it's tested like
> so:
>=20
> - Try a method. If it fails, set a global error and return.
> - If the global error is set, try a different method.
>=20
> Prior to OpenSSL 1.0.0, these were tested in the order:
> EC_GFp_nist_method -> EC_GFp_mont_method. The secp256k1 curve fails
> the ec_GFp_nist_group_set_curve test and sets the global error. That
> error is then checked for failure, and EC_GFp_mont_method is tried
> (and succeeds).
>=20
> Obviously that global error usage is dangerous, especially since it
> happens for _each_ transaction verification in libbitcoinconsensus. In
> a multi-threaded environment, a crash is guaranteed within a few
> seconds.
>=20
> However, OpenSSL 1.0.1 reversed the order, trying EC_GFp_mont_method
> first, so that the global error doesn't end up being used:
> =
https://github.com/openssl/openssl/commit/17674bfdf75bffa4e225f8328b9d42cb=
74504005
>=20
> This was backported from master back to 1.0.1, but not to 1.0.0 or =
0.9.8.
>=20
> So that change (accidentally) "solved" the problem. As you can see,
> it's still possible to hit the reversed order in the
> !defined(OPENSSL_BN_ASM_MONT) case. That's easily tested by building
> OpenSSL with the -no-asm config option. It's probably also the case
> for obscure architectures and OSs, but I haven't looked deeply into
> that. In that case, it's reasonable to assume that this crash would
> likely occur on such platforms.
>=20
> Also, OSX, even the latest version (10.10 as of now), still ships with
> OpenSSL 0.9.8. Which is how Tamas ran into it.
>=20
> Since Bitcoin Core and libbitcoinconsensus are switching away from
> OpenSSL for verification in the near future, I don't think this is
> much of an issue. Especially since the problem manifests as a
> controlled assertion failure/abort. However, I've prepared a patch for
> anyone who may run into the issue in the short-term:
> =
https://github.com/theuni/bitcoin/commit/adf0a691ee1c2f02e26828f976cfe5b78=
896b507
>=20
> I'll open a pull-request for Bitcoin Core to discuss whether it's
> worth merging or not.
>=20
> Regards,
> Cory
>=20
> On Fri, Aug 14, 2015 at 5:10 PM, Cory Fields <lists@coryfields.com> =
wrote:
>> Ugh, what an unfortunate oversight!
>>=20
>> The good news is that this issue should be solved in future versions
>> when we switch to the new libsecp256k1 lib for validation.
>>=20
>> For now, I've thrown together a quick hack to allow a =
user-specifiable
>> callback for libbitcoinconsensus. I think it's not worth messing with
>> the official API since it will be fixed soon, but rather hacked in as
>> a temporary work-around as needed. It _should_ be documented as an
>> issue with the current version, though.
>>=20
>> Please see here for a work-around to try:
>> https://github.com/theuni/bitcoin/commits/openssl-consensus-threads
>> Unfortunately it's not pretty, but it works fine here. Note that you
>> should give this some _serious_ testing before deploying in any real
>> way. It should mimic the way we do it in Core, though.
>>=20
>> That's on top of current master, but it should be trivial to apply to
>> release tags.
>>=20
>> Please let me know how it works out.
>>=20
>> Regards,
>> Cory
>>=20
>> On Fri, Aug 14, 2015 at 12:37 PM, Tamas Blummer via bitcoin-dev
>> <bitcoin-dev@lists.linuxfoundation.org> wrote:
>>> 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.
>>>=20
>>> The performance advantage unfortunatelly reverses if libconsensus is =
executed on several threads simultaneously as we do with the Java =
engine, since an error:
>>>=20
>>>        Assertion failed: (pkey !=3D NULL), function CECKey, file =
ecwrapper.cpp, line 96.
>>>=20
>>> arises under that stress.
>>>=20
>>> 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.
>>> Registering those however would require access to OpenSSL functions, =
not exported from the lib.
>>>=20
>>> I=E2=80=99d be thankful for a pointer to a workaround.
>>>=20
>>> Tamas Blummer
>>>=20
>>> _______________________________________________
>>> bitcoin-dev mailing list
>>> bitcoin-dev@lists.linuxfoundation.org
>>> https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev
>>>=20
>=20


--Apple-Mail=_02C4D528-0B90-446A-B445-C69F5D37CC35
Content-Transfer-Encoding: quoted-printable
Content-Type: text/html;
	charset=utf-8

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

--Apple-Mail=_02C4D528-0B90-446A-B445-C69F5D37CC35--

--Apple-Mail=_ABE2F079-7D85-4296-8776-AF2355709186
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename=signature.asc
Content-Type: application/pgp-signature;
	name=signature.asc
Content-Description: Message signed with OpenPGP using GPGMail

-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - https://gpgtools.org

iQEcBAEBCgAGBQJV0wmZAAoJEPZykcUXcTkcJaMH/iHf6zl+V7YVXtd5Ouvl7/pS
YSsFM36QaI/fbsOauAtXoy20OSF27r+WSx2lXTqCR5AMsx/JNgb+zmkmidYD1rGO
X3w2/5+9TXyAPr3PfZ/TFqPl5ns0h/a/Aaoq7BkFiHVz95ebv2QXd5/MUonBnq2H
xc+FvBUXm3j1sdTEFyTFfECuU7JEkPc9ctQmveC9YA9tLcQgx/honLXMsmU+IKHT
h0cBcNdEAkcnqMkJmdC8V+MAMcgnRjicSTtp7kBmkIW7NpwCuo43q42pAUkrz1kq
ASJyiAG+8W5gbBlQeArNRXQQe0Ca71lWW9KF2eqQnyFgJ0T+pG5bhdy+c2qnZsE=
=Lxv0
-----END PGP SIGNATURE-----

--Apple-Mail=_ABE2F079-7D85-4296-8776-AF2355709186--