1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
|
Return-Path: <cory@atlastechnologiesinc.com>
Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org
[172.17.192.35])
by mail.linuxfoundation.org (Postfix) with ESMTPS id E8EF586
for <bitcoin-dev@lists.linuxfoundation.org>;
Tue, 18 Aug 2015 05:03:44 +0000 (UTC)
X-Greylist: whitelisted by SQLgrey-1.7.6
Received: from mail-la0-f48.google.com (mail-la0-f48.google.com
[209.85.215.48])
by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 887A318D
for <bitcoin-dev@lists.linuxfoundation.org>;
Tue, 18 Aug 2015 05:03:42 +0000 (UTC)
Received: by labd1 with SMTP id d1so92441836lab.1
for <bitcoin-dev@lists.linuxfoundation.org>;
Mon, 17 Aug 2015 22:03:40 -0700 (PDT)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20130820;
h=x-gm-message-state:mime-version:in-reply-to:references:date
:message-id:subject:from:to:cc:content-type
:content-transfer-encoding;
bh=ptfgzu1q495XfF7T7q0V3y2vK1ZVKCW5Tvn8BI30w0g=;
b=byDfLP4AcPT03mIpMrNWfIu7Te9vhcNF2rLi9e0SttkrJMaobkq3EiFw7e04bQZ6oS
8UIf0OJiUG/QvknXIApoLlL4JvIsIccjWTYmIsRSknEMnOw8wjxfvwWOyI1d3ttH6OPZ
gIrU2SMigBcinsxgB8yhR8yuDBuYNm3pTqYl0jfkIgMIiMjOKRNmkMbf11bz+JWhFG2L
ggT0M46M7PhaUvX9cN8vdGclriuIarrXn2T31yaUlqt00x5AA6XGbRt0py6/MMpehdVD
QhO9UsVZqfczoXRhYN9yz3J6D2Vn6lKNIAa14NXI7tDh1E5GyBBHBQUmzypoTC2jQAgu
Z+IQ==
X-Gm-Message-State: ALoCoQlzaKc+Cov7IgExtIA7OqiU+RWOJBMbP3pum720zcla00oTPthI7BXcvu5E9nFamjfWUbXv
MIME-Version: 1.0
X-Received: by 10.112.129.167 with SMTP id nx7mr4030857lbb.27.1439874220447;
Mon, 17 Aug 2015 22:03:40 -0700 (PDT)
Received: by 10.114.83.200 with HTTP; Mon, 17 Aug 2015 22:03:40 -0700 (PDT)
In-Reply-To: <CAApLimiQFaUPgQTLiCKvEb+PQ7pOGg-JvfMfYGzq_X7SfmWLHQ@mail.gmail.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>
Date: Tue, 18 Aug 2015 01:03:40 -0400
Message-ID: <CAApLimgAHB4Bgqp9WG1LiGegUPeKxuppTgizheN4-jwxiuAUag@mail.gmail.com>
From: Cory Fields <lists@coryfields.com>
To: Tamas Blummer <tamas@bitsofproof.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,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 05:03:45 -0000
Back to the list (from github) in case anyone finds this via Google.
The patch that I posted here a few days ago did not fix the issue for Tamas=
.
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.
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.
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).
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:
- Try a method. If it fails, set a global error and return.
- If the global error is set, try a different method.
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).
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.
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/17674bfdf75bffa4e225f8328b9d42cb7=
4504005
This was backported from master back to 1.0.1, but not to 1.0.0 or 0.9.8.
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.
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.
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/adf0a691ee1c2f02e26828f976cfe5b788=
96b507
I'll open a pull-request for Bitcoin Core to discuss whether it's
worth merging or not.
Regards,
Cory
On Fri, Aug 14, 2015 at 5:10 PM, Cory Fields <lists@coryfields.com> wrote:
> Ugh, what an unfortunate oversight!
>
> The good news is that this issue should be solved in future versions
> when we switch to the new libsecp256k1 lib for validation.
>
> 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.
>
> 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.
>
> That's on top of current master, but it should be trivial to apply to
> release tags.
>
> Please let me know how it works out.
>
> Regards,
> Cory
>
> 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 fo=
r all test cases with our Java engine and is about 50% faster on a single t=
hread.
>>
>> The performance advantage unfortunatelly reverses if libconsensus is exe=
cuted on several threads simultaneously as we do with the Java engine, sinc=
e an error:
>>
>> Assertion failed: (pkey !=3D NULL), function CECKey, file ecwrap=
per.cpp, line 96.
>>
>> arises under that stress.
>>
>> I guess that the cause is that thread callbacks as advised for OpenSSL o=
n 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.
>>
>> I=E2=80=99d be thankful for a pointer to a workaround.
>>
>> Tamas Blummer
>>
>> _______________________________________________
>> bitcoin-dev mailing list
>> bitcoin-dev@lists.linuxfoundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev
>>
|
