Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

NEEDS REVIEW - Fix #30. Replace ctypes with cryptography (cffi).#62

Closed
posita wants to merge 1 commit intopetertodd:masterpetertodd/python-bitcoinlib:masterfrom
posita:use-pyca-cryptography-for-bignum-ops-30Copy head branch name to clipboard
Closed

NEEDS REVIEW - Fix #30. Replace ctypes with cryptography (cffi).#62
posita wants to merge 1 commit intopetertodd:masterpetertodd/python-bitcoinlib:masterfrom
posita:use-pyca-cryptography-for-bignum-ops-30Copy head branch name to clipboard

Conversation

@posita
Copy link
Contributor

@posita posita commented May 3, 2015

Fix #30. Replace ctypes with cryptography (cffi).

% python2.7 -m unittest discover && python3.4 -m unittest discover
..................................................................................................
----------------------------------------------------------------------
Ran 98 tests in 0.565s

OK
..................................................................................................
----------------------------------------------------------------------
Ran 98 tests in 0.502s

OK

There is currently one known issue: CECKey.set_privkey and CECKey.get_privkey require the FFI instance to expose the d2i_ECPrivateKey and i2d_ECPrivateKey functions, respectively, from the underlying OpenSSL library. Currently, cryptography does not expose these functions. This means that those functions will not work. I have not yet investigated or considered using a hybrid ctypes approach for that narrow set of functionality (if that is even possible).

This only affects the *_privkey getter and setter. I was able to provide alternatives to the o2i_ECPublicKey and i2o_ECPublicKey functions used in the corresponding *_pubkey functions (see this and this). I have not yet found (or am not clever enough to recognize) equivalents on the private key side.

The reason why unit tests still pass is because the *_privkey getter and setter are not called from any of the tests. This is unchanged from before (I did not add tests to cover those methods).

@posita posita mentioned this pull request May 3, 2015
Closed
@posita posita changed the title NOT READY YET - Fix #30. Replace ctypes with cffi/cryptography. Fix #30. Replace ctypes with cryptography (cffi). May 3, 2015
@posita posita changed the title Fix #30. Replace ctypes with cryptography (cffi). NEEDS REVIEW - Fix #30. Replace ctypes with cryptography (cffi). May 3, 2015
@posita posita changed the title NEEDS REVIEW - Fix #30. Replace ctypes with cryptography (cffi). NEEDS REVIEW - Fix #30. Replace ctypes with cryptography (cffi). May 3, 2015
@posita posita mentioned this pull request May 3, 2015
Closed
@HelloZeroNet HelloZeroNet mentioned this pull request May 13, 2015
Closed
@posita
Copy link
Contributor Author

posita commented May 17, 2015

@petertodd, can you tell me if this is worth pursuing? Please close if not.

Other than this, I do not have any suggestions for addressing #30.

@posita posita mentioned this pull request May 17, 2015
Closed
@petertodd
Copy link
Owner

petertodd commented May 19, 2015

Sorry, I'm off in the states travelling and haven't had a chance to look at this in detail. :( Give me another few days.

@posita
Copy link
Contributor Author

posita commented May 22, 2015

@petertodd, np. Sorry for nagging! 😊

I filed pyca/cryptography#1969 to request that d2i_ECPrivateKey and i2d_ECPrivateKey be added to cryptography. Apparently there's a workaround that I wasn't aware of, but it looks complicated. If this approach has legs, I'll investigate further. Otherwise, @HelloZeroNet has proposed an alternative, but it involves wrapping OpenSSL.

@petertodd
Copy link
Owner

petertodd commented May 23, 2015

Hmm, I'm pretty reluctant to move to https://github.com/pyca/cryptography, basically because it gets us further away from doing exactly what Bitcoin Core does by adding another layer/dependency. I'm inclined to wait for libsecp256k1 before doing anything wholesale. Equally, to fix #30 I'd rather see a more targeted patch.

What can I say, crypto code is dangerous and hard to review thoroughly, and I know people are using python-bitcoinlib in production, so I'm inclined to tread very carefully with regard to changing anything.

@posita
Copy link
Contributor Author

posita commented May 24, 2015

Totally understood. Closing as "won't merge". See also #77.

@posita posita closed this May 24, 2015
@posita posita deleted the use-pyca-cryptography-for-bignum-ops-30 branch May 24, 2015 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unit tests are segfaulting

2 participants

@posita @petertodd
Morty Proxy This is a proxified and sanitized view of the page, visit original site.