Pure python EC #15826

sipa · 2019-04-16T07:27:18Z

This removes the dependency on OpenSSL for the interaction tests, by providing a pure-Python
toy implementation of secp256k1.

jonasschnelli · 2019-04-16T07:35:29Z

Concept ACK

practicalswift · 2019-04-16T08:53:20Z

Concept ACK

Excellent work! Thanks for doing this!

maflcko · 2019-04-16T11:44:20Z

❤️ Makes it easier to run the tests on native windows, where I couldn't figure out how to install that library. Will give it a try...

test/functional/test_framework/key.py

sdaftuar · 2019-04-16T11:56:28Z

Makes it easier to run the tests on native windows, where I couldn't figure out how to install that library

That seems like a good motivation for making a change like this, but I'm curious if there are other reasons as well?

sipa · 2019-04-16T17:07:55Z

@sdaftuar The original motivation was that for testing a Schnorr signature implementation we'll want a Python version to test against, which needs much of this logic anyway, and it felt silly to maintain two independent versions just for testing (especially as one relies on OpenSSL we're trying to get rid of as a dependency).

@MarcoFalke I added the verify_ecdsa function to test the sign_ecdsa code while writing it, but expect it may be useful to test against the Bitcoin Core signing code. I can either make the linter ignore it, or at a simple test that invokes it. Up to you.

maflcko · 2019-04-16T19:26:59Z

Yeah, having a test that verify_ecdsas a Bitcoin Core generated signature would be nice

jnewbery

Concept ACK!

test/functional/p2p_segwit.py

sipa · 2019-04-16T20:02:44Z

I've added ecdsa_verify to the ignore list. I'll try to write a test that actually uses it, but it's not that trivial either.

gmaxwell · 2019-04-17T01:15:57Z

At a glance this looks fine, I was going to urge you to put a really strong warning on it, but the warning looks adequate. I'm not going to give this any cryptographic review because I don't think it needs/deserves any.

test/functional/test_framework/key.py

maflcko · 2019-04-17T18:29:45Z

This will be merged next Monday unless there are objections

test/functional/test_framework/key.py

jnewbery · 2019-04-17T21:07:40Z

test/functional/test_framework/key.py

@@ -1,226 +1,343 @@
-# Copyright (c) 2011 Sam Rushing
-"""ECC secp256k1 OpenSSL wrapper.
+# Copyright (c) 2019 Pieter Wuille


Does the project have a policy on individual contributors' copyright notices in source files? Do you also want to include the MIT license boilerplate here too?

There are a few files with individual author's names. As the "Copyright The Bitcoin Core contributors" is likely meaningless I think listing actual authors is preferable when it's a well-defined set of people, but I don't feel strongly.

jnewbery

Looks good to me. A couple of nits inline, but otherwise ACK 8cc786094fb55907a25a4b46caa7f1f8fb512363

I added copious comments and notes for myself as I was reviewing. They're here: jnewbery@af36c8a. Feel free to take that commit for this PR.

test/functional/test_framework/key.py

laanwj · 2019-04-18T18:34:18Z

good to lose this dependency on an external library w/ ctypes, this makes porting the tests easier

ACK 8cc786094fb55907a25a4b46caa7f1f8fb512363 (though would be good to integrate @jnewbery's comments)

This removes the dependency on OpenSSL for the interaction tests, by providing a pure-Python toy implementation of secp256k1.

sipa · 2019-04-18T19:31:44Z

I've rewritten the extgcd/modinv functions to be a bit faster, and also included @jnewbery's comments (after rebasing that commit).

jnewbery · 2019-04-18T20:07:49Z

Nice find for the quicker modinv().

utACK ac050deca0227fe44346f3f91c79d733ed5705d8

This removes the dependency on OpenSSL for the interaction tests, by providing a pure-Python toy implementation of secp256k1. Github-Pull: bitcoin#15826 Rebased-From: 8c7b932

Github-Pull: bitcoin#15826 Rebased-From: b679785

27fcb40 doc: replace outdated OpenSSL comment in test README (fanquake) Pull request description: The OpenSSL dependency was removed in #15826. ACKs for top commit: laanwj: ACK 27fcb40 theStack: ACK 27fcb40 Tree-SHA512: eb7a3b18fefa91e6f27c50fa065d6cc330f7b633ae8ee51145cdeec4df51dea5155f0d1fa91e75f1202adef04e063f3eda12773cd00a093f29f5a5e83c4fda73

Summary: ``` This removes the dependency on OpenSSL for the interaction tests, by providing a pure-Python toy implementation of secp256k1. ``` Backport of core [[bitcoin/bitcoin#15826 | PR15826]]. Test Plan: ninja check-functional-extended *The following depends on D14007:* Build for windows, then: - Install python if needed and add it to your path - If you copy the project to some place, you need at least the `src`, `test`, `share` and your build directories. - Copy the test_runner.py to the build directory to replace the symlink - Update the config.ini file to match you paths Then in a powershell: $env:PYTHONIOENCODING="utf-8" python .\test\functional\test_runner.py --force You should note that only a few tests remain red, in particular all the schnorr related tests since they still use openssl. Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5965

…ADME 27fcb40 doc: replace outdated OpenSSL comment in test README (fanquake) Pull request description: The OpenSSL dependency was removed in bitcoin#15826. ACKs for top commit: laanwj: ACK 27fcb40 theStack: ACK bitcoin@27fcb40 Tree-SHA512: eb7a3b18fefa91e6f27c50fa065d6cc330f7b633ae8ee51145cdeec4df51dea5155f0d1fa91e75f1202adef04e063f3eda12773cd00a093f29f5a5e83c4fda73

b679785 Add comments to Python ECDSA implementation (John Newbery) 8c7b932 Pure python EC (Pieter Wuille) Pull request description: This removes the dependency on OpenSSL for the interaction tests, by providing a pure-Python toy implementation of secp256k1. ACKs for commit b67978: jnewbery: utACK b679785 Tree-SHA512: 181445eb08b316c46937b80dc10aa50d103ab1fdddaf834896c0ea22204889f7b13fd33cbcbd00ddba15f7e4686fe0d9f8e8bb4c0ad0e9587490c90be83966dc

fanquake added the Tests label Apr 16, 2019

maflcko reviewed Apr 16, 2019

View reviewed changes

test/functional/test_framework/key.py Outdated Show resolved Hide resolved

jnewbery reviewed Apr 16, 2019

View reviewed changes

test/functional/p2p_segwit.py Outdated Show resolved Hide resolved

sipa force-pushed the 201904_pythonec branch from b567f72 to 3dfc518 Compare April 16, 2019 19:59

sipa force-pushed the 201904_pythonec branch 4 times, most recently from 17dca8e to 048cbc4 Compare April 16, 2019 23:59

jnewbery reviewed Apr 17, 2019

View reviewed changes

test/functional/test_framework/key.py Outdated Show resolved Hide resolved

sipa force-pushed the 201904_pythonec branch 2 times, most recently from 4571397 to c01a1b9 Compare April 17, 2019 17:39

jnewbery reviewed Apr 17, 2019

View reviewed changes

test/functional/test_framework/key.py Outdated Show resolved Hide resolved

jnewbery reviewed Apr 17, 2019

View reviewed changes

test/functional/test_framework/key.py Show resolved Hide resolved

jnewbery reviewed Apr 17, 2019

View reviewed changes

sipa force-pushed the 201904_pythonec branch from c01a1b9 to 8cc7860 Compare April 17, 2019 21:53

jnewbery reviewed Apr 18, 2019

View reviewed changes

test/functional/test_framework/key.py Outdated Show resolved Hide resolved

test/functional/test_framework/key.py Outdated Show resolved Hide resolved

Pure python EC

8c7b932

This removes the dependency on OpenSSL for the interaction tests, by providing a pure-Python toy implementation of secp256k1.

sipa force-pushed the 201904_pythonec branch from 8cc7860 to 8c7b932 Compare April 18, 2019 18:58

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Aug 24, 2019

Pure python EC

df9a063

This removes the dependency on OpenSSL for the interaction tests, by providing a pure-Python toy implementation of secp256k1. Github-Pull: bitcoin#15826 Rebased-From: 8c7b932

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Aug 24, 2019

Add comments to Python ECDSA implementation

98771b9

Github-Pull: bitcoin#15826 Rebased-From: b679785

fanquake mentioned this pull request Sep 25, 2019

doc: replace outdated OpenSSL comment in test README #16960

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Pure python EC #15826

Pure python EC #15826

Uh oh!

Conversation

sipa commented Apr 16, 2019

Uh oh!

jonasschnelli commented Apr 16, 2019

Uh oh!

practicalswift commented Apr 16, 2019

Uh oh!

maflcko commented Apr 16, 2019

Uh oh!

Uh oh!

sdaftuar commented Apr 16, 2019

Uh oh!

sipa commented Apr 16, 2019

Uh oh!

maflcko commented Apr 16, 2019

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sipa commented Apr 16, 2019

Uh oh!

gmaxwell commented Apr 17, 2019

Uh oh!

Uh oh!

maflcko commented Apr 17, 2019

Uh oh!

Uh oh!

Uh oh!

jnewbery Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

sipa Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

laanwj commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Apr 18, 2019

Uh oh!

jnewbery commented Apr 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

laanwj commented Apr 18, 2019 •

edited

Loading