đź“… Original date posted:2015-08-18
đź“ť Original message: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.
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’t assign a timeline to “near future" secp256k1 integration. Can you?
Tamas Blummer
> On Aug 18, 2015, at 07:03, Cory Fields <lists at coryfields.com> wrote:
>
> 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/17674bfdf75bffa4e225f8328b9d42cb74504005
>
> 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/adf0a691ee1c2f02e26828f976cfe5b78896b507
>
> 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 at 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 at 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.
>>>
>>> The performance advantage unfortunatelly reverses if libconsensus is executed on several threads simultaneously as we do with the Java engine, since an error:
>>>
>>> Assertion failed: (pkey != NULL), function CECKey, file ecwrapper.cpp, line 96.
>>>
>>> arises under that stress.
>>>
>>> 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.
>>>
>>> I’d be thankful for a pointer to a workaround.
>>>
>>> Tamas Blummer
>>>
>>> _______________________________________________
>>> bitcoin-dev mailing list
>>> bitcoin-dev at lists.linuxfoundation.org
>>> https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linuxfoundation.org/pipermail/bitcoin-dev/attachments/20150818/43bc1a5b/attachment.html>;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 496 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.linuxfoundation.org/pipermail/bitcoin-dev/attachments/20150818/43bc1a5b/attachment.sig>;
Tamas Blummer [ARCHIVE] /
npub1cc…ex8m8
2023-06-07 15:46:59
in reply to nevent1q…s2fl
Author Public Key
npub1ccegg9n9lnx6huppxg43m95488yur7pfemkn3pz0agjws5ffvtts0ex8m8Published at
2023-06-07 15:46:59Event JSON
{
"id": "3c0799ad16a766cf908852c0e8951ea2ff160013309d6cd033b169d0065e8c62",
"pubkey": "c632841665fccdabf021322b1d969539c9c1f829ceed38844fea24e8512962d7",
"created_at": 1686152819,
"kind": 1,
"tags": [
[
"e",
"f9cdbcb2f8408a8617a32357a9cd9ec7781e0a2acb315808f6e58178dd97a111",
"",
"root"
],
[
"e",
"ff456fe12eaae0cd271ac19a9b3cd55c99027068caee73df46c2492afb87c9a2",
"",
"reply"
],
[
"p",
"28e91e249bc93012b85689043e5284a72bacd58ceabf2f4fb6a8873d12e39f7c"
]
],
"content": "📅 Original date posted:2015-08-18\n📝 Original message:Thanks a lot Cory for following through the test case and producing a patch.\n\nI confirm that libconsensus is now running stable within the Bits of Proof stack,\nin-line with test cases we use to verify the java implementation of the script engine,\nthat are BTW borrowed from Bitcoin Core.\n\nThe performance of libconsensus is surprisingly close to the java one.\nValidating a 2-of-2 a multi-sig transaction runs at 1021 ops/sec with java and 1135 ops/sec\nin libconsensus. This is on a 2.2GH i7 laptop (4 hyper threading cores used by 8 threads).\nAnother nice demonstration why one should not trade in advances\nof languages for the last decades for a marginal gain of performance with C/C++,\nI assume thereby that Bouncy Castle’ EC lib s not superior to OpenSSL's.\n\nI disagree that the problem was rare in the real-world, it should affect any modern\nimplementation that validates transactions parallel in multiple threads.\n\nAborting also does not make the problem less severe in my opinion.\nTherefore hope the pull will be included into Core with next release.\n\nI can’t assign a timeline to “near future\" secp256k1 integration. Can you?\n\nTamas Blummer\n\n\n\u003e On Aug 18, 2015, at 07:03, Cory Fields \u003clists at coryfields.com\u003e wrote:\n\u003e \n\u003e Back to the list (from github) in case anyone finds this via Google.\n\u003e \n\u003e The patch that I posted here a few days ago did not fix the issue for Tamas.\n\u003e \n\u003e I spent some time tracking down this edge-case because\n\u003e libbitcoinconsensus needs to be as bullet-proof as possible. Thanks to\n\u003e Tamas for creating a bare-bones test case after some discussion.\n\u003e \n\u003e I finally managed to reproduce the issue on OSX. It's subtle and\n\u003e likely rare in the real-world, though obviously not impossible given\n\u003e the report here. For posterity, here's a rundown (braindump) of the\n\u003e issue.\n\u003e \n\u003e When calling EC_KEY_new_by_curve_name(), openssl internally checks to\n\u003e see how to setup the curve's EC_METHOD (simple, montgomery, or nist).\n\u003e \n\u003e Unfortunately, in all released OpenSSL versions (as far as I can tell\n\u003e master is the only branch that has fixed this issue), it's tested like\n\u003e so:\n\u003e \n\u003e - Try a method. If it fails, set a global error and return.\n\u003e - If the global error is set, try a different method.\n\u003e \n\u003e Prior to OpenSSL 1.0.0, these were tested in the order:\n\u003e EC_GFp_nist_method -\u003e EC_GFp_mont_method. The secp256k1 curve fails\n\u003e the ec_GFp_nist_group_set_curve test and sets the global error. That\n\u003e error is then checked for failure, and EC_GFp_mont_method is tried\n\u003e (and succeeds).\n\u003e \n\u003e Obviously that global error usage is dangerous, especially since it\n\u003e happens for _each_ transaction verification in libbitcoinconsensus. In\n\u003e a multi-threaded environment, a crash is guaranteed within a few\n\u003e seconds.\n\u003e \n\u003e However, OpenSSL 1.0.1 reversed the order, trying EC_GFp_mont_method\n\u003e first, so that the global error doesn't end up being used:\n\u003e https://github.com/openssl/openssl/commit/17674bfdf75bffa4e225f8328b9d42cb74504005\n\u003e \n\u003e This was backported from master back to 1.0.1, but not to 1.0.0 or 0.9.8.\n\u003e \n\u003e So that change (accidentally) \"solved\" the problem. As you can see,\n\u003e it's still possible to hit the reversed order in the\n\u003e !defined(OPENSSL_BN_ASM_MONT) case. That's easily tested by building\n\u003e OpenSSL with the -no-asm config option. It's probably also the case\n\u003e for obscure architectures and OSs, but I haven't looked deeply into\n\u003e that. In that case, it's reasonable to assume that this crash would\n\u003e likely occur on such platforms.\n\u003e \n\u003e Also, OSX, even the latest version (10.10 as of now), still ships with\n\u003e OpenSSL 0.9.8. Which is how Tamas ran into it.\n\u003e \n\u003e Since Bitcoin Core and libbitcoinconsensus are switching away from\n\u003e OpenSSL for verification in the near future, I don't think this is\n\u003e much of an issue. Especially since the problem manifests as a\n\u003e controlled assertion failure/abort. However, I've prepared a patch for\n\u003e anyone who may run into the issue in the short-term:\n\u003e https://github.com/theuni/bitcoin/commit/adf0a691ee1c2f02e26828f976cfe5b78896b507\n\u003e \n\u003e I'll open a pull-request for Bitcoin Core to discuss whether it's\n\u003e worth merging or not.\n\u003e \n\u003e Regards,\n\u003e Cory\n\u003e \n\u003e On Fri, Aug 14, 2015 at 5:10 PM, Cory Fields \u003clists at coryfields.com\u003e wrote:\n\u003e\u003e Ugh, what an unfortunate oversight!\n\u003e\u003e \n\u003e\u003e The good news is that this issue should be solved in future versions\n\u003e\u003e when we switch to the new libsecp256k1 lib for validation.\n\u003e\u003e \n\u003e\u003e For now, I've thrown together a quick hack to allow a user-specifiable\n\u003e\u003e callback for libbitcoinconsensus. I think it's not worth messing with\n\u003e\u003e the official API since it will be fixed soon, but rather hacked in as\n\u003e\u003e a temporary work-around as needed. It _should_ be documented as an\n\u003e\u003e issue with the current version, though.\n\u003e\u003e \n\u003e\u003e Please see here for a work-around to try:\n\u003e\u003e https://github.com/theuni/bitcoin/commits/openssl-consensus-threads\n\u003e\u003e Unfortunately it's not pretty, but it works fine here. Note that you\n\u003e\u003e should give this some _serious_ testing before deploying in any real\n\u003e\u003e way. It should mimic the way we do it in Core, though.\n\u003e\u003e \n\u003e\u003e That's on top of current master, but it should be trivial to apply to\n\u003e\u003e release tags.\n\u003e\u003e \n\u003e\u003e Please let me know how it works out.\n\u003e\u003e \n\u003e\u003e Regards,\n\u003e\u003e Cory\n\u003e\u003e \n\u003e\u003e On Fri, Aug 14, 2015 at 12:37 PM, Tamas Blummer via bitcoin-dev\n\u003e\u003e \u003cbitcoin-dev at lists.linuxfoundation.org\u003e wrote:\n\u003e\u003e\u003e 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.\n\u003e\u003e\u003e \n\u003e\u003e\u003e The performance advantage unfortunatelly reverses if libconsensus is executed on several threads simultaneously as we do with the Java engine, since an error:\n\u003e\u003e\u003e \n\u003e\u003e\u003e Assertion failed: (pkey != NULL), function CECKey, file ecwrapper.cpp, line 96.\n\u003e\u003e\u003e \n\u003e\u003e\u003e arises under that stress.\n\u003e\u003e\u003e \n\u003e\u003e\u003e 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.\n\u003e\u003e\u003e Registering those however would require access to OpenSSL functions, not exported from the lib.\n\u003e\u003e\u003e \n\u003e\u003e\u003e I’d be thankful for a pointer to a workaround.\n\u003e\u003e\u003e \n\u003e\u003e\u003e Tamas Blummer\n\u003e\u003e\u003e \n\u003e\u003e\u003e _______________________________________________\n\u003e\u003e\u003e bitcoin-dev mailing list\n\u003e\u003e\u003e bitcoin-dev at lists.linuxfoundation.org\n\u003e\u003e\u003e https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev\n\u003e\u003e\u003e \n\u003e \n\n-------------- next part --------------\nAn HTML attachment was scrubbed...\nURL: \u003chttp://lists.linuxfoundation.org/pipermail/bitcoin-dev/attachments/20150818/43bc1a5b/attachment.html\u003e\n-------------- next part --------------\nA non-text attachment was scrubbed...\nName: signature.asc\nType: application/pgp-signature\nSize: 496 bytes\nDesc: Message signed with OpenPGP using GPGMail\nURL: \u003chttp://lists.linuxfoundation.org/pipermail/bitcoin-dev/attachments/20150818/43bc1a5b/attachment.sig\u003e",
"sig": "82b90bb895fee6036ebf61830b11741b10a6ec21c052315a1672859fe22ac6a1e5d0351d7ff7c53c0be147e8d71183f25b0d75a7b9036629de7251022a433352"
}